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 B7ABB1FF13B for ; Wed, 25 Feb 2026 16:44:02 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D3C089956; Wed, 25 Feb 2026 16:44:59 +0100 (CET) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 25 Feb 2026 16:44:23 +0100 Message-Id: To: "Samuel Rufinatscha" Subject: Re: [PATCH proxmox-backup v5 2/4] pbs-config: cache verified API token secrets X-Mailer: aerc 0.20.0 References: <20260217111229.78661-1-s.rufinatscha@proxmox.com> <20260217111229.78661-3-s.rufinatscha@proxmox.com> In-Reply-To: <20260217111229.78661-3-s.rufinatscha@proxmox.com> From: "Shannon Sterz" X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1772034246310 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.970 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.113 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.358 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.659 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: A2UCTIOZSPZ3EIA63M2IZLASNSUP4EN3 X-Message-ID-Hash: A2UCTIOZSPZ3EIA63M2IZLASNSUP4EN3 X-MailFrom: s.sterz@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 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 =E2=80=9Czombie inserts=E2=80=9D 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 =3D "7" > num-traits =3D "0.2" > once_cell =3D "1.3.1" > openssl =3D "0.10.40" > +parking_lot =3D "0.12" > percent-encoding =3D "2.1" > pin-project-lite =3D "0.2" > regex =3D "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 =3D true > nix.workspace =3D true > once_cell.workspace =3D true > openssl.workspace =3D true > +parking_lot.workspace =3D true > regex.workspace =3D true > serde.workspace =3D true > serde_json.workspace =3D 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 =3D pbs_buildcfg::configdir!("/token.shadow.lock")= ; > const CONF_FILE: &str =3D 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 alre= ady been > +/// verified against the hashed values in `token.shadow`. This allows fo= r cheap > +/// subsequent authentications for the same token+secret combination, av= oiding > +/// recomputing the password hash on every request. > +static TOKEN_SECRET_CACHE: LazyLock> =3D Laz= yLock::new(|| { > + RwLock::new(ApiTokenSecretCache { > + secrets: HashMap::new(), > + shared_gen: 0, > + }) > +}); > + > #[derive(Serialize, Deserialize)] > #[serde(rename_all =3D "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 verifi= cation. > + let gen_before =3D token_shadow_shared_gen(); > + > let data =3D read_file()?; > match data.get(tokenid) { > - Some(hashed_secret) =3D> proxmox_sys::crypt::verify_crypt_pw(sec= ret, hashed_secret), > + Some(hashed_secret) =3D> { > + proxmox_sys::crypt::verify_crypt_pw(secret, hashed_secret)?; > + > + // Try to cache only if nothing changed while verifying the = secret. > + if let Some(gen) =3D gen_before { > + cache_try_insert_secret(tokenid.clone(), secret.to_owned= (), gen); > + } > + > + Ok(()) > + } > None =3D> bail!("invalid API token"), > } > } > @@ -75,13 +107,15 @@ fn set_secret(tokenid: &Authid, secret: &str) -> Res= ult<(), Error> { > bail!("not an API token ID"); > } > > - let _guard =3D lock_config()?; > + let guard =3D lock_config()?; > > let mut data =3D read_file()?; > let hashed_secret =3D 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 =3D lock_config()?; > + let guard =3D lock_config()?; > > let mut data =3D 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.sh= adow file. > + shared_gen: usize, > +} > + > +impl ApiTokenSecretCache { > + /// Resets all local cache contents and sets/updates the cached gene= ration. > + fn reset_and_set_gen(&mut self, gen: usize) { > + self.secrets.clear(); > + self.shared_gen =3D gen; > + } > + > + /// Caches a secret and sets/updates the cache generation. > + fn insert_and_set_gen(&mut self, tokenid: Authid, secret: CachedSecr= et, gen: usize) { > + self.secrets.insert(tokenid, secret); > + self.shared_gen =3D 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 =3D gen; > + } > +} > + > +fn cache_try_insert_secret(tokenid: Authid, secret: String, shared_gen_b= efore: usize) { > + let Some(mut cache) =3D TOKEN_SECRET_CACHE.try_write() else { > + return; > + }; > + > + let Some(shared_gen_now) =3D token_shadow_shared_gen() else { > + return; > + }; > + > + // If this process missed a generation bump, its cache is stale. > + if cache.shared_gen !=3D 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 =3D=3D shared_gen_before { > + cache.insert_and_set_gen(tokenid, CachedSecret { secret }, share= d_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 previou= s > +/// generation. > +fn cache_try_secret_matches(tokenid: &Authid, secret: &str) -> bool { > + let Some(cache) =3D TOKEN_SECRET_CACHE.try_read() else { > + return false; > + }; > + let Some(entry) =3D cache.secrets.get(tokenid) else { > + return false; > + }; > + let Some(current_gen) =3D token_shadow_shared_gen() else { > + return false; > + }; > + > + if current_gen =3D=3D cache.shared_gen { > + return openssl::memcmp::eq(entry.secret.as_bytes(), secret.as_by= tes()); 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. > + } > + > + false > +} > + > +fn apply_api_mutation(_guard: BackupLockGuard, tokenid: &Authid, new_sec= ret: Option<&str>) { > + // Signal cache invalidation to other processes (best-effort). > + let bumped_gen =3D bump_token_shadow_shared_gen(); > + > + let mut cache =3D TOKEN_SECRET_CACHE.write(); > + > + // If we cannot get the current generation, we cannot trust the cach= e > + let Some(current_gen) =3D 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 !=3D Some(current_gen) { > + cache.reset_and_set_gen(current_gen); > + return; > + } > + > + // Apply the new mutation. > + match new_secret { > + Some(secret) =3D> { > + let cached_secret =3D CachedSecret { > + secret: secret.to_owned(), > + }; > + cache.insert_and_set_gen(tokenid.clone(), cached_secret, cur= rent_gen); > + } > + None =3D> 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) > +}