From 864e595992cb38d4dfb3934e4df36d547268e7da Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Sat, 17 Oct 2026 10:31:33 +0300 Subject: [PATCH] [#343] Tamper-evident CSV audit: start the rotation checks once a resumed file is verified SecureCsvWriter built its RotatableWriter, which scheduled the rotation and retention checks, before it read the keys and verified the file it resumes. When the construction outlasted the check interval, a rotation renamed the file and its keystore and wrote new keys while the constructor still read or verified them, and the topic failed to start. RotatableWriter gets a constructor that leaves the checks to startRotationAndRetentionChecks(), which SecureCsvWriter calls once its constructor has succeeded. The file is still opened first, so a file that cannot be opened still fails only its topic. --- .../handlers/writers/RotatableWriter.java | 56 +++++++++++++++---- .../handlers/writers/RotatableWriterTest.java | 28 ++++++++++ .../audit/handlers/csv/SecureCsvWriter.java | 8 ++- .../handlers/csv/SecureCsvWriterTest.java | 53 ++++++++++++++++++ 4 files changed, 133 insertions(+), 12 deletions(-) diff --git a/commons/audit/core/src/main/java/org/forgerock/audit/events/handlers/writers/RotatableWriter.java b/commons/audit/core/src/main/java/org/forgerock/audit/events/handlers/writers/RotatableWriter.java index 95345de08..4f12350b3 100644 --- a/commons/audit/core/src/main/java/org/forgerock/audit/events/handlers/writers/RotatableWriter.java +++ b/commons/audit/core/src/main/java/org/forgerock/audit/events/handlers/writers/RotatableWriter.java @@ -73,6 +73,7 @@ public class RotatableWriter implements TextWriter, RotatableObject { private BufferedWriter writer; private final ReentrantReadWriteLock readWriteLock = new ReentrantReadWriteLock(); private final RolloverLifecycleHook rolloverLifecycleHook; + private final Duration rotationCheckInterval; /** * Constructs a {@link RotatableWriter} given an initial file to manage rotation/retention, and @@ -99,8 +100,29 @@ public RotatableWriter(final File file, final FileBasedEventHandlerConfiguration */ public RotatableWriter(final File file, final FileBasedEventHandlerConfiguration configuration, final boolean append, final RolloverLifecycleHook rolloverLifecycleHook) throws IOException { + this(file, configuration, append, rolloverLifecycleHook, true); + } + + /** + * Constructs a {@link RotatableWriter} given an initial file to manage rotation/retention, a + * {@link FileBasedEventHandlerConfiguration} and a {@link RolloverLifecycleHook}, and starts the rotation and + * retention checks, or leaves them to {@link #startRotationAndRetentionChecks()}. + *

+ * A rotation renames the file: a writer that reads the file before it writes to it starts the checks once it has + * read it. + * + * @param file The initial file to manage rotation/retention. + * @param configuration The configuration of the rotation and retention policies. + * @param append Whether to append to the rotatable file or not. + * @param rolloverLifecycleHook Hook to use before and after rotation/retention checks. + * @param startChecks Whether to start the rotation and retention checks now. + * @throws IOException If a problem occurs. + */ + public RotatableWriter(final File file, final FileBasedEventHandlerConfiguration configuration, + final boolean append, final RolloverLifecycleHook rolloverLifecycleHook, final boolean startChecks) + throws IOException { this(file, configuration, append, configuration.getFileRotation().buildTimeStampFileNamingPolicy(file), - rolloverLifecycleHook); + rolloverLifecycleHook, startChecks); } /** @@ -110,13 +132,13 @@ public RotatableWriter(final File file, final FileBasedEventHandlerConfiguration @VisibleForTesting RotatableWriter(final File file, final FileBasedEventHandlerConfiguration configuration, final boolean append, final FileNamingPolicy fileNamingPolicy) throws IOException { - this(file, configuration, append, fileNamingPolicy, NOOP_ROLLOVER_LIFECYCLE_HOOK); + this(file, configuration, append, fileNamingPolicy, NOOP_ROLLOVER_LIFECYCLE_HOOK, true); } /** Constructor with all possible parameters. */ private RotatableWriter(final File file, final FileBasedEventHandlerConfiguration configuration, final boolean append, final FileNamingPolicy fileNamingPolicy, - final RolloverLifecycleHook rolloverLifecycleHook) throws IOException { + final RolloverLifecycleHook rolloverLifecycleHook, final boolean startChecks) throws IOException { this.file = file; this.fileNamingPolicy = fileNamingPolicy; this.rotationEnabled = configuration.getFileRotation().isRotationEnabled(); @@ -128,7 +150,10 @@ private RotatableWriter(final File file, final FileBasedEventHandlerConfiguratio this.writer = constructWriter(file, append); retentionPolicies = configuration.getFileRetention().buildRetentionPolicies(); rotationPolicies = configuration.getFileRotation().buildRotationPolicies(); - scheduleRotationAndRetentionChecks(configuration); + rotationCheckInterval = parseRotationCheckInterval(configuration); + if (startChecks) { + startRotationAndRetentionChecks(); + } } /** @@ -355,21 +380,30 @@ private BufferedWriter constructWriter(File csvFile, boolean append) } /** - * Schedule checks for rotations and retention policies. + * Parses the interval of the checks for rotations and retention policies. *

* The check interval is provided by the RotationRetentionCheckInterval property, which must have * a non-zero value if at least one policy is enabled. */ - private void scheduleRotationAndRetentionChecks(FileBasedEventHandlerConfiguration configuration) + private Duration parseRotationCheckInterval(FileBasedEventHandlerConfiguration configuration) throws IOException { final Duration rotationCheckInterval = parseDuration("rotation and retention check interval", configuration.getRotationRetentionCheckInterval(), FIVE_SECONDS); - if (!rotationPolicies.isEmpty() || !retentionPolicies.isEmpty()) { - if (rotationCheckInterval.isUnlimited() || rotationCheckInterval.isZero()) { - throw new IOException("Rotation and retention check interval set to an invalid value: " - + rotationCheckInterval); - } + if ((!rotationPolicies.isEmpty() || !retentionPolicies.isEmpty()) + && (rotationCheckInterval.isUnlimited() || rotationCheckInterval.isZero())) { + throw new IOException("Rotation and retention check interval set to an invalid value: " + + rotationCheckInterval); + } + return rotationCheckInterval; + } + + /** + * Schedules the checks for rotations and retention policies, if at least one policy is enabled, at the check + * interval. Does nothing if they are scheduled already. + */ + public void startRotationAndRetentionChecks() { + if (rotator == null && (!rotationPolicies.isEmpty() || !retentionPolicies.isEmpty())) { rotator = Executors.newScheduledThreadPool(1); rotator.scheduleAtFixedRate( new Runnable() { diff --git a/commons/audit/core/src/test/java/org/forgerock/audit/events/handlers/writers/RotatableWriterTest.java b/commons/audit/core/src/test/java/org/forgerock/audit/events/handlers/writers/RotatableWriterTest.java index d2845fb30..bbc528671 100644 --- a/commons/audit/core/src/test/java/org/forgerock/audit/events/handlers/writers/RotatableWriterTest.java +++ b/commons/audit/core/src/test/java/org/forgerock/audit/events/handlers/writers/RotatableWriterTest.java @@ -442,6 +442,34 @@ public void testAutomaticallyEvaluatesPolicesPeriodicallyIfRotationIntervalSpeci assertThat(rotatableWriter.getBytesWritten()).isEqualTo(0L); } + @Test + public void testEvaluatesPoliciesPeriodicallyOnlyOnceTheChecksAreStarted() throws Exception { + // given + final File file = getTempFile(); + final String prefix = "testEvaluatesPoliciesPeriodicallyOnlyOnceTheChecksAreStarted"; + assertThat(file.setLastModified(System.currentTimeMillis() - HOURS.toMillis(2))).isTrue(); + final FileBasedEventHandlerConfiguration configuration = new DefaultFileBasedAuditEventHandlerConfiguration(); + configuration.setRotationRetentionCheckInterval("10 ms"); + configuration.getFileRotation().setRotationEnabled(true); + configuration.getFileRotation().setRotationFilePrefix(prefix); + configuration.getFileRotation().setRotationFileSuffix(ROTATION_FILE_SUFFIX.replace(".gz", "")); + configuration.getFileRotation().setRotationInterval("1 hour"); + rotatableWriter = new RotatableWriter( + file, configuration, true, RotatableWriter.NOOP_ROLLOVER_LIFECYCLE_HOOK, false); + + // when + Thread.sleep(500); + + // then + assertThat(getRetainedHistoricalFiles(file, prefix)).isEmpty(); + + // when + rotatableWriter.startRotationAndRetentionChecks(); + + // then + assertRetainedHistoricalFiles(file, prefix, 1); + } + private void writeThenFlushBytes(final RotatableWriter writer, final int bytesToWrite) throws IOException { writer.write(new String(new byte[bytesToWrite])); writer.flush(); 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..3ad652b56 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 @@ -176,6 +176,11 @@ public void run() { } catch (Exception e) { throw new RuntimeException("Error when initializing a secure CSV writer", e); } + if (rotatableWriter != null) { + // Only once the keys are read and the file is verified: a rotation renames the file and its keystore, + // and writes new keys into the secure storage that the verifier reads. + rotatableWriter.startRotationAndRetentionChecks(); + } } @Override @@ -208,7 +213,8 @@ private Writer constructWriter(File csvFile, boolean append, CsvAuditEventHandle throws IOException { TextWriter textWriter; if (config.getFileRotation().isRotationEnabled()) { - rotatableWriter = new RotatableWriter(csvFile, config, append, this); + // the checks are started at the end of the constructor + rotatableWriter = new RotatableWriter(csvFile, config, append, this, false); rotatableWriter.registerRotationHooks(new SecureCsvWriterRotationHooks()); textWriter = rotatableWriter; } else { 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..783a8cfd9 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 @@ -269,6 +269,59 @@ static void assertThreadsTerminate(ThreadGroup group) throws InterruptedExceptio } } + @Test(timeOut = 60_000) + public void shouldRotateResumedFileOnlyOnceItIsVerified() throws Exception { + final Path logDirectory = Files.createTempDirectory("SecureCsvWriterTest"); + final File actual = + new File(logDirectory.toFile(), CsvAuditEventHandler.SECURE_CSV_FILENAME_PREFIX + "access.csv"); + final String header = "FOO"; + final String rotationFileSuffix = "-yyyy.MM.dd-HH.mm.ss.SSS"; + try (SecureCsvWriter secureCsvWriter = new SecureCsvWriter(actual, new String[]{header}, + CsvPreference.EXCEL_PREFERENCE, createBasicSecureConfig("5 minutes"), keyStoreHandler, random)) { + secureCsvWriter.writeEvent(singletonMap(header, "one")); + } + // the time limit policy rotates the file at the first check after it is resumed + assertThat(actual.setLastModified(System.currentTimeMillis() - TimeUnit.HOURS.toMillis(2))).isTrue(); + + CsvAuditEventHandlerConfiguration config = createBasicSecureConfig("5 minutes"); + config.getFileRotation().setRotationEnabled(true); + config.getFileRotation().setRotationFileSuffix(rotationFileSuffix); + config.getFileRotation().setRotationInterval("1 hour"); + config.setRotationRetentionCheckInterval("10 milliseconds"); + // a keystore slow to read, so that the construction outlasts many check intervals + final KeyStoreHandlerDecorator slowKeyStoreHandler = new KeyStoreHandlerDecorator(keyStoreHandler) { + @Override + public KeyStore getStore() { + try { + Thread.sleep(100); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + return super.getStore(); + } + }; + final FileNamingPolicy fileNamingPolicy = new TimeStampFileNamingPolicy(actual, rotationFileSuffix, null); + try (SecureCsvWriter secureCsvWriter = new SecureCsvWriter(actual, new String[]{header}, + CsvPreference.EXCEL_PREFERENCE, config, slowKeyStoreHandler, random)) { + // close() waits for the rotation that renamed the file to end + final long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(10); + while (fileNamingPolicy.listFiles().isEmpty() && System.nanoTime() < deadline) { + Thread.sleep(10); + } + } + + // the resumed file is archived with its own keystore + final String keystorePassword = + Base64.encode(keyStoreHandler.readSecretKeyFromKeyStore(ENTRY_PASSWORD).getEncoded()); + final PublicKey publicKey = keyStoreHandler.readPublicKeyFromKeyStore(KeyStoreSecureStorage.ENTRY_SIGNATURE); + final List verificationResults = new CsvSecureArchiveVerifier( + fileNamingPolicy, keystorePassword, publicKey, CsvPreference.EXCEL_PREFERENCE).verify(); + assertThat(verificationResults).hasSize(1); + final VerificationResult archive = verificationResults.get(0); + assertThat(archive.hasPassedVerification()).as(archive.getFailureReason()).isTrue(); + assertThat(contentOf(archive.getArchiveFile())).containsPattern("(?m)^one,"); + } + @Test(timeOut = 60_000) public void shouldForceRotationWhileAnotherThreadWritesAnEvent() throws Exception { final Path logDirectory = Files.createTempDirectory("SecureCsvWriterTest");