feat(asm): redact sensitive querystrings in http.url (#3980)

## Description
By default query strings are not sent, unless `trace_query_string` is enabled.
In case `trace_query_string` is enabled, sensitive query strings will be redacted to be sent in http.url tag.

## Checklist
- [x] Title must conform to [conventional commit](https://github.com/conventional-changelog/commitlint/tree/master/%40commitlint/config-conventional).
- [x] Add additional sections for `feat` and `fix` pull requests.
- [x] Ensure tests are passing for affected code.
- [ ] [Library documentation](https://github.com/DataDog/dd-trace-py/tree/1.x/docs) and/or [Datadog's documentation site](https://github.com/DataDog/documentation/) is updated. Link to doc PR in description.

## Motivation
ASM will enable security analysis for the query strings in the backend, so query strings will be sent by default once sensitive query strings are redacted.

## Design 
This PR enables sensitive query strings redaction, next step will be enabling `trace_query_string` by default.

## Testing strategy
- A test added checking with an example of a sensitive query string.
- System tests for sensitive query string obfuscation.
- Performance test to evaluate re.sub overhead.

## Reviewer Checklist
- [ ] Title is accurate.
- [ ] Description motivates each change.
- [ ] No unnecessary changes were introduced in this PR.
- [ ] PR cannot be broken up into smaller PRs.
- [ ] Avoid breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes unless absolutely necessary.
- [ ] Tests provided or description of manual testing performed is included in the code or PR.
- [ ] Release note has been added for fixes and features, or else `changelog/no-changelog` label added.
- [ ] All relevant GitHub issues are correctly linked.
- [ ] Backports are identified and tagged with Mergifyio.
- [ ] Add to milestone.

Author: gnufede

ddtrace/contrib/aiohttp/patch.py

@@ -77,7 +77,7 @@ async def _traced_clientsession_request(aiohttp, pin, func, instance, args, kwar
             span,
             config.aiohttp_client,
             method=method,
-            url=url_str,
+            url=str(url),
             query=parsed_url.query,
             request_headers=headers,
         )

ddtrace/contrib/trace_utils.py

@@ -28,6 +28,7 @@
 from ddtrace.internal.utils.cache import cached
 from ddtrace.internal.utils.formats import asbool
 from ddtrace.internal.utils.http import normalize_header_name
+from ddtrace.internal.utils.http import redact_url
 from ddtrace.internal.utils.http import strip_query_string
 import ddtrace.internal.utils.wrappers
 from ddtrace.propagation.http import HTTPPropagator
@@ -419,7 +420,10 @@ def set_http_meta(
         span._set_str_tag(http.METHOD, method)
 
     if url is not None:
-        span._set_str_tag(http.URL, url if integration_config.trace_query_string else strip_query_string(url))
+        if integration_config.trace_query_string and not config.global_trace_query_string_disabled:
+            span._set_str_tag(http.URL, redact_url(url, config._obfuscation_query_string_pattern, query))
+        else:
+            span._set_str_tag(http.URL, strip_query_string(url))
 
     if status_code is not None:
         try:

ddtrace/internal/utils/http.py

@@ -1,9 +1,11 @@
 from contextlib import contextmanager
+import re
 from typing import Any
 from typing import Callable
 from typing import ContextManager
 from typing import Generator
 from typing import Optional
+from typing import Tuple
 from typing import Union
 
 from ddtrace.internal import compat
@@ -40,6 +42,60 @@ def strip_query_string(url):
     return h + fs + f
 
 
+def redact_query_string(query_string, query_string_obfuscation_pattern):
+    # type: (str, Optional[re.Pattern]) -> Union[bytes, str]
+    if query_string_obfuscation_pattern is None:
+        return query_string
+
+    bytes_query = query_string if isinstance(query_string, bytes) else query_string.encode("utf-8")
+    return query_string_obfuscation_pattern.sub(b"<redacted>", bytes_query)
+
+
+def redact_url(url, query_string_obfuscation_pattern, query_string=None):
+    # type: (str, re.Pattern, Optional[str]) -> Union[str,bytes]
+
+    # Avoid further processing if obfuscation is disabled
+    if query_string_obfuscation_pattern is None:
+        return url
+
+    parts = compat.parse.urlparse(url)
+    redacted_query = None
+
+    if query_string:
+        redacted_query = redact_query_string(query_string, query_string_obfuscation_pattern)
+    elif parts.query:
+        redacted_query = redact_query_string(parts.query, query_string_obfuscation_pattern)
+
+    if redacted_query is not None and len(parts) >= 5:
+        redacted_parts = parts[:4] + (redacted_query,) + parts[5:]  # type: Tuple[Union[str, bytes], ...]
+        bytes_redacted_parts = tuple(x if isinstance(x, bytes) else x.encode("utf-8") for x in redacted_parts)
+        return urlunsplit(bytes_redacted_parts, url)
+
+    # If no obfuscation is performed, return original url
+    return url
+
+
+def urlunsplit(components, original_url):
+    # type: (Tuple[bytes, ...], str) -> bytes
+    """
+    Adaptation from urlunsplit and urlunparse, using bytes components
+    """
+    scheme, netloc, url, params, query, fragment = components
+    if params:
+        url = b"%s;%s" % (url, params)
+    if netloc or (scheme and url[:2] != b"//"):
+        if url and url[:1] != b"/":
+            url = b"/" + url
+        url = b"//%s%s" % ((netloc or b""), url)
+    if scheme:
+        url = b"%s:%s" % (scheme, url)
+    if query or (original_url and original_url[-1] in ("?", b"?")):
+        url = b"%s?%s" % (url, query)
+    if fragment or (original_url and original_url[-1] in ("#", b"#")):
+        url = b"%s#%s" % (url, fragment)
+    return url
+
+
 def connector(url, **kwargs):
     # type: (str, Any) -> Connector
     """Create a connector context manager for the given URL.

ddtrace/settings/config.py

@@ -1,5 +1,6 @@
 from copy import deepcopy
 import os
+import re
 from typing import List
 from typing import Optional
 from typing import Tuple
@@ -19,6 +20,18 @@
 log = get_logger(__name__)
 
 
+DD_TRACE_OBFUSCATION_QUERY_STRING_PATTERN_DEFAULT = (
+    r"(?i)(?:p(?:ass)?w(?:or)?d|pass(?:_?phrase)?|secret|(?:api_?|"
+    r"private_?|public_?|access_?|secret_?)key(?:_?id)?|token|consumer_?(?:id|key|secret)|"
+    r'sign(?:ed|ature)?|auth(?:entication|orization)?)(?:(?:\s|%20)*(?:=|%3D)[^&]+|(?:"|%22)'
+    r'(?:\s|%20)*(?::|%3A)(?:\s|%20)*(?:"|%22)(?:%2[^2]|%[^2]|[^"%])+(?:"|%22))|bearer(?:\s|%20)'
+    r"+[a-z0-9\._\-]|token(?::|%3A)[a-z0-9]{13}|gh[opsu]_[0-9a-zA-Z]{36}|ey[I-L](?:[\w=-]|%3D)+\.ey[I-L]"
+    r"(?:[\w=-]|%3D)+(?:\.(?:[\w.+\/=-]|%3D|%2F|%2B)+)?|[\-]{5}BEGIN(?:[a-z\s]|%20)+"
+    r"PRIVATE(?:\s|%20)KEY[\-]{5}[^\-]+[\-]{5}END(?:[a-z\s]|%20)+PRIVATE(?:\s|%20)KEY|"
+    r"ssh-rsa(?:\s|%20)*(?:[a-z0-9\/\.+]|%2F|%5C|%2B){100,}"
+)
+
+
 def _parse_propagation_styles(name, default):
     # type: (str, str) -> set[str]
     """Helper to parse http propagation extract/inject styles via env variables.
@@ -212,6 +225,20 @@ def __init__(self):
         )
         self._appsec_enabled = asbool(os.getenv("DD_APPSEC_ENABLED", False))
 
+        dd_trace_obfuscation_query_string_pattern = os.getenv(
+            "DD_TRACE_OBFUSCATION_QUERY_STRING_PATTERN", DD_TRACE_OBFUSCATION_QUERY_STRING_PATTERN_DEFAULT
+        )
+        self.global_trace_query_string_disabled = False
+        self._obfuscation_query_string_pattern = None
+        if dd_trace_obfuscation_query_string_pattern != "":
+            try:
+                self._obfuscation_query_string_pattern = re.compile(
+                    dd_trace_obfuscation_query_string_pattern.encode("ascii")
+                )
+            except Exception:
+                log.warning("Invalid obfuscation pattern, disabling query string tracing")
+                self.global_trace_query_string_disabled = True
+
     def __getattr__(self, name):
         if name not in self._config:
             self._config[name] = IntegrationConfig(self, name)

docs/advanced_usage.rst

@@ -373,6 +373,14 @@ Examples::
     # Integration level config, e.g. 'falcon'
     config.falcon.http.trace_query_string = True
 
+The sensitive query strings (e.g: token, password) are obfuscated by default.
+
+It is possible to configure the obfuscation regexp by setting the ``DD_TRACE_OBFUSCATION_QUERY_STRING_PATTERN`` environment variable.
+
+To disable query string obfuscation, set the ``DD_TRACE_OBFUSCATION_QUERY_STRING_PATTERN`` environment variable to empty string ("")
+
+If the ``DD_TRACE_OBFUSCATION_QUERY_STRING_PATTERN`` environment variable is set to an invalid regexp, the query strings will not be traced.
+
 ..  _http-headers-tracing:
 
 Headers tracing

docs/configuration.rst

@@ -200,6 +200,12 @@ below:
      - The trace API version to use when sending traces to the Datadog agent.
        Currently, the supported versions are: ``v0.3``, ``v0.4`` and ``v0.5``.
 
+       .. _dd-trace-obfuscation-query-string-pattern:
+   * - ``DD_TRACE_OBFUSCATION_QUERY_STRING_PATTERN``
+     - String
+     - ``(?i)(?:p(?:ass)?w(?:or)?d|pass(?:_?phrase)?|secret|(?:api_?|private_?|public_?|access_?|secret_?)key(?:_?id)?|token|consumer_?(?:id|key|secret)|sign(?:ed|ature)?|auth(?:entication|orization)?)(?:(?:\s|%20)*(?:=|%3D)[^&]+|(?:"|%22)(?:\s|%20)*(?::|%3A)(?:\s|%20)*(?:"|%22)(?:%2[^2]|%[^2]|[^"%])+(?:"|%22))|bearer(?:\s|%20)+[a-z0-9\._\-]|token(?::|%3A)[a-z0-9]{13}|gh[opsu]_[0-9a-zA-Z]{36}|ey[I-L](?:[\w=-]|%3D)+\.ey[I-L](?:[\w=-]|%3D)+(?:\.(?:[\w.+\/=-]|%3D|%2F|%2B)+)?|[\-]{5}BEGIN(?:[a-z\s]|%20)+PRIVATE(?:\s|%20)KEY[\-]{5}[^\-]+[\-]{5}END(?:[a-z\s]|%20)+PRIVATE(?:\s|%20)KEY|ssh-rsa(?:\s|%20)*(?:[a-z0-9\/\.+]|%2F|%5C|%2B){100,}.``
+     - A regexp to redact sensitive query strings. Obfuscation disabled if set to empty string
+
        .. _dd-trace-propagation-style-extract:
    * - ``DD_TRACE_PROPAGATION_STYLE_EXTRACT``
      - String

releasenotes/notes/redact-sensitive-querystrings-in-http-url-522461022535a1e2.yaml

@@ -0,0 +1,4 @@
+---
+features:
+  - |
+    ASM: Redact sensitive query strings if sent in http.url.

tests/contrib/asgi/test_asgi.py

@@ -366,14 +366,14 @@ async def test_multiple_requests(tracer, test_spans):
     assert r1_span.name == "asgi.request"
     assert r1_span.span_type == "web"
     assert r1_span.get_tag("http.method") == "GET"
-    assert r1_span.get_tag("http.url") == "http://testserver/"
+    assert r1_span.get_tag("http.url") == "http://testserver/?sleep=true"
     assert r1_span.get_tag("http.query.string") == "sleep=true"
 
     r2_span = spans[0][0]
     assert r2_span.name == "asgi.request"
     assert r2_span.span_type == "web"
     assert r2_span.get_tag("http.method") == "GET"
-    assert r2_span.get_tag("http.url") == "http://testserver/"
+    assert r2_span.get_tag("http.url") == "http://testserver/?sleep=true"
     assert r2_span.get_tag("http.query.string") == "sleep=true"
 
 

tests/contrib/bottle/test.py

@@ -67,10 +67,11 @@ def hi(name):
         assert s.resource == "GET /hi/<name>"
         assert_span_http_status_code(s, 200)
         assert s.get_tag("http.method") == "GET"
-        assert s.get_tag(http.URL) == "http://localhost:80/hi/dougie"
         if ddtrace.config.bottle.trace_query_string:
+            assert s.get_tag(http.URL) == "http://localhost:80/hi/dougie" + fqs
             assert s.get_tag(http.QUERY_STRING) == query_string
         else:
+            assert s.get_tag(http.URL) == "http://localhost:80/hi/dougie"
             assert http.QUERY_STRING not in s.get_tags()
 
     def test_query_string(self):

tests/contrib/fastapi/test_fastapi.py

@@ -172,7 +172,7 @@ def test_read_item_query_string(client, tracer, test_spans):
     assert request_span.resource == "GET /items/{item_id}"
     assert request_span.error == 0
     assert request_span.get_tag("http.method") == "GET"
-    assert request_span.get_tag("http.url") == "http://testserver/items/foo"
+    assert request_span.get_tag("http.url") == "http://testserver/items/foo?q=query"
     assert request_span.get_tag("http.status_code") == "200"
     assert request_span.get_tag("http.query.string") == "q=query"
 
@@ -195,7 +195,7 @@ def test_200_multi_query_string(client, tracer, test_spans):
     assert request_span.resource == "GET /items/{item_id}"
     assert request_span.error == 0
     assert request_span.get_tag("http.method") == "GET"
-    assert request_span.get_tag("http.url") == "http://testserver/items/foo"
+    assert request_span.get_tag("http.url") == "http://testserver/items/foo?name=Foo&q=query"
     assert request_span.get_tag("http.status_code") == "200"
     assert request_span.get_tag("http.query.string") == "name=Foo&q=query"