From 8ff9b2c1e10a3c5deedac4e3a66b930c868052c4 Mon Sep 17 00:00:00 2001 From: Alan West <3676547+alanwest@users.noreply.github.com> Date: Fri, 20 Dec 2024 14:54:25 -0800 Subject: [PATCH 1/6] Add SQL sanitization to SqlClient --- .../SqlClientDiagnosticListener.cs | 12 +++---- ...Telemetry.Instrumentation.SqlClient.csproj | 1 + src/Shared/SqlProcessor.cs | 32 ++++++++++++++++++- 3 files changed, 38 insertions(+), 7 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs index 00342c6c58..c990695a54 100644 --- 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 dataSourceFetcher = new("DataSource"); private readonly PropertyFetcher databaseFetcher = new("Database"); private readonly PropertyFetcher commandTypeFetcher = new("CommandType"); - private readonly PropertyFetcher commandTextFetcher = new("CommandText"); + private readonly PropertyFetcher commandTextFetcher = new("CommandText"); private readonly PropertyFetcher exceptionFetcher = new("Exception"); private readonly PropertyFetcher exceptionNumberFetcher = new("Number"); private readonly AsyncLocal 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 index cdeb2cd9a4..cbb8e70bff 100644 --- a/src/OpenTelemetry.Instrumentation.SqlClient/OpenTelemetry.Instrumentation.SqlClient.csproj +++ b/src/OpenTelemetry.Instrumentation.SqlClient/OpenTelemetry.Instrumentation.SqlClient.csproj @@ -26,6 +26,7 @@ + diff --git a/src/Shared/SqlProcessor.cs b/src/Shared/SqlProcessor.cs index c5c5ee9a62..c23a7c8a28 100644 --- 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) { From 4ec21fbcf0ccbe4125ba6aa738549796f7fdf622 Mon Sep 17 00:00:00 2001 From: Alan West <3676547+alanwest@users.noreply.github.com> Date: Fri, 20 Dec 2024 14:54:44 -0800 Subject: [PATCH 2/6] Update readme --- src/OpenTelemetry.Instrumentation.SqlClient/README.md | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/README.md b/src/OpenTelemetry.Instrumentation.SqlClient/README.md index d77db1ce59..46f42f1666 100644 --- 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 +The text of a database query may include 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. +attribute is captured in scenarios where the query text requires sanitization to +prevent the capture of sensitive data. When enabled, literal values in the query +text are replaced by a `?` character. The behavior of `SetDbStatementForText` +depends on the runtime used. #### .NET From 0b098dba96dfe2ead0d5acde413407dd230039cc Mon Sep 17 00:00:00 2001 From: Alan West <3676547+alanwest@users.noreply.github.com> Date: Fri, 20 Dec 2024 14:56:44 -0800 Subject: [PATCH 3/6] Update changelog --- src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md index e1e5f5c1ee..82f89d9190 100644 --- 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. + ([#TBD](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/TBD)) + ## 1.10.0-beta.1 Released 2024-Dec-09 From cd903398c50bfccf610b9fe55490b559d46a207d Mon Sep 17 00:00:00 2001 From: Alan West <3676547+alanwest@users.noreply.github.com> Date: Mon, 23 Dec 2024 10:38:09 -0800 Subject: [PATCH 4/6] Update changelog --- src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md index 82f89d9190..b22e0c290d 100644 --- a/src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md @@ -13,7 +13,7 @@ * 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. - ([#TBD](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/TBD)) + ([#2446](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2446)) ## 1.10.0-beta.1 From 968bb60859ffebe9094c640f459e92d4eb489e49 Mon Sep 17 00:00:00 2001 From: Alan West <3676547+alanwest@users.noreply.github.com> Date: Mon, 23 Dec 2024 13:59:50 -0800 Subject: [PATCH 5/6] Update tests --- .../SqlClientIntegrationTests.cs | 13 +++++++------ .../SqlClientTests.cs | 2 +- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlClientIntegrationTests.cs b/test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlClientIntegrationTests.cs index 2185963f9e..bb72593691 100644 --- 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 index c31cd18c40..759cf81445 100644 --- 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, From 19ca7a7b7ed8022920b05dce8d61ef25a1072135 Mon Sep 17 00:00:00 2001 From: Alan West <3676547+alanwest@users.noreply.github.com> Date: Tue, 24 Dec 2024 09:37:01 -0800 Subject: [PATCH 6/6] Update changelog --- .../README.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/README.md b/src/OpenTelemetry.Instrumentation.SqlClient/README.md index 46f42f1666..f70e6b39ba 100644 --- a/src/OpenTelemetry.Instrumentation.SqlClient/README.md +++ b/src/OpenTelemetry.Instrumentation.SqlClient/README.md @@ -123,13 +123,13 @@ This instrumentation can be configured to change the default behavior by using ### SetDbStatementForText -The text of a database query may include 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 the query text requires sanitization to -prevent the capture of sensitive data. When enabled, literal values in the query -text are replaced by a `?` character. 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