Re: [pbs-devel] [PATCH proxmox-backup v3 3/4] pbs-config: invalidate token-secret cache on token.shadow changes

[Samuel Rufinatscha <s.rufinatscha@proxmox.com>]

On 1/14/26 11:44 AM, Fabian Grünbichler wrote:
> On January 2, 2026 5:07 pm, Samuel Rufinatscha wrote:
>> Previously the in-memory token-secret cache was only updated via
>> set_secret() and delete_secret(), so manual edits to token.shadow were
>> not reflected.
>>
>> This patch adds file change detection to the cache. It tracks the mtime
>> and length of token.shadow and clears the in-memory token secret cache
>> whenever these values change.
>>
>> Note, this patch fetches file stats on every request. An TTL-based
>> optimization will be covered in a subsequent patch of the series.
>>
>> This patch is part of the series which fixes bug #7017 [1].
>>
>> [1] https://bugzilla.proxmox.com/show_bug.cgi?id=7017
>>
>> Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>
>> ---
>> Changes from v1 to v2:
>>
>> * Add file metadata tracking (file_mtime, file_len) and
>>    FILE_GENERATION.
>> * Store file_gen in CachedSecret and verify it against the current
>>    FILE_GENERATION to ensure cached entries belong to the current file
>>    state.
>> * Add shadow_mtime_len() helper and convert refresh to best-effort
>>    (try_write, returns bool).
>> * Pass a pre-write metadata snapshot into apply_api_mutation and
>>    clear/bump generation if the cache metadata indicates missed external
>>    edits.
>>
>> Changes from v2 to v3:
>>
>> * Cache now tracks last_checked (epoch seconds).
>> * Simplified refresh_cache_if_file_changed, removed
>> FILE_GENERATION logic
>> * On first load, initializes file metadata and keeps empty cache.
>>
>>   pbs-config/src/token_shadow.rs | 122 +++++++++++++++++++++++++++++++--
>>   1 file changed, 118 insertions(+), 4 deletions(-)
>>
>> diff --git a/pbs-config/src/token_shadow.rs b/pbs-config/src/token_shadow.rs
>> index fa84aee5..02fb191b 100644
>> --- a/pbs-config/src/token_shadow.rs
>> +++ b/pbs-config/src/token_shadow.rs
>> @@ -1,5 +1,8 @@
>>   use std::collections::HashMap;
>> +use std::fs;
>> +use std::io::ErrorKind;
>>   use std::sync::LazyLock;
>> +use std::time::SystemTime;
>>   
>>   use anyhow::{bail, format_err, Error};
>>   use parking_lot::RwLock;
>> @@ -7,6 +10,7 @@ use serde::{Deserialize, Serialize};
>>   use serde_json::{from_value, Value};
>>   
>>   use proxmox_sys::fs::CreateOptions;
>> +use proxmox_time::epoch_i64;
>>   
>>   use pbs_api_types::Authid;
>>   //use crate::auth;
>> @@ -24,6 +28,9 @@ static TOKEN_SECRET_CACHE: LazyLock<RwLock<ApiTokenSecretCache>> = LazyLock::new
>>       RwLock::new(ApiTokenSecretCache {
>>           secrets: HashMap::new(),
>>           shared_gen: 0,
>> +        file_mtime: None,
>> +        file_len: None,
>> +        last_checked: None,
>>       })
>>   });
>>   
>> @@ -62,6 +69,63 @@ fn write_file(data: HashMap<Authid, String>) -> Result<(), Error> {
>>       proxmox_sys::fs::replace_file(CONF_FILE, &json, options, true)
>>   }
>>   
>> +/// Refreshes the in-memory cache if the on-disk token.shadow file changed.
>> +/// Returns true if the cache is valid to use, false if not.
>> +fn refresh_cache_if_file_changed() -> bool {
>> +    let now = epoch_i64();
>> +
>> +    // Best-effort refresh under write lock.
>> +    let Some(mut cache) = TOKEN_SECRET_CACHE.try_write() else {
>> +        return false;
>> +    };
>> +
>> +    let Some(shared_gen_now) = token_shadow_shared_gen() else {
>> +        return false;
>> +    };
>> +
>> +    // If another process bumped the generation, we don't know what changed -> clear cache
>> +    if cache.shared_gen != shared_gen_now {
>> +        invalidate_cache_state(&mut cache);
>> +        cache.shared_gen = shared_gen_now;
>> +    }
>> +
>> +    // Stat the file to detect manual edits.
>> +    let Ok((new_mtime, new_len)) = shadow_mtime_len() else {
>> +        return false;
>> +    };
>> +
>> +    // Initialize file stats if we have no prior state.
>> +    if cache.last_checked.is_none() {
>> +        cache.secrets.clear(); // ensure cache is empty on first load
>> +        cache.file_mtime = new_mtime;
>> +        cache.file_len = new_len;
>> +        cache.last_checked = Some(now);
>> +        return true;
> 
> this code here
> 
>> +    }
>> +
>> +    // No change detected.
>> +    if cache.file_mtime == new_mtime && cache.file_len == new_len {
>> +        cache.last_checked = Some(now);
>> +        return true;
>> +    }
>> +
>> +    // Manual edit detected -> invalidate cache and update stat.
>> +    cache.secrets.clear();
>> +    cache.file_mtime = new_mtime;
>> +    cache.file_len = new_len;
>> +    cache.last_checked = Some(now);
> 
> and this code here are identical. if this is the first invocation, then
> the change detection check above cannot be true (the cached mtime and
> len will be None).
> 
> so we can drop the first if above, and replace the last line in this
> hunk with
> 
> let prev_last_checked = cache.last_checked.replace(Some(now));
>
> and then skip bumping the generation if this is_none()

Great idea about the .replace()! Integrating it with the new
ShadowFileInfo :)

