fix(certbot): clear stale dns-01 records once per challenge name (#1136)

Author: kvinwang

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

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.<name>`. 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"));
+    }
+}