[#340] Tamper-evident CSV audit: close the file of a writer that fails to resume it

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);
         }
     }

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<String, JsonValue> 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<String> 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[][] {

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");
         }
     }