diff --git a/src/API/Trace/Propagation/TraceContextPropagator.php b/src/API/Trace/Propagation/TraceContextPropagator.php index 040501542..4e8fe0e91 100644 --- a/src/API/Trace/Propagation/TraceContextPropagator.php +++ b/src/API/Trace/Propagation/TraceContextPropagator.php @@ -93,20 +93,32 @@ private static function extractImpl($carrier, PropagationGetterInterface $getter return SpanContext::getInvalid(); } - // Traceparent = {version}-{trace-id}-{parent-id}-{trace-flags} + // traceParent = {version}-{trace-id}-{parent-id}-{trace-flags} $pieces = explode('-', $traceparent); - // Unable to extract traceparent. Expected 4 values - if (count($pieces) !== 4) { + // If the header does not have at least 4 pieces, it is invalid -- restart the trace. + if (count($pieces) < 4) { return SpanContext::getInvalid(); } [$version, $traceId, $spanId, $traceFlags] = $pieces; - // Validates the version, traceId, spanId and traceFlags - // Returns an invalid spanContext if any of the checks fail - if ($version !== self::VERSION || !SpanContext::isValidTraceId($traceId) || - !SpanContext::isValidSpanId($spanId) || !SpanContext::isValidTraceFlag($traceFlags)) { + /** + * Return invalid if: + * - Version is invalid (not 2 char hex or 'ff') + * - Trace version, trace ID, span ID or trace flag are invalid + */ + if (!SpanContext::isValidTraceVersion($version) + || !SpanContext::isValidTraceId($traceId) + || !SpanContext::isValidSpanId($spanId) + || !SpanContext::isValidTraceFlag($traceFlags) + ) { + return SpanContext::getInvalid(); + } + + // Return invalid if the trace version is not a future version but still has > 4 pieces. + $versionIsFuture = hexdec($version) > hexdec(self::VERSION); + if (count($pieces) > 4 && !$versionIsFuture) { return SpanContext::getInvalid(); } diff --git a/src/API/Trace/SpanContext.php b/src/API/Trace/SpanContext.php index 0b20b21d5..d846a9dad 100644 --- a/src/API/Trace/SpanContext.php +++ b/src/API/Trace/SpanContext.php @@ -11,6 +11,7 @@ final class SpanContext implements API\SpanContextInterface { public const INVALID_TRACE = '00000000000000000000000000000000'; + public const TRACE_VERSION_REGEX = '/^(?!ff)[\da-f]{2}$/'; public const VALID_TRACE = '/^[0-9a-f]{32}$/'; public const TRACE_LENGTH = 32; public const INVALID_SPAN = '0000000000000000'; @@ -56,6 +57,15 @@ public static function getInvalid(): API\SpanContextInterface return self::$invalidContext; } + /** + * @param string $traceVersion + * @return bool Returns a value that indicates whether a trace version is valid. + */ + public static function isValidTraceVersion(string $traceVersion): bool + { + return 1 === preg_match(self::TRACE_VERSION_REGEX, $traceVersion); + } + /** * @return bool Returns a value that indicates whether a trace id is valid */ diff --git a/src/API/Trace/TraceState.php b/src/API/Trace/TraceState.php index 5deb39175..06c82f949 100644 --- a/src/API/Trace/TraceState.php +++ b/src/API/Trace/TraceState.php @@ -7,6 +7,7 @@ use function array_reverse; use function array_walk; use function implode; +use function strlen; class TraceState implements TraceStateInterface { @@ -134,16 +135,16 @@ private function parse(string $rawTracestate): array { $parsedTracestate = []; - if (\strlen($rawTracestate) <= self::MAX_TRACESTATE_LENGTH) { + if (strlen($rawTracestate) <= self::MAX_TRACESTATE_LENGTH) { $listMembers = explode(self::LIST_MEMBERS_SEPARATOR, $rawTracestate); + // Discard tracestate if entries exceed max length. if (count($listMembers) > self::MAX_TRACESTATE_LIST_MEMBERS) { - - // Truncate the tracestate if it exceeds the maximum list-members allowed - // TODO: Log a message when truncation occurs - $listMembers = array_slice($listMembers, 0, self::MAX_TRACESTATE_LIST_MEMBERS); + // TODO: Log a message when we discard. + return []; } + $invalid = false; foreach ($listMembers as $listMember) { $vendor = explode(self::LIST_MEMBER_KEY_VALUE_SPLITTER, trim($listMember)); @@ -153,8 +154,17 @@ private function parse(string $rawTracestate): array // TODO: Log if we can't validate the key and value if ($this->validateKey($vendor[0]) && $this->validateValue($vendor[1])) { $parsedTracestate[$vendor[0]] = $vendor[1]; + + continue; } } + $invalid = true; + + break; + } + // Discard tracestate if any member is invalid. + if ($invalid) { + return []; } } diff --git a/src/Context/Propagation/SanitizeCombinedHeadersPropagationGetter.php b/src/Context/Propagation/SanitizeCombinedHeadersPropagationGetter.php new file mode 100644 index 000000000..40652982e --- /dev/null +++ b/src/Context/Propagation/SanitizeCombinedHeadersPropagationGetter.php @@ -0,0 +1,46 @@ +getter = $getter; + } + + public function keys($carrier): array + { + return $this->getter->keys($carrier); + } + + public function get($carrier, string $key): ?string + { + $value = $this->getter->get($carrier, $key); + if ($value === null) { + return null; + } + + return preg_replace( + [self::SERVER_CONCAT_HEADERS_REGEX, self::TRAILING_LEADING_SEPARATOR_REGEX], + [self::LIST_MEMBERS_SEPARATOR], + $value, + ); + } +} diff --git a/tests/TraceContext/W3CTestService/TestController.php b/tests/TraceContext/W3CTestService/TestController.php index 09201e6ee..b1d4cba33 100644 --- a/tests/TraceContext/W3CTestService/TestController.php +++ b/tests/TraceContext/W3CTestService/TestController.php @@ -4,9 +4,12 @@ namespace App\Controller; -use GuzzleHttp\Client; use OpenTelemetry\API\Trace\Propagation\TraceContextPropagator; -use OpenTelemetry\API\Trace\SpanContext; +use OpenTelemetry\Context\Context; +use OpenTelemetry\Context\Propagation\ArrayAccessGetterSetter; +use OpenTelemetry\Context\Propagation\SanitizeCombinedHeadersPropagationGetter; +use Symfony\Component\HttpClient\HttpClient; +use Symfony\Component\HttpClient\Psr18Client; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\Routing\Annotation\Route; @@ -15,39 +18,53 @@ class TestController { /** * @Route("/test") + * @throws \Psr\Http\Client\ClientExceptionInterface */ public function index(Request $request): Response { global $tracer; + if (!$tracer) { + return new Response('internal error', Response::HTTP_INTERNAL_SERVER_ERROR); + } + + $traceCtxPropagator = TraceContextPropagator::getInstance(); + $propagationGetter = new SanitizeCombinedHeadersPropagationGetter(new ArrayAccessGetterSetter()); + + try { + $context = $traceCtxPropagator->extract($request->headers->all(), $propagationGetter); + } catch (\InvalidArgumentException $th) { + $context = new Context(); + } - $array = $request->request->all(); $body = json_decode($request->getContent(), true); + if (!$body) { + return new Response('Invalid JSON', Response::HTTP_BAD_REQUEST); + } foreach ($body as $case) { - if ($tracer) { - $headers = ['content-type' => 'application/json']; - $url = $case['url']; - $arguments = $case['arguments']; + $headers = ['content-type' => 'application/json']; + $url = $case['url']; + $arguments = $case['arguments']; - try { - $context = TraceContextPropagator::extract($request->headers->all()); - } catch (\InvalidArgumentException $th) { - $context = SpanContext::generate(); - } + $span = $tracer->spanBuilder($url)->setParent($context)->startSpan(); + $context = $span->storeInContext($context); + $scope = $context->activate(); - $span = $tracer->startAndActivateSpanFromContext($url, $context, true); - TraceContextPropagator::inject($carrier, null, $context); + $traceCtxPropagator->inject($headers, null, $context); - $client = new Client([ + $client = new Psr18Client(HttpClient::create( + [ 'base_uri' => $url, 'timeout' => 2.0, - ]); + ] + )); + + $testServiceRequest = new \Nyholm\Psr7\Request('POST', $url, $headers, json_encode($arguments)); - $testServiceRequest = new \GuzzleHttp\Psr7\Request('POST', $url, $headers, json_encode($arguments)); - $response = $client->sendRequest($testServiceRequest); + $client->sendRequest($testServiceRequest); - $span->end(); - } + $span->end(); + $scope->detach(); } return new Response( diff --git a/tests/TraceContext/W3CTestService/index.php b/tests/TraceContext/W3CTestService/index.php index d3432eeaf..3cbabbffe 100644 --- a/tests/TraceContext/W3CTestService/index.php +++ b/tests/TraceContext/W3CTestService/index.php @@ -4,46 +4,37 @@ require_once __DIR__ . '/../vendor/autoload.php'; use App\Kernel; -use OpenTelemetry\API\Trace as API; -use OpenTelemetry\Context\Context; use OpenTelemetry\Contrib\Jaeger\Exporter as JaegerExporter; -use OpenTelemetry\SDK\Common\Time\ClockFactory; -use OpenTelemetry\SDK\Trace\Sampler\AlwaysOnSampler; -use OpenTelemetry\SDK\Trace\SamplingResult; use OpenTelemetry\SDK\Trace\SpanProcessor\BatchSpanProcessor; use OpenTelemetry\SDK\Trace\TracerProvider; use Symfony\Component\Dotenv\Dotenv; +use Symfony\Component\HttpClient\Psr18Client; use Symfony\Component\HttpFoundation\Request; (new Dotenv())->bootEnv(dirname(__DIR__) . '/.env'); -$sampler = new AlwaysOnSampler(); -$samplingResult = $sampler->shouldSample( - Context::getCurrent(), - md5((string) microtime(true)), - 'io.opentelemetry.example', - API\SpanKind::KIND_INTERNAL -); +$httpClient = new Psr18Client(); -$exporter = new JaegerExporter( - 'W3C Trace-Context Test Service', - 'http://localhost:9412/api/v2/spans' -); +$tracer = (new TracerProvider([ + new BatchSpanProcessor( + new JaegerExporter( + 'W3C Trace-Context Test Service', + 'http://localhost:9412/api/v2/spans', + $httpClient, + $httpClient, + $httpClient, + ) + ), +]))->getTracer('W3C Trace-Context Test Service'); -if (SamplingResult::RECORD_AND_SAMPLE === $samplingResult->getDecision()) { - $tracer = (new TracerProvider()) - ->addSpanProcessor(new BatchSpanProcessor($exporter, ClockFactory::getDefault())) - ->getTracer('io.opentelemetry.contrib.php'); - - $request = Request::createFromGlobals(); - $span = $tracer->startAndActivateSpan($request->getUri()); -} +$request = Request::createFromGlobals(); +$span = $tracer->spanBuilder($request->getUri())->startSpan(); +$spanScope = $span->activate(); $kernel = new Kernel($_SERVER['APP_ENV'], (bool) $_SERVER['APP_DEBUG']); $response = $kernel->handle($request); $response->send(); $kernel->terminate($request, $response); -if (SamplingResult::RECORD_AND_SAMPLED === $samplingResult->getDecision()) { - $span->end(); -} +$span->end(); +$spanScope->detach(); diff --git a/tests/TraceContext/W3CTestService/symfony-setup b/tests/TraceContext/W3CTestService/symfony-setup index 20dd52a61..5cab32c71 100755 --- a/tests/TraceContext/W3CTestService/symfony-setup +++ b/tests/TraceContext/W3CTestService/symfony-setup @@ -8,25 +8,28 @@ curl -sS https://get.symfony.com/cli/installer | bash mv /root/.symfony/bin/symfony /usr/local/bin/symfony # Set up Symfony test application -cd tests/TraceContext/W3CTestService +cd tests/TraceContext/W3CTestService || { printf "Something went wrong: could not cd into W3CTestService directory."; exit 1; } rm -rf test_app symfony new test_app --version=5.2 # Move our files into the test application cp index.php test_app/public/index.php cp TestController.php test_app/src/Controller/TestController.php -cd test_app -composer require annotations +cd test_app || { printf "Something went wrong: could not cd into test_app."; exit 1; } # Link the local opentelemetry changes to vendor/open-telemetry -composer require open-telemetry/opentelemetry "*" composer config repositories.local \ '{"type": "path", "url": "/usr/src/open-telemetry/", "options": {"symLink": true }}' \ --file composer.json composer config minimum-stability dev --file composer.json -composer require php-http/mock-client "*" + +composer require symfony/http-client +composer require annotations composer require nyholm/psr7 "*" +composer require open-telemetry/opentelemetry "*" +composer require php-http/mock-client "*" + rm composer.lock composer clearcache composer install @@ -41,7 +44,7 @@ cd .. rm -rf trace-context git clone https://github.com/w3c/trace-context.git -cd trace-context/test +cd trace-context/test || { printf "Failed to cd to trace-context/test" ; exit; } # Run the test python3 test.py http://127.0.0.1:8001/test diff --git a/tests/Unit/API/Trace/Propagation/TraceContextPropagatorTest.php b/tests/Unit/API/Trace/Propagation/TraceContextPropagatorTest.php index b77a4bb61..7c7cb8579 100644 --- a/tests/Unit/API/Trace/Propagation/TraceContextPropagatorTest.php +++ b/tests/Unit/API/Trace/Propagation/TraceContextPropagatorTest.php @@ -258,6 +258,57 @@ public function test_extract_empty_header(): void ]); } + public function test_extract_future_version(): void + { + $carrierFuture = [ + TraceContextPropagator::TRACEPARENT => 'aa-' . self::TRACE_ID_BASE16 . '-' . self::SPAN_ID_BASE16 . '-' . '00', + TraceContextPropagator::TRACESTATE => self::TRACESTATE_NOT_DEFAULT_ENCODING, + ]; + + // Tolerant of future versions with same parts. + $this->assertEquals( + SpanContext::createFromRemoteParent(self::TRACE_ID_BASE16, self::SPAN_ID_BASE16, SpanContextInterface::TRACE_FLAG_DEFAULT, $this->traceState), + $this->getSpanContext($this->traceContextPropagator->extract($carrierFuture)), + ); + + $carrierFutureMoreParts = [ + TraceContextPropagator::TRACEPARENT => 'af-' . self::TRACE_ID_BASE16 . '-' . self::SPAN_ID_BASE16 . '-' . '00' . '-000-this-is-the-future', + TraceContextPropagator::TRACESTATE => self::TRACESTATE_NOT_DEFAULT_ENCODING, + ]; + + // Tolerant of future versions with more parts. + $this->assertEquals( + SpanContext::createFromRemoteParent(self::TRACE_ID_BASE16, self::SPAN_ID_BASE16, SpanContextInterface::TRACE_FLAG_DEFAULT, $this->traceState), + $this->getSpanContext($this->traceContextPropagator->extract($carrierFutureMoreParts)), + ); + } + + public function test_invalid_traceparent_version_0xff(): void + { + $this->assertInvalid([ + TraceContextPropagator::TRACEPARENT => 'ff-' . self::TRACE_ID_BASE16 . '-' . self::SPAN_ID_BASE16 . '-' . '00', + ]); + } + + public function test_invalid_traceparent_version(): void + { + $this->assertInvalid([ + TraceContextPropagator::TRACEPARENT => 'aaa-' . self::TRACE_ID_BASE16 . '-' . self::SPAN_ID_BASE16 . '-' . '00', + ]); + + $this->assertInvalid([ + TraceContextPropagator::TRACEPARENT => 'gx-' . self::TRACE_ID_BASE16 . '-' . self::SPAN_ID_BASE16 . '-' . '00', + ]); + } + + public function test_invalid_trace_format(): void + { + // More than 4 parts to the trace but not a future version. + $this->assertInvalid([ + TraceContextPropagator::TRACEPARENT => '00-' . self::TRACE_ID_BASE16 . '-' . self::SPAN_ID_BASE16 . '-' . '00' . '-000-this-is-not-the-future', + ]); + } + public function test_invalid_trace_id(): void { $this->assertInvalid([ diff --git a/tests/Unit/API/Trace/TraceStateTest.php b/tests/Unit/API/Trace/TraceStateTest.php index e33f49c96..19ff6a877 100644 --- a/tests/Unit/API/Trace/TraceStateTest.php +++ b/tests/Unit/API/Trace/TraceStateTest.php @@ -6,6 +6,8 @@ use OpenTelemetry\API\Trace\TraceState; use PHPUnit\Framework\TestCase; +use function str_repeat; +use function strlen; /** * @covers OpenTelemetry\API\Trace\TraceState @@ -87,32 +89,32 @@ public function test_max_tracestate_list_members(): void $validTracestate = new TraceState(implode(TraceState::LIST_MEMBERS_SEPARATOR, $rawTraceState)); $this->assertSame(TraceState::MAX_TRACESTATE_LIST_MEMBERS, $validTracestate->getListMemberCount()); - // Add a list-member to the tracestate that exceeds the max of 32. This will cause it to be truncated + // Add a list-member to the tracestate that exceeds the max of 32. This will cause the tracestate to be discarded. $rawTraceState[32] = 'k32' . TraceState::LIST_MEMBER_KEY_VALUE_SPLITTER . 'v32'; $this->assertCount(TraceState::MAX_TRACESTATE_LIST_MEMBERS + 1, $rawTraceState); $truncatedTracestate = new TraceState(implode(TraceState::LIST_MEMBERS_SEPARATOR, $rawTraceState)); - $this->assertSame(TraceState::MAX_TRACESTATE_LIST_MEMBERS, $truncatedTracestate->getListMemberCount()); + $this->assertSame(0, $truncatedTracestate->getListMemberCount()); } public function test_max_tracestate_length(): void { // Build a vendor key with a length of 256 characters. The max characters allowed. - $vendorKey = \str_repeat('k', TraceState::MAX_TRACESTATE_LENGTH / 2); + $vendorKey = str_repeat('k', TraceState::MAX_TRACESTATE_LENGTH / 2); // Build a vendor value with a length of 255 characters. One below the max allowed. - $vendorValue = \str_repeat('v', TraceState::MAX_TRACESTATE_LENGTH / 2 - 1); + $vendorValue = str_repeat('v', TraceState::MAX_TRACESTATE_LENGTH / 2 - 1); // tracestate length = 513 characters (not accepted). $rawTraceState = $vendorKey . TraceState::LIST_MEMBER_KEY_VALUE_SPLITTER . $vendorValue . 'v'; - $this->assertGreaterThan(TraceState::MAX_TRACESTATE_LENGTH, \strlen($rawTraceState)); + $this->assertGreaterThan(TraceState::MAX_TRACESTATE_LENGTH, strlen($rawTraceState)); $validTracestate = new TraceState($rawTraceState); $this->assertNull($validTracestate->get($vendorKey)); // tracestate length = 512 characters (accepted). $rawTraceState = $vendorKey . TraceState::LIST_MEMBER_KEY_VALUE_SPLITTER . $vendorValue; - $this->assertSame(TraceState::MAX_TRACESTATE_LENGTH, \strlen($rawTraceState)); + $this->assertSame(TraceState::MAX_TRACESTATE_LENGTH, strlen($rawTraceState)); $validTracestate = new TraceState($rawTraceState); $this->assertSame($rawTraceState, (string) $validTracestate); @@ -135,61 +137,53 @@ public function test_validate_key(): void $mixedInvalidKeys = 'a=1=,c*d=2,@e/f=3,g_h=4,I-j=5,k&l=6'; $tracestate = new TraceState($mixedInvalidKeys); - $this->assertNull($tracestate->get('a=1')); - $this->assertNull($tracestate->get('@e')); - $this->assertNull($tracestate->get('I-j')); - $this->assertNull($tracestate->get('k&l')); - $this->assertSame('2', $tracestate->get('c*d')); - $this->assertSame('4', $tracestate->get('g_h')); - $this->assertSame('c*d=2,g_h=4', (string) $tracestate); + // Drop all keys on an invalid key + $this->assertSame(0, $tracestate->getListMemberCount()); // Tests a valid key with a length of 256 characters. The max characters allowed. - $validKey = \str_repeat('k', 256); + $validKey = str_repeat('k', 256); $validValue = 'v'; $tracestate = new TraceState($validKey . TraceState::LIST_MEMBER_KEY_VALUE_SPLITTER . $validValue); - $this->assertSame(256, \strlen($validKey)); + $this->assertSame(256, strlen($validKey)); $this->assertSame($validValue, $tracestate->get($validKey)); $this->assertSame($validKey . TraceState::LIST_MEMBER_KEY_VALUE_SPLITTER . $validValue, (string) $tracestate); // Tests an invalid key with a length of 257 characters. One more than the max characters allowed. - $invalidKey = \str_repeat('k', 257); + $invalidKey = str_repeat('k', 257); $tracestate = new TraceState($invalidKey . TraceState::LIST_MEMBER_KEY_VALUE_SPLITTER . $validValue); - $this->assertSame(257, \strlen($invalidKey)); + $this->assertSame(257, strlen($invalidKey)); $this->assertNull($tracestate->get($invalidKey)); } public function test_validate_value(): void { // Tests values are within the range of 0x20 to 0x7E characters - $tracestate = 'char1=value' . chr(0x19) . '1' + $tracestate = 'char1=value' . chr(0x19) . '1' . ',char2=value' . chr(0x20) . '2' . ',char3=value' . chr(0x7E) . '3' . ',char4=value' . chr(0x7F) . '4'; $parsedTracestate = new TraceState($tracestate); - $this->assertNull($parsedTracestate->get('char1')); - $this->assertNull($parsedTracestate->get('char4')); - $this->assertSame('value' . chr(0x20) . '2', $parsedTracestate->get('char2')); - $this->assertSame('value' . chr(0x7E) . '3', $parsedTracestate->get('char3')); - $this->assertSame('char2=value' . chr(0x20) . '2,char3=value' . chr(0x7E) . '3', (string) $parsedTracestate); + // Entire tracestate is dropped since the value for char1 is invalid. + $this->assertSame(0, $parsedTracestate->getListMemberCount()); // Tests a valid value with a length of 256 characters. The max allowed. - $validValue = \str_repeat('v', 256); + $validValue = str_repeat('v', 256); $validKey = 'k'; $tracestate = new TraceState($validKey . TraceState::LIST_MEMBER_KEY_VALUE_SPLITTER . $validValue); - $this->assertSame(256, \strlen($validValue)); + $this->assertSame(256, strlen($validValue)); $this->assertSame($validValue, $tracestate->get($validKey)); $this->assertSame($validKey . TraceState::LIST_MEMBER_KEY_VALUE_SPLITTER . $validValue, (string) $tracestate); // Tests an invalid value with a length of 257 characters. One more than the max allowed. - $invalidValue = \str_repeat('v', 257); + $invalidValue = str_repeat('v', 257); $tracestate = new TraceState($validKey . TraceState::LIST_MEMBER_KEY_VALUE_SPLITTER . $invalidValue); - $this->assertSame(257, \strlen($invalidValue)); + $this->assertSame(257, strlen($invalidValue)); $this->assertNull($tracestate->get($validKey)); } }