From a97e413a659b8f576112eae803d3a7c6314e36f8 Mon Sep 17 00:00:00 2001 From: Leonardo Freitas Gomes Date: Sat, 14 Mar 2015 20:03:40 +0100 Subject: [PATCH] Ensure server preference order in ALPN MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Motivation: With the current implementation the client protocol preference list takes precedence over the one of the server, since the select method will return the first item, in the client list, that matches any of the protocols supported by the server. This violates the recommendation of http://tools.ietf.org/html/rfc7301#section-3.2. It will also fail with the current implementation of Chrome, which sends back Extension application_layer_protocol_negotiation, protocols: [http/1.1, spdy/3.1, h2-14] Modifications: Changed the protocol negotiator to prefer server’s list. Added a test case that demonstrates the issue and that is fixed with the modifications of this commit. Result: Server’s preference list is used. --- .../netty/handler/ssl/JdkAlpnSslEngine.java | 5 +- .../JdkBaseApplicationProtocolNegotiator.java | 5 +- .../io/netty/handler/ssl/JdkNpnSslEngine.java | 5 +- .../netty/handler/ssl/JdkSslEngineTest.java | 48 +++++++++++++++---- 4 files changed, 46 insertions(+), 17 deletions(-) diff --git a/handler/src/main/java/io/netty/handler/ssl/JdkAlpnSslEngine.java b/handler/src/main/java/io/netty/handler/ssl/JdkAlpnSslEngine.java index 580b3f23f5..012b1676bf 100644 --- a/handler/src/main/java/io/netty/handler/ssl/JdkAlpnSslEngine.java +++ b/handler/src/main/java/io/netty/handler/ssl/JdkAlpnSslEngine.java @@ -20,7 +20,7 @@ import io.netty.handler.ssl.JdkApplicationProtocolNegotiator.ProtocolSelectionLi import io.netty.handler.ssl.JdkApplicationProtocolNegotiator.ProtocolSelector; import io.netty.util.internal.PlatformDependent; -import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.List; import javax.net.ssl.SSLEngine; @@ -64,7 +64,8 @@ final class JdkAlpnSslEngine extends JdkSslEngine { if (server) { final ProtocolSelector protocolSelector = checkNotNull(applicationNegotiator.protocolSelectorFactory() - .newSelector(this, new HashSet(applicationNegotiator.protocols())), "protocolSelector"); + .newSelector(this, new LinkedHashSet(applicationNegotiator.protocols())), + "protocolSelector"); ALPN.put(engine, new ServerProvider() { @Override public String select(List protocols) { 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 aacff48426..d9d0d83f20 100644 --- a/handler/src/main/java/io/netty/handler/ssl/JdkBaseApplicationProtocolNegotiator.java +++ b/handler/src/main/java/io/netty/handler/ssl/JdkBaseApplicationProtocolNegotiator.java @@ -142,9 +142,8 @@ class JdkBaseApplicationProtocolNegotiator implements JdkApplicationProtocolNego @Override public String select(List protocols) throws Exception { - for (int i = 0; i < protocols.size(); ++i) { - String p = protocols.get(i); - if (supportedProtocols.contains(p)) { + for (String p : supportedProtocols) { + if (protocols.contains(p)) { jettyWrapper.getSession().setApplicationProtocol(p); return p; } diff --git a/handler/src/main/java/io/netty/handler/ssl/JdkNpnSslEngine.java b/handler/src/main/java/io/netty/handler/ssl/JdkNpnSslEngine.java index 4864c6b568..43d5bb584f 100644 --- a/handler/src/main/java/io/netty/handler/ssl/JdkNpnSslEngine.java +++ b/handler/src/main/java/io/netty/handler/ssl/JdkNpnSslEngine.java @@ -21,7 +21,7 @@ import io.netty.handler.ssl.JdkApplicationProtocolNegotiator.ProtocolSelectionLi import io.netty.handler.ssl.JdkApplicationProtocolNegotiator.ProtocolSelector; import io.netty.util.internal.PlatformDependent; -import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.List; import javax.net.ssl.SSLEngine; @@ -88,7 +88,8 @@ final class JdkNpnSslEngine extends JdkSslEngine { }); } else { final ProtocolSelector protocolSelector = checkNotNull(applicationNegotiator.protocolSelectorFactory() - .newSelector(this, new HashSet(applicationNegotiator.protocols())), "protocolSelector"); + .newSelector(this, new LinkedHashSet(applicationNegotiator.protocols())), + "protocolSelector"); NextProtoNego.put(engine, new ClientProvider() { @Override public boolean supports() { diff --git a/handler/src/test/java/io/netty/handler/ssl/JdkSslEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/JdkSslEngineTest.java index 847d0b0920..4df0fc00a8 100644 --- a/handler/src/test/java/io/netty/handler/ssl/JdkSslEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/JdkSslEngineTest.java @@ -17,6 +17,7 @@ package io.netty.handler.ssl; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assume.assumeNoException; import static org.mockito.Mockito.verify; @@ -61,7 +62,8 @@ import org.mockito.Mock; import org.mockito.MockitoAnnotations; public class JdkSslEngineTest { - private static final String APPLICATION_LEVEL_PROTOCOL = "my-protocol"; + private static final String PREFERRED_APPLICATION_LEVEL_PROTOCOL = "my-protocol-http2"; + private static final String FALLBACK_APPLICATION_LEVEL_PROTOCOL = "my-protocol-http1_1"; private static final String APPLICATION_LEVEL_PROTOCOL_NOT_COMPATIBLE = "my-protocol-FOO"; @Mock @@ -123,6 +125,7 @@ public class JdkSslEngineTest { clientChannel = null; serverChannel = null; serverConnectedChannel = null; + serverException = null; } @Test @@ -135,7 +138,7 @@ public class JdkSslEngineTest { throw new RuntimeException("NPN not on classpath"); } JdkApplicationProtocolNegotiator apn = new JdkNpnApplicationProtocolNegotiator(true, true, - APPLICATION_LEVEL_PROTOCOL); + PREFERRED_APPLICATION_LEVEL_PROTOCOL); mySetup(apn); runTest(); } catch (RuntimeException e) { @@ -155,7 +158,7 @@ public class JdkSslEngineTest { throw new RuntimeException("NPN not on classpath"); } JdkApplicationProtocolNegotiator clientApn = new JdkNpnApplicationProtocolNegotiator(false, false, - APPLICATION_LEVEL_PROTOCOL); + PREFERRED_APPLICATION_LEVEL_PROTOCOL); JdkApplicationProtocolNegotiator serverApn = new JdkNpnApplicationProtocolNegotiator(false, false, APPLICATION_LEVEL_PROTOCOL_NOT_COMPATIBLE); mySetup(serverApn, clientApn); @@ -177,7 +180,7 @@ public class JdkSslEngineTest { throw new RuntimeException("NPN not on classpath"); } JdkApplicationProtocolNegotiator clientApn = new JdkNpnApplicationProtocolNegotiator(true, true, - APPLICATION_LEVEL_PROTOCOL); + PREFERRED_APPLICATION_LEVEL_PROTOCOL); JdkApplicationProtocolNegotiator serverApn = new JdkNpnApplicationProtocolNegotiator(false, false, APPLICATION_LEVEL_PROTOCOL_NOT_COMPATIBLE); mySetup(serverApn, clientApn); @@ -200,7 +203,7 @@ public class JdkSslEngineTest { throw new RuntimeException("NPN not on classpath"); } JdkApplicationProtocolNegotiator clientApn = new JdkNpnApplicationProtocolNegotiator(false, false, - APPLICATION_LEVEL_PROTOCOL); + PREFERRED_APPLICATION_LEVEL_PROTOCOL); JdkApplicationProtocolNegotiator serverApn = new JdkNpnApplicationProtocolNegotiator(true, true, APPLICATION_LEVEL_PROTOCOL_NOT_COMPATIBLE); mySetup(serverApn, clientApn); @@ -223,7 +226,7 @@ public class JdkSslEngineTest { throw new RuntimeException("ALPN not on classpath"); } JdkApplicationProtocolNegotiator apn = new JdkAlpnApplicationProtocolNegotiator(true, true, - APPLICATION_LEVEL_PROTOCOL); + PREFERRED_APPLICATION_LEVEL_PROTOCOL); mySetup(apn); runTest(); } catch (Exception e) { @@ -243,7 +246,7 @@ public class JdkSslEngineTest { throw new RuntimeException("ALPN not on classpath"); } JdkApplicationProtocolNegotiator clientApn = new JdkAlpnApplicationProtocolNegotiator(false, false, - APPLICATION_LEVEL_PROTOCOL); + PREFERRED_APPLICATION_LEVEL_PROTOCOL); JdkApplicationProtocolNegotiator serverApn = new JdkAlpnApplicationProtocolNegotiator(false, false, APPLICATION_LEVEL_PROTOCOL_NOT_COMPATIBLE); mySetup(serverApn, clientApn); @@ -265,7 +268,7 @@ public class JdkSslEngineTest { throw new RuntimeException("ALPN not on classpath"); } JdkApplicationProtocolNegotiator clientApn = new JdkAlpnApplicationProtocolNegotiator(false, false, - APPLICATION_LEVEL_PROTOCOL); + PREFERRED_APPLICATION_LEVEL_PROTOCOL); JdkApplicationProtocolNegotiator serverApn = new JdkAlpnApplicationProtocolNegotiator(true, true, APPLICATION_LEVEL_PROTOCOL_NOT_COMPATIBLE); mySetup(serverApn, clientApn); @@ -278,6 +281,31 @@ public class JdkSslEngineTest { } } + @Test + public void testAlpnCompatibleProtocolsDifferentClientOrder() throws Exception { + try { + // Typical code will not have to check this, but will get a initialization error on class load. + // Check in this test just in case we have multiple tests that just the class and we already ignored the + // initialization error. + if (!JdkAlpnSslEngine.isAvailable()) { + throw new RuntimeException("ALPN not on classpath"); + } + // Even the preferred application protocol appears second in the client's list, it will be picked + // because it's the first one on server's list. + JdkApplicationProtocolNegotiator clientApn = new JdkAlpnApplicationProtocolNegotiator(false, false, + FALLBACK_APPLICATION_LEVEL_PROTOCOL, PREFERRED_APPLICATION_LEVEL_PROTOCOL); + JdkApplicationProtocolNegotiator serverApn = new JdkAlpnApplicationProtocolNegotiator(true, true, + PREFERRED_APPLICATION_LEVEL_PROTOCOL, FALLBACK_APPLICATION_LEVEL_PROTOCOL); + mySetup(serverApn, clientApn); + assertNull(serverException); + runTest(PREFERRED_APPLICATION_LEVEL_PROTOCOL); + } catch (Exception e) { + // ALPN availability is dependent on the java version. If ALPN is not available because of + // java version incompatibility don't fail the test, but instead just skip the test + assumeNoException(e); + } + } + @Test public void testAlpnNoCompatibleProtocolsClientHandshakeFailure() throws Exception { try { @@ -288,7 +316,7 @@ public class JdkSslEngineTest { throw new RuntimeException("ALPN not on classpath"); } JdkApplicationProtocolNegotiator clientApn = new JdkAlpnApplicationProtocolNegotiator(true, true, - APPLICATION_LEVEL_PROTOCOL); + PREFERRED_APPLICATION_LEVEL_PROTOCOL); JdkApplicationProtocolNegotiator serverApn = new JdkAlpnApplicationProtocolNegotiator( new ProtocolSelectorFactory() { @Override @@ -514,7 +542,7 @@ public class JdkSslEngineTest { } private void runTest() throws Exception { - runTest(APPLICATION_LEVEL_PROTOCOL); + runTest(PREFERRED_APPLICATION_LEVEL_PROTOCOL); } private void runTest(String expectedApplicationProtocol) throws Exception {