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

[Robert Obkircher <r.obkircher@proxmox.com>]

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 <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. 

Yeah, I'll fix this in a v2.

>
> [..]