Skip to content

fix(certbot): tighten the ACME challenge-domain rule and the paths that report failures - #1133

Merged
kvinwang merged 5 commits into
nextfrom
fix/certbot-acme-followups
Aug 25, 2026
Merged

fix(certbot): tighten the ACME challenge-domain rule and the paths that report failures#1133
kvinwang merged 5 commits into
nextfrom
fix/certbot-acme-followups

Conversation

@kvinwang

@kvinwang kvinwang commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Problem

Four small things left over from the instant-acme 0.8 port in #1129. None of them break
issuance today; each one is a place where the next change is more likely to break it, or
where the log will not say why it broke.

1. The wildcard rule lives inline and is untested. authorize() picks the bare
identifier field out of the AuthorizedIdentifier because that type's Display renders
a wildcard authorization as *.example.com, and formatting that into
_acme-challenge.{} publishes the TXT record at _acme-challenge.*.example.com — wrong
for every wildcard certificate, which is every gateway certificate. #1129 got this right
and said so in a comment, but a comment is what a future refactor deletes. The rule was
also the one piece of the port that is dstack's own logic rather than a mechanical API
rename, and it was the only piece with no test.

2. set_challenges_ready no longer says which challenge. 0.7 logged
setting challenge ready for {url}; the port replaced it with a bare
setting challenge ready. An order for example.com and *.example.com now emits the
same contextless line twice, which is exactly the log you reach for when one authorization
of several fails.

3. authorize() logs the same domain twice in a row. creating dns record for {name}
followed immediately by removing existing TXT record for {acme_domain} — after the port
both render the same string.

4. find_error() swallows the reason it gave up. It matches Some(Ok(authz)), so a
failed authorization fetch ends the loop indistinguishably from a clean end of stream. The
caller then reports order is invalid: built from a default empty Problem, and the
actual transport error is gone. This is the path that runs when issuance has already
failed, so it is precisely when the missing detail costs the most.

Fix

Five commits, one concern each:

  • refactor(certbot): move the bare-vs-wildcard decision into challenge_domain(), which
    returns the full record name. The non-DNS identifier case now names the offending
    identifier in its error. Drops the duplicate debug line from (3).
  • test(certbot): pin the rule — wildcard: false and wildcard: true on the same
    identifier must both yield _acme-challenge.example.com, and a non-DNS identifier must
    error.
  • fix(certbot): restore the challenge URL in the readiness log. ChallengeHandle derefs
    to Challenge, so challenge.url is still available in 0.8 — the thing 0.8 actually
    removed is Order::set_challenge_ready(url), not access to the URL.
  • fix(certbot): propagate authorization fetch failures through the returned error chain instead of only logging them.

No behavior change to issuance itself: same records, same order of operations, same
requests.

Verification

The new test fails on the bug it pins. Reverting challenge_domain() to format the
AuthorizedIdentifier directly — i.e. the mistake the helper exists to prevent:

assertion `left == right` failed
  left: "_acme-challenge.*.example.com"
 right: "_acme-challenge.example.com"

That left-hand value is the record name that would fail every wildcard issuance, so the
test is anchored to the real failure rather than to the current implementation.

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

Not covered: no live ACME run. These changes do not touch the wire behavior — the
diff is one extracted helper, two log lines, and a test — so the offline evidence above is
the whole story.

Not in this PR

certbot/src/acme_client/tests.rs is dead: it opens with #![cfg(not(test))], so it is
compiled out of every test run, and it has drifted — it calls AcmeClient::load() with
three arguments where the function now takes five. It is a live-network test against
Cloudflare and Let's Encrypt, so deciding between fixing it, gating it on an env var, or
deleting it is its own conversation. Flagging it here so it is not mistaken for coverage.

Copilot AI lite review requested due to automatic review settings August 25, 2026 10:12

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
kvinwang force-pushed the fix/certbot-acme-followups branch from ff85f14 to 50f3e6a Compare August 25, 2026 10:50
@kvinwang
kvinwang merged commit 55021ed into next Aug 25, 2026
17 checks passed
@kvinwang
kvinwang deleted the fix/certbot-acme-followups branch August 25, 2026 11:09
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