Explorar el Código

fix: log fingerprints, not raw identifiers, in auth flows (SEC_REVIEW F34)

`LoginThrottle::recordFailure` logged the attempted username and source
IP in plaintext on every failure and again on every lockout. The
attempted username can be a password typed in the wrong field, and a
SIEM operator (or anyone with log-pipeline read) reading those records
amounts to accidental disclosure. `OidcController` had the same shape
on its denial paths: the IdP `sub` claim was attached to the
`user_disabled` warning and the no-role-assigned warning.

Introduce `App\Logging\LogIdentifier::fingerprint()`: a stable 12-hex-
char SHA-256 prefix of the input. Triage by counting hits on a single
fingerprint still works ("which `username_fp` failed 25 times across
these IPs?") but a casual log reader no longer sees the raw value.
Empty input collapses to a fixed `empty` sentinel so an absent field
doesn't fold into the SHA-256-of-empty-string bucket.

`LoginThrottle::recordFailure` now logs `username_fp` / `source_ip_fp`
instead of `username` / `source_ip` on both buckets, on both the
failure-with-no-lock and lockout-triggered paths. `OidcController`
logs `subject_fp` instead of `subject` on the user-disabled denial
and the no-role-assigned branch.

The fingerprint is not cryptographic protection against an attacker
with full log access who is willing to brute-force a small space such
as the IPv4 universe — that threat is out of scope for F34, which
targets accidental disclosure.

Regression tests:

  - ui/tests/Unit/Logging/LogIdentifierTest.php — stability,
    distinguishability, raw-value-non-recoverable, length-bounded,
    empty-sentinel.
  - LoginThrottleTest::testRecordFailureLogsFingerprintsNotRawIdentifiers
    swaps in a `TestHandler`, drives a lockout, and asserts neither
    the raw username nor the raw IP appears in any record context or
    message.
  - OidcFlowTest gains
    `testNoneRoleDoesNotLogRawSubject` /
    `testDisabledUserDeniedDoesNotLogRawSubject`, exercised via a new
    `AppTestCase::captureLogs()` helper that swaps in a Monolog
    `TestHandler` and rebuilds `OidcController` against it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa hace 4 días
padre
commit
3a4026baf6

+ 17 - 8
ui/src/Auth/LoginThrottle.php

@@ -4,6 +4,7 @@ declare(strict_types=1);
 
 namespace App\Auth;
 
+use App\Logging\LogIdentifier;
 use Psr\Log\LoggerInterface;
 
 /**
@@ -81,12 +82,20 @@ final class LoginThrottle
         /** @var list<array{level: string, message: string, context: array<string, mixed>}> $logEvents */
         $logEvents = [];
 
+        // SEC_REVIEW F34: log non-reversible fingerprints of the attempted
+        // username and source IP rather than the raw values. Triage by
+        // counting hits on a single fingerprint still works; a SIEM reader
+        // no longer sees passwords typed in the username field or raw
+        // client addresses.
+        $usernameFp = LogIdentifier::fingerprint($username);
+        $sourceIpFp = LogIdentifier::fingerprint($ip);
+
         $this->store->mutate(static function (array $state) use (
             $now,
             $ipKey,
             $userKey,
-            $username,
-            $ip,
+            $usernameFp,
+            $sourceIpFp,
             &$logEvents,
         ): array {
             $ipEntry = $state['ip'][$ipKey] ?? ['count' => 0, 'lockedUntil' => 0];
@@ -99,8 +108,8 @@ final class LoginThrottle
                     'message' => 'local login lockout triggered',
                     'context' => [
                         'bucket' => 'ip',
-                        'username' => $username,
-                        'source_ip' => $ip,
+                        'username_fp' => $usernameFp,
+                        'source_ip_fp' => $sourceIpFp,
                         'failure_count' => $ipEntry['count'],
                         'lock_seconds' => $ipLock,
                     ],
@@ -111,8 +120,8 @@ final class LoginThrottle
                     'message' => 'local login failure',
                     'context' => [
                         'bucket' => 'ip',
-                        'username' => $username,
-                        'source_ip' => $ip,
+                        'username_fp' => $usernameFp,
+                        'source_ip_fp' => $sourceIpFp,
                         'failure_count' => $ipEntry['count'],
                     ],
                 ];
@@ -129,8 +138,8 @@ final class LoginThrottle
                     'message' => 'local login lockout triggered',
                     'context' => [
                         'bucket' => 'username',
-                        'username' => $username,
-                        'source_ip' => $ip,
+                        'username_fp' => $usernameFp,
+                        'source_ip_fp' => $sourceIpFp,
                         'failure_count' => $userEntry['count'],
                         'lock_seconds' => $userLock,
                     ],

+ 11 - 2
ui/src/Auth/OidcController.php

@@ -6,6 +6,7 @@ namespace App\Auth;
 
 use App\ApiClient\ApiException;
 use App\ApiClient\AuthClient;
+use App\Logging\LogIdentifier;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
 use Psr\Log\LoggerInterface;
@@ -93,7 +94,11 @@ final class OidcController
             // "no role assigned" branch uses) instead of looping back
             // through /login with an "API unreachable" flash that misleads.
             if ($e->statusCode === 403 && $e->apiError === 'user_disabled') {
-                $this->logger->warning('oidc login denied: user disabled', ['subject' => $claims->subject]);
+                // SEC_REVIEW F34: log a fingerprint, not the raw OIDC subject.
+                $this->logger->warning(
+                    'oidc login denied: user disabled',
+                    ['subject_fp' => LogIdentifier::fingerprint($claims->subject)],
+                );
 
                 return $response->withStatus(302)->withHeader('Location', '/no-access');
             }
@@ -104,7 +109,11 @@ final class OidcController
         }
 
         if ($user->role === 'none' || $user->role === '') {
-            $this->logger->warning('oidc user has no role assigned', ['subject' => $claims->subject]);
+            // SEC_REVIEW F34: log a fingerprint, not the raw OIDC subject.
+            $this->logger->warning(
+                'oidc user has no role assigned',
+                ['subject_fp' => LogIdentifier::fingerprint($claims->subject)],
+            );
 
             return $response->withStatus(302)->withHeader('Location', '/no-access');
         }

+ 43 - 0
ui/src/Logging/LogIdentifier.php

@@ -0,0 +1,43 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Logging;
+
+/**
+ * Stable, non-reversible fingerprint of a sensitive identifier for log
+ * correlation (SEC_REVIEW F34).
+ *
+ * Auth flows previously logged the attempted username, source IP, and OIDC
+ * subject in plaintext. Lower-trust SIEM operators reading those records
+ * could see passwords typed in the wrong field, raw client addresses, and
+ * IdP subject claims — all "accidental disclosure" the review flagged.
+ *
+ * The fingerprint is the first 12 hex chars of SHA-256 over the value. That
+ * is enough entropy to keep collisions vanishingly rare across a single
+ * deployment's logs (so triage like "which `username_fp` failed 25 times
+ * across these IPs?" still works), while rendering the raw value
+ * non-recoverable from a casual look at a log line. It is *not* a
+ * cryptographic shield against a determined attacker who already has full
+ * log access and is willing to brute-force a small space (e.g. the IPv4
+ * universe) — defence against that is out of scope for F34, which is about
+ * the casual-viewing surface.
+ *
+ * Empty input is fingerprinted to a fixed sentinel so an empty username
+ * doesn't collide with the SHA-256 of the empty string by accident in a
+ * downstream string match.
+ */
+final class LogIdentifier
+{
+    private const FINGERPRINT_LENGTH = 12;
+    private const EMPTY_SENTINEL = 'empty';
+
+    public static function fingerprint(string $value): string
+    {
+        if ($value === '') {
+            return self::EMPTY_SENTINEL;
+        }
+
+        return substr(hash('sha256', $value), 0, self::FINGERPRINT_LENGTH);
+    }
+}

+ 66 - 0
ui/tests/Integration/Auth/OidcFlowTest.php

@@ -124,6 +124,72 @@ final class OidcFlowTest extends AppTestCase
         );
     }
 
