diff --git a/core/pom.xml b/core/pom.xml index 8c99423b1b..cecf765007 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -142,5 +142,10 @@ ${guava.version} test + + org.awaitility + awaitility + test + diff --git a/core/src/main/java/io/confluent/rest/Application.java b/core/src/main/java/io/confluent/rest/Application.java index 6bfbdd9521..b92318b163 100644 --- a/core/src/main/java/io/confluent/rest/Application.java +++ b/core/src/main/java/io/confluent/rest/Application.java @@ -545,7 +545,9 @@ public void join() throws InterruptedException { * @throws Exception If the application fails to stop */ public void stop() throws Exception { - server.stop(); + if (server != null) { + server.stop(); + } } final void doShutdown() { diff --git a/core/src/main/java/io/confluent/rest/ApplicationServer.java b/core/src/main/java/io/confluent/rest/ApplicationServer.java index e031caed16..490acecd1a 100644 --- a/core/src/main/java/io/confluent/rest/ApplicationServer.java +++ b/core/src/main/java/io/confluent/rest/ApplicationServer.java @@ -54,6 +54,7 @@ public final class ApplicationServer extends Server { private final T config; private final ApplicationGroup applications; private final SslContextFactory sslContextFactory; + private FileWatcher sslKeystoreFileWatcher; private List connectors = new ArrayList<>(); @@ -177,6 +178,9 @@ private void finalizeHandlerCollection(HandlerCollection handlers, HandlerCollec protected void doStop() throws Exception { super.doStop(); applications.doStop(); + if (sslKeystoreFileWatcher != null) { + sslKeystoreFileWatcher.shutdown(); + } } protected final void doStart() throws Exception { @@ -243,7 +247,9 @@ private Path getWatchLocation(RestConfig config) { return keystorePath; } + // CHECKSTYLE_RULES.OFF: CyclomaticComplexity|NPathComplexity private SslContextFactory createSslContextFactory(RestConfig config) { + // CHECKSTYLE_RULES.ON: CyclomaticComplexity|NPathComplexity SslContextFactory sslContextFactory = new SslContextFactory.Server(); if (!config.getString(RestConfig.SSL_KEYSTORE_LOCATION_CONFIG).isEmpty()) { sslContextFactory.setKeyStorePath( @@ -267,7 +273,9 @@ private SslContextFactory createSslContextFactory(RestConfig config) { if (config.getBoolean(RestConfig.SSL_KEYSTORE_RELOAD_CONFIG)) { Path watchLocation = getWatchLocation(config); try { - FileWatcher.onFileChange(watchLocation, () -> { + // create and shutdown a sslKeystoreFileWatcher for each Application, so that + // all Applications in the same JVM don't use the same shared threadpool + sslKeystoreFileWatcher = FileWatcher.onFileChange(watchLocation, () -> { // Need to reset the key store path for symbolic link case sslContextFactory.setKeyStorePath( config.getString(RestConfig.SSL_KEYSTORE_LOCATION_CONFIG) @@ -301,9 +309,11 @@ private SslContextFactory createSslContextFactory(RestConfig config) { sslContextFactory.setTrustStorePath( config.getString(RestConfig.SSL_TRUSTSTORE_LOCATION_CONFIG) ); - sslContextFactory.setTrustStorePassword( - config.getPassword(RestConfig.SSL_TRUSTSTORE_PASSWORD_CONFIG).value() - ); + if (config.getPassword(RestConfig.SSL_TRUSTSTORE_PASSWORD_CONFIG) != null) { + sslContextFactory.setTrustStorePassword( + config.getPassword(RestConfig.SSL_TRUSTSTORE_PASSWORD_CONFIG).value() + ); + } sslContextFactory.setTrustStoreType( config.getString(RestConfig.SSL_TRUSTSTORE_TYPE_CONFIG) ); diff --git a/core/src/main/java/io/confluent/rest/FileWatcher.java b/core/src/main/java/io/confluent/rest/FileWatcher.java index c129847a95..e80652402e 100644 --- a/core/src/main/java/io/confluent/rest/FileWatcher.java +++ b/core/src/main/java/io/confluent/rest/FileWatcher.java @@ -35,7 +35,9 @@ // reference https://gist.github.com/danielflower/f54c2fe42d32356301c68860a4ab21ed public class FileWatcher implements Runnable { private static final Logger log = LoggerFactory.getLogger(FileWatcher.class); - private static final ExecutorService executor = Executors.newFixedThreadPool(1, + + // don't have static shared threadpool for all FileWatchers in the JVM + private final ExecutorService executor = Executors.newFixedThreadPool(1, new ThreadFactory() { public Thread newThread(Runnable r) { Thread t = Executors.defaultThreadFactory().newThread(r); @@ -67,10 +69,11 @@ public FileWatcher(Path file, Callback callback) throws IOException { * Starts watching a file calls the callback when it is changed. * A shutdown hook is registered to stop watching. */ - public static void onFileChange(Path file, Callback callback) throws IOException { + public static FileWatcher onFileChange(Path file, Callback callback) throws IOException { log.info("Configure watch file change: " + file); FileWatcher fileWatcher = new FileWatcher(file, callback); - executor.submit(fileWatcher); + fileWatcher.executor.submit(fileWatcher); + return fileWatcher; } public void run() { diff --git a/core/src/main/java/io/confluent/rest/RestConfig.java b/core/src/main/java/io/confluent/rest/RestConfig.java index 54fba9c2e1..ce0a4acece 100644 --- a/core/src/main/java/io/confluent/rest/RestConfig.java +++ b/core/src/main/java/io/confluent/rest/RestConfig.java @@ -161,7 +161,7 @@ public class RestConfig extends AbstractConfig { public static final String SSL_TRUSTSTORE_PASSWORD_CONFIG = "ssl.truststore.password"; protected static final String SSL_TRUSTSTORE_PASSWORD_DOC = "The store password for the trust store file."; - protected static final String SSL_TRUSTSTORE_PASSWORD_DEFAULT = ""; + protected static final String SSL_TRUSTSTORE_PASSWORD_DEFAULT = null; public static final String SSL_TRUSTSTORE_TYPE_CONFIG = "ssl.truststore.type"; protected static final String SSL_TRUSTSTORE_TYPE_DOC = "The type of trust store file."; diff --git a/core/src/test/java/io/confluent/rest/ApiHeadersTest.java b/core/src/test/java/io/confluent/rest/ApiHeadersTest.java index ef5a0e13eb..bf24d7345b 100644 --- a/core/src/test/java/io/confluent/rest/ApiHeadersTest.java +++ b/core/src/test/java/io/confluent/rest/ApiHeadersTest.java @@ -45,6 +45,7 @@ import org.junit.AfterClass; import org.junit.BeforeClass; import org.junit.Test; +import org.junit.rules.TemporaryFolder; public class ApiHeadersTest { @@ -56,11 +57,16 @@ public class ApiHeadersTest { private static String clientKeystoreLocation; private static TestApplication app; + // Use a temporary folder so that .jks files created by this test are isolated + // and deleted when the test is done + public static TemporaryFolder tempFolder = new TemporaryFolder(); + @BeforeClass public static void setUp() throws Exception { - final File trustStore = File.createTempFile("ApiHeadersTest-truststore", ".jks"); - final File clientKeystore = File.createTempFile("ApiHeadersTest-client-keystore", ".jks"); - final File serverKeystore = File.createTempFile("ApiHeadersTest-server-keystore", ".jks"); + tempFolder.create(); + final File trustStore = File.createTempFile("ApiHeadersTest-truststore", ".jks", tempFolder.getRoot()); + final File clientKeystore = File.createTempFile("ApiHeadersTest-client-keystore", ".jks", tempFolder.getRoot()); + final File serverKeystore = File.createTempFile("ApiHeadersTest-server-keystore", ".jks", tempFolder.getRoot()); clientKeystoreLocation = clientKeystore.getAbsolutePath(); @@ -84,6 +90,7 @@ public static void teardown() throws Exception { if (app != null) { app.stop(); } + tempFolder.delete(); } @Test @@ -130,7 +137,7 @@ private static void createKeystoreWithCert(File file, String alias, Map certs) throws Exception { + @AfterClass + public static void teardown() { + tempFolder.delete(); + } + + private static void createKeystoreWithCert(File file, String alias, Map certs) throws Exception { KeyPair keypair = TestSslUtils.generateKeyPair("RSA"); CertificateBuilder certificateBuilder = new CertificateBuilder(30, "SHA1withRSA"); X509Certificate cCert = certificateBuilder.sanDnsName("localhost") .generate("CN=mymachine.local, O=A client", keypair); - TestSslUtils.createKeyStore(file.getPath(), new Password(SSL_PASSWORD), alias, keypair.getPrivate(), cCert); + TestSslUtils.createKeyStore(file.getPath(), new Password(SSL_PASSWORD), new Password(SSL_PASSWORD), alias, keypair.getPrivate(), cCert); certs.put(alias, cCert); } @@ -116,16 +144,25 @@ private void configServerTruststore(Properties props) { props.put(RestConfig.SSL_TRUSTSTORE_PASSWORD_CONFIG, SSL_PASSWORD); } + private void configServerTruststore(Properties props, String password) { + props.put(RestConfig.SSL_TRUSTSTORE_LOCATION_CONFIG, trustStore.getAbsolutePath()); + props.put(RestConfig.SSL_TRUSTSTORE_PASSWORD_CONFIG, password); + } + + private void configServerNoTruststorePassword(Properties props) { + props.put(RestConfig.SSL_TRUSTSTORE_LOCATION_CONFIG, trustStore.getAbsolutePath()); + } + private void enableSslClientAuth(Properties props) { props.put(RestConfig.SSL_CLIENT_AUTH_CONFIG, true); } - private void createWrongKeystoreWithCert(File file, String alias, Map certs) throws Exception { + private static void createWrongKeystoreWithCert(File file, String alias, Map certs) throws Exception { KeyPair keypair = TestSslUtils.generateKeyPair("RSA"); CertificateBuilder certificateBuilder = new CertificateBuilder(30, "SHA1withRSA"); X509Certificate cCert = certificateBuilder.sanDnsName("fail") .generate("CN=mymachine.local, O=A client", keypair); - TestSslUtils.createKeyStore(file.getPath(), new Password(SSL_PASSWORD), alias, keypair.getPrivate(), cCert); + TestSslUtils.createKeyStore(file.getPath(), new Password(SSL_PASSWORD), new Password(SSL_PASSWORD), alias, keypair.getPrivate(), cCert); certs.put(alias, cCert); } @@ -168,30 +205,44 @@ public void testHttpsWithAutoReload() throws Exception { SslTestApplication app = new SslTestApplication(config); try { app.start(); - int statusCode = makeGetRequest(httpsUri + "/test", + int startingCode = makeGetRequest(httpsUri + "/test", clientKeystore.getAbsolutePath(), SSL_PASSWORD, SSL_PASSWORD); - assertEquals(EXPECTED_200_MSG, 200, statusCode); + assertEquals(EXPECTED_200_MSG, 200, startingCode); assertMetricsCollected(); // verify reload -- override the server keystore with a wrong one Files.copy(serverKeystoreErr.toPath(), serverKeystore.toPath(), StandardCopyOption.REPLACE_EXISTING); - Thread.sleep(CERT_RELOAD_WAIT_TIME); - boolean hitError = false; - try { - makeGetRequest(httpsUri + "/test", - clientKeystore.getAbsolutePath(), SSL_PASSWORD, SSL_PASSWORD); - } catch (Exception e) { - System.out.println(e); - hitError = true; - } + log.info("\tKeystore reload test : Applied bad keystore file"); + + await().pollInterval(2, TimeUnit.SECONDS).atMost(30, TimeUnit.SECONDS).untilAsserted( () -> { + boolean hitError = false; + try { + log.info("\tKeystore reload test : Awaiting failed https connection"); + makeGetRequest(httpsUri + "/test", clientKeystore.getAbsolutePath(), SSL_PASSWORD, SSL_PASSWORD); + } catch (Exception e) { + System.out.println(e); + hitError = true; + } + assertTrue("Expecting to hit an error with new server cert", hitError); + }); // verify reload -- override the server keystore with a correct one Files.copy(serverKeystoreBak.toPath(), serverKeystore.toPath(), StandardCopyOption.REPLACE_EXISTING); - Thread.sleep(CERT_RELOAD_WAIT_TIME); - statusCode = makeGetRequest(httpsUri + "/test", - clientKeystore.getAbsolutePath(), SSL_PASSWORD, SSL_PASSWORD); - assertEquals(EXPECTED_200_MSG, 200, statusCode); - assertEquals("expect hit error with new server cert", true, hitError); + log.info("\tKeystore reload test : keystore set back to good value"); + + await().pollInterval(2, TimeUnit.SECONDS).atMost(30, TimeUnit.SECONDS).untilAsserted( () -> { + try { + log.info("\tKeystore reload test : Awaiting a valid https connection"); + int statusCode = makeGetRequest(httpsUri + "/test", clientKeystore.getAbsolutePath(), SSL_PASSWORD, SSL_PASSWORD); + assertEquals(EXPECTED_200_MSG, 200, statusCode); + log.info("\tKeystore reload test : Valid connection found"); + } + catch (Exception e) { + fail(); + // we have to wait for the good key to take affect + } + }); + } finally { if (app != null) { app.stop(); @@ -271,6 +322,45 @@ public void testHttpsWithNoClientCertAndNoServerTruststore() throws Exception { } } + @Test(expected = IOException.class) + public void testHttpsWithEmptyStringTruststorePassword() throws Exception { + Properties props = new Properties(); + String uri = "https://localhost:8080"; + props.put(RestConfig.LISTENERS_CONFIG, uri); + configServerKeystore(props); + configServerTruststore(props, ""); + TestRestConfig config = new TestRestConfig(props); + SslTestApplication app = new SslTestApplication(config); + try { + // Empty string is a valid password, but it's not the password the truststore uses + // The app should fail at startup with: + // java.io.IOException: Keystore was tampered with, or password was incorrect + app.start(); + } finally { + app.stop(); + } + } + + @Test + public void testHttpsWithNoTruststorePassword() throws Exception { + Properties props = new Properties(); + String uri = "https://localhost:8080"; + props.put(RestConfig.LISTENERS_CONFIG, uri); + configServerKeystore(props); + configServerNoTruststorePassword(props); + TestRestConfig config = new TestRestConfig(props); + SslTestApplication app = new SslTestApplication(config); + try { + // With no password set (null), verification of the truststore is disabled + app.start(); + + int statusCode = makeGetRequest(uri + "/test"); + assertEquals(EXPECTED_200_MSG, 200, statusCode); + } finally { + app.stop(); + } + } + @Test(expected = SocketException.class) public void testHttpsWithAuthAndBadClientCert() throws Exception { Properties props = new Properties(); @@ -285,7 +375,7 @@ public void testHttpsWithAuthAndBadClientCert() throws Exception { app.start(); // create a new client cert that isn't in the server's trust store. - File untrustedClient = File.createTempFile("SslTest-client-keystore", ".jks"); + File untrustedClient = File.createTempFile("SslTest-client-keystore", ".jks", tempFolder.getRoot()); Map certs = new HashMap<>(); createKeystoreWithCert(untrustedClient, "client", certs); try { diff --git a/pom.xml b/pom.xml index b41764f03f..60cb18e37d 100644 --- a/pom.xml +++ b/pom.xml @@ -55,6 +55,7 @@ 2.2.0 24.0-jre checkstyle/suppressions.xml + 4.0.3 @@ -182,6 +183,12 @@ jersey-test-framework-provider-jetty ${jersey.version} + + + org.awaitility + awaitility + ${awaitility.version} +