Skip to content

Commit

Permalink
No longer parse request cookies, but ensure they are still redacted i…
Browse files Browse the repository at this point in the history
…n the `Cookie` header string (#2444)

We've identified that sending transactions with cookies parsed to
`Request.Cookies` can result in failures to index those transactions
when any of the cookies include periods (which is common for ASP.NET
core by default).

This PR removes the extraction from the header onto `Request.Cookies`.
Instead, we now modify the `Cookie` header value to redact the cookie
items based on the configured list of field names to sanitize. For .NET
targets, we optimize this to avoid allocations as much as possible.
Unfortunately, for the .NET Framework, we can't be as efficient.
  • Loading branch information
stevejgordon authored Sep 17, 2024
1 parent 44820f4 commit 479ee08
Show file tree
Hide file tree
Showing 12 changed files with 452 additions and 248 deletions.
3 changes: 1 addition & 2 deletions docs/configuration.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -469,8 +469,7 @@ See https://www.owasp.org/index.php/Information_exposure_through_query_strings_i
*`Cookie` header sanitization:*

The `Cookie` header is automatically redacted for incoming HTTP request transactions. Each name-value pair from the
Cookie header is parsed by the agent and sent to the APM Server. Before the name-value pairs are recorded, they are
sanitized based on the `SanitizeFieldNames` configuration. Cookies with sensitive data in
cookie list is parsed by the agent and sanitized based on the `SanitizeFieldNames` configuration. Cookies with sensitive data in
their value can be redacted by adding the cookie's name to the comma-separated list.

[options="header"]
Expand Down
7 changes: 0 additions & 7 deletions src/Elastic.Apm/Api/Request.cs
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,6 @@ public class Request
/// </summary>
public Dictionary<string, string> Headers { get; set; }

/// <summary>
/// This field is sanitized by a filter
/// </summary>
public Dictionary<string, string> Cookies { get; set; }

[JsonProperty("http_version")]
[MaxLength]
public string HttpVersion { get; set; }
Expand All @@ -47,8 +42,6 @@ internal Request DeepCopy()
var newItem = (Request)MemberwiseClone();
if (Headers != null)
newItem.Headers = Headers.ToDictionary(entry => entry.Key, entry => entry.Value);
if (Cookies != null)
newItem.Cookies = Cookies.ToDictionary(entry => entry.Key, entry => entry.Value);

newItem.Socket = Socket?.DeepCopy();
newItem.Url = Url?.DeepCopy();
Expand Down
91 changes: 91 additions & 0 deletions src/Elastic.Apm/Filters/CookieHeaderRedactionFilter.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
// 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;
using System.Collections;
using System.Collections.Generic;
using Elastic.Apm.Api;
using Elastic.Apm.Config;
using Elastic.Apm.Helpers;
using Elastic.Apm.Model;
#if NET6_0_OR_GREATER
using System.Buffers;
#endif

namespace Elastic.Apm.Filters
{
/// <summary>
/// Redacts items from the cookie list of the Cookie request header.
/// </summary>
internal class CookieHeaderRedactionFilter
{
private const string CookieHeader = "Cookie";

public static IError Filter(IError error)
{
if (error is Error e && e.Context is not null)
HandleCookieHeader(e.Context?.Request?.Headers, e.Configuration.SanitizeFieldNames);
return error;
}

public static ITransaction Filter(ITransaction transaction)
{
if (transaction is Transaction { IsContextCreated: true })
HandleCookieHeader(transaction.Context?.Request?.Headers, transaction.Configuration.SanitizeFieldNames);
return transaction;
}

// internal for testing
internal static void HandleCookieHeader(Dictionary<string, string> headers, IReadOnlyList<WildcardMatcher> sanitizeFieldNames)
{
if (headers is not null)
{
// Realistically, this should be more than enough for all sensible scenarios
// e.g. Cookies | cookies | COOKIES
const int maxKeys = 4;

#if NET6_0_OR_GREATER
var matchedKeys = ArrayPool<string>.Shared.Rent(maxKeys);
var matchedValues = ArrayPool<string>.Shared.Rent(maxKeys);
#else
var matchedKeys = new string[maxKeys];
var matchedValues = new string[maxKeys];
#endif
var matchedCount = 0;

foreach (var header in headers)
{
if (matchedCount == maxKeys)
break;

if (header.Key.Equals(CookieHeader, StringComparison.OrdinalIgnoreCase))
{
matchedKeys[matchedCount] = header.Key;
matchedValues[matchedCount] = CookieHeaderRedacter.Redact(header.Value, sanitizeFieldNames);
matchedCount++;
}
}

var replacedCount = 0;

foreach (var headerKey in matchedKeys)
{
if (replacedCount == matchedCount)
break;

if (headerKey is not null)
{
headers[headerKey] = matchedValues[replacedCount];
replacedCount++;
}
}

#if NET6_0_OR_GREATER
ArrayPool<string>.Shared.Return(matchedKeys);
ArrayPool<string>.Shared.Return(matchedValues);
#endif
}
}
}
}
53 changes: 0 additions & 53 deletions src/Elastic.Apm/Filters/RequestCookieExtractionFilter.cs

This file was deleted.

3 changes: 0 additions & 3 deletions src/Elastic.Apm/Filters/Sanitization.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,6 @@ public static void SanitizeHeadersInContext(Context context, IConfiguration conf
if (context?.Request?.Headers is not null)
RedactMatches(context?.Request?.Headers, configuration);

if (context?.Request?.Cookies is not null)
RedactMatches(context?.Request?.Cookies, configuration);

if (context?.Response?.Headers is not null)
RedactMatches(context?.Response?.Headers, configuration);

Expand Down
108 changes: 0 additions & 108 deletions src/Elastic.Apm/Helpers/CookieHeaderParser.cs

This file was deleted.

Loading

0 comments on commit 479ee08

Please sign in to comment.