Repository navigation
TLS: return an error when the CA file holds no certificates - #7908
Open
sarankumar1325 wants to merge 1 commit into
Open
sarankumar1325 wants to merge 1 commit into
sarankumar1325 wants to merge 1 commit into
Conversation
GetTLSConfig ignored the result of AppendCertsFromPEM. A tls_ca_path file with no PEM certificates (empty, wrong file, a key) produced an empty root pool, so every TLS connection failed later with "certificate signed by unknown authority" and no hint about the file. Return an error naming the file instead. Signed-off-by: sarankumar1325 <120080287+sarankumar1325@users.noreply.github.com>
This branch has not been deployed
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.
What this PR does:
ClientConfig.GetTLSConfiginpkg/util/tlsignored the return value ofAppendCertsFromPEM. If the file set with a-<prefix>.tls-ca-pathflag holds no PEM certificates (an empty file, the wrong file, or a key instead of a certificate), the root pool stayed empty and setup went on as normal. Every TLS connection then failed later withx509: certificate signed by unknown authority, which doesn't point at the CA file.GetTLSConfignow returns an error naming the file when no certificates can be parsed from it, so a bad CA file shows up at startup.Tests: added cases to
TestGetTLSConfig_CAfor an empty file, a non-PEM file and a key-only file. They fail on master and pass with this change.go test ./pkg/util/tls/... ./pkg/util/grpcclient/...passes, andgolangci-lint run ./pkg/util/tls/...reports 0 issues.I used an AI assistant to help find and fix this, and I reviewed and tested the change myself.
Which issue(s) this PR fixes:
None, found while reading the code.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]docs/configuration/v1-guarantees.mdupdated if this PR introduces experimental flags