Improve ssl detail handling

neo4j · Jul 26, 2018 · af7bab5 · af7bab5
1 parent 5dc5cd4
commit af7bab5
Show file tree

Hide file tree

Showing 4 changed files with 38 additions and 26 deletions.
diff --git a/community/ssl/src/main/java/org/neo4j/ssl/OnConnectSslHandler.java b/community/ssl/src/main/java/org/neo4j/ssl/OnConnectSslHandler.java
@@ -22,10 +22,12 @@
 import io.netty.channel.Channel;
 import io.netty.channel.ChannelDuplexHandler;
 import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelInboundHandlerAdapter;
 import io.netty.channel.ChannelPipeline;
 import io.netty.channel.ChannelPromise;
 import io.netty.handler.ssl.SslContext;
 import io.netty.handler.ssl.SslHandler;
+import io.netty.handler.ssl.SslHandshakeCompletionEvent;
 
 import java.net.InetSocketAddress;
 import java.net.SocketAddress;
@@ -69,7 +71,6 @@ public void connect( ChannelHandlerContext ctx, SocketAddress remoteAddress, Soc
     {
         SslHandler sslHandler = createSslHandler( ctx, (InetSocketAddress) remoteAddress );
         replaceSelfWith( sslHandler );
-        fireHandlerReplaced( ctx, sslHandler );
         ctx.connect( remoteAddress, localAddress, promise );
     }
 
@@ -82,22 +83,9 @@ public void handlerAdded( ChannelHandlerContext ctx ) throws Exception
             SslHandler sslHandler = createSslHandler( ctx, (InetSocketAddress) ctx.channel().remoteAddress() );
             replaceSelfWith( sslHandler );
             sslHandler.handlerAdded( ctx );
-            fireHandlerReplaced( ctx, sslHandler );
         }
     }
 
-    /**
-     * An event that is used for testing
-     * @param ctx the channel context
-     * @param sslHandler the newly created sslHandler
-     */
-    private void fireHandlerReplaced( ChannelHandlerContext ctx, SslHandler sslHandler )
-    {
-        String ciphers = sslHandler.engine().getSession().getCipherSuite();
-        String protocols = sslHandler.engine().getSession().getProtocol();
-        ctx.fireUserEventTriggered( new SslHandlerReplacedEvent( ciphers, protocols ) );
-    }
-
     @Override
     public void write( ChannelHandlerContext ctx, Object msg, ChannelPromise promise ) throws Exception
     {
@@ -119,6 +107,7 @@ private void replaceSelfWith( SslHandler sslHandler )
                 .findFirst()
                 .orElseThrow( () -> new IllegalStateException( "This handler has no name" ) );
         pipeline.replace( this, myName, sslHandler );
+        pipeline.addAfter( myName, "handshakeCompletionSslDetailsHandler", new HandshakeCompletionSslDetailsHandler() );
     }
 
     private SslHandler createSslHandler( ChannelHandlerContext ctx, InetSocketAddress inetSocketAddress )
@@ -131,4 +120,30 @@ private SslHandler createSslHandler( ChannelHandlerContext ctx, InetSocketAddres
         // Don't need to set tls versions since that is set up from the context
         return new SslHandler( sslEngine );
     }
+
+    /**
+     * Ssl protocol details are negotiated after handshake is complete.
+     * Some tests rely on having these ssl details available.
+     * Having this adapter exposes those details to the tests.
+     */
+    private class HandshakeCompletionSslDetailsHandler extends ChannelInboundHandlerAdapter
+    {
+        @Override
+        public void userEventTriggered( ChannelHandlerContext ctx, Object evt ) throws Exception
+        {
+            if ( evt instanceof SslHandshakeCompletionEvent )
+            {
+                SslHandshakeCompletionEvent sslHandshakeEvent = (SslHandshakeCompletionEvent) evt;
+                if ( sslHandshakeEvent.cause() == null )
+                {
+                    SslHandler sslHandler = ctx.pipeline().get( SslHandler.class );
+                    String ciphers = sslHandler.engine().getSession().getCipherSuite();
+                    String protocols = sslHandler.engine().getSession().getProtocol();
+
+                    ctx.fireUserEventTriggered( new SslHandlerDetailsRegisteredEvent( ciphers, protocols ) );
+                }
+            }
+            ctx.fireUserEventTriggered( evt );
+        }
+    }
 }
diff --git a/...rg/neo4j/ssl/SslHandlerReplacedEvent.java → ...ssl/SslHandlerDetailsRegisteredEvent.java b/...rg/neo4j/ssl/SslHandlerReplacedEvent.java → ...ssl/SslHandlerDetailsRegisteredEvent.java
@@ -19,12 +19,12 @@
  */
 package org.neo4j.ssl;
 
-class SslHandlerReplacedEvent
+class SslHandlerDetailsRegisteredEvent
 {
     final String cipherSuite;
     final String protocol;
 
-    SslHandlerReplacedEvent( String cipherSuite, String protocol )
+    SslHandlerDetailsRegisteredEvent( String cipherSuite, String protocol )
     {
         this.cipherSuite = cipherSuite;
         this.protocol = protocol;

diff --git a/integrationtests/src/test/java/org/neo4j/ssl/SecureClient.java b/integrationtests/src/test/java/org/neo4j/ssl/SecureClient.java
@@ -169,11 +169,13 @@ protected void initChannel( SocketChannel channel ) throws Exception
                     @Override
                     public void userEventTriggered( ChannelHandlerContext ctx, Object evt ) throws Exception
                     {
-                        if ( evt instanceof SslHandlerReplacedEvent )
+                        if ( evt instanceof SslHandlerDetailsRegisteredEvent )
                         {
-                            SslHandlerReplacedEvent sslHandlerReplacedEvent = (SslHandlerReplacedEvent) evt;
-                            protocol = sslHandlerReplacedEvent.protocol;
-                            ciphers = sslHandlerReplacedEvent.cipherSuite;
+                            SslHandlerDetailsRegisteredEvent sslHandlerDetailsRegisteredEvent = (SslHandlerDetailsRegisteredEvent) evt;
+                            protocol = sslHandlerDetailsRegisteredEvent.protocol;
+                            ciphers = sslHandlerDetailsRegisteredEvent.cipherSuite;
+                            handshakeFuture.complete( ctx.channel() ); // We complete the handshake here since it will also signify that the correct
+                            // information has been carried
                             return;
                         }
                         if ( evt instanceof SslHandshakeCompletionEvent )
@@ -183,10 +185,7 @@ public void userEventTriggered( ChannelHandlerContext ctx, Object evt ) throws E
                             {
                                 handshakeFuture.completeExceptionally( handshakeEvent.cause() );
                             }
-                            else
-                            {
-                                handshakeFuture.complete( ctx.channel() );
-                            }
+                            // We do not complete if no error, that will be handled by the funky SslHandlerReplacedEvent
                         }
                     }
                 } );

diff --git a/integrationtests/src/test/java/org/neo4j/ssl/SslNegotiationTest.java b/integrationtests/src/test/java/org/neo4j/ssl/SslNegotiationTest.java
@@ -67,8 +67,6 @@ public class SslNegotiationTest
     @Parameterized.Parameter
     public TestSetup setup;
 
-    private static final LogProvider LOG_PROVIDER = NullLogProvider.getInstance();
-
     private SecureServer server;
     private SecureClient client;