ACM-37203: Add support for configurable TLS profiles - #372
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesTLS configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI flags
participant Options
participant Options.Validate
participant ServeHTTPS
participant TLS configuration
CLI flags->>Options: Set TLSMinVersion and TLSCipherSuites
Options->>Options.Validate: Validate TLS settings
Options.Validate-->>ServeHTTPS: Provide validated options
ServeHTTPS->>TLS configuration: Apply minimum TLS version and cipher suites
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@main.go`:
- Line 47: Update the tlsCipherSuites flag handling around TLSCipherSuites and
its Var registration to split each comma-separated value into individual cipher
suite names before applying TLS options. Preserve support for repeated flag
values and ensure validation receives separate entries rather than one combined
string.
In `@tls_config.go`:
- Around line 18-22: Update the TLSCipherSuites parsing and validation around
tlsCipherSuites so TLS 1.3 suite names are not accepted as enforceable
configuration when assigned to tls.Config.CipherSuites; either reject them with
a clear error or explicitly scope the flag to TLS 1.2 and earlier. Ensure tests
cover the chosen behavior and preserve valid TLS 1.2-and-earlier cipher suite
handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d231bd2-616d-412c-a82e-0e42fac56545
📒 Files selected for processing (5)
http.gomain.gooptions.gotls_config.gotls_config_test.go
liouk
left a comment
liouk
left a comment
There was a problem hiding this comment.
The TLS override logic implementation currently does not include any unit tests; we should add unit tests that would verify for example:
- Default config (no flags) keeps library-go defaults
- Setting
--tls-min-version=VersionTLS13actually changesconfig.MinVersion - Invalid version/cipher names produce errors
- The existing
StringArraytests instring_array_test.godon't cover the new comma-splitting behavior
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@http_test.go`:
- Around line 21-23: Replace the assignment-only checks in
TestTLSMinVersionParsing and related tests with coverage of a TLS configuration
resolver returning (*tls.Config, error). Extract the TLS parsing logic from
ServeHTTPS into that resolver, then assert secure defaults, valid TLS versions
and cipher suites, and errors for invalid inputs using the existing expected and
expectError cases.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e30290b2-fc4b-498f-ba9a-248830818618
📒 Files selected for processing (3)
http_test.gostring_array.gostring_array_test.go
0a9d488 to
80ce05c
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
options.go (1)
333-357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest
Options.Validatedirectly.The current TLS tests only copy values into
Options. They do not execute the validation added at Lines 333-357. Add table-driven cases that callValidateand assert rejection of TLS 1.0, TLS 1.1, unknown versions, and unknown cipher suites. Otherwise, the tests can pass if this validation is removed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@options.go` around lines 333 - 357, Add table-driven tests that invoke Options.Validate for TLS configuration cases, asserting errors for TLS 1.0, TLS 1.1, unknown TLS versions, and unknown cipher suites. Ensure each case supplies the relevant TLSMinVersion or TLSCipherSuites values and verifies Validate rejects the configuration, rather than only testing option assignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@http.go`:
- Around line 87-98: Update ServeHTTPS to capture and handle errors returned by
k8sapiflag.TLSVersion and k8sapiflag.TLSCipherSuites instead of discarding them.
Log the parsing failure and return before creating the HTTPS listener, ensuring
invalid or partial TLS options cannot start serving.
---
Nitpick comments:
In `@options.go`:
- Around line 333-357: Add table-driven tests that invoke Options.Validate for
TLS configuration cases, asserting errors for TLS 1.0, TLS 1.1, unknown TLS
versions, and unknown cipher suites. Ensure each case supplies the relevant
TLSMinVersion or TLSCipherSuites values and verifies Validate rejects the
configuration, rather than only testing option assignment.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ab32ed1-3616-4972-9322-aaf9df05a5d0
📒 Files selected for processing (5)
go.modhttp.gohttp_test.gomain.gooptions.go
🚧 Files skipped from review as they are similar to previous changes (3)
- main.go
- go.mod
- http_test.go
coleenquadros
changed the title
179e787 to
fabd77c
Compare
liouk
left a comment
liouk
left a comment
There was a problem hiding this comment.
Thanks for the improvements! Overall looks good now, just a couple more suggestions.
Add --tls-min-version and --tls-cipher-suites flags to enable oauth-proxy to honor cluster-wide TLS security profiles. - Use library-go (oscrypto) for TLS validation and parsing - Parse comma-separated cipher suites (MCO compatibility) - Reject TLS 1.0/1.1 and insecure ciphers (RC4, 3DES, CBC-SHA1) - Validate in Options.Validate() for early error detection - Comprehensive unit tests for validation and security Fixes: openshift#352 Signed-off-by: Coleen Iona Quadros <coleen.quadros27@gmail.com>
fabd77c to
70b62f9
Compare
|
Thank you for the changes @coleenquadros -- looks good! /lgtm |
|
Scheduling required tests: |
|
/jira refresh |
|
@liouk: No Jira issue is referenced in the title of this pull request. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/retitle ACM-37203: Add support for configurable TLS profiles |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@coleenquadros: This pull request references ACM-37203 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
sanchezl
left a comment
sanchezl
left a comment
There was a problem hiding this comment.
The approach here is seems over-scoped for what oauth-proxy needs. oauth-proxy is a sidecar and the operator that creates it is the one that owns TLS policy decisions: min version/ciphers/validation. A simpler PR where oauth-proxy just accepts the flags and apply them should be good enough .
Just add the flags, apply them, done: Drop all the validation in Validate() (insecure cipher rejection, version floor checks, cross-field warnings). The deploying operator determines what's valid for its context. oauth-proxy shouldn't second-guess it. This also eliminates a number of edge cases the current validation misses
Default to TLS 1.3 minimum: That eliminates the need for the library-go crypto dependency entirely.
@sanchezl I'm the one responsible for steering towards this change; @coleenquadros' initial implementation was as you describe it. While I agree that the operator should own policies, the validation doesn't really conflict with that -- it's more of a safety net, and an operator that sets valid values will never hit it. Plus, if the binary ever gets used standalone instead of a sidecar (e.g. testing/debugging), this might come in handy. At the same time, there's no real burden added: the validation code is ~30 lines, uses already vendored libs and only runs once at startup; no performance cost, no new dependencies, and no real maintenance burden added -- overall oauth-proxy still remains lean. If you feel strongly about this direction, I'm ok to yield and go back to the original approach. I am also lacking broader context of how this requirement is implemented across various components. One final note:
The library-go crypto dependency I think predates this PR, and AFAICS is a bigger change to drop. Holding the PR until we have a final decision on this. /hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
@liouk The model I am arguing from is operator and operand. The operator owns policy: the TLS floor, the ciphers, what is acceptable. The operand does what it is told. The point of that split is that policy lives in exactly one place. Validation in the operand breaks that, because to validate you have to encode a second copy of the policy. When the policy changes, and it will, you now have two places to update and they can drift. The worst case is the operand rejecting a value the operator considers valid, so the component that does not own the decision is the one blocking it. The standalone/testing case actually cuts the same way: when debugging you often want to set a value the policy would normally reject. I feel strongly that we should:
P.S. Dropping the library-go crypto usage was never a goal of its own. It just falls out for free if the operand stops carrying its own defaults and validation, and it seemed worth grabbing while we are already in this code. |
|
@sanchezl I'm ok with that -- I fully agree that the operator should own policy over the operand, and that enforcing policy in multiple places is fragile. However I was mainly referring to rejecting TLS versions and ciphers that are globally accepted as insecure and should never be enforced by the operator either rather than generic policy. @coleenquadros let's go back to the original implementation, and set the default to TLS 1.3. |
Per reviewer feedback, oauth-proxy is a sidecar — the deploying operator owns TLS policy decisions. Remove all validation from Validate(), use Go stdlib crypto/tls directly instead of library-go for TLS config construction, and default to TLS 1.3 minimum. Signed-off-by: Coleen Iona Quadros <coleen.quadros27@gmail.com>
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: coleenquadros, liouk 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 |
|
@coleenquadros: all tests passed! 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. |
5 participants
Summary
This PR adds command-line flags to configure TLS minimum version and cipher suites, allowing oauth-proxy to honor cluster-wide TLS security profiles from the OpenShift API server.
Fixes #352
Jira https://redhat.atlassian.net/browse/ACM-37203
Changes
--tls-min-versionflag to configure minimum TLS version--tls-cipher-suitesflag to configure allowed cipher suitesk8s.io/component-base/cli/flagpackage for TLS parsing (same as kube-rbac-proxy, kube-apiserver, and other core components)SecureTLSConfig()Motivation
Currently, oauth-proxy uses hardcoded TLS settings from
library-go/pkg/crypto.SecureTLSConfig(). While these defaults are secure (TLS 1.2+, strong ciphers), they cannot be dynamically configured to match cluster-wide TLS security profiles.This prevents components using oauth-proxy from achieving TLS profile compliance when the cluster is configured with Modern or Custom TLS profiles.
Impact
This change enables operators like multicluster-observability-operator to dynamically inject cluster TLS profile settings, achieving compliance with OpenShift TLS security profiles (Intermediate, Modern, Custom).
Example usage:
Implementation Details
This implementation follows the same pattern as MCO's rbac-query-proxy (stolostron/multicluster-observability-operator#2540):
Operators can use the existing
SetTLSSecurityConfiguration()utility to inject cluster TLS settings:Testing
--helpoutputRelated Work
Checklist
Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests