From a0170a15154ee06ea76005b459f1a1861bb962cf Mon Sep 17 00:00:00 2001 From: Kevin Herron Date: Wed, 19 Aug 2026 05:12:18 -0700 Subject: [PATCH 1/2] Write the KeyStore atomically and close its streams KeyStore.load and KeyStore.store do not close the streams they are given, so every load and store leaked a descriptor until the cleaner ran. On Windows that also kept the file locked. Opening a FileOutputStream on the KeyStore file was the worse problem: it truncates on open, so a store() that failed part way through left behind a KeyStore with no keys in it. Writes now go to a temporary file in the same directory and are moved into place, preserving the original file's POSIX permissions and following symlinks so an existing link is updated rather than replaced by a regular file. set() and remove() mutate the in-memory KeyStore before writing it out, so they now roll that mutation back when the write fails; otherwise memory and disk stay diverged for the life of the process. set() also picks up the null alias guard that contains(), get() and remove() already had, and getAlias is declared @Nullable to match how its callers treat it. The tests open a second store over the same file, which is the only way to tell that anything reached disk; the inherited assertions all pass against the in-memory KeyStore alone. --- .../security/KeyStoreCertificateStore.java | 154 ++++++++++++++++-- .../KeyStoreCertificateStoreTest.java | 94 +++++++++-- 2 files changed, 222 insertions(+), 26 deletions(-) diff --git a/opc-ua-stack/stack-core/src/main/java/org/eclipse/milo/opcua/stack/core/security/KeyStoreCertificateStore.java b/opc-ua-stack/stack-core/src/main/java/org/eclipse/milo/opcua/stack/core/security/KeyStoreCertificateStore.java index 8de3759839..4e125a2da4 100644 --- a/opc-ua-stack/stack-core/src/main/java/org/eclipse/milo/opcua/stack/core/security/KeyStoreCertificateStore.java +++ b/opc-ua-stack/stack-core/src/main/java/org/eclipse/milo/opcua/stack/core/security/KeyStoreCertificateStore.java @@ -16,13 +16,16 @@ import java.io.FileOutputStream; import java.io.IOException; import java.nio.file.FileSystems; +import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.StandardCopyOption; import java.nio.file.StandardWatchEventKinds; import java.nio.file.WatchEvent; import java.nio.file.WatchKey; import java.nio.file.WatchService; import java.security.Key; import java.security.KeyStore; +import java.security.KeyStoreException; import java.security.PrivateKey; import java.security.cert.Certificate; import java.security.cert.X509Certificate; @@ -35,6 +38,7 @@ import java.util.function.Supplier; import org.eclipse.milo.opcua.stack.core.NodeIds; import org.eclipse.milo.opcua.stack.core.types.builtin.NodeId; +import org.jspecify.annotations.Nullable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -64,10 +68,12 @@ public void initialize() throws Exception { keyStore = KeyStore.getInstance("pkcs12"); - File keyStoreFile = settings.keyStorePath.toFile(); + File keyStoreFile = settings.keyStorePath.toAbsolutePath().toFile(); if (keyStoreFile.exists()) { - keyStore.load(new FileInputStream(keyStoreFile), settings.getKeyStorePassword.get()); + try (var inputStream = new FileInputStream(keyStoreFile)) { + keyStore.load(inputStream, settings.getKeyStorePassword.get()); + } try { keyStoreLock.lock(); @@ -79,7 +85,7 @@ public void initialize() throws Exception { } else { keyStore.load(null, settings.getKeyStorePassword.get()); - keyStore.store(new FileOutputStream(keyStoreFile), settings.getKeyStorePassword.get()); + storeKeyStore(); } if (settings.watchForChanges) { @@ -172,17 +178,20 @@ public synchronized Entry remove(NodeId certificateTypeId) throws Exception { String alias = getAlias(certificateTypeId); if (alias != null) { - KeyStore.Entry entry = - keyStore.getEntry( - alias, new KeyStore.PasswordProtection(settings.getAliasPassword.apply(alias))); + char[] password = settings.getAliasPassword.apply(alias); + + KeyStore.Entry entry = keyStore.getEntry(alias, new KeyStore.PasswordProtection(password)); if (entry instanceof KeyStore.PrivateKeyEntry privateKeyEntry) { keyStore.deleteEntry(alias); entries.remove(alias); - keyStore.store( - new FileOutputStream(settings.keyStorePath.toFile()), - settings.getKeyStorePassword.get()); + try { + storeKeyStore(); + } catch (Exception e) { + restoreEntry(alias, entry, password, e); + throw e; + } return new Entry( privateKeyEntry.getPrivateKey(), @@ -209,11 +218,25 @@ public void set(NodeId certificateTypeId, Entry entry) throws Exception { String alias = getAlias(certificateTypeId); - keyStore.setKeyEntry( - alias, entry.privateKey, settings.getAliasPassword.apply(alias), entry.certificateChain); + if (alias == null) { + return; + } + + char[] password = settings.getAliasPassword.apply(alias); - keyStore.store( - new FileOutputStream(settings.keyStorePath.toFile()), settings.getKeyStorePassword.get()); + KeyStore.Entry previousEntry = + keyStore.isKeyEntry(alias) + ? keyStore.getEntry(alias, new KeyStore.PasswordProtection(password)) + : null; + + keyStore.setKeyEntry(alias, entry.privateKey, password, entry.certificateChain); + + try { + storeKeyStore(); + } catch (Exception e) { + restoreEntry(alias, previousEntry, password, e); + throw e; + } entries.put(alias, entry); } finally { @@ -225,9 +248,10 @@ public void set(NodeId certificateTypeId, Entry entry) throws Exception { * Get the alias to use when accessing certificates of type {@code certificateTypeId}. * * @param certificateTypeId the {@link NodeId} of the certificate type. - * @return the alias to use when accessing certificates of type {@code certificateTypeId}. + * @return the alias to use when accessing certificates of type {@code certificateTypeId}, or + * {@code null} if the certificate type is not supported. */ - protected String getAlias(NodeId certificateTypeId) { + protected @Nullable String getAlias(NodeId certificateTypeId) { if (certificateTypeId.equals(NodeIds.RsaSha256ApplicationCertificateType)) { return "server-rsa-sha256"; } else { @@ -248,6 +272,106 @@ protected void loadEntries() throws Exception { get(NodeIds.RsaSha256ApplicationCertificateType); } + /** + * Write the KeyStore to a temporary file in the same directory and then move it into place, + * replacing any existing file. + * + *

