diff --git a/CHANGELOG.md b/CHANGELOG.md index c8d2a28508..6acedf12e3 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/instrumentation/opentelemetry-instrumentation-aiohttp-client/setup.cfg b/instrumentation/opentelemetry-instrumentation-aiohttp-client/setup.cfg index 21a3d1160a..4de609a95b 100644 --- a/instrumentation/opentelemetry-instrumentation-aiohttp-client/setup.cfg +++ b/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] diff --git a/instrumentation/opentelemetry-instrumentation-aiohttp-client/src/opentelemetry/instrumentation/aiohttp_client/__init__.py b/instrumentation/opentelemetry-instrumentation-aiohttp-client/src/opentelemetry/instrumentation/aiohttp_client/__init__.py index 9597297b65..b9441ca108 100644 --- a/instrumentation/opentelemetry-instrumentation-aiohttp-client/src/opentelemetry/instrumentation/aiohttp_client/__init__.py +++ b/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) diff --git a/instrumentation/opentelemetry-instrumentation-aiohttp-client/tests/test_aiohttp_client_integration.py b/instrumentation/opentelemetry-instrumentation-aiohttp-client/tests/test_aiohttp_client_integration.py index 530245bab1..a9f82202d8 100644 --- a/instrumentation/opentelemetry-instrumentation-aiohttp-client/tests/test_aiohttp_client_integration.py +++ b/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:password@httpbin.org/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" diff --git a/instrumentation/opentelemetry-instrumentation-asgi/setup.cfg b/instrumentation/opentelemetry-instrumentation-asgi/setup.cfg index 48c7247503..a47609d423 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/setup.cfg +++ b/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 = diff --git a/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py b/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py index 6d8f0793ca..88835674a2 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py +++ b/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: diff --git a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py b/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py index 3ed438aa02..90910f7fe6 100644 --- a/instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py +++ b/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:password@httpbin.org", 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() diff --git a/instrumentation/opentelemetry-instrumentation-requests/setup.cfg b/instrumentation/opentelemetry-instrumentation-requests/setup.cfg index fcac4718fe..2decc00bac 100644 --- a/instrumentation/opentelemetry-instrumentation-requests/setup.cfg +++ b/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 = diff --git a/instrumentation/opentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py b/instrumentation/opentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py index 0731106449..451dca22a8 100644 --- a/instrumentation/opentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py +++ b/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 diff --git a/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py b/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py index 08562f6a36..95006d0602 100644 --- a/instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py +++ b/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:password@httpbin.org/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!") diff --git a/instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/client.py b/instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/client.py index 6bdcd58b7e..7fe85288ab 100644 --- a/instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/client.py +++ b/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(): diff --git a/instrumentation/opentelemetry-instrumentation-tornado/tests/test_instrumentation.py b/instrumentation/opentelemetry-instrumentation-tornado/tests/test_instrumentation.py index 58c0127647..338876f614 100644 --- a/instrumentation/opentelemetry-instrumentation-tornado/tests/test_instrumentation.py +++ b/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:password@httpbin.org/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 diff --git a/instrumentation/opentelemetry-instrumentation-urllib/setup.cfg b/instrumentation/opentelemetry-instrumentation-urllib/setup.cfg index 3830431f04..87fba0b355 100644 --- a/instrumentation/opentelemetry-instrumentation-urllib/setup.cfg +++ b/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 = diff --git a/instrumentation/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py b/instrumentation/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py index fcac480e02..1081a67d13 100644 --- a/instrumentation/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py +++ b/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, diff --git a/instrumentation/opentelemetry-instrumentation-urllib/tests/test_urllib_integration.py b/instrumentation/opentelemetry-instrumentation-urllib/tests/test_urllib_integration.py index b05fe00012..d35ceb4c9e 100644 --- a/instrumentation/opentelemetry-instrumentation-urllib/tests/test_urllib_integration.py +++ b/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:password@httpbin.org/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 diff --git a/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_integration.py b/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_integration.py index 843679f932..658044887b 100644 --- a/instrumentation/opentelemetry-instrumentation-urllib3/tests/test_urllib3_integration.py +++ b/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:password@httpbin.org/status/200" + + response = self.perform_request(url) + self.assert_success_span(response, self.HTTP_URL) diff --git a/instrumentation/opentelemetry-instrumentation-wsgi/setup.cfg b/instrumentation/opentelemetry-instrumentation-wsgi/setup.cfg index 31462974f8..0a924f6484 100644 --- a/instrumentation/opentelemetry-instrumentation-wsgi/setup.cfg +++ b/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 = diff --git a/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py b/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py index 823ede7903..f3bfd0cd25 100644 --- a/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py +++ b/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: diff --git a/instrumentation/opentelemetry-instrumentation-wsgi/tests/test_wsgi_middleware.py b/instrumentation/opentelemetry-instrumentation-wsgi/tests/test_wsgi_middleware.py index 6652f11ebb..38bdfa2614 100644 --- a/instrumentation/opentelemetry-instrumentation-wsgi/tests/test_wsgi_middleware.py +++ b/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:password@httpbin.com" + 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( diff --git a/tox.ini b/tox.ini index 72778e3ca1..e2b97cc61f 100644 --- a/tox.ini +++ b/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 \ No newline at end of file + {toxinidir}/scripts/generate_instrumentation_bootstrap.py diff --git a/util/opentelemetry-util-http/src/opentelemetry/util/http/__init__.py b/util/opentelemetry-util-http/src/opentelemetry/util/http/__init__.py index 068511010d..dfa186bf16 100644 --- a/util/opentelemetry-util-http/src/opentelemetry/util/http/__init__.py +++ b/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