Fix for Netty connection cleanup bug (#2233)

* feat: add IdleState handling for outbound connections so that they are automatically closed when idle as with inbound connections * fix: implement correct maxIdleTime in NettyOutboundConnectionHandler * fix: NettyIdleStateKeepAliveHandler should call close on connection not the channel context. Closign the connection should cascade and close both session and Netty channel * feat: add IdleState handling for outbound connections so that they are automatically closed when idle as with inbound connections * fix: implement correct maxIdleTime in NettyOutboundConnectionHandler * fix: NettyIdleStateKeepAliveHandler should call close on connection not the channel context. Closign the connection should cascade and close both session and Netty channel * OF-2559: Comment to explain hard-coded startTls(false) * fix: OF-2559 - cleanup Connection & session when channel unregistered to prevent NPE in session summary * fix: OF-2559 - add null check for connection close * Fix server-session-details NPE JSP resolvers are unable to resolve default properties on interfaces. NPEs were caused by the failure to resolve the new default methods on the ServerSession interface (e.g. `ServerSession#isUsingServerDialback()`) See this post for further details: https://stackoverflow.com/questions/35130290/property-not-found-on-type-when-using-interface-default-methods-in-jsp-el * fix: OF-2559 - fix for compression pipeline position * fix: OF-2559 - unique names for compression handlers --------- Co-authored-by: Alex Gidman <[email protected]>
igniterealtime · Aug 15, 2023 · c50cdf8 · c50cdf8
1 parent 5cd2521
commit c50cdf8
Show file tree

Hide file tree

Showing 7 changed files with 43 additions and 11 deletions.
diff --git a/xmppserver/src/main/java/org/jivesoftware/openfire/nio/NettyConnection.java b/xmppserver/src/main/java/org/jivesoftware/openfire/nio/NettyConnection.java
@@ -217,7 +217,7 @@ public void close() {
     public void close(@Nullable final StreamError error) {
         if (state.compareAndSet(State.OPEN, State.CLOSED)) {
 
-            // Ensure that the state of this connection, its session and the MINA context are eventually closed.
+            // Ensure that the state of this connection, its session and the Netty Channel are eventually closed.
 
             if (session != null) {
                 session.setStatus(Session.Status.CLOSED);
@@ -237,7 +237,7 @@ public void close(@Nullable final StreamError error) {
 
             try {
                 // TODO don't block, handle errors async with custom ChannelFutureListener
-                this.channelHandlerContext.close().addListener(ChannelFutureListener.FIRE_EXCEPTION_ON_FAILURE).sync();
+                this.channelHandlerContext.channel().close().addListener(ChannelFutureListener.FIRE_EXCEPTION_ON_FAILURE).sync();
             } catch (Exception e) {
                 Log.error("Exception while closing Netty session", e);
             }
@@ -410,13 +410,21 @@ public void startTLS(boolean clientMode, boolean directTLS) throws Exception {
     @Override
     public void addCompression() {
         // Inbound traffic only
-        channelHandlerContext.channel().pipeline().addFirst(new JZlibDecoder());
+        if (isEncrypted()) {
+            channelHandlerContext.channel().pipeline().addAfter(SSL_HANDLER_NAME, "inboundCompressionHandler", new JZlibDecoder());
+        }  else {
+            channelHandlerContext.channel().pipeline().addFirst(new JZlibDecoder());
+        }
     }
 
     @Override
     public void startCompression() {
         // Outbound traffic only
-        channelHandlerContext.channel().pipeline().addFirst(new JZlibEncoder(Z_BEST_COMPRESSION));
+        if (isEncrypted()) {
+            channelHandlerContext.channel().pipeline().addAfter(SSL_HANDLER_NAME, "outboundCompressionHandler", new JZlibEncoder(Z_BEST_COMPRESSION));
+        }  else {
+            channelHandlerContext.channel().pipeline().addFirst(new JZlibEncoder(Z_BEST_COMPRESSION));
+        }
         // Z_BEST_COMPRESSION is the same level as COMPRESSION_MAX in MINA
     }
 

diff --git a/xmppserver/src/main/java/org/jivesoftware/openfire/nio/NettyConnectionHandler.java b/xmppserver/src/main/java/org/jivesoftware/openfire/nio/NettyConnectionHandler.java
@@ -151,7 +151,16 @@ public void channelRead0(ChannelHandlerContext ctx, String message) {
     public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
         // Close the connection when an exception is raised.
         Log.error(cause.getMessage(), cause);
-        ctx.close();
+        ctx.channel().close();
+    }
+
+    @Override
+    public void channelUnregistered(ChannelHandlerContext ctx) throws Exception {
+        Connection connection = ctx.channel().attr(CONNECTION).get();
+        if (connection != null) {
+            connection.close(); // clean up resources (connection and session) when channel is unregistered.
+        }
+        super.channelUnregistered(ctx);
     }
 
     @Override

diff --git a/xmppserver/src/main/java/org/jivesoftware/openfire/nio/NettyIdleStateKeepAliveHandler.java b/xmppserver/src/main/java/org/jivesoftware/openfire/nio/NettyIdleStateKeepAliveHandler.java
@@ -81,7 +81,7 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exc
             IdleStateEvent e = (IdleStateEvent) evt;
             final boolean doPing = ConnectionSettings.Client.KEEP_ALIVE_PING_PROPERTY.getValue() && clientConnection;
             if (e.state() == IdleState.READER_IDLE) {
-                ctx.close();
+                ctx.channel().attr(CONNECTION).get().close();
             } else if (doPing && e.state() == IdleState.WRITER_IDLE) {
                 sendPingPacket(ctx);
             }

diff --git a/xmppserver/src/main/java/org/jivesoftware/openfire/nio/NettyOutboundConnectionHandler.java b/xmppserver/src/main/java/org/jivesoftware/openfire/nio/NettyOutboundConnectionHandler.java
@@ -33,6 +33,7 @@
 import org.slf4j.LoggerFactory;
 
 import java.security.cert.CertificateException;
+import java.time.Duration;
 
 /**
  * Outbound (S2S) specific ConnectionHandler that knows which subclass of {@link StanzaHandler} should be created
@@ -74,7 +75,11 @@ private static boolean configRequiresStrictCertificateValidation() {
 
     @Override
     public int getMaxIdleTime() {
-        return JiveGlobals.getIntProperty(ConnectionSettings.Server.IDLE_TIMEOUT_PROPERTY, 360);
+        return Math.toIntExact(
+            Duration.ofMillis(
+                JiveGlobals.getIntProperty(ConnectionSettings.Server.IDLE_TIMEOUT_PROPERTY,360000))
+                .toSeconds()
+        );
     }
 
     @Override

diff --git a/xmppserver/src/main/java/org/jivesoftware/openfire/nio/NettySessionInitializer.java b/xmppserver/src/main/java/org/jivesoftware/openfire/nio/NettySessionInitializer.java
@@ -22,6 +22,7 @@
 import io.netty.channel.socket.SocketChannel;
 import io.netty.channel.socket.nio.NioSocketChannel;
 import io.netty.handler.codec.string.StringEncoder;
+import io.netty.handler.timeout.IdleStateHandler;
 import org.jivesoftware.openfire.net.RespondingServerStanzaHandler;
 import org.jivesoftware.openfire.net.SocketUtil;
 import org.jivesoftware.openfire.server.ServerDialback;
@@ -86,9 +87,15 @@ public Future<LocalSession> init(ConnectionConfiguration listenerConfiguration)
             b.handler(new ChannelInitializer<SocketChannel>() {
                 @Override
                 public void initChannel(SocketChannel ch) throws Exception {
+                    NettyConnectionHandler businessLogicHandler = new NettyOutboundConnectionHandler(listenerConfiguration, domainPair, port);
+                    int maxIdleTimeBeforeClosing = businessLogicHandler.getMaxIdleTime() > -1 ? businessLogicHandler.getMaxIdleTime() : 0;
+                    int maxIdleTimeBeforePinging = maxIdleTimeBeforeClosing / 2;
+
                     ch.pipeline().addLast(new NettyXMPPDecoder());
                     ch.pipeline().addLast(new StringEncoder());
-                    ch.pipeline().addLast(new NettyOutboundConnectionHandler(listenerConfiguration, domainPair, port));
+                    ch.pipeline().addLast("idleStateHandler", new IdleStateHandler(maxIdleTimeBeforeClosing, maxIdleTimeBeforePinging, 0));
+                    ch.pipeline().addLast("keepAliveHandler", new NettyIdleStateKeepAliveHandler(false));
+                    ch.pipeline().addLast(businessLogicHandler);
                     // Should have a connection
                     if (directTLS) {
                         ch.attr(CONNECTION).get().startTLS(true, true);

diff --git a/xmppserver/src/main/java/org/jivesoftware/openfire/spi/EncryptionArtifactFactory.java b/xmppserver/src/main/java/org/jivesoftware/openfire/spi/EncryptionArtifactFactory.java
@@ -403,13 +403,16 @@ public SslContext createClientModeSslContext() throws SSLException, Unrecoverabl
         Set<String> protocols = new HashSet<>(configuration.getEncryptionProtocols());
         protocols.remove("SSLv2Hello");
 
+        // createClientModeSslContext is only used when the Openfire server is acting as a client when
+        // making outbound S2S connections so the first stanza we send should be encrypted hence startTls(false)
+
         return SslContextBuilder
             .forClient()
             .protocols(protocols)
             .ciphers(configuration.getEncryptionCipherSuites())
             .keyManager(getKeyManagerFactory())
             .trustManager(getTrustManagers()[0]) // The existing implementation never returns more than one trust manager.
-            .startTls(false)
+            .startTls(false) // Acting as client making outbound S2S connection so encrypt next stanza
             .build();
     }
 

diff --git a/xmppserver/src/main/webapp/server-session-details.jsp b/xmppserver/src/main/webapp/server-session-details.jsp
@@ -256,10 +256,10 @@
                                 </c:if>
                                 <td nowrap>
                                     <c:choose>
-                                        <c:when test="${session.usingServerDialback}">
+                                        <c:when test="${session.isUsingServerDialback()}">
                                             <fmt:message key="server.session.details.dialback"/>
                                         </c:when>
-                                        <c:when test="${session.usingSaslExternal}">
+                                        <c:when test="${session.isUsingSaslExternal()}">
                                             <fmt:message key="server.session.details.tlsauth"/>
                                         </c:when>
                                         <c:otherwise>