Skip to content

CNTRLPLANE-3742: Fix ciphersuite behavior misunderstanding and tweak validation recommendation#2057

Open
vincentdephily wants to merge 2 commits into
openshift:masterfrom
vincentdephily:vdp-tls13-ciphersuites
Open

CNTRLPLANE-3742: Fix ciphersuite behavior misunderstanding and tweak validation recommendation#2057
vincentdephily wants to merge 2 commits into
openshift:masterfrom
vincentdephily:vdp-tls13-ciphersuites

Conversation

@vincentdephily

Copy link
Copy Markdown
Contributor

This is split in two commits

  1. Go behaviour depends on negociated protocol version, not configured minversion, so validation should not look at minversion. This is probably uncontroversial.
  2. Go stack could evolve, and other stacks have fewer restrictions, so validation should only warn about (not reject) suspicious ciphersuites. I think this is the right thing to do, but I'd like others to chime in.

This prompts for code changes in APIServer's config validation, but also other places like etcd. Some are straightforward, others might require changes to distinguish error from warning.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 6, 2026
@openshift-ci-robot

openshift-ci-robot commented Jul 6, 2026

Copy link
Copy Markdown

@vincentdephily: This pull request references CNTRLPLANE-3742 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 story to target the "5.0.0" version, but no target version was set.

Details

In response to this:

This is split in two commits

  1. Go behaviour depends on negociated protocol version, not configured minversion, so validation should not look at minversion. This is probably uncontroversial.
  2. Go stack could evolve, and other stacks have fewer restrictions, so validation should only warn about (not reject) suspicious ciphersuites. I think this is the right thing to do, but I'd like others to chime in.

This prompts for code changes in APIServer's config validation, but also other places like etcd. Some are straightforward, others might require changes to distinguish error from warning.

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.

@openshift-ci openshift-ci Bot requested review from ashcrow and jwforres July 6, 2026 11:58
@openshift-ci

openshift-ci Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign coverprice for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Comment thread enhancements/security/centralized-tls-config.md Outdated
Comment thread enhancements/security/centralized-tls-config.md Outdated
The behavior does not depend on the configured minVersion, just on the effectively negociated version.
This future-proofs against upstream go changes, and improves compatibility with non-go stacks.
@vincentdephily vincentdephily force-pushed the vdp-tls13-ciphersuites branch from 0c9fef4 to 79ec2c0 Compare July 9, 2026 11:11
@openshift-ci

openshift-ci Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

@vincentdephily: all tests passed!

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.


#### Custom Profile with TLS 1.2 Example

Custom cipher configuration is only supported with TLS 1.2:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Relax this comment.


#### Invalid Configuration (Rejected)

The following configuration will be **rejected by validation**:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This should result in a warning, not a rejection

The API modifies existing behavior by:
- Establishing the APIServer configuration as the default source for TLS configuration that all core components will consume
- Introducing the `tlsAdherence` field to control enforcement behavior
- Adding validation to reject cipher suite configuration with TLS 1.3

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

s/reject/warn/


- `TLSAdherence` feature gate implemented
- Ability to configure TLS profile and `tlsAdherence` in APIServer resource
- Validation rejects cipher suites when `minTLSVersion` is TLS 1.3

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

s/reject/warn/

- Ability to configure TLS profile and `tlsAdherence` in APIServer resource
- Validation rejects cipher suites when `minTLSVersion` is TLS 1.3
- At least one core component (e.g., kube-apiserver) respects the cluster-wide profile
- Documentation of TLS 1.3 hardcoded cipher behavior

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

And hardcoded cipher preference for tls1.2


**Unit Tests:**
- Validation correctly rejects cipher suites with TLS 1.3
- Validation accepts valid configurations (ciphers with TLS 1.2, no ciphers with TLS 1.3)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Only check for warnings

@vincentdephily

Copy link
Copy Markdown
Contributor Author

@JoelSpeed @everettraven I've noted/reviewed a few places that need updating to make the whole document consistent, if ye agree with the basic direction of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants