Repository navigation
fix(oauth): redact the whole of a base64 Bearer token - #1785
Open
rudycelekli wants to merge 1 commit into
Open
rudycelekli wants to merge 1 commit into
rudycelekli wants to merge 1 commit into
Conversation
tokenPattern stopped at the first character outside [A-Za-z0-9-_.], so a standard-base64 token was redacted only up to its first '+' or '/' and the rest stayed in the text. Use RFC 6750's b64token class (adds ~ + / and trailing =), as the log sanitizer's bearer pattern already does.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
tokenPatternininternal/oauth/logging.gois(?i)(bearer\s+)[a-zA-Z0-9\-_\.]+. RFC 6750'sb64tokenalso allows~,+,/and trailing=, and opaque tokens encoded in standard base64 contain+and/. The match stops at the first one, so only the head of the token is replaced and the rest stays in the text:tokenPatternis the Bearer rule inRedactSensitiveData, and through it inScrubUpstreamText. Those functions scrub connection errors,last_error/health detail and tailed log lines. It is also used inLogSafeRequestPath. The value-shaped detector that runs afterwards does not recognise the leftover tail, so it ends up in the output.The log sanitizer's
bearer_tokenpattern (internal/logs/sanitizer.go) and the audit redactor already use the RFC 6750 class.Trigger
An upstream that rejects a request and echoes the credential in its error body, for example
upstream rejected Bearer dGhpcy1pcy1h+c2VjcmV0/dG9rZW4~cGFydA== (401). AfterScrubUpstreamText, everything from the first+on is still visible inlast_error.Fix
Use
[a-zA-Z0-9\-_\.~+/]+=*for the token value.Test
TestRedactSensitiveData/redacts_the_whole_of_a_base64_bearer_tokenexpects exactlyupstream rejected Bearer ***REDACTED*** (401).Commands run
CGO_ENABLED=0 go test -count=1 ./internal/oauth: okCGO_ENABLED=0 go test -count=1 ./internal/httpapi ./internal/transport ./internal/logs: oklogging.goreverted, the new subtest fails (actual: "upstream rejected Bearer ***REDACTED***+c2VjcmV0/dG9rZW4~cGFydA== (401)").gofmt -l internal/oauth: no output