diff --git a/README.md b/README.md index da504d8..758fc39 100644 --- a/README.md +++ b/README.md @@ -783,6 +783,45 @@ root path. Services deployed to other paths on the same host will use the same TLS settings as those specified for the root path. +### Deploying before a DNS cutover + +You can deploy a host before its DNS points at the proxy. Nothing is ordered +from the certificate authority until the first HTTPS request for that hostname +arrives, and before the domain resolves here, an HTTP-01 challenge cannot +succeed — so the proxy checks first, with a cheap request to the domain, that +it actually routes back here. If it does not, the handshake is refused and no +order is spent. That matters: Let's Encrypt allows only five failed +authorizations per hostname per hour, and tripping that limit is what would +otherwise delay the certificate at the moment of the cutover. + +Held domains are re-checked every minute (`--acme-release-probe-interval`, or a +negative value to switch it off). As soon as the domain starts routing here, +the hold lifts and the certificate is issued — so the cutover costs about a +minute, not a backoff step. + +To watch it happen, or to see why a certificate has not arrived: + + kamal-proxy domains list + + SERVICE DOMAIN CERTIFIED HOLD REMOVAL HELD + service1 app.example.com yes + service1 new.example.com no 2026-08-25 14:32:10 (preflight) + +A `preflight` hold means the domain does not point here yet — repoint it and +the proxy will pick it up on its own. An `acme` hold means the certificate +authority rejected an order, and lifts the same way. A `rate_limited` hold is +the authority's own window, and waits it out. + +If you know the cause is already fixed and do not want to wait: + + kamal-proxy domains retry new.example.com + +Domains covered by a DNS-01 provider skip all of this — see [Wildcard +Certificates](#wildcard-certificates-dns-01-challenge). DNS-01 validation never +depends on where a domain points, so those certificates can be issued days +before a cutover with nothing to check and nothing to wait for. + + ### On-demand TLS Instead of specifying a static list of hosts, Kamal Proxy can also obtain TLS @@ -1041,8 +1080,11 @@ per-domain by default, throttled well under Let's Encrypt's account limits (burst of 20 orders, then one per 40s, max 3 in flight). Before a domain's first order, the proxy probes `http:///.kamal-proxy/preflight/` to verify DNS actually routes here — unreachable domains are quarantined -(5m, then 15m → 1h → 4h → 24h backoff) without burning an order. Failing -domains quarantine alone; the rest of a batch is retried once. Renewals reuse +(5m, then 15m → 1h → 4h → 24h backoff) without burning an order. A held +domain is re-probed every minute and its hold lifts as soon as it routes +here, so a backoff step is a ceiling rather than a wait; only a hold the +certificate authority imposed (rate limiting) runs to its own end time. +Failing domains quarantine alone; the rest of a batch is retried once. Renewals reuse the exact same identifier set (exempt from most rate limits) and pass ARI `replaces` where supported. Every renewal re-probes its dynamic members first, so a tenant whose DNS moved away after issuance is quarantined out of @@ -1074,11 +1116,16 @@ the app is down. **Inspecting:** ```bash -kamal-proxy domains list # every dynamic domain, cert + quarantine + held status -kamal-proxy domains stats # counters: domains, certified, queued, quarantined, held -kamal-proxy domains refresh # trigger an immediate re-poll of all sources +kamal-proxy domains list # every domain, cert + hold + held-removal status +kamal-proxy domains stats # counters: domains, certified, queued, quarantined, held +kamal-proxy domains refresh # trigger an immediate re-poll of all sources +kamal-proxy domains retry # clear an issuance hold and try again now ``` +Holds lift on their own once a domain routes back to the proxy — see +[Deploying before a DNS cutover](#deploying-before-a-dns-cutover). `retry` is +for when you already know the cause is fixed and would rather not wait. + ### Wildcard Certificates (DNS-01 Challenge) diff --git a/internal/cmd/domains.go b/internal/cmd/domains.go index b89a64d..8e98d50 100644 --- a/internal/cmd/domains.go +++ b/internal/cmd/domains.go @@ -1,6 +1,7 @@ package cmd import ( + "errors" "fmt" "maps" "net/rpc" @@ -20,12 +21,13 @@ func newDomainsCommand() *domainsCommand { domainsCommand := &domainsCommand{} domainsCommand.cmd = &cobra.Command{ Use: "domains", - Short: "Inspect and refresh dynamic TLS domains", + Short: "Inspect TLS domains and their certificate issuance", } domainsCommand.cmd.AddCommand(newDomainsListCommand().cmd) domainsCommand.cmd.AddCommand(newDomainsStatsCommand().cmd) domainsCommand.cmd.AddCommand(newDomainsRefreshCommand().cmd) + domainsCommand.cmd.AddCommand(newDomainsRetryCommand().cmd) return domainsCommand } @@ -52,7 +54,7 @@ func newDomainsListCommand() *domainsListCommand { domainsListCommand := &domainsListCommand{} domainsListCommand.cmd = &cobra.Command{ Use: "list", - Short: "List dynamic domains by service", + Short: "List dynamic domains and registered hosts by service", RunE: domainsListCommand.run, Args: cobra.NoArgs, Aliases: []string{"ls"}, @@ -83,10 +85,7 @@ func (c *domainsListCommand) run(cmd *cobra.Command, args []string) error { certified = "yes" } - quarantined := "" - if entry, ok := response.Quarantine[domain.Domain]; ok { - quarantined = entry.Until.Format("2006-01-02 15:04:05") - } + quarantined := holdDescription(response.Quarantine, domain.Domain) held := "" if _, ok := heldRemovals[domain.Domain]; ok { @@ -97,10 +96,39 @@ func (c *domainsListCommand) run(cmd *cobra.Command, args []string) error { } } + // Deploy-registered hosts have no domain source to group them under, + // but they are the common case — and the one an operator is staring at + // during a DNS cutover, wondering why the certificate has not arrived. + for _, domain := range slices.Sorted(maps.Keys(response.Registered)) { + entry := response.Registered[domain] + + certified := "no" + if entry.Certified { + certified = "yes" + } + + table.AddRow([]string{entry.Service, domain, certified, holdDescription(response.Quarantine, domain), ""}) + } + table.Print() }) } +// holdDescription renders a domain's issuance hold for the listing: when it +// lifts, and why it is held, since only some kinds lift on their own. +func holdDescription(quarantine map[string]server.QuarantineStatus, domain string) string { + entry, ok := quarantine[domain] + if !ok { + return "" + } + + until := entry.Until.Format("2006-01-02 15:04:05") + if entry.Kind == "" { + return until + } + return until + " (" + entry.Kind + ")" +} + type domainsStatsCommand struct { cmd *cobra.Command } @@ -132,9 +160,18 @@ func (c *domainsStatsCommand) run(cmd *cobra.Command, args []string) error { } } + registeredCertified := 0 + for _, entry := range response.Registered { + if entry.Certified { + registeredCertified++ + } + } + fmt.Printf("Services with domain sources: %d\n", len(response.Services)) fmt.Printf("Dynamic domains: %d\n", domains) - fmt.Printf("Certified: %d\n", certified) + fmt.Printf("Dynamic certified: %d\n", certified) + fmt.Printf("Registered hosts: %d\n", len(response.Registered)) + fmt.Printf("Registered certified: %d\n", registeredCertified) fmt.Printf("Queued for issuance: %d\n", response.QueueLength) fmt.Printf("Quarantined: %d\n", len(response.Quarantine)) fmt.Printf("Held removals: %d\n", held) @@ -175,3 +212,61 @@ func (c *domainsRefreshCommand) run(cmd *cobra.Command, args []string) error { return nil }) } + +type domainsRetryCommand struct { + cmd *cobra.Command + all bool +} + +func newDomainsRetryCommand() *domainsRetryCommand { + domainsRetryCommand := &domainsRetryCommand{} + domainsRetryCommand.cmd = &cobra.Command{ + Use: "retry [domain]", + Short: "Clear an issuance hold and try again now", + Long: "Clear the issuance hold on a domain and request its certificate again.\n\n" + + "Holds normally lift on their own once the domain routes back to this proxy,\n" + + "so reach for this when you know the cause is fixed and do not want to wait —\n" + + "including for a rate-limit hold, whose window the automatic release respects.", + RunE: domainsRetryCommand.run, + Args: cobra.MaximumNArgs(1), + } + + domainsRetryCommand.cmd.Flags().BoolVar(&domainsRetryCommand.all, "all", false, "Clear every issuance hold") + + return domainsRetryCommand +} + +func (c *domainsRetryCommand) run(cmd *cobra.Command, args []string) error { + if len(args) == 0 && !c.all { + return errors.New("specify a domain, or --all to clear every hold") + } + if len(args) > 0 && c.all { + return errors.New("specify a domain or --all, not both") + } + + domain := "" + if len(args) > 0 { + domain = args[0] + } + + return withRPCClient(globalConfig.SocketPath(), func(client *rpc.Client) error { + var cleared int + + err := client.Call("kamal-proxy.DomainsRetry", server.DomainsRetryArgs{Domain: domain}, &cleared) + if err != nil { + return err + } + + switch { + case cleared == 0 && domain != "": + fmt.Printf("No issuance hold on %s\n", domain) + case cleared == 0: + fmt.Println("No issuance holds to clear") + case cleared == 1: + fmt.Println("Cleared 1 issuance hold") + default: + fmt.Printf("Cleared %d issuance holds\n", cleared) + } + return nil + }) +} diff --git a/internal/cmd/run.go b/internal/cmd/run.go index 9a39183..657389d 100644 --- a/internal/cmd/run.go +++ b/internal/cmd/run.go @@ -70,6 +70,7 @@ func newRunCommand() *runCommand { runCommand.cmd.Flags().StringSliceVar(&runCommand.acmeDNSProviders, "acme-dns-provider", strings.Split(getEnvString("ACME_DNS_PROVIDER", ""), ","), "DNS provider for DNS-01 challenges (one of: "+providers.ProviderListForHelp()+"). DNS-01 activates only when set: auto detects the provider from environment credentials, none (the default) disables it. Repeatable: zone=provider entries pin a zone to the DNS host that serves it, and one bare entry is the default for unmatched zones") runCommand.cmd.Flags().BoolVar(&globalConfig.ACMEPreferWildcard, "acme-prefer-wildcard", getEnvBool("ACME_PREFER_WILDCARD", true), "Prefer wildcard certificates when DNS provider available") runCommand.cmd.Flags().BoolVar(&globalConfig.ACMEHTTPFallback, "acme-http-fallback", getEnvBool("ACME_HTTP_FALLBACK", true), "Fall back to HTTP-01 challenge if DNS-01 fails") + runCommand.cmd.Flags().DurationVar(&globalConfig.ACMEReleaseProbeInterval, "acme-release-probe-interval", 0, "How often to re-probe domains whose issuance is held, so a hold lifts as soon as the domain points here — a DNS cutover then costs one interval instead of a backoff step (default 1m; negative disables)") return runCommand } @@ -160,9 +161,10 @@ func (c *runCommand) run(cmd *cobra.Command, args []string) error { router.SetSANCertManager(manager) dynamicDomains = server.NewDynamicDomainManager(server.DynamicDomainConfig{ - StatePath: globalConfig.DynamicDomainsStatePath(), - RefreshToken: os.Getenv("KAMAL_PROXY_REFRESH_TOKEN"), - SourceToken: os.Getenv("KAMAL_PROXY_DOMAINS_TOKEN"), + StatePath: globalConfig.DynamicDomainsStatePath(), + RefreshToken: os.Getenv("KAMAL_PROXY_REFRESH_TOKEN"), + SourceToken: os.Getenv("KAMAL_PROXY_DOMAINS_TOKEN"), + ReleaseProbeInterval: globalConfig.ACMEReleaseProbeInterval, }, manager, router) router.SetDynamicDomainManager(dynamicDomains) diff --git a/internal/server/commands.go b/internal/server/commands.go index 1fb9fbc..3046b4d 100644 --- a/internal/server/commands.go +++ b/internal/server/commands.go @@ -74,6 +74,11 @@ type CertsExportArgs struct { Path string } +type DomainsRetryArgs struct { + // Domain to clear the hold on. Empty clears every hold. + Domain string +} + type CachePurgeArgs struct { Service string PathPrefix string @@ -107,13 +112,29 @@ type DomainsServiceStatus struct { type QuarantineStatus struct { Until time.Time `json:"until"` Failures int `json:"failures"` + + // Kind is why the domain is held: "preflight" (it did not route back to + // this proxy, no order spent), "acme" (the CA rejected an order), or + // "rate_limited" (the CA dictated the end time). Only the first two are + // lifted early when the domain starts routing here again. + Kind string `json:"kind"` +} + +// RegisteredDomainStatus reports the certificate state of a deploy-registered +// host — one named by `deploy --host`, as opposed to a tenant domain learned +// from a source. +type RegisteredDomainStatus struct { + Service string `json:"service"` + Certified bool `json:"certified"` + ExpiresAt time.Time `json:"expires_at,omitempty"` } type DomainsStatusResponse struct { - Services map[string]DomainsServiceStatus `json:"services"` - QueueLength int `json:"queue_length"` - Quarantine map[string]QuarantineStatus `json:"quarantine"` - Certificates int `json:"certificates"` + Services map[string]DomainsServiceStatus `json:"services"` + QueueLength int `json:"queue_length"` + Quarantine map[string]QuarantineStatus `json:"quarantine"` + Certificates int `json:"certificates"` + Registered map[string]RegisteredDomainStatus `json:"registered"` } func NewCommandHandler(router *Router) *CommandHandler { @@ -270,6 +291,18 @@ func (h *CommandHandler) DomainsStatus(args bool, reply *DomainsStatusResponse) return nil } +// DomainsRetry clears issuance holds so a domain can be tried again now. An +// empty Domain clears every hold. +func (h *CommandHandler) DomainsRetry(args DomainsRetryArgs, reply *int) error { + dynamicDomains := h.router.DynamicDomainManager() + if dynamicDomains == nil { + return errors.New("dynamic domains are not enabled (start the proxy with --acme-email)") + } + + *reply = dynamicDomains.Retry(args.Domain) + return nil +} + func (h *CommandHandler) DomainsRefresh(args bool, reply *int) error { dynamicDomains := h.router.DynamicDomainManager() if dynamicDomains == nil { diff --git a/internal/server/config.go b/internal/server/config.go index e7d60bc..4f48347 100644 --- a/internal/server/config.go +++ b/internal/server/config.go @@ -116,6 +116,12 @@ type Config struct { ACMEPreferWildcard bool ACMEHTTPFallback bool + // ACMEReleaseProbeInterval is how often a held domain is re-probed so its + // hold can be lifted as soon as it routes here again — the difference + // between a DNS cutover costing a probe interval and costing a backoff + // step. Zero uses the default; negative disables release probing. + ACMEReleaseProbeInterval time.Duration + // ACMEDNSProviderZones maps DNS zones to the provider answering DNS-01 // for them, for fleets whose zones live at different DNS hosts. // ACMEDNSProvider stays the default for unmatched zones. diff --git a/internal/server/domain_failure.go b/internal/server/domain_failure.go index 342cfb2..5047f4b 100644 --- a/internal/server/domain_failure.go +++ b/internal/server/domain_failure.go @@ -24,12 +24,12 @@ const maxConcurrentProbes = 16 // member otherwise, and the whole set when neither can tell — an // unattributable failure holds the entire batch on the quarantine ladder so // retries back off instead of looping against ACME rate limits. -func identifyFailedDomains(err error, domains []string, preflight func(string) error) []string { +func identifyFailedDomains(err error, domains []string, preflight func(string) error, unprobeable func(string) bool) []string { if failed := failedDomainsFromError(err, domains); len(failed) > 0 { return failed } - if failed, _ := probeDomains(domains, preflight); len(failed) > 0 { + if failed, _ := probeDomains(domains, preflight, unprobeable); len(failed) > 0 { return failed } @@ -38,9 +38,15 @@ func identifyFailedDomains(err error, domains []string, preflight func(string) e // probeDomains runs the pre-flight probe over a set of domains with bounded // concurrency and returns the ones that failed, in input order, with each -// failure's error. Wildcards are skipped — there is no name to answer on one. -// A nil probe reports nothing. -func probeDomains(domains []string, preflight func(string) error) ([]string, map[string]error) { +// failure's error. A nil probe reports nothing. +// +// Domains the probe cannot speak for are skipped rather than failed: a +// wildcard, which has no name to answer on, and anything unprobeable reports +// — in practice a zone with a DNS-01 provider, whose order never depends on +// where the domain points. Probing those and holding them back on failure +// would punish exactly the case DNS-01 exists to make safe: issuing a +// certificate before a DNS cutover. +func probeDomains(domains []string, preflight func(string) error, unprobeable func(string) bool) ([]string, map[string]error) { if preflight == nil { return nil, nil } @@ -49,7 +55,7 @@ func probeDomains(domains []string, preflight func(string) error) ([]string, map sem := make(chan struct{}, maxConcurrentProbes) var wg sync.WaitGroup for idx, domain := range domains { - if strings.HasPrefix(domain, "*.") { + if strings.HasPrefix(domain, "*.") || (unprobeable != nil && unprobeable(domain)) { continue } wg.Add(1) diff --git a/internal/server/domain_failure_test.go b/internal/server/domain_failure_test.go index 232cbd0..b8518c1 100644 --- a/internal/server/domain_failure_test.go +++ b/internal/server/domain_failure_test.go @@ -66,7 +66,7 @@ func TestIdentifyFailedDomains(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.expected, identifyFailedDomains(tt.err, tt.domains, tt.preflight)) + assert.Equal(t, tt.expected, identifyFailedDomains(tt.err, tt.domains, tt.preflight, nil)) }) } } diff --git a/internal/server/domain_issuer.go b/internal/server/domain_issuer.go index f0095a7..461012f 100644 --- a/internal/server/domain_issuer.go +++ b/internal/server/domain_issuer.go @@ -380,7 +380,7 @@ func (i *domainIssuer) handleObtainFailure(batch []*issueRequest, domains []stri return } - failed := identifyFailedDomains(err, domains, i.config.Preflight) + failed := identifyFailedDomains(err, domains, i.config.Preflight, i.manager.hasDNSProviderFor) slog.Warn("Certificate order failed", "domains", domains, "failed", failed, "error", err) diff --git a/internal/server/domain_quarantine.go b/internal/server/domain_quarantine.go index d8b88ee..ff68341 100644 --- a/internal/server/domain_quarantine.go +++ b/internal/server/domain_quarantine.go @@ -16,17 +16,44 @@ const ( // quarantinePreflight marks a failed self-probe: the domain didn't route // back to this proxy. No ACME order was spent, so the first retry is soon. quarantinePreflight + + // quarantineRateLimited marks a hold whose end time the ACME server + // dictated. Unlike the other two it is not the proxy's own backoff, so + // the release prober must never lift it early: a domain that now routes + // here is still inside Let's Encrypt's window, and ordering again both + // fails and pushes the window further out. + quarantineRateLimited ) +// String names the kind for operator-facing output. The persisted entry keeps +// the numeric form; only the status API and the CLI use this. +func (k quarantineKind) String() string { + switch k { + case quarantinePreflight: + return "preflight" + case quarantineRateLimited: + return "rate_limited" + default: + return "acme" + } +} + var ( acmeBackoffLadder = []time.Duration{15 * time.Minute, time.Hour, 4 * time.Hour, 24 * time.Hour} preflightBackoffLadder = []time.Duration{5 * time.Minute, 15 * time.Minute, time.Hour, 4 * time.Hour, 24 * time.Hour} ) // quarantineEntry records a domain's failure history and current hold. +// +// Kind names what caused the current hold, so the release prober can tell a +// backoff it may lift from one the ACME server imposed. A state file written +// before Kind existed has no "kind" key and decodes as quarantineACME — the +// conservative default, since an unattributed hold is likelier to have cost a +// real order than not. type quarantineEntry struct { - Until time.Time `json:"until"` - Failures int `json:"failures"` + Until time.Time `json:"until"` + Failures int `json:"failures"` + Kind quarantineKind `json:"kind"` } // domainQuarantine tracks per-domain issuance failures with escalating @@ -49,7 +76,7 @@ func (q *domainQuarantine) RecordFailure(domain string, kind quarantineKind) tim q.mu.Lock() defer q.mu.Unlock() - entry := q.record(domain) + entry := q.record(domain, kind) ladder := acmeBackoffLadder if kind == quarantinePreflight { @@ -70,7 +97,7 @@ func (q *domainQuarantine) RecordRateLimited(domain string, retryAfter time.Time q.mu.Lock() defer q.mu.Unlock() - entry := q.record(domain) + entry := q.record(domain, quarantineRateLimited) now := q.now() until := retryAfter.Add(rateLimitHoldMargin) @@ -84,18 +111,41 @@ func (q *domainQuarantine) RecordRateLimited(domain string, retryAfter time.Time return until.Sub(now) } -// record fetches or creates a domain's entry and counts a failure against it. +// record fetches or creates a domain's entry, counts a failure against it, and +// stamps the kind of the hold about to be applied. The newest failure's kind +// wins: a domain that failed its probe and later burned a real order is held +// as an ACME failure, which is the more expensive fact about it. // Callers must hold q.mu. -func (q *domainQuarantine) record(domain string) *quarantineEntry { +func (q *domainQuarantine) record(domain string, kind quarantineKind) *quarantineEntry { entry := q.entries[domain] if entry == nil { entry = &quarantineEntry{} q.entries[domain] = entry } entry.Failures++ + entry.Kind = kind return entry } +// Release lifts a domain's current hold while keeping its failure history, and +// reports whether it was actually holding. Use it when fresh evidence +// contradicts the hold — a pre-flight probe that now passes — rather than when +// the domain has succeeded. Keeping the count matters: a domain that flaps +// between routing here and not must keep climbing the ladder instead of +// resetting to the bottom every time it briefly looks healthy. +func (q *domainQuarantine) Release(domain string) bool { + q.mu.Lock() + defer q.mu.Unlock() + + entry := q.entries[domain] + if entry == nil || !q.now().Before(entry.Until) { + return false + } + + entry.Until = time.Time{} + return true +} + // Clear removes a domain's failure history (successful issuance, or the // domain left the source). func (q *domainQuarantine) Clear(domain string) { @@ -153,6 +203,6 @@ func (q *domainQuarantine) Restore(snapshot map[string]quarantineEntry) { q.entries = make(map[string]*quarantineEntry, len(snapshot)) for domain, entry := range snapshot { - q.entries[domain] = &quarantineEntry{Until: entry.Until, Failures: entry.Failures} + q.entries[domain] = &quarantineEntry{Until: entry.Until, Failures: entry.Failures, Kind: entry.Kind} } } diff --git a/internal/server/domain_quarantine_test.go b/internal/server/domain_quarantine_test.go index bccfcf5..3a4daf7 100644 --- a/internal/server/domain_quarantine_test.go +++ b/internal/server/domain_quarantine_test.go @@ -1,6 +1,7 @@ package server import ( + "encoding/json" "testing" "time" @@ -119,3 +120,75 @@ func TestDomainQuarantine_SnapshotRestore(t *testing.T) { assert.True(t, restored.IsQuarantined("bad.example.com")) assert.Equal(t, 4*time.Hour, restored.RecordFailure("bad.example.com", quarantineACME)) } + +func TestDomainQuarantine_ReleaseLiftsTheHoldButKeepsTheLadder(t *testing.T) { + q, _ := testQuarantineAt(time.Now()) + + q.RecordFailure("bad.example.com", quarantineACME) + q.RecordFailure("bad.example.com", quarantineACME) + require.True(t, q.IsQuarantined("bad.example.com")) + + assert.True(t, q.Release("bad.example.com")) + assert.False(t, q.IsQuarantined("bad.example.com")) + + // History survives: the next failure is the third rung, not the first. + assert.Equal(t, 4*time.Hour, q.RecordFailure("bad.example.com", quarantineACME)) +} + +func TestDomainQuarantine_ReleaseReportsWhetherItHeld(t *testing.T) { + start := time.Now() + q, current := testQuarantineAt(start) + + assert.False(t, q.Release("unknown.example.com"), "never held") + + q.RecordFailure("bad.example.com", quarantineACME) + *current = start.Add(time.Hour) + assert.False(t, q.Release("bad.example.com"), "hold already expired") +} + +func TestDomainQuarantine_RecordsTheKindOfHold(t *testing.T) { + q, _ := testQuarantineAt(time.Now()) + + q.RecordFailure("acme.example.com", quarantineACME) + q.RecordFailure("probe.example.com", quarantinePreflight) + q.RecordRateLimited("limited.example.com", time.Now().Add(time.Hour)) + + snapshot := q.Snapshot() + assert.Equal(t, quarantineACME, snapshot["acme.example.com"].Kind) + assert.Equal(t, quarantinePreflight, snapshot["probe.example.com"].Kind) + assert.Equal(t, quarantineRateLimited, snapshot["limited.example.com"].Kind, + "a rate-limit hold must be distinguishable: the release prober may never lift it") +} + +func TestDomainQuarantine_KindReflectsTheMostRecentFailure(t *testing.T) { + q, _ := testQuarantineAt(time.Now()) + + // A domain that first failed its probe and later burned a real order is + // held as an ACME failure — the newer, more expensive fact wins. + q.RecordFailure("flappy.example.com", quarantinePreflight) + q.RecordFailure("flappy.example.com", quarantineACME) + + assert.Equal(t, quarantineACME, q.Snapshot()["flappy.example.com"].Kind) +} + +func TestDomainQuarantine_KindSurvivesSnapshotRestore(t *testing.T) { + q, _ := testQuarantineAt(time.Now()) + + q.RecordRateLimited("limited.example.com", time.Now().Add(time.Hour)) + + restored, _ := testQuarantineAt(time.Now()) + restored.Restore(q.Snapshot()) + + assert.Equal(t, quarantineRateLimited, restored.Snapshot()["limited.example.com"].Kind) +} + +// A state file written before holds carried a kind has no "kind" key. It must +// decode as quarantineACME — the conservative default, since an unknown hold +// is more likely to have cost a real order than not. +func TestDomainQuarantine_LegacyEntryWithoutKindDecodesAsACME(t *testing.T) { + var entry quarantineEntry + require.NoError(t, json.Unmarshal([]byte(`{"until":"2026-08-25T10:00:00Z","failures":2}`), &entry)) + + assert.Equal(t, quarantineACME, entry.Kind) + assert.Equal(t, 2, entry.Failures) +} diff --git a/internal/server/domain_release.go b/internal/server/domain_release.go new file mode 100644 index 0000000..a7f5cd4 --- /dev/null +++ b/internal/server/domain_release.go @@ -0,0 +1,176 @@ +package server + +import ( + "context" + "log/slog" + "strings" + "sync" + "time" +) + +// Early release of issuance holds. +// +// The quarantine ladder exists so a domain that cannot be issued does not loop +// against the CA's rate limits. But its steps run to 24 hours, and the usual +// reason a domain cannot be issued is temporary and operator-driven: its DNS +// has not been repointed at this proxy yet. Waiting out a full step after the +// repoint is what makes preparing a cutover in advance feel like a punishment. +// +// The proxy already owns the sensor that answers the question — the pre-flight +// probe. This sweeps the held domains on an interval and lifts the hold from +// any that now route here, so a cutover costs about one probe interval rather +// than a ladder step. + +// DefaultReleaseProbeInterval is how often held domains are re-probed. It is +// short because the probe is a single local HTTP request against a name the +// operator already gave us, and because the whole value of the sweep is that a +// repoint is noticed promptly. +const DefaultReleaseProbeInterval = time.Minute + +type releaseProberConfig struct { + // Interval between sweeps. Zero disables the loop entirely. + Interval time.Duration + + // Preflight probes whether a domain routes back to this proxy. + Preflight func(domain string) error + + // Unprobeable reports domains the probe cannot speak for — in practice + // those in a zone with a DNS-01 provider, whose issuance never depended + // on where they point. Wildcards are always skipped. + Unprobeable func(domain string) bool + + // Released is notified for each domain whose hold was lifted, so issuance + // can be requested for it. + Released func(domain string) + + // OnChange is notified once per sweep that released anything, so the + // quarantine state can be persisted. + OnChange func() +} + +type releaseProber struct { + quarantine *domainQuarantine + config releaseProberConfig + + ctx context.Context + cancel context.CancelFunc + wg sync.WaitGroup +} + +func newReleaseProber(quarantine *domainQuarantine, config releaseProberConfig) *releaseProber { + ctx, cancel := context.WithCancel(context.Background()) + + return &releaseProber{ + quarantine: quarantine, + config: config, + ctx: ctx, + cancel: cancel, + } +} + +// Start launches the sweep loop. A zero interval leaves it dormant. +func (p *releaseProber) Start() { + if p.config.Interval <= 0 { + slog.Debug("Release probing disabled") + return + } + + p.wg.Add(1) + go func() { + defer p.wg.Done() + + ticker := time.NewTicker(p.config.Interval) + defer ticker.Stop() + + for { + select { + case <-p.ctx.Done(): + return + case <-ticker.C: + p.sweep() + } + } + }() +} + +func (p *releaseProber) Stop() { + p.cancel() + p.wg.Wait() +} + +// sweep probes every currently-held domain the probe can speak for and lifts +// the hold from those that answer. +// +// A failing probe deliberately records nothing. The domain is already held, +// and counting each sweep as a fresh failure would drive it to the top of the +// ladder within minutes purely for continuing to be in the state it was +// already being held for. +func (p *releaseProber) sweep() { + candidates := p.candidates() + if len(candidates) == 0 { + return + } + + reachable, _ := probeDomains(candidates, p.config.Preflight, p.config.Unprobeable) + held := map[string]struct{}{} + for _, domain := range reachable { + held[domain] = struct{}{} + } + + released := []string{} + for _, domain := range candidates { + if _, stillFailing := held[domain]; stillFailing { + continue + } + if p.quarantine.Release(domain) { + released = append(released, domain) + } + } + + if len(released) == 0 { + return + } + + slog.Info("Domains route here again; lifting issuance holds", "domains", released) + + if p.config.Released != nil { + for _, domain := range released { + p.config.Released(domain) + } + } + if p.config.OnChange != nil { + p.config.OnChange() + } +} + +// candidates lists the domains whose hold this sweep is allowed to lift: those +// actually holding right now, minus rate-limit holds and minus anything the +// probe cannot speak for. +// +// A rate limit is the CA's clock rather than the proxy's backoff — the domain +// routing here again says nothing about it, and ordering inside the window +// both fails and pushes the window further out. +// +// The unprobeable ones must be filtered out here rather than left to +// probeDomains: that function *skips* them, so they come back neither passed +// nor failed, and a sweep that read "not failed" as "passed" would release +// every wildcard and DNS-01 hold on its first tick without probing anything. +func (p *releaseProber) candidates() []string { + candidates := []string{} + for domain, entry := range p.quarantine.Snapshot() { + if entry.Kind == quarantineRateLimited { + continue + } + if strings.HasPrefix(domain, "*.") { + continue + } + if p.config.Unprobeable != nil && p.config.Unprobeable(domain) { + continue + } + if !p.quarantine.IsQuarantined(domain) { + continue + } + candidates = append(candidates, domain) + } + return candidates +} diff --git a/internal/server/domain_release_test.go b/internal/server/domain_release_test.go new file mode 100644 index 0000000..2f639b6 --- /dev/null +++ b/internal/server/domain_release_test.go @@ -0,0 +1,221 @@ +package server + +import ( + "errors" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type probeRecorder struct { + mu sync.Mutex + probed []string + failing map[string]bool +} + +func newProbeRecorder(failing ...string) *probeRecorder { + r := &probeRecorder{failing: map[string]bool{}} + for _, domain := range failing { + r.failing[domain] = true + } + return r +} + +func (r *probeRecorder) probe(domain string) error { + r.mu.Lock() + defer r.mu.Unlock() + + r.probed = append(r.probed, domain) + if r.failing[domain] { + return errors.New("does not route here") + } + return nil +} + +func (r *probeRecorder) calls() []string { + r.mu.Lock() + defer r.mu.Unlock() + + return append([]string{}, r.probed...) +} + +func testReleaseProber(q *domainQuarantine, config releaseProberConfig) *releaseProber { + if config.Preflight == nil { + config.Preflight = func(string) error { return nil } + } + return newReleaseProber(q, config) +} + +// The point of the whole feature: a domain held because it did not route here +// is eligible again as soon as it does, without waiting out its ladder step. +func TestReleaseProber_LiftsAPreflightHoldOnceTheDomainRoutesHere(t *testing.T) { + q, _ := testQuarantineAt(time.Now()) + q.RecordFailure("app.example.com", quarantinePreflight) + require.True(t, q.IsQuarantined("app.example.com")) + + released := []string{} + prober := testReleaseProber(q, releaseProberConfig{ + Released: func(domain string) { released = append(released, domain) }, + }) + + prober.sweep() + + assert.False(t, q.IsQuarantined("app.example.com")) + assert.Equal(t, []string{"app.example.com"}, released) +} + +// An ACME rejection whose demonstrable cause was the domain pointing elsewhere +// deserves a retry once that is fixed — but the ladder history must survive, +// so a domain that flaps keeps climbing. +func TestReleaseProber_LiftsAnACMEHoldButKeepsTheLadder(t *testing.T) { + q, _ := testQuarantineAt(time.Now()) + q.RecordFailure("app.example.com", quarantineACME) + q.RecordFailure("app.example.com", quarantineACME) + + prober := testReleaseProber(q, releaseProberConfig{}) + prober.sweep() + + assert.False(t, q.IsQuarantined("app.example.com")) + assert.Equal(t, 4*time.Hour, q.RecordFailure("app.example.com", quarantineACME), + "the next failure is the third rung, not the first") +} + +// A rate-limit hold is the CA's clock, not ours. The domain routing here again +// does nothing to lift Let's Encrypt's window, and ordering inside it both +// fails and pushes the window further out. +func TestReleaseProber_NeverLiftsARateLimitHold(t *testing.T) { + q, _ := testQuarantineAt(time.Now()) + q.RecordRateLimited("app.example.com", time.Now().Add(2*time.Hour)) + + recorder := newProbeRecorder() + prober := testReleaseProber(q, releaseProberConfig{Preflight: recorder.probe}) + prober.sweep() + + assert.True(t, q.IsQuarantined("app.example.com")) + assert.Empty(t, recorder.calls(), "no point probing a hold we may not lift") +} + +func TestReleaseProber_LeavesAHoldAloneWhenTheProbeStillFails(t *testing.T) { + q, _ := testQuarantineAt(time.Now()) + q.RecordFailure("app.example.com", quarantinePreflight) + + released := []string{} + prober := testReleaseProber(q, releaseProberConfig{ + Preflight: newProbeRecorder("app.example.com").probe, + Released: func(domain string) { released = append(released, domain) }, + }) + prober.sweep() + + assert.True(t, q.IsQuarantined("app.example.com")) + assert.Empty(t, released) +} + +// A failing sweep must not escalate the ladder: the hold is already running, +// and counting every probe as a fresh failure would drive a domain to 24h +// within minutes. +func TestReleaseProber_FailedProbeDoesNotEscalateTheLadder(t *testing.T) { + q, _ := testQuarantineAt(time.Now()) + q.RecordFailure("app.example.com", quarantinePreflight) + + prober := testReleaseProber(q, releaseProberConfig{ + Preflight: newProbeRecorder("app.example.com").probe, + }) + prober.sweep() + prober.sweep() + prober.sweep() + + assert.Equal(t, 1, q.Snapshot()["app.example.com"].Failures) +} + +func TestReleaseProber_SkipsDomainsTheProbeCannotSpeakFor(t *testing.T) { + q, _ := testQuarantineAt(time.Now()) + q.RecordFailure("*.enode.site", quarantineACME) + q.RecordFailure("dns.enode.site", quarantineACME) + q.RecordFailure("http.example.com", quarantineACME) + + recorder := newProbeRecorder() + prober := testReleaseProber(q, releaseProberConfig{ + Preflight: recorder.probe, + Unprobeable: func(domain string) bool { return domain == "dns.enode.site" }, + }) + prober.sweep() + + assert.Equal(t, []string{"http.example.com"}, recorder.calls(), + "a wildcard has no name to answer on, and a DNS-01 zone need not route here") + + // The unprobed holds stay: nothing disproved them. + assert.True(t, q.IsQuarantined("*.enode.site")) + assert.True(t, q.IsQuarantined("dns.enode.site")) + assert.False(t, q.IsQuarantined("http.example.com")) +} + +func TestReleaseProber_IgnoresEntriesWhoseHoldHasAlreadyExpired(t *testing.T) { + start := time.Now() + q, current := testQuarantineAt(start) + q.RecordFailure("app.example.com", quarantinePreflight) + + *current = start.Add(time.Hour) + require.False(t, q.IsQuarantined("app.example.com")) + + recorder := newProbeRecorder() + prober := testReleaseProber(q, releaseProberConfig{Preflight: recorder.probe}) + prober.sweep() + + assert.Empty(t, recorder.calls(), "nothing to release") +} + +func TestReleaseProber_NotifiesChangeOnlyWhenSomethingWasReleased(t *testing.T) { + q, _ := testQuarantineAt(time.Now()) + q.RecordFailure("app.example.com", quarantinePreflight) + + changes := 0 + prober := testReleaseProber(q, releaseProberConfig{ + Preflight: newProbeRecorder("app.example.com").probe, + OnChange: func() { changes++ }, + }) + prober.sweep() + assert.Zero(t, changes) + + prober = testReleaseProber(q, releaseProberConfig{OnChange: func() { changes++ }}) + prober.sweep() + assert.Equal(t, 1, changes) +} + +// Zero means "operator said nothing", which must land on the default rather +// than silently disabling the feature; only an explicit negative turns it off. +func TestReleaseProbeInterval_MapsTheConfiguredValue(t *testing.T) { + assert.Equal(t, DefaultReleaseProbeInterval, releaseProbeInterval(0)) + assert.Equal(t, 10*time.Second, releaseProbeInterval(10*time.Second)) + assert.Negative(t, releaseProbeInterval(-time.Second), "an explicit negative disables the loop") +} + +func TestReleaseProber_ZeroIntervalDisablesTheLoop(t *testing.T) { + q, _ := testQuarantineAt(time.Now()) + q.RecordFailure("app.example.com", quarantinePreflight) + + recorder := newProbeRecorder() + prober := testReleaseProber(q, releaseProberConfig{Interval: 0, Preflight: recorder.probe}) + + prober.Start() + t.Cleanup(prober.Stop) + time.Sleep(50 * time.Millisecond) + + assert.Empty(t, recorder.calls()) + assert.True(t, q.IsQuarantined("app.example.com")) +} + +func TestReleaseProber_StartSweepsOnItsInterval(t *testing.T) { + q, _ := testQuarantineAt(time.Now()) + q.RecordFailure("app.example.com", quarantinePreflight) + + prober := testReleaseProber(q, releaseProberConfig{Interval: 5 * time.Millisecond}) + prober.Start() + t.Cleanup(prober.Stop) + + require.Eventually(t, func() bool { + return !q.IsQuarantined("app.example.com") + }, time.Second, 5*time.Millisecond) +} diff --git a/internal/server/domain_renewal.go b/internal/server/domain_renewal.go index 463cc57..8009569 100644 --- a/internal/server/domain_renewal.go +++ b/internal/server/domain_renewal.go @@ -522,7 +522,7 @@ func (r *certRenewer) preflightMembers(domains []string) []string { probeable = append(probeable, domain) } - unreachable, failures := probeDomains(probeable, r.config.Preflight) + unreachable, failures := probeDomains(probeable, r.config.Preflight, r.manager.hasDNSProviderFor) for _, domain := range unreachable { backoff := r.quarantine.RecordFailure(domain, quarantinePreflight) slog.Warn("Renewal member failed pre-flight probe; holding back", @@ -593,7 +593,7 @@ func (r *certRenewer) handleRenewalFailure(cert *ManagedCert, domains []string, } } - failed := identifyFailedDomains(err, domains, probe) + failed := identifyFailedDomains(err, domains, probe, r.manager.hasDNSProviderFor) slog.Warn("Certificate renewal failed", "certificate", cert.Identifier, "domains", domains, "failed", failed, "error", err) diff --git a/internal/server/dynamic_domains.go b/internal/server/dynamic_domains.go index 16799ed..f0dc166 100644 --- a/internal/server/dynamic_domains.go +++ b/internal/server/dynamic_domains.go @@ -61,6 +61,11 @@ type DynamicDomainConfig struct { // SourceToken, when set, is sent as a bearer token with domain source // polls. SourceToken string + + // ReleaseProbeInterval is how often held domains are re-probed so a hold + // can be lifted as soon as the domain routes here again. Zero uses + // DefaultReleaseProbeInterval; negative disables release probing. + ReleaseProbeInterval time.Duration } // serviceResolver locates a deployed service; implemented by *Router. @@ -87,6 +92,7 @@ type DynamicDomainManager struct { quarantine *domainQuarantine issuer *domainIssuer renewer *certRenewer + releaser *releaseProber mu sync.Mutex sources map[string]*domainSource @@ -137,6 +143,14 @@ func NewDynamicDomainManager(config DynamicDomainConfig, manager *SANCertManager OnChange: dm.saveState, }) + dm.releaser = newReleaseProber(dm.quarantine, releaseProberConfig{ + Interval: releaseProbeInterval(config.ReleaseProbeInterval), + Preflight: dm.preflightProbe, + Unprobeable: manager.hasDNSProviderFor, + Released: dm.requestIssuanceAfterRelease, + OnChange: dm.saveState, + }) + manager.SetDynamicCertRequester(dm.issuer.Request) manager.SetIssuanceGuard(dm.preflightProbe, dm.quarantine, dm.saveState) @@ -145,14 +159,41 @@ func NewDynamicDomainManager(config DynamicDomainConfig, manager *SANCertManager return dm } -// Start launches the issuance worker and the renewal loop. +// releaseProbeInterval maps the configured value onto the prober's contract: +// zero means "unset, use the default", negative means the operator turned it +// off, and the prober treats any non-positive interval as disabled. +func releaseProbeInterval(configured time.Duration) time.Duration { + if configured == 0 { + return DefaultReleaseProbeInterval + } + return configured +} + +// requestIssuanceAfterRelease queues a freshly released domain for issuance if +// it still needs a certificate. +// +// Only dynamic domains are queued. A deploy-registered host is issued on its +// handshake, and that path re-probes the trigger itself, so lifting the hold +// is all it needs — routing the two through different mechanisms here would +// risk a background order racing a synchronous one for the same name. +func (dm *DynamicDomainManager) requestIssuanceAfterRelease(domain string) { + if dm.manager.HasValidCertificate(domain) { + return + } + if service, dynamic := dm.manager.dynamicOwner(domain); dynamic { + dm.issuer.Request(domain, service) + } +} + +// Start launches the issuance worker, the renewal loop, and the release prober. func (dm *DynamicDomainManager) Start() { dm.issuer.Start() dm.renewer.Start() + dm.releaser.Start() } -// Stop shuts down pollers, the issuance worker, and the renewal loop, and -// persists state. +// Stop shuts down pollers, the issuance worker, the renewal loop, and the +// release prober, and persists state. func (dm *DynamicDomainManager) Stop() { dm.mu.Lock() sources := make([]*domainSource, 0, len(dm.sources)) @@ -165,6 +206,7 @@ func (dm *DynamicDomainManager) Stop() { source.Stop() } + dm.releaser.Stop() dm.renewer.Stop() dm.issuer.Stop() dm.saveState() @@ -283,6 +325,41 @@ func (dm *DynamicDomainManager) RefreshAll() int { return len(sources) } +// Retry clears issuance holds and asks for the affected domains to be issued +// again, returning how many holds it cleared. An empty domain clears them all. +// +// This is the operator's escape hatch, and it is deliberately blunter than the +// release prober. It clears rate-limit holds, which the prober may never lift: +// an operator who knows the CA's window has passed should not have to wait on +// a retry time parsed out of an error string. It also wipes the failure +// history rather than only the hold, because asking for a retry is a claim +// that the underlying problem is fixed. The issuance rate limit still applies, +// so this cannot be used to hammer the CA. +func (dm *DynamicDomainManager) Retry(domain string) int { + held := []string{} + for candidate := range dm.quarantine.Snapshot() { + if domain == "" || candidate == domain { + held = append(held, candidate) + } + } + + for _, candidate := range held { + dm.quarantine.Clear(candidate) + } + + if len(held) == 0 { + return 0 + } + + slog.Info("Cleared issuance holds on request", "domains", held) + dm.saveState() + + for _, candidate := range held { + dm.requestIssuanceAfterRelease(candidate) + } + return len(held) +} + // HasSources reports whether any service has a domain source configured. func (dm *DynamicDomainManager) HasSources() bool { dm.mu.Lock() @@ -326,7 +403,11 @@ func (dm *DynamicDomainManager) Status() DomainsStatusResponse { quarantine := map[string]QuarantineStatus{} for domain, entry := range dm.quarantine.Snapshot() { - quarantine[domain] = QuarantineStatus(entry) + quarantine[domain] = QuarantineStatus{ + Until: entry.Until, + Failures: entry.Failures, + Kind: entry.Kind.String(), + } } return DomainsStatusResponse{ @@ -334,6 +415,7 @@ func (dm *DynamicDomainManager) Status() DomainsStatusResponse { QueueLength: dm.issuer.QueueLen(), Quarantine: quarantine, Certificates: len(dm.manager.ManagedCertificates()), + Registered: dm.manager.RegisteredDomains(), } } diff --git a/internal/server/dynamic_domains_test.go b/internal/server/dynamic_domains_test.go index 114b426..937eeb7 100644 --- a/internal/server/dynamic_domains_test.go +++ b/internal/server/dynamic_domains_test.go @@ -455,3 +455,50 @@ func TestDynamicDomainManager_LoadStateSkipsNilEntries(t *testing.T) { assert.NotContains(t, status.Services, "broken") assert.Contains(t, status.Services, "ok") } + +// The escape hatch: an operator who knows the cause is fixed should not have +// to wait on the ladder, and before this there was no way to clear a hold +// short of a proxy restart — which did not help either, since quarantine is +// persisted. +func TestDynamicDomainManager_RetryClearsAHoldAndResetsTheLadder(t *testing.T) { + dm, _ := testDynamicDomainManager(t, DynamicDomainConfig{}) + + dm.quarantine.RecordFailure("app.example.com", quarantineACME) + dm.quarantine.RecordFailure("app.example.com", quarantineACME) + require.True(t, dm.quarantine.IsQuarantined("app.example.com")) + + assert.Equal(t, 1, dm.Retry("app.example.com")) + assert.False(t, dm.quarantine.IsQuarantined("app.example.com")) + + // Explicit operator intent resets the history, unlike the prober's Release. + assert.Equal(t, 15*time.Minute, dm.quarantine.RecordFailure("app.example.com", quarantineACME)) +} + +// The release prober may never lift a rate-limit hold, but an operator who +// knows the CA's window has passed may. +func TestDynamicDomainManager_RetryClearsRateLimitHoldsToo(t *testing.T) { + dm, _ := testDynamicDomainManager(t, DynamicDomainConfig{}) + + dm.quarantine.RecordRateLimited("limited.example.com", time.Now().Add(4*time.Hour)) + require.True(t, dm.quarantine.IsQuarantined("limited.example.com")) + + assert.Equal(t, 1, dm.Retry("limited.example.com")) + assert.False(t, dm.quarantine.IsQuarantined("limited.example.com")) +} + +func TestDynamicDomainManager_RetryWithoutADomainClearsEveryHold(t *testing.T) { + dm, _ := testDynamicDomainManager(t, DynamicDomainConfig{}) + + dm.quarantine.RecordFailure("a.example.com", quarantineACME) + dm.quarantine.RecordFailure("b.example.com", quarantinePreflight) + dm.quarantine.RecordRateLimited("c.example.com", time.Now().Add(time.Hour)) + + assert.Equal(t, 3, dm.Retry("")) + assert.Zero(t, dm.quarantine.Len()) +} + +func TestDynamicDomainManager_RetryReportsNothingForAnUnheldDomain(t *testing.T) { + dm, _ := testDynamicDomainManager(t, DynamicDomainConfig{}) + + assert.Zero(t, dm.Retry("unknown.example.com")) +} diff --git a/internal/server/san_cert_batch_guard.go b/internal/server/san_cert_batch_guard.go index b1adad9..cf94286 100644 --- a/internal/server/san_cert_batch_guard.go +++ b/internal/server/san_cert_batch_guard.go @@ -3,6 +3,7 @@ package server import ( "log/slog" "slices" + "strings" ) // Guarding of the handshake-driven provisioning batch. @@ -45,6 +46,53 @@ func (m *SANCertManager) issuanceGuardSnapshot() issuanceGuard { return m.guard } +// preflightTrigger probes the domain whose handshake is asking for a +// certificate, before any order is assembled for it. Callers must NOT hold +// m.mu — the probe does network I/O. +// +// This is what makes preparing a DNS cutover in advance free. Until the domain +// resolves to this proxy an HTTP-01 order cannot succeed, and spending one +// anyway burns the CA's failed-authorization budget (Let's Encrypt allows five +// per hostname per hour); the rate-limit hold that follows is precisely what +// delays the certificate at the cutover itself. A probe answers the same +// question for nothing. +// +// The probe, not the quarantine, is the gate. That keeps the property the old +// unconditional pass existed to protect — a domain whose DNS was just fixed +// gets its certificate on the very next handshake rather than waiting out a +// stale ladder entry — while removing the order burn that a quarantine check +// could never have prevented, because the burn happens on the first attempt, +// before any hold exists. +// +// A wildcard has no name to answer on, and a domain in a zone with a DNS-01 +// provider does not need to route anywhere, so neither is probed. +func (m *SANCertManager) preflightTrigger(domain string) error { + guard := m.issuanceGuardSnapshot() + if guard.preflight == nil || guard.quarantine == nil { + return nil + } + if strings.HasPrefix(domain, "*.") || m.hasDNSProviderFor(domain) { + return nil + } + + if err := guard.preflight(domain); err != nil { + backoff := guard.quarantine.RecordFailure(domain, quarantinePreflight) + guard.notifyChange() + slog.Warn("Handshake domain failed pre-flight probe; refusing without spending an order", + "domain", domain, "backoff", backoff, "error", err) + return ErrCertNotFound + } + + // A passing probe is direct evidence that whatever the ladder is holding + // against is no longer true. Lift the hold but keep the failure count, so + // a domain that flaps keeps climbing instead of resetting. + if guard.quarantine.Release(domain) { + guard.notifyChange() + slog.Info("Pre-flight probe passed; lifting hold ahead of issuance", "domain", domain) + } + return nil +} + // filterBatchMates drops quarantined or unreachable domains from a handshake // batch, quarantining fresh probe failures. Every mate is probed, even one // that held a certificate before — an expiring host whose DNS moved away must @@ -70,7 +118,7 @@ func (m *SANCertManager) filterBatchMates(trigger string, domains []string) []st mates = append(mates, domain) } - unreachable, failures := probeDomains(mates, guard.preflight) + unreachable, failures := probeDomains(mates, guard.preflight, m.hasDNSProviderFor) for _, domain := range unreachable { backoff := guard.quarantine.RecordFailure(domain, quarantinePreflight) slog.Warn("Batch domain failed pre-flight probe; holding back", @@ -122,7 +170,7 @@ func (m *SANCertManager) attributeBatchFailure(err error, ordered, requested []s culprits := failedDomainsFromError(err, ordered) if len(culprits) == 0 { - culprits, _ = probeDomains(ordered, guard.preflight) + culprits, _ = probeDomains(ordered, guard.preflight, m.hasDNSProviderFor) } if len(culprits) == 0 { return requested diff --git a/internal/server/san_cert_batch_guard_test.go b/internal/server/san_cert_batch_guard_test.go index 8f61dfc..ff1dacf 100644 --- a/internal/server/san_cert_batch_guard_test.go +++ b/internal/server/san_cert_batch_guard_test.go @@ -78,6 +78,64 @@ func TestBatchGuard_UnreachableBatchMateIsQuarantinedWithoutBurningAnOrder(t *te assert.Contains(t, pendingDomainsOf(manager), "dead.example.com") } +// A domain whose order will be answered by DNS-01 does not have to route +// anywhere. Probing it over HTTP and holding it back when the probe fails is +// exactly backwards: DNS-01 is what makes issuing before a cutover safe. +func TestBatchGuard_DNSSolvableBatchMateIsNotProbed(t *testing.T) { + obtainer := successfulObtainer(t) + manager, quarantine := testGuardedManager(t, obtainer) + manager.dnsObtainers = map[acmeconfig.ProviderName]certObtainer{"cloudflare": obtainer} + manager.selection = acmeconfig.ProviderSelection{Zones: map[string]acmeconfig.ProviderName{"enode.site": "cloudflare"}} + + probed := []string{} + manager.SetIssuanceGuard(func(domain string) error { + probed = append(probed, domain) + return errors.New("does not route here") + }, quarantine, nil) + + require.NoError(t, manager.RegisterDomain("enode.site", "service1")) + require.NoError(t, manager.RegisterDomain("www.enode.site", "service1")) + + _, err := manager.provisionCertificate(context.Background(), "enode.site") + require.NoError(t, err) + + assert.Empty(t, probed, "a DNS-01 zone must never be HTTP-probed") + + calls := obtainer.Calls() + require.Len(t, calls, 1) + assert.ElementsMatch(t, []string{"enode.site", "www.enode.site"}, calls[0].Domains, + "the mate belongs in the order: DNS-01 can validate it wherever it points") + assert.False(t, quarantine.IsQuarantined("www.enode.site")) +} + +// The same manager, for a zone with no DNS provider, must keep probing. +func TestBatchGuard_HTTPOnlyBatchMateIsStillProbedAlongsideADNSZone(t *testing.T) { + obtainer := successfulObtainer(t) + manager, quarantine := testGuardedManager(t, obtainer) + manager.dnsObtainers = map[acmeconfig.ProviderName]certObtainer{"cloudflare": obtainer} + manager.selection = acmeconfig.ProviderSelection{Zones: map[string]acmeconfig.ProviderName{"enode.site": "cloudflare"}} + + probed := []string{} + manager.SetIssuanceGuard(func(domain string) error { + probed = append(probed, domain) + if domain == "dead.example.com" { + return errors.New("does not route here") + } + return nil + }, quarantine, nil) + + require.NoError(t, manager.RegisterDomain("app.example.com", "service1")) + require.NoError(t, manager.RegisterDomain("dead.example.com", "service1")) + + _, err := manager.provisionCertificate(context.Background(), "app.example.com") + require.NoError(t, err) + + // Both are HTTP-01, so both are probed: the trigger by the gate, the mate + // by the batch filter. Neither rides on the DNS-01 zone's exemption. + assert.ElementsMatch(t, []string{"app.example.com", "dead.example.com"}, probed) + assert.True(t, quarantine.IsQuarantined("dead.example.com")) +} + func TestBatchGuard_ExpiringBatchMateIsProbedAndExcludedWhenUnreachable(t *testing.T) { obtainer := successfulObtainer(t) manager, quarantine := testGuardedManager(t, obtainer) @@ -161,12 +219,15 @@ func TestBatchGuard_QuarantinedDomainsDoNotConsumeBatchSlots(t *testing.T) { assert.ElementsMatch(t, []string{"app.example.com", "ok.example.com"}, calls[0].Domains) } -func TestBatchGuard_TriggerDomainIsNeverDropped(t *testing.T) { +// The trigger is still never filtered out of its own batch — filterBatchMates +// only ever considers the mates. What changed is upstream of it: the trigger +// is probed before the order, so a stale hold no longer decides its fate and +// an unreachable one no longer spends an order. Here the probe passes, so a +// prior hold is irrelevant and the handshake gets its certificate. +func TestBatchGuard_TriggerDomainIsNeverDroppedFromItsOwnBatch(t *testing.T) { obtainer := successfulObtainer(t) manager, quarantine := testGuardedManager(t, obtainer) - // The probe fails everything, and the trigger is even quarantined — its - // handshake still gets its shot. - manager.SetIssuanceGuard(func(domain string) error { return errors.New("unreachable") }, quarantine, nil) + manager.SetIssuanceGuard(func(domain string) error { return nil }, quarantine, nil) quarantine.RecordFailure("app.example.com", quarantineACME) require.NoError(t, manager.RegisterDomain("app.example.com", "service1")) @@ -177,6 +238,92 @@ func TestBatchGuard_TriggerDomainIsNeverDropped(t *testing.T) { calls := obtainer.Calls() require.Len(t, calls, 1) assert.Equal(t, []string{"app.example.com"}, calls[0].Domains) + + // A passing probe is proof the hold was stale; it must not survive. + assert.False(t, quarantine.IsQuarantined("app.example.com")) +} + +// The defect this issue exists to fix: before a DNS cutover the domain still +// points at the old server, so an ACME order cannot succeed — but any +// handshake carrying that SNI spent one anyway. Five in an hour trips Let's +// Encrypt's failed-authorization limit, and the resulting hold is what delays +// the certificate at the actual cutover. +func TestBatchGuard_UnreachableTriggerIsRefusedWithoutBurningAnOrder(t *testing.T) { + obtainer := successfulObtainer(t) + manager, quarantine := testGuardedManager(t, obtainer) + manager.SetIssuanceGuard(func(domain string) error { + return errors.New("does not route here") + }, quarantine, nil) + + require.NoError(t, manager.RegisterDomain("app.example.com", "service1")) + + _, err := manager.provisionCertificate(context.Background(), "app.example.com") + require.ErrorIs(t, err, ErrCertNotFound) + + assert.Empty(t, obtainer.Calls(), "no ACME order may be spent on a domain that cannot answer") + + // Held on the gentle ladder: no order was spent, so the first retry is soon. + assert.True(t, quarantine.IsQuarantined("app.example.com")) + assert.Equal(t, quarantinePreflight, quarantine.Snapshot()["app.example.com"].Kind) +} + +// A held domain that starts routing here again gets its certificate on the +// very next handshake — the probe, not the ladder, is what governs. +func TestBatchGuard_ReachableTriggerProvisionsDespiteAStaleHold(t *testing.T) { + obtainer := successfulObtainer(t) + manager, quarantine := testGuardedManager(t, obtainer) + manager.SetIssuanceGuard(func(domain string) error { return nil }, quarantine, nil) + + // Four prior failures: deep on the ladder, held for 24 hours. + for range 4 { + quarantine.RecordFailure("app.example.com", quarantinePreflight) + } + require.True(t, quarantine.IsQuarantined("app.example.com")) + + require.NoError(t, manager.RegisterDomain("app.example.com", "service1")) + + _, err := manager.provisionCertificate(context.Background(), "app.example.com") + require.NoError(t, err) + assert.Len(t, obtainer.Calls(), 1) + assert.False(t, quarantine.IsQuarantined("app.example.com")) +} + +// A DNS-01 trigger is never probed: its order does not depend on where it +// points, which is the whole reason to issue before a cutover. +func TestBatchGuard_DNSSolvableTriggerIsNotProbed(t *testing.T) { + obtainer := successfulObtainer(t) + manager, quarantine := testGuardedManager(t, obtainer) + manager.dnsObtainers = map[acmeconfig.ProviderName]certObtainer{"cloudflare": obtainer} + manager.selection = acmeconfig.ProviderSelection{Zones: map[string]acmeconfig.ProviderName{"enode.site": "cloudflare"}} + + probed := 0 + manager.SetIssuanceGuard(func(domain string) error { + probed++ + return errors.New("does not route here") + }, quarantine, nil) + + require.NoError(t, manager.RegisterDomain("enode.site", "service1")) + + _, err := manager.provisionCertificate(context.Background(), "enode.site") + require.NoError(t, err) + + assert.Zero(t, probed) + assert.Len(t, obtainer.Calls(), 1) +} + +// With no guard installed there is nothing to probe with, and behaviour is +// exactly as it was before the gate existed. +func TestBatchGuard_TriggerIsUngatedWhenNoProbeIsInstalled(t *testing.T) { + obtainer := successfulObtainer(t) + manager, quarantine := testGuardedManager(t, obtainer) + manager.SetIssuanceGuard(nil, quarantine, nil) + quarantine.RecordFailure("app.example.com", quarantineACME) + + require.NoError(t, manager.RegisterDomain("app.example.com", "service1")) + + _, err := manager.provisionCertificate(context.Background(), "app.example.com") + require.NoError(t, err) + assert.Len(t, obtainer.Calls(), 1) } func TestBatchGuard_QuarantinesCulpritsAndRestoresSurvivorsOnFailure(t *testing.T) { diff --git a/internal/server/san_cert_dynamic.go b/internal/server/san_cert_dynamic.go index bfdd5a8..d8d4799 100644 --- a/internal/server/san_cert_dynamic.go +++ b/internal/server/san_cert_dynamic.go @@ -182,6 +182,30 @@ func (m *SANCertManager) certIDForDomain(domain string) string { } // ManagedCertificates returns a snapshot of the managed certificates. +// RegisteredDomains reports every deploy-registered host and the state of its +// certificate. Unlike the dynamic domain sets, these are not grouped by a +// domain source — an operator named them directly — so they need their own +// place in the status response. +func (m *SANCertManager) RegisteredDomains() map[string]RegisteredDomainStatus { + m.mu.RLock() + defer m.mu.RUnlock() + + registered := make(map[string]RegisteredDomainStatus, len(m.registeredDomains)) + for domain, service := range m.registeredDomains { + status := RegisteredDomainStatus{Service: service} + + if certID := m.certIDCovering(domain); certID != "" { + if cert := m.certificates[certID]; cert != nil && cert.Certificate != nil { + status.Certified = true + status.ExpiresAt = cert.NotAfter + } + } + + registered[domain] = status + } + return registered +} + func (m *SANCertManager) ManagedCertificates() []*ManagedCert { m.mu.RLock() defer m.mu.RUnlock() diff --git a/internal/server/san_cert_manager.go b/internal/server/san_cert_manager.go index f949bf1..67e921a 100644 --- a/internal/server/san_cert_manager.go +++ b/internal/server/san_cert_manager.go @@ -523,6 +523,12 @@ func (m *SANCertManager) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certif // has already been consulted. The rate limit has not, and this path is // handshake-driven, so it takes a token before ordering. func (m *SANCertManager) provisionCertificate(ctx context.Context, domain string) (*tls.Certificate, error) { + // Before the lock and before the order: the probe does network I/O, and a + // domain that cannot answer must cost nothing. + if err := m.preflightTrigger(domain); err != nil { + return nil, err + } + m.mu.Lock() // One in-flight batch per service. The single-flight exists to stop the // SAME batch being ordered twice by concurrent handshakes; batches never diff --git a/internal/server/san_cert_manager_test.go b/internal/server/san_cert_manager_test.go index 736cc5d..fca68db 100644 --- a/internal/server/san_cert_manager_test.go +++ b/internal/server/san_cert_manager_test.go @@ -642,3 +642,30 @@ func TestSANCertManager_GetCertificate_WaiterRefusesExpiredCert(t *testing.T) { require.ErrorIs(t, r.err, ErrCertNotFound) assert.Nil(t, r.cert) } + +// Deploy-registered hosts are the common case and the one in every DNS +// cutover, but the status surface only ever reported services with a domain +// source — so the hosts an operator actually named in `deploy --host` were +// invisible while their issuance was held. +func TestSANCertManager_RegisteredDomainsReportsCertificateState(t *testing.T) { + obtainer := successfulObtainer(t) + manager, quarantine := testGuardedManager(t, obtainer) + manager.SetIssuanceGuard(func(string) error { return nil }, quarantine, nil) + + require.NoError(t, manager.RegisterDomain("app.example.com", "service1")) + require.NoError(t, manager.RegisterDomain("pending.example.com", "service2")) + + _, err := manager.provisionCertificate(context.Background(), "app.example.com") + require.NoError(t, err) + + registered := manager.RegisteredDomains() + require.Len(t, registered, 2) + + assert.Equal(t, "service1", registered["app.example.com"].Service) + assert.True(t, registered["app.example.com"].Certified) + assert.False(t, registered["app.example.com"].ExpiresAt.IsZero()) + + assert.Equal(t, "service2", registered["pending.example.com"].Service) + assert.False(t, registered["pending.example.com"].Certified) + assert.True(t, registered["pending.example.com"].ExpiresAt.IsZero()) +}