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 06E381FF135 for ; Sun, 05 Apr 2026 10:39:07 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0E70318818; Sun, 5 Apr 2026 10:39:39 +0200 (CEST) Message-ID: <8b96cc02-af6d-4053-9d4b-acaffed953bc@proxmox.com> Date: Sun, 5 Apr 2026 10:39:01 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH proxmox-backup] pbs-config: refactor and move helper to detect config digest changes To: Christian Ebner , pbs-devel@lists.proxmox.com References: <20260402134110.848575-1-c.ebner@proxmox.com> Content-Language: en-US, de-DE From: Thomas Lamprecht In-Reply-To: <20260402134110.848575-1-c.ebner@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1775378279760 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.001 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 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.001 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.001 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: QJRL6WT4ZKWIXWHYXEU572OCS7APCUGJ X-Message-ID-Hash: QJRL6WT4ZKWIXWHYXEU572OCS7APCUGJ X-MailFrom: t.lamprecht@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 02/04/2026 15:40, Christian Ebner wrote: > This is such a common pattern for a huge number of API handlers that > refactoring it into a common helper is waranted. As a positive side > effect, direct dependencies on the hex crate for the API handlers are > reduced. > > By moving the helper to pbs-config it can be used also from within > pbs-config, which will be used to detect config changes for encrypton > keys in the future. > > In that particular case, keeping all the locking and digest > change detection logic inside that crate makes sense since the keys > are stored separately, which is an implementation detail the API > handler should not be concerned with. > > No functional changes intended. > > Signed-off-by: Christian Ebner > --- > While encountered when working on the config api for encrypted push > sync jobs, this touches so many API handlers that sending this as > dedicated patch for independent review already. > > pbs-config/Cargo.toml | 1 + > pbs-config/src/lib.rs | 19 +++++++++++++++++- > src/api2/access/acl.rs | 6 +----- > src/api2/access/user.rs | 26 +++++-------------------- > src/api2/config/access/ad.rs | 6 +----- > src/api2/config/access/ldap.rs | 11 ++--------- > src/api2/config/access/openid.rs | 11 ++--------- > src/api2/config/access/pam.rs | 6 +----- > src/api2/config/access/pbs.rs | 6 +----- > src/api2/config/access/tfa.rs | 12 ++++-------- > src/api2/config/changer.rs | 6 +----- > src/api2/config/datastore.rs | 11 ++--------- > src/api2/config/drive.rs | 6 +----- > src/api2/config/metrics/influxdbhttp.rs | 11 ++--------- > src/api2/config/metrics/influxdbudp.rs | 11 ++--------- > src/api2/config/prune.rs | 11 ++--------- > src/api2/config/remote.rs | 11 ++--------- > src/api2/config/s3.rs | 11 ++--------- > src/api2/config/sync.rs | 11 ++--------- > src/api2/config/tape_backup_job.rs | 11 ++--------- > src/api2/config/tape_encryption_keys.rs | 11 ++--------- > src/api2/config/traffic_control.rs | 11 ++--------- > src/api2/config/verify.rs | 11 ++--------- > src/api2/node/config.rs | 4 +--- > src/api2/node/network.rs | 11 ++--------- > src/tools/mod.rs | 13 ------------- > 26 files changed, 63 insertions(+), 202 deletions(-) > > diff --git a/src/api2/config/access/tfa.rs b/src/api2/config/access/tfa.rs > index 5cb17bba0..19824298b 100644 > --- a/src/api2/config/access/tfa.rs > +++ b/src/api2/config/access/tfa.rs > @@ -2,7 +2,6 @@ > //! If we add more, it should be moved into a sub module. > > use anyhow::{format_err, Error}; > -use hex::FromHex; > use serde::{Deserialize, Serialize}; > > use proxmox_router::list_subdirs_api_method; > @@ -94,13 +93,10 @@ pub fn update_webauthn_config( > let mut tfa = tfa::read()?; > > if let Some(wa) = &mut tfa.webauthn { > - if let Some(ref digest) = digest { > - let digest = <[u8; 32]>::from_hex(digest)?; > - crate::tools::detect_modified_configuration_file( > - &digest, > - &crate::config::tfa::webauthn_config_digest(wa)?, > - )?; > - } > + pbs_config::detect_modified_configuration_file( > + digest, > + &crate::config::tfa::webauthn_config_digest(wa)?, The webauthn parsing was previously only done if there was an expected digest send along, now it's done unconditionally. It probably isn't a problem, it should be valid and thus parseable anyway, and this is the only place where we did not already parse upfront anyway. But that made we wonder if we could encapsulate that along side the config parsing directly, i.e., pass the config parse method the digest param as option (or add a new fn like config_if_not_modified where it's not an option). But probably not that much additional to gain with that, just wanted to put the idea out there. > + )?; > > if let Some(delete) = delete { > for delete in delete { > diff --git a/src/tools/mod.rs b/src/tools/mod.rs > index 6a975bde2..51d9ad777 100644 > --- a/src/tools/mod.rs > +++ b/src/tools/mod.rs > @@ -29,19 +29,6 @@ pub fn assert_if_modified(digest1: &str, digest2: &str) -> Result<(), Error> { assert_if_modified has only one user left in src/api2/node/dns.rs, might be worth to remove too now? > Ok(()) > } > > -/// Detect modified configuration files > -/// > -/// This function fails with a reasonable error message if checksums do not match. > -pub fn detect_modified_configuration_file( > - digest1: &[u8; 32], > - digest2: &[u8; 32], > -) -> Result<(), Error> { > - if digest1 != digest2 { > - bail!("detected modified configuration - file changed by other user? Try again."); > - } > - Ok(()) > -} > - > /// The default 2 hours are far too long for PBS > pub const PROXMOX_BACKUP_TCP_KEEPALIVE_TIME: u32 = 120; > pub const DEFAULT_USER_AGENT_STRING: &str = "proxmox-backup-client/1.0";