[pbs-devel] [PATCH proxmox{-backup, , -datacenter-manager} v3 00/10] token-shadow: reduce api token verification overhead

[pbs-devel] [PATCH proxmox{-backup, , -datacenter-manager} v3 00/10] token-shadow: reduce api token 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 #7017 [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 [2] 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 hashing 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, this series proposes to:

1. Introduce an in-memory cache for verified token secrets and
invalidate it through a shared ConfigVersionCache generation. Note, a
shared generation is required to keep privileged and unprivileged
daemon in sync to avoid caching inconsistencies across processes.
2. Invalidate on token.shadow file API changes (set_secret,
delete_secret)
3. Invalidate on direct/manual token.shadow file changes (mtime +
length)
4. Avoid per-request file stat calls using 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 #7017 [1] by profiling the /status
   endpoint with token-based authentication using cargo flamegraph [2].
2. Built PBS with pbs-config patches and re-ran the same workload and
   profiling setup. Confirmed that
   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.
3. Functionally-wise, I verified that:
   * valid tokens authenticate correctly when used in API requests
   * invalid secrets are rejected as before
   * generating a new token secret via dashboard (create token for user,
   regenerate existing secret) works and authenticates correctly

*PDM (proxmox-access-control)*

To verify the effect in PDM, I followed a similar testing approach.
Instead of PBS’ /status, I profiled the /version endpoint with cargo
flamegraph [2] and verified that the expensive hashing path disappears
from the hot section after introducing caching.

Functionally-wise, I verified that:
   * valid tokens authenticate correctly when used in API requests
   * invalid secrets are rejected as before
   * generating a new token secret via dashboard (create token for user,
   regenerate existing secret) works and authenticates correctly

Benchmarks:

Two different benchmarks have been run to measure caching effects
and RwLock contention:

(1) Requests per second for PBS /status endpoint (E2E)

Benchmarked parallel token auth requests for
/status?verbose=0 on top of the datastore lookup cache series [4]
to check throughput impact. With datastores=1, repeat=5000, parallel=16
this series gives ~172 req/s compared to ~65 req/s without it.
This is a ~2.6x improvement (and aligns with the ~179 req/s from the
previous series, which used per-process cache invalidation).

(2) RwLock contention for token create/delete under heavy load of
token-authenticated requests

The previous version of the series compared std::sync::RwLock and
parking_lot::RwLock contention for token create/delete under heavy
parallel token-authenticated readers. parking_lot::RwLock has been
chosen for the added fairness guarantees.

Patch summary

pbs-config:

0001 – pbs-config: add token.shadow generation to ConfigVersionCache
Extends ConfigVersionCache to provide a process-shared generation
number for token.shadow changes.

0002 – pbs-config: cache verified API token secrets
Adds an in-memory cache to cache verified, plain-text API token secrets.
Cache is invalidated through the process-shared ConfigVersionCache
generation number. Uses openssl’s memcmp constant-time for matching
secrets.

0003 – pbs-config: invalidate token-secret cache on token.shadow
changes
Stats token.shadow mtime and length and clears the cache when the
file changes, on each token verification request.

0004 – 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 calls are not performed on each request.

proxmox-access-control:

0005 – access-control: extend AccessControlConfig for token.shadow invalidation

Extends the AccessControlConfig trait with
token_shadow_cache_generation() and
increment_token_shadow_cache_generation() for
proxmox-access-control to get the shared token.shadow generation number
and bump it on token shadow changes.

0006 – access-control: cache verified API token secrets
Mirrors PBS PATCH 0002.

0007 – access-control: invalidate token-secret cache on token.shadow changes
Mirrors PBS PATCH 0003.

0008 – access-control: add TTL window to token-secret cache
Mirrors PBS PATCH 0004.

proxmox-datacenter-manager:

0009 – pdm-config: add token.shadow generation to ConfigVersionCache
Extends PDM ConfigVersionCache and implements
token_shadow_cache_generation() and
increment_token_shadow_cache_generation() from AccessControlConfig for
PDM.

0010 – docs: document API token-cache TTL effects
Documents the effects of the TTL window on token.shadow edits

Changes from v1 to v2:

* (refactor) Switched cache initialization to LazyLock
* (perf) Use parking_lot::RwLock and best-effort cache access on the
  read/refresh path (try_read/try_write) to avoid lock contention
* (doc) Document TTL-delayed effect of manual token.shadow edits
* (fix) Add generation guards (API_MUTATION_GENERATION +
  FILE_GENERATION) to prevent caching across concurrent set/delete and
  external edits

Changes from v2 to v3:

* (refactor) Replace PBS per-process cache invalidation with a
  cross-process token.shadow generation based on PBS
  ConfigVersionCache, ensuring cache consistency between privileged
  and unprivileged daemons.
* (refactor) Decoupling generation source from the
  proxmox/proxmox-access-control cache implementation: extend
  AccessControlConfig hooks so that products can provide the shared
  token.shadow generation source.
* (refactor) Extend PDM's ConfigVersionCache with
  token_shadow_generation
  and introduce a pdm_config::AccessControlConfig wrapper implementing
  the new proxmox-access-control trait hooks. Switch server and CLI
  initialization to use pdm_config::AccessControlConfig instead of
  pdm_api_types::AccessControlConfig.
* (refactor) Adapt generation checks around cached-secret comparison to
  use the new shared generation source.
* (fix/logic) cache_try_insert_secret: Update the local cache
  generation if stale, allowing the new secret to be inserted
  immediately
* (refactor) Extract cache invalidation logic into a
  invalidate_cache_state helper to reduce duplication and ensure
  consistent state resets
* (refactor) Simplify refresh_cache_if_file_changed: handle the
  un-initialized/reset state and adjust the generation mismatch
  path to ensure file metadata is always re-read.
* (doc) Clarify TTL-delayed effects of manual token.shadow edits.

Please see the patch specific changelogs for more details.

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

Best,
Samuel Rufinatscha

[1] https://bugzilla.proxmox.com/show_bug.cgi?id=7017
[2] attachment 1767 [1]: Flamegraph showing the proxmox_sys::crypt::verify_crypt_pw stack
[3] attachment 1794 [1]: Flamegraph PDM baseline
[4] https://bugzilla.proxmox.com/show_bug.cgi?id=6049

proxmox-backup:

