[stokpop]

Ties up the loose ends of #8 after #12. Before replacing the KeyManager, the private key is checked against the leaf certificate (RSA, the type the Diego executor issues). On a reload with a mismatch or an empty certificate file the current pair is kept; on the first load an invalid pair is not installed, so no client certificate is presented but other TLS connections keep working. Both cases, and an expired chain, are logged as errors (JUL SEVERE). Reloads are serialized, each load logs the certificate serial, subject and chain expiry, and the commented-out key watcher is replaced by a note on why only the certificate is watched. Supersedes #9 and #10.

Note: a mismatch is not retried and does not fail fast; see the new "Certificate rotation" section in the README for alerting and recovery.

Draft: planned for 1.22.0, after a 1.21.1 release with the recent dependency updates, as these changes need more review and testing.

Note: if a reload happens between choosing a client alias and looking up its chain or key, the lookup returns null. This is a pre-existing, very small window. It is logged at SEVERE with the prefix Should not happen:, so we can see whether it ever occurs. If it does, a possible fix is to tag aliases with a generation number and keep the previous KeyManager for a short grace period. That is a heavier change, so it is left out of this PR.

[Copilot]

🔵 Needs a closer look

Security-sensitive reload ordering needs deterministic concurrency coverage and final human validation.

1 open finding

• Add deterministic test for overlapping reload serialization · New

What changed in this PR

Protects certificate rotation by retaining the current KeyManager when a replacement RSA key and certificate do not match.

Changes:

• Serializes reloads and adds certificate identity and expiry logging.
• Adds reload tests and documents warning recovery.
• Advances the development version and adds locking annotations.

File	Description
src/​test/​java/​org/​cloudfoundry/​security/​FileWatchingX509ExtendedKeyManagerTest.java	Tests matching, mismatched, and empty certificate reloads and chain expiry.
src/​main/​java/​org/​cloudfoundry/​security/​FileWatchingX509ExtendedKeyManager.java	Serializes reloads, checks replacement pairs, and logs certificate details.
README.md	Documents rotation behavior, warnings, and recovery.
pom.xml	Sets 1.22.0-SNAPSHOT and adds compile-time locking annotations.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jcvrabo]

I would argue that mismatched errors should be logged as errors instead of warnings (keeping a soon-to-be-expired certificate will keep valid for about 1 hour and assuming that the issue will remain, the app will never really have a valid certificate again).

I would also wonder if in case the app starts without a valid key/certificate pair there is any point in actually loading a (useless) certificate/key pair. Since that is an edge case which would come from a deeper problem with the diego cell, I would just log the error (as error, not a warning and not throwing exceptions as the app might not even need the certificate to operate) and leave the keymanager uninitialized.

[stokpop]

@jcvrabo thanks, both adopted in faa3593:

• Mismatches (and expired chains) are now logged at SEVERE, shown as ERROR through SLF4J and Spring Boot.
• On the first load an invalid pair, or an empty certificate file, is not installed. An error is logged and nothing is thrown. No client certificate is presented, other TLS connections keep working, and the next certificate change with a matching pair installs it.

Compared with 1.21.1 this is never worse:

• A mismatch on first load used to be installed silently.
• An empty file used to throw on every SSLContext.
• A mismatch on reload used to replace the good pair.

An opt-in health signal (for example a JMX MBean that an app health check could read, triggering a restart) could be a follow-up issue.

[Copilot]

🟡 Changes recommended

An in-flight watcher log publication can race with iteration of LogRecorder’s backing ArrayList, causing intermittent ConcurrentModificationException failures in the test.

1 open issue

• Use a thread-safe collection for concurrent log recording · New

1 resolved since last review

• Add deterministic test for overlapping reload serialization

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

Human assessment is needed because the tests do not verify the expired-chain diagnostic or its severity, despite its documented role in operational alerting.

No open issues 🎉

1 resolved since last review

• Use a thread-safe collection for concurrent log recording

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

Replacing the watched certificate can trigger an asynchronous reload after currentAlias is captured, causing intermittent assertion failures because the explicit update() does not wait for the watcher callback.

1 open issue

• Avoid replacing watched certificate during reload test · New

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

Human assessment is needed because the unchecked fallback can accept mismatched RSA/non-RSA key and certificate pairs, bypassing safeguards that preserve the current KeyManager.

No open issues 🎉

1 resolved since last review

• Avoid replacing watched certificate during reload test

_{Review effort: Balanced}

[stokpop]

On the Copilot note about the unchecked non-RSA fallback in matches(): left as is on purpose. The Diego executor only generates RSA keys (credmanager.go uses *rsa.PrivateKey), and key and certificate are created together, so a mixed RSA/non-RSA pair would need an algorithm switch in Diego to coincide with the rename window during rotation. If Diego ever moves to EC or post-quantum keys, both change together and the existing INFO log ("not an RSA key ... the security provider probably needs an update") signals that a check for the new algorithm should be added.