feat(telemetry): implement secure telemetry logging with PII protection #3948
Conversation
Eliminate PII leakage in telemetry data sent to external services by enforcing
strict security controls on telemetry logging with comprehensive static analysis.
Redesigned telemetry Error API to require constant messages only, implemented
SafeError with automatic stack trace redaction, and created mission-critical
static checker rules with bulletproof scoping.
- Messages: Constant templates only (no dynamic strings/format verbs)
- Stack traces: Customer code frames replaced with "REDACTED" placeholder
- Error details: Only error type exposed, never error messages
- Collections: SafeSlice with explicit type conversions (no %v formatting)
- Static enforcement: Comprehensive ruleguard rules prevent all violations
SafeError extracts error types and redacts customer stack frames using contrib/
directory for frame classification. API breaking change from telemetrylog.Error(err)
to Error("message", SafeError(err)) ensures constant messages with secure error
details.
Signed-off-by: Kemal Akkoyun <[email protected]>
kakkoyun
force-pushed
the
kakkoyun/LANGPLAT-767/improve_telemtery_error_logs
branch
from
b572220 to
38893c1
Compare
There was a problem hiding this comment.
Wow this looks great thanks a bunch @kakkoyun
I think this will do it, I don't see anything that stands out.
I wasn't actually aware of WithTags being a thing, but they appear fine to me, I would like to follow up on that outside of this PR with the ASM team (I think they implemented it) to just figure out what the expectations are for that.
(don't take this as a go review 😛)
| @@ -175,14 +177,16 @@ func (m *WAFManager) RestoreDefaultConfig() error { | |||
| } | |||
|
|
|||
| func logLocalDiagnosticMessages(name string, feature *libddwaf.Feature) { | |||
| logger := telemetryLog.With(telemetry.WithTags([]string{"appsec_config_key:" + name, "log_type:local::diagnostic"})) | |||
There was a problem hiding this comment.
I think my understanding is that the WithTags is something that ASM designed / built into the telemetry logs.
I am actually not very familiar with them, but so far I think they are okay, they fall out of the message (I think) so hopefully they don't impact our deduplication logic.
I'll plan on following up on these though to find out more about how they work - it seems that ASM pushed for this and relies on it entirely as some of their logs don't get sent anywhere else.
| // This package implements strict security controls for telemetry logging to prevent | ||
| // PII and sensitive information from being sent to external telemetry services. |
There was a problem hiding this comment.
Awesome thanks 👍
One additional element is that constant messages also allows us to better deduplicate log messages to reduce how many of the same log we actually send
| redactedPlaceholder = "REDACTED" | ||
| ) | ||
|
|
||
| // TODO: Dynamically generate knownThirdPartyLibraries from contrib/ directory structure at build time |
There was a problem hiding this comment.
@quinna-h wondering if we maybe already have this or something like this? 😄
Just wondering, we don't need to act on this here.
There was a problem hiding this comment.
Wow this is a great addition!
|
|
||
| //go:build ruleguard | ||
|
|
||
| // Package gorules contains security-focused telemetry logging rules for dd-trace-go. |
There was a problem hiding this comment.
and an analyzer very nice 🚀 🎉
Eliminate PII leakage in telemetry data sent to external services by enforcing
strict security controls on telemetry logging with comprehensive static analysis.
Redesigned telemetry Error API to require constant messages only, implemented
SafeError with automatic stack trace redaction, and created mission-critical
static checker rules with bulletproof scoping.
SafeError extracts error types and redacts customer stack frames using contrib/
directory for frame classification. API breaking change from telemetrylog.Error(err)
to Error("message", SafeError(err)) ensures constant messages with secure error
details.
Signed-off-by: Kemal Akkoyun [email protected]
What does this PR do?
Motivation
Reviewer's Checklist
./scripts/lint.shlocally.Unsure? Have a question? Request a review!