From a32efeabfafe5f6ea73721f46552b993f0a52b67 Mon Sep 17 00:00:00 2001 From: coryb Date: Sun, 11 Jul 2021 23:58:54 -0700 Subject: [PATCH] Add WithRedactedHeaders and WithoutHeaders options to NewClientTrace On testing we learned that sensitive information was being stored in the traces. To prevent the security leak several security or sensitive headers will now be redacted. These are the headers redacted by default: Authorization WWW-Authenticate Proxy-Authenticate Proxy-Authorization Cookie Set-Cookie Users can add more headers to redact with WithRedactedHeaders. To disable adding headers to the span entirely users can use WithoutHeaders. Signed-off-by: coryb --- CHANGELOG.md | 6 +- .../httptrace/otelhttptrace/clienttrace.go | 63 ++++++++++++-- .../otelhttptrace/clienttrace_test.go | 83 ++++++++++++++++++- 3 files changed, 143 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 46684e0b29a..046cfe9136a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,9 +14,13 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm The `messaging.message_id` and `messaging.kafka.partition` attributes are now not set if a message was not processed. (#754) (#755) - Fix `otelsarama.WrapAsyncProducer` so that the messages from the `Errors` channel contain the original `Metadata`. (#754) +### Added + +- Options `WithoutSubSpans`, `WithRedactedHeaders`, `WithoutHeaders`, and `WithInsecureHeaders` added to `NewClientTrace` in the `instrumentation/net/http/httptrace/otelhttptrace` package. (#879) + ### Changed -- Added `WithoutSubSpans` option to `NewClientTrace` in the `instrumentation/net/http/httptrace/otelhttptrace` package (#879) +- `instrumentation/net/http/httptrace/otelhttptrace` will now redact known sensitive headers by default. (#879) ## [0.21.0] - 2021-06-18 diff --git a/instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go b/instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go index 52f45a8f76b..843b53eb357 100644 --- a/instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go +++ b/instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go @@ -73,22 +73,65 @@ func WithoutSubSpans() ClientTraceOption { } } +// WithRedactedHeaders will be replaced by fixed '****' values for the header +// names provided. These are in addition to the sensitive headers already +// redacted by default: Authorization, WWW-Authenticate, Proxy-Authenticate +// Proxy-Authorization, Cookie, Set-Cookie +func WithRedactedHeaders(headers ...string) ClientTraceOption { + return func(ct *clientTracer) { + for _, header := range headers { + ct.redactedHeaders[strings.ToLower(header)] = struct{}{} + } + } +} + +// WithoutHeaders will disable adding span attributes for the http headers +// and values. +func WithoutHeaders() ClientTraceOption { + return func(ct *clientTracer) { + ct.addHeaders = false + } +} + +// WithInsecureHeaders will add span attributes for all http headers *INCLUDING* +// the sensitive headers that are redacted by default. The attribute values +// will include the raw un-redacted text. This might be useful for +// debugging authentication related issues, but should not be used for +// production deployments. +func WithInsecureHeaders() ClientTraceOption { + return func(ct *clientTracer) { + ct.addHeaders = true + ct.redactedHeaders = nil + } +} + type clientTracer struct { context.Context tr trace.Tracer - activeHooks map[string]context.Context - root trace.Span - mtx sync.Mutex - useSpans bool + activeHooks map[string]context.Context + root trace.Span + mtx sync.Mutex + redactedHeaders map[string]struct{} + addHeaders bool + useSpans bool } func NewClientTrace(ctx context.Context, opts ...ClientTraceOption) *httptrace.ClientTrace { ct := &clientTracer{ Context: ctx, activeHooks: make(map[string]context.Context), - useSpans: true, + redactedHeaders: map[string]struct{}{ + "authorization": {}, + "www-authenticate": {}, + "proxy-authenticate": {}, + "proxy-authorization": {}, + "cookie": {}, + "set-cookie": {}, + }, + addHeaders: true, + useSpans: true, } for _, opt := range opts { opt(ct) @@ -258,7 +301,15 @@ func (ct *clientTracer) wroteHeaderField(k string, v []string) { if ct.useSpans && ct.span("http.headers") == nil { ct.start("http.headers", "http.headers") } - ct.root.SetAttributes(attribute.String("http."+strings.ToLower(k), sliceToString(v))) + if !ct.addHeaders { + return + } + k = strings.ToLower(k) + value := sliceToString(v) + if _, ok := ct.redactedHeaders[k]; ok { + value = "****" + } + ct.root.SetAttributes(attribute.String("http."+k, value)) } func (ct *clientTracer) wroteHeaders() { diff --git a/instrumentation/net/http/httptrace/otelhttptrace/clienttrace_test.go b/instrumentation/net/http/httptrace/otelhttptrace/clienttrace_test.go index fb4b9ba99d2..e28542ae3ac 100644 --- a/instrumentation/net/http/httptrace/otelhttptrace/clienttrace_test.go +++ b/instrumentation/net/http/httptrace/otelhttptrace/clienttrace_test.go @@ -255,7 +255,7 @@ func TestEndBeforeStartCreatesSpan(t *testing.T) { require.Len(t, spans, 1) } -func TestWithoutSubSpans(t *testing.T) { +func TestClientTraceOptions(t *testing.T) { sr := &oteltest.SpanRecorder{} otel.SetTracerProvider( oteltest.NewTracerProvider(oteltest.WithSpanRecorder(sr)), @@ -269,6 +269,8 @@ func TestWithoutSubSpans(t *testing.T) { defer ts.Close() address := ts.Listener.Addr().String() + // Test WithoutSubSpans + ctx := context.Background() ctx = httptrace.WithClientTrace(ctx, otelhttptrace.NewClientTrace(ctx, @@ -293,6 +295,7 @@ func TestWithoutSubSpans(t *testing.T) { ) req, err = http.NewRequestWithContext(ctx, http.MethodGet, ts.URL, nil) req.Header.Set("User-Agent", "oteltest/1.1") + req.Header.Set("Authorization", "Bearer token123") require.NoError(t, err) resp, err = ts.Client().Do(req) require.NoError(t, err) @@ -303,7 +306,7 @@ func TestWithoutSubSpans(t *testing.T) { recSpan := sr.Completed()[0] gotAttributes := recSpan.Attributes() - require.Len(t, gotAttributes, 3) + require.Len(t, gotAttributes, 4) assert.Equal(t, attribute.StringValue("gzip"), gotAttributes[attribute.Key("http.accept-encoding")], @@ -312,6 +315,11 @@ func TestWithoutSubSpans(t *testing.T) { attribute.StringValue("oteltest/1.1"), gotAttributes[attribute.Key("http.user-agent")], ) + // verify redacted auth headers + assert.Equal(t, + attribute.StringValue("****"), + gotAttributes[attribute.Key("http.authorization")], + ) assert.Equal(t, attribute.StringValue(address), gotAttributes[attribute.Key("http.host")], @@ -364,4 +372,75 @@ func TestWithoutSubSpans(t *testing.T) { }) } } + + // Test WithRedactedHeaders + + ctx, span = otel.Tracer("oteltest").Start(context.Background(), "root") + ctx = httptrace.WithClientTrace(ctx, + otelhttptrace.NewClientTrace(ctx, + otelhttptrace.WithoutSubSpans(), + otelhttptrace.WithRedactedHeaders("user-agent"), + ), + ) + req, err = http.NewRequestWithContext(ctx, http.MethodGet, ts.URL, nil) + require.NoError(t, err) + resp, err = ts.Client().Do(req) + require.NoError(t, err) + resp.Body.Close() + span.End() + require.Len(t, sr.Completed(), 2) + recSpan = sr.Completed()[1] + + gotAttributes = recSpan.Attributes() + assert.Equal(t, + attribute.StringValue("****"), + gotAttributes[attribute.Key("http.user-agent")], + ) + + // Test WithoutHeaders + + ctx, span = otel.Tracer("oteltest").Start(context.Background(), "root") + ctx = httptrace.WithClientTrace(ctx, + otelhttptrace.NewClientTrace(ctx, + otelhttptrace.WithoutSubSpans(), + otelhttptrace.WithoutHeaders(), + ), + ) + req, err = http.NewRequestWithContext(ctx, http.MethodGet, ts.URL, nil) + require.NoError(t, err) + resp, err = ts.Client().Do(req) + require.NoError(t, err) + resp.Body.Close() + span.End() + require.Len(t, sr.Completed(), 3) + recSpan = sr.Completed()[2] + + gotAttributes = recSpan.Attributes() + require.Len(t, gotAttributes, 0) + + // Test WithInsecureHeaders + + ctx, span = otel.Tracer("oteltest").Start(context.Background(), "root") + ctx = httptrace.WithClientTrace(ctx, + otelhttptrace.NewClientTrace(ctx, + otelhttptrace.WithoutSubSpans(), + otelhttptrace.WithInsecureHeaders(), + ), + ) + req, err = http.NewRequestWithContext(ctx, http.MethodGet, ts.URL, nil) + req.Header.Set("Authorization", "Bearer token123") + require.NoError(t, err) + resp, err = ts.Client().Do(req) + require.NoError(t, err) + resp.Body.Close() + span.End() + require.Len(t, sr.Completed(), 4) + recSpan = sr.Completed()[3] + + gotAttributes = recSpan.Attributes() + gotAttributes = recSpan.Attributes() + assert.Equal(t, + attribute.StringValue("Bearer token123"), + gotAttributes[attribute.Key("http.authorization")], + ) }