Ensure clean http url (#538)

Author: ryokather

CHANGELOG.md

@@ -14,6 +14,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   ([#530](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/530))
 - Fix weak reference error for pyodbc cursor in SQLAlchemy instrumentation.
   ([#469](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/469))
+- Implemented specification that HTTP span attributes must not contain username and password.
+  ([#538](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/538))
 
 ### Added
 - `opentelemetry-instrumentation-httpx` Add `httpx` instrumentation

instrumentation/opentelemetry-instrumentation-aiohttp-client/setup.cfg

@@ -41,6 +41,7 @@ install_requires =
     opentelemetry-api == 1.4.0.dev0
     opentelemetry-semantic-conventions == 0.23.dev0
     opentelemetry-instrumentation == 0.23.dev0
+    opentelemetry-util-http == 0.23.dev0
     wrapt >= 1.0.0, < 2.0.0
 
 [options.packages.find]

instrumentation/opentelemetry-instrumentation-aiohttp-client/src/opentelemetry/instrumentation/aiohttp_client/__init__.py

@@ -82,6 +82,7 @@ def strip_query_params(url: yarl.URL) -> str:
 from opentelemetry.semconv.trace import SpanAttributes
 from opentelemetry.trace import SpanKind, TracerProvider, get_tracer
 from opentelemetry.trace.status import Status, StatusCode
+from opentelemetry.util.http import remove_url_credentials
 
 _UrlFilterT = typing.Optional[typing.Callable[[str], str]]
 _SpanNameT = typing.Optional[
@@ -173,11 +174,11 @@ async def on_request_start(
         if trace_config_ctx.span.is_recording():
             attributes = {
                 SpanAttributes.HTTP_METHOD: http_method,
-                SpanAttributes.HTTP_URL: trace_config_ctx.url_filter(
-                    params.url
+                SpanAttributes.HTTP_URL: remove_url_credentials(
+                    trace_config_ctx.url_filter(params.url)
                 )
                 if callable(trace_config_ctx.url_filter)
-                else str(params.url),
+                else remove_url_credentials(str(params.url)),
             }
             for key, value in attributes.items():
                 trace_config_ctx.span.set_attribute(key, value)

instrumentation/opentelemetry-instrumentation-aiohttp-client/tests/test_aiohttp_client_integration.py

@@ -321,6 +321,37 @@ async def request_handler(request):
             ]
         )
 
+    def test_credential_removal(self):
+        trace_configs = [aiohttp_client.create_trace_config()]
+
+        url = "http://username:[email protected]/status/200"
+        with self.subTest(url=url):
+
+            async def do_request(url):
+                async with aiohttp.ClientSession(
+                    trace_configs=trace_configs,
+                ) as session:
+                    async with session.get(url):
+                        pass
+
+            loop = asyncio.get_event_loop()
+            loop.run_until_complete(do_request(url))
+
+        self.assert_spans(
+            [
+                (
+                    "HTTP GET",
+                    (StatusCode.UNSET, None),
+                    {
+                        SpanAttributes.HTTP_METHOD: "GET",
+                        SpanAttributes.HTTP_URL: "http://httpbin.org/status/200",
+                        SpanAttributes.HTTP_STATUS_CODE: int(HTTPStatus.OK),
+                    },
+                )
+            ]
+        )
+        self.memory_exporter.clear()
+
 
 class TestAioHttpClientInstrumentor(TestBase):
     URL = "/test-path"

instrumentation/opentelemetry-instrumentation-asgi/setup.cfg

@@ -41,6 +41,7 @@ install_requires =
     opentelemetry-api == 1.4.0.dev0
     opentelemetry-semantic-conventions == 0.23.dev0
     opentelemetry-instrumentation == 0.23.dev0
+    opentelemetry-util-http == 0.23.dev0
 
 [options.extras_require]
 test =

instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py

@@ -32,6 +32,7 @@
 from opentelemetry.propagators.textmap import Getter
 from opentelemetry.semconv.trace import SpanAttributes
 from opentelemetry.trace.status import Status, StatusCode
+from opentelemetry.util.http import remove_url_credentials
 
 
 class ASGIGetter(Getter):
@@ -86,7 +87,7 @@ def collect_request_attributes(scope):
         SpanAttributes.NET_HOST_PORT: port,
         SpanAttributes.HTTP_FLAVOR: scope.get("http_version"),
         SpanAttributes.HTTP_TARGET: scope.get("path"),
-        SpanAttributes.HTTP_URL: http_url,
+        SpanAttributes.HTTP_URL: remove_url_credentials(http_url),
     }
     http_method = scope.get("method")
     if http_method:

instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py

@@ -430,6 +430,14 @@ def test_response_attributes_invalid_status_code(self):
         otel_asgi.set_status_code(self.span, "Invalid Status Code")
         self.assertEqual(self.span.set_status.call_count, 1)
 
+    def test_credential_removal(self):
+        self.scope["server"] = ("username:[email protected]", 80)
+        self.scope["path"] = "/status/200"
+        attrs = otel_asgi.collect_request_attributes(self.scope)
+        self.assertEqual(
+            attrs[SpanAttributes.HTTP_URL], "http://httpbin.org/status/200"
+        )
+
 
 if __name__ == "__main__":
     unittest.main()

instrumentation/opentelemetry-instrumentation-requests/setup.cfg

@@ -41,6 +41,7 @@ install_requires =
     opentelemetry-api == 1.4.0.dev0
     opentelemetry-semantic-conventions == 0.23.dev0
     opentelemetry-instrumentation == 0.23.dev0
+    opentelemetry-util-http == 0.23.dev0
 
 [options.extras_require]
 test =

instrumentation/opentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py

@@ -50,6 +50,7 @@
 from opentelemetry.semconv.trace import SpanAttributes
 from opentelemetry.trace import SpanKind, get_tracer
 from opentelemetry.trace.status import Status
+from opentelemetry.util.http import remove_url_credentials
 
 # A key to a context variable to avoid creating duplicate spans when instrumenting
 # both, Session.request and Session.send, since Session.request calls into Session.send
@@ -124,6 +125,8 @@ def _instrumented_requests_call(
         if not span_name or not isinstance(span_name, str):
             span_name = get_default_span_name(method)
 
+        url = remove_url_credentials(url)
+
         labels = {}
         labels[SpanAttributes.HTTP_METHOD] = method
         labels[SpanAttributes.HTTP_URL] = url

instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py

@@ -357,6 +357,13 @@ def test_invalid_url(self):
         )
         self.assertEqual(span.status.status_code, StatusCode.ERROR)
 
+    def test_credential_removal(self):
+        new_url = "http://username:[email protected]/status/200"
+        self.perform_request(new_url)
+        span = self.assert_span()
+
+        self.assertEqual(span.attributes[SpanAttributes.HTTP_URL], self.URL)
+
     def test_if_headers_equals_none(self):
         result = requests.get(self.URL, headers=None)
         self.assertEqual(result.text, "Hello!")

instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/client.py

@@ -22,6 +22,7 @@
 from opentelemetry.semconv.trace import SpanAttributes
 from opentelemetry.trace.status import Status
 from opentelemetry.util._time import _time_ns
+from opentelemetry.util.http import remove_url_credentials
 
 
 def _normalize_request(args, kwargs):
@@ -61,7 +62,7 @@ def fetch_async(tracer, request_hook, response_hook, func, _, args, kwargs):
 
     if span.is_recording():
         attributes = {
-            SpanAttributes.HTTP_URL: request.url,
+            SpanAttributes.HTTP_URL: remove_url_credentials(request.url),
             SpanAttributes.HTTP_METHOD: request.method,
         }
         for key, value in attributes.items():

instrumentation/opentelemetry-instrumentation-tornado/tests/test_instrumentation.py

@@ -455,6 +455,29 @@ def test_response_headers(self):
         self.memory_exporter.clear()
         set_global_response_propagator(orig)
 
+    def test_credential_removal(self):
+        response = self.fetch(
+            "http://username:[email protected]/status/200"
+        )
+        self.assertEqual(response.code, 200)
+
+        spans = self.sorted_spans(self.memory_exporter.get_finished_spans())
+        self.assertEqual(len(spans), 1)
+        client = spans[0]
+
+        self.assertEqual(client.name, "GET")
+        self.assertEqual(client.kind, SpanKind.CLIENT)
+        self.assert_span_has_attributes(
+            client,
+            {
+                SpanAttributes.HTTP_URL: "http://httpbin.org/status/200",
+                SpanAttributes.HTTP_METHOD: "GET",
+                SpanAttributes.HTTP_STATUS_CODE: 200,
+            },
+        )
+
+        self.memory_exporter.clear()
+
 
 class TornadoHookTest(TornadoTest):
     _client_request_hook = None

instrumentation/opentelemetry-instrumentation-urllib/setup.cfg

@@ -41,6 +41,7 @@ install_requires =
     opentelemetry-api == 1.4.0.dev0
     opentelemetry-semantic-conventions == 0.23.dev0
     opentelemetry-instrumentation == 0.23.dev0
+    opentelemetry-util-http == 0.23.dev0
 
 [options.extras_require]
 test =

instrumentation/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py

@@ -53,6 +53,7 @@
 from opentelemetry.semconv.trace import SpanAttributes
 from opentelemetry.trace import SpanKind, get_tracer
 from opentelemetry.trace.status import Status
+from opentelemetry.util.http import remove_url_credentials
 
 # A key to a context variable to avoid creating duplicate spans when instrumenting
 _SUPPRESS_HTTP_INSTRUMENTATION_KEY = "suppress_http_instrumentation"
@@ -142,6 +143,8 @@ def _instrumented_open_call(
         if not span_name or not isinstance(span_name, str):
             span_name = get_default_span_name(method)
 
+        url = remove_url_credentials(url)
+
         labels = {
             SpanAttributes.HTTP_METHOD: method,
             SpanAttributes.HTTP_URL: url,

instrumentation/opentelemetry-instrumentation-urllib/tests/test_urllib_integration.py

@@ -35,6 +35,8 @@
 from opentelemetry.test.test_base import TestBase
 from opentelemetry.trace import StatusCode
 
+# pylint: disable=too-many-public-methods
+
 
 class RequestsIntegrationTestBase(abc.ABC):
     # pylint: disable=no-member
@@ -318,6 +320,15 @@ def test_requests_timeout_exception(self, *_, **__):
         span = self.assert_span()
         self.assertEqual(span.status.status_code, StatusCode.ERROR)
 
+    def test_credential_removal(self):
+        url = "http://username:[email protected]/status/200"
+
+        with self.assertRaises(Exception):
+            self.perform_request(url)
+
+        span = self.assert_span()
+        self.assertEqual(span.attributes[SpanAttributes.HTTP_URL], self.URL)
+
 
 class TestRequestsIntegration(RequestsIntegrationTestBase, TestBase):
     @staticmethod

instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_integration.py

@@ -287,3 +287,9 @@ def url_filter(url):
 
         response = self.perform_request(self.HTTP_URL + "?e=mcc")
         self.assert_success_span(response, self.HTTP_URL)
+
+    def test_credential_removal(self):
+        url = "http://username:[email protected]/status/200"
+
+        response = self.perform_request(url)
+        self.assert_success_span(response, self.HTTP_URL)

instrumentation/opentelemetry-instrumentation-wsgi/setup.cfg

@@ -41,6 +41,7 @@ install_requires =
     opentelemetry-api == 1.4.0.dev0
     opentelemetry-semantic-conventions == 0.23.dev0
     opentelemetry-instrumentation == 0.23.dev0
+    opentelemetry-util-http == 0.23.dev0
 
 [options.extras_require]
 test =

instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py

@@ -65,6 +65,7 @@ def hello():
 from opentelemetry.propagators.textmap import Getter
 from opentelemetry.semconv.trace import SpanAttributes
 from opentelemetry.trace.status import Status, StatusCode
+from opentelemetry.util.http import remove_url_credentials
 
 _HTTP_VERSION_PREFIX = "HTTP/"
 _CARRIER_KEY_PREFIX = "HTTP_"
@@ -128,7 +129,9 @@ def collect_request_attributes(environ):
     if target is not None:
         result[SpanAttributes.HTTP_TARGET] = target
     else:
-        result[SpanAttributes.HTTP_URL] = wsgiref_util.request_uri(environ)
+        result[SpanAttributes.HTTP_URL] = remove_url_credentials(
+            wsgiref_util.request_uri(environ)
+        )
 
     remote_addr = environ.get("REMOTE_ADDR")
     if remote_addr:

instrumentation/opentelemetry-instrumentation-wsgi/tests/test_wsgi_middleware.py

@@ -364,6 +364,18 @@ def test_response_attributes(self):
         self.assertEqual(self.span.set_attribute.call_count, len(expected))
         self.span.set_attribute.assert_has_calls(expected, any_order=True)
 
+    def test_credential_removal(self):
+        self.environ["HTTP_HOST"] = "username:[email protected]"
+        self.environ["PATH_INFO"] = "/status/200"
+        expected = {
+            SpanAttributes.HTTP_URL: "http://httpbin.com/status/200",
+            SpanAttributes.NET_HOST_PORT: 80,
+        }
+        self.assertGreaterEqual(
+            otel_wsgi.collect_request_attributes(self.environ).items(),
+            expected.items(),
+        )
+
 
 class TestWsgiMiddlewareWithTracerProvider(WsgiTestBase):
     def validate_response(

tox.ini

@@ -237,7 +237,7 @@ commands_pre =
 
   grpc: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-grpc[test]
 
-  falcon,flask,django,pyramid,tornado,starlette,fastapi: pip install {toxinidir}/util/opentelemetry-util-http[test]
+  falcon,flask,django,pyramid,tornado,starlette,fastapi,aiohttp,asgi,requests,urllib,wsgi: pip install {toxinidir}/util/opentelemetry-util-http[test]
   wsgi,falcon,flask,django,pyramid: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-wsgi[test]
   asgi,starlette,fastapi: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-asgi[test]
 
@@ -413,6 +413,7 @@ deps =
   protobuf>=3.13.0
   requests==2.25.0
   pyodbc~=4.0.30
+
 changedir =
   tests/opentelemetry-docker-tests/tests
 
@@ -435,17 +436,17 @@ commands_pre =
               -e {toxinidir}/opentelemetry-python-core/exporter/opentelemetry-exporter-opencensus
   docker-compose up -d
   python check_availability.py
+
 commands =
   pytest {posargs}
 
 commands_post =
   docker-compose down -v
 
-
 [testenv:generate]
 deps =
   -r {toxinidir}/gen-requirements.txt
 
 commands =
   {toxinidir}/scripts/generate_setup.py
-  {toxinidir}/scripts/generate_instrumentation_bootstrap.py
+  {toxinidir}/scripts/generate_instrumentation_bootstrap.py

util/opentelemetry-util-http/src/opentelemetry/util/http/__init__.py

@@ -15,6 +15,7 @@
 from os import environ
 from re import compile as re_compile
 from re import search
+from urllib.parse import urlparse, urlunparse
 
 
 class ExcludeList:
@@ -57,3 +58,30 @@ def get_excluded_urls(instrumentation):
         ]
 
     return ExcludeList(excluded_urls)
+
+
+def remove_url_credentials(url: str) -> str:
+    """Given a string url, remove the username and password only if it is a valid url"""
+
+    try:
+        parsed = urlparse(url)
+        if all([parsed.scheme, parsed.netloc]):  # checks for valid url
+            parsed_url = urlparse(url)
+            netloc = (
+                (":".join(((parsed_url.hostname or ""), str(parsed_url.port))))
+                if parsed_url.port
+                else (parsed_url.hostname or "")
+            )
+            return urlunparse(
+                (
+                    parsed_url.scheme,
+                    netloc,
+                    parsed_url.path,
+                    parsed_url.params,
+                    parsed_url.query,
+                    parsed_url.fragment,
+                )
+            )
+    except ValueError:  # an unparseable url was passed
+        pass
+    return url