From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id A0C9C1FF13B for ; Wed, 11 Mar 2026 09:59:52 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 1D25310DEF; Wed, 11 Mar 2026 09:59:47 +0100 (CET) Date: Wed, 11 Mar 2026 09:59:36 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= Subject: Re: [PATCH proxmox{-backup,,-datacenter-manager} v6 00/11] token-shadow: reduce api token verification overhead To: pbs-devel@lists.proxmox.com, Samuel Rufinatscha References: <20260303165005.373120-1-s.rufinatscha@proxmox.com> In-Reply-To: <20260303165005.373120-1-s.rufinatscha@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.17.0 (https://github.com/astroidmail/astroid) Message-Id: <1773219511.dbh9fr76xn.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1773219544815 X-SPAM-LEVEL: Spam detection results: 0 AWL -1.162 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 POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.408 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.819 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.903 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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: 7MEYUUNRLN5A35W2EU4GC6WEOLPEBASM X-Message-ID-Hash: 7MEYUUNRLN5A35W2EU4GC6WEOLPEBASM X-MailFrom: f.gruenbichler@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 X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On March 3, 2026 5:49 pm, Samuel Rufinatscha wrote: > Hi, >=20 > this series improves the performance of token-based API authentication > in PBS (pbs-config) and in PDM (underlying proxmox-access-control > crate), addressing the API token verification hotspot reported in our > bugtracker #7017 [1]. could you re-spin this so the proxmox/pdm parts compile (the proxmox workspace got switched to edition 2024, which clashes with the `gen` variable/binding name used in the patches here). please also adapt the PBS patches to the new compatible naming - at some point we probably want to switch that over as well ;) >=20 > When profiling PBS /status endpoint with cargo flamegraph [2], > token-based authentication showed up as a dominant hotspot via > proxmox_sys::crypt::verify_crypt_pw. Applying this series removes that > path from the hot section of the flamegraph. The same performance issue > was measured [2] for PDM. PDM uses the underlying shared > proxmox-access-control library for token handling, which is a > factored out version of the token.shadow handling code from PBS. >=20 > While this series fixes the immediate performance issue both in PBS > (pbs-config) and in the shared proxmox-access-control crate used by > PDM, PBS should eventually, ideally be refactored, in a separate > effort, to use proxmox-access-control for token handling instead of its > local implementation. >=20 > Approach >=20 > The goal is to reduce the cost of token-based authentication preserving > the existing token handling semantics (including detecting manual edits > to token.shadow) and be consistent between PBS (pbs-config) and > PDM (proxmox-access-control). For both sites, this series proposes to: >=20 > 1. Introduce an in-memory cache for verified token secrets and > invalidate it through a shared ConfigVersionCache generation. Note, a > shared generation is required to keep privileged and unprivileged > daemon in sync to avoid caching inconsistencies across processes. > 2. Invalidate on token.shadow API changes (set_secret, > delete_secret) > 3. Invalidate on direct/manual token.shadow file changes (mtime + > length) > 4. Avoid per-request file stat calls using a TTL window >=20 > Testing >=20 > To verify the effect in PBS (pbs-config changes), I: > 1. Set up test environment based on latest PBS ISO, installed Rust > toolchain, cloned proxmox-backup repository to use with cargo > flamegraph. Reproduced bug #7017 [1] by profiling the /status > endpoint with token-based authentication using cargo flamegraph [2]. > 2. Built PBS with pbs-config patches and re-ran the same workload and > profiling setup. Confirmed that > proxmox_sys::crypt::verify_crypt_pw path no longer appears in the > hot section of the flamegraph. CPU usage is now dominated by TLS > overhead. > 3. Functionally-wise, I verified that: > * valid tokens authenticate correctly when used in API requests > * invalid secrets are rejected as before > * generating a new token secret via dashboard (create token for > user, regenerate existing secret) works and authenticates correctly >=20 > To verify the effect in PDM (proxmox-access-control changes), instead > of PBS=E2=80=99 /status, I profiled the /version endpoint with cargo flam= egraph > [2] and verified that the expensive hashing path disappears from the > hot section after introducing caching. Functionally-wise, I verified > that: > * valid tokens authenticate correctly when used in API requests > * invalid secrets are rejected as before > * generating a new token secret via dashboard (create token for user, > regenerate existing secret) works and authenticates correctly >=20 > Results >=20 > To measure caching effect I benchmarked parallel token auth requests > for /status?verbose=3D0 on top of the datastore lookup cache series [3] > to check throughput impact. With datastores=3D1, repeat=3D5000, parallel= =3D16 > this series gives ~172 req/s compared to ~65 req/s without it. > This is a ~2.6x improvement (and aligns with the ~179 req/s from the > previous series, which used per-process cache invalidation). >=20 > Patch summary >=20 > pbs-config: > 0001 =E2=80=93 pbs-config: add token.shadow generation to ConfigVersionCa= che > 0002 =E2=80=93 pbs-config: cache verified API token secrets > 0003 =E2=80=93 pbs-config: invalidate token-secret cache on token.shadow > changes > 0004 =E2=80=93 pbs-config: add TTL window to token-secret cache >=20 > proxmox-access-control: > 0005 =E2=80=93 access-control: extend AccessControlConfig for token.shado= w invalidation > 0006 =E2=80=93 access-control: cache verified API token secrets > 0007 =E2=80=93 access-control: invalidate token-secret cache on token.sha= dow changes > 0008 =E2=80=93 access-control: add TTL window to token-secret cache >=20 > proxmox-datacenter-manager: > 0009 =E2=80=93 pdm-config: add token.shadow generation to ConfigVersionCa= che > 0010 =E2=80=93 docs: document API token-cache TTL effects > 0011 =E2=80=93 pdm-config: wire user+acl cache generation >=20 > Maintainer Notes: > * proxmox-access-control trait split: permissions now live in > AccessControlPermissions, and AccessControlConfig now requires > fn permissions(&self) -> &dyn AccessControlPermissions -> > version bump > * Renames ConfigVersionCache`s pub user_cache_generation and > increase_user_cache_generation -> version bump > * Adds parking_lot::RwLock dependency in PBS and proxmox-access-control >=20 > This version and the version before only incorporate the reviewers' > feedback [4][5], also please consider Christian's R-b tag [4]. >=20 > [1] https://bugzilla.proxmox.com/show_bug.cgi?id=3D7017 > [2] attachment 1767 [1]: Flamegraph showing the proxmox_sys::crypt::verif= y_crypt_pw stack > [3] https://bugzilla.proxmox.com/show_bug.cgi?id=3D6049 > [4] https://lore.proxmox.com/pbs-devel/20260121151408.731516-1-s.rufinats= cha@proxmox.com/T/#t > [5] https://lore.proxmox.com/pbs-devel/20260217111229.78661-1-s.rufinatsc= ha@proxmox.com/T/#t >=20 > proxmox-backup: >=20 > Samuel Rufinatscha (4): > pbs-config: add token.shadow generation to ConfigVersionCache > pbs-config: cache verified API token secrets > pbs-config: invalidate token-secret cache on token.shadow changes > pbs-config: add TTL window to token secret cache >=20 > Cargo.toml | 1 + > docs/user-management.rst | 4 + > pbs-config/Cargo.toml | 1 + > pbs-config/src/config_version_cache.rs | 18 ++ > pbs-config/src/token_shadow.rs | 314 ++++++++++++++++++++++++- > 5 files changed, 335 insertions(+), 3 deletions(-) >=20 >=20 > proxmox: >=20 > Samuel Rufinatscha (4): > proxmox-access-control: split AccessControlConfig and add token.shadow > gen > proxmox-access-control: cache verified API token secrets > proxmox-access-control: invalidate token-secret cache on token.shadow > changes > proxmox-access-control: add TTL window to token secret cache >=20 > Cargo.toml | 1 + > proxmox-access-control/Cargo.toml | 1 + > proxmox-access-control/src/acl.rs | 10 +- > proxmox-access-control/src/init.rs | 113 ++++++-- > proxmox-access-control/src/token_shadow.rs | 315 ++++++++++++++++++++- > 5 files changed, 413 insertions(+), 27 deletions(-) >=20 >=20 > proxmox-datacenter-manager: >=20 > Samuel Rufinatscha (3): > pdm-config: implement token.shadow generation > docs: document API token-cache TTL effects > pdm-config: wire user+acl cache generation >=20 > cli/admin/src/main.rs | 2 +- > docs/access-control.rst | 4 +++ > lib/pdm-api-types/src/acl.rs | 4 +-- > lib/pdm-config/Cargo.toml | 1 + > lib/pdm-config/src/access_control.rs | 31 ++++++++++++++++++++ > lib/pdm-config/src/config_version_cache.rs | 34 +++++++++++++++++----- > lib/pdm-config/src/lib.rs | 2 ++ > server/src/acl.rs | 3 +- > ui/src/main.rs | 10 ++++++- > 9 files changed, 77 insertions(+), 14 deletions(-) > create mode 100644 lib/pdm-config/src/access_control.rs >=20 >=20 > Summary over all repositories: > 19 files changed, 825 insertions(+), 44 deletions(-) >=20 > --=20 > Generated by git-murpp 0.8.1 >=20 >=20 >=20 >=20 >=20