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 A568C1FF0AB for ; Wed, 23 Sep 2026 23:01:33 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 04F5821782; Wed, 23 Sep 2026 23:00:17 +0200 (CEST) From: Thomas Lamprecht To: pve-devel@lists.proxmox.com Subject: [PATCH access-control 2/9] fix #7805: api: token: enforce datacenter token policy Date: Wed, 23 Sep 2026 22:59:51 +0200 Message-ID: <20260923210000.4031318-3-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: 1790197207279 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.473 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) POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes PROLO_LEO1 0.1 Meta Catches all Leo drug variations so far 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: WWEC6OAKQV3Y6LFEUF47UPXNB3BI772G X-Message-ID-Hash: WWEC6OAKQV3Y6LFEUF47UPXNB3BI772G 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: 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 --- 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