Fixes #5379 - Better handling for wrong SNI. (#5398)

* Fixes #5379 - Better handling for wrong SNI.

Reworked the SNI logic.
Added support for IP addresses in the SAN extension of certificates in the X509 class.
Fixed keystores to have CN=localhost and SAN with ip=127.0.0.1 and ip=[::1].
Fixed tests that were not using the correct Host header.

Signed-off-by: Simone Bordet <[email protected]>

jetty-client/src/test/java/org/eclipse/jetty/client/HostnameVerificationTest.java

@@ -27,10 +27,14 @@
 
 import org.eclipse.jetty.client.http.HttpClientTransportOverHTTP;
 import org.eclipse.jetty.io.ClientConnector;
+import org.eclipse.jetty.server.HttpConfiguration;
+import org.eclipse.jetty.server.HttpConnectionFactory;
 import org.eclipse.jetty.server.NetworkConnector;
 import org.eclipse.jetty.server.Request;
+import org.eclipse.jetty.server.SecureRequestCustomizer;
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.server.ServerConnector;
+import org.eclipse.jetty.server.SslConnectionFactory;
 import org.eclipse.jetty.server.handler.DefaultHandler;
 import org.eclipse.jetty.util.ssl.SslContextFactory;
 import org.eclipse.jetty.util.thread.QueuedThreadPool;
