From 9350f485f90c9980699cdee838c183af1d64ff72 Mon Sep 17 00:00:00 2001 From: MitchellDumovic Date: Mon, 27 Jul 2020 10:50:10 -0700 Subject: [PATCH] B3: Add Additional Validity Checks and tests for Trace and Span ID (#1457) --- .../trace/propagation/B3Propagator.java | 1 + .../propagation/B3PropagatorExtractor.java | 18 +++++- .../B3PropagatorExtractorMultipleHeaders.java | 7 +-- .../trace/propagation/B3PropagatorTest.java | 58 ++++++++++++++++--- 4 files changed, 69 insertions(+), 15 deletions(-) diff --git a/extensions/trace_propagators/src/main/java/io/opentelemetry/extensions/trace/propagation/B3Propagator.java b/extensions/trace_propagators/src/main/java/io/opentelemetry/extensions/trace/propagation/B3Propagator.java index 5caff59d16..51620de313 100644 --- a/extensions/trace_propagators/src/main/java/io/opentelemetry/extensions/trace/propagation/B3Propagator.java +++ b/extensions/trace_propagators/src/main/java/io/opentelemetry/extensions/trace/propagation/B3Propagator.java @@ -38,6 +38,7 @@ public class B3Propagator implements HttpTextFormat { static final String FALSE_INT = "0"; static final String COMBINED_HEADER = "b3"; static final String COMBINED_HEADER_DELIMITER = "-"; + static final int MIN_TRACE_ID_LENGTH = TraceId.getSize(); static final int MAX_TRACE_ID_LENGTH = 2 * TraceId.getSize(); static final int MAX_SPAN_ID_LENGTH = 2 * SpanId.getSize(); diff --git a/extensions/trace_propagators/src/main/java/io/opentelemetry/extensions/trace/propagation/B3PropagatorExtractor.java b/extensions/trace_propagators/src/main/java/io/opentelemetry/extensions/trace/propagation/B3PropagatorExtractor.java index da66e1c5bb..6de659a006 100644 --- a/extensions/trace_propagators/src/main/java/io/opentelemetry/extensions/trace/propagation/B3PropagatorExtractor.java +++ b/extensions/trace_propagators/src/main/java/io/opentelemetry/extensions/trace/propagation/B3PropagatorExtractor.java @@ -18,6 +18,7 @@ package io.opentelemetry.extensions.trace.propagation; import static io.opentelemetry.extensions.trace.propagation.B3Propagator.MAX_SPAN_ID_LENGTH; import static io.opentelemetry.extensions.trace.propagation.B3Propagator.MAX_TRACE_ID_LENGTH; +import static io.opentelemetry.extensions.trace.propagation.B3Propagator.MIN_TRACE_ID_LENGTH; import static io.opentelemetry.extensions.trace.propagation.B3Propagator.TRUE_INT; import io.grpc.Context; @@ -63,12 +64,25 @@ interface B3PropagatorExtractor { } } + private static boolean isHex(String value) { + for (int i = 0; i < value.length(); i++) { + if (Character.digit(value.charAt(i), 16) == -1) { + return false; + } + } + return true; + } + static boolean isTraceIdValid(String value) { - return !(StringUtils.isNullOrEmpty(value) || value.length() > MAX_TRACE_ID_LENGTH); + return !(StringUtils.isNullOrEmpty(value) + || (value.length() != MIN_TRACE_ID_LENGTH && value.length() != MAX_TRACE_ID_LENGTH) + || !isHex(value)); } static boolean isSpanIdValid(String value) { - return !(StringUtils.isNullOrEmpty(value) || value.length() > MAX_SPAN_ID_LENGTH); + return !(StringUtils.isNullOrEmpty(value) + || value.length() != MAX_SPAN_ID_LENGTH + || !isHex(value)); } } } diff --git a/extensions/trace_propagators/src/main/java/io/opentelemetry/extensions/trace/propagation/B3PropagatorExtractorMultipleHeaders.java b/extensions/trace_propagators/src/main/java/io/opentelemetry/extensions/trace/propagation/B3PropagatorExtractorMultipleHeaders.java index 36b5457836..d2b9b2bc3a 100644 --- a/extensions/trace_propagators/src/main/java/io/opentelemetry/extensions/trace/propagation/B3PropagatorExtractorMultipleHeaders.java +++ b/extensions/trace_propagators/src/main/java/io/opentelemetry/extensions/trace/propagation/B3PropagatorExtractorMultipleHeaders.java @@ -51,16 +51,13 @@ final class B3PropagatorExtractorMultipleHeaders implements B3PropagatorExtracto String traceId = getter.get(carrier, TRACE_ID_HEADER); if (!Util.isTraceIdValid(traceId)) { logger.info( - "Invalid TraceId in B3 header: " - + TRACE_ID_HEADER - + "'. Returning INVALID span context."); + "Invalid TraceId in B3 header: " + traceId + "'. Returning INVALID span context."); return SpanContext.getInvalid(); } String spanId = getter.get(carrier, SPAN_ID_HEADER); if (!Util.isSpanIdValid(spanId)) { - logger.info( - "Invalid SpanId in B3 header: " + SPAN_ID_HEADER + "'. Returning INVALID span context."); + logger.info("Invalid SpanId in B3 header: " + spanId + "'. Returning INVALID span context."); return SpanContext.getInvalid(); } diff --git a/extensions/trace_propagators/src/test/java/io/opentelemetry/extensions/trace/propagation/B3PropagatorTest.java b/extensions/trace_propagators/src/test/java/io/opentelemetry/extensions/trace/propagation/B3PropagatorTest.java index d785ede47a..8af89ade8e 100644 --- a/extensions/trace_propagators/src/test/java/io/opentelemetry/extensions/trace/propagation/B3PropagatorTest.java +++ b/extensions/trace_propagators/src/test/java/io/opentelemetry/extensions/trace/propagation/B3PropagatorTest.java @@ -45,11 +45,13 @@ public class B3PropagatorTest { private static final TraceState TRACE_STATE_DEFAULT = TraceState.builder().build(); private static final String TRACE_ID_BASE16 = "ff000000000000000000000000000041"; + private static final String TRACE_ID_ALL_ZERO = "00000000000000000000000000000000"; private static final TraceId TRACE_ID = TraceId.fromLowerBase16(TRACE_ID_BASE16, 0); private static final String SHORT_TRACE_ID_BASE16 = "ff00000000000000"; private static final TraceId SHORT_TRACE_ID = TraceId.fromLowerBase16(StringUtils.padLeft(SHORT_TRACE_ID_BASE16, 32), 0); private static final String SPAN_ID_BASE16 = "ff00000000000041"; + private static final String SPAN_ID_ALL_ZERO = "0000000000000000"; private static final SpanId SPAN_ID = SpanId.fromLowerBase16(SPAN_ID_BASE16, 0); private static final byte SAMPLED_TRACE_OPTIONS_BYTES = 1; private static final TraceFlags SAMPLED_TRACE_OPTIONS = @@ -221,9 +223,9 @@ public class B3PropagatorTest { } @Test - public void extract_InvalidTraceId() { + public void extract_InvalidTraceId_NotHex() { Map invalidHeaders = new LinkedHashMap<>(); - invalidHeaders.put(B3Propagator.TRACE_ID_HEADER, "abcdefghijklmnopabcdefghijklmnop"); + invalidHeaders.put(B3Propagator.TRACE_ID_HEADER, "g" + TRACE_ID_BASE16.substring(1)); invalidHeaders.put(B3Propagator.SPAN_ID_HEADER, SPAN_ID_BASE16); invalidHeaders.put(B3Propagator.SAMPLED_HEADER, B3Propagator.TRUE_INT); assertThat(getSpanContext(b3Propagator.extract(Context.current(), invalidHeaders, getter))) @@ -231,7 +233,17 @@ public class B3PropagatorTest { } @Test - public void extract_InvalidTraceId_Size() { + public void extract_InvalidTraceId_TooShort() { + Map invalidHeaders = new LinkedHashMap<>(); + invalidHeaders.put(B3Propagator.TRACE_ID_HEADER, TRACE_ID_BASE16.substring(2)); + invalidHeaders.put(B3Propagator.SPAN_ID_HEADER, SPAN_ID_BASE16); + invalidHeaders.put(B3Propagator.SAMPLED_HEADER, B3Propagator.TRUE_INT); + assertThat(getSpanContext(b3Propagator.extract(Context.current(), invalidHeaders, getter))) + .isSameInstanceAs(SpanContext.getInvalid()); + } + + @Test + public void extract_InvalidTraceId_TooLong() { Map invalidHeaders = new LinkedHashMap<>(); invalidHeaders.put(B3Propagator.TRACE_ID_HEADER, TRACE_ID_BASE16 + "00"); invalidHeaders.put(B3Propagator.SPAN_ID_HEADER, SPAN_ID_BASE16); @@ -241,20 +253,50 @@ public class B3PropagatorTest { } @Test - public void extract_InvalidSpanId() { + public void extract_InvalidTraceId_AllZero() { Map invalidHeaders = new LinkedHashMap<>(); - invalidHeaders.put(B3Propagator.TRACE_ID_HEADER, TRACE_ID_BASE16); - invalidHeaders.put(B3Propagator.SPAN_ID_HEADER, "abcdefghijklmnop"); + invalidHeaders.put(B3Propagator.TRACE_ID_HEADER, TRACE_ID_ALL_ZERO); + invalidHeaders.put(B3Propagator.SPAN_ID_HEADER, SPAN_ID_BASE16); invalidHeaders.put(B3Propagator.SAMPLED_HEADER, B3Propagator.TRUE_INT); assertThat(getSpanContext(b3Propagator.extract(Context.current(), invalidHeaders, getter))) .isSameInstanceAs(SpanContext.getInvalid()); } @Test - public void extract_InvalidSpanId_Size() { + public void extract_InvalidSpanId_NotHex() { Map invalidHeaders = new LinkedHashMap<>(); invalidHeaders.put(B3Propagator.TRACE_ID_HEADER, TRACE_ID_BASE16); - invalidHeaders.put(B3Propagator.SPAN_ID_HEADER, "abcdefghijklmnop" + "00"); + invalidHeaders.put(B3Propagator.SPAN_ID_HEADER, "g" + SPAN_ID_BASE16.substring(1)); + invalidHeaders.put(B3Propagator.SAMPLED_HEADER, B3Propagator.TRUE_INT); + assertThat(getSpanContext(b3Propagator.extract(Context.current(), invalidHeaders, getter))) + .isSameInstanceAs(SpanContext.getInvalid()); + } + + @Test + public void extract_InvalidSpanId_TooShort() { + Map invalidHeaders = new LinkedHashMap<>(); + invalidHeaders.put(B3Propagator.TRACE_ID_HEADER, TRACE_ID_BASE16); + invalidHeaders.put(B3Propagator.SPAN_ID_HEADER, SPAN_ID_BASE16.substring(2)); + invalidHeaders.put(B3Propagator.SAMPLED_HEADER, B3Propagator.TRUE_INT); + assertThat(getSpanContext(b3Propagator.extract(Context.current(), invalidHeaders, getter))) + .isSameInstanceAs(SpanContext.getInvalid()); + } + + @Test + public void extract_InvalidSpanId_TooLong() { + Map invalidHeaders = new LinkedHashMap<>(); + invalidHeaders.put(B3Propagator.TRACE_ID_HEADER, TRACE_ID_BASE16); + invalidHeaders.put(B3Propagator.SPAN_ID_HEADER, SPAN_ID_BASE16 + "00"); + invalidHeaders.put(B3Propagator.SAMPLED_HEADER, B3Propagator.TRUE_INT); + assertThat(getSpanContext(b3Propagator.extract(Context.current(), invalidHeaders, getter))) + .isSameInstanceAs(SpanContext.getInvalid()); + } + + @Test + public void extract_InvalidSpanId_AllZeros() { + Map invalidHeaders = new LinkedHashMap<>(); + invalidHeaders.put(B3Propagator.TRACE_ID_HEADER, TRACE_ID_BASE16); + invalidHeaders.put(B3Propagator.SPAN_ID_HEADER, SPAN_ID_ALL_ZERO); invalidHeaders.put(B3Propagator.SAMPLED_HEADER, B3Propagator.TRUE_INT); assertThat(getSpanContext(b3Propagator.extract(Context.current(), invalidHeaders, getter))) .isSameInstanceAs(SpanContext.getInvalid());