From e3516059622a09ea2ff439daffaeb87c649d9667 Mon Sep 17 00:00:00 2001 From: cbono Date: Sun, 1 Dec 2019 17:19:05 -0600 Subject: [PATCH] Verify ssl key alias on server startup See gh-19202 --- .../embedded/jetty/SslServerCustomizer.java | 39 ++++++++- .../embedded/netty/SslServerCustomizer.java | 3 + .../undertow/SslBuilderCustomizer.java | 3 + .../boot/web/server/SslUtils.java | 49 ++++++++++++ .../NettyReactiveWebServerFactoryTests.java | 10 --- .../TomcatReactiveWebServerFactoryTests.java | 18 +++++ .../TomcatServletWebServerFactoryTests.java | 16 ++++ ...AbstractReactiveWebServerFactoryTests.java | 38 +++++++++ .../boot/web/server/SslUtilsTest.java | 79 +++++++++++++++++++ .../AbstractServletWebServerFactoryTests.java | 14 +++- 10 files changed, 256 insertions(+), 13 deletions(-) create mode 100644 spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/server/SslUtils.java create mode 100644 spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/server/SslUtilsTest.java diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/jetty/SslServerCustomizer.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/jetty/SslServerCustomizer.java index b1ae3d5038..ef4d5499e7 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/jetty/SslServerCustomizer.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/jetty/SslServerCustomizer.java @@ -24,6 +24,7 @@ import org.eclipse.jetty.alpn.server.ALPNServerConnectionFactory; import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http2.HTTP2Cipher; import org.eclipse.jetty.http2.server.HTTP2ServerConnectionFactory; +import org.eclipse.jetty.server.ConnectionFactory; import org.eclipse.jetty.server.Connector; import org.eclipse.jetty.server.HttpConfiguration; import org.eclipse.jetty.server.HttpConnectionFactory; @@ -37,6 +38,7 @@ import org.eclipse.jetty.util.ssl.SslContextFactory; import org.springframework.boot.web.server.Http2; import org.springframework.boot.web.server.Ssl; import org.springframework.boot.web.server.SslStoreProvider; +import org.springframework.boot.web.server.SslUtils; import org.springframework.boot.web.server.WebServerException; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; @@ -105,7 +107,8 @@ class SslServerCustomizer implements JettyServerCustomizer { HttpConnectionFactory connectionFactory = new HttpConnectionFactory(config); SslConnectionFactory sslConnectionFactory = new SslConnectionFactory(sslContextFactory, HttpVersion.HTTP_1_1.asString()); - return new ServerConnector(server, sslConnectionFactory, connectionFactory); + return new SslValidatingServerConnector(server, sslContextFactory, this.ssl.getKeyAlias(), sslConnectionFactory, + connectionFactory); } private boolean isAlpnPresent() { @@ -123,7 +126,8 @@ class SslServerCustomizer implements JettyServerCustomizer { sslContextFactory.setCipherComparator(HTTP2Cipher.COMPARATOR); sslContextFactory.setProvider("Conscrypt"); SslConnectionFactory ssl = new SslConnectionFactory(sslContextFactory, alpn.getProtocol()); - return new ServerConnector(server, ssl, alpn, h2, new HttpConnectionFactory(config)); + return new SslValidatingServerConnector(server, sslContextFactory, this.ssl.getKeyAlias(), ssl, alpn, h2, + new HttpConnectionFactory(config)); } /** @@ -215,4 +219,35 @@ class SslServerCustomizer implements JettyServerCustomizer { } } + /** + * A {@link ServerConnector} that validates the ssl key alias on server startup. + */ + static class SslValidatingServerConnector extends ServerConnector { + + private SslContextFactory sslContextFactory; + + private String keyAlias; + + SslValidatingServerConnector(Server server, SslContextFactory sslContextFactory, String keyAlias, + SslConnectionFactory sslConnectionFactory, HttpConnectionFactory connectionFactory) { + super(server, sslConnectionFactory, connectionFactory); + this.sslContextFactory = sslContextFactory; + this.keyAlias = keyAlias; + } + + SslValidatingServerConnector(Server server, SslContextFactory sslContextFactory, String keyAlias, + ConnectionFactory... factories) { + super(server, factories); + this.sslContextFactory = sslContextFactory; + this.keyAlias = keyAlias; + } + + @Override + protected void doStart() throws Exception { + super.doStart(); + SslUtils.assertStoreContainsAlias(this.sslContextFactory.getKeyStore(), this.keyAlias); + } + + } + } diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/netty/SslServerCustomizer.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/netty/SslServerCustomizer.java index f2e32e4632..ac3fd47990 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/netty/SslServerCustomizer.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/netty/SslServerCustomizer.java @@ -44,6 +44,7 @@ import reactor.netty.tcp.SslProvider; import org.springframework.boot.web.server.Http2; import org.springframework.boot.web.server.Ssl; import org.springframework.boot.web.server.SslStoreProvider; +import org.springframework.boot.web.server.SslUtils; import org.springframework.boot.web.server.WebServerException; import org.springframework.util.ResourceUtils; @@ -106,6 +107,8 @@ public class SslServerCustomizer implements NettyServerCustomizer { protected KeyManagerFactory getKeyManagerFactory(Ssl ssl, SslStoreProvider sslStoreProvider) { try { KeyStore keyStore = getKeyStore(ssl, sslStoreProvider); + SslUtils.assertStoreContainsAlias(keyStore, ssl.getKeyAlias()); + KeyManagerFactory keyManagerFactory = (ssl.getKeyAlias() == null) ? KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm()) : new ConfigurableAliasKeyManagerFactory(ssl.getKeyAlias(), diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/undertow/SslBuilderCustomizer.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/undertow/SslBuilderCustomizer.java index 95a7e41848..1bde5d121b 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/undertow/SslBuilderCustomizer.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/undertow/SslBuilderCustomizer.java @@ -41,6 +41,7 @@ import org.xnio.SslClientAuthMode; import org.springframework.boot.web.server.Ssl; import org.springframework.boot.web.server.SslStoreProvider; +import org.springframework.boot.web.server.SslUtils; import org.springframework.boot.web.server.WebServerException; import org.springframework.util.ResourceUtils; @@ -107,6 +108,8 @@ class SslBuilderCustomizer implements UndertowBuilderCustomizer { private KeyManager[] getKeyManagers(Ssl ssl, SslStoreProvider sslStoreProvider) { try { KeyStore keyStore = getKeyStore(ssl, sslStoreProvider); + SslUtils.assertStoreContainsAlias(keyStore, ssl.getKeyAlias()); + KeyManagerFactory keyManagerFactory = KeyManagerFactory .getInstance(KeyManagerFactory.getDefaultAlgorithm()); char[] keyPassword = (ssl.getKeyPassword() != null) ? ssl.getKeyPassword().toCharArray() : null; diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/server/SslUtils.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/server/SslUtils.java new file mode 100644 index 0000000000..4f51a3d6b9 --- /dev/null +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/server/SslUtils.java @@ -0,0 +1,49 @@ +/* + * Copyright 2012-2019 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.boot.web.server; + +import java.security.KeyStore; +import java.security.KeyStoreException; + +import org.springframework.util.Assert; +import org.springframework.util.StringUtils; + +/** + * Provides utilities around SSL. + * + * @author Chris Bono + * @since 2.1.x + */ +public final class SslUtils { + + private SslUtils() { + } + + public static void assertStoreContainsAlias(KeyStore keyStore, String keyAlias) { + if (!StringUtils.isEmpty(keyAlias)) { + try { + Assert.state(keyStore.containsAlias(keyAlias), + () -> String.format("Keystore does not contain specified alias '%s'", keyAlias)); + } + catch (KeyStoreException ex) { + throw new IllegalStateException( + String.format("Could not determine if keystore contains alias '%s'", keyAlias), ex); + } + } + } + +} diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/netty/NettyReactiveWebServerFactoryTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/netty/NettyReactiveWebServerFactoryTests.java index 2c6743adf3..e9f35f8c72 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/netty/NettyReactiveWebServerFactoryTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/netty/NettyReactiveWebServerFactoryTests.java @@ -19,8 +19,6 @@ package org.springframework.boot.web.embedded.netty; import java.time.Duration; import java.util.Arrays; -import javax.net.ssl.SSLHandshakeException; - import org.junit.Test; import org.mockito.InOrder; import reactor.core.publisher.Mono; @@ -101,14 +99,6 @@ public class NettyReactiveWebServerFactoryTests extends AbstractReactiveWebServe StepVerifier.create(result).expectNext("Hello World").verifyComplete(); } - @Test - public void whenSslIsConfiguredWithAnInvalidAliasTheSslHandshakeFails() { - Mono result = testSslWithAlias("test-alias-bad"); - StepVerifier.setDefaultTimeout(Duration.ofSeconds(30)); - StepVerifier.create(result).expectErrorMatches((throwable) -> throwable instanceof SSLHandshakeException - && throwable.getMessage().contains("HANDSHAKE_FAILURE")).verify(); - } - protected Mono testSslWithAlias(String alias) { String keyStore = "classpath:test.jks"; String keyPassword = "password"; diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/tomcat/TomcatReactiveWebServerFactoryTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/tomcat/TomcatReactiveWebServerFactoryTests.java index d8744c4464..9c9149722f 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/tomcat/TomcatReactiveWebServerFactoryTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/tomcat/TomcatReactiveWebServerFactoryTests.java @@ -31,11 +31,14 @@ import org.junit.Test; import org.mockito.ArgumentCaptor; import org.mockito.InOrder; +import org.springframework.boot.web.reactive.server.AbstractReactiveWebServerFactory; import org.springframework.boot.web.reactive.server.AbstractReactiveWebServerFactoryTests; +import org.springframework.boot.web.server.Ssl; import org.springframework.http.server.reactive.HttpHandler; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; @@ -166,4 +169,19 @@ public class TomcatReactiveWebServerFactoryTests extends AbstractReactiveWebServ public void compressionOfResponseToPostRequest() { } + @Test + @Override + public void sslWithInvalidAliasFailsDuringStartup() { + String keyStore = "classpath:test.jks"; + String keyPassword = "password"; + AbstractReactiveWebServerFactory factory = getFactory(); + Ssl ssl = new Ssl(); + ssl.setKeyStore(keyStore); + ssl.setKeyPassword(keyPassword); + ssl.setKeyAlias("test-alias-404"); + factory.setSsl(ssl); + assertThatThrownBy(() -> factory.getWebServer(new EchoHandler()).start()) + .isInstanceOf(ConnectorStartFailedException.class); + } + } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/tomcat/TomcatServletWebServerFactoryTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/tomcat/TomcatServletWebServerFactoryTests.java index 92a81d99ae..c8f1ad9121 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/tomcat/TomcatServletWebServerFactoryTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/tomcat/TomcatServletWebServerFactoryTests.java @@ -63,8 +63,11 @@ import org.mockito.ArgumentCaptor; import org.mockito.InOrder; import org.springframework.boot.testsupport.rule.OutputCapture; +import org.springframework.boot.testsupport.web.servlet.ExampleServlet; +import org.springframework.boot.web.server.Ssl; import org.springframework.boot.web.server.WebServerException; import org.springframework.boot.web.servlet.ServletContextInitializer; +import org.springframework.boot.web.servlet.ServletRegistrationBean; import org.springframework.boot.web.servlet.server.AbstractServletWebServerFactory; import org.springframework.boot.web.servlet.server.AbstractServletWebServerFactoryTests; import org.springframework.core.io.ByteArrayResource; @@ -81,6 +84,7 @@ import org.springframework.web.client.RestTemplate; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; @@ -529,6 +533,18 @@ public class TomcatServletWebServerFactoryTests extends AbstractServletWebServer this.webServer.start(); } + @Test + @Override + public void sslWithInvalidAliasFailsDuringStartup() { + AbstractServletWebServerFactory factory = getFactory(); + Ssl ssl = getSsl(null, "password", "test-alias-404", "src/test/resources/test.jks"); + factory.setSsl(ssl); + ServletRegistrationBean registration = new ServletRegistrationBean<>( + new ExampleServlet(true, false), "/hello"); + assertThatThrownBy(() -> factory.getWebServer(registration).start()) + .isInstanceOf(ConnectorStartFailedException.class); + } + @Override protected JspServlet getJspServlet() throws ServletException { Tomcat tomcat = ((TomcatWebServer) this.webServer).getTomcat(); diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/reactive/server/AbstractReactiveWebServerFactoryTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/reactive/server/AbstractReactiveWebServerFactoryTests.java index c00003ef46..2949fd1d12 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/reactive/server/AbstractReactiveWebServerFactoryTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/reactive/server/AbstractReactiveWebServerFactoryTests.java @@ -137,6 +137,44 @@ public abstract class AbstractReactiveWebServerFactoryTests { assertThat(result.block(Duration.ofSeconds(30))).isEqualTo("Hello World"); } + @Test + public void sslWithValidAlias() { + String keyStore = "classpath:test.jks"; + String keyPassword = "password"; + AbstractReactiveWebServerFactory factory = getFactory(); + Ssl ssl = new Ssl(); + ssl.setKeyStore(keyStore); + ssl.setKeyPassword(keyPassword); + ssl.setKeyAlias("test-alias"); + factory.setSsl(ssl); + this.webServer = factory.getWebServer(new EchoHandler()); + this.webServer.start(); + ReactorClientHttpConnector connector = buildTrustAllSslConnector(); + WebClient client = WebClient.builder().baseUrl("https://localhost:" + this.webServer.getPort()) + .clientConnector(connector).build(); + + Mono result = client.post().uri("/test").contentType(MediaType.TEXT_PLAIN) + .body(BodyInserters.fromObject("Hello World")).exchange() + .flatMap((response) -> response.bodyToMono(String.class)); + + StepVerifier.setDefaultTimeout(Duration.ofSeconds(30)); + StepVerifier.create(result).expectNext("Hello World").verifyComplete(); + } + + @Test + public void sslWithInvalidAliasFailsDuringStartup() { + String keyStore = "classpath:test.jks"; + String keyPassword = "password"; + AbstractReactiveWebServerFactory factory = getFactory(); + Ssl ssl = new Ssl(); + ssl.setKeyStore(keyStore); + ssl.setKeyPassword(keyPassword); + ssl.setKeyAlias("test-alias-404"); + factory.setSsl(ssl); + assertThatThrownBy(() -> factory.getWebServer(new EchoHandler()).start()) + .hasStackTraceContaining("Keystore does not contain specified alias 'test-alias-404'"); + } + protected ReactorClientHttpConnector buildTrustAllSslConnector() { SslContextBuilder builder = SslContextBuilder.forClient().sslProvider(SslProvider.JDK) .trustManager(InsecureTrustManagerFactory.INSTANCE); diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/server/SslUtilsTest.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/server/SslUtilsTest.java new file mode 100644 index 0000000000..e3504dc7e2 --- /dev/null +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/server/SslUtilsTest.java @@ -0,0 +1,79 @@ +/* + * Copyright 2012-2019 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.boot.web.server; + +import java.io.File; +import java.io.FileInputStream; +import java.security.KeyStore; +import java.security.KeyStoreException; + +import org.junit.Before; +import org.junit.Test; + +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * Tests for {@link SslUtils}. + * + * @author Chris Bono + */ + +public class SslUtilsTest { + + private static final String VALID_ALIAS = "test-alias"; + + private static final String INVALID_ALIAS = "test-alias-5150"; + + private KeyStore keyStore; + + @Before + public void loadKeystore() throws Exception { + this.keyStore = KeyStore.getInstance(KeyStore.getDefaultType()); + this.keyStore.load(new FileInputStream(new File("src/test/resources/test.jks")), "secret".toCharArray()); + } + + @Test + public void assertStoreContainsAliasPassesWhenAliasFound() throws KeyStoreException { + SslUtils.assertStoreContainsAlias(this.keyStore, VALID_ALIAS); + } + + @Test + public void assertStoreContainsAliasPassesWhenNullAlias() throws KeyStoreException { + SslUtils.assertStoreContainsAlias(this.keyStore, null); + } + + @Test + public void assertStoreContainsAliasPassesWhenEmptyAlias() throws KeyStoreException { + SslUtils.assertStoreContainsAlias(this.keyStore, ""); + } + + @Test + public void assertStoreContainsAliasFailsWhenAliasNotFound() throws KeyStoreException { + assertThatThrownBy(() -> SslUtils.assertStoreContainsAlias(this.keyStore, INVALID_ALIAS)) + .isInstanceOf(IllegalStateException.class) + .hasMessage("Keystore does not contain specified alias '" + INVALID_ALIAS + "'"); + } + + @Test + public void assertStoreContainsAliasFailsWhenKeyStoreThrowsExceptionOnContains() throws KeyStoreException { + KeyStore uninitializedKeyStore = KeyStore.getInstance(KeyStore.getDefaultType()); + assertThatThrownBy(() -> SslUtils.assertStoreContainsAlias(uninitializedKeyStore, "alias")) + .isInstanceOf(IllegalStateException.class) + .hasMessage("Could not determine if keystore contains alias 'alias'"); + } + +} diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/server/AbstractServletWebServerFactoryTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/server/AbstractServletWebServerFactoryTests.java index 9f5bcd7c8f..a160da3dd1 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/server/AbstractServletWebServerFactoryTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/server/AbstractServletWebServerFactoryTests.java @@ -122,6 +122,7 @@ import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.assertj.core.api.Assertions.assertThatIOException; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; import static org.assertj.core.api.Assertions.assertThatIllegalStateException; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.hamcrest.CoreMatchers.notNullValue; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; @@ -423,6 +424,17 @@ public abstract class AbstractServletWebServerFactoryTests { assertThat(response).contains("scheme=https"); } + @Test + public void sslWithInvalidAliasFailsDuringStartup() { + AbstractServletWebServerFactory factory = getFactory(); + Ssl ssl = getSsl(null, "password", "test-alias-404", "src/test/resources/test.jks"); + factory.setSsl(ssl); + ServletRegistrationBean registration = new ServletRegistrationBean<>( + new ExampleServlet(true, false), "/hello"); + assertThatThrownBy(() -> factory.getWebServer(registration).start()) + .hasStackTraceContaining("Keystore does not contain specified alias 'test-alias-404'"); + } + @Test public void serverHeaderIsDisabledByDefaultWhenUsingSsl() throws Exception { AbstractServletWebServerFactory factory = getFactory(); @@ -594,7 +606,7 @@ public abstract class AbstractServletWebServerFactoryTests { return getSsl(clientAuth, keyPassword, keyStore, null, null, null); } - private Ssl getSsl(ClientAuth clientAuth, String keyPassword, String keyAlias, String keyStore) { + protected Ssl getSsl(ClientAuth clientAuth, String keyPassword, String keyAlias, String keyStore) { return getSsl(clientAuth, keyPassword, keyAlias, keyStore, null, null, null); }