[pbs-devel] [PATCH proxmox{-backup, } 0/6] Reduce token.shadow verification overhead

[pbs-devel] [PATCH proxmox{-backup, } 0/6] Reduce token.shadow verification overhead

[Samuel Rufinatscha]

Hi,

this series improves the performance of token-based API authentication
in PBS (pbs-config) and in PDM (underlying proxmox-access-control
crate), addressing the API token verification hotspot reported in our
bugtracker #6049 [1].

When profiling PBS /status endpoint with cargo flamegraph [2],
token-based authentication showed up as a dominant hotspot via
proxmox_sys::crypt::verify_crypt_pw. Applying this series removes that
path from the hot section of the flamegraph. The same performance issue
was measured [3] for PDM. PDM uses the underlying shared
proxmox-access-control library for token handling, which is a
factored out version of the token.shadow handling code from PBS.

While this series fixes the immediate performance issue both in PBS
(pbs-config) and in the shared proxmox-access-control crate used by
PDM, PBS should eventually, ideally be refactored, in a separate
effort, to use proxmox-access-control for token handling instead of its
local implementation.

Problem

For token-based API requests, both PBS’s pbs-config token.shadow
handling and PDM proxmox-access-control’s token.shadow handling
currently:

1. read the token.shadow file on each request
2. deserialize it into a HashMap<Authid, String>
3. run password hash verification via
   proxmox_sys::crypt::verify_crypt_pw for the provided token secret

Under load, this results in significant CPU usage spent in repeated
password hash computations for the same token+secret pairs. The
attached flamegraphs for PBS [2] and PDM [3] show
proxmox_sys::crypt::verify_crypt_pw dominating the hot path.

Approach

The goal is to reduce the cost of token-based authentication preserving
the existing token handling semantics (including detecting manual edits
to token.shadow) and be consistent between PBS (pbs-config) and
PDM (proxmox-access-control). For both sites, the series proposes
following approach:

1. Introduce an in-memory cache for verified token secrets
2. Invalidate the cache when token.shadow changes (detect manual edits)
3. Control metadata checks with a TTL window

Testing

*PBS (pbs-config)*

To verify the effect in PBS, I:
1. Set up test environment based on latest PBS ISO, installed Rust
   toolchain, cloned proxmox-backup repository to use with cargo
   flamegraph. Reproduced bug #6049 [1] by profiling the /status
   endpoint with token-based authentication using cargo flamegraph [2].
   The flamegraph showed proxmox_sys::crypt::verify_crypt_pw is the
   hotspot.
2. Built PBS with pbs-config patches and re-ran the same workload and
   profiling setup.
3. Confirmed that the proxmox_sys::crypt::verify_crypt_pw path no
   longer appears in the hot section of the flamegraph. CPU usage is
   now dominated by TLS overhead.
4. Functionally verified that:
   * token-based API authentication still works for valid tokens
   * invalid secrets are rejected as before
   * generating a new token secret via dashboard works and
   authenticates correctly

*PDM (proxmox-access-control)*

To verify the effect in PDM, I followed a similar testing approach.
Instead of /status, I profiled the /version endpoint with cargo
flamegraph [3] and verified that the token hashing path disappears
from the hot section after applying the proxmox-access-control patches.

Functionally I verified that:
   * token-based API authentication still works for valid tokens
   * invalid secrets are rejected as before
   * generating a new token secret via dashboard works and
   authenticates correctly

Patch summary

pbs-config:

0001 – pbs-config: cache verified API token secrets
Adds an in-memory cache keyed by Authid that stores plain text token
secrets after a successful verification or generation and uses
openssl’s memcmp constant-time for comparison.

0002 – pbs-config: invalidate token-secret cache on token.shadow changes
Tracks token.shadow mtime and length and clears the in-memory cache
when the file changes.

0003 – pbs-config: add TTL window to token-secret cache
Introduces a TTL (TOKEN_SECRET_CACHE_TTL_SECS, default 60) for metadata checks so
that fs::metadata is only called periodically.

proxmox-access-control:

0004 – access-control: cache verified API token secrets
Mirrors PBS patch 0001.

0005 – access-control: invalidate token-secret cache on token.shadow changes
Mirrors PBS patch 0002.

0006 – access-control: add TTL window to token-secret cache
Mirrors PBS patch 0003.

Thanks for considering this patch series, I look forward to your
feedback.

Best,
Samuel Rufinatscha

[1] https://bugzilla.proxmox.com/show_bug.cgi?id=6049
[2] Flamegraph illustrating the`proxmox_sys::crypt::verify_crypt_pw
hotspot before this series (attached to [1])

proxmox-backup:

Samuel Rufinatscha (3):
  pbs-config: cache verified API token secrets
  pbs-config: invalidate token-secret cache on token.shadow changes
  pbs-config: add TTL window to token secret cache

 pbs-config/src/token_shadow.rs | 109 ++++++++++++++++++++++++++++++++-
 1 file changed, 108 insertions(+), 1 deletion(-)


proxmox:

Samuel Rufinatscha (3):
  proxmox-access-control: cache verified API token secrets
  proxmox-access-control: invalidate token-secret cache on token.shadow
    changes
  proxmox-access-control: add TTL window to token secret cache

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


Summary over all repositories:
  2 files changed, 215 insertions(+), 2 deletions(-)

-- 
Generated by git-murpp 0.8.1


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

[pbs-devel] [PATCH proxmox-backup 1/3] pbs-config: cache verified API token secrets

[Samuel Rufinatscha]

Currently, every token-based API request reads the token.shadow file and
runs the expensive password hash verification for the given token
secret. This shows up as a hotspot in /status profiling (see
bug #6049 [1]).

This patch introduces an in-memory cache of successfully verified token
secrets. Subsequent requests for the same token+secret combination only
perform a comparison using openssl::memcmp::eq and avoid re-running the
password hash. The cache is updated when a token secret is set and
cleared when a token is deleted. Note, this does NOT include manual
config changes, which will be covered in a subsequent patch.

This patch partly fixes bug #6049 [1].

[1] https://bugzilla.proxmox.com/show_bug.cgi?id=7017

Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>
---
 pbs-config/src/token_shadow.rs | 58 +++++++++++++++++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/pbs-config/src/token_shadow.rs b/pbs-config/src/token_shadow.rs
index 640fabbf..47aa2fc2 100644
--- a/pbs-config/src/token_shadow.rs
+++ b/pbs-config/src/token_shadow.rs
@@ -1,6 +1,8 @@
 use std::collections::HashMap;
+use std::sync::RwLock;
 
 use anyhow::{bail, format_err, Error};
+use once_cell::sync::OnceCell;
 use serde::{Deserialize, Serialize};
 use serde_json::{from_value, Value};
 
@@ -13,6 +15,13 @@ use crate::{open_backup_lockfile, BackupLockGuard};
 const LOCK_FILE: &str = pbs_buildcfg::configdir!("/token.shadow.lock");
 const CONF_FILE: &str = pbs_buildcfg::configdir!("/token.shadow");
 
+/// Global in-memory cache for successfully verified API token secrets.
+/// The cache stores plain text secrets for token Authids that have already been
+/// verified against the hashed values in `token.shadow`. This allows for cheap
+/// subsequent authentications for the same token+secret combination, avoiding
+/// recomputing the password hash on every request.
+static TOKEN_SECRET_CACHE: OnceCell<RwLock<ApiTokenSecretCache>> = OnceCell::new();
+
 #[derive(Serialize, Deserialize)]
 #[serde(rename_all = "kebab-case")]
 /// ApiToken id / secret pair
@@ -54,9 +63,25 @@ pub fn verify_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> {
         bail!("not an API token ID");
     }
 
+    // Fast path
+    if let Some(cached) = token_secret_cache().read().unwrap().secrets.get(tokenid) {
+        // Compare cached secret with provided one using constant time comparison
+        if openssl::memcmp::eq(cached.as_bytes(), secret.as_bytes()) {
+            // Already verified before
+            return Ok(());
+        }
+        // Fall through to slow path if secret doesn't match cached one
+    }
+
+    // Slow path: read file + verify hash
     let data = read_file()?;
     match data.get(tokenid) {
-        Some(hashed_secret) => proxmox_sys::crypt::verify_crypt_pw(secret, hashed_secret),
+        Some(hashed_secret) => {
+            proxmox_sys::crypt::verify_crypt_pw(secret, hashed_secret)?;
+            // Cache the plain secret for future requests
+            cache_insert_secret(tokenid.clone(), secret.to_owned());
+            Ok(())
+        }
         None => bail!("invalid API token"),
     }
 }
@@ -82,6 +107,8 @@ fn set_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> {
     data.insert(tokenid.clone(), hashed_secret);
     write_file(data)?;
 
+    cache_insert_secret(tokenid.clone(), secret.to_owned());
+
     Ok(())
 }
 
@@ -97,5 +124,34 @@ pub fn delete_secret(tokenid: &Authid) -> Result<(), Error> {
     data.remove(tokenid);
     write_file(data)?;
 
+    cache_remove_secret(tokenid);
+
     Ok(())
 }
+
+struct ApiTokenSecretCache {
+    /// Keys are token Authids, values are the corresponding plain text secrets.
+    /// Entries are added after a successful on-disk verification in
+    /// `verify_secret` or when a new token secret is generated by
+    /// `generate_and_set_secret`. Used to avoid repeated
+    /// password-hash computation on subsequent authentications.
+    secrets: HashMap<Authid, String>,
+}
+
+fn token_secret_cache() -> &'static RwLock<ApiTokenSecretCache> {
+    TOKEN_SECRET_CACHE.get_or_init(|| {
+        RwLock::new(ApiTokenSecretCache {
+            secrets: HashMap::new(),
+        })
+    })
+}
+
+fn cache_insert_secret(tokenid: Authid, secret: String) {
+    let mut cache = token_secret_cache().write().unwrap();
+    cache.secrets.insert(tokenid, secret);
+}
+
+fn cache_remove_secret(tokenid: &Authid) {
+    let mut cache = token_secret_cache().write().unwrap();
+    cache.secrets.remove(tokenid);
+}
-- 
2.47.3



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

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

[Samuel Rufinatscha]

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 a partly-fix.

[1] https://bugzilla.proxmox.com/show_bug.cgi?id=7017

Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>
---
 pbs-config/src/token_shadow.rs | 35 ++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/pbs-config/src/token_shadow.rs b/pbs-config/src/token_shadow.rs
index 47aa2fc2..ed54cdfa 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::RwLock;
+use std::time::SystemTime;
 
 use anyhow::{bail, format_err, Error};
 use once_cell::sync::OnceCell;
@@ -57,12 +60,38 @@ fn write_file(data: HashMap<Authid, String>) -> Result<(), Error> {
     proxmox_sys::fs::replace_file(CONF_FILE, &json, options, true)
 }
 
+fn refresh_cache_if_file_changed() -> Result<(), Error> {
+    let mut cache = token_secret_cache().write().unwrap();
+
+    // Fetch the current token.shadow metadata
+    let (new_mtime, new_len) = match fs::metadata(CONF_FILE) {
+        Ok(meta) => (meta.modified().ok(), Some(meta.len())),
+        Err(e) if e.kind() == ErrorKind::NotFound => (None, None),
+        Err(e) => return Err(e.into()),
+    };
+
+    // Fast path: file did not change, keep the cache
+    if cache.file_mtime == new_mtime && cache.file_len == new_len {
+        return Ok(());
+    }
+
+    // File changed, drop all cached secrets
+    cache.secrets.clear();
+    cache.file_mtime = new_mtime;
+    cache.file_len = new_len;
+
+    Ok(())
+}
+
 /// 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() {
         bail!("not an API token ID");
     }
 
+    // Ensure cache is in sync with on-disk token.shadow file
+    refresh_cache_if_file_changed()?;
+
     // Fast path
     if let Some(cached) = token_secret_cache().read().unwrap().secrets.get(tokenid) {
         // Compare cached secret with provided one using constant time comparison
@@ -136,12 +165,18 @@ struct ApiTokenSecretCache {
     /// `generate_and_set_secret`. Used to avoid repeated
     /// password-hash computation on subsequent authentications.
     secrets: HashMap<Authid, String>,
+    // shadow file mtime to detect changes
+    file_mtime: Option<SystemTime>,
+    // shadow file length to detect changes
+    file_len: Option<u64>,
 }
 
 fn token_secret_cache() -> &'static RwLock<ApiTokenSecretCache> {
     TOKEN_SECRET_CACHE.get_or_init(|| {
         RwLock::new(ApiTokenSecretCache {
             secrets: HashMap::new(),
+            file_mtime: None,
+            file_len: None,
         })
     })
 }
-- 
2.47.3



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

[pbs-devel] [PATCH proxmox-backup 3/3] pbs-config: add TTL window to token secret cache

[Samuel Rufinatscha]

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.

This patch partly fixes bug #6049 [1].

[1] https://bugzilla.proxmox.com/show_bug.cgi?id=7017

Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>
---
 pbs-config/src/token_shadow.rs | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/pbs-config/src/token_shadow.rs b/pbs-config/src/token_shadow.rs
index ed54cdfa..23837c60 100644
--- a/pbs-config/src/token_shadow.rs
+++ b/pbs-config/src/token_shadow.rs
@@ -10,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 +25,8 @@ const CONF_FILE: &str = pbs_buildcfg::configdir!("/token.shadow");
 /// subsequent authentications for the same token+secret combination, avoiding
 /// recomputing the password hash on every request.
 static TOKEN_SECRET_CACHE: OnceCell<RwLock<ApiTokenSecretCache>> = OnceCell::new();
+/// 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")]
@@ -63,6 +66,15 @@ fn write_file(data: HashMap<Authid, String>) -> Result<(), Error> {
 fn refresh_cache_if_file_changed() -> Result<(), Error> {
     let mut cache = token_secret_cache().write().unwrap();
 
+    let now = epoch_i64();
+
+    // Fast path: Within TTL boundary
+    if let Some(last) = cache.last_checked {
+        if now - last < TOKEN_SECRET_CACHE_TTL_SECS {
+            return Ok(());
+        }
+    }
+
     // Fetch the current token.shadow metadata
     let (new_mtime, new_len) = match fs::metadata(CONF_FILE) {
         Ok(meta) => (meta.modified().ok(), Some(meta.len())),
@@ -79,6 +91,7 @@ fn refresh_cache_if_file_changed() -> Result<(), Error> {
     cache.secrets.clear();
     cache.file_mtime = new_mtime;
     cache.file_len = new_len;
+    cache.last_checked = Some(now);
 
     Ok(())
 }
@@ -169,6 +182,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>,
 }
 
 fn token_secret_cache() -> &'static RwLock<ApiTokenSecretCache> {
@@ -177,6 +192,7 @@ fn token_secret_cache() -> &'static RwLock<ApiTokenSecretCache> {
             secrets: HashMap::new(),
             file_mtime: None,
             file_len: None,
+            last_checked: None,
         })
     })
 }
-- 
2.47.3



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

[pbs-devel] [PATCH proxmox 1/3] proxmox-access-control: cache verified API token secrets

[Samuel Rufinatscha]

Currently, every token-based API request reads the token.shadow file and
runs the expensive password hash verification for the given token
secret. This issue was first observed as part of profiling the PBS
/status endpoint (see bug #6049 [1]) and is required for the factored
out proxmox_access_control token_shadow implementation too.

This patch introduces an in-memory cache of successfully verified token
secrets. Subsequent requests for the same token+secret combination only
perform a comparison using openssl::memcmp::eq and avoid re-running the
password hash. The cache is updated when a token secret is set and
cleared when a token is deleted. Note, this does NOT include manual
config changes, which will be covered in a subsequent patch.

This patch is a partly-fix.

[1] https://bugzilla.proxmox.com/show_bug.cgi?id=7017

Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>
---
 proxmox-access-control/src/token_shadow.rs | 57 +++++++++++++++++++++-
 1 file changed, 56 insertions(+), 1 deletion(-)

diff --git a/proxmox-access-control/src/token_shadow.rs b/proxmox-access-control/src/token_shadow.rs
index c586d834..2dcd117d 100644
--- a/proxmox-access-control/src/token_shadow.rs
+++ b/proxmox-access-control/src/token_shadow.rs
@@ -1,4 +1,5 @@
 use std::collections::HashMap;
+use std::sync::{OnceLock, RwLock};
 
 use anyhow::{bail, format_err, Error};
 use serde_json::{from_value, Value};
@@ -8,6 +9,13 @@ use proxmox_product_config::{open_api_lockfile, replace_config, ApiLockGuard};
 
 use crate::init::impl_feature::{token_shadow, token_shadow_lock};
 
+/// Global in-memory cache for successfully verified API token secrets.
+/// The cache stores plain text secrets for token Authids that have already been
+/// verified against the hashed values in `token.shadow`. This allows for cheap
+/// subsequent authentications for the same token+secret combination, avoiding
+/// recomputing the password hash on every request.
+static TOKEN_SECRET_CACHE: OnceLock<RwLock<ApiTokenSecretCache>> = OnceLock::new();
+
 // Get exclusive lock
 fn lock_config() -> Result<ApiLockGuard, Error> {
     open_api_lockfile(token_shadow_lock(), None, true)
@@ -36,9 +44,25 @@ pub fn verify_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> {
         bail!("not an API token ID");
     }
 
+    // Fast path
+    if let Some(cached) = token_secret_cache().read().unwrap().secrets.get(tokenid) {
+        // Compare cached secret with provided one using constant time comparison
+        if openssl::memcmp::eq(cached.as_bytes(), secret.as_bytes()) {
+            // Already verified before
+            return Ok(());
+        }
+        // Fall through to slow path if secret doesn't match cached one
+    }
+
+    // Slow path: read file + verify hash
     let data = read_file()?;
     match data.get(tokenid) {
-        Some(hashed_secret) => proxmox_sys::crypt::verify_crypt_pw(secret, hashed_secret),
+        Some(hashed_secret) => {
+            proxmox_sys::crypt::verify_crypt_pw(secret, hashed_secret)?;
+            // Cache the plain secret for future requests
+            cache_insert_secret(tokenid.clone(), secret.to_owned());
+            Ok(())
+        }
         None => bail!("invalid API token"),
     }
 }
@@ -56,6 +80,8 @@ pub fn set_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> {
     data.insert(tokenid.clone(), hashed_secret);
     write_file(data)?;
 
+    cache_insert_secret(tokenid.clone(), secret.to_owned());
+
     Ok(())
 }
 
@@ -71,6 +97,8 @@ pub fn delete_secret(tokenid: &Authid) -> Result<(), Error> {
     data.remove(tokenid);
     write_file(data)?;
 
+    cache_remove_secret(tokenid);
+
     Ok(())
 }
 
@@ -81,3 +109,30 @@ pub fn generate_and_set_secret(tokenid: &Authid) -> Result<String, Error> {
     set_secret(tokenid, &secret)?;
     Ok(secret)
 }
+
+struct ApiTokenSecretCache {
+    /// Keys are token Authids, values are the corresponding plain text secrets.
+    /// Entries are added after a successful on-disk verification in
+    /// `verify_secret` or when a new token secret is generated by
+    /// `generate_and_set_secret`. Used to avoid repeated
+    /// password-hash computation on subsequent authentications.
+    secrets: HashMap<Authid, String>,
+}
+
+fn token_secret_cache() -> &'static RwLock<ApiTokenSecretCache> {
+    TOKEN_SECRET_CACHE.get_or_init(|| {
+        RwLock::new(ApiTokenSecretCache {
+            secrets: HashMap::new(),
+        })
+    })
+}
+
+fn cache_insert_secret(tokenid: Authid, secret: String) {
+    let mut cache = token_secret_cache().write().unwrap();
+    cache.secrets.insert(tokenid, secret);
+}
+
+fn cache_remove_secret(tokenid: &Authid) {
+    let mut cache = token_secret_cache().write().unwrap();
+    cache.secrets.remove(tokenid);
+}
-- 
2.47.3



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

[pbs-devel] [PATCH proxmox 2/3] proxmox-access-control: invalidate token-secret cache on token.shadow changes

[Samuel Rufinatscha]

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 a partly-fix.

Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>
---
 proxmox-access-control/src/token_shadow.rs | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/proxmox-access-control/src/token_shadow.rs b/proxmox-access-control/src/token_shadow.rs
index 2dcd117d..d08fb06a 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::{OnceLock, RwLock};
+use std::time::SystemTime;
 
 use anyhow::{bail, format_err, Error};
 use serde_json::{from_value, Value};
@@ -38,12 +41,38 @@ fn write_file(data: HashMap<Authid, String>) -> Result<(), Error> {
     replace_config(token_shadow(), &json)
 }
 
+fn refresh_cache_if_file_changed() -> Result<(), Error> {
+    let mut cache = token_secret_cache().write().unwrap();
+
+    // Fetch the current token.shadow metadata
+    let (new_mtime, new_len) = match fs::metadata(token_shadow().as_path()) {
+        Ok(meta) => (meta.modified().ok(), Some(meta.len())),
+        Err(e) if e.kind() == ErrorKind::NotFound => (None, None),
+        Err(e) => return Err(e.into()),
+    };
+
+    // Fast path: file did not change, keep the cache
+    if cache.file_mtime == new_mtime && cache.file_len == new_len {
+        return Ok(());
+    }
+
+    // File changed, drop all cached secrets
+    cache.secrets.clear();
+    cache.file_mtime = new_mtime;
+    cache.file_len = new_len;
+
+    Ok(())
+}
+
 /// 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() {
         bail!("not an API token ID");
     }
 
+    // Ensure cache is in sync with on-disk token.shadow file
+    refresh_cache_if_file_changed()?;
+
     // Fast path
     if let Some(cached) = token_secret_cache().read().unwrap().secrets.get(tokenid) {
         // Compare cached secret with provided one using constant time comparison
@@ -117,12 +146,18 @@ struct ApiTokenSecretCache {
     /// `generate_and_set_secret`. Used to avoid repeated
     /// password-hash computation on subsequent authentications.
     secrets: HashMap<Authid, String>,
+    // shadow file mtime to detect changes
+    file_mtime: Option<SystemTime>,
+    // shadow file length to detect changes
+    file_len: Option<u64>,
 }
 
 fn token_secret_cache() -> &'static RwLock<ApiTokenSecretCache> {
     TOKEN_SECRET_CACHE.get_or_init(|| {
         RwLock::new(ApiTokenSecretCache {
             secrets: HashMap::new(),
+            file_mtime: None,
+            file_len: None,
         })
     })
 }
-- 
2.47.3



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

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

[Samuel Rufinatscha]

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.

Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>
---
 proxmox-access-control/src/token_shadow.rs | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/proxmox-access-control/src/token_shadow.rs b/proxmox-access-control/src/token_shadow.rs
index d08fb06a..885e629d 100644
--- a/proxmox-access-control/src/token_shadow.rs
+++ b/proxmox-access-control/src/token_shadow.rs
@@ -9,6 +9,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};
 
@@ -18,6 +19,8 @@ use crate::init::impl_feature::{token_shadow, token_shadow_lock};
 /// subsequent authentications for the same token+secret combination, avoiding
 /// recomputing the password hash on every request.
 static TOKEN_SECRET_CACHE: OnceLock<RwLock<ApiTokenSecretCache>> = OnceLock::new();
+/// 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> {
@@ -44,6 +47,15 @@ fn write_file(data: HashMap<Authid, String>) -> Result<(), Error> {
 fn refresh_cache_if_file_changed() -> Result<(), Error> {
     let mut cache = token_secret_cache().write().unwrap();
 
+    let now = epoch_i64();
+
+    // Fast path: Within TTL boundary
+    if let Some(last) = cache.last_checked {
+        if now - last < TOKEN_SECRET_CACHE_TTL_SECS {
+            return Ok(());
+        }
+    }
+
     // Fetch the current token.shadow metadata
     let (new_mtime, new_len) = match fs::metadata(token_shadow().as_path()) {
         Ok(meta) => (meta.modified().ok(), Some(meta.len())),
@@ -60,6 +72,7 @@ fn refresh_cache_if_file_changed() -> Result<(), Error> {
     cache.secrets.clear();
     cache.file_mtime = new_mtime;
     cache.file_len = new_len;
+    cache.last_checked = Some(now);
 
     Ok(())
 }
@@ -150,6 +163,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>,
 }
 
 fn token_secret_cache() -> &'static RwLock<ApiTokenSecretCache> {
@@ -158,6 +173,7 @@ fn token_secret_cache() -> &'static RwLock<ApiTokenSecretCache> {
             secrets: HashMap::new(),
             file_mtime: None,
             file_len: None,
+            last_checked: None,
         })
     })
 }
-- 
2.47.3



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

Re: [pbs-devel] [PATCH proxmox-backup 1/3] pbs-config: cache verified API token secrets

[Shannon Sterz]

On Fri Dec 5, 2025 at 2:25 PM CET, Samuel Rufinatscha wrote:
> Currently, every token-based API request reads the token.shadow file and
> runs the expensive password hash verification for the given token
> secret. This shows up as a hotspot in /status profiling (see
> bug #6049 [1]).
>
> This patch introduces an in-memory cache of successfully verified token
> secrets. Subsequent requests for the same token+secret combination only
> perform a comparison using openssl::memcmp::eq and avoid re-running the
> password hash. The cache is updated when a token secret is set and
> cleared when a token is deleted. Note, this does NOT include manual
> config changes, which will be covered in a subsequent patch.
>
> This patch partly fixes bug #6049 [1].
>
> [1] https://bugzilla.proxmox.com/show_bug.cgi?id=7017
>
> Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>
> ---
>  pbs-config/src/token_shadow.rs | 58 +++++++++++++++++++++++++++++++++-
>  1 file changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/pbs-config/src/token_shadow.rs b/pbs-config/src/token_shadow.rs
> index 640fabbf..47aa2fc2 100644
> --- a/pbs-config/src/token_shadow.rs
> +++ b/pbs-config/src/token_shadow.rs
> @@ -1,6 +1,8 @@
>  use std::collections::HashMap;
> +use std::sync::RwLock;
>
>  use anyhow::{bail, format_err, Error};
> +use once_cell::sync::OnceCell;
>  use serde::{Deserialize, Serialize};
>  use serde_json::{from_value, Value};
>
> @@ -13,6 +15,13 @@ use crate::{open_backup_lockfile, BackupLockGuard};
>  const LOCK_FILE: &str = pbs_buildcfg::configdir!("/token.shadow.lock");
>  const CONF_FILE: &str = pbs_buildcfg::configdir!("/token.shadow");
>
> +/// Global in-memory cache for successfully verified API token secrets.
> +/// The cache stores plain text secrets for token Authids that have already been
> +/// verified against the hashed values in `token.shadow`. This allows for cheap
> +/// subsequent authentications for the same token+secret combination, avoiding
> +/// recomputing the password hash on every request.
> +static TOKEN_SECRET_CACHE: OnceCell<RwLock<ApiTokenSecretCache>> = OnceCell::new();

any reason you are using a once cell with a cutom get_or_init function
instead of a simple `LazyCell` [1] here? seems to me that this would be
the more appropriate type here? similar question for the
proxmox-access-control portion of this series.

[1]: https://doc.rust-lang.org/std/cell/struct.LazyCell.html

> +
>  #[derive(Serialize, Deserialize)]
>  #[serde(rename_all = "kebab-case")]
>  /// ApiToken id / secret pair
> @@ -54,9 +63,25 @@ pub fn verify_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> {
>          bail!("not an API token ID");
>      }
>
> +    // Fast path
> +    if let Some(cached) = token_secret_cache().read().unwrap().secrets.get(tokenid) {
> +        // Compare cached secret with provided one using constant time comparison
> +        if openssl::memcmp::eq(cached.as_bytes(), secret.as_bytes()) {
> +            // Already verified before
> +            return Ok(());
> +        }
> +        // Fall through to slow path if secret doesn't match cached one
> +    }
> +
> +    // Slow path: read file + verify hash
>      let data = read_file()?;
>      match data.get(tokenid) {
> -        Some(hashed_secret) => proxmox_sys::crypt::verify_crypt_pw(secret, hashed_secret),
> +        Some(hashed_secret) => {
> +            proxmox_sys::crypt::verify_crypt_pw(secret, hashed_secret)?;
> +            // Cache the plain secret for future requests
> +            cache_insert_secret(tokenid.clone(), secret.to_owned());
> +            Ok(())
> +        }
>          None => bail!("invalid API token"),
>      }
>  }
> @@ -82,6 +107,8 @@ fn set_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> {
>      data.insert(tokenid.clone(), hashed_secret);
>      write_file(data)?;
>
> +    cache_insert_secret(tokenid.clone(), secret.to_owned());
> +
>      Ok(())
>  }
>
> @@ -97,5 +124,34 @@ pub fn delete_secret(tokenid: &Authid) -> Result<(), Error> {
>      data.remove(tokenid);
>      write_file(data)?;
>
> +    cache_remove_secret(tokenid);
> +
>      Ok(())
>  }
> +
> +struct ApiTokenSecretCache {
> +    /// Keys are token Authids, values are the corresponding plain text secrets.
> +    /// Entries are added after a successful on-disk verification in
> +    /// `verify_secret` or when a new token secret is generated by
> +    /// `generate_and_set_secret`. Used to avoid repeated
> +    /// password-hash computation on subsequent authentications.
> +    secrets: HashMap<Authid, String>,
> +}
> +
> +fn token_secret_cache() -> &'static RwLock<ApiTokenSecretCache> {
> +    TOKEN_SECRET_CACHE.get_or_init(|| {
> +        RwLock::new(ApiTokenSecretCache {
> +            secrets: HashMap::new(),
> +        })
> +    })
> +}
> +
> +fn cache_insert_secret(tokenid: Authid, secret: String) {
> +    let mut cache = token_secret_cache().write().unwrap();

unwrap here could panic if another thread is holding a guard, any reason
to not return a result here and bubble up the error instead?

> +    cache.secrets.insert(tokenid, secret);
> +}
> +
> +fn cache_remove_secret(tokenid: &Authid) {
> +    let mut cache = token_secret_cache().write().unwrap();

same here and in the following patches (i won't comment on each
occurrence there separately.)

> +    cache.secrets.remove(tokenid);
> +}



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

Re: [pbs-devel] [PATCH proxmox{-backup, } 0/6] Reduce token.shadow verification overhead

[Shannon Sterz]

thank you for this series and the extensive documentation in it. it was
very easy to follow. the changes look good to me for the most part, see
the comments on the first patch. one top level question, though:

should we publicly document that manually editing the token.shadow will
now not instantly make requests by tokens invalid, but changes will take
up to one minute to take effect? i don't think that this is necessarily
and issue, but imo we shouldn't make such a change without informing
users.

other than this and the comments in-line, consider this:

Reviewed-by: Shannon Sterz <s.sterz@proxmox.com>

On Fri Dec 5, 2025 at 2:25 PM CET, Samuel Rufinatscha wrote:
> Hi,
>
> this series improves the performance of token-based API authentication
> in PBS (pbs-config) and in PDM (underlying proxmox-access-control
> crate), addressing the API token verification hotspot reported in our
> bugtracker #6049 [1].
>
> When profiling PBS /status endpoint with cargo flamegraph [2],
> token-based authentication showed up as a dominant hotspot via
> proxmox_sys::crypt::verify_crypt_pw. Applying this series removes that
> path from the hot section of the flamegraph. The same performance issue
> was measured [3] for PDM. PDM uses the underlying shared
> proxmox-access-control library for token handling, which is a
> factored out version of the token.shadow handling code from PBS.
>
> While this series fixes the immediate performance issue both in PBS
> (pbs-config) and in the shared proxmox-access-control crate used by
> PDM, PBS should eventually, ideally be refactored, in a separate
> effort, to use proxmox-access-control for token handling instead of its
> local implementation.
>
> Problem
>
> For token-based API requests, both PBS’s pbs-config token.shadow
> handling and PDM proxmox-access-control’s token.shadow handling
> currently:
>
> 1. read the token.shadow file on each request
> 2. deserialize it into a HashMap<Authid, String>
> 3. run password hash verification via
>    proxmox_sys::crypt::verify_crypt_pw for the provided token secret
>
> Under load, this results in significant CPU usage spent in repeated
> password hash computations for the same token+secret pairs. The
> attached flamegraphs for PBS [2] and PDM [3] show
> proxmox_sys::crypt::verify_crypt_pw dominating the hot path.
>
> Approach
>
> The goal is to reduce the cost of token-based authentication preserving
> the existing token handling semantics (including detecting manual edits
> to token.shadow) and be consistent between PBS (pbs-config) and
> PDM (proxmox-access-control). For both sites, the series proposes
> following approach:
>
> 1. Introduce an in-memory cache for verified token secrets
> 2. Invalidate the cache when token.shadow changes (detect manual edits)
> 3. Control metadata checks with a TTL window
>
> Testing
>
> *PBS (pbs-config)*
>
> To verify the effect in PBS, I:
> 1. Set up test environment based on latest PBS ISO, installed Rust
>    toolchain, cloned proxmox-backup repository to use with cargo
>    flamegraph. Reproduced bug #6049 [1] by profiling the /status
>    endpoint with token-based authentication using cargo flamegraph [2].
>    The flamegraph showed proxmox_sys::crypt::verify_crypt_pw is the
>    hotspot.
> 2. Built PBS with pbs-config patches and re-ran the same workload and
>    profiling setup.
> 3. Confirmed that the proxmox_sys::crypt::verify_crypt_pw path no
>    longer appears in the hot section of the flamegraph. CPU usage is
>    now dominated by TLS overhead.
> 4. Functionally verified that:
>    * token-based API authentication still works for valid tokens
>    * invalid secrets are rejected as before
>    * generating a new token secret via dashboard works and
>    authenticates correctly
>
> *PDM (proxmox-access-control)*
>
> To verify the effect in PDM, I followed a similar testing approach.
> Instead of /status, I profiled the /version endpoint with cargo
> flamegraph [3] and verified that the token hashing path disappears
> from the hot section after applying the proxmox-access-control patches.
>
> Functionally I verified that:
>    * token-based API authentication still works for valid tokens
>    * invalid secrets are rejected as before
>    * generating a new token secret via dashboard works and
>    authenticates correctly
>
> Patch summary
>
> pbs-config:
>
> 0001 – pbs-config: cache verified API token secrets
> Adds an in-memory cache keyed by Authid that stores plain text token
> secrets after a successful verification or generation and uses
> openssl’s memcmp constant-time for comparison.
>
> 0002 – pbs-config: invalidate token-secret cache on token.shadow changes
> Tracks token.shadow mtime and length and clears the in-memory cache
> when the file changes.
>
> 0003 – pbs-config: add TTL window to token-secret cache
> Introduces a TTL (TOKEN_SECRET_CACHE_TTL_SECS, default 60) for metadata checks so
> that fs::metadata is only called periodically.
>
> proxmox-access-control:
>
> 0004 – access-control: cache verified API token secrets
> Mirrors PBS patch 0001.
>
> 0005 – access-control: invalidate token-secret cache on token.shadow changes
> Mirrors PBS patch 0002.
>
> 0006 – access-control: add TTL window to token-secret cache
> Mirrors PBS patch 0003.
>
> Thanks for considering this patch series, I look forward to your
> feedback.
>
> Best,
> Samuel Rufinatscha
>
> [1] https://bugzilla.proxmox.com/show_bug.cgi?id=6049
> [2] Flamegraph illustrating the`proxmox_sys::crypt::verify_crypt_pw
> hotspot before this series (attached to [1])
>
> proxmox-backup:
>
> Samuel Rufinatscha (3):
>   pbs-config: cache verified API token secrets
>   pbs-config: invalidate token-secret cache on token.shadow changes
>   pbs-config: add TTL window to token secret cache
>
>  pbs-config/src/token_shadow.rs | 109 ++++++++++++++++++++++++++++++++-
>  1 file changed, 108 insertions(+), 1 deletion(-)
>
>
> proxmox:
>
> Samuel Rufinatscha (3):
>   proxmox-access-control: cache verified API token secrets
>   proxmox-access-control: invalidate token-secret cache on token.shadow
>     changes
>   proxmox-access-control: add TTL window to token secret cache
>
>  proxmox-access-control/src/token_shadow.rs | 108 ++++++++++++++++++++-
>  1 file changed, 107 insertions(+), 1 deletion(-)
>
>
> Summary over all repositories:
>   2 files changed, 215 insertions(+), 2 deletions(-)



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