From 53073ca25704ebd49c7133226053418a5124900f Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Fri, 6 Dec 2019 13:20:24 -0600 Subject: [PATCH] Issue #4385 - Reverting WARN log in favor of IllegalStateException + Plus fleshing out the testcases more for Base / Client / Server with and without certificates that will trigger SNI requirement and ISE. Signed-off-by: Joakim Erdfelt --- .../jetty/util/ssl/SslContextFactory.java | 24 +++-- .../org/eclipse/jetty/util/ssl/X509Test.java | 91 ++++++++++--------- 2 files changed, 61 insertions(+), 54 deletions(-) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslContextFactory.java b/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslContextFactory.java index f141b846bec8..c1f356064306 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslContextFactory.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslContextFactory.java @@ -1249,18 +1249,13 @@ protected KeyManager[] getKeyManagers(KeyStore keyStore) throws Exception // Is SNI needed to select a certificate? if (!_certWilds.isEmpty() || _certHosts.size() > 1 || (_certHosts.size() == 1 && _aliasX509.size() > 1)) { - if (this instanceof SslContextFactory.Server) + for (int idx = 0; idx < managers.length; idx++) { - for (int idx = 0; idx < managers.length; idx++) + if (managers[idx] instanceof X509ExtendedKeyManager) { - if (managers[idx] instanceof X509ExtendedKeyManager) - managers[idx] = newSniX509ExtendedKeyManager((X509ExtendedKeyManager)managers[idx]); + managers[idx] = newSniX509ExtendedKeyManager((X509ExtendedKeyManager)managers[idx]); } } - else - { - LOG.warn("Unable to support SNI on {} (expecting {})", this.getClass().getName(), SslContextFactory.Server.class.getName()); - } } } } @@ -1277,7 +1272,11 @@ protected KeyManager[] getKeyManagers(KeyStore keyStore) throws Exception @Deprecated protected X509ExtendedKeyManager newSniX509ExtendedKeyManager(X509ExtendedKeyManager keyManager) { - throw new UnsupportedOperationException("X509ExtendedKeyManager only supported on " + SslContextFactory.Server.class.getName()); + throw new IllegalStateException(String.format( + "KeyStores with multiple certificates are not supported on the base class %s. (Use %s or %s instead)", + SslContextFactory.class.getName(), + Server.class.getName(), + Client.class.getName())); } protected TrustManager[] getTrustManagers(KeyStore trustStore, Collection crls) throws Exception @@ -2185,6 +2184,13 @@ protected void checkConfiguration() checkEndPointIdentificationAlgorithm(); super.checkConfiguration(); } + + @Override + protected X509ExtendedKeyManager newSniX509ExtendedKeyManager(X509ExtendedKeyManager keyManager) + { + // Client has no SNI functionality. + return keyManager; + } } @ManagedObject diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/ssl/X509Test.java b/jetty-util/src/test/java/org/eclipse/jetty/util/ssl/X509Test.java index a893be2d9ebe..85ca35233a9f 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/ssl/X509Test.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/ssl/X509Test.java @@ -20,8 +20,6 @@ import java.nio.file.Path; import java.security.cert.X509Certificate; -import javax.net.ssl.KeyManager; -import javax.net.ssl.X509ExtendedKeyManager; import org.eclipse.jetty.toolchain.test.MavenTestingUtils; import org.eclipse.jetty.util.resource.PathResource; @@ -31,7 +29,6 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.notNullValue; import static org.junit.jupiter.api.Assertions.assertThrows; public class X509Test @@ -133,66 +130,70 @@ public boolean[] getKeyUsage() assertThat("Normal X509", X509.isCertSign(bogusX509), is(false)); } - private X509ExtendedKeyManager getX509ExtendedKeyManager(SslContextFactory sslContextFactory) throws Exception + @Test + public void testBaseClass_WithSni() { - Resource keystoreResource = Resource.newSystemResource("keystore"); - Resource truststoreResource = Resource.newSystemResource("keystore"); - sslContextFactory.setKeyStoreResource(keystoreResource); - sslContextFactory.setTrustStoreResource(truststoreResource); - sslContextFactory.setKeyStorePassword("storepwd"); - sslContextFactory.setKeyManagerPassword("keypwd"); - sslContextFactory.setTrustStorePassword("storepwd"); - sslContextFactory.start(); - - KeyManager[] keyManagers = sslContextFactory.getKeyManagers(sslContextFactory.getKeyStore()); - X509ExtendedKeyManager x509ExtendedKeyManager = null; - - for (KeyManager keyManager : keyManagers) - { - if (keyManager instanceof X509ExtendedKeyManager) - { - x509ExtendedKeyManager = (X509ExtendedKeyManager)keyManager; - break; - } - } - assertThat("Found X509ExtendedKeyManager", x509ExtendedKeyManager, is(notNullValue())); - return x509ExtendedKeyManager; + SslContextFactory baseSsl = new SslContextFactory(); + Path keystorePath = MavenTestingUtils.getTestResourcePathFile("keystore_sni.p12"); + baseSsl.setKeyStoreResource(new PathResource(keystorePath)); + baseSsl.setKeyStorePassword("OBF:1vny1zlo1x8e1vnw1vn61x8g1zlu1vn4"); + baseSsl.setKeyManagerPassword("OBF:1u2u1wml1z7s1z7a1wnl1u2g"); + IllegalStateException ex = assertThrows(IllegalStateException.class, baseSsl::start); + assertThat("IllegalStateException.message", ex.getMessage(), containsString("KeyStores with multiple certificates are not supported on the base class")); } @Test - public void testSniX509ExtendedKeyManager_BaseClass() throws Exception + public void testServerClass_WithSni() throws Exception { - SslContextFactory baseSsl = new SslContextFactory(); - X509ExtendedKeyManager x509ExtendedKeyManager = getX509ExtendedKeyManager(baseSsl); - UnsupportedOperationException ex = assertThrows(UnsupportedOperationException.class, () -> baseSsl.newSniX509ExtendedKeyManager(x509ExtendedKeyManager)); - assertThat("UnsupportedOperationException.message", ex.getMessage(), containsString("X509ExtendedKeyManager only supported on " + SslContextFactory.Server.class.getName())); + SslContextFactory serverSsl = new SslContextFactory.Server(); + Path keystorePath = MavenTestingUtils.getTestResourcePathFile("keystore_sni.p12"); + serverSsl.setKeyStoreResource(new PathResource(keystorePath)); + serverSsl.setKeyStorePassword("OBF:1vny1zlo1x8e1vnw1vn61x8g1zlu1vn4"); + serverSsl.setKeyManagerPassword("OBF:1u2u1wml1z7s1z7a1wnl1u2g"); + serverSsl.start(); } @Test - public void testSniX509ExtendedKeyManager_BaseClass_Start() throws Exception + public void testClientClass_WithSni() throws Exception { - SslContextFactory baseSsl = new SslContextFactory(); + SslContextFactory clientSsl = new SslContextFactory.Client(); Path keystorePath = MavenTestingUtils.getTestResourcePathFile("keystore_sni.p12"); - baseSsl.setKeyStoreResource(new PathResource(keystorePath)); - baseSsl.setKeyStorePassword("OBF:1vny1zlo1x8e1vnw1vn61x8g1zlu1vn4"); - baseSsl.setKeyManagerPassword("OBF:1u2u1wml1z7s1z7a1wnl1u2g"); - baseSsl.start(); // should not throw an exception + clientSsl.setKeyStoreResource(new PathResource(keystorePath)); + clientSsl.setKeyStorePassword("OBF:1vny1zlo1x8e1vnw1vn61x8g1zlu1vn4"); + clientSsl.setKeyManagerPassword("OBF:1u2u1wml1z7s1z7a1wnl1u2g"); + clientSsl.start(); } @Test - public void testSniX509ExtendedKeyManager_ClientClass() throws Exception + public void testBaseClass_WithoutSni() throws Exception { - SslContextFactory clientSsl = new SslContextFactory.Client(); - X509ExtendedKeyManager x509ExtendedKeyManager = getX509ExtendedKeyManager(clientSsl); - UnsupportedOperationException ex = assertThrows(UnsupportedOperationException.class, () -> clientSsl.newSniX509ExtendedKeyManager(x509ExtendedKeyManager)); - assertThat("SNI X509 ExtendedKeyManager is unsupported in Client mode", ex.getMessage(), containsString("X509ExtendedKeyManager only supported on " + SslContextFactory.Server.class.getName())); + SslContextFactory baseSsl = new SslContextFactory(); + Resource keystoreResource = Resource.newSystemResource("keystore"); + baseSsl.setKeyStoreResource(keystoreResource); + baseSsl.setKeyStorePassword("storepwd"); + baseSsl.setKeyManagerPassword("keypwd"); + baseSsl.start(); } @Test - public void testSniX509ExtendedKeyManager_ServerClass() throws Exception + public void testServerClass_WithoutSni() throws Exception { SslContextFactory serverSsl = new SslContextFactory.Server(); - X509ExtendedKeyManager x509ExtendedKeyManager = getX509ExtendedKeyManager(serverSsl); - serverSsl.newSniX509ExtendedKeyManager(x509ExtendedKeyManager); + Resource keystoreResource = Resource.newSystemResource("keystore"); + serverSsl.setKeyStoreResource(keystoreResource); + serverSsl.setKeyStorePassword("storepwd"); + serverSsl.setKeyManagerPassword("keypwd"); + serverSsl.start(); + } + + @Test + public void testClientClass_WithoutSni() throws Exception + { + SslContextFactory clientSsl = new SslContextFactory.Client(); + Resource keystoreResource = Resource.newSystemResource("keystore"); + clientSsl.setKeyStoreResource(keystoreResource); + clientSsl.setKeyStorePassword("storepwd"); + clientSsl.setKeyManagerPassword("keypwd"); + clientSsl.start(); } }