Opening the KeyStore file directly would truncate it before the new contents have been + * written, so a failure part way through the write would leave behind a KeyStore with no keys in + * it. + * + * @throws Exception if an error occurs while writing the KeyStore. + */ + private void storeKeyStore() throws Exception { + Path keyStorePath = resolveKeyStorePath(); + Path tempPath = Files.createTempFile(keyStorePath.getParent(), ".keystore", ".tmp"); + + try { + copyPosixFilePermissions(keyStorePath, tempPath); + + try (var outputStream = new FileOutputStream(tempPath.toFile())) { + keyStore.store(outputStream, settings.getKeyStorePassword.get()); + + // Force the contents to disk before the move, so a crash can't leave the moved-into-place + // file holding nothing. + outputStream.getFD().sync(); + } + + Files.move( + tempPath, + keyStorePath, + StandardCopyOption.REPLACE_EXISTING, + StandardCopyOption.ATOMIC_MOVE); + } catch (Exception e) { + try { + Files.deleteIfExists(tempPath); + } catch (IOException ex) { + e.addSuppressed(ex); + } + + throw e; + } + } + + /** + * Resolve the path to write the KeyStore to, following symbolic links so that an existing link is + * updated in place rather than replaced by a regular file. + * + * @return the absolute, link-resolved path of the KeyStore file. + * @throws IOException if an error occurs while resolving the path. + */ + private Path resolveKeyStorePath() throws IOException { + Path keyStorePath = settings.keyStorePath.toAbsolutePath(); + + return Files.exists(keyStorePath) ? keyStorePath.toRealPath() : keyStorePath; + } + + /** + * Copy the POSIX permissions of {@code from} onto {@code to}, if {@code from} exists and the file + * system supports them. + * + *

Temporary files are created readable only by their owner, so without this the KeyStore would + * lose any permissions the user had configured every time it was replaced. + * + * @param from the file to read permissions from. + * @param to the file to apply them to. + * @throws IOException if an error occurs while reading or applying the permissions. + */ + private static void copyPosixFilePermissions(Path from, Path to) throws IOException { + if (Files.exists(from) + && from.getFileSystem().supportedFileAttributeViews().contains("posix")) { + + Files.setPosixFilePermissions(to, Files.getPosixFilePermissions(from)); + } + } + + /** + * Restore {@code previousEntry} under {@code alias} after a failed write, so the in-memory + * KeyStore does not diverge from the file on disk. + * + *

The {@code entries} cache needs no equivalent treatment: {@link #get(NodeId)} falls back to + * the KeyStore and repopulates it. + * + * @param alias the alias to restore. + * @param previousEntry the entry that was present before the write, or {@code null} if there was + * none. + * @param password the password protecting {@code alias}. + * @param cause the failure being recovered from, which any failure to restore is attached to. + */ + private void restoreEntry( + String alias, KeyStore.@Nullable Entry previousEntry, char[] password, Exception cause) { + + try { + if (previousEntry != null) { + keyStore.setEntry(alias, previousEntry, new KeyStore.PasswordProtection(password)); + } else { + keyStore.deleteEntry(alias); + } + } catch (KeyStoreException e) { + cause.addSuppressed(e); + } + } + private void configureWatchService(File keyStoreFile) throws IOException { watchService = FileSystems.getDefault().newWatchService(); diff --git a/opc-ua-stack/stack-core/src/test/java/org/eclipse/milo/opcua/stack/core/security/KeyStoreCertificateStoreTest.java b/opc-ua-stack/stack-core/src/test/java/org/eclipse/milo/opcua/stack/core/security/KeyStoreCertificateStoreTest.java index 70cff30f06..ba79778941 100644 --- a/opc-ua-stack/stack-core/src/test/java/org/eclipse/milo/opcua/stack/core/security/KeyStoreCertificateStoreTest.java +++ b/opc-ua-stack/stack-core/src/test/java/org/eclipse/milo/opcua/stack/core/security/KeyStoreCertificateStoreTest.java @@ -10,12 +10,24 @@ package org.eclipse.milo.opcua.stack.core.security; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; +import java.security.KeyPair; +import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Supplier; +import java.util.stream.Stream; import org.eclipse.milo.opcua.stack.core.types.builtin.NodeId; import org.jspecify.annotations.Nullable; import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; class KeyStoreCertificateStoreTest extends CertificateStoreTest { @@ -27,8 +39,8 @@ class KeyStoreCertificateStoreTest extends CertificateStoreTest { @AfterEach void deleteTestFiles() { try { - Files.deleteIfExists(testPath); Files.deleteIfExists(keyStorePath); + Files.deleteIfExists(testPath); } catch (Exception ignored) { testPath.toFile().deleteOnExit(); keyStorePath.toFile().deleteOnExit(); @@ -37,19 +49,79 @@ void deleteTestFiles() { @Override protected CertificateStore newCertificateStore() throws Exception { - var store = - new KeyStoreCertificateStore( - new KeyStoreCertificateStore.Settings( - keyStorePath, "password"::toCharArray, alias -> "password".toCharArray())) { - - @Override - protected @Nullable String getAlias(NodeId certificateTypeId) { - return certificateTypeId.getIdentifier().toString(); - } - }; + KeyStoreCertificateStore store = newKeyStoreCertificateStore("password"::toCharArray); store.initialize(); return store; } + + /** + * Opening a second store over the same file is the only way to tell that {@link + * CertificateStore#set(NodeId, CertificateStore.Entry)} actually wrote anything; the assertions + * inherited from {@link CertificateStoreTest} all pass against the in-memory KeyStore alone. + */ + @Test + void entriesAreWrittenToDisk() throws Exception { + var certificateTypeId = new NodeId(2, "persisted"); + + certificateStore.set(certificateTypeId, newEntry()); + + assertNotNull(newCertificateStore().get(certificateTypeId)); + } + + @Test + void failedWriteLeavesKeyStoreIntact() throws Exception { + var certificateTypeId = new NodeId(2, "unwritable"); + var failWrite = new AtomicBoolean(false); + + KeyStoreCertificateStore store = + newKeyStoreCertificateStore( + () -> { + if (failWrite.get()) { + throw new IllegalStateException("KeyStore password unavailable"); + } + return "password".toCharArray(); + }); + + store.initialize(); + store.set(new NodeId(2, "survivor"), newEntry()); + + failWrite.set(true); + assertThrows(IllegalStateException.class, () -> store.set(certificateTypeId, newEntry())); + failWrite.set(false); + + // The failed write must be rolled back in memory... + assertFalse(store.contains(certificateTypeId)); + + // ...must not have left a partially written temporary file behind... + try (Stream files = Files.list(testPath)) { + assertEquals(List.of(keyStorePath), files.toList()); + } + + // ...and must not have damaged what was already on disk. + CertificateStore reopened = newCertificateStore(); + assertTrue(reopened.contains(new NodeId(2, "survivor"))); + assertFalse(reopened.contains(certificateTypeId)); + } + + private KeyStoreCertificateStore newKeyStoreCertificateStore(Supplier keyStorePassword) { + return new KeyStoreCertificateStore( + new KeyStoreCertificateStore.Settings( + keyStorePath, keyStorePassword, alias -> "password".toCharArray())) { + + @Override + protected @Nullable String getAlias(NodeId certificateTypeId) { + return certificateTypeId.getIdentifier().toString(); + } + }; + } + + private static CertificateStore.Entry newEntry() { + var factory = new TestCertificateFactory(); + KeyPair keyPair = factory.createRsaSha256KeyPair(); + + return new CertificateStore.Entry( + keyPair.getPrivate(), factory.createRsaSha256CertificateChain(keyPair)); + } } From 6dc726c8a73c9faaff158c7f41afb1950bf648fc Mon Sep 17 00:00:00 2001 From: Kevin Herron Date: Wed, 19 Aug 2026 05:12:24 -0700 Subject: [PATCH 2/2] Close KeyStore streams in the example loaders The same unclosed-stream problem as KeyStoreCertificateStore. Both copies now use Path with Files.newInputStream/newOutputStream, matching the client-examples copy that already had the fix. --- .../milo/examples/server/KeyStoreLoader.java | 18 +++++++++++------- .../milo/opcua/sdk/test/KeyStoreLoader.java | 18 ++++++++++++------ 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/milo-examples/server-examples/src/main/java/org/eclipse/milo/examples/server/KeyStoreLoader.java b/milo-examples/server-examples/src/main/java/org/eclipse/milo/examples/server/KeyStoreLoader.java index 89ea40ec98..e972a3a255 100644 --- a/milo-examples/server-examples/src/main/java/org/eclipse/milo/examples/server/KeyStoreLoader.java +++ b/milo-examples/server-examples/src/main/java/org/eclipse/milo/examples/server/KeyStoreLoader.java @@ -11,9 +11,9 @@ package org.eclipse.milo.examples.server; import com.google.common.collect.Sets; -import java.io.File; -import java.io.FileInputStream; -import java.io.FileOutputStream; +import java.io.InputStream; +import java.io.OutputStream; +import java.nio.file.Files; import java.nio.file.Path; import java.security.Key; import java.security.KeyPair; @@ -48,11 +48,11 @@ class KeyStoreLoader { KeyStoreLoader load(Path baseDir) throws Exception { KeyStore keyStore = KeyStore.getInstance("PKCS12"); - File serverKeyStore = baseDir.resolve("example-server.pfx").toFile(); + Path serverKeyStore = baseDir.resolve("example-server.pfx"); logger.info("Loading KeyStore at {}", serverKeyStore); - if (!serverKeyStore.exists()) { + if (!Files.exists(serverKeyStore)) { keyStore.load(null, PASSWORD); KeyPair keyPair = SelfSignedCertificateGenerator.generateRsaKeyPair(2048); @@ -86,9 +86,13 @@ KeyStoreLoader load(Path baseDir) throws Exception { keyStore.setKeyEntry( SERVER_ALIAS, keyPair.getPrivate(), PASSWORD, new X509Certificate[] {certificate}); - keyStore.store(new FileOutputStream(serverKeyStore), PASSWORD); + try (OutputStream out = Files.newOutputStream(serverKeyStore)) { + keyStore.store(out, PASSWORD); + } } else { - keyStore.load(new FileInputStream(serverKeyStore), PASSWORD); + try (InputStream in = Files.newInputStream(serverKeyStore)) { + keyStore.load(in, PASSWORD); + } } Key serverPrivateKey = keyStore.getKey(SERVER_ALIAS, PASSWORD); diff --git a/opc-ua-sdk/integration-tests/src/test/java/org/eclipse/milo/opcua/sdk/test/KeyStoreLoader.java b/opc-ua-sdk/integration-tests/src/test/java/org/eclipse/milo/opcua/sdk/test/KeyStoreLoader.java index 2da81b06ed..264338fa47 100644 --- a/opc-ua-sdk/integration-tests/src/test/java/org/eclipse/milo/opcua/sdk/test/KeyStoreLoader.java +++ b/opc-ua-sdk/integration-tests/src/test/java/org/eclipse/milo/opcua/sdk/test/KeyStoreLoader.java @@ -11,8 +11,10 @@ package org.eclipse.milo.opcua.sdk.test; import java.io.File; -import java.io.FileInputStream; -import java.io.FileOutputStream; +import java.io.InputStream; +import java.io.OutputStream; +import java.nio.file.Files; +import java.nio.file.Path; import java.security.Key; import java.security.KeyPair; import java.security.KeyStore; @@ -46,11 +48,11 @@ class KeyStoreLoader { KeyStoreLoader load(File baseDir) throws Exception { KeyStore keyStore = KeyStore.getInstance("PKCS12"); - File serverKeyStore = baseDir.toPath().resolve("example-server.pfx").toFile(); + Path serverKeyStore = baseDir.toPath().resolve("example-server.pfx"); LOGGER.debug("Loading KeyStore at {}", serverKeyStore); - if (!serverKeyStore.exists()) { + if (!Files.exists(serverKeyStore)) { keyStore.load(null, PASSWORD); KeyPair keyPair = SelfSignedCertificateGenerator.generateRsaKeyPair(2048); @@ -84,9 +86,13 @@ KeyStoreLoader load(File baseDir) throws Exception { keyStore.setKeyEntry( SERVER_ALIAS, keyPair.getPrivate(), PASSWORD, new X509Certificate[] {certificate}); - keyStore.store(new FileOutputStream(serverKeyStore), PASSWORD); + try (OutputStream out = Files.newOutputStream(serverKeyStore)) { + keyStore.store(out, PASSWORD); + } } else { - keyStore.load(new FileInputStream(serverKeyStore), PASSWORD); + try (InputStream in = Files.newInputStream(serverKeyStore)) { + keyStore.load(in, PASSWORD); + } } Key serverPrivateKey = keyStore.getKey(SERVER_ALIAS, PASSWORD);