[PATCH proxmox v8 3/6] token shadow: invalidate token-secret cache on token.shadow changes

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

This patch adds manual/direct file change detection by tracking the
mtime and length of token.shadow and clears the in-memory token secret
cache whenever these values change.

Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>
---
Changes from v7 to v8:
* Merge refresh_cache_if_file_changed() and cache_try_secret_matches()
  into a single cached_secret_valid() function
* Move secret comparison logic into secret_matches() method on
  ApiTokenSecretCache
* Rename shadow field -> file_info

Changes from v6 to v7:
* Rebased

Changes from v5 to v6:
* Rebased

Changes from v4 to v5:
* Rebased

Changes from v3 to v4:
* make use of .replace() in refresh_cache_if_file_changed to get
previous state
* Group file stats with ShadowFileInfo
* Return false in refresh_cache_if_file_changed to avoid unnecessary cache
queries
* Adjusted commit message

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.

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.

 proxmox-access-control/src/token_shadow.rs | 167 +++++++++++++++++----
 1 file changed, 136 insertions(+), 31 deletions(-)

diff --git a/proxmox-access-control/src/token_shadow.rs b/proxmox-access-control/src/token_shadow.rs
index d0bf43d7..810ff0c3 100644
--- a/proxmox-access-control/src/token_shadow.rs
+++ b/proxmox-access-control/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_json::{from_value, Value};
 
 use proxmox_auth_api::types::Authid;
 use proxmox_product_config::{open_api_lockfile, replace_config, ApiLockGuard};
+use proxmox_time::epoch_i64;
 
 use crate::init::access_backend;
 use crate::init::impl_feature::{token_shadow, token_shadow_lock};
@@ -20,6 +24,7 @@ static TOKEN_SECRET_CACHE: LazyLock<RwLock<ApiTokenSecretCache>> = LazyLock::new
     RwLock::new(ApiTokenSecretCache {
         secrets: HashMap::new(),
         cached_gen: 0,
+        file_info: None,
     })
 });
 
@@ -45,6 +50,62 @@ fn write_file(data: HashMap<Authid, String>) -> Result<(), Error> {
     replace_config(token_shadow(), &json)
 }
 
+/// Tries to match the given token secret against the cached secret.
+///
+/// Verifies the generation/version before doing the constant-time
+/// comparison to reduce TOCTOU risk. During token rotation or deletion
+/// tokens for in-flight requests may still validate against the previous
+/// generation.
+///
+/// Returns true if secret is cached and cache is still valid
+fn cached_secret_valid(tokenid: &Authid, secret: &str) -> 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(current_gen) = token_shadow_generation() else {
+        return false;
+    };
+
+    // If another process bumped the generation, we don't know what changed -> clear cache
+    if cache.cached_gen != current_gen {
+        cache.reset_and_set_gen(current_gen);
+    }
+
+    // Stat the file to detect manual edits.
+    let Ok((new_mtime, new_len)) = shadow_mtime_len() else {
+        return false;
+    };
+
+    // If the file didn't change, only update last_checked
+    if let Some(shadow) = cache.file_info.as_mut() {
+        if shadow.mtime == new_mtime && shadow.len == new_len {
+            shadow.last_checked = now;
+            return cache.secret_matches(tokenid, secret);
+        }
+    }
+
+    cache.secrets.clear();
+
+    let prev = cache.file_info.replace(ShadowFileInfo {
+        mtime: new_mtime,
+        len: new_len,
+        last_checked: now,
+    });
+
+    if prev.is_some() {
+        // Best-effort propagation to other processes if a change was detected
+        if let Some(new_gen) = bump_token_shadow_generation() {
+            cache.cached_gen = new_gen;
+        }
+    }
+
+    false
+}
+
 /// 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() {
@@ -52,7 +113,7 @@ pub fn verify_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> {
     }
 
     // Fast path
-    if cache_try_secret_matches(tokenid, secret) {
+    if cached_secret_valid(tokenid, secret) {
         return Ok(());
     }
 
@@ -84,12 +145,15 @@ pub 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(guard, tokenid, Some(secret));
+    apply_api_mutation(guard, tokenid, Some(secret), pre_meta);
 
     Ok(())
 }
@@ -102,11 +166,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(guard, tokenid, None);
+    apply_api_mutation(guard, tokenid, None, pre_meta);
 
     Ok(())
 }
