[vharseko]

Fixes #344. Fixes #370.

Signed data

CsvSecureUtils.dataToSign() concatenated the values of a row, so the HMAC did not tell where a value ends, nor which column it is in. bob,x edited to bo,bx, or ,bob to bob,, kept the HMAC of the row, and CsvSecureVerifier accepted the edited file.

The HMAC of a row now covers the byte 0xFF, then, for each column in header order, the column name and the value as the CSV reader reads them back, in UTF-8, each after the number of its bytes as a big-endian int.

• The lengths fix where each value ends, empty values included, so the edits above change the signed data.
• The column names bind each value to its column. The header row has no HMAC of its own: swapping two names in it (userId,trackingId to trackingId,userId) kept every HMAC, and the verifier accepted a file whose values had silently changed columns. It now fails at the first row.
• Super CSV reads an empty column as null, quoted or not, so a file cannot hold the difference between null and "", and both are signed as an empty value. Signed differently, a "" handed to the writer would fail verification once read back as null. An empty column name, which is read as null too, is signed as an empty name.

Line breaks

Super CSV writes a CR, a CRLF or a LF in a name or a value as the end of line symbols, inside quotes, and reads every line break inside quotes back as a LF. The writer signed the value it was given, so an event with a CR in any field (x\ry or x\r\ny, read back as x\ny) made the file fail verification at its row, and the writer did not resume it (#370).

• The signed data hold each CR and CRLF of a name or a value as a LF, as the reader reads it back.
• When it resumes a file, the writer compares the column names it is given with the names it reads back in the same way. A name with a CR failed there with "the headers do not match".
• The rows that the previous version signed with a CR still fail verification: the CR they signed is not in the file.

Files signed before

CsvSecureVerifier checks the HMAC of a row in the new format, then in the previous one, with the same key of the chain:

• Files and archives written by the previous version pass verification as before.
• A live file written by the previous version is resumed. The rows appended after the upgrade are in the new format, and the verifier accepts this mixed file.
• The rows signed before the upgrade keep the weakness of their format: only the rows written after it are protected.

UTF-8 never holds the leading 0xFF, so the concatenated values of a row never equal the signed data of a row in the new format, and the fallback cannot accept an edited row of the new format. Without that byte, a row rewritten as one value that holds the encoding of the original row would pass the check in the previous format.

Downgrade

The previous version does not verify a row in the new format ("The HMac at row N is not correct."), so its writer does not resume a live file that holds such rows. Going back to it needs the live file and its keystore set aside, and the files written in the new format verified with the new version.

Row without HMAC

A row with an empty HMAC and an empty SIGNATURE threw NullPointerException from CsvSecureVerifier.verify(). It now fails with "The HMac at row N is not correct.".

Tests

• CsvSecureVerifierTest.shouldNotVerifyEditOfSignedData, on a file that the writer writes and that passes verification before the edit:

  1. characters moved across a column boundary;
  2. a value moved into the empty neighbouring column;
  3. two column names swapped in the header;
  4. the row rewritten as one value that holds its encoding without the leading byte;
  5. characters moved across a column boundary, next to a value that holds the next column name after its length;
  6. a column name removed from the header.

  The first three passed verification before this change. Each part of the format is caught when removed alone: the leading byte by 4, the names by 3, the value lengths by 5.

• CsvSecureVerifierTest.shouldVerifyFileWithHmacOfConcatenatedValues: secureCsvLegacyHmac.csv, the file that SecureCsvWriterTest expected before this change, passes verification.

• SecureCsvWriterTest.shouldResumeFileWithHmacOfConcatenatedValues: the writer resumes that file and appends a row, and the mixed file passes verification.

• CsvSecureVerifierTest.shouldVerifyLineBreaksInValues: values that hold a CR, a CRLF, a LF and a CR before a CRLF pass verification. It failed at row 2 before this change.

• CsvSecureVerifierTest.shouldVerifyAndResumeFileWithLineBreaksInColumnNames: a file whose column names hold a CR and a CRLF passes verification, the writer resumes it, and the resumed file passes verification. Signing the line breaks as a LF alone leaves the resume failing on the names.

• CsvSecureUtilsTest pins the signed data, which the writer and the verifier share, so that a round trip cannot catch a change of them: the values concatenated in the previous format (null skipped, header order, no separator), the lengths in UTF-8 bytes, an empty name and an absent value, and the line breaks as a LF.

• HmacCalculatorTest.shouldVerifyHMACOfOneOfTheDataWithOneKey: a match on the first data, a match on the second one and a mismatch each move to the next key once, checked against HMACs computed separately.

• CsvSecureVerifierTest.shouldNotVerifyRowWithoutHmac.

• The expected files of SecureCsvWriterTest hold the new HMACs, which match HMACs computed separately from the format above. CsvSecureVerifierTest.shouldVerifyValidFile verifies the new expected file with the truststore.

Without the fallback to the previous format, both tests of that format fail. Each case of CsvSecureUtilsTest fails when the part of the format it pins is changed: a "null" or a "|" in the values concatenated, or their order reversed; a "null" for an absent value or an empty name, or the length in UTF-16 chars; the line breaks signed unchanged. audit/core and audit/handler-csv pass locally on JDK 11.

[maximthomas]

praise: The format fix sits where the bug is, and old files stay verifiable without weakening the new format.

• CsvSecureUtils.DATA_TO_SIGN_MARKER (0xFF) keeps the two formats disjoint, so the concatenatedValues fallback cannot accept an edited new-format row.
• HmacCalculator.verify advances the key once per row whichever format matches, so a mixed file keeps its chain aligned.
• The new cases run on all nine CI rows: handler-csv goes from 38 to 48 tests, all green.

---

issue (non-blocking): A non-ASCII column name fails archive verification when the JVM's default charset is not UTF-8.

commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/CsvSecureVerifier.java:185, :93, commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/CsvSecureUtils.java:96

RotatableWriter writes the file in UTF-8 (commons/audit/core/src/main/java/org/forgerock/audit/events/handlers/writers/RotatableWriter.java:353) and the HMAC covers UTF-8 bytes, but the verifier decodes the file with new InputStreamReader(in). Now that names are signed, a field name such as näme, read back on JDK 11 or 17 with a windows-1252 or US-ASCII default (Windows, Linux under the POSIX locale), re-signs to different bytes: every new-format row fails and an untampered archive reports FAIL. Probed on 11.0.25 and 17.0.11. JDK 18+ defaults to UTF-8 (JEP 400): checked on macOS JDK 21 only, not on Windows. Non-ASCII values already failed this way before this PR, and a non-ASCII name already failed the resume's header check (SecureCsvWriter.java:139-146); only the archive path is new. Reading as UTF-8 fixes all three:

import java.nio.charset.StandardCharsets;

        return new CsvMapReader(new BufferedReader(new InputStreamReader(in, StandardCharsets.UTF_8)), csvPreference);

CsvAuditEventHandler.java:347 and StandardCsvWriter.java:58 also read with the default charset (FileReader). They sign nothing and are left out.

---

suggestion (non-blocking): The fallback that keeps pre-upgrade files verifiable is pinned only by a one-column fixture with no empty cell.

commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/CsvSecureUtils.java:114, commons/audit/handler-csv/src/test/resources/secureCsvLegacyHmac.csv

secureCsvLegacyHmac.csv (header FOO,HMAC,SIGNATURE, values bar and quix) is the only legacy input. Three concatenatedValues mutants pass all 48 handler-csv tests: "null" appended for a missing cell, "|" between values, and columns in reverse order. Real pre-upgrade files have many columns and empty cells. Under any of these mutants they stop verifying and the writer refuses to resume. This case passes at the head and fails under each of the three mutants:

@Test
public void shouldConcatenateNonNullValuesInHeaderOrder() {
    Map<String, String> 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));
}

