Skip to content

fix(certbot): reissue when the configured domains change - #1137

Merged
kvinwang merged 5 commits into
nextfrom
fix/certbot-san-config
Aug 26, 2026
Merged

fix(certbot): reissue when the configured domains change#1137
kvinwang merged 5 commits into
nextfrom
fix/certbot-san-config

Conversation

@kvinwang

@kvinwang kvinwang commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Problem

Editing domains in certbot.toml does nothing once a certificate exists. The
operator adds a name, restarts certbot, and every run reports the certificate as
current:

INFO certbot::bot: checking if certificate needs to be renewed
INFO certbot::bot: certificate /e2e/wd/live/cert.pem is up to date

Two places have to agree on the name list, and neither consults the
configuration:

// create_cert_if_needed: existence is the whole test -- the names are never looked at
if live_cert_pem_path.as_ref().exists() && live_key_pem_path.as_ref().exists() {
    return Ok(false);
}

// renew_cert: the names come from the certificate being replaced
let domains = extract_subject_alt_names(cert_pem)?;
self.request_new_certificate(key_pem, &domains).await

So the name list is pinned at whatever the first issuance used, and renewal
copies it forward. The configuration edit is not deferred until expiry -- it
never takes effect at all, and nothing in the log says so.

This is the CLI's issuance path (certbot/src/bot.rs, used by
certbot/cli), which is how self-hosted deployments obtain the gateway
certificate -- docs/deployment.md and docs/dstack-gateway.md both drive it
through certbot.toml. The in-CVM gateway path is unaffected: DistributedCertBot
computes *.{domain} from its config on every issuance and never calls
renew_cert.

Fix

The live certificate is only the certificate that was asked for if it carries
the configured names, so create_cert_if_needed compares them and reissues on a
mismatch:

match extract_subject_alt_names(&live_cert_pem) {
    Ok(names) if names_match(&names, domains) => return Ok(false),
    Ok(names) => info!("reissuing: the live certificate covers {}, the configuration asks for {}", ...),
    Err(err) => warn!("reissuing: cannot read the live certificate's names: {err:#}"),
}

names_match compares sets, not sequences: the CA returns names in its own
order (the staging CA puts the wildcard first), DNS names are case-insensitive,
and a trailing root dot is the same name. Equality rather than containment, so
narrowing the list reissues too -- a certificate covering more than the
configuration asks for is not the configured certificate either.

An unreadable live certificate reissues rather than being left alone: it cannot
be checked against the configuration, so continuing to serve it means serving
something this process can no longer reason about. Issuance replaces it.

renew_cert still derives its names from the certificate it replaces, which is
correct once the certificate itself is kept in step: create_cert_if_needed
runs first on every cycle, so renewal only ever sees an aligned certificate.
The existing key is reused, as renewal already does.

A reissue that fails must not take the renewal down with it. The configuration
can name something the CA will never validate -- a typo, a zone the Cloudflare
token cannot write -- and that reissue then fails on every cycle. Failing the
run at that point would skip the renewal check below it, so one name the
operator got wrong would stop renewing the certificate that is actually being
served, until it expires 90 days later: a worse outcome than the edit being
ignored, which is what this PR started from. renew_inner reports the failure
and carries on to the renewal, and returns the failure only if the renewal
committed nothing of its own -- so a daemon keeps the deployment alive, a
renew --once still exits non-zero, and a renewal that did commit still runs
the renewed_hook. Nothing to protect on a first run, where there is no live
certificate: the error propagates as before.

The decision itself is reissue_reason(live_cert_pem, domains) -> Option<String>,
a pure function of the certificate and the configuration, so all three of its
outcomes are unit-testable. A live certificate that cannot even be read off disk
takes the same path as one that cannot be parsed, rather than failing the run.

Verification

Pebble + a mock Cloudflare API. Same workdir, domains edited between the two
runs, origin/next (55021edbf8) and this branch:

Step origin/next this PR
1. ["sanchange.e2e.test"] issued DNS:sanchange.e2e.test issued DNS:sanchange.e2e.test
2. ["sanchange.e2e.test", "extra.e2e.test"] certificate ... is up to date, no new certificate reissued DNS:sanchange.e2e.test, DNS:extra.e2e.test

origin/next ends with one certificate in the backup directory, this branch
with two. The reissue announces itself:

INFO certbot::acme_client: reissuing: the live certificate covers sanchange.e2e.test, the configuration asks for sanchange.e2e.test, extra.e2e.test
INFO certbot::bot: created new certificate

Two distinct names rather than a name plus its wildcard, deliberately: a
base-plus-wildcard order fails on next for an unrelated reason, fixed
separately in #1136.

Unit tests cover names_match (reordering and case are not changes; an added or
dropped name is) and reissue_reason against real self-signed certificates (the
configured one is kept, a missing name reissues and names both lists, an
unparseable certificate reissues). cargo fmt, cargo clippy -p certbot -p certbot-cli --all-targets -D warnings and cargo test -p certbot are clean.

Two adjacent fixes

Found while documenting the CLI, both in separate commits so they can be dropped
independently.

certbot cfg walked the serialized document and the struct's doc comments in
lockstep by position, and a None option serializes to nothing -- so with
cf_api_url absent, every comment from there on described the key above the one
it belonged to:

