Merge pull request from GHSA-5rv5-6h4r-h22v

* Fix unbound cardinality for label http_method in wsgi based middlewares * cr: rename file * cr: change label UNKNOWN to NONSTANDARD * Update instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py --------- Co-authored-by: Diego Hurtado <[email protected]>
open-telemetry · Aug 15, 2023 · 6007e0c · 6007e0c
1 parent 1beab82
commit 6007e0c
Show file tree

Hide file tree

Showing 5 changed files with 162 additions and 19 deletions.
diff --git a/instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py b/instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py
@@ -40,6 +40,8 @@
     OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SANITIZE_FIELDS,
     OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST,
     OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE,
+    OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE,
+    OTEL_PYTHON_INSTRUMENTATION_HTTP_CAPTURE_ALL_METHODS,
     get_excluded_urls,
 )
 
@@ -326,6 +328,25 @@ def test_flask_metric_values(self):
                         if isinstance(point, NumberDataPoint):
                             self.assertEqual(point.value, 0)
 
+    def _assert_basic_metric(self, expected_duration_attributes, expected_requests_count_attributes):
+        metrics_list = self.memory_metrics_reader.get_metrics_data()
+        for resource_metric in metrics_list.resource_metrics:
+            for scope_metrics in resource_metric.scope_metrics:
+                for metric in scope_metrics.metrics:
+                    for point in list(metric.data.data_points):
+                        if isinstance(point, HistogramDataPoint):
+                            self.assertDictEqual(
+                                expected_duration_attributes,
+                                dict(point.attributes),
+                            )
+                            self.assertEqual(point.count, 1)
+                        elif isinstance(point, NumberDataPoint):
+                            self.assertDictEqual(
+                                expected_requests_count_attributes,
+                                dict(point.attributes),
+                            )
+                            self.assertEqual(point.value, 0)
+
     def test_basic_metric_success(self):
         self.client.get("/hello/756")
         expected_duration_attributes = {
@@ -344,23 +365,62 @@ def test_basic_metric_success(self):
             "http.flavor": "1.1",
             "http.server_name": "localhost",
         }
-        metrics_list = self.memory_metrics_reader.get_metrics_data()
-        for resource_metric in metrics_list.resource_metrics:
-            for scope_metrics in resource_metric.scope_metrics:
-                for metric in scope_metrics.metrics:
-                    for point in list(metric.data.data_points):
-                        if isinstance(point, HistogramDataPoint):
-                            self.assertDictEqual(
-                                expected_duration_attributes,
-                                dict(point.attributes),
-                            )
-                            self.assertEqual(point.count, 1)
-                        elif isinstance(point, NumberDataPoint):
-                            self.assertDictEqual(
-                                expected_requests_count_attributes,
-                                dict(point.attributes),
-                            )
-                            self.assertEqual(point.value, 0)
+        self._assert_basic_metric(
+            expected_duration_attributes,
+            expected_requests_count_attributes,
+        )
+
+    def test_basic_metric_nonstandard_http_method_success(self):
+        self.client.open("/hello/756", method="NONSTANDARD")
+        expected_duration_attributes = {
+            "http.method": "UNKNOWN",
+            "http.host": "localhost",
+            "http.scheme": "http",
+            "http.flavor": "1.1",
+            "http.server_name": "localhost",
+            "net.host.port": 80,
+            "http.status_code": 405,
+        }
+        expected_requests_count_attributes = {
+            "http.method": "UNKNOWN",
+            "http.host": "localhost",
+            "http.scheme": "http",
+            "http.flavor": "1.1",
+            "http.server_name": "localhost",
+        }
+        self._assert_basic_metric(
+            expected_duration_attributes,
+            expected_requests_count_attributes,
+        )
+
+    @patch.dict(
+    "os.environ",
+        {
+            OTEL_PYTHON_INSTRUMENTATION_HTTP_CAPTURE_ALL_METHODS: "1",
+        },
+    )
+    def test_basic_metric_nonstandard_http_method_allowed_success(self):
+        self.client.open("/hello/756", method="NONSTANDARD")
+        expected_duration_attributes = {
+            "http.method": "NONSTANDARD",
+            "http.host": "localhost",
+            "http.scheme": "http",
+            "http.flavor": "1.1",
+            "http.server_name": "localhost",
+            "net.host.port": 80,
+            "http.status_code": 405,
+        }
+        expected_requests_count_attributes = {
+            "http.method": "NONSTANDARD",
+            "http.host": "localhost",
+            "http.scheme": "http",
+            "http.flavor": "1.1",
+            "http.server_name": "localhost",
+        }
+        self._assert_basic_metric(
+            expected_duration_attributes,
+            expected_requests_count_attributes,
+        )
 
     def test_metric_uninstrument(self):
         self.client.delete("/hello/756")

diff --git a/...ion/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py b/...ion/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py
@@ -197,6 +197,12 @@ def response_hook(span: Span, environ: WSGIEnvironment, status: str, response_he
 Note:
     The environment variable names used to capture HTTP headers are still experimental, and thus are subject to change.
 
+Sanitizing methods
+******************
+In order to prevent unbound cardinality for HTTP methods by default nonstandard ones are labeled as ``NONSTANDARD``.
+To record all of the names set the environment variable  ``OTEL_PYTHON_INSTRUMENTATION_HTTP_CAPTURE_ALL_METHODS``
+to a value that evaluates to true, e.g. ``1``.
+
 API
 ---
 """
@@ -226,6 +232,7 @@ def response_hook(span: Span, environ: WSGIEnvironment, status: str, response_he
     normalise_request_header_name,
     normalise_response_header_name,
     remove_url_credentials,
+    sanitize_method,
 )
 
 _HTTP_VERSION_PREFIX = "HTTP/"
@@ -295,7 +302,7 @@ def collect_request_attributes(environ):
     """
 
     result = {
-        SpanAttributes.HTTP_METHOD: environ.get("REQUEST_METHOD"),
+        SpanAttributes.HTTP_METHOD: sanitize_method(environ.get("REQUEST_METHOD")),
         SpanAttributes.HTTP_SERVER_NAME: environ.get("SERVER_NAME"),
         SpanAttributes.HTTP_SCHEME: environ.get("wsgi.url_scheme"),
     }
@@ -450,7 +457,7 @@ def get_default_span_name(environ):
     Returns:
         The span name.
     """
-    method = environ.get("REQUEST_METHOD", "").strip()
+    method = sanitize_method(environ.get("REQUEST_METHOD", "").strip())
     path = environ.get("PATH_INFO", "").strip()
     if method and path:
         return f"{method} {path}"

diff --git a/instrumentation/opentelemetry-instrumentation-wsgi/tests/test_wsgi_middleware.py b/instrumentation/opentelemetry-instrumentation-wsgi/tests/test_wsgi_middleware.py
@@ -33,6 +33,7 @@
     OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SANITIZE_FIELDS,
     OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST,
     OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE,
+    OTEL_PYTHON_INSTRUMENTATION_HTTP_CAPTURE_ALL_METHODS,
 )
 
 
@@ -284,6 +285,24 @@ def test_wsgi_metrics(self):
                             )
         self.assertTrue(number_data_point_seen and histogram_data_point_seen)
 
+    def test_nonstandard_http_method(self):
+        self.environ["REQUEST_METHOD"]= "NONSTANDARD"
+        app = otel_wsgi.OpenTelemetryMiddleware(simple_wsgi)
+        response = app(self.environ, self.start_response)
+        self.validate_response(response, span_name="HTTP UNKNOWN", http_method="UNKNOWN")
+
+    @mock.patch.dict(
+    "os.environ",
+        {
+            OTEL_PYTHON_INSTRUMENTATION_HTTP_CAPTURE_ALL_METHODS: "1",
+        },
+    )
+    def test_nonstandard_http_method_allowed(self):
+        self.environ["REQUEST_METHOD"]= "NONSTANDARD"
+        app = otel_wsgi.OpenTelemetryMiddleware(simple_wsgi)
+        response = app(self.environ, self.start_response)
+        self.validate_response(response, span_name="HTTP NONSTANDARD", http_method="NONSTANDARD")
+
     def test_default_span_name_missing_path_info(self):
         """Test that default span_names with missing path info."""
         self.environ.pop("PATH_INFO")

diff --git a/util/opentelemetry-util-http/src/opentelemetry/util/http/__init__.py b/util/opentelemetry-util-http/src/opentelemetry/util/http/__init__.py
@@ -31,6 +31,10 @@
     "OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE"
 )
 
+OTEL_PYTHON_INSTRUMENTATION_HTTP_CAPTURE_ALL_METHODS = (
+    "OTEL_PYTHON_INSTRUMENTATION_HTTP_CAPTURE_ALL_METHODS"
+)
+
 # List of recommended metrics attributes
 _duration_attrs = {
     SpanAttributes.HTTP_METHOD,
@@ -186,6 +190,15 @@ def normalise_response_header_name(header: str) -> str:
     key = header.lower().replace("-", "_")
     return f"http.response.header.{key}"
 
+def sanitize_method(method: str | None) -> str | None:
+    if method is None:
+        return None
+    method = method.upper()
+    if (environ.get(OTEL_PYTHON_INSTRUMENTATION_HTTP_CAPTURE_ALL_METHODS) or
+        # Based on https://www.rfc-editor.org/rfc/rfc7231#section-4.1 and https://www.rfc-editor.org/rfc/rfc5789#section-2.
+        method in ["GET", "HEAD", "POST", "PUT", "DELETE", "CONNECT", "OPTIONS", "TRACE", "PATCH"]):
+        return method
+    return "NONSTANDARD"
 
 def get_custom_headers(env_var: str) -> List[str]:
     custom_headers = environ.get(env_var, [])

diff --git a/util/opentelemetry-util-http/tests/test_sanitize_method.py b/util/opentelemetry-util-http/tests/test_sanitize_method.py
@@ -0,0 +1,44 @@
+# Copyright The OpenTelemetry Authors
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import unittest
+from unittest.mock import patch
+
+from opentelemetry.util.http import (
+    OTEL_PYTHON_INSTRUMENTATION_HTTP_CAPTURE_ALL_METHODS,
+    sanitize_method,
+)
+
+class TestSanitizeMethod(unittest.TestCase):
+    def test_standard_method_uppercase(self):
+        method = sanitize_method("GET")
+        self.assertEqual(method, "GET")
+
+    def test_standard_method_lowercase(self):
+        method = sanitize_method("get")
+        self.assertEqual(method, "GET")
+
+    def test_nonstandard_method(self):
+        method = sanitize_method("UNKNOWN")
+        self.assertEqual(method, "NONSTANDARD")
+
+    @patch.dict(
+    "os.environ",
+        {
+            OTEL_PYTHON_INSTRUMENTATION_HTTP_CAPTURE_ALL_METHODS: "1",
+        },
+    )
+    def test_nonstandard_method_allowed(self):
+        method = sanitize_method("UNKNOWN")
+        self.assertEqual(method, "NONSTANDARD")