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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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()

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?

> +
> +    // 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