+    public function testNoneRoleDoesNotLogRawSubject(): void
+    {
+        // SEC_REVIEW F34: a SIEM operator must not see the raw OIDC subject
+        // in the no-role-assigned warning. A stable fingerprint is logged
+        // instead so triage can still correlate repeated denials for the
+        // same user.
+        $rawSubject = 'aad-objectid-secret-9b6f7e3c-1234-5678';
+        $this->bindOidcAuthenticator(new class ($rawSubject) implements OidcAuthenticator {
+            public function __construct(private readonly string $sub)
+            {
+            }
+
+            public function authenticate(): OidcClaims
+            {
+                return new OidcClaims($this->sub, 'x@x', 'X', []);
+            }
+        });
+        $this->enqueueApiResponse(200, [
+            'user_id' => 0, 'role' => 'none', 'email' => 'x@x', 'display_name' => 'X', 'is_local' => false,
+        ]);
+        $logs = $this->captureLogs();
+
+        $this->request('GET', '/oidc/callback');
+
+        self::assertNotEmpty($logs->getRecords(), 'expected the no-role denial to be logged');
+        foreach ($logs->getRecords() as $record) {
+            $serialized = json_encode([
+                'message' => $record->message,
+                'context' => $record->context,
+            ], \JSON_THROW_ON_ERROR);
+            self::assertStringNotContainsString($rawSubject, $serialized);
+            self::assertArrayNotHasKey('subject', $record->context);
+        }
+        self::assertTrue($logs->hasWarningThatContains('oidc user has no role assigned'));
+    }
+
+    public function testDisabledUserDeniedDoesNotLogRawSubject(): void
+    {
+        // SEC_REVIEW F34: same redaction on the user_disabled branch.
+        $rawSubject = 'aad-objectid-other-7c8d2f';
+        $this->bindOidcAuthenticator(new class ($rawSubject) implements OidcAuthenticator {
+            public function __construct(private readonly string $sub)
+            {
+            }
+
+            public function authenticate(): OidcClaims
+            {
+                return new OidcClaims($this->sub, 'x@x', 'X', []);
+            }
+        });
+        $this->enqueueApiResponse(403, ['error' => 'user_disabled']);
+        $logs = $this->captureLogs();
+
+        $this->request('GET', '/oidc/callback');
+
+        foreach ($logs->getRecords() as $record) {
+            $serialized = json_encode([
+                'message' => $record->message,
+                'context' => $record->context,
+            ], \JSON_THROW_ON_ERROR);
+            self::assertStringNotContainsString($rawSubject, $serialized);
+            self::assertArrayNotHasKey('subject', $record->context);
+        }
+        self::assertTrue($logs->hasWarningThatContains('oidc login denied: user disabled'));
+    }
+
     public function testApiUnreachableDuringUpsertFlashesAndRedirects(): void
     {
         $this->bindOidcAuthenticator(new class () implements OidcAuthenticator {

+ 28 - 0
ui/tests/Integration/Support/AppTestCase.php

@@ -12,6 +12,7 @@ use GuzzleHttp\Handler\MockHandler;
 use GuzzleHttp\HandlerStack;
 use GuzzleHttp\Middleware;
 use Monolog\Handler\NullHandler;
+use Monolog\Handler\TestHandler;
 use Monolog\Logger;
 use PHPUnit\Framework\TestCase;
 use Psr\Container\ContainerInterface;
@@ -151,6 +152,33 @@ abstract class AppTestCase extends TestCase
         return $this->app->handle($request);
     }
 
+    /**
+     * Swap in a Monolog `TestHandler` so a test can assert what reached
+     * the logger. Re-builds controllers that captured the old logger
+     * (currently just `OidcController`) so they pick up the new binding.
+     */
+    protected function captureLogs(): TestHandler
+    {
+        $handler = new TestHandler();
+        $logger = new Logger('test');
+        $logger->pushHandler($handler);
+        if (method_exists($this->container, 'set')) {
+            /** @var \DI\Container $c */
+            $c = $this->container;
+            $c->set(LoggerInterface::class, $logger);
+            $c->set(\App\Auth\OidcController::class, new \App\Auth\OidcController(
+                authenticator: $c->get(OidcAuthenticator::class),
+                auth: $c->get(\App\ApiClient\AuthClient::class),
+                sessions: $c->get(\App\Auth\SessionManager::class),
+                logger: $logger,
+                enabled: (bool) $c->get('settings.oidc_enabled'),
+            ));
+            $this->app = AppFactory::build($c);
+        }
+
+        return $handler;
+    }
+
     /**
      * Replace the `OidcAuthenticator` binding with a fake callback that
      * the test controls. Lets us drive the OIDC controller's success +

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

@@ -6,6 +6,7 @@ namespace App\Tests\Unit\Auth;
 
 use App\Auth\LoginThrottle;
 use Monolog\Handler\NullHandler;
+use Monolog\Handler\TestHandler;
 use Monolog\Logger;
 use PHPUnit\Framework\TestCase;
 use Psr\Log\LoggerInterface;
@@ -176,6 +177,39 @@ final class LoginThrottleTest extends TestCase
         self::assertTrue($t->isLocked('', '10.99.0.1'));
     }
 
+    public function testRecordFailureLogsFingerprintsNotRawIdentifiers(): void
+    {
+        // SEC_REVIEW F34: a SIEM operator reading these logs must not see
+        // the attempted username (which can be a password typed in the
+        // wrong field) or the raw client IP. They must see a stable
+        // fingerprint instead so triage can still correlate.
+        $handler = new TestHandler();
+        $logger = new Logger('test');
+        $logger->pushHandler($handler);
+        $t = new LoginThrottle($logger);
+
+        $rawUser = 'hunter2-typed-into-username-field';
+        $rawIp = '198.51.100.77';
+
+        for ($i = 0; $i < 5; ++$i) {
+            $t->recordFailure($rawUser, $rawIp);
+        }
+
+        self::assertNotEmpty($handler->getRecords(), 'expected lockout/failure events to be logged');
+        foreach ($handler->getRecords() as $record) {
+            $serialized = json_encode([
+                'message' => $record->message,
+                'context' => $record->context,
+            ], \JSON_THROW_ON_ERROR);
+            self::assertStringNotContainsString($rawUser, $serialized);
+            self::assertStringNotContainsString($rawIp, $serialized);
+            self::assertArrayHasKey('username_fp', $record->context);
+            self::assertArrayHasKey('source_ip_fp', $record->context);
+            self::assertArrayNotHasKey('username', $record->context);
+            self::assertArrayNotHasKey('source_ip', $record->context);
+        }
+    }
+
     private function throttle(?int $fixedTime = null): LoginThrottle
     {
         if ($fixedTime === null) {

+ 49 - 0
ui/tests/Unit/Logging/LogIdentifierTest.php

@@ -0,0 +1,49 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Unit\Logging;
+
+use App\Logging\LogIdentifier;
+use PHPUnit\Framework\TestCase;
+
+final class LogIdentifierTest extends TestCase
+{
+    public function testFingerprintIsStableAcrossCalls(): void
+    {
+        $a = LogIdentifier::fingerprint('admin');
+        $b = LogIdentifier::fingerprint('admin');
+        self::assertSame($a, $b);
+    }
+
+    public function testFingerprintDistinguishesDifferentInputs(): void
+    {
+        self::assertNotSame(
+            LogIdentifier::fingerprint('admin'),
+            LogIdentifier::fingerprint('Admin'),
+        );
+        self::assertNotSame(
+            LogIdentifier::fingerprint('10.0.0.1'),
+            LogIdentifier::fingerprint('10.0.0.2'),
+        );
+    }
+
+    public function testFingerprintDoesNotContainRawValue(): void
+    {
+        $fp = LogIdentifier::fingerprint('hunter2-this-was-typed-as-username');
+        self::assertStringNotContainsString('hunter2', $fp);
+    }
+
+    public function testFingerprintLengthIsBounded(): void
+    {
+        // 12 hex chars: short enough for log readability, long enough that
+        // collisions are negligible across a deployment's logs.
+        self::assertSame(12, strlen(LogIdentifier::fingerprint('admin')));
+        self::assertSame(12, strlen(LogIdentifier::fingerprint(str_repeat('x', 4096))));
+    }
+
+    public function testEmptyInputReturnsSentinel(): void
+    {
+        self::assertSame('empty', LogIdentifier::fingerprint(''));
+    }
+}