[Instrumentation.Http][Instrumentation.AspNetCore] Fix url.full and url.query attribute values

[vishweshbankwar]

Fixes #
Design discussion issue #

Changes

Fixed tracing instrumentation so that by default any values detected in the query string component of requests are replaced with the text Redacted. For ASP.NET Core instrumentation, fixed url.query attribute. For Http instrumentation, fixed url.full attribute.

Merge requirement checklist

• CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
• Unit tests added/updated
• Appropriate CHANGELOG.md files updated for non-trivial changes
• Changes in public API reviewed (if applicable)

[codecov]

Codecov Report

Attention: Patch coverage is 86.20690% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 20.07%. Comparing base (6250307) to head (e2d1d7e).
Report is 177 commits behind head on main.

❗ Current head e2d1d7e differs from pull request most recent head b6fde65. Consider uploading reports for the commit b6fde65 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #5532       +/-   ##
===========================================
- Coverage   83.38%   20.07%   -63.31%     
===========================================
  Files         297      183      -114     
  Lines       12531     7610     -4921     
===========================================
- Hits        10449     1528     -8921     
- Misses       2082     6082     +4000     

Flag	Coverage Δ
unittests	?
unittests-Instrumentation-Experimental	20.08% <86.20%> (?)
unittests-Instrumentation-Stable	18.35% <86.20%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...spNetCore/AspNetCoreTraceInstrumentationOptions.cs	100.00% <100.00%> (ø)
...tation.AspNetCore/Implementation/HttpInListener.cs	89.93% <100.00%> (+0.34%)	⬆️
...tInstrumentationTracerProviderBuilderExtensions.cs	100.00% <100.00%> (ø)
...tp/Implementation/HttpHandlerDiagnosticListener.cs	70.63% <100.00%> (-0.24%)	⬇️
...strumentation.Http/Implementation/HttpTagHelper.cs	75.00% <100.00%> (+2.27%)	⬆️
...tion.Http/HttpClientTraceInstrumentationOptions.cs	78.12% <80.00%> (-21.88%)	⬇️
...p/Implementation/HttpInstrumentationEventSource.cs	58.82% <50.00%> (-11.18%)	⬇️

... and 252 files with indirect coverage changes

[CodeBlanch]

LGTM

[julealgon]

Can someone elaborate where this requirement came from? Is it part of the OTEL Spec?

[cijothomas]

The changelog is not very clear. Could we make it clear the why here as well?
Also, using the word "tag" may not be obvious that it is referring to "attributes".

[CodeBlanch]

Apologies for not providing much detail on here. We wanted to have the mitigation in place before we provided a whole lot of information about a vulnerability. See: GHSA-vh2m-22xx-q94f

[julealgon]

Thanks for sharing that @CodeBlanch , but that feels overly defensive to me. Does the OTEL spec ask implementations to perform this redaction, or is the .NET team doing it on their own?

Using query string for sensitive information such as tokens is such a well-known bad practice for this precise reason, so I don't quite understand why the library would decide to redact all query string values by default.

What if people just share sensitive information as URL segments, then? Is the library going to start redacting route segments in the URL too?

This will just make debugging harder by default. It would made a lot more sense if the framework allowed redaction to be plugged-in instead and let consumers decide whether they need it or not, which is exactly what happens today with standard logs as well.

If there is a known practice to redact query string values everywhere I'm not aware about, please share that so I can learn from it.

On an unrelated note, it also makes me a bid sad this is not tapping into Microsoft.Extensions.Compliance.Redaction. If not even Microsoft dogfeeds on its own base libraries, it becomes even harder to get those libraries to be adopted by the community and get feedback etc.

[SaifAqqad]

Redacting all query parameters by default makes zero sense to me.

query parameters should not be used for sensitive info regardless of whether they're going to be redacted in logs and traces.

[reyang]

Related to open-telemetry/semantic-conventions#961.

[julealgon]

Related to open-telemetry/semantic-conventions#961.

Sad to see this being forced into the convention as well.

Oh well... at least now we are in a better position to disable the behavior across all of our services in a single place. That wasn't the case a couple weeks ago.

[CEbbinghaus]

Can I just add my 2 cents, that on the whole this approach of making OpenTelemetry responsible for everybody that does something insecure is not a particularly sustainable or sensical approach.

I don't request Microsoft fix the Console.WriteLine method just because I logged the password to console or fix File.WriteAllText because I wrote a credential to file. So why make a logging framework responsible for what is Logged.

It makes perfect sense to have sane defaults that do the best to keep a developer from writing insecure code but at the end of the day that is all we can do. This change goes a step further though since it not only enforces a default but doesn't even let the developer disable it. The only way to disable this functionality is through an environment variable OTEL_DOTNET_EXPERIMENTAL_ASPNETCORE_DISABLE_URL_QUERY_REDACTION. I am perhaps an outlier, but for me expecting an environment variable to be set is not an option, since my application runs on user machines with no option to modify system environment variables. And since setting it in app through Environment.SetEnvironmentVariable doesn't work there is no option but to set all of the OpenTelemetry versions to 1.8.0 just to avoid having every single query parameter (which by the way contain some critical information regarding the purpose of the query) being censored.

I really hope that the OpenTelemetry team and in particular those involved with open-telemetry/semantic-conventions#961 reconsider this approach and Favour a more robust and flexible redaction system rather than these rushed and half thought out defaults that worsen the 99% for those 1% cases of bad code.

[reyang]

... setting it in app through Environment.SetEnvironmentVariable doesn't work there is no option but to set all of the OpenTelemetry versions to 1.8.0 ...

@CEbbinghaus Could you file an issue with a repro?