fix: redact sensitive headers in tonic debug logs#3465
fix: redact sensitive headers in tonic debug logs#3465rornic wants to merge 2 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3465 +/- ##
=====================================
Coverage 83.7% 83.7%
=====================================
Files 126 126
Lines 25386 25386
=====================================
Hits 21255 21255
Misses 4131 4131 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| (k.clone(), v.clone()) | ||
| } | ||
| }) | ||
| .collect() |
There was a problem hiding this comment.
Can we also add redact for cookie and set-cookie header, as they do Carry sensitive data. And also make the test coverage better.
|
Add a Changelog entry, as this is user visible fix. |
|
|
||
| let (headers_from_env, headers_for_logging) = parse_headers_from_env(signal_headers_var); | ||
| let (headers_from_env, all_headers) = parse_headers_from_env(signal_headers_var); | ||
| let headers_for_logging = redact_sensitive_headers(all_headers); |
There was a problem hiding this comment.
stepping back question - do we actually need to include header keys and values in the debug logs at all? If we switch it to just have the header names, won't that be sufficient? (since the value is coming from env variable, operators can query for it anyway, once they know which keys are picked up by the application)..
if we do that, then there is no need of the redaction at all, and it is never possible catch all sensitive headers anyway. (what is sensitive to a user might be required for another user etc.)
There was a problem hiding this comment.
Yes this seems reasonable since headers_for_logging is not modified at all before it's logged - so the values must be coming straight from environment.
It was mentioned as an alternative fix in the original issue.
I'll update the PR soon when I get a chance.
cijothomas
left a comment
cijothomas
left a comment
There was a problem hiding this comment.
#3465 (comment) - better to skip logging values, as discussed here.
Adds function that ensures headers that look sensitive are redacted, to be used inside debug logs. Attempted a parameterised test but `run_env_test` only accepts static strings for env vars.
rornic
force-pushed
the
redact-sensitive-headers-in-debug-logs
branch
from
b3e755b to
eae289f
Compare
3 participants
Fixes #3367
Changes
Prevents logging of sensitive header values in OTLP tonic exporter by only logging header keys.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes