fix(otlp): prevent auth tokens from leaking in export error messages#3360
fix(otlp): prevent auth tokens from leaking in export error messages#3360bryantbiggs wants to merge 1 commit intoopen-telemetry:mainfrom
Conversation
2470648 to
f92b4b5
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3360 +/- ##
=====================================
Coverage 82.2% 82.3%
=====================================
Files 128 128
Lines 24626 24685 +59
=====================================
+ Hits 20266 20317 +51
- Misses 4360 4368 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
0023b45 to
1732e8a
Compare
| // Connection errors (e.g., "Connection refused", DNS failures) typically | ||
| // indicate user-side misconfigurations and don't contain sensitive data, | ||
| // so it's safe to log the error message at WARN level. | ||
| otel_warn!( |
There was a problem hiding this comment.
We emit otel_warn! here on export failure, but the SDK processors (BatchLogProcessor, BatchSpanProcessor, PeriodicReader, etc.) already log the returned error via otel_error!. This would result in duplicate log entries for the same export failure. Should we avoid logging in the macros and rely on the existing SDK-level logging instead?
| let is_connection_error = matches!( | ||
| code, | ||
| tonic::Code::Unavailable | ||
| | tonic::Code::Unknown |
There was a problem hiding this comment.
I think Unknown is pretty broad and could carry sensitive server responses. Might be safer to treat it as potentially sensitive.
1732e8a to
144d6f0
Compare
bryantbiggs
left a comment
There was a problem hiding this comment.
Addressed both review comments:
-
Duplicate logging (
http/mod.rs): Downgraded all exporter-level logging fromotel_warn!tootel_debug!in both the HTTP exporter and the tonic macros (handle_tonic_export_error!,handle_interceptor_error!). SDK processors will handle the WARN/ERROR level logging for returned errors. -
Unknowntoo broad (tonic/mod.rs): MovedUnknownout of theis_connection_errormatch. It's now treated as potentially sensitive alongsideUnauthenticated,PermissionDenied, etc.
Summary
Fixes #3021
Supersedes #3108
Supersedes #3343
When gRPC/HTTP errors occur,
tonic::Statuswas Debug-formatted into error messages, leaking Bearer tokens echoed back by the server. The HTTP exporter similarly included full response bodies in error messages.Changes
gRPC exporters — Two
macro_rules!macros (handle_tonic_export_error!,handle_interceptor_error!) defined once intonic/mod.rsand used by all three signal exporters (traces, metrics, logs):Unavailable,DeadlineExceeded,ResourceExhausted,Aborted,Cancelled): gRPC code + message at DEBUGUnknown,Unauthenticated,PermissionDenied, etc.): gRPC code, message, and details at DEBUG onlyOTelSdkErrorreturned to callers contains only the gRPC code, never the messageconcat!()(e.g.,TonicLogsClient.ExportFailed)otel_error!HTTP exporter — DEBUG-only logging:
HttpClient.NetworkErrorHttpClient.StatusError)otel_error!Review feedback addressed
From #3343:
InternalFailureand interceptor error messages (@utpilla, @lalitb)From #3360:
Unknownmoved to potentially sensitive errors — too broad, could carry sensitive server responses (@lalitb)otel_error!(@lalitb)Test plan
cargo clippy --all-features -- -D warningspassescargo test --all-features— 111 tests passcargo fmt -- --checkpasses