From c3577bbbb0a7505c3d808e34c3755ff55f3b7ad4 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Wed, 22 Apr 2015 15:05:56 +1000 Subject: [PATCH] 430951 Support SNI with ExtendedSslContextFactory refactored common code in SniX509ExtendedKeyManager added sniHostCheck code to ensure request host is the same as SNI host --- .../src/main/config/etc/jetty-ssl.xml | 6 +- jetty-server/src/main/config/modules/ssl.mod | 3 + .../org/eclipse/jetty/server/HttpChannel.java | 12 +- .../jetty/server/SecureRequestCustomizer.java | 24 ++++ .../org/eclipse/jetty/server/RequestTest.java | 6 +- .../server/ssl/SslConnectionFactoryTest.java | 31 ++++- .../util/ssl/ExtendedSslContextFactory.java | 4 +- .../util/ssl/SniX509ExtendedKeyManager.java | 128 +++++++----------- 8 files changed, 124 insertions(+), 90 deletions(-) diff --git a/jetty-server/src/main/config/etc/jetty-ssl.xml b/jetty-server/src/main/config/etc/jetty-ssl.xml index 0b8bb170541..257c83689cc 100644 --- a/jetty-server/src/main/config/etc/jetty-ssl.xml +++ b/jetty-server/src/main/config/etc/jetty-ssl.xml @@ -70,7 +70,11 @@ - + + + + + diff --git a/jetty-server/src/main/config/modules/ssl.mod b/jetty-server/src/main/config/modules/ssl.mod index e99ed7e3d7a..36cc2fa0681 100644 --- a/jetty-server/src/main/config/modules/ssl.mod +++ b/jetty-server/src/main/config/modules/ssl.mod @@ -38,6 +38,9 @@ http://git.eclipse.org/c/jetty/org.eclipse.jetty.project.git/plain/jetty-server/ ## Thread priority delta to give to acceptor threads # jetty.ssl.acceptorPriorityDelta=0 +## Whether request host names are checked to match any SNI names +# jetty.ssl.sniHostCheck=true + ### SslContextFactory Configuration ## Keystore file path (relative to $jetty.base) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java index c010a5fc31a..fbd82f70a3c 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java @@ -30,6 +30,7 @@ import javax.servlet.DispatcherType; import javax.servlet.RequestDispatcher; import javax.servlet.http.HttpServletRequest; +import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.HttpFields; import org.eclipse.jetty.http.HttpGenerator; import org.eclipse.jetty.http.HttpHeader; @@ -353,7 +354,7 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor } } - catch (EofException|QuietServletException e) + catch (EofException|QuietServletException|BadMessageException e) { error=true; LOG.debug(e); @@ -465,7 +466,14 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor else { _response.setHeader(HttpHeader.CONNECTION.asString(),HttpHeaderValue.CLOSE.asString()); - _response.sendError(500, x.getMessage()); + + if (x instanceof BadMessageException) + { + BadMessageException bme = (BadMessageException)x; + _response.sendError(bme.getCode(), bme.getReason()); + } + else + _response.sendError(500, x.getClass().toString()); } } catch (IOException e) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/SecureRequestCustomizer.java b/jetty-server/src/main/java/org/eclipse/jetty/server/SecureRequestCustomizer.java index 2ebfbaf92a8..4a3695350e8 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/SecureRequestCustomizer.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/SecureRequestCustomizer.java @@ -25,6 +25,7 @@ import javax.net.ssl.SSLEngine; import javax.net.ssl.SSLSession; import javax.servlet.ServletRequest; +import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.HttpScheme; import org.eclipse.jetty.io.ssl.SslConnection; import org.eclipse.jetty.io.ssl.SslConnection.DecryptedEndPoint; @@ -48,6 +49,19 @@ public class SecureRequestCustomizer implements HttpConfiguration.Customizer */ public static final String CACHED_INFO_ATTR = CachedInfo.class.getName(); + private boolean _sniHostCheck; + + + public SecureRequestCustomizer() + { + this(true); + } + + public SecureRequestCustomizer(boolean sniHostCheck) + { + _sniHostCheck=sniHostCheck; + } + @Override public void customize(Connector connector, HttpConfiguration channelConfig, Request request) { @@ -88,6 +102,16 @@ public class SecureRequestCustomizer implements HttpConfiguration.Customizer request.setScheme(HttpScheme.HTTPS.asString()); SSLSession sslSession = sslEngine.getSession(); + if (_sniHostCheck) + { + String sniName = (String)sslSession.getValue("org.eclipse.jetty.util.ssl.sniname"); + if (sniName!=null && !sniName.equalsIgnoreCase(request.getServerName())) + { + LOG.warn("Host does not match SNI Name: {}!={}",sniName,request.getServerName()); + throw new BadMessageException(400,"Host does not match SNI"); + } + } + try { String cipherSuite=sslSession.getCipherSuite(); diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java index f266b9609c8..96eba76a1b9 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java @@ -1261,7 +1261,7 @@ public class RequestTest { long start=System.currentTimeMillis(); String response = _connector.getResponses(request); - assertThat(response,Matchers.containsString("Form too many keys")); + assertThat(response,Matchers.containsString("IllegalStateException")); long now=System.currentTimeMillis(); assertTrue((now-start)<5000); } @@ -1307,7 +1307,7 @@ public class RequestTest { long start=System.currentTimeMillis(); String response = _connector.getResponses(request); - assertTrue(response.contains("Form too large:")); + assertTrue(response.contains("IllegalStateException")); long now=System.currentTimeMillis(); assertTrue((now-start)<5000); } @@ -1414,7 +1414,7 @@ public class RequestTest request.setAttribute(Request.__MULTIPART_CONFIG_ELEMENT, mpce); //We should get an error when we getParams if there was a problem parsing the multipart - String field1 = request.getParameter("xxx"); + request.getParameter("xxx"); //A 200 response is actually wrong here } catch (RuntimeException e) diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SslConnectionFactoryTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SslConnectionFactoryTest.java index 20fc3b89343..96d0f9315a8 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SslConnectionFactoryTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SslConnectionFactoryTest.java @@ -125,7 +125,9 @@ public class SslConnectionFactoryTest @Test public void testSNIConnect() throws Exception { - String response= getResponse("jetty.eclipse.org","jetty.eclipse.org"); + String response; + + response= getResponse("jetty.eclipse.org","jetty.eclipse.org"); Assert.assertThat(response,Matchers.containsString("host=jetty.eclipse.org")); response= getResponse("www.example.com","www.example.com"); @@ -139,10 +141,29 @@ public class SslConnectionFactoryTest response= getResponse("www.san.com","san example"); Assert.assertThat(response,Matchers.containsString("host=www.san.com")); + + } + + @Test + public void testBadSNIConnect() throws Exception + { + String response; + + response= getResponse("www.example.com","some.other.com","www.example.com"); + Assert.assertThat(response,Matchers.containsString("HTTP/1.1 400 ")); + Assert.assertThat(response,Matchers.containsString("Host does not match SNI")); } - + private String getResponse(String host,String cn) throws Exception + { + String response = getResponse(host,host,cn); + Assert.assertThat(response,Matchers.startsWith("HTTP/1.1 200 OK")); + Assert.assertThat(response,Matchers.containsString("url=/ctx/path")); + return response; + } + + private String getResponse(String sniHost,String reqHost, String cn) throws Exception { SslContextFactory clientContextFactory = new SslContextFactory(true); clientContextFactory.start(); @@ -152,7 +173,7 @@ public class SslConnectionFactoryTest if (cn!=null) { - SNIHostName serverName = new SNIHostName(host); + SNIHostName serverName = new SNIHostName(sniHost); List serverNames = new ArrayList<>(); serverNames.add(serverName); @@ -170,10 +191,8 @@ public class SslConnectionFactoryTest Assert.assertThat(cert.getSubjectX500Principal().getName("CANONICAL"), Matchers.startsWith("cn="+cn)); } - sslSocket.getOutputStream().write(("GET /ctx/path HTTP/1.0\r\nHost: "+host+":"+_port+"\r\n\r\n").getBytes(StandardCharsets.ISO_8859_1)); + sslSocket.getOutputStream().write(("GET /ctx/path HTTP/1.0\r\nHost: "+reqHost+":"+_port+"\r\n\r\n").getBytes(StandardCharsets.ISO_8859_1)); String response = IO.toString(sslSocket.getInputStream()); - Assert.assertThat(response,Matchers.startsWith("HTTP/1.1 200 OK")); - Assert.assertThat(response,Matchers.containsString("url=/ctx/path")); sslSocket.close(); clientContextFactory.stop(); diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/ExtendedSslContextFactory.java b/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/ExtendedSslContextFactory.java index 5fc615f7c12..f3cee8618dd 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/ExtendedSslContextFactory.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/ExtendedSslContextFactory.java @@ -223,9 +223,9 @@ public class ExtendedSslContextFactory extends SslContextFactory return _alias; } - public SNIHostName getServerName() + public String getServerName() { - return _name; + return _name==null?null:_name.getAsciiName(); } } } diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SniX509ExtendedKeyManager.java b/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SniX509ExtendedKeyManager.java index 8f117923ecf..9730e3caabe 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SniX509ExtendedKeyManager.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SniX509ExtendedKeyManager.java @@ -27,6 +27,7 @@ import java.util.Collection; import javax.net.ssl.SNIMatcher; import javax.net.ssl.SSLEngine; +import javax.net.ssl.SSLSession; import javax.net.ssl.SSLSocket; import javax.net.ssl.X509ExtendedKeyManager; @@ -42,6 +43,8 @@ import org.eclipse.jetty.util.log.Logger; public class SniX509ExtendedKeyManager extends X509ExtendedKeyManager { static final Logger LOG = Log.getLogger(SniX509ExtendedKeyManager.class); + public final static String SNI_NAME = "org.eclipse.jetty.util.ssl.sniname"; + public final static String NO_MATCHERS="No Matchers"; private final X509ExtendedKeyManager _delegate; /* ------------------------------------------------------------ */ @@ -55,7 +58,6 @@ public class SniX509ExtendedKeyManager extends X509ExtendedKeyManager { _delegate = keyManager; } - @Override public String chooseClientAlias(String[] keyType, Principal[] issuers, Socket socket) @@ -68,88 +70,64 @@ public class SniX509ExtendedKeyManager extends X509ExtendedKeyManager { return _delegate.chooseEngineClientAlias(keyType,issuers,engine); } + + protected String chooseServerAlias(String keyType, Principal[] issuers, Collection matchers, SSLSession session) + { + // Look for the aliases that are suitable for the keytype and issuers + String[] aliases = _delegate.getServerAliases(keyType,issuers); + if (aliases==null || aliases.length==0) + return null; + + // Look for an SNI alias + String alias=null; + String host=null; + if (matchers!=null) + { + for (SNIMatcher m : matchers) + { + if (m instanceof ExtendedSslContextFactory.AliasSNIMatcher) + { + ExtendedSslContextFactory.AliasSNIMatcher matcher = (ExtendedSslContextFactory.AliasSNIMatcher)m; + alias=matcher.getAlias(); + host=matcher.getServerName(); + break; + } + } + } + + if (LOG.isDebugEnabled()) + LOG.debug("choose {} from {}",alias,Arrays.asList(aliases)); + + // Check if the SNI selected alias is allowable + if (alias!=null) + { + for (String a:aliases) + { + if (a.equals(alias)) + { + session.putValue(SNI_NAME,host); + return alias; + } + } + return null; + } + return NO_MATCHERS; + } @Override public String chooseServerAlias(String keyType, Principal[] issuers, Socket socket) { - // Look for the aliases that are suitable for the keytype and issuers - String[] aliases = _delegate.getServerAliases(keyType,issuers); + SSLSocket sslSocket = (SSLSocket)socket; - if (aliases==null || aliases.length==0) - return null; - - // Look for an SNI alias - String alias=null; - Collection matchers = ((SSLSocket)socket).getSSLParameters().getSNIMatchers(); - if (matchers!=null) - { - for (SNIMatcher m : matchers) - { - if (m instanceof ExtendedSslContextFactory.AliasSNIMatcher) - { - alias=((ExtendedSslContextFactory.AliasSNIMatcher)m).getAlias(); - break; - } - } - } - - if (LOG.isDebugEnabled()) - LOG.debug("choose {} from {}",alias,Arrays.asList(aliases)); - - // Check if the SNI selected alias is allowable - if (alias!=null) - { - for (String a:aliases) - { - if (a.equals(alias)) - return alias; - } - - return null; - } - - return _delegate.chooseServerAlias(keyType,issuers,socket); + String alias = chooseServerAlias(keyType,issuers,sslSocket.getSSLParameters().getSNIMatchers(),sslSocket.getHandshakeSession()); + return alias==NO_MATCHERS?_delegate.chooseServerAlias(keyType,issuers,socket):alias; } - + @Override public String chooseEngineServerAlias(String keyType, Principal[] issuers, SSLEngine engine) { - // Look for the aliases that are suitable for the keytype and issuers - String[] aliases = _delegate.getServerAliases(keyType,issuers); - if (aliases==null || aliases.length==0) - return null; - - // Look for an SNI alias - String alias=null; - Collection matchers = engine.getSSLParameters().getSNIMatchers(); - if (matchers!=null) - { - for (SNIMatcher m : matchers) - { - if (m instanceof ExtendedSslContextFactory.AliasSNIMatcher) - { - alias=((ExtendedSslContextFactory.AliasSNIMatcher)m).getAlias(); - break; - } - } - } - - if (LOG.isDebugEnabled()) - LOG.debug("choose {} from {}",alias,Arrays.asList(aliases)); - - // Check if the SNI selected alias is allowable - if (alias!=null) - { - for (String a:aliases) - { - if (a.equals(alias)) - return alias; - } - - return null; - } - - return _delegate.chooseEngineServerAlias(keyType,issuers,engine); + String alias = chooseServerAlias(keyType,issuers,engine.getSSLParameters().getSNIMatchers(),engine.getHandshakeSession()); + return alias==NO_MATCHERS?_delegate.chooseEngineServerAlias(keyType,issuers,engine):alias; } @Override @@ -175,6 +153,4 @@ public class SniX509ExtendedKeyManager extends X509ExtendedKeyManager { return _delegate.getServerAliases(keyType,issuers); } - - }