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 973D91FF183 for ; Wed, 17 Dec 2025 17:25:00 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id B8D1C9181; Wed, 17 Dec 2025 17:25:43 +0100 (CET) From: Samuel Rufinatscha To: pbs-devel@lists.proxmox.com Date: Wed, 17 Dec 2025 17:25:16 +0100 Message-ID: <20251217162520.486520-6-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: 1765988726806 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.254 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 SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: [pbs-devel] [PATCH proxmox v2 2/3] proxmox-access-control: 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. proxmox-access-control/src/token_shadow.rs | 128 +++++++++++++++++++-- 1 file changed, 116 insertions(+), 12 deletions(-) diff --git a/proxmox-access-control/src/token_shadow.rs b/proxmox-access-control/src/token_shadow.rs index c0285b62..efadce94 100644 --- a/proxmox-access-control/src/token_shadow.rs +++ b/proxmox-access-control/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; @@ -19,10 +22,14 @@ use crate::init::impl_feature::{token_shadow, token_shadow_lock}; 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); // Get exclusive lock fn lock_config() -> Result { @@ -46,6 +53,29 @@ fn write_file(data: HashMap) -> Result<(), Error> { replace_config(token_shadow(), &json) } +/// 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() { @@ -53,12 +83,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) { @@ -66,7 +97,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(()) } @@ -82,12 +118,15 @@ pub 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(()) } @@ -100,11 +139,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(()) } @@ -124,20 +166,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, + }, + ); } } @@ -149,22 +211,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, }, ); } @@ -172,4 +256,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(token_shadow().as_path()) { + 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