Samuel Rufinatscha (4):
  pbs-config: add token.shadow generation to ConfigVersionCache
  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

 Cargo.toml                             |   1 +
 docs/user-management.rst               |   4 +
 pbs-config/Cargo.toml                  |   1 +
 pbs-config/src/config_version_cache.rs |  18 ++
 pbs-config/src/token_shadow.rs         | 298 ++++++++++++++++++++++++-
 5 files changed, 321 insertions(+), 1 deletion(-)


proxmox:

Samuel Rufinatscha (4):
  proxmox-access-control: extend AccessControlConfig for token.shadow
    invalidation
  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

 Cargo.toml                                 |   1 +
 proxmox-access-control/Cargo.toml          |   1 +
 proxmox-access-control/src/init.rs         |  17 ++
 proxmox-access-control/src/token_shadow.rs | 299 ++++++++++++++++++++-
 4 files changed, 317 insertions(+), 1 deletion(-)


proxmox-datacenter-manager:

Samuel Rufinatscha (2):
  pdm-config: implement token.shadow generation
  docs: document API token-cache TTL effects

 cli/admin/src/main.rs                       |  2 +-
 docs/access-control.rst                     |  4 ++
 lib/pdm-config/Cargo.toml                   |  1 +
 lib/pdm-config/src/access_control_config.rs | 73 +++++++++++++++++++++
 lib/pdm-config/src/config_version_cache.rs  | 18 +++++
 lib/pdm-config/src/lib.rs                   |  2 +
 server/src/acl.rs                           |  3 +-
 7 files changed, 100 insertions(+), 3 deletions(-)
 create mode 100644 lib/pdm-config/src/access_control_config.rs


Summary over all repositories:
  16 files changed, 738 insertions(+), 5 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 v3 1/4] pbs-config: add token.shadow generation to ConfigVersionCache

[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 #7017 [1]).

To solve the issue, this patch prepares the config version cache,
so that token_shadow_generation config caching can be built on
top of it.

This patch specifically:
(1) implements increment function in order to invalidate generations

This patch is part of the series which fixes bug #7017 [1].

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

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

diff --git a/pbs-config/src/config_version_cache.rs b/pbs-config/src/config_version_cache.rs
index e8fb994f..1376b11d 100644
--- a/pbs-config/src/config_version_cache.rs
+++ b/pbs-config/src/config_version_cache.rs
@@ -28,6 +28,8 @@ struct ConfigVersionCacheDataInner {
     // datastore (datastore.cfg) generation/version
     // FIXME: remove with PBS 3.0
     datastore_generation: AtomicUsize,
+    // Token shadow (token.shadow) generation/version.
+    token_shadow_generation: AtomicUsize,
     // Add further atomics here
 }
 
@@ -153,4 +155,20 @@ impl ConfigVersionCache {
             .datastore_generation
             .fetch_add(1, Ordering::AcqRel)
     }
+
+    /// Returns the token shadow generation number.
+    pub fn token_shadow_generation(&self) -> usize {
+        self.shmem
+            .data()
+            .token_shadow_generation
+            .load(Ordering::Acquire)
+    }
+
+    /// Increase the token shadow generation number.
+    pub fn increase_token_shadow_generation(&self) -> usize {
+        self.shmem
+            .data()
+            .token_shadow_generation
+            .fetch_add(1, Ordering::AcqRel)
+    }
 }
-- 
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 v3 2/4] 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 #7017 [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 is part of the series which 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:

* Replace OnceCell with LazyLock, and std::sync::RwLock with
parking_lot::RwLock.
* Add API_MUTATION_GENERATION and guard cache inserts
to prevent “zombie inserts” across concurrent set/delete.
* Refactor cache operations into cache_try_secret_matches,
cache_try_insert_secret, and centralize write-side behavior in
apply_api_mutation.
* Switch fast-path cache access to try_read/try_write (best-effort).

Changes from v2 to v3:

* Replaced process-local cache invalidation (AtomicU64
API_MUTATION_GENERATION) with a cross-process shared generation via
ConfigVersionCache.
* Validate shared generation before/after the constant-time secret
compare; only insert into cache if the generation is unchanged.
* invalidate_cache_state() on insert if shared generation changed.

 Cargo.toml                     |   1 +
 pbs-config/Cargo.toml          |   1 +
 pbs-config/src/token_shadow.rs | 157 ++++++++++++++++++++++++++++++++-
 3 files changed, 158 insertions(+), 1 deletion(-)

diff --git a/Cargo.toml b/Cargo.toml
index 1aa57ae5..821b63b7 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -143,6 +143,7 @@ nom = "7"
 num-traits = "0.2"
 once_cell = "1.3.1"
 openssl = "0.10.40"
+parking_lot = "0.12"
 percent-encoding = "2.1"
 pin-project-lite = "0.2"
 regex = "1.5.5"
diff --git a/pbs-config/Cargo.toml b/pbs-config/Cargo.toml
index 74afb3c6..eb81ce00 100644
--- a/pbs-config/Cargo.toml
+++ b/pbs-config/Cargo.toml
@@ -13,6 +13,7 @@ libc.workspace = true
 nix.workspace = true
 once_cell.workspace = true
 openssl.workspace = true
+parking_lot.workspace = true
 regex.workspace = true
 serde.workspace = true
 serde_json.workspace = true
diff --git a/pbs-config/src/token_shadow.rs b/pbs-config/src/token_shadow.rs
index 640fabbf..fa84aee5 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::LazyLock;
 
 use anyhow::{bail, format_err, Error};
+use parking_lot::RwLock;
 use serde::{Deserialize, Serialize};
 use serde_json::{from_value, Value};
 
@@ -13,6 +15,18 @@ 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: LazyLock<RwLock<ApiTokenSecretCache>> = LazyLock::new(|| {
+    RwLock::new(ApiTokenSecretCache {
+        secrets: HashMap::new(),
+        shared_gen: 0,
+    })
+});
+
 #[derive(Serialize, Deserialize)]
 #[serde(rename_all = "kebab-case")]
 /// ApiToken id / secret pair
@@ -54,9 +68,27 @@ pub fn verify_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> {
         bail!("not an API token ID");
     }
 
+    // Fast path
+    if cache_try_secret_matches(tokenid, secret) {
+        return Ok(());
+    }
+
+    // Slow path
+    // First, capture the shared generation before doing the hash verification.
+    let gen_before = token_shadow_shared_gen();
+
     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)?;
+
+            // Try to cache only if nothing changed while verifying the secret.
+            if let Some(gen) = gen_before {
+                cache_try_insert_secret(tokenid.clone(), secret.to_owned(), gen);
+            }
+
+            Ok(())
+        }
         None => bail!("invalid API token"),
     }
 }
@@ -82,6 +114,8 @@ fn set_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> {
     data.insert(tokenid.clone(), hashed_secret);
     write_file(data)?;
 
+    apply_api_mutation(tokenid, Some(secret));
+
     Ok(())
 }
 
@@ -97,5 +131,126 @@ pub fn delete_secret(tokenid: &Authid) -> Result<(), Error> {
     data.remove(tokenid);
     write_file(data)?;
 
+    apply_api_mutation(tokenid, None);
+
     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, CachedSecret>,
+    /// Shared generation to detect mutations of the underlying token.shadow file.
+    shared_gen: usize,
+}
+
+/// Cached secret.
+struct CachedSecret {
+    secret: String,
+}
+
+fn cache_try_insert_secret(tokenid: Authid, secret: String, shared_gen_before: usize) {
+    let Some(mut cache) = TOKEN_SECRET_CACHE.try_write() else {
+        return;
+    };
+
+    let Some(shared_gen_now) = token_shadow_shared_gen() else {
+        return;
+    };
+
+    // If this process missed a generation bump, its cache is stale.
+    if cache.shared_gen != shared_gen_now {
+        invalidate_cache_state(&mut cache);
+        cache.shared_gen = shared_gen_now;
+    }
+
+    // If a mutation happened while we were verifying the secret, do not insert.
+    if shared_gen_now == shared_gen_before {
+        cache.secrets.insert(tokenid, CachedSecret { secret });
+    }
+}
+
+// Tries to match the given token secret against the cached secret.
+// Checks the generation before and after the constant-time compare to avoid a
+// TOCTOU window. If another process rotates/deletes a token while we're validating
+// the cached secret, the generation will change, and we
+// must not trust the cache for this request.
+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 cache_gen = cache.shared_gen;
+
+    let Some(gen1) = token_shadow_shared_gen() else {
+        return false;
+    };
+    if gen1 != cache_gen {
+        return false;
+    }
+
+    let eq = openssl::memcmp::eq(entry.secret.as_bytes(), secret.as_bytes());
+
+    let Some(gen2) = token_shadow_shared_gen() else {
+        return false;
+    };
+
+    eq && gen2 == cache_gen
+}
+
+fn apply_api_mutation(tokenid: &Authid, new_secret: Option<&str>) {
+    // Signal cache invalidation to other processes (best-effort).
+    let new_shared_gen = bump_token_shadow_shared_gen();
+
+    let mut cache = TOKEN_SECRET_CACHE.write();
+
+    // If we cannot read/bump the shared generation, we cannot safely trust the cache.
+    let Some(gen) = new_shared_gen else {
+        invalidate_cache_state(&mut cache);
+        cache.shared_gen = 0;
+        return;
+    };
+
+    // Update to the post-mutation generation.
+    cache.shared_gen = gen;
+
+    // Apply the new mutation.
+    match new_secret {
+        Some(secret) => {
+            cache.secrets.insert(
+                tokenid.clone(),
+                CachedSecret {
+                    secret: secret.to_owned(),
+                },
+            );
+        }
+        None => {
+            cache.secrets.remove(tokenid);
+        }
+    }
+}
+
+/// Get the current shared generation.
+fn token_shadow_shared_gen() -> Option<usize> {
+    crate::ConfigVersionCache::new()
+        .ok()
+        .map(|cvc| cvc.token_shadow_generation())
+}
+
+/// Bump and return the new shared generation.
+fn bump_token_shadow_shared_gen() -> Option<usize> {
+    crate::ConfigVersionCache::new()
+        .ok()
+        .map(|cvc| cvc.increase_token_shadow_generation() + 1)
+}
+
+/// Invalidates the cache state and only keeps the shared generation.
+fn invalidate_cache_state(cache: &mut ApiTokenSecretCache) {
+    cache.secrets.clear();
+}
-- 
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 v3 3/4] 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 part of the series which 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.

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.

 pbs-config/src/token_shadow.rs | 122 +++++++++++++++++++++++++++++++--
 1 file changed, 118 insertions(+), 4 deletions(-)

diff --git a/pbs-config/src/token_shadow.rs b/pbs-config/src/token_shadow.rs
index fa84aee5..02fb191b 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::LazyLock;
+use std::time::SystemTime;
 
 use anyhow::{bail, format_err, Error};
 use parking_lot::RwLock;
@@ -7,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 +28,9 @@ static TOKEN_SECRET_CACHE: LazyLock<RwLock<ApiTokenSecretCache>> = LazyLock::new
     RwLock::new(ApiTokenSecretCache {
         secrets: HashMap::new(),
         shared_gen: 0,
+        file_mtime: None,
+        file_len: None,
+        last_checked: None,
     })
 });
 
@@ -62,6 +69,63 @@ fn write_file(data: HashMap<Authid, String>) -> Result<(), Error> {
     proxmox_sys::fs::replace_file(CONF_FILE, &json, options, true)
 }
 
+/// 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();
+
+    // Best-effort refresh under write lock.
+    let Some(mut cache) = TOKEN_SECRET_CACHE.try_write() else {
+        return false;
+    };
+
+    let Some(shared_gen_now) = token_shadow_shared_gen() else {
+        return false;
+    };
+
+    // If another process bumped the generation, we don't know what changed -> clear cache
+    if cache.shared_gen != shared_gen_now {
+        invalidate_cache_state(&mut cache);
+        cache.shared_gen = shared_gen_now;
+    }
+
+    // Stat the file to detect manual edits.
+    let Ok((new_mtime, new_len)) = shadow_mtime_len() else {
+        return false;
+    };
+
+    // Initialize file stats if we have no prior state.
+    if cache.last_checked.is_none() {
+        cache.secrets.clear(); // ensure cache is empty on first load
+        cache.file_mtime = new_mtime;
+        cache.file_len = new_len;
+        cache.last_checked = Some(now);
+        return true;
+    }
+
+    // No change detected.
+    if cache.file_mtime == new_mtime && cache.file_len == new_len {
+        cache.last_checked = Some(now);
+        return true;
+    }
+
+    // Manual edit detected -> invalidate cache and update stat.
+    cache.secrets.clear();
+    cache.file_mtime = new_mtime;
+    cache.file_len = new_len;
+    cache.last_checked = Some(now);
+
+    // Best-effort propagation to other processes + update local view.
+    if let Some(shared_gen_new) = bump_token_shadow_shared_gen() {
+        cache.shared_gen = shared_gen_new;
+    } else {
+        // Do not fail: local cache is already safe as we cleared it above.
+        // Keep local shared_gen as-is to avoid repeated failed attempts.
+    }
+
+    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() {
@@ -69,7 +133,7 @@ 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(());
     }
 
