feat(service): require client certificates with --tls-client-ca-path - #53
Merged
Conversation
Ports basecamp#204, which has been open upstream since April. Enables Cloudflare Authenticated Origin Pulls: with a CA bundle configured, only clients holding a certificate that chains to it can complete the handshake, so nobody can reach the origin by pointing at its IP directly. The requirement is per-service and resolved per-connection through GetConfigForClient, so services on one proxy can have different client certificate rules. Two departures from basecamp#204, both load-bearing: Its GetConfigForClient builds a fresh tls.Config holding only GetCertificate, ClientAuth and ClientCAs. Go replaces the connection's config with whatever that returns, so an mTLS host would lose NextProtos and MinVersion -- silently downgrading to HTTP/1.1 and breaking tls-alpn-01 challenges. Clone the listener's config and set only the two client-auth fields instead. Its lookup is guarded by hello.ServerName != "". Dropped: an empty SNI resolves to the catch-all service, which is the service that would serve the connection, so its client CA should apply. The CA bundle is read during deploy rather than at handshake time, so a bad path fails the deploy instead of bringing up a service that serves without the client verification it asked for. Refs #12
6 tasks
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
Ports basecamp#204 (open upstream since April 2026, unmerged — there is no
ClientCAs/ClientAuthanywhere inupstream/main). Enables Cloudflare Authenticated Origin Pulls: with a CA bundle configured, only clients holding a certificate that chains to it complete the handshake, so nobody reaches the origin by pointing at its IP.The requirement is per-service, resolved per-connection via
GetConfigForClient, so services on one proxy can have different client certificate rules.Wiring:
--tls-client-ca-path(internal/cmd/deploy.go) →ServiceOptions.TLSClientCACertificatePath→Service.clientCAsloaded ininitialize(internal/server/service.go) →Router.clientCAsForHost(internal/server/router.go) →Server.clientCertificateConfigon both the HTTPS and HTTP/3 listeners (internal/server/server.go). Newinternal/server/client_ca.goholds the PEM loading, keeping the fork-only surface localized.Two departures from PR basecamp#204, both load-bearing
1. The upstream PR would have broken HTTP/2 on every mTLS host. Its
createGetConfigForClientreturns a freshtls.Configcarrying onlyGetCertificate,ClientAuthandClientCAs. Go replaces the connection's config with whatever that returns, so an mTLS host losesNextProtos(h2,http/1.1,acme-tls/1) andMinVersion— silently downgrading to HTTP/1.1 and breaking tls-alpn-01 challenges. This clones the listener's config and sets only the two client-auth fields.TestServer_MutualTLS/still_negotiates_HTTP/2is the regression guard.2. Dropped its
hello.ServerName != ""guard. An empty SNI resolves throughserviceForHost("")to the catch-all service — the service that would actually serve that connection — so its client CA should apply.Also: the CA bundle is read during deploy, not at handshake time. A bad path fails the deploy rather than bringing up a service that serves without the client verification the operator asked for.
Test plan
TestServer_MutualTLS— end-to-end through a real TLS listener: rejects no certificate, rejects a certificate from a different CA, accepts one from the configured CA, and still negotiates HTTP/2TestServer_WithoutMutualTLSClientCertificatesAreNotRequired— no--tls-client-ca-pathmeans nothing about the handshake changesTestRouter_ClientCAsForHost— pool for an mTLS host, nil for a plain service, nil for an unclaimed hostTestRouter_ClientCAsForHost_OnDemandCatchAllCoversUnclaimedHosts— pins the catch-all consequence (see below)TestServiceOptions_ClientCARequiresTLS— a CA path without--tlsis rejected rather than silently ignoredTestService_UnloadableClientCAFailsDeploy— a missing bundle fails the deployTestService_ClientCASurvivesStateRoundTrip— new field round-trips; state files predating it restore with mTLS offTestLoadClientCAs— missing file and no-PEM-found both name the path (the CLI only sees this string overnet/rpc)make testgreen;go test -race ./internal/server/ ./internal/cmd/clean (902 tests);gofmt -l internal/ cmd/andgo vet ./...cleanPerformance
GetConfigForClientis now invoked on every TLS handshake, not only mTLS ones, so the nil path matters as much as the clone.BenchmarkServer_ClientCertificateConfigis added; M2 Max,-count 3:No baseline column: neither path exists on
dash, so these are the costs added, not a delta. Both are proxy-level and negligible against the handshake they sit inside — a TLS handshake is microseconds to milliseconds of asymmetric crypto and a round trip. The 480 B clone is paid only by hosts that actually require client certificates.Refs #12
Deviations & judgment calls
createGetConfigForClientverbatim. See departure 1 above — it drops ALPN andMinVersion. This is the single most important thing to check in review; if you disagree with the clone approach, the alternative is enumerating the fields to copy, which rots the moment anyone adds one to the listener config.hello.ServerName != ""guard. See departure 2. Behavior difference from Add mutual TLS (mTLS) support basecamp/kamal-proxy#204: a no-SNI connection to a catch-all mTLS service now requires a client certificate. I believe that is correct, but it is a deliberate divergence.--hostand a client CA;Validate()rejects it with "host must be set when using TLS". Only--tls-on-demand-urlor--tls-domains-sourcecan hold the catch-all binding with TLS on. Rewrote the test against the on-demand shape — the only reachable mTLS catch-all — and documented the consequence in the README: such a service's client CA governs every hostname no other service claims, including ones never named on the command line. Worth a second opinion; it is the surprising part of this change.initialize's error path, not theresolve*group.Servicealready has aresolveBasicAuth/resolveIPAllowList/resolveRateLimiterpattern for derived per-service state, which would have been the natural home — but those cannot fail. Silently skipping an unloadable CA would leave an origin open that the operator believes is locked down, so it sits next tocreateCertManagerwhere it can fail the deploy.TLSClientCACertificatePath, matching Add mutual TLS (mTLS) support basecamp/kamal-proxy#204 exactly even though it is long, so that if upstream ever merges Add mutual TLS (mTLS) support basecamp/kamal-proxy#204 the conflict is as small as possible. The flag is--tls-client-ca-path, which matches both Add mutual TLS (mTLS) support basecamp/kamal-proxy#204 and issue R3: mTLS client certs (--tls-client-ca-path) #12 — the gem-facing contract is identical either way.GetConfigForClienttoo, as Add mutual TLS (mTLS) support basecamp/kamal-proxy#204 did, but is untested here. An mTLS-over-QUIC test needs an h3 client that presents certificates, and HTTP/3 is off by default. Flagging rather than silently skipping: if you want mTLS over h3 to be a supported claim, it needs a test I have not written.../kamal, not attempted here.