Skip to content

Commit

Permalink
Fixes #6099 - Cipher preference may break SNI if certificates have di…
Browse files Browse the repository at this point in the history 
…fferent 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 <[email protected]>
  • Loading branch information
@sbordet
sbordet committed May 10, 2021
1 parent e5f28db commit 6829691
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 22 deletions.
31 changes: 27 additions & 4 deletions jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SniSslConnectionFactoryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
});
Expand All @@ -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()
Expand All @@ -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) ->
Expand Down Expand Up @@ -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);
});

Expand Down Expand Up @@ -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);
Expand Down
Binary file added jetty-server/src/test/resources/keystore_sni_key_types.p12
View file
Binary file not shown.
2 changes: 1 addition & 1 deletion jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SniX509ExtendedKeyManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
50 changes: 33 additions & 17 deletions jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslContextFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
{
Expand Down Expand Up @@ -2118,7 +2123,7 @@ public String toString()
_trustStoreResource);
}

class Factory
private static class Factory
{
private final KeyStore _keyStore;
private final KeyStore _trustStore;
Expand All @@ -2133,7 +2138,7 @@ class Factory
}
}

class AliasSNIMatcher extends SNIMatcher
static class AliasSNIMatcher extends SNIMatcher
{
private String _host;

Expand Down Expand Up @@ -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()
Expand All @@ -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)
{
Expand Down Expand Up @@ -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
{
Expand All @@ -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)
Expand Down

0 comments on commit 6829691

Please sign in to comment.