Skip to content

Commit

Permalink
Merge pull request #5205 from eclipse/jetty-9.4.x-5204-sni_with_pkix
Browse files Browse the repository at this point in the history 
Fixes #5204 - SNI does not work with PKIX.
  • Loading branch information
@sbordet
sbordet committed Sep 2, 2020
2 parents f813b38 + bec2cdb commit 5a87469
Show file tree
Hide file tree
Showing 3 changed files with 164 additions and 24 deletions.
94 changes: 71 additions & 23 deletions 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 @@ -25,7 +25,10 @@
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.Objects;
import java.util.function.UnaryOperator;
import java.util.stream.Collectors;
import javax.net.ssl.SNIMatcher;
import javax.net.ssl.SSLEngine;
Expand All @@ -49,6 +52,7 @@ public class SniX509ExtendedKeyManager extends X509ExtendedKeyManager

private final X509ExtendedKeyManager _delegate;
private final SslContextFactory.Server _sslContextFactory;
private UnaryOperator<String> _aliasMapper = UnaryOperator.identity();

/**
* @deprecated not supported, you must have a {@link SslContextFactory.Server} for this to work.
Expand All @@ -65,6 +69,38 @@ public SniX509ExtendedKeyManager(X509ExtendedKeyManager keyManager, SslContextFa
_sslContextFactory = Objects.requireNonNull(sslContextFactory, "SslContextFactory.Server must be provided");
}

/**
* @return the function that transforms the alias
* @see #setAliasMapper(UnaryOperator)
*/
public UnaryOperator<String> getAliasMapper()
{
return _aliasMapper;
}

/**
* <p>Sets a function that transforms the alias into a possibly different alias,
* invoked when the SNI logic must choose the alias to pick the right certificate.</p>
* <p>This function is required when using the
* {@link SslContextFactory.Server#setKeyManagerFactoryAlgorithm(String) PKIX KeyManagerFactory algorithm}
* which suffers from bug https://bugs.openjdk.java.net/browse/JDK-8246262,
* where aliases are returned by the OpenJDK implementation to the application
* in the form {@code N.0.alias} where {@code N} is an always increasing number.
* Such mangled aliases won't match the aliases in the keystore, so that for
* example SNI matching will always fail.</p>
* <p>Other implementations such as BouncyCastle have been reported to mangle
* the alias in a different way, namely {@code 0.alias.N}.</p>
* <p>This function allows to "unmangle" the alias from the implementation
* specific mangling back to just {@code alias} so that SNI matching will work
* again.</p>
*
* @param aliasMapper the function that transforms the alias
*/
public void setAliasMapper(UnaryOperator<String> aliasMapper)
{
_aliasMapper = Objects.requireNonNull(aliasMapper);
}

