Re: [PATCH proxmox 8/9] access-control: add API token policy type with expiry checks

["Shannon Sterz" <s.sterz@proxmox.com>]

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 <t.lamprecht@proxmox.com>
> ---
>  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<Option<TokenPolicy>, 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<bool>,
> +
> +    /// 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<i64>,

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 tokens can still be deleted
> +    /// and recreated.
> +    #[serde(default, skip_serializing_if = "Option::is_none")]
> +    pub disallow_expiry_changes: Option<bool>,

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 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()) {

nit: should probably use `unwrap_or_default`

> +            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) {

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 == 0` is also checked above.

> +                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());
> +    }
> +}