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 1F1DB1FF187 for ; Fri, 2 Jan 2026 17:07:42 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8E53FEB75; Fri, 2 Jan 2026 17:08:52 +0100 (CET) From: Samuel Rufinatscha To: pbs-devel@lists.proxmox.com Date: Fri, 2 Jan 2026 17:07:46 +0100 Message-ID: <20260102160750.285157-8-s.rufinatscha@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260102160750.285157-1-s.rufinatscha@proxmox.com> References: <20260102160750.285157-1-s.rufinatscha@proxmox.com> MIME-Version: 1.0 X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1767370060840 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.233 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 v3 3/4] 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 is part of the series which 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. Changes from v2 to v3: * Cache now tracks last_checked (epoch seconds). * Simplified refresh_cache_if_file_changed, removed FILE_GENERATION logic * On first load, initializes file metadata and keeps empty cache. proxmox-access-control/src/token_shadow.rs | 129 ++++++++++++++++++++- 1 file changed, 123 insertions(+), 6 deletions(-) diff --git a/proxmox-access-control/src/token_shadow.rs b/proxmox-access-control/src/token_shadow.rs index 895309d2..f30c8ed5 100644 --- a/proxmox-access-control/src/token_shadow.rs +++ b/proxmox-access-control/src/token_shadow.rs @@ -1,5 +1,8 @@ use std::collections::HashMap; +use std::fs; +use std::io::ErrorKind; use std::sync::LazyLock; +use std::time::SystemTime; use anyhow::{bail, format_err, Error}; use parking_lot::RwLock; @@ -7,6 +10,7 @@ use serde_json::{from_value, Value}; use proxmox_auth_api::types::Authid; use proxmox_product_config::{open_api_lockfile, replace_config, ApiLockGuard}; +use proxmox_time::epoch_i64; use crate::init::access_conf; use crate::init::impl_feature::{token_shadow, token_shadow_lock}; @@ -20,6 +24,9 @@ static TOKEN_SECRET_CACHE: LazyLock> = LazyLock::new RwLock::new(ApiTokenSecretCache { secrets: HashMap::new(), shared_gen: 0, + file_mtime: None, + file_len: None, + last_checked: None, }) }); @@ -45,6 +52,63 @@ 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 now = epoch_i64(); + + // Best-effort refresh under write lock. + let Some(mut cache) = TOKEN_SECRET_CACHE.try_write() else { + return false; + }; + + let Some(shared_gen_now) = token_shadow_shared_gen() else { + return false; + }; + + // If another process bumped the generation, we don't know what changed -> clear cache + if cache.shared_gen != shared_gen_now { + invalidate_cache_state(&mut cache); + cache.shared_gen = shared_gen_now; + } + + // Stat the file to detect manual edits. + let Ok((new_mtime, new_len)) = shadow_mtime_len() else { + return false; + }; + + // Initialize file stats if we have no prior state. + if cache.last_checked.is_none() { + cache.secrets.clear(); // ensure cache is empty on first load + cache.file_mtime = new_mtime; + cache.file_len = new_len; + cache.last_checked = Some(now); + return true; + } + + // No change detected. + if cache.file_mtime == new_mtime && cache.file_len == new_len { + cache.last_checked = Some(now); + return true; + } + + // Manual edit detected -> invalidate cache and update stat. + cache.secrets.clear(); + cache.file_mtime = new_mtime; + cache.file_len = new_len; + cache.last_checked = Some(now); + + // Best-effort propagation to other processes + update local view. + if let Some(shared_gen_new) = bump_token_shadow_shared_gen() { + cache.shared_gen = shared_gen_new; + } else { + // Do not fail: local cache is already safe as we cleared it above. + // Keep local shared_gen as-is to avoid repeated failed attempts. + } + + 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() { @@ -52,7 +116,7 @@ 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(()); } @@ -84,12 +148,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(()) } @@ -102,11 +169,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(()) } @@ -128,6 +198,12 @@ struct ApiTokenSecretCache { secrets: HashMap, /// Shared generation to detect mutations of the underlying token.shadow file. shared_gen: usize, + // shadow file mtime to detect changes + file_mtime: Option, + // shadow file length to detect changes + file_len: Option, + // last time the file metadata was checked + last_checked: Option, } /// Cached secret. @@ -187,7 +263,13 @@ fn cache_try_secret_matches(tokenid: &Authid, secret: &str) -> bool { eq && gen2 == cache_gen } -fn apply_api_mutation(tokenid: &Authid, new_secret: Option<&str>) { +fn apply_api_mutation( + tokenid: &Authid, + new_secret: Option<&str>, + pre_write_meta: (Option, Option), +) { + let now = epoch_i64(); + // Signal cache invalidation to other processes (best-effort). let new_shared_gen = bump_token_shadow_shared_gen(); @@ -203,6 +285,13 @@ fn apply_api_mutation(tokenid: &Authid, new_secret: Option<&str>) { // Update to the post-mutation generation. cache.shared_gen = gen; + // If our cached file metadata does not match the on-disk state before our write, + // we likely missed an external/manual edit. We can no longer trust any cached secrets. + let (pre_mtime, pre_len) = pre_write_meta; + if cache.file_mtime != pre_mtime || cache.file_len != pre_len { + cache.secrets.clear(); + } + // Apply the new mutation. match new_secret { Some(secret) => { @@ -217,6 +306,20 @@ fn apply_api_mutation(tokenid: &Authid, new_secret: Option<&str>) { cache.secrets.remove(tokenid); } } + + // Update our view of the file metadata to the post-write state (best-effort). + // (If this fails, drop local cache so callers fall back to slow path until refreshed.) + match shadow_mtime_len() { + Ok((mtime, len)) => { + cache.file_mtime = mtime; + cache.file_len = len; + cache.last_checked = Some(now); + } + Err(_) => { + // If we cannot validate state, do not trust cache. + invalidate_cache_state(&mut cache); + } + } } /// Get the current shared generation. @@ -226,10 +329,24 @@ fn token_shadow_shared_gen() -> Option { /// Bump and return the new shared generation. fn bump_token_shadow_shared_gen() -> Option { - access_conf().increment_token_shadow_cache_generation().ok().map(|prev| prev + 1) + access_conf() + .increment_token_shadow_cache_generation() + .ok() + .map(|prev| prev + 1) } /// Invalidates the cache state and only keeps the shared generation. fn invalidate_cache_state(cache: &mut ApiTokenSecretCache) { cache.secrets.clear(); -} \ No newline at end of file + cache.file_mtime = None; + cache.file_len = None; + cache.last_checked = None; +} + +fn shadow_mtime_len() -> Result<(Option, Option), Error> { + match fs::metadata(token_shadow()) { + 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