@Override
public String chooseClientAlias(String[] keyType, Principal[] issuers, Socket socket)
{
Expand All @@ -80,10 +116,15 @@ public String chooseEngineClientAlias(String[] keyType, Principal[] issuers, SSL
protected String chooseServerAlias(String keyType, Principal[] issuers, Collection<SNIMatcher> 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)
String[] mangledAliases = _delegate.getServerAliases(keyType, issuers);
if (mangledAliases == null || mangledAliases.length == 0)
return null;

// Apply the alias mapping, keeping the alias order.
Map<String, String> aliasMap = new LinkedHashMap<>();
Arrays.stream(mangledAliases)
.forEach(alias -> aliasMap.put(getAliasMapper().apply(alias), alias));

// Find our SNIMatcher. There should only be one and it always matches (always returns true
// from AliasSNIMatcher.matches), but it will capture the SNI Host if one was presented.
String host = matchers == null ? null : matchers.stream()
Expand All @@ -96,35 +137,40 @@ protected String chooseServerAlias(String keyType, Principal[] issuers, Collecti
try
{
// Filter the certificates by alias.
Collection<X509> certificates = Arrays.stream(aliases)
Collection<X509> certificates = aliasMap.keySet().stream()
.map(_sslContextFactory::getX509)
.filter(Objects::nonNull)
.collect(Collectors.toList());

// delegate the decision to accept to the sniSelector
// Delegate the decision to accept to the sniSelector.
SniSelector sniSelector = _sslContextFactory.getSNISelector();
if (sniSelector == null)
sniSelector = _sslContextFactory;
String alias = sniSelector.sniSelect(keyType, issuers, session, host, certificates);

// Check selected alias
if (alias != null && alias != SniSelector.DELEGATE)
{
// Make sure we got back an alias from the acceptable aliases.
X509 x509 = _sslContextFactory.getX509(alias);
if (!Arrays.asList(aliases).contains(alias) || x509 == null)
{
if (LOG.isDebugEnabled())
LOG.debug("Invalid X509 match for SNI {}: {}", host, alias);
return null;
}
// Check the selected alias.
if (alias == null || alias == SniSelector.DELEGATE)
return alias;

// Make sure we got back an alias from the acceptable aliases.
X509 x509 = _sslContextFactory.getX509(alias);
if (!aliasMap.containsKey(alias) || x509 == null)
{
if (LOG.isDebugEnabled())
LOG.debug("Matched SNI {} with X509 {} from {}", host, x509, Arrays.asList(aliases));
if (session != null)
session.putValue(SNI_X509, x509);
LOG.debug("Invalid X509 match for SNI {}: {}", host, alias);
return null;
}
return alias;

if (session != null)
session.putValue(SNI_X509, x509);

// Convert the selected alias back to the original
// value before the alias mapping performed above.
String mangledAlias = aliasMap.get(alias);

if (LOG.isDebugEnabled())
LOG.debug("Matched SNI {} with alias {}, certificate {} from aliases {}", host, mangledAlias, x509, aliasMap.keySet());
return mangledAlias;
}
catch (Throwable x)
{
Expand All @@ -141,10 +187,11 @@ public String chooseServerAlias(String keyType, Principal[] issuers, Socket sock
String alias = (socket == null)
? chooseServerAlias(keyType, issuers, Collections.emptyList(), null)
: chooseServerAlias(keyType, issuers, sslSocket.getSSLParameters().getSNIMatchers(), sslSocket.getHandshakeSession());
if (alias == SniSelector.DELEGATE)
boolean delegate = alias == SniSelector.DELEGATE;
if (delegate)
alias = _delegate.chooseServerAlias(keyType, issuers, socket);
if (LOG.isDebugEnabled())
LOG.debug("Chose alias {}/{} on {}", alias, keyType, socket);
LOG.debug("Chose {} alias {}/{} on {}", delegate ? "delegate" : "explicit", alias, keyType, socket);
return alias;
}

Expand All @@ -154,10 +201,11 @@ public String chooseEngineServerAlias(String keyType, Principal[] issuers, SSLEn
String alias = (engine == null)
? chooseServerAlias(keyType, issuers, Collections.emptyList(), null)
: chooseServerAlias(keyType, issuers, engine.getSSLParameters().getSNIMatchers(), engine.getHandshakeSession());
if (alias == SniSelector.DELEGATE)
boolean delegate = alias == SniSelector.DELEGATE;
if (delegate)
alias = _delegate.chooseEngineServerAlias(keyType, issuers, engine);
if (LOG.isDebugEnabled())
LOG.debug("Chose alias {}/{} on {}", alias, keyType, engine);
LOG.debug("Chose {} alias {}/{} on {}", delegate ? "delegate" : "explicit", alias, keyType, engine);
return alias;
}

Expand Down
93 changes: 93 additions & 0 deletions jetty-util/src/test/java/org/eclipse/jetty/util/ssl/SslContextFactoryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,28 @@

import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.net.InetSocketAddress;
import java.nio.charset.StandardCharsets;
import java.security.KeyStore;
import java.security.cert.Certificate;
import java.security.cert.X509Certificate;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import javax.net.ssl.SNIHostName;
import javax.net.ssl.SSLContext;
import javax.net.ssl.SSLEngine;
import javax.net.ssl.SSLParameters;
import javax.net.ssl.SSLServerSocket;
import javax.net.ssl.SSLSocket;
import javax.net.ssl.X509ExtendedKeyManager;

import org.eclipse.jetty.util.IO;
import org.eclipse.jetty.util.component.AbstractLifeCycle;
import org.eclipse.jetty.util.log.StacklessLogging;
import org.eclipse.jetty.util.resource.Resource;
Expand All @@ -43,6 +57,8 @@
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.matchesRegex;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.startsWith;
import static org.junit.jupiter.api.Assertions.assertArrayEquals;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
Expand Down Expand Up @@ -362,4 +378,81 @@ public void testServerSslContextFactory() throws Exception

assertNull(cf.getEndpointIdentificationAlgorithm());
}

@Test
public void testSNIWithPKIX() throws Exception
{
SslContextFactory.Server serverTLS = new SslContextFactory.Server()
{
@Override
protected X509ExtendedKeyManager newSniX509ExtendedKeyManager(X509ExtendedKeyManager keyManager)
{
SniX509ExtendedKeyManager result = new SniX509ExtendedKeyManager(keyManager, this);
result.setAliasMapper(alias ->
{
// Workaround for https://bugs.openjdk.java.net/browse/JDK-8246262.
Matcher matcher = Pattern.compile(".*\\..*\\.(.*)").matcher(alias);
if (matcher.matches())
return matcher.group(1);
return alias;
});
return result;
}
};
// This test requires a SNI keystore so that the X509ExtendedKeyManager is wrapped.
serverTLS.setKeyStoreResource(Resource.newSystemResource("keystore_sni.p12"));
serverTLS.setKeyStorePassword("storepwd");
serverTLS.setKeyManagerPassword("keypwd");
serverTLS.setKeyManagerFactoryAlgorithm("PKIX");
// Don't pick a default certificate if SNI does not match.
serverTLS.setSniRequired(true);
serverTLS.start();

SslContextFactory.Client clientTLS = new SslContextFactory.Client(true);
clientTLS.start();

try (SSLServerSocket serverSocket = serverTLS.newSslServerSocket(null, 0, 128);
SSLSocket clientSocket = clientTLS.newSslSocket())
{
SSLParameters sslParameters = clientSocket.getSSLParameters();
String hostName = "jetty.eclipse.org";
sslParameters.setServerNames(Collections.singletonList(new SNIHostName(hostName)));
clientSocket.setSSLParameters(sslParameters);
clientSocket.connect(new InetSocketAddress("localhost", serverSocket.getLocalPort()), 5000);
try (SSLSocket sslSocket = (SSLSocket)serverSocket.accept())
{
byte[] data = "HELLO".getBytes(StandardCharsets.UTF_8);
new Thread(() ->
{
try
{
// Start the TLS handshake and verify that
// the client got the right server certificate.
clientSocket.startHandshake();
Certificate[] certificates = clientSocket.getSession().getPeerCertificates();
assertThat(certificates.length, greaterThan(0));
X509Certificate certificate = (X509Certificate)certificates[0];
assertThat(certificate.getSubjectX500Principal().getName(), startsWith("CN=" + hostName));
// Send some data to verify communication is ok.
OutputStream output = clientSocket.getOutputStream();
output.write(data);
output.flush();
clientSocket.close();
}
catch (Throwable x)
{
x.printStackTrace();
}
}).start();
// Verify that we received the data the client sent.
sslSocket.setSoTimeout(5000);
InputStream input = sslSocket.getInputStream();
byte[] bytes = IO.readBytes(input);
assertArrayEquals(data, bytes);
}
}

clientTLS.stop();
serverTLS.stop();
}
}
1 change: 0 additions & 1 deletion jetty-util/src/test/resources/jetty-logging.properties
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
# Setup default logging implementation for during testing
org.eclipse.jetty.util.log.class=org.eclipse.jetty.util.log.StdErrLog
#org.eclipse.jetty.util.LEVEL=DEBUG
#org.eclipse.jetty.util.PathWatcher.LEVEL=DEBUG
Expand Down

0 comments on commit 5a87469

Please sign in to comment.