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 9C3BE1FF1A6 for ; Fri, 5 Dec 2025 15:04:11 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3B1931F6E5; Fri, 5 Dec 2025 15:04:39 +0100 (CET) Mime-Version: 1.0 Date: Fri, 05 Dec 2025 15:04:34 +0100 Message-Id: To: "Samuel Rufinatscha" X-Mailer: aerc 0.20.0 References: <20251205132559.197434-1-s.rufinatscha@proxmox.com> <20251205132559.197434-2-s.rufinatscha@proxmox.com> In-Reply-To: <20251205132559.197434-2-s.rufinatscha@proxmox.com> From: "Shannon Sterz" X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1764943426997 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.060 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 POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes 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 Subject: Re: [pbs-devel] [PATCH proxmox-backup 1/3] pbs-config: cache verified API token secrets X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Cc: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" On Fri Dec 5, 2025 at 2:25 PM CET, Samuel Rufinatscha wrote: > 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 #6049 [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 partly fixes bug #6049 [1]. > > [1] https://bugzilla.proxmox.com/show_bug.cgi?id=7017 > > Signed-off-by: Samuel Rufinatscha > --- > pbs-config/src/token_shadow.rs | 58 +++++++++++++++++++++++++++++++++- > 1 file changed, 57 insertions(+), 1 deletion(-) > > diff --git a/pbs-config/src/token_shadow.rs b/pbs-config/src/token_shadow.rs > index 640fabbf..47aa2fc2 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::RwLock; > > use anyhow::{bail, format_err, Error}; > +use once_cell::sync::OnceCell; > use serde::{Deserialize, Serialize}; > use serde_json::{from_value, Value}; > > @@ -13,6 +15,13 @@ 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: OnceCell> = OnceCell::new(); any reason you are using a once cell with a cutom get_or_init function instead of a simple `LazyCell` [1] here? seems to me that this would be the more appropriate type here? similar question for the proxmox-access-control portion of this series. [1]: https://doc.rust-lang.org/std/cell/struct.LazyCell.html > + > #[derive(Serialize, Deserialize)] > #[serde(rename_all = "kebab-case")] > /// ApiToken id / secret pair > @@ -54,9 +63,25 @@ pub fn verify_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> { > bail!("not an API token ID"); > } > > + // Fast path > + if let Some(cached) = token_secret_cache().read().unwrap().secrets.get(tokenid) { > + // Compare cached secret with provided one using constant time comparison > + if openssl::memcmp::eq(cached.as_bytes(), secret.as_bytes()) { > + // Already verified before > + return Ok(()); > + } > + // Fall through to slow path if secret doesn't match cached one > + } > + > + // Slow path: read file + verify hash > 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)?; > + // Cache the plain secret for future requests > + cache_insert_secret(tokenid.clone(), secret.to_owned()); > + Ok(()) > + } > None => bail!("invalid API token"), > } > } > @@ -82,6 +107,8 @@ fn set_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> { > data.insert(tokenid.clone(), hashed_secret); > write_file(data)?; > > + cache_insert_secret(tokenid.clone(), secret.to_owned()); > + > Ok(()) > } > > @@ -97,5 +124,34 @@ pub fn delete_secret(tokenid: &Authid) -> Result<(), Error> { > data.remove(tokenid); > write_file(data)?; > > + cache_remove_secret(tokenid); > + > 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, > +} > + > +fn token_secret_cache() -> &'static RwLock { > + TOKEN_SECRET_CACHE.get_or_init(|| { > + RwLock::new(ApiTokenSecretCache { > + secrets: HashMap::new(), > + }) > + }) > +} > + > +fn cache_insert_secret(tokenid: Authid, secret: String) { > + let mut cache = token_secret_cache().write().unwrap(); unwrap here could panic if another thread is holding a guard, any reason to not return a result here and bubble up the error instead? > + cache.secrets.insert(tokenid, secret); > +} > + > +fn cache_remove_secret(tokenid: &Authid) { > + let mut cache = token_secret_cache().write().unwrap(); same here and in the following patches (i won't comment on each occurrence there separately.) > + cache.secrets.remove(tokenid); > +} _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel