From ca3ba4fb635c29c0c6cca1f53457d6a5e37b4fb1 Mon Sep 17 00:00:00 2001 From: Kevin <36448536+millionsouls@users.noreply.github.com> Date: Sat, 14 Nov 2026 21:23:51 -0500 Subject: [PATCH] feat(oauth2): Apply default lifetime to service grants Service grants created withput expires_at on a client with no max_grant_lifetime were previously permanent. They now expire after the configured [oauth2]access_token_lifetime_minutes. Explicitly declared expiries function the same as before. Lifetime calculation moves into a shared deadline_after helper that returns a provider error instead of panicking on zero or out of range values. Docs are updated, added unit tests to cover default, configured, and zero/out of range cases. Signed-off-by: Kevin <36448536+millionsouls@users.noreply.github.com> --- crates/api-types/src/v4/service_grant.rs | 5 +- .../core-types/src/service_grant/resource.rs | 6 +- crates/core/src/service_grant/service.rs | 178 ++++++++++++++++-- doc/src/admin/features/service-grants.md | 5 +- doc/src/adr/0036-service-delegation.md | 6 +- 5 files changed, 172 insertions(+), 28 deletions(-) diff --git a/crates/api-types/src/v4/service_grant.rs b/crates/api-types/src/v4/service_grant.rs index 28c0811f4..777ca2b83 100644 --- a/crates/api-types/src/v4/service_grant.rs +++ b/crates/api-types/src/v4/service_grant.rs @@ -88,7 +88,7 @@ pub struct ServiceGrant { /// Whether the grant may be redeemed. pub enabled: bool, - /// Expiry, if any. + /// Expiry. #[serde(default, skip_serializing_if = "Option::is_none")] pub expires_at: Option>, @@ -122,7 +122,8 @@ pub struct ServiceGrantCreate { #[cfg_attr(feature = "validate", validate(length(max = 255)))] pub description: Option, - /// Expiry; defaults to the grantee's `max_grant_lifetime`, if any. + /// Expiry; defaults to the grantee's `max_grant_lifetime`, or the + /// configured `[oauth2] access_token_lifetime_minutes` when it has none. #[serde(default, skip_serializing_if = "Option::is_none")] pub expires_at: Option>, diff --git a/crates/core-types/src/service_grant/resource.rs b/crates/core-types/src/service_grant/resource.rs index 383946eb5..e4cf5fadd 100644 --- a/crates/core-types/src/service_grant/resource.rs +++ b/crates/core-types/src/service_grant/resource.rs @@ -128,7 +128,7 @@ pub struct ServiceGrant { /// Delegated role ids; never more than the grantor held at creation, /// and intersected with the grantor's live roles at every redemption. pub role_ids: Vec, - /// Expiry; `None` = until revoked. + /// Expiry. Always set for grants created with a default lifetime; pub expires_at: Option>, /// Free-form description. pub description: Option, @@ -160,8 +160,8 @@ impl ServiceGrant { pub struct ServiceGrantCreate { /// Free-form description. pub description: Option, - /// Expiry; defaults to the client's `max_grant_lifetime` when it has - /// one. + /// Expiry; defaults to the client's `max_grant_lifetime`, or the + /// configured `[oauth2] access_token_lifetime_minutes` when it has none. pub expires_at: Option>, /// `client_id` of the delegatable service. pub grantee_client_id: String, diff --git a/crates/core/src/service_grant/service.rs b/crates/core/src/service_grant/service.rs index 2633059cd..9aac93e48 100644 --- a/crates/core/src/service_grant/service.rs +++ b/crates/core/src/service_grant/service.rs @@ -17,7 +17,7 @@ use std::collections::BTreeSet; use std::sync::Arc; use async_trait::async_trait; -use chrono::{Duration, Utc}; +use chrono::{DateTime, Duration, Utc}; use openstack_keystone_config::Config; use openstack_keystone_core_types::assignment::RoleAssignmentListParametersBuilder; @@ -168,6 +168,23 @@ fn resolve_role_ids( Ok(roles.into_iter().collect()) } +fn deadline_after( + now: DateTime, + secs: u64, + what: &str, +) -> Result, ServiceGrantProviderError> { + (secs > 0) + .then_some(secs) + .and_then(|s| i64::try_from(s).ok()) + .and_then(Duration::try_seconds) + .and_then(|lifetime| now.checked_add_signed(lifetime)) + .ok_or_else(|| { + ServiceGrantProviderError::provider(format!( + "{what} must be positive and within the supported range" + )) + }) +} + /// Attempts at disabling a grant that is concurrently being updated. const DISABLE_ATTEMPTS: usize = 3; @@ -311,22 +328,35 @@ impl ServiceGrantApi for ServiceGrantService { let role_ids = resolve_role_ids(data.role_ids, &effective, &policy.max_role_ids)?; let now = Utc::now(); - let max_lifetime = policy + let (max_grants_per_project, access_token_lifetime_seconds) = { + let config = state.config_manager.config.read().await; + ( + config.delegation.max_grants_per_project, + u64::from(config.oauth2.access_token_lifetime_minutes) * 60, + ) + }; + let max_expires_at = policy .max_grant_lifetime - .map(|secs| Duration::seconds(i64::try_from(secs).unwrap_or(i64::MAX))); - let expires_at = match (data.expires_at, max_lifetime) { + .map(|secs| deadline_after(now, secs, "max_grant_lifetime")) + .transpose()?; + let expires_at = match (data.expires_at, max_expires_at) { (Some(exp), _) if exp <= now => { return Err(ServiceGrantProviderError::Validation( "expires_at must be in the future".to_string(), )); } - (Some(exp), Some(max)) if exp > now + max => { + (Some(exp), Some(max)) if exp > max => { return Err(ServiceGrantProviderError::Validation( "expires_at is beyond the service's max_grant_lifetime".to_string(), )); } - (None, Some(max)) => Some(now + max), - (exp, _) => exp, + (None, Some(max)) => Some(max), + (Some(exp), _) => Some(exp), + (None, None) => Some(deadline_after( + now, + access_token_lifetime_seconds, + "oauth2.access_token_lifetime_minutes", + )?), }; // Count caps (ADR 0036 §4.2). Best effort: two concurrent creations @@ -343,20 +373,13 @@ impl ServiceGrantApi for ServiceGrantService { ))); } } - let backstop = state - .config_manager - .config - .read() - .await - .delegation - .max_grants_per_project; let count = self .backend_driver .count_service_grants(state, &project.id, None) .await?; - if count as u64 >= backstop { + if count as u64 >= max_grants_per_project { return Err(ServiceGrantProviderError::QuotaExceeded(format!( - "the project reached the limit of {backstop} service grants" + "the project reached the limit of {max_grants_per_project} service grants" ))); } @@ -653,12 +676,13 @@ mod tests { async fn test_create_defaults_to_effective_roles_within_ceiling() { let policy = DelegationPolicy { max_role_ids: vec!["member".into(), "reader".into()], - max_grant_lifetime: Some(3600), + max_grant_lifetime: Some(120), ..grants_policy() }; let state = state(Some(client("d1", policy)), &["admin", "member", "reader"]).await; let auth = vsc(AuthenticationContext::Password); let ctx = ExecutionContext::from_auth(&state, &auth); + let started = Utc::now(); let grant = service(0) .create_service_grant(&ctx, create(None)) .await @@ -669,8 +693,87 @@ mod tests { assert_eq!(grant.grantee.client_id(), "c1"); assert_eq!(grant.target.project_id(), "p1"); assert!(grant.enabled); - // The lifetime ceiling becomes the default expiry. - assert!(grant.expires_at.is_some()); + // The client's lifetime ceiling takes precedence over the OAuth2 default. + let expires_at = grant.expires_at.unwrap(); + assert!(expires_at >= started + Duration::seconds(119)); + assert!(expires_at <= Utc::now() + Duration::seconds(120)); + } + + #[tokio::test] + async fn test_create_uses_oauth2_default_when_client_has_no_lifetime() { + let state = state(Some(client("d1", grants_policy())), &["member"]).await; + let auth = vsc(AuthenticationContext::Password); + let ctx = ExecutionContext::from_auth(&state, &auth); + let started = Utc::now(); + let expected = u64::from( + openstack_keystone_config::Oauth2Provider::default().access_token_lifetime_minutes, + ) * 60; + let grant = service(0) + .create_service_grant(&ctx, create(None)) + .await + .unwrap(); + let expires_at = grant.expires_at.unwrap(); + assert!(expires_at >= started + Duration::seconds(expected as i64 - 1)); + assert!(expires_at <= Utc::now() + Duration::seconds(expected as i64)); + } + + #[tokio::test] + async fn test_create_uses_configured_oauth2_access_token_lifetime() { + let state = state(Some(client("d1", grants_policy())), &["member"]).await; + state + .config_manager + .config + .write() + .await + .oauth2 + .access_token_lifetime_minutes = 2; + let auth = vsc(AuthenticationContext::Password); + let ctx = ExecutionContext::from_auth(&state, &auth); + let started = Utc::now(); + let grant = service(0) + .create_service_grant(&ctx, create(None)) + .await + .unwrap(); + let expires_at = grant.expires_at.unwrap(); + assert!(expires_at >= started + Duration::seconds(119)); + assert!(expires_at <= Utc::now() + Duration::seconds(120)); + } + + #[tokio::test] + async fn test_create_preserves_explicit_expiry_without_client_lifetime() { + let state = state(Some(client("d1", grants_policy())), &["member"]).await; + let auth = vsc(AuthenticationContext::Password); + let ctx = ExecutionContext::from_auth(&state, &auth); + let requested_expiry = Utc::now() + Duration::hours(2); + let grant = service(0) + .create_service_grant( + &ctx, + ServiceGrantCreate { + expires_at: Some(requested_expiry), + ..create(None) + }, + ) + .await + .unwrap(); + assert_eq!(grant.expires_at, Some(requested_expiry)); + } + + /// A lifetime outside chrono's range is returned as an error instead of + /// panicking while calculating its deadline. + #[tokio::test] + async fn test_create_out_of_range_client_lifetime_does_not_panic() { + let auth = vsc(AuthenticationContext::Password); + + let policy = DelegationPolicy { + max_grant_lifetime: Some(u64::MAX), + ..grants_policy() + }; + let client_state = state(Some(client("d1", policy)), &["member"]).await; + let ctx = ExecutionContext::from_auth(&client_state, &auth); + assert!(matches!( + service(0).create_service_grant(&ctx, create(None)).await, + Err(ServiceGrantProviderError::Provider(_)) + )); } /// A grant on a project of another domain lives in the project's @@ -914,4 +1017,41 @@ mod tests { assert!(resolve_role_ids(None, &effective, &["z".into()]).is_err()); assert!(resolve_role_ids(Some(vec!["z".into()]), &effective, &[]).is_err()); } + + #[tokio::test] + async fn test_create_zero_client_lifetime_is_rejected() { + let policy = DelegationPolicy { + max_grant_lifetime: Some(0), + ..grants_policy() + }; + let state = state(Some(client("d1", policy)), &["member"]).await; + let auth = vsc(AuthenticationContext::Password); + let ctx = ExecutionContext::from_auth(&state, &auth); + assert!( + service(0) + .create_service_grant(&ctx, create(None)) + .await + .is_err() + ); + } + + #[tokio::test] + async fn test_create_zero_access_token_lifetime_is_rejected() { + let state = state(Some(client("d1", grants_policy())), &["member"]).await; + state + .config_manager + .config + .write() + .await + .oauth2 + .access_token_lifetime_minutes = 0; + let auth = vsc(AuthenticationContext::Password); + let ctx = ExecutionContext::from_auth(&state, &auth); + assert!( + service(0) + .create_service_grant(&ctx, create(None)) + .await + .is_err() + ); + } } diff --git a/doc/src/admin/features/service-grants.md b/doc/src/admin/features/service-grants.md index e8014f713..92c772905 100644 --- a/doc/src/admin/features/service-grants.md +++ b/doc/src/admin/features/service-grants.md @@ -82,8 +82,9 @@ requires an administrator. id added to the ceiling must be an existing role; ids already stored are not re-checked, so a role deleted later simply never matches and does not block edits. -- `max_grant_lifetime` (seconds) caps `expires_at`. Without it a grant may be - created without an expiry. +- `max_grant_lifetime` (seconds) caps `expires_at`. Without it, an omitted + `expires_at` uses `[oauth2] access_token_lifetime_minutes` converted to + seconds; explicitly supplied expiries are not capped by this client setting. - `max_grants_per_project` caps live grants per (project, client). The `[delegation] max_grants_per_project` backstop applies on top. Exceeding either answers `429`. diff --git a/doc/src/adr/0036-service-delegation.md b/doc/src/adr/0036-service-delegation.md index ace17fd58..b179d67df 100644 --- a/doc/src/adr/0036-service-delegation.md +++ b/doc/src/adr/0036-service-delegation.md @@ -365,7 +365,9 @@ Rules applied at creation: for the caller's own scope — further capped by `DelegationPolicy.max_role_ids`. Omitting `roles` freezes the caller's current effective roles, as application credentials do. -- `expires_at` is capped by `max_grant_lifetime`. +- `expires_at` is capped by `max_grant_lifetime`. If both `expires_at` and + `max_grant_lifetime` are absent, the configured + `[oauth2] access_token_lifetime_minutes` supplies the expiry in seconds. - The grantee client must exist, be enabled, and have `DelegationPolicy.service_grants == true`. For a grantee in another domain, `cross_domain` must be true. A grant naming anything else is rejected at @@ -1394,4 +1396,4 @@ will stop carrying scope. `keystone-rs`. - **New state and a new driver crate**, plus one more `AuthenticationContext` pair to keep in every match arm. The ADR 0017 checklist and compile-time - exhaustive matches make omissions a build error rather than a runtime hole. \ No newline at end of file + exhaustive matches make omissions a build error rather than a runtime hole.