From a83844df32f08a18ba2e78a76f98205a611d1254 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 14 Jul 2020 16:10:07 +1000 Subject: [PATCH] changes from review Signed-off-by: Lachlan Roberts --- ...eload.xml => jetty-ssl-context-reload.xml} | 5 +- .../src/main/config/modules/ssl-reload.mod | 7 +- ...StoreScanner.java => KeyStoreScanner.java} | 75 +++++++------------ ...loadTest.java => KeyStoreScannerTest.java} | 10 +-- .../test/resources/jetty-logging.properties | 2 +- 5 files changed, 38 insertions(+), 61 deletions(-) rename jetty-server/src/main/config/etc/{jetty-ssl-reload.xml => jetty-ssl-context-reload.xml} (74%) rename jetty-util/src/main/java/org/eclipse/jetty/util/ssl/{SslKeyStoreScanner.java => KeyStoreScanner.java} (60%) rename tests/test-integration/src/test/java/org/eclipse/jetty/test/{KeystoreReloadTest.java => KeyStoreScannerTest.java} (96%) diff --git a/jetty-server/src/main/config/etc/jetty-ssl-reload.xml b/jetty-server/src/main/config/etc/jetty-ssl-context-reload.xml similarity index 74% rename from jetty-server/src/main/config/etc/jetty-ssl-reload.xml rename to jetty-server/src/main/config/etc/jetty-ssl-context-reload.xml index 7520ee85378..267bda744bc 100644 --- a/jetty-server/src/main/config/etc/jetty-ssl-reload.xml +++ b/jetty-server/src/main/config/etc/jetty-ssl-context-reload.xml @@ -1,12 +1,11 @@ - - + - + diff --git a/jetty-server/src/main/config/modules/ssl-reload.mod b/jetty-server/src/main/config/modules/ssl-reload.mod index e77bec8259f..68eb6c95012 100644 --- a/jetty-server/src/main/config/modules/ssl-reload.mod +++ b/jetty-server/src/main/config/modules/ssl-reload.mod @@ -10,12 +10,9 @@ ssl [depend] ssl -[lib] -lib/jetty-ssl-reload-${jetty.version}.jar - [xml] -etc/jetty-ssl-reload.xml +etc/jetty-ssl-context-reload.xml [ini-template] # Monitored directory scan period (seconds) -# jetty.ssl.reload.scanInterval=1 \ No newline at end of file +# jetty.sslContext.reload.scanInterval=1 \ No newline at end of file diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslKeyStoreScanner.java b/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/KeyStoreScanner.java similarity index 60% rename from jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslKeyStoreScanner.java rename to jetty-util/src/main/java/org/eclipse/jetty/util/ssl/KeyStoreScanner.java index b6224151fff..2e311dd6b69 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslKeyStoreScanner.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/KeyStoreScanner.java @@ -19,74 +19,56 @@ package org.eclipse.jetty.util.ssl; import java.io.File; -import java.net.URI; -import java.util.ArrayList; -import java.util.List; +import java.io.IOException; +import java.util.Collections; import org.eclipse.jetty.util.Scanner; import org.eclipse.jetty.util.annotation.ManagedAttribute; import org.eclipse.jetty.util.annotation.ManagedOperation; -import org.eclipse.jetty.util.component.AbstractLifeCycle; +import org.eclipse.jetty.util.component.ContainerLifeCycle; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; -public class SslKeyStoreScanner extends AbstractLifeCycle implements Scanner.DiscreteListener +public class KeyStoreScanner extends ContainerLifeCycle implements Scanner.DiscreteListener { - private static final Logger LOG = Log.getLogger(SslKeyStoreScanner.class); + private static final Logger LOG = Log.getLogger(KeyStoreScanner.class); private final SslContextFactory sslContextFactory; private final File keystoreFile; - private final List files = new ArrayList<>(); - private Scanner _scanner; - private int _scanInterval = 1; + private final Scanner _scanner; - public SslKeyStoreScanner(SslContextFactory sslContextFactory) + public KeyStoreScanner(SslContextFactory sslContextFactory) { this.sslContextFactory = sslContextFactory; - this.keystoreFile = new File(URI.create(sslContextFactory.getKeyStorePath())); // getKeyStorePath is giving url instead of path? - if (!keystoreFile.exists()) - throw new IllegalArgumentException("keystore file does not exist"); - if (keystoreFile.isDirectory()) - throw new IllegalArgumentException("expected keystore file not directory"); + try + { + keystoreFile = sslContextFactory.getKeyStoreResource().getFile(); + if (keystoreFile == null || !keystoreFile.exists()) + throw new IllegalArgumentException("keystore file does not exist"); + if (keystoreFile.isDirectory()) + throw new IllegalArgumentException("expected keystore file not directory"); + } + catch (IOException e) + { + throw new IllegalArgumentException("could not obtain keystore file", e); + } File parentFile = keystoreFile.getParentFile(); if (!parentFile.exists() || !parentFile.isDirectory()) throw new IllegalArgumentException("error obtaining keystore dir"); - files.add(parentFile); - if (LOG.isDebugEnabled()) - LOG.debug("created {}", this); - } - - @Override - protected void doStart() throws Exception - { - if (LOG.isDebugEnabled()) - LOG.debug(this.getClass().getSimpleName() + ".doStart()"); - _scanner = new Scanner(); - _scanner.setScanDirs(files); - _scanner.setScanInterval(_scanInterval); + _scanner.setScanDirs(Collections.singletonList(parentFile)); + _scanner.setScanInterval(1); _scanner.setReportDirs(false); _scanner.setReportExistingFilesOnStartup(false); _scanner.setScanDepth(1); _scanner.addListener(this); - _scanner.start(); + addBean(_scanner); } @Override - protected void doStop() throws Exception - { - if (_scanner != null) - { - _scanner.stop(); - _scanner.removeListener(this); - _scanner = null; - } - } - - @Override - public void fileAdded(String filename) throws Exception + public void fileAdded(String filename) { if (LOG.isDebugEnabled()) LOG.debug("added {}", filename); @@ -96,7 +78,7 @@ public class SslKeyStoreScanner extends AbstractLifeCycle implements Scanner.Dis } @Override - public void fileChanged(String filename) throws Exception + public void fileChanged(String filename) { if (LOG.isDebugEnabled()) LOG.debug("changed {}", filename); @@ -106,18 +88,17 @@ public class SslKeyStoreScanner extends AbstractLifeCycle implements Scanner.Dis } @Override - public void fileRemoved(String filename) throws Exception + public void fileRemoved(String filename) { if (LOG.isDebugEnabled()) LOG.debug("removed {}", filename); - // TODO: do we want to do this? if (keystoreFile.toPath().toString().equals(filename)) reload(); } @ManagedOperation(value = "Reload the SSL Keystore", impact = "ACTION") - public void reload() throws Exception + public void reload() { if (LOG.isDebugEnabled()) LOG.debug("reloading keystore file {}", keystoreFile); @@ -135,11 +116,11 @@ public class SslKeyStoreScanner extends AbstractLifeCycle implements Scanner.Dis @ManagedAttribute("scanning interval to detect changes which need reloaded") public int getScanInterval() { - return _scanInterval; + return _scanner.getScanInterval(); } public void setScanInterval(int scanInterval) { - _scanInterval = scanInterval; + _scanner.setScanInterval(scanInterval); } } diff --git a/tests/test-integration/src/test/java/org/eclipse/jetty/test/KeystoreReloadTest.java b/tests/test-integration/src/test/java/org/eclipse/jetty/test/KeyStoreScannerTest.java similarity index 96% rename from tests/test-integration/src/test/java/org/eclipse/jetty/test/KeystoreReloadTest.java rename to tests/test-integration/src/test/java/org/eclipse/jetty/test/KeyStoreScannerTest.java index 6ce107718c8..1b54fdefae7 100644 --- a/tests/test-integration/src/test/java/org/eclipse/jetty/test/KeystoreReloadTest.java +++ b/tests/test-integration/src/test/java/org/eclipse/jetty/test/KeyStoreScannerTest.java @@ -43,8 +43,8 @@ import org.eclipse.jetty.toolchain.test.MavenTestingUtils; import org.eclipse.jetty.toolchain.test.jupiter.WorkDir; import org.eclipse.jetty.toolchain.test.jupiter.WorkDirExtension; import org.eclipse.jetty.util.log.StacklessLogging; +import org.eclipse.jetty.util.ssl.KeyStoreScanner; import org.eclipse.jetty.util.ssl.SslContextFactory; -import org.eclipse.jetty.util.ssl.SslKeyStoreScanner; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -55,7 +55,7 @@ import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertThrows; @ExtendWith(WorkDirExtension.class) -public class KeystoreReloadTest +public class KeyStoreScannerTest { private static final int scanInterval = 1; public WorkDir testdir; @@ -99,7 +99,7 @@ public class KeystoreReloadTest server.addConnector(connector); // Configure Keystore Reload. - SslKeyStoreScanner keystoreScanner = new SslKeyStoreScanner(sslContextFactory); + KeyStoreScanner keystoreScanner = new KeyStoreScanner(sslContextFactory); keystoreScanner.setScanInterval(scanInterval); server.addBean(keystoreScanner); @@ -140,7 +140,7 @@ public class KeystoreReloadTest assertThat(getExpiryYear(cert1), is(2015)); // Switch to use badKeystore which has the incorrect passwords. - try (StacklessLogging ignored = new StacklessLogging(SslKeyStoreScanner.class)) + try (StacklessLogging ignored = new StacklessLogging(KeyStoreScanner.class)) { useKeystore("badKeystore"); Thread.sleep(Duration.ofSeconds(scanInterval * 2).toMillis()); @@ -160,7 +160,7 @@ public class KeystoreReloadTest assertThat(getExpiryYear(cert1), is(2015)); // Delete the keystore. - try (StacklessLogging ignored = new StacklessLogging(SslKeyStoreScanner.class)) + try (StacklessLogging ignored = new StacklessLogging(KeyStoreScanner.class)) { useKeystore(null); Thread.sleep(Duration.ofSeconds(scanInterval * 2).toMillis()); diff --git a/tests/test-integration/src/test/resources/jetty-logging.properties b/tests/test-integration/src/test/resources/jetty-logging.properties index faa733ccad5..d608f24d0e0 100644 --- a/tests/test-integration/src/test/resources/jetty-logging.properties +++ b/tests/test-integration/src/test/resources/jetty-logging.properties @@ -2,4 +2,4 @@ org.eclipse.jetty.util.log.class=org.eclipse.jetty.util.log.StdErrLog #org.eclipse.jetty.util.log.class=org.eclipse.jetty.util.log.Slf4jLog #org.eclipse.jetty.LEVEL=DEBUG #org.eclipse.jetty.websocket.LEVEL=DEBUG -#org.eclipse.jetty.util.ssl.SslKeyStoreScanner.LEVEL=DEBUG +#org.eclipse.jetty.util.ssl.KeyStoreScanner.LEVEL=DEBUG