From d35f7d84bdb5f47712c2836b567a15b0fa619b61 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 16 Oct 2026 20:49:41 +0300 Subject: [PATCH 1/3] [#340] Tamper-evident CSV audit: close the file of a writer that fails to resume it When the keys cannot be read or the resumed file fails verification, the SecureCsvWriter constructor now closes the file, and with rotation the RotatableWriter with its rotator thread. A file that cannot be resumed (it fails verification, its headers do not match, its keystore is missing, cannot be loaded or lacks its keys) now fails with an IOException instead of a RuntimeException, so that CsvAuditEventHandler.startup() logs it and opens the other topics, and a publish to the topic fails with a BadRequestException. --- .../audit/handlers/csv/SecureCsvWriter.java | 36 +++++++- .../csv/CsvAuditEventHandlerTest.java | 52 ++++++++++++ .../audit/handlers/csv/CsvWriterTest.java | 4 +- .../handlers/csv/SecureCsvWriterTest.java | 83 +++++++++++++++++++ 4 files changed, 170 insertions(+), 5 deletions(-) diff --git a/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/SecureCsvWriter.java b/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/SecureCsvWriter.java index 1109e708d..41840fda7 100644 --- a/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/SecureCsvWriter.java +++ b/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/SecureCsvWriter.java @@ -102,9 +102,9 @@ class SecureCsvWriter implements CsvWriter, RolloverLifecycleHook { this.headers = checkNotNull(headers, "The headers can't be null."); this.csvFormatter = new CsvFormatter(csvPreference); this.csvWriter = constructWriter(csvFile, fileAlreadyInitialized, config); - this.hmacCalculator = new HmacCalculator(CsvSecureConstants.HMAC_ALGORITHM); try { + this.hmacCalculator = new HmacCalculator(CsvSecureConstants.HMAC_ALGORITHM); KeyStoreHandlerDecorator keyStoreHandlerDecorated = new KeyStoreHandlerDecorator(keyStoreHandler); SecretKey password = keyStoreHandlerDecorated.readSecretKeyFromKeyStore(CsvSecureConstants.ENTRY_PASSWORD); if (password == null) { @@ -113,8 +113,19 @@ class SecureCsvWriter implements CsvWriter, RolloverLifecycleHook { CsvSecureConstants.ENTRY_PASSWORD, keyStoreHandlerDecorated.getLocation())); } this.keyStorePassword = Base64.encode(password.getEncoded()); - KeyStoreHandler hmacKeyStoreHandler = - new JcaKeyStoreHandler(CsvSecureConstants.KEYSTORE_TYPE, keyStoreFile.getPath(), keyStorePassword); + final KeyStoreHandler hmacKeyStoreHandler; + try { + hmacKeyStoreHandler = new JcaKeyStoreHandler(CsvSecureConstants.KEYSTORE_TYPE, keyStoreFile.getPath(), + keyStorePassword); + } catch (IllegalStateException e) { + // KeyStoreBuilder throws it for a keystore it cannot load: an empty one, left by a crash while it was + // stored, or one stored under another password, after a change of the Password key. + if (!fileAlreadyInitialized) { + throw e; + } + throw new IOException( + "Resuming an existing CSV file but its keystore " + keyStoreFile + " cannot be loaded.", e); + } PublicKey publicSignatureKey = keyStoreHandlerDecorated.readPublicKeyFromKeyStore(CsvSecureConstants.ENTRY_SIGNATURE); PrivateKey privateSignatureKey = @@ -128,6 +139,12 @@ class SecureCsvWriter implements CsvWriter, RolloverLifecycleHook { privateSignatureKey); final CsvAuditEventHandlerConfiguration.CsvSecurity securityConfiguration = config.getSecurity(); if (fileAlreadyInitialized) { + // A missing keystore file reads as an empty keystore. Without the keys of the file, the file cannot be + // verified, nor resumed: this is the state of the file, not a configuration error. + if (secureStorage.readInitialKey() == null) { + throw new IOException("Resuming an existing CSV file but its keystore " + keyStoreFile + + " is missing or holds no initial key."); + } // Run the CsvVerifier to check that the file was not tampered. CsvSecureVerifier verifier = new CsvSecureVerifier(csvFile, csvPreference, secureStorage); CsvSecureVerifier.VerificationResult verificationResult = verifier.verify(); @@ -150,7 +167,7 @@ class SecureCsvWriter implements CsvWriter, RolloverLifecycleHook { SecretKey currentKey = secureStorage.readCurrentKey(); if (currentKey == null) { - throw new IllegalStateException("We are supposed to resume but there is not entry for CurrentKey."); + throw new IOException("We are supposed to resume but there is not entry for CurrentKey."); } this.hmacCalculator.setCurrentKey(currentKey.getEncoded()); @@ -174,6 +191,17 @@ public void run() { } }; } catch (Exception e) { + // Nothing else closes a writer that fails to construct: the file would stay open, and with rotation the + // rotator thread, which is not a daemon thread, would keep running. + try { + csvWriter.close(); + } catch (IOException | RuntimeException closeFailure) { + e.addSuppressed(closeFailure); + } + if (e instanceof IOException) { + // The file cannot be resumed: the handler reports it for the topic, and opens the other topics. + throw (IOException) e; + } throw new RuntimeException("Error when initializing a secure CSV writer", e); } } diff --git a/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/CsvAuditEventHandlerTest.java b/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/CsvAuditEventHandlerTest.java index 2779ea886..bd9a8b0ad 100644 --- a/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/CsvAuditEventHandlerTest.java +++ b/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/CsvAuditEventHandlerTest.java @@ -35,6 +35,7 @@ import java.io.OutputStream; import java.nio.file.Files; import java.nio.file.Path; +import java.util.ArrayList; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -437,6 +438,57 @@ public void testShutdownClosesEveryTamperEvidentWriterWhenOneFailsToClose() thro assertThreadsTerminate(writerThreads); } + @Test + public void testStartupOpensOtherTopicsWhenTamperEvidentFileCannotBeResumed() throws Exception { + final Path logDirectory = Files.createTempDirectory("CsvAuditEventHandlerTest"); + logDirectory.toFile().deleteOnExit(); + final CsvAuditEventHandlerBuilder builder = + csvAuditEventHandler().loggingTo(logDirectory).withSecureLoggingEnabled(); + // A second topic, so that a topic is left to open after the first one fails. + final Map topics = new LinkedHashMap<>(); + topics.put("access", builder.eventTopicsMetaData.getSchema("access")); + topics.put("activity", builder.eventTopicsMetaData.getSchema("access")); + final EventTopicsMetaData eventTopicsMetaData = new EventTopicsMetaData(topics); + builder.config.setTopics(eventTopicsMetaData.getTopics()); + final Context context = new RootContext(); + + // A file and a keystore for every topic. + CsvAuditEventHandler csvHandler = + new CsvAuditEventHandler(builder.config, eventTopicsMetaData, builder.keystoreHandlerProvider); + csvHandler.startup(); + for (String topic : topics.keySet()) { + AssertJPromiseAssert.assertThat(csvHandler.publishEvent(context, topic, buildEvent(1))).succeeded(); + } + csvHandler.shutdown(); + + csvHandler = new CsvAuditEventHandler(builder.config, eventTopicsMetaData, builder.keystoreHandlerProvider); + // The topic that startup opens first, so that the other topic is opened only if startup goes on. + final List handledTopics = new ArrayList<>(csvHandler.getHandledTopics()); + final String failingTopic = handledTopics.get(0); + final String otherTopic = handledTopics.get(1); + // The HMAC of the row no longer matches its value, so the file fails verification when it is resumed. + final Path failingFile = logDirectory.resolve(SECURE_CSV_FILENAME_PREFIX + failingTopic + ".csv"); + final String content = new String(Files.readAllBytes(failingFile), UTF_8); + Files.write(failingFile, content.replace("transactionId-X", "transactionId-Y").getBytes(UTF_8)); + + final AuditService auditService = newAuditService() + .withEventTopicsMetaData(eventTopicsMetaData) + .withAuditEventHandler(csvHandler) + .build(); + try { + auditService.startup(); + assertThat(auditService.isRunning()).isTrue(); + + AssertJPromiseAssert.assertThat(csvHandler.publishEvent(context, otherTopic, buildEvent(2))).succeeded(); + assertThat(logDirectory.resolve(SECURE_CSV_FILENAME_PREFIX + otherTopic + ".csv").toFile()) + .content().contains("\"_id2\""); + AssertJPromiseAssert.assertThat(csvHandler.publishEvent(context, failingTopic, buildEvent(2))) + .failedWithException().isInstanceOf(BadRequestException.class); + } finally { + auditService.shutdown(); + } + } + @DataProvider private Object[][] rotateActionData() { return new Object[][] { diff --git a/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/CsvWriterTest.java b/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/CsvWriterTest.java index cf8718860..98c20e040 100644 --- a/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/CsvWriterTest.java +++ b/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/CsvWriterTest.java @@ -12,6 +12,7 @@ * information: "Portions copyright [year] [name of copyright owner]". * * Copyright 2015-2016 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC. */ package org.forgerock.audit.handlers.csv; @@ -144,8 +145,9 @@ public void shouldResumeExistingCsvSecureFile() throws Exception { new SecureCsvWriter(csvFile, new String[] { "child1", "child2", "child3", "enfant4" }, csvPreference, configuration, keyStoreHandler, random); fail("Should have failed because headers do not match."); - } catch (RuntimeException e) { + } catch (IOException e) { // This is ok, we expect to have this exception. + assertThat(e).hasMessageContaining("the headers do not match"); } } diff --git a/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/SecureCsvWriterTest.java b/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/SecureCsvWriterTest.java index 58274d13c..11ed058ec 100644 --- a/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/SecureCsvWriterTest.java +++ b/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/SecureCsvWriterTest.java @@ -27,10 +27,12 @@ import java.io.ByteArrayOutputStream; import java.io.File; import java.io.IOException; +import java.io.OutputStream; import java.io.PrintStream; import java.lang.management.ManagementFactory; import java.lang.management.ThreadInfo; import java.lang.management.ThreadMXBean; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.security.KeyStore; @@ -39,6 +41,7 @@ import java.util.List; import java.util.Map; import java.util.Random; +import java.util.concurrent.Callable; import java.util.concurrent.FutureTask; import java.util.concurrent.TimeUnit; @@ -340,6 +343,86 @@ private static boolean isWaitingForLockOf(final ThreadMXBean threads, final Thre return threadInfo != null && threadInfo.getLockOwnerId() == owner.getId(); } + /** Leaves a written file and its keystore in a state that cannot be resumed. */ + private interface Damage { + void apply(File csvFile, File keyStoreFile) throws Exception; + } + + @DataProvider + private Object[][] unresumableFiles() { + return new Object[][] { + { "changed row", (Damage) (csvFile, keyStoreFile) -> { + // The HMAC of the row no longer matches its value. + final String content = new String(Files.readAllBytes(csvFile.toPath()), StandardCharsets.UTF_8); + Files.write(csvFile.toPath(), + content.replace("first-event", "forged-event").getBytes(StandardCharsets.UTF_8)); + }, "The CSV file was tampered" }, + { "missing keystore", (Damage) (csvFile, keyStoreFile) -> Files.delete(keyStoreFile.toPath()), + "no initial key" }, + { "keystore without current key", (Damage) (csvFile, keyStoreFile) -> { + final JcaKeyStoreHandler fileKeyStore = loadFileKeyStore(keyStoreFile); + fileKeyStore.getStore().deleteEntry(KeyStoreSecureStorage.ENTRY_CURRENT_KEY); + fileKeyStore.store(); + }, "Final HMAC key is null" }, + // as left by a crash while the keystore was stored + { "empty keystore", (Damage) (csvFile, keyStoreFile) -> Files.write(keyStoreFile.toPath(), new byte[0]), + "cannot be loaded" }, + // as after a change of the Password key in the handler keystore + { "keystore of another password", (Damage) (csvFile, keyStoreFile) -> { + final KeyStore store = loadFileKeyStore(keyStoreFile).getStore(); + try (OutputStream out = Files.newOutputStream(keyStoreFile.toPath())) { + store.store(out, "another password".toCharArray()); + } + }, "cannot be loaded" }, + }; + } + + /** Loads the keystore of a file with the password that the writer derives from the handler keystore. */ + private JcaKeyStoreHandler loadFileKeyStore(File keyStoreFile) throws Exception { + final String password = Base64.encode(keyStoreHandler.readSecretKeyFromKeyStore(ENTRY_PASSWORD).getEncoded()); + return new JcaKeyStoreHandler(KEYSTORE_TYPE, keyStoreFile.getPath(), password); + } + + @Test(dataProvider = "unresumableFiles") + public void shouldStopRotatorAndFailWithIOExceptionWhenFileCannotBeResumed( + final String description, final Damage damage, final String reason) throws Exception { + final Path logDirectory = Files.createTempDirectory("SecureCsvWriterTest"); + final File actual = new File(logDirectory.toFile(), "tamper-evident-test.csv"); + final String[] headers = {"FOO"}; + final CsvAuditEventHandlerConfiguration config = createBasicSecureConfig("5 minutes"); + // A rotation policy, so that the RotatableWriter starts its rotator thread; no check runs during the test. + config.getFileRotation().setRotationEnabled(true); + config.getFileRotation().setMaxFileSize(1024 * 1024); + config.setRotationRetentionCheckInterval("5 minutes"); + + // The executors of a writer create their threads in the group of the thread that creates the writer. + final ThreadGroup writerThreads = new ThreadGroup("SecureCsvWriterTest"); + final SecureCsvWriter secureCsvWriter = callInThreadGroup(writerThreads, () -> new SecureCsvWriter( + actual, headers, CsvPreference.EXCEL_PREFERENCE, config, keyStoreHandler, random)); + assertThat(writerThreads.activeCount()).as("rotator threads").isEqualTo(1); + secureCsvWriter.writeEvent(singletonMap(headers[0], "first-event")); + secureCsvWriter.close(); + assertThreadsTerminate(writerThreads); + + damage.apply(actual, new File(actual.getPath() + ".keystore")); + + final ThreadGroup resumingThreads = new ThreadGroup("SecureCsvWriterTest-resume"); + final Throwable failure = callInThreadGroup(resumingThreads, () -> catchThrowable(() -> new SecureCsvWriter( + actual, headers, CsvPreference.EXCEL_PREFERENCE, config, keyStoreHandler, random))); + assertThat(failure).as(description).isInstanceOf(IOException.class).hasMessageContaining(reason); + // The rotator thread is not a daemon thread, it would keep the JVM from exiting. + assertThreadsTerminate(resumingThreads); + } + + /** Runs the task in a new thread of the group, and returns its result once the thread has ended. */ + private static T callInThreadGroup(ThreadGroup group, Callable task) throws Exception { + final FutureTask future = new FutureTask<>(task); + final Thread thread = new Thread(group, future); + thread.start(); + thread.join(); + return future.get(); + } + private CsvAuditEventHandlerConfiguration createBasicSecureConfig(String interval) { CsvAuditEventHandlerConfiguration configuration = new CsvAuditEventHandlerConfiguration(); configuration.getSecurity().setEnabled(true); From d0eb5f9515d6254e063fc7380a7854b9a2bd3821 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Sat, 17 Oct 2026 09:30:20 +0300 Subject: [PATCH 2/3] [#340] Fail only the topic of an unparseable file or an unloadable keystore A resumed file with a row, a header or a final signature cut short by a crash made CsvSecureVerifier.verify() throw a RuntimeException: a SuperCsvException, a NullPointerException for a signature that no longer decodes as Base64, an ArrayIndexOutOfBoundsException for a header of one column. The constructor wrapped it, and the startup failed. It now turns it into an IOException, as CsvSecureArchiveVerifier turns it into a failed result. A keystore that cannot be opened or loaded now fails its topic with an IOException also when the file is not resumed. A topic that got no event keeps an empty file next to its keystore, and after a change of the Password key it failed the whole startup. --- .../audit/handlers/csv/SecureCsvWriter.java | 27 +++++--- .../handlers/csv/SecureCsvWriterTest.java | 67 ++++++++++++++++--- 2 files changed, 76 insertions(+), 18 deletions(-) diff --git a/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/SecureCsvWriter.java b/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/SecureCsvWriter.java index 41840fda7..6938e9a6d 100644 --- a/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/SecureCsvWriter.java +++ b/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/SecureCsvWriter.java @@ -117,14 +117,12 @@ class SecureCsvWriter implements CsvWriter, RolloverLifecycleHook { try { hmacKeyStoreHandler = new JcaKeyStoreHandler(CsvSecureConstants.KEYSTORE_TYPE, keyStoreFile.getPath(), keyStorePassword); - } catch (IllegalStateException e) { - // KeyStoreBuilder throws it for a keystore it cannot load: an empty one, left by a crash while it was - // stored, or one stored under another password, after a change of the Password key. - if (!fileAlreadyInitialized) { - throw e; - } - throw new IOException( - "Resuming an existing CSV file but its keystore " + keyStoreFile + " cannot be loaded.", e); + } catch (IOException | IllegalStateException e) { + // KeyStoreBuilder cannot open a directory or a file it may not read, and throws an IllegalStateException + // for a keystore it cannot load: an empty one, left by a crash while it was stored, or one stored under + // another password, after a change of the Password key. Also when the file is not resumed: a topic + // that got no event keeps an empty file next to its keystore. + throw new IOException("The keystore " + keyStoreFile + " of the CSV file cannot be loaded.", e); } PublicKey publicSignatureKey = keyStoreHandlerDecorated.readPublicKeyFromKeyStore(CsvSecureConstants.ENTRY_SIGNATURE); @@ -147,7 +145,15 @@ class SecureCsvWriter implements CsvWriter, RolloverLifecycleHook { } // Run the CsvVerifier to check that the file was not tampered. CsvSecureVerifier verifier = new CsvSecureVerifier(csvFile, csvPreference, secureStorage); - CsvSecureVerifier.VerificationResult verificationResult = verifier.verify(); + final CsvSecureVerifier.VerificationResult verificationResult; + try { + verificationResult = verifier.verify(); + } catch (RuntimeException e) { + // The verifier fails this way on a row it cannot parse or decode: a row, a header or a signature + // cut short by a crash while it was written, or a row edited. As CsvSecureArchiveVerifier does, it + // is a file that fails verification. + throw new IOException("Resuming an existing CSV file but it cannot be verified: " + csvFile, e); + } if (!verificationResult.hasPassedVerification()) { throw new IOException("The CSV file was tampered: " + verificationResult.getFailureReason()); } @@ -199,7 +205,8 @@ public void run() { e.addSuppressed(closeFailure); } if (e instanceof IOException) { - // The file cannot be resumed: the handler reports it for the topic, and opens the other topics. + // The file cannot be resumed, or its keystore cannot be loaded: the handler reports it for the topic, + // and opens the other topics. throw (IOException) e; } throw new RuntimeException("Error when initializing a secure CSV writer", e); diff --git a/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/SecureCsvWriterTest.java b/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/SecureCsvWriterTest.java index 11ed058ec..a1537c5d9 100644 --- a/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/SecureCsvWriterTest.java +++ b/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/SecureCsvWriterTest.java @@ -35,8 +35,10 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.StandardOpenOption; import java.security.KeyStore; import java.security.PublicKey; +import java.util.Arrays; import java.util.Comparator; import java.util.List; import java.util.Map; @@ -343,7 +345,7 @@ private static boolean isWaitingForLockOf(final ThreadMXBean threads, final Thre return threadInfo != null && threadInfo.getLockOwnerId() == owner.getId(); } - /** Leaves a written file and its keystore in a state that cannot be resumed. */ + /** Leaves a file and its keystore in a state that a writer cannot reopen. */ private interface Damage { void apply(File csvFile, File keyStoreFile) throws Exception; } @@ -368,12 +370,24 @@ private Object[][] unresumableFiles() { { "empty keystore", (Damage) (csvFile, keyStoreFile) -> Files.write(keyStoreFile.toPath(), new byte[0]), "cannot be loaded" }, // as after a change of the Password key in the handler keystore - { "keystore of another password", (Damage) (csvFile, keyStoreFile) -> { - final KeyStore store = loadFileKeyStore(keyStoreFile).getStore(); - try (OutputStream out = Files.newOutputStream(keyStoreFile.toPath())) { - store.store(out, "another password".toCharArray()); - } - }, "cannot be loaded" }, + { "keystore of another password", + (Damage) (csvFile, keyStoreFile) -> storeUnderAnotherPassword(keyStoreFile), "cannot be loaded" }, + // The verifier fails with a RuntimeException on the rows below. + // as left by a crash while the last row was written: a row with fewer columns than the header + { "row cut short", (Damage) (csvFile, keyStoreFile) -> Files.write(csvFile.toPath(), + "cut-short-event\n".getBytes(StandardCharsets.UTF_8), StandardOpenOption.APPEND), + "cannot be verified" }, + { "quoted value cut short", (Damage) (csvFile, keyStoreFile) -> Files.write(csvFile.toPath(), + "\"cut, short".getBytes(StandardCharsets.UTF_8), StandardOpenOption.APPEND), + "cannot be verified" }, + // the padding of the final signature is cut, so that it no longer decodes as Base64 + { "signature cut short", (Damage) (csvFile, keyStoreFile) -> { + final byte[] content = Files.readAllBytes(csvFile.toPath()); + Files.write(csvFile.toPath(), Arrays.copyOf(content, content.length - 3)); + }, "cannot be verified" }, + // as left by a crash while the header was written along with the first event + { "header cut short", (Damage) (csvFile, keyStoreFile) -> + Files.write(csvFile.toPath(), "FOO".getBytes(StandardCharsets.UTF_8)), "cannot be verified" }, }; } @@ -383,9 +397,42 @@ private JcaKeyStoreHandler loadFileKeyStore(File keyStoreFile) throws Exception return new JcaKeyStoreHandler(KEYSTORE_TYPE, keyStoreFile.getPath(), password); } + private void storeUnderAnotherPassword(File keyStoreFile) throws Exception { + final KeyStore store = loadFileKeyStore(keyStoreFile).getStore(); + try (OutputStream out = Files.newOutputStream(keyStoreFile.toPath())) { + store.store(out, "another password".toCharArray()); + } + } + @Test(dataProvider = "unresumableFiles") public void shouldStopRotatorAndFailWithIOExceptionWhenFileCannotBeResumed( final String description, final Damage damage, final String reason) throws Exception { + assertReopeningFailsWithIOException(true, description, damage, reason); + } + + @DataProvider + private Object[][] unloadableKeystoresOfEmptyFiles() { + return new Object[][] { + { "keystore replaced by a directory", (Damage) (csvFile, keyStoreFile) -> { + Files.delete(keyStoreFile.toPath()); + Files.createDirectory(keyStoreFile.toPath()); + }, "cannot be loaded" }, + // as after a change of the Password key in the handler keystore + { "keystore of another password", + (Damage) (csvFile, keyStoreFile) -> storeUnderAnotherPassword(keyStoreFile), "cannot be loaded" }, + }; + } + + @Test(dataProvider = "unloadableKeystoresOfEmptyFiles") + public void shouldStopRotatorAndFailWithIOExceptionWhenKeystoreOfEmptyFileCannotBeLoaded( + final String description, final Damage damage, final String reason) throws Exception { + // A topic that got no event keeps an empty file next to its keystore. + assertReopeningFailsWithIOException(false, description, damage, reason); + } + + /** Writes a file, with an event or without, damages it, and asserts that a writer fails to reopen it. */ + private void assertReopeningFailsWithIOException(final boolean withEvent, final String description, + final Damage damage, final String reason) throws Exception { final Path logDirectory = Files.createTempDirectory("SecureCsvWriterTest"); final File actual = new File(logDirectory.toFile(), "tamper-evident-test.csv"); final String[] headers = {"FOO"}; @@ -400,9 +447,13 @@ public void shouldStopRotatorAndFailWithIOExceptionWhenFileCannotBeResumed( final SecureCsvWriter secureCsvWriter = callInThreadGroup(writerThreads, () -> new SecureCsvWriter( actual, headers, CsvPreference.EXCEL_PREFERENCE, config, keyStoreHandler, random)); assertThat(writerThreads.activeCount()).as("rotator threads").isEqualTo(1); - secureCsvWriter.writeEvent(singletonMap(headers[0], "first-event")); + if (withEvent) { + secureCsvWriter.writeEvent(singletonMap(headers[0], "first-event")); + } secureCsvWriter.close(); assertThreadsTerminate(writerThreads); + // An empty file is not resumed: a writer reopens it as a new file. + assertThat(actual.length() > 0).as("file written").isEqualTo(withEvent); damage.apply(actual, new File(actual.getPath() + ".keystore")); From 3a4b8b3903697576a5bca4fd61707aa6b851c760 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Sun, 18 Oct 2026 14:33:43 +0300 Subject: [PATCH 3/3] [#340] Fail only the topic of a corrupted keystore KeyStore.load lets a RuntimeException, such as a ClassCastException or a NullPointerException, through for a corrupted JCEKS keystore. The constructor now turns it into the IOException that names the keystore, as it does for an empty keystore or one stored under another password. The IllegalStateException of KeyStoreBuilder whose cause is a KeyStoreException or a NoSuchAlgorithmException is rethrown instead: the JVM cannot create or check a JCEKS keystore, a configuration error that still fails the startup. The two parameterised tests of SecureCsvWriterTest get a corrupted-keystore row and a 60 s timeOut. --- .../audit/handlers/csv/SecureCsvWriter.java | 21 ++++++++++---- .../handlers/csv/SecureCsvWriterTest.java | 29 +++++++++++++++++-- 2 files changed, 43 insertions(+), 7 deletions(-) diff --git a/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/SecureCsvWriter.java b/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/SecureCsvWriter.java index 6938e9a6d..a0ef061ff 100644 --- a/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/SecureCsvWriter.java +++ b/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/SecureCsvWriter.java @@ -29,6 +29,8 @@ import java.io.FileOutputStream; import java.io.IOException; import java.io.Writer; +import java.security.KeyStoreException; +import java.security.NoSuchAlgorithmException; import java.security.PrivateKey; import java.security.PublicKey; import java.security.SignatureException; @@ -117,11 +119,20 @@ class SecureCsvWriter implements CsvWriter, RolloverLifecycleHook { try { hmacKeyStoreHandler = new JcaKeyStoreHandler(CsvSecureConstants.KEYSTORE_TYPE, keyStoreFile.getPath(), keyStorePassword); - } catch (IOException | IllegalStateException e) { - // KeyStoreBuilder cannot open a directory or a file it may not read, and throws an IllegalStateException - // for a keystore it cannot load: an empty one, left by a crash while it was stored, or one stored under - // another password, after a change of the Password key. Also when the file is not resumed: a topic - // that got no event keeps an empty file next to its keystore. + } catch (IllegalStateException e) { + if (e.getCause() instanceof KeyStoreException || e.getCause() instanceof NoSuchAlgorithmException) { + // The JVM cannot create a keystore of this type, or find the digest that checks it: a + // configuration error, not the state of the file. + throw e; + } + // KeyStoreBuilder throws it for a keystore it cannot load: an empty one, left by a crash while it was + // stored, or one stored under another password, after a change of the Password key. Also when the + // file is not resumed: a topic that got no event keeps an empty file next to its keystore. + throw new IOException("The keystore " + keyStoreFile + " of the CSV file cannot be loaded.", e); + } catch (IOException | RuntimeException e) { + // KeyStoreBuilder cannot open a directory or a file it may not read, and KeyStore.load lets a + // RuntimeException, such as a ClassCastException or a NullPointerException, through for a corrupted + // keystore. throw new IOException("The keystore " + keyStoreFile + " of the CSV file cannot be loaded.", e); } PublicKey publicSignatureKey = diff --git a/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/SecureCsvWriterTest.java b/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/SecureCsvWriterTest.java index a1537c5d9..4b2ef8bdb 100644 --- a/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/SecureCsvWriterTest.java +++ b/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/SecureCsvWriterTest.java @@ -25,8 +25,10 @@ import static org.forgerock.util.time.Duration.duration; import java.io.ByteArrayOutputStream; +import java.io.DataOutputStream; import java.io.File; import java.io.IOException; +import java.io.ObjectOutputStream; import java.io.OutputStream; import java.io.PrintStream; import java.lang.management.ManagementFactory; @@ -372,6 +374,8 @@ private Object[][] unresumableFiles() { // as after a change of the Password key in the handler keystore { "keystore of another password", (Damage) (csvFile, keyStoreFile) -> storeUnderAnotherPassword(keyStoreFile), "cannot be loaded" }, + { "corrupted keystore", (Damage) (csvFile, keyStoreFile) -> corruptKeyStore(keyStoreFile), + "cannot be loaded" }, // The verifier fails with a RuntimeException on the rows below. // as left by a crash while the last row was written: a row with fewer columns than the header { "row cut short", (Damage) (csvFile, keyStoreFile) -> Files.write(csvFile.toPath(), @@ -404,7 +408,26 @@ private void storeUnderAnotherPassword(File keyStoreFile) throws Exception { } } - @Test(dataProvider = "unresumableFiles") + /** + * Replaces the keystore by a JCEKS keystore whose secret key entry holds a string in place of the sealed key. + * {@link KeyStore#load} fails on it with a {@link ClassCastException}, as it lets a {@link RuntimeException} + * through for some corrupted keystores, before it checks the digest of the keystore. + */ + private static void corruptKeyStore(File keyStoreFile) throws IOException { + try (DataOutputStream out = new DataOutputStream(Files.newOutputStream(keyStoreFile.toPath()))) { + out.writeInt(0xcececece); // the magic number of JCEKS + out.writeInt(2); // the version + out.writeInt(1); // the number of entries + out.writeInt(3); // a secret key entry + out.writeUTF(KeyStoreSecureStorage.ENTRY_INITIAL_KEY); + out.writeLong(0L); // the creation date + final ObjectOutputStream objects = new ObjectOutputStream(out); + objects.writeObject("not a sealed key"); + objects.flush(); + } + } + + @Test(dataProvider = "unresumableFiles", timeOut = 60_000) public void shouldStopRotatorAndFailWithIOExceptionWhenFileCannotBeResumed( final String description, final Damage damage, final String reason) throws Exception { assertReopeningFailsWithIOException(true, description, damage, reason); @@ -420,10 +443,12 @@ private Object[][] unloadableKeystoresOfEmptyFiles() { // as after a change of the Password key in the handler keystore { "keystore of another password", (Damage) (csvFile, keyStoreFile) -> storeUnderAnotherPassword(keyStoreFile), "cannot be loaded" }, + { "corrupted keystore", (Damage) (csvFile, keyStoreFile) -> corruptKeyStore(keyStoreFile), + "cannot be loaded" }, }; } - @Test(dataProvider = "unloadableKeystoresOfEmptyFiles") + @Test(dataProvider = "unloadableKeystoresOfEmptyFiles", timeOut = 60_000) public void shouldStopRotatorAndFailWithIOExceptionWhenKeystoreOfEmptyFileCannotBeLoaded( final String description, final Damage damage, final String reason) throws Exception { // A topic that got no event keeps an empty file next to its keystore.