Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

Commit

Permalink
security: redact unrecognized auth HTTP headers from logs (#46112)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
jhchabran authored Jan 5, 2023
1 parent 5ae3c5b commit 866d822
Showing 1 changed file with 20 additions and 2 deletions.
22 changes: 20 additions & 2 deletions cmd/frontend/internal/httpapi/auth.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package httpapi

import (
"crypto/md5"
"encoding/json"
"fmt"
"io"
"net/http"
"time"

Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit 866d822

Please sign in to comment.