fix(certbot): clear stale dns-01 records once per challenge name - #1136
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A certificate covering both a name and its wildcard --
example.comand*.example.com-- can never be issued over dns-01. The order fails atvalidation:
The root cause is in
authorize(). Those two identifiers produce two separateauthorizations with two separate tokens, but RFC 8555 answers a wildcard
authorization under the bare name, so both TXT values have to be live at
_acme-challenge.example.comat the same time. The loop instead cleared everyTXT record at the challenge name before publishing its own:
So the second authorization deleted the record answering the first. Whichever
challenge the ACME server validated second passed; the other one had nothing to
find. This is not new -- it predates the
instant-acme0.8 upgrade (#1129) andthe ACME follow-ups (#1130, #1133); see the A/B below.
Wildcard-only orders (
*.example.comalone) and single-name orders were neveraffected, which is why this survived: one authorization, one record, nothing to
overwrite. That also bounds who hits it today.
DistributedCertBot-- thein-CVM path every current gateway uses -- orders exactly one SAN,
request_new_certificate(&key_pem, &[format!("*.{domain}")]), so it cannottrigger this. The affected surface is the
certbotCLI, where the operatorsupplies the SAN list in
certbot.toml(docs/deployment.md,docs/dstack-gateway.md); an apex listed next to its wildcard there fails everyissuance. The fix lives in the shared
AcmeClient, so it also covers thegateway if the apex is ever added to its order.
Fix
Clearing leftovers is a per-name preparation step, not a per-authorization
one. It now runs before the first record published under a challenge name in
this issuance, and subsequent authorizations for the same name publish
alongside rather than replacing:
challengesis the list this run has already published, threaded through fromrequest_new_certificate-- every purge is immediately followed by a push forthat name, so "already in
challenges" is exactly "already cleared this run".No new state, and it stays correct if
authorize()is ever entered twice.Nothing else needs to change:
check_dns()already keys its resolvers bychallenge name and asserts each challenge's own value against the answer set,
and cleanup already deletes by record id, so both records for a shared name are
removed on the way out.
Multiple TXT records at one challenge name are what the protocol expects --
RFC 8555 §8.4 validates if any RRSet entry matches -- so this is additive on
the DNS side, not a new requirement on the provider.
Verification
Let's Encrypt staging + real Cloudflare DNS
Same zone (
kvin.wang), one fresh subdomain per run so no cached DNS view isshared,
origin/next(55021edbf8) and this branch built identically:origin/nextdns01test4.kvin.wang+ wildcardError: order is invalid: Incorrect TXT record "w4Vr..." found at _acme-challenge.dns01test4.kvin.wangdns01test.kvin.wang+ wildcardDNS:*.dns01test.kvin.wang, DNS:dns01test.kvin.wang, issuer(STAGING) Artificial Amaranth YE1The client's own DNS self-check tells the whole story before Let's Encrypt is
even consulted.
origin/nextpublishes twice under one name and only ever seesone value live, because the second publish cleared the first:
With the fix, the purge happens once and both values answer from one RRset:
Both records are deleted by id afterwards, and the zone holds no
_acme-challenge.dns01test*records once the runs finish.Pebble + mock Cloudflare API
Faster loop, same A/B, plus the cases the staging runs do not cover:
origin/next["e2e.test", "*.e2e.test"]Error: order is invalid: Correct value not found for DNS challengeDNS:e2e.test, DNS:*.e2e.test["single.e2e.test"]DNS:single.e2e.testStale-record handling is unchanged, checked explicitly: a leftover
_acme-challenge.e2e.testTXT record injected before the run("stale-from-an-aborted-run") is gone afterwards, both challenge records are
published and validated, and the mock API reports zero records once issuance
completes -- so neither the purge nor the cleanup regressed into leaking
records.
Unit tests cover the purge decision itself, and
cargo fmt,cargo clippy -p certbot --all-targets -D warningsandcargo test -p certbotare clean.Note
dns-persist-01(#1132) is immune by construction -- one persistent recordsatisfies every authorization for the zone -- so this only affects the dns-01
path. That PR is unaffected either way; this one is independent of it.