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 0121C1FF13B for ; Wed, 22 Apr 2026 11:10:08 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D23ED11315; Wed, 22 Apr 2026 11:10:07 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 22 Apr 2026 11:10:03 +0200 Message-Id: To: "Manuel Federanko" , Subject: Re: [PATCH proxmox-backup] acme: partially fix #6372: scale certificate renewal checks by lifetime X-Mailer: aerc 0.20.0 References: <20260421144645.275884-1-m.federanko@proxmox.com> In-Reply-To: <20260421144645.275884-1-m.federanko@proxmox.com> From: "Shannon Sterz" X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1776848916605 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.122 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 SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox-daily-update.rs,certificates.rs,proxmox.com,acme.rs,letsencrypt.org] Message-ID-Hash: 3EPBIHHUY6IAZB7VABM4US5FKJX63BE6 X-Message-ID-Hash: 3EPBIHHUY6IAZB7VABM4US5FKJX63BE6 X-MailFrom: s.sterz@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 Tue Apr 21, 2026 at 4:46 PM CEST, Manuel Federanko wrote: > Start renewing a certificate once 2/3 of its total lifetime have passed, > instead of the hardcoded 30 days. This stays consistent with many > certificates, which are valid for 90 days. > > The update service runs daily, impose a 3 day minimum remaining lifetime > to still be able to handle transient failures for certificate renewals. > > Signed-off-by: Manuel Federanko > Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=3D6372 > --- > src/api2/node/certificates.rs | 21 +++++++++++++++++++-- > src/bin/proxmox-daily-update.rs | 3 ++- > src/bin/proxmox_backup_manager/acme.rs | 3 ++- > 3 files changed, 23 insertions(+), 4 deletions(-) > > diff --git a/src/api2/node/certificates.rs b/src/api2/node/certificates.r= s > index a69f6511..6e7b3326 100644 > --- a/src/api2/node/certificates.rs > +++ b/src/api2/node/certificates.rs > @@ -306,16 +306,33 @@ pub fn new_acme_cert(force: bool, rpcenv: &mut dyn = RpcEnvironment) -> Result /// parameter is set). > pub fn renew_acme_cert(force: bool, rpcenv: &mut dyn RpcEnvironment) -> = Result { > if !cert_expires_soon()? && !force { > - bail!("Certificate does not expire within the next 30 days and '= force' is not set.") > + let lead =3D cert_renew_lead_time()? / (24 * 60 * 60); > + bail!("Certificate does not expire within the next {lead} days a= nd 'force' is not set.") > } > > spawn_certificate_worker("acme-renew-cert", force, rpcenv) > } > > +/// When to start checking for new certs. > +pub fn cert_renew_lead_time() -> Result { > + let cert =3D pem_to_cert_info(get_certificate_pem()?.as_bytes())?; > + if let (Some(notafter), Some(notbefore)) =3D > + (cert.not_after_unix().ok(), cert.not_before_unix().ok()) no need to convert between `Option` and `Result` here. simply do: if let (Ok(notafter), Ok(notbefore)) =3D (cert.not_after_unix(), cert.n= ot_before_unix()) { here :) > + { > + // gets usually checked every day by the daily-update service, > + // start checking at least 3 days before expiry > + let lifetime =3D notafter - notbefore; > + let lead =3D std::cmp::max(lifetime / 3, 3 * 24 * 60 * 60); i talked to Fabian a bit and we came to the following consensus regarding the 3 day cut-off here. this function should probably just encode the lead time itself, not the 3 day cut off as that is an artifact of how we refresh acme certificates in the daily-update task. essentially the plan is to have this function return the lead time as follows: 1. for short-lived certificates (< 10 days) the lead time should be half of the lifetime of the certificate [1]. 2. for other certificates the lead time should be 1/3 of the certificates lifetime [2]. this is based on recommendations by let's encrypt [1,2]. for ultra-short-lived certificates, however, the daily update service will only have very limited chances to successfully renew the certificate, since it will only run once a day. hence, the 3 day cut-off should be moved to the daily update service. the service should check the validity of the certificate with the 3 day cut-off in mind. it should then call the acme renewal endpoint with the `force` parameter set to `true` to by-pass the validity check based on the lead time outlined above. does that make sense? [1]: https://letsencrypt.org/docs/integration-guide/ [2]: https://letsencrypt.org/2025/12/02/from-90-to-45#action-required > + Ok(lead) > + } else { > + Ok(30 * 24 * 60 * 60) > + } tiny nit: imo it might be a bit cleaner to do `return Ok(lead)` in the if branch above, drop the else and return `Ok(30 * 24 * 60 * 60)` by default. > +} > + > /// Check whether the current certificate expires within the next 30 day= s. > pub fn cert_expires_soon() -> Result { > let cert =3D pem_to_cert_info(get_certificate_pem()?.as_bytes())?; > - cert.is_expired_after_epoch(proxmox_time::epoch_i64() + 30 * 24 * 60= * 60) > + cert.is_expired_after_epoch(proxmox_time::epoch_i64() + cert_renew_l= ead_time()?) > .map_err(|err| format_err!("Failed to check certificate expirati= on date: {}", err)) > } > > diff --git a/src/bin/proxmox-daily-update.rs b/src/bin/proxmox-daily-upda= te.rs > index c4d68e30..e5e96eb9 100644 > --- a/src/bin/proxmox-daily-update.rs > +++ b/src/bin/proxmox-daily-update.rs > @@ -75,7 +75,8 @@ async fn check_acme_certificates(rpcenv: &mut dyn RpcEn= vironment) -> Result<(), > } > > if !api2::node::certificates::cert_expires_soon()? { > - log::info!("Certificate does not expire within the next 30 days,= not renewing."); > + let lead =3D api2::node::certificates::cert_renew_lead_time()? /= (24 * 60 * 60); > + log::info!("Certificate does not expire within the next {lead} d= ays, not renewing."); > return Ok(()); > } > > diff --git a/src/bin/proxmox_backup_manager/acme.rs b/src/bin/proxmox_bac= kup_manager/acme.rs > index 57431225..d1a2323f 100644 > --- a/src/bin/proxmox_backup_manager/acme.rs > +++ b/src/bin/proxmox_backup_manager/acme.rs > @@ -415,7 +415,8 @@ pub fn plugin_cli() -> CommandLineInterface { > async fn order_acme_cert(param: Value, rpcenv: &mut dyn RpcEnvironment) = -> Result<(), Error> { > if !param["force"].as_bool().unwrap_or(false) && !api2::node::certif= icates::cert_expires_soon()? > { > - println!("Certificate does not expire within the next 30 days, n= ot renewing."); > + let lead =3D api2::node::certificates::cert_renew_lead_time()? /= (24 * 60 * 60); > + println!("Certificate does not expire within the next {lead} day= s, not renewing."); > return Ok(()); > } >