@@ -109,12 +173,15 @@ 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(())
 }
@@ -127,11 +194,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(())
 }
@@ -145,6 +215,12 @@ struct ApiTokenSecretCache {
     secrets: HashMap<Authid, CachedSecret>,
     /// Shared generation to detect mutations of the underlying token.shadow file.
     shared_gen: usize,
+    // shadow file mtime to detect changes
+    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.
@@ -204,7 +280,13 @@ fn cache_try_secret_matches(tokenid: &Authid, secret: &str) -> bool {
     eq && gen2 == cache_gen
 }
 
-fn apply_api_mutation(tokenid: &Authid, new_secret: Option<&str>) {
+fn apply_api_mutation(
+    tokenid: &Authid,
+    new_secret: Option<&str>,
+    pre_write_meta: (Option<SystemTime>, Option<u64>),
+) {
+    let now = epoch_i64();
+
     // Signal cache invalidation to other processes (best-effort).
     let new_shared_gen = bump_token_shadow_shared_gen();
 
@@ -220,6 +302,13 @@ fn apply_api_mutation(tokenid: &Authid, new_secret: Option<&str>) {
     // Update to the post-mutation generation.
     cache.shared_gen = gen;
 
+    // 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.
+    let (pre_mtime, pre_len) = pre_write_meta;
+    if cache.file_mtime != pre_mtime || cache.file_len != pre_len {
+        cache.secrets.clear();
+    }
+
     // Apply the new mutation.
     match new_secret {
         Some(secret) => {
@@ -234,6 +323,20 @@ fn apply_api_mutation(tokenid: &Authid, new_secret: Option<&str>) {
             cache.secrets.remove(tokenid);
         }
     }
+
+    // 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_mtime = mtime;
+            cache.file_len = len;
+            cache.last_checked = Some(now);
+        }
+        Err(_) => {
+            // If we cannot validate state, do not trust cache.
+            invalidate_cache_state(&mut cache);
+        }
+    }
 }
 
 /// Get the current shared generation.
@@ -253,4 +356,15 @@ fn bump_token_shadow_shared_gen() -> Option<usize> {
 /// Invalidates the cache state and only keeps the shared generation.
 fn invalidate_cache_state(cache: &mut ApiTokenSecretCache) {
     cache.secrets.clear();
+    cache.file_mtime = None;
+    cache.file_len = None;
+    cache.last_checked = None;
+}
+
+fn shadow_mtime_len() -> Result<(Option<SystemTime>, Option<u64>), Error> {
+    match fs::metadata(CONF_FILE) {
+        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

[pbs-devel] [PATCH proxmox-backup v3 4/4] 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. Documents TTL effects.

This patch is part of the series which 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.

Changes from v2 to v3:

* Refactored refresh_cache_if_file_changed TTL logic.
* Remove had_prior_state check (replaced by last_checked logic).
* Improve TTL bound checks.
* Reword documentation warning for clarity.

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

diff --git a/docs/user-management.rst b/docs/user-management.rst
index 41b43d60..8dfae528 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:: Direct/manual edits to ``token.shadow`` may take up to 60 seconds (or
+   longer in edge cases) to take effect due to caching. Restart services for
+   immediate effect of manual edits.
+
 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 02fb191b..e3529b40 100644
--- a/pbs-config/src/token_shadow.rs
+++ b/pbs-config/src/token_shadow.rs
@@ -33,6 +33,8 @@ static TOKEN_SECRET_CACHE: LazyLock<RwLock<ApiTokenSecretCache>> = LazyLock::new
         last_checked: None,
     })
 });
+/// 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,11 +76,28 @@ fn write_file(data: HashMap<Authid, String>) -> Result<(), Error> {
 fn refresh_cache_if_file_changed() -> bool {
     let now = epoch_i64();
 
-    // Best-effort refresh under write lock.
+    // Fast path: cache is fresh if shared-gen matches and TTL not expired.
+    if let (Some(cache), Some(shared_gen_read)) =
+        (TOKEN_SECRET_CACHE.try_read(), token_shadow_shared_gen())
+    {
+        if cache.shared_gen == shared_gen_read
+            && cache
+                .last_checked
+                .is_some_and(|last| now >= last && (now - last) < TOKEN_SECRET_CACHE_TTL_SECS)
+        {
+            return true;
+        }
+        // read lock drops here
+    } else {
+        return false;
+    }
+
+    // Slow path: best-effort refresh under write lock.
     let Some(mut cache) = TOKEN_SECRET_CACHE.try_write() else {
         return false;
     };
 
+    // Re-read generation after acquiring the lock (may have changed meanwhile).
     let Some(shared_gen_now) = token_shadow_shared_gen() else {
         return false;
     };
@@ -89,6 +108,14 @@ fn refresh_cache_if_file_changed() -> bool {
         cache.shared_gen = shared_gen_now;
     }
 
+    // TTL check again after acquiring the lock
+    if cache
+        .last_checked
+        .is_some_and(|last| now >= last && (now - last) < TOKEN_SECRET_CACHE_TTL_SECS)
+    {
+        return true;
+    }
+
     // Stat the file to detect manual edits.
     let Ok((new_mtime, new_len)) = shadow_mtime_len() else {
         return false;
-- 
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 v3 1/4] proxmox-access-control: extend AccessControlConfig for token.shadow invalidation

[Samuel Rufinatscha]

Add token_shadow_cache_generation() and
increment_token_shadow_cache_generation()
hooks to AccessControlConfig. This lets products provide a cross-process
invalidation signal for token.shadow so proxmox-access-control can cache
verified API token secrets and invalidate that cache on token
rotation/deletion.

This patch is part of the series which fixes bug #7017 [1].

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

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

diff --git a/proxmox-access-control/src/init.rs b/proxmox-access-control/src/init.rs
index e64398e8..0ba1a526 100644
--- a/proxmox-access-control/src/init.rs
+++ b/proxmox-access-control/src/init.rs
@@ -51,6 +51,23 @@ pub trait AccessControlConfig: Send + Sync {
         Ok(())
     }
 
+    /// Returns the current cache generation of the token shadow cache. If the generation was
+    /// incremented since the last time the cache was queried, the token shadow cache is reloaded
+    /// from disk.
+    ///
+    /// Default: Always returns `None`.
+    fn token_shadow_cache_generation(&self) -> Option<usize> {
+        None
+    }
+
+    /// Increment the cache generation of the token shadow cache. This indicates that it was
+    /// changed on disk.
+    ///
+    /// Default: Returns an error as token shadow generation is not supported.
+    fn increment_token_shadow_cache_generation(&self) -> Result<usize, Error> {
+        anyhow::bail!("token shadow generation not supported");
+    }
+
     /// Optionally returns a role that has no access to any resource.
     ///
     /// Default: Returns `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 v3 2/4] 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 #7017 [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 part of the series which 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:

* Replace OnceCell with LazyLock, and std::sync::RwLock with
parking_lot::RwLock.
* Add API_MUTATION_GENERATION and guard cache inserts
to prevent “zombie inserts” across concurrent set/delete.
* Refactor cache operations into cache_try_secret_matches,
cache_try_insert_secret, and centralize write-side behavior in
apply_api_mutation.
* Switch fast-path cache access to try_read/try_write (best-effort).

Changes from v2 to v3:

* Replaced process-local cache invalidation (AtomicU64
API_MUTATION_GENERATION) with a cross-process shared generation via
ConfigVersionCache.
* Validate shared generation before/after the constant-time secret
compare; only insert into cache if the generation is unchanged.
* invalidate_cache_state() on insert if shared generation changed.

 Cargo.toml                                 |   1 +
 proxmox-access-control/Cargo.toml          |   1 +
 proxmox-access-control/src/token_shadow.rs | 154 ++++++++++++++++++++-
 3 files changed, 155 insertions(+), 1 deletion(-)

diff --git a/Cargo.toml b/Cargo.toml
index 27a69afa..59a2ec93 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -112,6 +112,7 @@ native-tls = "0.2"
 nix = "0.29"
 openssl = "0.10"
 pam-sys = "0.5"
+parking_lot = "0.12"
 percent-encoding = "2.1"
 pin-utils = "0.1.0"
 proc-macro2 = "1.0"
diff --git a/proxmox-access-control/Cargo.toml b/proxmox-access-control/Cargo.toml
index ec189664..1de2842c 100644
--- a/proxmox-access-control/Cargo.toml
+++ b/proxmox-access-control/Cargo.toml
@@ -16,6 +16,7 @@ anyhow.workspace = true
 const_format.workspace = true
 nix = { workspace = true, optional = true }
 openssl = { workspace = true, optional = true }
+parking_lot.workspace = true
 regex.workspace = true
 hex = { workspace = true, optional = true }
 serde.workspace = true
diff --git a/proxmox-access-control/src/token_shadow.rs b/proxmox-access-control/src/token_shadow.rs
index c586d834..895309d2 100644
--- a/proxmox-access-control/src/token_shadow.rs
+++ b/proxmox-access-control/src/token_shadow.rs
@@ -1,13 +1,28 @@
 use std::collections::HashMap;
+use std::sync::LazyLock;
 
 use anyhow::{bail, format_err, Error};
+use parking_lot::RwLock;
 use serde_json::{from_value, Value};
 
 use proxmox_auth_api::types::Authid;
 use proxmox_product_config::{open_api_lockfile, replace_config, ApiLockGuard};
 
+use crate::init::access_conf;
 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: LazyLock<RwLock<ApiTokenSecretCache>> = LazyLock::new(|| {
+    RwLock::new(ApiTokenSecretCache {
+        secrets: HashMap::new(),
+        shared_gen: 0,
+    })
+});
+
 // Get exclusive lock
 fn lock_config() -> Result<ApiLockGuard, Error> {
     open_api_lockfile(token_shadow_lock(), None, true)
@@ -36,9 +51,27 @@ pub fn verify_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> {
         bail!("not an API token ID");
     }
 
+    // Fast path
+    if cache_try_secret_matches(tokenid, secret) {
+        return Ok(());
+    }
+
+    // Slow path
+    // First, capture the shared generation before doing the hash verification.
+    let gen_before = token_shadow_shared_gen();
+
     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)?;
+
+            // Try to cache only if nothing changed while verifying the secret.
+            if let Some(gen) = gen_before {
+                cache_try_insert_secret(tokenid.clone(), secret.to_owned(), gen);
+            }
+
+            Ok(())
+        }
         None => bail!("invalid API token"),
     }
 }
@@ -56,6 +89,8 @@ pub fn set_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> {
     data.insert(tokenid.clone(), hashed_secret);
     write_file(data)?;
 
+    apply_api_mutation(tokenid, Some(secret));
+
     Ok(())
 }
 
@@ -71,6 +106,8 @@ pub fn delete_secret(tokenid: &Authid) -> Result<(), Error> {
     data.remove(tokenid);
     write_file(data)?;
 
+    apply_api_mutation(tokenid, None);
+
     Ok(())
 }
 
@@ -81,3 +118,118 @@ 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, CachedSecret>,
+    /// Shared generation to detect mutations of the underlying token.shadow file.
+    shared_gen: usize,
+}
+
+/// Cached secret.
+struct CachedSecret {
+    secret: String,
+}
+
+fn cache_try_insert_secret(tokenid: Authid, secret: String, shared_gen_before: usize) {
+    let Some(mut cache) = TOKEN_SECRET_CACHE.try_write() else {
+        return;
+    };
+
+    let Some(shared_gen_now) = token_shadow_shared_gen() else {
+        return;
+    };
+
+    // If this process missed a generation bump, its cache is stale.
+    if cache.shared_gen != shared_gen_now {
+        invalidate_cache_state(&mut cache);
+        cache.shared_gen = shared_gen_now;
+    }
+
+    // If a mutation happened while we were verifying the secret, do not insert.
+    if shared_gen_now == shared_gen_before {
+        cache.secrets.insert(tokenid, CachedSecret { secret });
+    }
+}
+
+// Tries to match the given token secret against the cached secret.
+// Checks the generation before and after the constant-time compare to avoid a
+// TOCTOU window. If another process rotates/deletes a token while we're validating
+// the cached secret, the generation will change, and we
+// must not trust the cache for this request.
+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 cache_gen = cache.shared_gen;
+
+    let Some(gen1) = token_shadow_shared_gen() else {
+        return false;
+    };
+    if gen1 != cache_gen {
+        return false;
+    }
+
+    let eq = openssl::memcmp::eq(entry.secret.as_bytes(), secret.as_bytes());
+
+    let Some(gen2) = token_shadow_shared_gen() else {
+        return false;
+    };
+
+    eq && gen2 == cache_gen
+}
+
+fn apply_api_mutation(tokenid: &Authid, new_secret: Option<&str>) {
+    // Signal cache invalidation to other processes (best-effort).
+    let new_shared_gen = bump_token_shadow_shared_gen();
+
+    let mut cache = TOKEN_SECRET_CACHE.write();
+
+    // If we cannot read/bump the shared generation, we cannot safely trust the cache.
+    let Some(gen) = new_shared_gen else {
+        invalidate_cache_state(&mut cache);
+        cache.shared_gen = 0;
+        return;
+    };
+
+    // Update to the post-mutation generation.
+    cache.shared_gen = gen;
+
+    // Apply the new mutation.
+    match new_secret {
+        Some(secret) => {
+            cache.secrets.insert(
+                tokenid.clone(),
+                CachedSecret {
+                    secret: secret.to_owned(),
+                },
+            );
+        }
+        None => {
+            cache.secrets.remove(tokenid);
+        }
+    }
+}
+
+/// Get the current shared generation.
+fn token_shadow_shared_gen() -> Option<usize> {
+    access_conf().token_shadow_cache_generation()
+}
+
+/// Bump and return the new shared generation.
+fn bump_token_shadow_shared_gen() -> Option<usize> {
+    access_conf().increment_token_shadow_cache_generation().ok().map(|prev| prev + 1)
+}
+
+/// Invalidates the cache state and only keeps the shared generation.
+fn invalidate_cache_state(cache: &mut ApiTokenSecretCache) {
+    cache.secrets.clear();
+}
\ No newline at end of file
-- 
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 v3 3/4] 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 part of the series which 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.

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.

 proxmox-access-control/src/token_shadow.rs | 129 ++++++++++++++++++++-
 1 file changed, 123 insertions(+), 6 deletions(-)

