From 866d8220f7deb9696626435cf53c13d8b9baaa68 Mon Sep 17 00:00:00 2001
From: Jean-Hadrien Chabran <jh@chabran.fr>
Date: Thu, 5 Jan 2023 13:40:51 +0100
Subject: [PATCH] security: redact unrecognized auth HTTP headers from logs
 (#46112)

* security: redact unrecognized auth HTTP headers from logs

When the Authentication middleware stumbled across a HTTP Header set to
`Authorization: [name value]` and `name` is not one of the recognized
Sourcegraph ones (i.e "token" or "sudo-token"), the logs were printing
that value in clear.

While it's a rather limited concern as the logs are only accessible to
ops and the use of custom authentication tokens are not a very
frequent, it's still not okay to log them in clear.

The present code instead logs a md5sum in place of the value, allowing
to still link back to the value we're expecting if we're debugging. If
the hashing fails for any reason, the value is simply set to
"[REDACTED]".

It is to be noted, that there are legitimate cases where can have
unrecognized Authorization headers, and we should not log in those
cases. As this would require to parse the current config, which
requires more work, I decided for keeping this PR simple and focused
on the security concern first.

* Clarify log message
---
 cmd/frontend/internal/httpapi/auth.go | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/cmd/frontend/internal/httpapi/auth.go b/cmd/frontend/internal/httpapi/auth.go
index 3183843e11ab..e0cf469213e4 100644
--- a/cmd/frontend/internal/httpapi/auth.go
+++ b/cmd/frontend/internal/httpapi/auth.go
@@ -1,8 +1,10 @@
 package httpapi
 
 import (
+	"crypto/md5"
 	"encoding/json"
 	"fmt"
+	"io"
 	"net/http"
 	"time"
 
@@ -43,9 +45,25 @@ func AccessTokenAuthMiddleware(db database.DB, logger log.Logger, next http.Hand
 			if err != nil {
 				if authz.IsUnrecognizedScheme(err) {
 					// Ignore Authorization headers that we don't handle.
+					// 🚨 SECURITY: md5sum the authorization header value so we redact it
+					// while still retaining the ability to link it back to a token, assuming
+					// the logs reader has the value in clear.
+					var redactedValue string
+					h := md5.New()
+					if _, err := io.WriteString(h, headerValue); err != nil {
+						redactedValue = "[REDACTED]"
+					} else {
+						redactedValue = fmt.Sprintf("md5sum:%x", h.Sum(nil))
+					}
+					// TODO: It is possible for the unrecognized header to be legitimate, in the case
+					// of a customer setting up a HTTP header based authentication and decide to still
+					// use the "Authorization" key.
+					//
+					// We should parse the configuration to see if that's the case and only log if it's
+					// not defined over there.
 					logger.Warn(
-						"ignoring unrecognized Authorization header",
-						log.String("value", headerValue),
+						"ignoring unrecognized Authorization header, passing it down to the next layer",
+						log.String("redacted_value", redactedValue),
 						log.Error(err),
 					)
 					next.ServeHTTP(w, r)