From 265ebead0311d94380048c29f36c8081c39211e3 Mon Sep 17 00:00:00 2001 From: coryb Date: Thu, 2 Sep 2021 09:03:33 -0700 Subject: [PATCH] Add WithoutSubSpans option to NewClientTrace (#879) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add WithoutSubSpans option to NewClientTrace This adds an optional behavior to NewClientTrace so that it only adds events and annotations to the span found in SpanFromContext. The default behavior remains unchanged, several sub-spans will be created for each request. This optional behavior is useful when tracing "complex" processes that have many http calls. In this case the added sub-spans become "noise" and may distract from the overall trace. This also adds several useful attributes from data available in the httptrace callbacks: http.conn.reused - bool if connection was reused http.conn.wasidle - bool if connection was idle http.conn.idletime - if wasidle, duration of idletime http.conn.start.network - start connection network type [tcp/udp] http.conn.done.network - end connection network type [tcp/udp] http.conn.done.addr - connection address when done http.dns.addrs - list of addrs returned from dns lookup Signed-off-by: coryb * 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 Co-authored-by: Robert Pająk Co-authored-by: Robert Pająk Co-authored-by: Anthony Mirabella --- CHANGELOG.md | 5 + .../httptrace/otelhttptrace/clienttrace.go | 174 ++++++++++++-- .../otelhttptrace/test/clienttrace_test.go | 224 ++++++++++++++++++ 3 files changed, 383 insertions(+), 20 deletions(-) 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, + ) +}