Browse Source

fix: cap per-failure warning logs to first attempt per bucket (SEC_REVIEW F74)

`LoginThrottle::recordFailure` emitted a structured `warning` line
on every failed login attempt that didn't trip a lockout. At the
F1/F2 fixed-mitigation rates, a 1000-IP botnet spraying one
username pre-emitted ~24 (per-username-bucket) + 4×1000 (per-IP-
bucket) = ~4024 warnings before everything was locked. Each line
is ~300 bytes through the JSON formatter, so a single brute-force
burst could drop a megabyte or more on disk and into whatever
SIEM is downstream — and there was no per-bucket rate cap to
flatten that.

Drop the warning to fire only on `failure_count === 1`. Subsequent
failures in the bucket stay silent until a lockout fires, which
still emits the existing `error` line with `failure_count` and
`lock_seconds`. Total visibility is preserved at aggregate (the
lockout error captures the burst size); the noisy per-attempt
visibility was always redundant with the lockout signal.

The same botnet now emits 1 username-bucket warning + 1000
per-IP-bucket warnings ≈ 1001 lines. ~4× quieter, with no lost
information — the eventual lockout errors carry the per-bucket
counts. The `error`-on-lockout signal stays loud because that's
the genuinely-actionable event for SOC tooling.

Regression tests in `ui/tests/Unit/Auth/LoginThrottleTest.php`:

  - `testRecordFailureLogRateIsCappedToFirstWarningPerBucket`
    drives 5 recordFailures, asserts exactly 1 warning + ≥1
    error, and verifies the warning's `failure_count == 1`.
  - `testHighRateBruteForceDoesNotSpamPerFailureWarnings` drives
    100 recordFailures into the same bucket and asserts the
    warning count stays at 1, no matter what (defends against a
    future change that slips past the controller-level
    `isLocked` guard).

The pre-existing 14 LoginThrottle tests still pass — the lockout
ladder, time advance, per-username bucket, persistence, etc. don't
care about the log output.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa 3 ngày trước cách đây
mục cha
commit
3235d35651
2 tập tin đã thay đổi với 79 bổ sung1 xóa
  1. 9 1
      ui/src/Auth/LoginThrottle.php
  2. 70 0
      ui/tests/Unit/Auth/LoginThrottleTest.php

+ 9 - 1
ui/src/Auth/LoginThrottle.php

@@ -114,7 +114,15 @@ final class LoginThrottle
                         'lock_seconds' => $ipLock,
                     ],
                 ];
-            } else {
+            } elseif ($ipEntry['count'] === 1) {
+                // SEC_REVIEW F74: log-rate cap. Emit a warning at most
+                // once per (username, ip) bucket — the FIRST failure
+                // — then stay silent until either the window resets
+                // or a lockout fires (which always emits the error
+                // above). The lockout's `failure_count` field
+                // captures the total burst size, so the per-attempt
+                // visibility is preserved at aggregate without 4×
+                // log lines per IP and 24× per username.
                 $logEvents[] = [
                     'level' => 'warning',
                     'message' => 'local login failure',

+ 70 - 0
ui/tests/Unit/Auth/LoginThrottleTest.php

@@ -177,6 +177,76 @@ final class LoginThrottleTest extends TestCase
         self::assertTrue($t->isLocked('', '10.99.0.1'));
     }
 
+    public function testRecordFailureLogRateIsCappedToFirstWarningPerBucket(): void
+    {
+        // SEC_REVIEW F74: a sustained brute-force attack must not
+        // pour one structured log line per failed attempt into the
+        // disk/SIEM pipeline. The throttle now emits a single
+        // `warning` per (username, ip) bucket — at the first
+        // failure — and stays silent until a lockout fires (which
+        // emits an `error`). The lockout's `failure_count` field
+        // captures the burst size, so total visibility is preserved
+        // without per-attempt spam.
+        $handler = new TestHandler();
+        $logger = new Logger('test');
+        $logger->pushHandler($handler);
+        $t = new LoginThrottle($logger);
+
+        // Drive 5 failures: counts 1, 2, 3, 4, 5. Lockout fires at
+        // 5. Expected log emissions: 1 warning (count=1) + 1 error
+        // (count=5 lockout) = 2 lines, NOT 5.
+        for ($i = 0; $i < 5; ++$i) {
+            $t->recordFailure('admin', '10.0.0.1');
+        }
+
+        $warnings = array_filter(
+            $handler->getRecords(),
+            static fn ($r) => $r->level->name === 'Warning',
+        );
+        $errors = array_filter(
+            $handler->getRecords(),
+            static fn ($r) => $r->level->name === 'Error',
+        );
+        self::assertCount(1, $warnings, 'expected a single warning at the first failure');
+        self::assertGreaterThanOrEqual(1, count($errors), 'expected at least one lockout error');
+
+        // The single warning must record `failure_count = 1`.
+        /** @var \Monolog\LogRecord $w */
+        $w = array_values($warnings)[0];
+        self::assertSame(1, $w->context['failure_count']);
+    }
+
+    public function testHighRateBruteForceDoesNotSpamPerFailureWarnings(): void
+    {
+        // 100 attempts from one IP for one username: pre-fix would
+        // have emitted ~4 warnings per bucket (counts 1..4) before
+        // lockout. With the rate cap, exactly 1 warning fires.
+        // Subsequent attempts past the lockout are not recorded
+        // (the controller-side `isLocked` check skips
+        // `recordFailure`), so the test models the bucket-internal
+        // behaviour by calling `recordFailure` directly 100 times —
+        // we want to see the cap survive even if a future change
+        // slipped past the controller-level guard.
+        $handler = new TestHandler();
+        $logger = new Logger('test');
+        $logger->pushHandler($handler);
+        $t = new LoginThrottle($logger);
+
+        for ($i = 0; $i < 100; ++$i) {
+            $t->recordFailure('admin', '10.0.0.1');
+        }
+
+        $warnings = array_filter(
+            $handler->getRecords(),
+            static fn ($r) => $r->level->name === 'Warning',
+        );
+        // No matter how many failures we drive into the same bucket,
+        // we get a single warning. The error count is bounded by
+        // the lockout ladder (5 / 10 / 15 IP-bucket triggers and
+        // 25 / 50 / 100 username-bucket triggers).
+        self::assertCount(1, $warnings);
+    }
+
     public function testRecordFailureLogsFingerprintsNotRawIdentifiers(): void
     {
         // SEC_REVIEW F34: a SIEM operator reading these logs must not see