Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SqlClient] Sanitize SQL query text #2446

Merged
merged 7 commits into from
Dec 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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)
{
Expand All @@ -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);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
13 changes: 7 additions & 6 deletions src/OpenTelemetry.Instrumentation.SqlClient/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
32 changes: 31 additions & 1 deletion src/Shared/SqlProcessor.cs
Original file line number Diff line number Diff line change
@@ -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)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,7 @@ public void ShouldNotCollectTelemetryAndShouldNotPropagateExceptionWhenFilterThr

internal static void VerifyActivityData(
CommandType commandType,
string commandText,
string? commandText,
bool captureTextCommandContent,
bool isFailure,
bool recordException,
Expand Down
Loading