diff --git a/contrib/google.golang.org/api/api_test.go b/contrib/google.golang.org/api/api_test.go index d5104b9690..a9b8f54347 100644 --- a/contrib/google.golang.org/api/api_test.go +++ b/contrib/google.golang.org/api/api_test.go @@ -63,7 +63,7 @@ func TestBooks(t *testing.T) { assert.Equal(t, "books.bookshelves.list", s0.Tag(ext.ResourceName)) assert.Equal(t, "400", s0.Tag(ext.HTTPCode)) assert.Equal(t, "GET", s0.Tag(ext.HTTPMethod)) - assert.Equal(t, svc.BasePath+"books/v1/users/montana.banana/bookshelves", s0.Tag(ext.HTTPURL)) + assert.Equal(t, svc.BasePath+"books/v1/users/montana.banana/bookshelves?alt=json&prettyPrint=false", s0.Tag(ext.HTTPURL)) assert.Equal(t, "google.golang.org/api", s0.Tag(ext.Component)) assert.Equal(t, ext.SpanKindClient, s0.Tag(ext.SpanKind)) } @@ -88,7 +88,7 @@ func TestCivicInfo(t *testing.T) { assert.Equal(t, "civicinfo.representatives.representativeInfoByAddress", s0.Tag(ext.ResourceName)) assert.Equal(t, "400", s0.Tag(ext.HTTPCode)) assert.Equal(t, "GET", s0.Tag(ext.HTTPMethod)) - assert.Equal(t, svc.BasePath+"civicinfo/v2/representatives", s0.Tag(ext.HTTPURL)) + assert.Equal(t, svc.BasePath+"civicinfo/v2/representatives?alt=json&prettyPrint=false", s0.Tag(ext.HTTPURL)) assert.Equal(t, "google.golang.org/api", s0.Tag(ext.Component)) assert.Equal(t, ext.SpanKindClient, s0.Tag(ext.SpanKind)) } @@ -115,7 +115,7 @@ func TestURLShortener(t *testing.T) { assert.Equal(t, "urlshortener.url.list", s0.Tag(ext.ResourceName)) assert.Equal(t, "400", s0.Tag(ext.HTTPCode)) assert.Equal(t, "GET", s0.Tag(ext.HTTPMethod)) - assert.Equal(t, "https://www.googleapis.com/urlshortener/v1/url/history", s0.Tag(ext.HTTPURL)) + assert.Equal(t, "https://www.googleapis.com/urlshortener/v1/url/history?alt=json&prettyPrint=false", s0.Tag(ext.HTTPURL)) assert.Equal(t, "google.golang.org/api", s0.Tag(ext.Component)) assert.Equal(t, ext.SpanKindClient, s0.Tag(ext.SpanKind)) } @@ -140,7 +140,7 @@ func TestWithEndpointMetadataDisabled(t *testing.T) { assert.Equal(t, "GET civicinfo.googleapis.com", s0.Tag(ext.ResourceName)) assert.Equal(t, "400", s0.Tag(ext.HTTPCode)) assert.Equal(t, "GET", s0.Tag(ext.HTTPMethod)) - assert.Equal(t, svc.BasePath+"civicinfo/v2/representatives", s0.Tag(ext.HTTPURL)) + assert.Equal(t, svc.BasePath+"civicinfo/v2/representatives?alt=json&prettyPrint=false", s0.Tag(ext.HTTPURL)) assert.Equal(t, "google.golang.org/api", s0.Tag(ext.Component)) assert.Equal(t, ext.SpanKindClient, s0.Tag(ext.SpanKind)) } diff --git a/contrib/internal/httptrace/config.go b/contrib/internal/httptrace/config.go index 57d9a05c98..2ef253088c 100644 --- a/contrib/internal/httptrace/config.go +++ b/contrib/internal/httptrace/config.go @@ -46,7 +46,7 @@ func ResetCfg() { func newConfig() config { c := config{ queryString: !internal.BoolEnv(envQueryStringDisabled, false), - queryStringRegexp: defaultQueryStringRegexp, + queryStringRegexp: QueryStringRegexp(), traceClientIP: internal.BoolEnv(envTraceClientIPEnabled, false), isStatusError: isServerError, } @@ -54,21 +54,25 @@ func newConfig() config { if fn := GetErrorCodesFromInput(v); fn != nil { c.isStatusError = fn } + return c +} + +func isServerError(statusCode int) bool { + return statusCode >= 500 && statusCode < 600 +} + +func QueryStringRegexp() *regexp.Regexp { if s, ok := os.LookupEnv(envQueryStringRegexp); !ok { - return c + return defaultQueryStringRegexp } else if s == "" { - c.queryStringRegexp = nil log.Debug("%s is set but empty. Query string obfuscation will be disabled.", envQueryStringRegexp) + return nil } else if r, err := regexp.Compile(s); err == nil { - c.queryStringRegexp = r + return r } else { log.Error("Could not compile regexp from %s. Using default regexp instead.", envQueryStringRegexp) + return defaultQueryStringRegexp } - return c -} - -func isServerError(statusCode int) bool { - return statusCode >= 500 && statusCode < 600 } // GetErrorCodesFromInput parses a comma-separated string s to determine which codes are to be considered errors diff --git a/contrib/internal/httptrace/httptrace.go b/contrib/internal/httptrace/httptrace.go index 4aad5aba38..e5ad7d9d64 100644 --- a/contrib/internal/httptrace/httptrace.go +++ b/contrib/internal/httptrace/httptrace.go @@ -38,27 +38,27 @@ func StartRequestSpan(r *http.Request, opts ...ddtrace.StartSpanOption) (tracer. } nopts := make([]ddtrace.StartSpanOption, 0, len(opts)+1+len(ipTags)) nopts = append(nopts, - func(cfg *ddtrace.StartSpanConfig) { - if cfg.Tags == nil { - cfg.Tags = make(map[string]interface{}) + func(ssCfg *ddtrace.StartSpanConfig) { + if ssCfg.Tags == nil { + ssCfg.Tags = make(map[string]interface{}) } - cfg.Tags[ext.SpanType] = ext.SpanTypeWeb - cfg.Tags[ext.HTTPMethod] = r.Method - cfg.Tags[ext.HTTPURL] = urlFromRequest(r) - cfg.Tags[ext.HTTPUserAgent] = r.UserAgent() - cfg.Tags["_dd.measured"] = 1 + ssCfg.Tags[ext.SpanType] = ext.SpanTypeWeb + ssCfg.Tags[ext.HTTPMethod] = r.Method + ssCfg.Tags[ext.HTTPURL] = UrlFromRequest(r, cfg.queryString) + ssCfg.Tags[ext.HTTPUserAgent] = r.UserAgent() + ssCfg.Tags["_dd.measured"] = 1 if r.Host != "" { - cfg.Tags["http.host"] = r.Host + ssCfg.Tags["http.host"] = r.Host } if spanctx, err := tracer.Extract(tracer.HTTPHeadersCarrier(r.Header)); err == nil { // If there are span links as a result of context extraction, add them as a StartSpanOption if linksCtx, ok := spanctx.(ddtrace.SpanContextWithLinks); ok && linksCtx.SpanLinks() != nil { - tracer.WithSpanLinks(linksCtx.SpanLinks())(cfg) + tracer.WithSpanLinks(linksCtx.SpanLinks())(ssCfg) } - tracer.ChildOf(spanctx)(cfg) + tracer.ChildOf(spanctx)(ssCfg) } for k, v := range ipTags { - cfg.Tags[k] = v + ssCfg.Tags[k] = v } }) nopts = append(nopts, opts...) @@ -93,18 +93,19 @@ func FinishRequestSpan(s tracer.Span, status int, errorFn func(int) bool, opts . s.Finish(opts...) } -// urlFromRequest returns the full URL from the HTTP request. If query params are collected, they are obfuscated granted -// obfuscation is not disabled by the user (through DD_TRACE_OBFUSCATION_QUERY_STRING_REGEXP) -// See https://docs.datadoghq.com/tracing/configure_data_security#redacting-the-query-in-the-url for more information. -func urlFromRequest(r *http.Request) string { +// UrlFromRequest returns the full URL from the HTTP request. If queryString is true, params are collected and they are obfuscated either by the default query string obfuscator or the custom obfuscator provided by the user (through DD_TRACE_OBFUSCATION_QUERY_STRING_REGEXP) +// See https://docs.datadoghq.com/tracing/configure_data_security/?tab=net#redact-query-strings for more information. +func UrlFromRequest(r *http.Request, queryString bool) string { // Quoting net/http comments about net.Request.URL on server requests: // "For most requests, fields other than Path and RawQuery will be // empty. (See RFC 7230, Section 5.3)" - // This is why we don't rely on url.URL.String(), url.URL.Host, url.URL.Scheme, etc... + // This is why we can't rely entirely on url.URL.String(), url.URL.Host, url.URL.Scheme, etc... var url string path := r.URL.EscapedPath() scheme := "http" - if r.TLS != nil { + if s := r.URL.Scheme; s != "" { + scheme = s + } else if r.TLS != nil { scheme = "https" } if r.Host != "" { @@ -113,7 +114,7 @@ func urlFromRequest(r *http.Request) string { url = path } // Collect the query string if we are allowed to report it and obfuscate it if possible/allowed - if cfg.queryString && r.URL.RawQuery != "" { + if queryString && r.URL.RawQuery != "" { query := r.URL.RawQuery if cfg.queryStringRegexp != nil { query = cfg.queryStringRegexp.ReplaceAllLiteralString(query, "") diff --git a/contrib/internal/httptrace/httptrace_test.go b/contrib/internal/httptrace/httptrace_test.go index 22de28c62d..4dbd98d793 100644 --- a/contrib/internal/httptrace/httptrace_test.go +++ b/contrib/internal/httptrace/httptrace_test.go @@ -345,7 +345,7 @@ func TestURLTag(t *testing.T) { if tc.port != "" { r.Host += ":" + tc.port } - url := urlFromRequest(&r) + url := UrlFromRequest(&r, true) require.Equal(t, tc.expectedURL, url) }) } diff --git a/contrib/net/http/option.go b/contrib/net/http/option.go index a862fc605a..4d874a3635 100644 --- a/contrib/net/http/option.go +++ b/contrib/net/http/option.go @@ -26,6 +26,8 @@ const ( envClientQueryStringEnabled = "DD_TRACE_HTTP_CLIENT_TAG_QUERY_STRING" // envClientErrorStatuses is the name of the env var that specifies error status codes on http client spans envClientErrorStatuses = "DD_TRACE_HTTP_CLIENT_ERROR_STATUSES" + // envQueryStringRegexp is the name of the env var used to specify the regexp to use for query string obfuscation. + envQueryStringRegexp = "DD_TRACE_OBFUSCATION_QUERY_STRING_REGEXP" ) type config struct { @@ -183,7 +185,7 @@ func newRoundTripperConfig() *roundTripperConfig { propagation: true, spanNamer: defaultSpanNamer, ignoreRequest: func(_ *http.Request) bool { return false }, - queryString: internal.BoolEnv(envClientQueryStringEnabled, false), + queryString: internal.BoolEnv(envClientQueryStringEnabled, true), isStatusError: isClientError, } v := os.Getenv(envClientErrorStatuses) diff --git a/contrib/net/http/roundtripper.go b/contrib/net/http/roundtripper.go index b9ee812508..c9b64236d5 100644 --- a/contrib/net/http/roundtripper.go +++ b/contrib/net/http/roundtripper.go @@ -11,9 +11,9 @@ import ( "net/http" "os" "strconv" - "strings" "gopkg.in/DataDog/dd-trace-go.v1/appsec/events" + "gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" @@ -39,7 +39,7 @@ func (rt *roundTripper) RoundTrip(req *http.Request) (res *http.Response, err er tracer.SpanType(ext.SpanTypeHTTP), tracer.ResourceName(resourceName), tracer.Tag(ext.HTTPMethod, req.Method), - tracer.Tag(ext.HTTPURL, urlFromRequest(req, rt.cfg.queryString)), + tracer.Tag(ext.HTTPURL, httptrace.UrlFromRequest(req, rt.cfg.queryString)), tracer.Tag(ext.Component, componentName), tracer.Tag(ext.SpanKind, ext.SpanKindClient), tracer.Tag(ext.NetworkDestinationName, url.Hostname()), @@ -134,32 +134,3 @@ func WrapClient(c *http.Client, opts ...RoundTripperOption) *http.Client { c.Transport = WrapRoundTripper(c.Transport, opts...) return c } - -// urlFromRequest returns the URL from the HTTP request. The URL query string is included in the return object iff queryString is true -// See https://docs.datadoghq.com/tracing/configure_data_security#redacting-the-query-in-the-url for more information. -func urlFromRequest(r *http.Request, queryString bool) string { - // Quoting net/http comments about net.Request.URL on server requests: - // "For most requests, fields other than Path and RawQuery will be - // empty. (See RFC 7230, Section 5.3)" - // This is why we don't rely on url.URL.String(), url.URL.Host, url.URL.Scheme, etc... - var url string - path := r.URL.EscapedPath() - scheme := r.URL.Scheme - if r.TLS != nil { - scheme = "https" - } - if r.Host != "" { - url = strings.Join([]string{scheme, "://", r.Host, path}, "") - } else { - url = path - } - // Collect the query string if we are allowed to report it and obfuscate it if possible/allowed - if queryString && r.URL.RawQuery != "" { - query := r.URL.RawQuery - url = strings.Join([]string{url, query}, "?") - } - if frag := r.URL.EscapedFragment(); frag != "" { - url = strings.Join([]string{url, frag}, "#") - } - return url -} diff --git a/contrib/net/http/roundtripper_test.go b/contrib/net/http/roundtripper_test.go index a15904fb7f..1b9cbbcb3c 100644 --- a/contrib/net/http/roundtripper_test.go +++ b/contrib/net/http/roundtripper_test.go @@ -566,12 +566,12 @@ func TestSpanOptions(t *testing.T) { assert.Equal(t, tagValue, spans[0].Tag(tagKey)) } -func TestClientQueryString(t *testing.T) { +func TestClientQueryStringCollected(t *testing.T) { s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Write([]byte("Hello World")) })) defer s.Close() - t.Run("default", func(t *testing.T) { + t.Run("default true", func(t *testing.T) { mt := mocktracer.Start() defer mt.Stop() @@ -579,19 +579,19 @@ func TestClientQueryString(t *testing.T) { client := &http.Client{ Transport: rt, } - resp, err := client.Get(s.URL + "/hello/world?API_KEY=1234") + resp, err := client.Get(s.URL + "/hello/world?something=fun") assert.Nil(t, err) defer resp.Body.Close() spans := mt.FinishedSpans() assert.Len(t, spans, 1) - assert.Regexp(t, regexp.MustCompile(`^http://.*?/hello/world$`), spans[0].Tag(ext.HTTPURL)) + assert.Regexp(t, regexp.MustCompile(`^http://.*?/hello/world\?something=fun$`), spans[0].Tag(ext.HTTPURL)) }) - t.Run("true", func(t *testing.T) { + t.Run("false", func(t *testing.T) { mt := mocktracer.Start() defer mt.Stop() - os.Setenv("DD_TRACE_HTTP_CLIENT_TAG_QUERY_STRING", "true") + os.Setenv("DD_TRACE_HTTP_CLIENT_TAG_QUERY_STRING", "false") defer os.Unsetenv("DD_TRACE_HTTP_CLIENT_TAG_QUERY_STRING") rt := WrapRoundTripper(http.DefaultTransport) @@ -604,27 +604,85 @@ func TestClientQueryString(t *testing.T) { spans := mt.FinishedSpans() assert.Len(t, spans, 1) - assert.Regexp(t, regexp.MustCompile(`^http://.*?/hello/world\?querystring=xyz$`), spans[0].Tag(ext.HTTPURL)) + assert.Regexp(t, regexp.MustCompile(`^http://.*?/hello/world$`), spans[0].Tag(ext.HTTPURL)) }) // DD_TRACE_HTTP_URL_QUERY_STRING_DISABLED applies only to server spans, not client t.Run("Not impacted by DD_TRACE_HTTP_URL_QUERY_STRING_DISABLED", func(t *testing.T) { mt := mocktracer.Start() defer mt.Stop() - os.Setenv("DD_TRACE_HTTP_URL_QUERY_STRING_DISABLED", "false") - defer os.Unsetenv("DD_TRACE_HTTP_URL_QUERY_STRING_DISABLED") + t.Setenv("DD_TRACE_HTTP_URL_QUERY_STRING_DISABLED", "false") rt := WrapRoundTripper(http.DefaultTransport) client := &http.Client{ Transport: rt, } - resp, err := client.Get(s.URL + "/hello/world?API_KEY=1234") + resp, err := client.Get(s.URL + "/hello/world?something=fun") assert.Nil(t, err) defer resp.Body.Close() spans := mt.FinishedSpans() assert.Len(t, spans, 1) - assert.Regexp(t, regexp.MustCompile(`^http://.*?/hello/world$`), spans[0].Tag(ext.HTTPURL)) + assert.Regexp(t, regexp.MustCompile(`^http://.*?/hello/world\?something=fun$`), spans[0].Tag(ext.HTTPURL)) + }) +} + +func TestClientQueryStringObfuscated(t *testing.T) { + s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte("Hello World")) + })) + defer s.Close() + t.Run("default", func(t *testing.T) { + mt := mocktracer.Start() + defer mt.Stop() + + rt := WrapRoundTripper(http.DefaultTransport) + client := &http.Client{ + Transport: rt, + } + resp, err := client.Get(s.URL + "/hello/world?token=value") + assert.Nil(t, err) + defer resp.Body.Close() + spans := mt.FinishedSpans() + assert.Len(t, spans, 1) + + assert.Regexp(t, regexp.MustCompile(`^http://.*?/hello/world\?$`), spans[0].Tag(ext.HTTPURL)) + }) + t.Run("empty", func(t *testing.T) { + mt := mocktracer.Start() + defer mt.Stop() + + t.Setenv(envQueryStringRegexp, "") + + rt := WrapRoundTripper(http.DefaultTransport) + client := &http.Client{ + Transport: rt, + } + resp, err := client.Get(s.URL + "/hello/world?custom=xyz") + assert.Nil(t, err) + defer resp.Body.Close() + spans := mt.FinishedSpans() + assert.Len(t, spans, 1) + + assert.Regexp(t, regexp.MustCompile(`^http://.*?/hello/world\?custom=xyz$`), spans[0].Tag(ext.HTTPURL)) + }) + t.Run("custom", func(t *testing.T) { + mt := mocktracer.Start() + defer mt.Stop() + + t.Setenv(envQueryStringRegexp, "^custom") + + rt := WrapRoundTripper(http.DefaultTransport) + client := &http.Client{ + Transport: rt, + } + resp, err := client.Get(s.URL + "/hello/world?token=value") + assert.Nil(t, err) + defer resp.Body.Close() + spans := mt.FinishedSpans() + assert.Len(t, spans, 1) + + assert.Regexp(t, regexp.MustCompile(`^http://.*?/hello/world\?$`), spans[0].Tag(ext.HTTPURL)) }) }