OCPBUGS-17349: crypto: remove deprecated TLS ciphers #2051
Conversation
sanchezl
changed the title
openshift-ci-robot
added
jira/severity-critical
|
@sanchezl: This pull request references Jira Issue OCPBUGS-17349, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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
changed the title
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@sanchezl: This pull request references Jira Issue OCPBUGS-17349, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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. |
| // | ||
| // These API resources allow cluster administrators to choose between Old, Intermediate, | ||
| // Modern, or Custom TLS profiles. Components should observe these settings. | ||
| func DefaultCiphers() []uint16 { |
There was a problem hiding this comment.
How are we going to test if the updated list doesn't break the platform ?
There was a problem hiding this comment.
I think all we can do is test as we update each repo and monitor for issues post-merge. A few points to consider:
Go's cipher suite preference ordering:
Since Go 1.17 (ref), Go enforces a cipher suite ordering preference for all Go clients. This means all Go-based clients (which is most of OpenShift) should have been preferring the stronger ciphers anyway, even when the weaker ones were available.
Testing strategy:
- I've created a proof PR (#2836 in cluster-network-operator) to demonstrate the changes work in at least one repo. Tools in this repo do not seem to directly read the cluster level TLS config. This repo was also explicitly called out in the original issue.
- Standard e2e tests will exercise TLS connections across the platform
- The CI suite should catch any immediate TLS negotiation failures
Real risk:
The danger lies in clients (especially non-Go ones) that only support one of the removed ciphers, preventing negotiation from selecting one of the supported ciphers. I'll keep an eye out for these, but I would be surprised if we find one as the supported ciphers have been around for a long time now and are widely supported.
Post-merge monitoring:
We should monitor for TLS handshake failures after this merges, particularly from:
- Older automation tools
- Non-Go clients (curl, openssl-based tools, etc.)
- Third-party integrations
pkg/crypto/crypto_test.go
Outdated
| // TestDefaultCiphersAgainstMozillaGuidelines fetches the latest Mozilla SSL configuration | ||
| // guidelines and compares them with our DefaultCiphers() to detect drift from recommendations. | ||
| // | ||
| // This test reports but does not fail when discrepancies are found, as we may intentionally |
There was a problem hiding this comment.
If this test never fails, we won’t be able to detect any drift.
not sure if we need that test then.
There was a problem hiding this comment.
Just a suggestion in case we want to keep the test.
It'd be nice to download a versioned of the file (it seems like 5.7 is the latest one) and commit it as a fixture. Then we can add a script + make target to update the fixture.
In order to make the test useful, we could make the test fail (not only log) and hardcode the current drift. That way, the test becomes a contract and any future changes (in the guidelines or in our code) will need to be manually checked.
There was a problem hiding this comment.
@bertinatto I took a stab at your suggestion.
p0lyn0mial
added
the
approved
sanchezl
force-pushed
the
update-default-ciphersuites
branch
from
b830b8a to
7ed68dd
Compare
| tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, | ||
| tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256, | ||
| tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, | ||
| tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, // required by http/2 |
There was a problem hiding this comment.
what happens if a client wants to use deprecated ciphers? Will it fail to establish a TLS connection with the server?
There was a problem hiding this comment.
and if that client is our customer, what would be our response to such an incident?
There was a problem hiding this comment.
What happens if a client wants to use deprecated ciphers? Will it fail to establish a TLS connection?
Yes, if a client only supports the removed ciphers and doesn't support any of the remaining ones, the TLS handshake will fail.
and if that client is our customer, what would be our response to such an incident?
Our response would be that these ciphers were removed due to security vulnerabilities.
Note that a customer client would most likely interact with either a route or the API Server. Both of these components have configurable TLS profiles and are, in effect, not directly affected by this update.
Also, note that this update brings the defaults more in line with the OpenShift's pre-defined TLS profiles, which do not contain the deprecated ciphers being removed. There is a slight discrepancy with regards to the DHE profiles which were removed (superseded by ECDHE) for performance issues, and never in our list of default ciphers, but are still in our intermediate TLS profile.
As for components that are not configurable, there is an effort to find and fix this: https://issues.redhat.com/browse/OCPSTRAT-769
| tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, // required by HTTP/2 | ||
| tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, | ||
| tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, | ||
| tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256, // forbidden by http/2, not flagged by http2isBadCipher() in go1.8 |
There was a problem hiding this comment.
since this bug was raised by the product security team, would it make sense to ask them for a review to double check we removed all weak ciphers?
There was a problem hiding this comment.
I have reached out to the product security team.
There was a problem hiding this comment.
@p0lyn0mial I have heard back from Product Security. It will be a while before they can have a technical review, so I suggest we proceed with merging. Given that this PR is removing weak ciphers (not adding new capabilities), if their future review identifies any issues, we can address them in a follow-up.
Update DefaultCiphers() to align with Mozilla SSL Configuration Guidelines 5.7 Intermediate profile. Remove deprecated CBC-mode ciphers and RSA key exchange ciphers that lack perfect forward secrecy.
sanchezl
force-pushed
the
update-default-ciphersuites
branch
from
7ed68dd to
2ba049b
Compare
pkg/crypto/crypto_test.go
Outdated
|
|
||
| // TestDefaultCiphersAgainstMozillaGuidelines compares our DefaultCiphers() with the Mozilla TLS | ||
| // configuration guidelines stored in testfiles/mozilla-guidelines.json. | ||
| func TestDefaultCiphersAgainstMozillaGuidelines(t *testing.T) { |
There was a problem hiding this comment.
If I’m not mistaken, this test is static and requires manual intervention to update the guidelines. This means it may fail when we change our defaults.
I’m not sure whether we should follow these guidelines strictly.
Is there a requirement to follow the guidelines ?
The test also doesn’t specify what to do when we need to drift. What would that require?
There was a problem hiding this comment.
This means it may fail when we change our defaults.
Yes
The test also doesn’t specify what to do when we need to drift. What would that require?
The test error message instructs you to run make update-mozilla-guidelines on failure. If we no longer follow the recommendations, then we can drop the test at that time and
Is there a requirement to follow the guidelines ?
No. We followed them, and I continue to follow them, because the Mozilla SSL/TLS Configuration Guidelines are continuously updated by their security experts and so far, have always been in sync with industry consensus on these matters.
| // TLS 1.2 cipher suites with ECDHE + AEAD | ||
| tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, | ||
| tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256, | ||
| tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, |
There was a problem hiding this comment.
I’m wondering how we decided which ciphers to remove.
If I’m not mistaken, TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 appears in the old section of the guideline.
There was a problem hiding this comment.
OK. Found it I think that the ask is to remove deprecated CBC-mode ciphers and RSA key exchange ciphers that lack perfect forward secrecy.
| tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, // required by HTTP/2 | ||
| tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, | ||
| tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, | ||
| tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256, // forbidden by http/2, not flagged by http2isBadCipher() in go1.8 |
There was a problem hiding this comment.
removed because of CBC encryption, right ?
There was a problem hiding this comment.
CBC ciphers in TLS are vulnerable to padding oracle and timing attacks.
| tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, | ||
| tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, | ||
| tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256, // forbidden by http/2, not flagged by http2isBadCipher() in go1.8 | ||
| tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256, // forbidden by http/2, not flagged by http2isBadCipher() in go1.8 |
There was a problem hiding this comment.
removed because of CBC encryption, right ?
There was a problem hiding this comment.
CBC ciphers in TLS are vulnerable to padding oracle and timing attacks.
| tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, | ||
| tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256, // forbidden by http/2, not flagged by http2isBadCipher() in go1.8 | ||
| tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256, // forbidden by http/2, not flagged by http2isBadCipher() in go1.8 | ||
| tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, // forbidden by http/2 |
There was a problem hiding this comment.
removed because of CBC encryption, right ?
There was a problem hiding this comment.
CBC ciphers in TLS are vulnerable to padding oracle and timing attacks.
| tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256, // forbidden by http/2, not flagged by http2isBadCipher() in go1.8 | ||
| tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256, // forbidden by http/2, not flagged by http2isBadCipher() in go1.8 | ||
| tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, // forbidden by http/2 | ||
| tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, // forbidden by http/2 |
There was a problem hiding this comment.
removed because of CBC encryption, right ?
There was a problem hiding this comment.
CBC ciphers in TLS are vulnerable to padding oracle and timing attacks.
| tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256, // forbidden by http/2, not flagged by http2isBadCipher() in go1.8 | ||
| tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, // forbidden by http/2 | ||
| tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, // forbidden by http/2 | ||
| tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, // forbidden by http/2 |
There was a problem hiding this comment.
removed because of CBC encryption, right ?
There was a problem hiding this comment.
CBC ciphers in TLS are vulnerable to padding oracle and timing attacks.
| tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, // forbidden by http/2 | ||
| tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, // forbidden by http/2 | ||
| tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, // forbidden by http/2 | ||
| tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, // forbidden by http/2 |
There was a problem hiding this comment.
removed because of CBC encryption, right ?
There was a problem hiding this comment.
CBC ciphers in TLS are vulnerable to padding oracle and timing attacks.
| tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, // forbidden by http/2 | ||
| tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, // forbidden by http/2 | ||
| tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, // forbidden by http/2 | ||
| tls.TLS_RSA_WITH_AES_128_GCM_SHA256, // forbidden by http/2 |
There was a problem hiding this comment.
removed because of RSA key exchange, right ?
There was a problem hiding this comment.
This suite does not provide Forward Secrecy.
| tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, // forbidden by http/2 | ||
| tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, // forbidden by http/2 | ||
| tls.TLS_RSA_WITH_AES_128_GCM_SHA256, // forbidden by http/2 | ||
| tls.TLS_RSA_WITH_AES_256_GCM_SHA384, // forbidden by http/2 |
There was a problem hiding this comment.
removed because of RSA key exchange, right ?
There was a problem hiding this comment.
This suite does not provide Forward Secrecy.
| // tls.TLS_RSA_WITH_AES_128_CBC_SHA256, | ||
| // tls.TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA, // forbidden by http/2, disabled to mitigate SWEET32 attack | ||
| // tls.TLS_RSA_WITH_3DES_EDE_CBC_SHA, // forbidden by http/2, disabled to mitigate SWEET32 attack | ||
| tls.TLS_RSA_WITH_AES_128_CBC_SHA, // forbidden by http/2 |
There was a problem hiding this comment.
removed because of RSA key exchange and CBC encryption, right ?
There was a problem hiding this comment.
This suite does not provide Forward Secrecy.
CBC ciphers in TLS are vulnerable to padding oracle and timing attacks.
| // tls.TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA, // forbidden by http/2, disabled to mitigate SWEET32 attack | ||
| // tls.TLS_RSA_WITH_3DES_EDE_CBC_SHA, // forbidden by http/2, disabled to mitigate SWEET32 attack | ||
| tls.TLS_RSA_WITH_AES_128_CBC_SHA, // forbidden by http/2 | ||
| tls.TLS_RSA_WITH_AES_256_CBC_SHA, // forbidden by http/2 |
There was a problem hiding this comment.
removed because of RSA key exchange and CBC encryption, right ?
There was a problem hiding this comment.
This suite does not provide Forward Secrecy.
CBC ciphers in TLS are vulnerable to padding oracle and timing attacks.
sanchezl
force-pushed
the
update-default-ciphersuites
branch
from
2ba049b to
4a03f7c
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: p0lyn0mial, sanchezl The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@sanchezl: 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. |
|
@sanchezl: Jira Issue OCPBUGS-17349: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-17349 has been moved to the MODIFIED state. 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. |
This commit pulls openshift/library-go#2051. Signed-off-by: Simon Pasquier <[email protected]>
This commit pulls openshift/library-go#2051. Signed-off-by: Simon Pasquier <[email protected]>
Pulling in openshift/library-go@4a03f7ce49 (crypto: drop weak TLS ciphers, 2025-11-14, openshift/library-go#2051). Generated with: $ go get github.com/openshift/library-go@master $ go mod tidy $ go mod vendor $ git add -A go.* vendor using: $ go version go version go1.24.0 linux/amd64
Pulling in openshift/library-go@4a03f7ce49 (crypto: drop weak TLS ciphers, 2025-11-14, openshift/library-go#2051). Generated with: $ go get github.com/openshift/library-go@master $ go mod tidy $ go mod vendor $ git add -A go.* vendor using: $ go version go version go1.24.0 linux/amd64
4 participants
Update DefaultCiphers() to align with Mozilla SSL Configuration Guidelines (latest) Intermediate profile. Remove deprecated CBC-mode ciphers and RSA key exchange ciphers that lack perfect forward secrecy.
This PR replaces #1964 with broader scope to match the complete Mozilla intermediate profile recommendations.
Reference: https://ssl-config.mozilla.org/guidelines/latest.json