[pbs-devel] [PATCH proxmox-backup v2 3/3] pbs-config: 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
usually 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. Documents TTL effects.

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.
- Add documentation warning about TTL-delayed effect of manual
  token.shadow edits.

 docs/user-management.rst       |  4 ++++
 pbs-config/src/token_shadow.rs | 42 +++++++++++++++++++++++++++++++++-
 2 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/docs/user-management.rst b/docs/user-management.rst
index 41b43d60..32a9ec29 100644
--- a/docs/user-management.rst
+++ b/docs/user-management.rst
@@ -156,6 +156,10 @@ metadata:
 Similarly, the ``user delete-token`` subcommand can be used to delete a token
 again.
 
+.. WARNING:: If you manually remove a generated API token from the token secrets
+   file (token.shadow), it can take up to one minute before the token is
+   rejected. This is due to caching.
+
 Newly generated API tokens don't have any permissions. Please read the next
 section to learn how to set access permissions.
 
diff --git a/pbs-config/src/token_shadow.rs b/pbs-config/src/token_shadow.rs
index 71553aae..79940fd5 100644
--- a/pbs-config/src/token_shadow.rs
+++ b/pbs-config/src/token_shadow.rs
@@ -11,6 +11,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;
@@ -29,12 +30,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;
 
 #[derive(Serialize, Deserialize)]
 #[serde(rename_all = "kebab-case")]
@@ -74,22 +78,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
 }
@@ -188,6 +224,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.
@@ -280,10 +318,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