[SqlClient] Sanitize SQL query text (#2446)

open-telemetry · Dec 24, 2024 · 5ac64ed · 5ac64ed
1 parent 1dae73c
commit 5ac64ed
Show file tree

Hide file tree

Showing 7 changed files with 58 additions and 20 deletions.
diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md
@@ -10,6 +10,11 @@
   to set default explicit buckets following the [OpenTelemetry Specification](https://github.com/open-telemetry/semantic-conventions/blob/v1.29.0/docs/database/database-metrics.md).
   ([#2430](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2430))
 
+* Enabling `SetDbStatementForText` will no longer capture the raw query text.
+  The query is now sanitized. Literal values in the query text are replaced
+  by a `?` character.
+  ([#2446](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2446))
+
 ## 1.10.0-beta.1
 
 Released 2024-Dec-09

diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs
@@ -31,7 +31,7 @@ internal sealed class SqlClientDiagnosticListener : ListenerHandler
     private readonly PropertyFetcher<string> dataSourceFetcher = new("DataSource");
     private readonly PropertyFetcher<string> databaseFetcher = new("Database");
     private readonly PropertyFetcher<CommandType> commandTypeFetcher = new("CommandType");
-    private readonly PropertyFetcher<object> commandTextFetcher = new("CommandText");
+    private readonly PropertyFetcher<string> commandTextFetcher = new("CommandText");
     private readonly PropertyFetcher<Exception> exceptionFetcher = new("Exception");
     private readonly PropertyFetcher<int> exceptionNumberFetcher = new("Number");
     private readonly AsyncLocal<long> beginTimestamp = new();
@@ -102,9 +102,8 @@ public override void OnEventWritten(string name, object? payload)
                             return;
                         }
 
-                        _ = this.commandTextFetcher.TryFetch(command, out var commandText);
-
-                        if (this.commandTypeFetcher.TryFetch(command, out var commandType))
+                        if (this.commandTypeFetcher.TryFetch(command, out var commandType) &&
+                            this.commandTextFetcher.TryFetch(command, out var commandText))
                         {
                             switch (commandType)
                             {
@@ -126,14 +125,15 @@ public override void OnEventWritten(string name, object? payload)
                                 case CommandType.Text:
                                     if (options.SetDbStatementForText)
                                     {
+                                        var sanitizedSql = SqlProcessor.GetSanitizedSql(commandText);
                                         if (options.EmitOldAttributes)
                                         {
-                                            activity.SetTag(SemanticConventions.AttributeDbStatement, commandText);
+                                            activity.SetTag(SemanticConventions.AttributeDbStatement, sanitizedSql);
                                         }
 
                                         if (options.EmitNewAttributes)
                                         {
-                                            activity.SetTag(SemanticConventions.AttributeDbQueryText, commandText);
+                                            activity.SetTag(SemanticConventions.AttributeDbQueryText, sanitizedSql);
                                         }
                                     }
 

diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/OpenTelemetry.Instrumentation.SqlClient.csproj b/src/OpenTelemetry.Instrumentation.SqlClient/OpenTelemetry.Instrumentation.SqlClient.csproj
@@ -26,6 +26,7 @@
     <Compile Include="$(RepoRoot)\src\Shared\NullableAttributes.cs" Link="Includes\NullableAttributes.cs" />
     <Compile Include="$(RepoRoot)\src\Shared\PropertyFetcher.AOT.cs" Link="Includes\PropertyFetcher.AOT.cs" />
     <Compile Include="$(RepoRoot)\src\Shared\SemanticConventions.cs" Link="Includes\SemanticConventions.cs" />
+    <Compile Include="$(RepoRoot)\src\Shared\SqlProcessor.cs" Link="Includes\SqlProcessor.cs" />
   </ItemGroup>
 
   <ItemGroup>

diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/README.md b/src/OpenTelemetry.Instrumentation.SqlClient/README.md
@@ -123,12 +123,13 @@ This instrumentation can be configured to change the default behavior by using
 
 ### SetDbStatementForText
 
-Capturing the text of a database query may run the risk of capturing sensitive data.
-`SetDbStatementForText` controls whether the
-[`db.statement`](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/database/database-spans.md#call-level-attributes)
-attribute is captured in scenarios where there could be a risk of exposing
-sensitive data. The behavior of `SetDbStatementForText` depends on the runtime
-used.
+`SetDbStatementForText` controls whether the `db.statement` attribute is
+emitted. The behavior of `SetDbStatementForText` depends on the runtime used,
+see below for more details.
+
+Query text may contain sensitive data, so when `SetDbStatementForText` is
+enabled the raw query text is sanitized by automatically replacing literal
+values with a `?` character.
 
 #### .NET
 

diff --git a/src/Shared/SqlProcessor.cs b/src/Shared/SqlProcessor.cs
@@ -1,19 +1,49 @@
 // Copyright The OpenTelemetry Authors
 // SPDX-License-Identifier: Apache-2.0
 
+using System.Collections;
 using System.Text;
 
 namespace OpenTelemetry.Instrumentation;
 
 internal static class SqlProcessor
 {
-    public static string GetSanitizedSql(string sql)
+    private const int CacheCapacity = 1000;
+    private static readonly Hashtable Cache = [];
+
+    public static string GetSanitizedSql(string? sql)
     {
         if (sql == null)
         {
             return string.Empty;
         }
 
+        if (Cache[sql] is not string sanitizedSql)
+        {
+            sanitizedSql = SanitizeSql(sql);
+
+            if (Cache.Count == CacheCapacity)
+            {
+                return sanitizedSql;
+            }
+
+            lock (Cache)
+            {
+                if ((Cache[sql] as string) == null)
+                {
+                    if (Cache.Count < CacheCapacity)
+                    {
+                        Cache[sql] = sanitizedSql;
+                    }
+                }
+            }
+        }
+
+        return sanitizedSql!;
+    }
+
+    private static string SanitizeSql(string sql)
+    {
         var sb = new StringBuilder(capacity: sql.Length);
         for (var i = 0; i < sql.Length; ++i)
         {

diff --git a/test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlClientIntegrationTests.cs b/test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlClientIntegrationTests.cs
@@ -24,15 +24,16 @@ public SqlClientIntegrationTests(SqlClientIntegrationTestsFixture fixture)
 
     [EnabledOnDockerPlatformTheory(EnabledOnDockerPlatformTheoryAttribute.DockerPlatform.Linux)]
     [InlineData(CommandType.Text, "select 1/1")]
-    [InlineData(CommandType.Text, "select 1/1", true)]
-    [InlineData(CommandType.Text, "select 1/0", false, true)]
-    [InlineData(CommandType.Text, "select 1/0", false, true, false, false)]
-    [InlineData(CommandType.Text, "select 1/0", false, true, true, false)]
-    [InlineData(CommandType.StoredProcedure, "sp_who")]
+    [InlineData(CommandType.Text, "select 1/1", true, "select ?/?")]
+    [InlineData(CommandType.Text, "select 1/0", false, null, true)]
+    [InlineData(CommandType.Text, "select 1/0", false, null, true, false, false)]
+    [InlineData(CommandType.Text, "select 1/0", false, null, true, true, false)]
+    [InlineData(CommandType.StoredProcedure, "sp_who", false, "sp_who")]
     public void SuccessfulCommandTest(
         CommandType commandType,
         string commandText,
         bool captureTextCommandContent = false,
+        string? sanitizedCommandText = null,
         bool isFailure = false,
         bool recordException = false,
         bool shouldEnrich = true)
@@ -84,7 +85,7 @@ public void SuccessfulCommandTest(
         Assert.Single(activities);
         var activity = activities[0];
 
-        SqlClientTests.VerifyActivityData(commandType, commandText, captureTextCommandContent, isFailure, recordException, shouldEnrich, activity);
+        SqlClientTests.VerifyActivityData(commandType, sanitizedCommandText, captureTextCommandContent, isFailure, recordException, shouldEnrich, activity);
         SqlClientTests.VerifySamplingParameters(sampler.LatestSamplingParameters);
 
         if (isFailure)

diff --git a/test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlClientTests.cs b/test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlClientTests.cs
@@ -408,7 +408,7 @@ public void ShouldNotCollectTelemetryAndShouldNotPropagateExceptionWhenFilterThr
 
     internal static void VerifyActivityData(
         CommandType commandType,
-        string commandText,
+        string? commandText,
         bool captureTextCommandContent,
         bool isFailure,
         bool recordException,