# Optional Cloudflare-compatible API base URL
dns_txt_ttl = 60
# TTL for DNS TXT challenge records in seconds
auto_set_caa = true

The comment is now looked up by key name. init's help said it initializes the
configuration file; it reads the configuration file and creates the ACME
account.

The CLI also gets a README saying what it is for: it keeps the ACME account key
and the certificate key in a plain directory on the host that runs it, so
nothing about the issuance is attested, while a deployed gateway issues its own
certificates from the configuration it already holds and publishes the public
keys for ct_monitor to check.

The deployment guide was pointing at the CLI

docs/deployment.md step 4 told the operator to set GATEWAY_CERT/GATEWAY_KEY
in build-config.sh and run ./certbot renew -c certbot.toml on the host. A
gateway deployed by that guide runs as a CVM and issues its own certificates
from its admin config, keeping the ACME account key and every certificate in the
CVM's WaveKV store -- so the old text produced a certificate on the host that
nothing reads, from a file that is not part of the guide's flow.

The step now describes what step 3's bootstrap-cluster.sh already configured
(SetCertbotConfig, CreateDnsCredential, AddZtDomain), how to watch
issuance land in ListZtDomains, how SetCaa pins issuance to the gateway's
own ACME account, and what switching from staging to production takes. It moves
to the required part of the checklist: without a certificate for the domain the
gateway cannot serve an app over TLS.

docs/dstack-gateway.md had the same problem in the other deployment shape: it
walked a host-mode gateway through certbot.toml, ./certbot set-caa && ./certbot renew, and cert_chain/cert_key. A gateway on the host links the same crate
and issues from its own process just as one in a CVM does, so that guide now
configures the gateway -- the proxy fields, the admin API the ACME settings are
stored through, and a data_dir that survives restarts -- and then walks the
same three admin RPCs. cert_chain/cert_key are documented for what they are:
a certificate something else produced, loaded at startup.

Two things noticed in that file while editing it: core.admin.auth_token is the
current key name (the guide used the accepted older admin_token), and the
alerting table listed dstack_gateway_kv_persist_failures_total twice.

Copilot AI lite review requested due to automatic review settings August 25, 2026 14:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

…keys

`certbot cfg` walks the serialized document and the struct's doc comments in
lockstep, by position. An `Option` that is `None` serializes to nothing, so
`cf_api_url` and `renewed_hook` are absent from the document and every comment
after the first of them describes the key above the one it belongs to:

    # Optional Cloudflare-compatible API base URL
    dns_txt_ttl = 60
    # TTL for DNS TXT challenge records in seconds
    auto_set_caa = true

Look the comment up by key name instead. A key with no doc comment keeps none
rather than borrowing its neighbour's.
The CLI keeps the ACME account key and the certificate key in a plain directory
on the host that runs it, so nothing about the issuance is attested. A deployed
gateway issues its own certificates from the configuration it already holds and
publishes the public keys for `ct_monitor` to check. Say so in a README, next
to what the subcommands do and where the workdir keeps things.

`init`'s help said it initializes the configuration file; it reads the
configuration file and creates the ACME account.
@kvinwang
kvinwang force-pushed the fix/certbot-san-config branch from 1431ee1 to a896a25 Compare August 26, 2026 03:23
Step 4 told the operator to configure `GATEWAY_CERT`/`GATEWAY_KEY` in
`build-config.sh` and run `./certbot renew -c certbot.toml` on the host. That
is not how a deployed gateway gets a certificate, and `build-config.sh` is not
part of this guide's flow: `dstack-gateway` links the `certbot` crate and runs
it inside the CVM, answering dns-01 with the DNS credential in its admin
config and keeping the ACME account key and every certificate in the CVM's
WaveKV store. Following the old text produced a certificate on the host that
nothing reads.

Describe what step 3 already set up -- `bootstrap-cluster.sh` calling
SetCertbotConfig, CreateDnsCredential and AddZtDomain -- how to watch issuance
land in `ListZtDomains`, how to pin CAA, and what switching from staging to
production takes. Zero-trust HTTPS moves to the required part of the checklist:
without a certificate for the domain the gateway cannot serve an app over TLS.
The production setup guide walked the operator through editing `certbot.toml`,
running `./certbot set-caa && ./certbot renew` by hand, and pointing
`cert_chain`/`cert_key` at the result. A gateway running on the host links the
same `certbot` crate as one running in a CVM and issues over dns-01 from its
own process, keeping the ACME account key and every certificate in its WaveKV
store -- `cert_chain`/`cert_key` only load a certificate something else
produced.

Configure the gateway instead: the proxy fields, the admin API the ACME
settings are stored through, and a `data_dir` that survives restarts so a
restart does not ask the CA for a fresh certificate. Then SetCertbotConfig,
CreateDnsCredential and AddZtDomain, with what to watch in ListZtDomains, what
SetCaa pins, and what switching from staging to production takes.

Also: `core.admin.auth_token` is the current key name (`admin_token` is the
accepted older one), URL Format becomes its own section rather than sitting
inside a configuration step, and the alerting table listed
`dstack_gateway_kv_persist_failures_total` twice.
@kvinwang
kvinwang merged commit 96de058 into next Aug 26, 2026
17 checks passed
@kvinwang
kvinwang deleted the fix/certbot-san-config branch August 26, 2026 04:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants