Write the KeyStore atomically and close its streams

[kevinherron]

A bug report pointed out that KeyStoreCertificateStore hands streams to KeyStore.load and KeyStore.store without ever closing them. That much is true — neither method takes ownership of the stream it's given. On Java 17 the descriptor is released by a cleaner once the stream becomes unreachable, so the practical cost is a non-deterministic delay rather than an unbounded leak, but on Windows the file stays locked for that interval, which is enough to block replacing or deleting the keystore.

The more serious problem is on the same lines, and the report didn't mention it. new FileOutputStream(file) truncates on open, so a store() that threw part way through — bad password supplier, unwritable disk, unsupported key — left the keystore truncated and the server's private keys gone. initialize() only writes once, but set() and remove() write on every certificate update, which made this a routine operation capable of destroying a working keystore. Those two also mutate the in-memory KeyStore before persisting it, so a failed write left memory holding the new state and disk holding the old one for the rest of the process lifetime: get() would keep reporting a certificate that vanished on restart.

Writes now go through a single helper that writes to a temporary file in the same directory and moves it into place. It preserves the original file's POSIX permissions, since temporary files are created owner-only and a replace would otherwise silently tighten whatever the operator had configured, and it resolves symlinks so a symlinked keystore is written through rather than replaced by a regular file. ATOMIC_MOVE is deliberate rather than incidental: a non-atomic Files.move with REPLACE_EXISTING unlinks the target before renaming, which is a worse failure window than a loud error. set() and remove() roll their in-memory mutation back when the write fails. The entries cache needs no equivalent treatment, since get() falls back to the KeyStore and repopulates it.

Two smaller things came along for the ride. set() picks up the null-alias guard that contains(), get() and remove() already had, and getAlias is now declared @Nullable to match how its callers already treat it. Separately, the KeyStoreLoader copies under server-examples and integration-tests had the same unclosed-stream bug, and now match the client-examples copy that already had the fix — no atomic-write treatment there, since they only write when the file doesn't yet exist.

Testing

Everything CertificateStoreTest asserts runs against the live in-memory KeyStore, so it would pass unchanged even if storeKeyStore() wrote nothing at all. What's added opens a second store over the same file, which is the only way to tell that anything reached disk. One case covers the ordinary persistence path; another injects a failing password supplier and checks what the new code actually claims — that the entry is rolled back in memory, that no temporary file is left behind, and that what was already on disk still reads back afterwards.

Not in this PR

watchForChanges is separately broken; #1842 stacks on this branch and fixes it. The same truncate-on-open pattern also exists in FileBasedTrustListManager and FileBasedCertificateQuarantine, which is still open — lower stakes there, since a truncated .der is logged and skipped rather than costing a private key.

🤖 Generated with Claude Code