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..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; @@ -102,9 +104,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 +115,26 @@ 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) { + 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 = keyStoreHandlerDecorated.readPublicKeyFromKeyStore(CsvSecureConstants.ENTRY_SIGNATURE); PrivateKey privateSignatureKey = @@ -128,9 +148,23 @@ 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(); + 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()); } @@ -150,7 +184,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 +208,18 @@ 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, 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/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..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,20 +25,27 @@ 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; 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.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; import java.util.Random; +import java.util.concurrent.Callable; import java.util.concurrent.FutureTask; import java.util.concurrent.TimeUnit; @@ -340,6 +347,158 @@ private static boolean isWaitingForLockOf(final ThreadMXBean threads, final Thre return threadInfo != null && threadInfo.getLockOwnerId() == owner.getId(); } + /** 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; + } + + @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) -> 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(), + "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" }, + }; + } + + /** 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); + } + + 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()); + } + } + + /** + * 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); + } + + @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" }, + { "corrupted keystore", (Damage) (csvFile, keyStoreFile) -> corruptKeyStore(keyStoreFile), + "cannot be loaded" }, + }; + } + + @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. + 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"}; + 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); + 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")); + + 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);