contrib/net/http: Obfuscate query strings on http client spans (#3071)

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))
 }

contrib/internal/httptrace/config.go

@@ -46,29 +46,33 @@ func ResetCfg() {
 func newConfig() config {
 	c := config{
 		queryString:       !internal.BoolEnv(envQueryStringDisabled, false),
-		queryStringRegexp: defaultQueryStringRegexp,
+		queryStringRegexp: QueryStringRegexp(),
 		traceClientIP:     internal.BoolEnv(envTraceClientIPEnabled, false),
 		isStatusError:     isServerError,
 	}
 	v := os.Getenv(envServerErrorStatuses)
 	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

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, "<redacted>")

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)
 		})
 	}

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)

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
-}

contrib/net/http/roundtripper_test.go

@@ -566,32 +566,32 @@ 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()
 
 		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))
 	})
-	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\?<redacted>$`), 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\?<redacted>$`), spans[0].Tag(ext.HTTPURL))
 	})
 }