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