diff --git a/proxmox-access-control/src/token_shadow.rs b/proxmox-access-control/src/token_shadow.rs
index 895309d2..f30c8ed5 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_conf;
 use crate::init::impl_feature::{token_shadow, token_shadow_lock};
@@ -20,6 +24,9 @@ static TOKEN_SECRET_CACHE: LazyLock<RwLock<ApiTokenSecretCache>> = LazyLock::new
     RwLock::new(ApiTokenSecretCache {
         secrets: HashMap::new(),
         shared_gen: 0,
+        file_mtime: None,
+        file_len: None,
+        last_checked: None,
     })
 });
 
@@ -45,6 +52,63 @@ 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 now = epoch_i64();
+
+    // Best-effort refresh under write lock.
+    let Some(mut cache) = TOKEN_SECRET_CACHE.try_write() else {
+        return false;
+    };
+
+    let Some(shared_gen_now) = token_shadow_shared_gen() else {
+        return false;
+    };
+
+    // If another process bumped the generation, we don't know what changed -> clear cache
+    if cache.shared_gen != shared_gen_now {
+        invalidate_cache_state(&mut cache);
+        cache.shared_gen = shared_gen_now;
+    }
+
+    // Stat the file to detect manual edits.
+    let Ok((new_mtime, new_len)) = shadow_mtime_len() else {
+        return false;
+    };
+
+    // Initialize file stats if we have no prior state.
+    if cache.last_checked.is_none() {
+        cache.secrets.clear(); // ensure cache is empty on first load
+        cache.file_mtime = new_mtime;
+        cache.file_len = new_len;
+        cache.last_checked = Some(now);
+        return true;
+    }
+
+    // No change detected.
+    if cache.file_mtime == new_mtime && cache.file_len == new_len {
+        cache.last_checked = Some(now);
+        return true;
+    }
+
+    // Manual edit detected -> invalidate cache and update stat.
+    cache.secrets.clear();
+    cache.file_mtime = new_mtime;
+    cache.file_len = new_len;
+    cache.last_checked = Some(now);
+
+    // Best-effort propagation to other processes + update local view.
+    if let Some(shared_gen_new) = bump_token_shadow_shared_gen() {
+        cache.shared_gen = shared_gen_new;
+    } else {
+        // Do not fail: local cache is already safe as we cleared it above.
+        // Keep local shared_gen as-is to avoid repeated failed attempts.
+    }
+
+    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() {
@@ -52,7 +116,7 @@ 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(());
     }
 
@@ -84,12 +148,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(())
 }
@@ -102,11 +169,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(())
 }
