Re: [pbs-devel] [PATCH proxmox-backup v4 2/4] acme: drop local AcmeClient

["Max R. Carrara" <m.carrara@proxmox.com>]

On Wed Dec 3, 2025 at 11:22 AM CET, Samuel Rufinatscha wrote:
> PBS currently uses its own ACME client and API logic, while PDM uses the
> factored out proxmox-acme and proxmox-acme-api crates. This duplication
> risks differences in behaviour and requires ACME maintenance in two
> places. This patch is part of a series to move PBS over to the shared
> ACME stack.
>
> Changes:
> - Remove the local src/acme/client.rs and switch to
> proxmox_acme::async_client::AcmeClient where needed.
> - Use proxmox_acme_api::load_client_with_account to the custom
> AcmeClient::load() function
> - Replace the local do_register() logic with
> proxmox_acme_api::register_account, to further ensure accounts are persisted
> - Replace the local AcmeAccountName type, required for
> proxmox_acme_api::register_account
>
> Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>
> ---

Since you changed a lot of the imported types and traits in this patch
(and later ones), note that we have a particular ordering regarding
imports:
(At least as far as I'm aware at least; otherwise, someone please
correct me if I'm wrong)

1. imports from the stdlib 
2. imports from external dependencies
3. imports from internal dependencies (so, mostly stuff from proxmox/)
4. imports from crates local to the repository
5. imports from the current crate

All of these groups are then separated by a blank line. The `use`
statements within those groups are (usually) ordered alphabetically. For
some examples, just browse around PBS a little bit.

Note that we're not suuuper strict about it, since we seem to not follow
that all too precisely in some isolated cases, but nevertheless, it's
good to stick to that format in order to keep things neat.

Unfortunately this isn't something we've automated yet due to it not
(completely?) supported in `rustfmt` / `cargo fmt` AFAIK. `cargo fmt`
should at least sort the individual groups, though.

Also, one apparently common exception to that format is the placement of
`pbs_api_types`—sometimes its part of 3., sometimes it's thrown in with
the crates of 4. In my suggestions in this patch (and the following
ones), I've added it to 3. for consistency's sake.

I would say that overall when you add new `use` statements, just make
sure they're added to the corresponding group if it exists already;
otherwise, add the group using the ordering above. It's not worth to
change the ordering of existing groups, at least not as part of the same
patch.

>  src/acme/client.rs                     | 691 -------------------------
>  src/acme/mod.rs                        |   3 -
>  src/acme/plugin.rs                     |   2 +-
>  src/api2/config/acme.rs                |  50 +-
>  src/api2/node/certificates.rs          |   2 +-
>  src/api2/types/acme.rs                 |   8 -
>  src/bin/proxmox_backup_manager/acme.rs |  17 +-
>  src/config/acme/mod.rs                 |   8 +-
>  src/config/node.rs                     |   9 +-
>  9 files changed, 36 insertions(+), 754 deletions(-)
>  delete mode 100644 src/acme/client.rs
>
> diff --git a/src/acme/client.rs b/src/acme/client.rs
> deleted file mode 100644
> index 9fb6ad55..00000000
> --- a/src/acme/client.rs
> +++ /dev/null
> @@ -1,691 +0,0 @@

snip 8<---------

> diff --git a/src/acme/mod.rs b/src/acme/mod.rs
> index bf61811c..cc561f9a 100644
> --- a/src/acme/mod.rs
> +++ b/src/acme/mod.rs
> @@ -1,5 +1,2 @@
> -mod client;
> -pub use client::AcmeClient;
> -
>  pub(crate) mod plugin;
>  pub(crate) use plugin::get_acme_plugin;
> diff --git a/src/acme/plugin.rs b/src/acme/plugin.rs
> index f756e9b5..5bc09e1f 100644
> --- a/src/acme/plugin.rs
> +++ b/src/acme/plugin.rs
> @@ -20,8 +20,8 @@ use tokio::process::Command;
>  
>  use proxmox_acme::{Authorization, Challenge};
>  
> -use crate::acme::AcmeClient;
>  use crate::api2::types::AcmeDomain;
> +use proxmox_acme::async_client::AcmeClient;
>  use proxmox_rest_server::WorkerTask;

use proxmox_acme::{Authorization, Challenge};
use proxmox_acme::async_client::AcmeClient;
use proxmox_rest_server::WorkerTask;

use crate::api2::types::AcmeDomain;

>  
>  use crate::config::acme::plugin::{DnsPlugin, PluginData};
> diff --git a/src/api2/config/acme.rs b/src/api2/config/acme.rs
> index 35c3fb77..02f88e2e 100644
> --- a/src/api2/config/acme.rs
> +++ b/src/api2/config/acme.rs
> @@ -16,15 +16,15 @@ use proxmox_router::{
>  use proxmox_schema::{api, param_bail};
>  
>  use proxmox_acme::types::AccountData as AcmeAccountData;
> -use proxmox_acme::Account;
>  
>  use pbs_api_types::{Authid, PRIV_SYS_MODIFY};
>  
> -use crate::acme::AcmeClient;
> -use crate::api2::types::{AcmeAccountName, AcmeChallengeSchema, KnownAcmeDirectory};
> +use crate::api2::types::{AcmeChallengeSchema, KnownAcmeDirectory};
>  use crate::config::acme::plugin::{
>      self, DnsPlugin, DnsPluginCore, DnsPluginCoreUpdater, PLUGIN_ID_SCHEMA,
>  };
> +use proxmox_acme::async_client::AcmeClient;
> +use proxmox_acme_api::AcmeAccountName;
>  use proxmox_rest_server::WorkerTask;

This file is a good example where we weren't strictly following that
format yet ...

use pbs_api_types::{Authid, PRIV_SYS_MODIFY};
use proxmox_acme::async_client::AcmeClient;
use proxmox_acme::types::AccountData as AcmeAccountData;
use proxmox_acme_api::AcmeAccountName;
use proxmox_rest_server::WorkerTask;
use proxmox_schema::{api, param_bail};

use crate::api2::types::{AcmeChallengeSchema, KnownAcmeDirectory};
use crate::config::acme::plugin::{
    self, DnsPlugin, DnsPluginCore, DnsPluginCoreUpdater, PLUGIN_ID_SCHEMA,
};

>  
>  pub(crate) const ROUTER: Router = Router::new()
> @@ -143,15 +143,15 @@ pub struct AccountInfo {
>  )]
>  /// Return existing ACME account information.
>  pub async fn get_account(name: AcmeAccountName) -> Result<AccountInfo, Error> {
> -    let client = AcmeClient::load(&name).await?;
> -    let account = client.account()?;
> +    let account_info = proxmox_acme_api::get_account(name).await?;
> +
>      Ok(AccountInfo {
> -        location: account.location.clone(),
> -        tos: client.tos().map(str::to_owned),
> -        directory: client.directory_url().to_owned(),
> +        location: account_info.location,
> +        tos: account_info.tos,
> +        directory: account_info.directory,
>          account: AcmeAccountData {
>              only_return_existing: false, // don't actually write this out in case it's set
> -            ..account.data.clone()
> +            ..account_info.account
>          },
>      })
>  }
> @@ -240,41 +240,24 @@ fn register_account(
>          auth_id.to_string(),
>          true,
>          move |_worker| async move {
> -            let mut client = AcmeClient::new(directory);
> -
>              info!("Registering ACME account '{}'...", &name);
>  
> -            let account = do_register_account(
> -                &mut client,
> +            let location = proxmox_acme_api::register_account(
>                  &name,
> -                tos_url.is_some(),
>                  contact,
> -                None,
> +                tos_url,
> +                Some(directory),
>                  eab_kid.zip(eab_hmac_key),
>              )
>              .await?;
>  
> -            info!("Registration successful, account URL: {}", account.location);
> +            info!("Registration successful, account URL: {}", location);
>  
>              Ok(())
>          },
>      )
>  }
>  
> -pub async fn do_register_account<'a>(
> -    client: &'a mut AcmeClient,
> -    name: &AcmeAccountName,
> -    agree_to_tos: bool,
> -    contact: String,
> -    rsa_bits: Option<u32>,
> -    eab_creds: Option<(String, String)>,
> -) -> Result<&'a Account, Error> {
> -    let contact = account_contact_from_string(&contact);
> -    client
> -        .new_account(name, agree_to_tos, contact, rsa_bits, eab_creds)
> -        .await
> -}
> -
>  #[api(
>      input: {
>          properties: {
> @@ -312,7 +295,10 @@ pub fn update_account(
>                  None => json!({}),
>              };
>  
> -            AcmeClient::load(&name).await?.update_account(&data).await?;
> +            proxmox_acme_api::load_client_with_account(&name)
> +                .await?
> +                .update_account(&data)
> +                .await?;
>  
>              Ok(())
>          },
> @@ -350,7 +336,7 @@ pub fn deactivate_account(
>          auth_id.to_string(),
>          true,
>          move |_worker| async move {
> -            match AcmeClient::load(&name)
> +            match proxmox_acme_api::load_client_with_account(&name)
>                  .await?
>                  .update_account(&json!({"status": "deactivated"}))
>                  .await
> diff --git a/src/api2/node/certificates.rs b/src/api2/node/certificates.rs
> index 61ef910e..31196715 100644
> --- a/src/api2/node/certificates.rs
> +++ b/src/api2/node/certificates.rs
> @@ -17,10 +17,10 @@ use pbs_buildcfg::configdir;
>  use pbs_tools::cert;
>  use tracing::warn;
>  
> -use crate::acme::AcmeClient;
>  use crate::api2::types::AcmeDomain;
>  use crate::config::node::NodeConfig;
>  use crate::server::send_certificate_renewal_mail;
> +use proxmox_acme::async_client::AcmeClient;
>  use proxmox_rest_server::WorkerTask;

use tracing::warn;

use proxmox_acme::async_client::AcmeClient;
use proxmox_rest_server::WorkerTask;

use pbs_tools::cert;

use crate::api2::types::AcmeDomain;
use crate::config::node::NodeConfig;
use crate::server::send_certificate_renewal_mail;

>  
>  pub const ROUTER: Router = Router::new()
> diff --git a/src/api2/types/acme.rs b/src/api2/types/acme.rs
> index 210ebdbc..7c9063c0 100644
> --- a/src/api2/types/acme.rs
> +++ b/src/api2/types/acme.rs
> @@ -60,14 +60,6 @@ pub struct KnownAcmeDirectory {
>      pub url: &'static str,
>  }
>  
> -proxmox_schema::api_string_type! {
> -    #[api(format: &PROXMOX_SAFE_ID_FORMAT)]
> -    /// ACME account name.
> -    #[derive(Clone, Eq, PartialEq, Hash, Deserialize, Serialize)]
> -    #[serde(transparent)]
> -    pub struct AcmeAccountName(String);
> -}
> -
>  #[api(
>      properties: {
>          schema: {
> diff --git a/src/bin/proxmox_backup_manager/acme.rs b/src/bin/proxmox_backup_manager/acme.rs
> index 0f0eafea..bb987b26 100644
> --- a/src/bin/proxmox_backup_manager/acme.rs
> +++ b/src/bin/proxmox_backup_manager/acme.rs
> @@ -7,9 +7,9 @@ use proxmox_router::{cli::*, ApiHandler, RpcEnvironment};
>  use proxmox_schema::api;
>  use proxmox_sys::fs::file_get_contents;
>  
> -use proxmox_backup::acme::AcmeClient;
> +use proxmox_acme::async_client::AcmeClient;
> +use proxmox_acme_api::AcmeAccountName;
>  use proxmox_backup::api2;
> -use proxmox_backup::api2::types::AcmeAccountName;
>  use proxmox_backup::config::acme::plugin::DnsPluginCore;
>  use proxmox_backup::config::acme::KNOWN_ACME_DIRECTORIES;

use proxmox_acme::async_client::AcmeClient;
use proxmox_acme_api::AcmeAccountName;
use proxmox_schema::api;
use proxmox_sys::fs::file_get_contents;

use proxmox_backup::acme::AcmeClient;
use proxmox_backup::api2;
use proxmox_backup::api2::types::AcmeAccountName;
use proxmox_backup::config::acme::plugin::DnsPluginCore;
use proxmox_backup::config::acme::KNOWN_ACME_DIRECTORIES;

>  
> @@ -188,17 +188,20 @@ async fn register_account(
>  
>      println!("Attempting to register account with {directory_url:?}...");
>  
> -    let account = api2::config::acme::do_register_account(
> -        &mut client,
> +    let tos_agreed = tos_agreed
> +        .then(|| directory.terms_of_service_url().map(str::to_owned))
> +        .flatten();
> +
> +    let location = proxmox_acme_api::register_account(
>          &name,
> -        tos_agreed,
>          contact,
> -        None,
> +        tos_agreed,
> +        Some(directory_url),
>          eab_creds,
>      )
>      .await?;
>  
> -    println!("Registration successful, account URL: {}", account.location);
> +    println!("Registration successful, account URL: {}", location);
>  
>      Ok(())
>  }
> diff --git a/src/config/acme/mod.rs b/src/config/acme/mod.rs
> index 274a23fd..d31b2bc9 100644
> --- a/src/config/acme/mod.rs
> +++ b/src/config/acme/mod.rs
> @@ -10,7 +10,8 @@ use proxmox_sys::fs::{file_read_string, CreateOptions};
>  
>  use pbs_api_types::PROXMOX_SAFE_ID_REGEX;
>  
> -use crate::api2::types::{AcmeAccountName, AcmeChallengeSchema, KnownAcmeDirectory};
> +use crate::api2::types::{AcmeChallengeSchema, KnownAcmeDirectory};
> +use proxmox_acme_api::AcmeAccountName;

use proxmox_acme_api::AcmeAccountName;
[...]
use proxmox_sys::fs::{file_read_string, CreateOptions};

use pbs_api_types::PROXMOX_SAFE_ID_REGEX;

use crate::api2::types::{AcmeChallengeSchema, KnownAcmeDirectory};

>  
>  pub(crate) const ACME_DIR: &str = pbs_buildcfg::configdir!("/acme");
>  pub(crate) const ACME_ACCOUNT_DIR: &str = pbs_buildcfg::configdir!("/acme/accounts");
> @@ -35,11 +36,6 @@ pub(crate) fn make_acme_dir() -> Result<(), Error> {
>      create_acme_subdir(ACME_DIR)
>  }
>  
> -pub(crate) fn make_acme_account_dir() -> Result<(), Error> {
> -    make_acme_dir()?;
> -    create_acme_subdir(ACME_ACCOUNT_DIR)
> -}
> -
>  pub const KNOWN_ACME_DIRECTORIES: &[KnownAcmeDirectory] = &[
>      KnownAcmeDirectory {
>          name: "Let's Encrypt V2",
> diff --git a/src/config/node.rs b/src/config/node.rs
> index d2d6e383..d2a17a49 100644
> --- a/src/config/node.rs
> +++ b/src/config/node.rs
> @@ -16,10 +16,9 @@ use pbs_api_types::{
>  use pbs_buildcfg::configdir;
>  use pbs_config::{open_backup_lockfile, BackupLockGuard};
>  
> -use crate::acme::AcmeClient;
> -use crate::api2::types::{
> -    AcmeAccountName, AcmeDomain, ACME_DOMAIN_PROPERTY_SCHEMA, HTTP_PROXY_SCHEMA,
> -};
> +use crate::api2::types::{AcmeDomain, ACME_DOMAIN_PROPERTY_SCHEMA, HTTP_PROXY_SCHEMA};
> +use proxmox_acme::async_client::AcmeClient;
> +use proxmox_acme_api::AcmeAccountName;

use proxmox_acme::async_client::AcmeClient;
use proxmox_acme_api::AcmeAccountName;

use pbs_buildcfg::configdir;
use pbs_config::{open_backup_lockfile, BackupLockGuard};

use crate::api2::types::{AcmeDomain, ACME_DOMAIN_PROPERTY_SCHEMA, HTTP_PROXY_SCHEMA};

>  
>  const CONF_FILE: &str = configdir!("/node.cfg");
>  const LOCK_FILE: &str = configdir!("/.node.lck");
> @@ -249,7 +248,7 @@ impl NodeConfig {
>          } else {
>              AcmeAccountName::from_string("default".to_string())? // should really not happen
>          };
> -        AcmeClient::load(&account).await
> +        proxmox_acme_api::load_client_with_account(&account).await
>      }
>  
>      pub fn acme_domains(&'_ self) -> AcmeDomainIter<'_> {



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