Repository navigation
[#346] Tamper-evident CSV audit: read and query a topic with no event yet - #354
Merged
vharseko merged 1 commit intoOct 18, 2026
Conversation
… topic with no event yet SecureCsvWriter writes the header with the first event, so the audit file of a tamper-evident topic is empty after startup, and after the rotate action of a topic without file rotation, until an event is published. getEntries() passed the null header of such a file to convertDotNotationToSlashes(), so readEvent() threw a NullPointerException out of the call and queryEvents() answered BadRequestException. A file without a header holds no event: readEvent() now answers NotFoundException, and queryEvents() answers no result.
vharseko
added
bug
tests
maximthomas
approved these changes
Oct 18, 2026
maximthomas
left a comment
maximthomas
left a comment
Contributor
There was a problem hiding this comment.
praise: The fix sits where the bug is, and the tests are shown to pin it.
- The null-header check is in
CsvAuditEventHandler.getEntries()(commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/CsvAuditEventHandler.java:539-543). BothreadEvent()andqueryEvents()go through that method, so one guard fixes both of them. leaveTamperEvidentFileWithNoEvent()asserts that the live fileisEmpty()before it reads. Neither test can pass against a file that already has a header.- With the
getEntries()hunk reverted, all four new instances ofCsvAuditEventHandlerTestfail: thereadEventrows withNullPointerException, thequeryEventsrows withBadRequestException. The other 15 tests in the class pass.
This was referenced Oct 18, 2026
vharseko
added a commit
that referenced
this pull request
Oct 19, 2026
…8 on every path (#348) Fixes #336. ## Charset `RotatableWriter` writes in UTF-8, and `CsvSecureUtils.dataToSign()` computes each HMAC over the UTF-8 bytes of the values. `TextWriter.Stream`, which writes when rotation is disabled, and `CsvSecureVerifier` used the platform default charset. With a default charset other than UTF-8 (JDK 11–17 on Windows, or `-Dfile.encoding`), an untampered file with a non-ASCII value failed verification with "The HMac at row N is not correct", the live file too, so the writer could not resume it. With rotation disabled, a character that the platform charset cannot encode, such as a Cyrillic letter in windows-1252, was written as `?`. Every reader and writer of the audit files now uses UTF-8: - `TextWriter.Stream` writes in UTF-8, as `RotatableWriter` does. In this repository only the two CSV writers use it. - `CsvSecureVerifier` reads in UTF-8. - `CsvAuditEventHandler` reads the audit file in UTF-8 for `readEvent` and `queryEvents`, which returned non-ASCII values garbled. - `StandardCsvWriter` reads the header of the file it resumes in UTF-8. A non-ASCII column name failed with "Resuming an existing CSV file but the headers do not match." ## Malformed input `CsvSecureVerifier` decodes with `UTF_8.newDecoder()`, which reports malformed input instead of replacing it with U+FFFD. A file that is not UTF-8 fails with "The file is not valid UTF-8" instead of an HMAC failure, and a byte sequence that is not UTF-8 can no longer pass for a U+FFFD that the writer logged. ## Upgrade On a host whose default charset is not UTF-8 (Windows, JDK 11–17, or `-Dfile.encoding`), the files that the previous version wrote with rotation disabled are in that charset: - A tamper-evident file with a non-ASCII value that this charset encodes (`ü` or `é` in windows-1252) passed verification before and now fails with "The file is not valid UTF-8". When its bytes happen to be valid UTF-8, as some sequences of a multi-byte charset such as MS932 or GBK are, it fails with "The HMac at row N is not correct." instead. This includes the live file, which the writer verifies before it resumes it. Until #341, this failure stops the startup of the whole audit service, not only of the topic: `SecureCsvWriter` wraps it in a `RuntimeException`, which neither `CsvAuditEventHandler.startup()` nor `AuditServiceImpl.startup()` catches. It also leaves the live file open (#340). - A plain CSV file is resumed as it is, so the rows appended after the upgrade are in UTF-8. `readEvent` and `queryEvents` read the rows written before with U+FFFD in place of the bytes that are not UTF-8, and a query no longer matches such a value. A file whose header holds a non-ASCII column name, which only a custom topic schema has, is not resumed: the topic fails with "Resuming an existing CSV file but the headers do not match." On such a host, before the upgrade, stop the service and move aside the files of each topic written with rotation disabled. For a tamper-evident topic, rename `tamper-evident-<topic>.csv` and `tamper-evident-<topic>.csv.keystore` to `tamper-evident-<topic>.csv<timestamp>` and `tamper-evident-<topic>.csv<timestamp>.keystore`, with `<timestamp>` in the format of the CLI's `--suffix` without `.gz` (`-yyyy.MM.dd-HH.mm.ss` by default, as in `tamper-evident-access.csv-2026.10.19-12.00.00`): the file is not compressed. Verify them on that host with the CLI of the previous version, which reads in the platform charset. Do not use the `rotate` action of a version up to 3.2.1 for such a topic: with rotation disabled, it renames only the CSV file, under a `.gz` name although it does not compress it, and the writer of the new file overwrites the keys of the archive in the keystore that it leaves; #332 fixes this in this version. For a plain CSV topic, the `rotate` action is enough. Files written with rotation enabled were in UTF-8 already: they failed verification on such a host, and now pass. On such a host, the files written with rotation disabled are now in UTF-8 too: a tool that reads them must decode them as UTF-8. Rolling back on such a host: the previous version reads in the platform charset, so it fails the tamper-evident files that this version wrote with rotation disabled and a non-ASCII value, the live file included. It reads the non-ASCII values of a plain CSV file that this version wrote with rotation disabled garbled, and does not resume such a file whose header holds a non-ASCII column name. Run the `rotate` action of this version before the rollback, and verify the archives written after the upgrade with the CLI of this version. `TextWriter.Stream` is public: audit-core exports `org.forgerock.audit.events.handlers.writers`. With a default charset other than UTF-8, a caller of `new TextWriter.Stream(out)` outside this repository now gets the UTF-8 bytes of a non-ASCII character, and `getBytesWritten()` counts them; a file that such a caller appends to then mixes two encodings. With a UTF-8 default charset, the output is unchanged. ## Tests - `CsvWriterTest.shouldWriteAndVerifyNonAsciiValuesInUtf8`, with rotation disabled and enabled: the file is in UTF-8 and passes verification. - `CsvWriterTest.shouldNotVerifyFileNotInUtf8`: a file re-encoded in windows-1252 fails with "The file is not valid UTF-8". - `CsvWriterTest.shouldNotResumeFileNotInUtf8`: `SecureCsvWriter` does not resume such a file. - `CsvWriterTest.shouldResumeCsvFileWithNonAsciiHeader`. - `CsvAuditEventHandlerTest.testReadingAuditLogEntryWithNonAsciiValue`. - The plain CSV case of the Upgrade section: `CsvAuditEventHandlerTest.testReadingAuditLogEntryNotInUtf8` resumes a file re-encoded in windows-1252, reads its old row back with U+FFFD and the row appended after the resume as written; `CsvWriterTest.shouldNotResumeCsvFileWithNonAsciiHeaderNotInUtf8` does not resume a file whose header, with a non-ASCII column name, it writes in windows-1252. The bug shows only with a default charset other than UTF-8, so surefire runs the tests of `handler-csv` with `JAVA_TOOL_OPTIONS=-Dfile.encoding=windows-1252`, on every platform and JDK. This replaces a `JAVA_TOOL_OPTIONS` of the environment in these test JVMs. `CsvWriterTest.shouldRunWithDefaultCharsetOtherThanUtf8` fails when the tests run with a UTF-8 default charset, as in a run that does not apply this setting: there, six of the new cases pass against the unfixed code. With windows-1252, every new case but that check fails against the unfixed code, and each of the five changes is caught when reverted alone: the writer, the verifier, the strict decoder, the reader of the handler and the header check. `audit/core` and `audit/handler-csv` pass locally on JDK 11 and 27. ## Not in this PR Found by reading the code in the review of this change, older than it, and not reproduced; filed separately: - #344: the row HMAC does not cover the column boundaries, so an edit that moves characters between adjacent columns keeps it. - #345: `StandardCsvWriter` throws `ArrayIndexOutOfBoundsException` when it resumes a file with more columns than the topic schema. - #346: reading or querying a tamper-evident topic with no event yet throws `NullPointerException`. Fixed since by #354. `TextWriter.Stream` still swallows write failures through `PrintWriter`; that is #349.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #346.
SecureCsvWriterwrites the CSV header with the first event, so the audit file of a tamper-evident topic is empty after startup, and after therotateaction of a topic without file rotation, until an event is published.CsvAuditEventHandler.getEntries()passed thenullheader of such a file toconvertDotNotationToSlashes():readEvent()threw theNullPointerExceptionout of the call instead of answering a promise;queryEvents()answeredBadRequestException.getEntries()now returns no entry for a file without a header:readEvent()answersNotFoundException, andqueryEvents()answers an empty result. This also covers the empty file that a rotation before the first signature leaves with #342.Tests
CsvAuditEventHandlerTest.testReadingTamperEvidentAuditLogWithNoEventYetandtestQueryOnTamperEvidentAuditLogWithNoEventYet, after startup and after therotateaction. Each case first checks that the audit file is empty, and fails without the fix as the issue describes.audit/coreandaudit/handler-csvpass locally on JDK 11 and 27.The change applies cleanly on #341, #342, #347 and #348.