Re: [pbs-devel] [PATCH proxmox-backup v3 2/4] pbs-config: cache verified API token secrets

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

Quoting Samuel Rufinatscha (2026-01-16 16:13:17)
> On 1/14/26 11:44 AM, Fabian Grünbichler wrote:
> > On January 2, 2026 5:07 pm, Samuel Rufinatscha wrote:
> >> Currently, every token-based API request reads the token.shadow file and
> >> runs the expensive password hash verification for the given token
> >> secret. This shows up as a hotspot in /status profiling (see
> >> bug #7017 [1]).
> >>
> >> This patch introduces an in-memory cache of successfully verified token
> >> secrets. Subsequent requests for the same token+secret combination only
> >> perform a comparison using openssl::memcmp::eq and avoid re-running the
> >> password hash. The cache is updated when a token secret is set and
> >> cleared when a token is deleted. Note, this does NOT include manual
> >> config changes, which will be covered in a subsequent patch.
> >>
> >> 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 <s.rufinatscha@proxmox.com>
> >> ---

[..]

> >> +
> >> +// Tries to match the given token secret against the cached secret.
> >> +// Checks the generation before and after the constant-time compare to avoid a
> >> +// TOCTOU window. If another process rotates/deletes a token while we're validating
> >> +// the cached secret, the generation will change, and we
> >> +// must not trust the cache for this request.
> >> +fn cache_try_secret_matches(tokenid: &Authid, secret: &str) -> bool {
> >> +    let Some(cache) = TOKEN_SECRET_CACHE.try_read() else {
> >> +        return false;
> >> +    };
> >> +    let Some(entry) = cache.secrets.get(tokenid) else {
> >> +        return false;
> >> +    };
> >> +
> >> +    let cache_gen = cache.shared_gen;
> >> +
> >> +    let Some(gen1) = token_shadow_shared_gen() else {
> >> +        return false;
> >> +    };
> >> +    if gen1 != cache_gen {
> >> +        return false;
> >> +    }
> >> +
> >> +    let eq = openssl::memcmp::eq(entry.secret.as_bytes(), secret.as_bytes());
> > 
> > should we invalidate the cache here for this particular authid in case
> > of a mismatch, to avoid making brute forcing too easy/cheap?
> >
> 
> We are not doing a cheap reject, in mismatch we do still fall through to
> verify_crypt_pw(). Evicting on mismatch could however enable cache
> thrashing where wrong secrets for a known tokenid would evict cached
> entries. So I think we should not invalidate here on mismatch.

forgot this part here, sorry. you are right, this *should* be okay. I do think
the second generation check there serves no purpose though. the token config
can change at any point after we've validated the secret using the old state,
there is nothing we can do about that, and it's totally fine to accept a token
that is modified at exactly the same moment, even if that same token wouldn't
be valid 2 seconds later..

there has to be a point where we have to say "this token is valid", and at the
point of memcmp here we have already:
- verified we don't need to reload the file
- verified we didn't have any API changes to the token config
- verified that the secret matches what we have cached

redoing the first two changes after that point doesn't protect us against
changes afterwards either, so we might as well not do that extra work that
doesn't give us any extra safety guarantees anyway..

> 
> >> +    let Some(gen2) = token_shadow_shared_gen() else {
> >> +        return false;
> >> +    };
> >> +
> >> +    eq && gen2 == cache_gen
> >> +}
> >> +
> >> +fn apply_api_mutation(tokenid: &Authid, new_secret: Option<&str>) {
> >> +    // Signal cache invalidation to other processes (best-effort).
> >> +    let new_shared_gen = bump_token_shadow_shared_gen();
> >> +
> >> +    let mut cache = TOKEN_SECRET_CACHE.write();

because I mentioned switching those two around - this actually requires more
thought I think..

right now, calling apply_api_mutation happens under a lock, but there are other
calls that bump the generation, so this is actually racy here. OTOH, bumping
the generation before locking the cache means faster cache invalidation..

maybe we should re-verify the generation after obtaining the lock? and maybe
make apply_api_mutation consume the shadow config file lock, to ensure it's
only called while that lock is being held?

> >> +
> >> +    // If we cannot read/bump the shared generation, we cannot safely trust the cache.
> >> +    let Some(gen) = new_shared_gen else {
> >> +        invalidate_cache_state(&mut cache);
> >> +        cache.shared_gen = 0;
> >> +        return;
> >> +    };
> >> +
> >> +    // Update to the post-mutation generation.
> >> +    cache.shared_gen = gen;
> >> +
> >> +    // Apply the new mutation.
> >> +    match new_secret {
> >> +        Some(secret) => {
> >> +            cache.secrets.insert(
> >> +                tokenid.clone(),
> >> +                CachedSecret {
> >> +                    secret: secret.to_owned(),
> >> +                },
> >> +            );
> >> +        }
> >> +        None => {
> >> +            cache.secrets.remove(tokenid);
> >> +        }
> >> +    }
> >> +}
> >> +
> >> +/// Get the current shared generation.
> >> +fn token_shadow_shared_gen() -> Option<usize> {
> >> +    crate::ConfigVersionCache::new()
> >> +        .ok()
> >> +        .map(|cvc| cvc.token_shadow_generation())
> >> +}
> >> +
> >> +/// Bump and return the new shared generation.
> >> +fn bump_token_shadow_shared_gen() -> Option<usize> {
> >> +    crate::ConfigVersionCache::new()
> >> +        .ok()
> >> +        .map(|cvc| cvc.increase_token_shadow_generation() + 1)
> >> +}
> >> +
> >> +/// Invalidates the cache state and only keeps the shared generation.
> > 
> > both calls to this actually set the cached generation to some value
> > right after, so maybe this should take a generation directly and set it?
> >
> 
> patch 3/4 doesn’t always update the gen on cache invalidation
> (shadow_mtime_len() error branch in apply_api_mutation) but most other
> call sites do. Agreed this can be refactored, maybe:
> 
> fn invalidate_cache_state(cache: &mut ApiTokenSecretCache) {
>      cache.secrets.clear();
>      // clear other cache fields (mtime/len/last_checked) as needed
> }
> 
> fn invalidate_cache_state_and_set_gen(cache: &mut ApiTokenSecretCache, 
> gen: usize) {
>      invalidate_cache_state(cache);
>      cache.shared_gen = gen;
> }
> 
> We could also do a single helper with Option<usize> but two helpers make 
> the call sites more explicit.
> 
> >> +fn invalidate_cache_state(cache: &mut ApiTokenSecretCache) {
> >> +    cache.secrets.clear();
> >> +}
> >> -- 
> >> 2.47.3
> >>
> >>
> >>
> >> _______________________________________________
> >> pbs-devel mailing list
> >> pbs-devel@lists.proxmox.com
> >> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> >>
> > 
> > 
> > _______________________________________________
> > pbs-devel mailing list
> > pbs-devel@lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
>


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel