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 F0D7C1FF0B4 for ; Tue, 10 Nov 2026 16:05:07 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id DB2722164B; Tue, 10 Nov 2026 16:05:02 +0100 (CET) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Tue, 10 Nov 2026 16:04:58 +0100 Message-Id: Subject: Re: [PATCH proxmox 9/9] access-control: enforce token policy on token create and update To: "Thomas Lamprecht" , X-Mailer: aerc 0.20.0 References: <20260923210000.4031318-1-t.lamprecht@proxmox.com> <20260923210000.4031318-10-t.lamprecht@proxmox.com> In-Reply-To: <20260923210000.4031318-10-t.lamprecht@proxmox.com> From: "Shannon Sterz" X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1794323098297 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.648 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: NKWGPX2VGMGVFDPQTAXVYFPGJQCBFHQ3 X-Message-ID-Hash: NKWGPX2VGMGVFDPQTAXVYFPGJQCBFHQ3 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: > An update only checks the expiration date if its value 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, or rotated on update, to avoid > leaving an orphaned entry in the token shadow file, or losing the > old secret, for a rejected request. > > Signed-off-by: Thomas Lamprecht > --- > proxmox-access-control/src/api/tokens.rs | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/proxmox-access-control/src/api/tokens.rs b/proxmox-access-co= ntrol/src/api/tokens.rs > index f7417934..58295793 100644 > --- a/proxmox-access-control/src/api/tokens.rs > +++ b/proxmox-access-control/src/api/tokens.rs > @@ -97,6 +97,12 @@ pub fn generate_token( > ); > } > > + // check the policy before generating the secret to avoid leaving an= orphaned entry in the > + // token shadow file on rejection > + if let Some(policy) =3D crate::init::access_conf().token_policy()? { > + policy.check_expiry_change(expire.unwrap_or(0), false, proxmox_t= ime::epoch_i64())?; > + } > + > let secret =3D token_shadow::generate_and_set_secret(&tokenid)?; > > let token =3D ApiToken { > @@ -180,6 +186,7 @@ pub fn update_token( > let tokenid_string =3D tokenid.to_string(); > > let mut data: ApiToken =3D config.lookup("token", &tokenid_string)?; > + let old_expire =3D data.expire.unwrap_or(0); > > if let Some(delete) =3D delete { > for delete_prop in delete { > @@ -206,6 +213,16 @@ pub fn update_token( > data.expire =3D if expire > 0 { Some(expire) } else { None }; > } > > + // enforce the policy only if this update actually changes the expir= ation date, so unrelated > + // edits do not retroactively reject tokens that predate the policy;= check before a requested > + // secret regeneration so a rejected update does not rotate the secr= et either > + let new_expire =3D data.expire.unwrap_or(0); > + if new_expire !=3D old_expire { > + if let Some(policy) =3D crate::init::access_conf().token_policy(= )? { > + policy.check_expiry_change(new_expire, true, proxmox_time::e= poch_i64())?; > + } > + } nit: moving this check in the previous if-let clause that checks whether the expiry is to be updated would make it a bit cleaner imo. note that there is no way besides setting `expire =3D 0` to remove an expiry. > + > let new_secret =3D if regenerate =3D=3D Some(true) { > let secret =3D token_shadow::generate_and_set_secret(&tokenid)?; > Some(ApiTokenSecret { tokenid, secret })