@@ -128,6 +198,12 @@ struct ApiTokenSecretCache {
     secrets: HashMap<Authid, CachedSecret>,
     /// Shared generation to detect mutations of the underlying token.shadow file.
     shared_gen: usize,
+    // shadow file mtime to detect changes
+    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.
@@ -187,7 +263,13 @@ fn cache_try_secret_matches(tokenid: &Authid, secret: &str) -> bool {
     eq && gen2 == cache_gen
 }
 
-fn apply_api_mutation(tokenid: &Authid, new_secret: Option<&str>) {
+fn apply_api_mutation(
+    tokenid: &Authid,
+    new_secret: Option<&str>,
+    pre_write_meta: (Option<SystemTime>, Option<u64>),
+) {
+    let now = epoch_i64();
+
     // Signal cache invalidation to other processes (best-effort).
     let new_shared_gen = bump_token_shadow_shared_gen();
 
@@ -203,6 +285,13 @@ fn apply_api_mutation(tokenid: &Authid, new_secret: Option<&str>) {
     // Update to the post-mutation generation.
     cache.shared_gen = gen;
 
+    // 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.
+    let (pre_mtime, pre_len) = pre_write_meta;
+    if cache.file_mtime != pre_mtime || cache.file_len != pre_len {
+        cache.secrets.clear();
+    }
+
     // Apply the new mutation.
     match new_secret {
         Some(secret) => {
@@ -217,6 +306,20 @@ fn apply_api_mutation(tokenid: &Authid, new_secret: Option<&str>) {
             cache.secrets.remove(tokenid);
         }
     }
+
+    // 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_mtime = mtime;
+            cache.file_len = len;
+            cache.last_checked = Some(now);
+        }
+        Err(_) => {
+            // If we cannot validate state, do not trust cache.
+            invalidate_cache_state(&mut cache);
+        }
+    }
 }
 
 /// Get the current shared generation.
@@ -226,10 +329,24 @@ fn token_shadow_shared_gen() -> Option<usize> {
 
 /// Bump and return the new shared generation.
 fn bump_token_shadow_shared_gen() -> Option<usize> {
-    access_conf().increment_token_shadow_cache_generation().ok().map(|prev| prev + 1)
+    access_conf()
+        .increment_token_shadow_cache_generation()
+        .ok()
+        .map(|prev| prev + 1)
 }
 
 /// Invalidates the cache state and only keeps the shared generation.
 fn invalidate_cache_state(cache: &mut ApiTokenSecretCache) {
     cache.secrets.clear();
-}
\ No newline at end of file
+    cache.file_mtime = None;
+    cache.file_len = None;
+    cache.last_checked = None;
+}
+
+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



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

[pbs-devel] [PATCH proxmox v3 4/4] 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.

This patch is part of the series which 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.

Changes from v2 to v3:

* Refactored refresh_cache_if_file_changed TTL logic.
* Remove had_prior_state check (replaced by last_checked logic).
* Improve TTL bound checks.
* Reword documentation warning for clarity.

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

diff --git a/proxmox-access-control/src/token_shadow.rs b/proxmox-access-control/src/token_shadow.rs
index f30c8ed5..14eea560 100644
--- a/proxmox-access-control/src/token_shadow.rs
+++ b/proxmox-access-control/src/token_shadow.rs
@@ -30,6 +30,9 @@ static TOKEN_SECRET_CACHE: LazyLock<RwLock<ApiTokenSecretCache>> = LazyLock::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> {
     open_api_lockfile(token_shadow_lock(), None, true)
@@ -57,11 +60,28 @@ fn write_file(data: HashMap<Authid, String>) -> Result<(), Error> {
 fn refresh_cache_if_file_changed() -> bool {
     let now = epoch_i64();
 
-    // Best-effort refresh under write lock.
+    // Fast path: cache is fresh if shared-gen matches and TTL not expired.
+    if let (Some(cache), Some(shared_gen_read)) =
+        (TOKEN_SECRET_CACHE.try_read(), token_shadow_shared_gen())
+    {
+        if cache.shared_gen == shared_gen_read
+            && cache
+                .last_checked
+                .is_some_and(|last| now >= last && (now - last) < TOKEN_SECRET_CACHE_TTL_SECS)
+        {
+            return true;
+        }
+        // read lock drops here
+    } else {
+        return false;
+    }
+
+    // Slow path: best-effort refresh under write lock.
     let Some(mut cache) = TOKEN_SECRET_CACHE.try_write() else {
         return false;
     };
 
+    // Re-read generation after acquiring the lock (may have changed meanwhile).
     let Some(shared_gen_now) = token_shadow_shared_gen() else {
         return false;
     };
@@ -72,6 +92,14 @@ fn refresh_cache_if_file_changed() -> bool {
         cache.shared_gen = shared_gen_now;
     }
 
+    // TTL check again after acquiring the lock
+    if cache
+        .last_checked
+        .is_some_and(|last| now >= last && (now - last) < TOKEN_SECRET_CACHE_TTL_SECS)
+    {
+        return true;
+    }
+
     // Stat the file to detect manual edits.
     let Ok((new_mtime, new_len)) = shadow_mtime_len() else {
         return false;
-- 
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-datacenter-manager v3 1/2] pdm-config: implement token.shadow generation

[Samuel Rufinatscha]

PDM depends on the shared proxmox/proxmox-access-control crate for
token.shadow handling, which expects the product to provide a
cross-process invalidation signal so it can safely cache verified API
token secrets and invalidate them when token.shadow is changed.

This patch

* adds a token_shadow_generation to PDM’s shared-memory
ConfigVersionCache
* implements proxmox_access_control::init::AccessControlConfig
for pdm_config::AccessControlConfig, which
   - delegates roles/privs/path checks to the existing
pdm_api_types::AccessControlConfig implementation
   - implements the shadow cache generation trait functions
* switches the AccessControlConfig init paths (server + CLI) to use
pdm_config::AccessControlConfig instead of
pdm_api_types::AccessControlConfig

This patch is part of the series which fixes bug #7017 [1].

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

Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>
---
 cli/admin/src/main.rs                       |  2 +-
 lib/pdm-config/Cargo.toml                   |  1 +
 lib/pdm-config/src/access_control_config.rs | 73 +++++++++++++++++++++
 lib/pdm-config/src/config_version_cache.rs  | 18 +++++
 lib/pdm-config/src/lib.rs                   |  2 +
 server/src/acl.rs                           |  3 +-
 6 files changed, 96 insertions(+), 3 deletions(-)
 create mode 100644 lib/pdm-config/src/access_control_config.rs

diff --git a/cli/admin/src/main.rs b/cli/admin/src/main.rs
index f698fa2..916c633 100644
--- a/cli/admin/src/main.rs
+++ b/cli/admin/src/main.rs
@@ -19,7 +19,7 @@ fn main() {
     proxmox_product_config::init(api_user, priv_user);
 
     proxmox_access_control::init::init(
-        &pdm_api_types::AccessControlConfig,
+        &pdm_config::AccessControlConfig,
         pdm_buildcfg::configdir!("/access"),
     )
     .expect("failed to setup access control config");
diff --git a/lib/pdm-config/Cargo.toml b/lib/pdm-config/Cargo.toml
index d39c2ad..19781d2 100644
--- a/lib/pdm-config/Cargo.toml
+++ b/lib/pdm-config/Cargo.toml
@@ -13,6 +13,7 @@ once_cell.workspace = true
 openssl.workspace = true
 serde.workspace = true
 
+proxmox-access-control.workspace = true
 proxmox-config-digest = { workspace = true, features = [ "openssl" ] }
 proxmox-http = { workspace = true, features = [ "http-helpers" ] }
 proxmox-ldap = { workspace = true, features = [ "types" ]}
diff --git a/lib/pdm-config/src/access_control_config.rs b/lib/pdm-config/src/access_control_config.rs
new file mode 100644
index 0000000..6f2e6b3
--- /dev/null
+++ b/lib/pdm-config/src/access_control_config.rs
@@ -0,0 +1,73 @@
+// e.g. in src/main.rs or server::context mod, wherever convenient
+
+use anyhow::Error;
+use pdm_api_types::{Authid, Userid};
+use proxmox_section_config::SectionConfigData;
+use std::collections::HashMap;
+
+pub struct AccessControlConfig;
+
+impl proxmox_access_control::init::AccessControlConfig for AccessControlConfig {
+    fn privileges(&self) -> &HashMap<&str, u64> {
+        pdm_api_types::AccessControlConfig.privileges()
+    }
+
+    fn roles(&self) -> &HashMap<&str, (u64, &str)> {
+        pdm_api_types::AccessControlConfig.roles()
+    }
+
+    fn is_superuser(&self, auth_id: &Authid) -> bool {
+        pdm_api_types::AccessControlConfig.is_superuser(auth_id)
+    }
+
+    fn is_group_member(&self, user_id: &Userid, group: &str) -> bool {
+        pdm_api_types::AccessControlConfig.is_group_member(user_id, group)
+    }
+
+    fn role_admin(&self) -> Option<&str> {
+        pdm_api_types::AccessControlConfig.role_admin()
+    }
+
+    fn role_no_access(&self) -> Option<&str> {
+        pdm_api_types::AccessControlConfig.role_no_access()
+    }
+
+    fn init_user_config(&self, config: &mut SectionConfigData) -> Result<(), Error> {
+        pdm_api_types::AccessControlConfig.init_user_config(config)
+    }
+
+    fn acl_audit_privileges(&self) -> u64 {
+        pdm_api_types::AccessControlConfig.acl_audit_privileges()
+    }
+
+    fn acl_modify_privileges(&self) -> u64 {
+        pdm_api_types::AccessControlConfig.acl_modify_privileges()
+    }
+
+    fn check_acl_path(&self, path: &str) -> Result<(), Error> {
+        pdm_api_types::AccessControlConfig.check_acl_path(path)
+    }
+
+    fn allow_partial_permission_match(&self) -> bool {
+        pdm_api_types::AccessControlConfig.allow_partial_permission_match()
+    }
+
+    fn cache_generation(&self) -> Option<usize> {
+        pdm_api_types::AccessControlConfig.cache_generation()
+    }
+
+    fn increment_cache_generation(&self) -> Result<(), Error> {
+        pdm_api_types::AccessControlConfig.increment_cache_generation()
+    }
+
+    fn token_shadow_cache_generation(&self) -> Option<usize> {
+        crate::ConfigVersionCache::new()
+            .ok()
+            .map(|c| c.token_shadow_generation())
+    }
+
+    fn increment_token_shadow_cache_generation(&self) -> Result<usize, Error> {
+        let c = crate::ConfigVersionCache::new()?;
+        Ok(c.increase_token_shadow_generation())
+    }
+}
diff --git a/lib/pdm-config/src/config_version_cache.rs b/lib/pdm-config/src/config_version_cache.rs
index 36a6a77..933140c 100644
--- a/lib/pdm-config/src/config_version_cache.rs
+++ b/lib/pdm-config/src/config_version_cache.rs
@@ -27,6 +27,8 @@ struct ConfigVersionCacheDataInner {
     traffic_control_generation: AtomicUsize,
     // Tracks updates to the remote/hostname/nodename mapping cache.
     remote_mapping_cache: AtomicUsize,
+    // Token shadow (token.shadow) generation/version.
+    token_shadow_generation: AtomicUsize,
     // Add further atomics here
 }
 
@@ -172,4 +174,20 @@ impl ConfigVersionCache {
             .fetch_add(1, Ordering::Relaxed)
             + 1
     }
+
+    /// Returns the token shadow generation number.
+    pub fn token_shadow_generation(&self) -> usize {
+        self.shmem
+            .data()
+            .token_shadow_generation
+            .load(Ordering::Acquire)
+    }
+
+    /// Increase the token shadow generation number.
+    pub fn increase_token_shadow_generation(&self) -> usize {
+        self.shmem
+            .data()
+            .token_shadow_generation
+            .fetch_add(1, Ordering::AcqRel)
+    }
 }
diff --git a/lib/pdm-config/src/lib.rs b/lib/pdm-config/src/lib.rs
index 4c49054..a15a006 100644
--- a/lib/pdm-config/src/lib.rs
+++ b/lib/pdm-config/src/lib.rs
@@ -9,6 +9,8 @@ pub mod remotes;
 pub mod setup;
 pub mod views;
 
+mod access_control_config;
+pub use access_control_config::AccessControlConfig;
 mod config_version_cache;
 pub use config_version_cache::ConfigVersionCache;
 
diff --git a/server/src/acl.rs b/server/src/acl.rs
index f421814..e6e007b 100644
--- a/server/src/acl.rs
+++ b/server/src/acl.rs
@@ -1,6 +1,5 @@
 pub(crate) fn init() {
-    static ACCESS_CONTROL_CONFIG: pdm_api_types::AccessControlConfig =
-        pdm_api_types::AccessControlConfig;
+    static ACCESS_CONTROL_CONFIG: pdm_config::AccessControlConfig = pdm_config::AccessControlConfig;
 
     proxmox_access_control::init::init(&ACCESS_CONTROL_CONFIG, pdm_buildcfg::configdir!("/access"))
         .expect("failed to setup access control config");
-- 
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-datacenter-manager v3 2/2] docs: document API token-cache TTL effects

[Samuel Rufinatscha]

Documents the effects of the added API token-cache in the
proxmox-access-control crate. This patch is part of the
series that fixes bug #7017 [1].

This patch is part of the series which 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 v2 to v3:

* Reword documentation warning for clarity.

 docs/access-control.rst | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/docs/access-control.rst b/docs/access-control.rst
index adf26cd..18e57a2 100644
--- a/docs/access-control.rst
+++ b/docs/access-control.rst
@@ -47,6 +47,10 @@ place of the user ID (``user@realm``) and the user password, respectively.
 The API token is passed from the client to the server by setting the ``Authorization`` HTTP header
 with method ``PDMAPIToken`` to the value ``TOKENID:TOKENSECRET``.
 
+.. WARNING:: Direct/manual edits to ``token.shadow`` may take up to 60 seconds (or
+   longer in edge cases) to take effect due to caching. Restart services for
+   immediate effect of manual edits.
+
 .. _access_control:
 
 Access Control
-- 
2.47.3



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