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

Possible JDK 11 fixes #4138

Merged
merged 5 commits into from Jul 13, 2018

Conversation

3 participants
@yschimke
Collaborator

yschimke commented Jul 11, 2018

Putting this up for discussion and reference, I'll raise this as a JDK bug and see fi the change is deliberate

yschimke added some commits Jul 11, 2018

@yschimke

This comment has been minimized.

Collaborator

yschimke commented Jul 11, 2018

Relates to #4119

@yschimke

This comment has been minimized.

Collaborator

yschimke commented Jul 11, 2018

Failing due to maven->groovy->asm support for JDK 11. Runs fine in Intellij.

@yschimke yschimke requested a review from swankjesse Jul 11, 2018

@@ -181,6 +183,9 @@ public void setUp() {
call.execute();
fail();
} catch (SSLHandshakeException expected) {
} catch (SSLException expected) {
String jvmVersion = System.getProperty("java.specification.version");
assertTrue(jvmVersion.equals("11"));

This comment has been minimized.

@swankjesse

swankjesse Jul 11, 2018

Member

assertEquals?

@yschimke

This comment has been minimized.

Collaborator

yschimke commented Jul 11, 2018

@swankjesse do you think I should land this? The biggest issue is ConnectionSpecSelector which changes behaviour pre JDK 11.

@swankjesse

This comment has been minimized.

Member

swankjesse commented Jul 11, 2018

I see the changed exception as a possible unintentional regression by the JDK team. My preference is to report that upstream & see if they agree.

If they're gonna fix it then we don't need to change OkHttp and widen more things to be retried. If they aren't going to fix it then we might as well land this.

@yschimke

This comment has been minimized.

Collaborator

yschimke commented Jul 12, 2018

Raised internal review ID : 9056013, will replace with a bug report when available

@yschimke yschimke added this to the 3.12 milestone Jul 12, 2018

@yschimke

This comment has been minimized.

@yschimke yschimke merged commit 9828d9d into square:master Jul 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@yschimke yschimke deleted the yschimke:jdk11_test_fixes branch Jul 13, 2018

gnastnosaj added a commit to gnastnosaj/okhttp that referenced this pull request Jul 20, 2018

@wangweij

This comment has been minimized.

wangweij commented Jul 20, 2018

OpenJDK developer here. Can you share the exact stack trace of the exception(s)?

@yschimke

This comment has been minimized.

Collaborator

yschimke commented Jul 20, 2018

@wangweij with JDK 11

java version "11-ea" 2018-09-25
Java(TM) SE Runtime Environment 18.9 (build 11-ea+22)
Java HotSpot(TM) 64-Bit Server VM 18.9 (build 11-ea+22, mixed mode)

javax.net.ssl.SSLException: Unexpected handshake message: client_hello

	at java.base/sun.security.ssl.Alert.createSSLException(Alert.java:125)
	at java.base/sun.security.ssl.Alert.createSSLException(Alert.java:116)
	at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:312)
	at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:268)
	at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:259)
	at java.base/sun.security.ssl.HandshakeContext.dispatch(HandshakeContext.java:438)
	at java.base/sun.security.ssl.HandshakeContext.dispatch(HandshakeContext.java:422)
	at java.base/sun.security.ssl.TransportContext.dispatch(TransportContext.java:178)
	at java.base/sun.security.ssl.SSLTransport.decode(SSLTransport.java:164)
	at java.base/sun.security.ssl.SSLSocketImpl.decode(SSLSocketImpl.java:877)
	at java.base/sun.security.ssl.SSLSocketImpl.readRecord(SSLSocketImpl.java:810)
	at java.base/sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:383)
	at okhttp3.DelegatingSSLSocket.startHandshake(DelegatingSSLSocket.java:89)
	at okhttp3.internal.connection.RealConnection.connectTls(RealConnection.java:318)
	at okhttp3.internal.connection.RealConnection.establishProtocol(RealConnection.java:282)
	at okhttp3.internal.connection.RealConnection.connect(RealConnection.java:167)
	at okhttp3.internal.connection.StreamAllocation.findConnection(StreamAllocation.java:257)
	at okhttp3.internal.connection.StreamAllocation.findHealthyConnection(StreamAllocation.java:135)
	at okhttp3.internal.connection.StreamAllocation.newStream(StreamAllocation.java:114)
	at okhttp3.internal.connection.ConnectInterceptor.intercept(ConnectInterceptor.java:42)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:147)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:121)
	at okhttp3.internal.cache.CacheInterceptor.intercept(CacheInterceptor.java:93)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:147)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:121)
	at okhttp3.internal.http.BridgeInterceptor.intercept(BridgeInterceptor.java:93)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:147)
	at okhttp3.internal.http.RetryAndFollowUpInterceptor.intercept(RetryAndFollowUpInterceptor.java:126)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:147)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:121)
	at okhttp3.internal.huc.OkHttpURLConnection$UnexpectedException$1.intercept(OkHttpURLConnection.java:600)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:147)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:121)
	at okhttp3.RealCall.getResponseWithInterceptorChain(RealCall.java:200)
	at okhttp3.RealCall$AsyncCall.execute(RealCall.java:147)
	at okhttp3.internal.NamedRunnable.run(NamedRunnable.java:32)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:834)
@yschimke

This comment has been minimized.

Collaborator

yschimke commented Jul 20, 2018

@wangweij with JDK 10

javax.net.ssl.SSLProtocolException: Handshake message sequence violation, 1
	at java.base/sun.security.ssl.HandshakeStateManager.check(HandshakeStateManager.java:398)
	at java.base/sun.security.ssl.ClientHandshaker.processMessage(ClientHandshaker.java:215)
	at java.base/sun.security.ssl.Handshaker.processLoop(Handshaker.java:1098)
	at java.base/sun.security.ssl.Handshaker.processRecord(Handshaker.java:1026)
	at java.base/sun.security.ssl.SSLSocketImpl.processInputRecord(SSLSocketImpl.java:1137)
	at java.base/sun.security.ssl.SSLSocketImpl.readRecord(SSLSocketImpl.java:1074)
	at java.base/sun.security.ssl.SSLSocketImpl.readRecord(SSLSocketImpl.java:973)
	at java.base/sun.security.ssl.SSLSocketImpl.performInitialHandshake(SSLSocketImpl.java:1402)
	at java.base/sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:1429)
	at java.base/sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:1413)
	at okhttp3.DelegatingSSLSocket.startHandshake(DelegatingSSLSocket.java:89)
	at okhttp3.internal.connection.RealConnection.connectTls(RealConnection.java:318)
	at okhttp3.internal.connection.RealConnection.establishProtocol(RealConnection.java:282)
	at okhttp3.internal.connection.RealConnection.connect(RealConnection.java:167)
	at okhttp3.internal.connection.StreamAllocation.findConnection(StreamAllocation.java:257)
	at okhttp3.internal.connection.StreamAllocation.findHealthyConnection(StreamAllocation.java:135)
	at okhttp3.internal.connection.StreamAllocation.newStream(StreamAllocation.java:114)
	at okhttp3.internal.connection.ConnectInterceptor.intercept(ConnectInterceptor.java:42)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:147)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:121)
	at okhttp3.internal.cache.CacheInterceptor.intercept(CacheInterceptor.java:93)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:147)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:121)
	at okhttp3.internal.http.BridgeInterceptor.intercept(BridgeInterceptor.java:93)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:147)
	at okhttp3.internal.http.RetryAndFollowUpInterceptor.intercept(RetryAndFollowUpInterceptor.java:126)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:147)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:121)
	at okhttp3.internal.huc.OkHttpURLConnection$UnexpectedException$1.intercept(OkHttpURLConnection.java:600)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:147)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:121)
	at okhttp3.RealCall.getResponseWithInterceptorChain(RealCall.java:200)
	at okhttp3.RealCall$AsyncCall.execute(RealCall.java:147)
	at okhttp3.internal.NamedRunnable.run(NamedRunnable.java:32)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1135)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.lang.Thread.run(Thread.java:844)
@wangweij

This comment has been minimized.

wangweij commented Jul 20, 2018

Thanks. We'll look into it.

@yschimke

This comment has been minimized.

Collaborator

yschimke commented Jul 20, 2018

@wangweij If observed behaviour has to change so be it. But as a client library that someone hides and simplifies this for users, as much as possible it would be great to have either

a) some stability to the same observed behaviour persists across versions. e.g. keep it the same.
b) documented clear behaviour that is determinable via types and introspectible. e.g. improve it so its actionable from JDK 11 going forward.

Thanks for looking into this.

@wangweij

This comment has been minimized.

wangweij commented Jul 20, 2018

As a preparation of TLS 1.3, we rewrote JSSE a lot and you can see many new class names in the stack trace. While the exact type of SSLException is implementation detail, we do believe it's better to make it consistent. I cannot make any guarantee but will try. Thanks for the report.

@wangweij

This comment has been minimized.

wangweij commented Jul 21, 2018

I've proposed a fix at http://mail.openjdk.java.net/pipermail/security-dev/2018-July/017670.html. If approved hopefully it will be included in EA build 24 of JDK 11, which should be available next Thursday.

@swankjesse swankjesse modified the milestones: 3.13, 3.12 Nov 4, 2018

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