fix(router): let on-demand TLS gate issuance ahead of the cert registry - #50
Merged
Merged
Conversation
Upstream shipped on-demand TLS in basecamp#225, which the July 26 sync merged into dash. That merge reconciled it with the SAN cert manager and with tls-domains-source, but not with the fork's certificate registry. Router.GetCertificate consulted the registry first, and the registry provisions on lookup -- it has no host policy. So on a proxy running with the registry enabled, any SNI reaching a service deployed with --tls-on-demand-url started an ACME order before the ask endpoint was consulted. The gate the feature exists to provide was bypassed, and attacker-chosen SNI could drive unbounded orders against the ACME account. Resolve the service first and hand off to its cert manager when it carries an on-demand URL, so the endpoint decides issuance for every host that service catches. Hosts routed to a host-scoped service still go to the registry. This mirrors createCertManager, where an explicit --tls-on-demand-url already wins over the proxy-wide SAN manager. Refs #13
This was referenced Jul 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Issue #13 asked us to port basecamp#63 (on-demand TLS). Upstream got there first: PR basecamp#225 landed on 2026-07-16 as an explicit port of #63, upstream closed #63 as superseded, and our July 26 sync merge (
0f93ce3) brought it intodashbyte-for-byte.git diff upstream/main dash -- internal/server/tls_on_demand.gois empty.So this PR is not the implementation — it is the verification #13 still needed, plus the one real gap it turned up.
The gap
That sync merge reconciled on-demand TLS with two of the fork's three cert subsystems:
service.go:584— an explicit--tls-on-demand-urlwins over the proxy-wide SAN managerservice_options_validation.go:36—tls-domains-sourceandtls-on-demand-urlare rejected togetherIt did not reconcile it with the certificate registry.
Router.GetCertificateconsultedcertRegistryfirst (router.go:475), andCertificateRegistry.GetCertificateprovisions on lookup (cert_registry.go:284→provisionCertificate) — it has no host policy at all.On a proxy running with the registry enabled (
--acme-email+ a DNS provider) and a service deployed with--tls-on-demand-url, every SNI reaching the listener started an ACME order before the ask endpoint was consulted. The gate the feature exists to provide was bypassed, and attacker-chosen SNI could drive unbounded orders against the ACME account.The fix
Resolve the service before the registry, and when it carries an on-demand URL hand off to its cert manager directly. Hosts routed to a host-scoped service still go through the registry unchanged.
This mirrors the precedent already set in
createCertManager.Test plan
TestRouter_GetCertificate_OnDemandServiceGatesRegistryProvisioning— the registry drains pending domains under a root as soon as it begins provisioning, so a seeded sibling surviving proves it never started. Fails ondash, passes here.TestRouter_GetCertificate_RegistryStillServesHostScopedServices— an on-demand catch-all must not disable the registry wholesale.TestRouter_DeployService_CatchAllTLSServicesConflict— dynamic domains and on-demand TLS both bind the host-less catch-all; nothing downstream breaks the tie, so they must collide at deploy. Already correct; now covered.make testgreen (752 tests ininternal/server)go test -race ./internal/server/cleangofmt -l internal/ cmd/clean,go vet ./...cleanPerformance
Router.GetCertificateruns on every TLS handshake, and the fix moves a service-map lookup ahead of the registry, so registry-served handshakes now pay for it. No benchmark existed for this path;BenchmarkRouter_GetCertificateis added here and was run on both sides, same machine (M2 Max),-count 5:dashbaselineAbout +20 ns/op, no new allocations. That is proxy-level cost only — it is noise against the microseconds-to-milliseconds of the handshake it sits inside, and it buys back an unbounded ACME order on the same path. Handshakes that were already falling through to a per-service cert manager pay nothing new; that lookup was always on their path.
Refs #13
Deviations & judgment calls
CertificateRegistryrather than the PR's standalone path." Upstream shipped it instead, using anautocert.HostPolicyon the per-service cert manager, anddashalready had it. I did not re-implement it against the registry — upstream's approach is in production and reverting to the issue's design would be a rewrite with no user-visible gain. The registry integration became a precedence question, which is what this PR answers.CertificateRegistryits own host-policy hook. That is a larger change to fork-only code with no upstream safety net, and it would duplicate the gate that already exists in the on-demand checker. Skipping the registry for on-demand-served hosts is narrower and matches the existingcreateCertManagerprecedent. The tradeoff: the registry is now unreachable for any host an on-demand catch-all catches. That is correct — those hosts are precisely the ones the ask endpoint should decide — but it is a real behavior change worth knowing about.NewCertificateRegistry+ready = truewithoutInitialize, which leaves both solvers nil — a ready registry that provisions with no network. The pending-domain drain is used as the provisioning-attempt observable rather than adding a counter to production code. Existingcert_registry_test.goalready setsreadydirectly, so this follows house practice.defaultServiceOptionsleavesACMECachePathempty, soScopedCachePath()resolves to a sha256-named directory relative to the working directory — deploying a TLS service in a test writes into the repo. The new tests setACMECachePathto at.TempDir(). Pre-existing tests (e.g.TestRouter_RestoreLastSavedState_TLSOnDemandURL) still have this; not fixed here to keep the diff scoped, but worth a follow-up.ClientCAsinupstream/main) and lands next, on top of this.