Apply Client Ordering of CipherSuites. #7452
Conversation
|
@swankjesse if we announce this change, we could also provide a utility to adopt the JDK platform defaults, for the supported set including ordering. Thoughts? |
|
Possibly relevant https://go.dev/blog/tls-cipher-suites |
# Conflicts: # okhttp/src/jvmTest/java/okhttp3/CallHandshakeTest.kt
|
@swankjesse Thoughts on this? |
| @@ -111,7 +111,7 @@ public class CipherSuiteTest { | |||
| forJavaName("TLS_FAKE_NEW_CIPHER")); | |||
| } | |||
|
|
|||
| @Test public void applyIntersectionRetainsSslPrefixes() throws Exception { | |||
| @Test public void applyIntersectionRetainsTlsPrefixes() throws Exception { | |||
There was a problem hiding this comment.
What is happening here?
There was a problem hiding this comment.
Yep, it needs to switch SSL -> TLS because it follows a different source.
| // to match legacy i.e. the platform/provider | ||
| // | ||
| // Opting for 2 here and keeping MODERN_TLS in line with secure browsers. | ||
| cipherSuitesAsString.intersect(socketEnabledCipherSuites, CipherSuite.ORDER_BY_NAME) |
There was a problem hiding this comment.
So this is the behavior change of the PR? Retaining the caller’s order rather than retaining the socket’s order? I like it.
| TLS_AES_128_GCM_SHA256, | ||
| TLS_AES_256_GCM_SHA384, |
There was a problem hiding this comment.
Is this a behavior change? I assume containsExactlyElementsOf is unordered?
There was a problem hiding this comment.
Same as containsExactly(Object...) but handles the Iterable to array conversion : verifies that actual contains exactly the elements of the given Iterable and nothing else in the same order.
| // return client.connectionSpecs.first().cipherSuites!!.map { it.javaName }.intersect(defaultEnabledCipherSuites) | ||
| return defaultEnabledCipherSuites.intersect( | ||
| client.connectionSpecs.first().cipherSuites!!.map { it.javaName }) | ||
| return client.connectionSpecs.first().cipherSuites!!.map { it.javaName }.intersect(defaultEnabledCipherSuites.toSet()) |
There was a problem hiding this comment.
... and this is the change to the test that matches the corresponding change to the production code. Makes sense.
| @@ -111,7 +111,7 @@ public class CipherSuiteTest { | |||
| forJavaName("TLS_FAKE_NEW_CIPHER")); | |||
| } | |||
|
|
|||
| @Test public void applyIntersectionRetainsSslPrefixes() throws Exception { | |||
| @Test public void applyIntersectionRetainsTlsPrefixes() throws Exception { | |||
# Conflicts: # okhttp/src/jvmTest/java/okhttp3/CallHandshakeTest.kt
Follow up to #6407
For 5.x only, apply the client specified cipher ordering, which defaults to those selected by MODERN_TLS.
This may effect performance of connections, but clients can and should override if required. Specifically it is likely to be a difference in ordering from the JDK defaults, but those have changed with JDK releases, and these should be kept inline
with modern secure browsers.