From c2b519aaa0fe5e5044b736cfec695342f124bf30 Mon Sep 17 00:00:00 2001 From: Gergely Kalapos Date: Tue, 4 May 2021 23:37:15 +0200 Subject: [PATCH] Sanitize HTTP headers when the agent reads them (#1286) * Sanitize HTTP headers when the agent reads them * Review --- .../WebRequestTransactionCreator.cs | 11 ++++-- .../ElasticApmModule.cs | 14 +++++--- src/Elastic.Apm/Elastic.Apm.csproj | 2 +- .../Filters/ErrorContextSanitizerFilter.cs | 36 +++++++++++++++++++ .../HeaderDictionarySanitizerFilter.cs | 2 +- src/Elastic.Apm/Model/Error.cs | 8 +++-- src/Elastic.Apm/Report/PayloadSenderV2.cs | 4 ++- .../SanitizeFieldNamesTests.cs | 28 +++++++++++++++ .../MockPayloadSender.cs | 3 +- 9 files changed, 95 insertions(+), 13 deletions(-) create mode 100644 src/Elastic.Apm/Filters/ErrorContextSanitizerFilter.cs diff --git a/src/Elastic.Apm.AspNetCore/WebRequestTransactionCreator.cs b/src/Elastic.Apm.AspNetCore/WebRequestTransactionCreator.cs index a6609a5b8..67e1f9814 100644 --- a/src/Elastic.Apm.AspNetCore/WebRequestTransactionCreator.cs +++ b/src/Elastic.Apm.AspNetCore/WebRequestTransactionCreator.cs @@ -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); } @@ -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); } @@ -124,7 +126,10 @@ private static void FillSampledTransactionContextRequest(HttpContext context, Tr private static Dictionary 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) diff --git a/src/Elastic.Apm.AspNetFullFramework/ElasticApmModule.cs b/src/Elastic.Apm.AspNetFullFramework/ElasticApmModule.cs index 596f5c76a..573b85bc7 100644 --- a/src/Elastic.Apm.AspNetFullFramework/ElasticApmModule.cs +++ b/src/Elastic.Apm.AspNetFullFramework/ElasticApmModule.cs @@ -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; @@ -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 }; } @@ -246,14 +249,17 @@ private static string GetHttpVersion(string protocol) } } - private static Dictionary ConvertHeaders(NameValueCollection headers) + private static Dictionary ConvertHeaders(NameValueCollection headers, IConfigSnapshot configSnapshot) { var convertedHeaders = new Dictionary(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; } @@ -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) diff --git a/src/Elastic.Apm/Elastic.Apm.csproj b/src/Elastic.Apm/Elastic.Apm.csproj index 14380300d..5f8909254 100644 --- a/src/Elastic.Apm/Elastic.Apm.csproj +++ b/src/Elastic.Apm/Elastic.Apm.csproj @@ -68,7 +68,7 @@ - + diff --git a/src/Elastic.Apm/Filters/ErrorContextSanitizerFilter.cs b/src/Elastic.Apm/Filters/ErrorContextSanitizerFilter.cs new file mode 100644 index 000000000..089edff61 --- /dev/null +++ b/src/Elastic.Apm/Filters/ErrorContextSanitizerFilter.cs @@ -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 +{ + /// + /// A filter that sanitizes fields on error based on the setting + /// + 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; + } + } +} diff --git a/src/Elastic.Apm/Filters/HeaderDictionarySanitizerFilter.cs b/src/Elastic.Apm/Filters/HeaderDictionarySanitizerFilter.cs index d3b570ea4..729b641f4 100644 --- a/src/Elastic.Apm/Filters/HeaderDictionarySanitizerFilter.cs +++ b/src/Elastic.Apm/Filters/HeaderDictionarySanitizerFilter.cs @@ -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; diff --git a/src/Elastic.Apm/Model/Error.cs b/src/Elastic.Apm/Model/Error.cs index c6276b98e..eafd6a1f8 100644 --- a/src/Elastic.Apm/Model/Error.cs +++ b/src/Elastic.Apm/Model/Error.cs @@ -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; @@ -13,8 +14,10 @@ namespace Elastic.Apm.Model { internal class Error : IError { - public Error(CapturedException capturedException, Transaction transaction, string parentId, IApmLogger loggerArg, - Dictionary labels = null + [JsonIgnore] + internal IConfigSnapshot ConfigSnapshot { get; } + + public Error(CapturedException capturedException, Transaction transaction, string parentId, IApmLogger loggerArg, Dictionary labels = null ) : this(transaction, parentId, loggerArg, labels) => Exception = capturedException; @@ -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; diff --git a/src/Elastic.Apm/Report/PayloadSenderV2.cs b/src/Elastic.Apm/Report/PayloadSenderV2.cs index a96edf6a0..178ac2c47 100644 --- a/src/Elastic.Apm/Report/PayloadSenderV2.cs +++ b/src/Elastic.Apm/Report/PayloadSenderV2.cs @@ -109,13 +109,14 @@ public PayloadSenderV2( _eventQueue = new BatchBlock(config.MaxBatchEventCount); - SetUpFilters(TransactionFilters, SpanFilters, apmServerInfo, logger); + SetUpFilters(TransactionFilters, SpanFilters, ErrorFilters, apmServerInfo, logger); StartWorkLoop(); } internal static void SetUpFilters( List> transactionFilters, List> spanFilters, + List> errorFilters, IApmServerInfo apmServerInfo, IApmLogger logger) { @@ -123,6 +124,7 @@ internal static void SetUpFilters( 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; diff --git a/test/Elastic.Apm.AspNetCore.Tests/SanitizeFieldNamesTests.cs b/test/Elastic.Apm.AspNetCore.Tests/SanitizeFieldNamesTests.cs index 98c9ca3b6..b55faf533 100644 --- a/test/Elastic.Apm.AspNetCore.Tests/SanitizeFieldNamesTests.cs +++ b/test/Elastic.Apm.AspNetCore.Tests/SanitizeFieldNamesTests.cs @@ -299,6 +299,34 @@ public async Task DefaultsWithHeaders(string headerName, bool useOnlyDiagnosticS _capturedPayload.FirstTransaction.Context.Request.Headers[headerName].Should().Be("[REDACTED]"); } + /// + /// Asserts that context on error is sanitized in case of HTTP calls. + /// + /// + /// + [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]"); + } + ///// ///// ASP.NET Core seems to rewrite the name of these headers (so authorization becomes Authorization). ///// Our "by default case insensitivity" still works, the only difference is that if we send a header with name diff --git a/test/Elastic.Apm.Tests.Utilities/MockPayloadSender.cs b/test/Elastic.Apm.Tests.Utilities/MockPayloadSender.cs index a46510870..15d5244c9 100644 --- a/test/Elastic.Apm.Tests.Utilities/MockPayloadSender.cs +++ b/test/Elastic.Apm.Tests.Utilities/MockPayloadSender.cs @@ -19,6 +19,7 @@ namespace Elastic.Apm.Tests.Utilities internal class MockPayloadSender : IPayloadSender { private readonly List _errors = new List(); + private readonly List> _errorFilters = new List>(); private readonly object _lock = new object(); private readonly List _metrics = new List(); private readonly List> _spanFilters = new List>(); @@ -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;