ROX-33255: Env-based TLS profile configuration for Go services#19110
Merged
vladbologa merged 2 commits intomasterfrom Feb 25, 2026
Merged
ROX-33255: Env-based TLS profile configuration for Go services#19110vladbologa merged 2 commits intomasterfrom
vladbologa merged 2 commits intomasterfrom
Conversation
|
Skipping CI for Draft Pull Request. |
Contributor
|
Images are ready for the commit at 92beeca. To use with deploy scripts, first |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19110 +/- ##
==========================================
+ Coverage 49.52% 49.53% +0.01%
==========================================
Files 2672 2673 +1
Lines 201665 201730 +65
==========================================
+ Hits 99870 99923 +53
- Misses 94337 94345 +8
- Partials 7458 7462 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
f52f1fe to
1669dc0
Compare
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- MinVersion() and CipherSuites() re-read and re-parse the environment settings on every call; if these are used in hot paths, consider resolving and caching the effective values once (e.g., via sync.Once) to avoid repeated work and ensure consistency across calls.
- parseMinVersion currently hardcodes the list of accepted versions in the error message; consider deriving the allowed values from supportedVersions to keep the error message in sync if you add more versions in the future.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- MinVersion() and CipherSuites() re-read and re-parse the environment settings on every call; if these are used in hot paths, consider resolving and caching the effective values once (e.g., via sync.Once) to avoid repeated work and ensure consistency across calls.
- parseMinVersion currently hardcodes the list of accepted versions in the error message; consider deriving the allowed values from supportedVersions to keep the error message in sync if you add more versions in the future.
## Individual Comments
### Comment 1
<location> `pkg/tlsprofile/profile.go:51-27` </location>
<code_context>
+// TLS_CHACHA20_POLY1305_SHA256) are always enabled and not configurable.
+// This setting only affects the TLS 1.2 cipher negotiation.
+func CipherSuites() []uint16 {
+ envValue := env.TLSCipherSuites.Setting()
+ if envValue == "" {
+ return defaultCipherSuites
+ }
</code_context>
<issue_to_address>
**suggestion:** Consider trimming whitespace when deciding whether TLS cipher suites are configured
A value like `ROX_TLS_CIPHER_SUITES=" "` is currently treated as configured and passed to `parseCipherSuites`, which fails with `no valid cipher suites in input`, logs a warning, and falls back to defaults. Trimming whitespace before the empty-string check (e.g. `envValue := strings.TrimSpace(env.TLSCipherSuites.Setting())`) would make blank values behave like unset ones and avoid unnecessary warnings.
Suggested implementation:
```golang
envValue := strings.TrimSpace(env.TLSCipherSuites.Setting())
```
You will also need to:
1. Ensure `strings` is imported at the top of `pkg/tlsprofile/profile.go`:
- Add `strings` to the existing import block, e.g. `import ("strings" ... )`, or extend the current import list accordingly.
2. Confirm that there are no other references to `envValue` in `CipherSuites()` that assume it contains untrimmed content. The rest of the logic can remain unchanged because `parseCipherSuites` should already handle non-empty values as before.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
788827e to
5ccfa6c
Compare
5ccfa6c to
f8e9651
Compare
1402502 to
c9e27e3
Compare
porridge
approved these changes
Feb 24, 2026
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.
Description
This PR introduces the
ROX_TLS_MIN_VERSIONandROX_TLS_CIPHER_SUITESenvironment variables that allow Go services to configure TLS minimum version and cipher suites. When unset, the existing defaults (TLS 1.2, AES-256-GCM preferred) are preserved.The new
pkg/tlsprofilepackage parses and validates the env vars, rejecting insecure cipher suites and TLS versions below 1.2.DefaultTLSServerConfiginpkg/mtls/verifiernow reads from these env vars, so all Go servers (Central, Sensor, Scanner, Admission Controller) will use this without requiring individual changes.This is one building block for the Operator to propagate cluster-wide TLS profile settings (from
apiserver.config.openshift.io/v1) to all ACS deployments. That logic will be implemented in a follow-up PR.More context available here
Note on invalid cipher combinations
Go's net/http HTTP/2 implementation requires at least one of
TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256orTLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256in the cipher suite list (per RFC 7540). IfROX_TLS_CIPHER_SUITESis set without one of these, the secure metrics server will panic. All standard OpenShift TLS profiles (Old, Intermediate, Modern) include these ciphers, so this only affects custom profiles with a very narrow cipher list.User-facing documentation
Testing and quality
Automated testing
How I validated my change
Deployed both Central and Secured Cluster, applied the env values both individually and using
customize.envVars.