From bb1833f22c1e43cd5ea2d75fbc2c5587d062ac04 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Thu, 28 Sep 2017 09:16:03 +0200 Subject: [PATCH] Fix Java9SslEngine implementation of ApplicationProtocolAccessor and so fix ApplicationProtocolNegationHandler Motivation: Java9SslEngine did not correctly implement ApplicationProtocolAccessor and so returned an empty String when no extension was used while the interface contract is to return null. This lead to the situation that ApplicationProtocolNegationHandler did not correct work. Modifications: - Rename ApplicationProtocolAccessor.getApplicationProtocol() to getNegotiatedApplicationProtocol() which resolves the clash with the method exposed by Java9s SSLEngine. - Correctly implement getNegotiatedApplicationProtocol() for Java9Sslengine - Add delegate in Java9Sslengine.getApplicationProtocol() which is provided by Java9 - Adjust tests to test the correct behaviour. Result: Fixes [#7251]. --- .../handler/ssl/ApplicationProtocolAccessor.java | 2 +- .../java/io/netty/handler/ssl/Java9SslEngine.java | 14 +++++++++++--- .../ssl/JdkBaseApplicationProtocolNegotiator.java | 10 +++++----- .../java/io/netty/handler/ssl/JdkSslEngine.java | 4 ++-- .../ssl/ReferenceCountedOpenSslEngine.java | 2 +- .../java/io/netty/handler/ssl/SslHandler.java | 3 +-- .../java/io/netty/handler/ssl/SSLEngineTest.java | 15 +++++++++++---- 7 files changed, 32 insertions(+), 18 deletions(-) diff --git a/handler/src/main/java/io/netty/handler/ssl/ApplicationProtocolAccessor.java b/handler/src/main/java/io/netty/handler/ssl/ApplicationProtocolAccessor.java index 8835bc810a..1db0e1408a 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ApplicationProtocolAccessor.java +++ b/handler/src/main/java/io/netty/handler/ssl/ApplicationProtocolAccessor.java @@ -26,5 +26,5 @@ interface ApplicationProtocolAccessor { * @return the application-level protocol name or * {@code null} if the negotiation failed or the client does not have ALPN/NPN extension */ - String getApplicationProtocol(); + String getNegotiatedApplicationProtocol(); } diff --git a/handler/src/main/java/io/netty/handler/ssl/Java9SslEngine.java b/handler/src/main/java/io/netty/handler/ssl/Java9SslEngine.java index 6534f39b50..5910855b41 100644 --- a/handler/src/main/java/io/netty/handler/ssl/Java9SslEngine.java +++ b/handler/src/main/java/io/netty/handler/ssl/Java9SslEngine.java @@ -150,17 +150,25 @@ final class Java9SslEngine extends JdkSslEngine { } @Override - void setApplicationProtocol(String applicationProtocol) { + void setNegotiatedApplicationProtocol(String applicationProtocol) { // Do nothing as this is handled internally by the Java9 implementation of SSLEngine. } @Override - public String getApplicationProtocol() { - return Java9SslUtils.getApplicationProtocol(getWrappedEngine()); + public String getNegotiatedApplicationProtocol() { + String protocol = getApplicationProtocol(); + if (protocol != null) { + return protocol.isEmpty() ? null : protocol; + } + return protocol; } // These methods will override the methods defined by Java 9. As we compile with Java8 we can not add // @Override annotations here. + public String getApplicationProtocol() { + return Java9SslUtils.getApplicationProtocol(getWrappedEngine()); + } + public String getHandshakeApplicationProtocol() { return Java9SslUtils.getHandshakeApplicationProtocol(getWrappedEngine()); } diff --git a/handler/src/main/java/io/netty/handler/ssl/JdkBaseApplicationProtocolNegotiator.java b/handler/src/main/java/io/netty/handler/ssl/JdkBaseApplicationProtocolNegotiator.java index 8e20c9b1b1..068c494601 100644 --- a/handler/src/main/java/io/netty/handler/ssl/JdkBaseApplicationProtocolNegotiator.java +++ b/handler/src/main/java/io/netty/handler/ssl/JdkBaseApplicationProtocolNegotiator.java @@ -137,14 +137,14 @@ class JdkBaseApplicationProtocolNegotiator implements JdkApplicationProtocolNego @Override public void unsupported() { - engineWrapper.setApplicationProtocol(null); + engineWrapper.setNegotiatedApplicationProtocol(null); } @Override public String select(List protocols) throws Exception { for (String p : supportedProtocols) { if (protocols.contains(p)) { - engineWrapper.setApplicationProtocol(p); + engineWrapper.setNegotiatedApplicationProtocol(p); return p; } } @@ -152,7 +152,7 @@ class JdkBaseApplicationProtocolNegotiator implements JdkApplicationProtocolNego } public String noSelectMatchFound() throws Exception { - engineWrapper.setApplicationProtocol(null); + engineWrapper.setNegotiatedApplicationProtocol(null); return null; } } @@ -179,13 +179,13 @@ class JdkBaseApplicationProtocolNegotiator implements JdkApplicationProtocolNego @Override public void unsupported() { - engineWrapper.setApplicationProtocol(null); + engineWrapper.setNegotiatedApplicationProtocol(null); } @Override public void selected(String protocol) throws Exception { if (supportedProtocols.contains(protocol)) { - engineWrapper.setApplicationProtocol(protocol); + engineWrapper.setNegotiatedApplicationProtocol(protocol); } else { noSelectedMatchFound(protocol); } diff --git a/handler/src/main/java/io/netty/handler/ssl/JdkSslEngine.java b/handler/src/main/java/io/netty/handler/ssl/JdkSslEngine.java index 1cc3bdfa32..3304e38485 100644 --- a/handler/src/main/java/io/netty/handler/ssl/JdkSslEngine.java +++ b/handler/src/main/java/io/netty/handler/ssl/JdkSslEngine.java @@ -33,11 +33,11 @@ class JdkSslEngine extends SSLEngine implements ApplicationProtocolAccessor { } @Override - public String getApplicationProtocol() { + public String getNegotiatedApplicationProtocol() { return applicationProtocol; } - void setApplicationProtocol(String applicationProtocol) { + void setNegotiatedApplicationProtocol(String applicationProtocol) { this.applicationProtocol = applicationProtocol; } diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java index 2670e53b23..0c4bf5c2d4 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java +++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java @@ -1829,7 +1829,7 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc } @Override - public String getApplicationProtocol() { + public String getNegotiatedApplicationProtocol() { return applicationProtocol; } diff --git a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java index 3724b77271..ac32c9c76f 100644 --- a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java +++ b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java @@ -66,7 +66,6 @@ import javax.net.ssl.SSLEngineResult; import javax.net.ssl.SSLEngineResult.HandshakeStatus; import javax.net.ssl.SSLEngineResult.Status; import javax.net.ssl.SSLException; -import javax.net.ssl.SSLSession; import static io.netty.buffer.ByteBufUtil.ensureWritableSuccess; import static io.netty.handler.ssl.SslUtils.getEncryptedPacketLength; @@ -578,7 +577,7 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH return null; } - return ((ApplicationProtocolAccessor) engine).getApplicationProtocol(); + return ((ApplicationProtocolAccessor) engine).getNegotiatedApplicationProtocol(); } /** diff --git a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java index 0ab9456b0f..43f1c781ab 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java @@ -42,6 +42,7 @@ import io.netty.util.concurrent.Future; import io.netty.util.concurrent.Promise; import io.netty.util.internal.EmptyArrays; import io.netty.util.internal.PlatformDependent; +import io.netty.util.internal.StringUtil; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -996,10 +997,8 @@ public abstract class SSLEngineTest { try { writeAndVerifyReceived(clientMessage.retain(), clientChannel, serverLatch, serverReceiver); writeAndVerifyReceived(serverMessage.retain(), serverConnectedChannel, clientLatch, clientReceiver); - if (expectedApplicationProtocol != null) { - verifyApplicationLevelProtocol(clientChannel, expectedApplicationProtocol); - verifyApplicationLevelProtocol(serverConnectedChannel, expectedApplicationProtocol); - } + verifyApplicationLevelProtocol(clientChannel, expectedApplicationProtocol); + verifyApplicationLevelProtocol(serverConnectedChannel, expectedApplicationProtocol); } finally { clientMessage.release(); serverMessage.release(); @@ -1011,6 +1010,14 @@ public abstract class SSLEngineTest { assertNotNull(handler); String appProto = handler.applicationProtocol(); assertEquals(appProto, expectedApplicationProtocol); + + SSLEngine engine = handler.engine(); + if (engine instanceof Java9SslEngine) { + // Also verify the Java9 exposed method. + Java9SslEngine java9SslEngine = (Java9SslEngine) engine; + assertEquals(expectedApplicationProtocol == null ? StringUtil.EMPTY_STRING : expectedApplicationProtocol, + java9SslEngine.getApplicationProtocol()); + } } private static void writeAndVerifyReceived(ByteBuf message, Channel sendChannel, CountDownLatch receiverLatch,