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 87FB91FF14C for ; Fri, 15 May 2026 11:20:14 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 67BBD12EDA; Fri, 15 May 2026 11:20:14 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Fri, 15 May 2026 11:19:40 +0200 Message-Id: Subject: Re: [PATCH datacenter-manager 1/4] add persistent, generic, namespaced key-value cache implementation From: "Lukas Wagner" To: "Thomas Lamprecht" , "Lukas Wagner" X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20260513135457.573414-1-l.wagner@proxmox.com> <20260513135457.573414-2-l.wagner@proxmox.com> <20260515090637.950992-1-t.lamprecht@proxmox.com> In-Reply-To: <20260515090637.950992-1-t.lamprecht@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1778836774395 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.054 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 Message-ID-Hash: AI73FX23AMEIKMJVNTWWZXJHYF3ISKRL X-Message-ID-Hash: AI73FX23AMEIKMJVNTWWZXJHYF3ISKRL X-MailFrom: l.wagner@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: pdm-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Fri May 15, 2026 at 11:06 AM CEST, Thomas Lamprecht wrote: > On Wed, 13 May 2026 15:54:54 +0200, Lukas Wagner wrote: >> diff --git a/server/src/namespaced_cache.rs b/server/src/namespaced_cach= e.rs >> new file mode 100644 >> --- /dev/null >> +++ b/server/src/namespaced_cache.rs >> @@ -0,0 +1,742 @@ > [...] >> +/// A generic, namespaced cache with optional value expiration. >> +pub struct NamespacedCache { >> + base_directory: PathBuf, >> + file_options: CreateOptions, >> + dir_options: CreateOptions, >> +} >> + >> +impl NamespacedCache { >> + /// Create a new cache instance. >> + /// >> + /// Cache entries will be persisted in the provided `base_directory= `. >> + /// `dir_options` are the [`CreateOptions`] used the namespace dire= ctories, while `file_options` > > nit: s/used the/used for the/ in the doc. > Fixed for the next revision, thanks! >> + /// are the ones for persisted cache entries. >> + pub fn new>( >> + base_directory: P, >> + dir_options: CreateOptions, >> + file_options: CreateOptions, >> + ) -> Self { > > tiny nit: Struct and new constructr disagree on parameter order > (file_options/dir_options vs. dir_options/file_options), but both are > the same type. > Fixed for the next revision, thanks! >> +/// A writable cache namespace (blocking interface). >> +pub struct BlockingWritableCacheNamespace { >> + inner: WriteableInner, >> +} >> + >> +/// A writable cache namespace (async interface). >> +pub struct WritableCacheNamespace { >> + inner: Arc, >> +} >> + >> +struct WriteableInner { > > nit: public types are `Writable`, inner is `Writeable` - unify the > spelling here (e.g. rename to `WritableInner`). > Fixed for the next revision, thanks! > [...] >> +impl BlockingWritableCacheNamespace { >> + /// Remote a cache entry. > > nit: s/Remote/Remove/ (and on `WritableCacheNamespace::remove`). Fixed for the next revision, thanks! > > [...] >> +// Make sure that an identifier is safe to use as a cache key. >> +// >> +// At the moment, this only checks for '../', as to ensure that the bas= e directory >> +// is not escaped. >> +fn ensure_valid_key(key: &str) -> Result<(), CacheError> { >> + if key.contains("../") { >> + return Err(CacheError::InvalidKey(key.into())); >> + } >> + >> + Ok(()) >> +} >> + >> +// Make sure that an identifier is safe to use as a namespace. >> +// >> +// At the moment, this ensures that there is no '../' in the namespace,= as well as that the >> +// identifier does not start with '/'. >> +fn ensure_valid_namespace(namespace: &str) -> Result<(), CacheError> { >> + if namespace.contains("../") || namespace.starts_with("/") { >> + return Err(CacheError::InvalidNamespace(namespace.into())); >> + } >> + >> + Ok(()) >> +} > > Something that I missed for the RRD fix back then w.r.t. "ensure that > the base directory is not escaped", but it's not enough to only reject > the `../` substring, as with that a couple of inputs still slip through: > > - a key like `/etc/foo` passes; joining an absolute path replaces the > prefix, so the write ends up at `/etc/foo.json` outside the cache. > - a namespace of just `".."` passes too (no `../` substring, no leading > `/`), and then `base.join("..")` walks one level up. > > No caller hits either today (keys are hardcoded and namespaces always go > through `format_remote_namespace`), but allowing only a known-good > character set (like the SAFE_ID regex) would match the stated intent > more directly than blocking specific substrings. Ack, will switch to a regex-based verification in the next revision! Thanks for the feedback!