Re: [PATCH proxmox-backup] pbs-config: refactor and move helper to detect config digest changes

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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 <c.ebner@proxmox.com>
> ---
> 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";