[pbs-devel] [PATCH proxmox v2 2/3] proxmox-access-control: 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 partly 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.

 proxmox-access-control/src/token_shadow.rs | 128 +++++++++++++++++++--
 1 file changed, 116 insertions(+), 12 deletions(-)

diff --git a/proxmox-access-control/src/token_shadow.rs b/proxmox-access-control/src/token_shadow.rs
index c0285b62..efadce94 100644
--- a/proxmox-access-control/src/token_shadow.rs
+++ b/proxmox-access-control/src/token_shadow.rs
@@ -1,6 +1,9 @@
 use std::collections::HashMap;
+use std::fs;
+use std::io::ErrorKind;
 use std::sync::atomic::{AtomicU64, Ordering};
 use std::sync::LazyLock;
+use std::time::SystemTime;
 
 use anyhow::{bail, format_err, Error};
 use parking_lot::RwLock;
@@ -19,10 +22,14 @@ use crate::init::impl_feature::{token_shadow, token_shadow_lock};
 static TOKEN_SECRET_CACHE: LazyLock<RwLock<ApiTokenSecretCache>> = LazyLock::new(|| {
     RwLock::new(ApiTokenSecretCache {
         secrets: HashMap::new(),
+        file_mtime: None,
+        file_len: None,
     })
 });
 /// API mutation generation (set/delete)
 static API_MUTATION_GENERATION: AtomicU64 = AtomicU64::new(0);
+/// External/manual edits generation for the token.shadow file
+static FILE_GENERATION: AtomicU64 = AtomicU64::new(0);
 
 // Get exclusive lock
 fn lock_config() -> Result<ApiLockGuard, Error> {
@@ -46,6 +53,29 @@ fn write_file(data: HashMap<Authid, String>) -> Result<(), Error> {
     replace_config(token_shadow(), &json)
 }
 
+/// 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 Some(mut cache) = TOKEN_SECRET_CACHE.try_write() else {
+        return false; // cannot validate external changes -> don't trust cache
+    };
+
+    let Ok((new_mtime, new_len)) = shadow_mtime_len() else {
+        return false; // cannot validate external changes -> don't trust cache
+    };
+
+    if cache.file_mtime == new_mtime && cache.file_len == new_len {
+        return true;
+    }
+
+    cache.secrets.clear();
+    cache.file_mtime = new_mtime;
+    cache.file_len = new_len;
+    FILE_GENERATION.fetch_add(1, Ordering::AcqRel);
+
+    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() {
@@ -53,12 +83,13 @@ 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(());
     }
 
     // Slow path snapshot (before expensive work)
     let api_gen_before = API_MUTATION_GENERATION.load(Ordering::Acquire);
+    let file_gen_before = FILE_GENERATION.load(Ordering::Acquire);
 
     let data = read_file()?;
     match data.get(tokenid) {
@@ -66,7 +97,12 @@ pub fn verify_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> {
             proxmox_sys::crypt::verify_crypt_pw(secret, hashed_secret)?;
 
             // Try to cache only if nothing changed while we verified
-            cache_try_insert_secret(tokenid.clone(), secret.to_owned(), api_gen_before);
+            cache_try_insert_secret(
+                tokenid.clone(),
+                secret.to_owned(),
+                api_gen_before,
+                file_gen_before,
+            );
 
             Ok(())
         }
@@ -82,12 +118,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(tokenid, Some(secret));
+    apply_api_mutation(tokenid, Some(secret), pre_meta);
 
     Ok(())
 }
@@ -100,11 +139,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(())
 }
@@ -124,20 +166,40 @@ struct ApiTokenSecretCache {
     /// `generate_and_set_secret`. Used to avoid repeated
     /// password-hash computation on subsequent authentications.
     secrets: HashMap<Authid, CachedSecret>,
+    // shadow file mtime to detect changes
+    file_mtime: Option<SystemTime>,
+    // shadow file length to detect changes
+    file_len: Option<u64>,
 }
 
