From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 3610D1FF137 for ; Tue, 17 Feb 2026 12:12:23 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 41D5E3443B; Tue, 17 Feb 2026 12:13:13 +0100 (CET) From: Samuel Rufinatscha To: pbs-devel@lists.proxmox.com Subject: [PATCH proxmox v5 2/4] proxmox-access-control: cache verified API token secrets Date: Tue, 17 Feb 2026 12:12:23 +0100 Message-ID: <20260217111229.78661-7-s.rufinatscha@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260217111229.78661-1-s.rufinatscha@proxmox.com> References: <20260217111229.78661-1-s.rufinatscha@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1771326748343 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.255 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 5TYLTQAQ4YNJKXL7UAIY4VGIWY4ZI5MZ X-Message-ID-Hash: 5TYLTQAQ4YNJKXL7UAIY4VGIWY4ZI5MZ X-MailFrom: s.rufinatscha@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Adds 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. A shared generation counter (via ConfigVersionCache) is used to invalidate caches across processes when token secrets are modified or deleted. This keeps privileged and unprivileged daemons in sync. Signed-off-by: Samuel Rufinatscha --- Changes from v4 to v5: * Rebased * Fix wrong type compilation issue; replaced with ApiLockGuard * Move invalidate_cache_state_and_set_gen into cache object impl rename to reset_and_set_gen * Add additional insert/remove helpers which set/update the generation directly * Clarified the usage of shared generation counter in the commit message Changes from v3 to v4: * Add gen param to invalidate_cache_state() * Validates the generation bump after obtaining write lock in apply_api_mutation * Pass lock to apply_api_mutation * Remove unnecessary gen check cache_try_secret_matches * Adjusted commit message 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. 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). Cargo.toml | 1 + proxmox-access-control/Cargo.toml | 1 + proxmox-access-control/src/token_shadow.rs | 167 ++++++++++++++++++++- 3 files changed, 166 insertions(+), 3 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 6ce4d5ec..d31e4e30 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..ba5fb3f8 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> = LazyLock::new(|| { + RwLock::new(ApiTokenSecretCache { + secrets: HashMap::new(), + shared_gen: 0, + }) +}); + // Get exclusive lock fn lock_config() -> Result { 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"), } } @@ -49,13 +82,15 @@ pub fn set_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> { bail!("not an API token ID"); } - let _guard = lock_config()?; + let guard = lock_config()?; 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(guard, tokenid, Some(secret)); + Ok(()) } @@ -65,12 +100,14 @@ pub fn delete_secret(tokenid: &Authid) -> Result<(), Error> { bail!("not an API token ID"); } - let _guard = lock_config()?; + let guard = lock_config()?; let mut data = read_file()?; data.remove(tokenid); write_file(data)?; + apply_api_mutation(guard, tokenid, None); + Ok(()) } @@ -81,3 +118,127 @@ pub fn generate_and_set_secret(tokenid: &Authid) -> Result { set_secret(tokenid, &secret)?; Ok(secret) } + +/// Cached secret. +struct CachedSecret { + secret: String, +} + +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, + /// Shared generation to detect mutations of the underlying token.shadow file. + shared_gen: usize, +} + +impl ApiTokenSecretCache { + /// Resets all local cache contents and sets/updates the cached generation. + fn reset_and_set_gen(&mut self, gen: usize) { + self.secrets.clear(); + self.shared_gen = gen; + } + + /// Caches a secret and sets/updates the cache generation. + fn insert_and_set_gen(&mut self, tokenid: Authid, secret: CachedSecret, gen: usize) { + self.secrets.insert(tokenid, secret); + self.shared_gen = gen; + } + + /// Evicts a cached secret and sets/updates the cached generation. + fn evict_and_set_gen(&mut self, tokenid: &Authid, gen: usize) { + self.secrets.remove(tokenid); + self.shared_gen = gen; + } +} + +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 { + cache.reset_and_set_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.insert_and_set_gen(tokenid, CachedSecret { secret }, shared_gen_now); + } +} + +/// Tries to match the given token secret against the cached secret. +/// +/// Verifies the generation/version before doing the constant-time +/// comparison to reduce TOCTOU risk. During token rotation or deletion +/// tokens for in-flight requests may still validate against the previous +/// generation. +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 Some(current_gen) = token_shadow_shared_gen() else { + return false; + }; + + if current_gen == cache.shared_gen { + return openssl::memcmp::eq(entry.secret.as_bytes(), secret.as_bytes()); + } + + false +} + +fn apply_api_mutation(_guard: ApiLockGuard, tokenid: &Authid, new_secret: Option<&str>) { + // Signal cache invalidation to other processes (best-effort). + let bumped_gen = bump_token_shadow_shared_gen(); + + let mut cache = TOKEN_SECRET_CACHE.write(); + + // If we cannot get the current generation, we cannot trust the cache + let Some(current_gen) = token_shadow_shared_gen() else { + cache.reset_and_set_gen(0); + return; + }; + + // If we cannot bump the shared generation, or if it changed after + // obtaining the cache write lock, we cannot trust the cache + if bumped_gen != Some(current_gen) { + cache.reset_and_set_gen(current_gen); + return; + } + + // Apply the new mutation. + match new_secret { + Some(secret) => { + let cached_secret = CachedSecret { + secret: secret.to_owned(), + }; + cache.insert_and_set_gen(tokenid.clone(), cached_secret, current_gen); + } + None => cache.evict_and_set_gen(tokenid, current_gen), + } +} + +/// Get the current shared generation. +fn token_shadow_shared_gen() -> Option { + access_conf().token_shadow_cache_generation() +} + +/// Bump and return the new shared generation. +fn bump_token_shadow_shared_gen() -> Option { + access_conf() + .increment_token_shadow_cache_generation() + .ok() + .map(|prev| prev + 1) +} -- 2.47.3