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 4D3A71FF139 for ; Tue, 10 Feb 2026 13:53:54 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2E791356; Tue, 10 Feb 2026 13:54:38 +0100 (CET) Message-ID: <1f0a4451-a767-46d2-976d-7c7cef86cd9b@proxmox.com> Date: Tue, 10 Feb 2026 13:54:01 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [pbs-devel] [PATCH proxmox-backup v4 2/4] pbs-config: cache verified API token secrets To: Proxmox Backup Server development discussion , Samuel Rufinatscha References: <20260121151408.731516-1-s.rufinatscha@proxmox.com> <20260121151408.731516-3-s.rufinatscha@proxmox.com> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <20260121151408.731516-3-s.rufinatscha@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1770727957701 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.047 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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: 7YJ7SSXQ7CXORWLXFPT4OVKQL42VOEEN X-Message-ID-Hash: 7YJ7SSXQ7CXORWLXFPT4OVKQL42VOEEN X-MailFrom: c.ebner@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: one suggestion below On 1/21/26 4:13 PM, Samuel Rufinatscha wrote: > 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. > > Signed-off-by: Samuel Rufinatscha > --- > 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 + > pbs-config/Cargo.toml | 1 + > pbs-config/src/token_shadow.rs | 160 ++++++++++++++++++++++++++++++++- > 3 files changed, 159 insertions(+), 3 deletions(-) > > diff --git a/Cargo.toml b/Cargo.toml > index 0da18383..aed66fe3 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..d5aa5de2 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> = 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"), > } > } > @@ -75,13 +107,15 @@ 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(()) > } > > @@ -91,11 +125,131 @@ 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(()) > } > + > +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, > +} > + > +/// 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_and_set_gen(&mut cache, 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. > +/// > +/// 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: BackupLockGuard, 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 { > + invalidate_cache_state_and_set_gen(&mut cache, 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) { > + invalidate_cache_state_and_set_gen(&mut cache, current_gen); > + return; > + } > + > + // Update to the post-mutation generation. > + cache.shared_gen = current_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 { > + crate::ConfigVersionCache::new() > + .ok() > + .map(|cvc| cvc.token_shadow_generation()) > +} > + > +/// Bump and return the new shared generation. > +fn bump_token_shadow_shared_gen() -> Option { > + crate::ConfigVersionCache::new() > + .ok() > + .map(|cvc| cvc.increase_token_shadow_generation() + 1) > +} > + > +/// Invalidates local cache contents and sets/updates the cached generation. > +fn invalidate_cache_state_and_set_gen(cache: &mut ApiTokenSecretCache, gen: usize) { > + cache.secrets.clear(); > + cache.shared_gen = gen; > +} above function operates on the chache, so why not make it a method thereof? And also bundle the generation bumps, so they might not be forgotten. Something along the lines of the following diff on top of this patch: diff --git a/pbs-config/src/token_shadow.rs b/pbs-config/src/token_shadow.rs index d5aa5de28..a8104f142 100644 --- a/pbs-config/src/token_shadow.rs +++ b/pbs-config/src/token_shadow.rs @@ -136,6 +136,11 @@ pub fn delete_secret(tokenid: &Authid) -> Result<(), Error> { Ok(()) } +/// 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 @@ -147,9 +152,22 @@ struct ApiTokenSecretCache { shared_gen: usize, } -/// Cached secret. -struct CachedSecret { - secret: String, +impl ApiTokenSecretCache { + /// Invalidates local cache contents and sets/updates the cached generation. + fn invalidate_state_and_set_gen(&mut self, gen: usize) { + self.secrets.clear(); + self.shared_gen = gen; + } + + fn insert_and_set_gen(&mut self, tokenid: Authid, secret: CachedSecret, gen: usize) { + self.secrets.insert(tokenid.clone(), secret); + self.shared_gen = gen; + } + + 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) { @@ -163,12 +181,12 @@ fn cache_try_insert_secret(tokenid: Authid, secret: String, shared_gen_before: u // If this process missed a generation bump, its cache is stale. if cache.shared_gen != shared_gen_now { - invalidate_cache_state_and_set_gen(&mut cache, shared_gen_now); + cache.invalidate_state_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.secrets.insert(tokenid, CachedSecret { secret }); + cache.insert_and_set_gen(tokenid, CachedSecret { secret }, shared_gen_now); } } @@ -204,33 +222,24 @@ fn apply_api_mutation(_guard: BackupLockGuard, tokenid: &Authid, new_secret: Opt // If we cannot get the current generation, we cannot trust the cache let Some(current_gen) = token_shadow_shared_gen() else { - invalidate_cache_state_and_set_gen(&mut cache, 0); + cache.invalidate_state_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) { - invalidate_cache_state_and_set_gen(&mut cache, current_gen); + cache.invalidate_state_and_set_gen(current_gen); return; } - // Update to the post-mutation generation. - cache.shared_gen = current_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); + 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), } } @@ -248,8 +257,3 @@ fn bump_token_shadow_shared_gen() -> Option { .map(|cvc| cvc.increase_token_shadow_generation() + 1) } -/// Invalidates local cache contents and sets/updates the cached generation. -fn invalidate_cache_state_and_set_gen(cache: &mut ApiTokenSecretCache, gen: usize) { - cache.secrets.clear(); - cache.shared_gen = gen; -}