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

security: redact unrecognized auth HTTP headers from logs #46112

Merged
merged 3 commits into from
Jan 5, 2023

Conversation

jhchabran
Copy link
Contributor

@jhchabran jhchabran commented Jan 4, 2023

TL;DR The Authentication middleware currently logs custom authorization headers in clear.

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.

Test plan

Locally tested. Because the authorization header clashes with the one I'm testing my code with, I reproduced the customer setup with this addition to the site-config:

  "auth.providers": [
    // ...
    {
      "type": "http-header",
      "usernameHeader": "X-Forwarded-User",
      "emailHeader": "X-Forwarded-Email"
    }

And used the following cURL:

~ $ curl -X POST -H 'Authorization: Bearer 2323' -H 'X-Forwarded-User: sourcegraph' -H 'Authorization: Bearer foobar' -d '{"query": "query { currentUser { username } }"}' 'https://sourcegraph.test:3443/.api/graphql'

Which then logs:

[       frontend] WARN external.accessTokenAuth httpapi/auth.go:58 ignoring unrecognized Authorization header {"redacted_value": "md5sum:f3dce97acd1145b1274d05117a3847da", "error": "unrecognized HTTP Authorization request header scheme (supported values: \"token\", \"token-sudo\")"}

As expected.

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.
@cla-bot cla-bot bot added the cla-signed label Jan 4, 2023
@jhchabran jhchabran requested review from andreeleuterio and a team January 4, 2023 09:59
@jhchabran jhchabran added security NOTE: Disclose live security issues to [email protected], NOT here. team/devx labels Jan 4, 2023
Copy link
Contributor

@sanderginn sanderginn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[ frontend] WARN external.accessTokenAuth httpapi/auth.go:58 ignoring unrecognized Authorization header {"redacted_value": "md5sum:f3dce97acd1145b1274d05117a3847da", "error": "unrecognized HTTP Authorization request header scheme (supported values: \"token\", \"token-sudo\")"}

Perhaps it would be slightly more informative to include the unrecognized scheme in the log message? E.g. in this case Bearer?

cmd/frontend/internal/httpapi/auth.go Outdated Show resolved Hide resolved
@jhchabran jhchabran merged commit 866d822 into main Jan 5, 2023
@jhchabran jhchabran deleted the jh/redact-unrecognized-authentication-header branch January 5, 2023 12:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed security NOTE: Disclose live security issues to [email protected], NOT here.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants