Skip to content

Commit 12f82c8

Browse files
sl0thentr0pysl0thentr0py
committed
Implement strict_trace_continuation
Fixes a security hole where incoming traces from other orgs can cause a DOS-like attack on another org by injecting Sentry propagation headers. Spec: https://develop.sentry.dev/sdk/telemetry/traces/#stricttracecontinuation
1 parent c55c400 commit 12f82c8

File tree

6 files changed

+270
-27
lines changed

6 files changed

+270
-27
lines changed

‎sentry_sdk/consts.py‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1024,6 +1024,7 @@ def __init__(
10241024
enable_metrics=True, # type: bool
10251025
before_send_metric=None, # type: Optional[Callable[[Metric, Hint], Optional[Metric]]]
10261026
org_id=None, # type: Optional[str]
1027+
strict_trace_continuation=False, # type: bool
10271028
):
10281029
# type: (...) -> None
10291030
"""Initialize the Sentry SDK with the given parameters. All parameters described here can be used in a call to `sentry_sdk.init()`.
@@ -1427,6 +1428,15 @@ def __init__(
14271428
If `trace_ignore_status_codes` is not provided, requests with any status code
14281429
may be traced.
14291430
1431+
:param strict_trace_continuation: If set to `True`, the SDK will only continue a trace if the `org_id` of the incoming trace found in the
1432+
`baggage` header matches the `org_id` of the current Sentry client and only if BOTH are present.
1433+
1434+
If set to `False`, consistency of `org_id`s will only be enforced if both are present. If either are missing, the trace will be continued.
1435+
1436+
The client's organization ID is extracted from the DSN or can be set with the `org_id` option.
1437+
If the organization IDs do not match, the SDK will start a new trace instead of continuing the incoming one.
1438+
This is useful to prevent traces of unknown third-party services from being continued in your application.
1439+
14301440
:param org_id: An optional organization ID. The SDK will try to extract if from the DSN in most cases
14311441
but you can provide it explicitly for self-hosted and Relay setups. This value is used for
14321442
trace propagation and for features like `strict_trace_continuation`.

‎sentry_sdk/tracing_utils.py‎

Lines changed: 47 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
from sentry_sdk.utils import (
1616
capture_internal_exceptions,
1717
filename_for_module,
18-
Dsn,
1918
logger,
2019
match_regex_list,
2120
qualname_from_function,
@@ -453,15 +452,23 @@ def from_incoming_data(cls, incoming_data):
453452

454453
sentry_trace_header = normalized_data.get(SENTRY_TRACE_HEADER_NAME)
455454
sentrytrace_data = extract_sentrytrace_data(sentry_trace_header)
455+
456+
# nothing to propagate if no sentry-trace
456457
if sentrytrace_data is None:
457458
return None
458459

460+
baggage_header = normalized_data.get(BAGGAGE_HEADER_NAME)
461+
baggage = (
462+
Baggage.from_incoming_header(baggage_header) if baggage_header else None
463+
)
464+
465+
if not _should_continue_trace(baggage):
466+
return None
467+
459468
propagation_context = PropagationContext()
460469
propagation_context.update(sentrytrace_data)
461-
462-
baggage_header = normalized_data.get(BAGGAGE_HEADER_NAME)
463-
if baggage_header:
464-
propagation_context.baggage = Baggage.from_incoming_header(baggage_header)
470+
if baggage:
471+
propagation_context.baggage = baggage
465472

466473
propagation_context._fill_sample_rand()
467474

@@ -1230,6 +1237,41 @@ def _set_output_attributes(span, template, send_pii, result):
12301237
span.update_data(_get_output_attributes(template, send_pii, result) or {})
12311238

12321239

1240+
def _should_continue_trace(baggage):
1241+
# type: (Optional[Baggage]) -> bool
1242+
"""
1243+
Check if we should continue the incoming trace according to the strict_trace_continuation spec.
1244+
https://develop.sentry.dev/sdk/telemetry/traces/#stricttracecontinuation
1245+
"""
1246+
1247+
client = sentry_sdk.get_client()
1248+
parsed_dsn = client.parsed_dsn
1249+
client_org_id = parsed_dsn.org_id if parsed_dsn else None
1250+
baggage_org_id = baggage.sentry_items.get("org_id") if baggage else None
1251+
1252+
if (
1253+
client_org_id is not None
1254+
and baggage_org_id is not None
1255+
and client_org_id != baggage_org_id
1256+
):
1257+
logger.debug(
1258+
f"Starting a new trace because org IDs don't match (incoming baggage org_id: {baggage_org_id}, SDK org_id: {client_org_id})"
1259+
)
1260+
return False
1261+
1262+
strict_trace_continuation = client.options.get("strict_trace_continuation", False) # type: bool
1263+
if strict_trace_continuation:
1264+
if (baggage_org_id is not None and client_org_id is None) or (
1265+
baggage_org_id is None and client_org_id is not None
1266+
):
1267+
logger.debug(
1268+
f"Starting a new trace because strict trace continuation is enabled and one org ID is missing (incoming baggage org_id: {baggage_org_id}, SDK org_id: {client_org_id})"
1269+
)
1270+
return False
1271+
1272+
return True
1273+
1274+
12331275
# Circular imports
12341276
from sentry_sdk.tracing import (
12351277
BAGGAGE_HEADER_NAME,

‎tests/conftest.py‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,19 @@ def capture_envelope(self, _: Envelope) -> None:
221221
pass
222222

223223

224+
class TestTransportWithOptions(Transport):
225+
"""TestTransport above does not pass in the options and for some tests we need them"""
226+
227+
__test__ = False
228+
229+
def __init__(self, options=None):
230+
Transport.__init__(self, options)
231+
232+
def capture_envelope(self, _: Envelope) -> None:
233+
"""No-op capture_envelope for tests"""
234+
pass
235+
236+
224237
@pytest.fixture
225238
def capture_events(monkeypatch):
226239
def inner():

‎tests/test_dsc.py‎

Lines changed: 8 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -13,19 +13,7 @@
1313
import pytest
1414

1515
import sentry_sdk
16-
from sentry_sdk.transport import Transport
17-
from sentry_sdk.envelope import Envelope
18-
19-
20-
class TransportWithOptions(Transport):
21-
"""conftest.TestTransport does not pass in the options so we need this here"""
22-
23-
def __init__(self, options=None):
24-
Transport.__init__(self, options)
25-
26-
def capture_envelope(self, _: Envelope) -> None:
27-
"""No-op capture_envelope for tests"""
28-
pass
16+
from tests.conftest import TestTransportWithOptions
2917

3018

3119
def test_dsc_head_of_trace(sentry_init, capture_envelopes):
@@ -38,7 +26,7 @@ def test_dsc_head_of_trace(sentry_init, capture_envelopes):
3826
release="myapp@0.0.1",
3927
environment="canary",
4028
traces_sample_rate=1.0,
41-
transport=TransportWithOptions,
29+
transport=TestTransportWithOptions,
4230
)
4331
envelopes = capture_envelopes()
4432

@@ -94,7 +82,7 @@ def test_dsc_head_of_trace_uses_custom_org_id(sentry_init, capture_envelopes):
9482
release="myapp@0.0.1",
9583
environment="canary",
9684
traces_sample_rate=1.0,
97-
transport=TransportWithOptions,
85+
transport=TestTransportWithOptions,
9886
)
9987
envelopes = capture_envelopes()
10088

@@ -122,7 +110,7 @@ def test_dsc_continuation_of_trace(sentry_init, capture_envelopes):
122110
release="myapp@0.0.1",
123111
environment="canary",
124112
traces_sample_rate=1.0,
125-
transport=TransportWithOptions,
113+
transport=TestTransportWithOptions,
126114
)
127115
envelopes = capture_envelopes()
128116

@@ -200,7 +188,7 @@ def my_traces_sampler(sampling_context):
200188
release="myapp@0.0.1",
201189
environment="canary",
202190
traces_sampler=my_traces_sampler,
203-
transport=TransportWithOptions,
191+
transport=TestTransportWithOptions,
204192
)
205193
envelopes = capture_envelopes()
206194

@@ -270,7 +258,7 @@ def test_dsc_issue(sentry_init, capture_envelopes):
270258
dsn="https://mysecret@o1234.ingest.sentry.io/12312012",
271259
release="myapp@0.0.1",
272260
environment="canary",
273-
transport=TransportWithOptions,
261+
transport=TestTransportWithOptions,
274262
)
275263
envelopes = capture_envelopes()
276264

@@ -322,7 +310,7 @@ def test_dsc_issue_with_tracing(sentry_init, capture_envelopes):
322310
release="myapp@0.0.1",
323311
environment="canary",
324312
traces_sample_rate=1.0,
325-
transport=TransportWithOptions,
313+
transport=TestTransportWithOptions,
326314
)
327315
envelopes = capture_envelopes()
328316

@@ -394,7 +382,7 @@ def test_dsc_issue_twp(sentry_init, capture_envelopes, traces_sample_rate):
394382
release="myapp@0.0.1",
395383
environment="canary",
396384
traces_sample_rate=traces_sample_rate,
397-
transport=TransportWithOptions,
385+
transport=TestTransportWithOptions,
398386
)
399387
envelopes = capture_envelopes()
400388

‎tests/test_tracing_utils.py‎

Lines changed: 134 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,13 @@
1+
import pytest
12
from dataclasses import asdict, dataclass
23
from typing import Optional, List
34

4-
from sentry_sdk.tracing_utils import _should_be_included, Baggage
5-
import pytest
5+
from sentry_sdk.tracing_utils import (
6+
_should_be_included,
7+
_should_continue_trace,
8+
Baggage,
9+
)
10+
from tests.conftest import TestTransportWithOptions
611

712

813
def id_function(val):
@@ -146,3 +151,130 @@ def test_strip_sentry_baggage(header, expected):
146151
)
147152
def test_baggage_repr(baggage, expected_repr):
148153
assert repr(baggage) == expected_repr
154+
155+
156+
@pytest.mark.parametrize(
157+
(
158+
"baggage_header",
159+
"dsn",
160+
"explicit_org_id",
161+
"strict_trace_continuation",
162+
"should_continue_trace",
163+
),
164+
(
165+
# continue cases when strict_trace_continuation=False
166+
(
167+
"sentry-trace_id=771a43a4192642f0b136d5159a501700, sentry-org_id=1234",
168+
"https://mysecret@o1234.ingest.sentry.io/12312012",
169+
None,
170+
False,
171+
True,
172+
),
173+
(
174+
"sentry-trace_id=771a43a4192642f0b136d5159a501700",
175+
"https://mysecret@o1234.ingest.sentry.io/12312012",
176+
None,
177+
False,
178+
True,
179+
),
180+
(
181+
"sentry-trace_id=771a43a4192642f0b136d5159a501700, sentry-org_id=1234",
182+
None,
183+
None,
184+
False,
185+
True,
186+
),
187+
(
188+
"sentry-trace_id=771a43a4192642f0b136d5159a501700, sentry-org_id=1234",
189+
None,
190+
"1234",
191+
False,
192+
True,
193+
),
194+
(
195+
"sentry-trace_id=771a43a4192642f0b136d5159a501700, sentry-org_id=1234",
196+
"https://mysecret@not_org_id.ingest.sentry.io/12312012",
197+
None,
198+
False,
199+
True,
200+
),
201+
# start new cases when strict_trace_continuation=False
202+
(
203+
"sentry-trace_id=771a43a4192642f0b136d5159a501700, sentry-org_id=1234",
204+
"https://mysecret@o9999.ingest.sentry.io/12312012",
205+
None,
206+
False,
207+
False,
208+
),
209+
(
210+
"sentry-trace_id=771a43a4192642f0b136d5159a501700, sentry-org_id=1234",
211+
"https://mysecret@o1234.ingest.sentry.io/12312012",
212+
"9999",
213+
False,
214+
False,
215+
),
216+
# continue cases when strict_trace_continuation=True
217+
(
218+
"sentry-trace_id=771a43a4192642f0b136d5159a501700, sentry-org_id=1234",
219+
"https://mysecret@o1234.ingest.sentry.io/12312012",
220+
None,
221+
True,
222+
True,
223+
),
224+
("sentry-trace_id=771a43a4192642f0b136d5159a501700", None, None, True, True),
225+
# start new cases when strict_trace_continuation=True
226+
(
227+
"sentry-trace_id=771a43a4192642f0b136d5159a501700",
228+
"https://mysecret@o1234.ingest.sentry.io/12312012",
229+
None,
230+
True,
231+
False,
232+
),
233+
(
234+
"sentry-trace_id=771a43a4192642f0b136d5159a501700, sentry-org_id=1234",
235+
None,
236+
None,
237+
True,
238+
False,
239+
),
240+
(
241+
"sentry-trace_id=771a43a4192642f0b136d5159a501700, sentry-org_id=1234",
242+
"https://mysecret@not_org_id.ingest.sentry.io/12312012",
243+
None,
244+
True,
245+
False,
246+
),
247+
(
248+
"sentry-trace_id=771a43a4192642f0b136d5159a501700, sentry-org_id=1234",
249+
"https://mysecret@o9999.ingest.sentry.io/12312012",
250+
None,
251+
True,
252+
False,
253+
),
254+
(
255+
"sentry-trace_id=771a43a4192642f0b136d5159a501700, sentry-org_id=1234",
256+
"https://mysecret@o1234.ingest.sentry.io/12312012",
257+
"9999",
258+
True,
259+
False,
260+
),
261+
),
262+
)
263+
def test_should_continue_trace(
264+
sentry_init,
265+
baggage_header,
266+
dsn,
267+
explicit_org_id,
268+
strict_trace_continuation,
269+
should_continue_trace,
270+
):
271+
sentry_init(
272+
dsn=dsn,
273+
org_id=explicit_org_id,
274+
strict_trace_continuation=strict_trace_continuation,
275+
traces_sample_rate=1.0,
276+
transport=TestTransportWithOptions,
277+
)
278+
279+
baggage = Baggage.from_incoming_header(baggage_header) if baggage_header else None
280+
assert _should_continue_trace(baggage) == should_continue_trace

0 commit comments

Comments
 (0)