Added new defaults for cipher suites and protocols. #928
Conversation
| @@ -0,0 +1,120 @@ | |||
| /* | |||
| * Copyright (C) 2013 Square, Inc. | |||
|
(CLA is good from July 2013) |
| ENABLED_PROTOCOLS)); | ||
| } | ||
|
|
||
| private String[] intersect(String[] supported, String[] enabled) { |
There was a problem hiding this comment.
This is cool! But it makes a lot of garbage. You can probably make things better by calling this from the constructor rather than from each place that it's used.
There was a problem hiding this comment.
That said, don't worry about it. We can probably optimize this in a follow-up change.
|
If you can document the source of the protocols, I think we're good to go. I want to sit on this until OkHttp 2.1, because if we try to put it in OkHttp 2.0 we'll not be able to ship that next week. |
| String[] intersect(String[] supported, String[] enabled) { | ||
| List<String> supportedList = new ArrayList<String>(Arrays.asList(supported)); | ||
|
|
||
| supportedList.retainAll(Arrays.asList(enabled)); |
There was a problem hiding this comment.
I think this is backwards? The enabled list is in priority order; shouldn't we want to maintain that order? Not entirely certain, though.
There was a problem hiding this comment.
Yep, You're right - I've turned it around. Although supported protocols do not maintain order.
|
|
||
| public class OkSslFactoryTest { | ||
|
|
||
| private MockWebServer server = new MockWebServer(); |
There was a problem hiding this comment.
Use MockWebServerRule
| sslSocket.setEnabledCipherSuites(intersectedCiphers); | ||
| } | ||
|
|
||
| private void applyProtocolOrder(SSLSocket sslSocket) { |
There was a problem hiding this comment.
I don't think this and the above method add value and they're only called once. Inline into the caller.
There was a problem hiding this comment.
Preventing 65k method limit in dex? ;)
- Corrected Indent - Closing socket - Added MockWebServerRule - Refined test
|
Can we tie this in to somehow help address #959? either by instructions, or automagically? |
|
"Clients MAY advertise support of other cipher suites in order to An implementation SHOULD NOT negotiate a TLS connection for HTTP/2 As far as I understand we can leave protocols as they are but we need to prevent establishing TLS connection for HTTP/2 |
|
@swankjesse How Can I rerun travis build? |
|
It passed on JDK 7. JDK 8 is broken. You should really squash all these commits into one, though. |
|
We're now tracking Chrome 37's suites. |
|
@swankjesse what about protocols? According to latest news we should also disable sslv3. Without this PR this can be problematic in current codebase. |
|
Good point! I think the fix is to add |
|
@swankjesse It will work for legacy servers supporting only SSLv3. What if other security breach will come up in the future? Shouldn't we have some API to disable ciphers / protocols in the future? You are main architects in this project so decision is up to You :) According to |
|
I'd like to add more methods to |
Added new defaults for cipher suites and protocols.
#330
#919