Skip to content

ROX-33255: Env-based TLS profile configuration for Go services#19110

Merged
vladbologa merged 2 commits intomasterfrom
vb/tls-settings
Feb 25, 2026
Merged

ROX-33255: Env-based TLS profile configuration for Go services#19110
vladbologa merged 2 commits intomasterfrom
vb/tls-settings

Conversation

@vladbologa
Copy link
Contributor

@vladbologa vladbologa commented Feb 19, 2026

Description

This PR introduces the ROX_TLS_MIN_VERSION and ROX_TLS_CIPHER_SUITES environment 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/tlsprofile package parses and validates the env vars, rejecting insecure cipher suites and TLS versions below 1.2.

DefaultTLSServerConfig in pkg/mtls/verifier now 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_SHA256 or TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 in the cipher suite list (per RFC 7540). If ROX_TLS_CIPHER_SUITES is 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

  • the change is production ready: the change is GA, or otherwise the functionality is gated by a feature flag
  • CI results are inspected

Automated testing

  • added unit tests
  • added e2e tests
  • added regression tests
  • added compatibility tests
  • modified existing tests

How I validated my change

Deployed both Central and Secured Cluster, applied the env values both individually and using customize.envVars.

NS=acs-central

# Start a debug pod in a separate terminal
kubectl run tls-test -n "$NS" --rm -it --image=registry.access.redhat.com/ubi8/ubi -- bash

# --- Test 1: TLS 1.3 only ---
kubectl set env deployment/central -n "$NS" \
  ROX_TLS_MIN_VERSION=TLSv1.3 \
  ROX_TLS_CIPHER_SUITES-

# From the debug pod:
# Should FAIL (TLS 1.2 rejected)
openssl s_client -connect central.acs-central.svc:443 -tls1_2
# Should SUCCEED
openssl s_client -connect central.acs-central.svc:443 -tls1_3

# --- Test 2: TLS 1.2 with restricted ciphers ---
kubectl set env deployment/central -n "$NS" \
  ROX_TLS_MIN_VERSION=TLSv1.2 \
  ROX_TLS_CIPHER_SUITES=TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384 


# This test caused crash-loops due to the cipher configuration not being accepted by Go for HTTP/2 connections.

# --- Test 3: TLS 1.3 + TLS 1.2 ciphers (ciphers should be ignored) ---
kubectl set env deployment/central -n "$NS" \
  ROX_TLS_MIN_VERSION=TLSv1.3 \
  ROX_TLS_CIPHER_SUITES=TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384

# From the debug pod:
# Should SUCCEED (TLS 1.3 works, cipher setting is silently ignored by Go)
openssl s_client -connect central.acs-central.svc:443 -tls1_3
# Should FAIL (TLS 1.2 still rejected)
openssl s_client -connect central.acs-central.svc:443 -tls1_2

# --- Test 4: Invalid values (should fall back to defaults) ---
kubectl set env deployment/central -n "$NS" \
  ROX_TLS_MIN_VERSION=bogus \
  ROX_TLS_CIPHER_SUITES=NOT_A_CIPHER

# From the debug pod:
# Should SUCCEED with TLS 1.2 (default fallback)
openssl s_client -connect central.acs-central.svc:443 -tls1_2
# Verify error logs
kubectl logs deployment/central -n "$NS" | grep -i "ROX_TLS"

@openshift-ci
Copy link

openshift-ci bot commented Feb 19, 2026

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

sourcery-ai[bot]

This comment was marked as outdated.

@rhacs-bot
Copy link
Contributor

rhacs-bot commented Feb 19, 2026

Images are ready for the commit at 92beeca.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.11.x-158-g92beecaa71.

@codecov
Copy link

codecov bot commented Feb 19, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 49.53%. Comparing base (573923b) to head (c9e27e3).

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     
Flag Coverage Δ
go-unit-tests 49.53% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@vladbologa vladbologa marked this pull request as ready for review February 19, 2026 16:16
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@vladbologa vladbologa force-pushed the vb/tls-settings branch 2 times, most recently from 788827e to 5ccfa6c Compare February 19, 2026 17:56
@vladbologa vladbologa changed the title ROX-33255: Add env-based TLS profile configuration for Go services ROX-33255: Env-based TLS profile configuration for Go services Feb 19, 2026
@vladbologa vladbologa requested review from a team, mclasmeier and porridge and removed request for a team February 23, 2026 18:28
@vladbologa vladbologa merged commit 82c3a37 into master Feb 25, 2026
103 checks passed
@vladbologa vladbologa deleted the vb/tls-settings branch February 25, 2026 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants