Re: [PATCH v1 proxmox-backup] config: invalidate CachedUserInfo after ACL changes

[Christian Ebner <c.ebner@proxmox.com>]

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 <r.obkircher@proxmox.com>

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
Tested-by: Christian Ebner <c.ebner@proxmox.com>

> ---
>   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<Arc<CachedUserInfo>>,
>       last_update: i64,
>       last_user_cache_generation: usize,
> +    last_acl_cache_generation: usize,
>   }
>   
>   static CACHED_CONFIG: LazyLock<RwLock<ConfigCache>> = LazyLock::new(|| {
> @@ -30,6 +31,7 @@ static CACHED_CONFIG: LazyLock<RwLock<ConfigCache>> = 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);
> +    }
>   }