[pbs-devel] [PATCH proxmox v2 3/3] proxmox-access-control: add TTL window to token secret cache

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

Verify_secret() currently calls refresh_cache_if_file_changed() on every
request, which performs a metadata() call on token.shadow each time.
Under load this adds unnecessary overhead, considering also the file
should rarely change.

This patch introduces a TTL boundary, controlled by
TOKEN_SECRET_CACHE_TTL_SECS. File metadata is only re-loaded once the
TTL has expired.

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 TOKEN_SECRET_CACHE_TTL_SECS and last_checked.
- Implement double-checked TTL: check with try_read first; only attempt
  refresh with try_write if expired/unknown.
- Fix TTL bookkeeping: update last_checked on the “file unchanged” path
  and after API mutations.

 proxmox-access-control/src/token_shadow.rs | 42 +++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/proxmox-access-control/src/token_shadow.rs b/proxmox-access-control/src/token_shadow.rs
index efadce94..4ca56de9 100644
--- a/proxmox-access-control/src/token_shadow.rs
+++ b/proxmox-access-control/src/token_shadow.rs
@@ -11,6 +11,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::impl_feature::{token_shadow, token_shadow_lock};
 
@@ -24,12 +25,15 @@ static TOKEN_SECRET_CACHE: LazyLock<RwLock<ApiTokenSecretCache>> = LazyLock::new
         secrets: HashMap::new(),
         file_mtime: None,
         file_len: None,
+        last_checked: 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);
+/// Max age in seconds of the token secret cache before checking for file changes.
+const TOKEN_SECRET_CACHE_TTL_SECS: i64 = 60;
 
 // Get exclusive lock
 fn lock_config() -> Result<ApiLockGuard, Error> {
@@ -56,22 +60,54 @@ fn write_file(data: HashMap<Authid, String>) -> Result<(), Error> {
 /// 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();
+
+    // Check TTL (best-effort)
+    let Some(cache) = TOKEN_SECRET_CACHE.try_read() else {
+        return false; // cannot validate external changes -> don't trust cache
+    };
+
+    let ttl_ok = cache
+        .last_checked
+        .is_some_and(|last| now.saturating_sub(last) < TOKEN_SECRET_CACHE_TTL_SECS);
+
+    drop(cache);
+
+    if ttl_ok {
+        return true;
+    }
+
+    // TTL expired/unknown at this point -> do best-effort refresh.
     let Some(mut cache) = TOKEN_SECRET_CACHE.try_write() else {
         return false; // cannot validate external changes -> don't trust cache
     };
 
+    // Check TTL after acquiring write lock.
+    if let Some(last) = cache.last_checked {
+        if now.saturating_sub(last) < TOKEN_SECRET_CACHE_TTL_SECS {
+            return true;
+        }
+    }
+
+    let had_prior_state = cache.last_checked.is_some();
+
     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 {
+        cache.last_checked = Some(now);
         return true;
     }
 
     cache.secrets.clear();
     cache.file_mtime = new_mtime;
     cache.file_len = new_len;
-    FILE_GENERATION.fetch_add(1, Ordering::AcqRel);
+    cache.last_checked = Some(now);
+
+    if had_prior_state {
+        FILE_GENERATION.fetch_add(1, Ordering::AcqRel);
+    }
 
     true
 }
@@ -170,6 +206,8 @@ struct ApiTokenSecretCache {
     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 and the file generation it was cached at.
@@ -262,10 +300,12 @@ fn apply_api_mutation(
         Ok((mtime, len)) => {
             cache.file_mtime = mtime;
             cache.file_len = len;
+            cache.last_checked = Some(epoch_i64());
         }
         Err(_) => {
             cache.file_mtime = None;
             cache.file_len = None;
+            cache.last_checked = None; // to force refresh next time
         }
     }
 }
-- 
2.47.3



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