diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c7099e02ae..b9ae10df577 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,11 +8,16 @@ 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) - Migrated EC2 resource detector support from root module `go.opentelemetry.io/contrib/detectors/aws` to a separate EC2 resource detector module `go.opentelemetry.io/contrib/detectors/aws/ec2` (#1017) +- `otelhttptrace.NewClientTrace` now redacts known sensitive headers by default. (#879) ### Fixed diff --git a/instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go b/instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go index e83cbf268d8..e2588a06d29 100644 --- a/instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go +++ b/instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go @@ -32,10 +32,17 @@ import ( // HTTP attributes. var ( - HTTPStatus = attribute.Key("http.status") - HTTPHeaderMIME = attribute.Key("http.mime") - HTTPRemoteAddr = attribute.Key("http.remote") - HTTPLocalAddr = attribute.Key("http.local") + HTTPStatus = attribute.Key("http.status") + HTTPHeaderMIME = attribute.Key("http.mime") + HTTPRemoteAddr = attribute.Key("http.remote") + HTTPLocalAddr = attribute.Key("http.local") + HTTPConnectionReused = attribute.Key("http.conn.reused") + HTTPConnectionWasIdle = attribute.Key("http.conn.wasidle") + HTTPConnectionIdleTime = attribute.Key("http.conn.idletime") + HTTPConnectionStartNetwork = attribute.Key("http.conn.start.network") + HTTPConnectionDoneNetwork = attribute.Key("http.conn.done.network") + HTTPConnectionDoneAddr = attribute.Key("http.conn.done.addr") + HTTPDNSAddrs = attribute.Key("http.dns.addrs") ) var ( @@ -53,20 +60,96 @@ func parentHook(hook string) string { return hookMap[hook] } +// ClientTraceOption allows customizations to how the httptrace.Client +// collects information. +type ClientTraceOption interface { + apply(*clientTracer) +} + +type clientTraceOptionFunc func(*clientTracer) + +func (fn clientTraceOptionFunc) apply(c *clientTracer) { + fn(c) +} + +// 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 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 + activeHooks map[string]context.Context + root trace.Span + mtx sync.Mutex + redactedHeaders map[string]struct{} + addHeaders bool + useSpans bool } -func NewClientTrace(ctx context.Context) *httptrace.ClientTrace { +// 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), + redactedHeaders: map[string]struct{}{ + "authorization": {}, + "www-authenticate": {}, + "proxy-authenticate": {}, + "proxy-authorization": {}, + "cookie": {}, + "set-cookie": {}, + }, + addHeaders: true, + useSpans: true, + } + for _, opt := range opts { + opt.apply(ct) } ct.tr = otel.GetTracerProvider().Tracer( @@ -95,6 +178,14 @@ func NewClientTrace(ctx context.Context) *httptrace.ClientTrace { } func (ct *clientTracer) start(hook, spanName string, attrs ...attribute.KeyValue) { + if !ct.useSpans { + if ct.root == nil { + ct.root = trace.SpanFromContext(ct.Context) + } + ct.root.AddEvent(hook+".start", trace.WithAttributes(attrs...)) + return + } + ct.mtx.Lock() defer ct.mtx.Unlock() @@ -115,6 +206,14 @@ func (ct *clientTracer) start(hook, spanName string, attrs ...attribute.KeyValue } func (ct *clientTracer) end(hook string, err error, attrs ...attribute.KeyValue) { + if !ct.useSpans { + if err != nil { + attrs = append(attrs, attribute.String(hook+".error", err.Error())) + } + ct.root.AddEvent(hook+".done", trace.WithAttributes(attrs...)) + return + } + ct.mtx.Lock() defer ct.mtx.Unlock() if ctx, ok := ct.activeHooks[hook]; ok { @@ -159,11 +258,16 @@ func (ct *clientTracer) getConn(host string) { } func (ct *clientTracer) gotConn(info httptrace.GotConnInfo) { - ct.end("http.getconn", - nil, + attrs := []attribute.KeyValue{ HTTPRemoteAddr.String(info.Conn.RemoteAddr().String()), HTTPLocalAddr.String(info.Conn.LocalAddr().String()), - ) + HTTPConnectionReused.Bool(info.Reused), + HTTPConnectionWasIdle.Bool(info.WasIdle), + } + if info.WasIdle { + attrs = append(attrs, HTTPConnectionIdleTime.String(info.IdleTime.String())) + } + ct.end("http.getconn", nil, attrs...) } func (ct *clientTracer) putIdleConn(err error) { @@ -179,15 +283,25 @@ func (ct *clientTracer) dnsStart(info httptrace.DNSStartInfo) { } func (ct *clientTracer) dnsDone(info httptrace.DNSDoneInfo) { - ct.end("http.dns", info.Err) + var addrs []string + for _, netAddr := range info.Addrs { + addrs = append(addrs, netAddr.String()) + } + ct.end("http.dns", info.Err, HTTPDNSAddrs.String(sliceToString(addrs))) } func (ct *clientTracer) connectStart(network, addr string) { - ct.start("http.connect."+addr, "http.connect", HTTPRemoteAddr.String(addr)) + ct.start("http.connect."+addr, "http.connect", + HTTPRemoteAddr.String(addr), + HTTPConnectionStartNetwork.String(network), + ) } func (ct *clientTracer) connectDone(network, addr string, err error) { - ct.end("http.connect."+addr, err) + ct.end("http.connect."+addr, err, + HTTPConnectionDoneAddr.String(addr), + HTTPConnectionDoneNetwork.String(network), + ) } func (ct *clientTracer) tlsHandshakeStart() { @@ -199,14 +313,22 @@ func (ct *clientTracer) tlsHandshakeDone(_ tls.ConnectionState, err error) { } func (ct *clientTracer) wroteHeaderField(k string, v []string) { - if ct.span("http.headers") == nil { + 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() { - if ct.span("http.headers") != nil { + if ct.useSpans && ct.span("http.headers") != nil { ct.end("http.headers", nil) } ct.start("http.send", "http.send") @@ -220,15 +342,27 @@ func (ct *clientTracer) wroteRequest(info httptrace.WroteRequestInfo) { } func (ct *clientTracer) got100Continue() { - ct.span("http.receive").AddEvent("GOT 100 - Continue") + span := ct.root + if ct.useSpans { + span = ct.span("http.receive") + } + span.AddEvent("GOT 100 - Continue") } func (ct *clientTracer) wait100Continue() { - ct.span("http.receive").AddEvent("GOT 100 - Wait") + span := ct.root + if ct.useSpans { + span = ct.span("http.receive") + } + span.AddEvent("GOT 100 - Wait") } func (ct *clientTracer) got1xxResponse(code int, header textproto.MIMEHeader) error { - ct.span("http.receive").AddEvent("GOT 1xx", trace.WithAttributes( + span := ct.root + if ct.useSpans { + span = ct.span("http.receive") + } + span.AddEvent("GOT 1xx", trace.WithAttributes( HTTPStatus.Int(code), HTTPHeaderMIME.String(sm2s(header)), )) diff --git a/instrumentation/net/http/httptrace/otelhttptrace/test/clienttrace_test.go b/instrumentation/net/http/httptrace/otelhttptrace/test/clienttrace_test.go index 4a62dbb0053..0df63aba276 100644 --- a/instrumentation/net/http/httptrace/otelhttptrace/test/clienttrace_test.go +++ b/instrumentation/net/http/httptrace/otelhttptrace/test/clienttrace_test.go @@ -91,6 +91,9 @@ func TestHTTPRequestWithClientTrace(t *testing.T) { { name: "http.connect", attributes: []attribute.KeyValue{ + attribute.Key("http.conn.done.addr").String(address.String()), + attribute.Key("http.conn.done.network").String("tcp"), + attribute.Key("http.conn.start.network").String("tcp"), attribute.Key("http.remote").String(address.String()), }, parent: "http.getconn", @@ -100,6 +103,8 @@ func TestHTTPRequestWithClientTrace(t *testing.T) { attributes: []attribute.KeyValue{ attribute.Key("http.remote").String(address.String()), attribute.Key("http.host").String(address.String()), + attribute.Key("http.conn.reused").Bool(false), + attribute.Key("http.conn.wasidle").Bool(false), }, parent: "test", }, @@ -214,7 +219,13 @@ func TestConcurrentConnectionStart(t *testing.T) { expectedRemotes := []attribute.KeyValue{ attribute.String("http.remote", "127.0.0.1:3000"), + attribute.String("http.conn.start.network", "tcp"), + attribute.String("http.conn.done.addr", "127.0.0.1:3000"), + attribute.String("http.conn.done.network", "tcp"), attribute.String("http.remote", "[::1]:3000"), + attribute.String("http.conn.start.network", "tcp"), + attribute.String("http.conn.done.addr", "[::1]:3000"), + attribute.String("http.conn.done.network", "tcp"), } for _, tt := range tts { t.Run(tt.name, func(t *testing.T) { @@ -247,3 +258,216 @@ func TestEndBeforeStartCreatesSpan(t *testing.T) { spans := getSpansFromRecorder(sr, name) require.Len(t, spans, 1) } + +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( + trace.NewTracerProvider(trace.WithSpanProcessor(fixture.SpanRecorder)), + ) + + ts := httptest.NewServer( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + }), + ) + 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, fixture.URL, nil) + require.NoError(t, err) + 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, fixture.SpanRecorder.Ended(), 0) + + // Start again with a "real" span in the context, now tracing should add + // events and annotations. + ctx, span := otel.Tracer("oteltest").Start(context.Background(), "root") + ctx = httptrace.WithClientTrace(ctx, + otelhttptrace.NewClientTrace(ctx, + otelhttptrace.WithoutSubSpans(), + ), + ) + 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() + // we just have the one span we created + require.Len(t, fixture.SpanRecorder.Ended(), 1) + recSpan := fixture.SpanRecorder.Ended()[0] + + gotAttributes := recSpan.Attributes() + require.Len(t, gotAttributes, 4) + 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("****"), + attribute.Key("http.accept-encoding").String("gzip"), + }, + gotAttributes, + ) + + type attrMap = map[attribute.Key]attribute.Value + expectedEvents := []struct { + Event string + VerifyAttrs func(t *testing.T, got attrMap) + }{ + {"http.getconn.start", func(t *testing.T, got attrMap) { + assert.Equal(t, + attribute.StringValue(fixture.Address), + got[attribute.Key("http.host")], + ) + }}, + {"http.getconn.done", func(t *testing.T, got attrMap) { + // value is dynamic, just verify we have the attribute + assert.Contains(t, got, attribute.Key("http.conn.idletime")) + assert.Equal(t, + attribute.BoolValue(true), + got[attribute.Key("http.conn.reused")], + ) + assert.Equal(t, + attribute.BoolValue(true), + got[attribute.Key("http.conn.wasidle")], + ) + assert.Equal(t, + attribute.StringValue(fixture.Address), + got[attribute.Key("http.remote")], + ) + // value is dynamic, just verify we have the attribute + assert.Contains(t, got, attribute.Key("http.local")) + }}, + {"http.send.start", nil}, + {"http.send.done", nil}, + {"http.receive.start", nil}, + {"http.receive.done", nil}, + } + 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, 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, + ) +}