diff --git a/crates/common/src/network/acme/order.rs b/crates/common/src/network/acme/order.rs index 41f47c8..68dacd4 100644 --- a/crates/common/src/network/acme/order.rs +++ b/crates/common/src/network/acme/order.rs @@ -234,7 +234,7 @@ impl AcmeRequestBuilder { let mut retry_after = response.retry_after; let auth = response.body; - let (domain, challenge_url) = match auth.status { + let domain = match auth.status { AuthStatus::Pending => { let Identifier::Dns(domain) = auth.identifier; @@ -318,7 +318,7 @@ impl AcmeRequestBuilder { } self.challenge(&challenge.url).await?; - (domain, challenge.url.clone()) + domain } AuthStatus::Valid => return Ok(()), _ => { @@ -345,14 +345,20 @@ impl AcmeRequestBuilder { match response.body.status { AuthStatus::Pending => { + // inbuxa: keep polling, don't post the challenge again. + // RFC 8555 section 7.5.1 has the client post a challenge + // once to say it's ready and then poll the authorization, + // which stays pending while validation runs. Posting it + // again is refused once the server has moved the + // challenge to "processing" (pebble answers 400 + // malformed, "Cannot update challenge with status + // processing"), and that refusal failed the renewal. trc::event!( Acme(AcmeEvent::AuthPending), Hostname = domain.to_string(), Url = self.directory.new_order.to_string(), Total = i, ); - - self.challenge(&challenge_url).await? } AuthStatus::Valid => { trc::event!( diff --git a/docs/spec/container-tests.md b/docs/spec/container-tests.md index d5719eb..c79c04e 100644 --- a/docs/spec/container-tests.md +++ b/docs/spec/container-tests.md @@ -63,6 +63,28 @@ STORE=PostgreSqlReplicated cargo test -p tests --features postgres,redis \ replica_cluster_tests -- --ignored ``` +`LOG=` turns on the test server's own logging, which is the only way +to see why a task failed rather than that it failed: `LOG=error` is what +found the ACME fault below. + +## The suites the regression runs, but not with its own settings + +Three of the suites a plain `cargo test -p tests` runs can't pass on the +settings it uses. Run them by name, with these: + +``` +# Two nodes, so a shared store and a coordinator, never RocksDb +STORE=PostgreSql COORDINATOR=Redis cargo test -p tests --features postgres,redis \ + -- --exact cluster::broadcast::cluster_tests + +# The spam rules the expectations were recorded against +STORE=RocksDb SPAM_RULES_URL=file:///path/to/spam-filter-rules.json.gz \ + cargo test -p tests -- --exact smtp::inbound::antispam::antispam + +# The ACME pair, which keeps its containers between runs +STORE=RocksDb cargo test -p tests -- --exact automation::automation_tests +``` + ## What a plain regression leaves failing `STORE=RocksDb cargo test -p tests -- --test-threads=1` was 86 passed, 4 @@ -79,13 +101,19 @@ and three are the invocation or the environment rather than the code: can't work on RocksDb, where each node gets its own. - `smtp::outbound::lmtp::lmtp_delivery` counted three DSNs where it wanted four, and passes on its own: queue timing under a loaded sequential run. -- `automation::automation_tests` fails against pebble with +- `automation::automation_tests` failed against pebble with `400 malformed: "Cannot update challenge with status processing, only - status pending"`. `crates/common/src/network/acme/order.rs` re-POSTs the - challenge each time it polls an authorization that is still pending; - pebble accepts that only while the challenge itself is pending. Upstream - code, untouched by the fork, and unfixed: worth treating as a real - renewal bug rather than a test artefact. + status pending"`, because the client re-posted the challenge on every + poll. That was a real renewal bug and is fixed; the 400s are gone and the + client polls as RFC 8555 section 7.5.1 says to. + + The suite still doesn't pass here: pebble never validates the TLS-ALPN + challenge, so the authorizations stay pending until the client gives up + and no certificate is issued. Validation needs pebble, in its container, + to reach the test server's `0.0.0.0:8899` across the docker bridge, and + `ufw` is active on this machine. That wasn't proved — standing up a + listener to test it needs a permission this session didn't have — so + before reading anything into an ACME failure, check that path first. ## When one fails