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 474C61FF141 for ; Fri, 27 Feb 2026 10:27:46 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D88E3B379; Fri, 27 Feb 2026 10:28:44 +0100 (CET) Message-ID: <159649fd-1b42-453b-b2be-b812a47dc74c@proxmox.com> Date: Fri, 27 Feb 2026 10:28:08 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH proxmox-backup v5 2/4] pbs-config: cache verified API token secrets To: Shannon Sterz References: <20260217111229.78661-1-s.rufinatscha@proxmox.com> <20260217111229.78661-3-s.rufinatscha@proxmox.com> Content-Language: en-US From: Samuel Rufinatscha In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.821 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 KAM_SHORT 0.001 Use of a URL Shortener for very short URL RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 1.158 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.306 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.668 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: 6PYLBLKZAL2Y56VLG4JVSWO2L6MYATZY X-Message-ID-Hash: 6PYLBLKZAL2Y56VLG4JVSWO2L6MYATZY 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 CC: pbs-devel@lists.proxmox.com 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: On 2/25/26 4:44 PM, Shannon Sterz wrote: > On Tue Feb 17, 2026 at 12:12 PM CET, 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. 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 >> * 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 + >> pbs-config/Cargo.toml | 1 + >> pbs-config/src/token_shadow.rs | 167 ++++++++++++++++++++++++++++++++- >> 3 files changed, 166 insertions(+), 3 deletions(-) >> >> diff --git a/Cargo.toml b/Cargo.toml >> index dd8af85f..469538bb 100644 >> --- a/Cargo.toml >> +++ b/Cargo.toml >> @@ -144,6 +144,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..ad766671 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,138 @@ 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(()) >> } >> + >> +/// 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()); > > tiny comment here: if we ever allow secrets to have different lengths > this could panic: > >> This function will panic the current task if a and b do not have the >> same length. >> - https://docs.rs/openssl/latest/openssl/memcmp/fn.eq.html > > might be worth guarding against that or at least documenting that we > expect these to always have the same length. Thanks, yes I agree. Will add a length check! In that case we should invalidate/clear the cache and run the slow path and then cache the secret with the new length. > >> + } >> + >> + 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 { >> + 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 { >> + 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) >> +} >