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

Sanitize HTTP headers when the agent reads them #1286

Merged
merged 2 commits into from
May 4, 2021
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
11 changes: 8 additions & 3 deletions src/Elastic.Apm.AspNetCore/WebRequestTransactionCreator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ internal static ITransaction StartTransactionAsync(HttpContext context, IApmLogg
logger.Debug()
?.Log(
"Incoming request with {TraceParentHeaderName} header. DistributedTracingData: {DistributedTracingData}. Continuing trace.",
containsPrefixedTraceParentHeader? TraceContext.TraceParentHeaderNamePrefixed : TraceContext.TraceParentHeaderName, tracingData);
containsPrefixedTraceParentHeader ? TraceContext.TraceParentHeaderNamePrefixed : TraceContext.TraceParentHeaderName,
tracingData);

transaction = tracer.StartTransaction(transactionName, ApiConstants.TypeRequest, tracingData);
}
Expand All @@ -63,7 +64,8 @@ internal static ITransaction StartTransactionAsync(HttpContext context, IApmLogg
logger.Debug()
?.Log(
"Incoming request with invalid {TraceParentHeaderName} header (received value: {TraceParentHeaderValue}). Starting trace with new trace id.",
containsPrefixedTraceParentHeader? TraceContext.TraceParentHeaderNamePrefixed : TraceContext.TraceParentHeaderName, traceParentHeader);
containsPrefixedTraceParentHeader ? TraceContext.TraceParentHeaderNamePrefixed : TraceContext.TraceParentHeaderName,
traceParentHeader);

transaction = tracer.StartTransaction(transactionName, ApiConstants.TypeRequest);
}
Expand Down Expand Up @@ -124,7 +126,10 @@ private static void FillSampledTransactionContextRequest(HttpContext context, Tr

private static Dictionary<string, string> GetHeaders(IHeaderDictionary headers, IConfigSnapshot configSnapshot) =>
configSnapshot.CaptureHeaders && headers != null
? headers.ToDictionary(header => header.Key, header => header.Value.ToString())
? headers.ToDictionary(header => header.Key,
header => WildcardMatcher.IsAnyMatch(configSnapshot.SanitizeFieldNames, header.Key)
? Apm.Consts.Redacted
: header.Value.ToString())
: null;

private static string GetRawUrl(HttpRequest httpRequest, IApmLogger logger)
Expand Down
14 changes: 10 additions & 4 deletions src/Elastic.Apm.AspNetFullFramework/ElasticApmModule.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using Elastic.Apm.Api;
using Elastic.Apm.AspNetFullFramework.Extensions;
using Elastic.Apm.AspNetFullFramework.Helper;
using Elastic.Apm.Config;
using Elastic.Apm.DiagnosticSource;
using Elastic.Apm.Helpers;
using Elastic.Apm.Logging;
Expand Down Expand Up @@ -227,7 +228,9 @@ private static void FillSampledTransactionContextRequest(HttpRequest request, IT
{
Socket = new Socket { Encrypted = request.IsSecureConnection, RemoteAddress = request.UserHostAddress },
HttpVersion = GetHttpVersion(request.ServerVariables["SERVER_PROTOCOL"]),
Headers = _isCaptureHeadersEnabled ? ConvertHeaders(request.Unvalidated.Headers) : null
Headers = _isCaptureHeadersEnabled
? ConvertHeaders(request.Unvalidated.Headers, (transaction as Transaction)?.ConfigSnapshot)
: null
};
}

Expand All @@ -246,14 +249,17 @@ private static string GetHttpVersion(string protocol)
}
}

private static Dictionary<string, string> ConvertHeaders(NameValueCollection headers)
private static Dictionary<string, string> ConvertHeaders(NameValueCollection headers, IConfigSnapshot configSnapshot)
{
var convertedHeaders = new Dictionary<string, string>(headers.Count);
foreach (var key in headers.AllKeys)
{
var value = headers.Get(key);
if (value != null)
convertedHeaders.Add(key, value);
{
convertedHeaders.Add(key,
WildcardMatcher.IsAnyMatch(configSnapshot?.SanitizeFieldNames, key) ? Consts.Redacted : value);
}
}
return convertedHeaders;
}
Expand Down Expand Up @@ -374,7 +380,7 @@ private static void FillSampledTransactionContextResponse(HttpResponse response,
{
Finished = true,
StatusCode = response.StatusCode,
Headers = _isCaptureHeadersEnabled ? ConvertHeaders(response.Headers) : null
Headers = _isCaptureHeadersEnabled ? ConvertHeaders(response.Headers, (transaction as Transaction)?.ConfigSnapshot) : null
};

private void FillSampledTransactionContextUser(HttpContext context, ITransaction transaction)
Expand Down
2 changes: 1 addition & 1 deletion src/Elastic.Apm/Elastic.Apm.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@
<PackageReference Include="Microsoft.Diagnostics.Tracing.TraceEvent" Version="2.0.2" />
<PackageReference Include="System.Threading.Tasks.Dataflow" Version="4.9.0" />
<!-- Used by Ben.Demystifier -->
<PackageReference Include="System.Reflection.Metadata" Version="5.0.0"/>
<PackageReference Include="System.Reflection.Metadata" Version="5.0.0" />
<PackageReference Include="System.Threading.Tasks.Extensions" Version="4.5.4" />
</ItemGroup>

Expand Down
36 changes: 36 additions & 0 deletions src/Elastic.Apm/Filters/ErrorContextSanitizerFilter.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// Licensed to Elasticsearch B.V under
// one or more agreements.
// Elasticsearch B.V licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information

using System.Linq;
using Elastic.Apm.Api;
using Elastic.Apm.Config;
using Elastic.Apm.Helpers;
using Elastic.Apm.Model;

namespace Elastic.Apm.Filters
{
/// <summary>
/// A filter that sanitizes fields on error based on the <see cref="IConfigurationReader.SanitizeFieldNames"/> setting
/// </summary>
internal class ErrorContextSanitizerFilter
{
public IError Filter(IError error)
{
if (error is Error realError)
{
if (realError.Context.Request?.Headers != null && realError.ConfigSnapshot != null)
{
foreach (var key in realError.Context?.Request?.Headers?.Keys)
{
if (WildcardMatcher.IsAnyMatch(realError.ConfigSnapshot.SanitizeFieldNames, key))
realError.Context.Request.Headers[key] = Consts.Redacted;
}
}
}

return error;
}
}
}
2 changes: 1 addition & 1 deletion src/Elastic.Apm/Filters/HeaderDictionarySanitizerFilter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public ITransaction Filter(ITransaction transaction)
{
if (realTransaction.IsContextCreated && realTransaction.Context.Request?.Headers != null)
{
foreach (var key in realTransaction.Context?.Request?.Headers?.Keys.ToList())
foreach (var key in realTransaction.Context?.Request?.Headers?.Keys)
{
if (WildcardMatcher.IsAnyMatch(realTransaction.ConfigSnapshot.SanitizeFieldNames, key))
realTransaction.Context.Request.Headers[key] = Consts.Redacted;
Expand Down
8 changes: 6 additions & 2 deletions src/Elastic.Apm/Model/Error.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Collections.Generic;
using Elastic.Apm.Api;
using Elastic.Apm.Api.Constraints;
using Elastic.Apm.Config;
using Elastic.Apm.Helpers;
using Elastic.Apm.Logging;
using Elastic.Apm.Libraries.Newtonsoft.Json;
Expand All @@ -13,8 +14,10 @@ namespace Elastic.Apm.Model
{
internal class Error : IError
{
public Error(CapturedException capturedException, Transaction transaction, string parentId, IApmLogger loggerArg,
Dictionary<string, Label> labels = null
[JsonIgnore]
internal IConfigSnapshot ConfigSnapshot { get; }

public Error(CapturedException capturedException, Transaction transaction, string parentId, IApmLogger loggerArg, Dictionary<string, Label> labels = null
)
: this(transaction, parentId, loggerArg, labels) => Exception = capturedException;

Expand All @@ -33,6 +36,7 @@ private Error(Transaction transaction, string parentId, IApmLogger loggerArg, Di
TraceId = transaction.TraceId;
TransactionId = transaction.Id;
Transaction = new TransactionData(transaction.IsSampled, transaction.Type);
ConfigSnapshot = transaction.ConfigSnapshot;
}

ParentId = parentId;
Expand Down
4 changes: 3 additions & 1 deletion src/Elastic.Apm/Report/PayloadSenderV2.cs
Original file line number Diff line number Diff line change
Expand Up @@ -109,20 +109,22 @@ public PayloadSenderV2(

_eventQueue = new BatchBlock<object>(config.MaxBatchEventCount);

SetUpFilters(TransactionFilters, SpanFilters, apmServerInfo, logger);
SetUpFilters(TransactionFilters, SpanFilters, ErrorFilters, apmServerInfo, logger);
StartWorkLoop();
}

internal static void SetUpFilters(
List<Func<ITransaction, ITransaction>> transactionFilters,
List<Func<ISpan, ISpan>> spanFilters,
List<Func<IError, IError>> errorFilters,
IApmServerInfo apmServerInfo,
IApmLogger logger)
{
transactionFilters.Add(new TransactionIgnoreUrlsFilter().Filter);
transactionFilters.Add(new HeaderDictionarySanitizerFilter().Filter);
// with this, stack trace demystification and conversion to the intake API model happens on a non-application thread:
spanFilters.Add(new SpanStackTraceCapturingFilter(logger, apmServerInfo).Filter);
errorFilters.Add(new ErrorContextSanitizerFilter().Filter);
}

private bool _getApmServerVersion;
Expand Down
28 changes: 28 additions & 0 deletions test/Elastic.Apm.AspNetCore.Tests/SanitizeFieldNamesTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,34 @@ public async Task DefaultsWithHeaders(string headerName, bool useOnlyDiagnosticS
_capturedPayload.FirstTransaction.Context.Request.Headers[headerName].Should().Be("[REDACTED]");
}

/// <summary>
/// Asserts that context on error is sanitized in case of HTTP calls.
/// </summary>
/// <param name="headerName"></param>
/// <param name="useOnlyDiagnosticSource"></param>
[Theory]
[MemberData(nameof(GetData), Tests.DefaultsWithHeaders)]
public async Task SanitizeHeadersOnError(string headerName, bool useOnlyDiagnosticSource)
{
CreateAgent(useOnlyDiagnosticSource);
_client.DefaultRequestHeaders.Add(headerName, "123");
await _client.GetAsync("/Home/TriggerError");

_capturedPayload.WaitForTransactions();
_capturedPayload.Transactions.Should().ContainSingle();
_capturedPayload.FirstTransaction.Context.Should().NotBeNull();
_capturedPayload.FirstTransaction.Context.Request.Should().NotBeNull();
_capturedPayload.FirstTransaction.Context.Request.Headers.Should().NotBeNull();
_capturedPayload.FirstTransaction.Context.Request.Headers[headerName].Should().Be("[REDACTED]");

_capturedPayload.WaitForErrors();
_capturedPayload.Errors.Should().ContainSingle();
_capturedPayload.FirstError.Context.Should().NotBeNull();
_capturedPayload.FirstError.Context.Request.Should().NotBeNull();
_capturedPayload.FirstError.Context.Request.Headers.Should().NotBeNull();
_capturedPayload.FirstError.Context.Request.Headers[headerName].Should().Be("[REDACTED]");
}

///// <summary>
///// ASP.NET Core seems to rewrite the name of these headers (so <code>authorization</code> becomes <code>Authorization</code>).
///// Our "by default case insensitivity" still works, the only difference is that if we send a header with name
Expand Down
3 changes: 2 additions & 1 deletion test/Elastic.Apm.Tests.Utilities/MockPayloadSender.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ namespace Elastic.Apm.Tests.Utilities
internal class MockPayloadSender : IPayloadSender
{
private readonly List<IError> _errors = new List<IError>();
private readonly List<Func<IError, IError>> _errorFilters = new List<Func<IError, IError>>();
private readonly object _lock = new object();
private readonly List<IMetricSet> _metrics = new List<IMetricSet>();
private readonly List<Func<ISpan, ISpan>> _spanFilters = new List<Func<ISpan, ISpan>>();
Expand All @@ -41,7 +42,7 @@ public MockPayloadSender(IApmLogger logger = null)
_errorWaitHandle = _waitHandles[2];
_metricSetWaitHandle = _waitHandles[3];

PayloadSenderV2.SetUpFilters(_transactionFilters, _spanFilters, MockApmServerInfo.Version710, logger ?? new NoopLogger());
PayloadSenderV2.SetUpFilters(_transactionFilters, _spanFilters, _errorFilters, MockApmServerInfo.Version710, logger ?? new NoopLogger());
}

private readonly AutoResetEvent _transactionWaitHandle;
Expand Down