-
Notifications
You must be signed in to change notification settings - Fork 63
Clamp TTL on device token confirmed with existing device token #9411
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
52ac410
2de0e44
8ea093a
9d69edd
1f83ffc
843de4e
5128550
8799e7d
3f925e9
f7aa780
da126ed
461549d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,6 +38,7 @@ pub use nexus_db_fixed_data::user_builtin::USER_SAGA_RECOVERY; | |
| pub use nexus_db_fixed_data::user_builtin::USER_SERVICE_BALANCER; | ||
|
|
||
| use crate::authz; | ||
| use chrono::{DateTime, Utc}; | ||
| use newtype_derive::NewtypeDisplay; | ||
| use nexus_db_fixed_data::silo::DEFAULT_SILO; | ||
| use nexus_types::external_api::shared::FleetRole; | ||
|
|
@@ -84,7 +85,7 @@ impl Context { | |
| &self, | ||
| ) -> Result<&Actor, omicron_common::api::external::Error> { | ||
| match &self.kind { | ||
| Kind::Authenticated(Details { actor }, ..) => Ok(actor), | ||
| Kind::Authenticated(Details { actor, .. }, ..) => Ok(actor), | ||
| Kind::Unauthenticated => { | ||
| Err(omicron_common::api::external::Error::Unauthenticated { | ||
| internal_message: "Actor required".to_string(), | ||
|
|
@@ -93,6 +94,21 @@ impl Context { | |
| } | ||
| } | ||
|
|
||
| /// Returns the expiration time if authenticated via a device token. | ||
| /// | ||
| /// This is used to prevent token lifetime extension during token creation: | ||
| /// a new token created using an existing token should not outlive the | ||
| /// token used to authenticate. | ||
| pub fn device_token_expiration(&self) -> Option<DateTime<Utc>> { | ||
| match &self.kind { | ||
| Kind::Authenticated( | ||
| Details { device_token_expiration, .. }, | ||
| .., | ||
| ) => *device_token_expiration, | ||
| Kind::Unauthenticated => None, | ||
| } | ||
| } | ||
|
|
||
| /// Returns the current actor's Silo if they have one or an appropriate | ||
| /// error otherwise | ||
| /// | ||
|
|
@@ -216,7 +232,10 @@ impl Context { | |
| fn context_for_builtin_user(user_builtin_id: BuiltInUserUuid) -> Context { | ||
| Context { | ||
| kind: Kind::Authenticated( | ||
| Details { actor: Actor::UserBuiltin { user_builtin_id } }, | ||
| Details { | ||
| actor: Actor::UserBuiltin { user_builtin_id }, | ||
| device_token_expiration: None, | ||
| }, | ||
| None, | ||
| ), | ||
| schemes_tried: Vec::new(), | ||
|
|
@@ -234,6 +253,7 @@ impl Context { | |
| silo_user_id: USER_TEST_PRIVILEGED.id(), | ||
| silo_id: USER_TEST_PRIVILEGED.silo_id, | ||
| }, | ||
| device_token_expiration: None, | ||
| }, | ||
| Some(SiloAuthnPolicy::try_from(&*DEFAULT_SILO).unwrap()), | ||
| ), | ||
|
|
@@ -261,7 +281,10 @@ impl Context { | |
| ) -> Context { | ||
| Context { | ||
| kind: Kind::Authenticated( | ||
| Details { actor: Actor::SiloUser { silo_user_id, silo_id } }, | ||
| Details { | ||
| actor: Actor::SiloUser { silo_user_id, silo_id }, | ||
| device_token_expiration: None, | ||
| }, | ||
| Some(silo_authn_policy), | ||
| ), | ||
| schemes_tried: Vec::new(), | ||
|
|
@@ -273,7 +296,10 @@ impl Context { | |
| pub fn for_scim(silo_id: Uuid) -> Context { | ||
| Context { | ||
| kind: Kind::Authenticated( | ||
| Details { actor: Actor::Scim { silo_id } }, | ||
| Details { | ||
| actor: Actor::Scim { silo_id }, | ||
| device_token_expiration: None, | ||
| }, | ||
| // This should never be non-empty, we don't want the SCIM user | ||
| // to ever have associated roles. | ||
| Some(SiloAuthnPolicy::new(BTreeMap::default())), | ||
|
|
@@ -391,6 +417,11 @@ enum Kind { | |
| pub struct Details { | ||
| /// the actor performing the request | ||
| actor: Actor, | ||
| /// When the device token expires. Present only when authenticating via | ||
| /// a device token. This is a slightly awkward fit but is included here | ||
| /// because we need to use this to clamp the expiration time when device | ||
| /// tokens are confirmed using an existing device token. | ||
| device_token_expiration: Option<DateTime<Utc>>, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the sad part. I considered doing a proper |
||
| } | ||
|
|
||
| /// Who is performing an operation | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -123,20 +123,68 @@ impl super::Nexus { | |
| let silo_max_ttl = silo_auth_settings.device_token_max_ttl_seconds; | ||
| let requested_ttl = db_request.token_ttl_seconds; | ||
|
|
||
| // Validate the requested TTL against the silo's max TTL | ||
| if let (Some(requested), Some(max)) = (requested_ttl, silo_max_ttl) { | ||
| if requested > max.0.into() { | ||
| return Err(Error::invalid_request(&format!( | ||
| "Requested TTL {} seconds exceeds maximum \ | ||
| allowed TTL for this silo of {} seconds", | ||
| requested, max | ||
| ))); | ||
| // This logic is a bit gnarly, but we landed on it as the least bad | ||
| // option. Error out if the user requests a token TTL that is longer | ||
| // than allowed, i.e., either | ||
| // | ||
| // a) it is longer than the silo max TTL, or | ||
| // b) this request was authenticated with a device token and the TTL | ||
| // would produce an expiration later than the current token's. | ||
| // | ||
| // If the user does not request a specific TTL, we do not error out. | ||
| // We calculate the token TTL as min(silo max TTL, current token TTL | ||
| // if present). Token confirm requests authenticated with a console | ||
| // session can get device tokens with TTLs up to the silo max. | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We discussed this clamping logic in chat and decided that while it was more user-friendly to error out when TTL is specified, it is too easy to get the error when not specifying a TTL, so we clamp instead. Users confirming a token through a console session will never hit any of this logic anyway. |
||
|
|
||
| let time_expires = if let Some(requested_ttl) = requested_ttl { | ||
| // If the user requested a TTL, validate it against the silo max | ||
| // TTL as well as the expiration time of the token being used (if a | ||
| // token is being used) | ||
|
|
||
| // Validate the requested TTL against the silo's max TTL | ||
| if let Some(max) = silo_max_ttl { | ||
| if requested_ttl > max.0.into() { | ||
| return Err(Error::invalid_request(&format!( | ||
| "Requested TTL {} seconds exceeds maximum allowed \ | ||
| TTL for this silo of {} seconds", | ||
david-crespo marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| requested_ttl, max | ||
| ))); | ||
| } | ||
| }; | ||
|
|
||
| let requested_exp = | ||
| Utc::now() + Duration::seconds(requested_ttl.0.into()); | ||
|
|
||
| // If currently authenticated via token, error if requested exceeds it | ||
| if let Some(auth_exp) = opctx.authn.device_token_expiration() { | ||
| if requested_exp > auth_exp { | ||
| return Err(Error::invalid_request( | ||
| "Requested token TTL would exceed the expiration time \ | ||
| of the token being used to authenticate the confirm \ | ||
| request. To get the full requested TTL, confirm \ | ||
| this token using a web console session. Alternatively, \ | ||
| omit requested TTL to get a token with the longest \ | ||
| allowed lifetime, determined by the lesser of the silo \ | ||
| max and the current token's expiration time.", | ||
| )); | ||
| } | ||
| } | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. new validation logic for explicit TTL request |
||
| } | ||
|
|
||
| let time_expires = requested_ttl | ||
| .or(silo_max_ttl) | ||
| .map(|ttl| Utc::now() + Duration::seconds(ttl.0.into())); | ||
| Some(requested_exp) | ||
| } else { | ||
| // No explicit TTL requested. Rather than erroring out if silo max | ||
| // exceeds expiration time of current token, just clamp. | ||
| let silo_max_exp = silo_max_ttl | ||
| .map(|ttl| Utc::now() + Duration::seconds(ttl.0.into())); | ||
| match (silo_max_exp, opctx.authn.device_token_expiration()) { | ||
| (Some(silo_exp), Some(token_exp)) => { | ||
| Some(silo_exp.min(token_exp)) | ||
| } | ||
| (Some(silo_exp), None) => Some(silo_exp), | ||
| (None, Some(token_exp)) => Some(token_exp), | ||
| (None, None) => None, | ||
| } | ||
david-crespo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. new clamping logic for no explicit TTL |
||
|
|
||
| let token = DeviceAccessToken::new( | ||
| db_request.client_id, | ||
|
|
@@ -193,11 +241,12 @@ impl super::Nexus { | |
|
|
||
| /// Look up the actor for which a token was granted. | ||
| /// Corresponds to a request *after* completing the flow above. | ||
| pub(crate) async fn device_access_token_actor( | ||
| /// Returns the actor and the token's expiration time (if any). | ||
| pub(crate) async fn authenticate_token( | ||
| &self, | ||
| opctx: &OpContext, | ||
| token: String, | ||
| ) -> Result<Actor, Reason> { | ||
| ) -> Result<(Actor, Option<chrono::DateTime<Utc>>), Reason> { | ||
| let (.., db_access_token) = self | ||
| .db_datastore | ||
| .device_token_lookup_by_token(opctx, token) | ||
|
|
@@ -222,7 +271,9 @@ impl super::Nexus { | |
| })?; | ||
| let silo_id = db_silo_user.silo_id; | ||
|
|
||
| if let Some(time_expires) = db_access_token.time_expires { | ||
| let expiration = db_access_token.time_expires; | ||
|
|
||
| if let Some(time_expires) = expiration { | ||
| let now = Utc::now(); | ||
| if time_expires < now { | ||
| return Err(Reason::BadCredentials { | ||
|
|
@@ -236,7 +287,7 @@ impl super::Nexus { | |
| } | ||
| } | ||
|
|
||
| Ok(Actor::SiloUser { silo_user_id, silo_id }) | ||
| Ok((Actor::SiloUser { silo_user_id, silo_id }, expiration)) | ||
| } | ||
|
|
||
| pub(crate) async fn device_access_token( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: