Ensure server preference order in ALPN

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
https://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.

handler/src/main/java/io/netty/handler/ssl/JdkAlpnSslEngine.java

@@ -20,7 +20,7 @@
 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 @@ private static void updateAvailability() {
 
  if (server) {
  final ProtocolSelector protocolSelector = checkNotNull(applicationNegotiator.protocolSelectorFactory()
- .newSelector(this, new HashSet<String>(applicationNegotiator.protocols())), "protocolSelector");
+ .newSelector(this, new LinkedHashSet<String>(applicationNegotiator.protocols())),
+ "protocolSelector");
  ALPN.put(engine, new ServerProvider() {
  @Override
  public String select(List<String> protocols) {

handler/src/main/java/io/netty/handler/ssl/JdkBaseApplicationProtocolNegotiator.java

@@ -142,9 +142,8 @@ public void unsupported() {
 
  @Override
  public String select(List<String> 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;
  }

handler/src/main/java/io/netty/handler/ssl/JdkNpnSslEngine.java

@@ -21,7 +21,7 @@
 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 @@ public void protocolSelected(String protocol) {
  });
  } else {
  final ProtocolSelector protocolSelector = checkNotNull(applicationNegotiator.protocolSelectorFactory()
- .newSelector(this, new HashSet<String>(applicationNegotiator.protocols())), "protocolSelector");
+ .newSelector(this, new LinkedHashSet<String>(applicationNegotiator.protocols())),
+ "protocolSelector");
  NextProtoNego.put(engine, new ClientProvider() {
  @Override
  public boolean supports() {

handler/src/test/java/io/netty/handler/ssl/JdkSslEngineTest.java

@@ -17,6 +17,7 @@
 
 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.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 void tearDown() throws InterruptedException {
  clientChannel = null;
  serverChannel = null;
  serverConnectedChannel = null;
+ serverException = null;
  }
 
  @Test
@@ -135,7 +138,7 @@ public void testNpn() throws Exception {
  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 void testNpnNoCompatibleProtocolsNoHandshakeFailure() throws Exception {
  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 void testNpnNoCompatibleProtocolsClientHandshakeFailure() throws Exceptio
  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 void testNpnNoCompatibleProtocolsServerHandshakeFailure() throws Exceptio
  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 void testAlpn() throws Exception {
  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 void testAlpnNoCompatibleProtocolsNoHandshakeFailure() throws Exception {
  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 void testAlpnNoCompatibleProtocolsServerHandshakeFailure() throws Excepti
  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 void testAlpnNoCompatibleProtocolsServerHandshakeFailure() throws Excepti
  }
  }
 
+ @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 void testAlpnNoCompatibleProtocolsClientHandshakeFailure() throws Excepti
  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 void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws E
  }
 
  private void runTest() throws Exception {
- runTest(APPLICATION_LEVEL_PROTOCOL);
+ runTest(PREFERRED_APPLICATION_LEVEL_PROTOCOL);
  }
 
  private void runTest(String expectedApplicationProtocol) throws Exception {