Re: [pbs-devel] [PATCH proxmox-backup v5 5/5] acme: certificate ordering through proxmox-acme-api

[Samuel Rufinatscha <s.rufinatscha@proxmox.com>]

comments inline

On 1/13/26 2:45 PM, Fabian Grünbichler wrote:
> On January 8, 2026 12:26 pm, 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:
>> - Replace the custom ACME order/authorization loop in node certificates
>> with a call to proxmox_acme_api::order_certificate.
>> - Build domain + config data as proxmox-acme-api types
>> - Remove obsolete local ACME ordering and plugin glue code.
>>
>> Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>
>> ---
>>   src/acme/mod.rs               |   2 -
>>   src/acme/plugin.rs            | 335 ----------------------------------
>>   src/api2/node/certificates.rs | 229 ++++-------------------
>>   src/api2/types/acme.rs        |  73 --------
>>   src/api2/types/mod.rs         |   3 -
>>   src/config/acme/mod.rs        |   8 +-
>>   src/config/acme/plugin.rs     |  92 +---------
>>   src/config/node.rs            |  20 +-
>>   src/lib.rs                    |   2 -
>>   9 files changed, 38 insertions(+), 726 deletions(-)
>>   delete mode 100644 src/acme/mod.rs
>>   delete mode 100644 src/acme/plugin.rs
>>   delete mode 100644 src/api2/types/acme.rs
>>
> 
> [..]
> 
>> diff --git a/src/api2/node/certificates.rs b/src/api2/node/certificates.rs
>> index 47ff8de5..73401c41 100644
>> --- a/src/api2/node/certificates.rs
>> +++ b/src/api2/node/certificates.rs
>> @@ -1,14 +1,11 @@
>> -use std::sync::Arc;
>> -use std::time::Duration;
>> -
>>   use anyhow::{bail, format_err, Error};
>>   use openssl::pkey::PKey;
>>   use openssl::x509::X509;
>>   use serde::{Deserialize, Serialize};
>> -use tracing::{info, warn};
>> +use tracing::info;
>>   
>>   use pbs_api_types::{NODE_SCHEMA, PRIV_SYS_MODIFY};
>> -use proxmox_acme::async_client::AcmeClient;
>> +use proxmox_acme_api::AcmeDomain;
>>   use proxmox_rest_server::WorkerTask;
>>   use proxmox_router::list_subdirs_api_method;
>>   use proxmox_router::SubdirMap;
>> @@ -18,8 +15,6 @@ use proxmox_schema::api;
>>   use pbs_buildcfg::configdir;
>>   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()
>> @@ -268,193 +263,6 @@ pub async fn delete_custom_certificate() -> Result<(), Error> {
>>       Ok(())
>>   }
>>   
>> -struct OrderedCertificate {
>> -    certificate: hyper::body::Bytes,
>> -    private_key_pem: Vec<u8>,
>> -}
>> -
>> -async fn order_certificate(
>> -    worker: Arc<WorkerTask>,
>> -    node_config: &NodeConfig,
>> -) -> Result<Option<OrderedCertificate>, Error> {
>> -    use proxmox_acme::authorization::Status;
>> -    use proxmox_acme::order::Identifier;
>> -
>> -    let domains = node_config.acme_domains().try_fold(
>> -        Vec::<AcmeDomain>::new(),
>> -        |mut acc, domain| -> Result<_, Error> {
>> -            let mut domain = domain?;
>> -            domain.domain.make_ascii_lowercase();
>> -            if let Some(alias) = &mut domain.alias {
>> -                alias.make_ascii_lowercase();
>> -            }
>> -            acc.push(domain);
>> -            Ok(acc)
>> -        },
>> -    )?;
>> -
>> -    let get_domain_config = |domain: &str| {
>> -        domains
>> -            .iter()
>> -            .find(|d| d.domain == domain)
>> -            .ok_or_else(|| format_err!("no config for domain '{}'", domain))
>> -    };
>> -
>> -    if domains.is_empty() {
>> -        info!("No domains configured to be ordered from an ACME server.");
>> -        return Ok(None);
>> -    }
>> -
>> -    let (plugins, _) = crate::config::acme::plugin::config()?;
>> -
>> -    let mut acme = node_config.acme_client().await?;
>> -
>> -    info!("Placing ACME order");
>> -    let order = acme
>> -        .new_order(domains.iter().map(|d| d.domain.to_ascii_lowercase()))
>> -        .await?;
>> -    info!("Order URL: {}", order.location);
>> -
>> -    let identifiers: Vec<String> = order
>> -        .data
>> -        .identifiers
>> -        .iter()
>> -        .map(|identifier| match identifier {
>> -            Identifier::Dns(domain) => domain.clone(),
>> -        })
>> -        .collect();
>> -
>> -    for auth_url in &order.data.authorizations {
>> -        info!("Getting authorization details from '{auth_url}'");
>> -        let mut auth = acme.get_authorization(auth_url).await?;
>> -
>> -        let domain = match &mut auth.identifier {
>> -            Identifier::Dns(domain) => domain.to_ascii_lowercase(),
>> -        };
>> -
>> -        if auth.status == Status::Valid {
>> -            info!("{domain} is already validated!");
>> -            continue;
>> -        }
>> -
>> -        info!("The validation for {domain} is pending");
>> -        let domain_config: &AcmeDomain = get_domain_config(&domain)?;
>> -        let plugin_id = domain_config.plugin.as_deref().unwrap_or("standalone");
>> -        let mut plugin_cfg = crate::acme::get_acme_plugin(&plugins, plugin_id)?
>> -            .ok_or_else(|| format_err!("plugin '{plugin_id}' for domain '{domain}' not found!"))?;
>> -
>> -        info!("Setting up validation plugin");
>> -        let validation_url = plugin_cfg
>> -            .setup(&mut acme, &auth, domain_config, Arc::clone(&worker))
>> -            .await?;
>> -
>> -        let result = request_validation(&mut acme, auth_url, validation_url).await;
>> -
>> -        if let Err(err) = plugin_cfg
>> -            .teardown(&mut acme, &auth, domain_config, Arc::clone(&worker))
>> -            .await
>> -        {
>> -            warn!("Failed to teardown plugin '{plugin_id}' for domain '{domain}' - {err}");
>> -        }
>> -
>> -        result?;
>> -    }
>> -
>> -    info!("All domains validated");
>> -    info!("Creating CSR");
>> -
>> -    let csr = proxmox_acme::util::Csr::generate(&identifiers, &Default::default())?;
>> -    let mut finalize_error_cnt = 0u8;
>> -    let order_url = &order.location;
>> -    let mut order;
>> -    loop {
>> -        use proxmox_acme::order::Status;
>> -
>> -        order = acme.get_order(order_url).await?;
>> -
>> -        match order.status {
>> -            Status::Pending => {
>> -                info!("still pending, trying to finalize anyway");
>> -                let finalize = order
>> -                    .finalize
>> -                    .as_deref()
>> -                    .ok_or_else(|| format_err!("missing 'finalize' URL in order"))?;
>> -                if let Err(err) = acme.finalize(finalize, &csr.data).await {
>> -                    if finalize_error_cnt >= 5 {
>> -                        return Err(err);
>> -                    }
>> -
>> -                    finalize_error_cnt += 1;
>> -                }
>> -                tokio::time::sleep(Duration::from_secs(5)).await;
>> -            }
>> -            Status::Ready => {
>> -                info!("order is ready, finalizing");
>> -                let finalize = order
>> -                    .finalize
>> -                    .as_deref()
>> -                    .ok_or_else(|| format_err!("missing 'finalize' URL in order"))?;
>> -                acme.finalize(finalize, &csr.data).await?;
>> -                tokio::time::sleep(Duration::from_secs(5)).await;
>> -            }
>> -            Status::Processing => {
>> -                info!("still processing, trying again in 30 seconds");
>> -                tokio::time::sleep(Duration::from_secs(30)).await;
>> -            }
>> -            Status::Valid => {
>> -                info!("valid");
>> -                break;
>> -            }
>> -            other => bail!("order status: {:?}", other),
>> -        }
>> -    }
>> -
>> -    info!("Downloading certificate");
>> -    let certificate = acme
>> -        .get_certificate(
>> -            order
>> -                .certificate
>> -                .as_deref()
>> -                .ok_or_else(|| format_err!("missing certificate url in finalized order"))?,
>> -        )
>> -        .await?;
>> -
>> -    Ok(Some(OrderedCertificate {
>> -        certificate,
>> -        private_key_pem: csr.private_key_pem,
>> -    }))
>> -}
>> -
>> -async fn request_validation(
>> -    acme: &mut AcmeClient,
>> -    auth_url: &str,
>> -    validation_url: &str,
>> -) -> Result<(), Error> {
>> -    info!("Triggering validation");
>> -    acme.request_challenge_validation(validation_url).await?;
>> -
>> -    info!("Sleeping for 5 seconds");
>> -    tokio::time::sleep(Duration::from_secs(5)).await;
>> -
>> -    loop {
>> -        use proxmox_acme::authorization::Status;
>> -
>> -        let auth = acme.get_authorization(auth_url).await?;
>> -        match auth.status {
>> -            Status::Pending => {
>> -                info!("Status is still 'pending', trying again in 10 seconds");
>> -                tokio::time::sleep(Duration::from_secs(10)).await;
>> -            }
>> -            Status::Valid => return Ok(()),
>> -            other => bail!(
>> -                "validating challenge '{}' failed - status: {:?}",
>> -                validation_url,
>> -                other
>> -            ),
>> -        }
>> -    }
>> -}
>> -
>>   #[api(
>>       input: {
>>           properties: {
>> @@ -524,9 +332,30 @@ fn spawn_certificate_worker(
>>   
>>       let auth_id = rpcenv.get_auth_id().unwrap();
>>   
>> +    let acme_config = if let Some(cfg) = node_config.acme_config().transpose()? {
>> +        cfg
>> +    } else {
>> +        proxmox_acme_api::parse_acme_config_string("account=default")?
>> +    };
> 
> wouldn't it make sense to inline this into acme_config() ? the same
> fallback is already there for acme_client()
>

Good catch, will refactor!

>> +
>> +    let domains = node_config.acme_domains().try_fold(
>> +        Vec::<AcmeDomain>::new(),
>> +        |mut acc, domain| -> Result<_, Error> {
>> +            let mut domain = domain?;
>> +            domain.domain.make_ascii_lowercase();
>> +            if let Some(alias) = &mut domain.alias {
>> +                alias.make_ascii_lowercase();
>> +            }
>> +            acc.push(domain);
>> +            Ok(acc)
>> +        },
>> +    )?;
>> +
>>       WorkerTask::spawn(name, None, auth_id, true, move |worker| async move {
>>           let work = || async {
>> -            if let Some(cert) = order_certificate(worker, &node_config).await? {
>> +            if let Some(cert) =
>> +                proxmox_acme_api::order_certificate(worker, &acme_config, &domains).await?
>> +            {
>>                   crate::config::set_proxy_certificate(&cert.certificate, &cert.private_key_pem)?;
>>                   crate::server::reload_proxy_certificate().await?;
>>               }
>> @@ -562,16 +391,20 @@ pub fn revoke_acme_cert(rpcenv: &mut dyn RpcEnvironment) -> Result<String, Error
>>   
>>       let auth_id = rpcenv.get_auth_id().unwrap();
>>   
>> +    let acme_config = if let Some(cfg) = node_config.acme_config().transpose()? {
>> +        cfg
>> +    } else {
>> +        proxmox_acme_api::parse_acme_config_string("account=default")?
>> +    };
> 
> here as well
>

Will adjust!

>> +
>>       WorkerTask::spawn(
>>           "acme-revoke-cert",
>>           None,
>>           auth_id,
>>           true,
>>           move |_worker| async move {
>> -            info!("Loading ACME account");
>> -            let mut acme = node_config.acme_client().await?;
>>               info!("Revoking old certificate");
>> -            acme.revoke_certificate(cert_pem.as_bytes(), None).await?;
>> +            proxmox_acme_api::revoke_certificate(&acme_config, &cert_pem.as_bytes()).await?;
>>               info!("Deleting certificate and regenerating a self-signed one");
>>               delete_custom_certificate().await?;
>>               Ok(())
> 
> [..]
> 
>> diff --git a/src/config/acme/plugin.rs b/src/config/acme/plugin.rs
>> index 8ce852ec..4b4a216e 100644
>> --- a/src/config/acme/plugin.rs
>> +++ b/src/config/acme/plugin.rs
>> @@ -1,104 +1,16 @@
>>   use std::sync::LazyLock;
>>   
>>   use anyhow::Error;
>> -use serde::{Deserialize, Serialize};
>>   use serde_json::Value;
>>   
>> -use pbs_api_types::PROXMOX_SAFE_ID_FORMAT;
>> -use proxmox_schema::{api, ApiType, Schema, StringSchema, Updater};
>> +use proxmox_acme_api::{DnsPlugin, StandalonePlugin, PLUGIN_ID_SCHEMA};
>> +use proxmox_schema::{ApiType, Schema};
>>   use proxmox_section_config::{SectionConfig, SectionConfigData, SectionConfigPlugin};
>>   
>>   use pbs_config::{open_backup_lockfile, BackupLockGuard};
>>   
>> -pub const PLUGIN_ID_SCHEMA: Schema = StringSchema::new("ACME Challenge Plugin ID.")
>> -    .format(&PROXMOX_SAFE_ID_FORMAT)
>> -    .min_length(1)
>> -    .max_length(32)
>> -    .schema();
>> -
>>   pub static CONFIG: LazyLock<SectionConfig> = LazyLock::new(init);
>>   
>> -#[api(
>> -    properties: {
>> -        id: { schema: PLUGIN_ID_SCHEMA },
>> -    },
>> -)]
>> -#[derive(Deserialize, Serialize)]
>> -/// Standalone ACME Plugin for the http-1 challenge.
>> -pub struct StandalonePlugin {
>> -    /// Plugin ID.
>> -    id: String,
>> -}
>> -
>> -impl Default for StandalonePlugin {
>> -    fn default() -> Self {
>> -        Self {
>> -            id: "standalone".to_string(),
>> -        }
>> -    }
>> -}
>> -
>> -#[api(
>> -    properties: {
>> -        id: { schema: PLUGIN_ID_SCHEMA },
>> -        disable: {
>> -            optional: true,
>> -            default: false,
>> -        },
>> -        "validation-delay": {
>> -            default: 30,
>> -            optional: true,
>> -            minimum: 0,
>> -            maximum: 2 * 24 * 60 * 60,
>> -        },
>> -    },
>> -)]
>> -/// DNS ACME Challenge Plugin core data.
>> -#[derive(Deserialize, Serialize, Updater)]
>> -#[serde(rename_all = "kebab-case")]
>> -pub struct DnsPluginCore {
>> -    /// Plugin ID.
>> -    #[updater(skip)]
>> -    pub id: String,
>> -
>> -    /// DNS API Plugin Id.
>> -    pub api: String,
>> -
>> -    /// Extra delay in seconds to wait before requesting validation.
>> -    ///
>> -    /// Allows to cope with long TTL of DNS records.
>> -    #[serde(skip_serializing_if = "Option::is_none", default)]
>> -    pub validation_delay: Option<u32>,
>> -
>> -    /// Flag to disable the config.
>> -    #[serde(skip_serializing_if = "Option::is_none", default)]
>> -    pub disable: Option<bool>,
>> -}
>> -
>> -#[api(
>> -    properties: {
>> -        core: { type: DnsPluginCore },
>> -    },
>> -)]
>> -/// DNS ACME Challenge Plugin.
>> -#[derive(Deserialize, Serialize)]
>> -#[serde(rename_all = "kebab-case")]
>> -pub struct DnsPlugin {
>> -    #[serde(flatten)]
>> -    pub core: DnsPluginCore,
>> -
>> -    // We handle this property separately in the API calls.
>> -    /// DNS plugin data (base64url encoded without padding).
>> -    #[serde(with = "proxmox_serde::string_as_base64url_nopad")]
>> -    pub data: String,
>> -}
>> -
>> -impl DnsPlugin {
>> -    pub fn decode_data(&self, output: &mut Vec<u8>) -> Result<(), Error> {
>> -        Ok(proxmox_base64::url::decode_to_vec(&self.data, output)?)
>> -    }
>> -}
>> -
>>   fn init() -> SectionConfig {
>>       let mut config = SectionConfig::new(&PLUGIN_ID_SCHEMA);
>>   
>> diff --git a/src/config/node.rs b/src/config/node.rs
>> index e4b66a20..6865b815 100644
>> --- a/src/config/node.rs
>> +++ b/src/config/node.rs
>> @@ -9,14 +9,14 @@ use pbs_api_types::{
>>       OPENSSL_CIPHERS_TLS_1_3_SCHEMA,
>>   };
>>   use proxmox_acme::async_client::AcmeClient;
>> -use proxmox_acme_api::AcmeAccountName;
>> +use proxmox_acme_api::{AcmeAccountName, AcmeConfig, AcmeDomain, ACME_DOMAIN_PROPERTY_SCHEMA};
>>   use proxmox_http::ProxyConfig;
>>   use proxmox_schema::{api, ApiStringFormat, ApiType, Updater};
>>   
>>   use pbs_buildcfg::configdir;
>>   use pbs_config::{open_backup_lockfile, BackupLockGuard};
>>   
>> -use crate::api2::types::{AcmeDomain, ACME_DOMAIN_PROPERTY_SCHEMA, HTTP_PROXY_SCHEMA};
>> +use crate::api2::types::HTTP_PROXY_SCHEMA;
>>   
>>   const CONF_FILE: &str = configdir!("/node.cfg");
>>   const LOCK_FILE: &str = configdir!("/.node.lck");
>> @@ -43,20 +43,6 @@ pub fn save_config(config: &NodeConfig) -> Result<(), Error> {
>>       pbs_config::replace_backup_config(CONF_FILE, &raw)
>>   }
>>   
>> -#[api(
>> -    properties: {
>> -        account: { type: AcmeAccountName },
>> -    }
>> -)]
>> -#[derive(Deserialize, Serialize)]
>> -/// The ACME configuration.
>> -///
>> -/// Currently only contains the name of the account use.
>> -pub struct AcmeConfig {
>> -    /// Account to use to acquire ACME certificates.
>> -    account: AcmeAccountName,
>> -}
>> -
>>   /// All available languages in Proxmox. Taken from proxmox-i18n repository.
>>   /// pt_BR, zh_CN, and zh_TW use the same case in the translation files.
>>   // TODO: auto-generate from available translations
>> @@ -242,7 +228,7 @@ impl NodeConfig {
>>   
>>       pub async fn acme_client(&self) -> Result<AcmeClient, Error> {
>>           let account = if let Some(cfg) = self.acme_config().transpose()? {
>> -            cfg.account
>> +            AcmeAccountName::from_string(cfg.account)?
>>           } else {
>>               AcmeAccountName::from_string("default".to_string())? // should really not happen
>>           };
>> diff --git a/src/lib.rs b/src/lib.rs
>> index 8633378c..828f5842 100644
>> --- a/src/lib.rs
>> +++ b/src/lib.rs
>> @@ -27,8 +27,6 @@ pub(crate) mod auth;
>>   
>>   pub mod tape;
>>   
>> -pub mod acme;
>> -
>>   pub mod client_helpers;
>>   
>>   pub mod traffic_control_cache;
>> -- 
>> 2.47.3
>>
>>
>>
>> _______________________________________________
>> pbs-devel mailing list
>> pbs-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>>
>>
>>
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 



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