Skip to content

Commit

Permalink
Tracing: redact query params in urls (#39925)
Browse files Browse the repository at this point in the history 
* Tracing: redact query params in url
  • Loading branch information
@lmolkova
lmolkova committed Apr 30, 2024
1 parent 61e0271 commit af6d7e7
Show file tree
Hide file tree
Showing 11 changed files with 339 additions and 170 deletions.
2 changes: 2 additions & 0 deletions sdk/core/azure-core-tracing-opentelemetry/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

### Breaking Changes

- Added default query params sanitization for HTTP spans.

### Bugs Fixed

### Other Changes
Expand Down
105 changes: 80 additions & 25 deletions ...etry/src/test/java/com/azure/core/tracing/opentelemetry/OpenTelemetryHttpPolicyTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@
import com.azure.core.http.policy.RetryPolicy;
import com.azure.core.test.http.MockHttpResponse;
import com.azure.core.util.BinaryData;
import com.azure.core.util.ClientOptions;
import com.azure.core.util.Context;
import com.azure.core.util.TracingOptions;
import io.opentelemetry.api.OpenTelemetry;
import io.opentelemetry.api.common.AttributeKey;
import io.opentelemetry.api.common.Attributes;
Expand Down Expand Up @@ -54,9 +56,12 @@
import java.time.Duration;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.TimeoutException;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
Expand All @@ -79,11 +84,16 @@ public class OpenTelemetryHttpPolicyTests {
private static final String X_MS_REQUEST_ID_1 = "response id 1";
private static final String X_MS_REQUEST_ID_2 = "response id 2";
private static final int RESPONSE_STATUS_CODE = 201;
private static final String ORIGINAL_URL_WITH_QUERY = "https://httpbin.org/hello?n=otel&api-version=1.2.3";
private static final String EXPECTED_URL_REDACTED = "https://httpbin.org/hello?n=REDACTED&api-version=1.2.3";
private static final String ORIGINAL_URL_NO_QUERY = "https://httpbin.org/hello";
private static final ClientOptions DEFAULT_CLIENT_OPTIONS = new ClientOptions();
private InMemorySpanExporter exporter;
private SdkTracerProvider tracerProvider;
private OpenTelemetry openTelemetry;
private Tracer tracer;
private com.azure.core.util.tracing.Tracer azTracer;

private static final String SPAN_NAME = "foo";

@BeforeEach
Expand All @@ -108,11 +118,13 @@ public void openTelemetryHttpPolicyTest() {
.addData("az.namespace", "foo");

// Act
HttpRequest request = new HttpRequest(HttpMethod.POST, "https://httpbin.org/hello?there#otel");
HttpRequest request = new HttpRequest(HttpMethod.POST, ORIGINAL_URL_WITH_QUERY);
request.setHeader(HttpHeaderName.USER_AGENT, "user-agent");

try (Scope scope = parentSpan.makeCurrent()) {
createHttpPipeline(azTracer).send(request, tracingContext).block().getBodyAsBinaryData();
createHttpPipeline(DEFAULT_CLIENT_OPTIONS, azTracer).send(request, tracingContext)
.block()
.getBodyAsBinaryData();
}
// Assert
List<SpanData> exportedSpans = exporter.getFinishedSpanItems();
Expand All @@ -129,7 +141,7 @@ public void openTelemetryHttpPolicyTest() {
Map<String, Object> httpAttributes = getAttributes(httpSpan);

assertEquals(7, httpAttributes.size());
assertEquals("https://httpbin.org/hello?there#otel", httpAttributes.get("url.full"));
assertEquals(EXPECTED_URL_REDACTED, httpAttributes.get("url.full"));
assertEquals("httpbin.org", httpAttributes.get("server.address"));
assertEquals(443L, httpAttributes.get("server.port"));
assertEquals("POST", httpAttributes.get("http.request.method"));
Expand All @@ -138,6 +150,30 @@ public void openTelemetryHttpPolicyTest() {
assertEquals(X_MS_REQUEST_ID_1, httpAttributes.get("az.service_request_id"));
}

@ParameterizedTest
@MethodSource("urlSanitizationArgs")
public void urlSanitizationTests(String originalUrl, Set<String> allowedQueryParams, String expectedUrl) {
// Act
HttpRequest request = new HttpRequest(HttpMethod.POST, originalUrl);

ClientOptions options = new ClientOptions()
.setTracingOptions(new TracingOptions().setAllowedTracingQueryParamNames(allowedQueryParams));

Span parentSpan = tracer.spanBuilder(SPAN_NAME).startSpan();
try (Scope scope = parentSpan.makeCurrent()) {
createHttpPipeline(options, azTracer).send(request, Context.NONE).block().getBodyAsBinaryData();
}

// Assert
List<SpanData> exportedSpans = exporter.getFinishedSpanItems();
// rest proxy span is not exported as global otel is not configured
assertEquals(1, exportedSpans.size());
SpanData httpSpan = exportedSpans.get(0);

assertEquals("POST", httpSpan.getName());
assertEquals(expectedUrl, getAttributes(httpSpan).get("url.full"));
}

@Test
public void presamplingAttributesArePopulatedBeforeSpanStarts() {
AtomicBoolean samplerCalled = new AtomicBoolean();
Expand All @@ -149,8 +185,7 @@ public SamplingResult shouldSample(io.opentelemetry.context.Context parentContex
assertEquals(4, attributes.size());
assertEquals("DELETE", name);
assertEquals("DELETE", attributes.get(AttributeKey.stringKey("http.request.method")));
assertEquals("https://httpbin.org/hello?there#otel",
attributes.get(AttributeKey.stringKey("url.full")));
assertEquals(EXPECTED_URL_REDACTED, attributes.get(AttributeKey.stringKey("url.full")));
assertEquals("httpbin.org", attributes.get(AttributeKey.stringKey("server.address")));
assertEquals(443, attributes.get(AttributeKey.longKey("server.port")));
return SamplingResult.create(SamplingDecision.DROP);
Expand All @@ -163,13 +198,14 @@ public String getDescription() {
}).addSpanProcessor(SimpleSpanProcessor.create(exporter)).build();

// Act
HttpRequest request = new HttpRequest(HttpMethod.DELETE, "https://httpbin.org/hello?there#otel");
HttpRequest request = new HttpRequest(HttpMethod.DELETE, ORIGINAL_URL_WITH_QUERY);
try (Scope scope = tracer.spanBuilder("test").startSpan().makeCurrent()) {
createHttpPipeline(new OpenTelemetryTracer("test", null, null,
new OpenTelemetryTracingOptions()
.setOpenTelemetry(OpenTelemetrySdk.builder().setTracerProvider(providerWithSampler).build())))
.send(request)
.block();
createHttpPipeline(DEFAULT_CLIENT_OPTIONS,
new OpenTelemetryTracer("test", null, null,
new OpenTelemetryTracingOptions()
.setOpenTelemetry(OpenTelemetrySdk.builder().setTracerProvider(providerWithSampler).build())))
.send(request)
.block();
}
// Assert
List<SpanData> exportedSpans = exporter.getFinishedSpanItems();
Expand All @@ -180,10 +216,11 @@ public String getDescription() {
@Test
public void clientRequestIdIsStamped() {
try (Scope scope = tracer.spanBuilder("test").startSpan().makeCurrent()) {
HttpRequest request = new HttpRequest(HttpMethod.PUT, "https://httpbin.org/hello?there#otel");
HttpResponse response = createHttpPipeline(azTracer, new RequestIdPolicy()).send(request)
.flatMap(r -> r.getBodyAsByteArray().thenReturn(r))
.block();
HttpRequest request = new HttpRequest(HttpMethod.PUT, ORIGINAL_URL_WITH_QUERY);
HttpResponse response
= createHttpPipeline(DEFAULT_CLIENT_OPTIONS, azTracer, new RequestIdPolicy()).send(request)
.flatMap(r -> r.getBodyAsByteArray().thenReturn(r))
.block();

// Assert
List<SpanData> exportedSpans = exporter.getFinishedSpanItems();
Expand All @@ -198,7 +235,7 @@ public void clientRequestIdIsStamped() {
httpAttributes.get("az.client_request_id"));
assertEquals(X_MS_REQUEST_ID_1, httpAttributes.get("az.service_request_id"));

assertEquals("https://httpbin.org/hello?there#otel", httpAttributes.get("url.full"));
assertEquals(EXPECTED_URL_REDACTED, httpAttributes.get("url.full"));
assertEquals("httpbin.org", httpAttributes.get("server.address"));
assertEquals(443L, httpAttributes.get("server.port"));
assertEquals("PUT", httpAttributes.get("http.request.method"));
Expand Down Expand Up @@ -245,7 +282,7 @@ public void everyTryIsTraced() {
= new Context(PARENT_TRACE_CONTEXT_KEY, io.opentelemetry.context.Context.root().with(parentSpan))
.addData("az.namespace", "foo");

StepVerifier.create(pipeline.send(new HttpRequest(HttpMethod.GET, "http://localhost/hello"), tracingContext))
StepVerifier.create(pipeline.send(new HttpRequest(HttpMethod.GET, ORIGINAL_URL_NO_QUERY), tracingContext))
.assertNext(response -> {
assertEquals(200, response.getStatusCode());
response.close();
Expand Down Expand Up @@ -292,7 +329,7 @@ public void endStatusDependingOnStatusCode(int statusCode, StatusCode status) {
.tracer(azTracer)
.build();

StepVerifier.create(pipeline.send(new HttpRequest(HttpMethod.GET, "http://localhost/hello")))
StepVerifier.create(pipeline.send(new HttpRequest(HttpMethod.GET, ORIGINAL_URL_NO_QUERY)))
.assertNext(response -> {
response.getBodyAsByteArray().block();
assertEquals(statusCode, response.getStatusCode());
Expand Down Expand Up @@ -322,7 +359,7 @@ public void exceptionEventIsNotRecorded() {
.tracer(azTracer)
.build();

StepVerifier.create(pipeline.send(new HttpRequest(HttpMethod.GET, "http://localhost/hello")))
StepVerifier.create(pipeline.send(new HttpRequest(HttpMethod.GET, ORIGINAL_URL_NO_QUERY)))
.expectErrorMessage("foo")
.verify();

Expand Down Expand Up @@ -367,7 +404,7 @@ public void timeoutIsTraced() {
= new Context(PARENT_TRACE_CONTEXT_KEY, io.opentelemetry.context.Context.root().with(parentSpan))
.addData("az.namespace", "foo");

StepVerifier.create(pipeline.send(new HttpRequest(HttpMethod.GET, "http://localhost/hello"), tracingContext)
StepVerifier.create(pipeline.send(new HttpRequest(HttpMethod.GET, ORIGINAL_URL_NO_QUERY), tracingContext)
.flatMap(r -> r.getBody().collectList().thenReturn(r))).assertNext(response -> {
assertEquals(200, response.getStatusCode());
}).verifyComplete();
Expand Down Expand Up @@ -397,7 +434,7 @@ public void connectionErrorAfterResponseCodeIsTraced() {
.tracer(azTracer)
.build();

StepVerifier.create(pipeline.send(new HttpRequest(HttpMethod.GET, "http://localhost/hello"), Context.NONE)
StepVerifier.create(pipeline.send(new HttpRequest(HttpMethod.GET, ORIGINAL_URL_NO_QUERY), Context.NONE)
.flatMap(response -> response.getBodyAsInputStream())).expectError(IOException.class).verify();

List<SpanData> exportedSpans = exporter.getFinishedSpanItems();
Expand Down Expand Up @@ -426,7 +463,7 @@ public void cancelIsTraced() {
.tracer(azTracer)
.build();

pipeline.send(new HttpRequest(HttpMethod.GET, "http://localhost/hello"), Context.NONE).toFuture().cancel(true);
pipeline.send(new HttpRequest(HttpMethod.GET, ORIGINAL_URL_NO_QUERY), Context.NONE).toFuture().cancel(true);

List<SpanData> exportedSpans = exporter.getFinishedSpanItems();
assertEquals(1, exportedSpans.size());
Expand All @@ -445,12 +482,13 @@ private Map<String, Object> getAttributes(SpanData span) {
return attributes;
}

private static HttpPipeline createHttpPipeline(com.azure.core.util.tracing.Tracer azTracer,
HttpPipelinePolicy... beforeRetryPolicies) {
private static HttpPipeline createHttpPipeline(ClientOptions clientOptions,
com.azure.core.util.tracing.Tracer azTracer, HttpPipelinePolicy... beforeRetryPolicies) {
List<HttpPipelinePolicy> policies = new ArrayList<>(Arrays.asList(beforeRetryPolicies));
HttpPolicyProviders.addAfterRetryPolicies(policies);

return new HttpPipelineBuilder().policies(policies.toArray(new HttpPipelinePolicy[0]))
return new HttpPipelineBuilder().clientOptions(clientOptions)
.policies(policies.toArray(new HttpPipelinePolicy[0]))
.httpClient(new SimpleMockHttpClient())
.tracer(azTracer)
.build();
Expand All @@ -464,6 +502,23 @@ public static Stream<Arguments> getStatusCodes() {
Arguments.of(503, StatusCode.ERROR));
}

public static Stream<Arguments> urlSanitizationArgs() {
List<Arguments> arguments = new ArrayList<>();

arguments.add(Arguments.of(ORIGINAL_URL_WITH_QUERY, Collections.emptySet(), EXPECTED_URL_REDACTED));
arguments.add(Arguments.of(ORIGINAL_URL_NO_QUERY, Collections.emptySet(), ORIGINAL_URL_NO_QUERY));
arguments.add(Arguments.of(ORIGINAL_URL_WITH_QUERY, Collections.singleton("n"),
"https://httpbin.org/hello?n=otel&api-version=1.2.3"));

Set<String> allowed = new HashSet<>();
allowed.add("n");
allowed.add("m");
arguments
.add(Arguments.of(ORIGINAL_URL_WITH_QUERY, allowed, "https://httpbin.org/hello?n=otel&api-version=1.2.3"));

return arguments.stream();
}

private static class SimpleMockHttpClient implements HttpClient {

@Override
Expand Down
8 changes: 8 additions & 0 deletions sdk/core/azure-core/spotbugs-exclude.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,10 @@
<Method name="getAllowedQueryParamNames" />
</Or>
</And>
<And>
<Class name="com.azure.core.util.TracingOptions" />
<Method name="getAllowedTracingQueryParamNames" />
</And>
<And>
<Class name="com.azure.core.http.policy.HttpRequestLoggingContext" />
<Method name="getHttpRequest" />
Expand Down Expand Up @@ -421,6 +425,10 @@
<Class name="com.azure.core.models.MessageContent" />
<Method name="setBodyAsBinaryData" />
</And>
<And>
<Class name="com.azure.core.util.TracingOptions" />
<Method name="setAllowedTracingQueryParamNames" />
</And>
</Or>
</Match>

Expand Down
18 changes: 14 additions & 4 deletions sdk/core/azure-core/src/main/java/com/azure/core/http/HttpPipelineBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import com.azure.core.http.policy.HttpPipelinePolicy;
import com.azure.core.implementation.http.policy.InstrumentationPolicy;
import com.azure.core.implementation.http.UrlSanitizer;
import com.azure.core.util.ClientOptions;
import com.azure.core.util.HttpClientOptions;
import com.azure.core.util.TracingOptions;
Expand All @@ -14,6 +15,7 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Set;

/**
* This class provides a fluent builder API to help aid the configuration and instantiation of the {@link HttpPipeline},
Expand Down Expand Up @@ -86,24 +88,32 @@ public HttpPipeline build() {
client = HttpClient.createDefault();
}

configureTracing(policies);
configureTracing(policies, clientOptions);

return new HttpPipeline(client, policies, tracer);
}

private void configureTracing(List<HttpPipelinePolicy> policies) {
private void configureTracing(List<HttpPipelinePolicy> policies, ClientOptions clientOptions) {
if (tracer == null) {
TracingOptions tracingOptions = clientOptions == null ? null : clientOptions.getTracingOptions();
tracer = TracerProvider.getDefaultProvider().createTracer("azure-core", null, null, tracingOptions);
}

for (HttpPipelinePolicy policy : policies) {
if (policy instanceof InstrumentationPolicy) {
((InstrumentationPolicy) policy).initialize(tracer);
UrlSanitizer sanitizer = new UrlSanitizer(getAllowedQueryParams(clientOptions));
((InstrumentationPolicy) policy).initialize(tracer, sanitizer);
}
}
}

private static Set<String> getAllowedQueryParams(ClientOptions options) {
if (options == null || options.getTracingOptions() == null) {
return null;
}

return options.getTracingOptions().getAllowedTracingQueryParamNames();
}

/**
* Sets the HttpClient that the pipeline will use to send requests.
*
Expand Down

0 comments on commit af6d7e7

Please sign in to comment.