From 7ee49baf8485278e34200bb2ce923bb0f7604962 Mon Sep 17 00:00:00 2001 From: Aleksei Bavshin Date: Wed, 20 Aug 2025 11:32:21 -0700 Subject: [PATCH 01/10] Avoid allocating String in CertificateOrder::cache_key. Also, prefer first available DNS name. --- src/conf/order.rs | 44 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 9 deletions(-) diff --git a/src/conf/order.rs b/src/conf/order.rs index 7e2483c..2e5f23c 100644 --- a/src/conf/order.rs +++ b/src/conf/order.rs @@ -43,20 +43,22 @@ where } /// Generates a stable unique identifier for this order. - pub fn cache_key(&self) -> std::string::String + pub fn cache_key(&self) -> PrintableOrderId<'_, S, A> where S: fmt::Display + hash::Hash, { - if self.identifiers.is_empty() { - return "".into(); - } - - let name = self.identifiers[0].value(); + PrintableOrderId(self) + } - let mut hasher = SipHasher::default(); - self.hash(&mut hasher); + /// Attempts to find the first DNS identifier, with fallback to a first identifier of any kind. + pub fn first_name(&self) -> Option<&S> { + let dns = self + .identifiers + .iter() + .find(|x| matches!(x, Identifier::Dns(_))); - std::format!("{name}-{hash:x}", hash = hasher.finish()) + dns.or_else(|| self.identifiers.first()) + .map(Identifier::value) } pub fn to_str_order(&self, alloc: NewA) -> CertificateOrder<&str, NewA> @@ -247,6 +249,30 @@ impl CertificateOrder { } } +/// Unique identifier for the CertificateOrder. +/// +/// This identifier should be suitable for logs, file names or cache keys. +pub struct PrintableOrderId<'a, S, A>(&'a CertificateOrder) +where + A: ngx::allocator::Allocator; + +impl fmt::Display for PrintableOrderId<'_, S, A> +where + A: ngx::allocator::Allocator, + S: fmt::Display + hash::Hash, +{ + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let Some(name) = self.0.first_name() else { + return Ok(()); + }; + + let mut hasher = SipHasher::default(); + self.0.hash(&mut hasher); + + write!(f, "{name}-{hash:x}", hash = hasher.finish()) + } +} + fn validate_host(pool: &Pool, mut host: ngx_str_t) -> Result { let mut pool = pool.clone(); let rc = Status(unsafe { nginx_sys::ngx_http_validate_host(&mut host, pool.as_mut(), 1) }); From 1ec24c8057adb416b72cfdc943f4abe09ced3c25 Mon Sep 17 00:00:00 2001 From: Aleksei Bavshin Date: Wed, 20 Aug 2025 17:17:33 -0700 Subject: [PATCH 02/10] Do not log renewal checks for invalid certificates. --- src/lib.rs | 6 +++++- src/state/certificate.rs | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 856fc56..270a48e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -285,10 +285,14 @@ async fn ngx_http_acme_update_certificates_for_issuer( { let locked = cert.read(); + if !locked.is_valid() { + continue; + } + if !locked.is_renewable() { ngx_log_debug!( log.as_ptr(), - "acme: certificate \"{}/{}\" is not renewable", + "acme: certificate \"{}/{}\" is not due for renewal", issuer.name, order.cache_key() ); diff --git a/src/state/certificate.rs b/src/state/certificate.rs index 69cc3df..690f0fe 100644 --- a/src/state/certificate.rs +++ b/src/state/certificate.rs @@ -243,7 +243,11 @@ where } pub fn is_renewable(&self) -> bool { - !matches!(self.state, CertificateState::Invalid { .. }) && Time::now() >= self.next + self.is_valid() && Time::now() >= self.next + } + + pub fn is_valid(&self) -> bool { + !matches!(self.state, CertificateState::Invalid { .. }) } } From 6d1a12c419d2d9a53601a8a0bfb8918ffa87fc83 Mon Sep 17 00:00:00 2001 From: Aleksei Bavshin Date: Thu, 31 Jul 2025 15:14:15 -0700 Subject: [PATCH 03/10] ACME: replace anyhow with error enums. --- src/acme.rs | 133 +++++++++++++++++++++++------------------ src/acme/error.rs | 149 ++++++++++++++++++++++++++++++++++++++++++++++ src/lib.rs | 36 +++++------ 3 files changed, 239 insertions(+), 79 deletions(-) create mode 100644 src/acme/error.rs diff --git a/src/acme.rs b/src/acme.rs index 21b1762..43b7de7 100644 --- a/src/acme.rs +++ b/src/acme.rs @@ -9,8 +9,8 @@ use core::time::Duration; use std::collections::VecDeque; use std::string::{String, ToString}; -use anyhow::{anyhow, Result}; use bytes::Bytes; +use error::{NewAccountError, NewCertificateError, RequestError}; use http::Uri; use ngx::allocator::{Allocator, Box}; use ngx::async_::sleep; @@ -18,8 +18,9 @@ use ngx::collections::Vec; use ngx::ngx_log_debug; use openssl::pkey::{PKey, PKeyRef, Private}; use openssl::x509::{self, extension as x509_ext, X509Req}; +use types::ProblemCategory; -use self::account_key::AccountKey; +use self::account_key::{AccountKey, AccountKeyError}; use self::types::{AuthorizationStatus, ChallengeKind, ChallengeStatus, OrderStatus}; use crate::conf::identifier::Identifier; use crate::conf::issuer::Issuer; @@ -28,6 +29,7 @@ use crate::net::http::HttpClient; use crate::time::Time; pub mod account_key; +pub mod error; pub mod solvers; pub mod types; @@ -97,8 +99,13 @@ fn try_get_header( impl<'a, Http> AcmeClient<'a, Http> where Http: HttpClient, + RequestError: From<::Error>, { - pub fn new(http: Http, issuer: &'a Issuer, log: NonNull) -> Result { + pub fn new( + http: Http, + issuer: &'a Issuer, + log: NonNull, + ) -> Result { let key = AccountKey::try_from( issuer .pkey @@ -137,21 +144,21 @@ where self.solvers.iter().any(|s| s.supports(kind)) } - async fn get_directory(&self) -> Result { + async fn get_directory(&self) -> Result { let res = self.get(&self.issuer.uri).await?; - let directory = serde_json::from_slice(res.body())?; + let directory = deserialize_body(res.body())?; Ok(directory) } - async fn get_nonce(&self) -> Result { + async fn get_nonce(&self) -> Result { let res = self.get(&self.directory.new_nonce).await?; try_get_header(res.headers(), &REPLAY_NONCE) - .ok_or(anyhow!("no nonce in response headers")) + .ok_or(RequestError::Nonce) .map(String::from) } - pub async fn get(&self, url: &Uri) -> Result> { + pub async fn get(&self, url: &Uri) -> Result, RequestError> { let req = http::Request::builder() .uri(url) .method(http::Method::GET) @@ -164,7 +171,7 @@ where &self, url: &Uri, payload: P, - ) -> Result> { + ) -> Result, RequestError> { let mut nonce = if let Some(nonce) = self.nonce.get() { nonce } else { @@ -215,10 +222,10 @@ where // 8555.6.5, when retrying in response to a "badNonce" error, the client MUST use // the nonce provided in the error response. nonce = try_get_header(res.headers(), &REPLAY_NONCE) - .ok_or(anyhow!("no nonce in response"))? + .ok_or(RequestError::Nonce)? .to_string(); - let err: types::Problem = serde_json::from_slice(res.body())?; + let err: types::Problem = deserialize_body(res.body())?; let retriable = matches!( err.kind, @@ -239,20 +246,23 @@ where Ok(res) } - pub async fn new_account(&mut self) -> Result { - self.directory = self.get_directory().await?; + pub async fn new_account(&mut self) -> Result<(), NewAccountError> { + self.directory = self + .get_directory() + .await + .map_err(NewAccountError::Directory)?; if self.directory.meta.external_account_required == Some(true) && self.issuer.eab_key.is_none() { - return Err(anyhow!("external account key required")); + return Err(NewAccountError::ExternalAccount); } let external_account_binding = self .issuer .eab_key .as_ref() - .map(|x| -> Result<_> { + .map(|x| -> Result<_, RequestError> { let key = crate::jws::ShaWithHmacKey::new(&x.key, 256); let payload = serde_json::to_vec(&self.key)?; let message = crate::jws::sign_jws( @@ -273,19 +283,16 @@ where ..Default::default() }; - let payload = serde_json::to_string(&payload)?; + let payload = serde_json::to_string(&payload).map_err(RequestError::RequestFormat)?; let res = self.post(&self.directory.new_account, payload).await?; - let key_id = res - .headers() - .get("location") - .ok_or(anyhow!("account URL unavailable"))? - .to_str()? - .to_string(); - self.account = Some(key_id); + let key_id: &str = + try_get_header(res.headers(), http::header::LOCATION).ok_or(NewAccountError::Url)?; + + self.account = Some(key_id.to_string()); - Ok(serde_json::from_slice(res.body())?) + Ok(()) } pub fn is_ready(&self) -> bool { @@ -295,7 +302,7 @@ where pub async fn new_certificate( &self, req: &CertificateOrder<&str, A>, - ) -> Result + ) -> Result where A: Allocator, { @@ -313,30 +320,27 @@ where not_after: None, }; - let payload = serde_json::to_string(&payload)?; + let payload = serde_json::to_string(&payload).map_err(RequestError::RequestFormat)?; let res = self.post(&self.directory.new_order, payload).await?; - let order_url = res - .headers() - .get("location") - .and_then(|x| x.to_str().ok()) - .ok_or(anyhow!("no order URL"))?; + let order_url = try_get_header(res.headers(), http::header::LOCATION) + .and_then(|x| Uri::try_from(x).ok()) + .ok_or(NewCertificateError::OrderUrl)?; - let order_url = Uri::try_from(order_url)?; - let order: types::Order = serde_json::from_slice(res.body())?; + let order: types::Order = deserialize_body(res.body())?; let mut authorizations: Vec<(http::Uri, types::Authorization)> = Vec::new(); for auth_url in order.authorizations { let res = self.post(&auth_url, b"").await?; - let mut authorization: types::Authorization = serde_json::from_slice(res.body())?; + let mut authorization: types::Authorization = deserialize_body(res.body())?; authorization .challenges .retain(|x| self.is_supported_challenge(&x.kind)); if authorization.challenges.is_empty() { - anyhow::bail!("no supported challenge for {:?}", authorization.identifier) + return Err(NewCertificateError::NoSupportedChallenges); } match authorization.status { @@ -351,11 +355,7 @@ where authorization.identifier ); } - status => anyhow::bail!( - "unexpected authorization status for {:?}: {:?}", - authorization.identifier, - status - ), + status => return Err(NewCertificateError::AuthorizationStatus(status)), } } @@ -371,27 +371,35 @@ where } let mut res = self.post(&order_url, b"").await?; - let mut order: types::Order = serde_json::from_slice(res.body())?; + let mut order: types::Order = deserialize_body(res.body())?; if order.status != OrderStatus::Ready { - anyhow::bail!("not ready"); + if let Some(err) = order.error { + return Err(err.into()); + } + return Err(NewCertificateError::OrderStatus(order.status)); } - let csr = make_certificate_request(&order.identifiers, &pkey).and_then(|x| x.to_der())?; + let csr = make_certificate_request(&order.identifiers, &pkey) + .and_then(|x| x.to_der()) + .map_err(NewCertificateError::Csr)?; let payload = std::format!(r#"{{"csr":"{}"}}"#, crate::jws::base64url(csr)); match self.post(&order.finalize, payload).await { Ok(x) => { drop(order); res = x; - order = serde_json::from_slice(res.body())?; + order = deserialize_body(res.body())?; } - Err(err) => { - if !err.to_string().contains("orderNotReady") { - return Err(err); - } - order.status = OrderStatus::Processing + Err(RequestError::Protocol(problem)) + if matches!( + problem.category(), + ProblemCategory::Order | ProblemCategory::Malformed + ) => + { + return Err(problem.into()) } + _ => order.status = OrderStatus::Processing, }; let mut tries = backoff(MAX_RETRY_INTERVAL, self.finalize_timeout); @@ -399,10 +407,12 @@ where while order.status == OrderStatus::Processing && wait_for_retry(&res, &mut tries).await { drop(order); res = self.post(&order_url, b"").await?; - order = serde_json::from_slice(res.body())?; + order = deserialize_body(res.body())?; } - let certificate = order.certificate.ok_or(anyhow!("certificate not ready"))?; + let certificate = order + .certificate + .ok_or(NewCertificateError::CertificateUrl)?; let chain = self.post(&certificate, b"").await?.into_body(); @@ -414,7 +424,7 @@ where order: &AuthorizationContext<'_>, url: http::Uri, authorization: types::Authorization, - ) -> Result<()> { + ) -> Result<(), NewCertificateError> { let identifier = authorization.identifier.as_ref(); // Find and set up first supported challenge. @@ -425,7 +435,7 @@ where let solver = self.find_solver_for(&x.kind)?; Some((x, solver)) }) - .ok_or(anyhow!("no supported challenge for {identifier:?}"))?; + .ok_or(NewCertificateError::NoSupportedChallenges)?; solver.register(order, &identifier, challenge)?; @@ -434,12 +444,12 @@ where }; let res = self.post(&challenge.url, b"{}").await?; - let result: types::Challenge = serde_json::from_slice(res.body())?; + let result: types::Challenge = deserialize_body(res.body())?; if !matches!( result.status, ChallengeStatus::Pending | ChallengeStatus::Processing | ChallengeStatus::Valid ) { - return Err(anyhow!("unexpected challenge status {:?}", result.status)); + return Err(NewCertificateError::ChallengeStatus(result.status)); } let mut tries = backoff(MAX_RETRY_INTERVAL, self.authorization_timeout); @@ -447,7 +457,7 @@ where let result = loop { let res = self.post(&url, b"").await?; - let result: types::Authorization = serde_json::from_slice(res.body())?; + let result: types::Authorization = deserialize_body(res.body())?; if result.status != AuthorizationStatus::Pending || !wait_for_retry(&res, &mut tries).await @@ -464,7 +474,7 @@ where ); if result.status != AuthorizationStatus::Valid { - return Err(anyhow!("authorization failed ({:?})", result.status)); + return Err(NewCertificateError::AuthorizationStatus(result.status)); } Ok(()) @@ -539,6 +549,15 @@ fn backoff(max: Duration, timeout: Duration) -> impl Iterator { .map(move |(_, x)| x.min(max)) } +/// Deserializes JSON response body as T and converts error type. +#[inline(always)] +fn deserialize_body<'a, T>(bytes: &'a Bytes) -> Result +where + T: serde::Deserialize<'a>, +{ + serde_json::from_slice(bytes).map_err(RequestError::ResponseFormat) +} + fn parse_retry_after(val: &http::HeaderValue) -> Option { let val = val.to_str().ok()?; diff --git a/src/acme/error.rs b/src/acme/error.rs new file mode 100644 index 0000000..4abc70d --- /dev/null +++ b/src/acme/error.rs @@ -0,0 +1,149 @@ +// Copyright (c) F5, Inc. +// +// This source code is licensed under the Apache License, Version 2.0 license found in the +// LICENSE file in the root directory of this source tree. + +use core::error::Error as StdError; + +use ngx::allocator::{unsize_box, Box}; +use thiserror::Error; + +use super::solvers::SolverError; +use super::types::{Problem, ProblemCategory}; +use crate::net::http::HttpClientError; + +#[derive(Debug, Error)] +pub enum NewAccountError { + #[error("directory update failed ({0})")] + Directory(RequestError), + + #[error("external account key required")] + ExternalAccount, + + #[error(transparent)] + Protocol(#[from] Problem), + + #[error("account request failed ({0})")] + Request(RequestError), + + #[error("no account URL in response")] + Url, +} + +impl From for NewAccountError { + fn from(value: RequestError) -> Self { + match value { + RequestError::Protocol(problem) => Self::Protocol(problem), + _ => Self::Request(value), + } + } +} + +impl NewAccountError { + pub fn is_invalid(&self) -> bool { + match self { + Self::ExternalAccount => true, + Self::Protocol(err) => matches!( + err.category(), + ProblemCategory::Account | ProblemCategory::Malformed + ), + _ => false, + } + } +} + +#[derive(Debug, Error)] +pub enum NewCertificateError { + #[error("unexpected authorization status {0:?}")] + AuthorizationStatus(super::types::AuthorizationStatus), + + #[error("no certificate in the validated order")] + CertificateUrl, + + #[error("unexpected challenge status {0:?}")] + ChallengeStatus(super::types::ChallengeStatus), + + #[error("csr generation failed ({0})")] + Csr(openssl::error::ErrorStack), + + #[error("no supported challenges")] + NoSupportedChallenges, + + #[error("unexpected order status {0:?}")] + OrderStatus(super::types::OrderStatus), + + #[error("invalid or missing order URL")] + OrderUrl, + + #[error(transparent)] + PrivateKey(#[from] crate::conf::pkey::PKeyGenError), + + #[error(transparent)] + Protocol(#[from] Problem), + + #[error(transparent)] + Request(RequestError), + + #[error(transparent)] + Solver(#[from] SolverError), +} + +impl From for NewCertificateError { + fn from(value: RequestError) -> Self { + match value { + RequestError::Protocol(problem) => Self::Protocol(problem), + _ => Self::Request(value), + } + } +} + +impl NewCertificateError { + pub fn is_invalid(&self) -> bool { + match self { + Self::Protocol(err) => matches!( + err.category(), + ProblemCategory::Order | ProblemCategory::Malformed + ), + _ => false, + } + } +} + +#[derive(Debug, Error)] +pub enum RequestError { + #[error(transparent)] + Client(Box), + + #[error("cannot deserialize problem document ({0})")] + ErrorFormat(#[from] serde_json::Error), + + #[error("cannot build request ({0})")] + Http(#[from] http::Error), + + #[error(transparent)] + Io(#[from] std::io::Error), + + #[error("cannot obtain replay nonce")] + Nonce, + + #[error(transparent)] + Protocol(#[from] Problem), + + #[error("cannot serialize request ({0})")] + RequestFormat(serde_json::Error), + + #[error("cannot deserialize response ({0})")] + ResponseFormat(serde_json::Error), + + #[error("cannot sign request body ({0})")] + Sign(#[from] crate::jws::Error), +} + +impl From for RequestError { + fn from(value: HttpClientError) -> Self { + match value { + HttpClientError::Io(err) => Self::Io(err), + _ => Self::Client(unsize_box!(Box::new(value))), + } + } +} diff --git a/src/lib.rs b/src/lib.rs index 270a48e..0ce76c2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -214,10 +214,8 @@ async fn ngx_http_acme_update_certificates(amcf: &AcmeMainConfig) -> Time { Err(err) => { // Check if the server rejected this ACME account configuration. if err - .downcast_ref::() - .is_some_and(|err| { - matches!(err.category(), acme::types::ProblemCategory::Account) - }) + .downcast_ref::() + .is_some_and(|err| err.is_invalid()) { ngx_log_error!( NGX_LOG_ERR, @@ -346,26 +344,20 @@ async fn ngx_http_acme_update_certificates_for_issuer( next } Err(ref err) => { - if let Some(err) = err.downcast_ref::() { - if matches!( - err.category(), - acme::types::ProblemCategory::Malformed - | acme::types::ProblemCategory::Order - ) { - ngx_log_error!( - NGX_LOG_ERR, - log.as_ptr(), - "acme certificate \"{}/{}\" request is not valid: {}", - issuer.name, - order.cache_key(), - err - ); - cert.write().set_invalid(&err); - continue; - } + if err.is_invalid() { + ngx_log_error!( + NGX_LOG_ERR, + log.as_ptr(), + "acme certificate \"{}/{}\" request is not valid: {}", + issuer.name, + order.cache_key(), + err + ); + cert.write().set_invalid(&err); + continue; } - cert.write().set_error(err.as_ref()) + cert.write().set_error(&err) } }; From 8aea6020fe3beeda33acca6645bf45228326c5f1 Mon Sep 17 00:00:00 2001 From: Aleksei Bavshin Date: Tue, 7 Oct 2025 21:35:27 -0700 Subject: [PATCH 04/10] ACME: check that the account status is valid. --- src/acme.rs | 7 ++++++- src/acme/error.rs | 6 +++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/acme.rs b/src/acme.rs index 43b7de7..1d8ea09 100644 --- a/src/acme.rs +++ b/src/acme.rs @@ -18,7 +18,7 @@ use ngx::collections::Vec; use ngx::ngx_log_debug; use openssl::pkey::{PKey, PKeyRef, Private}; use openssl::x509::{self, extension as x509_ext, X509Req}; -use types::ProblemCategory; +use types::{AccountStatus, ProblemCategory}; use self::account_key::{AccountKey, AccountKeyError}; use self::types::{AuthorizationStatus, ChallengeKind, ChallengeStatus, OrderStatus}; @@ -287,6 +287,11 @@ where let res = self.post(&self.directory.new_account, payload).await?; + let account: types::Account = deserialize_body(res.body())?; + if !matches!(account.status, AccountStatus::Valid) { + return Err(NewAccountError::Status(account.status)); + } + let key_id: &str = try_get_header(res.headers(), http::header::LOCATION).ok_or(NewAccountError::Url)?; diff --git a/src/acme/error.rs b/src/acme/error.rs index 4abc70d..3374cad 100644 --- a/src/acme/error.rs +++ b/src/acme/error.rs @@ -9,7 +9,7 @@ use ngx::allocator::{unsize_box, Box}; use thiserror::Error; use super::solvers::SolverError; -use super::types::{Problem, ProblemCategory}; +use super::types::{AccountStatus, Problem, ProblemCategory}; use crate::net::http::HttpClientError; #[derive(Debug, Error)] @@ -26,6 +26,9 @@ pub enum NewAccountError { #[error("account request failed ({0})")] Request(RequestError), + #[error("unexpected account status {0:?}")] + Status(AccountStatus), + #[error("no account URL in response")] Url, } @@ -47,6 +50,7 @@ impl NewAccountError { err.category(), ProblemCategory::Account | ProblemCategory::Malformed ), + Self::Status(_) => true, _ => false, } } From 6f7788211f17612c2a0ae89b58bc83ffeb295fe7 Mon Sep 17 00:00:00 2001 From: Aleksei Bavshin Date: Mon, 15 Sep 2025 20:50:23 -0700 Subject: [PATCH 05/10] ACME: report challenge error for failed authorizations. --- src/acme.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/acme.rs b/src/acme.rs index 1d8ea09..d2ec75d 100644 --- a/src/acme.rs +++ b/src/acme.rs @@ -479,7 +479,16 @@ where ); if result.status != AuthorizationStatus::Valid { - return Err(NewCertificateError::AuthorizationStatus(result.status)); + if let Some(err) = result + .challenges + .iter() + .find(|x| x.kind == challenge.kind) + .and_then(|x| x.error.clone()) + { + return Err(err.into()); + } else { + return Err(NewCertificateError::AuthorizationStatus(result.status)); + } } Ok(()) From 87a5017654c9f2864c3538b2429b996e73bc218e Mon Sep 17 00:00:00 2001 From: Aleksei Bavshin Date: Mon, 29 Sep 2025 21:05:20 -0700 Subject: [PATCH 06/10] Log successfully issued certificates. Clean up error handling in the main update loop. --- src/lib.rs | 90 +++++++++++++++++++++++++++++------------------------- 1 file changed, 48 insertions(+), 42 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 0ce76c2..20c340f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -280,6 +280,8 @@ async fn ngx_http_acme_update_certificates_for_issuer( continue; }; + let order_id = order.cache_key(); + { let locked = cert.read(); @@ -290,9 +292,8 @@ async fn ngx_http_acme_update_certificates_for_issuer( if !locked.is_renewable() { ngx_log_debug!( log.as_ptr(), - "acme: certificate \"{}/{}\" is not due for renewal", - issuer.name, - order.cache_key() + "acme: certificate \"{issuer}/{order_id}\" is not due for renewal", + issuer = issuer.name, ); next = cmp::min(locked.next, next); continue; @@ -308,71 +309,76 @@ async fn ngx_http_acme_update_certificates_for_issuer( // Acme client wants &str and we already validated that the identifiers are valid UTF-8. let str_order = order.to_str_order(&*alloc); - let res = client.new_certificate(&str_order).await; - let cert_next = match res { + let cert_next = match client.new_certificate(&str_order).await { Ok(ref val) => { let pkey = Zeroizing::new(val.pkey.private_key_to_pem_pkcs8()?); let x509 = X509::from_pem(&val.chain)?; + let now = Time::now(); - let valid = - TimeRange::from_x509(&x509).unwrap_or(TimeRange::new(Time::now(), Time::now())); + let valid = TimeRange::from_x509(&x509).unwrap_or(TimeRange::new(now, now)); - let next = match cert.write().set(&val.chain, &pkey, valid) { - Ok(x) => x, + let res = cert.write().set(&val.chain, &pkey, valid); + + let next = match res { + Ok(x) => { + ngx_log_error!( + NGX_LOG_INFO, + log.as_ptr(), + "acme certificate \"{}/{}\" issued, next update in {:?}", + issuer.name, + order_id, + (x - now) + ); + x + } Err(err) => { ngx_log_error!( NGX_LOG_WARN, log.as_ptr(), - "acme certificate \"{}/{}\" request failed: {}", - issuer.name, - order.cache_key(), - err + "{err} while updating certificate \"{issuer}/{order_id}\"", + issuer = issuer.name, ); - Time::now() + ACME_MIN_INTERVAL + now + ACME_MIN_INTERVAL } }; - let _ = - issuer.write_state_file(std::format!("{}.crt", order.cache_key()), &val.chain); + // Write files even if we failed to update the shared zone. + + let _ = issuer.write_state_file(std::format!("{order_id}.crt"), &val.chain); if !matches!(order.key, conf::pkey::PrivateKey::File(_)) { - let _ = - issuer.write_state_file(std::format!("{}.key", order.cache_key()), &pkey); + let _ = issuer.write_state_file(std::format!("{order_id}.key"), &pkey); } next } - Err(ref err) => { - if err.is_invalid() { - ngx_log_error!( - NGX_LOG_ERR, - log.as_ptr(), - "acme certificate \"{}/{}\" request is not valid: {}", - issuer.name, - order.cache_key(), - err - ); - cert.write().set_invalid(&err); - continue; - } + Err(ref err) if err.is_invalid() => { + ngx_log_error!( + NGX_LOG_ERR, + log.as_ptr(), + "{err} while updating certificate \"{issuer}/{order_id}\"", + issuer = issuer.name, + ); + cert.write().set_invalid(&err); + // We marked the order as invalid and will stop attempting to update it until the + // next configuration reload. It should not affect the next update schedule. + + continue; + } + Err(ref err) => { + ngx_log_error!( + NGX_LOG_WARN, + log.as_ptr(), + "{err} while updating certificate \"{issuer}/{order_id}\"", + issuer = issuer.name, + ); cert.write().set_error(&err) } }; next = cmp::min(cert_next, next); - - if let Err(e) = res { - ngx_log_error!( - NGX_LOG_WARN, - log.as_ptr(), - "acme certificate \"{}/{}\" request failed: {}", - issuer.name, - order.cache_key(), - e - ); - } } Ok(next) } From e413e66e83f2ea574079a9ad4a38a3d01c0d4573 Mon Sep 17 00:00:00 2001 From: Aleksei Bavshin Date: Mon, 6 Oct 2025 11:39:39 -0700 Subject: [PATCH 07/10] ACME account login refactoring. * Implement backoff for login errors * Log new account URLs --- src/acme.rs | 13 ++++++++-- src/conf/issuer.rs | 9 +++++++ src/lib.rs | 63 +++++++++++++++++++++++++++++---------------- src/state/issuer.rs | 22 ++++++++++++++++ 4 files changed, 83 insertions(+), 24 deletions(-) diff --git a/src/acme.rs b/src/acme.rs index d2ec75d..45dbadf 100644 --- a/src/acme.rs +++ b/src/acme.rs @@ -37,6 +37,11 @@ const DEFAULT_RETRY_INTERVAL: Duration = Duration::from_secs(1); const MAX_RETRY_INTERVAL: Duration = Duration::from_secs(8); static REPLAY_NONCE: http::HeaderName = http::HeaderName::from_static("replay-nonce"); +pub enum NewAccountOutput<'a> { + Created(&'a str), + Found(&'a str), +} + pub struct NewCertificateOutput { pub chain: Bytes, pub pkey: PKey, @@ -246,7 +251,7 @@ where Ok(res) } - pub async fn new_account(&mut self) -> Result<(), NewAccountError> { + pub async fn new_account(&mut self) -> Result, NewAccountError> { self.directory = self .get_directory() .await @@ -297,7 +302,11 @@ where self.account = Some(key_id.to_string()); - Ok(()) + let key_id = self.account.as_ref().unwrap(); + match res.status() { + http::StatusCode::CREATED => Ok(NewAccountOutput::Created(key_id)), + _ => Ok(NewAccountOutput::Found(key_id)), + } } pub fn is_ready(&self) -> bool { diff --git a/src/conf/issuer.rs b/src/conf/issuer.rs index 23c2d2e..b79cff8 100644 --- a/src/conf/issuer.rs +++ b/src/conf/issuer.rs @@ -118,6 +118,15 @@ impl Issuer { .is_some_and(|x| x.read().state != IssuerState::Invalid) } + /// Marks the last issuer login attempt as failed. + pub fn set_error(&self, err: &dyn StdError) -> Time { + if let Some(data) = self.data.as_ref() { + data.write().set_error(err) + } else { + Time::MAX + } + } + /// Marks the issuer as misconfigured or otherwise unusable. pub fn set_invalid(&self, err: &dyn StdError) { if let Some(data) = self.data.as_ref() { diff --git a/src/lib.rs b/src/lib.rs index 20c340f..24a3f05 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -212,29 +212,11 @@ async fn ngx_http_acme_update_certificates(amcf: &AcmeMainConfig) -> Time { let issuer_next = match ngx_http_acme_update_certificates_for_issuer(amcf, issuer).await { Ok(x) => x, Err(err) => { - // Check if the server rejected this ACME account configuration. - if err - .downcast_ref::() - .is_some_and(|err| err.is_invalid()) - { - ngx_log_error!( - NGX_LOG_ERR, - log.as_ptr(), - "acme issuer \"{}\" is not valid: {}", - issuer.name, - err - ); - - issuer.set_invalid(err.as_ref()); - continue; - } - ngx_log_error!( - NGX_LOG_INFO, + NGX_LOG_WARN, log.as_ptr(), - "update failed for acme issuer \"{}\": {}", - issuer.name, - err + "{err} while processing renewals for acme issuer \"{}\"", + issuer.name ); now + ACME_DEFAULT_INTERVAL } @@ -301,7 +283,44 @@ async fn ngx_http_acme_update_certificates_for_issuer( } if !client.is_ready() { - client.new_account().await?; + match client.new_account().await { + Ok(acme::NewAccountOutput::Created(x)) => { + ngx_log_error!( + NGX_LOG_INFO, + log.as_ptr(), + "account \"{}\" created for acme issuer \"{}\"", + x, + issuer.name + ); + } + Ok(acme::NewAccountOutput::Found(x)) => { + ngx_log_debug!( + log.as_ptr(), + "account \"{}\" found for acme issuer \"{}\"", + x, + issuer.name + ); + } + Err(err) if err.is_invalid() => { + ngx_log_error!( + NGX_LOG_ERR, + log.as_ptr(), + "{err} while creating account for acme issuer \"{}\"", + issuer.name + ); + issuer.set_invalid(&err); + return Ok(Time::MAX); + } + Err(err) => { + ngx_log_error!( + NGX_LOG_WARN, + log.as_ptr(), + "{err} while creating account for acme issuer \"{}\"", + issuer.name + ); + return Ok(issuer.set_error(&err)); + } + } } let alloc = crate::util::OwnedPool::new(nginx_sys::NGX_DEFAULT_POOL_SIZE as _, log) diff --git a/src/state/issuer.rs b/src/state/issuer.rs index c9efd3c..1774d42 100644 --- a/src/state/issuer.rs +++ b/src/state/issuer.rs @@ -5,6 +5,7 @@ use core::error::Error as StdError; use core::ptr; +use core::time::Duration; use ngx::allocator::{AllocError, TryCloneIn}; use ngx::collections::Queue; @@ -13,10 +14,12 @@ use ngx::sync::RwLock; use super::certificate::{CertificateContext, CertificateContextInner, SharedCertificateContext}; use crate::conf::issuer::Issuer; +use crate::time::{jitter, Time}; #[derive(Debug, Eq, PartialEq)] pub enum IssuerState { Idle, + Error { fails: usize }, Invalid, } @@ -49,6 +52,25 @@ impl IssuerContext { }) } + pub fn set_error(&mut self, _err: &dyn StdError) -> Time { + let fails = match self.state { + IssuerState::Error { fails } => fails + 1, + IssuerState::Invalid => return Time::MAX, + _ => 1, + }; + + self.state = IssuerState::Error { fails }; + + let interval = Duration::from_secs(match fails { + 1 => 60, + 2 => 600, + 3 => 6000, + _ => 24 * 60 * 60, + }); + + Time::now() + jitter(interval, 2) + } + pub fn set_invalid(&mut self, _err: &dyn StdError) { self.state = IssuerState::Invalid; } From 1ebcc50f3b9862ce2bea7931e071a0d595b8780f Mon Sep 17 00:00:00 2001 From: Aleksei Bavshin Date: Tue, 7 Oct 2025 20:49:47 -0700 Subject: [PATCH 08/10] ACME: save account URL to the state directory. We already log the URL, but on INFO level and only once. And while it is possible to find the URL from private key, the operation is not trivial. Let's make this simpler by writing a file named "account.url" to the state directory whenever the issuer tells us it created a new account. Fixes #54 --- src/conf/issuer.rs | 2 ++ src/lib.rs | 1 + 2 files changed, 3 insertions(+) diff --git a/src/conf/issuer.rs b/src/conf/issuer.rs index b79cff8..8af6dbf 100644 --- a/src/conf/issuer.rs +++ b/src/conf/issuer.rs @@ -33,6 +33,8 @@ use crate::state::certificate::{CertificateContext, CertificateContextInner}; use crate::state::issuer::{IssuerContext, IssuerState}; use crate::time::{Time, TimeRange}; +pub const ACCOUNT_URL_FILE: &str = "account.url"; + const ACCOUNT_KEY_FILE: &str = "account.key"; const NGX_ACME_DEFAULT_RESOLVER_TIMEOUT: ngx_msec_t = 30000; const NGX_CONF_UNSET_FLAG: ngx_flag_t = nginx_sys::NGX_CONF_UNSET as _; diff --git a/src/lib.rs b/src/lib.rs index 24a3f05..d779fd0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -292,6 +292,7 @@ async fn ngx_http_acme_update_certificates_for_issuer( x, issuer.name ); + let _ = issuer.write_state_file(conf::issuer::ACCOUNT_URL_FILE, x.as_bytes()); } Ok(acme::NewAccountOutput::Found(x)) => { ngx_log_debug!( From 0589b22068c44980c42b601cab1ba1333cdf5595 Mon Sep 17 00:00:00 2001 From: Aleksei Bavshin Date: Tue, 30 Sep 2025 18:06:07 -0700 Subject: [PATCH 09/10] ACME: set a limit on server-indicated retry intervals. Previously, it was possible to suspend the module execution by sending a sufficiently high Retry-After value. Now we will refuse to wait longer than one minute. --- src/acme.rs | 35 +++++++++++++++++++++++++++-------- src/acme/error.rs | 4 ++++ src/lib.rs | 19 +++++++++++++++++++ 3 files changed, 50 insertions(+), 8 deletions(-) diff --git a/src/acme.rs b/src/acme.rs index 45dbadf..6876326 100644 --- a/src/acme.rs +++ b/src/acme.rs @@ -34,7 +34,13 @@ pub mod solvers; pub mod types; const DEFAULT_RETRY_INTERVAL: Duration = Duration::from_secs(1); -const MAX_RETRY_INTERVAL: Duration = Duration::from_secs(8); + +/// Upper limit for locally generated increasing backoff interval. +const MAX_BACKOFF_INTERVAL: Duration = Duration::from_secs(8); + +/// Upper limit for server-generated retry intervals (Retry-After). +const MAX_SERVER_RETRY_INTERVAL: Duration = Duration::from_secs(60); + static REPLAY_NONCE: http::HeaderName = http::HeaderName::from_static("replay-nonce"); pub enum NewAccountOutput<'a> { @@ -232,10 +238,22 @@ where let err: types::Problem = deserialize_body(res.body())?; - let retriable = matches!( - err.kind, - types::ErrorKind::BadNonce | types::ErrorKind::RateLimited - ); + let retriable = match err.kind { + types::ErrorKind::RateLimited => { + // The server may ask us to retry in several hours or days. + if let Some(val) = res + .headers() + .get(http::header::RETRY_AFTER) + .and_then(parse_retry_after) + .filter(|x| x > &MAX_SERVER_RETRY_INTERVAL) + { + return Err(RequestError::RateLimited(val)); + } + true + } + types::ErrorKind::BadNonce => true, + _ => false, + }; if retriable && wait_for_retry(&res, &mut tries).await { ngx_log_debug!(self.log.as_ptr(), "retrying failed request ({err})"); @@ -416,7 +434,7 @@ where _ => order.status = OrderStatus::Processing, }; - let mut tries = backoff(MAX_RETRY_INTERVAL, self.finalize_timeout); + let mut tries = backoff(MAX_BACKOFF_INTERVAL, self.finalize_timeout); while order.status == OrderStatus::Processing && wait_for_retry(&res, &mut tries).await { drop(order); @@ -466,7 +484,7 @@ where return Err(NewCertificateError::ChallengeStatus(result.status)); } - let mut tries = backoff(MAX_RETRY_INTERVAL, self.authorization_timeout); + let mut tries = backoff(MAX_BACKOFF_INTERVAL, self.authorization_timeout); wait_for_retry(&res, &mut tries).await; let result = loop { @@ -552,7 +570,8 @@ async fn wait_for_retry( .headers() .get(http::header::RETRY_AFTER) .and_then(parse_retry_after) - .unwrap_or(interval); + .unwrap_or(interval) + .min(MAX_SERVER_RETRY_INTERVAL); sleep(retry_after).await; true diff --git a/src/acme/error.rs b/src/acme/error.rs index 3374cad..6e0e626 100644 --- a/src/acme/error.rs +++ b/src/acme/error.rs @@ -4,6 +4,7 @@ // LICENSE file in the root directory of this source tree. use core::error::Error as StdError; +use core::time::Duration; use ngx::allocator::{unsize_box, Box}; use thiserror::Error; @@ -133,6 +134,9 @@ pub enum RequestError { #[error(transparent)] Protocol(#[from] Problem), + #[error("rate limit exceeded, next attempt in {0:?}")] + RateLimited(Duration), + #[error("cannot serialize request ({0})")] RequestFormat(serde_json::Error), diff --git a/src/lib.rs b/src/lib.rs index d779fd0..eee905f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -23,6 +23,7 @@ use openssl::x509::X509; use time::TimeRange; use zeroize::Zeroizing; +use crate::acme::error::RequestError; use crate::acme::AcmeClient; use crate::conf::{AcmeMainConfig, AcmeServerConfig, NGX_HTTP_ACME_COMMANDS}; use crate::net::http::NgxHttpClient; @@ -312,6 +313,15 @@ async fn ngx_http_acme_update_certificates_for_issuer( issuer.set_invalid(&err); return Ok(Time::MAX); } + Err(acme::error::NewAccountError::Request(err @ RequestError::RateLimited(x))) => { + ngx_log_error!( + NGX_LOG_WARN, + log.as_ptr(), + "{err} while creating account for acme issuer \"{issuer}\"", + issuer = issuer.name + ); + return Ok(Time::now() + x); + } Err(err) => { ngx_log_error!( NGX_LOG_WARN, @@ -373,6 +383,15 @@ async fn ngx_http_acme_update_certificates_for_issuer( next } + Err(acme::error::NewCertificateError::Request(err @ RequestError::RateLimited(x))) => { + ngx_log_error!( + NGX_LOG_WARN, + log.as_ptr(), + "{err} while updating certificate \"{issuer}/{order_id}\"", + issuer = issuer.name, + ); + return Ok(Time::now() + x); + } Err(ref err) if err.is_invalid() => { ngx_log_error!( NGX_LOG_ERR, From 527e7a77a1594b824c82bc0fa040dd75e3728fd8 Mon Sep 17 00:00:00 2001 From: Aleksei Bavshin Date: Tue, 7 Oct 2025 21:48:02 -0700 Subject: [PATCH 10/10] Replace last use of anyhow with Box. We don't care about the error kind here, we just need to pass it to the caller and print. anyhow seems a bit too heavy for the task. While we're at it, remove unused futures-channel. --- Cargo.lock | 8 -------- Cargo.toml | 2 -- src/lib.rs | 2 +- 3 files changed, 1 insertion(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f00a319..18d2323 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -45,12 +45,6 @@ dependencies = [ "yansi-term", ] -[[package]] -name = "anyhow" -version = "1.0.99" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b0674a1ddeecb70197781e945de4b3b8ffb61fa939a5597bcf48503737663100" - [[package]] name = "async-task" version = "4.7.1" @@ -458,12 +452,10 @@ dependencies = [ name = "nginx-acme" version = "0.1.1" dependencies = [ - "anyhow", "base64", "bytes", "constcat", "foreign-types", - "futures-channel", "http", "http-body", "http-body-util", diff --git a/Cargo.toml b/Cargo.toml index 56a0a7a..4a358cd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -10,11 +10,9 @@ rust-version = "1.81.0" crate-type = ["cdylib"] [dependencies] -anyhow = "1.0.98" base64 = "0.22.1" bytes = "1.10.1" constcat = "0.6.1" -futures-channel = "0.3.31" http = "1.3.1" http-body = "1.0.1" http-body-util = "0.1.3" diff --git a/src/lib.rs b/src/lib.rs index eee905f..9ab46ae 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -231,7 +231,7 @@ async fn ngx_http_acme_update_certificates(amcf: &AcmeMainConfig) -> Time { async fn ngx_http_acme_update_certificates_for_issuer( amcf: &AcmeMainConfig, issuer: &conf::issuer::Issuer, -) -> anyhow::Result