-
Notifications
You must be signed in to change notification settings - Fork 142
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
[ASM] Send cookie values as single string to the WAF #6164
Changes from 3 commits
dc010ce
615aa0e
f26fa7b
64b49cd
be9408a
85635e6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,18 +7,20 @@ | |
#pragma warning disable CS0282 | ||
using System; | ||
using System.Collections.Generic; | ||
using System.IO; | ||
using System.IO.Compression; | ||
using System.Text; | ||
using Datadog.Trace.AppSec.Waf; | ||
using Datadog.Trace.AppSec.Waf.ReturnTypes.Managed; | ||
using Datadog.Trace.ExtensionMethods; | ||
using Datadog.Trace.Logging; | ||
using Datadog.Trace.Telemetry; | ||
using Datadog.Trace.Telemetry.Metrics; | ||
using Datadog.Trace.Vendors.MessagePack; | ||
using Datadog.Trace.Vendors.Newtonsoft.Json; | ||
using Datadog.Trace.Util; | ||
using Datadog.Trace.Vendors.Serilog.Events; | ||
#if !NETFRAMEWORK | ||
using System.Linq; | ||
using Microsoft.AspNetCore.Http; | ||
#else | ||
using System.Collections.Specialized; | ||
using System.Web; | ||
#endif | ||
|
||
namespace Datadog.Trace.AppSec.Coordinator; | ||
|
||
|
@@ -165,5 +167,84 @@ public void AddResponseHeadersToSpanAndCleanup() | |
_httpTransport.DisposeAdditiveContext(); | ||
} | ||
|
||
internal static Dictionary<string, object> ExtractCookiesFromRequest(HttpRequest request) | ||
{ | ||
var cookies = RequestDataHelper.GetCookies(request); | ||
var cookiesDic = new Dictionary<string, object>(); | ||
|
||
if (cookies != null) | ||
{ | ||
for (var i = 0; i < cookies.Count; i++) | ||
{ | ||
#if NETCOREAPP || NETSTANDARD | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe we could use both |
||
var cookie = cookies.ElementAt(i); | ||
var keyForDictionary = cookie.Key ?? string.Empty; | ||
#else | ||
var cookie = cookies[i]; | ||
var keyForDictionary = cookie.Name ?? string.Empty; | ||
#endif | ||
if (!cookiesDic.TryGetValue(keyForDictionary, out var value)) | ||
{ | ||
cookiesDic.Add(keyForDictionary, cookie.Value ?? string.Empty); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know it was like this before, but wondering if we really want to send cookies with a null key and/or null value to the waf 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I think that we should exclude cookies with null keys or values. I have updated the code. |
||
} | ||
else | ||
{ | ||
if (value is string stringValue) | ||
{ | ||
cookiesDic[keyForDictionary] = new List<string> { stringValue, cookie.Value ?? string.Empty }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here, does it make sense to send an empty value to the list, especially if there are other actual values in the list already? |
||
} | ||
else if (value is List<string> valueList) | ||
{ | ||
valueList.Add(cookie.Value ?? string.Empty); | ||
} | ||
else | ||
{ | ||
Log.Warning("Cookie {Key} couldn't be added as argument to the waf", keyForDictionary); | ||
} | ||
} | ||
} | ||
} | ||
|
||
return cookiesDic; | ||
} | ||
|
||
#if NETFRAMEWORK | ||
internal static Dictionary<string, object> ExtractHeadersFromRequest(NameValueCollection headers) | ||
anna-git marked this conversation as resolved.
Show resolved
Hide resolved
|
||
#else | ||
internal static Dictionary<string, object> ExtractHeadersFromRequest(IHeaderDictionary headers) | ||
#endif | ||
{ | ||
var headersDic = new Dictionary<string, object>(headers.Keys.Count); | ||
foreach (string key in headers.Keys) | ||
{ | ||
var currentKey = key ?? string.Empty; | ||
if (!currentKey.Equals("cookie", System.StringComparison.OrdinalIgnoreCase)) | ||
{ | ||
currentKey = currentKey.ToLowerInvariant(); | ||
|
||
#if NETCOREAPP || NETSTANDARD | ||
var value = GetHeaderValueForWaf(headers[currentKey]); | ||
#else | ||
var value = GetHeaderValueForWaf(headers.GetValues(currentKey)); | ||
#endif | ||
#if NETCOREAPP | ||
if (!headersDic.TryAdd(currentKey, value)) | ||
{ | ||
#else | ||
if (!headersDic.ContainsKey(currentKey)) | ||
{ | ||
headersDic.Add(currentKey, value); | ||
} | ||
else | ||
{ | ||
#endif | ||
Log.Warning("Header {Key} couldn't be added as argument to the waf", currentKey); | ||
} | ||
} | ||
} | ||
|
||
return headersDic; | ||
} | ||
|
||
private static Span TryGetRoot(Span span) => span.Context.TraceContext?.RootSpan ?? span; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could instantiate it inside clause cookies != null to avoid an allocation in some cases and have a
Dictionary<string, object>?
as return type, especially that I see you check for null when the method is calledThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! Thanks!