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