[Instrumentation.AspNet][Instrumentation.Owin] Redact query parameters (#1656)

vishweshbankwar · web-flow · commit 113e5b82b06b · 2024-04-17T11:52:24.000-07:00
diff --git a/opentelemetry-dotnet-contrib.sln b/opentelemetry-dotnet-contrib.sln
@@ -280,8 +280,9 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Shared", "Shared", "{1FCC8E
 		src\Shared\ListenerHandler.cs = src\Shared\ListenerHandler.cs
 		src\Shared\MultiTypePropertyFetcher.cs = src\Shared\MultiTypePropertyFetcher.cs
 		src\Shared\NullableAttributes.cs = src\Shared\NullableAttributes.cs
-		src\Shared\PropertyFetcher.cs = src\Shared\PropertyFetcher.cs
 		src\Shared\PropertyFetcher.AOT.cs = src\Shared\PropertyFetcher.AOT.cs
+		src\Shared\PropertyFetcher.cs = src\Shared\PropertyFetcher.cs
+		src\Shared\RedactionHelper.cs = src\Shared\RedactionHelper.cs
 		src\Shared\ResourceSemanticConventions.cs = src\Shared\ResourceSemanticConventions.cs
 		src\Shared\SemanticConventions.cs = src\Shared\SemanticConventions.cs
 		src\Shared\SpanAttributeConstants.cs = src\Shared\SpanAttributeConstants.cs
diff --git a/src/OpenTelemetry.Instrumentation.AspNet/AspNetTraceInstrumentationOptions.cs b/src/OpenTelemetry.Instrumentation.AspNet/AspNetTraceInstrumentationOptions.cs
@@ -4,6 +4,7 @@
 using System;
 using System.Diagnostics;
 using System.Web;
+using OpenTelemetry.Instrumentation.AspNet.Implementation;
 
 namespace OpenTelemetry.Instrumentation.AspNet;
 
@@ -12,6 +13,27 @@ namespace OpenTelemetry.Instrumentation.AspNet;
 /// </summary>
 public class AspNetTraceInstrumentationOptions
 {
+    private const string DisableQueryRedactionEnvVar = "OTEL_DOTNET_EXPERIMENTAL_ASPNET_DISABLE_URL_QUERY_REDACTION";
+
+    /// <summary>
+    /// Initializes a new instance of the <see cref="AspNetTraceInstrumentationOptions"/> class.
+    /// </summary>
+    public AspNetTraceInstrumentationOptions()
+    {
+        try
+        {
+            var disableQueryRedaction = Environment.GetEnvironmentVariable(DisableQueryRedactionEnvVar);
+            if (disableQueryRedaction != null && disableQueryRedaction.Equals("true", StringComparison.OrdinalIgnoreCase))
+            {
+                this.DisableUrlQueryRedaction = true;
+            }
+        }
+        catch (Exception ex)
+        {
+            AspNetInstrumentationEventSource.Log.FailedToReadEnvironmentVariable(DisableQueryRedactionEnvVar, ex);
+        }
+    }
+
     /// <summary>
     /// Gets or sets a filter callback function that determines on a per
     /// request basis whether or not to collect telemetry.
@@ -46,4 +68,14 @@ public class AspNetTraceInstrumentationOptions
     /// See: <see href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/exceptions.md"/>.
     /// </remarks>
     public bool RecordException { get; set; }
+
+    /// <summary>
+    /// Gets or sets a value indicating whether the url query value should be redacted or not.
+    /// </summary>
+    /// <remarks>
+    /// The query parameter values are redacted with value set as Redacted.
+    /// e.g. `?key1=value1` is set as `?key1=Redacted`.
+    /// The redaction can be disabled by setting this property to <see langword="true" />.
+    /// </remarks>
+    internal bool DisableUrlQueryRedaction { get; set; }
 }
diff --git a/src/OpenTelemetry.Instrumentation.AspNet/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNet/CHANGELOG.md
@@ -1,6 +1,16 @@
 # Changelog
 
-## Unreleased
+## 1.8.0-beta.2
+
+Released 2024-Apr-17
+
+* **Breaking Change**: Fixed tracing instrumentation so that by default any
+  values detected in the query string component of requests are replaced with
+  the text `Redacted` when building the `url.query` attribute. For example,
+  `?key1=value1&key2=value2` becomes `?key1=Redacted&key2=Redacted`. You can
+  disable this redaction by setting the environment variable
+  `OTEL_DOTNET_EXPERIMENTAL_ASPNET_DISABLE_URL_QUERY_REDACTION` to `true`.
+  ([#1656](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1656))
 
 ## 1.8.0-beta.1
 
diff --git a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/AspNetInstrumentationEventSource.cs b/src/OpenTelemetry.Instrumentation.AspNet/Implementation/AspNetInstrumentationEventSource.cs
@@ -33,6 +33,15 @@ public void EnrichmentException(string eventName, Exception ex)
         }
     }
 
+    [NonEvent]
+    public void FailedToReadEnvironmentVariable(string envVarName, Exception ex)
+    {
+        if (this.IsEnabled(EventLevel.Error, EventKeywords.All))
+        {
+            this.EnrichmentException(envVarName, ex.ToInvariantString());
+        }
+    }
+
     [Event(1, Message = "Request is filtered out and will not be collected. Operation='{0}'", Level = EventLevel.Verbose)]
     public void RequestIsFilteredOut(string operationName)
     {
@@ -50,4 +59,10 @@ public void EnrichmentException(string eventName, string exception)
     {
         this.WriteEvent(3, eventName, exception);
     }
+
+    [Event(4, Message = "Failed to read environment variable='{0}': {1}", Level = EventLevel.Error)]
+    public void FailedToReadEnvironmentVariable(string envVarName, string exception)
+    {
+        this.WriteEvent(4, envVarName, exception);
+    }
 }
diff --git a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs
@@ -91,14 +91,8 @@ private void OnStartActivity(Activity activity, HttpContext context)
             var query = url.Query;
             if (!string.IsNullOrEmpty(query))
             {
-                if (query.StartsWith("?", StringComparison.InvariantCulture))
-                {
-                    activity.SetTag(SemanticConventions.AttributeUrlQuery, query.Substring(1));
-                }
-                else
-                {
-                    activity.SetTag(SemanticConventions.AttributeUrlQuery, query);
-                }
+                var queryString = query.StartsWith("?", StringComparison.InvariantCulture) ? query.Substring(1) : query;
+                activity.SetTag(SemanticConventions.AttributeUrlQuery, this.options.DisableUrlQueryRedaction ? queryString : RedactionHelper.GetRedactedQueryString(queryString));
             }
 
             var userAgent = request.UserAgent;
diff --git a/src/OpenTelemetry.Instrumentation.AspNet/OpenTelemetry.Instrumentation.AspNet.csproj b/src/OpenTelemetry.Instrumentation.AspNet/OpenTelemetry.Instrumentation.AspNet.csproj
@@ -17,6 +17,7 @@
     <Compile Include="$(RepoRoot)\src\Shared\ExceptionExtensions.cs" Link="Includes\ExceptionExtensions.cs" />
     <Compile Include="$(RepoRoot)\src\Shared\Guard.cs" Link="Includes\Guard.cs" />
     <Compile Include="$(RepoRoot)\src\Shared\PropertyFetcher.AOT.cs" Link="Includes\PropertyFetcher.AOT.cs" />
+    <Compile Include="$(RepoRoot)\src\Shared\RedactionHelper.cs" Link="Includes\RedactionHelper.cs" />
     <Compile Include="$(RepoRoot)\src\Shared\SemanticConventions.cs" Link="Includes\SemanticConventions.cs" />
     <Compile Include="$(RepoRoot)\src\Shared\SpanHelper.cs" Link="Includes\SpanHelper.cs" />
   </ItemGroup>
diff --git a/src/OpenTelemetry.Instrumentation.Owin/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Owin/CHANGELOG.md
@@ -1,6 +1,16 @@
 # Changelog
 
-## Unreleased
+## 1.0.0-rc.5
+
+Released 2024-Apr-17
+
+* **Breaking Change**: Fixed tracing instrumentation so that by default any
+  values detected in the query string component of requests are replaced with
+  the text `Redacted` when building the `http.url` tag. For example,
+  `?key1=value1&key2=value2` becomes `?key1=Redacted&key2=Redacted`. You can
+  disable this redaction by setting the environment variable
+  `OTEL_DOTNET_EXPERIMENTAL_OWIN_DISABLE_URL_QUERY_REDACTION` to `true`.
+  ([#1656](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1656))
 
 * `ActivitySource.Version` and `Meter.Version` are set to NuGet package version.
   ([#1624](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1624))
diff --git a/src/OpenTelemetry.Instrumentation.Owin/Implementation/DiagnosticsMiddleware.cs b/src/OpenTelemetry.Instrumentation.Owin/Implementation/DiagnosticsMiddleware.cs
@@ -9,6 +9,7 @@
 using Microsoft.Owin;
 using OpenTelemetry.Context.Propagation;
 using OpenTelemetry.Instrumentation.Owin.Implementation;
+using OpenTelemetry.Internal;
 using OpenTelemetry.Trace;
 
 namespace OpenTelemetry.Instrumentation.Owin;
@@ -114,7 +115,7 @@ private static void BeginRequest(IOwinContext owinContext)
 
                 activity.SetTag(SemanticConventions.AttributeHttpMethod, request.Method);
                 activity.SetTag(SemanticConventions.AttributeHttpTarget, request.Uri.AbsolutePath);
-                activity.SetTag(SemanticConventions.AttributeHttpUrl, GetUriTagValueFromRequestUri(request.Uri));
+                activity.SetTag(SemanticConventions.AttributeHttpUrl, GetUriTagValueFromRequestUri(request.Uri, OwinInstrumentationActivitySource.Options.DisableUrlQueryRedaction));
 
                 if (request.Headers.TryGetValue("User-Agent", out string[] userAgent) && userAgent.Length > 0)
                 {
@@ -228,13 +229,15 @@ private static void RequestEnd(IOwinContext owinContext, Exception? exception, l
     /// </summary>
     /// <param name="uri"><see cref="Uri"/>.</param>
     /// <returns>Span uri value.</returns>
-    private static string GetUriTagValueFromRequestUri(Uri uri)
+    private static string GetUriTagValueFromRequestUri(Uri uri, bool disableQueryRedaction)
     {
-        if (string.IsNullOrEmpty(uri.UserInfo))
+        if (string.IsNullOrEmpty(uri.UserInfo) && disableQueryRedaction)
         {
-            return uri.ToString();
+            return uri.OriginalString;
         }
 
-        return string.Concat(uri.Scheme, Uri.SchemeDelimiter, uri.Authority, uri.PathAndQuery, uri.Fragment);
+        var query = disableQueryRedaction ? uri.Query : RedactionHelper.GetRedactedQueryString(uri.Query);
+
+        return string.Concat(uri.Scheme, Uri.SchemeDelimiter, uri.Authority, uri.AbsolutePath, query, uri.Fragment);
     }
 }
diff --git a/src/OpenTelemetry.Instrumentation.Owin/Implementation/OwinInstrumentationEventSource.cs b/src/OpenTelemetry.Instrumentation.Owin/Implementation/OwinInstrumentationEventSource.cs
@@ -45,16 +45,32 @@ public void EnrichmentException(Exception exception)
         }
     }
 
+    [NonEvent]
+    public void FailedToReadEnvironmentVariable(string envVarName, Exception ex)
+    {
+        if (this.IsEnabled(EventLevel.Error, EventKeywords.All))
+        {
+            this.FailedToReadEnvironmentVariable(envVarName, ex.ToInvariantString());
+        }
+    }
+
     [Event(EventIds.EnrichmentException, Message = "Enrichment threw exception. Exception {0}.", Level = EventLevel.Error)]
     public void EnrichmentException(string exception)
     {
         this.WriteEvent(EventIds.EnrichmentException, exception);
     }
 
+    [Event(EventIds.FailedToReadEnvironmentVariable, Message = "Failed to read environment variable='{0}': {1}", Level = EventLevel.Error)]
+    public void FailedToReadEnvironmentVariable(string envVarName, string exception)
+    {
+        this.WriteEvent(4, envVarName, exception);
+    }
+
     private class EventIds
     {
         public const int RequestIsFilteredOut = 1;
         public const int RequestFilterException = 2;
         public const int EnrichmentException = 3;
+        public const int FailedToReadEnvironmentVariable = 4;
     }
 }
diff --git a/src/OpenTelemetry.Instrumentation.Owin/OpenTelemetry.Instrumentation.Owin.csproj b/src/OpenTelemetry.Instrumentation.Owin/OpenTelemetry.Instrumentation.Owin.csproj
@@ -10,6 +10,7 @@
     <Compile Include="$(RepoRoot)\src\Shared\AssemblyVersionExtensions.cs" Link="Includes\AssemblyVersionExtensions.cs" />
     <Compile Include="$(RepoRoot)\src\Shared\ExceptionExtensions.cs" Link="Includes\ExceptionExtensions.cs" />
     <Compile Include="$(RepoRoot)\src\Shared\Guard.cs" Link="Includes\Guard.cs" />
+    <Compile Include="$(RepoRoot)\src\Shared\RedactionHelper.cs" Link="Includes\RedactionHelper.cs" />
     <Compile Include="$(RepoRoot)\src\Shared\SemanticConventions.cs" Link="Includes\SemanticConventions.cs"/>
     <Compile Include="$(RepoRoot)\src\Shared\SpanHelper.cs" Link="Includes\SpanHelper.cs" />
   </ItemGroup>
diff --git a/src/OpenTelemetry.Instrumentation.Owin/OwinInstrumentationOptions.cs b/src/OpenTelemetry.Instrumentation.Owin/OwinInstrumentationOptions.cs
@@ -12,6 +12,27 @@ namespace OpenTelemetry.Instrumentation.Owin;
 /// </summary>
 public class OwinInstrumentationOptions
 {
+    private const string DisableQueryRedactionEnvVar = "OTEL_DOTNET_EXPERIMENTAL_OWIN_DISABLE_URL_QUERY_REDACTION";
+
+    /// <summary>
+    /// Initializes a new instance of the <see cref="OwinInstrumentationOptions"/> class.
+    /// </summary>
+    public OwinInstrumentationOptions()
+    {
+        try
+        {
+            var disableQueryRedaction = Environment.GetEnvironmentVariable(DisableQueryRedactionEnvVar);
+            if (disableQueryRedaction != null && disableQueryRedaction.Equals("true", StringComparison.OrdinalIgnoreCase))
+            {
+                this.DisableUrlQueryRedaction = true;
+            }
+        }
+        catch (Exception ex)
+        {
+            OwinInstrumentationEventSource.Log.FailedToReadEnvironmentVariable(DisableQueryRedactionEnvVar, ex);
+        }
+    }
+
     /// <summary>
     /// Gets or sets a Filter function that determines whether or not to collect telemetry about requests on a per request basis.
     /// The Filter gets the <see cref="IOwinContext"/>, and should return a boolean.
@@ -32,4 +53,14 @@ public class OwinInstrumentationOptions
     /// https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/exceptions.md.
     /// </remarks>
     public bool RecordException { get; set; }
+
+    /// <summary>
+    /// Gets or sets a value indicating whether the url query value should be redacted or not.
+    /// </summary>
+    /// <remarks>
+    /// The query parameter values are redacted with value set as Redacted.
+    /// e.g. `?key1=value1` is set as `?key1=Redacted`.
+    /// The redaction can be disabled by setting this property to <see langword="true" />.
+    /// </remarks>
+    internal bool DisableUrlQueryRedaction { get; set; }
 }
diff --git a/src/Shared/RedactionHelper.cs b/src/Shared/RedactionHelper.cs
@@ -0,0 +1,59 @@
+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+#nullable enable
+
+using System.Text;
+
+namespace OpenTelemetry.Internal;
+
+internal sealed class RedactionHelper
+{
+    private const string RedactedText = "Redacted";
+
+    public static string? GetRedactedQueryString(string query)
+    {
+        int length = query.Length;
+        int index = 0;
+
+        // Preallocate some size to avoid re-sizing multiple times.
+        // Since the size will increase, allocating twice as much.
+        // TODO: Check to see if we can borrow from https://github.com/dotnet/runtime/blob/main/src/libraries/Common/src/System/Text/ValueStringBuilder.cs
+        // to improve perf.
+        StringBuilder queryBuilder = new(2 * length);
+        while (index < query.Length)
+        {
+            // Check if the character is = for redacting value.
+            if (query[index] == '=')
+            {
+                // Append =
+                queryBuilder.Append('=');
+                index++;
+
+                // Append redactedText in place of original value.
+                queryBuilder.Append(RedactedText);
+
+                // Move until end of this key/value pair.
+                while (index < length && query[index] != '&')
+                {
+                    index++;
+                }
+
+                // End of key/value.
+                if (index < length && query[index] == '&')
+                {
+                    queryBuilder.Append(query[index]);
+                }
+            }
+            else
+            {
+                // Keep adding to the result
+                queryBuilder.Append(query[index]);
+            }
+
+            index++;
+        }
+
+        return queryBuilder.ToString();
+    }
+}
diff --git a/test/OpenTelemetry.Instrumentation.AspNet.Tests/HttpInListenerTests.cs b/test/OpenTelemetry.Instrumentation.AspNet.Tests/HttpInListenerTests.cs
diff --git a/test/OpenTelemetry.Instrumentation.Owin.Tests/DiagnosticsMiddlewareTests.cs b/test/OpenTelemetry.Instrumentation.Owin.Tests/DiagnosticsMiddlewareTests.cs

Original file line number	Diff line number	Diff line change
`@@ -33,6 +33,15 @@ public void EnrichmentException(string eventName, Exception ex)`
`33`	`33`	`}`
`34`	`34`	`}`
`35`	`35`
	`36`	`+ [NonEvent]`
	`37`	`+ public void FailedToReadEnvironmentVariable(string envVarName, Exception ex)`
	`38`	`+ {`
	`39`	`+ if (this.IsEnabled(EventLevel.Error, EventKeywords.All))`
	`40`	`+ {`
	`41`	`+ this.EnrichmentException(envVarName, ex.ToInvariantString());`
	`42`	`+ }`
	`43`	`+ }`
	`44`	`+`
`36`	`45`	`[Event(1, Message = "Request is filtered out and will not be collected. Operation='{0}'", Level = EventLevel.Verbose)]`
`37`	`46`	`public void RequestIsFilteredOut(string operationName)`
`38`	`47`	`{`
`@@ -50,4 +59,10 @@ public void EnrichmentException(string eventName, string exception)`
`50`	`59`	`{`
`51`	`60`	`this.WriteEvent(3, eventName, exception);`
`52`	`61`	`}`
	`62`	`+`
	`63`	`+ [Event(4, Message = "Failed to read environment variable='{0}': {1}", Level = EventLevel.Error)]`
	`64`	`+ public void FailedToReadEnvironmentVariable(string envVarName, string exception)`
	`65`	`+ {`
	`66`	`+ this.WriteEvent(4, envVarName, exception);`
	`67`	`+ }`
`53`	`68`	`}`
Original file line number	Diff line number	Diff line change
`@@ -9,6 +9,7 @@`
`9`	`9`	`using Microsoft.Owin;`
`10`	`10`	`using OpenTelemetry.Context.Propagation;`
`11`	`11`	`using OpenTelemetry.Instrumentation.Owin.Implementation;`
	`12`	`+using OpenTelemetry.Internal;`
`12`	`13`	`using OpenTelemetry.Trace;`
`13`	`14`
`14`	`15`	`namespace OpenTelemetry.Instrumentation.Owin;`
`@@ -114,7 +115,7 @@ private static void BeginRequest(IOwinContext owinContext)`
`114`	`115`
`115`	`116`	`activity.SetTag(SemanticConventions.AttributeHttpMethod, request.Method);`
`116`	`117`	`activity.SetTag(SemanticConventions.AttributeHttpTarget, request.Uri.AbsolutePath);`
`117`		`- activity.SetTag(SemanticConventions.AttributeHttpUrl, GetUriTagValueFromRequestUri(request.Uri));`
	`118`	`+ activity.SetTag(SemanticConventions.AttributeHttpUrl, GetUriTagValueFromRequestUri(request.Uri, OwinInstrumentationActivitySource.Options.DisableUrlQueryRedaction));`
`118`	`119`
`119`	`120`	`if (request.Headers.TryGetValue("User-Agent", out string[] userAgent) && userAgent.Length > 0)`
`120`	`121`	`{`
`@@ -228,13 +229,15 @@ private static void RequestEnd(IOwinContext owinContext, Exception? exception, l`
`228`	`229`	`/// </summary>`
`229`	`230`	`/// <param name="uri"><see cref="Uri"/>.</param>`
`230`	`231`	`/// <returns>Span uri value.</returns>`
`231`		`- private static string GetUriTagValueFromRequestUri(Uri uri)`
	`232`	`+ private static string GetUriTagValueFromRequestUri(Uri uri, bool disableQueryRedaction)`
`232`	`233`	`{`
`233`		`- if (string.IsNullOrEmpty(uri.UserInfo))`
	`234`	`+ if (string.IsNullOrEmpty(uri.UserInfo) && disableQueryRedaction)`
`234`	`235`	`{`
`235`		`- return uri.ToString();`
	`236`	`+ return uri.OriginalString;`
`236`	`237`	`}`
`237`	`238`
`238`		`- return string.Concat(uri.Scheme, Uri.SchemeDelimiter, uri.Authority, uri.PathAndQuery, uri.Fragment);`
	`239`	`+ var query = disableQueryRedaction ? uri.Query : RedactionHelper.GetRedactedQueryString(uri.Query);`
	`240`	`+`
	`241`	`+ return string.Concat(uri.Scheme, Uri.SchemeDelimiter, uri.Authority, uri.AbsolutePath, query, uri.Fragment);`
`239`	`242`	`}`
`240`	`243`	`}`
Original file line number	Diff line number	Diff line change
`@@ -45,16 +45,32 @@ public void EnrichmentException(Exception exception)`
`45`	`45`	`}`
`46`	`46`	`}`
`47`	`47`
	`48`	`+ [NonEvent]`
	`49`	`+ public void FailedToReadEnvironmentVariable(string envVarName, Exception ex)`
	`50`	`+ {`
	`51`	`+ if (this.IsEnabled(EventLevel.Error, EventKeywords.All))`
	`52`	`+ {`
	`53`	`+ this.FailedToReadEnvironmentVariable(envVarName, ex.ToInvariantString());`
	`54`	`+ }`
	`55`	`+ }`
	`56`	`+`
`48`	`57`	`[Event(EventIds.EnrichmentException, Message = "Enrichment threw exception. Exception {0}.", Level = EventLevel.Error)]`
`49`	`58`	`public void EnrichmentException(string exception)`
`50`	`59`	`{`
`51`	`60`	`this.WriteEvent(EventIds.EnrichmentException, exception);`
`52`	`61`	`}`
`53`	`62`
	`63`	`+ [Event(EventIds.FailedToReadEnvironmentVariable, Message = "Failed to read environment variable='{0}': {1}", Level = EventLevel.Error)]`
	`64`	`+ public void FailedToReadEnvironmentVariable(string envVarName, string exception)`
	`65`	`+ {`
	`66`	`+ this.WriteEvent(4, envVarName, exception);`
	`67`	`+ }`
	`68`	`+`
`54`	`69`	`private class EventIds`
`55`	`70`	`{`
`56`	`71`	`public const int RequestIsFilteredOut = 1;`
`57`	`72`	`public const int RequestFilterException = 2;`
`58`	`73`	`public const int EnrichmentException = 3;`
	`74`	`+ public const int FailedToReadEnvironmentVariable = 4;`
`59`	`75`	`}`
`60`	`76`	`}`