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

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

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