From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id EA6891FF0C5 for ; Fri, 28 Aug 2026 11:18:21 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 4CBBB213F6; Fri, 28 Aug 2026 11:18:21 +0200 (CEST) Message-ID: Date: Fri, 28 Aug 2026 11:18:15 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v1 proxmox-backup] config: invalidate CachedUserInfo after ACL changes To: Robert Obkircher , pbs-devel@lists.proxmox.com References: <20260818133556.298498-1-r.obkircher@proxmox.com> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <20260818133556.298498-1-r.obkircher@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1787908686627 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.703 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: 4CU5NPBPEWZKKTEVSQ7ONWS3PY52R635 X-Message-ID-Hash: 4CU5NPBPEWZKKTEVSQ7ONWS3PY52R635 X-MailFrom: c.ebner@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: 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? [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. > + let version_cache = ConfigVersionCache::new()?; > + version_cache.increase_acl_cache_generation(); > + > + Ok(()) > } > > #[cfg(test)] > diff --git a/pbs-config/src/cached_user_info.rs b/pbs-config/src/cached_user_info.rs > index 0c1c1ed6d..511c8f160 100644 > --- a/pbs-config/src/cached_user_info.rs > +++ b/pbs-config/src/cached_user_info.rs > @@ -23,6 +23,7 @@ struct ConfigCache { > data: Option>, > last_update: i64, > last_user_cache_generation: usize, > + last_acl_cache_generation: usize, > } > > static CACHED_CONFIG: LazyLock> = LazyLock::new(|| { > @@ -30,6 +31,7 @@ static CACHED_CONFIG: LazyLock> = LazyLock::new(|| { > data: None, > last_update: 0, > last_user_cache_generation: 0, > + last_acl_cache_generation: 0, > }) > }); > > @@ -40,11 +42,13 @@ impl CachedUserInfo { > > let version_cache = ConfigVersionCache::new()?; > let user_cache_generation = version_cache.user_cache_generation(); > + let acl_cache_generation = version_cache.acl_cache_generation(); > > { > // limit scope > let cache = CACHED_CONFIG.read().unwrap(); > if (user_cache_generation == cache.last_user_cache_generation) > + && (acl_cache_generation == cache.last_acl_cache_generation) > && ((now - cache.last_update) < 5) > { > if let Some(ref config) = cache.data { > @@ -61,6 +65,7 @@ impl CachedUserInfo { > let mut cache = CACHED_CONFIG.write().unwrap(); > cache.last_update = now; > cache.last_user_cache_generation = user_cache_generation; > + cache.last_acl_cache_generation = acl_cache_generation; > cache.data = Some(config.clone()); > > Ok(config) > diff --git a/pbs-config/src/config_version_cache.rs b/pbs-config/src/config_version_cache.rs > index 5053f6dc6..600b7e0a1 100644 > --- a/pbs-config/src/config_version_cache.rs > +++ b/pbs-config/src/config_version_cache.rs > @@ -29,6 +29,8 @@ struct ConfigVersionCacheDataInner { > datastore_generation: AtomicUsize, > // Token shadow (token.shadow) generation/version. > token_shadow_generation: AtomicUsize, > + // ACL (acl.cfg) cache generation/version. > + acl_cache_generation: AtomicUsize, > // Add further atomics here > } > > @@ -177,4 +179,20 @@ impl ConfigVersionCache { > .token_shadow_generation > .fetch_add(1, Ordering::AcqRel) > } > + > + /// Returns the acl cache generation number. > + pub fn acl_cache_generation(&self) -> usize { > + self.shmem > + .data() > + .acl_cache_generation > + .load(Ordering::Acquire) > + } > + > + /// Increase the acl cache generation number. > + pub fn increase_acl_cache_generation(&self) { > + self.shmem > + .data() > + .acl_cache_generation > + .fetch_add(1, Ordering::AcqRel); > + } > }