@@ -49,7 +53,7 @@
  */
 public class HostnameVerificationTest
 {
-    private SslContextFactory.Client clientSslContextFactory = new SslContextFactory.Client();
+    private final SslContextFactory.Client clientSslContextFactory = new SslContextFactory.Client();
     private Server server;
     private HttpClient client;
     private NetworkConnector connector;
@@ -64,7 +68,13 @@ public void setUp() throws Exception
         SslContextFactory.Server serverSslContextFactory = new SslContextFactory.Server();
         serverSslContextFactory.setKeyStorePath("src/test/resources/keystore.p12");
         serverSslContextFactory.setKeyStorePassword("storepwd");
-        connector = new ServerConnector(server, serverSslContextFactory);
+        HttpConfiguration httpConfig = new HttpConfiguration();
+        SecureRequestCustomizer customizer = new SecureRequestCustomizer();
+        customizer.setSniHostCheck(false);
+        httpConfig.addCustomizer(customizer);
+        HttpConnectionFactory http = new HttpConnectionFactory(httpConfig);
+        SslConnectionFactory ssl = new SslConnectionFactory(serverSslContextFactory, http.getProtocol());
+        connector = new ServerConnector(server, 1, 1, ssl, http);
         server.addConnector(connector);
         server.setHandler(new DefaultHandler()
         {
@@ -102,14 +112,17 @@ public void tearDown() throws Exception
 
     /**
      * This test is supposed to verify that hostname verification works as described in:
-     * http://www.ietf.org/rfc/rfc2818.txt section 3.1. It uses a certificate with a common name different to localhost
-     * and sends a request to localhost. This should fail with an SSLHandshakeException.
+     * http://www.ietf.org/rfc/rfc2818.txt section 3.1.
+     * It uses a certificate with a common name "localhost" and SAN=127.0.0.1,
+     * and sends a request to 127.0.0.2.
+     * This should fail with on the client an SSLHandshakeException, because SNI
+     * host checking on the server side is disabled.
      */
     @Test
     public void simpleGetWithHostnameVerificationEnabledTest()
     {
         clientSslContextFactory.setEndpointIdentificationAlgorithm("HTTPS");
-        String uri = "https://localhost:" + connector.getLocalPort() + "/";
+        String uri = "https://127.0.0.2:" + connector.getLocalPort() + "/";
 
         ExecutionException x = assertThrows(ExecutionException.class, () -> client.GET(uri));
         Throwable cause = x.getCause();

jetty-client/src/test/resources/readme_keystores.txt

@@ -1,9 +1,9 @@
-Since OpenJDK 13.0.2/11.0.6 it is required that CA certificates have the extension CA=true.
+Since OpenJDK 13.0.2/11.0.6 it is required that CA certificates have the extension "bc=ca:true".
 
 The keystores are generated in the following way:
 
 # Generates the server keystore. Note the BasicConstraint=CA:true extension.
-$ keytool -v -genkeypair -validity 36500 -keyalg RSA -keysize 2048 -keystore keystore.p12 -storetype pkcs12 -dname "CN=server, OU=Jetty, O=Webtide, L=Omaha, S=NE, C=US" -ext BC=CA:true
+$ keytool -v -genkeypair -validity 36500 -keyalg RSA -keysize 2048 -keystore keystore.p12 -storetype pkcs12 -dname "CN=localhost, OU=Jetty, O=Webtide, L=Omaha, S=NE, C=US" -ext bc=ca:true -ext san=ip:127.0.0.1,ip:[::1]
 
 # Export the server certificate.
 $ keytool -v -export -keystore keystore.p12 -rfc -file server.crt

jetty-documentation/src/main/asciidoc/operations-guide/keystore/keystore-create.adoc

@@ -57,4 +57,7 @@ For example, if you have bought domains `domain.com` and `domain.org`, you want
 
 Furthermore, to specify additional domains or subdomains within the same certificate, you must specify the SAN extension.
 In the example above, `san=dns:www.domain.com,dns:domain.org` specifies `www.domain.com` and `domain.org` as alternative names for your web applications (that you can configure using xref:og-deploy-virtual-hosts[virtual hosts]).
+
+In rare cases, you may want to specify IP addresses, rather than domains, in the SAN extension.
+The syntax in such case is `san=ip:127.0.0.1,ip:[::1]`, which specifies as subject alternative names IPv4 `127.0.0.1` and IPv6 `[::1]`.
 ====

jetty-documentation/src/main/asciidoc/operations-guide/protocols/protocols-ssl.adoc

@@ -195,8 +195,10 @@ The KeyStore contains two certificates, one for `one.com` and one for `two.net`.
 
 There are three `ssl` module properties that control the SNI behavior on the server: one that works at the TLS level, and two that works at the HTTP level.
 
+The property that works at the TLS level is:
+
 `jetty.sslContext.sniRequired`::
-This is the TLS level property, defaults to `false`.
+Whether SNI is required at the TLS level, defaults to `false`.
 
 Its behavior is explained by the following table:
 
@@ -222,54 +224,77 @@ Its behavior is explained by the following table:
 
 |===
 
-WARNING: The _default certificate_ is the certificate returned by the TLS implementation in case there is no SNI match, and you should not rely on this certificate to be the same across Java vendors and versions, or Jetty versions, or TLS provider vendors and versions.
+[WARNING]
+====
+The _default certificate_ is the certificate returned by the TLS implementation in case there is no SNI match, and you should not rely on this certificate to be the same across Java vendors and versions, or Jetty versions, or TLS provider vendors and versions.
+
+In the example above it could be either the `one.com` certificate or the `two.net` certificate.
+====
+
+When `jetty.sslContext.sniRequired=true`, clients that don't send a valid SNI receive a TLS failure, and their attempt to connect to the server fails.
+The details of this failure may not be reported and could be difficult to figure out that the failure is related to an invalid SNI.
+
+For this reason, other two properties are defined at the HTTP level, so that clients can received an HTTP 400 response with more details about what went wrong while trying to connect to the server:
 
 `jetty.ssl.sniRequired`::
 Whether SNI is required at the HTTP level, defaults to `false`.
-`jetty.ssl.sniHostCheck`::
-Whether the SNI domain must be verified at the HTTP level against the `Host` header, defaults to `true`.
 
-Their combined behavior is explained by the following table.
+Its behavior is similar to the `jetty.sslContext.sniRequired` property above, and is explained by the following table:
 
-.Behavior of the `jetty.ssl.[sniRequired|sniHostCheck]` properties
-[cols="5*a"]
+.Behavior of the `jetty.ssl.sniRequired` property
+[cols=3*a]
 |===
 
 |
-| `sniRequired=false` +
-  `sniHostCheck=false`
-
-| `sniRequired=false` +
-  `sniHostCheck=true`
-
-| `sniRequired=true` +
-  `sniHostCheck=false`
-
-| `sniRequired=true` +
-  `sniHostCheck=true`
+| `sniRequired=false`
+| `sniRequired=true`
 
 | SNI = `null`
-| 200 OK
-| 200 OK
-| 400 Bad Request
-| 400 Bad Request
+| Accept
+| Reject: 400 Bad Request
 
 | SNI = `wrong.org`
-| 200 OK
-| 400 Bad Request
-| 400 Bad Request
-| 400 Bad Request
+| Accept
+| Reject: 400 Bad Request
 
 | SNI = `one.com`
-| 200 OK
-| SNI matches `Host` header ? +
-200 OK : 400 Bad Request
-| 200 OK
-| SNI matches `Host` header ? +
-200 OK : 400 Bad Request
+| Accept
+| Accept
+
+|===
+
+When `jetty.ssl.sniRequired=true`, the SNI is matched against the certificate sent to the client, and only if there is a match the request is accepted.
 
+When the request is accepted, there could be an additional check controlled by the following property:
+
+`jetty.ssl.sniHostCheck`::
+Whether the certificate sent to the client matches the `Host` header, defaults to `true`.
+
+Its behavior is explained by the following table:
+
+.Behavior of the `jetty.ssl.sniHostCheck` property
+[cols="3*a"]
 |===
 
-In the normal case, with modern TLS clients and HTTP requests with a correct `Host` header, Jetty will pick the correct certificate from the KeyStore based on the SNI and respond with an HTTP 200 OK.
+|
+| `sniHostCheck=false`
+| `sniHostCheck=true`
+
+| certificate = `one.com` +
+`Host: wrong.org`
+| Accept
+| Reject: 400 Bad Request
+
+| certificate = `one.com` +
+`Host: one.com`
+| Accept
+| Accept
+
+|===
+
+In the normal case with the default server configuration, for a TLS clients that sends SNI, and then sends an HTTP request with the correct `Host` header, Jetty will pick the correct certificate from the KeyStore based on the SNI received from the client, and accept the request.
+
+Accepting the request does not mean that the request is responded with an HTTP 200 OK, but just that the request passed successfully the SNI checks and will be processed by the server.
+If the request URI is for a resource that does not exist, the response will likely be a 404 Not Found.
 
 You may modify the default values of the SNI properties if you want stricter control over old/broken TLS clients or bad HTTP requests.

jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ForwardProxyTLSServerTest.java

@@ -646,35 +646,21 @@ private void testProxyAuthentication(SslContextFactory.Server proxyTLS, ConnectH
     @Test
     public void testBothProxyAndServerNeedClientAuth() throws Exception
     {
-        // Keystore server_keystore.p12 contains:
-        // - alias "mykey": self-signed certificate with private key.
-        // - alias "client_root": certificate from client_keystore.p12 under the "server" alias.
-        // Keystore proxy_keystore.p12 contains:
-        // - alias "mykey": self-signed certificate with private key.
-        // - alias "client_root": certificate from client_keystore.p12 under the "proxy" alias.
-        // Keystore client_keystore.p12 contains:
-        // - alias "proxy": self-signed certificate with private key to send to the proxy.
-        // - alias "server": self-signed certificate with private key to send to the server.
-        // - alias "proxy_root": certificate from proxy_keystore under the "mykey" alias.
-        // - alias "server_root": certificate from server_keystore under the "mykey" alias.
-
-        // We want setEndpointIdentificationAlgorithm(null) for all 3 SslContextFactory
-        // because the certificate common names do not match the host names.
+        // See src/test/resources/readme_keystores.txt.
 
         SslContextFactory.Server serverTLS = newServerSslContextFactory();
-        serverTLS.setEndpointIdentificationAlgorithm(null);
         serverTLS.setNeedClientAuth(true);
         startTLSServer(serverTLS, new ServerHandler());
         int serverPort = serverConnector.getLocalPort();
-        String serverAlias = "server";
 
         SslContextFactory.Server proxyTLS = newProxySslContextFactory();
         proxyTLS.setEndpointIdentificationAlgorithm(null);
         proxyTLS.setNeedClientAuth(true);
         startProxy(proxyTLS);
         int proxyPort = proxyConnector.getLocalPort();
-        String proxyAlias = "proxy";
 
+        String proxyAlias = "client_to_proxy";
+        String serverAlias = "client_to_server";
         SslContextFactory.Client clientSslContextFactory = new SslContextFactory.Client()
         {
             @Override

jetty-proxy/src/test/resources/readme_keystores.txt

@@ -0,0 +1,22 @@
+The keystores are generated differently from jetty-client's readme_keystores.txt.
+
+Since SslContextFactory also loads the KeyStore as a TrustStore, rather than doing
+CSR for the client certificates and sign them with the server|proxy certificate,
+we just load the client certificates in the server|proxy KeyStores so that they
+are trusted.
+
+Structure is the following:
+
+server_keystore.p12:
+  mykey: self-signed certificate with private key
+  client: certificate from client_keystore.p12@client_to_server
+
+proxy_keystore.p12:
+  mykey: self-signed certificate with private key
+  client: certificate from client_keystore.p12@client_to_proxy
+
+client_keystore.p12
+  client_to_proxy: self-signed certificate with private key (client certificate to send to proxy)
+  client_to_server: self-signed certificate with private key (client certificate to send to server)
+  proxy: certificate from proxy_keystore.p12@mykey (to trust proxy certificate)
+  server: certificate from server_keystore.p12@mykey (to trust server certificate)

jetty-server/src/main/java/org/eclipse/jetty/server/SecureRequestCustomizer.java

@@ -40,7 +40,6 @@
 import org.eclipse.jetty.util.StringUtil;
 import org.eclipse.jetty.util.TypeUtil;
 import org.eclipse.jetty.util.annotation.Name;
-import org.eclipse.jetty.util.ssl.SniX509ExtendedKeyManager;
 import org.eclipse.jetty.util.ssl.SslContextFactory;
 import org.eclipse.jetty.util.ssl.X509;
 import org.slf4j.Logger;
@@ -63,7 +62,7 @@ public class SecureRequestCustomizer implements HttpConfiguration.Customizer
 
     private boolean _sniRequired;
     private boolean _sniHostCheck;
-    private long _stsMaxAge = -1;
+    private long _stsMaxAge;
     private boolean _stsIncludeSubDomains;
     private HttpField _stsField;
 
@@ -247,26 +246,32 @@ protected void customize(SSLEngine sslEngine, Request request)
     {
         SSLSession sslSession = sslEngine.getSession();
 
-        if (_sniHostCheck || _sniRequired)
+        if (isSniRequired() || isSniHostCheck())
         {
-            X509 x509 = (X509)sslSession.getValue(SniX509ExtendedKeyManager.SNI_X509);
+            String sniHost = (String)sslSession.getValue(SslContextFactory.Server.SNI_HOST);
+            X509 cert = new X509(null, (X509Certificate)sslSession.getLocalCertificates()[0]);
+            String serverName = request.getServerName();
             if (LOG.isDebugEnabled())
-                LOG.debug("Host {} with SNI {}", request.getServerName(), x509);
+                LOG.debug("Host={}, SNI={}, SNI Certificate={}", serverName, sniHost, cert);
 
-            if (x509 == null)
+            if (isSniRequired())
             {
-                if (_sniRequired)
-                    throw new BadMessageException(400, "SNI required");
+                if (sniHost == null)
+                    throw new BadMessageException(400, "Invalid SNI");
+                if (!cert.matches(sniHost))
+                    throw new BadMessageException(400, "Invalid SNI");
             }
-            else if (_sniHostCheck && !x509.matches(request.getServerName()))
+
+            if (isSniHostCheck())
             {
-                throw new BadMessageException(400, "Host does not match SNI");
+                if (!cert.matches(serverName))
+                    throw new BadMessageException(400, "Invalid SNI");
             }
         }
 
-        request.setAttributes(new SslAttributes(request, sslSession, request.getAttributes()));
+        request.setAttributes(new SslAttributes(request, sslSession));
     }
-    
+
     /**
      * Customizes the request attributes for general secure settings.
      * The default impl calls {@link Request#setSecure(boolean)} with true
@@ -325,9 +330,9 @@ private class SslAttributes extends Attributes.Wrapper
         private String _sessionId;
         private String _sessionAttribute;
 
-        public SslAttributes(Request request, SSLSession sslSession, Attributes attributes)
+        private SslAttributes(Request request, SSLSession sslSession)
         {
-            super(attributes);
+            super(request.getAttributes());
             this._request = request;
             this._session = sslSession;