From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 10A1F1FF183 for ; Wed, 17 Dec 2025 17:24:57 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 607118F60; Wed, 17 Dec 2025 17:25:42 +0100 (CET) From: Samuel Rufinatscha To: pbs-devel@lists.proxmox.com Date: Wed, 17 Dec 2025 17:25:13 +0100 Message-ID: <20251217162520.486520-3-s.rufinatscha@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20251217162520.486520-1-s.rufinatscha@proxmox.com> References: <20251217162520.486520-1-s.rufinatscha@proxmox.com> MIME-Version: 1.0 X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1765988726622 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.260 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com] Subject: [pbs-devel] [PATCH proxmox-backup v2 2/3] pbs-config: invalidate token-secret cache on token.shadow changes 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 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" Previously the in-memory token-secret cache was only updated via set_secret() and delete_secret(), so manual edits to token.shadow were not reflected. This patch adds file change detection to the cache. It tracks the mtime and length of token.shadow and clears the in-memory token secret cache whenever these values change. Note, this patch fetches file stats on every request. An TTL-based optimization will be covered in a subsequent patch of the series. This patch partly fixes bug #7017 [1]. [1] https://bugzilla.proxmox.com/show_bug.cgi?id=7017 Signed-off-by: Samuel Rufinatscha --- Changes from v1 to v2: - Add file metadata tracking (file_mtime, file_len) and FILE_GENERATION. - Store file_gen in CachedSecret and verify it against the current FILE_GENERATION to ensure cached entries belong to the current file state. - Add shadow_mtime_len() helper and convert refresh to best-effort (try_write, returns bool). - Pass a pre-write metadata snapshot into apply_api_mutation and clear/bump generation if the cache metadata indicates missed external edits. pbs-config/src/token_shadow.rs | 128 +++++++++++++++++++++++++++++---- 1 file changed, 116 insertions(+), 12 deletions(-) diff --git a/pbs-config/src/token_shadow.rs b/pbs-config/src/token_shadow.rs index ce845e8d..71553aae 100644 --- a/pbs-config/src/token_shadow.rs +++ b/pbs-config/src/token_shadow.rs @@ -1,6 +1,9 @@ use std::collections::HashMap; +use std::fs; +use std::io::ErrorKind; use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::LazyLock; +use std::time::SystemTime; use anyhow::{bail, format_err, Error}; use parking_lot::RwLock; @@ -24,10 +27,14 @@ const CONF_FILE: &str = pbs_buildcfg::configdir!("/token.shadow"); static TOKEN_SECRET_CACHE: LazyLock> = LazyLock::new(|| { RwLock::new(ApiTokenSecretCache { secrets: HashMap::new(), + file_mtime: None, + file_len: None, }) }); /// API mutation generation (set/delete) static API_MUTATION_GENERATION: AtomicU64 = AtomicU64::new(0); +/// External/manual edits generation for the token.shadow file +static FILE_GENERATION: AtomicU64 = AtomicU64::new(0); #[derive(Serialize, Deserialize)] #[serde(rename_all = "kebab-case")] @@ -64,6 +71,29 @@ fn write_file(data: HashMap) -> Result<(), Error> { proxmox_sys::fs::replace_file(CONF_FILE, &json, options, true) } +/// Refreshes the in-memory cache if the on-disk token.shadow file changed. +/// Returns true if the cache is valid to use, false if not. +fn refresh_cache_if_file_changed() -> bool { + let Some(mut cache) = TOKEN_SECRET_CACHE.try_write() else { + return false; // cannot validate external changes -> don't trust cache + }; + + let Ok((new_mtime, new_len)) = shadow_mtime_len() else { + return false; // cannot validate external changes -> don't trust cache + }; + + if cache.file_mtime == new_mtime && cache.file_len == new_len { + return true; + } + + cache.secrets.clear(); + cache.file_mtime = new_mtime; + cache.file_len = new_len; + FILE_GENERATION.fetch_add(1, Ordering::AcqRel); + + true +} + /// Verifies that an entry for given tokenid / API token secret exists pub fn verify_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> { if !tokenid.is_token() { @@ -71,12 +101,13 @@ pub fn verify_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> { } // Fast path - if cache_try_secret_matches(tokenid, secret) { + if refresh_cache_if_file_changed() && cache_try_secret_matches(tokenid, secret) { return Ok(()); } // Slow path snapshot (before expensive work) let api_gen_before = API_MUTATION_GENERATION.load(Ordering::Acquire); + let file_gen_before = FILE_GENERATION.load(Ordering::Acquire); let data = read_file()?; match data.get(tokenid) { @@ -84,7 +115,12 @@ pub fn verify_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> { proxmox_sys::crypt::verify_crypt_pw(secret, hashed_secret)?; // Try to cache only if nothing changed while we verified - cache_try_insert_secret(tokenid.clone(), secret.to_owned(), api_gen_before); + cache_try_insert_secret( + tokenid.clone(), + secret.to_owned(), + api_gen_before, + file_gen_before, + ); Ok(()) } @@ -108,12 +144,15 @@ fn set_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> { let _guard = lock_config()?; + // Capture state BEFORE we write to detect external edits. + let pre_meta = shadow_mtime_len().unwrap_or((None, None)); + 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(tokenid, Some(secret)); + apply_api_mutation(tokenid, Some(secret), pre_meta); Ok(()) } @@ -126,11 +165,14 @@ pub fn delete_secret(tokenid: &Authid) -> Result<(), Error> { let _guard = lock_config()?; + // Capture state BEFORE we write to detect external edits. + let pre_meta = shadow_mtime_len().unwrap_or((None, None)); + let mut data = read_file()?; data.remove(tokenid); write_file(data)?; - apply_api_mutation(tokenid, None); + apply_api_mutation(tokenid, None, pre_meta); Ok(()) } @@ -142,20 +184,40 @@ struct ApiTokenSecretCache { /// `generate_and_set_secret`. Used to avoid repeated /// password-hash computation on subsequent authentications. secrets: HashMap, + // shadow file mtime to detect changes + file_mtime: Option, + // shadow file length to detect changes + file_len: Option, } -/// Cached secret. +/// Cached secret and the file generation it was cached at. struct CachedSecret { secret: String, + file_gen: u64, } -fn cache_try_insert_secret(tokenid: Authid, secret: String, api_gen_snapshot: u64) { +fn cache_try_insert_secret( + tokenid: Authid, + secret: String, + api_gen_snapshot: u64, + file_gen_snapshot: u64, +) { let Some(mut cache) = TOKEN_SECRET_CACHE.try_write() else { return; }; - if API_MUTATION_GENERATION.load(Ordering::Acquire) == api_gen_snapshot { - cache.secrets.insert(tokenid, CachedSecret { secret }); + // Check generations to avoid zombie-inserts + let cur_file_gen = FILE_GENERATION.load(Ordering::Acquire); + let cur_api_gen = API_MUTATION_GENERATION.load(Ordering::Acquire); + + if cur_file_gen == file_gen_snapshot && cur_api_gen == api_gen_snapshot { + cache.secrets.insert( + tokenid, + CachedSecret { + secret, + file_gen: cur_file_gen, + }, + ); } } @@ -167,22 +229,44 @@ fn cache_try_secret_matches(tokenid: &Authid, secret: &str) -> bool { return false; }; - openssl::memcmp::eq(entry.secret.as_bytes(), secret.as_bytes()) + let gen1 = FILE_GENERATION.load(Ordering::Acquire); + if entry.file_gen != gen1 { + return false; + } + + let eq = openssl::memcmp::eq(entry.secret.as_bytes(), secret.as_bytes()); + + let gen2 = FILE_GENERATION.load(Ordering::Acquire); + eq && gen1 == gen2 } -fn apply_api_mutation(tokenid: &Authid, new_secret: Option<&str>) { - // Prevent in-flight verify_secret() from caching results across a mutation. +fn apply_api_mutation( + tokenid: &Authid, + new_secret: Option<&str>, + pre_write_meta: (Option, Option), +) { API_MUTATION_GENERATION.fetch_add(1, Ordering::AcqRel); - // Mutations must be reflected immediately once set/delete returns. let mut cache = TOKEN_SECRET_CACHE.write(); + // If the cache meta doesn't match the file state before the on-disk write, + // external/manual edits happened -> drop everything and bump FILE_GENERATION. + let (pre_mtime, pre_len) = pre_write_meta; + if cache.file_mtime != pre_mtime || cache.file_len != pre_len { + cache.secrets.clear(); + FILE_GENERATION.fetch_add(1, Ordering::AcqRel); + } + + let file_gen = FILE_GENERATION.load(Ordering::Acquire); + + // Apply the API mutation to the cache. match new_secret { Some(secret) => { cache.secrets.insert( tokenid.clone(), CachedSecret { secret: secret.to_owned(), + file_gen, }, ); } @@ -190,4 +274,24 @@ fn apply_api_mutation(tokenid: &Authid, new_secret: Option<&str>) { cache.secrets.remove(tokenid); } } + + // Keep cache metadata aligned if possible. + match shadow_mtime_len() { + Ok((mtime, len)) => { + cache.file_mtime = mtime; + cache.file_len = len; + } + Err(_) => { + cache.file_mtime = None; + cache.file_len = None; + } + } +} + +fn shadow_mtime_len() -> Result<(Option, Option), Error> { + match fs::metadata(CONF_FILE) { + Ok(meta) => Ok((meta.modified().ok(), Some(meta.len()))), + Err(e) if e.kind() == ErrorKind::NotFound => Ok((None, None)), + Err(e) => Err(e.into()), + } } -- 2.47.3 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel