Skip to content

Commit

Permalink
fix(propagation): filter out unsupported dd_origin values (#6854)
Browse files Browse the repository at this point in the history
We ran into situations where public clients were sending HTTP requests
with unsupported non-ascii characters in the `x-datadog-origin` headers.
These headers were being propagated, via the
[`HTTPPropagator.extract`](https://github.com/DataDog/dd-trace-py/blob/fd0820b9d7b561d6b75b75c6865a5b3b5792054d/ddtrace/propagation/http.py#L835)
and [gRPC client
interceptor](https://github.com/DataDog/dd-trace-py/blob/c5dfcec45ae21483171c0ac148f94e61ade0b4d2/ddtrace/contrib/grpc/client_interceptor.py#L203),
to branching gRPC calls. The gRPC calls are then failing on the client
side due to gRPC client side validation which [forbids metadata values
with non-ascii](grpc/grpc-go#5318) characters.

The proposed solution here will prevent these unsupported headers values
from being propagated by preventing them from being set within
`Context`. This approach may impact HTTP propagation to branching HTTP
requests, but my understanding is that non-ascii characters are not
supported in these situations either.

## Checklist

- [x] Change(s) are motivated and described in the PR description.
- [x] Testing strategy is described if automated tests are not included
in the PR.
- [x] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
- [x] If this PR touches code that signs or publishes builds or
packages, or handles credentials of any kind, I've requested a review
from `@DataDog/security-design-and-guidance`.
- [x] This PR doesn't touch any of that.

---------

Co-authored-by: ZStriker19 <zach.groves@datadoghq.com>
Co-authored-by: Zachary Groves <32471391+ZStriker19@users.noreply.github.com>
  • Loading branch information
3 people committed Sep 8, 2023
1 parent 2f366b6 commit 7903340
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 1 deletion.
4 changes: 3 additions & 1 deletion ddtrace/context.py
Expand Up @@ -32,6 +32,8 @@
]


_DD_ORIGIN_INVALID_CHARS_REGEX = re.compile(r"[^\x20-\x7E]+")

log = get_logger(__name__)


Expand Down Expand Up @@ -64,7 +66,7 @@ def __init__(
self.trace_id = trace_id # type: Optional[int]
self.span_id = span_id # type: Optional[int]

if dd_origin is not None:
if dd_origin is not None and _DD_ORIGIN_INVALID_CHARS_REGEX.search(dd_origin) is None:
self._meta[ORIGIN_KEY] = dd_origin
if sampling_priority is not None:
self._metrics[SAMPLING_PRIORITY_KEY] = sampling_priority
Expand Down
1 change: 1 addition & 0 deletions docs/spelling_wordlist.txt
Expand Up @@ -27,6 +27,7 @@ app
appsec
aredis
args
ascii
asgi
async
asyncio
Expand Down
@@ -0,0 +1,4 @@
---
fixes:
- |
propagation: Prevent propagating unsupported non-ascii ``origin`` header values.
18 changes: 18 additions & 0 deletions tests/tracer/test_context.py
@@ -1,5 +1,6 @@
# -*- coding: utf-8 -*-
import pickle
from typing import Optional

import pytest

Expand Down Expand Up @@ -326,3 +327,20 @@ def test_traceparent(context, expected_traceparent):
def test_tracestate(context, expected_tracestate):
# type: (Context,str) -> None
assert context._tracestate == expected_tracestate


@pytest.mark.parametrize(
"ctx,expected_dd_origin",
[
(Context(), None),
(Context(dd_origin="abcABC"), "abcABC"),
(Context(dd_origin="abcABC123"), "abcABC123"),
(Context(dd_origin="!@#$%^&*()>?"), "!@#$%^&*()>?"),
(Context(dd_origin="\x00\x10"), None),
(Context(dd_origin="\x80"), None),
(Context(dd_origin="§¢À"), None),
],
)
def test_dd_origin_character_set(ctx, expected_dd_origin):
# type: (Context,Optional[str]) -> None
assert ctx.dd_origin == expected_dd_origin

0 comments on commit 7903340

Please sign in to comment.