feat: NATS mTLS pubsub implementation - #26902
Conversation
Docs preview📖 View docs preview for |
cc97307 to
6d28412
Compare
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 2 | Last posted: Round 2, 10 findings (1 P1, 1 P2, 4 P3, 4 Nit), COMMENT. Review Finding inventoryFindings
Law analysisEffective LOC: 1448 (722 prod, 726 test, 13 generated). Head SHA: 6d28412. Verdict: Don't split. Enforcement: Advisory. The PR is one reviewable idea (NATS cluster mTLS), all concerns serve it, and no independent risk domains or unrelated work is bundled. Round logRound 1Netero-only. 1 P1. Reviewed against c40b3b9..6d28412. Round 2Panel (18 reviewers). CRF-1 addressed. 1 P2, 4 P3, 5 Nit new. 7 dropped (P4/Nit below threshold). Reviewed against a73e677..5e5174e. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
First-pass review (Netero). 1 P1 finding. The full review panel has not yet reviewed this PR; it will review after this finding is addressed.
The mTLS design is well-structured overall: CA generation, leaf minting, and verification callbacks are cleanly separated. Test coverage is thorough with a ~1:1 test-to-production ratio.
However, there is a P1 that can break all key rotation when the NATS experiment is toggled off after having been on.
"Every 10-minute rotation tick fails with 'keys by feature: unknown feature: nats_ca'. The initial rotateKeys call in StartRotator also fails." (Netero)
coderd/cryptokeys/rotate.go:328
P1 [CRF-1] keysByFeature rejects DB keys for features outside the rotator's managed set, breaking ALL key rotation when the NATS experiment is toggled off after having been on. (Netero)
GetCryptoKeys returns every feature's rows. keysByFeature errors if any key's feature is absent from the features map. Before this PR, the rotator always ran with the full feature set, so no mismatch was possible. Now WithFeatures allows a subset (excluding nats_ca when the experiment is off), but once nats_ca rows exist in the DB from a prior experiment-on run, every rotation tick aborts:
rotateKeys
-> tx.GetCryptoKeys(ctx) // returns ALL features
-> keysByFeature(keys, k.features)
for _, key := range keys {
if _, ok := m[key.Feature]; !ok {
return nil, xerrors.Errorf("unknown feature: %s", key.Feature) // breaks here
}
}
This means workspace apps, OIDC convert, and tailnet resume keys all stop rotating.
Fix: keysByFeature should skip (not error on) keys for features outside the managed set. They belong to other features the rotator is not responsible for.
🤖
🤖 This review was automatically generated with Coder Agents.
179b894 to
5e5174e
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Panel review (R2, 18 reviewers). CRF-1 (P1) addressed. 1 P2, 4 P3, 5 Nit new.
The mTLS design is structurally sound. Lock ordering is strict (clusterTLS.mu -> cache.mu, never reversed). The noop/real cache swap pattern cleanly separates the enterprise enable path from the boot path. The CRF-1 fix generalizes correctly, protecting any future gated feature, not just nats_ca. Test coverage is thorough across unit, behavioral, and integration layers. Error messages name the failing operation and wrap the cause consistently. The two-commit split is clean.
Seven reviewers independently flagged the NATSCALeafValidity naming/invariant issue, the strongest signal from the panel. The leaf-minting error invisibility is an operational gap worth closing before this ships.
"An operator whose CA cache is broken would see zero routes forming with zero diagnostic output. The only recovery path is noticing the rotator's Critical log, which is about the rotator, not about why cluster routes aren't forming." (Chopper)
🤖 This review was automatically generated with Coder Agents.
| func (t *clusterTLS) tlsConfig() *tls.Config { | ||
| return &tls.Config{ | ||
| MinVersion: tls.VersionTLS13, | ||
| GetCertificate: func(*tls.ClientHelloInfo) (*tls.Certificate, error) { return t.currentLeaf() }, |
There was a problem hiding this comment.
P2 [CRF-2] currentLeaf() errors are invisible to operators. (Chopper)
GetCertificate and GetClientCertificate call currentLeaf() on every route handshake. When it fails (CA cache error, wrong key type, mint failure), the error returns to Go's TLS stack, which sends a generic alert and drops the connection. The embedded NATS server runs with NoLog, so it never surfaces the cause.
The verify path has verifyLogged (line 164) which wraps rejections with a Debug log. The currentLeaf path has no equivalent. An operator whose CA cache is broken sees zero routes forming with zero diagnostic output. The "minted nats cluster leaf" log at line 230 fires on success, not failure.
A currentLeafLogged wrapper parallel to verifyLogged would close this gap.
🤖
| // NATSCALeafValidity is the maximum lifetime of a leaf certificate | ||
| // minted under the NATS cluster CA. Old CA rows must remain valid trust | ||
| // roots for this long after rotation so that replicas holding leaves | ||
| // signed by the old CA can still be verified. | ||
| NATSCALeafValidity = time.Hour * 24 * 30 |
There was a problem hiding this comment.
P3 [CRF-3] NATSCALeafValidity name and doc say "maximum lifetime of a leaf certificate," but the actual leaf lifetime is leafCertValidity = 24h in coderd/x/nats/tls.go:29. This constant (30 days) is a CA retention budget, not a leaf lifetime. (Hisoka P3, Mafuuu P3, Gon P3, Leorio P3, Knov P3, Ryosuke P3, Kite P3)
Seven reviewers independently flagged this. The invariant leafCertValidity <= NATSCALeafValidity must hold, or old CAs get deleted while in-flight leaves still chain to them, breaking verification during rotation overlap. Today the 30x headroom absorbs any mistake, but the constants live in separate packages with no compile-time, init-time, or runtime check linking them.
A developer reading "maximum lifetime of a leaf" would either shrink NATSCALeafValidity to 24h to match the actual leaf, or raise leafCertValidity to 30d to match this constant. Either breaks the system.
Fix: rename to something like NATSCAKeyRetention, update the godoc to describe what it actually controls (CA retention window, not leaf lifetime), and cross-reference leafCertValidity. A compile-time assertion would make the invariant mechanical rather than documentary.
🤖
| // the IP SAN peers verify, so without an IP-based relay URL the cache is left | ||
| // as the boot-time noop and routes stay plaintext (token auth only). The CA is | ||
| // read lazily by the TLS callbacks on each handshake, so nothing reads it here. | ||
| func (api *API) configureNATSClusterTLS(natsPubsub *nats.Pubsub) { |
There was a problem hiding this comment.
P3 [CRF-4] Silent return when relay URL is nil or empty. When the hostname is not an IP, line 898 logs a Warn. When the relay URL is absent, this branch returns without any log. (Mafuuu)
An operator who enables HA without setting CODER_DERP_SERVER_RELAY_URL gets no indication that cluster routes are running on token auth only, no mTLS. Debugging cluster security requires knowing that mTLS depends on the DERP relay URL, a non-obvious coupling.
Fix: log at Warn level before the early return, matching the pattern on line 898.
🤖
| func generateCACryptoKeySecret() (string, error) { | ||
| key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) | ||
| if err != nil { | ||
| return "", xerrors.Errorf("generate key: %w", err) | ||
| } | ||
| template := &x509.Certificate{ | ||
| SerialNumber: big.NewInt(1), | ||
| Subject: pkix.Name{CommonName: "dbgen-ca"}, |
There was a problem hiding this comment.
P3 [CRF-5] generateCACryptoKeySecret near-duplicates generateCASecret at coderd/cryptokeys/ca.go:52. (Robin)
~28 lines of identical flow: generate P256 key, build self-signed CA template, CreateCertificate, MarshalECPrivateKey, PEM-encode. If the bundle format changes (key type, PEM block labels, or the post-parse validation in parseCASecret that rejects non-CA certs), the dbgen copy won't track.
cryptokeys does not import dbgen, so exporting generateCASecret (or a GenerateTestCASecret wrapper with sensible defaults) and calling it from dbgen eliminates the copy with no import cycle.
🤖
| func (t *clusterTLS) verifyLogged(cs tls.ConnectionState, sourceIP net.IP) error { | ||
| err := t.verify(cs, sourceIP) | ||
| if err != nil { | ||
| t.logger.Debug(t.ctx, "rejected nats cluster peer certificate", slog.Error(err)) |
There was a problem hiding this comment.
P3 [CRF-6] Rejected peer certificates are logged at Debug, invisible at production log levels. The comment at line 161 states its purpose: "logging here gives deployments a way to see why a cluster peer was rejected." At the default production log level (Info or Warn), this purpose is not achieved. (Chopper)
The PR description flags this as an open question: "should we use a metric instead?" The noise concern during rotation overlap is valid. A rate-limited Warn or a counter metric would preserve the operator signal without saturating logs during expected transient rejections.
🤖
| CommonName: "coder-nats-cluster-leaf", | ||
| // SerialNumber carries the sequence of the CA that signed this | ||
| // leaf, letting a verifier fetch exactly that CA from its cache. | ||
| SerialNumber: strconv.FormatInt(int64(ca.Sequence), 10), |
There was a problem hiding this comment.
Nit [CRF-12] The leaf's Subject.SerialNumber carries the signing CA's crypto_keys sequence, which is a different entity's identifier. RFC 5280 section 4.1.2.6 defines this field for the entity itself. (Mafuuu)
The PR description flags this as an open question. A custom X.509 extension (OID under coder's arc) would be semantically correct and avoids confusion when PKI tooling or log indexers interpret this field as an entity identifier. The current approach is functional and safe (trust comes from chain verification, not the stamped sequence), so this is a convention/semantics decision for stabilization.
🤖
| } | ||
|
|
||
| // leafHasIP reports whether the leaf carries ip as an IP SAN. | ||
| func leafHasIP(leaf *x509.Certificate, ip net.IP) bool { |
There was a problem hiding this comment.
Nit [CRF-13] leafHasIP is a manual contains check; slices.ContainsFunc is the project pattern (dozens of call sites). (Ging-Go)
func leafHasIP(leaf *x509.Certificate, ip net.IP) bool {
return slices.ContainsFunc(leaf.IPAddresses, ip.Equal)
}🤖
| if experiments.Enabled(codersdk.ExperimentNATSPubsub) { | ||
| options.NATSCACache, err = cryptokeys.NewSigningCache(ctx, options.Logger.Named("nats_ca_cache"), &cryptokeys.DBFetcher{DB: options.Database}, codersdk.CryptoKeyFeatureNATSCA) | ||
| if err != nil { | ||
| options.Logger.Fatal(ctx, "failed to properly instantiate NATS CA cache", slog.Error(err)) |
There was a problem hiding this comment.
Nit [CRF-15] "properly" is noise. (Leorio)
options.Logger.Fatal(ctx, "failed to instantiate NATS CA cache", slog.Error(err))🤖
| // Leaves carry both ServerAuth and ClientAuth, since each replica is both a | ||
| // route server and client. Requiring those specific usages rejects a leaf | ||
| // with some unexpected EKU rather than accepting any usage. | ||
| if _, err := leaf.Verify(x509.VerifyOptions{ |
There was a problem hiding this comment.
Nit [CRF-16] x509.VerifyOptions does not set CurrentTime, so Go defaults to time.Now(). Every other time-sensitive operation in clusterTLS uses t.clock.Now(). (Knov)
In production both are real time. In tests with a mock clock, a test that advances the clock past a CA's NotAfter would see pool pruning fire correctly but x509.Verify would still accept the leaf. Setting CurrentTime: t.clock.Now() closes the gap for future test authors.
🤖
Add a nats_ca crypto-key feature holding the NATS cluster mTLS CA: a PEM cert+key bundle minted by the key rotator (generateCASecret/parseCASecret), sized so an old CA stays a valid trust root for the maximum leaf lifetime after rotation. The CA is served through the generic cryptokeys signing cache rather than a bespoke cache: idSecret decodes the PEM bundle into a *NATSCA for nats_ca (hex bytes otherwise), so SigningKey returns the active CA and VerifyingKey returns a specific CA by sequence, reusing the cache's fetch/refresh/rotation logic. The feature is experiment-gated. The rotator only mints nats_ca when opted in via WithFeatures (default rotation excludes it), and coderd.New opts it in and builds a real signing cache only when ExperimentNATSPubsub is enabled; otherwise the cache is a NoopSigningKeycache so callers still get a valid response (treated as mTLS-off). nats_ca is kept off the workspace-proxy crypto key allowlist so the CA private key is never served over the API. Co-authored-by: Mux <mux@coder.com>
Add cluster-route mTLS driven by tls.Config callbacks that read the nats_ca CA cache on each handshake, so a CA rotation is tracked with no restart. Each replica mints an ephemeral leaf from the active CA, stamping the signing CA's crypto_keys sequence into the leaf so a verifier loads exactly that CA (rotation overlap works in both directions). InsecureSkipVerify is set so verification runs in VerifyConnection against the live CA instead of a static, rotation-blind RootCAs pool; the connection is still mutually verified (RequireAnyClientCert). The leaf carries the replica's relay IP as an IP SAN. On the accepting side, where the dialing peer's source address is available, verification also requires the leaf SAN to match the connection source IP; the dialing side has no equivalent hook in Go and verifies the chain only. mTLS is optional: the pubsub boots with a noop CA cache (no leaf can be minted, so no route forms) and zero CA dependency. Enterprise HA swaps the real nats_ca cache plus the relay IP in via SetClusterCA when the high-availability feature is enabled, and reverts to noop when disabled. mTLS complements the existing shared route token (defense in depth). Co-authored-by: Mux <mux@coder.com>
5e5174e to
f24b480
Compare
| CommonName: "coder-nats-ca", | ||
| }, | ||
| NotBefore: anchorTime.Add(-clockSkewTolerance), | ||
| NotAfter: anchorTime.Add(keyDuration + NATSCALeafValidity + clockSkewTolerance), |
There was a problem hiding this comment.
this should still change
| // The certificate's NotAfter must track the supplied keyDuration, not a | ||
| // hardcoded default, so a CA stays valid for as long as it can be the | ||
| // active signer plus the longest leaf it signs. |
There was a problem hiding this comment.
the leaf duration needs to change when we get proper rotation squared away
| // WithFeatures sets the crypto key features the rotator manages, replacing the | ||
| // default set. Use this to opt experiment- or deployment-gated features (such | ||
| // as the NATS cluster CA) into rotation only when their owner is active. | ||
| func WithFeatures(features []database.CryptoKeyFeature) RotatorOption { | ||
| return func(r *rotator) { | ||
| r.features = slices.Clone(features) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
should we change this to add features onto the default set instead of replacing the default set?
| // generateNewSecret generates the secret for a new key of the given feature. | ||
| // keyDuration is the rotator's key duration; it is only used by features whose | ||
| // secret encodes its own validity window (currently only the NATS CA, whose | ||
| // certificate must outlive the key row's active-signer period). | ||
| func generateNewSecret(feature database.CryptoKeyFeature, startsAt time.Time, keyDuration time.Duration) (string, error) { |
There was a problem hiding this comment.
should the time be optional/a pointer, or should we have a new generateSecretWithStartsAt?
| // The old CA must remain a valid trust root for the maximum leaf | ||
| // lifetime after rotation. | ||
| expectedDeletesAt := oldKey.ExpiresAt(keyDuration).Add(NATSCALeafValidity + time.Hour) | ||
| oldKey, err = db.GetCryptoKeyByFeatureAndSequence(ctx, database.GetCryptoKeyByFeatureAndSequenceParams{ | ||
| Feature: oldKey.Feature, | ||
| Sequence: oldKey.Sequence, | ||
| }) | ||
| require.NoError(t, err) | ||
| require.Equal(t, expectedDeletesAt, oldKey.DeletesAt.Time.UTC()) |
There was a problem hiding this comment.
this will have to change with rotation validity times as well
| // SetClusterCA swaps the cluster mTLS CA cache and this replica's leaf IP SAN, | ||
| // then triggers a peer refresh so any route blocked by the previous (for | ||
| // example noop) cache is retried. It is a no-op unless the pubsub was started | ||
| // with cluster TLS enabled (Options.ClusterCA set, which installs the TLS | ||
| // callbacks). Passing a noop cache reverts to no mTLS: new route handshakes | ||
| // can no longer mint a leaf and will not form. | ||
| func (p *Pubsub) SetClusterCA(ca ClusterCAKeycache, ip net.IP) { | ||
| if p.clusterTLS == nil { | ||
| return | ||
| } | ||
| p.clusterTLS.setClusterCA(ca, ip) | ||
| p.RefreshPeers() | ||
| } | ||
|
|
There was a problem hiding this comment.
should we rename this to EnableClusterMTLS, and potentially have a Disable that resets the cache to the noop cache?
| // leafCertValidity is the lifetime of an ephemeral cluster leaf | ||
| // certificate. Leaves are re-minted before expiry and whenever the active | ||
| // CA rotates, so this can be well under the CA's own validity window | ||
| // (cryptokeys.NATSCALeafValidity). | ||
| leafCertValidity = 24 * time.Hour |
There was a problem hiding this comment.
this should be min(24*time.hour, CA cert validity -1h) or something similar
| type ClusterCAKeycache interface { | ||
| SigningKey(ctx context.Context) (id string, key interface{}, err error) | ||
| VerifyingKey(ctx context.Context, id string) (key interface{}, err error) | ||
| } | ||
|
|
There was a problem hiding this comment.
do we need to use a separate interface here?
50306a4 to
071f40f
Compare
sreya
left a comment
sreya
left a comment
There was a problem hiding this comment.
-
Do we need any verification whether the peer IP is a valid member of the replicas table in addition to just the IP SAN check? This would allow for dial side verification in addition to the existing accepting side verification
Yeah I think we should -
Currently we just shoehorn a sequence number for the CA certs into the x509 subject serial number, is this reasonable or should we use a custom extension? this would be relatively simple, mostly just requiring a bit of glue code plus some parsing function
I think its fine as-is -
right now we don't have warn/error logging of issues such as handshake failures/invalid leaf keys/etc. as it could be too noisy without a log deduper, should we use a metric instead?
I think logging is fine, we consider these real errors right? I don't think we're presuming these are going to occur frequently and transiently over the lifespan of a replica.
A cluster route leaf only authenticates a handshake, so it needs no independent lifetime: its NotAfter now tracks the signing CA's, and re-minting is driven purely by CA rotation.
Accept-side verification now requires the connection source IP to be one of this replica's configured cluster-route peers (from the replicas table via the NATS peer fetcher) in addition to matching the peer leaf's IP SAN.
In the case of a connection error, it would be frequently as IIUC we attempt a reconnection on a 1s timer (with some amount of jitter). It's still not too much volume, given the replica count we have, so if you still think these are okay to move to warn/error I'll do so. I think a metric makes sense to have as well or instead of moving the logging from debug level. |
Drop the redundant ClusterCAKeycache interface in favor of cryptokeys.SigningKeycache, remove the unused CryptoKey.DecodeString method, merge the NATS TLS integration test into the internal test file, and note the CA-cache boot refactor as a TODO.
Derive the leaf IP SAN and accept-side binding from the replica's ClusterHost (the CODER_CLUSTER_HOST argument) fixed at construction, replacing the DERP-relay-URL source, and rename Pubsub.SetClusterCA to SetCACache now that it only swaps the cache.
Reuse the cached leaf while it is still within its validity window before consulting the signing cache, dropping the now-redundant leafSeq field. Keep verify and currentLeaf as pure functions returning wrapped errors, and log at the tls.Config callback sites where the embedded NATS server would otherwise swallow them.
The startup peer refresh runs once with the boot-time noop fetcher and can race a manual setPeerAddresses call, wiping the route and known-peer set; with fail-closed membership that made the route-forming tests flaky, so drive their peers through fetchers as production does.
It is the only clock field on Options, so the shorter name is unambiguous.
…etcher Drop configureNATSClusterTLS so SetCACache sits beside SetPeerFetcher in the HA enable block, moving only the mTLS-status logging into a small helper.
…tests Production keeps the NATS default (2s); the longer 10s timeout that avoids flaky handshakes under load and in CI is now set through a test-only Options field.
| // ClusterHost is this replica's routable cluster address. It | ||
| // is the NATS route listener host and, when it is an IP, the | ||
| // leaf certificate's IP SAN for cluster mTLS. | ||
| ClusterHost: options.DeploymentValues.Cluster.Host.String(), |
There was a problem hiding this comment.
We technically fallback to the DERP host if Cluster Host is unset. We should just move that code into cli/server.go instead of doing it enterprise/cli to prevent this divergence. It's technically an enterprise concept but we gate that functionality in the entitlements loop so we aren't leaking enterprise features into open source.
Cluster route admission now relies on CA-chain verification and the leaf IP-SAN to source-IP binding; the extra check that the source is a current replicas-table member (and its peerIP plumbing) is removed.
… set Emit the mTLS enabled/disabled/inactive log from setCACache, the point where the state transition actually happens, instead of from the enterprise HA wiring.
The accept side now fails closed when it cannot determine the peer's source IP, instead of leaving it nil and silently skipping the leaf SAN to source-IP binding in verify.
Move the Cluster.Host to DERP-relay-host fallback into cli/server.go behind a new coderd.Options.ClusterHost so the NATS pubsub, leaf SAN, and replicas table all use the same resolved value instead of the enterprise-only resolution diverging from AGPL.
2 participants
This PR supersedes the existing chain of 3 PRs for mTLS, implementing the CA cert fetching/leaf key generation via an extension of the existing signing cache + functions on TLS structs themselves which allow for signing/verification on each request without storage of the root CA cert locally (other than the cache).
Summary of Changes
updated the
cryptokeyssigning cache to allow for fetch/refresh/rotation via the existing logic, with a modification to allow for returning a different result when fetching/reading a CA cert instead of the existing byte slice we use for JWTs.We use the tls
ConfigcallbacksGetCertificate/GetClientCertificate/VerifyConnectionto mint/return the replicas leaf key, and to verify peers against the CA fetched from the cache. This means that each leaf includes the relevant CA certs sequence # in its leaf certificate, meaning it should be easy/consistent to fetch the correct cert. during rotation overlap periods on the receiving end.Notes
nats_cacert generation/rotation is disabled unless the experiment is enabledInsecureSkipVerifyis used intentionally, by default Go has a staticRootCAfield but we need to have CA cert rotation, so we do our own verification viaVerifyConnectionRequireAnyClientCertstill requires a peer cert, so all connections are still mutually verifiedOpen Questions
replicastable in addition to just the IP SAN check? This would allow for dial side verification in addition to the existing accepting side verification