From 8d9b5c5f3019322fdd6d7bd88026e4cb39405a8d Mon Sep 17 00:00:00 2001 From: Mario Jonke Date: Fri, 24 Sep 2021 10:24:03 +0200 Subject: [PATCH 1/4] Fix validity calculation for trace/span ID * fixes trace ID validation which was off by one and considered the max trace ID (0xffff...ffff) value as invalid. * adds check to also validate a span ID for its max value * adds checks that trace and span ID must not be negative --- .../src/opentelemetry/trace/span.py | 8 ++-- .../tests/trace/test_span_context.py | 44 +++++++++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/opentelemetry-api/src/opentelemetry/trace/span.py b/opentelemetry-api/src/opentelemetry/trace/span.py index 239fbfd3af..4d7e692710 100644 --- a/opentelemetry-api/src/opentelemetry/trace/span.py +++ b/opentelemetry-api/src/opentelemetry/trace/span.py @@ -387,7 +387,8 @@ def values(self) -> typing.ValuesView[str]: DEFAULT_TRACE_STATE = TraceState.get_default() -_TRACE_ID_HEX_LENGTH = 2 ** 128 - 1 +_TRACE_ID_MAX_VALUE = 2 ** 128 - 1 +_SPAN_ID_MAX_VALUE = 2 ** 64 - 1 class SpanContext( @@ -420,9 +421,8 @@ def __new__( trace_state = DEFAULT_TRACE_STATE is_valid = ( - trace_id != INVALID_TRACE_ID - and span_id != INVALID_SPAN_ID - and trace_id < _TRACE_ID_HEX_LENGTH + 0 < trace_id <= _TRACE_ID_MAX_VALUE + and 0 < span_id <= _SPAN_ID_MAX_VALUE ) return tuple.__new__( diff --git a/opentelemetry-api/tests/trace/test_span_context.py b/opentelemetry-api/tests/trace/test_span_context.py index 1ec32253c3..55abb0f559 100644 --- a/opentelemetry-api/tests/trace/test_span_context.py +++ b/opentelemetry-api/tests/trace/test_span_context.py @@ -43,3 +43,47 @@ def test_span_context_pickle(self): trace_state=trace.DEFAULT_TRACE_STATE, ) self.assertFalse(invalid_sc.is_valid) + + def test_trace_id_validity(self): + trace_id_max_value = int("f" * 32, 16) + span_id = 1 + + # valid trace IDs + sc = trace.SpanContext(trace_id_max_value, span_id, is_remote=False) + self.assertTrue(sc.is_valid) + + sc = trace.SpanContext(1, span_id, is_remote=False) + self.assertTrue(sc.is_valid) + + # invalid trace IDs + sc = trace.SpanContext(0, span_id, is_remote=False) + self.assertFalse(sc.is_valid) + + sc = trace.SpanContext(-1, span_id, is_remote=False) + self.assertFalse(sc.is_valid) + + sc = trace.SpanContext( + trace_id_max_value + 1, span_id, is_remote=False + ) + self.assertFalse(sc.is_valid) + + def test_span_id_validity(self): + span_id_max = int("f" * 16, 16) + trace_id = 1 + + # valid span IDs + sc = trace.SpanContext(trace_id, span_id_max, is_remote=False) + self.assertTrue(sc.is_valid) + + sc = trace.SpanContext(trace_id, 1, is_remote=False) + self.assertTrue(sc.is_valid) + + # invalid span IDs + sc = trace.SpanContext(trace_id, 0, is_remote=False) + self.assertFalse(sc.is_valid) + + sc = trace.SpanContext(trace_id, -1, is_remote=False) + self.assertFalse(sc.is_valid) + + sc = trace.SpanContext(trace_id, span_id_max + 1, is_remote=False) + self.assertFalse(sc.is_valid) From dd22410fab7a2c0b728363431c645ae0861ea2c3 Mon Sep 17 00:00:00 2001 From: Mario Jonke Date: Fri, 24 Sep 2021 11:10:59 +0200 Subject: [PATCH 2/4] changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e07232d06..42ac86206c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#2101](https://github.com/open-telemetry/opentelemetry-python/pull/2101)) - Fix incorrect headers parsing via environment variables ([#2103](https://github.com/open-telemetry/opentelemetry-python/pull/2103)) +- Fix validity calculation for trace and span IDs + ([#2145](https://github.com/open-telemetry/opentelemetry-python/pull/2145)) ## [1.5.0-0.24b0](https://github.com/open-telemetry/opentelemetry-python/releases/tag/v1.5.0-0.24b0) - 2021-08-26 From 50d41b31831edf8473149a7e41be76541e1a022a Mon Sep 17 00:00:00 2001 From: Mario Jonke Date: Tue, 28 Sep 2021 07:36:12 +0200 Subject: [PATCH 3/4] use constants in trace/span ID validity check --- opentelemetry-api/src/opentelemetry/trace/span.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/opentelemetry-api/src/opentelemetry/trace/span.py b/opentelemetry-api/src/opentelemetry/trace/span.py index 4d7e692710..23eac7337e 100644 --- a/opentelemetry-api/src/opentelemetry/trace/span.py +++ b/opentelemetry-api/src/opentelemetry/trace/span.py @@ -421,8 +421,8 @@ def __new__( trace_state = DEFAULT_TRACE_STATE is_valid = ( - 0 < trace_id <= _TRACE_ID_MAX_VALUE - and 0 < span_id <= _SPAN_ID_MAX_VALUE + INVALID_TRACE_ID < trace_id <= _TRACE_ID_MAX_VALUE + and INVALID_SPAN_ID < span_id <= _SPAN_ID_MAX_VALUE ) return tuple.__new__( From 06fcb5a9270aba9ab187615e0cf211fa5347a7a8 Mon Sep 17 00:00:00 2001 From: Mario Jonke Date: Tue, 28 Sep 2021 08:07:42 +0200 Subject: [PATCH 4/4] update contrib sha --- .github/workflows/test.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 0377126522..795a288f2e 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -10,7 +10,7 @@ env: # Otherwise, set variable to the commit of your branch on # opentelemetry-python-contrib which is compatible with these Core repo # changes. - CONTRIB_REPO_SHA: dde62cebffe519c35875af6d06fae053b3be65ec + CONTRIB_REPO_SHA: d2984f5242ed2250ad1c11b6164e2e8e11e2a804 jobs: build: