IsoHex/edge-router
Watch
1
0
Fork
You've already forked edge-router
 0
Code Issues 28 Pull requests Releases 4 Packages 1 Activity Actions

acme.rs leaks challenge records on most failure paths, which permanently blocks reissuance #30

New issue
Closed
opened 2026-08-28 19:48:08 +00:00 by james.oates · 1 comment
james.oates commented 2026-08-28 19:48:08 +00:00
Owner
Copy link

Found while porting this module into cert-keeper, where both bugs bit in a live deployment. Not fixing here — logging it for whoever owns this. Verified against 04c0cef.

Relevant if ACME stays in EdgeRouter. Increment 7 of the central-cert-issuance design retires it, so this may be moot — but it is live on kuruk and roku today.

1. Challenge records leak on most failure paths (the serious one)

cleanup_txt_records is called in exactly two places: the order-not-ready branch (acme.rs:169) and success (:193). Every other exit from acquire_certificate returns with records already created and never removed:

  • :135 — propagation failure, ? on wait_for_txt_propagation. The most likely failure of the lot, and see §2 for why it fires.
  • :106 — unexpected authorization status
  • :111, :125 — challenge lookup / record creation, part-way through the create loop
  • :143, :149, :153 — pass-2 notification
  • :167 — poll_ready
  • :183, :189 — finalize / poll_certificate

A leak is not merely untidy. Let's Encrypt reuses the challenge value of a pending authorization, so the next attempt tries to create a record identical to the orphan, Cloudflare rejects it with 81058 An identical record already exists, and the order fails at creation — before the cleanup that would have removed the orphan. Self-perpetuating, and it survives a restart because the state is at Cloudflare, not in the process.

Observed exactly this in cert-keeper on 2026-08-28: every subsequent renewal failed at creation until the record was cleared by hand.

Two fixes, and the second matters more:

  • Treat 81057/81058 as success by adopting the existing record (matched on exact content, so it cannot claim someone else's).
  • Sweep _acme-challenge.<domain> before ordering rather than relying on cleanup afterwards. Cleanup-on-exit cannot be relied on at all — the process can be killed between create and delete, and docker stop gives SIGKILL after 10s against a 2-minute propagation wait.

cert-keeper fb4f60d has both if it's useful as a reference.

2. The DoH propagation check can poison itself for 30 minutes

wait_for_txt_propagation (:216) asks Cloudflare's public DoH resolver. oates.ws has an SOA minimum of 1800 seconds, so a query made before the record is live caches the negative answer for half an hour — far longer than the 120-second window.

EdgeRouter is partly protected: it sleeps 5 seconds before its first query (:218), where cert-keeper queried immediately and failed 100% of the time. But 5 seconds is a race, not a guarantee. When it loses, the failure is total rather than partial, because client is built once outside the loop (:216) and reqwest keeps the connection alive — so all 24 attempts return to the same anycast node, the one the first query poisoned. A fresh curl from the same host will see the record while EdgeRouter never does, which makes this extremely confusing to diagnose from outside.

Worth knowing: kuruk's EdgeRouter was serving its holding certificate on 2026-08-26 while its renewal was in flight. That is consistent with this, though not proof of it.

The robust fix is to query the zone's authoritative nameservers instead of a caching resolver — they hold the zone rather than a cache of it, cannot serve a stale negative, and are what Let's Encrypt itself asks. cert-keeper 9b182a9 does this with hickory-resolver. Cheaper mitigations (wait longer first, disable connection reuse) reduce the odds without removing the failure mode.

Testing note

cert-keeper had tests asserting that a matching value succeeds and a non-matching one fails, and both passed throughout. Neither could distinguish "the resolver returned no records" from "the resolver returned a different value" — which is precisely the distinction that identified the bug. Worth having whichever way this is fixed.

Found while porting this module into `cert-keeper`, where both bugs bit in a live deployment. Not fixing here — logging it for whoever owns this. Verified against `04c0cef`. Relevant if ACME stays in EdgeRouter. Increment 7 of the central-cert-issuance design retires it, so this may be moot — but it is live on kuruk and roku today. ## 1. Challenge records leak on most failure paths (the serious one) `cleanup_txt_records` is called in exactly two places: the order-not-ready branch (`acme.rs:169`) and success (`:193`). Every other exit from `acquire_certificate` returns with records already created and never removed: - `:135` — **propagation failure**, `?` on `wait_for_txt_propagation`. The most likely failure of the lot, and see §2 for why it fires. - `:106` — unexpected authorization status - `:111`, `:125` — challenge lookup / record creation, part-way through the create loop - `:143`, `:149`, `:153` — pass-2 notification - `:167` — `poll_ready` - `:183`, `:189` — `finalize` / `poll_certificate` A leak is not merely untidy. Let's Encrypt reuses the challenge value of a pending authorization, so the next attempt tries to create a record **identical** to the orphan, Cloudflare rejects it with `81058 An identical record already exists`, and the order fails at *creation* — before the cleanup that would have removed the orphan. Self-perpetuating, and it survives a restart because the state is at Cloudflare, not in the process. Observed exactly this in `cert-keeper` on 2026-08-28: every subsequent renewal failed at creation until the record was cleared by hand. Two fixes, and the second matters more: - Treat `81057`/`81058` as success by adopting the existing record (matched on exact content, so it cannot claim someone else's). - Sweep `_acme-challenge.<domain>` before ordering rather than relying on cleanup afterwards. Cleanup-on-exit cannot be relied on at all — the process can be killed between create and delete, and `docker stop` gives SIGKILL after 10s against a 2-minute propagation wait. `cert-keeper` `fb4f60d` has both if it's useful as a reference. ## 2. The DoH propagation check can poison itself for 30 minutes `wait_for_txt_propagation` (`:216`) asks Cloudflare's public DoH resolver. `oates.ws` has an SOA minimum of **1800 seconds**, so a query made before the record is live caches the negative answer for half an hour — far longer than the 120-second window. EdgeRouter is *partly* protected: it sleeps 5 seconds before its first query (`:218`), where `cert-keeper` queried immediately and failed 100% of the time. But 5 seconds is a race, not a guarantee. When it loses, the failure is total rather than partial, because `client` is built once outside the loop (`:216`) and `reqwest` keeps the connection alive — so all 24 attempts return to the same anycast node, the one the first query poisoned. A fresh `curl` from the same host will see the record while EdgeRouter never does, which makes this extremely confusing to diagnose from outside. Worth knowing: kuruk's EdgeRouter was serving its holding certificate on 2026-08-26 while its renewal was in flight. That is consistent with this, though not proof of it. The robust fix is to query the zone's authoritative nameservers instead of a caching resolver — they hold the zone rather than a cache of it, cannot serve a stale negative, and are what Let's Encrypt itself asks. `cert-keeper` `9b182a9` does this with `hickory-resolver`. Cheaper mitigations (wait longer first, disable connection reuse) reduce the odds without removing the failure mode. ## Testing note `cert-keeper` had tests asserting that a matching value succeeds and a non-matching one fails, and both passed throughout. Neither could distinguish "the resolver returned no records" from "the resolver returned a different value" — which is precisely the distinction that identified the bug. Worth having whichever way this is fixed.
james.oates commented 2026-10-26 13:37:17 +00:00
Author
Owner
Copy link

Fixed by the per-name ACME rewrite in v2.0.0. acme.rs now records every published record and cleans up on every exit path, not just success:

let outcome = finish_order(name, &mut order, &published).await;
for (txt_name, value) in &published {
    cleanup_with_retry(dns, txt_name, value).await;
}
let (cert_pem, key_pem) = outcome?;

finish_order exists precisely so the result can be held while cleanup runs before the ?, and cleanup_with_retry retries once rather than giving up on a transient failure. Propagation failure, validation failure, finalize failure and an unparseable certificate all now retire the record.

One residual, noted rather than left implicit: a failure inside the publish loop returns via ? without cleaning records pushed by earlier iterations. That is unreachable today because an order carries exactly one identifier, so at most one record exists and the failing one was never published. It becomes reachable if orders ever batch several names — worth a glance if #50's multi-directory work changes the order shape.

Closing on the behaviour that was actually reported.

Fixed by the per-name ACME rewrite in v2.0.0. `acme.rs` now records every published record and cleans up on every exit path, not just success: ```rust let outcome = finish_order(name, &mut order, &published).await; for (txt_name, value) in &published { cleanup_with_retry(dns, txt_name, value).await; } let (cert_pem, key_pem) = outcome?; ``` `finish_order` exists precisely so the result can be held while cleanup runs before the `?`, and `cleanup_with_retry` retries once rather than giving up on a transient failure. Propagation failure, validation failure, finalize failure and an unparseable certificate all now retire the record. One residual, noted rather than left implicit: a failure inside the publish loop returns via `?` without cleaning records pushed by earlier iterations. That is unreachable today because an order carries exactly one identifier, so at most one record exists and the failing one was never published. It becomes reachable if orders ever batch several names — worth a glance if #50's multi-directory work changes the order shape. Closing on the behaviour that was actually reported.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
feat/direct-tpm
gitlab-final/feat/openbao-transit-broker
v2.0.0
v1.1.0
v1.0.6
v1.0.5
v1.0.4
v1.0.3
v1.0.2
v1.0.1
v1.0.0
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
IsoHex/edge-router#30
No description provided.