From 21bc0dd40b718fcd2c0eb3981dbfd244cbbe86a1 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Sat, 17 Oct 2026 18:28:39 +0300 Subject: [PATCH 1/2] [#344] Tamper-evident CSV audit: sign the column names and the value lengths in the row HMAC The HMAC of a row covered its values concatenated, so an edit that moved characters across a column boundary, or a value into an empty neighbouring column, kept it, and swapping two column names in the header kept every HMAC of the file. The HMAC now covers the byte 0xFF, then the name and the value of each column, each after the number of its UTF-8 bytes. The verifier still accepts a row signed in the previous format, with the same key, so the files written before are verified and resumed as before. UTF-8 never holds the leading byte, so a row of the new format cannot pass as a row of the previous one. A row with an empty HMAC fails verification instead of throwing NullPointerException. --- .../audit/handlers/csv/CsvSecureUtils.java | 37 +++++- .../audit/handlers/csv/CsvSecureVerifier.java | 8 +- .../audit/handlers/csv/HmacCalculator.java | 37 +++++- .../handlers/csv/CsvSecureVerifierTest.java | 116 ++++++++++++++++++ .../handlers/csv/HmacCalculatorTest.java | 17 +++ .../handlers/csv/SecureCsvWriterTest.java | 31 +++++ .../test/resources/secureCsvLegacyHmac.csv | 5 + .../shouldGenerateHMACColumn-expected.txt | 6 +- ...GeneratePeriodicallySignature-expected.txt | 8 +- ...dGeneratePeriodicallySignature-partial.txt | 4 +- 10 files changed, 250 insertions(+), 19 deletions(-) create mode 100644 commons/audit/handler-csv/src/test/resources/secureCsvLegacyHmac.csv diff --git a/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/CsvSecureUtils.java b/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/CsvSecureUtils.java index 98597406e1..8cd3532d07 100644 --- a/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/CsvSecureUtils.java +++ b/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/CsvSecureUtils.java @@ -17,7 +17,9 @@ package org.forgerock.audit.handlers.csv; +import java.io.ByteArrayOutputStream; import java.io.File; +import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.Map; @@ -30,6 +32,13 @@ */ final class CsvSecureUtils { + /** + * The first byte of the data that the HMAC of a row covers. UTF-8 never holds it, so these data never equal + * {@link #concatenatedValues(Map, String...)}, the UTF-8 bytes of a row that a previous version signed: an HMAC + * verifies a row in one format only. + */ + private static final int DATA_TO_SIGN_MARKER = 0xFF; + /** * Obtain the data to sign. * @param lastSignature The last signature. @@ -71,12 +80,38 @@ private static byte[] concat(byte[]... arrays) { } /** - * Get the data to sign. + * Get the data to sign: the byte 0xFF, then the name and the value of each column in UTF-8, each one after the + * number of its bytes as a big-endian int. + *

+ * The CSV reader reads an empty name or value as {@code null}, so both are signed as empty. * @param logger the value of logger * @param values the value of values * @param nameMapping the value of nameMapping */ static byte[] dataToSign(Logger logger, Map values, String... nameMapping) { + ByteArrayOutputStream data = new ByteArrayOutputStream(); + data.write(DATA_TO_SIGN_MARKER); + for (String h : nameMapping) { + final Object value = values.get(h); + writeLengthAndBytes(data, h != null ? h : ""); + writeLengthAndBytes(data, value != null ? value.toString() : ""); + } + return data.toByteArray(); + } + + private static void writeLengthAndBytes(ByteArrayOutputStream data, String field) { + final byte[] bytes = field.getBytes(StandardCharsets.UTF_8); + data.write(ByteBuffer.allocate(Integer.BYTES).putInt(bytes.length).array(), 0, Integer.BYTES); + data.write(bytes, 0, bytes.length); + } + + /** + * Get the data that the HMAC of a row covered before {@link #dataToSign(Logger, Map, String...)}: the values + * concatenated, which do not tell where a value ends, nor its column. + * @param values the value of values + * @param nameMapping the value of nameMapping + */ + static byte[] concatenatedValues(Map values, String... nameMapping) { StringBuilder tmp = new StringBuilder(); for (String h : nameMapping) { final Object value = values.get(h); diff --git a/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/CsvSecureVerifier.java b/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/CsvSecureVerifier.java index 4041ab0c03..3908b4b53b 100644 --- a/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/CsvSecureVerifier.java +++ b/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/CsvSecureVerifier.java @@ -196,9 +196,11 @@ private VerificationResult newVerificationSuccessResult() { private boolean verifyHMAC(Map values, String[] header) throws IOException { try { String actualHMAC = values.get(HEADER_HMAC); - String expectedHMAC = hmacCalculator.calculate(dataToSign(logger, values, dropExtraHeaders(header))); - if (!actualHMAC.equals(expectedHMAC)) { - logger.trace("The HMAC is not valid. Expected : {} Found : {}", expectedHMAC, actualHMAC); + String[] columns = dropExtraHeaders(header); + // A row that a previous version wrote has the HMAC of its values concatenated. + if (!hmacCalculator.verify(actualHMAC, dataToSign(logger, values, columns), + concatenatedValues(values, columns))) { + logger.trace("The HMAC is not valid. Found : {}", actualHMAC); return false; } else { lastHMAC = actualHMAC; diff --git a/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/HmacCalculator.java b/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/HmacCalculator.java index db7b1e38af..cfa712a227 100644 --- a/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/HmacCalculator.java +++ b/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/HmacCalculator.java @@ -65,6 +65,36 @@ public void setCurrentKey(byte[] bytes) { * @throws SignatureException if the HMAC cannot be generated. */ public String calculate(byte[] data) throws SignatureException { + String result = hmac(data); + + // Compute the next key's iteration + computeNextKeyIteration(); + + return result; + } + + /** + * Tells whether the HMAC of one of the given data is the expected one. The current key computes the HMAC of + * each data, then the next key's iteration is computed once, as {@link #calculate(byte[])} does. + * + * @param expectedHmac the expected HMAC, base64 encoded + * @param data the data that the expected HMAC may have been calculated for, in the order they are tried + * @return {@code true} if the HMAC of one of the data is the expected one. + * @throws SignatureException if the HMAC cannot be generated. + */ + public boolean verify(String expectedHmac, byte[]... data) throws SignatureException { + boolean verified = false; + for (int i = 0; i < data.length && !verified; i++) { + verified = hmac(data[i]).equals(expectedHmac); + } + + // Compute the next key's iteration + computeNextKeyIteration(); + + return verified; + } + + private String hmac(byte[] data) throws SignatureException { try { mac.reset(); mac.init(currentKey); @@ -73,12 +103,7 @@ public String calculate(byte[] data) throws SignatureException { byte[] rawHmac = mac.doFinal(data); // base64-encode the hmac - String result = Base64.encode(rawHmac); - - // Compute the next key's iteration - computeNextKeyIteration(); - - return result; + return Base64.encode(rawHmac); } catch (InvalidKeyException | IllegalStateException e) { throw new SignatureException("Failed to generate HMAC : " + e.getMessage()); } diff --git a/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/CsvSecureVerifierTest.java b/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/CsvSecureVerifierTest.java index 071cbe03b0..869db591fe 100644 --- a/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/CsvSecureVerifierTest.java +++ b/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/CsvSecureVerifierTest.java @@ -16,14 +16,22 @@ */ package org.forgerock.audit.handlers.csv; +import static java.nio.charset.StandardCharsets.UTF_8; import static org.assertj.core.api.Assertions.assertThat; +import static org.forgerock.audit.handlers.csv.SecureCsvWriterTest.KEYSTORE_FILENAME; +import static org.forgerock.audit.handlers.csv.SecureCsvWriterTest.KEYSTORE_PASSWORD; import java.io.File; import java.nio.file.Files; +import java.util.HashMap; +import java.util.Map; +import java.util.Random; +import org.forgerock.audit.handlers.csv.CsvSecureVerifier.VerificationResult; import org.forgerock.audit.secure.JcaKeyStoreHandler; import org.forgerock.audit.secure.KeyStoreHandlerDecorator; import org.forgerock.audit.secure.KeyStoreSecureStorage; +import org.forgerock.util.encode.Base64; import org.supercsv.prefs.CsvPreference; import org.testng.annotations.DataProvider; import org.testng.annotations.Test; @@ -45,6 +53,73 @@ public void shouldVerifyValidFile() throws Exception { assertThat(csvVerifier.verify().hasPassedVerification()).isTrue(); } + @Test + public void shouldVerifyFileWithHmacOfConcatenatedValues() throws Exception { + // written before the HMAC of a row covered the column names and where each value ends + File csvFile = new File("src/test/resources/secureCsvLegacyHmac.csv"); + KeyStoreHandlerDecorator keyStoreHandler = new KeyStoreHandlerDecorator( + new JcaKeyStoreHandler(CsvSecureConstants.KEYSTORE_TYPE, TRUSTSTORE_FILENAME, TRUSTSTORE_PASSWORD)); + CsvSecureVerifier csvVerifier = new CsvSecureVerifier(csvFile, CsvPreference.EXCEL_PREFERENCE, + new KeyStoreSecureStorage(keyStoreHandler, + keyStoreHandler.readPublicKeyFromKeyStore(KeyStoreSecureStorage.ENTRY_SIGNATURE))); + VerificationResult verificationResult = csvVerifier.verify(); + assertThat(verificationResult.hasPassedVerification()).as(verificationResult.getFailureReason()).isTrue(); + } + + @DataProvider + public Object[][] editsOfSignedData() { + return new Object[][] { + // Edits that keep the values concatenated, which the HMAC of a row covered before: + // characters moved across a column boundary + { "\nbob,x,", "\nbo,bx,", "The HMac at row 2 is not correct." }, + // a value moved into the empty neighbouring column + { "\n,bob,", "\nbob,,", "The HMac at row 3 is not correct." }, + // the column names swapped in the header + { "first,second,", "second,first,", "The HMac at row 2 is not correct." }, + // The row as one value that holds the data its HMAC covers, but the leading byte: the values concatenated + // are these data, which the verifier would accept if the data did not start with a byte UTF-8 never holds. + { "\nbob,x,", "\n\u0000\u0000\u0000\u0005first\u0000\u0000\u0000\u0003bob" + + "\u0000\u0000\u0000\u0006second\u0000\u0000\u0000\u0001x,,", + "The HMac at row 2 is not correct." }, + // Characters moved across a column boundary, next to a value that holds the next column name after its + // length: the names alone would tell where a value ends, but for such a value. + { "\na\u0000\u0000\u0000\u0006secondb,c,", "\na,b\u0000\u0000\u0000\u0006secondc,", + "The HMac at row 4 is not correct." }, + // a column name removed from the header, which the CSV reader reads as null + { "first,second,", ",second,", "The HMac at row 2 is not correct." } + }; + } + + @Test(dataProvider = "editsOfSignedData") + public void shouldNotVerifyEditOfSignedData(String original, String edited, String failureReason) + throws Exception { + final File csvFile = writeSecureCsvFile(new String[] { "bob", "x" }, new String[] { null, "bob" }, + new String[] { "a\u0000\u0000\u0000\u0006secondb", "c" }); + VerificationResult verificationResult = verifySecureCsvFile(csvFile); + assertThat(verificationResult.hasPassedVerification()).as(verificationResult.getFailureReason()).isTrue(); + + final String content = new String(Files.readAllBytes(csvFile.toPath()), UTF_8); + assertThat(content.indexOf(original)).as(original).isNotNegative().isEqualTo(content.lastIndexOf(original)); + Files.write(csvFile.toPath(), content.replace(original, edited).getBytes(UTF_8)); + + verificationResult = verifySecureCsvFile(csvFile); + assertThat(verificationResult.hasPassedVerification()).isFalse(); + assertThat(verificationResult.getFailureReason()).isEqualTo(failureReason); + } + + @Test + public void shouldNotVerifyRowWithoutHmac() throws Exception { + final File csvFile = writeSecureCsvFile(new String[] { "bob", "x" }); + final String content = new String(Files.readAllBytes(csvFile.toPath()), UTF_8); + final String edited = content.replaceFirst("\nbob,x,[^,\r\n]+,", "\nbob,x,,"); + assertThat(edited).isNotEqualTo(content); + Files.write(csvFile.toPath(), edited.getBytes(UTF_8)); + + final VerificationResult verificationResult = verifySecureCsvFile(csvFile); + assertThat(verificationResult.hasPassedVerification()).isFalse(); + assertThat(verificationResult.getFailureReason()).isEqualTo("The HMac at row 2 is not correct."); + } + @Test(dataProvider = "invalidContent") public void shouldNotVerify(String filename) throws Exception { File csvFile = new File(filename); @@ -84,4 +159,45 @@ public Object[][] invalidContent() { { "src/test/resources/secureCsvInvalidMissingFinalSignature.csv" } }; } + + /** Writes a tamper evident file with the columns first and second, one row per array of values. */ + private static File writeSecureCsvFile(String[]... rows) throws Exception { + final String[] header = { "first", "second" }; + final File logDirectory = Files.createTempDirectory("CsvSecureVerifierTest").toFile(); + final File csvFile = new File(logDirectory, "tamper-evident.csv"); + final CsvAuditEventHandlerConfiguration config = new CsvAuditEventHandlerConfiguration(); + config.getSecurity().setEnabled(true); + // leave the final signature to close() + config.getSecurity().setSignatureInterval("5 minutes"); + try (SecureCsvWriter writer = new SecureCsvWriter(csvFile, header, CsvPreference.EXCEL_PREFERENCE, config, + signatureKeyStore(), new Random(42))) { + for (String[] row : rows) { + final Map values = new HashMap<>(); + for (int i = 0; i < header.length; i++) { + if (row[i] != null) { + values.put(header[i], row[i]); + } + } + writer.writeEvent(values); + } + } + return csvFile; + } + + /** Verifies a file that {@link #writeSecureCsvFile} wrote, with its own keystore. */ + private static VerificationResult verifySecureCsvFile(File csvFile) throws Exception { + final KeyStoreHandlerDecorator signatureKeyStore = signatureKeyStore(); + final String password = Base64.encode( + signatureKeyStore.readSecretKeyFromKeyStore(CsvSecureConstants.ENTRY_PASSWORD).getEncoded()); + final KeyStoreSecureStorage secureStorage = new KeyStoreSecureStorage( + new JcaKeyStoreHandler(CsvSecureConstants.KEYSTORE_TYPE, + CsvSecureUtils.keyStoreFile(csvFile).getPath(), password), + signatureKeyStore.readPublicKeyFromKeyStore(KeyStoreSecureStorage.ENTRY_SIGNATURE)); + return new CsvSecureVerifier(csvFile, CsvPreference.EXCEL_PREFERENCE, secureStorage).verify(); + } + + private static KeyStoreHandlerDecorator signatureKeyStore() throws Exception { + return new KeyStoreHandlerDecorator( + new JcaKeyStoreHandler(CsvSecureConstants.KEYSTORE_TYPE, KEYSTORE_FILENAME, KEYSTORE_PASSWORD)); + } } diff --git a/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/HmacCalculatorTest.java b/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/HmacCalculatorTest.java index 1d1d65d9ab..9e124eab7e 100644 --- a/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/HmacCalculatorTest.java +++ b/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/HmacCalculatorTest.java @@ -12,6 +12,7 @@ * information: "Portions copyright [year] [name of copyright owner]". * * Copyright 2015 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC. */ package org.forgerock.audit.handlers.csv; @@ -58,4 +59,20 @@ public void shouldCalculateHMAC() throws Exception { assertThat(base64HMAC).isEqualTo("xDSrLEfpe3V1t5Y9y2I4bCIUIdWcQKum9HiVDHXfHFQ="); } + @Test + public void shouldVerifyHMACOfOneOfTheDataWithOneKey() throws Exception { + HmacCalculator hmacCalculator = new HmacCalculator("HmacSHA256"); + hmacCalculator.setCurrentKey("forgerock".getBytes(StandardCharsets.UTF_8)); + + byte[] other = "rock".getBytes(StandardCharsets.UTF_8); + byte[] data = "rockstar".getBytes(StandardCharsets.UTF_8); + + // The HMACs of data with the first, second and fourth keys, the first two as in shouldCalculateHMAC. + // A match on the second data, a match on the first data and a mismatch each update the key once. + assertThat(hmacCalculator.verify("m9Yir8xEvAS4leA6IZGJQ++OGkyMMHnf334N94AlVIs=", other, data)).isTrue(); + assertThat(hmacCalculator.verify("xDSrLEfpe3V1t5Y9y2I4bCIUIdWcQKum9HiVDHXfHFQ=", data, other)).isTrue(); + assertThat(hmacCalculator.verify(null, data, other)).isFalse(); + assertThat(hmacCalculator.verify("vDQb23X5yYDWoL7NcWmSGzrHBBYNWwtvA9w6oGOq58o=", data)).isTrue(); + } + } 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 4b2ef8bdb3..124f528f04 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 @@ -37,6 +37,7 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.Paths; import java.nio.file.StandardOpenOption; import java.security.KeyStore; import java.security.PublicKey; @@ -231,6 +232,36 @@ public void shouldGeneratePeriodicallySignature() throws Exception { new File("target/test-classes/shouldGeneratePeriodicallySignature-expected.txt"))); } + @Test + public void shouldResumeFileWithHmacOfConcatenatedValues() throws Exception { + final Path logDirectory = Files.createTempDirectory("SecureCsvWriterTest"); + final File actual = new File(logDirectory.toFile(), "tamper-evident-test.csv"); + // written before the HMAC of a row covered the column names and where each value ends + Files.copy(Paths.get("target/test-classes/secureCsvLegacyHmac.csv"), actual.toPath()); + // the keystore of the file holds the keys that the truststore of CsvSecureVerifierTest holds + final KeyStoreHandlerDecorator truststore = new KeyStoreHandlerDecorator(new JcaKeyStoreHandler(KEYSTORE_TYPE, + CsvSecureVerifierTest.TRUSTSTORE_FILENAME, CsvSecureVerifierTest.TRUSTSTORE_PASSWORD)); + final String keystorePassword = + Base64.encode(keyStoreHandler.readSecretKeyFromKeyStore(ENTRY_PASSWORD).getEncoded()); + final PublicKey publicKey = keyStoreHandler.readPublicKeyFromKeyStore(KeyStoreSecureStorage.ENTRY_SIGNATURE); + final SecureStorage fileSecureStorage = new KeyStoreSecureStorage( + new JcaKeyStoreHandler(KEYSTORE_TYPE, actual.getPath() + ".keystore", keystorePassword), publicKey); + fileSecureStorage.writeInitialKey(truststore.readSecretKeyFromKeyStore(ENTRY_INITIAL_KEY)); + fileSecureStorage.writeCurrentKey(truststore.readSecretKeyFromKeyStore(ENTRY_CURRENT_KEY)); + + try (SecureCsvWriter secureCsvWriter = new SecureCsvWriter( + actual, new String[]{"FOO"}, CsvPreference.EXCEL_PREFERENCE, + createBasicSecureConfig("5 minutes"), keyStoreHandler, random)) { + secureCsvWriter.writeEvent(singletonMap("FOO", "resumed")); + } + + final VerificationResult verificationResult = new CsvSecureVerifier(actual, CsvPreference.EXCEL_PREFERENCE, + new KeyStoreSecureStorage(new JcaKeyStoreHandler(KEYSTORE_TYPE, actual.getPath() + ".keystore", + keystorePassword), publicKey)).verify(); + assertThat(verificationResult.hasPassedVerification()).as(verificationResult.getFailureReason()).isTrue(); + assertThat(contentOf(actual)).containsPattern("(?m)^quix,").containsPattern("(?m)^resumed,"); + } + @Test public void shouldStopSchedulerAndCloseFileWhenFinalSignatureFails() throws Exception { final Path logDirectory = Files.createTempDirectory("SecureCsvWriterTest"); diff --git a/commons/audit/handler-csv/src/test/resources/secureCsvLegacyHmac.csv b/commons/audit/handler-csv/src/test/resources/secureCsvLegacyHmac.csv new file mode 100644 index 0000000000..605283f0da --- /dev/null +++ b/commons/audit/handler-csv/src/test/resources/secureCsvLegacyHmac.csv @@ -0,0 +1,5 @@ +FOO,HMAC,SIGNATURE +bar,uLkBIiPY0+yyseXNACJX5SBwqV4RDSN8Ab8Jz7c3cYI=, +,,BZJxI9CrZhjPkuOZNFv+qBR1+PYk9y/sZMAZcvuhfXkbYCZ8Nu6g6OHxWxaPdEJn3aiRGR7e0IVD9DfeGUrI02wSpLCkOZCNv1KdsEHadO4NbTQvnOyN16RqX2C85aHjTgVBbpLr88ZZAxEU99bE+jocHNci7n3dXSoS2RnC1lZyqqiiyTlSh9J3OjpizfBxhc+R5B+JKKnGee2gArgF8PHgqcRV2cYL1r5lmEnwk4i0vcHadp1gNfH3UiWsa8u28xBHtecOm2+49l+t+HbIfTssSJVxv35ISNgOUJfofddq30ECTCVhk9j277e3F/F4sjNeMP7iJtDGMUmUolp6Gg== +quix,taHEQat9MgErQvhG1iRGSJQPfe1SzIemjSyBtB5+hfU=, +,,Q7/AxkQixVpyRfDnCv8Sj36Tn55cWfjNHZ7+qjX+BlQSmNf5B4M1tB2EOu+B+pPccOaB2fvooDtpAMo7E9Qo0tDJtX0xiV2D/sGY5gHNS/UD8gX0gaFafJsP9bBcE/Zt1u1zGPKlqlot4V8+ke6clW8FrjdC9AQGW9nRiAIKHEGDjv8+g0vvMy4PrczmrIaDLskkFcU+4uPqsAJMev6mg/JA+3Dwd5Htfh0c53OZe6lxI+MYm2nFnF81U/3Nr2/mT/rJDkFBJEGkLcUgjXhckXX4WVuhzS92bYJfrE3JmKcgYSrmu5ajtNiDAh1/RjHGS8cGpKLnZ4ekQDr4u253AQ== diff --git a/commons/audit/handler-csv/src/test/resources/shouldGenerateHMACColumn-expected.txt b/commons/audit/handler-csv/src/test/resources/shouldGenerateHMACColumn-expected.txt index 55790108f3..85b16ab05a 100644 --- a/commons/audit/handler-csv/src/test/resources/shouldGenerateHMACColumn-expected.txt +++ b/commons/audit/handler-csv/src/test/resources/shouldGenerateHMACColumn-expected.txt @@ -1,4 +1,4 @@ FOO,HMAC,SIGNATURE -bar,uLkBIiPY0+yyseXNACJX5SBwqV4RDSN8Ab8Jz7c3cYI=, -quix,taHEQat9MgErQvhG1iRGSJQPfe1SzIemjSyBtB5+hfU=, -,,g0dSW8i7orW7Mb4voYoAGI+v1Lqp0zsEZGBZ6CWSpcjeswHGgXho9HggqTQgT3nnezj/hl6djnePg2LrIBCCkDGpuWAIXerMpyJO0eySSWi8cdNckyMnZZNU3YgaTmmg16mcwn1CQ9O8XO4a9suAovvAmq+6iVMgjK05wKAZORLq01MqIPCxsH9xaoUgBIVDXJcmgu5UlYAYPjcc4tgcAFx9Sph3M697VyI2iyZuD5fvsho7kJZTAqVF4ijRlQ0WHQGWJXKLcQ6OJeBARRPSP9KXODTRMmtJHCMcz6eTclAR5zkGxso+nDXO3dlzwSruEoZxNcV4llAzLH7gvgbfiQ== +bar,L1QgQJehhDbP8DQWdHh2l4czH9EamrzjzZ7MHjFxyAY=, +quix,96AsMYOd6jt5bRo2UtX1GKN7891ZFFe1iuz4chWK7B4=, +,,UAx5ihDeksfv+nbLB7F6xHQUDCyzMemGrXR8huJuh0IPqKKt6mu9NdkeRYgbzeRQkUL6m1WKGl+TvDYa8oaz5WoM6JyrqGapV9lKOQ3VdfoKGmmIbkhatYPcSMyY8kXlUw/ous7VIEZ+lY+i1IgHiA+fSdGHZJSYDjdl1kq9OL+BbYlZEur8Olhu10GEim/otwG18OQl6ZRBWeXz/atKqw7GRKAVa2RDlI2I/16/6FLkxyAr+nXk4hOSGRwJZK1IEcEgOQyfo0MeeidhLZCkRrcK9BpLeLWrSod9nURaMoZTKFMaUthOCtQr/dLMSXUOv8IloHkBkQrAtjnYb2ILjg== diff --git a/commons/audit/handler-csv/src/test/resources/shouldGeneratePeriodicallySignature-expected.txt b/commons/audit/handler-csv/src/test/resources/shouldGeneratePeriodicallySignature-expected.txt index 605283f0da..b4965204f9 100644 --- a/commons/audit/handler-csv/src/test/resources/shouldGeneratePeriodicallySignature-expected.txt +++ b/commons/audit/handler-csv/src/test/resources/shouldGeneratePeriodicallySignature-expected.txt @@ -1,5 +1,5 @@ FOO,HMAC,SIGNATURE -bar,uLkBIiPY0+yyseXNACJX5SBwqV4RDSN8Ab8Jz7c3cYI=, -,,BZJxI9CrZhjPkuOZNFv+qBR1+PYk9y/sZMAZcvuhfXkbYCZ8Nu6g6OHxWxaPdEJn3aiRGR7e0IVD9DfeGUrI02wSpLCkOZCNv1KdsEHadO4NbTQvnOyN16RqX2C85aHjTgVBbpLr88ZZAxEU99bE+jocHNci7n3dXSoS2RnC1lZyqqiiyTlSh9J3OjpizfBxhc+R5B+JKKnGee2gArgF8PHgqcRV2cYL1r5lmEnwk4i0vcHadp1gNfH3UiWsa8u28xBHtecOm2+49l+t+HbIfTssSJVxv35ISNgOUJfofddq30ECTCVhk9j277e3F/F4sjNeMP7iJtDGMUmUolp6Gg== -quix,taHEQat9MgErQvhG1iRGSJQPfe1SzIemjSyBtB5+hfU=, -,,Q7/AxkQixVpyRfDnCv8Sj36Tn55cWfjNHZ7+qjX+BlQSmNf5B4M1tB2EOu+B+pPccOaB2fvooDtpAMo7E9Qo0tDJtX0xiV2D/sGY5gHNS/UD8gX0gaFafJsP9bBcE/Zt1u1zGPKlqlot4V8+ke6clW8FrjdC9AQGW9nRiAIKHEGDjv8+g0vvMy4PrczmrIaDLskkFcU+4uPqsAJMev6mg/JA+3Dwd5Htfh0c53OZe6lxI+MYm2nFnF81U/3Nr2/mT/rJDkFBJEGkLcUgjXhckXX4WVuhzS92bYJfrE3JmKcgYSrmu5ajtNiDAh1/RjHGS8cGpKLnZ4ekQDr4u253AQ== +bar,L1QgQJehhDbP8DQWdHh2l4czH9EamrzjzZ7MHjFxyAY=, +,,IU9/MzHdMKqAb2bbhA552djuHapXO3SElBXffucIbiN6qeUYwCCi/YKK7vKmk51WBeKKEw9zZ9kf8188ie2wubYX/sWtAFclTOpbUmLNHoiOzyxrQ3vI3tT+lAKEriDBRzPk+EyS1NLfs5E7oyxMWd1I1WYftjNZFugFszRXtVZ94PO6QwiaUpZh1pRZbZKo0YXFUTzXQTINoqXryGBdxT03xlD2DQHdnHnLG4+9ncBjXGhtXdWVwyjcjl5jDQVR7SIhBt5O8Uj7k0lITTyc6oPbzhoFIfMH37pyoDYi0Y852PCkqPB/2JB2wKBYDRfoXNOCWD+DTM8lDjd3f2FJAA== +quix,96AsMYOd6jt5bRo2UtX1GKN7891ZFFe1iuz4chWK7B4=, +,,LpFoIXPF209t/+r8f7jzWIy5/h684rn9G9StDlh5oNcxdR7yBbccwhLpIbEtc4Hi0jLdnLkW9aTbwDUY0oJZtuEChCQc6mRFrucYFQ38z6iWr9W+R+Igqm0HJwiIqqB3JOsTkeGivDL873nM1JBtLQxHWvW2UeuJsF/X88MxBksAPzGrkXMRB5KcayoOfaN8UfGdqDBP1eptGrO+tBzuRSPmn/oYFlMbSnoQ7qQRgIL3WReZn2KmbzJ1SOCWJCT09B/alFEGppKPBe1CryvDJLTp11stNCZFJVI/x9wCJ14ek1wmba9AjZdYQLxaGN+DzBQeoa/h4M5v3TvKxnO0eA== diff --git a/commons/audit/handler-csv/src/test/resources/shouldGeneratePeriodicallySignature-partial.txt b/commons/audit/handler-csv/src/test/resources/shouldGeneratePeriodicallySignature-partial.txt index 3e4fe86724..5d89113199 100644 --- a/commons/audit/handler-csv/src/test/resources/shouldGeneratePeriodicallySignature-partial.txt +++ b/commons/audit/handler-csv/src/test/resources/shouldGeneratePeriodicallySignature-partial.txt @@ -1,3 +1,3 @@ FOO,HMAC,SIGNATURE -bar,uLkBIiPY0+yyseXNACJX5SBwqV4RDSN8Ab8Jz7c3cYI=, -,,BZJxI9CrZhjPkuOZNFv+qBR1+PYk9y/sZMAZcvuhfXkbYCZ8Nu6g6OHxWxaPdEJn3aiRGR7e0IVD9DfeGUrI02wSpLCkOZCNv1KdsEHadO4NbTQvnOyN16RqX2C85aHjTgVBbpLr88ZZAxEU99bE+jocHNci7n3dXSoS2RnC1lZyqqiiyTlSh9J3OjpizfBxhc+R5B+JKKnGee2gArgF8PHgqcRV2cYL1r5lmEnwk4i0vcHadp1gNfH3UiWsa8u28xBHtecOm2+49l+t+HbIfTssSJVxv35ISNgOUJfofddq30ECTCVhk9j277e3F/F4sjNeMP7iJtDGMUmUolp6Gg== +bar,L1QgQJehhDbP8DQWdHh2l4czH9EamrzjzZ7MHjFxyAY=, +,,IU9/MzHdMKqAb2bbhA552djuHapXO3SElBXffucIbiN6qeUYwCCi/YKK7vKmk51WBeKKEw9zZ9kf8188ie2wubYX/sWtAFclTOpbUmLNHoiOzyxrQ3vI3tT+lAKEriDBRzPk+EyS1NLfs5E7oyxMWd1I1WYftjNZFugFszRXtVZ94PO6QwiaUpZh1pRZbZKo0YXFUTzXQTINoqXryGBdxT03xlD2DQHdnHnLG4+9ncBjXGhtXdWVwyjcjl5jDQVR7SIhBt5O8Uj7k0lITTyc6oPbzhoFIfMH37pyoDYi0Y852PCkqPB/2JB2wKBYDRfoXNOCWD+DTM8lDjd3f2FJAA== From 5b56c7362b8349b88f6127e7ebc016418bde1c5c Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Sun, 18 Oct 2026 12:52:00 +0300 Subject: [PATCH 2/2] [#370] Tamper-evident CSV audit: sign the names and the values as the CSV reader reads them back Super CSV writes a CR, a CRLF or a LF in a quoted name or value as the end of line symbols and reads each line break back as a LF, so a row with a CR failed verification and its file could not be resumed. The row HMAC now signs each CR and CRLF as a LF, and the resume compares the column names the same way. Pin the signed data in CsvSecureUtilsTest, and reword the Javadoc of HmacCalculator.verify. --- .../audit/handlers/csv/CsvSecureUtils.java | 28 ++++++-- .../audit/handlers/csv/HmacCalculator.java | 5 +- .../audit/handlers/csv/SecureCsvWriter.java | 6 +- .../handlers/csv/CsvSecureUtilsTest.java | 69 +++++++++++++++++++ .../handlers/csv/CsvSecureVerifierTest.java | 37 +++++++++- 5 files changed, 132 insertions(+), 13 deletions(-) create mode 100644 commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/CsvSecureUtilsTest.java diff --git a/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/CsvSecureUtils.java b/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/CsvSecureUtils.java index 8cd3532d07..ebc9df1d48 100644 --- a/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/CsvSecureUtils.java +++ b/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/CsvSecureUtils.java @@ -23,6 +23,7 @@ import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.Map; +import java.util.regex.Pattern; import org.forgerock.util.encode.Base64; import org.slf4j.Logger; @@ -39,6 +40,12 @@ final class CsvSecureUtils { */ private static final int DATA_TO_SIGN_MARKER = 0xFF; + /** + * A line break that the CSV writer writes as its end of line symbols: a CR, a CRLF, or a LF, which the CSV reader + * reads back as a LF. The writer writes a CR followed by a LF as one line break. + */ + private static final Pattern LINE_BREAK = Pattern.compile("\r\n?"); + /** * Obtain the data to sign. * @param lastSignature The last signature. @@ -80,10 +87,11 @@ private static byte[] concat(byte[]... arrays) { } /** - * Get the data to sign: the byte 0xFF, then the name and the value of each column in UTF-8, each one after the - * number of its bytes as a big-endian int. + * Get the data to sign: the byte 0xFF, then the name and the value of each column, as the CSV reader reads them + * back, in UTF-8, each one after the number of its bytes as a big-endian int. *

- * The CSV reader reads an empty name or value as {@code null}, so both are signed as empty. + * The CSV reader reads an empty name or value as {@code null}, so both are signed as empty, and a line break + * as a LF, so a CR or a CRLF is signed as a LF. * @param logger the value of logger * @param values the value of values * @param nameMapping the value of nameMapping @@ -93,8 +101,8 @@ static byte[] dataToSign(Logger logger, Map values, String... nameMap data.write(DATA_TO_SIGN_MARKER); for (String h : nameMapping) { final Object value = values.get(h); - writeLengthAndBytes(data, h != null ? h : ""); - writeLengthAndBytes(data, value != null ? value.toString() : ""); + writeLengthAndBytes(data, readBack(h)); + writeLengthAndBytes(data, readBack(value != null ? value.toString() : null)); } return data.toByteArray(); } @@ -105,6 +113,16 @@ private static void writeLengthAndBytes(ByteArrayOutputStream data, String field data.write(bytes, 0, bytes.length); } + /** + * Returns a name or a value as the CSV reader reads it back once written: empty if it is {@code null} or empty, + * with each line break as a LF. + * @param field the name or the value, may be {@code null} + * @return the name or the value as the CSV reader reads it back, empty for {@code null} + */ + static String readBack(String field) { + return field != null ? LINE_BREAK.matcher(field).replaceAll("\n") : ""; + } + /** * Get the data that the HMAC of a row covered before {@link #dataToSign(Logger, Map, String...)}: the values * concatenated, which do not tell where a value ends, nor its column. diff --git a/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/HmacCalculator.java b/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/HmacCalculator.java index cfa712a227..de748cc97d 100644 --- a/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/HmacCalculator.java +++ b/commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/HmacCalculator.java @@ -74,8 +74,9 @@ public String calculate(byte[] data) throws SignatureException { } /** - * Tells whether the HMAC of one of the given data is the expected one. The current key computes the HMAC of - * each data, then the next key's iteration is computed once, as {@link #calculate(byte[])} does. + * Tells whether the HMAC of one of the given data is the expected one. With the current key, computes the HMAC + * of each data in turn until one is the expected HMAC, then computes the next key's iteration once, as + * {@link #calculate(byte[])} does. * * @param expectedHmac the expected HMAC, base64 encoded * @param data the data that the expected HMAC may have been calculated for, in the order they are tried 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 0e57b3a2b8..8e4c016263 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 @@ -23,6 +23,7 @@ import static org.forgerock.audit.handlers.csv.CsvSecureConstants.HEADER_SIGNATURE; import static org.forgerock.audit.handlers.csv.CsvSecureConstants.SIGNATURE_ALGORITHM; import static org.forgerock.audit.handlers.csv.CsvSecureUtils.dataToSign; +import static org.forgerock.audit.handlers.csv.CsvSecureUtils.readBack; import static org.forgerock.util.Reject.checkNotNull; import java.io.File; @@ -169,15 +170,14 @@ class SecureCsvWriter implements CsvWriter, RolloverLifecycleHook { throw new IOException("The CSV file was tampered: " + verificationResult.getFailureReason()); } - // Assert that the 2 headers are equal. + // Assert that the 2 headers are equal, as the CSV reader reads them back. final String[] actualHeaders = verifier.getHeaders(); if (actualHeaders != null) { if (actualHeaders.length != headers.length) { throw new IOException("Resuming an existing CSV file but the headers do not match."); } for (int idx = 0; idx < actualHeaders.length; idx++) { - // Super CSV reads an empty column name as null, the expected headers hold none - if (!headers[idx].equals(actualHeaders[idx])) { + if (!readBack(actualHeaders[idx]).equals(readBack(headers[idx]))) { throw new IOException("Resuming an existing CSV file but the headers do not match."); } } diff --git a/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/CsvSecureUtilsTest.java b/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/CsvSecureUtilsTest.java new file mode 100644 index 0000000000..0719d1a28c --- /dev/null +++ b/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/CsvSecureUtilsTest.java @@ -0,0 +1,69 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ +package org.forgerock.audit.handlers.csv; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.nio.charset.StandardCharsets; +import java.util.HashMap; +import java.util.Map; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.testng.annotations.Test; + +@SuppressWarnings("javadoc") +public class CsvSecureUtilsTest { + + private static final Logger logger = LoggerFactory.getLogger(CsvSecureUtilsTest.class); + + @Test + public void shouldConcatenateNonNullValuesInHeaderOrder() { + // the data that the HMAC of a row covered before, which the files written then still need + Map values = new HashMap<>(); + values.put("a", "x"); + values.put("b", null); + values.put("c", "z"); + + assertThat(CsvSecureUtils.concatenatedValues(values, "c", "b", "a")) + .isEqualTo("zx".getBytes(StandardCharsets.UTF_8)); + } + + @Test + public void shouldSignUtf8LengthsAndEmptyNamesAndValues() { + Map values = new HashMap<>(); + values.put("a", "é"); + + assertThat(CsvSecureUtils.dataToSign(logger, values, "a", "b", null)) + .isEqualTo(new byte[] { + (byte) 0xFF, + 0, 0, 0, 1, 'a', 0, 0, 0, 2, (byte) 0xC3, (byte) 0xA9, + 0, 0, 0, 1, 'b', 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0 }); + } + + @Test + public void shouldSignLineBreaksAsLf() { + Map values = new HashMap<>(); + values.put("a\rb", "c\r\nd\r\r\ne\nf"); + + assertThat(CsvSecureUtils.dataToSign(logger, values, "a\rb")) + .isEqualTo(new byte[] { + (byte) 0xFF, + 0, 0, 0, 3, 'a', '\n', 'b', + 0, 0, 0, 8, 'c', '\n', 'd', '\n', '\n', 'e', '\n', 'f' }); + } +} diff --git a/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/CsvSecureVerifierTest.java b/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/CsvSecureVerifierTest.java index 869db591fe..68d1efdb9b 100644 --- a/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/CsvSecureVerifierTest.java +++ b/commons/audit/handler-csv/src/test/java/org/forgerock/audit/handlers/csv/CsvSecureVerifierTest.java @@ -120,6 +120,29 @@ public void shouldNotVerifyRowWithoutHmac() throws Exception { assertThat(verificationResult.getFailureReason()).isEqualTo("The HMac at row 2 is not correct."); } + @Test + public void shouldVerifyLineBreaksInValues() throws Exception { + // The CSV writer writes a CR, a CRLF and a LF in a value as its end of line symbols, read back as LF. + final File csvFile = writeSecureCsvFile(new String[] { "a\rb", "c\r\nd" }, new String[] { "e\nf", "g\r\r\nh" }); + + final VerificationResult verificationResult = verifySecureCsvFile(csvFile); + assertThat(verificationResult.hasPassedVerification()).as(verificationResult.getFailureReason()).isTrue(); + } + + @Test + public void shouldVerifyAndResumeFileWithLineBreaksInColumnNames() throws Exception { + final String[] header = { "first\rname", "second\r\nname" }; + final File csvFile = newSecureCsvFile(); + writeSecureCsvFile(csvFile, header, new String[] { "a", "b" }); + VerificationResult verificationResult = verifySecureCsvFile(csvFile); + assertThat(verificationResult.hasPassedVerification()).as(verificationResult.getFailureReason()).isTrue(); + + // the writer compares the names it reads back from the file with the names it writes + writeSecureCsvFile(csvFile, header, new String[] { "c", "d" }); + verificationResult = verifySecureCsvFile(csvFile); + assertThat(verificationResult.hasPassedVerification()).as(verificationResult.getFailureReason()).isTrue(); + } + @Test(dataProvider = "invalidContent") public void shouldNotVerify(String filename) throws Exception { File csvFile = new File(filename); @@ -162,9 +185,18 @@ public Object[][] invalidContent() { /** Writes a tamper evident file with the columns first and second, one row per array of values. */ private static File writeSecureCsvFile(String[]... rows) throws Exception { - final String[] header = { "first", "second" }; + final File csvFile = newSecureCsvFile(); + writeSecureCsvFile(csvFile, new String[] { "first", "second" }, rows); + return csvFile; + } + + private static File newSecureCsvFile() throws Exception { final File logDirectory = Files.createTempDirectory("CsvSecureVerifierTest").toFile(); - final File csvFile = new File(logDirectory, "tamper-evident.csv"); + return new File(logDirectory, "tamper-evident.csv"); + } + + /** Writes one row per array of values to a tamper evident file, which the writer resumes if it exists. */ + private static void writeSecureCsvFile(File csvFile, String[] header, String[]... rows) throws Exception { final CsvAuditEventHandlerConfiguration config = new CsvAuditEventHandlerConfiguration(); config.getSecurity().setEnabled(true); // leave the final signature to close() @@ -181,7 +213,6 @@ private static File writeSecureCsvFile(String[]... rows) throws Exception { writer.writeEvent(values); } } - return csvFile; } /** Verifies a file that {@link #writeSecureCsvFile} wrote, with its own keystore. */