Skip to content

fix(certbot): upgrade instant-acme so an unknown challenge type does not break issuance - #1129

Merged
kvinwang merged 3 commits into
nextfrom
fix/acme-dns-persist-challenge
Aug 25, 2026
Merged

fix(certbot): upgrade instant-acme so an unknown challenge type does not break issuance#1129
kvinwang merged 3 commits into
nextfrom
fix/acme-dns-persist-challenge

Conversation

@kvinwang

Copy link
Copy Markdown
Collaborator

Problem

Certificate issuance against Let's Encrypt fails outright. A gateway asking staging for *.06rc0.kvin.wang never gets a certificate:

WARN dstack_gateway::main_service: cert[06rc0.kvin.wang]: auto-renewal failed: failed to request new certificate

Caused by:
    0: failed to authorize
    1: failed to get authorizations
    2: failed to (de)serialize JSON: missing field `token` at line 16 column 5

Let's Encrypt now offers two DNS challenges on the same authorization:

"challenges": [
  { "type": "dns-persist-01", "url": ".../chall/329528784/4019775194/a8DD1w", "status": "pending" },
  { "type": "dns-01",         "url": ".../chall/329528784/4019775194/HV5G9g", "status": "pending", "token": "..." }
]

dns-persist-01 carries no token. instant-acme 0.7.2 declares Challenge.token as a plain String with no default (instant-acme-0.7.2/src/types.rs:262), so that entry fails to deserialize — and because challenges is a single array, the failure takes the whole authorization down with it, including the perfectly usable dns-01 challenge sitting next to it.

The challenge type is not the problem: 0.7.2 already models unknown types as ChallengeType::Unknown(String). It is the required token field on a sibling object that breaks the parse, which is why the error is missing field rather than unknown variant.

Nothing in certbot's own code is wrong, and no configuration avoids this: the failure happens inside the library while reading the authorization, before certbot ever picks a challenge.

This is not limited to staging. Once dns-persist-01 reaches production, every gateway's issuance and renewal fails the same way, so deployments would start losing certificates as they expire rather than failing loudly on day one.

Fix

Upgrade instant-acme 0.7.2 → 0.8.5, which fixes exactly this upstream:

/// Token for this challenge
///
/// Unknown `ChallengeType` instances may omit this field, leaving it empty.
#[serde(default)]
pub token: String,

certbot's challenge selection is unchanged — it still does find(|c| c.r#type == ChallengeType::Dns01) — it just no longer loses the array before getting there.

The 0.8 API required matching changes, all mechanical:

  • Account::from_credentials_and_http / create_with_httpAccount::builder_with_http(..).from_credentials(..) / .create(..).
  • NewOrder fields are private → NewOrder::new(&identifiers).
  • Order::authorizations() returns a stream instead of a Vec, so authorize() and find_error() iterate with while let Some(..) = .next().await.
  • Order::set_challenge_ready(url) is gone; readiness now goes through ChallengeHandle::set_ready(). The handle borrows the order and cannot be held across the DNS-propagation wait, so setting challenges ready became a second pass over the authorizations (set_challenges_ready). The ordering is unchanged: publish every TXT record, verify propagation, then tell the server. Because the handle no longer exposes the challenge URL, the url field was dropped from certbot's own Challenge record, which only used it for this call and a log line.
  • Order::finalize(csr)finalize_csr(csr); Problem gained subproblems.
  • HttpClient::request now takes Request<BodyWrapper<Bytes>>. BodyWrapper<Bytes> implements http_body::Body, so the existing .collect() works unchanged.

One subtlety worth calling out: the identifier now arrives as AuthorizedIdentifier, whose Display renders a wildcard authorization as *.example.com. Formatting that into _acme-challenge.{} would publish the record at _acme-challenge.*.example.com. The code takes the bare identifier field instead, preserving 0.7's behaviour.

Verification

The bug, isolated. Same authorization JSON, same library, two versions:

instant-acme Result
0.7.2 FAILED: missing field 'token' at line 5 column 83
0.8.5 PARSED ok, 2 challenges

The 0.7.2 message is character-for-character the one from the gateway log above, which is what ties this minimal case to the real failure rather than to something merely similar.

Regression test (challenge_parsing_tests, shaped from a real acme-staging-v02 response) asserts that an authorization containing dns-persist-01 parses, that both challenges survive, that dns-01 keeps its token, that the tokenless challenge yields an empty token rather than an error, and that a wildcard authorization still reports the bare name.

Note on its scope: this test cannot be run against 0.7.2 as a counterfactual — AuthorizationState did not exist there (it was Authorization), so the crate does not compile at all on the old version. The counterfactual above is what proves the failure; the test exists to stop a future downgrade from silently reintroducing it.

Build/lint: cargo check --workspace, cargo test -p certbot, cargo fmt --check, and cargo clippy -p certbot -p dstack-gateway -- -D warnings all pass.

Not yet covered: a live issuance against Let's Encrypt staging through a deployed gateway. The image build for that was still running when this was opened; I'll add the result as a comment. Everything above is offline evidence.

Copilot AI lite review requested due to automatic review settings August 25, 2026 08:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@kvinwang

Copy link
Copy Markdown
Collaborator Author

Live run against Let's Encrypt staging

Deployed a gateway built from 1fdfa2c35e as a real TDX CVM and pointed it at staging for *.06rc0.kvin.wang. This is the same setup that produced the missing field 'token' failure in the description, with only the image changed.

The parse failure is gone, and issuance now runs deep into the flow:

DEBUG try_renew{domain="06rc0.kvin.wang"}: certbot::acme_client: Unsettled challenges: [
        acme_domain: "_acme-challenge.06rc0.kvin.wang",
        dns_value: "a-RtPuJHEnlOmefe8XWQDWlfLJPd8b6Rrfsw6Ygma8o",

Reaching that line means the authorization was fetched and deserialized, the dns-01 challenge was found next to the dns-persist-01 one, the key authorization was computed, and the TXT record was written. On 0.7.2 none of these happened — it failed at the first of them.

The record is real, and its value matches what certbot computed:

$ dig +short TXT _acme-challenge.06rc0.kvin.wang @1.1.1.1
"a-RtPuJHEnlOmefe8XWQDWlfLJPd8b6Rrfsw6Ygma8o"
$ dig +short TXT _acme-challenge.06rc0.kvin.wang @8.8.8.8
"a-RtPuJHEnlOmefe8XWQDWlfLJPd8b6Rrfsw6Ygma8o"

A certificate was not obtained, for a reason unrelated to this change, which I want to state plainly rather than leave implied by a green result.

check_dns queries the challenge record immediately after creating it. Cloudflare has not propagated yet at that instant, so the first lookup returns NXDOMAIN — and the recursive resolver caches that negative answer for the zone's SOA minimum. For kvin.wang that is 1800s:

$ dig +short SOA kvin.wang
jocelyn.ns.cloudflare.com. dns.cloudflare.com. 2413134396 10000 2400 604800 1800

The self-check budget is max_dns_wait, five minutes here, so every retry inside the window hits the cached negative and the whole renewal times out with certificate request timed out. Building a fresh TokioResolver each iteration does not help: it clears hickory's in-process cache, not the upstream recursive resolver's.

So on any zone whose SOA minimum exceeds max_dns_wait, the self-check cannot succeed regardless of whether the record exists. Querying the authoritative nameservers directly, or delaying the first lookup past propagation, would fix it. That is a separate robustness issue in check_dns and out of scope here — this PR is about the authorization parse, and I did not want to bundle an unrelated behavioural change into it.

CI: all 17 checks pass.

@kvinwang

Copy link
Copy Markdown
Collaborator Author

Added 9cee658303 build: sync Cargo.lock with the instant-acme bump.

The lockfile in the original push still pinned instant-acme 0.7.2 while Cargo.toml asked for 0.8.5 — an artifact of the two-version comparison in the description, where I restored Cargo.toml from a backup but not the lockfile that cargo had rewritten alongside it.

CI did not catch this because the steps that build certbot do not pass --locked, so cargo silently re-resolved to 0.8.5 and passed. That is precisely the failure mode a lockfile exists to prevent, and it matters more than usual in a repo whose reproducibility story depends on pinned inputs.

cargo check --workspace --locked now passes.

@kvinwang

Copy link
Copy Markdown
Collaborator Author

A certificate was issued

Following up on the earlier comment, which reported issuance getting as far as publishing the TXT record but timing out on the self-check. Once the recursive resolver's cached negative expired, the same gateway completed the flow:

$ Admin.RenewZtDomainCert {"domain":"06rc0.kvin.wang","force":true}
{"renewed":true,"not_after":1795423139}

has_cert: True | loaded: True | expires: 2026-11-23T08:38:59

The certificate is real, from Let's Encrypt staging, and is being served by the proxy:

issuer=C = US, O = Let's Encrypt, CN = (STAGING) Baloney Bulgur YE2
subject=CN = *.06rc0.kvin.wang
notBefore=Aug 25 08:39:00 2026 GMT
notAfter=Nov 23 08:38:59 2026 GMT

And traffic reaches the application inside the CVM through it:

$ curl https://<instance>-80.06rc0.kvin.wang:9350/
<!DOCTYPE html>
<html>
<head>
<title>Welcome to nginx!</title>

That closes the loop this PR opened: order creation, authorization parsing, challenge selection, key authorization, DNS publication, challenge validation, finalization and certificate retrieval all work against a real ACME server. On 0.7.2 none of it did — issuance failed at the first step.

The self-check delay that produced the earlier timeout is a separate defect, fixed in #1130.

@kvinwang
kvinwang merged commit 2a97a29 into next Aug 25, 2026
17 checks passed
@kvinwang
kvinwang deleted the fix/acme-dns-persist-challenge branch August 25, 2026 10:07
kvinwang added a commit that referenced this pull request Aug 25, 2026
Brings in the three fixes found while testing this release candidate:
KMS CA certificates (#1128), ACME authorization parsing (#1129), and the
DNS-01 self check against authoritative nameservers (#1130).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants