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

Support of TLSv1.3 by JDK Client #707

Merged
merged 2 commits into from
Jul 8, 2020
Merged

Conversation

jbescos
Copy link
Member

@jbescos jbescos commented Jun 22, 2020

Partially fixes #697

I found 2 problems:

  1. Need to regenerate certificates, because current ones has a weak algorithm that is not supported in JDK11.
  2. Handshaking has changed in TLSv1.3, and existing logic was not working fine.

Before merging this, I would like that we test it more. I guess we should try this generated jar to execute TCKs, and some more tests where they are running during days.

Signed-off-by: Jorge Bescos Gascon <[email protected]>
@jansupol jansupol changed the title Supports TLSv1.3 Supports TLSv1.3 by JDK Client Jun 29, 2020
@jansupol jansupol changed the title Supports TLSv1.3 by JDK Client Support of TLSv1.3 by JDK Client Jun 29, 2020
@jansupol jansupol merged commit d884985 into eclipse-ee4j:master Jul 8, 2020
jansupol pushed a commit to jansupol/tyrus that referenced this pull request Jul 8, 2020
Support TLSv1.3

Signed-off-by: Jorge Bescos Gascon <[email protected]>
@nuessgens
Copy link
Contributor

nuessgens commented Jul 14, 2020

Hi @jbescos ,

I was looking into #676, too (which is actually not related to java 11but rather to TLS 1.3 )
After investigating i came to the same conclusion as you did.

details

Problem is located org.glassfish.tyrus.container.jdk.client.SslFilter
During the doHandshakeStep() the handshakeFinished is already set during wrap (switch-case NEED_WRAP; wrap result with HandshakeStatus.FINISHED)
The SslFilter itself has now switched from state HANDSHAKING to DATA
But the networkData does still contain data.
Due the fact that the handshake has finished the remaining data is treated as application-data and passed to the handledRead() data.
In this method the remaining networkData is unwrapped. The unwrap result has HandshakeStatus.FINISHED, too (with 88 bytes consumed, 0 produced)
Because the unwrap-result is not HandshakeStatus.NOT_HANDSHAKING the SslFilter status switches from DATA to REHANDSHAKING.
Therefore no write is allowed and the whole HTTP/WS Upgrade procedure is not sent which in turn results in "Handshake response not received."

just a quick question:
with the given fix you implemented in case of TLSv1.3 "REHANDSHAKING" can not occure (via handleRead()) anymore (although unwrap result could contain handshake-status NEED_TASK, NEED_WRAP or NEED_UNWRAP).
Is REHANDSHAKING something which is not applicable to TLSv1.3?

edit
Or to put it differently: is the explicit check for !tlsv13 still necessary as the isHandshaking() method does handle the TLSv1.3 behaviour HandshakeStatus.FINISHED

Kind regards

@jbescos
Copy link
Member Author

jbescos commented Jul 15, 2020

@nuessgens I was checking the rehandshaking in the tlsv13 and there is a ticket reused for rehandshaking.

In TLSv13 the handshake status where there was a rehandshake was other different than NOT_HANDSHAKING and FINISHED, so the isHandshaking() was not true. I don't remember what was the status but it is easy to reproduce if you remove the !tlsv13 and print some line with the status.

My conclusion was that the rehandshake is not needed, as we have some tests that tests rehandshaking and after executing them around 1400 times it was also successful.

We also tested the TCK in Weblogic (that uses the JDK connector) and it was fine.

@jglick
Copy link

jglick commented Aug 16, 2021

Is this ever going to be released, even as a beta? The Jenkins project is waiting to pick up a fix for JENKINS-61212.

@jbescos
Copy link
Member Author

jbescos commented Aug 23, 2021

@jansupol ^^

@arjantijms
Copy link
Contributor

I'll wait a little for @jansupol, but if nothing else I can release a "-M1" of the current master.

@arjantijms
Copy link
Contributor

After discussion on the list, we should be able to get this out next week. Thanks for the patience of everyone!

@jansupol
Copy link
Contributor

jansupol commented Sep 7, 2021

Tyrus 1.18 is out.

@jansupol jansupol added this to the 1.18 milestone Sep 7, 2021
@jglick
Copy link

jglick commented Sep 7, 2021

Does #741 mean that this fix is also available in https://github.com/eclipse-ee4j/tyrus/releases/tag/2.0.1?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Be able to use TLS 1.3 with Tyrus
5 participants