Fixes #6099 - Cipher preference may break SNI if certificates have different key types.

Updated the logic in SslContextFactory.Server.sniSelect(...) to check if there is
any certificate that matches, and if so return a null alias in the hope to be called
again and pick the right alias for the SNI.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Author: sbordet

jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SniSslConnectionFactoryTest.java

@@ -44,6 +44,7 @@
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
+import org.eclipse.jetty.http.HttpStatus;
 import org.eclipse.jetty.http.HttpTester;
 import org.eclipse.jetty.http.HttpVersion;
 import org.eclipse.jetty.io.Connection;
@@ -123,21 +124,23 @@ public void before()
 
     protected void start(String keystorePath) throws Exception
     {
-        start(ssl -> ssl.setKeyStorePath(keystorePath));
+        start(ssl ->
+        {
+            ssl.setKeyStorePath(keystorePath);
+            ssl.setKeyManagerPassword("keypwd");
+        });
     }
 
     protected void start(Consumer<SslContextFactory.Server> sslConfig) throws Exception
     {
         SslContextFactory.Server sslContextFactory = new SslContextFactory.Server();
+        sslContextFactory.setKeyStorePassword("storepwd");
         sslConfig.accept(sslContextFactory);
 
         File keystoreFile = sslContextFactory.getKeyStoreResource().getFile();
         if (!keystoreFile.exists())
             throw new FileNotFoundException(keystoreFile.getAbsolutePath());
 
-        sslContextFactory.setKeyStorePassword("OBF:1vny1zlo1x8e1vnw1vn61x8g1zlu1vn4");
-        sslContextFactory.setKeyManagerPassword("OBF:1u2u1wml1z7s1z7a1wnl1u2g");
-
         ServerConnector https = _connector = new ServerConnector(_server,
             new SslConnectionFactory(sslContextFactory, HttpVersion.HTTP_1_1.asString()),
             new HttpConnectionFactory(_httpsConfiguration));
@@ -194,6 +197,7 @@ public void testSNIConnect() throws Exception
         start(ssl ->
         {
             ssl.setKeyStorePath("src/test/resources/keystore_sni.p12");
+            ssl.setKeyManagerPassword("OBF:1u2u1wml1z7s1z7a1wnl1u2g");
             ssl.setSNISelector((keyType, issuers, session, sniHost, certificates) ->
             {
                 // Make sure the *.domain.com comes before sub.domain.com
@@ -264,6 +268,7 @@ public void testWrongSNIRejectedConnection() throws Exception
         start(ssl ->
         {
             ssl.setKeyStorePath("src/test/resources/keystore_sni.p12");
+            ssl.setKeyManagerPassword("OBF:1u2u1wml1z7s1z7a1wnl1u2g");
             // Do not allow unmatched SNI.
             ssl.setSniRequired(true);
         });
@@ -281,6 +286,7 @@ public void testWrongSNIRejectedBadRequest() throws Exception
         start(ssl ->
         {
             ssl.setKeyStorePath("src/test/resources/keystore_sni.p12");
+            ssl.setKeyManagerPassword("OBF:1u2u1wml1z7s1z7a1wnl1u2g");
             // Do not allow unmatched SNI.
             ssl.setSniRequired(false);
             _httpsConfiguration.getCustomizers().stream()
@@ -307,6 +313,7 @@ public void testWrongSNIRejectedFunction() throws Exception
         start(ssl ->
         {
             ssl.setKeyStorePath("src/test/resources/keystore_sni.p12");
+            ssl.setKeyManagerPassword("OBF:1u2u1wml1z7s1z7a1wnl1u2g");
             // Do not allow unmatched SNI.
             ssl.setSniRequired(true);
             ssl.setSNISelector((keyType, issuers, session, sniHost, certificates) ->
@@ -338,6 +345,7 @@ public void testWrongSNIRejectedConnectionWithNonSNIKeystore() throws Exception
         {
             // Keystore has only one certificate, but we want to enforce SNI.
             ssl.setKeyStorePath("src/test/resources/keystore");
+            ssl.setKeyManagerPassword("OBF:1u2u1wml1z7s1z7a1wnl1u2g");
             ssl.setSniRequired(true);
         });
 
@@ -519,6 +527,21 @@ protected void customize(Socket socket, Class<? extends Connection> connection,
         assertEquals(0, history.size());
     }
 
+    @Test
+    public void testSNIWithDifferentKeyTypes() throws Exception
+    {
+        // This KeyStore contains 2 certificates, one with keyAlg=EC, one with keyAlg=RSA.
+        start(ssl -> ssl.setKeyStorePath("src/test/resources/keystore_sni_key_types.p12"));
+
+        // Make a request with SNI = rsa.domain.com, the RSA certificate should be chosen.
+        HttpTester.Response response1 = HttpTester.parseResponse(getResponse("rsa.domain.com", "rsa.domain.com"));
+        assertEquals(HttpStatus.OK_200, response1.getStatus());
+
+        // Make a request with SNI = ec.domain.com, the EC certificate should be chosen.
+        HttpTester.Response response2 = HttpTester.parseResponse(getResponse("ec.domain.com", "ec.domain.com"));
+        assertEquals(HttpStatus.OK_200, response2.getStatus());
+    }
+
     private String getResponse(String host, String cn) throws Exception
     {
         String response = getResponse(host, host, cn);

jetty-server/src/test/resources/keystore_sni_key_types.p12

@@ -245,7 +245,7 @@ public interface SniSelector
          * <p>Selects a certificate based on SNI information.</p>
          * <p>This method may be invoked multiple times during the TLS handshake, with different parameters.
          * For example, the {@code keyType} could be different, and subsequently the collection of certificates
-         * (because they need to match the {@code keyType}.</p>
+         * (because they need to match the {@code keyType}).</p>
          *
          * @param keyType the key algorithm type name
          * @param issuers the list of acceptable CA issuer subject names or null if it does not matter which issuers are used

jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SniX509ExtendedKeyManager.java

@@ -479,6 +479,11 @@ private void unload()
         _certWilds.clear();
     }
 
+    Map<String, X509> aliasCerts()
+    {
+        return _aliasX509;
+    }
+
     @ManagedAttribute(value = "The selected TLS protocol versions", readonly = true)
     public String[] getSelectedProtocols()
     {
@@ -2118,7 +2123,7 @@ public String toString()
             _trustStoreResource);
     }
 
-    class Factory
+    private static class Factory
     {
         private final KeyStore _keyStore;
         private final KeyStore _trustStore;
@@ -2133,7 +2138,7 @@ class Factory
         }
     }
 
-    class AliasSNIMatcher extends SNIMatcher
+    static class AliasSNIMatcher extends SNIMatcher
     {
         private String _host;
 
@@ -2235,11 +2240,17 @@ public void setNeedClientAuth(boolean needClientAuth)
         }
 
         /**
-         * Does the default {@link #sniSelect(String, Principal[], SSLSession, String, Collection)} implementation
-         * require an SNI match?  Note that if a non SNI handshake is accepted, requests may still be rejected
-         * at the HTTP level for incorrect SNI (see SecureRequestCustomizer).
-         * @return true if no SNI match is handled as no certificate match, false if no SNI match is handled by
-         *         delegation to the non SNI matching methods.
+         * <p>Returns whether an SNI match is required when choosing the alias that
+         * identifies the certificate to send to the client.</p>
+         * <p>The exact logic to choose an alias given the SNI is configurable via
+         * {@link #setSNISelector(SniX509ExtendedKeyManager.SniSelector)}.</p>
+         * <p>The default implementation is {@link #sniSelect(String, Principal[], SSLSession, String, Collection)}
+         * and if SNI is not required it will delegate the TLS implementation to
+         * choose an alias (typically the first alias in the KeyStore).</p>
+         * <p>Note that if a non SNI handshake is accepted, requests may still be rejected
+         * at the HTTP level for incorrect SNI (see SecureRequestCustomizer).</p>
+         *
+         * @return whether an SNI match is required when choosing the alias that identifies the certificate
          */
         @ManagedAttribute("Whether the TLS handshake is rejected if there is no SNI host match")
         public boolean isSniRequired()
@@ -2248,13 +2259,12 @@ public boolean isSniRequired()
         }
 
         /**
-         * Set if the default {@link #sniSelect(String, Principal[], SSLSession, String, Collection)} implementation
-         * require an SNI match? Note that if a non SNI handshake is accepted, requests may still be rejected
-         * at the HTTP level for incorrect SNI (see SecureRequestCustomizer).
-         * This setting may have no effect if {@link #sniSelect(String, Principal[], SSLSession, String, Collection)} is
-         * overridden or a non null function is passed to {@link #setSNISelector(SniX509ExtendedKeyManager.SniSelector)}.
-         * @param sniRequired true if no SNI match is handled as no certificate match, false if no SNI match is handled by
-         *                    delegation to the non SNI matching methods.
+         * <p>Sets whether an SNI match is required when choosing the alias that
+         * identifies the certificate to send to the client.</p>
+         * <p>This setting may have no effect if {@link #sniSelect(String, Principal[], SSLSession, String, Collection)} is
+         * overridden or a custom function is passed to {@link #setSNISelector(SniX509ExtendedKeyManager.SniSelector)}.</p>
+         *
+         * @param sniRequired whether an SNI match is required when choosing the alias that identifies the certificate
          */
         public void setSniRequired(boolean sniRequired)
         {
@@ -2297,7 +2307,7 @@ public String sniSelect(String keyType, Principal[] issuers, SSLSession session,
             if (sniHost == null)
             {
                 // No SNI, so reject or delegate.
-                return _sniRequired ? null : SniX509ExtendedKeyManager.SniSelector.DELEGATE;
+                return isSniRequired() ? null : SniX509ExtendedKeyManager.SniSelector.DELEGATE;
             }
             else
             {
@@ -2306,9 +2316,15 @@ public String sniSelect(String keyType, Principal[] issuers, SSLSession session,
                     .filter(x509 -> x509.matches(sniHost))
                     .collect(Collectors.toList());
 
-                // No match, let the JDK decide unless unmatched SNIs are rejected.
                 if (matching.isEmpty())
-                    return isSniRequired() ? null : SniX509ExtendedKeyManager.SniSelector.DELEGATE;
+                {
+                    // There is no match for this SNI among the certificates valid for
+                    // this keyType; check if there is any certificate that matches this
+                    // SNI, as we will likely be called again with a different keyType.
+                    boolean anyMatching = aliasCerts().values().stream()
+                        .anyMatch(x509 -> x509.matches(sniHost));
+                    return isSniRequired() || anyMatching ? null : SniX509ExtendedKeyManager.SniSelector.DELEGATE;
+                }
 
                 String alias = matching.get(0).getAlias();
                 if (matching.size() == 1)