[#343] Tamper-evident CSV audit: start the rotation checks once a resumed file is verified

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()}.
+     * <p>
+     * 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.
      * <p>
      * 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() {

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

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 {

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