@@ -133,6 +200,8 @@ struct ApiTokenSecretCache {
     secrets: HashMap<Authid, CachedSecret>,
     /// token.shadow generation of cached secrets.
     cached_gen: usize,
+    /// Shadow file info to detect changes
+    file_info: Option<ShadowFileInfo>,
 }
 
 impl ApiTokenSecretCache {
@@ -140,6 +209,7 @@ impl ApiTokenSecretCache {
     fn reset_and_set_gen(&mut self, new_gen: usize) {
         self.secrets.clear();
         self.cached_gen = new_gen;
+        self.file_info = None;
     }
 
     /// Caches a secret and sets/updates the cache generation.
@@ -153,6 +223,28 @@ impl ApiTokenSecretCache {
         self.secrets.remove(tokenid);
         self.cached_gen = new_gen;
     }
+
+    /// Returns true if there is a matching cached entry
+    fn secret_matches(&self, tokenid: &Authid, secret: &str) -> bool {
+        let Some(entry) = self.secrets.get(tokenid) else {
+            return false;
+        };
+        let cached_secret_bytes = entry.secret.as_bytes();
+        let secret_bytes = secret.as_bytes();
+
+        cached_secret_bytes.len() == secret_bytes.len()
+            && openssl::memcmp::eq(cached_secret_bytes, secret_bytes)
+    }
+}
+
+/// Shadow file info
+struct ShadowFileInfo {
+    // shadow file mtime to detect changes
+    mtime: Option<SystemTime>,
+    // shadow file length to detect changes
+    len: Option<u64>,
+    // last time the file metadata was checked
+    last_checked: i64,
 }
 
 fn cache_try_insert_secret(tokenid: Authid, secret: String, gen_before: usize) {
@@ -175,35 +267,14 @@ fn cache_try_insert_secret(tokenid: Authid, secret: String, gen_before: usize) {
     }
 }
 
-/// Tries to match the given token secret against the cached secret.
-///
-/// Verifies the generation/version before doing the constant-time
-/// comparison to reduce TOCTOU risk. During token rotation or deletion
-/// tokens for in-flight requests may still validate against the previous
-/// generation.
-fn cache_try_secret_matches(tokenid: &Authid, secret: &str) -> bool {
-    let Some(cache) = TOKEN_SECRET_CACHE.try_read() else {
-        return false;
-    };
-    let Some(entry) = cache.secrets.get(tokenid) else {
-        return false;
-    };
-    let Some(current_gen) = token_shadow_generation() else {
-        return false;
-    };
-
-    if current_gen == cache.cached_gen {
-        let cached_secret_bytes = entry.secret.as_bytes();
-        let secret_bytes = secret.as_bytes();
-
-        return cached_secret_bytes.len() == secret_bytes.len()
-            && openssl::memcmp::eq(cached_secret_bytes, secret_bytes);
-    }
-
-    false
-}
+fn apply_api_mutation(
+    _guard: ApiLockGuard,
+    tokenid: &Authid,
+    secret: Option<&str>,
+    pre_write_meta: (Option<SystemTime>, Option<u64>),
+) {
+    let now = epoch_i64();
 
-fn apply_api_mutation(_guard: ApiLockGuard, tokenid: &Authid, secret: Option<&str>) {
     // Signal cache invalidation to other processes (best-effort).
     let bumped_gen = bump_token_shadow_generation();
     let mut cache = TOKEN_SECRET_CACHE.write();
@@ -221,6 +292,16 @@ fn apply_api_mutation(_guard: ApiLockGuard, tokenid: &Authid, secret: Option<&st
         return;
     }
 
+    // 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.
+    if cache
+        .file_info
+        .as_ref()
+        .is_some_and(|s| (s.mtime, s.len) != pre_write_meta)
+    {
+        cache.secrets.clear();
+    }
+
     // Apply the new mutation.
     match secret {
         Some(secret) => {
@@ -231,6 +312,22 @@ fn apply_api_mutation(_guard: ApiLockGuard, tokenid: &Authid, secret: Option<&st
         }
         None => cache.evict_and_set_gen(tokenid, current_gen),
     }
+
+    // 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_info = Some(ShadowFileInfo {
+                mtime,
+                len,
+                last_checked: now,
+            });
+        }
+        Err(_) => {
+            // If we cannot validate state, do not trust cache.
+            cache.reset_and_set_gen(current_gen);
+        }
+    }
 }
 
 /// Get the current generation.
@@ -245,3 +342,11 @@ fn bump_token_shadow_generation() -> Option<usize> {
         .ok()
         .map(|prev| prev + 1)
 }
+
+fn shadow_mtime_len() -> Result<(Option<SystemTime>, Option<u64>), Error> {
+    match fs::metadata(token_shadow()) {
+        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