Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request / response ip filter #28

Merged
merged 9 commits into from
Jul 4, 2024
8 changes: 6 additions & 2 deletions docs/configuration/tracecontext.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,14 @@ return static function (SymfonyTraceConfig $config): void {
// If true a value in the `traceparent` header in the request
// will be used and parsed to get the trace ID for the rest of the request. If false
// those values are ignored and new trace ID's are generated.
$config->trustRequestHeader(true);
frankdekker marked this conversation as resolved.
Show resolved Hide resolved
$config->request()
->trustHeader(true)
->trustedIps(env('TRUSTED_IPS')); // Only trust the header from these IP's

// Whether to send the trace details in the response headers. This is turned on by default.
$config->sendResponseHeader(true);
$config->response()
->sendHeader(true)
->trustedIps(env('TRUSTED_IPS')); // Only send the header to these IP's

// The service key of an object that implements
// DR\SymfonyTraceBundle\TraceStorageInterface
Expand Down
9 changes: 7 additions & 2 deletions docs/configuration/traceid.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,14 @@ return static function (SymfonyTraceConfig $config): void {
// on by default. If true a value in the `X-Trace-Id` header in the request
// will be used as the trace ID for the rest of the request. If false
// those values are ignored.
$config->trustRequestHeader(true);
$config->request()
->trustHeader(true)
->trustedIps(env('TRUSTED_IPS')); // Only trust the header from these IP's

// Whether to send the trace details in the response headers. This is turned on by default.
$config->sendResponseHeader(true);
$config->response()
->sendHeader(true)
->trustedIps(env('TRUSTED_IPS')); // Only send the header to these IP's

$config->traceid()
// The header which the bundle inspects for the incoming trace ID
Expand Down Expand Up @@ -72,3 +76,4 @@ return static function (SymfonyTraceConfig $config): void {
->enabled(true)
->hubService(HubInterface::class);
};
```
2 changes: 1 addition & 1 deletion phpstan-baseline.neon
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
parameters:
ignoreErrors:
-
message: "#^Parameter \\#1 \\$mergedConfig \\(array\\{traceMode\\: 'tracecontext'\\|'traceid', traceid\\: array\\{request_header\\: string, response_header\\: string, generator_service\\: string\\|null\\}, trust_request_header\\: bool, send_response_header\\: bool, storage_service\\: string\\|null, enable_monolog\\: bool, enable_console\\: bool, console\\: array\\{enabled\\: bool, trace_id\\: string\\|null\\}, \\.\\.\\.\\}\\) of method DR\\\\SymfonyTraceBundle\\\\DependencyInjection\\\\SymfonyTraceExtension\\:\\:loadInternal\\(\\) should be contravariant with parameter \\$mergedConfig \\(array\\) of method Symfony\\\\Component\\\\HttpKernel\\\\DependencyInjection\\\\ConfigurableExtension\\:\\:loadInternal\\(\\)$#"
message: "#^Parameter \\#1 \\$mergedConfig \\(array\\{traceMode\\: 'tracecontext'\\|'traceid', traceid\\: array\\{request_header\\: string, response_header\\: string, generator_service\\: string\\|null\\}, request\\: array\\{trust_header\\: bool, trusted_ips\\: array\\<string\\>\\|string\\|null\\}, response\\: array\\{send_header\\: bool, trusted_ips\\: array\\<string\\>\\|string\\|null\\}, storage_service\\: string\\|null, enable_monolog\\: bool, enable_console\\: bool, console\\: array\\{enabled\\: bool, trace_id\\: string\\|null\\}, \\.\\.\\.\\}\\) of method DR\\\\SymfonyTraceBundle\\\\DependencyInjection\\\\SymfonyTraceExtension\\:\\:loadInternal\\(\\) should be contravariant with parameter \\$mergedConfig \\(array\\) of method Symfony\\\\Component\\\\HttpKernel\\\\DependencyInjection\\\\ConfigurableExtension\\:\\:loadInternal\\(\\)$#"
count: 1
path: src/DependencyInjection/SymfonyTraceExtension.php

Expand Down
54 changes: 46 additions & 8 deletions src/DependencyInjection/Configuration.php
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,8 @@ public function getConfigTreeBuilder(): TreeBuilder
->end()
->end()
->end()
->booleanNode('trust_request_header')
->defaultTrue()
->info("Whether or not to trust the incoming request's `Trace-Id` header as a real ID")
->end()
->booleanNode('send_response_header')
->defaultTrue()
->info("Whether or not to send a response header with the trace ID. Defaults to true")
->end()
->append($this->createRequestConfiguration())
->append($this->createResponseConfiguration())
->scalarNode('storage_service')
->info('The service name for trace ID storage. Defaults to `TraceStorage`')
->end()
Expand Down Expand Up @@ -101,6 +95,50 @@ public function getConfigTreeBuilder(): TreeBuilder
return $tree;
}

private function createRequestConfiguration(): NodeDefinition
{
$node = (new TreeBuilder('request'))->getRootNode();
$node
->addDefaultsIfNotSet()
->children()
->booleanNode('trust_header')
->defaultTrue()
->info("Whether or not to trust the incoming request's headers as a real TraceID")
->end()
->scalarNode('trusted_ips')
->info(
"Only trust incoming request's headers if the request comes from one of these IPs (supports ranges/masks). " .
"Accepts a string-array, comma separated string or null. Defaults to null, accepting all request IPs. "
)
->defaultNull()
->end()
->end();

return $node;
}

private function createResponseConfiguration(): NodeDefinition
{
$node = (new TreeBuilder('response'))->getRootNode();
$node
->addDefaultsIfNotSet()
->children()
->booleanNode('send_header')
->defaultTrue()
->info("Whether or not to send a response header with the trace ID. Defaults to true")
->end()
->scalarNode('trusted_ips')
->info(
"Only send response if the request comes from one of these IPs (supports ranges/masks) " .
"Accepts a string-array, comma separated string or null. Defaults to null, accepting all request IPs. "
)
->defaultNull()
->end()
->end();

return $node;
}

private function createHttpClientConfiguration(): NodeDefinition
{
$node = (new TreeBuilder('http_client'))->getRootNode();
Expand Down
16 changes: 12 additions & 4 deletions src/DependencyInjection/SymfonyTraceExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,14 @@
* response_header: string,
* generator_service: ?string,
* },
* trust_request_header: bool,
* send_response_header: bool,
* request: array{
* trust_header: bool,
* trusted_ips: string[]|string|null,
* },
* response: array{
* send_header: bool,
* trusted_ips: string[]|string|null,
* },
* storage_service: ?string,
* enable_monolog: bool,
* enable_console: bool,
Expand Down Expand Up @@ -84,8 +90,10 @@ protected function loadInternal(array $mergedConfig, ContainerBuilder $container
$container->register(TraceSubscriber::class)
->setArguments(
[
$mergedConfig['trust_request_header'],
$mergedConfig['send_response_header'],
$mergedConfig['request']['trust_header'],
$mergedConfig['request']['trusted_ips'],
$mergedConfig['response']['send_header'],
$mergedConfig['response']['trusted_ips'],
new Reference(TraceServiceInterface::class),
new Reference(TraceStorageInterface::class)
]
Expand Down
37 changes: 31 additions & 6 deletions src/EventSubscriber/TraceSubscriber.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
use DR\SymfonyTraceBundle\Service\TraceServiceInterface;
use DR\SymfonyTraceBundle\TraceStorageInterface;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpFoundation\IpUtils;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Event\RequestEvent;
use Symfony\Component\HttpKernel\Event\ResponseEvent;
use Symfony\Component\HttpKernel\KernelEvents;
Expand All @@ -18,12 +20,16 @@
final class TraceSubscriber implements EventSubscriberInterface
{
/**
* @param bool $trustRequest Trust the value from the request? Or generate?
* @param TraceStorageInterface $traceStorage The trace ID storage, used to store the ID from the request or a newly generated ID.
* @param bool $trustRequest Trust the value from the request? Or generate?
* @param string[]|string|null $trustedIpsRequest The IPs to trust the request from.
* @param string[]|string|null $trustedIpsResponse The IPs to send the response header to.
* @param TraceStorageInterface $traceStorage The trace ID storage, used to store the ID from the request or a newly generated ID.
*/
public function __construct(
private readonly bool $trustRequest,
private readonly array|string|null $trustedIpsRequest,
private readonly bool $sendResponseHeader,
private readonly array|string|null $trustedIpsResponse,
private readonly TraceServiceInterface $traceService,
private readonly TraceStorageInterface $traceStorage,
) {
Expand All @@ -35,7 +41,7 @@ public function __construct(
public static function getSubscribedEvents(): array
{
return [
KernelEvents::REQUEST => ['onRequest', 100],
KernelEvents::REQUEST => ['onRequest', 100],
KernelEvents::RESPONSE => ['onResponse', -99],
];
}
Expand All @@ -46,10 +52,11 @@ public function onRequest(RequestEvent $event): void
return;
}

$request = $event->getRequest();
$request = $event->getRequest();
$trustRequest = $this->trustRequest && $this->isTrustedRequest($request, $this->trustedIpsRequest);

// If we trust the request, check if the traceService supports it and use the request data
if ($this->trustRequest && $this->traceService->supports($request)) {
if ($trustRequest && $this->traceService->supports($request)) {
$this->traceStorage->setTrace($this->traceService->getRequestTrace($request));

return;
Expand All @@ -69,8 +76,26 @@ public function onResponse(ResponseEvent $event): void
return;
}

if ($this->sendResponseHeader) {
$request = $event->getRequest();
$sendHeader = $this->sendResponseHeader && $this->isTrustedRequest($request, $this->trustedIpsResponse);
if ($sendHeader) {
$this->traceService->handleResponse($event->getResponse(), $this->traceStorage->getTrace());
}
}

/**
* @param string[]|string|null $trustedIps
*/
private function isTrustedRequest(Request $request, array|string|null $trustedIps): bool
{
if ($trustedIps === null) {
return true;
}

if (is_string($trustedIps)) {
$trustedIps = array_map('trim', explode(',', $trustedIps));
bram123 marked this conversation as resolved.
Show resolved Hide resolved
}

return IpUtils::checkIp((string)$request->getClientIp(), $trustedIps);
}
}
3 changes: 2 additions & 1 deletion tests/Functional/App/config/tracecontext.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
symfony_trace:
traceMode: 'tracecontext'
trust_request_header: true
request:
trust_header: true
storage_service: request.id.storage
enable_messenger: true
http_client:
Expand Down
3 changes: 2 additions & 1 deletion tests/Functional/App/config/traceid.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ symfony_trace:
traceid:
request_header: Trace-Id
response_header: Trace-Id
trust_request_header: true
request:
trust_header: true
storage_service: request.id.storage
enable_messenger: true
console:
Expand Down
68 changes: 65 additions & 3 deletions tests/Unit/EventSubscriber/TraceSubscriberTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ protected function setUp(): void
{
$this->service = $this->createMock(TraceServiceInterface::class);
$this->storage = $this->createMock(TraceStorageInterface::class);
$this->listener = new TraceSubscriber(true, true, $this->service, $this->storage);
$this->listener = new TraceSubscriber(true, null, true, null, $this->service, $this->storage);

$this->dispatcher = new EventDispatcher();
$this->dispatcher->addSubscriber($this->listener);
Expand Down Expand Up @@ -74,7 +74,7 @@ public function testListenerSetsTheTraceToStorageWhenFoundInRequestHeaders(): vo
public function testListenerIgnoresIncomingRequestHeadersWhenTrustRequestIsFalse(): void
{
$this->dispatcher->removeSubscriber($this->listener);
$this->dispatcher->addSubscriber(new TraceSubscriber(false, true, $this->service, $this->storage));
$this->dispatcher->addSubscriber(new TraceSubscriber(false, null, true, null, $this->service, $this->storage));

$trace = new TraceContext();
$this->service->expects(static::never())->method('supports');
Expand Down Expand Up @@ -138,7 +138,7 @@ public function testRequestSetsIdOnResponse(): void
public function testListenerDoesNothingToResponseWithoutMasterRequestWhenSendResponseHeaderIsFalse(): void
{
$this->dispatcher->removeSubscriber($this->listener);
$this->dispatcher->addSubscriber(new TraceSubscriber(false, false, $this->service, $this->storage));
$this->dispatcher->addSubscriber(new TraceSubscriber(false, null, false, null, $this->service, $this->storage));

$this->storage->expects(static::never())->method('getTrace');
$this->service->expects(static::never())->method('handleResponse');
Expand All @@ -148,4 +148,66 @@ public function testListenerDoesNothingToResponseWithoutMasterRequestWhenSendRes
KernelEvents::RESPONSE
);
}

public function testListenerSetsTheTraceToStorageWhenFoundInTrustedRequestHeaders(): void
{
$listener = new TraceSubscriber(true, '127.0.0.1', true, '127.0.0.1', $this->service, $this->storage);
$dispatcher = new EventDispatcher();
$dispatcher->addSubscriber($listener);

$this->service->expects(static::once())->method('getRequestTrace')->with($this->request);
$this->service->expects(static::once())->method('supports')->with($this->request)->willReturn(true);
$this->storage->expects(static::once())->method('setTrace');

$dispatcher->dispatch(
new RequestEvent($this->kernel, $this->request, HttpKernelInterface::MAIN_REQUEST),
KernelEvents::REQUEST
);
}

public function testListenerIgnoresRequestHeadersOnNonTrustedRequest(): void
{
$listener = new TraceSubscriber(true, '127.0.0.2', true, '127.0.0.2', $this->service, $this->storage);
$dispatcher = new EventDispatcher();
$dispatcher->addSubscriber($listener);

$this->service->expects(static::never())->method('supports');
$this->service->expects(static::never())->method('getRequestTrace');

$dispatcher->dispatch(
new RequestEvent($this->kernel, $this->request, HttpKernelInterface::MAIN_REQUEST),
KernelEvents::REQUEST
);
}

public function testRequestSetsIdOnResponseOnTrustedIp(): void
{
$listener = new TraceSubscriber(true, '127.0.0.1', true, '127.0.0.1', $this->service, $this->storage);
$dispatcher = new EventDispatcher();
$dispatcher->addSubscriber($listener);

$trace = new TraceContext();
$this->storage->expects(static::once())->method('getTrace')->willReturn($trace);
$this->service->expects(static::once())->method('handleResponse')->with($this->response, $trace);

$dispatcher->dispatch(
new ResponseEvent($this->kernel, $this->request, HttpKernelInterface::MAIN_REQUEST, $this->response),
KernelEvents::RESPONSE
);
}

public function testRequestDoesNotSetIdOnResponseOnNonTrustedIp(): void
{
$listener = new TraceSubscriber(true, '127.0.0.2', true, '127.0.0.2', $this->service, $this->storage);
$dispatcher = new EventDispatcher();
$dispatcher->addSubscriber($listener);

$this->storage->expects(static::never())->method('getTrace');
$this->service->expects(static::never())->method('handleResponse');

$dispatcher->dispatch(
new ResponseEvent($this->kernel, $this->request, HttpKernelInterface::MAIN_REQUEST, $this->response),
KernelEvents::RESPONSE
);
}
}