Replace TLS_MIN_VERSION & TLS_CIPHERS env vars with flags#608
Replace TLS_MIN_VERSION & TLS_CIPHERS env vars with flags#608ormergi wants to merge 2 commits intok8snetworkplumbingwg:mainfrom
Conversation
Flags are easier to work with and better fit for TLS settings. Resolve k8snetworkplumbingwg#606 Signed-off-by: Or Mergi <ormergi@redhat.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the application's TLS configuration by transitioning from environment variables to command-line flags for specifying the minimum TLS version and cipher suites. This change provides a more robust and explicit method for managing these security-critical settings, improving configurability and maintainability. Additionally, it standardizes the default minimum TLS version to TLS 1.3, bolstering the application's security posture. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request replaces the TLS_MIN_VERSION and TLS_CIPHERS environment variables with command-line flags, which is a good improvement for configuration management. The changes are applied consistently across the Go code and Kubernetes manifests. I've found a minor issue in the help text for the new tls-cipher-suites flag where missing spaces would lead to a garbled message.
Set the TLS minimal version default value to 1.3. Remove redundent the flag occurances in manifests. Signed-off-by: Or Mergi <ormergi@redhat.com>
30c457e to
fc28122
Compare
RamLavi
left a comment
There was a problem hiding this comment.
Hey, thanks! only small changes
| flag.StringVar(&tlsCiphers, "tls-cipher-suites", "", "Comma-separated list of TLS cipher suite names."+ | ||
| "Supported values are tls package constants names (e.g. TLS_AES_128_GCM_SHA256), please see "+ | ||
| "https://pkg.go.dev/crypto/tls#pkg-constants"+ | ||
| "When 'min-tls-version' is 'VersionTLS13', cipher suites are selected by the runtime.") |
There was a problem hiding this comment.
there is a typo in the flag description which could be confusing: please change min-tls-version --> tls-min-version
| flag.StringVar(&tlsCiphers, "tls-cipher-suites", "", "Comma-separated list of TLS cipher suite names. "+ | ||
| "Supported values are tls package constants names (e.g. TLS_AES_128_GCM_SHA256), please see "+ | ||
| "https://pkg.go.dev/crypto/tls#pkg-constants"+ | ||
| "https://pkg.go.dev/crypto/tls#pkg-constants. "+ |
There was a problem hiding this comment.
nit: . (with space)-->. (without)
What this PR does / why we need it:
Resolve TLS related env vars part of #606
Special notes for your reviewer:
min-tls-versionto TLS 1.3.TLS_VERSIONenv var toVersioTLS13, this is not longer needed as flags support default values.Release note: