[PATCH proxmox{,-backup,-datacenter-manager} 0/7] acme: fix #6372 implement basic ARI support

[PATCH proxmox{,-backup,-datacenter-manager} 0/7] acme: fix #6372 implement basic ARI support

[Manuel Federanko]

This series implements basic ACME ARI [0] support for Proxmox Backup
Server and Proxmox Datacenter Manager. Currently both projects renew
once a fixed time has passed:

* Proxmox Backup Manager already considers the life-time of a
  certificate and starts renewal attemps based on that [1]
* Proxmox Datacenter Manager still assumes that a certificate should
  start to be renewed 30 days before it is invalid.

This series changes the behavior to first attempt to get a time renewal
window from the server, and if that fails to fall back to the life-time
based lead percentages. Importantly it also moves the check for the
remaining life-time into the worker.

## Testing

The pebble acme server [2] is easy to set up and intended to be used
to develop clients against it. Changing the date of the system and the
system hosting pebble and then manually triggering a update check is a
easy way to test the behavior.

## Further worker

We currently only check the certificate with the daily update services.
It would be desirable to have a second service for ARI checks which runs
more often.
There also is currently no handling of Retry-After headers, which are
use by the ACME server to indicate when we should check for a new
renewal window again.

Proxmox Backup Server uses a very similar struct for Certificate
Information "CertInfo" which could be replaced by the one provided by
the proxmox-acme-api crate "CertificateInfo".

[0] https://datatracker.ietf.org/doc/rfc9773/
[1] https://lore.proxmox.com/pbs-devel/20260423134607.105229-2-m.federanko@proxmox.com/
[2] https://github.com/letsencrypt/pebble

proxmox:

Manuel Federanko (4):
  acme: client: add methods to fetch renewal information.
  acme: add retry-after header to renewal information.
  acme: allow specifying the certificate that is replaced by an order
  acme: cert: add dedicated ari_id field to the certificate info.

 proxmox-acme-api/src/certificate_helpers.rs | 60 ++++++++++++++++++-
 proxmox-acme-api/src/lib.rs                 |  5 +-
 proxmox-acme-api/src/types.rs               |  4 ++
 proxmox-acme/src/async_client.rs            | 66 +++++++++++++++++++--
 proxmox-acme/src/directory.rs               |  8 +++
 proxmox-acme/src/lib.rs                     |  3 +
 proxmox-acme/src/order.rs                   | 13 ++++
 proxmox-acme/src/renewal.rs                 | 36 +++++++++++
 8 files changed, 187 insertions(+), 8 deletions(-)
 create mode 100644 proxmox-acme/src/renewal.rs


proxmox-backup:

Manuel Federanko (2):
  acme: add ari_id to cert info.
  acme: implement ARI renewal information fetching.

 pbs-tools/Cargo.toml                   |   1 +
 pbs-tools/src/cert.rs                  |   4 +
 src/api2/node/certificates.rs          | 106 +++++++++++++++++++------
 src/bin/proxmox-daily-update.rs        |   6 --
 src/bin/proxmox_backup_manager/acme.rs |   8 --
 5 files changed, 86 insertions(+), 39 deletions(-)


proxmox-datacenter-manager:

Manuel Federanko (1):
  acme: certificates: fix #6372 use ARI for renewal if available.

 cli/admin/src/acme.rs                         |   7 -
 server/src/api/nodes/certificates.rs          | 129 +++++++++++++++---
 ...proxmox-datacenter-manager-daily-update.rs |   5 -
 3 files changed, 113 insertions(+), 28 deletions(-)


Summary over all repositories:
  16 files changed, 386 insertions(+), 75 deletions(-)

--
Generated by murpp 0.12.0

[PATCH proxmox 1/7] acme: client: add methods to fetch renewal information.

[Manuel Federanko]

Add new structs representing the renewal information returned by the
server.

Introduce a method on the client that fetches the renewal information
from the server. A helper method computes the certificate ID passed to
this method.

Signed-off-by: Manuel Federanko <m.federanko@proxmox.com>
---
 proxmox-acme-api/src/certificate_helpers.rs | 30 +++++++++++++++++++
 proxmox-acme-api/src/lib.rs                 |  5 +++-
 proxmox-acme/src/async_client.rs            | 31 +++++++++++++++++++
 proxmox-acme/src/directory.rs               |  8 +++++
 proxmox-acme/src/lib.rs                     |  3 ++
 proxmox-acme/src/renewal.rs                 | 33 +++++++++++++++++++++
 6 files changed, 109 insertions(+), 1 deletion(-)
 create mode 100644 proxmox-acme/src/renewal.rs

diff --git a/proxmox-acme-api/src/certificate_helpers.rs b/proxmox-acme-api/src/certificate_helpers.rs
index 3921b18e..7dc06c2d 100644
--- a/proxmox-acme-api/src/certificate_helpers.rs
+++ b/proxmox-acme-api/src/certificate_helpers.rs
@@ -31,6 +31,36 @@ pub struct OrderedCertificate {
     pub private_key_pem: Vec<u8>,
 }
 
+pub fn compute_ari_certificate_id(cert: &openssl::x509::X509) -> Option<String> {
+    let authority_key_identifier = match cert.authority_key_id() {
+        Some(v) => v.as_slice(),
+        None => return None,
+    };
+    let mut serial_number = (match cert.serial_number().to_bn() {
+        Ok(v) => v,
+        Err(_) => return None,
+    })
+    .to_vec();
+    if !serial_number.is_empty() && (serial_number[0] & 0x80) > 0 {
+        // check for negative numbers and prepend leading 0
+        serial_number.insert(0, 0);
+    }
+
+    let authority_key_identifier = proxmox_base64::url::encode_no_pad(authority_key_identifier);
+    let serial_number = proxmox_base64::url::encode_no_pad(serial_number);
+    Some(format!("{authority_key_identifier}.{serial_number}"))
+}
+
+pub async fn get_renewal_info(
+    acme_config: &AcmeConfig,
+    certificate_id: &str,
+) -> Result<Option<proxmox_acme::renewal::RenewalInformation>, Error> {
+    let mut acme = super::account_config::load_account_config(&acme_config.account)
+        .await?
+        .client();
+    acme.get_renewal_info(certificate_id).await
+}
+
 pub async fn order_certificate(
     worker: Arc<WorkerTask>,
     acme_config: &AcmeConfig,
diff --git a/proxmox-acme-api/src/lib.rs b/proxmox-acme-api/src/lib.rs
index c315d137..89a5e9a2 100644
--- a/proxmox-acme-api/src/lib.rs
+++ b/proxmox-acme-api/src/lib.rs
@@ -45,7 +45,10 @@ pub(crate) mod acme_plugin;
 #[cfg(feature = "impl")]
 mod certificate_helpers;
 #[cfg(feature = "impl")]
-pub use certificate_helpers::{create_self_signed_cert, order_certificate, revoke_certificate};
+pub use certificate_helpers::{
+    compute_ari_certificate_id, create_self_signed_cert, get_renewal_info, order_certificate,
+    revoke_certificate,
+};
 
 #[cfg(feature = "impl")]
 pub mod completion {
diff --git a/proxmox-acme/src/async_client.rs b/proxmox-acme/src/async_client.rs
index bba92023..6670bc24 100644
--- a/proxmox-acme/src/async_client.rs
+++ b/proxmox-acme/src/async_client.rs
@@ -335,6 +335,37 @@ impl AcmeClient {
         }
     }
 
+    /// Get the renewal information for a certificate
+    /// Returns None if the server does not support ARI
+    pub async fn get_renewal_info(
+        &mut self,
+        certificate_id: &str, // computed according to rfc9773 section 4.1
+    ) -> Result<Option<crate::renewal::RenewalInformation>, anyhow::Error> {
+        let directory = self.directory().await?;
+        if directory.renewal_info_url().is_none() {
+            return Ok(None);
+        }
+        let url = format!(
+            "{}/{}",
+            directory.renewal_info_url().unwrap(),
+            certificate_id,
+        );
+        let request = crate::request::Request {
+            url,
+            method: "GET",
+            content_type: crate::request::JSON_CONTENT_TYPE,
+            body: "".into(),
+            expected: &[crate::request::http_status::OK],
+        };
+        match Self::execute(&mut self.http_client, request, &mut self.nonce).await {
+            Ok(response) => {
+                let data: crate::renewal::RenewalInformationData = response.json()?;
+                Ok(Some(crate::renewal::RenewalInformation { data }))
+            }
+            Err(err) => Err(err.into()),
+        }
+    }
+
     fn need_account(account: &Option<Account>) -> Result<&Account, anyhow::Error> {
         account
             .as_ref()
diff --git a/proxmox-acme/src/directory.rs b/proxmox-acme/src/directory.rs
index b940901a..bbce4da5 100644
--- a/proxmox-acme/src/directory.rs
+++ b/proxmox-acme/src/directory.rs
@@ -38,6 +38,10 @@ pub struct DirectoryData {
     #[serde(skip_serializing_if = "Option::is_none")]
     pub key_change: Option<String>,
 
+    /// URL to get renewal information
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub renewal_info: Option<String>,
+
     /// Metadata object, for additional information which aren't directly part of the API
     /// itself, such as the terms of service.
     #[serde(skip_serializing_if = "Option::is_none")]
@@ -104,6 +108,10 @@ impl Directory {
         self.data.new_order.as_deref()
     }
 
+    pub(crate) fn renewal_info_url(&self) -> Option<&str> {
+        self.data.renewal_info.as_deref()
+    }
+
     /// Access to the in the Acme spec defined metadata structure.
     pub fn meta(&self) -> Option<&Meta> {
         self.data.meta.as_ref()
diff --git a/proxmox-acme/src/lib.rs b/proxmox-acme/src/lib.rs
index 6b774746..370d5ec0 100644
--- a/proxmox-acme/src/lib.rs
+++ b/proxmox-acme/src/lib.rs
@@ -43,6 +43,9 @@ pub mod error;
 #[cfg(feature = "impl")]
 pub mod order;
 
+#[cfg(feature = "impl")]
+pub mod renewal;
+
 #[cfg(feature = "impl")]
 pub mod util;
 
diff --git a/proxmox-acme/src/renewal.rs b/proxmox-acme/src/renewal.rs
new file mode 100644
index 00000000..eb4ff96a
--- /dev/null
+++ b/proxmox-acme/src/renewal.rs
@@ -0,0 +1,33 @@
+//! Acme renewal information
+use serde::{Deserialize, Serialize};
+
+/// The suggested renewal time window
+#[derive(Clone, Debug, Default, Deserialize, Serialize)]
+#[serde(rename_all = "camelCase")]
+pub struct SuggestedWindowData {
+    /// RFC3339 encoded time strings
+    pub start: String,
+    /// RFC3339 encoded time strings
+    pub end: String,
+}
+
+/// This contains the renewal information data returned by the ACME server
+#[derive(Clone, Debug, Default, Deserialize, Serialize)]
+#[serde(rename_all = "camelCase")]
+pub struct RenewalInformationData {
+    /// the suggested time window to renew the certificate
+    pub suggested_window: SuggestedWindowData,
+
+    /// explanatory URL why the windows is suggested
+    #[serde(skip_serializing_if = "Option::is_none")]
+    #[serde(rename = "explanatoryURL")]
+    pub explanation_url: Option<String>,
+}
+
+/// Renewal and retry information
+#[derive(Clone, Debug, Default, Deserialize, Serialize)]
+#[serde(rename_all = "kebab-case")]
+pub struct RenewalInformation {
+    /// the actual response of the acme server
+    pub data: RenewalInformationData,
+}
-- 
2.47.3

[PATCH proxmox 2/7] acme: add retry-after header to renewal information.

[Manuel Federanko]

This is not yet used but makes it easier to make the ARI workflow of
the client smarter later.

Signed-off-by: Manuel Federanko <m.federanko@proxmox.com>
---
 proxmox-acme/src/async_client.rs | 19 ++++++++++++++++++-
 proxmox-acme/src/renewal.rs      |  3 +++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/proxmox-acme/src/async_client.rs b/proxmox-acme/src/async_client.rs
index 6670bc24..c133a54e 100644
--- a/proxmox-acme/src/async_client.rs
+++ b/proxmox-acme/src/async_client.rs
@@ -360,7 +360,10 @@ impl AcmeClient {
         match Self::execute(&mut self.http_client, request, &mut self.nonce).await {
             Ok(response) => {
                 let data: crate::renewal::RenewalInformationData = response.json()?;
-                Ok(Some(crate::renewal::RenewalInformation { data }))
+                Ok(Some(crate::renewal::RenewalInformation {
+                    data,
+                    retry_after: response.retry_after,
+                }))
             }
             Err(err) => Err(err.into()),
         }
@@ -384,6 +387,7 @@ impl AcmeClient {
 struct AcmeResponse {
     body: Bytes,
     location: Option<String>,
+    retry_after: Option<String>,
     got_nonce: bool,
 }
 
@@ -470,9 +474,22 @@ impl AcmeClient {
                 })
                 .transpose()?;
 
+            let retry_after = parts
+                .headers
+                .get("Retry-After")
+                .map(|header| {
+                    header.to_str().map(str::to_owned).map_err(|err| {
+                        Error::Client(format!(
+                            "received invalid retry-after header from ACME server: {err}"
+                        ))
+                    })
+                })
+                .transpose()?;
+
             return Ok(AcmeResponse {
                 body,
                 location,
+                retry_after,
                 got_nonce,
             });
         }
diff --git a/proxmox-acme/src/renewal.rs b/proxmox-acme/src/renewal.rs
index eb4ff96a..6454affc 100644
--- a/proxmox-acme/src/renewal.rs
+++ b/proxmox-acme/src/renewal.rs
@@ -30,4 +30,7 @@ pub struct RenewalInformationData {
 pub struct RenewalInformation {
     /// the actual response of the acme server
     pub data: RenewalInformationData,
+
+    /// when to check the renewal endpoint again
+    pub retry_after: Option<String>,
 }
-- 
2.47.3

[PATCH proxmox 3/7] acme: allow specifying the certificate that is replaced by an order

[Manuel Federanko]

There isn't a foolproof way of determining if the id should be sent
along yet. If the request fails try again without the ari id.

Signed-off-by: Manuel Federanko <m.federanko@proxmox.com>
---
 proxmox-acme-api/src/certificate_helpers.rs | 27 ++++++++++++++++++---
 proxmox-acme/src/async_client.rs            | 18 +++++++++++---
 proxmox-acme/src/order.rs                   | 13 ++++++++++
 3 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/proxmox-acme-api/src/certificate_helpers.rs b/proxmox-acme-api/src/certificate_helpers.rs
index 7dc06c2d..5d35f86a 100644
--- a/proxmox-acme-api/src/certificate_helpers.rs
+++ b/proxmox-acme-api/src/certificate_helpers.rs
@@ -65,6 +65,7 @@ pub async fn order_certificate(
     worker: Arc<WorkerTask>,
     acme_config: &AcmeConfig,
     domains: &[AcmeDomain],
+    replaces_certificate_id: Option<&str>,
 ) -> Result<Option<OrderedCertificate>, Error> {
     use proxmox_acme::authorization::Status;
     use proxmox_acme::order::Identifier;
@@ -88,10 +89,30 @@ pub async fn order_certificate(
     let (plugins, _) = super::plugin_config::plugin_config()?;
 
     info!("Placing ACME order");
-
     let order = acme
-        .new_order(domains.iter().map(|d| d.domain.to_ascii_lowercase()))
-        .await?;
+        .new_order(
+            domains.iter().map(|d| d.domain.to_ascii_lowercase()),
+            replaces_certificate_id,
+        )
+        .await;
+    // there isn't really a nice way to know if a specific certificate
+    // is issued by the acme server, retry without specifying the id
+    // on failures
+    let order = match order {
+        Ok(o) => o,
+        Err(e) => if replaces_certificate_id.is_some() {
+            info!("Failed to place order, retrying without ARI id: {}", e);
+            acme
+                .new_order(
+                    domains.iter().map(|d| d.domain.to_ascii_lowercase()),
+                    None,
+                )
+                .await?
+        } else {
+            Err(e)?
+        },
+
+    };
 
     info!("Order URL: {}", order.location);
 
diff --git a/proxmox-acme/src/async_client.rs b/proxmox-acme/src/async_client.rs
index c133a54e..7738346e 100644
--- a/proxmox-acme/src/async_client.rs
+++ b/proxmox-acme/src/async_client.rs
@@ -172,15 +172,25 @@ impl AcmeClient {
     ///
     /// Please remember to persist the order somewhere (ideally along with the account data) in
     /// order to finish & query it later on.
-    pub async fn new_order<I>(&mut self, domains: I) -> Result<Order, anyhow::Error>
+    pub async fn new_order<I>(
+        &mut self,
+        domains: I,
+        replaces_certificate_id: Option<&str>,
+    ) -> Result<Order, anyhow::Error>
     where
         I: IntoIterator<Item = String>,
     {
         let account = Self::need_account(&self.account)?;
 
-        let order = domains
-            .into_iter()
-            .fold(OrderData::new(), |order, domain| order.domain(domain));
+        let order = {
+            let mut order = domains
+                .into_iter()
+                .fold(OrderData::new(), |order, domain| order.domain(domain));
+            if let Some(replaces_certificate_id) = replaces_certificate_id {
+                order = order.replaces(replaces_certificate_id);
+            }
+            order
+        };
 
         let mut retry = retry();
         loop {
diff --git a/proxmox-acme/src/order.rs b/proxmox-acme/src/order.rs
index d75fbde1..36ee5fe1 100644
--- a/proxmox-acme/src/order.rs
+++ b/proxmox-acme/src/order.rs
@@ -81,6 +81,10 @@ pub struct OrderData {
     /// List of identifiers to order for the certificate.
     pub identifiers: Vec<Identifier>,
 
+    /// Reference to the certificate being replaced
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub replaces: Option<String>,
+
     /// An RFC3339 formatted time string. It is up to the user to choose a dev dependency for this
     /// shit.
     #[serde(skip_serializing_if = "Option::is_none")]
@@ -120,6 +124,15 @@ impl OrderData {
         self.identifiers.push(Identifier::Dns(domain));
         self
     }
+
+    /// Builder-style method to specify which certificate this order replaces.
+    pub fn replaces<K>(mut self, ari_id: K) -> Self
+    where
+        K: ToString,
+    {
+        self.replaces = Some(ari_id.to_string());
+        self
+    }
 }
 
 /// Represents an order for a new certificate. This combines the order's own location (URL) with
-- 
2.47.3

[PATCH proxmox 4/7] acme: cert: add dedicated ari_id field to the certificate info.

[Manuel Federanko]

This is used often enough in the ARI implementation to warrant it's own
field. With that we don't have to create separate x509 instances and
carry them around just for that.

Signed-off-by: Manuel Federanko <m.federanko@proxmox.com>
---
 proxmox-acme-api/src/certificate_helpers.rs | 3 +++
 proxmox-acme-api/src/types.rs               | 4 ++++
 2 files changed, 7 insertions(+)

diff --git a/proxmox-acme-api/src/certificate_helpers.rs b/proxmox-acme-api/src/certificate_helpers.rs
index 5d35f86a..e7a2a16d 100644
--- a/proxmox-acme-api/src/certificate_helpers.rs
+++ b/proxmox-acme-api/src/certificate_helpers.rs
@@ -396,6 +396,8 @@ impl CertificateInfo {
             })
             .unwrap_or_default();
 
+        let ari_id = compute_ari_certificate_id(&x509);
+
         Ok(CertificateInfo {
             filename: filename.to_string(),
             pem: Some(cert_pem),
@@ -407,6 +409,7 @@ impl CertificateInfo {
             notafter: asn1_time_to_unix(x509.not_after()).ok(),
             public_key_type,
             san,
+            ari_id,
         })
     }
 
diff --git a/proxmox-acme-api/src/types.rs b/proxmox-acme-api/src/types.rs
index 934e6ece..00cd353d 100644
--- a/proxmox-acme-api/src/types.rs
+++ b/proxmox-acme-api/src/types.rs
@@ -60,6 +60,10 @@ pub struct CertificateInfo {
     /// The SSL Fingerprint.
     #[serde(skip_serializing_if = "Option::is_none")]
     pub fingerprint: Option<String>,
+
+    /// The ARI ID of this certificate
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub ari_id: Option<String>,
 }
 
 proxmox_schema::api_string_type! {
-- 
2.47.3

[PATCH proxmox-backup 5/7] acme: add ari_id to cert info.

[Manuel Federanko]

Signed-off-by: Manuel Federanko <m.federanko@proxmox.com>
---
 pbs-tools/Cargo.toml  | 1 +
 pbs-tools/src/cert.rs | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/pbs-tools/Cargo.toml b/pbs-tools/Cargo.toml
index 6b1d92fa6..e05dbac29 100644
--- a/pbs-tools/Cargo.toml
+++ b/pbs-tools/Cargo.toml
@@ -20,6 +20,7 @@ tokio = { workspace = true, features = [ "fs", "io-util", "rt", "rt-multi-thread
 tracing.workspace = true
 
 proxmox-async.workspace = true
+proxmox-acme-api.workspace = true
 proxmox-io = { workspace = true, features = [ "tokio" ] }
 proxmox-human-byte.workspace = true
 proxmox-log.workspace = true
diff --git a/pbs-tools/src/cert.rs b/pbs-tools/src/cert.rs
index 61ccce952..d29587eaa 100644
--- a/pbs-tools/src/cert.rs
+++ b/pbs-tools/src/cert.rs
@@ -101,4 +101,8 @@ impl CertInfo {
     pub fn is_expired_after_epoch(&self, epoch: i64) -> Result<bool, Error> {
         Ok(self.not_after_unix()? < epoch)
     }
+
+    pub fn ari_id(&self) -> Option<String> {
+        proxmox_acme_api::compute_ari_certificate_id(&self.x509)
+    }
 }
-- 
2.47.3

[PATCH proxmox-backup 6/7] acme: fix #6372 implement ARI renewal information fetching.

[Manuel Federanko]

Try to fetch ARI renewal information and renew based on that, if it is
not available fall back to normal lifetime based renewal logic.

The ARI check needs to talk to the server, move all checks into the
worker process and unconditionally call the worker process from all
entry points.

Add a method that returns the ARI ID from the CertInfo struct, which is
easier than trying to pass along other information or switching to
proxmox-acme-api's CertificateInfo struct.

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=6372
Signed-off-by: Manuel Federanko <m.federanko@proxmox.com>
---
 src/api2/node/certificates.rs          | 106 +++++++++++++++++++------
 src/bin/proxmox-daily-update.rs        |   6 --
 src/bin/proxmox_backup_manager/acme.rs |   8 --
 3 files changed, 81 insertions(+), 39 deletions(-)

diff --git a/src/api2/node/certificates.rs b/src/api2/node/certificates.rs
index 3df05b020..a6a4b67cc 100644
--- a/src/api2/node/certificates.rs
+++ b/src/api2/node/certificates.rs
@@ -1,11 +1,11 @@
-use anyhow::{Error, bail, format_err};
+use anyhow::{Error, format_err};
 use openssl::pkey::PKey;
 use openssl::x509::X509;
 use serde::{Deserialize, Serialize};
 use tracing::info;

 use pbs_api_types::{NODE_SCHEMA, PRIV_SYS_MODIFY};
-use proxmox_acme_api::AcmeDomain;
+use proxmox_acme_api::{AcmeConfig, AcmeDomain};
 use proxmox_rest_server::WorkerTask;
 use proxmox_router::SubdirMap;
 use proxmox_router::list_subdirs_api_method;
@@ -307,13 +307,6 @@ pub fn new_acme_cert(force: bool, rpcenv: &mut dyn RpcEnvironment) -> Result<Str
 /// Renew the current ACME certificate if it is within its renewal lead time (or always if the
 /// `force` parameter is set).
 pub fn renew_acme_cert(force: bool, rpcenv: &mut dyn RpcEnvironment) -> Result<String, Error> {
-    let (expires_soon, lead_days) = check_renewal_needed()?;
-    if !expires_soon && !force {
-        bail!(
-            "Certificate does not expire within the next {lead_days} days and 'force' is not set."
-        )
-    }
-
     spawn_certificate_worker("acme-renew-cert", force, rpcenv)
 }

@@ -341,17 +334,67 @@ fn cert_renew_lead_time(cert: &cert::CertInfo) -> i64 {
     }
 }

+/// ARI renewal time if available
+///
+/// Query the ARI endpoint for a suggested renewal window, draw a uniform random time in this window
+/// Return None if ARI does not apply.
+async fn cert_renew_lead_time_ari(
+    acme_config: &AcmeConfig,
+    cert_info: &cert::CertInfo,
+) -> Result<Option<i64>, Error> {
+    let now = proxmox_time::epoch_i64();
+    if cert_info.is_expired_after_epoch(now)? {
+        return Ok(Some(0));
+    }
+    let ari_id = &match cert_info.ari_id() {
+        Some(x) => x,
+        None => return Ok(None),
+    };
+    let window = match proxmox_acme_api::get_renewal_info(acme_config, &ari_id).await? {
+        Some(x) => x,
+        None => return Ok(None),
+    };
+    if let Some(reason) = window.data.explanation_url {
+        info!(
+            "Obtained renewal window, for information on this chosen window please visit {reason}"
+        );
+    }
+    let window_start = proxmox_time::parse_rfc3339(&window.data.suggested_window.start)?;
+    let window_end = proxmox_time::parse_rfc3339(&window.data.suggested_window.end)?;
+    let rand = proxmox_sys::linux::random_data(8)?
+        .into_iter()
+        .enumerate()
+        .fold(0, |acc, (index, x)| acc + ((x as u64) << (index * 8))) as f64
+        / (u64::MAX as f64);
+    let renew = window_start + (((window_end - window_start) as f64) * rand) as i64;
+    // need max since the randomness could result in negative values
+    Ok(Some(std::cmp::max(0, renew - now)))
+}
+
 /// Check whether the current certificate expires within its renewal lead time.
 ///
 /// Returns `(expires_soon, lead_time_in_days)`; the lead time is returned so callers can produce
 /// consistent user-facing messages without re-reading and re-parsing the certificate.
-pub fn check_renewal_needed() -> Result<(bool, i64), Error> {
-    let cert = pem_to_cert_info(get_certificate_pem()?.as_bytes())?;
-    let lead = cert_renew_lead_time(&cert);
-    let expires_soon = cert
-        .is_expired_after_epoch(proxmox_time::epoch_i64() + lead)
-        .map_err(|err| format_err!("Failed to check certificate expiration date: {}", err))?;
-    Ok((expires_soon, lead / SECONDS_PER_DAY))
+async fn check_renewal_needed(
+    acme_config: &AcmeConfig,
+    cert_info: &cert::CertInfo,
+) -> Result<bool, Error> {
+    let lead_ari = cert_renew_lead_time_ari(acme_config, cert_info).await?;
+    if let Some(lead_ari) = lead_ari {
+        // rfc9773 section 4.2 tells us to renew if the chosen renewal time would be before the next check
+        let expires_soon = lead_ari < SECONDS_PER_DAY;
+        let ts = proxmox_time::TimeSpan::from(std::time::Duration::new(lead_ari as u64, 0));
+        info!("Certificate is scheduled for renewal in {ts:.0} by ARI");
+        Ok(expires_soon)
+    } else {
+        let lead = cert_renew_lead_time(&cert_info);
+        let expires_soon = cert_info
+            .is_expired_after_epoch(proxmox_time::epoch_i64() + lead)
+            .map_err(|err| format_err!("Failed to check certificate expiration date: {}", err))?;
+        let ts = proxmox_time::TimeSpan::from(std::time::Duration::new(lead as u64, 0));
+        info!("Certificate renewal lead time is {ts:.0}");
+        Ok(expires_soon)
+    }
 }

 fn spawn_certificate_worker(
@@ -359,10 +402,6 @@ fn spawn_certificate_worker(
     force: bool,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<String, Error> {
-    // We only have 1 certificate path in PBS which makes figuring out whether or not it is a
-    // custom one too hard... We keep the parameter because the widget-toolkit may be using it...
-    let _ = force;
-
     let (node_config, _digest) = pbs_config::node::config()?;

     let auth_id = rpcenv.get_auth_id().unwrap();
@@ -384,11 +423,28 @@ fn spawn_certificate_worker(

     WorkerTask::spawn(name, None, auth_id, true, move |worker| async move {
         let work = || async {
-            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?;
+            let cert_info = pem_to_cert_info(get_certificate_pem()?.as_bytes())?;
+            let expires_soon = if !force {
+                let expires_soon = check_renewal_needed(&acme_config, &cert_info).await?;
+                if !expires_soon {
+                    info!("Certificate does not expire soon and 'force' was not set, not renewing");
+                }
+                expires_soon
+            } else {
+                false
+            };
+            if force || expires_soon {
+                if let Some(cert) = proxmox_acme_api::order_certificate(
+                    worker,
+                    &acme_config,
+                    &domains,
+                    cert_info.ari_id().as_deref(),
+                )
+                .await?
+                {
+                    crate::config::set_proxy_certificate(&cert.certificate, &cert.private_key_pem)?;
+                    crate::server::reload_proxy_certificate().await?;
+                }
             }

             Ok(())
diff --git a/src/bin/proxmox-daily-update.rs b/src/bin/proxmox-daily-update.rs
index 42ce62d16..eeadf9d13 100644
--- a/src/bin/proxmox-daily-update.rs
+++ b/src/bin/proxmox-daily-update.rs
@@ -74,12 +74,6 @@ async fn check_acme_certificates(rpcenv: &mut dyn RpcEnvironment) -> Result<(),
         return Ok(());
     }

-    let (expires_soon, lead_days) = api2::node::certificates::check_renewal_needed()?;
-    if !expires_soon {
-        log::info!("Certificate does not expire within the next {lead_days} days, not renewing.");
-        return Ok(());
-    }
-
     let info = &api2::node::certificates::API_METHOD_RENEW_ACME_CERT;
     let result = match info.handler {
         ApiHandler::Sync(handler) => (handler)(json!({}), info, rpcenv)?,
diff --git a/src/bin/proxmox_backup_manager/acme.rs b/src/bin/proxmox_backup_manager/acme.rs
index ed9e5868c..ea14cfe2b 100644
--- a/src/bin/proxmox_backup_manager/acme.rs
+++ b/src/bin/proxmox_backup_manager/acme.rs
@@ -413,14 +413,6 @@ pub fn plugin_cli() -> CommandLineInterface {
 )]
 /// Order a new ACME certificate.
 async fn order_acme_cert(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<(), Error> {
-    if !param["force"].as_bool().unwrap_or(false) {
-        let (expires_soon, lead_days) = api2::node::certificates::check_renewal_needed()?;
-        if !expires_soon {
-            println!("Certificate does not expire within the next {lead_days} days, not renewing.");
-            return Ok(());
-        }
-    }
-
     let info = &api2::node::certificates::API_METHOD_RENEW_ACME_CERT;
     let result = match info.handler {
         ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?,
--
2.47.3

[PATCH proxmox-datacenter-manager 7/7] acme: fix #6372 use ARI for renewal if available.

[Manuel Federanko]

Try to fetch ARI renewal information if it is available and renew based
on that. If not fall back to 1/3 of the remaining lifetime for
long-lived certificates or 1/2 of it for short-lived ones.
This thus also incorporates some of the changes already present in
backup server [0].

Since the ARI check needs to talk to the ACME directory the check is
move to the worker.

[0] https://lore.proxmox.com/pbs-devel/b8e5bd1b-bfbc-4b9e-befa-cd4b0157ed22@proxmox.com/

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=6372
Signed-off-by: Manuel Federanko <m.federanko@proxmox.com>
---
 cli/admin/src/acme.rs                         |   7 -
 server/src/api/nodes/certificates.rs          | 129 +++++++++++++++---
 ...proxmox-datacenter-manager-daily-update.rs |   5 -
 3 files changed, 113 insertions(+), 28 deletions(-)

diff --git a/cli/admin/src/acme.rs b/cli/admin/src/acme.rs
index e61bb1ef..be6e0018 100644
--- a/cli/admin/src/acme.rs
+++ b/cli/admin/src/acme.rs
@@ -405,13 +405,6 @@ pub fn plugin_cli() -> CommandLineInterface {
 )]
 /// Order a new ACME certificate.
 async fn order_acme_cert(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<(), Error> {
-    if !param["force"].as_bool().unwrap_or(false)
-        && !dc_api::nodes::certificates::cert_expires_soon()?
-    {
-        println!("Certificate does not expire within the next 30 days, not renewing.");
-        return Ok(());
-    }
-
     let info = &dc_api::nodes::certificates::API_METHOD_RENEW_ACME_CERT;
     let result = match info.handler {
         ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?,
diff --git a/server/src/api/nodes/certificates.rs b/server/src/api/nodes/certificates.rs
index 0f391499..08838bea 100644
--- a/server/src/api/nodes/certificates.rs
+++ b/server/src/api/nodes/certificates.rs
@@ -1,4 +1,4 @@
-use anyhow::{Context, Error, bail, format_err};
+use anyhow::{Context, Error, format_err};
 use openssl::pkey::PKey;
 use openssl::x509::X509;

@@ -8,7 +8,7 @@ use proxmox_router::list_subdirs_api_method;
 use proxmox_router::{Permission, Router, RpcEnvironment};
 use proxmox_schema::api;

-use proxmox_acme_api::{AcmeDomain, CertificateInfo};
+use proxmox_acme_api::{AcmeConfig, AcmeDomain, CertificateInfo};

 use proxmox_rest_server::WorkerTask;
 use proxmox_schema::api_types::NODE_SCHEMA;
@@ -43,12 +43,14 @@ const ACME_SUBDIRS: SubdirMap = &[(
         .put(&API_METHOD_RENEW_ACME_CERT),
 )];

+const SECONDS_PER_DAY: i64 = 24 * 60 * 60;
+
 fn get_certificate_pem() -> Result<Vec<u8>, Error> {
     let cert_pem = proxmox_sys::fs::file_get_contents(API_CERT_FN)?;
     Ok(cert_pem)
 }

-fn get_certificate_info() -> Result<CertificateInfo, Error> {
+pub fn get_certificate_info() -> Result<CertificateInfo, Error> {
     let cert_pem = get_certificate_pem()?;
     CertificateInfo::from_pem("proxy.pem", &cert_pem)
 }
@@ -233,18 +235,93 @@ pub fn new_acme_cert(force: bool, rpcenv: &mut dyn RpcEnvironment) -> Result<Str
 /// Renew the current ACME certificate if it expires within 30 days (or always if the `force`
 /// parameter is set).
 pub fn renew_acme_cert(force: bool, rpcenv: &mut dyn RpcEnvironment) -> Result<String, Error> {
-    if !cert_expires_soon()? && !force {
-        bail!("Certificate does not expire within the next 30 days and 'force' is not set.")
+    spawn_certificate_worker("acme-renew-cert", force, rpcenv)
+}
+
+/// Renewal lead time in seconds for the given certificate.
+///
+/// Long-lived certs are renewed once 2/3 of their lifetime has elapsed; short-lived ones (under
+/// ten days) already at 1/2, following Let's Encrypt's integration guide. A 3-day floor still
+/// applies so the daily-update service has a couple of chances to retry transient failures.
+fn cert_renew_lead_time(cert: &CertificateInfo) -> i64 {
+    if let (Some(notafter), Some(notbefore)) = (cert.notafter, cert.notbefore) {
+        let lifetime = notafter - notbefore;
+        let scale = if lifetime < 10 * SECONDS_PER_DAY {
+            2
+        } else {
+            3
+        };
+        std::cmp::max(lifetime / scale, 3 * SECONDS_PER_DAY)
+    } else {
+        log::warn!(
+            "certificate notBefore/notAfter unavailable, falling back to 30-day renewal lead time"
+        );
+        30 * SECONDS_PER_DAY
     }
+}

-    spawn_certificate_worker("acme-renew-cert", force, rpcenv)
+/// ARI renewal time if available
+///
+/// Query the ARI endpoint for a suggested renewal window, draw a uniform random time in this window
+/// Return None if ARI does not apply.
+async fn cert_renew_lead_time_ari(
+    acme_config: &AcmeConfig,
+    cert_info: &CertificateInfo,
+) -> Result<Option<i64>, Error> {
+    let now = proxmox_time::epoch_i64();
+    if cert_info.is_expired_after_epoch(now)? {
+        return Ok(Some(0));
+    }
+    let ari_id = match cert_info.ari_id.as_deref() {
+        Some(x) => x,
+        None => return Ok(None),
+    };
+
+    let window = match proxmox_acme_api::get_renewal_info(acme_config, ari_id).await? {
+        Some(x) => x,
+        None => return Ok(None),
+    };
+    if let Some(reason) = window.data.explanation_url {
+        info!(
+            "Obtained renewal window, for information on this chosen window please visit {reason}"
+        );
+    }
+    let window_start = proxmox_time::parse_rfc3339(&window.data.suggested_window.start)?;
+    let window_end = proxmox_time::parse_rfc3339(&window.data.suggested_window.end)?;
+    let rand = proxmox_sys::linux::random_data(8)?
+        .into_iter()
+        .enumerate()
+        .fold(0, |acc, (index, x)| acc + ((x as u64) << (index * 8))) as f64
+        / (u64::MAX as f64);
+    let renew = window_start + (((window_end - window_start) as f64) * rand) as i64;
+    // need max since the randomness could result in negative values
+    Ok(Some(std::cmp::max(0, renew - now)))
 }

-/// Check whether the current certificate expires within the next 30 days.
-pub fn cert_expires_soon() -> Result<bool, Error> {
-    let cert = get_certificate_info()?;
-    cert.is_expired_after_epoch(proxmox_time::epoch_i64() + 30 * 24 * 60 * 60)
-        .map_err(|err| format_err!("Failed to check certificate expiration date: {}", err))
+/// Should the certificate be renewed now.
+///
+/// Is true if the ceriticates expires within its lead time.
+/// Returns if the certificate should be renewed and the lead time in days.
+pub async fn check_renewal_needed(
+    acme_config: &AcmeConfig,
+    cert_info: &CertificateInfo,
+) -> Result<bool, Error> {
+    let lead_ari = cert_renew_lead_time_ari(acme_config, cert_info).await?;
+    if let Some(lead_ari) = lead_ari {
+        // rfc9773 section 4.2 tells us to renew if the chosen renewal time would be before the next check
+        let expires_soon = lead_ari < SECONDS_PER_DAY;
+        let ts = proxmox_time::TimeSpan::from(std::time::Duration::new(lead_ari as u64, 0));
+        info!("Certificate is scheduled for renewal in {ts:.0} by ARI");
+        Ok(expires_soon)
+    } else {
+        let lead = cert_renew_lead_time(cert_info);
+        let expires_soon = cert_info
+            .is_expired_after_epoch(proxmox_time::epoch_i64() + lead)
+            .map_err(|err| format_err!("Failed to check certificate expiration date: {}", err))?;
+        let ts = proxmox_time::TimeSpan::from(std::time::Duration::new(lead as u64, 0));
+        info!("Certificate renewal lead time is {ts:.0}");
+        Ok(expires_soon)
+    }
 }

 fn spawn_certificate_worker(
@@ -281,11 +358,31 @@ fn spawn_certificate_worker(

     WorkerTask::spawn(name, None, auth_id, true, move |worker| async move {
         let work = || async {
-            if let Some(cert) =
-                proxmox_acme_api::order_certificate(worker, &acme_config, &domains).await?
-            {
-                crate::auth::certs::set_api_certificate(&cert.certificate, &cert.private_key_pem)?;
-                crate::reload_api_certificate().await?;
+            let cert_info = get_certificate_info()?;
+            let expires_soon = if !force {
+                let expires_soon = check_renewal_needed(&acme_config, &cert_info).await?;
+                if !expires_soon {
+                    info!("Certificate does not expire soon and 'force' was not set, not renewing");
+                }
+                expires_soon
+            } else {
+                false
+            };
+            if force || expires_soon {
+                if let Some(cert) = proxmox_acme_api::order_certificate(
+                    worker,
+                    &acme_config,
+                    &domains,
+                    cert_info.ari_id.as_deref(),
+                )
+                .await?
+                {
+                    crate::auth::certs::set_api_certificate(
+                        &cert.certificate,
+                        &cert.private_key_pem,
+                    )?;
+                    crate::reload_api_certificate().await?;
+                }
             }

             Ok(())
diff --git a/server/src/bin/proxmox-datacenter-manager-daily-update.rs b/server/src/bin/proxmox-datacenter-manager-daily-update.rs
index 314b3399..e70033a4 100644
--- a/server/src/bin/proxmox-datacenter-manager-daily-update.rs
+++ b/server/src/bin/proxmox-datacenter-manager-daily-update.rs
@@ -72,11 +72,6 @@ async fn check_acme_certificates(rpcenv: &mut dyn RpcEnvironment) -> Result<(),
         return Ok(());
     }

-    if !api::nodes::certificates::cert_expires_soon()? {
-        log::info!("Certificate does not expire within the next 30 days, not renewing.");
-        return Ok(());
-    }
-
     let info = &api::nodes::certificates::API_METHOD_RENEW_ACME_CERT;
     let result = match info.handler {
         ApiHandler::Sync(handler) => (handler)(json!({}), info, rpcenv)?,
--
2.47.3