Re: [PATCH datacenter-manager 1/4] add persistent, generic, namespaced key-value cache implementation

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On Wed, 13 May 2026 15:54:54 +0200, Lukas Wagner wrote:
> diff --git a/server/src/namespaced_cache.rs b/server/src/namespaced_cache.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 directories, while `file_options`

nit: s/used the/used for the/ in the doc.

> +    /// are the ones for persisted cache entries.
> +    pub fn new<P: Into<PathBuf>>(
> +        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.

> +/// A writable cache namespace (blocking interface).
> +pub struct BlockingWritableCacheNamespace {
> +    inner: WriteableInner,
> +}
> +
> +/// A writable cache namespace (async interface).
> +pub struct WritableCacheNamespace {
> +    inner: Arc<WriteableInner>,
> +}
> +
> +struct WriteableInner {

nit: public types are `Writable`, inner is `Writeable` - unify the
spelling here (e.g.  rename to `WritableInner`).

[...]
> +impl BlockingWritableCacheNamespace {
> +    /// Remote a cache entry.

nit: s/Remote/Remove/ (and on `WritableCacheNamespace::remove`).

[...]
> +// 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 base 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.