From 23ce787037c25733b591203c8036745f5daa041c Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Tue, 25 Aug 2026 06:47:54 -0700 Subject: [PATCH] fix(certbot): clear stale dns-01 records once per challenge name --- CHANGELOG.md | 1 + dstack/certbot/src/acme_client.rs | 65 ++++++++++++++++++++++++++++--- 2 files changed, 61 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 36d762f98..d3d358564 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - http-client: a caller can bound the response body (`http_request_bounded`, `PrpcClient::with_max_response_bytes`). Nothing is bounded by default — `dstack vmm logs --lines 100000` is a legitimate multi-megabyte fetch — but every client that talks to a guest agent opts in, in the gateway and in the VMM, because a CVM is untrusted and one of them polls on a timer against the whole fleet ### Fixed +- certbot: a certificate covering both a name and its wildcard (`example.com` and `*.example.com`) could never be issued over dns-01. The two authorizations are answered under one `_acme-challenge.example.com`, each with its own TXT value, and the publish step cleared every TXT record at that name before writing its own -- so the second authorization deleted the record answering the first, and the order failed with `Correct value not found for DNS challenge`. Clearing leftovers from an aborted run is now done once per challenge name per issuance, and the records for one name accumulate instead of replacing each other; cleanup afterwards is unchanged, deleting each record this run created by id - gateway: a node removed via `RemoveNode` silently rejoined the cluster the next time it started, because every node re-registers its own sync address on boot. Once tombstone GC is collecting, that comeback is worse than an annoyance: a stale data directory diverges from every digest, and the divergence repair's full re-exchange resurrects records whose deletes the cluster already collected. Removal now writes a durable marker — a live record, so the GC can never eat it — that every gateway's sync endpoints enforce; a removed node's envelopes are refused until an operator re-admits it with `SetNodeUrl`. The refused node counts HTTP 403 sync rejections (`dstack_gateway_sync_rejected_total`), including removal lockouts and app-identity mismatches. Every gateway also exposes `dstack_gateway_node_last_seen_timestamp_seconds` per known node, so a long-offline gateway is a one-line alert instead of an ack-watermark puzzle - gateway: deleted KV records left a tombstone that nothing ever collected, so every deregistered CVM stayed on disk for the life of the deployment. Tombstones every peer has acknowledged are now dropped once every `tombstone_gc_writes` replicated writes (default 10000, zero disables); the trigger counts replicated writes rather than reading a clock, so nodes in a cluster collect in the same window without depending on time synchronization. A `SetTombstoneGcConfig` admin RPC stores an operator override in the KV itself, replicating one pace to every node - gateway: `Admin.RemoveCvm` now reports the outcome of the `inst/` tombstone alone. A failure to delete associated override or telemetry records is logged instead of failing the call, so a removal that did take effect is no longer reported as failed — which also aborted the local routing cleanup that follows it. Re-issuing a removal still sweeps up override and telemetry records orphaned by an earlier partial failure diff --git a/dstack/certbot/src/acme_client.rs b/dstack/certbot/src/acme_client.rs index 0182779b9..acc1a8907 100644 --- a/dstack/certbot/src/acme_client.rs +++ b/dstack/certbot/src/acme_client.rs @@ -338,11 +338,22 @@ impl AcmeClient { let acme_domain = challenge_domain(challenge.identifier())?; let dns_value = challenge.key_authorization().dns_value(); - debug!("removing existing TXT record for {acme_domain}"); - self.dns01_client - .remove_txt_records(&acme_domain) - .await - .context("failed to remove existing dns record")?; + // Clearing stale records is a per-name preparation step, not a + // per-authorization one. An order for `example.com` and + // `*.example.com` yields two authorizations that are both answered + // under `_acme-challenge.example.com`, each with its own value, and + // both values have to be live at validation time. Purging again for + // the second authorization would delete the record the first one + // just published, so one of the two challenges could never be + // answered and the order failed with "Correct value not found for + // DNS challenge". + if needs_purge(challenges, &acme_domain) { + debug!("removing existing TXT records for {acme_domain}"); + self.dns01_client + .remove_txt_records(&acme_domain) + .await + .context("failed to remove existing dns record")?; + } debug!( "creating TXT record for {acme_domain} with TTL {}s", self.dns_txt_ttl @@ -649,6 +660,17 @@ impl AcmeClient { } } +/// Whether the stale TXT records under `acme_domain` still have to be cleared. +/// +/// The purge runs once per challenge name per issuance: the records this run +/// has already published live under the names in `published`, and clearing +/// those would take an answered challenge back down. +fn needs_purge(published: &[Challenge], acme_domain: &str) -> bool { + !published + .iter() + .any(|challenge| challenge.acme_domain == acme_domain) +} + /// The name of the TXT record that answers a dns-01 challenge for `identifier`. /// /// The record always lives under the bare name: a wildcard authorization for @@ -922,3 +944,36 @@ mod challenge_domain_tests { assert!(challenge_domain(&ip.authorized(false)).is_err()); } } + +#[cfg(test)] +mod purge_tests { + use super::{needs_purge, Challenge}; + + fn challenge(acme_domain: &str, dns_value: &str) -> Challenge { + Challenge { + id: format!("rec-{dns_value}"), + acme_domain: acme_domain.to_string(), + dns_value: dns_value.to_string(), + } + } + + /// A base name and its wildcard are two authorizations answered under one + /// `_acme-challenge.`. The first one clears whatever an aborted run + /// left behind; the second must publish alongside it instead of wiping it. + #[test] + fn a_name_is_cleared_once_per_issuance() { + let mut published = vec![]; + assert!(needs_purge(&published, "_acme-challenge.example.com")); + + published.push(challenge("_acme-challenge.example.com", "value-for-base")); + assert!(!needs_purge(&published, "_acme-challenge.example.com")); + } + + /// A SAN list can span zones, and clearing one challenge name says nothing + /// about the others. + #[test] + fn an_untouched_name_is_still_cleared() { + let published = vec![challenge("_acme-challenge.example.com", "value-for-base")]; + assert!(needs_purge(&published, "_acme-challenge.example.org")); + } +}