Skip to content

v1.0.0.1 release gate: unify the two ACME systems, close unbounded SNI cert issuance #84

Description

@mhenrixon

v1.0.0.1 release gate: unify the two ACME systems, close unbounded SNI cert issuance

Part of the dash v3.0 release audit (2026-07-30, 18-reviewer pre-release sweep of every merged PR in both repos). The gem-side gate issue in mhenrixon/kamal links back here. The gem's MINIMUM_VERSION will move to v1.0.0.1; this image must be published before the gem ships (release ordering per .claude/rules/upstream-sync.md).

Problem / Goal

The published v1.0.0.0 image has a real security hole: CertificateRegistry provisions Let's Encrypt certificates for any SNI presented at the TLS handshake, with no domain allowlist and no rate limit. Verified end to end on origin/dash:

  • internal/server/router.go Router.GetCertificate consults r.certRegistry.GetCertificate(hello) (~line 565) before checking whether any service matches the SNI at all. The on-demand-TLS gate added in PR fix(router): let on-demand TLS gate issuance ahead of the cert registry #50 only protects services with an explicit --tls-on-demand-url; every other hostname falls into the registry branch.
  • cert_registry.go GetCertificate (~246–285) has no allowlist: an uncached domain goes straight to provisionCertificate → DNS-01 via r.dnsSolver, or provisionWithHTTPr.httpFallback, an autocert.Manager built in Initialize() (~147–153) with no HostPolicy — the exact attack autocert's own docs warn about. The HTTP-01 challenge handler serves any token with no domain check either.
  • The only issuance guard is a per-root-domain in-flight lock (dedupes concurrent orders for the same domain); there is no ceiling on distinct domains over time. SANCertManager's path has all of this (domainIssuer token bucket ~270 orders/3h, max 3 concurrent orders, hard allowlist).
  • Attack: point any domain's A record at the proxy's public IP, send a ClientHello with that SNI → real LE issuance. Reachable on every deployment with --acme-email set, since ACMEHTTPFallback defaults true.

Beyond the hole, the audit confirmed the deeper wrongness: two independent ACME systems answer "automatic TLS"CertificateRegistry/CertificateRenewalManager/RegistryCertManager vs the hardened SANCertManager + domainIssuer + certRenewer stack — with two separate ACME accounts (acme_user.json vs the registry's own DNS-solver/autocert account) sharing one CertificatePath() directory. NewRegistryCertManager has no production callers. PR #42's own Deviations section already flagged folding DNS-01/wildcard into SANCertManager as a deferred follow-up.

Decision (settled with the maintainer, 2026-07-30): unify now, not patch-in-place. Delete the registry stack; SANCertManager becomes the single cert system, absorbing DNS-01/wildcard. Release as v1.0.0.1.

Context (read these first)

  • internal/server/router.goGetCertificate branch order: on-demand → registry → SAN stack. The end state is on-demand → SAN stack (single system) → nil-service rejection.
  • internal/server/cert_registry.go, cert_renewal.go, registry_cert_manager.go — the stack to delete.
  • internal/server/san_cert_manager.go, domain_issuer.go — the surviving stack: allowlist gate, token-bucket rate limit, single ACME account (loadOrCreateUser/saveUser<CachePath>/acme_user.json).
  • internal/server/tls_on_demand.go — accepts only exactly http.StatusOK; the gem's docs promise "answer 2xx to approve". Pick one side (recommendation: widen the code to any 2xx, matching the published gem docs).
  • internal/cmd/run.goCertificateRenewalManager start/stop lifecycle (correct today; the replacement must keep the LIFO stop-before-listener-close ordering).
  • PR chore: sync with basecamp/kamal-proxy main (on-demand TLS, client-IP header, Go 1.26.5) #42 (upstream sync) Deviations section — the unification was already named there as the intended follow-up.
  • PR feat(cache): keep the cache on disk, so it outlives the proxy #83 (disk-persistent cache) is on dash but not in the published v1.0.0.0 image (tag points at PR feat(cache): key varying responses on what they vary on #81's commit 90f7134). It rides along in v1.0.0.1 — this closes the "no published image contains feat(cache): keep the cache on disk, so it outlives the proxy #83" blind spot. Do not retag v1.0.0.0.

Implementation steps

  1. Fold DNS-01/wildcard issuance into the SANCertManager/domainIssuer stack: DNS solver becomes an issuance strategy behind the existing allowlist + rate-limit gate; prefer_wildcard/http_fallback semantics preserved. Any autocert fallback gets an explicit HostPolicy bound to the allowed domain set.
  2. Delete cert_registry.go, cert_renewal.go, registry_cert_manager.go and their wiring in router.go/run.go. One ACME account (acme_user.json); migrate/ignore old registry state files gracefully (a proxy rebooting from v1.0.0.0 state must come up clean).
  3. router.go GetCertificate final order: per-service on-demand gate → unified SAN stack (allowlisted domains: static hosts, dynamic tls_domains source, wildcard/DNS-01) → refuse.
  4. Widen tls_on_demand.go approval to any 2xx (or, if rejected, change the gem docs — decide once, both sides must agree).
  5. Verify the ACME cache default lands inside the persistent volume mount (/home/kamal-proxy/.config/kamal-proxy) with no operator-visible path knob needed — the gem is cutting tls.acme_cache_path on the strength of this.
  6. Tests: unit tests proving (a) an SNI outside the allowlist provisions nothing and costs no ACME order, (b) HTTP-01 challenge handler refuses tokens for non-allowlisted domains, (c) issuance rate limiting applies to the DNS-01 path, (d) renewal covers certs from both prior systems' cache layouts.
  7. Release v1.0.0.1: script/release-dash v1.0.0.1, then docker buildx imagetools inspect ghcr.io/mhenrixon/kamal-proxy:v1.0.0.1amd64 AND arm64 present, org.opencontainers.image.title=kamal-proxy label intact.

Verification gates

  • go test ./... green; make bench on the routing/cert lookup hot paths — no regression vs dash baseline (same machine, both numbers reported).
  • Manual: boot with --acme-email, present a foreign SNI → no issuance attempt in logs, connection refused cleanly.
  • The gem repo's bin/sync-proxy-flags regenerated manifest at v1.0.0.1 shows no unexpected flag drift (the flag canary in mhenrixon/kamal fails the build otherwise).

Out of scope

  • No retag or mutation of the published v1.0.0.0 image.
  • No new flag surface (this is a consolidation; internal/cmd flag set stays put apart from anything the deletion strictly removes — coordinate with the gem's flag canary if a flag dies).
  • No plain v* upstream-colliding tags beyond the fork grammar; push the single tag only (git push origin tag v1.0.0.1).

Execution

Hand to a fresh implementation session (sonnet tier) in ../kamal-proxy on a branch off dash. The gem-side gate (mhenrixon/kamal, cross-linked) starts its MINIMUM_VERSION bump only after step 7 publishes.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions