Sanitize HTTP headers when the agent reads them (#1286)

* Sanitize HTTP headers when the agent reads them * Review
elastic · May 4, 2021 · c2b519a · c2b519a
1 parent f6a33a1
commit c2b519a
Show file tree

Hide file tree

Showing 9 changed files with 95 additions and 13 deletions.
diff --git 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<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)

diff --git 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<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;
 		}
@@ -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
@@ -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>
 

diff --git a/src/Elastic.Apm/Filters/ErrorContextSanitizerFilter.cs 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
+{
+	/// <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;
+		}
+	}
+}
diff --git 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
@@ -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<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;
 
@@ -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
@@ -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;

diff --git 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]");
 		}
 
+		/// <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

diff --git 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<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>>();
@@ -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;