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

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

[Robert Obkircher]

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>
---
 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
+    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);
+    }
 }
-- 
2.47.3

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

[Christian Ebner]

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);
> +    }
>   }