Support session cache for client and server when using native SSLEngi… by normanmaurer · Pull Request #10331 · netty/netty

normanmaurer · 2020-05-29T15:48:03Z

…ne implementation

Motivation:

At the moment we don't support session caching for client side when using native SSLEngine implementation and our implementation of SSLSessionContext is incomplete.

Modification:

Consume netty-tcnative changes to be able to cache session in an external cache
Add and adjust unit tests to test session caching
Add an in memory session cache that is hooked into native SSLEngine

Result:

Support session caching on the client and server side

normanmaurer · 2020-05-29T15:48:25Z

Depends on netty/netty-tcnative#548

idelpivnitskiy

Looks promising, great work!
Will do a second review pass later

handler/src/main/java/io/netty/handler/ssl/NullOpenSslSession.java

handler/src/main/java/io/netty/handler/ssl/ExtendedOpenSslSession.java

handler/src/main/java/io/netty/handler/ssl/OpenSslSessionCache.java

handler/src/main/java/io/netty/handler/ssl/OpenSslSessionContext.java

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslClientContext.java

handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java

pom.xml

handler/src/main/java/io/netty/handler/ssl/OpenSslSessionCache.java

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslClientContext.java

handler/src/main/java/io/netty/handler/ssl/OpenSslSessionCache.java

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslServerContext.java

handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java

handler/src/main/java/io/netty/handler/ssl/DefaultOpenSslSession.java

normanmaurer · 2020-06-09T18:05:57Z

@idelpivnitskiy PTAL again

normanmaurer · 2020-06-26T15:12:30Z

@netty-bot test this please

idelpivnitskiy

LGTM, awesome work!
A few minor comments and questions:

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java

handler/src/main/java/io/netty/handler/ssl/OpenSslClientSessionCache.java

handler/src/main/java/io/netty/handler/ssl/OpenSslSessionContext.java

idelpivnitskiy · 2020-06-26T23:37:32Z

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslClientContext.java

+                context.setSessionCacheSize((int) Math.min(sessionCacheSize, Integer.MAX_VALUE));
+            }
+            if (sessionTimeout > 0) {
+                context.setSessionTimeout((int) Math.min(sessionTimeout, Integer.MAX_VALUE));


Should we throw if the requested sessionCacheSize/sessionTimeout is higher than Integer.MAX_VALUE? Or at least log it? Silent override may be confusing.

this is the same code we use for the JDK implementation. Let me think about it and make a follow up if needed to keep it consistent.

idelpivnitskiy · 2020-06-26T23:43:12Z

handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java

+        try {
+            doHandshakeVerifyReusedAndClose("a.netty.io", 9999, false);
+            // As we have a cache size of 1 we should never have more then one session in the cache
+            doHandshakeVerifyReusedAndClose("b.netty.io", 9999, false);


Can we verify that a single session available in cache is reused for a.netty.io?

handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java

idelpivnitskiy · 2020-06-26T23:49:53Z

handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java

+            // the handshake is done.
+            // See https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_sess_set_get_cb.html
+            if (!SslUtils.PROTOCOL_TLS_V1_3.equals(engine.getSession().getProtocol())) {
+                assertEquals(isReused, engine.isSessionReused());


I probably miss it somewhere, but how do you know if the session was reused via session ticket, not session id for TLSv1.2?

normanmaurer · 2020-06-28T06:21:03Z

@netty-bot test this please

handler/src/main/java/io/netty/handler/ssl/DefaultOpenSslSession.java

handler/src/main/java/io/netty/handler/ssl/OpenSslNullSession.java

normanmaurer · 2020-07-02T13:47:08Z

Factored out some bug fix in another PR that is not really related to this one #10388

normanmaurer · 2020-07-07T10:07:01Z

@netty-bot test this please

normanmaurer · 2020-07-13T06:31:37Z

@netty-bot test this please

normanmaurer · 2020-07-13T12:24:41Z

Once this build passes I will merge this one in....

…ne implementation Motivation: At the moment we don't support session caching for client side when using native SSLEngine implementation and our implementation of SSLSessionContext is incomplete. Modification: - Consume netty-tcnative changes to be able to cache session in an external cache - Add and adjust unit tests to test session caching - Add an in memory session cache that is hooked into native SSLEngine Result: Support session caching on the client and server side

…ne implementation (#10331) Motivation: At the moment we don't support session caching for client side when using native SSLEngine implementation and our implementation of SSLSessionContext is incomplete. Modification: - Consume netty-tcnative changes to be able to cache session in an external cache - Add and adjust unit tests to test session caching - Add an in memory session cache that is hooked into native SSLEngine Result: Support session caching on the client and server side

* '4.1' of github.com:netty/netty: Support session cache for client and server when using native SSLEngine implementation (netty#10331) Simple fix typo (netty#10403) Eliminate a redundant method call in HpackDynamicTable.add(...) (netty#10399) jdk.tls.client.enableSessionTicketExtension must be respected by OPENSSL and OPENSSL_REFCNT SslProviders (netty#10401)

* '4.1-read' of github.com:Kvicii/netty: (43 commits) Make the TLSv1.3 check more robust and not depend on the Java version… (netty#10409) Reduce the scope of synchronized block in PoolArena (netty#10410) Add IndexOutOfBoundsException error message (netty#10405) Add default handling for switch statement (netty#10408) Review PooledByteBufAllocator in respect of jemalloc 4.x changes and update allocate algorithm.(netty#10267) Support session cache for client and server when using native SSLEngine implementation (netty#10331) Simple fix typo (netty#10403) Eliminate a redundant method call in HpackDynamicTable.add(...) (netty#10399) jdk.tls.client.enableSessionTicketExtension must be respected by OPENSSL and OPENSSL_REFCNT SslProviders (netty#10401) 重新编译4.1分支 [maven-release-plugin] prepare for next development iteration [maven-release-plugin] prepare release netty-4.1.51.Final Correctly return NEED_WRAP if we produced some data even if we could not consume any during SSLEngine.wrap(...) (netty#10396) Modify OpenSSL native library loading to accommodate GraalVM (netty#10395) Update to netty-tcnative 2.0.31.Final and make SslErrorTest more robust (netty#10392) Add option to HttpObjectDecoder to allow duplicate Content-Lengths (netty#10349) Add detailed error message corresponding to the IndexOutOfBoundsException while calling getEntry(...) (netty#10386) Do not report ReferenceCountedOpenSslClientContext$ExtendedTrustManagerVerifyCallback.verify as blocking call (netty#10387) Add unit test for HpackDynamicTable. (netty#10389) netty用于测试的内嵌Channel | ChannelInboundHandler主要方法 ...

…ne implementation (netty#10331) Motivation: At the moment we don't support session caching for client side when using native SSLEngine implementation and our implementation of SSLSessionContext is incomplete. Modification: - Consume netty-tcnative changes to be able to cache session in an external cache - Add and adjust unit tests to test session caching - Add an in memory session cache that is hooked into native SSLEngine Result: Support session caching on the client and server side

… SSLEngine implementation (#10331)" Motivation: This reverts commit 825916c as it turns out it introduced a big performance regression. Modifications: Revert 825916c Result: Performance of TLS is back to normal

… SSLEngine implementation (#10331)" (#10528) Motivation: This reverts commit 825916c as it turns out it introduced a big performance regression. Modifications: Revert 825916c Result: Performance of TLS is back to normal

… SSLEngine implementation (#10331)" Motivation: This reverts commit 7bf1ffb as it turns out it introduced a big performance regression. Modifications: Revert 7bf1ffb Result: Performance of TLS is back to normal

…g native SSLEngine implementation (#10331)" (#10528)" This reverts commit 5157d3b.

normanmaurer requested review from Scottmitch, danbim and idelpivnitskiy and removed request for danbim May 29, 2020 15:48

normanmaurer force-pushed the session_reuse branch from c33d76d to 7e8856a Compare May 29, 2020 15:54

normanmaurer requested review from carl-mastrangelo and removed request for idelpivnitskiy June 2, 2020 14:34

idelpivnitskiy reviewed Jun 2, 2020

View reviewed changes

normanmaurer commented Jun 3, 2020

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

normanmaurer requested a review from idelpivnitskiy June 4, 2020 16:30

normanmaurer marked this pull request as ready for review June 5, 2020 07:03

idelpivnitskiy reviewed Jun 9, 2020

View reviewed changes

normanmaurer force-pushed the session_reuse branch 2 times, most recently from d92883c to c57927a Compare June 25, 2020 20:21

idelpivnitskiy approved these changes Jun 26, 2020

View reviewed changes

carl-mastrangelo reviewed Jun 30, 2020

View reviewed changes

handler/src/main/java/io/netty/handler/ssl/DefaultOpenSslSession.java Show resolved Hide resolved

handler/src/main/java/io/netty/handler/ssl/OpenSslNullSession.java Show resolved Hide resolved

normanmaurer mentioned this pull request Jul 2, 2020

Correctly include TLS1.3 ciphers in the enabled ciphersuites when usi… #10388

Merged

normanmaurer force-pushed the session_reuse branch 2 times, most recently from c943882 to e1bcbd5 Compare July 7, 2020 08:59

normanmaurer force-pushed the session_reuse branch from e1bcbd5 to 220ec3f Compare July 8, 2020 12:15

normanmaurer mentioned this pull request Jul 8, 2020

Correctly return NEED_WRAP if we produced some data even if we could … #10396

Merged

normanmaurer force-pushed the session_reuse branch 2 times, most recently from 730f023 to f8be4b2 Compare July 10, 2020 09:28

normanmaurer mentioned this pull request Jul 13, 2020

jdk.tls.client.enableSessionTicketExtension must be respected by OPEN… #10401

Merged

normanmaurer requested a review from carl-mastrangelo July 13, 2020 12:24

normanmaurer force-pushed the session_reuse branch from bc3568e to de9c85b Compare July 14, 2020 10:08

normanmaurer merged commit 825916c into 4.1 Jul 14, 2020

normanmaurer deleted the session_reuse branch July 14, 2020 10:22

normanmaurer added this to the 4.1.52.Final milestone Jul 14, 2020

normanmaurer mentioned this pull request Sep 2, 2020

Revert "Support session cache for client and server when using native… #10528

Merged

normanmaurer removed this from the 4.1.52.Final milestone Sep 8, 2020

normanmaurer added a commit that referenced this pull request Oct 2, 2020

Revert "Revert "Support session cache for client and server when usin…

62beaa5

…g native SSLEngine implementation (#10331)" (#10528)" This reverts commit 5157d3b.

normanmaurer added a commit that referenced this pull request Oct 14, 2020

Revert "Revert "Support session cache for client and server when usin…

7a728a6

…g native SSLEngine implementation (#10331)" (#10528)" This reverts commit 5157d3b.

normanmaurer added a commit that referenced this pull request Feb 2, 2021

Revert "Revert "Support session cache for client and server when usin…

d6e139d

…g native SSLEngine implementation (#10331)" (#10528)" This reverts commit 5157d3b.

normanmaurer added a commit that referenced this pull request Feb 11, 2021

Revert "Revert "Support session cache for client and server when usin…

e5d8ae2

…g native SSLEngine implementation (#10331)" (#10528)" This reverts commit 5157d3b.

Uh oh!

Conversation

normanmaurer commented May 29, 2020

Uh oh!

normanmaurer commented May 29, 2020

Uh oh!

idelpivnitskiy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

normanmaurer commented Jun 9, 2020

Uh oh!

normanmaurer commented Jun 26, 2020

Uh oh!

idelpivnitskiy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

idelpivnitskiy Jun 26, 2020

Choose a reason for hiding this comment

Uh oh!

normanmaurer Jun 29, 2020

Choose a reason for hiding this comment

Uh oh!

idelpivnitskiy Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

idelpivnitskiy Jun 26, 2020

Choose a reason for hiding this comment

Uh oh!

normanmaurer commented Jun 28, 2020

Uh oh!

Uh oh!

Uh oh!

normanmaurer commented Jul 2, 2020

Uh oh!

normanmaurer commented Jul 7, 2020

Uh oh!

normanmaurer commented Jul 13, 2020

Uh oh!

normanmaurer commented Jul 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

idelpivnitskiy Jun 26, 2020 •

edited

Loading