-/// Cached secret.
+/// Cached secret and the file generation it was cached at.
 struct CachedSecret {
     secret: String,
+    file_gen: u64,
 }
 
-fn cache_try_insert_secret(tokenid: Authid, secret: String, api_gen_snapshot: u64) {
+fn cache_try_insert_secret(
+    tokenid: Authid,
+    secret: String,
+    api_gen_snapshot: u64,
+    file_gen_snapshot: u64,
+) {
     let Some(mut cache) = TOKEN_SECRET_CACHE.try_write() else {
         return;
     };
 
-    if API_MUTATION_GENERATION.load(Ordering::Acquire) == api_gen_snapshot {
-        cache.secrets.insert(tokenid, CachedSecret { secret });
+    // Check generations to avoid zombie-inserts
+    let cur_file_gen = FILE_GENERATION.load(Ordering::Acquire);
+    let cur_api_gen = API_MUTATION_GENERATION.load(Ordering::Acquire);
+
+    if cur_file_gen == file_gen_snapshot && cur_api_gen == api_gen_snapshot {
+        cache.secrets.insert(
+            tokenid,
+            CachedSecret {
+                secret,
+                file_gen: cur_file_gen,
+            },
+        );
     }
 }
 
@@ -149,22 +211,44 @@ fn cache_try_secret_matches(tokenid: &Authid, secret: &str) -> bool {
         return false;
     };
 
-    openssl::memcmp::eq(entry.secret.as_bytes(), secret.as_bytes())
+    let gen1 = FILE_GENERATION.load(Ordering::Acquire);
+    if entry.file_gen != gen1 {
+        return false;
+    }
+
+    let eq = openssl::memcmp::eq(entry.secret.as_bytes(), secret.as_bytes());
+
+    let gen2 = FILE_GENERATION.load(Ordering::Acquire);
+    eq && gen1 == gen2
 }
 
-fn apply_api_mutation(tokenid: &Authid, new_secret: Option<&str>) {
-    // Prevent in-flight verify_secret() from caching results across a mutation.
+fn apply_api_mutation(
+    tokenid: &Authid,
+    new_secret: Option<&str>,
+    pre_write_meta: (Option<SystemTime>, Option<u64>),
+) {
     API_MUTATION_GENERATION.fetch_add(1, Ordering::AcqRel);
 
-    // Mutations must be reflected immediately once set/delete returns.
     let mut cache = TOKEN_SECRET_CACHE.write();
 
+    // If the cache meta doesn't match the file state before the on-disk write,
+    // external/manual edits happened -> drop everything and bump FILE_GENERATION.
+    let (pre_mtime, pre_len) = pre_write_meta;
+    if cache.file_mtime != pre_mtime || cache.file_len != pre_len {
+        cache.secrets.clear();
+        FILE_GENERATION.fetch_add(1, Ordering::AcqRel);
+    }
+
+    let file_gen = FILE_GENERATION.load(Ordering::Acquire);
+
+    // Apply the API mutation to the cache.
     match new_secret {
         Some(secret) => {
             cache.secrets.insert(
                 tokenid.clone(),
                 CachedSecret {
                     secret: secret.to_owned(),
+                    file_gen,
                 },
             );
         }
@@ -172,4 +256,24 @@ fn apply_api_mutation(tokenid: &Authid, new_secret: Option<&str>) {
             cache.secrets.remove(tokenid);
         }
     }
+
+    // Keep cache metadata aligned if possible.
+    match shadow_mtime_len() {
+        Ok((mtime, len)) => {
+            cache.file_mtime = mtime;
+            cache.file_len = len;
+        }
+        Err(_) => {
+            cache.file_mtime = None;
+            cache.file_len = None;
+        }
+    }
+}
+
+fn shadow_mtime_len() -> Result<(Option<SystemTime>, Option<u64>), Error> {
+    match fs::metadata(token_shadow().as_path()) {
+        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