From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 736181FF0AB for ; Wed, 23 Sep 2026 23:01:24 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 837D52176B; Wed, 23 Sep 2026 23:00:16 +0200 (CEST) From: Thomas Lamprecht To: pve-devel@lists.proxmox.com Subject: [PATCH proxmox 8/9] access-control: add API token policy type with expiry checks Date: Wed, 23 Sep 2026 22:59:57 +0200 Message-ID: <20260923210000.4031318-9-t.lamprecht@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260923210000.4031318-1-t.lamprecht@proxmox.com> References: <20260923210000.4031318-1-t.lamprecht@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1790197207694 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.834 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLACK 3 Contains an URL listed in the URIBL blacklist [types.rs] Message-ID-Hash: POYBIJCIPO3YYUU6MQG54J3APZOJKUBY X-Message-ID-Hash: POYBIJCIPO3YYUU6MQG54J3APZOJKUBY X-MailFrom: t.lamprecht@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Counterpart of the datacenter-wide token-policy option introduced for PVE with #7805, so that products building on this crate can centrally require expiration dates for API tokens, limit the maximum token lifetime, and forbid changing the expiration date of existing tokens. Compliance rules like PCI DSS, SOC 2, or ISO 27001 often require such limits. Products opt in by overriding the new AccessControlConfig method to return the policy from their configuration, by default none is enforced. The check is meant to be called only with an expiration date an operation actually sets or changes, existing tokens stay valid on purpose, so opting into a policy does not invalidate already deployed tokens. Only the expiry-related settings are ported, tokens in this stack always use separate ACLs, so PVE's privilege separation setting only becomes relevant once it moves over to this implementation. Signed-off-by: Thomas Lamprecht --- proxmox-access-control/src/init.rs | 15 ++ proxmox-access-control/src/types.rs | 227 ++++++++++++++++++++++++++++ 2 files changed, 242 insertions(+) diff --git a/proxmox-access-control/src/init.rs b/proxmox-access-control/src/init.rs index 52c3393a..b8bcdafd 100644 --- a/proxmox-access-control/src/init.rs +++ b/proxmox-access-control/src/init.rs @@ -6,6 +6,8 @@ use anyhow::{Error, format_err}; use proxmox_auth_api::types::{Authid, Userid}; use proxmox_section_config::SectionConfigData; +use crate::types::TokenPolicy; + static ACCESS_CONF: OnceLock<&'static dyn AccessControlConfig> = OnceLock::new(); /// This trait specifies the functions a product needs to implement to get ACL tree based access @@ -101,6 +103,19 @@ pub trait AccessControlConfig: Send + Sync { fn allow_partial_permission_match(&self) -> bool { true } + + /// Returns the token policy to enforce when API tokens are created or updated, if any. + /// + /// Existing tokens are deliberately never re-validated against a policy, so enabling one + /// does not invalidate already deployed tokens. Override this to return the policy from the + /// product's configuration. Return an error if that configuration cannot be loaded, so token + /// creation and expiration-date updates fail instead of silently proceeding without the + /// policy. + /// + /// Default: Returns `Ok(None)`, no policy is enforced. + fn token_policy(&self) -> Result, Error> { + Ok(None) + } } pub fn init_access_config(config: &'static dyn AccessControlConfig) -> Result<(), Error> { diff --git a/proxmox-access-control/src/types.rs b/proxmox-access-control/src/types.rs index 875b3d93..7fef5adc 100644 --- a/proxmox-access-control/src/types.rs +++ b/proxmox-access-control/src/types.rs @@ -1,3 +1,4 @@ +use anyhow::{Error, bail}; use serde::{Deserialize, Serialize}; use const_format::concatcp; @@ -174,6 +175,82 @@ pub struct TokenApiEntry { pub token: ApiToken, } +#[api( + properties: { + "require-expiry": { + optional: true, + default: false, + }, + "max-lifetime": { + optional: true, + minimum: 1, + }, + "disallow-expiry-changes": { + optional: true, + default: false, + }, + }, +)] +#[derive(Clone, Copy, Debug, Default, Deserialize, PartialEq, Serialize, Updater)] +#[serde(rename_all = "kebab-case")] +/// Policy for the creation and update of API tokens. +/// +/// The policy is enforced when API tokens are created or updated only, existing tokens are +/// deliberately not affected, so enabling a policy does not invalidate already deployed tokens. +pub struct TokenPolicy { + /// Require an expiration date for new API tokens and when changing the expiration date of + /// existing ones. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub require_expiry: Option, + + /// Maximum lifetime of API tokens in seconds, counted from when the expiration date is set, + /// that is on creation or when an update changes it. Implies `require-expiry`. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub max_lifetime: Option, + + /// Disallow changing the expiration date of existing API tokens, so that `max-lifetime` + /// cannot be circumvented by extending tokens repeatedly. Such tokens can still be deleted + /// and recreated. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub disallow_expiry_changes: Option, +} + +impl TokenPolicy { + /// Check a new or changed token expiration date against the policy. + /// + /// Must only be called when an operation actually sets or changes the expiration date, with + /// an `expire` of 0 meaning no expiration date. This lets callers enforce the policy only for + /// the expiration date an operation sets, so unrelated updates do not retroactively reject + /// tokens that predate the policy. `is_update` distinguishes updating an existing token from + /// creating a new one, `now` is the reference time the maximum lifetime is bound against. + pub fn check_expiry_change(&self, expire: i64, is_update: bool, now: i64) -> Result<(), Error> { + if is_update && self.disallow_expiry_changes.unwrap_or(false) { + bail!( + "the token policy does not allow changing the expiration date of an existing token" + ); + } + + // a token that never expires trivially exceeds any finite maximum lifetime, so a set + // maximum lifetime also requires an expiration date + if expire == 0 && (self.require_expiry.unwrap_or(false) || self.max_lifetime.is_some()) { + bail!("the token policy requires an expiration date"); + } + + if let Some(max_lifetime) = self.max_lifetime { + // an extreme configured lifetime must not overflow, which panics in debug builds and + // wraps into a bound rejecting everything otherwise + if expire > 0 && expire > now.saturating_add(max_lifetime) { + bail!( + "the expiration date exceeds the maximum token lifetime of {max_lifetime} \ + seconds set by the token policy" + ); + } + } + + Ok(()) + } +} + #[api( properties: { userid: { @@ -316,3 +393,153 @@ pub const REGENERATE_TOKEN_SCHEMA: Schema = BooleanSchema::new("Regenerate token secret while keeping permissions.") .default(false) .schema(); + +#[cfg(test)] +mod tests { + use super::TokenPolicy; + + // fixed reference time and a 90 day maximum lifetime for deterministic bounds checks + const NOW: i64 = 1_000_000_000; + const MAX: i64 = 90 * 24 * 60 * 60; + + const EMPTY: TokenPolicy = TokenPolicy { + require_expiry: None, + max_lifetime: None, + disallow_expiry_changes: None, + }; + + const REQUIRE_EXPIRY: TokenPolicy = TokenPolicy { + require_expiry: Some(true), + ..EMPTY + }; + + const MAX_LIFETIME: TokenPolicy = TokenPolicy { + max_lifetime: Some(MAX), + ..EMPTY + }; + + const FIXED_EXPIRY: TokenPolicy = TokenPolicy { + disallow_expiry_changes: Some(true), + ..EMPTY + }; + + #[test] + fn empty_policy_allows_everything() { + assert!(EMPTY.check_expiry_change(0, false, NOW).is_ok()); + assert!(EMPTY.check_expiry_change(NOW + 100, false, NOW).is_ok()); + assert!(EMPTY.check_expiry_change(0, true, NOW).is_ok()); + } + + #[test] + fn require_expiry() { + assert!(REQUIRE_EXPIRY.check_expiry_change(0, false, NOW).is_err()); + assert!( + REQUIRE_EXPIRY + .check_expiry_change(NOW + 100, false, NOW) + .is_ok() + ); + // an update removing the expiration date is a change and gets rejected as well + assert!(REQUIRE_EXPIRY.check_expiry_change(0, true, NOW).is_err()); + // while a compliant expiration date change on update stays allowed + assert!( + REQUIRE_EXPIRY + .check_expiry_change(NOW + 100, true, NOW) + .is_ok() + ); + } + + #[test] + fn max_lifetime_implies_require_expiry() { + assert!(MAX_LIFETIME.check_expiry_change(0, false, NOW).is_err()); + let explicitly_not_required = TokenPolicy { + require_expiry: Some(false), + ..MAX_LIFETIME + }; + assert!( + explicitly_not_required + .check_expiry_change(0, false, NOW) + .is_err() + ); + } + + #[test] + fn max_lifetime_bounds() { + assert!( + MAX_LIFETIME + .check_expiry_change(NOW + 100, false, NOW) + .is_ok() + ); + assert!( + MAX_LIFETIME + .check_expiry_change(NOW + MAX, false, NOW) + .is_ok() + ); + assert!( + MAX_LIFETIME + .check_expiry_change(NOW + MAX + 1, false, NOW) + .is_err() + ); + // prolonging past the bound on update is rejected just the same + assert!( + MAX_LIFETIME + .check_expiry_change(NOW + MAX + 1, true, NOW) + .is_err() + ); + // an expiration date in the past is odd, but within any bound + assert!( + MAX_LIFETIME + .check_expiry_change(NOW - 100, false, NOW) + .is_ok() + ); + // prolonging within the bound on update stays allowed + assert!( + MAX_LIFETIME + .check_expiry_change(NOW + 100, true, NOW) + .is_ok() + ); + } + + #[test] + fn extreme_max_lifetime_does_not_overflow() { + let policy = TokenPolicy { + max_lifetime: Some(i64::MAX), + ..EMPTY + }; + assert!(policy.check_expiry_change(NOW + 100, false, NOW).is_ok()); + assert!(policy.check_expiry_change(i64::MAX, false, NOW).is_ok()); + } + + #[test] + fn disallow_expiry_changes() { + // any change on update is rejected: prolonging, shortening and removal + assert!( + FIXED_EXPIRY + .check_expiry_change(NOW + 100, true, NOW) + .is_err() + ); + assert!( + FIXED_EXPIRY + .check_expiry_change(NOW - 100, true, NOW) + .is_err() + ); + assert!(FIXED_EXPIRY.check_expiry_change(0, true, NOW).is_err()); + // creation is unaffected, with or without an expiration date + assert!( + FIXED_EXPIRY + .check_expiry_change(NOW + 100, false, NOW) + .is_ok() + ); + assert!(FIXED_EXPIRY.check_expiry_change(0, false, NOW).is_ok()); + } + + #[test] + fn disallow_expiry_changes_wins_over_compliant_expiry() { + let policy = TokenPolicy { + require_expiry: Some(true), + disallow_expiry_changes: Some(true), + ..EMPTY + }; + assert!(policy.check_expiry_change(NOW + 100, true, NOW).is_err()); + assert!(policy.check_expiry_change(NOW + 100, false, NOW).is_ok()); + } +} -- 2.47.3