Make baggage implementation w3c spec complaint (#2167)

* Add regexes to check keys and values

Fixes #2010

* Fix tests

* Add changelog

* Fix test

* Add checks to set_baggage

* Fix lint

* Add changelog entry

* Fix mypy

* Fix mypy

* Cast to string

* WIP

* Key value format

* Mostly done

* Remove old changelog entry

* fomat

* Correct typing

* Fix lint

* Fix issues

* Add CHANGELOG entry

* Make changes as discussed in SIG meeting

* Update opentelemetry-api/src/opentelemetry/baggage/__init__.py

Co-authored-by: Leighton Chen <[email protected]>

Co-authored-by: Diego Hurtado <[email protected]>
Co-authored-by: Leighton Chen <[email protected]>

Author: 3 people

CHANGELOG.md

@@ -33,6 +33,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   ([#2145](https://github.com/open-telemetry/opentelemetry-python/pull/2145))
 - Add `schema_url` to `TracerProvider.get_tracer`
   ([#2154](https://github.com/open-telemetry/opentelemetry-python/pull/2154))
+- Make baggage implementation w3c spec complaint
+  ([#2167](https://github.com/open-telemetry/opentelemetry-python/pull/2167))
 
 ## [1.5.0-0.24b0](https://github.com/open-telemetry/opentelemetry-python/releases/tag/v1.5.0-0.24b0) - 2021-08-26
 

opentelemetry-api/src/opentelemetry/baggage/__init__.py

@@ -12,18 +12,30 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import typing
+from logging import getLogger
+from re import compile
 from types import MappingProxyType
+from typing import Mapping, Optional
 
 from opentelemetry.context import create_key, get_value, set_value
 from opentelemetry.context.context import Context
+from opentelemetry.util.re import (
+    _BAGGAGE_PROPERTY_FORMAT,
+    _KEY_FORMAT,
+    _VALUE_FORMAT,
+)
 
 _BAGGAGE_KEY = create_key("baggage")
+_logger = getLogger(__name__)
+
+_KEY_PATTERN = compile(_KEY_FORMAT)
+_VALUE_PATTERN = compile(_VALUE_FORMAT)
+_PROPERT_PATTERN = compile(_BAGGAGE_PROPERTY_FORMAT)
 
 
 def get_all(
-    context: typing.Optional[Context] = None,
-) -> typing.Mapping[str, object]:
+    context: Optional[Context] = None,
+) -> Mapping[str, object]:
     """Returns the name/value pairs in the Baggage
 
     Args:
@@ -39,8 +51,8 @@ def get_all(
 
 
 def get_baggage(
-    name: str, context: typing.Optional[Context] = None
-) -> typing.Optional[object]:
+    name: str, context: Optional[Context] = None
+) -> Optional[object]:
     """Provides access to the value for a name/value pair in the
     Baggage
 
@@ -56,7 +68,7 @@ def get_baggage(
 
 
 def set_baggage(
-    name: str, value: object, context: typing.Optional[Context] = None
+    name: str, value: object, context: Optional[Context] = None
 ) -> Context:
     """Sets a value in the Baggage
 
@@ -69,13 +81,20 @@ def set_baggage(
         A Context with the value updated
     """
     baggage = dict(get_all(context=context))
-    baggage[name] = value
+    if not _is_valid_key(name):
+        _logger.warning(
+            "Baggage key `%s` does not match format, ignoring", name
+        )
+    elif not _is_valid_value(str(value)):
+        _logger.warning(
+            "Baggage value `%s` does not match format, ignoring", value
+        )
+    else:
+        baggage[name] = value
     return set_value(_BAGGAGE_KEY, baggage, context=context)
 
 
-def remove_baggage(
-    name: str, context: typing.Optional[Context] = None
-) -> Context:
+def remove_baggage(name: str, context: Optional[Context] = None) -> Context:
     """Removes a value from the Baggage
 
     Args:
@@ -91,7 +110,7 @@ def remove_baggage(
     return set_value(_BAGGAGE_KEY, baggage, context=context)
 
 
-def clear(context: typing.Optional[Context] = None) -> Context:
+def clear(context: Optional[Context] = None) -> Context:
     """Removes all values from the Baggage
 
     Args:
@@ -101,3 +120,22 @@ def clear(context: typing.Optional[Context] = None) -> Context:
         A Context with all baggage entries removed
     """
     return set_value(_BAGGAGE_KEY, {}, context=context)
+
+
+def _is_valid_key(name: str) -> bool:
+    return _KEY_PATTERN.fullmatch(str(name)) is not None
+
+
+def _is_valid_value(value: object) -> bool:
+    parts = str(value).split(";")
+    is_valid_value = _VALUE_PATTERN.fullmatch(parts[0]) is not None
+    if len(parts) > 1:  # one or more properties metadata
+        for property in parts[1:]:
+            if _PROPERT_PATTERN.fullmatch(property) is None:
+                is_valid_value = False
+                break
+    return is_valid_value
+
+
+def _is_valid_pair(key: str, value: str) -> bool:
+    return _is_valid_key(key) and _is_valid_value(value)

opentelemetry-api/src/opentelemetry/baggage/propagation/__init__.py

@@ -12,13 +12,18 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 #
-import typing
+from logging import getLogger
+from re import split
+from typing import Iterable, Mapping, Optional, Set
 from urllib.parse import quote_plus, unquote_plus
 
-from opentelemetry.baggage import get_all, set_baggage
+from opentelemetry.baggage import _is_valid_pair, get_all, set_baggage
 from opentelemetry.context import get_current
 from opentelemetry.context.context import Context
 from opentelemetry.propagators import textmap
+from opentelemetry.util.re import _DELIMITER_PATTERN
+
+_logger = getLogger(__name__)
 
 
 class W3CBaggagePropagator(textmap.TextMapPropagator):
@@ -32,7 +37,7 @@ class W3CBaggagePropagator(textmap.TextMapPropagator):
     def extract(
         self,
         carrier: textmap.CarrierT,
-        context: typing.Optional[Context] = None,
+        context: Optional[Context] = None,
         getter: textmap.Getter = textmap.default_getter,
     ) -> Context:
         """Extract Baggage from the carrier.
@@ -49,20 +54,46 @@ def extract(
         )
 
         if not header or len(header) > self._MAX_HEADER_LENGTH:
+            _logger.warning(
+                "Baggage header `%s` exceeded the maximum number of bytes per baggage-string",
+                header,
+            )
             return context
 
-        baggage_entries = header.split(",")
+        baggage_entries = split(_DELIMITER_PATTERN, header)
         total_baggage_entries = self._MAX_PAIRS
+
+        if len(baggage_entries) > self._MAX_PAIRS:
+            _logger.warning(
+                "Baggage header `%s` exceeded the maximum number of list-members",
+                header,
+            )
+
         for entry in baggage_entries:
             if len(entry) > self._MAX_PAIR_LENGTH:
+                _logger.warning(
+                    "Baggage entry `%s` exceeded the maximum number of bytes per list-member",
+                    entry,
+                )
+                continue
+            if not entry:  # empty string
                 continue
             try:
                 name, value = entry.split("=", 1)
             except Exception:  # pylint: disable=broad-except
+                _logger.warning(
+                    "Baggage list-member `%s` doesn't match the format", entry
+                )
                 continue
+            name = unquote_plus(name).strip().lower()
+            value = unquote_plus(value).strip()
+            if not _is_valid_pair(name, value):
+                _logger.warning("Invalid baggage entry: `%s`", entry)
+                continue
+
             context = set_baggage(
-                unquote_plus(name).strip(),
-                unquote_plus(value).strip(),
+                name,
+                value,
                 context=context,
             )
             total_baggage_entries -= 1
@@ -74,7 +105,7 @@ def extract(
     def inject(
         self,
         carrier: textmap.CarrierT,
-        context: typing.Optional[Context] = None,
+        context: Optional[Context] = None,
         setter: textmap.Setter = textmap.default_setter,
     ) -> None:
         """Injects Baggage into the carrier.
@@ -90,21 +121,21 @@ def inject(
         setter.set(carrier, self._BAGGAGE_HEADER_NAME, baggage_string)
 
     @property
-    def fields(self) -> typing.Set[str]:
+    def fields(self) -> Set[str]:
         """Returns a set with the fields set in `inject`."""
         return {self._BAGGAGE_HEADER_NAME}
 
 
-def _format_baggage(baggage_entries: typing.Mapping[str, object]) -> str:
+def _format_baggage(baggage_entries: Mapping[str, object]) -> str:
     return ",".join(
         quote_plus(str(key)) + "=" + quote_plus(str(value))
         for key, value in baggage_entries.items()
     )
 
 
 def _extract_first_element(
-    items: typing.Optional[typing.Iterable[textmap.CarrierT]],
-) -> typing.Optional[textmap.CarrierT]:
+    items: Optional[Iterable[textmap.CarrierT]],
+) -> Optional[textmap.CarrierT]:
     if items is None:
         return None
     return next(iter(items), None)

opentelemetry-api/src/opentelemetry/util/re.py

@@ -27,10 +27,12 @@
 )
 # A value contains a URL encoded UTF-8 string.
 _VALUE_FORMAT = r"[\x21\x23-\x2b\x2d-\x3a\x3c-\x5b\x5d-\x7e]*"
-_HEADER_FORMAT = _KEY_FORMAT + _OWS + r"=" + _OWS + _VALUE_FORMAT
-_HEADER_PATTERN = compile(_HEADER_FORMAT)
+_KEY_VALUE_FORMAT = rf"{_OWS}{_KEY_FORMAT}{_OWS}={_OWS}{_VALUE_FORMAT}{_OWS}"
+_HEADER_PATTERN = compile(_KEY_VALUE_FORMAT)
 _DELIMITER_PATTERN = compile(r"[ \t]*,[ \t]*")
 
+_BAGGAGE_PROPERTY_FORMAT = rf"{_KEY_VALUE_FORMAT}|{_OWS}{_KEY_FORMAT}{_OWS}"
+
 
 # pylint: disable=invalid-name
 def parse_headers(s: str) -> Mapping[str, str]: