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 <cbennett@netflix.com>
Co-authored-by: Robert Pająk <pellared@hotmail.com>

CHANGELOG.md

@@ -8,10 +8,15 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ## [Unreleased]
 
+### Added
+
+- Add `WithoutSubSpans`, `WithRedactedHeaders`, `WithoutHeaders`, and `WithInsecureHeaders` options for `otelhttptrace.NewClientTrace`. (#879)
+
 ### Changed
 
 - Split `go.opentelemetry.io/contrib/propagators` module into `b3`, `jaeger`, `ot` modules. (#985)
 - `otelmongodb` span attributes, name and span status now conform to specification. (#769)
+- `otelhttptrace.NewClientTrace` now redacts known sensitive headers by default. (#879)
 
 ### Fixed
 
@@ -34,10 +39,6 @@ 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) (#881)
 - Fix `otelsarama.WrapAsyncProducer` so that the messages from the `Errors` channel contain the original `Metadata`. (#754)
 
-### Changed
-
-- Added `WithoutSubSpans` option to `NewClientTrace` in the `instrumentation/net/http/httptrace/otelhttptrace` package (#879)
-
 ## [0.21.0] - 2021-06-18
 
 ### Fixed

instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go

@@ -62,36 +62,94 @@ func parentHook(hook string) string {
 
 // ClientTraceOption allows customizations to how the httptrace.Client
 // collects information.
-type ClientTraceOption func(*clientTracer)
+type ClientTraceOption interface {
+	apply(*clientTracer)
+}
+
+type clientTraceOptionFunc func(*clientTracer)
+
+func (fn clientTraceOptionFunc) apply(c *clientTracer) {
+	fn(c)
+}
 
-// WithoutSubSpans will modify the httptrace.Client to only collect data
+// WithoutSubSpans will modify the httptrace.ClientTrace to only collect data
 // as Events and Attributes on a span found in the context.  By default
 // sub-spans will be generated.
 func WithoutSubSpans() ClientTraceOption {
-	return func(ct *clientTracer) {
+	return clientTraceOptionFunc(func(ct *clientTracer) {
 		ct.useSpans = false
-	}
+	})
+}
+
+// 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 clientTraceOptionFunc(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 clientTraceOptionFunc(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 clientTraceOptionFunc(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
 }
 
+// NewClientTrace returns an httptrace.ClientTrace implementation that will
+// record OpenTelemetry spans for requests made by an http.Client. By default
+// several spans will be added to the trace for various stages of a request
+// (dns, connection, tls, etc). Also by default, all HTTP headers will be
+// added as attributes to spans, although several headers will be automatically
+// redacted: Authorization, WWW-Authenticate, Proxy-Authenticate,
+// Proxy-Authorization, Cookie, and Set-Cookie.
 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)
+		opt.apply(ct)
 	}
 
 	ct.tr = otel.GetTracerProvider().Tracer(
@@ -258,7 +316,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() {

instrumentation/net/http/httptrace/otelhttptrace/test/clienttrace_test.go

@@ -259,33 +259,47 @@ func TestEndBeforeStartCreatesSpan(t *testing.T) {
 	require.Len(t, spans, 1)
 }
 
-func TestWithoutSubSpans(t *testing.T) {
-	sr := &oteltest.SpanRecorder{}
+type clientTraceTestFixture struct {
+	Address      string
+	URL          string
+	Client       *http.Client
+	SpanRecorder *tracetest.SpanRecorder
+}
+
+func prepareClientTraceTest(t *testing.T) clientTraceTestFixture {
+	fixture := clientTraceTestFixture{}
+	fixture.SpanRecorder = tracetest.NewSpanRecorder()
 	otel.SetTracerProvider(
-		oteltest.NewTracerProvider(oteltest.WithSpanRecorder(sr)),
+		trace.NewTracerProvider(trace.WithSpanProcessor(fixture.SpanRecorder)),
 	)
 
-	// Mock http server
 	ts := httptest.NewServer(
 		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		}),
 	)
-	defer ts.Close()
-	address := ts.Listener.Addr().String()
+	t.Cleanup(ts.Close)
+	fixture.Client = ts.Client()
+	fixture.URL = ts.URL
+	fixture.Address = ts.Listener.Addr().String()
+	return fixture
+}
+
+func TestWithoutSubSpans(t *testing.T) {
+	fixture := prepareClientTraceTest(t)
 
 	ctx := context.Background()
 	ctx = httptrace.WithClientTrace(ctx,
 		otelhttptrace.NewClientTrace(ctx,
 			otelhttptrace.WithoutSubSpans(),
 		),
 	)
-	req, err := http.NewRequestWithContext(ctx, http.MethodGet, ts.URL, nil)
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, fixture.URL, nil)
 	require.NoError(t, err)
-	resp, err := ts.Client().Do(req)
+	resp, err := fixture.Client.Do(req)
 	require.NoError(t, err)
 	resp.Body.Close()
 	// no spans created because we were just using background context without span
-	require.Len(t, sr.Completed(), 0)
+	require.Len(t, fixture.SpanRecorder.Ended(), 0)
 
 	// Start again with a "real" span in the context, now tracing should add
 	// events and annotations.
@@ -295,30 +309,28 @@ func TestWithoutSubSpans(t *testing.T) {
 			otelhttptrace.WithoutSubSpans(),
 		),
 	)
-	req, err = http.NewRequestWithContext(ctx, http.MethodGet, ts.URL, nil)
+	req, err = http.NewRequestWithContext(ctx, http.MethodGet, fixture.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)
+	resp, err = fixture.Client.Do(req)
 	require.NoError(t, err)
 	resp.Body.Close()
 	span.End()
 	// we just have the one span we created
-	require.Len(t, sr.Completed(), 1)
-	recSpan := sr.Completed()[0]
+	require.Len(t, fixture.SpanRecorder.Ended(), 1)
+	recSpan := fixture.SpanRecorder.Ended()[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")],
-	)
-	assert.Equal(t,
-		attribute.StringValue("oteltest/1.1"),
-		gotAttributes[attribute.Key("http.user-agent")],
-	)
-	assert.Equal(t,
-		attribute.StringValue(address),
-		gotAttributes[attribute.Key("http.host")],
+		[]attribute.KeyValue{
+			attribute.Key("http.host").String(fixture.Address),
+			attribute.Key("http.user-agent").String("oteltest/1.1"),
+			attribute.Key("http.authorization").String("****"),
+			attribute.Key("http.accept-encoding").String("gzip"),
+		},
+		gotAttributes,
 	)
 
 	type attrMap = map[attribute.Key]attribute.Value
@@ -328,7 +340,7 @@ func TestWithoutSubSpans(t *testing.T) {
 	}{
 		{"http.getconn.start", func(t *testing.T, got attrMap) {
 			assert.Equal(t,
-				attribute.StringValue(address),
+				attribute.StringValue(fixture.Address),
 				got[attribute.Key("http.host")],
 			)
 		}},
@@ -344,7 +356,7 @@ func TestWithoutSubSpans(t *testing.T) {
 				got[attribute.Key("http.conn.wasidle")],
 			)
 			assert.Equal(t,
-				attribute.StringValue(address),
+				attribute.StringValue(fixture.Address),
 				got[attribute.Key("http.remote")],
 			)
 			// value is dynamic, just verify we have the attribute
@@ -357,15 +369,105 @@ func TestWithoutSubSpans(t *testing.T) {
 	}
 	require.Len(t, recSpan.Events(), len(expectedEvents))
 	for i, e := range recSpan.Events() {
+		attrs := attrMap{}
+		for _, a := range e.Attributes {
+			attrs[a.Key] = a.Value
+		}
 		expected := expectedEvents[i]
 		assert.Equal(t, expected.Event, e.Name)
 		if expected.VerifyAttrs == nil {
 			assert.Nil(t, e.Attributes, "Event %q has no attributes", e.Name)
 		} else {
 			e := e // make loop var lexical
 			t.Run(e.Name, func(t *testing.T) {
-				expected.VerifyAttrs(t, e.Attributes)
+				expected.VerifyAttrs(t, attrs)
 			})
 		}
 	}
 }
+
+func TestWithRedactedHeaders(t *testing.T) {
+	fixture := prepareClientTraceTest(t)
+
+	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, fixture.URL, nil)
+	require.NoError(t, err)
+	resp, err := fixture.Client.Do(req)
+	require.NoError(t, err)
+	resp.Body.Close()
+	span.End()
+	require.Len(t, fixture.SpanRecorder.Ended(), 1)
+	recSpan := fixture.SpanRecorder.Ended()[0]
+
+	gotAttributes := recSpan.Attributes()
+	assert.Equal(t,
+		[]attribute.KeyValue{
+			attribute.Key("http.host").String(fixture.Address),
+			attribute.Key("http.user-agent").String("****"),
+			attribute.Key("http.accept-encoding").String("gzip"),
+		},
+		gotAttributes,
+	)
+}
+
+func TestWithoutHeaders(t *testing.T) {
+	fixture := prepareClientTraceTest(t)
+
+	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, fixture.URL, nil)
+	require.NoError(t, err)
+	resp, err := fixture.Client.Do(req)
+	require.NoError(t, err)
+	resp.Body.Close()
+	span.End()
+	require.Len(t, fixture.SpanRecorder.Ended(), 1)
+	recSpan := fixture.SpanRecorder.Ended()[0]
+
+	gotAttributes := recSpan.Attributes()
+	require.Len(t, gotAttributes, 0)
+}
+
+func TestWithInsecureHeaders(t *testing.T) {
+	fixture := prepareClientTraceTest(t)
+
+	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, fixture.URL, nil)
+	req.Header.Set("User-Agent", "oteltest/1.1")
+	req.Header.Set("Authorization", "Bearer token123")
+	require.NoError(t, err)
+	resp, err := fixture.Client.Do(req)
+	require.NoError(t, err)
+	resp.Body.Close()
+	span.End()
+	require.Len(t, fixture.SpanRecorder.Ended(), 1)
+	recSpan := fixture.SpanRecorder.Ended()[0]
+
+	gotAttributes := recSpan.Attributes()
+	assert.Equal(t,
+		[]attribute.KeyValue{
+			attribute.Key("http.host").String(fixture.Address),
+			attribute.Key("http.user-agent").String("oteltest/1.1"),
+			attribute.Key("http.authorization").String("Bearer token123"),
+			attribute.Key("http.accept-encoding").String("gzip"),
+		},
+		gotAttributes,
+	)
+}