-
Notifications
You must be signed in to change notification settings - Fork 249
OCPBUGS-17349: crypto: remove deprecated TLS ciphers #2051
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -242,35 +242,41 @@ func ValidCipherSuites() []string { | |
| sort.Strings(validCipherSuites) | ||
| return validCipherSuites | ||
| } | ||
|
|
||
| // DefaultCiphers returns the default cipher suites for TLS connections. | ||
| // | ||
| // RECOMMENDATION: Instead of relying on this function directly, consumers should respect | ||
| // TLSSecurityProfile settings from one of the OpenShift API configuration resources: | ||
| // - For API servers: Use apiserver.config.openshift.io/cluster Spec.TLSSecurityProfile | ||
| // - For ingress controllers: Use operator.openshift.io/v1 IngressController Spec.TLSSecurityProfile | ||
| // - For kubelet: Use machineconfiguration.openshift.io/v1 KubeletConfig Spec.TLSSecurityProfile | ||
| // | ||
| // These API resources allow cluster administrators to choose between Old, Intermediate, | ||
| // Modern, or Custom TLS profiles. Components should observe these settings. | ||
| func DefaultCiphers() []uint16 { | ||
| // HTTP/2 mandates TLS 1.2 or higher with an AEAD cipher | ||
| // suite (GCM, Poly1305) and ephemeral key exchange (ECDHE, DHE) for | ||
| // perfect forward secrecy. Servers may provide additional cipher | ||
| // suites for backwards compatibility with HTTP/1.1 clients. | ||
| // See RFC7540, section 9.2 (Use of TLS Features) and Appendix A | ||
| // (TLS 1.2 Cipher Suite Black List). | ||
| // Aligned with intermediate profile of the 5.7 version of the Mozilla Server | ||
| // Side TLS guidelines found at: https://ssl-config.mozilla.org/guidelines/5.7.json | ||
| // | ||
| // Latest guidelines: https://ssl-config.mozilla.org/guidelines/latest.json | ||
| // | ||
| // This profile provides strong security with wide compatibility. | ||
| // It requires TLS 1.2+ and uses only AEAD cipher suites (GCM, ChaCha20-Poly1305) | ||
| // with ECDHE key exchange for perfect forward secrecy. | ||
| // | ||
| // All CBC-mode ciphers have been removed due to padding oracle vulnerabilities. | ||
| // All RSA key exchange ciphers have been removed due to lack of perfect forward secrecy. | ||
| // | ||
| // HTTP/2 compliance: All ciphers are compliant with RFC7540, section 9.2. | ||
| return []uint16{ | ||
| // 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, | ||
p0lyn0mial marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’m wondering how we decided which ciphers to remove. If I’m not mistaken,
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what happens if a client wants to use deprecated ciphers? Will it fail to establish a TLS connection with the server?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and if that client is our customer, what would be our response to such an incident?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, if a client only supports the removed ciphers and doesn't support any of the remaining ones, the TLS handshake will fail.
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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have reached out to the product security team.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed because of CBC encryption, right ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed because of CBC encryption, right ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed because of CBC encryption, right ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed because of CBC encryption, right ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CBC ciphers in TLS are vulnerable to padding oracle and timing attacks. |
||
| tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, // forbidden by http/2 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed because of CBC encryption, right ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CBC ciphers in TLS are vulnerable to padding oracle and timing attacks. |
||
| tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, // forbidden by http/2 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed because of CBC encryption, right ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CBC ciphers in TLS are vulnerable to padding oracle and timing attacks. |
||
| tls.TLS_RSA_WITH_AES_128_GCM_SHA256, // forbidden by http/2 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed because of RSA key exchange, right ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This suite does not provide Forward Secrecy. |
||
| tls.TLS_RSA_WITH_AES_256_GCM_SHA384, // forbidden by http/2 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed because of RSA key exchange, right ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This suite does not provide Forward Secrecy. |
||
| // the next one is in the intermediate suite, but go1.8 http2isBadCipher() complains when it is included at the recommended index | ||
| // because it comes after ciphers forbidden by the http/2 spec | ||
| // 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed because of RSA key exchange and CBC encryption, right ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This suite does not provide Forward Secrecy. |
||
| tls.TLS_RSA_WITH_AES_256_CBC_SHA, // forbidden by http/2 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed because of RSA key exchange and CBC encryption, right ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This suite does not provide Forward Secrecy. |
||
|
|
||
| // TLS 1.3 cipher suites (negotiated automatically, not configurable) | ||
| tls.TLS_AES_128_GCM_SHA256, | ||
| tls.TLS_AES_256_GCM_SHA384, | ||
| tls.TLS_CHACHA20_POLY1305_SHA256, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are we going to test if the updated list doesn't break the platform ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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: