Hey Xuelei, I just re-ran our testsuite with your patch and everything pass except two tests. After digging a bit I found that we needed to add explicit calls to `SSLEngine.setUSeClientMode(false)` now in these test where we did not need to do this before.
The tests in question are: https://github.com/netty/netty/blob/netty-4.1.28.Final/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java#L400 <https://github.com/netty/netty/blob/netty-4.1.28.Final/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java#L400> https://github.com/netty/netty/blob/netty-4.1.28.Final/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java#L418 <https://github.com/netty/netty/blob/netty-4.1.28.Final/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java#L418> Here we use SslContext.getDefault().createSSLEngine() and did not set the mode explicitly before. With the following patch to netty all works when using your patch: diff --git a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java index e982b6a63..40d6e7b59 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java @@ -398,7 +398,9 @@ public class SslHandlerTest { @Test public void testCloseFutureNotified() throws Exception { - SslHandler handler = new SslHandler(SSLContext.getDefault().createSSLEngine()); + SSLEngine engine = SSLContext.getDefault().createSSLEngine(); + engine.setUseClientMode(false); + SslHandler handler = new SslHandler(engine); EmbeddedChannel ch = new EmbeddedChannel(handler); ch.close(); @@ -417,6 +419,7 @@ public class SslHandlerTest { @Test(timeout = 5000) public void testEventsFired() throws Exception { SSLEngine engine = SSLContext.getDefault().createSSLEngine(); + engine.setUseClientMode(false); final BlockingQueue<SslCompletionEvent> events = new LinkedBlockingQueue<SslCompletionEvent>(); EmbeddedChannel channel = new EmbeddedChannel(new SslHandler(engine), new ChannelInboundHandlerAdapter() { @Override The exception we see without the patch is: java.lang.IllegalStateException: Client/Server mode has not yet been set. at java.base/sun.security.ssl.SSLEngineImpl.beginHandshake(SSLEngineImpl.java:98) at io.netty.handler.ssl.SslHandler.handshake(SslHandler.java:1731) at io.netty.handler.ssl.SslHandler.startHandshakeProcessing(SslHandler.java:1644) at io.netty.handler.ssl.SslHandler.handlerAdded(SslHandler.java:1634) at io.netty.channel.DefaultChannelPipeline.callHandlerAdded0(DefaultChannelPipeline.java:637) at io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:235) at io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:409) at io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:396) at io.netty.channel.embedded.EmbeddedChannel$2.initChannel(EmbeddedChannel.java:203) at io.netty.channel.ChannelInitializer.initChannel(ChannelInitializer.java:115) at io.netty.channel.ChannelInitializer.handlerAdded(ChannelInitializer.java:107) at io.netty.channel.DefaultChannelPipeline.callHandlerAdded0(DefaultChannelPipeline.java:637) at io.netty.channel.DefaultChannelPipeline.access$000(DefaultChannelPipeline.java:46) at io.netty.channel.DefaultChannelPipeline$PendingHandlerAddedTask.execute(DefaultChannelPipeline.java:1487) at io.netty.channel.DefaultChannelPipeline.callHandlerAddedForAllHandlers(DefaultChannelPipeline.java:1161) at io.netty.channel.DefaultChannelPipeline.invokeHandlerAddedIfNeeded(DefaultChannelPipeline.java:686) at io.netty.channel.AbstractChannel$AbstractUnsafe.register0(AbstractChannel.java:510) at io.netty.channel.AbstractChannel$AbstractUnsafe.register(AbstractChannel.java:476) at io.netty.channel.embedded.EmbeddedChannel$EmbeddedUnsafe$1.register(EmbeddedChannel.java:773) at io.netty.channel.embedded.EmbeddedEventLoop.register(EmbeddedEventLoop.java:130) at io.netty.channel.embedded.EmbeddedEventLoop.register(EmbeddedEventLoop.java:124) at io.netty.channel.embedded.EmbeddedChannel.setup(EmbeddedChannel.java:208) at io.netty.channel.embedded.EmbeddedChannel.<init>(EmbeddedChannel.java:167) at io.netty.channel.embedded.EmbeddedChannel.<init>(EmbeddedChannel.java:148) at io.netty.channel.embedded.EmbeddedChannel.<init>(EmbeddedChannel.java:135) at io.netty.channel.embedded.EmbeddedChannel.<init>(EmbeddedChannel.java:100) at io.netty.handler.ssl.SslHandlerTest.testCloseFutureNotified(SslHandlerTest.java:404) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:566) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57) at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) at org.junit.runners.ParentRunner.run(ParentRunner.java:363) at org.junit.runner.JUnitCore.run(JUnitCore.java:137) at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68) at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47) at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242) at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70) So I have no problem to patch our test-case but I wondered if this may break others in other cases and so is a regression. Let me know what you think. Norman > On 30. Jul 2018, at 20:06, Norman Maurer <norman.mau...@googlemail.com> wrote: > > Will do and report back as soon as possible. > > Thanks > Norman > > >> On 30. Jul 2018, at 19:57, Xuelei Fan <xuelei....@oracle.com> wrote: >> >> Hi Norman, >> >> Would you mind look at the code I posted in the following thread: >> http://mail.openjdk.java.net/pipermail/security-dev/2018-July/017708.html >> >> I appreciate if you could have a test by the end of this week. >> >> Note that with this update, a complete TLS connection should close both >> inbound and outbound explicitly. However, existing applications may not did >> this way. If the source code update is not available, please consider to >> use the "jdk.tls.acknowledgeCloseNotify" as a workaround. >> >> Thanks, >> Xuelei >> >> On 7/25/2018 11:22 PM, Norman Maurer wrote: >>> Just FYI… I tested this patch via the netty ssl tests and we no longer see >>> the class-cast-exception problems I reported before dso I think this solves >>> the issue. >>> That said we still encounter a few test-failures for tests that test >>> behaviour of closing outbound of the SSLEngine but I think these are more >>> related to >>> http://mail.openjdk.java.net/pipermail/security-dev/2018-July/017633.html >>> and >>> http://mail.openjdk.java.net/pipermail/security-dev/2018-July/017566.html . >>> Bye >>> Norman >>>> On 25. Jul 2018, at 20:30, Xuelei Fan <xuelei....@oracle.com >>>> <mailto:xuelei....@oracle.com>> wrote: >>>> >>>> Hi, >>>> >>>> Please review the update for JDK-8208166: >>>> http://cr.openjdk.java.net/~xuelei/8208166/webrev.00/ >>>> <http://cr.openjdk.java.net/%7Exuelei/8208166/webrev.00/> >>>> https://bugs.openjdk.java.net/browse/JDK-8208166 >>>> >>>> Thanks, >>>> Xuelei >
