fix: enable gosec G402 (TLS MinVersion) linter rule - #19588
fix: enable gosec G402 (TLS MinVersion) linter rule#19588raajheshkannaa wants to merge 2 commits into
Conversation
Add G402 to the gosec includes list in .golangci.yml and fix all violations across the codebase: - Add MinVersion: tls.VersionTLS12 to tls.Config structs that were missing it (production code, test helpers, test servers) - Add #nosec G402 annotations to intentional InsecureSkipVerify usages where TLS verification is user-configurable (notifiers, scanners, registries) or where mTLS provides authentication (sensor-to-central) - Add #nosec G402 annotations to test code that uses self-signed certs Fixes part of stackrox#3545
|
Hi @raajheshkannaa. Thanks for your PR. I'm waiting for a stackrox member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
vikin91
added
external-contributor
|
/ok-to-test |
janisz
left a comment
janisz
left a comment
There was a problem hiding this comment.
Since Go 1.18+ defaults to TLS 1.2, this change might be redundant. Could you clarify the use case in the PR description?
raajheshkannaa
left a comment
raajheshkannaa
left a comment
There was a problem hiding this comment.
You're right that Go 1.18+ defaults to TLS 1.2 at runtime, so the MinVersion additions don't change behavior. The driver here is enabling the gosec G402 rule (part of #3545), which flags any tls.Config without an explicit MinVersion regardless of Go's runtime default. Without setting it explicitly, the linter fires on every struct literal. I'll update the PR description to make that motivation clearer.
|
/retest-required |
|
@raajheshkannaa: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Updated the description with the G402 motivation. The short version: gosec does static analysis and can't infer Go's runtime TLS defaults, so explicit MinVersion is needed to satisfy the linter. Let me know if that covers it. |
|
I created issue for gosec: securego/gosec#1627 |
|
Good call, and thanks for filing the upstream issue. You're right that Go 1.18+ defaults to TLS 1.2, so G402 firing on safe defaults is just noise. Happy to keep this PR open but on hold until securego/gosec#1627 is resolved. Once gosec handles this properly, we can revisit whether G402 is worth enabling here. |
raajheshkannaa
left a comment
raajheshkannaa
left a comment
There was a problem hiding this comment.
You're right that Go 1.18+ defaults to TLS 1.2 at runtime. The change is for gosec's static analysis, which can't infer runtime defaults. Without an explicit MinVersion, G402 fires on every tls.Config{} in the codebase. Setting it explicitly satisfies the linter and makes the security posture visible in code without relying on runtime knowledge. This is part of the broader #3545 effort to enable all security-relevant gosec rules.
There was a problem hiding this comment.
Hi @raajheshkannaa, thanks for the contribution! Unfortunately OpenShift TLS profile consistency requires us not to hardcode TLS settings in the application.
Instead they will be propagated from the Operator (which in turn retrieves them from apiserver.config.openshift.io/cluster).
You can also force the configuration on the deployments manually by setting ROX_TLS_MIN_VERSION and ROX_TLS_CIPHER_SUITES (ROX_TLS_OPENSSL_CIPHERS for the Postgres services).
See these PRs for more context:
These changes will be available in the next ACS minor release (4.11).
|
Thanks for the context, @vladbologa. The OpenShift TLS profile + ROX_TLS_MIN_VERSION/CIPHER_SUITES approach makes sense for ensuring cluster-wide consistency, and hardcoding MinVersion in code would conflict with the Operator-propagated config path. Combined with @janisz's earlier point about gosec G402 firing noisily on Go's safe TLS 1.2 defaults (securego/gosec#1627 still open), the architectural direction here is clear. Closing this PR. Appreciate the review and the pointers to #19110 and #18864. |
btw @raajheshkannaa the cluster-wide consistency feature is not available yet in OpenShift. But if you're using ACS 4.11, you will still be able to tell ACS to follow the profile from In the future, it will work automatically by checking the TLSAdherence field. |
5 participants
Part of #3545
Why G402?
The gosec G402 rule flags any
tls.Configliteral that omitsMinVersion. While Go 1.18+ defaults to TLS 1.2 at runtime, gosec performs static analysis and cannot infer runtime defaults. Without an explicitMinVersion, G402 fires on everytls.Config{}in the codebase. Setting it explicitly satisfies the linter and makes the security intent visible in code, so future readers do not have to know about Go's runtime behavior to confirm TLS 1.2 is enforced.This also aligns with the broader #3545 effort to enable all security-relevant gosec rules across the repo.
Summary
.golangci.ymlto enforce TLS minimum version settingsMinVersion: tls.VersionTLS12to 15tls.Configstructs missing it#nosec G402annotations to 28 intentionalInsecureSkipVerifyusages with explanatory commentsCategories of nosec annotations
Changes
43 files changed, 59 insertions, 46 deletions
Test plan
golangci-lintpasses with G402 enabled