[PATCH access-control 2/9] fix #7805: api: token: enforce datacenter token policy

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Check the new datacenter.cfg token-policy option on API token
creation and update, so admins can centrally require expiration
dates, bound the maximum token lifetime, forbid changing existing
expiration dates, and require privilege separation, as commonly
mandated by compliance rules like PCI DSS, SOC 2, or ISO 27001.

Existing tokens stay valid, and an update only checks settings it
actually changes, so unrelated edits or clients resubmitting
unchanged fields are not rejected for tokens that predate the policy.
The check runs before the secret is generated to avoid leaving an
orphaned entry in token.cfg on rejection. Regenerating the secret of
an existing token is deliberately not restricted, rotation does not
extend validity and blocking it would only discourage rotating old
credentials.

Changing the policy itself requires Sys.Modify on '/', like any
other datacenter option. An extra auth-specific privilege check was
skipped for now, for the current built-in roles it would land in the
same roles as Sys.Modify anyway, and anyone allowed to change
datacenter options can adapt the policy in any case.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 src/PVE/API2/User.pm              |  32 ++++-
 src/PVE/AccessControl.pm          |  59 ++++++++
 src/test/Makefile                 |   2 +
 src/test/token-policy-api-test.pl | 102 ++++++++++++++
 src/test/token-policy-test.pl     | 217 ++++++++++++++++++++++++++++++
 5 files changed, 409 insertions(+), 3 deletions(-)
 create mode 100644 src/test/token-policy-api-test.pl
 create mode 100644 src/test/token-policy-test.pl

diff --git a/src/PVE/API2/User.pm b/src/PVE/API2/User.pm
index 579783b..0155b6b 100644
--- a/src/PVE/API2/User.pm
+++ b/src/PVE/API2/User.pm
@@ -806,14 +806,20 @@ __PACKAGE__->register_method({
             die "Token already exists.\n"
                 if defined(PVE::AccessControl::check_token_exist($usercfg, $userid, $tokenid, 1));
 
-            $full_tokenid = PVE::AccessControl::join_tokenid($userid, $tokenid);
-            $value = PVE::TokenConfig::generate_token($full_tokenid);
-
             $token = {};
             $token->{privsep} = defined($param->{privsep}) ? $param->{privsep} : 1;
             $token->{expire} = $param->{expire} if defined($param->{expire});
             $token->{comment} = $param->{comment} if $param->{comment};
 
+            # check policy before generating the secret to avoid leaving an orphan in token.cfg
+            PVE::AccessControl::assert_token_policy({
+                expire => $token->{expire} // 0,
+                'privilege-separation' => $token->{privsep},
+            });
+
+            $full_tokenid = PVE::AccessControl::join_tokenid($userid, $tokenid);
+            $value = PVE::TokenConfig::generate_token($full_tokenid);
+
             $usercfg->{users}->{$userid}->{tokens}->{$tokenid} = $token;
             cfs_write_file("user.cfg", $usercfg);
         };
@@ -899,11 +905,31 @@ __PACKAGE__->register_method({
                 $usercfg = cfs_read_file("user.cfg");
                 $token = PVE::AccessControl::check_token_exist($usercfg, $userid, $tokenid);
 
+                my $old_expire = $token->{expire} // 0;
+                my $old_privilege_separation = $token->{privsep} // 0;
+
                 $token->{privsep} = $param->{privsep} if defined($param->{privsep});
                 $token->{expire} = $param->{expire} if defined($param->{expire});
                 $token->{comment} = $param->{comment} if defined($param->{comment});
                 delete $token->{comment} if (!length $token->{comment});
 
+                my $new_expire = $token->{expire} // 0;
+                my $new_privilege_separation = $token->{privsep} // 0;
+
+                # enforce the policy only on the dimensions this update actually changes, so
+                # unrelated edits (or a client that resubmits unchanged fields) do not
+                # retroactively reject tokens that predate the policy
+                my $expire_change = $new_expire != $old_expire ? $new_expire : undef;
+                my $privilege_separation_change =
+                    $new_privilege_separation != $old_privilege_separation
+                    ? $new_privilege_separation
+                    : undef;
+                PVE::AccessControl::assert_token_policy({
+                    expire => $expire_change,
+                    'privilege-separation' => $privilege_separation_change,
+                    'is-update' => 1,
+                });
+
                 my $deletable = {
                     comment => 1,
                 };
diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index 8879ab8..48e02cb 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -21,6 +21,7 @@ use PVE::OTP;
 use PVE::Ticket;
 use PVE::Tools qw(run_command lock_file file_get_contents split_list safe_print);
 use PVE::Cluster qw(cfs_register_file cfs_read_file cfs_write_file cfs_lock_file);
+use PVE::DataCenterConfig; # ensure the 'datacenter.cfg' cfs parser is registered
 use PVE::JSONSchema qw(register_standard_option get_standard_option);
 
 use PVE::RS::TFA;
@@ -716,6 +717,64 @@ sub check_token_exist {
     return undef;
 }
 
+# Assert that a token's expiration and privilege-separation setting conform to the cluster-wide
+# token policy from datacenter.cfg. Dies with a descriptive message on violation.
+#
+# Takes a hash reference with the following keys:
+# - expire: the effective expiration date (epoch seconds, 0 == never); undef skips all
+#   expiration-related checks
+# - privilege-separation: the effective privilege separation flag; undef skips that dimension
+# - is-update: whether an existing token is updated; with 'disallow-expiry-changes' configured,
+#   any passed (i.e. actually changed) expiration date is then rejected outright
+# - ctime, policy: override the reference time to bound the expiration date against and the
+#   policy from the datacenter config, mainly for testing
+#
+# The checks are independent per dimension and only run for values that are actually passed. This
+# lets callers enforce a dimension only when the current operation sets it, so unrelated updates
+# (e.g. a comment change) do not retroactively reject tokens that predate the policy.
+sub assert_token_policy {
+    my ($param) = @_;
+
+    # integer-typed API parameters keep their submitted string spelling, and zero spellings
+    # like '00', '+0', or '-0' are true as Perl scalars, so numify to not mistake them for a
+    # set expiration date in the truthiness checks below
+    my $expire = defined($param->{expire}) ? int($param->{expire}) : undef;
+    my $privilege_separation = $param->{'privilege-separation'};
+
+    return if !defined($expire) && !defined($privilege_separation);
+
+    my $policy = $param->{policy} // cfs_read_file('datacenter.cfg')->{'token-policy'};
+    return if !$policy;
+
+    if (defined($expire)) {
+        die "the datacenter token policy does not allow changing the expiration date of an"
+            . " existing token\n"
+            if $param->{'is-update'} && $policy->{'disallow-expiry-changes'};
+
+        my $ctime = $param->{ctime} // time();
+        my $max_lifetime = $policy->{'max-lifetime'};
+        # a token that never expires trivially exceeds any finite maximum lifetime, so a set
+        # maximum lifetime also requires an expiration date
+        my $require_expiry = $policy->{'require-expiry'} || $max_lifetime;
+
+        die "the datacenter token policy requires an expiration date\n"
+            if $require_expiry && !$expire;
+
+        if ($max_lifetime && $expire && $expire > $ctime + $max_lifetime) {
+            die "the expiration date exceeds the maximum token lifetime of $max_lifetime seconds"
+                . " set by the datacenter token policy\n";
+        }
+    }
+
+    if (
+        defined($privilege_separation)
+        && $policy->{'require-privilege-separation'}
+        && !$privilege_separation
+    ) {
+        die "the datacenter token policy requires privilege separation\n";
+    }
+}
+
 # deprecated
 sub verify_one_time_pw {
     my ($type, $username, $keys, $tfa_cfg, $otp) = @_;
diff --git a/src/test/Makefile b/src/test/Makefile
index 53f2da7..51db8cb 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -13,4 +13,6 @@ check:
 	perl -I.. perm-test7.pl
 	perl -I.. perm-test8.pl
 	perl -I.. realm_sync_test.pl
+	perl -I.. token-policy-test.pl
+	perl -I.. token-policy-api-test.pl
 	perl -I.. api-tests.pl
diff --git a/src/test/token-policy-api-test.pl b/src/test/token-policy-api-test.pl
new file mode 100644
index 0000000..dece4db
--- /dev/null
+++ b/src/test/token-policy-api-test.pl
@@ -0,0 +1,102 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use Test::More;
+use Test::MockModule;
+
+use PVE::AccessControl;
+use PVE::API2::User;
+
+# Exercise the token create and update API handlers with the real schema validation and user
+# config parser/writer, mocking out the cluster file system and secret generation. The integer
+# schema accepts zero spellings like '00', '+0', or '-0' without canonicalizing them, so the
+# policy enforcement at the API boundary must not mistake them for a set expiration date.
+
+my ($policy, $user_cfg_raw, $secret_calls);
+
+my $read = sub {
+    my ($file) = @_;
+    return PVE::AccessControl::parse_user_config($file, $user_cfg_raw) if $file eq 'user.cfg';
+    return { 'token-policy' => $policy } if $file eq 'datacenter.cfg';
+    die "unexpected cfs_read_file('$file') call\n";
+};
+
+my $access_control_module = Test::MockModule->new('PVE::AccessControl');
+$access_control_module->mock(cfs_read_file => $read);
+$access_control_module->mock(lock_user_config => sub { $_[0]->() });
+
+my $api_user_module = Test::MockModule->new('PVE::API2::User');
+$api_user_module->mock(cfs_read_file => $read);
+$api_user_module->mock(
+    cfs_write_file => sub {
+        my ($file, $cfg) = @_;
+        die "unexpected cfs_write_file('$file') call\n" if $file ne 'user.cfg';
+        $user_cfg_raw = PVE::AccessControl::write_user_config($file, $cfg);
+    },
+);
+
+my $token_config_module = Test::MockModule->new('PVE::TokenConfig');
+$token_config_module->mock(
+    generate_token => sub {
+        $secret_calls++;
+        return 'test-secret';
+    },
+);
+
+my $reset = sub {
+    my ($test_policy, $expire) = @_;
+    $policy = $test_policy;
+    $user_cfg_raw = "user:test\@pve:1:0::::::\n";
+    $user_cfg_raw .= "token:test\@pve!old:$expire:1:some comment:\n" if defined($expire);
+    $secret_calls = 0;
+};
+
+my $invoke = sub {
+    my ($method, $param) = @_;
+    my $ok = eval {
+        PVE::API2::User->$method({ userid => 'test@pve', %$param });
+        1;
+    };
+    return ($ok, $@);
+};
+
+my $now = time();
+
+for my $policy_variant ({ 'require-expiry' => 1 }, { 'max-lifetime' => 86400 }) {
+    my $desc = join(',', map { "$_=$policy_variant->{$_}" } sort keys $policy_variant->%*);
+
+    for my $zero ('0', '00', '+0', '-0') {
+        $reset->($policy_variant);
+        my ($ok, $err) = $invoke->('generate_token', { tokenid => 'new', expire => $zero });
+        ok(
+            !$ok && $err =~ /requires an expiration date/,
+            "$desc: create with expire=$zero rejected",
+        );
+        is($secret_calls, 0, "$desc: rejected create generates no secret");
+        ok($user_cfg_raw !~ /!new/, "$desc: rejected create writes no token");
+
+        $reset->($policy_variant, $now + 600);
+        my $before = $user_cfg_raw;
+        ($ok, $err) =
+            $invoke->('update_token_info', { tokenid => 'old', expire => $zero, regenerate => 1 });
+        ok(
+            !$ok && $err =~ /requires an expiration date/, "$desc: update to expire=$zero rejected",
+        );
+        is($secret_calls, 0, "$desc: rejected update rotates no secret");
+        is($user_cfg_raw, $before, "$desc: rejected update writes nothing");
+    }
+
+    $reset->($policy_variant);
+    my ($ok, $err) = $invoke->('generate_token', { tokenid => 'new', expire => $now + 600 });
+    ok($ok, "$desc: compliant create accepted") or diag($err);
+    is($secret_calls, 1, "$desc: accepted create generates a secret");
+}
+
+# unrelated updates of a token that predates the policy must keep working
+$reset->({ 'require-expiry' => 1 }, 0);
+my ($ok, $err) = $invoke->('update_token_info', { tokenid => 'old', comment => 'changed' });
+ok($ok, 'unrelated update of a non-conforming token stays allowed') or diag($err);
+
+done_testing();
diff --git a/src/test/token-policy-test.pl b/src/test/token-policy-test.pl
new file mode 100644
index 0000000..f4c8fa5
--- /dev/null
+++ b/src/test/token-policy-test.pl
@@ -0,0 +1,217 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use Test::More;
+use Test::MockModule;
+
+use PVE::AccessControl;
+
+# tests pass the policy explicitly; keep the fallback read of datacenter.cfg from hitting a
+# real pmxcfs so the cases without a policy exercise that path against an empty config
+my $access_control_module = Test::MockModule->new('PVE::AccessControl');
+$access_control_module->mock(
+    cfs_read_file => sub {
+        my ($file) = @_;
+        die "unexpected cfs_read_file('$file') call\n" if $file ne 'datacenter.cfg';
+        return {};
+    },
+);
+
+# fixed reference time and a 90-day maximum lifetime for deterministic bounds checks
+my $now = 1_000_000_000;
+my $max = 90 * 24 * 60 * 60; # 7776000 seconds
+
+# assert_token_policy({...}) dies on a policy violation. The 'expire' and
+# 'privilege-separation' keys may be undef to skip that dimension (used for partial updates);
+# 'is-update' marks an update of an existing token (relevant for 'disallow-expiry-changes').
+sub check {
+    my ($desc, $policy, $param, $should_die) = @_;
+
+    my $err;
+    eval {
+        PVE::AccessControl::assert_token_policy({
+            ctime => $now,
+            policy => $policy,
+            $param->%*,
+        });
+        1;
+    } or $err = $@;
+
+    if ($should_die) {
+        ok($err, "$desc: rejected") or diag("expected rejection but call succeeded");
+    } else {
+        ok(!$err, "$desc: accepted") or diag("unexpected rejection: $err");
+    }
+}
+
+# no policy configured -> everything is allowed
+check(
+    'no policy, never-expiring full-priv token',
+    undef,
+    { expire => 0, 'privilege-separation' => 0 },
+    0,
+);
+check('no policy, undef dimensions', undef, {}, 0);
+check(
+    'empty policy, never-expiring full-priv token',
+    {},
+    { expire => 0, 'privilege-separation' => 0 },
+    0,
+);
+
+# require-expiry
+my $req_expire = { 'require-expiry' => 1 };
+check('require-expiry, no expiry', $req_expire, { expire => 0 }, 1);
+check('require-expiry, future expiry', $req_expire, { expire => $now + 100 }, 0);
+check('require-expiry, expiry dimension not touched', $req_expire, {}, 0);
+check(
+    'require-expiry, update removing the expiry',
+    $req_expire,
+    { expire => 0, 'is-update' => 1 },
+    1,
+);
+check(
+    'require-expiry, does not constrain privsep',
+    $req_expire,
+    { expire => $now + 100, 'privilege-separation' => 0 },
+    0,
+);
+
+# max-lifetime implies an expiry is required and bounds how far it may reach
+my $max_life = { 'max-lifetime' => $max };
+check('max-lifetime, never-expiring is rejected', $max_life, { expire => 0 }, 1);
+check('max-lifetime, within bound', $max_life, { expire => $now + 100 }, 0);
+check('max-lifetime, exactly at bound', $max_life, { expire => $now + $max }, 0);
+check('max-lifetime, one second past bound', $max_life, { expire => $now + $max + 1 }, 1);
+check('max-lifetime, expiry dimension not touched', $max_life, {}, 0);
+check(
+    'max-lifetime, update prolonging past the bound',
+    $max_life,
+    { expire => $now + $max + 1, 'is-update' => 1 },
+    1,
+);
+check('max-lifetime, expiry in the past accepted', $max_life, { expire => $now - 100 }, 0);
+check(
+    'max-lifetime with explicit require-expiry=0 still requires an expiry',
+    { 'require-expiry' => 0, 'max-lifetime' => $max },
+    { expire => 0 },
+    1,
+);
+
+# the integer schema passes zero spellings like '00' through with their submitted string
+# spelling, and those are true as Perl scalars; they must still count as "no expiration date"
+for my $zero ('0', '00', '+0', '-0') {
+    check("require-expiry, create with zero spelling '$zero'", $req_expire, { expire => $zero }, 1);
+    check(
+        "require-expiry, update removing the expiry via zero spelling '$zero'",
+        $req_expire,
+        { expire => $zero, 'is-update' => 1 },
+        1,
+    );
+    check("max-lifetime, create with zero spelling '$zero'", $max_life, { expire => $zero }, 1);
+}
+
+# disallow-expiry-changes rejects any changed expiration date on update, but neither the
+# initial expiration date on creation nor updates that leave the expiration date untouched
+my $fixed_expiry = { 'disallow-expiry-changes' => 1 };
+check(
+    'disallow-expiry-changes, update changing expiry',
+    $fixed_expiry,
+    { expire => $now + 100, 'is-update' => 1 },
+    1,
+);
+check(
+    'disallow-expiry-changes, update shortening expiry',
+    $fixed_expiry,
+    { expire => $now - 100, 'is-update' => 1 },
+    1,
+);
+check(
+    'disallow-expiry-changes, update removing expiry',
+    $fixed_expiry,
+    { expire => 0, 'is-update' => 1 },
+    1,
+);
+check('disallow-expiry-changes, update not touching expiry', $fixed_expiry, { 'is-update' => 1 },
+    0);
+check('disallow-expiry-changes, creation sets expiry', $fixed_expiry, { expire => $now + 100 }, 0);
+check('disallow-expiry-changes, creation without expiry', $fixed_expiry, { expire => 0 }, 0);
+check(
+    'disallow-expiry-changes, does not constrain privsep',
+    $fixed_expiry,
+    { 'privilege-separation' => 0, 'is-update' => 1 },
+    0,
+);
+check(
+    'disallow-expiry-changes wins over a compliant new expiry under require-expiry',
+    { 'disallow-expiry-changes' => 1, 'require-expiry' => 1 },
+    { expire => $now + 100, 'is-update' => 1 },
+    1,
+);
+
+# require-privilege-separation
+my $require_privilege_separation = { 'require-privilege-separation' => 1 };
+check(
+    'require-privilege-separation, full-priv token rejected',
+    $require_privilege_separation,
+    { 'privilege-separation' => 0 },
+    1,
+);
+check(
+    'require-privilege-separation, privsep token accepted',
+    $require_privilege_separation,
+    { 'privilege-separation' => 1 },
+    0,
+);
+check(
+    'require-privilege-separation, privsep dimension not touched',
+    $require_privilege_separation,
+    {},
+    0,
+);
+check(
+    'require-privilege-separation, does not constrain expiry',
+    $require_privilege_separation,
+    { expire => 0, 'privilege-separation' => 1 },
+    0,
+);
+
+# combined policy, as an admin would set for a compliance baseline
+my $full = { 'require-expiry' => 1, 'max-lifetime' => $max, 'require-privilege-separation' => 1 };
+check(
+    'full policy, compliant token',
+    $full,
+    { expire => $now + 100, 'privilege-separation' => 1 },
+    0,
+);
+check('full policy, missing expiry', $full, { expire => 0, 'privilege-separation' => 1 }, 1);
+check(
+    'full policy, expiry too far out',
+    $full,
+    { expire => $now + $max + 1, 'privilege-separation' => 1 },
+    1,
+);
+check(
+    'full policy, full-priv token',
+    $full,
+    { expire => $now + 100, 'privilege-separation' => 0 },
+    1,
+);
+# partial update: only the touched dimension is enforced (decision to not retroactively
+# reject tokens that predate the policy on unrelated edits)
+check(
+    'full policy, update touching only a valid expiry',
+    $full,
+    { expire => $now + 100, 'is-update' => 1 },
+    0,
+);
+check(
+    'full policy, update touching only privsep',
+    $full,
+    { 'privilege-separation' => 1, 'is-update' => 1 },
+    0,
+);
+
+done_testing();
-- 
2.47.3