From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 42BA91FF0B4 for ; Tue, 10 Nov 2026 16:05:55 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id A08FB21669; Tue, 10 Nov 2026 16:05:43 +0100 (CET) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Tue, 10 Nov 2026 16:05:16 +0100 Message-Id: Subject: Re: [PATCH proxmox 8/9] access-control: add API token policy type with expiry checks To: "Thomas Lamprecht" , X-Mailer: aerc 0.20.0 References: <20260923210000.4031318-1-t.lamprecht@proxmox.com> <20260923210000.4031318-9-t.lamprecht@proxmox.com> In-Reply-To: <20260923210000.4031318-9-t.lamprecht@proxmox.com> From: "Shannon Sterz" X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1794323116838 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.629 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 Message-ID-Hash: CFKOWBNII4ZEJ2FR2AGWA4OP7KRNVN5P X-Message-ID-Hash: CFKOWBNII4ZEJ2FR2AGWA4OP7KRNVN5P X-MailFrom: s.sterz@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: On Wed Sep 23, 2026 at 10:59 PM CEST, Thomas Lamprecht wrote: > 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> =3D OnceL= ock::new(); > > /// This trait specifies the functions a product needs to implement to g= et 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 po= licy, so enabling one > + /// does not invalidate already deployed tokens. Override this to re= turn the policy from the > + /// product's configuration. Return an error if that configuration c= annot be loaded, so token > + /// creation and expiration-date updates fail instead of silently pr= oceeding 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) -> R= esult<(), 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 =3D "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 =3D "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 `requi= re-expiry`. > + #[serde(default, skip_serializing_if =3D "Option::is_none")] > + pub max_lifetime: Option, nit: the above already sets `minimum: 1`, this could probably be `u64`. > + > + /// Disallow changing the expiration date of existing API tokens, so= that `max-lifetime` > + /// cannot be circumvented by extending tokens repeatedly. Such toke= ns can still be deleted > + /// and recreated. > + #[serde(default, skip_serializing_if =3D "Option::is_none")] > + pub disallow_expiry_changes: Option, see my notes on the perl version of this patch > +} > + > +impl TokenPolicy { > + /// Check a new or changed token expiration date against the policy. > + /// > + /// Must only be called when an operation actually sets or changes t= he expiration date, with > + /// an `expire` of 0 meaning no expiration date. This lets callers e= nforce the policy only for > + /// the expiration date an operation sets, so unrelated updates do n= ot retroactively reject > + /// tokens that predate the policy. `is_update` distinguishes updati= ng an existing token from > + /// creating a new one, `now` is the reference time the maximum life= time 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 maxim= um lifetime, so a set > + // maximum lifetime also requires an expiration date > + if expire =3D=3D 0 && (self.require_expiry.unwrap_or(false) || s= elf.max_lifetime.is_some()) { nit: should probably use `unwrap_or_default` > + bail!("the token policy requires an expiration date"); > + } > + > + if let Some(max_lifetime) =3D self.max_lifetime { > + // an extreme configured lifetime must not overflow, which p= anics in debug builds and > + // wraps into a bound rejecting everything otherwise > + if expire > 0 && expire > now.saturating_add(max_lifetime) { nit: the `expire > 0` seems redundant now + max_lifetime should always be a positive value afaict, so if that's true `expire > 0` holds trivially. `expire =3D=3D 0` is also checked above. > + bail!( > + "the expiration date exceeds the maximum token lifet= ime of {max_lifetime} \ > + seconds set by the token policy" > + ); > + } > + } > + > + Ok(()) > + } > +} > + > #[api( > properties: { > userid: { > @@ -316,3 +393,153 @@ pub const REGENERATE_TOKEN_SCHEMA: Schema =3D > BooleanSchema::new("Regenerate token secret while keeping permission= s.") > .default(false) > .schema(); > + > +#[cfg(test)] > +mod tests { > + use super::TokenPolicy; > + > + // fixed reference time and a 90 day maximum lifetime for determinis= tic bounds checks > + const NOW: i64 =3D 1_000_000_000; > + const MAX: i64 =3D 90 * 24 * 60 * 60; > + > + const EMPTY: TokenPolicy =3D TokenPolicy { > + require_expiry: None, > + max_lifetime: None, > + disallow_expiry_changes: None, > + }; > + > + const REQUIRE_EXPIRY: TokenPolicy =3D TokenPolicy { > + require_expiry: Some(true), > + ..EMPTY > + }; > + > + const MAX_LIFETIME: TokenPolicy =3D TokenPolicy { > + max_lifetime: Some(MAX), > + ..EMPTY > + }; > + > + const FIXED_EXPIRY: TokenPolicy =3D 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 r= ejected as well > + assert!(REQUIRE_EXPIRY.check_expiry_change(0, true, NOW).is_err(= )); > + // while a compliant expiration date change on update stays allo= wed > + 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 =3D 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 =3D 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 =3D 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(= )); > + } > +}