fix: keep current key pair when key and certificate mismatch

README.md

@@ -7,6 +7,33 @@
 
 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](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 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 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.
+
+```
+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 errors 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:
 

pom.xml

@@ -53,17 +53,25 @@
 
     <properties>
         <!-- The development version, the only place it is set. Release and snapshot builds override it with -Drevision. -->
-        <revision>1.21.0-SNAPSHOT</revision>
+        <revision>1.22.0-SNAPSHOT</revision>
         <bouncycastle.version>1.86</bouncycastle.version>
         <junit.version>4.13.2</junit.version>
         <mockito.version>4.11.0</mockito.version>
         <slf4j.version>1.7.36</slf4j.version>
         <logback.version>1.2.13</logback.version>
         <assertj.version>3.27.7</assertj.version>
+        <errorprone.version>2.50.0</errorprone.version>
         <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
     </properties>
 
     <dependencies>
+        <!-- Compile-time only: @GuardedBy has class retention, so it is neither needed at runtime nor shaded. -->
+        <dependency>
+            <groupId>com.google.errorprone</groupId>
+            <artifactId>error_prone_annotations</artifactId>
+            <version>${errorprone.version}</version>
+            <scope>provided</scope>
+        </dependency>
         <dependency>
             <groupId>org.assertj</groupId>
             <artifactId>assertj-core</artifactId>

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,62 +50,90 @@ final class FileWatchingX509ExtendedKeyManager extends X509ExtendedKeyManager {
 
     private final AtomicReference<X509ExtendedKeyManager> 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) {
         this.certificates = certificates;
         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. 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();
     }
 
     @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();
+        X509Certificate[] chain = keyManager != null ? keyManager.getCertificateChain(s) : null;
+        logUnknownAlias(keyManager, s, chain, "certificate chain");
+        return chain;
     }
 
     @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();
+        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
     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) {
@@ -119,33 +152,93 @@ private X509ExtendedKeyManager getKeyManager(KeyStore keyStore) {
         }
     }
 
-    private KeyStore getKeyStore() {
+    private KeyStore getKeyStore(PrivateKey privateKey, List<X509Certificate> certificates) {
         try {
             KeyStore keyStore = KeyStoreEntryCollector.identity();
-            PrivateKey privateKey = PrivateKeyFactory.generate(this.privateKey);
-            List<X509Certificate> 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<X509Certificate> certificates) {
+        if (certificates.isEmpty()) {
+            return false;
+        }
+
+        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;
+        }
 
-        @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));
+        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<X509Certificate> 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.
+            // 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: 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.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));
+                } 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));
+                }
+                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.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:
+    // matches() rejects it first.
+    static Instant notAfter(List<X509Certificate> certificates) {
+        Instant notAfter = Instant.MAX;
+        for (X509Certificate certificate : certificates) {
+            Instant certificateNotAfter = certificate.getNotAfter().toInstant();
+            if (certificateNotAfter.isBefore(notAfter)) {
+                notAfter = certificateNotAfter;
+            }
+        }
+        return notAfter;
     }
 
 }