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 129961FF09B for ; Mon, 31 Aug 2026 15:30:41 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id D86F421309; Mon, 31 Aug 2026 15:30:40 +0200 (CEST) Message-ID: <73eba739-16da-4b38-9aa4-6947064d0d5e@proxmox.com> Date: Mon, 31 Aug 2026 15:30:36 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v1 proxmox-backup] config: invalidate CachedUserInfo after ACL changes To: Christian Ebner , pbs-devel@lists.proxmox.com References: <20260818133556.298498-1-r.obkircher@proxmox.com> Content-Language: en-US, de-AT From: Robert Obkircher In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1788183022998 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.511 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: CKVGM5VUXTYCJYFRXQ33A3PPZM4QKOET X-Message-ID-Hash: CKVGM5VUXTYCJYFRXQ33A3PPZM4QKOET X-MailFrom: r.obkircher@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 Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On 28.08.26 11:18, Christian Ebner wrote: > One tiny nit inline and one high level comment: We currently do not > document the 5 second caching behavior for manual config file edits, > not for the user and not for the acl introduced here. We do have a > warning with respect to the more critical 60 seconds for > token.shadow caching [0] but maybe we could add a note to the end of > [1,2] for completeness as well?  Do we support manual changes at all? If so, we would also have to document that changes must be atomic renames instead of in-place updates, and that we recognize them based on mtime. The timeout is also not even guaranteed to be 5 seconds, because it is based on system time instead of a monotonic clock. I assume this is because Instant doesn't work on wasm, but it woudn't be that hard to conditionally re-export it in proxmox-sys and replace it with a performance.now() wrapper on wasm targets. (Imo the generation and timeout checks also belong in the individual cached_config methods instead of CachedUserInfo::new, but I'll leave it as-is for now.) > > [0] https://pbs.proxmox.com/docs/user-management.html#api-tokens > [1] > https://pbs.proxmox.com/docs/user-management.html#user-configuration > [2] https://pbs.proxmox.com/docs/user-management.html#access-control > > On 8/18/26 3:36 PM, Robert Obkircher wrote: >> The ACL tree was cached for up to 5 seconds in CachedUserInfo, even >> when the configuration was modified. This forced automated scripts to >> wait for an arbitrary amount of time, and it was an unnecessary >> security risk to keep revoked permissions for this long. >> >> Add a generation counter for acl.cfg and bump it upon save to force >> subsequent requests to reload the file. A separate counter was chosen >> for clarity, though incrementing the user_cache_generation would have >> worked as well. >> >> Fixes: https://forum.proxmox.com/threads/185745 >> Signed-off-by: Robert Obkircher > > Reviewed-by: Christian Ebner > Tested-by: Christian Ebner > >> --- >>   pbs-config/src/acl.rs                  | 11 +++++++++-- >>   pbs-config/src/cached_user_info.rs     |  5 +++++ >>   pbs-config/src/config_version_cache.rs | 18 ++++++++++++++++++ >>   3 files changed, 32 insertions(+), 2 deletions(-) >> >> diff --git a/pbs-config/src/acl.rs b/pbs-config/src/acl.rs >> index 8612abed7..584238cde 100644 >> --- a/pbs-config/src/acl.rs >> +++ b/pbs-config/src/acl.rs >> @@ -11,7 +11,7 @@ use proxmox_schema::{ApiStringFormat, ApiType, >> Schema, StringSchema}; >>     use pbs_api_types::{Authid, ROLE_NAME_NO_ACCESS, Role, Userid}; >>   -use crate::{BackupLockGuard, open_backup_lockfile}; >> +use crate::{BackupLockGuard, ConfigVersionCache, >> open_backup_lockfile}; >>     /// Map of pre-defined [Roles](Role) to their associated >>   /// [privileges](pbs_api_types::PRIVILEGES) combination and >> description. >> @@ -768,7 +768,14 @@ pub fn save_config(acl: &AclTree) -> >> Result<(), Error> { >>         acl.write_config(&mut raw)?; >>   -    replace_privileged_config(ACL_CFG_FILENAME, &raw) >> +    replace_privileged_config(ACL_CFG_FILENAME, &raw)?; >> + >> +    // increase acl version >> +    // We use this in CachedUserInfo > > nit: I see that this is pre-existing in > pbs_config::user::save_config(), but these comments do not give much > context, especially the first line is redundant as already implied > by the method call below. > > I would suggest to either drop the comment altogether or be more > verbose, e.g.: > `generation bump invalidates cached user and acl tree in > CachedUserInfo` > > and adapt the comment in user config as well.  Yeah, I'll fix this in a v2. > > [..]