From 0099105bb54455abc6f0278820fbfefb247cae9b Mon Sep 17 00:00:00 2001 From: Peter Paul Bakker Date: Thu, 15 Oct 2026 09:54:09 +0000 Subject: [PATCH 01/10] fix: keep current key pair when key and certificate mismatch - Check the private key matches the leaf certificate before replacing the KeyManager; keep the current one otherwise, also for an empty certificate file. Non-RSA keys are accepted with an info message. - Serialize reloads so the shared KeyManagerFactory is never initialized concurrently. - Log certificate serial, subject and chain expiry (UTC) on load; warn when the chain has expired. - Remove the commented-out private key watcher and document why only the certificate file is watched. - Document certificate rotation and mismatch recovery in the README. - Bump revision to 1.22.0-SNAPSHOT. --- README.md | 21 ++++ pom.xml | 10 +- .../FileWatchingX509ExtendedKeyManager.java | 99 +++++++++++++++---- ...ileWatchingX509ExtendedKeyManagerTest.java | 84 +++++++++++++++- 4 files changed, 191 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index 8c0bb44..1e5f617 100644 --- a/README.md +++ b/README.md @@ -7,6 +7,27 @@ The `java-buildpack-security-provider` is a utility that watches for changes to container identity and trust stores and dynamically updates the KeyManager and TrustManager of an application. +## Certificate rotation +The KeyManager is reloaded when the instance identity certificate (`CF_INSTANCE_CERT`) changes. The Diego executor writes the new private key (`CF_INSTANCE_KEY`) before the new certificate, so only the certificate file is watched. Each load logs the certificate serial, subject and when the certificate chain expires (earliest expiry in the chain, in UTC). + +Before replacing the KeyManager, the private key is checked against the certificate. Only RSA keys, the type the Diego executor issues, are checked; other key types are accepted with an informational log message. On a mismatch or an empty certificate file the current KeyManager is kept and a warning is logged: + +``` +Private key ... does not match certificate ..., keeping current KeyManager (certificate chain expires at 2026-10-15T09:30:00Z) +``` + +This is not retried. With Diego defaults (24 hour validity, rotation 30 minutes before expiry) the kept certificate expires about 30 minutes later, after which TLS handshakes using it fail. A loaded certificate chain that has already expired is installed as before, with a warning: + +``` +Certificate chain ... expired at 2026-10-15T09:30:00Z, TLS handshakes using it will fail +``` + +Alert on these warnings and restart the affected instance to get a new key pair: + +```shell +$ cf restart-app-instance APP_NAME INDEX +``` + ## Development The project depends on Java 8. To build from source, run the following: diff --git a/pom.xml b/pom.xml index 5f13e6f..1142118 100644 --- a/pom.xml +++ b/pom.xml @@ -53,17 +53,25 @@ - 1.21.0-SNAPSHOT + 1.22.0-SNAPSHOT 1.86 4.13.2 4.11.0 1.7.36 1.2.13 3.27.7 + 2.50.0 UTF-8 + + + com.google.errorprone + error_prone_annotations + ${errorprone.version} + provided + org.assertj assertj-core diff --git a/src/main/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManager.java b/src/main/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManager.java index 9fba576..80e17ff 100644 --- a/src/main/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManager.java +++ b/src/main/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManager.java @@ -16,6 +16,8 @@ package org.cloudfoundry.security; +import com.google.errorprone.annotations.concurrent.GuardedBy; + import javax.net.ssl.KeyManager; import javax.net.ssl.KeyManagerFactory; import javax.net.ssl.SSLEngine; @@ -29,10 +31,13 @@ import java.security.NoSuchAlgorithmException; import java.security.Principal; import java.security.PrivateKey; +import java.security.PublicKey; import java.security.UnrecoverableKeyException; import java.security.cert.Certificate; import java.security.cert.CertificateException; import java.security.cert.X509Certificate; +import java.security.interfaces.RSAKey; +import java.time.Instant; import java.util.List; import java.util.concurrent.atomic.AtomicReference; import java.util.logging.Logger; @@ -45,8 +50,14 @@ final class FileWatchingX509ExtendedKeyManager extends X509ExtendedKeyManager { private final AtomicReference keyManager = new AtomicReference<>(); + @GuardedBy("updateLock") private final KeyManagerFactory keyManagerFactory; + private final Object updateLock = new Object(); + + @GuardedBy("updateLock") + private Instant notAfter; + private final Path privateKey; FileWatchingX509ExtendedKeyManager(Path certificates, Path privateKey, KeyManagerFactory keyManagerFactory) { @@ -54,13 +65,11 @@ final class FileWatchingX509ExtendedKeyManager extends X509ExtendedKeyManager { this.privateKey = privateKey; this.keyManagerFactory = keyManagerFactory; - new FileWatcher(this.certificates, new FileWatcherCallback()).watch(); - // disable watching the key file to prevent race condition bug - a certificate file change covers the key change - // new FileWatcher(this.privateKey, new FileWatcherCallback()).watch(); - - if (this.keyManager.compareAndSet(null, getKeyManager(getKeyStore()))) { - this.logger.info(String.format("Initialized KeyManager for %s and %s", this.privateKey, this.certificates)); - } + // Only the certificate file is watched: the Diego executor renames the new private key into place + // before the new certificate, so a certificate change means both files are new. + // Watching the key file as well caused #8. + new FileWatcher(this.certificates, this::update).watch(); + update(); } @Override @@ -119,33 +128,81 @@ private X509ExtendedKeyManager getKeyManager(KeyStore keyStore) { } } - private KeyStore getKeyStore() { + private KeyStore getKeyStore(PrivateKey privateKey, List certificates) { try { KeyStore keyStore = KeyStoreEntryCollector.identity(); - PrivateKey privateKey = PrivateKeyFactory.generate(this.privateKey); - List certificates = X509CertificateFactory.generate(this.certificates); - KeyStoreEntryCollector.accumulate(keyStore, privateKey, new char[0], certificates.toArray(new Certificate[certificates.size()])); - return keyStore; } catch (CertificateException | IOException | KeyStoreException | NoSuchAlgorithmException e) { throw new UndeclaredThrowableException(e); } } - private final class FileWatcherCallback implements Runnable { + // Only RSA keys, the type the Diego executor issues, are compared; other key types are accepted unchecked. + private boolean matches(PrivateKey privateKey, List certificates) { + if (certificates.isEmpty()) { + return false; + } - @Override - public void run() { - if (FileWatchingX509ExtendedKeyManager.this.keyManager.getAndSet(getKeyManager(getKeyStore())) == null) { - FileWatchingX509ExtendedKeyManager.this.logger.info(String.format("Initialized KeyManager for %s and %s", FileWatchingX509ExtendedKeyManager.this.privateKey, - FileWatchingX509ExtendedKeyManager.this.certificates)); - } else { - FileWatchingX509ExtendedKeyManager.this.logger.info(String.format("Updated KeyManager for %s and %s", FileWatchingX509ExtendedKeyManager.this.privateKey, - FileWatchingX509ExtendedKeyManager.this.certificates)); + PublicKey publicKey = certificates.get(0).getPublicKey(); + if (!(privateKey instanceof RSAKey) || !(publicKey instanceof RSAKey)) { + this.logger.info(String.format("Private key %s is not an RSA key, not checked against certificate %s. This is unexpected: " + + "check the key generator, the security provider probably needs an update", this.privateKey, this.certificates)); + return true; + } + + return ((RSAKey) privateKey).getModulus().equals(((RSAKey) publicKey).getModulus()); + } + + void update() { + // The whole update is locked: with the files read outside the lock, an update that read the files earlier + // could finish later and replace a newer pair with the older one. Also, the shared KeyManagerFactory is not + // thread-safe. No contention cost: updates come from the constructor and the single watcher thread, and + // handshakes never take the lock. + synchronized (this.updateLock) { + PrivateKey privateKey; + List certificates; + try { + privateKey = PrivateKeyFactory.generate(this.privateKey); + certificates = X509CertificateFactory.generate(this.certificates); + } catch (CertificateException | IOException e) { + throw new UndeclaredThrowableException(e); + } + + // No retry: the Diego executor never writes a mismatched pair, and the next certificate change + // triggers a new update. Until then the current pair, still valid, beats one that cannot authenticate. + if (this.keyManager.get() != null && !matches(privateKey, certificates)) { + this.logger.warning(String.format("Private key %s does not match certificate %s, keeping current KeyManager (certificate chain expires at %s)", + this.privateKey, this.certificates, this.notAfter)); + return; + } + + X509ExtendedKeyManager keyManager = getKeyManager(getKeyStore(privateKey, certificates)); + Instant notAfter = notAfter(certificates); + boolean initialized = this.keyManager.getAndSet(keyManager) == null; + this.notAfter = notAfter; + + X509Certificate leaf = certificates.get(0); + this.logger.info(String.format("%s KeyManager for %s and %s: serial %s, subject %s, certificate chain expires at %s", + initialized ? "Initialized" : "Updated", this.privateKey, this.certificates, + leaf.getSerialNumber().toString(16), leaf.getSubjectX500Principal().getName(), notAfter)); + + if (notAfter.isBefore(Instant.now())) { + this.logger.warning(String.format("Certificate chain %s expired at %s, TLS handshakes using it will fail", this.certificates, notAfter)); } } + } + // Instant prints as ISO-8601 in UTC, independent of the default time zone. + static Instant notAfter(List certificates) { + Instant notAfter = Instant.MAX; + for (X509Certificate certificate : certificates) { + Instant certificateNotAfter = certificate.getNotAfter().toInstant(); + if (certificateNotAfter.isBefore(notAfter)) { + notAfter = certificateNotAfter; + } + } + return notAfter; } } diff --git a/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java b/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java index 44c1e68..fd4006f 100644 --- a/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java +++ b/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java @@ -25,10 +25,17 @@ import java.nio.file.Paths; import java.nio.file.StandardCopyOption; import java.security.NoSuchAlgorithmException; +import java.security.cert.CertificateException; +import java.security.cert.X509Certificate; +import java.time.Instant; +import java.util.Collections; +import java.util.List; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.fail; +// The client-certificates-*.pem fixtures are Diego-issued and expired in 2017 (CA in 2018). That is fine: loading and +// matching never check validity dates, only a TLS handshake would. The expired-chain warning is logged but not asserted. public final class FileWatchingX509ExtendedKeyManagerTest extends AbstractLoggingTest { @Test @@ -57,8 +64,9 @@ public void watchesWatchedFile() throws IOException, InterruptedException, NoSuc String alias = keyManager.getClientAliases("RSA", null)[0]; Thread.sleep(5_000); - Files.copy(Paths.get("src/test/resources/client-certificates-2.pem"), watchedCertificates, StandardCopyOption.REPLACE_EXISTING); + // Same order as Diego: the private key is replaced before the certificate. Files.copy(Paths.get("src/test/resources/client-private-key-2.pem"), watchedPrivateKey, StandardCopyOption.REPLACE_EXISTING); + Files.copy(Paths.get("src/test/resources/client-certificates-2.pem"), watchedCertificates, StandardCopyOption.REPLACE_EXISTING); long timeout = System.currentTimeMillis() + 300_000; for (; ; ) { @@ -74,6 +82,80 @@ public void watchesWatchedFile() throws IOException, InterruptedException, NoSuc } } + @Test + public void initializedWhenPrivateKeyDoesNotMatchCertificate() throws IOException, NoSuchAlgorithmException { + Path watchedCertificates = getWatchedCertificatesFile(); + Files.copy(Paths.get("src/test/resources/client-certificates-2.pem"), watchedCertificates); + + Path watchedPrivateKey = getWatchedPrivateKeyFile(); + Files.copy(Paths.get("src/test/resources/client-private-key-1.pem"), watchedPrivateKey); + + FileWatchingX509ExtendedKeyManager keyManager = new FileWatchingX509ExtendedKeyManager(watchedCertificates, watchedPrivateKey, KeyManagerFactory.getInstance("SunX509")); + + assertThat(keyManager.getClientAliases("RSA", null)).hasSize(1); + } + + @Test + public void updatesWhenPrivateKeyMatchesCertificate() throws IOException, CertificateException, NoSuchAlgorithmException { + Path watchedCertificates = getWatchedCertificatesFile(); + Files.copy(Paths.get("src/test/resources/client-certificates-1.pem"), watchedCertificates); + + Path watchedPrivateKey = getWatchedPrivateKeyFile(); + Files.copy(Paths.get("src/test/resources/client-private-key-1.pem"), watchedPrivateKey); + + FileWatchingX509ExtendedKeyManager keyManager = new FileWatchingX509ExtendedKeyManager(watchedCertificates, watchedPrivateKey, KeyManagerFactory.getInstance("SunX509")); + + Files.copy(Paths.get("src/test/resources/client-private-key-2.pem"), watchedPrivateKey, StandardCopyOption.REPLACE_EXISTING); + Files.copy(Paths.get("src/test/resources/client-certificates-2.pem"), watchedCertificates, StandardCopyOption.REPLACE_EXISTING); + keyManager.update(); + + assertThat(getLeafCertificate(keyManager)).isEqualTo(X509CertificateFactory.generate(Paths.get("src/test/resources/client-certificates-2.pem")).get(0)); + } + + @Test + public void keepsCurrentWhenPrivateKeyDoesNotMatchCertificate() throws IOException, CertificateException, NoSuchAlgorithmException { + Path watchedCertificates = getWatchedCertificatesFile(); + Files.copy(Paths.get("src/test/resources/client-certificates-1.pem"), watchedCertificates); + + Path watchedPrivateKey = getWatchedPrivateKeyFile(); + Files.copy(Paths.get("src/test/resources/client-private-key-1.pem"), watchedPrivateKey); + + FileWatchingX509ExtendedKeyManager keyManager = new FileWatchingX509ExtendedKeyManager(watchedCertificates, watchedPrivateKey, KeyManagerFactory.getInstance("SunX509")); + + Files.copy(Paths.get("src/test/resources/client-certificates-2.pem"), watchedCertificates, StandardCopyOption.REPLACE_EXISTING); + keyManager.update(); + + assertThat(getLeafCertificate(keyManager)).isEqualTo(X509CertificateFactory.generate(Paths.get("src/test/resources/client-certificates-1.pem")).get(0)); + } + + @Test + public void keepsCurrentWhenCertificatesAreEmpty() throws IOException, CertificateException, NoSuchAlgorithmException { + Path watchedCertificates = getWatchedCertificatesFile(); + Files.copy(Paths.get("src/test/resources/client-certificates-1.pem"), watchedCertificates); + + Path watchedPrivateKey = getWatchedPrivateKeyFile(); + Files.copy(Paths.get("src/test/resources/client-private-key-1.pem"), watchedPrivateKey); + + FileWatchingX509ExtendedKeyManager keyManager = new FileWatchingX509ExtendedKeyManager(watchedCertificates, watchedPrivateKey, KeyManagerFactory.getInstance("SunX509")); + + Files.write(watchedCertificates, new byte[0]); + keyManager.update(); + + assertThat(getLeafCertificate(keyManager)).isEqualTo(X509CertificateFactory.generate(Paths.get("src/test/resources/client-certificates-1.pem")).get(0)); + } + + @Test + public void notAfterIsEarliestInChain() throws IOException, CertificateException { + List certificates = X509CertificateFactory.generate(Paths.get("src/test/resources/client-certificates-1.pem")); + Collections.reverse(certificates); + + assertThat(FileWatchingX509ExtendedKeyManager.notAfter(certificates)).isEqualTo(Instant.parse("2017-05-19T17:58:20Z")); + } + + private X509Certificate getLeafCertificate(FileWatchingX509ExtendedKeyManager keyManager) { + return keyManager.getCertificateChain(keyManager.getClientAliases("RSA", null)[0])[0]; + } + private Path getWatchedCertificatesFile() throws IOException { Path workDirectory = Files.createTempDirectory("file-watching-key-manager-test-"); return workDirectory.resolve("certificates.pem"); From 5ae37e310114bf1916bb90a9aeab18159acc8673 Mon Sep 17 00:00:00 2001 From: Peter Paul Bakker Date: Thu, 15 Oct 2026 10:07:38 +0000 Subject: [PATCH 02/10] refactor: log lazily with suppliers --- .../security/FileWatchingX509ExtendedKeyManager.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManager.java b/src/main/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManager.java index 80e17ff..aa443a6 100644 --- a/src/main/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManager.java +++ b/src/main/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManager.java @@ -146,7 +146,7 @@ private boolean matches(PrivateKey privateKey, List certificate PublicKey publicKey = certificates.get(0).getPublicKey(); if (!(privateKey instanceof RSAKey) || !(publicKey instanceof RSAKey)) { - this.logger.info(String.format("Private key %s is not an RSA key, not checked against certificate %s. This is unexpected: " + + this.logger.info(() -> String.format("Private key %s is not an RSA key, not checked against certificate %s. This is unexpected: " + "check the key generator, the security provider probably needs an update", this.privateKey, this.certificates)); return true; } @@ -172,8 +172,9 @@ void update() { // No retry: the Diego executor never writes a mismatched pair, and the next certificate change // triggers a new update. Until then the current pair, still valid, beats one that cannot authenticate. if (this.keyManager.get() != null && !matches(privateKey, certificates)) { - this.logger.warning(String.format("Private key %s does not match certificate %s, keeping current KeyManager (certificate chain expires at %s)", - this.privateKey, this.certificates, this.notAfter)); + Instant currentNotAfter = this.notAfter; + this.logger.warning(() -> String.format("Private key %s does not match certificate %s, keeping current KeyManager (certificate chain expires at %s)", + this.privateKey, this.certificates, currentNotAfter)); return; } @@ -183,12 +184,12 @@ void update() { this.notAfter = notAfter; X509Certificate leaf = certificates.get(0); - this.logger.info(String.format("%s KeyManager for %s and %s: serial %s, subject %s, certificate chain expires at %s", + this.logger.info(() -> String.format("%s KeyManager for %s and %s: serial %s, subject %s, certificate chain expires at %s", initialized ? "Initialized" : "Updated", this.privateKey, this.certificates, leaf.getSerialNumber().toString(16), leaf.getSubjectX500Principal().getName(), notAfter)); if (notAfter.isBefore(Instant.now())) { - this.logger.warning(String.format("Certificate chain %s expired at %s, TLS handshakes using it will fail", this.certificates, notAfter)); + this.logger.warning(() -> String.format("Certificate chain %s expired at %s, TLS handshakes using it will fail", this.certificates, notAfter)); } } } From a277aa5e9ebba98c66c11cc0e76e46d120da7254 Mon Sep 17 00:00:00 2001 From: Peter Paul Bakker Date: Thu, 15 Oct 2026 12:17:54 +0000 Subject: [PATCH 03/10] test: retry leaf certificate lookup during watcher reloads --- .../security/FileWatchingX509ExtendedKeyManagerTest.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java b/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java index fd4006f..bebadda 100644 --- a/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java +++ b/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java @@ -152,8 +152,15 @@ public void notAfterIsEarliestInChain() throws IOException, CertificateException assertThat(FileWatchingX509ExtendedKeyManager.notAfter(certificates)).isEqualTo(Instant.parse("2017-05-19T17:58:20Z")); } + // The file watcher can install a new KeyManager between the two calls; aliases change on every load, so retry. private X509Certificate getLeafCertificate(FileWatchingX509ExtendedKeyManager keyManager) { - return keyManager.getCertificateChain(keyManager.getClientAliases("RSA", null)[0])[0]; + for (int i = 0; i < 100; i++) { + X509Certificate[] chain = keyManager.getCertificateChain(keyManager.getClientAliases("RSA", null)[0]); + if (chain != null) { + return chain[0]; + } + } + throw new IllegalStateException("No stable certificate chain"); } private Path getWatchedCertificatesFile() throws IOException { From dd59b7552d83df429dd29e6218bde131dbe13d83 Mon Sep 17 00:00:00 2001 From: Peter Paul Bakker Date: Wed, 21 Oct 2026 07:16:58 +0000 Subject: [PATCH 04/10] fix: warn on mismatch at first load and link Diego write order - Warn when the first loaded pair mismatches; there is no current pair to keep, so it is installed as before - Hint at the likely cause in the reload mismatch warning - Link the Diego executor source for the key-before-certificate order - Replace files atomically in the watcher test, fail within 30s --- README.md | 10 +++-- .../FileWatchingX509ExtendedKeyManager.java | 29 +++++++++---- ...ileWatchingX509ExtendedKeyManagerTest.java | 41 +++++++++++++++---- 3 files changed, 61 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index 1e5f617..40da970 100644 --- a/README.md +++ b/README.md @@ -8,15 +8,17 @@ The `java-buildpack-security-provider` is a utility that watches for changes to container identity and trust stores and dynamically updates the KeyManager and TrustManager of an application. ## Certificate rotation -The KeyManager is reloaded when the instance identity certificate (`CF_INSTANCE_CERT`) changes. The Diego executor writes the new private key (`CF_INSTANCE_KEY`) before the new certificate, so only the certificate file is watched. Each load logs the certificate serial, subject and when the certificate chain expires (earliest expiry in the chain, in UTC). +The KeyManager is reloaded when the instance identity certificate (`CF_INSTANCE_CERT`) changes. The Diego executor [writes the new private key (`CF_INSTANCE_KEY`) before the new certificate](https://github.com/cloudfoundry/executor/blob/208a08c51850167d93e1040cfd8538fc1925c882/depot/containerstore/instance_identity_handler.go#L94-L102), so only the certificate file is watched. Since 1.22.0, each load logs the certificate serial, subject and when the certificate chain expires (earliest expiry in the chain, in UTC). -Before replacing the KeyManager, the private key is checked against the certificate. Only RSA keys, the type the Diego executor issues, are checked; other key types are accepted with an informational log message. On a mismatch or an empty certificate file the current KeyManager is kept and a warning is logged: +Since 1.22.0, the private key is checked against the certificate on every load. Only RSA keys, the type the Diego executor issues, are checked; other key types are accepted with an informational log message. On a reload with a mismatch or an empty certificate file, the current KeyManager is kept and a warning is logged: ``` -Private key ... does not match certificate ..., keeping current KeyManager (certificate chain expires at 2026-10-15T09:30:00Z) +Private key ... does not match certificate ..., keeping current KeyManager (certificate chain expires at 2026-10-15T09:30:00Z). The private key may not have been replaced before the certificate ``` -This is not retried. With Diego defaults (24 hour validity, rotation 30 minutes before expiry) the kept certificate expires about 30 minutes later, after which TLS handshakes using it fail. A loaded certificate chain that has already expired is installed as before, with a warning: +On the first load there is no current KeyManager to keep: a mismatched pair is installed with a warning, and an empty certificate file fails to load, as before. + +A kept KeyManager is not replaced until the next certificate change. With Diego defaults (24 hour validity, rotation 30 minutes before expiry) the kept certificate expires about 30 minutes later, after which TLS handshakes using it fail. A loaded certificate chain that has already expired is installed as before, with a warning: ``` Certificate chain ... expired at 2026-10-15T09:30:00Z, TLS handshakes using it will fail diff --git a/src/main/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManager.java b/src/main/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManager.java index aa443a6..3db4c88 100644 --- a/src/main/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManager.java +++ b/src/main/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManager.java @@ -66,8 +66,11 @@ final class FileWatchingX509ExtendedKeyManager extends X509ExtendedKeyManager { this.keyManagerFactory = keyManagerFactory; // Only the certificate file is watched: the Diego executor renames the new private key into place - // before the new certificate, so a certificate change means both files are new. - // Watching the key file as well caused #8. + // before the new certificate, so a certificate change means both files are new. See + // https://github.com/cloudfoundry/executor/blob/208a08c51850167d93e1040cfd8538fc1925c882/depot/containerstore/instance_identity_handler.go#L94-L102 + // If that order ever changes, the update reads the old key with the new certificate and the mismatch check + // below keeps the current pair. Watching the key file as well caused + // https://github.com/cloudfoundry/java-buildpack-security-provider/issues/8 new FileWatcher(this.certificates, this::update).watch(); update(); } @@ -171,11 +174,20 @@ void update() { // No retry: the Diego executor never writes a mismatched pair, and the next certificate change // triggers a new update. Until then the current pair, still valid, beats one that cannot authenticate. - if (this.keyManager.get() != null && !matches(privateKey, certificates)) { - Instant currentNotAfter = this.notAfter; - this.logger.warning(() -> String.format("Private key %s does not match certificate %s, keeping current KeyManager (certificate chain expires at %s)", - this.privateKey, this.certificates, currentNotAfter)); - return; + // A single delayed retry would also cover a certificate renamed before its key, but Diego renames the + // key first; add one only if real-world cases show the need. + // On the first load there is no current pair, so the mismatched one is installed, with a warning. + if (!matches(privateKey, certificates)) { + if (this.keyManager.get() != null) { + Instant currentNotAfter = this.notAfter; + this.logger.warning(() -> String.format("Private key %s does not match certificate %s, keeping current KeyManager (certificate chain expires at %s). " + + "The private key may not have been replaced before the certificate", + this.privateKey, this.certificates, currentNotAfter)); + return; + } + + this.logger.warning(() -> String.format("Private key %s does not match certificate %s, no current KeyManager to keep, " + + "TLS handshakes using it will fail", this.privateKey, this.certificates)); } X509ExtendedKeyManager keyManager = getKeyManager(getKeyStore(privateKey, certificates)); @@ -194,7 +206,8 @@ void update() { } } - // Instant prints as ISO-8601 in UTC, independent of the default time zone. + // Instant prints as ISO-8601 in UTC, independent of the default time zone. Never called with an empty list: + // on the first load the KeyStore rejects a key without certificates, on reloads matches() does. static Instant notAfter(List certificates) { Instant notAfter = Instant.MAX; for (X509Certificate certificate : certificates) { diff --git a/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java b/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java index bebadda..3d28f59 100644 --- a/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java +++ b/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java @@ -30,8 +30,12 @@ import java.time.Instant; import java.util.Collections; import java.util.List; +import java.util.concurrent.atomic.AtomicReference; +import java.util.logging.Level; +import java.util.logging.LogRecord; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.assertj.core.api.Assertions.fail; // The client-certificates-*.pem fixtures are Diego-issued and expired in 2017 (CA in 2018). That is fine: loading and @@ -64,11 +68,12 @@ public void watchesWatchedFile() throws IOException, InterruptedException, NoSuc String alias = keyManager.getClientAliases("RSA", null)[0]; Thread.sleep(5_000); - // Same order as Diego: the private key is replaced before the certificate. - Files.copy(Paths.get("src/test/resources/client-private-key-2.pem"), watchedPrivateKey, StandardCopyOption.REPLACE_EXISTING); - Files.copy(Paths.get("src/test/resources/client-certificates-2.pem"), watchedCertificates, StandardCopyOption.REPLACE_EXISTING); + // Same as Diego: each file is replaced atomically, as a rename, the private key before the certificate, + // so no update sees a half-written file. + replace(Paths.get("src/test/resources/client-private-key-2.pem"), watchedPrivateKey); + replace(Paths.get("src/test/resources/client-certificates-2.pem"), watchedCertificates); - long timeout = System.currentTimeMillis() + 300_000; + long timeout = System.currentTimeMillis() + 30_000; for (; ; ) { if (System.currentTimeMillis() > timeout) { fail("Failed to update within timeout"); @@ -83,16 +88,32 @@ public void watchesWatchedFile() throws IOException, InterruptedException, NoSuc } @Test - public void initializedWhenPrivateKeyDoesNotMatchCertificate() throws IOException, NoSuchAlgorithmException { + public void initializedWithWarningWhenPrivateKeyDoesNotMatchCertificate() throws Exception { Path watchedCertificates = getWatchedCertificatesFile(); Files.copy(Paths.get("src/test/resources/client-certificates-2.pem"), watchedCertificates); Path watchedPrivateKey = getWatchedPrivateKeyFile(); Files.copy(Paths.get("src/test/resources/client-private-key-1.pem"), watchedPrivateKey); - FileWatchingX509ExtendedKeyManager keyManager = new FileWatchingX509ExtendedKeyManager(watchedCertificates, watchedPrivateKey, KeyManagerFactory.getInstance("SunX509")); + AtomicReference keyManager = new AtomicReference<>(); + List records = LogRecorder.record(FileWatchingX509ExtendedKeyManager.class, () -> + keyManager.set(new FileWatchingX509ExtendedKeyManager(watchedCertificates, watchedPrivateKey, KeyManagerFactory.getInstance("SunX509")))); - assertThat(keyManager.getClientAliases("RSA", null)).hasSize(1); + assertThat(keyManager.get().getClientAliases("RSA", null)).hasSize(1); + assertThat(records).filteredOn(r -> r.getLevel() == Level.WARNING && r.getMessage().startsWith("Private key " + watchedPrivateKey + " does not match certificate")) + .hasSize(1); + } + + @Test + public void initializationFailsWhenCertificatesAreEmpty() throws IOException { + Path watchedCertificates = getWatchedCertificatesFile(); + Files.write(watchedCertificates, new byte[0]); + + Path watchedPrivateKey = getWatchedPrivateKeyFile(); + Files.copy(Paths.get("src/test/resources/client-private-key-1.pem"), watchedPrivateKey); + + assertThatThrownBy(() -> new FileWatchingX509ExtendedKeyManager(watchedCertificates, watchedPrivateKey, KeyManagerFactory.getInstance("SunX509"))) + .isInstanceOf(IllegalArgumentException.class); } @Test @@ -163,6 +184,12 @@ private X509Certificate getLeafCertificate(FileWatchingX509ExtendedKeyManager ke throw new IllegalStateException("No stable certificate chain"); } + private void replace(Path source, Path target) throws IOException { + Path replacement = Files.createTempFile(target.getParent(), "replacement-", ".tmp"); + Files.copy(source, replacement, StandardCopyOption.REPLACE_EXISTING); + Files.move(replacement, target, StandardCopyOption.ATOMIC_MOVE); + } + private Path getWatchedCertificatesFile() throws IOException { Path workDirectory = Files.createTempDirectory("file-watching-key-manager-test-"); return workDirectory.resolve("certificates.pem"); From faa3593dac4a5af048969be152cff63c2e719c6d Mon Sep 17 00:00:00 2001 From: Peter Paul Bakker Date: Sat, 24 Oct 2026 18:37:45 +0000 Subject: [PATCH 05/10] fix: log key pair errors as SEVERE and skip invalid first pair - On first load, don't install a mismatched pair or an empty certificate file; delegate methods return null, so other TLS connections keep working and the next matching rotation installs it - Log mismatches and expired chains as SEVERE (ERROR via SLF4J) - README: describe the new first-load behaviour and error levels Refs #16 (review by jcvrabo) --- README.md | 12 ++-- .../FileWatchingX509ExtendedKeyManager.java | 41 ++++++++----- ...ileWatchingX509ExtendedKeyManagerTest.java | 58 +++++++++++++++---- 3 files changed, 81 insertions(+), 30 deletions(-) diff --git a/README.md b/README.md index 40da970..e6d67e5 100644 --- a/README.md +++ b/README.md @@ -10,21 +10,25 @@ The `java-buildpack-security-provider` is a utility that watches for changes to ## Certificate rotation The KeyManager is reloaded when the instance identity certificate (`CF_INSTANCE_CERT`) changes. The Diego executor [writes the new private key (`CF_INSTANCE_KEY`) before the new certificate](https://github.com/cloudfoundry/executor/blob/208a08c51850167d93e1040cfd8538fc1925c882/depot/containerstore/instance_identity_handler.go#L94-L102), so only the certificate file is watched. Since 1.22.0, each load logs the certificate serial, subject and when the certificate chain expires (earliest expiry in the chain, in UTC). -Since 1.22.0, the private key is checked against the certificate on every load. Only RSA keys, the type the Diego executor issues, are checked; other key types are accepted with an informational log message. On a reload with a mismatch or an empty certificate file, the current KeyManager is kept and a warning is logged: +Since 1.22.0, the private key is checked against the certificate on every load. Only RSA keys, the type the Diego executor issues, are checked; other key types are accepted with an informational log message. On a reload with a mismatch or an empty certificate file, the current KeyManager is kept and an error is logged (JUL level `SEVERE`, shown as `ERROR` through SLF4J or Spring Boot): ``` Private key ... does not match certificate ..., keeping current KeyManager (certificate chain expires at 2026-10-15T09:30:00Z). The private key may not have been replaced before the certificate ``` -On the first load there is no current KeyManager to keep: a mismatched pair is installed with a warning, and an empty certificate file fails to load, as before. +On the first load there is no current KeyManager to keep: a mismatched pair or an empty certificate file is not installed and an error is logged. No client certificate is presented until a certificate change brings a matching pair; other TLS connections keep working. -A kept KeyManager is not replaced until the next certificate change. With Diego defaults (24 hour validity, rotation 30 minutes before expiry) the kept certificate expires about 30 minutes later, after which TLS handshakes using it fail. A loaded certificate chain that has already expired is installed as before, with a warning: +``` +Private key ... does not match certificate ..., no KeyManager installed: no client certificate is presented until a matching pair is loaded +``` + +A kept KeyManager is not replaced until the next certificate change. With Diego defaults (24 hour validity, rotation 30 minutes before expiry) the kept certificate expires about 30 minutes later, after which TLS handshakes using it fail. A loaded certificate chain that has already expired is installed as before, with an error: ``` Certificate chain ... expired at 2026-10-15T09:30:00Z, TLS handshakes using it will fail ``` -Alert on these warnings and restart the affected instance to get a new key pair: +Alert on these errors and restart the affected instance to get a new key pair: ```shell $ cf restart-app-instance APP_NAME INDEX diff --git a/src/main/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManager.java b/src/main/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManager.java index 3db4c88..9173ef4 100644 --- a/src/main/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManager.java +++ b/src/main/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManager.java @@ -77,42 +77,50 @@ final class FileWatchingX509ExtendedKeyManager extends X509ExtendedKeyManager { @Override public String chooseClientAlias(String[] strings, Principal[] principals, Socket socket) { - return this.keyManager.get().chooseClientAlias(strings, principals, socket); + X509ExtendedKeyManager keyManager = this.keyManager.get(); + return keyManager != null ? keyManager.chooseClientAlias(strings, principals, socket) : null; } @Override public String chooseEngineClientAlias(String[] strings, Principal[] principals, SSLEngine sslEngine) { - return this.keyManager.get().chooseEngineClientAlias(strings, principals, sslEngine); + X509ExtendedKeyManager keyManager = this.keyManager.get(); + return keyManager != null ? keyManager.chooseEngineClientAlias(strings, principals, sslEngine) : null; } @Override public String chooseEngineServerAlias(String s, Principal[] principals, SSLEngine sslEngine) { - return this.keyManager.get().chooseEngineServerAlias(s, principals, sslEngine); + X509ExtendedKeyManager keyManager = this.keyManager.get(); + return keyManager != null ? keyManager.chooseEngineServerAlias(s, principals, sslEngine) : null; } @Override public String chooseServerAlias(String s, Principal[] principals, Socket socket) { - return this.keyManager.get().chooseServerAlias(s, principals, socket); + X509ExtendedKeyManager keyManager = this.keyManager.get(); + return keyManager != null ? keyManager.chooseServerAlias(s, principals, socket) : null; } @Override public X509Certificate[] getCertificateChain(String s) { - return this.keyManager.get().getCertificateChain(s); + X509ExtendedKeyManager keyManager = this.keyManager.get(); + return keyManager != null ? keyManager.getCertificateChain(s) : null; } @Override public String[] getClientAliases(String s, Principal[] principals) { - return this.keyManager.get().getClientAliases(s, principals); + X509ExtendedKeyManager keyManager = this.keyManager.get(); + return keyManager != null ? keyManager.getClientAliases(s, principals) : null; } @Override public PrivateKey getPrivateKey(String s) { - return this.keyManager.get().getPrivateKey(s); + X509ExtendedKeyManager keyManager = this.keyManager.get(); + return keyManager != null ? keyManager.getPrivateKey(s) : null; } @Override public String[] getServerAliases(String s, Principal[] principals) { - return this.keyManager.get().getServerAliases(s, principals); + X509ExtendedKeyManager keyManager = this.keyManager.get(); + return keyManager != null ? keyManager.getServerAliases(s, principals) : null; } private X509ExtendedKeyManager getKeyManager(KeyStore keyStore) { @@ -176,18 +184,19 @@ void update() { // triggers a new update. Until then the current pair, still valid, beats one that cannot authenticate. // A single delayed retry would also cover a certificate renamed before its key, but Diego renames the // key first; add one only if real-world cases show the need. - // On the first load there is no current pair, so the mismatched one is installed, with a warning. + // On the first load there is no current pair: none is installed, so no client certificate is presented + // and other TLS connections keep working, until a certificate change brings a matching pair. if (!matches(privateKey, certificates)) { if (this.keyManager.get() != null) { Instant currentNotAfter = this.notAfter; - this.logger.warning(() -> String.format("Private key %s does not match certificate %s, keeping current KeyManager (certificate chain expires at %s). " + + this.logger.severe(() -> String.format("Private key %s does not match certificate %s, keeping current KeyManager (certificate chain expires at %s). " + "The private key may not have been replaced before the certificate", this.privateKey, this.certificates, currentNotAfter)); - return; + } else { + this.logger.severe(() -> String.format("Private key %s does not match certificate %s, no KeyManager installed: " + + "no client certificate is presented until a matching pair is loaded", this.privateKey, this.certificates)); } - - this.logger.warning(() -> String.format("Private key %s does not match certificate %s, no current KeyManager to keep, " + - "TLS handshakes using it will fail", this.privateKey, this.certificates)); + return; } X509ExtendedKeyManager keyManager = getKeyManager(getKeyStore(privateKey, certificates)); @@ -201,13 +210,13 @@ void update() { leaf.getSerialNumber().toString(16), leaf.getSubjectX500Principal().getName(), notAfter)); if (notAfter.isBefore(Instant.now())) { - this.logger.warning(() -> String.format("Certificate chain %s expired at %s, TLS handshakes using it will fail", this.certificates, notAfter)); + this.logger.severe(() -> String.format("Certificate chain %s expired at %s, TLS handshakes using it will fail", this.certificates, notAfter)); } } } // Instant prints as ISO-8601 in UTC, independent of the default time zone. Never called with an empty list: - // on the first load the KeyStore rejects a key without certificates, on reloads matches() does. + // matches() rejects it first. static Instant notAfter(List certificates) { Instant notAfter = Instant.MAX; for (X509Certificate certificate : certificates) { diff --git a/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java b/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java index 3d28f59..db7fd4e 100644 --- a/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java +++ b/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java @@ -28,6 +28,7 @@ import java.security.cert.CertificateException; import java.security.cert.X509Certificate; import java.time.Instant; +import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.concurrent.atomic.AtomicReference; @@ -35,7 +36,6 @@ import java.util.logging.LogRecord; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.assertj.core.api.Assertions.fail; // The client-certificates-*.pem fixtures are Diego-issued and expired in 2017 (CA in 2018). That is fine: loading and @@ -88,7 +88,7 @@ public void watchesWatchedFile() throws IOException, InterruptedException, NoSuc } @Test - public void initializedWithWarningWhenPrivateKeyDoesNotMatchCertificate() throws Exception { + public void notInitializedWhenPrivateKeyDoesNotMatchCertificate() throws Exception { Path watchedCertificates = getWatchedCertificatesFile(); Files.copy(Paths.get("src/test/resources/client-certificates-2.pem"), watchedCertificates); @@ -99,21 +99,40 @@ public void initializedWithWarningWhenPrivateKeyDoesNotMatchCertificate() throws List records = LogRecorder.record(FileWatchingX509ExtendedKeyManager.class, () -> keyManager.set(new FileWatchingX509ExtendedKeyManager(watchedCertificates, watchedPrivateKey, KeyManagerFactory.getInstance("SunX509")))); - assertThat(keyManager.get().getClientAliases("RSA", null)).hasSize(1); - assertThat(records).filteredOn(r -> r.getLevel() == Level.WARNING && r.getMessage().startsWith("Private key " + watchedPrivateKey + " does not match certificate")) - .hasSize(1); + assertNotInitialized(keyManager.get()); + assertThat(severe(records, "Private key " + watchedPrivateKey + " does not match certificate")).hasSize(1); } @Test - public void initializationFailsWhenCertificatesAreEmpty() throws IOException { + public void notInitializedWhenCertificatesAreEmpty() throws Exception { Path watchedCertificates = getWatchedCertificatesFile(); Files.write(watchedCertificates, new byte[0]); Path watchedPrivateKey = getWatchedPrivateKeyFile(); Files.copy(Paths.get("src/test/resources/client-private-key-1.pem"), watchedPrivateKey); - assertThatThrownBy(() -> new FileWatchingX509ExtendedKeyManager(watchedCertificates, watchedPrivateKey, KeyManagerFactory.getInstance("SunX509"))) - .isInstanceOf(IllegalArgumentException.class); + AtomicReference keyManager = new AtomicReference<>(); + List records = LogRecorder.record(FileWatchingX509ExtendedKeyManager.class, () -> + keyManager.set(new FileWatchingX509ExtendedKeyManager(watchedCertificates, watchedPrivateKey, KeyManagerFactory.getInstance("SunX509")))); + + assertNotInitialized(keyManager.get()); + assertThat(severe(records, "Private key " + watchedPrivateKey + " does not match certificate")).hasSize(1); + } + + @Test + public void initializedByUpdateAfterInvalidFirstLoad() throws IOException, CertificateException, NoSuchAlgorithmException { + Path watchedCertificates = getWatchedCertificatesFile(); + Files.copy(Paths.get("src/test/resources/client-certificates-2.pem"), watchedCertificates); + + Path watchedPrivateKey = getWatchedPrivateKeyFile(); + Files.copy(Paths.get("src/test/resources/client-private-key-1.pem"), watchedPrivateKey); + + FileWatchingX509ExtendedKeyManager keyManager = new FileWatchingX509ExtendedKeyManager(watchedCertificates, watchedPrivateKey, KeyManagerFactory.getInstance("SunX509")); + + Files.copy(Paths.get("src/test/resources/client-private-key-2.pem"), watchedPrivateKey, StandardCopyOption.REPLACE_EXISTING); + keyManager.update(); + + assertThat(getLeafCertificate(keyManager)).isEqualTo(X509CertificateFactory.generate(Paths.get("src/test/resources/client-certificates-2.pem")).get(0)); } @Test @@ -134,7 +153,7 @@ public void updatesWhenPrivateKeyMatchesCertificate() throws IOException, Certif } @Test - public void keepsCurrentWhenPrivateKeyDoesNotMatchCertificate() throws IOException, CertificateException, NoSuchAlgorithmException { + public void keepsCurrentWhenPrivateKeyDoesNotMatchCertificate() throws Exception { Path watchedCertificates = getWatchedCertificatesFile(); Files.copy(Paths.get("src/test/resources/client-certificates-1.pem"), watchedCertificates); @@ -144,9 +163,11 @@ public void keepsCurrentWhenPrivateKeyDoesNotMatchCertificate() throws IOExcepti FileWatchingX509ExtendedKeyManager keyManager = new FileWatchingX509ExtendedKeyManager(watchedCertificates, watchedPrivateKey, KeyManagerFactory.getInstance("SunX509")); Files.copy(Paths.get("src/test/resources/client-certificates-2.pem"), watchedCertificates, StandardCopyOption.REPLACE_EXISTING); - keyManager.update(); + List records = LogRecorder.record(FileWatchingX509ExtendedKeyManager.class, keyManager::update); assertThat(getLeafCertificate(keyManager)).isEqualTo(X509CertificateFactory.generate(Paths.get("src/test/resources/client-certificates-1.pem")).get(0)); + // The watcher thread may also react to the change, so the error can be logged more than once + assertThat(severe(records, "Private key " + watchedPrivateKey + " does not match certificate")).isNotEmpty(); } @Test @@ -173,6 +194,23 @@ public void notAfterIsEarliestInChain() throws IOException, CertificateException assertThat(FileWatchingX509ExtendedKeyManager.notAfter(certificates)).isEqualTo(Instant.parse("2017-05-19T17:58:20Z")); } + private static void assertNotInitialized(FileWatchingX509ExtendedKeyManager keyManager) { + assertThat(keyManager.getClientAliases("RSA", null)).isNull(); + assertThat(keyManager.chooseClientAlias(new String[]{"RSA"}, null, null)).isNull(); + assertThat(keyManager.getCertificateChain("any")).isNull(); + assertThat(keyManager.getPrivateKey("any")).isNull(); + } + + private static List severe(List records, String messagePrefix) { + List severe = new ArrayList<>(); + for (LogRecord record : records) { + if (record.getLevel() == Level.SEVERE && record.getMessage().startsWith(messagePrefix)) { + severe.add(record); + } + } + return severe; + } + // The file watcher can install a new KeyManager between the two calls; aliases change on every load, so retry. private X509Certificate getLeafCertificate(FileWatchingX509ExtendedKeyManager keyManager) { for (int i = 0; i < 100; i++) { From fabe771295a84162f277f5601df3c48637e34943 Mon Sep 17 00:00:00 2001 From: Peter Paul Bakker Date: Sat, 31 Oct 2026 14:48:12 +0000 Subject: [PATCH 06/10] test: record logs in a thread-safe list The watcher thread can still publish a log record while the test reads the recorded records, which could throw ConcurrentModificationException. --- src/test/java/org/cloudfoundry/security/LogRecorder.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/cloudfoundry/security/LogRecorder.java b/src/test/java/org/cloudfoundry/security/LogRecorder.java index af13fae..14bfd70 100644 --- a/src/test/java/org/cloudfoundry/security/LogRecorder.java +++ b/src/test/java/org/cloudfoundry/security/LogRecorder.java @@ -16,8 +16,8 @@ package org.cloudfoundry.security; -import java.util.ArrayList; import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; import java.util.logging.Handler; import java.util.logging.Level; import java.util.logging.LogRecord; @@ -31,7 +31,7 @@ private LogRecorder() { // Records everything logged to the logger named after the given class while the action runs, at all levels. static List record(Class loggerClass, Action action) throws Exception { Logger logger = Logger.getLogger(loggerClass.getName()); - List records = new ArrayList<>(); + List records = new CopyOnWriteArrayList<>(); Handler handler = new Handler() { @Override From b8a2092f851ad2509808efc42d7f745fec585f8f Mon Sep 17 00:00:00 2001 From: Peter Paul Bakker Date: Sat, 7 Nov 2026 08:29:04 +0000 Subject: [PATCH 07/10] test: make key manager test helper fail clearly without aliases getClientAliases can return null or an empty array, so the helper no longer indexes it blindly. It retries a bounded number of times, because the watcher can swap the key manager between the alias and chain calls, and then fails with a descriptive message. --- .../FileWatchingX509ExtendedKeyManagerTest.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java b/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java index db7fd4e..5c06c5e 100644 --- a/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java +++ b/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java @@ -214,12 +214,15 @@ private static List severe(List records, String messagePre // The file watcher can install a new KeyManager between the two calls; aliases change on every load, so retry. private X509Certificate getLeafCertificate(FileWatchingX509ExtendedKeyManager keyManager) { for (int i = 0; i < 100; i++) { - X509Certificate[] chain = keyManager.getCertificateChain(keyManager.getClientAliases("RSA", null)[0]); - if (chain != null) { - return chain[0]; + String[] aliases = keyManager.getClientAliases("RSA", null); + if (aliases != null && aliases.length > 0) { + X509Certificate[] chain = keyManager.getCertificateChain(aliases[0]); + if (chain != null) { + return chain[0]; + } } } - throw new IllegalStateException("No stable certificate chain"); + throw new IllegalStateException("No client alias with a certificate chain after 100 attempts"); } private void replace(Path source, Path target) throws IOException { From 7819b49a3bfc49d4218722fb9770c7952d67ee25 Mon Sep 17 00:00:00 2001 From: Peter Paul Bakker Date: Sat, 7 Nov 2026 09:34:53 +0000 Subject: [PATCH 08/10] fix: log when a KeyManager alias is unknown after a reload --- .../FileWatchingX509ExtendedKeyManager.java | 17 +++++++++-- ...ileWatchingX509ExtendedKeyManagerTest.java | 28 +++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManager.java b/src/main/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManager.java index 9173ef4..b27305b 100644 --- a/src/main/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManager.java +++ b/src/main/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManager.java @@ -102,7 +102,9 @@ public String chooseServerAlias(String s, Principal[] principals, Socket socket) @Override public X509Certificate[] getCertificateChain(String s) { X509ExtendedKeyManager keyManager = this.keyManager.get(); - return keyManager != null ? keyManager.getCertificateChain(s) : null; + X509Certificate[] chain = keyManager != null ? keyManager.getCertificateChain(s) : null; + logUnknownAlias(keyManager, s, chain, "certificate chain"); + return chain; } @Override @@ -114,7 +116,18 @@ public String[] getClientAliases(String s, Principal[] principals) { @Override public PrivateKey getPrivateKey(String s) { X509ExtendedKeyManager keyManager = this.keyManager.get(); - return keyManager != null ? keyManager.getPrivateKey(s) : null; + PrivateKey privateKey = keyManager != null ? keyManager.getPrivateKey(s) : null; + logUnknownAlias(keyManager, s, privateKey, "private key"); + return privateKey; + } + + // An alias is chosen and looked up in separate calls, and aliases change on every load. A reload in between would make the + // lookup fail. This is not expected to happen in practice; if it is ever logged, the lookup needs to be made reload-safe. + // The delegating key manager asks the system key manager first, so only aliases unknown to it reach this check. + private void logUnknownAlias(X509ExtendedKeyManager keyManager, String alias, Object found, String what) { + if (keyManager != null && alias != null && found == null) { + this.logger.severe(() -> String.format("Should not happen: no %s for alias %s, the KeyManager was probably reloaded between choosing and looking up the alias", what, alias)); + } } @Override diff --git a/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java b/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java index 5c06c5e..0489cfa 100644 --- a/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java +++ b/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java @@ -152,6 +152,34 @@ public void updatesWhenPrivateKeyMatchesCertificate() throws IOException, Certif assertThat(getLeafCertificate(keyManager)).isEqualTo(X509CertificateFactory.generate(Paths.get("src/test/resources/client-certificates-2.pem")).get(0)); } + // A reload between choosing an alias and looking it up is not expected to cause trouble, but would make the lookup return null. + @Test + public void logsWhenAliasFromBeforeReloadIsNoLongerKnown() throws Exception { + Path watchedCertificates = getWatchedCertificatesFile(); + Files.copy(Paths.get("src/test/resources/client-certificates-1.pem"), watchedCertificates); + + Path watchedPrivateKey = getWatchedPrivateKeyFile(); + Files.copy(Paths.get("src/test/resources/client-private-key-1.pem"), watchedPrivateKey); + + FileWatchingX509ExtendedKeyManager keyManager = new FileWatchingX509ExtendedKeyManager(watchedCertificates, watchedPrivateKey, KeyManagerFactory.getInstance("SunX509")); + String staleAlias = keyManager.getClientAliases("RSA", null)[0]; + + replace(Paths.get("src/test/resources/client-private-key-2.pem"), watchedPrivateKey); + replace(Paths.get("src/test/resources/client-certificates-2.pem"), watchedCertificates); + keyManager.update(); + + String currentAlias = keyManager.getClientAliases("RSA", null)[0]; + List records = LogRecorder.record(FileWatchingX509ExtendedKeyManager.class, () -> { + assertThat(keyManager.getCertificateChain(currentAlias)).isNotNull(); + assertThat(keyManager.getPrivateKey(currentAlias)).isNotNull(); + assertThat(keyManager.getCertificateChain(staleAlias)).isNull(); + assertThat(keyManager.getPrivateKey(staleAlias)).isNull(); + }); + + assertThat(severe(records, "Should not happen:")).extracting(LogRecord::getMessage).hasSize(2) + .allMatch(m -> m.contains(staleAlias)); + } + @Test public void keepsCurrentWhenPrivateKeyDoesNotMatchCertificate() throws Exception { Path watchedCertificates = getWatchedCertificatesFile(); From 6f0f6041767d3c74ac3010bbe0ddcd473f3dc740 Mon Sep 17 00:00:00 2001 From: Peter Paul Bakker Date: Sat, 7 Nov 2026 09:36:42 +0000 Subject: [PATCH 09/10] test: retry key manager test helper 3 times instead of 100 --- .../security/FileWatchingX509ExtendedKeyManagerTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java b/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java index 0489cfa..c143b26 100644 --- a/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java +++ b/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java @@ -241,7 +241,7 @@ private static List severe(List records, String messagePre // The file watcher can install a new KeyManager between the two calls; aliases change on every load, so retry. private X509Certificate getLeafCertificate(FileWatchingX509ExtendedKeyManager keyManager) { - for (int i = 0; i < 100; i++) { + for (int i = 0; i < 3; i++) { String[] aliases = keyManager.getClientAliases("RSA", null); if (aliases != null && aliases.length > 0) { X509Certificate[] chain = keyManager.getCertificateChain(aliases[0]); @@ -250,7 +250,7 @@ private X509Certificate getLeafCertificate(FileWatchingX509ExtendedKeyManager ke } } } - throw new IllegalStateException("No client alias with a certificate chain after 100 attempts"); + throw new IllegalStateException("No client alias with a certificate chain after 3 attempts"); } private void replace(Path source, Path target) throws IOException { From 5ad0e2c882598d978898c2e123cc2d79405b96de Mon Sep 17 00:00:00 2001 From: Peter Paul Bakker Date: Sat, 7 Nov 2026 10:01:31 +0000 Subject: [PATCH 10/10] test: avoid file watcher race in stale alias test --- .../security/FileWatchingX509ExtendedKeyManagerTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java b/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java index c143b26..b6385e4 100644 --- a/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java +++ b/src/test/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManagerTest.java @@ -164,8 +164,7 @@ public void logsWhenAliasFromBeforeReloadIsNoLongerKnown() throws Exception { FileWatchingX509ExtendedKeyManager keyManager = new FileWatchingX509ExtendedKeyManager(watchedCertificates, watchedPrivateKey, KeyManagerFactory.getInstance("SunX509")); String staleAlias = keyManager.getClientAliases("RSA", null)[0]; - replace(Paths.get("src/test/resources/client-private-key-2.pem"), watchedPrivateKey); - replace(Paths.get("src/test/resources/client-certificates-2.pem"), watchedCertificates); + // Every load generates new aliases; reloading unchanged files avoids a racing reload by the file watcher. keyManager.update(); String currentAlias = keyManager.getClientAliases("RSA", null)[0];