From f4f393e5de1cdf19c5eeecc9a4156f0a414221ab Mon Sep 17 00:00:00 2001 From: Joshua Casey Date: Fri, 8 Nov 2024 15:36:04 -0600 Subject: [PATCH] Audit event 'HTTP Request Completed' will now log the location with err, error, and error_description query parameters Co-authored-by: Ryan Richard --- .../requestlogger/request_logger.go | 8 ++++ .../requestlogger/request_logger_test.go | 45 +++++++++---------- 2 files changed, 30 insertions(+), 23 deletions(-) diff --git a/internal/federationdomain/requestlogger/request_logger.go b/internal/federationdomain/requestlogger/request_logger.go index e2f9e74d7..4ea6fdbeb 100644 --- a/internal/federationdomain/requestlogger/request_logger.go +++ b/internal/federationdomain/requestlogger/request_logger.go @@ -134,6 +134,14 @@ func (rl *requestLogger) logRequestComplete() { // We don't know what this `Location` header is used for, so redact all query params redactedParams := parsedLocation.Query() for k, v := range redactedParams { + // Due to https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1, + // authorize errors can have an 'error' and an 'error_description' parameter + // which should never contain PII and is safe to log. + // The 'err' parameter may be populated by the post_login_handler to indicate issues + // when using Supervisor's built-in login page. + if k == "error" || k == "error_description" || k == "err" { + continue + } for i := range v { redactedParams[k][i] = "redacted" } diff --git a/internal/federationdomain/requestlogger/request_logger_test.go b/internal/federationdomain/requestlogger/request_logger_test.go index b2a22504f..37bf208b9 100644 --- a/internal/federationdomain/requestlogger/request_logger_test.go +++ b/internal/federationdomain/requestlogger/request_logger_test.go @@ -142,7 +142,7 @@ func TestLogRequestComplete(t *testing.T) { wantAuditLogs: noAuditEventsWanted, }, { - name: "when internal paths are not Enabled, audits external path with location (redacting all query params)", + name: "when internal paths are not Enabled, audits external path with location (redacting unknown query params)", path: "/pretend-to-login", location: "http://127.0.0.1?foo=bar&foo=quz&lorem=ipsum", auditCfg: supervisor.AuditSpec{ @@ -151,31 +151,12 @@ func TestLogRequestComplete(t *testing.T) { wantAuditLogs: happyAuditEventWanted("/pretend-to-login", "http://127.0.0.1?foo=redacted&foo=redacted&lorem=redacted"), }, { - name: "when internal paths are not Enabled, audits external path without location", - path: "/pretend-to-login", - location: "", // make it obvious - auditCfg: supervisor.AuditSpec{ - InternalPaths: "Disabled", - }, - wantAuditLogs: happyAuditEventWanted("/pretend-to-login", "no location header"), - }, - { - name: "when internal paths are not Enabled, audits external path with invalid location", - path: "/pretend-to-login", - location: "http://e x a m p l e.com", - auditCfg: supervisor.AuditSpec{ - InternalPaths: "Disabled", - }, - wantAuditLogs: happyAuditEventWanted("/pretend-to-login", "unparsable location header"), - }, - { - name: "when internal paths are Enabled, audits internal paths", - path: "/healthz", - location: "some-location", + name: "when internal paths are Enabled, audits internal paths", + path: "/healthz", auditCfg: supervisor.AuditSpec{ InternalPaths: "Enabled", }, - wantAuditLogs: happyAuditEventWanted("/healthz", "some-location"), + wantAuditLogs: happyAuditEventWanted("/healthz", ""), }, { name: "when internal paths are Enabled, audits external paths", @@ -186,6 +167,24 @@ func TestLogRequestComplete(t *testing.T) { }, wantAuditLogs: happyAuditEventWanted("/pretend-to-login", "some-location"), }, + { + name: "audits path without location", + path: "/pretend-to-login", + location: "", // make it obvious + wantAuditLogs: happyAuditEventWanted("/pretend-to-login", "no location header"), + }, + { + name: "audits path with invalid location", + path: "/pretend-to-login", + location: "http://e x a m p l e.com", + wantAuditLogs: happyAuditEventWanted("/pretend-to-login", "unparsable location header"), + }, + { + name: "audits path with location redacting all query params except err, error, and error_description", + path: "/pretend-to-login", + location: "http://127.0.0.1:1234?code=pin_ac_FAKE&foo=bar&foo=quz&lorem=ipsum&err=some-err&error=some-error&error_description=some-error-description&zzlast=some-value", + wantAuditLogs: happyAuditEventWanted("/pretend-to-login", "http://127.0.0.1:1234?code=redacted&err=some-err&error=some-error&error_description=some-error-description&foo=redacted&foo=redacted&lorem=redacted&zzlast=redacted"), + }, } nowDoesntMatter := time.Date(1122, time.September, 33, 4, 55, 56, 778899, time.Local)