> 
> OTOH, if we just cleared the cache here, does it make sense to return
> true? the cache is empty, so likely querying it *now* makes no sense?

Agree, we should just return false here

> 
>> +
>> +    // Best-effort propagation to other processes + update local view.
>> +    if let Some(shared_gen_new) = bump_token_shadow_shared_gen() {
>> +        cache.shared_gen = shared_gen_new;
>> +    } else {
>> +        // Do not fail: local cache is already safe as we cleared it above.
>> +        // Keep local shared_gen as-is to avoid repeated failed attempts.
>> +    }
>> +
>> +    true
>> +}
>> +
>>   /// Verifies that an entry for given tokenid / API token secret exists
>>   pub fn verify_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> {
>>       if !tokenid.is_token() {
>> @@ -69,7 +133,7 @@ pub fn verify_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> {
>>       }
>>   
>>       // Fast path
>> -    if cache_try_secret_matches(tokenid, secret) {
>> +    if refresh_cache_if_file_changed() && cache_try_secret_matches(tokenid, secret) {
>>           return Ok(());
>>       }
>>   
>> @@ -109,12 +173,15 @@ fn set_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> {
>>   
>>       let _guard = lock_config()?;
>>   
>> +    // Capture state before we write to detect external edits.
>> +    let pre_meta = shadow_mtime_len().unwrap_or((None, None));
>> +
>>       let mut data = read_file()?;
>>       let hashed_secret = proxmox_sys::crypt::encrypt_pw(secret)?;
>>       data.insert(tokenid.clone(), hashed_secret);
>>       write_file(data)?;
>>   
>> -    apply_api_mutation(tokenid, Some(secret));
>> +    apply_api_mutation(tokenid, Some(secret), pre_meta);
>>   
>>       Ok(())
>>   }
>> @@ -127,11 +194,14 @@ pub fn delete_secret(tokenid: &Authid) -> Result<(), Error> {
>>   
>>       let _guard = lock_config()?;
>>   
>> +    // Capture state before we write to detect external edits.
>> +    let pre_meta = shadow_mtime_len().unwrap_or((None, None));
>> +
>>       let mut data = read_file()?;
>>       data.remove(tokenid);
>>       write_file(data)?;
>>   
>> -    apply_api_mutation(tokenid, None);
>> +    apply_api_mutation(tokenid, None, pre_meta);
>>   
>>       Ok(())
>>   }
>> @@ -145,6 +215,12 @@ struct ApiTokenSecretCache {
>>       secrets: HashMap<Authid, CachedSecret>,
>>       /// Shared generation to detect mutations of the underlying token.shadow file.
>>       shared_gen: usize,
>> +    // shadow file mtime to detect changes
>> +    file_mtime: Option<SystemTime>,
>> +    // shadow file length to detect changes
>> +    file_len: Option<u64>,
>> +    // last time the file metadata was checked
>> +    last_checked: Option<i64>,
> 
> these three are always set together, so wouldn't it make more sense to
> make them an Option<ShadowFileInfo> ?
>
>>   }
>>   
>>   /// Cached secret.
>> @@ -204,7 +280,13 @@ fn cache_try_secret_matches(tokenid: &Authid, secret: &str) -> bool {
>>       eq && gen2 == cache_gen
>>   }
>>   
>> -fn apply_api_mutation(tokenid: &Authid, new_secret: Option<&str>) {
>> +fn apply_api_mutation(
>> +    tokenid: &Authid,
>> +    new_secret: Option<&str>,
>> +    pre_write_meta: (Option<SystemTime>, Option<u64>),
>> +) {
>> +    let now = epoch_i64();
>> +
>>       // Signal cache invalidation to other processes (best-effort).
>>       let new_shared_gen = bump_token_shadow_shared_gen();
>>   
>> @@ -220,6 +302,13 @@ fn apply_api_mutation(tokenid: &Authid, new_secret: Option<&str>) {
>>       // Update to the post-mutation generation.
>>       cache.shared_gen = gen;
>>   
>> +    // If our cached file metadata does not match the on-disk state before our write,
>> +    // we likely missed an external/manual edit. We can no longer trust any cached secrets.
>> +    let (pre_mtime, pre_len) = pre_write_meta;
>> +    if cache.file_mtime != pre_mtime || cache.file_len != pre_len {
>> +        cache.secrets.clear();
>> +    }
>> +
>>       // Apply the new mutation.
>>       match new_secret {
>>           Some(secret) => {
>> @@ -234,6 +323,20 @@ fn apply_api_mutation(tokenid: &Authid, new_secret: Option<&str>) {
>>               cache.secrets.remove(tokenid);
>>           }
>>       }
>> +
>> +    // Update our view of the file metadata to the post-write state (best-effort).
>> +    // (If this fails, drop local cache so callers fall back to slow path until refreshed.)
>> +    match shadow_mtime_len() {
>> +        Ok((mtime, len)) => {
>> +            cache.file_mtime = mtime;
>> +            cache.file_len = len;
>> +            cache.last_checked = Some(now);
>> +        }
>> +        Err(_) => {
>> +            // If we cannot validate state, do not trust cache.
>> +            invalidate_cache_state(&mut cache);
>> +        }
>> +    }
>>   }
>>   
>>   /// Get the current shared generation.
>> @@ -253,4 +356,15 @@ fn bump_token_shadow_shared_gen() -> Option<usize> {
>>   /// Invalidates the cache state and only keeps the shared generation.
>>   fn invalidate_cache_state(cache: &mut ApiTokenSecretCache) {
>>       cache.secrets.clear();
>> +    cache.file_mtime = None;
>> +    cache.file_len = None;
>> +    cache.last_checked = None;
>> +}
>> +
>> +fn shadow_mtime_len() -> Result<(Option<SystemTime>, Option<u64>), Error> {
>> +    match fs::metadata(CONF_FILE) {
>> +        Ok(meta) => Ok((meta.modified().ok(), Some(meta.len()))),
>> +        Err(e) if e.kind() == ErrorKind::NotFound => Ok((None, None)),
>> +        Err(e) => Err(e.into()),
>> +    }
>>   }
>> -- 
>> 2.47.3
>>
>>
>>
>> _______________________________________________
>> pbs-devel mailing list
>> pbs-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>>
>>
>>
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel