fix(nodes): validate every certificate in the node mTLS trust bundle - #6188
Open
n0ctal wants to merge 1 commit into
Open
fix(nodes): validate every certificate in the node mTLS trust bundle#6188n0ctal wants to merge 1 commit into
n0ctal wants to merge 1 commit into
Conversation
AppendCertsFromPEM reports success once a single certificate parses, so a trust bundle whose later entries are damaged or truncated was accepted with those entries silently absent from the pool. Parse and validate every PEM block instead, and reject the bundle if any of them is malformed.
n0ctal
force-pushed
the
upstream-strict-mtls-ca-bundle
branch
from
August 8, 2026 15:12
e64ec1c to
a738507
Compare
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
Parse and validate every certificate in the node mTLS trust bundle instead of only the first one, and reject the bundle if any block is malformed.
Why
Two places accept the
nodeMtlsClientCAPemtrust bundle, and both are satisfied by a bundle that is only partly valid:NodeMtlsClientCAPoolbuilds the listener'sClientCAswithx509.CertPool.AppendCertsFromPEM, which returnstrueas soon as one certificate parses. Later entries that are damaged or truncated are skipped, and the pool is returned as if nothing were wrong.SetNodeMtlsTrustCAvalidates the value before storing it, butpem.Decodereturns only the first block and the remainder is discarded, so only the first certificate is ever checked.Together these mean a bundle can be stored, reported as valid, and then produce a trust pool that is missing certificates nobody was told about.
This matters most during a CA rotation, which is the one time a bundle legitimately holds two certificates. The operator stores
old CA + new CA, both calls report success, and the new CA is silently absent from the pool. Every node then rejects the panel's new client certificate at once — and the failure surfaces only when the panel next needs the node API, because data-plane traffic does not go through it.Scope
parseCertificateBundlePEMwalks the whole bundle, requiring every block to be a parseableCERTIFICATE, and rejects an empty bundle, a non-certificate block, and trailing non-PEM bytes.NodeMtlsClientCAPoolbuilds the pool from the parsed certificates withAddCert, so a partially valid bundle is an error rather than a quietly reduced pool.SetNodeMtlsTrustCAvalidates through the same helper, so what is accepted on write is exactly what will load on read.Validation
go build ./...,go vet,golangci-lint runclean onmainatece16559.go test ./internal/web/service/green.AppendCertsFromPEMimplementation it fails, reporting a pool built from one certificate while the second was dropped; it passes with this change.Risk
Low. The change is strictly a tightening of validation on a value that is expected to contain only certificates. Existing single-certificate configurations are unaffected.
One behaviour worth calling out: a stored bundle with trailing text after the last certificate used to load, and now will not. Operators who paste output from tools that append human-readable text would see the trust pool fail to build. That failure is currently non-fatal —
web.gologs a warning and starts the listener without mTLS, falling back to bearer-token auth for the node API — so a bad bundle degrades the node API from two factors to one rather than stopping the panel. I think that fallback deserves to fail closed instead, but it predates this change and I did not want to fold an unrelated behaviour change into a validation fix. Happy to send it separately if you agree.