From 207dc529b262a051ebbd29838a38acf590bc4440 Mon Sep 17 00:00:00 2001 From: Aleksei Bavshin Date: Tue, 26 Aug 2025 16:14:24 -0700 Subject: [PATCH 1/2] ACME: poll authorizations instead of challenges. Previously, we assumed that we should poll the challenge URL until completion, because all the servers we tested correctly responded to a POST-as-GET to the challenger URL. We also made an initial POST-as-GET request to fetch the status and skip the challenges in "valid" state. RFC8555 7.5.1 specifies that we should poll the authorization resource status instead. Additionally, the discussion around Errata ID 6317 implies that it is harmful to initiate multiple challenges, as a single failure invalidates the whole authorization. Given the above, we can simplify the code and proceed to poll the authorization status after initiating the first available challenge. --- src/acme.rs | 88 +++++++++++++++-------------------------------------- 1 file changed, 24 insertions(+), 64 deletions(-) diff --git a/src/acme.rs b/src/acme.rs index f6a69bb..0b04b54 100644 --- a/src/acme.rs +++ b/src/acme.rs @@ -405,18 +405,34 @@ where url: http::Uri, authorization: types::Authorization, ) -> Result<()> { - let mut result = Err(anyhow!("no challenges")); let identifier = authorization.identifier.as_ref(); - for challenge in authorization.challenges { - result = self.do_challenge(order, &identifier, &challenge).await; + // Find and set up first supported challenge. + let (challenge, solver) = authorization + .challenges + .iter() + .find_map(|x| { + let solver = self.find_solver_for(&x.kind)?; + Some((x, solver)) + }) + .ok_or(anyhow!("no supported challenge for {identifier:?}"))?; - if result.is_ok() { - break; - } + solver.register(order, &identifier, challenge)?; + + scopeguard::defer! { + let _ = solver.unregister(&identifier, challenge); + }; + + let res = self.post(&challenge.url, b"{}").await?; + let result: types::Challenge = serde_json::from_slice(res.body())?; + if !matches!( + result.status, + ChallengeStatus::Pending | ChallengeStatus::Processing | ChallengeStatus::Valid + ) { + return Err(anyhow!("unexpected challenge status {:?}", result.status)); } - result?; + wait_for_retry(&res).await; let mut tries = 10; @@ -440,63 +456,7 @@ where ); if result.status != AuthorizationStatus::Valid { - return Err(anyhow!("authorization failed")); - } - - Ok(()) - } - - async fn do_challenge( - &self, - ctx: &AuthorizationContext<'_>, - identifier: &Identifier<&str>, - challenge: &types::Challenge, - ) -> Result<()> { - let res = self.post(&challenge.url, b"").await?; - let result: types::Challenge = serde_json::from_slice(res.body())?; - - // Previous challenge result is still valid. - // Should not happen as we already skip valid authorizations. - if result.status == ChallengeStatus::Valid { - return Ok(()); - } - - let solver = self - .find_solver_for(&challenge.kind) - .ok_or(anyhow!("no solver for {:?}", challenge.kind))?; - - solver.register(ctx, identifier, challenge)?; - - scopeguard::defer! { - let _ = solver.unregister(identifier, challenge); - }; - - // "{}" in request payload initiates the challenge, "" checks the status. - let mut payload: &[u8] = b"{}"; - let mut tries = 10; - - let result = loop { - let res = self.post(&challenge.url, payload).await?; - let result: types::Challenge = serde_json::from_slice(res.body())?; - - if !matches!( - result.status, - ChallengeStatus::Pending | ChallengeStatus::Processing, - ) || tries == 0 - { - break result; - } - - tries -= 1; - payload = b""; - wait_for_retry(&res).await; - }; - - if result.status != ChallengeStatus::Valid { - return Err(result - .error - .map(Into::into) - .unwrap_or(anyhow!("unknown error"))); + return Err(anyhow!("authorization failed ({:?})", result.status)); } Ok(()) From 5e648e44d6bf1fbeb2ae3a15db02fc14c12fc54f Mon Sep 17 00:00:00 2001 From: Aleksei Bavshin Date: Wed, 27 Aug 2025 10:58:23 -0700 Subject: [PATCH 2/2] ACME: use increasing intervals with timeout when polling. Previously, we limited polling for challenge and order status to a fixed number of tries. The updated algorithm will use increasing intervals and give up when the total wait timeout has elapsed. --- src/acme.rs | 87 ++++++++++++++++++++++++++++++++++------------------- 1 file changed, 56 insertions(+), 31 deletions(-) diff --git a/src/acme.rs b/src/acme.rs index 0b04b54..cc755f7 100644 --- a/src/acme.rs +++ b/src/acme.rs @@ -32,6 +32,7 @@ pub mod solvers; pub mod types; 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 struct NewCertificateOutput { @@ -55,6 +56,9 @@ where nonce: NoncePool, directory: types::Directory, solvers: Vec>, + authorization_timeout: Duration, + finalize_timeout: Duration, + network_error_retries: usize, } #[derive(Default)] @@ -106,6 +110,9 @@ where nonce: Default::default(), directory: Default::default(), solvers: Vec::new(), + authorization_timeout: Duration::from_secs(60), + finalize_timeout: Duration::from_secs(60), + network_error_retries: 3, }) } @@ -152,14 +159,14 @@ where url: &Uri, payload: P, ) -> Result> { - let mut fails = 0; - let mut nonce = if let Some(nonce) = self.nonce.get() { nonce } else { self.get_nonce().await? }; + let mut tries = core::iter::repeat(DEFAULT_RETRY_INTERVAL).take(self.network_error_retries); + ngx_log_debug!(self.log.as_ptr(), "sending request to {url:?}"); let res = loop { let body = crate::jws::sign_jws( @@ -183,13 +190,15 @@ where let res = match self.http.request(req).await { Ok(res) => res, - Err(e) if fails >= 3 => return Err(e.into()), - // TODO: limit retries to connection errors - Err(_) => { - fails += 1; - sleep(DEFAULT_RETRY_INTERVAL).await; - ngx_log_debug!(self.log.as_ptr(), "retrying: {} of 3", fails + 1); - continue; + Err(err) => { + // TODO: limit retries to connection errors + if let Some(tm) = tries.next() { + sleep(tm).await; + ngx_log_debug!(self.log.as_ptr(), "retrying failed request ({err})"); + continue; + } else { + return Err(err.into()); + } } }; @@ -210,15 +219,13 @@ where types::ErrorKind::BadNonce | types::ErrorKind::RateLimited ); - if !retriable || fails >= 3 { - self.nonce.add(nonce); - return Err(err.into()); + if retriable && wait_for_retry(&res, &mut tries).await { + ngx_log_debug!(self.log.as_ptr(), "retrying failed request ({err})"); + continue; } - fails += 1; - - wait_for_retry(&res).await; - ngx_log_debug!(self.log.as_ptr(), "retrying: {} of 3", fails + 1); + self.nonce.add(nonce); + return Err(err.into()); }; self.nonce.add_from_response(&res); @@ -381,12 +388,9 @@ where } }; - let mut tries = 10; - - while order.status == OrderStatus::Processing && tries > 0 { - tries -= 1; - wait_for_retry(&res).await; + let mut tries = backoff(MAX_RETRY_INTERVAL, self.finalize_timeout); + 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())?; @@ -432,20 +436,18 @@ where return Err(anyhow!("unexpected challenge status {:?}", result.status)); } - wait_for_retry(&res).await; - - let mut tries = 10; + let mut tries = backoff(MAX_RETRY_INTERVAL, self.authorization_timeout); + wait_for_retry(&res, &mut tries).await; let result = loop { let res = self.post(&url, b"").await?; let result: types::Authorization = serde_json::from_slice(res.body())?; - if result.status != AuthorizationStatus::Pending || tries == 0 { + if result.status != AuthorizationStatus::Pending + || !wait_for_retry(&res, &mut tries).await + { break result; } - - tries -= 1; - wait_for_retry(&res).await; }; ngx_log_debug!( @@ -499,13 +501,36 @@ pub fn make_certificate_request( } /// Waits until the next retry attempt is allowed. -async fn wait_for_retry(res: &http::Response) { +async fn wait_for_retry( + res: &http::Response, + policy: &mut impl Iterator, +) -> bool { + let Some(interval) = policy.next() else { + return false; + }; + let retry_after = res .headers() .get(http::header::RETRY_AFTER) .and_then(parse_retry_after) - .unwrap_or(DEFAULT_RETRY_INTERVAL); - sleep(retry_after).await + .unwrap_or(interval); + + sleep(retry_after).await; + true +} + +/// Generate increasing intervals saturated at `max` until `timeout` has passed. +fn backoff(max: Duration, timeout: Duration) -> impl Iterator { + let first = (Duration::ZERO, Duration::from_secs(1)); + let stop = Time::now() + timeout; + + core::iter::successors(Some(first), move |prev: &(Duration, Duration)| { + if Time::now() >= stop { + return None; + } + Some((prev.1, prev.0.saturating_add(prev.1))) + }) + .map(move |(_, x)| x.min(max)) } fn parse_retry_after(val: &http::HeaderValue) -> Option {