From 7e1de8b1e2877663c1ff25b1e3cc15be6de35271 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Wed, 7 Dec 2022 09:24:19 -0600 Subject: [PATCH] Issue #8973 - Rework KeyStoreScanner handling for symlink related changes (#9014) * Issue #8973 - Rework KeyStoreScanner handling for symlink related changes + Removed changes from #8786 and #8787 + More test cases + revert jetty.sslContext.reload.followLinks boolean + Scanner should follow its own linkOptions setting + remove bad documentation in module-ssl-reload.adoc Signed-off-by: Joakim Erdfelt Signed-off-by: Lachlan Roberts Co-authored-by: Lachlan Roberts --- .../modules/module-ssl-reload.adoc | 3 - .../config/etc/jetty-ssl-context-reload.xml | 1 - .../src/main/config/modules/ssl-reload.mod | 3 - .../java/org/eclipse/jetty/util/Scanner.java | 6 +- .../jetty/util/ssl/KeyStoreScanner.java | 32 ++-- .../jetty/test/KeyStoreScannerTest.java | 167 +++++++++++++++++- .../test/resources/jetty-logging.properties | 1 + 7 files changed, 181 insertions(+), 32 deletions(-) diff --git a/documentation/jetty-documentation/src/main/asciidoc/operations-guide/modules/module-ssl-reload.adoc b/documentation/jetty-documentation/src/main/asciidoc/operations-guide/modules/module-ssl-reload.adoc index 96ff3d6270c9..b8621b8a5cc5 100644 --- a/documentation/jetty-documentation/src/main/asciidoc/operations-guide/modules/module-ssl-reload.adoc +++ b/documentation/jetty-documentation/src/main/asciidoc/operations-guide/modules/module-ssl-reload.adoc @@ -22,6 +22,3 @@ The module properties are: ---- include::{JETTY_HOME}/modules/ssl-reload.mod[tags=documentation] ---- - -The `followLinks` property is used to specify whether symlinks should be resolved in the path of the KeyStore. -If set to false and the path of the KeyStore is a symbolic link, the scanner will monitor the symbolic link file for changes instead of its target. diff --git a/jetty-server/src/main/config/etc/jetty-ssl-context-reload.xml b/jetty-server/src/main/config/etc/jetty-ssl-context-reload.xml index e1de1cc08feb..46346359ed74 100644 --- a/jetty-server/src/main/config/etc/jetty-ssl-context-reload.xml +++ b/jetty-server/src/main/config/etc/jetty-ssl-context-reload.xml @@ -5,7 +5,6 @@ - diff --git a/jetty-server/src/main/config/modules/ssl-reload.mod b/jetty-server/src/main/config/modules/ssl-reload.mod index 68ff5d5d9336..9dca8473d55b 100644 --- a/jetty-server/src/main/config/modules/ssl-reload.mod +++ b/jetty-server/src/main/config/modules/ssl-reload.mod @@ -15,7 +15,4 @@ etc/jetty-ssl-context-reload.xml # tag::documentation[] # Monitored directory scan period, in seconds. # jetty.sslContext.reload.scanInterval=1 - -# Whether to resolve symbolic links in the KeyStore path. -# jetty.sslContext.reload.followLinks=true # end::documentation[] diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/Scanner.java b/jetty-util/src/main/java/org/eclipse/jetty/util/Scanner.java index 11c602341de6..556c5a29e28f 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/Scanner.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/Scanner.java @@ -422,8 +422,8 @@ public void addFile(Path path) try { - // Always follow links when check ultimate type of the path - Path real = path.toRealPath(); + // Check status of the real path + Path real = path.toRealPath(_linkOptions); if (!Files.exists(real) || Files.isDirectory(real)) throw new IllegalStateException("Not file or doesn't exist: " + path); @@ -452,7 +452,7 @@ public IncludeExcludeSet addDirectory(Path p) try { // Check status of the real path - Path real = p.toRealPath(); + Path real = p.toRealPath(_linkOptions); if (!Files.exists(real) || !Files.isDirectory(real)) throw new IllegalStateException("Not directory or doesn't exist: " + p); diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/KeyStoreScanner.java b/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/KeyStoreScanner.java index 5bc750ea6f70..874dee7b87ba 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/KeyStoreScanner.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/KeyStoreScanner.java @@ -15,6 +15,7 @@ import java.io.File; import java.io.IOException; +import java.nio.file.Path; import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; import java.util.function.Consumer; @@ -43,11 +44,6 @@ public class KeyStoreScanner extends ContainerLifeCycle implements Scanner.Discr private final Scanner _scanner; public KeyStoreScanner(SslContextFactory sslContextFactory) - { - this(sslContextFactory, true); - } - - public KeyStoreScanner(SslContextFactory sslContextFactory, boolean followLinks) { this.sslContextFactory = sslContextFactory; try @@ -59,12 +55,6 @@ public KeyStoreScanner(SslContextFactory sslContextFactory, boolean followLinks) if (monitoredFile.isDirectory()) throw new IllegalArgumentException("expected keystore file not directory"); - if (followLinks && keystoreResource.isAlias()) - { - // This resource has an alias, so monitor the target of the alias. - monitoredFile = new File(keystoreResource.getAlias()); - } - keystoreFile = monitoredFile; if (LOG.isDebugEnabled()) LOG.debug("Monitored Keystore File: {}", monitoredFile); @@ -78,7 +68,7 @@ public KeyStoreScanner(SslContextFactory sslContextFactory, boolean followLinks) if (!parentFile.exists() || !parentFile.isDirectory()) throw new IllegalArgumentException("error obtaining keystore dir"); - _scanner = new Scanner(null, followLinks); + _scanner = new Scanner(null, false); _scanner.addDirectory(parentFile.toPath()); _scanner.setScanInterval(1); _scanner.setReportDirs(false); @@ -88,11 +78,23 @@ public KeyStoreScanner(SslContextFactory sslContextFactory, boolean followLinks) addBean(_scanner); } + private Path getRealKeyStorePath() + { + try + { + return keystoreFile.toPath().toRealPath(); + } + catch (IOException e) + { + return keystoreFile.toPath(); + } + } + @Override public void fileAdded(String filename) { if (LOG.isDebugEnabled()) - LOG.debug("added {}", filename); + LOG.debug("fileAdded {} - keystoreFile.toReal {}", filename, getRealKeyStorePath()); if (keystoreFile.toPath().toString().equals(filename)) reload(); @@ -102,7 +104,7 @@ public void fileAdded(String filename) public void fileChanged(String filename) { if (LOG.isDebugEnabled()) - LOG.debug("changed {}", filename); + LOG.debug("fileChanged {} - keystoreFile.toReal {}", filename, getRealKeyStorePath()); if (keystoreFile.toPath().toString().equals(filename)) reload(); @@ -112,7 +114,7 @@ public void fileChanged(String filename) public void fileRemoved(String filename) { if (LOG.isDebugEnabled()) - LOG.debug("removed {}", filename); + LOG.debug("fileRemoved {} - keystoreFile.toReal {}", filename, getRealKeyStorePath()); if (keystoreFile.toPath().toString().equals(filename)) reload(); diff --git a/tests/test-integration/src/test/java/org/eclipse/jetty/test/KeyStoreScannerTest.java b/tests/test-integration/src/test/java/org/eclipse/jetty/test/KeyStoreScannerTest.java index 323ec38ec824..a91fbcfa7ffa 100644 --- a/tests/test-integration/src/test/java/org/eclipse/jetty/test/KeyStoreScannerTest.java +++ b/tests/test-integration/src/test/java/org/eclipse/jetty/test/KeyStoreScannerTest.java @@ -18,6 +18,7 @@ import java.nio.file.FileSystemException; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.Paths; import java.nio.file.StandardCopyOption; import java.security.SecureRandom; import java.security.cert.Certificate; @@ -87,11 +88,6 @@ public void start() throws Exception } public void start(Configuration configuration) throws Exception - { - start(configuration, true); - } - - public void start(Configuration configuration, boolean resolveAlias) throws Exception { SslContextFactory.Server sslContextFactory = new SslContextFactory.Server(); configuration.configure(sslContextFactory); @@ -105,7 +101,7 @@ public void start(Configuration configuration, boolean resolveAlias) throws Exce server.addConnector(connector); // Configure Keystore Reload. - keyStoreScanner = new KeyStoreScanner(sslContextFactory, resolveAlias); + keyStoreScanner = new KeyStoreScanner(sslContextFactory); keyStoreScanner.setScanInterval(0); server.addBean(keyStoreScanner); @@ -197,7 +193,7 @@ public void testReloadChangingSymbolicLink() throws Exception sslContextFactory.setKeyStorePath(symlinkKeystorePath.toString()); sslContextFactory.setKeyStorePassword("storepwd"); sslContextFactory.setKeyManagerPassword("keypwd"); - }, false); + }); // Check the original certificate expiry. X509Certificate cert1 = getCertificateFromServer(); @@ -245,6 +241,163 @@ public void testReloadChangingTargetOfSymbolicLink() throws Exception assertThat(getExpiryYear(cert2), is(2020)); } + @Test + public void testReloadChangingLinkTargetOfSymbolicLink() throws Exception + { + assumeFileSystemSupportsSymlink(); + Path oldKeyStoreSrc = MavenTestingUtils.getTestResourcePathFile("oldKeyStore"); + Path newKeyStoreSrc = MavenTestingUtils.getTestResourcePathFile("newKeyStore"); + + Path sslDir = keystoreDir.resolve("ssl"); + Path optDir = keystoreDir.resolve("opt"); + Path optKeystoreLink = optDir.resolve("keystore"); + Path optKeystore1 = optDir.resolve("keystore.1"); + Path optKeystore2 = optDir.resolve("keystore.2"); + Path keystoreFile = sslDir.resolve("keystore"); + + start(sslContextFactory -> + { + // What we want is .. + // (link) ssl/keystore -> opt/keystore + // (link) opt/keystore -> opt/keystore.1 + // (file) opt/keystore.1 (actual certificate) + + FS.ensureEmpty(sslDir); + FS.ensureEmpty(optDir); + Files.copy(oldKeyStoreSrc, optKeystore1); + Files.createSymbolicLink(optKeystoreLink, optKeystore1); + Files.createSymbolicLink(keystoreFile, optKeystoreLink); + + sslContextFactory.setKeyStorePath(keystoreFile.toString()); + sslContextFactory.setKeyStorePassword("storepwd"); + sslContextFactory.setKeyManagerPassword("keypwd"); + }); + + // Check the original certificate expiry. + X509Certificate cert1 = getCertificateFromServer(); + assertThat(getExpiryYear(cert1), is(2015)); + + // Create a new keystore file opt/keystore.2 with new expiry + Files.copy(newKeyStoreSrc, optKeystore2); + // Change (link) opt/keystore -> opt/keystore.2 + Files.delete(optKeystoreLink); + Files.createSymbolicLink(optKeystoreLink, optKeystore2); + System.err.println("### Triggering scan"); + keyStoreScanner.scan(5000); + + // The scanner should have detected the updated keystore, expiry should be renewed. + X509Certificate cert2 = getCertificateFromServer(); + assertThat(getExpiryYear(cert2), is(2020)); + } + + /** + * Test a keystore, where the monitored directory is a symlink. + */ + @Test + public void testSymlinkedMonitoredDirectory() throws Exception + { + assumeFileSystemSupportsSymlink(); + Path oldKeyStoreSrc = MavenTestingUtils.getTestResourcePathFile("oldKeyStore"); + Path newKeyStoreSrc = MavenTestingUtils.getTestResourcePathFile("newKeyStore"); + + Path dataLinkDir = keystoreDir.resolve("data_symlink"); + Path dataDir = keystoreDir.resolve("data"); + Path etcDir = keystoreDir.resolve("etc"); + Path dataLinkKeystore = dataLinkDir.resolve("keystore"); + Path dataKeystore = dataDir.resolve("keystore"); + Path etcKeystore = etcDir.resolve("keystore"); + + start(sslContextFactory -> + { + // What we want is .. + // (link) data_symlink/ -> data/ + // (link) data/keystore -> etc/keystore + // (file) etc/keystore (actual certificate) + + FS.ensureEmpty(etcDir); + FS.ensureEmpty(dataDir); + Files.copy(oldKeyStoreSrc, etcKeystore); + Files.createSymbolicLink(dataLinkDir, dataDir); + Files.createSymbolicLink(dataKeystore, etcKeystore); + + sslContextFactory.setKeyStorePath(dataLinkKeystore.toString()); + sslContextFactory.setKeyStorePassword("storepwd"); + sslContextFactory.setKeyManagerPassword("keypwd"); + }); + + // Check the original certificate expiry. + X509Certificate cert1 = getCertificateFromServer(); + assertThat(getExpiryYear(cert1), is(2015)); + + // Update etc/keystore + Files.copy(newKeyStoreSrc, etcKeystore, StandardCopyOption.REPLACE_EXISTING); + System.err.println("### Triggering scan"); + keyStoreScanner.scan(5000); + + // The scanner should have detected the updated keystore, expiry should be renewed. + X509Certificate cert2 = getCertificateFromServer(); + assertThat(getExpiryYear(cert2), is(2020)); + } + + /** + * Test a doubly-linked keystore, and refreshing by only modifying the middle symlink. + */ + @Test + public void testDoublySymlinkedTimestampedDir() throws Exception + { + assumeFileSystemSupportsSymlink(); + Path oldKeyStoreSrc = MavenTestingUtils.getTestResourcePathFile("oldKeyStore"); + Path newKeyStoreSrc = MavenTestingUtils.getTestResourcePathFile("newKeyStore"); + + Path sslDir = keystoreDir.resolve("ssl"); + Path dataDir = sslDir.resolve("data"); + Path timestampNovDir = sslDir.resolve("2022-11"); + Path timestampDecDir = sslDir.resolve("2022-12"); + Path targetNov = timestampNovDir.resolve("keystore.p12"); + Path targetDec = timestampDecDir.resolve("keystore.p12"); + + start(sslContextFactory -> + { + // What we want is .. + // (link) keystore.p12 -> data/keystore.p12 + // (link) data/ -> 2022-11/ + // (file) 2022-11/keystore.p12 (actual certificate) + + FS.ensureEmpty(sslDir); + FS.ensureEmpty(timestampNovDir); + FS.ensureEmpty(timestampDecDir); + Files.copy(oldKeyStoreSrc, targetNov); + Files.copy(newKeyStoreSrc, targetDec); + + // Create symlink of data/ to 2022-11/ + Files.createSymbolicLink(dataDir, timestampNovDir.getFileName()); + + // Create symlink of keystore.p12 to data/keystore.p12 + Path keystoreLink = sslDir.resolve("keystore.p12"); + Files.createSymbolicLink(keystoreLink, Paths.get("data/keystore.p12")); + + sslContextFactory.setKeyStorePath(keystoreLink.toString()); + sslContextFactory.setKeyStorePassword("storepwd"); + sslContextFactory.setKeyManagerPassword("keypwd"); + }); + + // Check the original certificate expiry. + X509Certificate cert1 = getCertificateFromServer(); + assertThat(getExpiryYear(cert1), is(2015)); + + // Replace keystore link + Files.delete(dataDir); + Files.createSymbolicLink(dataDir, timestampDecDir.getFileName()); + // (link) data/ -> 2022-12/ + // now keystore.p12 points to data/keystore.p12 which points to 2022-12/keystore.p12 + System.err.println("### Triggering scan"); + keyStoreScanner.scan(5000); + + // The scanner should have detected the updated keystore, expiry should be renewed. + X509Certificate cert2 = getCertificateFromServer(); + assertThat(getExpiryYear(cert2), is(2020)); + } + public Path useKeystore(String keystoreToUse, String keystorePath) throws Exception { return useKeystore(MavenTestingUtils.getTestResourcePath(keystoreToUse), keystoreDir.resolve(keystorePath)); diff --git a/tests/test-integration/src/test/resources/jetty-logging.properties b/tests/test-integration/src/test/resources/jetty-logging.properties index 0003cced627a..a3114ec6f6df 100644 --- a/tests/test-integration/src/test/resources/jetty-logging.properties +++ b/tests/test-integration/src/test/resources/jetty-logging.properties @@ -2,3 +2,4 @@ #org.eclipse.jetty.LEVEL=DEBUG #org.eclipse.jetty.websocket.LEVEL=DEBUG #org.eclipse.jetty.util.ssl.KeyStoreScanner.LEVEL=DEBUG +#org.eclipse.jetty.util.Scanner.LEVEL=DEBUG