Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions nexus/auth/src/authn/external/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,7 @@ mod test {
SKIP => SchemeResult::NotRequested,
OK => SchemeResult::Authenticated(authn::Details {
actor: self.actor,
device_token_expiration: None,
}),
FAIL => SchemeResult::Failed(Reason::BadCredentials {
actor: self.actor,
Expand Down
5 changes: 4 additions & 1 deletion nexus/auth/src/authn/external/scim.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,10 @@ where
Ok(None) => SchemeResult::NotRequested,
Ok(Some(token)) => match ctx.scim_token_actor(token).await {
Err(error) => SchemeResult::Failed(error),
Ok(actor) => SchemeResult::Authenticated(Details { actor }),
Ok(actor) => SchemeResult::Authenticated(Details {
actor,
device_token_expiration: None,
}),
},
}
}
Expand Down
10 changes: 8 additions & 2 deletions nexus/auth/src/authn/external/session_cookie.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,10 @@ where
debug!(log, "failed to extend session")
}

SchemeResult::Authenticated(Details { actor })
SchemeResult::Authenticated(Details {
actor,
device_token_expiration: None,
})
}
}

Expand Down Expand Up @@ -395,7 +398,10 @@ mod test {
let result = authn_with_cookie(&context, Some("session=abc")).await;
assert!(matches!(
result,
SchemeResult::Authenticated(Details { actor: _ })
SchemeResult::Authenticated(Details {
actor: _,
device_token_expiration: _
})
Comment on lines +401 to +404
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
SchemeResult::Authenticated(Details {
actor: _,
device_token_expiration: _
})
SchemeResult::Authenticated(Details { .. })

));

// valid cookie should have updated time_last_used
Expand Down
5 changes: 4 additions & 1 deletion nexus/auth/src/authn/external/spoof.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,10 @@ where
Err(error) => SchemeResult::Failed(error),
Ok(silo_id) => {
let actor = Actor::SiloUser { silo_id, silo_user_id };
SchemeResult::Authenticated(Details { actor })
SchemeResult::Authenticated(Details {
actor,
device_token_expiration: None,
})
}
}
}
Expand Down
17 changes: 14 additions & 3 deletions nexus/auth/src/authn/external/token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use super::SchemeResult;
use super::SiloUserSilo;
use crate::authn;
use async_trait::async_trait;
use chrono::{DateTime, Utc};
use headers::HeaderMapExt;
use headers::authorization::{Authorization, Bearer};

Expand Down Expand Up @@ -63,9 +64,14 @@ where
match parse_token(headers.typed_get().as_ref()) {
Err(error) => SchemeResult::Failed(error),
Ok(None) => SchemeResult::NotRequested,
Ok(Some(token)) => match ctx.token_actor(token).await {
Ok(Some(token)) => match ctx.authenticate_token(token).await {
Err(error) => SchemeResult::Failed(error),
Ok(actor) => SchemeResult::Authenticated(Details { actor }),
Ok((actor, device_token_expiration)) => {
SchemeResult::Authenticated(Details {
actor,
device_token_expiration,
})
}
},
}
}
Expand All @@ -91,7 +97,12 @@ fn parse_token(
/// A context that can look up a Silo user and client ID from a token.
#[async_trait]
pub trait TokenContext {
async fn token_actor(&self, token: String) -> Result<authn::Actor, Reason>;
/// Returns the actor authenticated by the token and the token's expiration
/// time (if any).
async fn authenticate_token(
&self,
token: String,
) -> Result<(authn::Actor, Option<DateTime<Utc>>), Reason>;
}

#[cfg(test)]
Expand Down
39 changes: 35 additions & 4 deletions nexus/auth/src/authn/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(),
Expand All @@ -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
///
Expand Down Expand Up @@ -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(),
Expand All @@ -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()),
),
Expand Down Expand Up @@ -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(),
Expand All @@ -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())),
Expand Down Expand Up @@ -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>>,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the sad part. I considered doing a proper scheme: AuthnScheme struct here like AuthnScheme::DeviceToken { time_expires: Option<DateTime<Utc>> } but I think that makes the abstraction-leaking here even worse. We don't want app-layer code to be doing conditional logic based on the authn scheme. Authz policy is generally meant to cover that. The device token confirmation logic in this PR is a special case.

}

/// Who is performing an operation
Expand Down
8 changes: 8 additions & 0 deletions nexus/external-api/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4130,6 +4130,14 @@ pub trait NexusExternalApi {
/// This endpoint is designed to be accessed by the user agent (browser),
/// not the client requesting the token. So we do not actually return the
/// token here; it will be returned in response to the poll on `/device/token`.
///
/// Some special logic applies when authenticating this request with an
/// existing device token instead of a console session: the requested
/// TTL must not produce an expiration time later than the authenticating
/// token's expiration. If no TTL was specified in the initial grant
/// request, the expiration will be the lesser of the silo max and the
/// authenticating token's expiration time. To get the longest allowed
/// lifetime, omit the TTL and authenticate with a web console session.
#[endpoint {
method = POST,
path = "/device/confirm",
Expand Down
82 changes: 66 additions & 16 deletions nexus/src/app/device_auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,20 +123,67 @@ 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.
Copy link
Contributor Author

@david-crespo david-crespo Nov 18, 2025

Choose a reason for hiding this comment

The 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",
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.",
));
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 TTL exceeds expiration time of current token, just clamp.
let silo_max_exp = silo_max_ttl
.map(|ttl| Utc::now() + Duration::seconds(ttl.0.into()));
// a.min(b) doesn't do it because None is always less than Some(_)
match (silo_max_exp, opctx.authn.device_token_expiration()) {
(Some(silo_exp), Some(token_exp)) => {
Some(silo_exp.min(token_exp))
}
(a, b) => a.or(b),
}
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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,
Expand Down Expand Up @@ -193,11 +240,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)
Expand All @@ -222,7 +270,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 {
Expand All @@ -236,7 +286,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(
Expand Down
9 changes: 6 additions & 3 deletions nexus/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -458,12 +458,15 @@ impl authn::external::SiloUserSilo for ServerContext {

#[async_trait]
impl authn::external::token::TokenContext for ServerContext {
async fn token_actor(
async fn authenticate_token(
&self,
token: String,
) -> Result<authn::Actor, authn::Reason> {
) -> Result<
(authn::Actor, Option<chrono::DateTime<chrono::Utc>>),
authn::Reason,
> {
let opctx = self.nexus.opctx_external_authn();
self.nexus.device_access_token_actor(opctx, token).await
self.nexus.authenticate_token(opctx, token).await
}
}

Expand Down
Loading
Loading