Re: [pbs-devel] [PATCH proxmox-backup 1/3] pbs-config: cache verified API token secrets

["Shannon Sterz" <s.sterz@proxmox.com>]

On Wed Dec 17, 2025 at 12:16 PM CET, Christian Ebner wrote:
> On 12/9/25 2:29 PM, Samuel Rufinatscha wrote:
>> On 12/5/25 3:03 PM, Shannon Sterz wrote:
>>> On Fri Dec 5, 2025 at 2:25 PM CET, 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 #6049 [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 partly fixes bug #6049 [1].
>>>>
>>>> [1] https://bugzilla.proxmox.com/show_bug.cgi?id=7017
>>>>
>>>> Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>
>>>> ---
>>>>   pbs-config/src/token_shadow.rs | 58 +++++++++++++++++++++++++++++++++-
>>>>   1 file changed, 57 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/pbs-config/src/token_shadow.rs b/pbs-config/src/
>>>> token_shadow.rs
>>>> index 640fabbf..47aa2fc2 100644
>>>> --- a/pbs-config/src/token_shadow.rs
>>>> +++ b/pbs-config/src/token_shadow.rs
>>>> @@ -1,6 +1,8 @@
>>>>   use std::collections::HashMap;
>>>> +use std::sync::RwLock;
>>>>
>>>>   use anyhow::{bail, format_err, Error};
>>>> +use once_cell::sync::OnceCell;
>>>>   use serde::{Deserialize, Serialize};
>>>>   use serde_json::{from_value, Value};
>>>>
>>>> @@ -13,6 +15,13 @@ use crate::{open_backup_lockfile, BackupLockGuard};
>>>>   const LOCK_FILE: &str = pbs_buildcfg::configdir!("/
>>>> token.shadow.lock");
>>>>   const CONF_FILE: &str = pbs_buildcfg::configdir!("/token.shadow");
>>>>
>>>> +/// Global in-memory cache for successfully verified API token secrets.
>>>> +/// The cache stores plain text secrets for token Authids that have
>>>> already been
>>>> +/// verified against the hashed values in `token.shadow`. This
>>>> allows for cheap
>>>> +/// subsequent authentications for the same token+secret
>>>> combination, avoiding
>>>> +/// recomputing the password hash on every request.
>>>> +static TOKEN_SECRET_CACHE: OnceCell<RwLock<ApiTokenSecretCache>> =
>>>> OnceCell::new();
>>>
>>> any reason you are using a once cell with a cutom get_or_init function
>>> instead of a simple `LazyCell` [1] here? seems to me that this would be
>>> the more appropriate type here? similar question for the
>>> proxmox-access-control portion of this series.
>>>
>>> [1]: https://doc.rust-lang.org/std/cell/struct.LazyCell.html
>>>
>>
>> Good point, we should / can directly initialize it! Will change
>> to LazyCell. Thanks!
>
> LazyCell is however not thread safe, so could cause issues with
> concurrent inits from different threads. IMO std::sync::LazyLock [0] is
> a better fit here and follows along the line of what we do for other
> caches in PBS, e.g. in pbs-config::user.
>
> [0] https://doc.rust-lang.org/std/sync/struct.LazyLock.html

ah right, yes that makes a lot of sense, thanks for catching that.


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