Redact query string values

[jeanbisutti]

See https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-spans.md

[jeanbisutti]

This test was (probably inadvertly) removed in #9925.

The test is added again with new test cases.

[laurit]

This does not align with our usual way of handling configuration options. It only supports system properties while we usually also accept environment variables. Perhaps it would be better to handle it the same way as other http client experimental options in

opentelemetry-java-instrumentation/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/builder/internal/DefaultHttpClientInstrumenterBuilder.java

Line 243 in ed0fb2e

public DefaultHttpClientInstrumenterBuilder<REQUEST, RESPONSE> configure(CommonConfig config) {

so that it could also be used from spring boot configuration. Since the HttpClientAttributesExtractor and builder are stable introducing experimental options will require trickery with internal classes.

[laurit]

Suggested change

	int questionMarkIndex = urlpart.indexOf('?');
	int questionMarkIndex = urlpart.indexOf('?');

[laurit]

have you considered structuring it like

String stripSensitiveData(@Nullable String url) {
  if (url == null || url.isEmpty()) {
    return url;
  }
  url = redactUserInfo(url);
  url = redactQueryParameters(url);
  return url;
}

then you will not need to duplicate redactSensitiveParameters ? redactUrlParameters(url) : url;

[laurit]

did you consider that parameter names and values in url may be percent encoded?

[jeanbisutti]

The parsing do processing based on the presence of the ?, =, & and # characters. All these characters are reserved ones after percent-encoding: https://en.wikipedia.org/wiki/Percent-encoding

[laurit]

You have misunderstood, the problem isn't in these characters but rather in the names of the query parameters. For example sig could be encoded as %73%69%67. I'm not claiming that such encoding would be common or sensible, just that it is acceptable to the the web server.

[trask]

interesting, I suspect it's ok to ignore a query param encoded as %73%69%67, the goal of this masking isn't to account for malicious usage, but for valid usage of specific client libraries

@jeanbisutti can you open a semconv issue asking for this clarification?

[laurit]

I also think it is fine to not bother with this since what is used by the http client is set by the application author. If needed we can always make the redacted parameter list configurable.

[jeanbisutti]

open-telemetry/semantic-conventions#1852

[trask]

thanks @jeanbisutti, I think we can proceed with the behavior in this PR (no percent encoding, exact case matching) and revisit after getting any semconv clarification