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

["Lukas Wagner" <l.wagner@proxmox.com>]

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_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.
>

Fixed for the next revision, thanks!

>> +    /// 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.
>
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<WriteableInner>,
>> +}
>> +
>> +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 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.

Ack, will switch to a regex-based verification in the next revision!


Thanks for the feedback!