Skip to content
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

Fixes #4113 - HttpClient fails with JDK 13 and TLS 1.3. #4114

Merged
merged 2 commits into from
Sep 23, 2019

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented Sep 23, 2019

  1. Now forwarding the fillable event rather than assuming that is due
    to garbage bytes or by a server close. This ensures that a HTTP read
    consumes the TLS bytes and the NewSessionTicket message.
  2. Avoid to set the SslConnection onto the EndPoint in
    SslClientConnectionFactory - this allows upgrades to work properly,
    for example when tunnelling through a secure proxy.

Signed-off-by: Simone Bordet simone.bordet@gmail.com

1. Now forwarding the fillable event rather than assuming that is due
to garbage bytes or by a server close. This ensures that a HTTP read
consumes the TLS bytes and the `NewSessionTicket` message.
2. Avoid to set the `SslConnection` onto the `EndPoint` in
`SslClientConnectionFactory` - this allows upgrades to work properly,
for example when tunnelling through a secure proxy.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@@ -97,7 +97,6 @@ public void setAllowMissingCloseMessage(boolean allowMissingCloseMessage)
context.put(SSL_ENGINE_CONTEXT_KEY, engine);

SslConnection sslConnection = newSslConnection(byteBufferPool, executor, endPoint, engine);
endPoint.setConnection(sslConnection);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this right?
It looks like a bug to remove this line.

Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a simple fix.
Removing the endpoint.setConnection() looks weird, but the testing proves that the change has no real impact.

I'm good with this as it stands.

@joakime joakime merged commit 943a8c6 into jetty-9.4.x Sep 23, 2019
@joakime joakime deleted the jetty-9.4.x-4113-httpclient_failure_jdk13_tls13 branch September 23, 2019 17:07
@joakime joakime added this to To do in Jetty 9.4.21 via automation Sep 23, 2019
@joakime joakime moved this from To do to Done in Jetty 9.4.21 Sep 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Jetty 9.4.21
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants