diff --git a/CHANGELOG.md b/CHANGELOG.md index 4636d26da..f4d6c5756 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - guest-agent: v1 `Attest` accepts `include_boottime_gpu_evidence` and returns the boot-time GPU attestation evidence in `AttestResponse.boottime_gpu_evidence`, so a verifier fetches the attestation and the GPU evidence in one round trip. It arrives as a `GpuEvidenceBundle` list -- the same shape `AttestGpu` returns, so a consumer writes one bundle parser and dispatches on `format`: `nvidia-nvattest-boottime-json-v1` is the record written at boot, `nvidia-nvattest-collect-evidence-json-v1` is collected on demand against a caller's nonce, and a verifier for one does not appraise the other. Absence is the empty list. The bundle's `evidence` is the nvattest output byte for byte as read from disk, because the only thing binding it to the boot is sha256 over precisely those bytes against the measured `gpu-attestation` event. `Attest` is also v1's sole CVM attestation entry point: the `VersionedAttestation` it returns already carries the TDX quote and event log, and unlike `GetQuote` it answers on every supported platform - sdk: `AppCompose` in the Go SDK gained `init_script`, `storage_fs`, `swap_size`, `event_log_version`, `port_policy` and `verity_volumes`, and `Requirements` gained `gpu_policy` in the Go and Python SDKs - shared API authentication (`dstack-api-auth`) protecting the full VMM HTTP/pRPC/UI surface and unifying Gateway/KMS admin auth: bearer/`X-Admin-Token`/HTTP Basic/bcrypt htpasswd, constant-time verification (#796) +- certbot/gateway: opt-in `dns-persist-01` certificate validation ([draft-ietf-acme-dns-persist-01](https://datatracker.ietf.org/doc/html/draft-ietf-acme-dns-persist-01)), which issues without a DNS provider credential at all. `dns-01` needs write access to the zone on every order, so a gateway CVM holds a Cloudflare API token for the life of the deployment -- a token that rewrites the whole zone, not just `_acme-challenge`. Under `dns-persist-01` the zone owner publishes one `_validation-persist.` TXT record naming the CA and the ACME account; nothing about it changes between orders, so certbot only ever reads DNS and the zone can be hosted anywhere, with no provider integration. Set `challenge = "dns-persist-01"` in `certbot.toml`, or `challenge` on a gateway ZT domain; the default stays `dns-01` and existing deployments are untouched. `certbot dns-records` prints the records to publish, `GetZtDomain`/`ListZtDomains` return them in `required_dns_records`, and the gateway logs them wherever it would otherwise have written DNS. CAA records carry `validationmethods=dns-persist-01` to match, so switching methods means republishing both. Two gateway operations change shape for such a domain: `SetCaa` skips it, having nothing to reconcile without write access, and `RotateAcmeCredentials` leaves it broken until the operator republishes -- the record still names the old account -- so the response now returns the new records in `required_dns_records`. A rotation that registers the account but fails to re-pin some domain's CAA now reports those domains in `repin_failed_domains` instead of failing the call: the account exists and the cluster is using it, so raising an error there reads as "nothing happened" and invites a retry that registers another account. **Experimental**: the draft is still changing, and Let's Encrypt serves the challenge on staging only pending an open working-group issue. Documented in `docs/certbot-dns-persist-01.md`. + + Two settings changed shape for every deployment, not only `dns-persist-01` ones. `issuer_domain_name` now names the CA in the CAA records written for `dns-01` too -- its default is `letsencrypt.org`, so an untouched configuration writes exactly what it wrote before -- and it is validated where it is set, since it lands verbatim in an RFC 8659 `issue-value` and CAA is republished by deleting the old records first. The pre-order DNS self-check is now capped at half of `renew_timeout` rather than at its own configured value: both defaulted to 300s in the gateway, and the CLI wrapped a 300s wait in a 120s budget, so the timeout around the order always fired first and a missing record was reported as `certificate request timed out` instead of by name - gateway: `Admin.Status` reports `health_gating`, so an operator can see whether this node's health polling is switched on. With it off, instances that opted in sit at `unknown` forever and are all in rotation, which is otherwise indistinguishable on the dashboard from being held out pending a first answer - gateway: `Admin.SetInstanceReady` takes a CVM instance out of its app's load-balancing rotation without stopping it; instance-id routing stays open so the instance can still be investigated, and the setting survives re-registration - gateway: operator-set per-instance overrides now live under their own KV keys — `admin//ready` and `admin//port_policy` — instead of inside the instance record, so a CVM re-registration can no longer drop them and setting one cannot discard a peer's unsynced change to the other. An override left in an instance record by an earlier build is moved across on load diff --git a/docs/certbot-dns-persist-01.md b/docs/certbot-dns-persist-01.md new file mode 100644 index 000000000..0e090fd80 --- /dev/null +++ b/docs/certbot-dns-persist-01.md @@ -0,0 +1,233 @@ +# Certificate issuance without a DNS credential (`dns-persist-01`) + +`dns-01` asks certbot to write a fresh `_acme-challenge` TXT record for every +order, so whatever runs certbot holds a DNS API token with write access to the +zone, forever. In dstack that token lives inside the gateway CVM. Attestation +covers what the CVM is running, but a token is a token: anything that gets hold +of it can rewrite the zone, including records that have nothing to do with +certificates. + +`dns-persist-01` moves the proof out of the issuance loop. The zone owner +publishes one record naming the CA and the ACME account allowed to issue: + +```dns +_validation-persist.example.com. IN TXT "letsencrypt.org; accounturi=https://acme-v02.api.letsencrypt.org/acme/acct/1234567890" +``` + +The account key proves who is asking, the record proves the zone owner agreed, +and neither changes between orders. certbot reads DNS and never writes it, so +the CVM holds no DNS credential and the zone can be hosted anywhere — no +Cloudflare account, no API token, no provider integration. + +> **Experimental.** `dns-persist-01` is specified in +> [draft-ietf-acme-dns-persist-01][draft], which is still changing: an open +> working-group issue may add a client-key-derived value to the record, and +> Let's Encrypt has said it will not deploy the challenge to production until +> that is resolved. It is live on Let's Encrypt **staging** and in +> [Pebble][pebble]. Treat the record format as unstable, and expect to +> republish when the draft settles. + +[draft]: https://datatracker.ietf.org/doc/html/draft-ietf-acme-dns-persist-01 +[pebble]: https://github.com/letsencrypt/pebble + +## What the record means + +| Part | Effect | +| --- | --- | +| `letsencrypt.org` | Issuer Domain Name. A CA ignores records naming a different issuer, so one label can hold records for several CAs. | +| `accounturi=` | The ACME account authorized to issue. Compared byte for byte — no case folding, no URI normalization. | +| `policy=wildcard` | Extends the record to `*.example.com`. Without it the CA authorizes `example.com` alone and refuses wildcard orders. | +| `persistUntil=` | Optional UNIX timestamp after which the CA stops accepting the record. | + +A wildcard order authorizes from its base name — `*.example.com` is validated +against `_validation-persist.example.com`, not +`_validation-persist.*.example.com` — so one record covers a name and its +wildcard. + +Two things about the syntax bite in practice, because a CA rejects the whole +record rather than ignoring the offending part: **no trailing semicolon**, and +**no whitespace inside a value**. certbot renders records that satisfy both; +copy them verbatim rather than retyping. + +The scope stops at the names above. Let's Encrypt does not walk up the tree, so +a record on `example.com` does not authorize `sub.example.com` — give each base +name its own record. + +## Standalone certbot + +`certbot` never writes DNS in this mode, so setup is: create the account, read +the records off it, publish them, then issue. + +```toml +# certbot.toml +workdir = "/var/lib/certbot" +acme_url = "https://acme-staging-v02.api.letsencrypt.org/directory" +challenge = "dns-persist-01" +issuer_domain_name = "letsencrypt.org" +# auto_set_caa promises certbot keeps CAA in sync, which it cannot do without +# write access. Leave it off and publish the CAA records below by hand. +auto_set_caa = false +domains = ["example.com", "*.example.com"] +renew_interval = 3600 +renew_days_before = 10 +renew_timeout = 120 +max_dns_wait = 300 +``` + +`cf_api_token` is unused and can be left out; certbot warns if one is set. + +```console +$ certbot init -c certbot.toml +INFO certbot::bot: creating new ACME account +INFO certbot::bot: created new ACME account: https://acme-staging-v02.api.letsencrypt.org/acme/acct/1234567890 + +$ certbot dns-records -c certbot.toml +_validation-persist.example.com. IN TXT "letsencrypt.org; accounturi=https://acme-staging-v02.api.letsencrypt.org/acme/acct/1234567890; policy=wildcard" +example.com. IN CAA 0 issue "letsencrypt.org;validationmethods=dns-persist-01;accounturi=https://acme-staging-v02.api.letsencrypt.org/acme/acct/1234567890" +example.com. IN CAA 0 issuewild "letsencrypt.org;validationmethods=dns-persist-01;accounturi=https://acme-staging-v02.api.letsencrypt.org/acme/acct/1234567890" +``` + +Publish all three, wait for them to propagate, then issue: + +```console +$ certbot renew --once -c certbot.toml +INFO certbot::acme_client: requesting new certificates for example.com, *.example.com +INFO certbot::bot: created new certificate +``` + +Renewals need nothing further. The record stays, and `certbot renew` reuses it +for every order. + +The CAA records are optional but recommended — they stop any other account, at +Let's Encrypt or elsewhere, from being issued for your name. Note the +`validationmethods=dns-persist-01` in them: a CAA record left pinned to +`dns-01` refuses every `dns-persist-01` order, so switching methods means +updating CAA and the validation record together. + +### When issuance fails + +certbot checks its own resolver before starting an order, and says exactly what +it expected to find: + +``` +WARN certbot::acme_client: no TXT record at _validation-persist.example.com matches the expected value: letsencrypt.org; accounturi=https://acme-staging-v02.api.letsencrypt.org/acme/acct/1234567890; policy=wildcard +Error: order is invalid: API error: Checking DNS-PERSIST-01 challenge TXT record with issuer-domain-name "letsencrypt.org": accounturi mismatch: expected "https://acme-staging-v02.api.letsencrypt.org/acme/acct/1234567890", got "https://acme-staging-v02.api.letsencrypt.org/acme/acct/9876543210" (urn:ietf:params:acme:error:unauthorized) +``` + +The check is advisory and never blocks an order: certbot's resolver is not the +CA's, and its expectation can be stricter than what the CA would accept. A +warning with a successful issuance underneath it is a resolver difference, not +a problem. A record that genuinely does not match costs the full `max_dns_wait` +before the order is sent, because the check waits out its budget first. + +If the CA rejects the order, compare the published record against +`certbot dns-records` character by character. The usual causes are a stale +`accounturi` after the account was recreated, a missing `policy=wildcard` on a +wildcard order, and an issuer domain name that does not match the CA. + +## dstack-gateway + +A ZT domain picks its method with the `challenge` field, which defaults to +`dns-01` — existing domains are unaffected: + +```json +{ "domain": "app.example.com", "port": 443, "challenge": "dns-persist-01" } +``` + +Such a domain needs no `dns_cred_id`, and the gateway CVM never receives a DNS +credential for it. `GetZtDomain` and `ListZtDomains` return the records to +publish in `required_dns_records`, and the gateway logs them whenever it cannot +write DNS itself: + +``` +WARN cert[app.example.com]: publish this record by hand: _validation-persist.app.example.com. IN TXT "letsencrypt.org; accounturi=...; policy=wildcard" +``` + +The gateway issues for `*.{domain}` only, so each domain needs one validation +record with `policy=wildcard`. + +### Setup order + +The records name the ACME account, so the account comes first: + +1. `SetCertbotConfig` — `acme_url`, and `issuer_domain_name` if the CA is not + Let's Encrypt. +2. `RotateAcmeCredentials` — registers the account and returns its `account_uri`. + It needs no ZT domain: registration proves nothing about a domain. Every call + registers a *new* account, so call it once here; running it again later is a + rotation, with the consequences described below. +3. `AddZtDomain` with `challenge: "dns-persist-01"`. No `dns_cred_id`: the + domain needs none, and the gateway CVM never receives one for it. +4. Read the records — the dashboard shows them when the domain is added and + again under its **Records** button, and `GetZtDomain`, `ListZtDomains` and + the gateway log all carry them — then publish the `_validation-persist` TXT + record, plus the CAA records if you want issuance pinned to this account. +5. `RenewZtDomainCert`. Renewals from here need nothing further. + +Adding another dns-persist-01 domain later starts at step 3: the account already +exists, so the new domain's records are available as soon as it is. A domain's +challenge is chosen when it is added — in the dashboard's ZT-Domain form as well +as over the API — and carried forward by every edit. `UpdateZtDomain` replaces +the whole record, so `challenge` is optional there and omitting it means "leave +it as it is": a caller that predates the field — a cached dashboard bundle, a +script, an older SDK — cannot downgrade a `dns-persist-01` domain to `dns-01` by +editing something else, which its hand-published CAA would then refuse. Send the +field explicitly to switch a domain over. + +### Naming the CA + +For a non-production ACME server, set `issuer_domain_name` in the global certbot +config — `Admin.SetCertbotConfig`, or the field of that name in the dashboard's +Certbot Configuration — to whatever that server puts in `issuer-domain-names`, +`pebble.letsencrypt.org` for Pebble. Empty means `letsencrypt.org`. + +It also names the CA in the CAA records certbot writes for `dns-01` domains, so +one setting covers both challenges rather than pinning CAA to Let's Encrypt +while orders go somewhere else. `acme_url` is one setting for the whole +deployment; this follows it. + +The value is checked when it is set. It ends up verbatim inside an RFC 8659 +`issue-value`, and publishing CAA deletes the records it replaces before writing +the new ones, so a name carrying a space or a `;` would leave the zone holding a +malformed `issue` property with nothing valid behind it — CAA that forbids every +issuer. Anything that is not a DNS name is refused by `SetCertbotConfig`, and by +`certbot` at startup. + +Publishing CAA installs a temporary `;` guard, deletes the records it replaces, +writes the new ones and drops the guard. A run interrupted part-way leaves the +guard behind, which denies every issuer until a later run finishes the job — so +each run now sweeps stale guards before installing its own, and a rerun is +enough to recover. + +### How long the check waits + +The self-check before each order waits for the record to become visible, but +never longer than half of `renew_timeout`, the budget for the whole renewal. +Past that the outer timeout fires first and the renewal ends as `certificate +request timed out` with no mention of the record it could not find — the +opposite of what the check is for. Both values defaulted to 300s in the gateway +(and the CLI wrapped a 300s wait in a 120s budget), so this was the default +behaviour rather than an edge case. + +Two operations behave differently on these domains: + +- **`SetCaa`** skips them. There is nothing to reconcile without write access; + the records are logged instead, and the summary reports how many were left to + the operator. +- **`RotateAcmeCredentials`** is not self-service. Rotation moves the cluster + to a new ACME account, and every `_validation-persist` record still names the + old one, so orders for those domains fail until the operator republishes. The + response returns the new records in `required_dns_records` and the gateway + logs them; publish before the next renewal comes due. + + A rotation that registers the account but cannot re-pin every `dns-01` + domain's CAA still succeeds — the account exists and the cluster is on it — + and names those domains in `repin_failed_domains`. Finish with `SetCaa` + rather than another rotation, which would register yet another account + against a rate-limited quota. The dashboard reports both lists in one dialog + after a rotation, with a **Run SetCaa** button for the second. + +## Related + +- [dstack-gateway](dstack-gateway.md) — gateway architecture and TLS termination +- [deployment.md](deployment.md#4-zero-trust-https-optional) — the `dns-01` setup this replaces diff --git a/dstack/certbot/cli/src/main.rs b/dstack/certbot/cli/src/main.rs index 68348b5f9..bcc41ca8f 100644 --- a/dstack/certbot/cli/src/main.rs +++ b/dstack/certbot/cli/src/main.rs @@ -6,7 +6,7 @@ use std::{path::PathBuf, time::Duration}; use anyhow::{Context, Result}; -use certbot::{CertBotConfig, WorkDir}; +use certbot::{CertBotConfig, ChallengeKind, WorkDir, LETS_ENCRYPT_ISSUER_DOMAIN_NAME}; use clap::Parser; use documented::DocumentedFields; use fs_err as fs; @@ -40,6 +40,15 @@ enum Command { #[arg(short, long, default_value = "certbot.toml")] config: PathBuf, }, + /// Print the DNS records the configured domains need + /// + /// With `challenge = "dns-persist-01"` these are not written by certbot and + /// have to be published once, by hand, before the first issuance. + DnsRecords { + /// Path to the configuration file + #[arg(short, long, default_value = "certbot.toml")] + config: PathBuf, + }, /// Generate configuration template Cfg { /// Write to file @@ -60,7 +69,23 @@ struct Config { workdir: PathBuf, /// ACME server URL acme_url: String, - /// Cloudflare API token + /// ACME challenge used to prove control of the domains + /// + /// "dns-01" (default) writes a TXT record per order through the Cloudflare + /// API and needs cf_api_token. + /// + /// "dns-persist-01" proves control with a _validation-persist TXT record + /// published once, by hand: no API token, and the zone can be hosted + /// anywhere. Run `certbot init` then `certbot dns-records` to get the + /// records to publish. Experimental: the draft is still changing and + /// Let's Encrypt serves this challenge on staging only. + #[serde(default)] + challenge: ChallengeKind, + /// Issuer Domain Name naming the CA in dns-persist-01 and CAA records + #[serde(default = "default_issuer_domain_name")] + issuer_domain_name: String, + /// Cloudflare API token (unused with dns-persist-01) + #[serde(default)] cf_api_token: String, /// Optional Cloudflare-compatible API base URL #[serde(default)] @@ -90,6 +115,8 @@ impl Default for Config { Self { workdir: ".".into(), acme_url: "https://acme-staging-v02.api.letsencrypt.org/directory".into(), + challenge: ChallengeKind::default(), + issuer_domain_name: default_issuer_domain_name(), cf_api_token: "".into(), cf_api_url: None, dns_txt_ttl: default_dns_txt_ttl(), @@ -108,6 +135,10 @@ const fn default_dns_txt_ttl() -> u32 { 60 } +fn default_issuer_domain_name() -> String { + LETS_ENCRYPT_ISSUER_DOMAIN_NAME.to_string() +} + impl Config { fn to_commented_toml(&self) -> Result { let mut doc = to_document(self)?; @@ -152,6 +183,8 @@ fn load_config(config: &PathBuf) -> Result { .key_file(workdir.key_path()) .auto_create_account(true) .cert_subject_alt_names(config.domains) + .challenge(config.challenge) + .issuer_domain_name(config.issuer_domain_name) .cf_api_token(config.cf_api_token) .maybe_cf_api_url(config.cf_api_url) .dns_txt_ttl(config.dns_txt_ttl) @@ -231,6 +264,16 @@ async fn main() -> Result<()> { .context("Failed to build bot")?; bot.set_caa().await?; } + Command::DnsRecords { config } => { + let bot_config = load_config(&config).context("Failed to load configuration")?; + let bot = bot_config + .build_bot() + .await + .context("Failed to build bot")?; + for record in bot.required_dns_records() { + println!("{record}"); + } + } Command::Cfg { write_to } => { let toml_str = Config::default().to_commented_toml()?; match write_to { @@ -241,3 +284,62 @@ async fn main() -> Result<()> { } Ok(()) } + +#[cfg(test)] +mod config_template_tests { + use super::*; + + /// Every key has to carry its own doc comment. `toml_edit` drops `None` + /// fields from the serialized document -- `cf_api_url` is unset by default + /// -- so indexing `FIELD_DOCS` by position shifts every comment after the + /// gap onto the wrong setting. + #[test] + fn every_key_is_labelled_with_its_own_doc_comment() { + let rendered = Config::default() + .to_commented_toml() + .expect("the default config renders"); + + // Pair each key with the comment block immediately above it. + let mut comment = String::new(); + let mut pairs = Vec::new(); + for line in rendered.lines() { + match line.strip_prefix('#') { + Some(text) => comment.push_str(text.trim()), + None => { + if let Some((key, _)) = line.split_once('=') { + pairs.push((key.trim().to_string(), std::mem::take(&mut comment))); + } + } + } + } + assert!(!pairs.is_empty(), "no keys rendered:\n{rendered}"); + + for (key, comment) in &pairs { + let expected = Config::get_field_docs(key) + .unwrap_or_else(|err| panic!("no doc comment for {key:?}: {err}")); + let expected: String = expected.lines().map(str::trim).collect(); + assert_eq!( + comment, &expected, + "key {key:?} is labelled with another field's doc comment" + ); + } + } + + /// The field that made the misalignment visible: it sat after the dropped + /// `cf_api_url` and was labelled "Renew timeout in seconds", next to the one + /// value whose interaction with `renew_timeout` this crate clamps. + #[test] + fn max_dns_wait_is_not_labelled_as_a_renew_timeout() { + let rendered = Config::default() + .to_commented_toml() + .expect("the default config renders"); + let (before, _) = rendered + .split_once("max_dns_wait") + .expect("max_dns_wait is rendered"); + let label = before.lines().last().expect("it has a comment above it"); + assert!( + label.contains("DNS propagation"), + "max_dns_wait is labelled {label:?}" + ); + } +} diff --git a/dstack/certbot/src/acme_client.rs b/dstack/certbot/src/acme_client.rs index 6288303b9..56034b1d1 100644 --- a/dstack/certbot/src/acme_client.rs +++ b/dstack/certbot/src/acme_client.rs @@ -16,6 +16,7 @@ use rcgen::{CertificateParams, DistinguishedName, KeyPair}; use serde::{Deserialize, Serialize}; use std::{ collections::{BTreeMap, BTreeSet}, + fmt, net::SocketAddr, path::{Path, PathBuf}, time::Duration, @@ -25,23 +26,306 @@ use tracing::{debug, error, info, warn}; use x509_parser::prelude::{GeneralName, Pem}; use super::dns01_client::{Dns01Api, Dns01Client}; +use super::dns_persist::{self, AuthorizationRecord}; use super::http_client::ReqwestHttpClient; +/// The ACME challenge used to prove control of the certificate's domains. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] +pub enum ChallengeKind { + /// RFC 8555 `dns-01`. certbot answers every order by writing a TXT record + /// through the DNS provider API, so it needs a credential with write access + /// to the zone. + #[default] + #[serde(rename = "dns-01")] + Dns01, + /// draft-ietf-acme-dns-persist-01 `dns-persist-01`. Control is proven by a + /// `_validation-persist` record published once, out of band; certbot needs no + /// DNS credential and the zone can be hosted anywhere. + /// + /// Experimental — see `docs/certbot-dns-persist-01.md`. + #[serde(rename = "dns-persist-01")] + DnsPersist01, +} + +/// How the client proves control of a domain to the ACME server. +/// +/// The two methods differ in who writes DNS. `dns-01` needs certbot to hold a +/// provider credential with write access to the zone for the lifetime of the +/// deployment; `dns-persist-01` moves that to a one-time record the operator +/// publishes by hand, after which certbot only ever reads DNS. +#[derive(Debug)] +pub enum ValidationMethod { + /// RFC 8555 `dns-01`: certbot publishes a fresh `_acme-challenge` TXT record + /// through the provider API for every order and removes it afterwards. + Dns01 { + /// Provider client with write access to the zone. + client: Dns01Client, + /// TTL of the published records, in seconds (1 = auto, min 60 on Cloudflare). + txt_ttl: u32, + /// Issuer Domain Name to name in the CAA records certbot publishes. + issuer_domain_name: String, + }, + /// draft-ietf-acme-dns-persist-01 `dns-persist-01`: control is proven by a + /// `_validation-persist` TXT record naming the CA and this ACME account, + /// published once and left in place. certbot needs no provider credential, + /// and the zone can be hosted anywhere. + /// + /// Experimental: the draft is still changing and Let's Encrypt serves this + /// challenge on staging only. See `docs/certbot-dns-persist-01.md`. + DnsPersist01 { + /// Issuer Domain Name to name in the record and in CAA records. Must be + /// one of the `issuer-domain-names` the CA sends in the challenge — + /// `letsencrypt.org` for Let's Encrypt. + issuer_domain_name: String, + }, +} + +impl ValidationMethod { + /// Which challenge this method answers. + fn kind(&self) -> ChallengeKind { + match self { + Self::Dns01 { .. } => ChallengeKind::Dns01, + Self::DnsPersist01 { .. } => ChallengeKind::DnsPersist01, + } + } + + /// The challenge type to look for in an authorization. + fn challenge_type(&self) -> ChallengeType { + match self { + Self::Dns01 { .. } => ChallengeType::Dns01, + // instant-acme has no variant for the draft challenge, so it lands in + // `Unknown`. Matching on the wire string is what selects it. + Self::DnsPersist01 { .. } => ChallengeType::Unknown(DNS_PERSIST_01.to_string()), + } + } + + /// Issuer Domain Name to write into CAA records. + /// + /// Both methods read it from configuration, whose default is + /// `letsencrypt.org` -- the name existing deployments already have published + /// -- so an untouched configuration writes what it wrote before. A CAA + /// record naming a CA other than the one at `acme_url` forbids the very + /// issuance it is published to enable, and that is not a dns-01/ + /// dns-persist-01 distinction. + fn issuer_domain_name(&self) -> &str { + match self { + Self::Dns01 { + issuer_domain_name, .. + } => issuer_domain_name, + Self::DnsPersist01 { issuer_domain_name } => issuer_domain_name, + } + } + + /// The provider client, or an error naming why there isn't one. + fn dns01_client(&self) -> Result<&Dns01Client> { + match self { + Self::Dns01 { client, .. } => Ok(client), + Self::DnsPersist01 { .. } => bail!( + "dns-persist-01 holds no DNS provider credential, so certbot cannot write \ + records; publish them out of band (see the `dns-records` command)" + ), + } + } +} + +/// Wire name of the draft challenge, as it appears in the authorization. +const DNS_PERSIST_01: &str = "dns-persist-01"; + +/// A DNS record that has to exist before the CA will issue. +/// +/// Rendered as a zone-file line so it can be pasted into any provider. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct RequiredRecord { + /// FQDN the record lives at. + pub name: String, + /// Record type, e.g. `TXT` or `CAA`. + pub record_type: String, + /// Record value, including any CAA flags and tag. + pub content: String, +} + +impl fmt::Display for RequiredRecord { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}. IN {} {}", self.name, self.record_type, self.content) + } +} + +/// The CAA `issue`/`issuewild` value that pins issuance to one account. +/// +/// `validationmethods` names the challenge actually in use: a record left +/// pinned to `dns-01` after switching to `dns-persist-01` refuses every order, +/// and vice versa. +fn caa_content(challenge: ChallengeKind, issuer_domain_name: &str, account_uri: &str) -> String { + let method = match challenge { + ChallengeKind::Dns01 => "dns-01", + ChallengeKind::DnsPersist01 => DNS_PERSIST_01, + }; + format!("{issuer_domain_name};validationmethods={method};accounturi={account_uri}") +} + +/// Every DNS record that has to exist for the CA to issue for `domains`. +/// +/// Under `dns-01` certbot writes these itself and the list is informational. +/// Under `dns-persist-01` it holds no credential and cannot write anything, so +/// this list *is* the one-time setup an operator has to publish by hand. +/// +/// Takes the account URI rather than a client so that callers holding only the +/// stored credentials — an admin listing, say — can render the records without +/// a round trip to the CA. +pub fn required_dns_records( + challenge: ChallengeKind, + issuer_domain_name: &str, + account_uri: &str, + domains: &[String], +) -> Vec { + let caa_content = caa_content(challenge, issuer_domain_name, account_uri); + let mut records = Vec::new(); + for base_name in base_names(domains) { + if challenge == ChallengeKind::DnsPersist01 { + // One record covers the base name and, with the wildcard policy, + // `*.`; only ask for the policy when a wildcard is + // actually requested, so the record grants no more than needed. + let record = AuthorizationRecord { + issuer_domain_name: issuer_domain_name.to_string(), + account_uri: account_uri.to_string(), + wildcard: domains + .iter() + .any(|name| name.strip_prefix("*.") == Some(base_name)), + }; + records.push(RequiredRecord { + name: dns_persist::validation_domain(base_name), + record_type: "TXT".to_string(), + content: format!("\"{}\"", record.rdata()), + }); + } + for tag in ["issue", "issuewild"] { + records.push(RequiredRecord { + name: base_name.to_string(), + record_type: "CAA".to_string(), + content: format!("0 {tag} \"{caa_content}\""), + }); + } + } + records +} + +/// An ACME account, as registered, before any validation method is chosen. +/// +/// Returned by [`AcmeClient::register_account`] so a caller can store the +/// credentials and report the URI -- the two things a `dns-persist-01` record +/// and an account-pinned CAA record are written from -- without holding a +/// client it has no use for yet. +pub struct AcmeAccount { + /// Encoded credentials, in the form [`AcmeClient::load`] takes. + pub credentials: String, + /// URI the CA identifies this account by. + pub account_uri: String, +} + +/// The share of `renew_timeout` the pre-order DNS check may spend. +/// +/// The rest pays for the order itself: `new_order`, the authorizations, the +/// provider writes under `dns-01`, then finalize and the certificate fetch. +const DNS_WAIT_SHARE_OF_RENEW_TIMEOUT: u32 = 2; + +/// The DNS wait to actually use, given what is configured and how long the whole +/// order is allowed to take. +/// +/// The wait is advisory by design: certbot polls its own resolver, logs what it +/// could not see, and starts the order regardless, because the CA's DNS view is +/// not this node's. That only holds if the wait ends before the timeout wrapping +/// the order does. It does not by default -- a DNS credential's `max_dns_wait` +/// defaults to 300s and `renew_timeout` defaults to 300s, and the wait is +/// measured from after the order and its authorizations are fetched, so the +/// outer timeout always fires first. The renewal then dies with "certificate +/// request timed out", and the warning naming the record that was missing -- +/// the first thing an operator is told to look for -- is never logged. +/// +/// Clamping here rather than picking a smaller constant covers both challenges +/// with one rule, and keeps holding when an operator lowers `renew_timeout` +/// from the dashboard, which no constant can. +pub fn advisory_dns_wait(configured: Duration, renew_timeout: Duration) -> Duration { + let capped = renew_timeout / DNS_WAIT_SHARE_OF_RENEW_TIMEOUT; + if configured > capped { + // At `debug!` deliberately: the stock defaults have both values at 300s, + // so this fires on every issuance in every deployment. Phrased as the + // budget it is rather than as an override, so an operator who does raise + // `max_dns_wait` and finds nothing changed can see why here, without a + // permanent line in everyone else's log implying they configured + // something that was ignored. + debug!( + "DNS check budget is {capped:?}, half of the {renew_timeout:?} renewal timeout; \ + the configured wait of {configured:?} does not fit inside it" + ); + } + configured.min(capped) +} + /// A AcmeClient instance. pub struct AcmeClient { account: Account, credentials: Credentials, - dns01_client: Dns01Client, + validation: ValidationMethod, max_dns_wait: Duration, - /// TTL for DNS TXT records used in ACME challenges (in seconds). - dns_txt_ttl: u32, } +/// One pending authorization and the DNS record that answers it. #[derive(Debug, Clone)] struct Challenge { - id: String, + /// Provider-assigned record id, used by the cleanup pass after the order + /// settles. `None` when certbot did not create the record and must not + /// delete it — `dns-persist-01` records belong to the operator. + id: Option, + /// FQDN the TXT record lives at. acme_domain: String, - dns_value: String, + /// What a TXT record there has to say for the CA to accept the challenge. + expected: Expected, +} + +/// The condition a challenge's TXT records have to meet. +#[derive(Debug, Clone)] +enum Expected { + /// `dns-01`: the key authorization digest, matched verbatim. + KeyAuthorization(String), + /// `dns-persist-01`: an issue-value naming our issuer and account. + Authorization(AuthorizationRecord), +} + +impl Expected { + /// Whether the TXT records currently published at the challenge domain answer + /// the challenge. + fn satisfied_by(&self, published: &[String], now: u64) -> bool { + match self { + Self::KeyAuthorization(value) => published.iter().any(|txt| txt == value), + Self::Authorization(record) => record.satisfied_by_any(published, now), + } + } +} + +impl Challenge { + /// A line naming what is missing at the challenge domain, phrased so an + /// operator can act on it: under `dns-persist-01` it is the exact record to + /// publish, under `dns-01` it is the value certbot just wrote. + fn unsettled_hint(&self) -> String { + format!( + "no TXT record at {} matches the expected value: {}", + self.acme_domain, self.expected + ) + } +} + +impl fmt::Display for Expected { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::KeyAuthorization(value) => f.write_str(value), + Self::Authorization(record) => write!(f, "{}", record.rdata()), + } + } +} + +/// Current UNIX time, for comparing against a record's `persistUntil`. +fn now_secs() -> u64 { + time::OffsetDateTime::now_utc().unix_timestamp().max(0) as u64 } #[derive(Serialize, Deserialize)] @@ -59,16 +343,39 @@ pub(crate) fn acme_matches(encoded_credentials: &str, acme_url: &str) -> bool { credentials.acme_url == acme_url } +/// The names CAA and `dns-persist-01` records are published under, deduplicated. +/// +/// A wildcard request is authorized from its base name, so `example.com` and +/// `*.example.com` share one set of records. +fn base_names(domains: &[String]) -> BTreeSet<&str> { + domains + .iter() + .map(|name| name.strip_prefix("*.").unwrap_or(name)) + .collect() +} + fn caa_tag(content: &str) -> Option<&str> { content.split_whitespace().nth(1) } +/// Whether a rendered CAA record is one of the `;` guards [`AcmeClient::set_caa_records`] +/// installs while it swaps the real records out. +/// +/// A guard denies every issuer, which is the point while the old records are +/// being replaced and the wrong thing to leave behind. +fn is_caa_guard(content: &str) -> bool { + let mut parts = content.split_whitespace(); + let (_flags, tag, value) = (parts.next(), parts.next(), parts.next()); + matches!(tag, Some("issue" | "issuewild")) + && value.map(|value| value.trim_matches('"')) == Some(";") + && parts.next().is_none() +} + impl AcmeClient { pub async fn load( - dns01_client: Dns01Client, + validation: ValidationMethod, encoded_credentials: &str, max_dns_wait: Duration, - dns_txt_ttl: u32, ) -> Result { let credentials: Credentials = serde_json::from_str(encoded_credentials)?; let http_client = Box::new(ReqwestHttpClient::new()?); @@ -78,20 +385,51 @@ impl AcmeClient { let credentials: Credentials = serde_json::from_str(encoded_credentials)?; Ok(Self { account, - dns01_client, + validation, credentials, max_dns_wait, - dns_txt_ttl, }) } - /// Create a new account. + /// Register a new ACME account, with no validation method attached. + /// + /// Registration is a POST to the directory's `newAccount`. It asserts + /// nothing about any domain and touches no DNS, so it needs neither a + /// provider credential nor a challenge choice -- and a deployment can + /// therefore have an account before it has anything to validate. That order + /// is forced under `dns-persist-01`: the record an operator publishes names + /// the account, so the account has to exist first. + pub async fn register_account(acme_url: &str) -> Result { + let (_, credentials) = Self::register(acme_url).await?; + Ok(AcmeAccount { + account_uri: credentials.account_id.clone(), + credentials: serde_json::to_string(&credentials)?, + }) + } + + /// Create a new account and a client bound to `validation`. pub async fn new_account( acme_url: &str, - dns01_client: Dns01Client, + validation: ValidationMethod, max_dns_wait: Duration, - dns_txt_ttl: u32, ) -> Result { + // Built from the account `register` already holds rather than by + // reloading the credentials: `load` fetches the directory, and a + // transient failure there would drop an account that exists at the CA + // and counts against its registration limit, on a path whose caller has + // not persisted anything yet. + let (account, credentials) = Self::register(acme_url).await?; + Ok(Self { + account, + validation, + credentials, + max_dns_wait, + }) + } + + /// Register an account and keep both halves: the live account and the + /// credentials that reconstruct it. + async fn register(acme_url: &str) -> Result<(Account, Credentials)> { let http_client = Box::new(ReqwestHttpClient::new()?); let (account, credentials) = Account::builder_with_http(http_client) .create( @@ -105,18 +443,15 @@ impl AcmeClient { ) .await .with_context(|| format!("failed to create ACME account for {acme_url}"))?; - let credentials = Credentials { - acme_url: acme_url.to_string(), - account_id: account.id().to_string(), - credentials, - }; - Ok(Self { + let account_id = account.id().to_string(); + Ok(( account, - dns01_client, - credentials, - max_dns_wait, - dns_txt_ttl, - }) + Credentials { + acme_url: acme_url.to_string(), + account_id, + credentials, + }, + )) } /// Dump the account credentials to a JSON string. @@ -129,27 +464,89 @@ impl AcmeClient { &self.credentials.account_id } + /// The CAA `issue`/`issuewild` value that pins issuance to this account. + fn caa_content(&self) -> String { + caa_content( + self.validation.kind(), + self.validation.issuer_domain_name(), + self.account_id(), + ) + } + + /// Every DNS record that has to exist for the CA to issue for `domains`. + /// + /// See [`required_dns_records`], which this fills in from the live account. + pub fn required_dns_records(&self, domains: &[String]) -> Vec { + required_dns_records( + self.validation.kind(), + self.validation.issuer_domain_name(), + self.account_id(), + domains, + ) + } + pub async fn set_caa_records(&self, domains: &[String]) -> Result<()> { - let account_id = self.account_id(); - let content = format!("letsencrypt.org;validationmethods=dns-01;accounturi={account_id}"); - let base_names = domains - .iter() - .map(|name| name.strip_prefix("*.").unwrap_or(name)) - .collect::>(); + let dns01_client = self.validation.dns01_client().context( + "cannot set CAA records without DNS write access; publish the records \ + printed by `certbot dns-records` instead", + )?; + let content = self.caa_content(); + let base_names = base_names(domains); for base_name in base_names { + // 0. Adopt guards an earlier run left behind, rather than + // replacing them. A run that died between steps 1 and 4 stranded + // `;` records, and step 1 would re-add byte-identical ones -- + // which a provider that refuses duplicates rejects, so the rerun + // the operator is told to perform fails at its very first call + // and the zone never recovers. + // + // Deleting them first would fix that and open a worse hole: from + // the delete until step 1 succeeds the name has no issue or + // issuewild record at all, which is not "denied" but "any CA may + // issue" -- and a certificate obtained in that window outlives + // the window, where a stranded guard is merely a renewal that + // fails until someone reruns. Reusing the record keeps the + // deny-all continuous, which is the whole point of a guard. + // + // Extras beyond the two adopted here are ordinary issue records + // to step 2, which removes them. + let mut adopted_issue = None; + let mut adopted_issuewild = None; + for record in dns01_client.get_records(base_name).await? { + if record.r#type != "CAA" || !is_caa_guard(&record.content) { + continue; + } + let slot = match caa_tag(&record.content) { + Some("issue") => &mut adopted_issue, + Some("issuewild") => &mut adopted_issuewild, + _ => continue, + }; + if slot.is_none() { + debug!("adopting stale guard CAA record {}", record.name); + *slot = Some(record.id); + } + } // 1. Set ";" to guard timing gap between the operations. debug!("setting guard CAA records for {base_name}"); - let guard0 = self - .dns01_client - .add_caa_record(base_name, 0, "issue", ";") - .await?; - let guard1 = self - .dns01_client - .add_caa_record(base_name, 0, "issuewild", ";") - .await?; + let guard0 = match adopted_issue { + Some(id) => id, + None => { + dns01_client + .add_caa_record(base_name, 0, "issue", ";") + .await? + } + }; + let guard1 = match adopted_issuewild { + Some(id) => id, + None => { + dns01_client + .add_caa_record(base_name, 0, "issuewild", ";") + .await? + } + }; // 2. Remove the existing constraints - for record in self.dns01_client.get_records(base_name).await? { + for record in dns01_client.get_records(base_name).await? { if record.id == guard0 || record.id == guard1 { continue; } @@ -161,22 +558,22 @@ impl AcmeClient { "removing existing issuer CAA record {} {}", record.name, record.content ); - self.dns01_client.remove_record(&record.id).await?; + dns01_client.remove_record(&record.id).await?; } } // 3. Set the new constraints debug!("setting CAA records for {base_name}, 0 issue \"{content}\""); - self.dns01_client + dns01_client .add_caa_record(base_name, 0, "issue", &content) .await?; debug!("setting CAA records for {base_name}, 0 issuewild \"{content}\""); - self.dns01_client + dns01_client .add_caa_record(base_name, 0, "issuewild", &content) .await?; debug!("removing guard CAA records for {base_name}"); // 4. Remove the guards - self.dns01_client.remove_record(&guard0).await?; - self.dns01_client.remove_record(&guard1).await?; + dns01_client.remove_record(&guard0).await?; + dns01_client.remove_record(&guard1).await?; } Ok(()) } @@ -190,10 +587,18 @@ impl AcmeClient { let result = self .request_new_certificate_inner(key, domains, &mut challenges) .await; - for challenge in &challenges { - debug!("removing dns record {}", challenge.id); - if let Err(err) = self.dns01_client.remove_record(&challenge.id).await { - error!("failed to remove dns record {}: {err}", challenge.id); + // Only records certbot created are cleaned up. A dns-persist-01 record + // is the operator's and outlives every order, so it carries no id. + for id in challenges + .iter() + .filter_map(|challenge| challenge.id.as_ref()) + { + let Ok(dns01_client) = self.validation.dns01_client() else { + break; + }; + debug!("removing dns record {id}"); + if let Err(err) = dns01_client.remove_record(id).await { + error!("failed to remove dns record {id}: {err}"); } } result @@ -347,6 +752,7 @@ impl AcmeClient { impl AcmeClient { async fn authorize(&self, order: &mut Order, challenges: &mut Vec) -> Result<()> { + let challenge_type = self.validation.challenge_type(); let mut authorizations = order.authorizations(); while let Some(authz) = authorizations.next().await { let mut authz = authz.context("failed to get authorizations")?; @@ -356,42 +762,61 @@ impl AcmeClient { _ => bail!("unsupported authorization status: {:?}", authz.status), } + // Read before taking the challenge handle, which borrows the + // authorization for the rest of the iteration. + let wildcard = authz.wildcard; let challenge = authz - .challenge(ChallengeType::Dns01) - .context("no dns01 challenge found")?; - - let acme_domain = challenge_domain(challenge.identifier())?; - let dns_value = challenge.key_authorization().dns_value(); - // 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 - ); - let id = self - .dns01_client - .add_txt_record(&acme_domain, &dns_value, self.dns_txt_ttl) - .await - .context("failed to create dns record")?; - challenges.push(Challenge { - id, - acme_domain, - dns_value, - }); + .challenge(challenge_type.clone()) + .with_context(|| format!("no {challenge_type:?} challenge found"))?; + + let acme_domain = challenge_domain(self.validation.kind(), challenge.identifier())?; + let challenge = match &self.validation { + ValidationMethod::Dns01 { + client, txt_ttl, .. + } => { + let dns_value = challenge.key_authorization().dns_value(); + // 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}"); + client + .remove_txt_records(&acme_domain) + .await + .context("failed to remove existing dns record")?; + } + debug!("creating TXT record for {acme_domain} with TTL {txt_ttl}s"); + let id = client + .add_txt_record(&acme_domain, &dns_value, *txt_ttl) + .await + .context("failed to create dns record")?; + Challenge { + id: Some(id), + acme_domain, + expected: Expected::KeyAuthorization(dns_value), + } + } + // Nothing to publish: the record is already in the zone, or the + // order is about to fail and say so. There is likewise nothing + // to purge -- the record is the operator's, and one persistent + // record answers every authorization under the name. + ValidationMethod::DnsPersist01 { issuer_domain_name } => Challenge { + id: None, + acme_domain, + expected: Expected::Authorization(AuthorizationRecord { + issuer_domain_name: issuer_domain_name.clone(), + account_uri: self.account_id().to_string(), + wildcard, + }), + }, + }; + challenges.push(challenge); } Ok(()) } @@ -416,10 +841,10 @@ impl AcmeClient { // the name below it: for `_acme-challenge.a.example.com` the NS records // usually live on `example.com`. Querying the full name returns NODATA, // so walk up a label at a time until a name actually carries NS records. - let mut candidate = domain - .strip_prefix("_acme-challenge.") - .unwrap_or(domain) - .to_string(); + // The leading label is dropped for any challenge prefix -- `_acme-challenge` + // for dns-01, `_validation-persist` for dns-persist-01 -- since an + // underscore label never carries NS records. + let mut candidate = strip_challenge_label(domain).to_string(); let mut addrs = Vec::new(); let mut zone = candidate.clone(); loop { @@ -452,7 +877,13 @@ impl AcmeClient { resolver_for(&addrs) } - /// Self check the TXT records for the given challenges. + /// Wait until every challenge's records are visible to us. + /// + /// Advisory, not a gate: our resolver is not the CA's, and under + /// `dns-persist-01` our expectation can even be stricter than the CA's -- + /// the `issuer-domain-names` it would accept are not exposed by + /// instant-acme. A record we never see is reported and the order proceeds, + /// letting the CA decide. async fn check_dns(&self, challenges: &[Challenge]) -> Result<()> { let mut delay = Duration::from_millis(250); let mut tries = 1u8; @@ -492,7 +923,17 @@ impl AcmeClient { } 'outer: loop { - sleep(delay).await; + // The budget is checked before sleeping, and the sleep is cut to + // what is left of it. Sleeping first and checking after overshoots + // by a whole backoff step -- up to 32s -- which for a short + // `renew_timeout` is enough to hand the deadline to the timeout + // wrapping the order, so the graceful exit below is missed and the + // renewal ends as a timeout with nothing named. + let elapsed = start_time.elapsed(); + let remaining = self.max_dns_wait.saturating_sub(elapsed); + if !remaining.is_zero() { + sleep(delay.min(remaining)).await; + } let elapsed = start_time.elapsed(); if elapsed >= self.max_dns_wait { @@ -500,24 +941,26 @@ impl AcmeClient { "DNS propagation timeout after {elapsed:?}, max wait time is {max:?}. proceeding anyway as ACME server may have different DNS view", max = self.max_dns_wait ); + for challenge in &unsettled_challenges { + warn!("{}", challenge.unsettled_hint()); + } break; } while let Some(challenge) = unsettled_challenges.pop() { - let expected_txt = &challenge.dns_value; let dns_resolver = resolvers .get(&challenge.acme_domain) .context("no resolver for challenge domain")?; - let settled = match dns_resolver.txt_lookup(&challenge.acme_domain).await { - Ok(record) => record.answers().iter().any(|answer| { - let RData::TXT(txt) = &answer.data else { - return false; - }; - let actual_txt = txt.to_string(); - debug!("Expected challenge: {expected_txt}, actual: {actual_txt}"); - actual_txt == *expected_txt - }), - Err(err) if err.is_no_records_found() => false, + let published = match dns_resolver.txt_lookup(&challenge.acme_domain).await { + Ok(records) => records + .answers() + .iter() + .filter_map(|answer| match &answer.data { + RData::TXT(txt) => Some(txt.to_string()), + _ => None, + }) + .collect::>(), + Err(err) if err.is_no_records_found() => Vec::new(), Err(err) if !fell_back.contains(&challenge.acme_domain) => { // Transport failures land here rather than in the arm // above: `is_no_records_found` covers only @@ -548,10 +991,14 @@ impl AcmeClient { domain = &challenge.acme_domain, "dns lookup failed: {err:#}" ); - false + Vec::new() } }; - if !settled { + debug!( + "Expected challenge: {}, actual: {published:?}", + challenge.expected + ); + if !challenge.expected.satisfied_by(&published, now_secs()) { delay = Duration::from_secs(32).min(delay * 2); tries += 1; debug!( @@ -570,13 +1017,14 @@ impl AcmeClient { Ok(()) } - /// Tell the ACME server every pending dns-01 challenge is answerable. + /// Tell the ACME server every pending challenge is answerable. /// - /// The TXT records are published first and verified for propagation, so this - /// is a second pass over the same authorizations: 0.8 dropped - /// `Order::set_challenge_ready(url)`, and `ChallengeHandle::set_ready()` - /// borrows the order, so the handle cannot be held across the DNS wait. + /// The DNS records are published and checked first, so this is a second pass + /// over the same authorizations: 0.8 dropped `Order::set_challenge_ready(url)`, + /// and `ChallengeHandle::set_ready()` borrows the order, so the handle cannot + /// be held across the DNS wait. async fn set_challenges_ready(&self, order: &mut Order) -> Result<()> { + let challenge_type = self.validation.challenge_type(); let mut authorizations = order.authorizations(); while let Some(authz) = authorizations.next().await { let mut authz = authz.context("failed to get authorizations")?; @@ -584,8 +1032,8 @@ impl AcmeClient { continue; } let mut challenge = authz - .challenge(ChallengeType::Dns01) - .context("no dns01 challenge found")?; + .challenge(challenge_type.clone()) + .with_context(|| format!("no {challenge_type:?} challenge found"))?; debug!("setting challenge ready for {}", challenge.url); challenge .set_ready() @@ -695,17 +1143,36 @@ fn needs_purge(published: &[Challenge], acme_domain: &str) -> bool { .any(|challenge| challenge.acme_domain == acme_domain) } -/// The name of the TXT record that answers a dns-01 challenge for `identifier`. +/// The name of the TXT record that answers `challenge`'s validation for `identifier`. /// /// The record always lives under the bare name: a wildcard authorization for -/// `*.example.com` is answered at `_acme-challenge.example.com`. `AuthorizedIdentifier` +/// `*.example.com` is answered at `_acme-challenge.example.com`, and at +/// `_validation-persist.example.com` under dns-persist-01. `AuthorizedIdentifier` /// renders the wildcard prefix in its `Display`, so formatting it directly would publish /// the record at `_acme-challenge.*.example.com` and fail every wildcard issuance. -fn challenge_domain(identifier: &AuthorizedIdentifier<'_>) -> Result { +fn challenge_domain( + challenge: ChallengeKind, + identifier: &AuthorizedIdentifier<'_>, +) -> Result { let Identifier::Dns(name) = identifier.identifier else { bail!("unsupported identifier type in authorization: {identifier}"); }; - Ok(format!("_acme-challenge.{name}")) + Ok(match challenge { + ChallengeKind::Dns01 => format!("_acme-challenge.{name}"), + ChallengeKind::DnsPersist01 => dns_persist::validation_domain(name), + }) +} + +/// Drop a leading underscore label, so the zone walk starts at a real name. +/// +/// Challenge records live under a reserved label -- `_acme-challenge` for +/// dns-01, `_validation-persist` for dns-persist-01 -- which never carries NS +/// records, so querying it only costs a round trip. +fn strip_challenge_label(domain: &str) -> &str { + match domain.starts_with('_') { + true => domain.split_once('.').map_or(domain, |(_, rest)| rest), + false => domain, + } } async fn find_error(order: &mut Order) -> Result> { @@ -952,6 +1419,23 @@ mod challenge_parsing_tests { mod ns_discovery_tests { use super::parent_zone; + #[test] + fn a_challenge_label_is_dropped_before_the_walk_starts() { + use super::strip_challenge_label; + + // Both challenge methods put their record under a reserved underscore + // label, and neither label can carry NS records. + assert_eq!( + strip_challenge_label("_acme-challenge.example.com"), + "example.com" + ); + assert_eq!( + strip_challenge_label("_validation-persist.example.com"), + "example.com" + ); + assert_eq!(strip_challenge_label("example.com"), "example.com"); + } + #[test] fn the_walk_climbs_to_a_name_that_can_carry_ns_records() { // A challenge name is not a zone cut, so the walk has to climb to the @@ -974,43 +1458,156 @@ mod ns_discovery_tests { #[cfg(test)] mod challenge_domain_tests { - use super::challenge_domain; + use super::{challenge_domain, ChallengeKind}; use instant_acme::Identifier; /// A wildcard order authorizes the bare name with `wildcard: true`, and the TXT record /// answering it must be published under that bare name. Formatting the identifier /// through its `Display` instead would ask for `_acme-challenge.*.example.com`. + /// dns-persist-01 answers the same authorization under its own label, and the + /// wildcard prefix must not leak into that name either. #[test] fn the_challenge_domain_never_carries_a_wildcard_prefix() { let dns = Identifier::Dns("example.com".to_string()); + for wildcard in [false, true] { + assert_eq!( + challenge_domain(ChallengeKind::Dns01, &dns.authorized(wildcard)).unwrap(), + "_acme-challenge.example.com", + "wildcard={wildcard}" + ); + assert_eq!( + challenge_domain(ChallengeKind::DnsPersist01, &dns.authorized(wildcard)).unwrap(), + "_validation-persist.example.com", + "wildcard={wildcard}" + ); + } + } + + /// Both methods are only defined for DNS identifiers; anything else is a bug in the + /// order we built, so it must be an error rather than a nonsensical record name. + #[test] + fn a_non_dns_identifier_is_rejected() { + let ip = Identifier::Ip("192.0.2.1".parse().unwrap()); + assert!(challenge_domain(ChallengeKind::Dns01, &ip.authorized(false)).is_err()); + assert!(challenge_domain(ChallengeKind::DnsPersist01, &ip.authorized(false)).is_err()); + } +} + +#[cfg(test)] +mod required_record_tests { + use super::*; + + const ACCOUNT: &str = "https://acme-v02.api.letsencrypt.org/acme/acct/1234567890"; + + fn records(challenge: ChallengeKind, domains: &[&str]) -> Vec { + required_dns_records( + challenge, + dns_persist::LETS_ENCRYPT_ISSUER_DOMAIN_NAME, + ACCOUNT, + &domains.iter().map(|d| d.to_string()).collect::>(), + ) + .iter() + .map(ToString::to_string) + .collect() + } + + #[test] + fn dns01_caa_content_is_byte_identical_to_the_pinned_format() { + // CAA values published by earlier releases have to keep matching, or + // issuance stops the moment this string drifts. assert_eq!( - challenge_domain(&dns.authorized(false)).unwrap(), - "_acme-challenge.example.com" + caa_content( + ChallengeKind::Dns01, + dns_persist::LETS_ENCRYPT_ISSUER_DOMAIN_NAME, + ACCOUNT + ), + format!("letsencrypt.org;validationmethods=dns-01;accounturi={ACCOUNT}") ); + } + + #[test] + fn caa_content_names_the_challenge_in_use() { + // A CAA record still pinned to dns-01 refuses every dns-persist-01 order. + assert!(caa_content( + ChallengeKind::DnsPersist01, + dns_persist::LETS_ENCRYPT_ISSUER_DOMAIN_NAME, + ACCOUNT + ) + .contains("validationmethods=dns-persist-01")); + } + + #[test] + fn dns01_needs_no_validation_record() { + // certbot writes the `_acme-challenge` record itself, per order. assert_eq!( - challenge_domain(&dns.authorized(true)).unwrap(), - "_acme-challenge.example.com" + records(ChallengeKind::Dns01, &["*.example.com"]), + vec![ + format!( + "example.com. IN CAA 0 issue \"letsencrypt.org;validationmethods=dns-01;accounturi={ACCOUNT}\"" + ), + format!( + "example.com. IN CAA 0 issuewild \"letsencrypt.org;validationmethods=dns-01;accounturi={ACCOUNT}\"" + ), + ] ); } - /// dns-01 is only defined for DNS identifiers; anything else is a bug in the order we - /// built, so it must be an error rather than a nonsensical record name. #[test] - fn a_non_dns_identifier_is_rejected() { - let ip = Identifier::Ip("192.0.2.1".parse().unwrap()); - assert!(challenge_domain(&ip.authorized(false)).is_err()); + fn dns_persist_asks_for_the_wildcard_policy_only_when_a_wildcard_is_ordered() { + let plain = records(ChallengeKind::DnsPersist01, &["example.com"]); + assert_eq!( + plain[0], + format!( + "_validation-persist.example.com. IN TXT \"letsencrypt.org; accounturi={ACCOUNT}\"" + ) + ); + + let wildcard = records(ChallengeKind::DnsPersist01, &["*.example.com"]); + assert_eq!( + wildcard[0], + format!( + "_validation-persist.example.com. IN TXT \"letsencrypt.org; accounturi={ACCOUNT}; policy=wildcard\"" + ) + ); + } + + #[test] + fn a_name_and_its_wildcard_share_one_validation_record() { + // Both identifiers authorize from the same base name, so asking the + // operator for two records would be asking for one too many. + let records = records( + ChallengeKind::DnsPersist01, + &["example.com", "*.example.com"], + ); + assert_eq!( + records.iter().filter(|r| r.contains(" TXT ")).count(), + 1, + "{records:#?}" + ); + assert!(records[0].contains("policy=wildcard"), "{records:#?}"); + } + + #[test] + fn each_base_name_gets_its_own_records() { + let records = records( + ChallengeKind::DnsPersist01, + &["*.a.example.com", "*.b.example.com"], + ); + assert_eq!(records.len(), 6, "{records:#?}"); + assert!(records[0].starts_with("_validation-persist.a.example.com.")); + assert!(records[3].starts_with("_validation-persist.b.example.com.")); } } #[cfg(test)] mod purge_tests { - use super::{needs_purge, Challenge}; + use super::{needs_purge, Challenge, Expected}; fn challenge(acme_domain: &str, dns_value: &str) -> Challenge { Challenge { - id: format!("rec-{dns_value}"), + id: Some(format!("rec-{dns_value}")), acme_domain: acme_domain.to_string(), - dns_value: dns_value.to_string(), + expected: Expected::KeyAuthorization(dns_value.to_string()), } } @@ -1128,3 +1725,69 @@ mod reissue_reason_tests { assert!(reason.contains("cannot read"), "{reason}"); } } + +#[cfg(test)] +mod caa_guard_tests { + use super::{caa_tag, is_caa_guard}; + + /// The guard is what an interrupted `set_caa_records` leaves behind, and a + /// rerun has to recognize its own leftovers to be able to replace them. + #[test] + fn a_guard_is_recognized_however_the_provider_renders_it() { + assert!(is_caa_guard(r#"0 issue ";""#)); + assert!(is_caa_guard(r#"0 issuewild ";""#)); + assert!(is_caa_guard("0 issue ;")); + assert!(is_caa_guard("128 issue \";\"")); + } + + /// A real issuer record must never be mistaken for a guard: sweeping one + /// early would delete the zone's only valid CAA before its replacement is + /// written. + #[test] + fn a_real_issuer_record_is_not_a_guard() { + assert!(!is_caa_guard( + r#"0 issue "letsencrypt.org;validationmethods=dns-01;accounturi=https://acme.example/acct/1""# + )); + assert!(!is_caa_guard(r#"0 issue "letsencrypt.org""#)); + // `iodef` is neither an issuer constraint nor a guard. + assert!(!is_caa_guard(r#"0 iodef "mailto:x@example.com""#)); + // A value that merely starts with the guard's character. + assert!(!is_caa_guard(r#"0 issue ";extra""#)); + // Trailing junk means it is not the record this wrote. + assert!(!is_caa_guard(r#"0 issue ";" extra"#)); + assert!(!is_caa_guard("")); + } + + /// The sweep is scoped by the same tag test the replacement pass uses. + #[test] + fn the_guard_tags_are_the_ones_the_replacement_pass_replaces() { + for content in [r#"0 issue ";""#, r#"0 issuewild ";""#] { + let tag = caa_tag(content).expect("a rendered CAA record has a tag"); + assert!(matches!(tag, "issue" | "issuewild"), "{tag}"); + } + } +} + +#[cfg(test)] +mod dns_wait_tests { + use super::advisory_dns_wait; + use std::time::Duration; + + /// The CLI's own defaults are the failing case: a 300s wait inside a 120s + /// renewal budget never reaches its "proceed anyway" exit, so an unanswered + /// check ends as a timeout rather than as the warning naming the record. + #[test] + fn the_wait_ends_before_the_renewal_budget_does() { + let wait = advisory_dns_wait(Duration::from_secs(300), Duration::from_secs(120)); + assert!(wait < Duration::from_secs(120), "{wait:?}"); + } + + /// A wait already inside the budget is left where the operator put it. + #[test] + fn a_wait_that_already_fits_is_unchanged() { + assert_eq!( + advisory_dns_wait(Duration::from_secs(30), Duration::from_secs(600)), + Duration::from_secs(30) + ); + } +} diff --git a/dstack/certbot/src/bot.rs b/dstack/certbot/src/bot.rs index 0daf499e3..2ead321b6 100644 --- a/dstack/certbot/src/bot.rs +++ b/dstack/certbot/src/bot.rs @@ -9,14 +9,16 @@ use std::{ time::Duration, }; -use anyhow::{Context, Result}; +use anyhow::{bail, Context, Result}; use fs_err as fs; use tokio::time::sleep; -use tracing::{error, info}; +use tracing::{error, info, warn}; -use crate::acme_client::{acme_matches, read_pem}; +use crate::acme_client::{acme_matches, read_pem, ChallengeKind, RequiredRecord, ValidationMethod}; +use crate::dns_persist::{resolve_issuer_domain_name, LETS_ENCRYPT_ISSUER_DOMAIN_NAME}; use super::{AcmeClient, Dns01Client}; +use crate::acme_client::advisory_dns_wait; #[allow(clippy::duplicated_attributes)] #[derive(Clone, Debug, bon::Builder)] @@ -27,6 +29,15 @@ pub struct CertBotConfig { auto_set_caa: bool, credentials_file: PathBuf, auto_create_account: bool, + /// ACME challenge used to prove control of the domains. + #[builder(default)] + challenge: ChallengeKind, + /// Issuer Domain Name naming the CA in `dns-persist-01` and CAA records. + /// + /// Must be one of the `issuer-domain-names` the CA sends in the challenge. + #[builder(default = LETS_ENCRYPT_ISSUER_DOMAIN_NAME.to_string())] + issuer_domain_name: String, + /// Cloudflare API token. Unused, and warned about, under `dns-persist-01`. cf_api_token: String, cf_api_url: Option, cert_file: PathBuf, @@ -57,17 +68,12 @@ pub struct CertBot { async fn create_new_account( config: &CertBotConfig, - dns01_client: Dns01Client, + validation: ValidationMethod, ) -> Result { info!("creating new ACME account"); - let client = AcmeClient::new_account( - &config.acme_url, - dns01_client, - config.max_dns_wait, - config.dns_txt_ttl, - ) - .await - .context("failed to create new account")?; + let client = AcmeClient::new_account(&config.acme_url, validation, dns_wait(config)) + .await + .context("failed to create new account")?; let credentials = client .dump_credentials() .context("failed to dump credentials")?; @@ -87,39 +93,20 @@ async fn create_new_account( impl CertBot { /// Build a new `CertBot` from a `CertBotConfig`. pub async fn build(config: CertBotConfig) -> Result { - let base_domain = config - .cert_subject_alt_names - .first() - .context("cert_subject_alt_names is empty")? - .trim() - .trim_start_matches("*.") - .trim_end_matches('.') - .to_string(); - let dns01_client = Dns01Client::new_cloudflare( - base_domain, - config.cf_api_token.clone(), - config.cf_api_url.clone(), - ) - .await?; + let validation = build_validation_method(&config).await?; let acme_client = match fs::read_to_string(&config.credentials_file) { Ok(credentials) => { if acme_matches(&credentials, &config.acme_url) { - AcmeClient::load( - dns01_client, - &credentials, - config.max_dns_wait, - config.dns_txt_ttl, - ) - .await? + AcmeClient::load(validation, &credentials, dns_wait(&config)).await? } else { - create_new_account(&config, dns01_client).await? + create_new_account(&config, validation).await? } } Err(e) if e.kind() == ErrorKind::NotFound => { if !config.auto_create_account { return Err(e).context("credentials file not found"); } - create_new_account(&config, dns01_client).await? + create_new_account(&config, validation).await? } Err(e) => { return Err(e).context("failed to read credentials file"); @@ -262,6 +249,96 @@ impl CertBot { .set_caa_records(&self.config.cert_subject_alt_names) .await } + + /// The DNS records that have to exist for the configured domains. + pub fn required_dns_records(&self) -> Vec { + self.acme_client + .required_dns_records(&self.config.cert_subject_alt_names) + } +} + +/// The DNS wait this configuration should actually use. +/// +/// `renew_timeout` wraps the whole renewal here exactly as it does in the +/// gateway, and the defaults are skewed the same way -- further, in fact: +/// `max_dns_wait` defaults to 300s against a 120s renewal budget, so an +/// unanswered check runs the renewal into its timeout every time instead of +/// reporting the record it could not see. +fn dns_wait(config: &CertBotConfig) -> Duration { + advisory_dns_wait(config.max_dns_wait, config.renew_timeout) +} + +/// The Issuer Domain Name this configuration names, checked before it is used. +/// +/// Both challenges read the same setting, so both get the same treatment: empty +/// means the default, and a value that would not survive being written into a +/// CAA or validation record is refused here rather than at the point it would +/// corrupt a zone. +fn issuer_domain_name(config: &CertBotConfig) -> Result { + resolve_issuer_domain_name(&config.issuer_domain_name) + .context("invalid issuer_domain_name in the certbot configuration") +} + +/// Resolve the configured challenge into a live validation method. +/// +/// `dns-01` resolves the Cloudflare zone here, which is an authenticated call, +/// so a bad credential fails at startup rather than at the first renewal. +/// `dns-persist-01` talks to no provider at all. +async fn build_validation_method(config: &CertBotConfig) -> Result { + match config.challenge { + ChallengeKind::Dns01 => { + // Named here rather than left to the provider. `cf_api_token` is + // `#[serde(default)]` so a dns-persist-01 config can omit it, which + // also means a dns-01 config that forgets it no longer fails + // deserialization -- it reaches Cloudflare and comes back as an + // "Invalid format for Authorization header", naming the header + // instead of the setting the operator has to add. + if config.cf_api_token.is_empty() { + bail!( + "cf_api_token is required with dns-01, which proves control by writing a \ + TXT record through the DNS provider; set it, or switch to \ + challenge = \"dns-persist-01\", which needs no provider credential" + ); + } + let base_domain = config + .cert_subject_alt_names + .first() + .context("cert_subject_alt_names is empty")? + .trim() + .trim_start_matches("*.") + .trim_end_matches('.') + .to_string(); + let client = Dns01Client::new_cloudflare( + base_domain, + config.cf_api_token.clone(), + config.cf_api_url.clone(), + ) + .await?; + Ok(ValidationMethod::Dns01 { + client, + txt_ttl: config.dns_txt_ttl, + issuer_domain_name: issuer_domain_name(config)?, + }) + } + ChallengeKind::DnsPersist01 => { + // Refuse rather than silently skip: `auto_set_caa` promises the CAA + // records are kept in sync, and without DNS write access nothing here + // can keep that promise. `certbot dns-records` prints what to publish. + if config.auto_set_caa { + bail!( + "auto_set_caa is not supported with dns-persist-01, which has no DNS \ + write access; set auto_set_caa = false and publish the records from \ + `certbot dns-records` by hand" + ); + } + if !config.cf_api_token.is_empty() { + warn!("ignoring cf_api_token: dns-persist-01 needs no DNS provider credential"); + } + Ok(ValidationMethod::DnsPersist01 { + issuer_domain_name: issuer_domain_name(config)?, + }) + } + } } pub fn read_pubkey(cert_pem: &str) -> Result> { diff --git a/dstack/certbot/src/dns_persist.rs b/dstack/certbot/src/dns_persist.rs new file mode 100644 index 000000000..ec21b0e98 --- /dev/null +++ b/dstack/certbot/src/dns_persist.rs @@ -0,0 +1,617 @@ +// SPDX-FileCopyrightText: © 2026 Phala Network +// +// SPDX-License-Identifier: Apache-2.0 + +//! The persistent DNS authorization record used by the `dns-persist-01` challenge. +//! +//! `dns-persist-01` (draft-ietf-acme-dns-persist-01) replaces the per-order +//! `_acme-challenge` TXT record of `dns-01` with a single record that stays in the +//! zone: `_validation-persist.`, naming the CA and the ACME account allowed +//! to issue for that name. The account key proves who is asking; the record proves +//! the zone owner agreed. Nothing about it changes between orders, so a client that +//! uses this method never needs write access to the zone. +//! +//! This module owns the record's syntax: rendering the line an operator has to +//! publish, and deciding whether what is currently published would satisfy the CA. +//! The RDATA is an RFC 8659 `issue-value` — the same grammar as a CAA `issue` +//! record — so the parser here mirrors the one CAs run (see `va/dns_persist.go` in +//! Boulder), including the parts that reject rather than ignore: a trailing +//! semicolon, a repeated tag, whitespace inside a value. + +use std::fmt; + +use anyhow::{bail, Context, Result}; + +/// Label prepended to the name being validated to form the validation domain name. +/// +/// draft-ietf-acme-dns-persist-01, section 4. +const VALIDATION_LABEL: &str = "_validation-persist"; + +/// Issuer Domain Name for Let's Encrypt, matching the `caaIdentities` it advertises. +/// +/// A CA lists the names it answers to in the challenge object's +/// `issuer-domain-names`; a record naming anything else is ignored by that CA. +pub const LETS_ENCRYPT_ISSUER_DOMAIN_NAME: &str = "letsencrypt.org"; + +/// The Issuer Domain Name to use, given what an operator configured. +/// +/// Empty means the default, which is what an untouched configuration carries. +/// Anything else is checked before it is believed: the name is interpolated +/// straight into an RFC 8659 `issue-value`, both in a `_validation-persist` +/// record and in the CAA records certbot publishes, and `set_caa_records` +/// installs the new value *after* deleting the records it replaces. A value +/// carrying a `;`, a space, or a `#` does not merely fail to authorize -- it +/// leaves the zone holding a malformed `issue` property with no valid record +/// behind it, which is CAA that forbids every issuer. Rejecting it at the point +/// it is configured keeps that out of the zone entirely. +pub fn resolve_issuer_domain_name(configured: &str) -> Result { + let name = configured.trim(); + if name.is_empty() { + return Ok(LETS_ENCRYPT_ISSUER_DOMAIN_NAME.to_string()); + } + let name = name.trim_end_matches('.'); + if name.is_empty() || name.len() > 253 { + bail!("issuer domain name must be between 1 and 253 characters: {configured:?}"); + } + for label in name.split('.') { + // RFC 8659 spells a label `(ALPHA / DIGIT) *( *("-") (ALPHA / DIGIT) )`: + // alphanumeric at both ends, hyphens only between them, and nothing + // else -- no underscore, which is a DNS convention for names that are + // never issuer names. A CA holding the record to that grammar reads + // anything looser as a malformed `issue` property, which is the state + // this check exists to keep out of a zone. + // Each rejection names what it rejected: the operator has to fix the + // value, and "malformed" alone does not say which character to remove -- + // an underscore, say, which is legal in plenty of DNS names and is the + // one most likely to be reached for here. + let bytes = label.as_bytes(); + let context = "it is written verbatim into CAA and validation records, \ + where a malformed label makes the whole record unparseable"; + if bytes.is_empty() { + bail!("issuer domain name {configured:?} has an empty label: {context}"); + } + if bytes.len() > 63 { + bail!("issuer domain name {configured:?} has a label over 63 octets: {context}"); + } + // Scanned as characters, not bytes: a byte cast to `char` renders a + // multi-byte character's first byte as some unrelated Latin-1 one, so + // `lé.org` would be rejected for a `Ã` that appears nowhere in the + // value the same message quotes back. + if let Some(ch) = label + .chars() + .find(|ch| !ch.is_ascii_alphanumeric() && *ch != '-') + { + bail!( + "issuer domain name {configured:?} contains {ch:?}, which a DNS label cannot: \ + {context}" + ); + } + if !bytes[0].is_ascii_alphanumeric() || !bytes[bytes.len() - 1].is_ascii_alphanumeric() { + bail!( + "issuer domain name {configured:?} has a label starting or ending with a \ + hyphen: {context}" + ); + } + } + Ok(name.to_string()) +} + +/// The `policy` value that widens a record to cover wildcards. +const POLICY_WILDCARD: &str = "wildcard"; + +/// The validation domain name for `name`, where the CA looks for the TXT record. +/// +/// A wildcard request is authorized by the record on its base name: ACME strips the +/// `*.` before creating the authorization, so `*.example.com` and `example.com` +/// share `_validation-persist.example.com` and are told apart by `policy=wildcard`. +pub fn validation_domain(name: &str) -> String { + let base = name.strip_prefix("*.").unwrap_or(name); + format!("{VALIDATION_LABEL}.{base}") +} + +/// The record an operator has to publish for one name. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct AuthorizationRecord { + /// Issuer Domain Name of the CA this record authorizes. + pub issuer_domain_name: String, + /// URI of the ACME account allowed to issue, compared byte for byte by the CA. + pub account_uri: String, + /// Whether the record also covers `*.`. + pub wildcard: bool, +} + +impl AuthorizationRecord { + /// The TXT RDATA to publish, as a single string. + /// + /// Rendered without a trailing semicolon: CAs read the RDATA as an RFC 8659 + /// `issue-value`, where a trailing semicolon is an empty parameter and makes + /// the whole record malformed. + pub fn rdata(&self) -> String { + let mut rdata = format!( + "{}; accounturi={}", + self.issuer_domain_name, self.account_uri + ); + if self.wildcard { + rdata.push_str("; policy=wildcard"); + } + rdata + } + + /// Whether `rdata` currently published at the validation domain satisfies this + /// record's requirement, as of `now` (UNIX seconds). + /// + /// Follows the CA's own filter: a record naming a different issuer is not a + /// failure, it belongs to another CA and is skipped. Only a record that names + /// our issuer is held to the account, policy and lifetime checks. + fn satisfied_by(&self, rdata: &str, now: u64) -> bool { + let Ok(parsed) = IssueValue::parse(rdata) else { + return false; + }; + if normalized_issuer(&parsed.issuer_domain_name) + != normalized_issuer(&self.issuer_domain_name) + { + return false; + } + if parsed.account_uri != self.account_uri { + return false; + } + if parsed.persist_until.is_some_and(|until| now > until) { + return false; + } + // A record without `policy=wildcard` authorizes the exact name only, so it + // cannot stand in for a wildcard request. The reverse is fine: a wildcard + // record also covers the name itself. + !self.wildcard || parsed.policy.as_deref().is_some_and(is_wildcard_policy) + } + + /// Whether any of the TXT records at the validation domain satisfies this one. + /// + /// Several records may sit at the same label, one per CA or per account, and + /// the CA accepts the name if any single record passes. + pub fn satisfied_by_any(&self, published: &[String], now: u64) -> bool { + published.iter().any(|rdata| self.satisfied_by(rdata, now)) + } +} + +impl fmt::Display for AuthorizationRecord { + /// Renders the full zone-file line, ready to paste into a DNS provider. + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "IN TXT \"{}\"", self.rdata()) + } +} + +/// A parsed RFC 8659 `issue-value`, the grammar shared with CAA `issue` records. +#[derive(Debug, PartialEq, Eq)] +struct IssueValue { + issuer_domain_name: String, + account_uri: String, + policy: Option, + /// UNIX timestamp after which the CA stops accepting the record. + persist_until: Option, +} + +impl IssueValue { + fn parse(rdata: &str) -> Result { + let mut parts = rdata.split(';'); + let issuer_domain_name = trim_wsp(parts.next().unwrap_or_default()); + if issuer_domain_name.is_empty() { + bail!("missing issuer domain name"); + } + + let mut account_uri = None; + let mut policy = None; + let mut persist_until = None; + let mut seen = Vec::new(); + for part in parts { + let part = trim_wsp(part); + // An empty parameter means a doubled or trailing semicolon. CAs treat + // that as malformed rather than skipping it, so neither do we. + if part.is_empty() { + bail!("empty parameter or trailing semicolon"); + } + let (tag, value) = part.split_once('=').context("parameter is not tag=value")?; + // RFC 8659's grammar is `parameter = tag *WSP "=" *WSP value`, so the + // separator may be padded on either side and each half is trimmed on + // its own. Trimming only the whole parameter would read the tag of + // `accounturi = ` as `"accounturi "` and report the mandatory + // parameter missing on a record the CA accepts. + let tag = trim_wsp(tag).to_lowercase(); + let value = trim_wsp(value); + if tag.is_empty() { + bail!("parameter has an empty tag"); + } + if !value.bytes().all(is_value_byte) { + bail!("parameter {tag} has a value with a forbidden character"); + } + match tag.as_str() { + "accounturi" | "policy" | "persistuntil" => { + // Only recognized tags are held to uniqueness: the draft has + // the CA ignore unknown tags outright, so a record repeating + // one is still a record the CA issues from. + if seen.contains(&tag) { + bail!("duplicate parameter {tag}"); + } + seen.push(tag.clone()); + } + // The draft requires unrecognized tags to be ignored, so that + // later revisions can add parameters without invalidating records. + _ => continue, + } + match tag.as_str() { + "accounturi" => { + if value.is_empty() { + bail!("empty value for the mandatory accounturi parameter"); + } + account_uri = Some(value.to_string()); + } + "policy" => policy = Some(value.to_string()), + "persistuntil" => { + persist_until = Some( + value + .parse::() + .context("persistUntil is not a base-10 timestamp")?, + ) + } + _ => {} + } + } + + Ok(Self { + issuer_domain_name: issuer_domain_name.to_string(), + account_uri: account_uri.context("missing mandatory accounturi parameter")?, + policy, + persist_until, + }) + } +} + +/// An Issuer Domain Name folded the way the CA folds it before comparing. +/// +/// Boulder normalizes both sides (lowercase, then drop the root dot) before +/// deciding whether a record is one of its own, so `LetsEncrypt.ORG.` names the +/// same CA as `letsencrypt.org`. Comparing raw bytes here would treat a record +/// the CA honours as belonging to someone else and warn about a missing record +/// through every issuance. IDNA folding is left out: this compares against a +/// name dstack configures, and an operator writing a non-ASCII issuer name has +/// a mismatch a self-check cannot paper over. +fn normalized_issuer(name: &str) -> String { + name.trim_end_matches('.').to_lowercase() +} + +fn is_wildcard_policy(policy: &str) -> bool { + policy.eq_ignore_ascii_case(POLICY_WILDCARD) +} + +/// Trim the whitespace RFC 8659 allows around the issuer name and each parameter. +fn trim_wsp(part: &str) -> &str { + part.trim_matches([' ', '\t']) +} + +/// Whether a byte may appear in a parameter value. +/// +/// RFC 8659 allows printable ASCII except `;`, which excludes whitespace: a value +/// containing a space is a malformed record, not a value with a space in it. +fn is_value_byte(byte: u8) -> bool { + matches!(byte, 0x21..=0x3a | 0x3c..=0x7e) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// An arbitrary "now" that precedes every `persistUntil` used below. + const NOW: u64 = 1_700_000_000; + const ACCOUNT: &str = "https://acme-v02.api.letsencrypt.org/acme/acct/1234567890"; + + fn record(wildcard: bool) -> AuthorizationRecord { + AuthorizationRecord { + issuer_domain_name: LETS_ENCRYPT_ISSUER_DOMAIN_NAME.to_string(), + account_uri: ACCOUNT.to_string(), + wildcard, + } + } + + /// Whether `published` satisfies a request for `name`, wildcard or not. + fn accepts(wildcard: bool, published: &str) -> bool { + record(wildcard).satisfied_by(published, NOW) + } + + /// RFC 8659 spells the parameter grammar `tag *WSP "=" *WSP value`, and + /// Boulder trims each half separately. A record padded around the separator + /// is one the CA issues from, so the self-check has to read it the same way. + #[test] + fn whitespace_around_the_separator_is_not_part_of_the_tag_or_value() { + assert!(accepts( + false, + &format!("letsencrypt.org; accounturi = {ACCOUNT}") + )); + assert!(accepts( + true, + &format!("letsencrypt.org;\taccounturi\t=\t{ACCOUNT}; policy\t=\twildcard") + )); + } + + /// The draft has the CA ignore unrecognized tags outright, so repeating one + /// cannot invalidate a record. Uniqueness is only enforced where the CA + /// enforces it. + #[test] + fn a_repeated_unknown_tag_is_ignored_rather_than_fatal() { + assert!(accepts( + false, + &format!("letsencrypt.org; accounturi={ACCOUNT}; futuretag=a; futuretag=b") + )); + assert!(!accepts( + false, + &format!("letsencrypt.org; accounturi={ACCOUNT}; accounturi={ACCOUNT}") + )); + } + + /// Boulder folds case and drops the root dot before deciding whether a + /// record names it, so the same record must not read as another CA's here. + #[test] + fn the_issuer_name_is_compared_the_way_the_ca_compares_it() { + assert!(accepts( + false, + &format!("LetsEncrypt.ORG.; accounturi={ACCOUNT}") + )); + assert!(!accepts( + false, + &format!("other-ca.example; accounturi={ACCOUNT}") + )); + } + + /// `accounturi` is mandatory, and an empty value does not supply it. + #[test] + fn an_empty_account_uri_is_not_an_account_uri() { + assert!(!accepts(false, "letsencrypt.org; accounturi=")); + } + + /// A label longer than DNS allows is refused, and one right at the limit is + /// not: the bound is the protocol's, not a guess. + #[test] + fn a_label_is_held_to_its_dns_length() { + let sixty_three = "a".repeat(63); + assert!(resolve_issuer_domain_name(&format!("{sixty_three}.org")).is_ok()); + assert!(resolve_issuer_domain_name(&format!("{sixty_three}a.org")).is_err()); + } + + /// A rejection has to say what to change: the character it objected to, not + /// just that the value was wrong. + #[test] + fn a_rejection_names_the_character_it_rejected() { + let err = resolve_issuer_domain_name("my_ca.example.com") + .expect_err("an underscore is not a DNS label character"); + assert!(err.to_string().contains('_'), "{err:#}"); + + // A multi-byte character has to name itself. Scanning bytes and casting + // to `char` would report the first byte of `é` as `Ã`, which does not + // occur in the value the same message quotes back. + let err = resolve_issuer_domain_name("lé.org").expect_err("é is not a DNS label character"); + assert!(err.to_string().contains('é'), "{err:#}"); + assert!(!err.to_string().contains('Ã'), "{err:#}"); + } + + /// Empty is what an untouched configuration carries, and it means the + /// default rather than a name of zero length -- which as an `issue-value` + /// would be CAA denying every issuer. + #[test] + fn an_unset_issuer_name_is_the_default_one() { + assert_eq!( + resolve_issuer_domain_name("").unwrap(), + LETS_ENCRYPT_ISSUER_DOMAIN_NAME + ); + assert_eq!( + resolve_issuer_domain_name(" ").unwrap(), + LETS_ENCRYPT_ISSUER_DOMAIN_NAME + ); + assert_eq!( + resolve_issuer_domain_name("pebble.letsencrypt.org.").unwrap(), + "pebble.letsencrypt.org" + ); + } + + /// The name is interpolated into records that a `;` or a space makes + /// malformed, and CAA is rewritten by deleting the old records first, so a + /// typo accepted here is a zone that forbids all issuance. + #[test] + fn a_name_that_would_corrupt_a_record_is_refused() { + for bad in [ + "lets encrypt.org", + "letsencrypt.org;policy=wildcard", + "#letsencrypt.org", + "letsencrypt..org", + "\"letsencrypt.org\"", + // RFC 8659's label grammar bounds both ends of a label, so these + // are as unparseable to a strict CA as a space is. + "_letsencrypt.org", + "-letsencrypt.org", + "letsencrypt-.org", + "lé.org", + ] { + assert!( + resolve_issuer_domain_name(bad).is_err(), + "{bad:?} should be refused" + ); + } + } + + #[test] + fn validation_domain_prepends_the_label() { + assert_eq!( + validation_domain("example.com"), + "_validation-persist.example.com" + ); + } + + #[test] + fn validation_domain_strips_the_wildcard_prefix() { + // The CA looks up the base name for a wildcard authorization, so a record + // at `_validation-persist.*.example.com` would never be read. + assert_eq!( + validation_domain("*.example.com"), + "_validation-persist.example.com" + ); + } + + #[test] + fn rdata_has_no_trailing_semicolon() { + assert_eq!( + record(false).rdata(), + format!("letsencrypt.org; accounturi={ACCOUNT}") + ); + } + + #[test] + fn rdata_carries_the_wildcard_policy() { + assert_eq!( + record(true).rdata(), + format!("letsencrypt.org; accounturi={ACCOUNT}; policy=wildcard") + ); + } + + #[test] + fn display_renders_a_zone_file_line() { + assert_eq!( + record(false).to_string(), + format!("IN TXT \"letsencrypt.org; accounturi={ACCOUNT}\"") + ); + } + + #[test] + fn rendered_record_parses_back() { + for wildcard in [false, true] { + assert!( + accepts(wildcard, &record(wildcard).rdata()), + "wildcard={wildcard}" + ); + } + } + + #[test] + fn accepts_the_draft_example_layout() { + // draft-ietf-acme-dns-persist-01, figure 2, plus the whitespace RFC 8659 + // allows around the issuer name and each parameter. + let record = AuthorizationRecord { + issuer_domain_name: "authority.example".to_string(), + account_uri: "https://ca.example/acct/123".to_string(), + wildcard: false, + }; + for published in [ + "authority.example; accounturi=https://ca.example/acct/123", + "authority.example;accounturi=https://ca.example/acct/123", + "\tauthority.example ;\taccounturi=https://ca.example/acct/123 ", + ] { + assert!(record.satisfied_by(published, NOW), "{published:?}"); + } + } + + #[test] + fn matches_tags_case_insensitively() { + assert!(accepts( + true, + &format!("letsencrypt.org; AccountURI={ACCOUNT}; Policy=WILDCARD") + )); + } + + #[test] + fn rejects_a_different_account_uri() { + assert!(!accepts( + false, + "letsencrypt.org; accounturi=https://acme-v02.api.letsencrypt.org/acme/acct/9" + )); + } + + #[test] + fn compares_the_account_uri_without_case_folding() { + // The draft pins Simple String Comparison, so an upper-cased URI is a + // different account as far as the CA is concerned. + assert!(!accepts( + false, + &format!("letsencrypt.org; accounturi={}", ACCOUNT.to_uppercase()) + )); + } + + #[test] + fn ignores_a_record_naming_another_issuer() { + assert!(!accepts( + false, + &format!("otherca.example; accounturi={ACCOUNT}") + )); + } + + #[test] + fn rejects_a_plain_record_for_a_wildcard_request() { + let published = record(false).rdata(); + assert!(accepts(false, &published)); + assert!(!accepts(true, &published)); + } + + #[test] + fn accepts_a_wildcard_record_for_a_plain_request() { + assert!(accepts(false, &record(true).rdata())); + } + + #[test] + fn rejects_a_trailing_semicolon() { + // A CA reads the empty tail as an empty parameter and fails the whole + // record, so a record rendered with one would be silently unusable. + assert!(!accepts( + false, + &format!("letsencrypt.org; accounturi={ACCOUNT};") + )); + } + + #[test] + fn rejects_a_duplicate_parameter() { + assert!(!accepts( + false, + &format!("letsencrypt.org; accounturi={ACCOUNT}; accounturi={ACCOUNT}") + )); + } + + #[test] + fn rejects_whitespace_inside_a_value() { + assert!(!accepts( + false, + "letsencrypt.org; accounturi=https://ca.example/acct/1 2" + )); + } + + #[test] + fn rejects_a_record_without_accounturi() { + assert!(!accepts(false, "letsencrypt.org; policy=wildcard")); + } + + #[test] + fn ignores_unrecognized_parameters() { + assert!(accepts( + true, + &format!("letsencrypt.org; accounturi={ACCOUNT}; policy=wildcard; futuretag=whatever") + )); + } + + #[test] + fn rejects_an_expired_persist_until() { + let published = format!("letsencrypt.org; accounturi={ACCOUNT}; persistUntil={NOW}"); + assert!(record(false).satisfied_by(&published, NOW)); + assert!(!record(false).satisfied_by(&published, NOW + 1)); + } + + #[test] + fn rejects_a_malformed_persist_until() { + assert!(!accepts( + false, + &format!("letsencrypt.org; accounturi={ACCOUNT}; persistUntil=tomorrow") + )); + } + + #[test] + fn accepts_any_one_of_the_published_records() { + let published = vec![ + "otherca.example; accounturi=https://other.example/acct/1".to_string(), + record(true).rdata(), + ]; + assert!(record(true).satisfied_by_any(&published, NOW)); + assert!(!record(true).satisfied_by_any(&published[..1], NOW)); + } +} diff --git a/dstack/certbot/src/lib.rs b/dstack/certbot/src/lib.rs index df71b9935..e2a0314c9 100644 --- a/dstack/certbot/src/lib.rs +++ b/dstack/certbot/src/lib.rs @@ -11,18 +11,24 @@ //! //! - Automatic certificate issuance and renewal //! - DNS-01 challenge support (currently implemented for Cloudflare) +//! - DNS-PERSIST-01 challenge support, which needs no DNS provider credential //! - Easy integration with existing Rust applications //! //! For more detailed information on the available methods and their usage, please refer //! to the documentation of individual structs and functions. -pub use acme_client::AcmeClient; +pub use acme_client::{ + advisory_dns_wait, required_dns_records, AcmeAccount, AcmeClient, ChallengeKind, + RequiredRecord, ValidationMethod, +}; pub use bot::{read_pubkey, CertBot, CertBotConfig}; pub use dns01_client::Dns01Client; +pub use dns_persist::{resolve_issuer_domain_name, LETS_ENCRYPT_ISSUER_DOMAIN_NAME}; pub use workdir::WorkDir; mod acme_client; mod bot; mod dns01_client; +mod dns_persist; mod http_client; mod workdir; diff --git a/dstack/gateway/rpc/proto/gateway_rpc.proto b/dstack/gateway/rpc/proto/gateway_rpc.proto index 4935974e5..5a59732de 100644 --- a/dstack/gateway/rpc/proto/gateway_rpc.proto +++ b/dstack/gateway/rpc/proto/gateway_rpc.proto @@ -210,6 +210,15 @@ message RotateAcmeCredentialsResponse { string account_uri = 1; // Number of ZT domains whose CAA records were updated for the new account. uint32 domains_updated = 2; + // Zone-file lines an operator must publish by hand, for domains the gateway + // cannot write (dns-persist-01). Non-empty means issuance for those domains + // stays broken until they name the new account. + repeated string required_dns_records = 3; + // Domains whose CAA records could not be re-pinned to the new account. The + // rotation itself succeeded -- the account exists and the cluster is using + // it -- so this is reported rather than raised: repeating the rotation would + // register yet another account. Rerun SetCaa until it succeeds. + repeated string repin_failed_domains = 4; } // Get HostInfo for associated instance id. @@ -742,6 +751,21 @@ message ZtDomainConfig { optional uint32 node = 4; // Priority for default base_domain selection (higher = preferred) int32 priority = 5; + // ACME challenge proving control of this domain: "dns-01" (default) or + // "dns-persist-01". Empty means "dns-01". + // + // "dns-persist-01" needs no DNS credential: control comes from a + // _validation-persist TXT record the operator publishes once, so the gateway + // CVM never holds write access to the zone. Experimental — the draft is still + // changing and Let's Encrypt serves it on staging only. + // + // Optional so that omitting it means "leave it alone" rather than "dns-01". + // UpdateZtDomain replaces the whole record, so a caller that predates this + // field -- a cached dashboard bundle, a script, an older SDK -- would + // otherwise downgrade a dns-persist-01 domain to dns-01 cluster-wide on any + // unrelated edit, and its hand-published CAA naming dns-persist-01 then + // refuses every order. + optional string challenge = 6; } // ZT-Domain information (config + certificate status) @@ -750,6 +774,10 @@ message ZtDomainInfo { ZtDomainConfig config = 1; // Certificate status ZtDomainCertStatus cert_status = 2; + // Zone-file lines the domain's DNS must contain. Under "dns-01" the gateway + // writes these itself and they are informational; under "dns-persist-01" they + // are the one-time setup an operator has to publish. + repeated string required_dns_records = 3; } // ZT-Domain certificate status @@ -844,6 +872,9 @@ message CertbotConfigResponse { uint64 renew_timeout_secs = 3; // ACME server URL (empty means default Let's Encrypt production) string acme_url = 4; + // Issuer Domain Name naming the CA in dns-persist-01 and CAA records + // (empty means letsencrypt.org) + string issuer_domain_name = 5; } // Set certbot configuration request @@ -856,6 +887,10 @@ message SetCertbotConfigRequest { optional uint64 renew_timeout_secs = 3; // ACME server URL (empty means use default Let's Encrypt production) optional string acme_url = 4; + // Issuer Domain Name naming the CA in dns-persist-01 and CAA records + // (empty means letsencrypt.org). Must be one of the issuer-domain-names the + // ACME server at acme_url advertises, or every dns-persist-01 order fails. + optional string issuer_domain_name = 5; } // ==================== Tombstone GC Configuration Messages ==================== diff --git a/dstack/gateway/src/admin_service.rs b/dstack/gateway/src/admin_service.rs index b9f5e6955..d840642e3 100644 --- a/dstack/gateway/src/admin_service.rs +++ b/dstack/gateway/src/admin_service.rs @@ -6,6 +6,7 @@ use std::sync::atomic::Ordering; use std::time::{Duration, SystemTime, UNIX_EPOCH}; use anyhow::{bail, ensure, Context, Result}; +use certbot::ChallengeKind; use dstack_gateway_rpc::{ admin_server::{AdminRpc, AdminServer}, CertAttestationInfo, CertbotConfigResponse, ClearInstancePortPolicyRequest, @@ -110,10 +111,12 @@ impl AdminRpc for AdminRpcHandler { } async fn rotate_acme_credentials(self) -> Result { - let (account_uri, domains_updated) = self.state.rotate_acme_credentials().await?; + let outcome = self.state.rotate_acme_credentials().await?; Ok(RotateAcmeCredentialsResponse { - account_uri, - domains_updated: domains_updated.try_into().unwrap_or(u32::MAX), + account_uri: outcome.account_uri, + domains_updated: outcome.domains_updated.try_into().unwrap_or(u32::MAX), + required_dns_records: outcome.required_dns_records, + repin_failed_domains: outcome.repin_failed, }) } @@ -554,11 +557,15 @@ impl AdminRpc for AdminRpcHandler { async fn list_zt_domains(self) -> Result { let kv_store = self.state.kv_store(); let cert_resolver = &self.state.cert_resolver; + let certbot = &self.state.certbot; let domains = kv_store .list_zt_domain_configs() .into_iter() - .map(|config| zt_domain_to_proto(config, kv_store, cert_resolver)) + .map(|config| { + let records = certbot.required_dns_records(&config); + zt_domain_to_proto(config, kv_store, cert_resolver, records) + }) .collect(); Ok(ListZtDomainsResponse { domains }) @@ -573,14 +580,16 @@ impl AdminRpc for AdminRpcHandler { .get_zt_domain_config(&domain) .context("ZT-Domain config not found")?; - Ok(zt_domain_to_proto(config, kv_store, cert_resolver)) + let records = self.state.certbot.required_dns_records(&config); + Ok(zt_domain_to_proto(config, kv_store, cert_resolver, records)) } async fn add_zt_domain(self, request: ProtoZtDomainConfig) -> Result { let kv_store = self.state.kv_store(); let cert_resolver = &self.state.cert_resolver; - let config = proto_to_zt_domain_config(&request, kv_store)?; + // Nothing stored to preserve: an omitted challenge is the default. + let config = proto_to_zt_domain_config(&request, kv_store, None)?; // Uniqueness is checked after normalization so wildcard, case, and a // trailing root dot cannot silently overwrite the same DNS name. @@ -591,24 +600,29 @@ impl AdminRpc for AdminRpcHandler { kv_store.save_zt_domain_config(&config)?; info!("Added ZT-Domain config: {}", config.domain); - Ok(zt_domain_to_proto(config, kv_store, cert_resolver)) + let records = self.state.certbot.required_dns_records(&config); + Ok(zt_domain_to_proto(config, kv_store, cert_resolver, records)) } async fn update_zt_domain(self, request: ProtoZtDomainConfig) -> Result { let kv_store = self.state.kv_store(); let cert_resolver = &self.state.cert_resolver; - let config = proto_to_zt_domain_config(&request, kv_store)?; - - // Check the normalized key rather than the caller's presentation. - kv_store - .get_zt_domain_config(&config.domain) + // Read the stored record first: an omitted `challenge` means "leave it + // alone", and this is the only place its current value is known. Looked + // up on the normalized key rather than the caller's presentation. + let domain = normalize_zt_domain(&request.domain)?; + let stored = kv_store + .get_zt_domain_config(&domain) .context("ZT-Domain config not found")?; + let config = proto_to_zt_domain_config(&request, kv_store, Some(stored.challenge))?; + kv_store.save_zt_domain_config(&config)?; info!("Updated ZT-Domain config: {}", config.domain); - Ok(zt_domain_to_proto(config, kv_store, cert_resolver)) + let records = self.state.certbot.required_dns_records(&config); + Ok(zt_domain_to_proto(config, kv_store, cert_resolver, records)) } async fn delete_zt_domain(self, request: DeleteZtDomainRequest) -> Result<()> { @@ -708,6 +722,7 @@ impl AdminRpc for AdminRpcHandler { renew_before_expiration_secs: config.renew_before_expiration.as_secs(), renew_timeout_secs: config.renew_timeout.as_secs(), acme_url: config.acme_url, + issuer_domain_name: config.issuer_domain_name, }) } @@ -716,11 +731,12 @@ impl AdminRpc for AdminRpcHandler { let config = merge_certbot_config(kv_store.get_certbot_config(), request)?; kv_store.set_certbot_config(&config)?; info!( - "Updated certbot config: renew_interval={:?}, renew_before_expiration={:?}, renew_timeout={:?}, acme_url={:?}", + "Updated certbot config: renew_interval={:?}, renew_before_expiration={:?}, renew_timeout={:?}, acme_url={:?}, issuer_domain_name={:?}", config.renew_interval, config.renew_before_expiration, config.renew_timeout, - config.acme_url + config.acme_url, + config.issuer_domain_name ); Ok(()) } @@ -953,9 +969,14 @@ fn validate_zt_domain(domain: &str) -> Result<()> { } /// Convert proto ZtDomainConfig to internal ZtDomainConfig +/// +/// `current` is the challenge already stored for this domain, on an update. +/// An omitted `challenge` resolves to it rather than to the default, so a caller +/// that predates the field leaves it alone instead of downgrading the domain. fn proto_to_zt_domain_config( proto: &ProtoZtDomainConfig, kv_store: &crate::kv::KvStore, + current: Option, ) -> Result { // Normalize dns_cred_id: treat empty string as None (use default) let dns_cred_id = proto @@ -976,20 +997,42 @@ fn proto_to_zt_domain_config( bail!("port must be between 1 and 65535"); } + let challenge = match proto.challenge.as_deref() { + // Absent means "leave it as it is". UpdateZtDomain replaces the whole + // record, so reading absence as the default would let any edit from a + // caller that does not know the field -- a cached dashboard bundle, a + // script, an older SDK -- downgrade a dns-persist-01 domain to dns-01 + // cluster-wide, after which its hand-published CAA naming + // dns-persist-01 refuses every order. On an add there is nothing to + // preserve, so it falls back to the historical default. + None => current.unwrap_or_default(), + // Present but empty is still the default: that is what an explicit + // proto3 zero value carries, and every ZT domain predates the choice. + Some("" | "dns-01") => ChallengeKind::Dns01, + Some("dns-persist-01") => ChallengeKind::DnsPersist01, + Some(other) => bail!("unsupported challenge {other:?}, expected dns-01 or dns-persist-01"), + }; + Ok(ZtDomainConfig { domain, dns_cred_id, port: proto.port.try_into().context("port out of range")?, node: proto.node, priority: proto.priority, + challenge, }) } /// Convert internal ZtDomainConfig to proto ZtDomainInfo (with cert status) +/// +/// `required_dns_records` is best effort: rendering it needs the ACME account +/// URI, and a domain whose ACME client cannot be built yet still has to be +/// listable. It comes back empty in that case rather than failing the call. fn zt_domain_to_proto( config: ZtDomainConfig, kv_store: &crate::kv::KvStore, cert_resolver: &crate::cert_store::CertResolver, + required_dns_records: Vec, ) -> ZtDomainInfo { // Get certificate data for status let cert_data = kv_store.get_cert_data(&config.domain); @@ -1003,6 +1046,11 @@ fn zt_domain_to_proto( loaded_in_memory, }); + let challenge = match config.challenge { + ChallengeKind::Dns01 => "dns-01", + ChallengeKind::DnsPersist01 => "dns-persist-01", + }; + ZtDomainInfo { config: Some(ProtoZtDomainConfig { domain: config.domain, @@ -1010,8 +1058,10 @@ fn zt_domain_to_proto( port: config.port.into(), node: config.node, priority: config.priority, + challenge: Some(challenge.to_string()), }), cert_status, + required_dns_records, } } @@ -1042,10 +1092,12 @@ fn merge_certbot_config( request.renew_interval_secs.is_some() && request.renew_before_expiration_secs.is_some() && request.renew_timeout_secs.is_some() - && request.acme_url.is_some(), + && request.acme_url.is_some() + && request.issuer_domain_name.is_some(), "the stored certbot config is unreadable ({err:#}), so it can only be \ replaced as a whole: resend with renew_interval_secs, \ - renew_before_expiration_secs, renew_timeout_secs and acme_url all set" + renew_before_expiration_secs, renew_timeout_secs, acme_url and \ + issuer_domain_name all set" ); warn!("certbot config is unreadable ({err:#}); replacing it wholesale"); GlobalCertbotConfig::default() @@ -1060,11 +1112,23 @@ fn merge_certbot_config( config.renew_before_expiration = Duration::from_secs(secs); } if let Some(secs) = request.renew_timeout_secs { + // Zero would abort every order before it starts, and the DNS wait is + // derived from this value. + ensure!(secs > 0, "renew_timeout_secs must be greater than zero"); config.renew_timeout = Duration::from_secs(secs); } if let Some(url) = request.acme_url { config.acme_url = url; } + if let Some(name) = request.issuer_domain_name { + // Checked here rather than where it is used: it is written verbatim into + // CAA records, and those are rewritten by deleting the old ones first, + // so a value that cannot be a DNS name takes the zone's issuance + // permission down with it. Stored as configured -- empty keeps meaning + // the default -- but only once it is known to be usable. + certbot::resolve_issuer_domain_name(&name).context("invalid issuer_domain_name")?; + config.issuer_domain_name = name; + } Ok(config) } @@ -1119,11 +1183,180 @@ mod certbot_config_tests { renew_before_expiration_secs: Some(86400), renew_timeout_secs: Some(30), acme_url: Some("https://acme-staging.example/directory".to_string()), + issuer_domain_name: Some("pebble.letsencrypt.org".to_string()), }, ) .expect("a complete request replaces the record"); assert_eq!(merged.renew_interval, Duration::from_secs(60)); assert_eq!(merged.acme_url, "https://acme-staging.example/directory"); + assert_eq!(merged.issuer_domain_name, "pebble.letsencrypt.org"); + } + + /// Repairing from the defaults would reset the issuer name to empty -- + /// Let's Encrypt -- for a deployment pointed at another CA, which then + /// publishes CAA and validation records naming a CA its orders never reach. + /// That is the same hazard `acme_url` is in the required set for. + #[test] + fn repairing_an_unreadable_record_cannot_forget_the_issuer_name() { + let err = merge_certbot_config( + Err(anyhow::anyhow!("corrupt record")), + SetCertbotConfigRequest { + renew_interval_secs: Some(60), + renew_before_expiration_secs: Some(86400), + renew_timeout_secs: Some(30), + acme_url: Some("https://pebble.example/dir".to_string()), + issuer_domain_name: None, + }, + ) + .expect_err("a repair that omits the issuer name must be refused"); + assert!(err.to_string().contains("issuer_domain_name"), "{err:#}"); + } + + /// The name is written verbatim into CAA records, and writing them deletes + /// the records they replace, so a value that cannot be a DNS name is + /// refused where it is set rather than where it would corrupt a zone. + #[test] + fn a_malformed_issuer_name_is_refused() { + let err = merge_certbot_config( + Ok(stored()), + SetCertbotConfigRequest { + issuer_domain_name: Some("lets encrypt.org".to_string()), + ..Default::default() + }, + ) + .expect_err("a name with a space must be refused"); + assert!(err.to_string().contains("issuer_domain_name"), "{err:#}"); + } + + /// Zero would abort every order before it began, and the DNS wait is + /// derived from this value. + #[test] + fn a_zero_renew_timeout_is_refused() { + let err = merge_certbot_config( + Ok(stored()), + SetCertbotConfigRequest { + renew_timeout_secs: Some(0), + ..Default::default() + }, + ) + .expect_err("a zero renew timeout must be refused"); + assert!(err.to_string().contains("renew_timeout_secs"), "{err:#}"); + } + + /// The name the CA is known by has to be settable, or a deployment pointed + /// at a non-Let's-Encrypt ACME server publishes records naming the wrong CA + /// with no way to correct them. + #[test] + fn the_issuer_domain_name_round_trips() { + let merged = merge_certbot_config( + Ok(stored()), + SetCertbotConfigRequest { + issuer_domain_name: Some("pebble.letsencrypt.org".to_string()), + ..Default::default() + }, + ) + .expect("a partial update keeps the rest"); + assert_eq!(merged.issuer_domain_name, "pebble.letsencrypt.org"); + assert_eq!(merged.acme_url, stored().acme_url); + } +} + +/// An omitted `challenge` has to mean "leave it as it is", or every caller that +/// predates the field silently downgrades a dns-persist-01 domain. +#[cfg(test)] +mod zt_domain_challenge_tests { + use super::*; + + fn kv_store(dir: &std::path::Path) -> crate::kv::KvStore { + crate::kv::KvStore::new(1, vec![], dir, None).expect("failed to create kv store") + } + + fn request(challenge: Option<&str>) -> ProtoZtDomainConfig { + ProtoZtDomainConfig { + domain: "example.com".to_string(), + dns_cred_id: None, + port: 443, + node: None, + priority: 7, + challenge: challenge.map(ToString::to_string), + } + } + + /// The case the field is `optional` for: a cached dashboard bundle, a curl + /// script, or an older SDK edits an unrelated field and must not take the + /// domain's challenge down with it. + #[test] + fn an_omitted_challenge_keeps_the_stored_one() { + let dir = tempfile::tempdir().expect("failed to create temp dir"); + let config = proto_to_zt_domain_config( + &request(None), + &kv_store(dir.path()), + Some(ChallengeKind::DnsPersist01), + ) + .expect("an update without a challenge is valid"); + assert_eq!(config.challenge, ChallengeKind::DnsPersist01); + assert_eq!(config.priority, 7); + } + + /// On an add there is no stored value to preserve, so absence is the + /// historical default rather than an error. + #[test] + fn an_omitted_challenge_on_an_add_is_dns01() { + let dir = tempfile::tempdir().expect("failed to create temp dir"); + let config = proto_to_zt_domain_config(&request(None), &kv_store(dir.path()), None) + .expect("an add without a challenge is valid"); + assert_eq!(config.challenge, ChallengeKind::Dns01); + } + + /// Preserving an omitted value must not make the field unsettable: a caller + /// that names dns-01 is asking to switch back, and gets it. + #[test] + fn an_explicit_challenge_overrides_the_stored_one() { + let dir = tempfile::tempdir().expect("failed to create temp dir"); + let store = kv_store(dir.path()); + let config = proto_to_zt_domain_config( + &request(Some("dns-01")), + &store, + Some(ChallengeKind::DnsPersist01), + ) + .expect("an explicit challenge is valid"); + assert_eq!(config.challenge, ChallengeKind::Dns01); + + let config = proto_to_zt_domain_config( + &request(Some("dns-persist-01")), + &store, + Some(ChallengeKind::Dns01), + ) + .expect("an explicit challenge is valid"); + assert_eq!(config.challenge, ChallengeKind::DnsPersist01); + } + + /// An explicit empty string is what a proto3 zero value carries, and every + /// ZT domain predates the choice, so it still reads as the default. + #[test] + fn an_explicitly_empty_challenge_is_the_default() { + let dir = tempfile::tempdir().expect("failed to create temp dir"); + let config = proto_to_zt_domain_config( + &request(Some("")), + &kv_store(dir.path()), + Some(ChallengeKind::DnsPersist01), + ) + .expect("an empty challenge is valid"); + assert_eq!(config.challenge, ChallengeKind::Dns01); + } + + /// An unrecognized value is refused rather than silently defaulted: a typo + /// that read as dns-01 would be the downgrade this field exists to prevent. + #[test] + fn an_unknown_challenge_is_refused() { + let dir = tempfile::tempdir().expect("failed to create temp dir"); + let err = proto_to_zt_domain_config( + &request(Some("dns-persist-02")), + &kv_store(dir.path()), + None, + ) + .expect_err("an unknown challenge must be refused"); + assert!(err.to_string().contains("dns-persist-02"), "{err:#}"); } } diff --git a/dstack/gateway/src/distributed_certbot.rs b/dstack/gateway/src/distributed_certbot.rs index 288b3403e..05a59656a 100644 --- a/dstack/gateway/src/distributed_certbot.rs +++ b/dstack/gateway/src/distributed_certbot.rs @@ -11,7 +11,10 @@ use std::sync::Arc; use std::time::Duration; use anyhow::{bail, Context, Result}; -use certbot::{AcmeClient, Dns01Client}; +use certbot::{ + advisory_dns_wait, resolve_issuer_domain_name, AcmeAccount, AcmeClient, ChallengeKind, + Dns01Client, ValidationMethod, +}; use dstack_guest_agent_rpc::v0::RawQuoteArgs; use ra_tls::attestation::QuoteContentType; use ra_tls::rcgen::KeyPair; @@ -33,6 +36,73 @@ const ROTATION_LOCK_TIMEOUT_SECS: u64 = 600; /// Default ACME URL (Let's Encrypt production) const DEFAULT_ACME_URL: &str = "https://acme-v02.api.letsencrypt.org/directory"; +/// How long a `dns-persist-01` domain waits for its record to become visible. +/// +/// A `dns-01` domain reads this from its DNS credential; a `dns-persist-01` +/// domain has none, so it starts from the same 300s a credential defaults to +/// and is clamped by [`advisory_dns_wait`] like every other domain. +const DNS_PERSIST_MAX_DNS_WAIT: Duration = Duration::from_secs(300); + +/// What an ACME credential rotation left behind. +#[derive(Debug)] +pub struct RotationOutcome { + /// URI of the new ACME account. + pub account_uri: String, + /// Domains whose CAA could not be re-pinned to the new account. + pub repin_failed: Vec, + /// Domains whose CAA records were re-pinned to the new account. + pub domains_updated: usize, + /// Records an operator has to publish by hand, for domains the gateway + /// cannot write. Non-empty means issuance for those domains is broken until + /// they are published. + pub required_dns_records: Vec, +} + +/// What the CAA re-pin pass over a rotation's domains produced. +/// +/// The three numbers a rotation reports are all derived from one tally, so they +/// cannot disagree: `total` counts every domain the rotation covered, `manual` +/// the ones a re-pin was never attempted for -- a `dns-persist-01` domain has +/// no CAA the gateway can write -- and `failed` the ones that were tried and +/// did not take. +#[derive(Debug, Default)] +struct RepinTally { + /// Domains skipped because the gateway cannot write their DNS. + manual: usize, + /// Domains a re-pin was attempted for and failed. + failed: Vec, +} + +impl RepinTally { + /// Domains a re-pin was actually attempted for. + /// + /// The denominator a failure count belongs over. Counting the manual ones + /// in reports them as successes: "2/5 failed" says three domains were + /// re-pinned when the other three were never candidates. + fn attempted(&self, total: usize) -> usize { + total.saturating_sub(self.manual) + } + + /// Domains now pinned to the new account. + fn updated(&self, total: usize) -> usize { + self.attempted(total).saturating_sub(self.failed.len()) + } +} + +/// How one ZT domain answers ACME challenges. +struct DomainValidation { + method: ValidationMethod, + /// How long to wait for the challenge records to become visible. + max_dns_wait: Duration, +} + +impl DomainValidation { + /// Whether the gateway can write this domain's DNS records itself. + fn writes_dns(&self) -> bool { + matches!(self.method, ValidationMethod::Dns01 { .. }) + } +} + /// Multi-domain certificate manager pub struct DistributedCertBot { kv_store: Arc, @@ -120,11 +190,102 @@ impl DistributedCertBot { } } + /// Build the ACME validation method for one ZT domain. + /// + /// `dns-01` resolves the zone through the provider API, so a missing or + /// broken credential surfaces here rather than at the first order. + /// `dns-persist-01` reads no credential at all: the gateway CVM holds no + /// DNS write access for that domain, which is the point of the method. + async fn validation_for( + &self, + domain: &str, + config: &ZtDomainConfig, + ) -> Result { + let renew_timeout = self.config()?.renew_timeout; + match config.challenge { + ChallengeKind::Dns01 => { + let dns_cred = dns_credential_for(&self.kv_store, config)?; + let client = self.dns_client(domain, &dns_cred).await?; + Ok(DomainValidation { + method: ValidationMethod::Dns01 { + client, + txt_ttl: dns_cred.dns_txt_ttl, + issuer_domain_name: self.issuer_domain_name()?, + }, + max_dns_wait: advisory_dns_wait(dns_cred.max_dns_wait, renew_timeout), + }) + } + ChallengeKind::DnsPersist01 => Ok(DomainValidation { + method: ValidationMethod::DnsPersist01 { + issuer_domain_name: self.issuer_domain_name()?, + }, + max_dns_wait: advisory_dns_wait(DNS_PERSIST_MAX_DNS_WAIT, renew_timeout), + }), + } + } + + /// Issuer Domain Name to name in `dns-persist-01` records and in the CAA + /// records written for either challenge. + fn issuer_domain_name(&self) -> Result { + resolve_issuer_domain_name(&self.config()?.issuer_domain_name) + .context("invalid issuer_domain_name in the certbot config") + } + + /// The DNS records a ZT domain's zone has to contain, as zone-file lines. + /// + /// Rendered from the stored account URI with no ACME round trip, so the + /// admin listing endpoints stay cheap. Empty until an ACME account exists — + /// every record names one — and empty rather than an error when the stored + /// credentials cannot be read, since listing a domain must not depend on + /// them. + pub fn required_dns_records(&self, config: &ZtDomainConfig) -> Vec { + let Ok(issuer_domain_name) = self.issuer_domain_name() else { + return Vec::new(); + }; + let Some(account_uri) = self + .kv_store + .get_acme_credentials() + .ok() + .flatten() + .and_then(|creds| extract_account_uri(&creds.acme_credentials)) + else { + return Vec::new(); + }; + // The gateway only ever orders `*.{domain}`, so that is the whole set of + // identifiers the CA will look records up for. + certbot::required_dns_records( + config.challenge, + &issuer_domain_name, + &account_uri, + &[format!("*.{}", config.domain)], + ) + .iter() + .map(ToString::to_string) + .collect() + } + + /// Log the records an operator has to publish for a domain, and return them. + fn report_manual_records(&self, config: &ZtDomainConfig) -> Vec { + let records = self.required_dns_records(config); + for record in &records { + warn!( + "cert[{}]: publish this record by hand: {record}", + config.domain + ); + } + records + } + /// Rotate the shared ACME account without interrupting certificate serving. /// - /// The sequence is: validate every domain's DNS credential, create the + /// The sequence is: validate every domain's DNS credential, register the /// replacement account, publish the new credentials, then re-pin every - /// domain's CAA record to the new account. Publishing before re-pinning + /// domain's CAA record to the new account. + /// + /// A cluster with no ZT domain registers an account and stops there, which + /// is how a `dns-persist-01` deployment bootstraps: the record an operator + /// publishes names the account, so there is nothing to publish -- and + /// therefore nothing that can be issued -- until one exists. Publishing before re-pinning /// makes the failure mode convergent: if some domains fail to re-pin, the /// cluster is already on the new account and rerunning `SetCaa` finishes /// the switch without registering yet another account (Let's Encrypt caps @@ -142,7 +303,7 @@ impl DistributedCertBot { /// /// Runs under the shared ACME lock, so a concurrent reconciliation or /// first-use registration on any node is refused for the duration. - pub async fn rotate_acme_credentials(&self) -> Result<(String, usize)> { + pub async fn rotate_acme_credentials(&self) -> Result { let rotation_lock = self.acquire_acme_lock("rotate ACME credentials")?; let result = self.do_rotate_acme_credentials().await; if let Err(err) = self.release_rotation_lock(&rotation_lock) { @@ -151,7 +312,7 @@ impl DistributedCertBot { result } - async fn do_rotate_acme_credentials(&self) -> Result<(String, usize)> { + async fn do_rotate_acme_credentials(&self) -> Result { let configs = self.kv_store.list_zt_domain_configs(); let certbot_config = self.config()?; let acme_url = if certbot_config.acme_url.is_empty() { @@ -163,34 +324,29 @@ impl DistributedCertBot { // Validate every domain's DNS credential up front: constructing a DNS // client resolves the zone through an authenticated API call, so a // misconfigured domain aborts the rotation here with no side effects - // and no ACME account consumed. + // and no ACME account consumed. A dns-persist-01 domain has no + // credential to check and is prepared without touching any provider. let mut prepared = Vec::with_capacity(configs.len()); for config in &configs { - let dns_cred = dns_credential_for(&self.kv_store, config)?; - let dns_client = self - .dns_client(&config.domain, &dns_cred) + let validation = self + .validation_for(&config.domain, config) .await .with_context(|| format!("DNS credential check failed for {}", config.domain))?; - prepared.push((&config.domain, dns_cred, dns_client)); + prepared.push((config, validation)); } let total = prepared.len(); - let mut prepared = prepared.into_iter(); - let Some((first_domain, first_cred, first_client)) = prepared.next() else { - bail!("no ZT-Domain configured for ACME credential rotation"); - }; - let client = AcmeClient::new_account( - acme_url, - first_client, - first_cred.max_dns_wait, - first_cred.dns_txt_ttl, - ) - .await - .context("failed to create replacement ACME account")?; - let credentials = client - .dump_credentials() - .context("failed to encode replacement ACME credentials")?; - let account_uri = client.account_id().to_string(); + // Registration needs no domain: it proves nothing about one. A cluster + // with no ZT domain yet registers here and gets an account URI, which is + // what a `dns-persist-01` record has to name before the domain it + // authorizes can ever be issued. + let account = AcmeClient::register_account(acme_url) + .await + .context("failed to create replacement ACME account")?; + let AcmeAccount { + credentials, + account_uri, + } = account; // Publish immediately. From here the cluster converges on the new // account, and recovering from a partial re-pin below never needs to @@ -203,41 +359,47 @@ impl DistributedCertBot { // Re-pin every domain's CAA to the new account, best effort across all // domains: one failing domain must not block re-pinning the rest. The // first domain reuses the registration client, which is already bound - // to its DNS client and the new credentials. - let mut failed = Vec::new(); - let mut record = |domain: &String, result: Result<()>| match result { + // to its validation method and the new credentials. + // + // A dns-persist-01 domain has nothing to re-pin from here, and rotation + // is not transparent for it either: its `_validation-persist` record + // still names the *old* account, so orders for that domain fail until + // the operator republishes. Those records are reported below. + let mut tally = RepinTally::default(); + let repin = |tally: &mut RepinTally, domain: &str, result: Result<()>| match result { Ok(()) => info!("cert[{domain}]: CAA re-pinned to {account_uri}"), Err(err) => { error!("cert[{domain}]: failed to re-pin CAA: {err:?}"); - failed.push(domain.clone()); + tally.failed.push(domain.to_string()); } }; - record( - first_domain, - client - .set_caa_records(std::slice::from_ref(first_domain)) - .await - .context("failed to update CAA records"), - ); - for (domain, dns_cred, dns_client) in prepared { + for (config, validation) in prepared { + if !validation.writes_dns() { + tally.manual += 1; + continue; + } let result = async { - let client = AcmeClient::load( - dns_client, - &credentials, - dns_cred.max_dns_wait, - dns_cred.dns_txt_ttl, - ) - .await - .context("failed to prepare ACME client")?; + let client = + AcmeClient::load(validation.method, &credentials, validation.max_dns_wait) + .await + .context("failed to prepare ACME client")?; client - .set_caa_records(std::slice::from_ref(domain)) + .set_caa_records(std::slice::from_ref(&config.domain)) .await .context("failed to update CAA records") } .await; - record(domain, result); + repin(&mut tally, &config.domain, result); } + // Rendered after the new credentials are published, so the records name + // the account the cluster has actually switched to. + let manual = configs + .iter() + .filter(|config| config.challenge != ChallengeKind::Dns01) + .flat_map(|config| self.report_manual_records(config)) + .collect::>(); + // Attest the new account only after CAA re-pinning: attestation does // not gate issuance, so its agent round trips must not widen the // window where the published account and the CAA records disagree. @@ -247,16 +409,38 @@ impl DistributedCertBot { warn!("failed to attest rotated ACME account: {err:?}"); } - if !failed.is_empty() { - bail!( + if !tally.failed.is_empty() { + // Reported, not raised. The account is registered and the cluster is + // already using it, so an error here reads as "nothing happened" and + // invites a retry -- which would register yet another account + // against a rate-limited quota and rewrite CAA a second time. What + // is left is a `SetCaa` run, and the caller is told exactly that. + // + // Out of the domains a re-pin was attempted for, not out of every + // domain: a dns-persist-01 domain was never a candidate, and + // counting it in the denominator reports it as re-pinned. + error!( "rotated to {account_uri} and published the new credentials, but failed to \ - re-pin CAA for {}/{total} domains: {}; rerun SetCaa until it succeeds — \ + re-pin CAA for {}/{} domains: {}; rerun SetCaa until it succeeds — \ retrying the rotation would register yet another account", - failed.len(), - failed.join(", ") + tally.failed.len(), + tally.attempted(total), + tally.failed.join(", ") + ); + } + if tally.manual > 0 { + warn!( + "{}/{total} domains use dns-persist-01: issuance for them stays \ + broken until the records above name the new account {account_uri}", + tally.manual ); } - Ok((account_uri, total)) + Ok(RotationOutcome { + account_uri, + domains_updated: tally.updated(total), + required_dns_records: manual, + repin_failed: tally.failed, + }) } /// Get the current certbot configuration from KV store. @@ -376,8 +560,17 @@ impl DistributedCertBot { async fn do_set_caa_all(&self, configs: Vec) -> Result<()> { let total = configs.len(); let mut failed = Vec::new(); + let mut skipped = 0usize; for config in configs { let domain = config.domain.clone(); + // A dns-persist-01 domain has no DNS write access to reconcile with. + // Log the records the operator owns instead of failing the call, so + // one such domain does not make SetCaa unusable for the rest. + if config.challenge != ChallengeKind::Dns01 { + self.report_manual_records(&config); + skipped += 1; + continue; + } match self.set_caa(&domain, &config).await { Ok(()) => info!("cert[{domain}]: CAA records reconciled"), Err(err) => { @@ -395,7 +588,10 @@ impl DistributedCertBot { failed.join(", ") ); } - info!("CAA records reconciled for {total} domains"); + info!( + "CAA records reconciled for {} domains ({skipped} on dns-persist-01 left to the operator)", + total - skipped + ); Ok(()) } @@ -413,10 +609,9 @@ impl DistributedCertBot { // the lock may call [`Self::acquire_acme_lock`], and keeping the // registering variant out of this path is what makes that structural // rather than a rule to remember. - let dns_cred = dns_credential_for(&self.kv_store, config)?; let acme_url = self.acme_url()?; let acme_client = self - .load_stored_acme_client(domain, &dns_cred, &acme_url) + .load_stored_acme_client(domain, config, &acme_url) .await .context("failed to initialize ACME client")? .context("no shared ACME account is registered for this cluster")?; @@ -627,12 +822,10 @@ impl DistributedCertBot { domain: &str, config: &ZtDomainConfig, ) -> Result { - // Get DNS credential (from config or default) - let dns_cred = dns_credential_for(&self.kv_store, config)?; let acme_url = self.acme_url()?; if let Some(client) = self - .load_stored_acme_client(domain, &dns_cred, &acme_url) + .load_stored_acme_client(domain, config, &acme_url) .await? { info!("loaded global ACME account credentials from KvStore"); @@ -650,7 +843,7 @@ impl DistributedCertBot { // lock and look again before spending a registration. let rotation_lock = self.acquire_acme_lock("register the shared ACME account")?; let client = self - .register_or_adopt_account(domain, &dns_cred, &acme_url) + .register_or_adopt_account(domain, config, &acme_url) .await; if let Err(err) = self.release_rotation_lock(&rotation_lock) { error!("failed to release ACME rotation lock: {err:?}"); @@ -670,7 +863,7 @@ impl DistributedCertBot { async fn load_stored_acme_client( &self, domain: &str, - dns_cred: &DnsCredential, + config: &ZtDomainConfig, acme_url: &str, ) -> Result> { let Some(creds) = self @@ -688,12 +881,11 @@ impl DistributedCertBot { call RotateAcmeCredentials to switch directories" ); } - let dns01_client = self.dns_client(domain, dns_cred).await?; + let validation = self.validation_for(domain, config).await?; let client = AcmeClient::load( - dns01_client, + validation.method, &creds.acme_credentials, - dns_cred.max_dns_wait, - dns_cred.dns_txt_ttl, + validation.max_dns_wait, ) .await .context("failed to load ACME client from KvStore credentials")?; @@ -709,11 +901,11 @@ impl DistributedCertBot { async fn register_or_adopt_account( &self, domain: &str, - dns_cred: &DnsCredential, + config: &ZtDomainConfig, acme_url: &str, ) -> Result { if let Some(client) = self - .load_stored_acme_client(domain, dns_cred, acme_url) + .load_stored_acme_client(domain, config, acme_url) .await? { info!("adopted the ACME account registered while this node waited for the lock"); @@ -721,15 +913,10 @@ impl DistributedCertBot { } info!("creating new global ACME account at {acme_url}"); - let dns01_client = self.dns_client(domain, dns_cred).await?; - let client = AcmeClient::new_account( - acme_url, - dns01_client, - dns_cred.max_dns_wait, - dns_cred.dns_txt_ttl, - ) - .await - .context("failed to create new ACME account")?; + let validation = self.validation_for(domain, config).await?; + let client = AcmeClient::new_account(acme_url, validation.method, validation.max_dns_wait) + .await + .context("failed to create new ACME account")?; let creds_json = client .dump_credentials() @@ -967,6 +1154,7 @@ mod tests { port: 443, node: None, priority: 0, + challenge: ChallengeKind::Dns01, } } @@ -1168,7 +1356,7 @@ mod tests { let err = match certbot .register_or_adopt_account( "app.example.com", - &unreachable_dns_credential(), + &test_zt_domain_config(), DEFAULT_ACME_URL, ) .await @@ -1185,6 +1373,74 @@ mod tests { /// Two rotations on this node are ordered by the same lock that orders two /// nodes: the lock lives in the KV store, so a second in-process run sees /// the first one's record. + /// The check has to finish inside the timeout wrapping the order, or the + /// "proceed anyway" exit it is designed around is never reached and the + /// renewal dies with a timeout instead of a diagnosis. Both defaults are + /// 300s, which is exactly the case that fails. + #[test] + fn the_dns_wait_ends_before_the_order_times_out() { + let default_credential = Duration::from_secs(300); + let default_renew_timeout = Duration::from_secs(300); + assert!( + advisory_dns_wait(default_credential, default_renew_timeout) < default_renew_timeout + ); + + // Lowering renew_timeout from the dashboard has to keep the invariant, + // which is why this is derived rather than a constant. + for secs in [30, 60, 120, 600] { + let renew_timeout = Duration::from_secs(secs); + assert!( + advisory_dns_wait(DNS_PERSIST_MAX_DNS_WAIT, renew_timeout) < renew_timeout, + "renew_timeout={secs}s leaves no room for the order" + ); + } + } + + /// A dns-persist-01 domain is never a re-pin candidate, so counting it into + /// the denominator reports it as re-pinned. Five domains, three of them + /// manual and the other two failing, is "2/2 failed" and zero updated -- + /// not "2/5 failed", which claims three succeeded. + #[test] + fn the_failure_count_is_out_of_the_domains_actually_tried() { + let tally = RepinTally { + manual: 3, + failed: vec!["a.example".to_string(), "b.example".to_string()], + }; + assert_eq!(tally.attempted(5), 2); + assert_eq!(tally.updated(5), 0); + } + + /// Every domain manual: nothing was attempted and nothing was updated. The + /// arithmetic has to land on zero rather than underflow, which as a usize + /// subtraction would panic the rotation after the account was published. + #[test] + fn an_all_manual_rotation_updates_nothing() { + let tally = RepinTally { + manual: 4, + failed: vec![], + }; + assert_eq!(tally.attempted(4), 0); + assert_eq!(tally.updated(4), 0); + } + + /// The ordinary case: every domain tried and every one taken. + #[test] + fn a_clean_rotation_updates_every_domain() { + let tally = RepinTally::default(); + assert_eq!(tally.attempted(3), 3); + assert_eq!(tally.updated(3), 3); + } + + /// A credential asking for less than its share keeps asking for less: the + /// clamp is a ceiling, not a target. + #[test] + fn a_shorter_configured_wait_is_left_alone() { + assert_eq!( + advisory_dns_wait(Duration::from_secs(20), Duration::from_secs(300)), + Duration::from_secs(20) + ); + } + #[tokio::test] async fn rotate_acme_credentials_rejects_concurrent_runs() { let data_dir = tempfile::tempdir().expect("failed to create temp dir"); @@ -1220,16 +1476,27 @@ mod tests { ); } + /// Registering without a domain is the `dns-persist-01` bootstrap: the + /// record names the account, so the account comes first. The run here gets + /// as far as the ACME server -- an unroutable one, so the test stays + /// offline -- which is proof enough that no domain was demanded before it. #[tokio::test] - async fn rotate_acme_credentials_requires_a_configured_domain() { + async fn rotate_acme_credentials_registers_without_any_domain() { let data_dir = tempfile::tempdir().expect("failed to create temp dir"); let certbot = test_certbot(data_dir.path()); + certbot + .kv_store + .set_certbot_config(&crate::kv::GlobalCertbotConfig { + acme_url: "http://127.0.0.1:1/directory".to_string(), + ..Default::default() + }) + .expect("failed to store certbot config"); let err = certbot .rotate_acme_credentials() .await - .expect_err("rotation without domains should fail"); + .expect_err("an unroutable ACME server cannot register an account"); assert!( - err.to_string().contains("no ZT-Domain configured"), + err.to_string().contains("replacement ACME account"), "unexpected error: {err}" ); // The failed rotation must release the KV lock so a later run can proceed. diff --git a/dstack/gateway/src/kv/mod.rs b/dstack/gateway/src/kv/mod.rs index 62bdc3870..7896d029a 100644 --- a/dstack/gateway/src/kv/mod.rs +++ b/dstack/gateway/src/kv/mod.rs @@ -57,6 +57,7 @@ use std::{ }; use anyhow::{Context, Result}; +use certbot::ChallengeKind; use crate::models::InstanceInfo; use crate::time::{encode_ts, now_secs}; @@ -378,6 +379,12 @@ pub struct ZtDomainConfig { /// The domain with highest priority is returned as the default base_domain in APIs #[serde(default)] pub priority: i32, + /// ACME challenge used to prove control of this domain. + /// + /// Records written before dns-persist-01 support have no such field and + /// decode as `dns-01`, which is the only method those deployments had. + #[serde(default)] + pub challenge: ChallengeKind, } /// Global certbot configuration (stored in KV, synced across nodes) @@ -394,6 +401,15 @@ pub struct GlobalCertbotConfig { pub renew_timeout: Duration, /// ACME server URL (None means use default Let's Encrypt production) pub acme_url: String, + /// Issuer Domain Name naming the CA in dns-persist-01 and CAA records. + /// + /// Empty means Let's Encrypt. A `dns-persist-01` record has to name a CA + /// the challenge lists in `issuer-domain-names`, and the CAA records written + /// for either challenge name the same CA, so a private or staging ACME + /// server needs its own value here -- one setting for both, since `acme_url` + /// is one setting for both. + #[serde(default)] + pub issuer_domain_name: String, } impl Default for GlobalCertbotConfig { @@ -403,6 +419,7 @@ impl Default for GlobalCertbotConfig { renew_before_expiration: Duration::from_secs(30 * 86400), // 30 days renew_timeout: Duration::from_secs(300), // 5 minutes acme_url: Default::default(), // default Let's Encrypt + issuer_domain_name: Default::default(), // default Let's Encrypt } } } @@ -2524,6 +2541,43 @@ mod value_encoding_tests { } } + /// A ZT domain stored before `dns-persist-01` existed has no `challenge` + /// field, and must keep decoding as the method it was actually using. + #[test] + fn a_zt_domain_without_a_challenge_field_decodes_as_dns01() { + /// The shape `ZtDomainConfig` had before the field was added. + #[derive(Serialize)] + struct LegacyZtDomainConfig { + domain: String, + dns_cred_id: Option, + port: u16, + node: Option, + priority: i32, + } + + let legacy = LegacyZtDomainConfig { + domain: "app.example.com".to_string(), + dns_cred_id: Some("cred-1".to_string()), + port: 443, + node: None, + priority: 7, + }; + + for (label, encoded) in [ + ("named", encode(&legacy).expect("named encode")), + ( + "legacy positional", + rmp_serde::encode::to_vec(&legacy).expect("positional encode"), + ), + ] { + let decoded: ZtDomainConfig = + decode(&encoded).unwrap_or_else(|err| panic!("{label} decode failed: {err}")); + assert_eq!(decoded.domain, "app.example.com", "{label}"); + assert_eq!(decoded.priority, 7, "{label}"); + assert_eq!(decoded.challenge, ChallengeKind::Dns01, "{label}"); + } + } + /// The configured window has to reach the store, not just the config file. /// /// An fsync per write runs under the store lock, so it bounds how fast this @@ -3413,6 +3467,7 @@ mod corruption_tests { port: 443, node: None, priority: 0, + challenge: Default::default(), }) .expect("save should succeed"); // Same record filed under another domain's key: honouring the value @@ -3427,6 +3482,7 @@ mod corruption_tests { port: 443, node: None, priority: 100, + challenge: Default::default(), }, true, ) diff --git a/dstack/gateway/src/main_service.rs b/dstack/gateway/src/main_service.rs index 427405c59..678117b91 100644 --- a/dstack/gateway/src/main_service.rs +++ b/dstack/gateway/src/main_service.rs @@ -619,7 +619,9 @@ impl Proxy { } } - pub(crate) async fn rotate_acme_credentials(&self) -> Result<(String, usize)> { + pub(crate) async fn rotate_acme_credentials( + &self, + ) -> Result { self.certbot.rotate_acme_credentials().await } diff --git a/dstack/gateway/templates/dashboard.html b/dstack/gateway/templates/dashboard.html index 7d62d69e0..d3854fafb 100644 --- a/dstack/gateway/templates/dashboard.html +++ b/dstack/gateway/templates/dashboard.html @@ -522,7 +522,7 @@

Global Connections

Account URI - {{ acme_info.account_uri }} + {{ acme_info.account_uri }} Historical Certificate Public Keys @@ -549,6 +549,24 @@

Certbot Configuration

ACME server URL (empty = Let's Encrypt production) + + ACME Account + + + + + Registers the shared ACME account. Every run registers a new one, so + dns-persist-01 validation records and account-pinned CAA have to be + republished afterwards. + + + + Issuer Domain Name + + + + Name of the CA in dns-persist-01 and CAA records; must be one the ACME server advertises + Renewal Interval @@ -760,6 +778,13 @@

Edit ZT-Domain

+
+ + +
@@ -779,6 +804,39 @@

Edit ZT-Domain

+ + +