Pin: a new CsvSecureUtilsTest in org.forgerock.audit.handlers.csv, shared with the next comment.

---

suggestion (non-blocking): The new-format bytes for an absent value, an empty column name and a non-ASCII field are not pinned.

commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/CsvSecureUtils.java:91-106

The writer and the verifier share dataToSign, so round trips cannot detect a format change, and the expected files hold one ASCII column that is never empty. Three mutants pass all 48 tests: an absent value signed as "null", an empty name signed as "null", and the length written as the UTF-16 char count instead of the UTF-8 byte count that the Javadoc states. Files that this version writes would stop verifying after such a change. This case passes at the head and fails under each of the three mutants:

@Test
public void shouldSignUtf8LengthsAndEmptyNamesAndValues() {
    Map<String, String> values = new HashMap<>();
    values.put("a", "é");

    assertThat(CsvSecureUtils.dataToSign(LoggerFactory.getLogger(CsvSecureUtilsTest.class), 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 });
}

Imports for both cases: java.nio.charset.StandardCharsets, java.util.HashMap, java.util.Map, org.slf4j.LoggerFactory, org.testng.annotations.Test, static org.assertj.core.api.Assertions.assertThat. Measured with JDK 11 against the head's CsvSecureUtils, TestNG 6.9.8 and AssertJ 3.27.7, outside the Maven build: the head passes 2/2, and each of the six mutants fails.

---

suggestion (non-blocking): A column name that holds a CR fails archive verification.

commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/CsvSecureUtils.java:96, commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/CsvSecureVerifier.java:93

The writer signs the configured name, and the verifier signs the name that Super CSV 2.4.0 reads back. A CR inside a quoted header reads back as LF under every end-of-line setting: a\rb becomes a\nb. Quote, delimiter, spaces, tab and LF all round-trip (probed). A schema property key that holds \r therefore fails an untampered archive at row 2. The live-file resume already failed on such a name ("headers do not match"). No code change is needed. At most, add one sentence to the dataToSign Javadoc saying that a name must read back unchanged, which a name with a CR does not.

---

nitpick (non-blocking): The HmacCalculator.verify Javadoc says the HMAC of each data is computed, but the loop stops at the first match.

commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/HmacCalculator.java:77-78, :87

     * 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.

[vharseko]

@maximthomas, the review round is in 5c3c9dc, with the branch rebased on master.

issue (charset): Tracked as #336, and fixed on every path by #348. Reading as UTF-8 alone would break the other path: with rotation disabled, SecureCsvWriter writes through TextWriter.Stream, whose PrintWriter uses the platform charset, so a file with a non-ASCII value that verifies today on JDK 11–17 with a non-UTF-8 default would stop verifying. Left to #348, which touches other hunks of CsvSecureVerifier.

suggestion (legacy fallback): Added as CsvSecureUtilsTest.shouldConcatenateNonNullValuesInHeaderOrder.

suggestion (new-format bytes): Added as CsvSecureUtilsTest.shouldSignUtf8LengthsAndEmptyNamesAndValues.

suggestion (CR in a column name): Values had the same problem before this PR: Super CSV reads x\ry and x\r\ny back as x\ny, so an event with a CR in any field made the file fail verification, and the writer did not resume it. Filed as #370 and fixed here instead of documenting it: the signed data hold each CR and CRLF of a name or a value as a LF, as the reader reads it back, and the resume compares the column names the same way. CsvSecureUtilsTest.shouldSignLineBreaksAsLf pins the bytes, and CsvSecureVerifierTest.shouldVerifyLineBreaksInValues and shouldVerifyAndResumeFileWithLineBreaksInColumnNames cover the file. The rows that the previous version signed with a CR still fail verification: the CR they signed is not in the file.

nitpick: Reworded as proposed.

Each of the six mutants from the review fails one of the two proposed cases, and signing the line breaks unchanged fails shouldSignLineBreaksAsLf.

[maximthomas]

praise: The #370 fix signs what the CSV reader reads back, so a CR or a CRLF in a name or a value now verifies and resumes.

• CsvSecureUtils.readBack serves both dataToSign and the resume's name comparison (SecureCsvWriter.java:146), so the writer, the verifier and the resume agree on one form.
• shouldVerifyLineBreaksInValues and shouldVerifyAndResumeFileWithLineBreaksInColumnNames fail with "The HMac at row 2 is not correct." when the main hunks of 5c3c9dc are reverted, as the description says.
• handler-csv runs 57 tests at the head on the ubuntu JDK 27 and windows JDK 11 rows, 15 more than master, and all nine CI rows are green.