Skip to content

Commit ebc42dd

Browse files
anna-gitanna-git
authored
[ASM] Introduce SecurityReporter for all reporting functions of SecurityCoordinator (#6481)
## Summary of changes This PR is just moving code around, to allow reporting security infos on spans without needing a security coordinator. Basically we are extracting from `SecurityCoordinator` the reporting functions to the already existing underused `SecurityReporter`, we are nesting core/framework classes inside of `SecurityCoordinator` and `SecurityReporter`. `SecurityReporter` can function without the `Security` instance, it just needs the span and transport to add info. This allows for better separation of concerns and prepares things for ATO as some contexts just need to report additional headers without running the waf necessarily, or already, within the catch (BlockException) inside the `BlockingMiddleware` where we just want to report the result within the exception and don't need any security features ## Reason for change ## Implementation details ## Test coverage ## Other details <!-- Fixes #{issue} --> <!-- ⚠️ Note: where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. -->
1 parent 2cdde8d commit ebc42dd

16 files changed

Lines changed: 286 additions & 222 deletions

‎tracer/src/Datadog.Trace/AppSec/Coordinator/SecurityCoordinator.Core.cs‎

Lines changed: 10 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,9 @@ private SecurityCoordinator(Security security, Span span, HttpTransport transpor
2626
_security = security;
2727
_localRootSpan = TryGetRoot(span);
2828
_httpTransport = transport;
29+
Reporter = new SecurityReporter(_localRootSpan, transport, true);
2930
}
3031

31-
private static bool CanAccessHeaders => true;
32-
3332
internal static SecurityCoordinator? TryGet(Security security, Span span)
3433
{
3534
var context = CoreHttpContextStore.Instance.Get();
@@ -59,27 +58,6 @@ private static void GetCookieKeyValueFromIndex(IRequestCookieCollection cookies,
5958
value = cookie.Value;
6059
}
6160

62-
[MethodImpl(MethodImplOptions.NoInlining)]
63-
internal static void CollectHeaders(Span internalSpan)
64-
{
65-
if (AspNetCoreAvailabilityChecker.IsAspNetCoreAvailable())
66-
{
67-
CollectHeadersImpl(internalSpan);
68-
}
69-
70-
[MethodImpl(MethodImplOptions.NoInlining)]
71-
static void CollectHeadersImpl(Span internalSpan)
72-
{
73-
var context = CoreHttpContextStore.Instance.Get();
74-
internalSpan = TryGetRoot(internalSpan);
75-
if (context is not null)
76-
{
77-
var headers = new HeadersCollectionAdapter(context.Request.Headers);
78-
AddRequestHeaders(internalSpan, headers);
79-
}
80-
}
81-
}
82-
8361
internal void BlockAndReport(IResult? result)
8462
{
8563
if (result is not null)
@@ -89,15 +67,15 @@ internal void BlockAndReport(IResult? result)
8967
throw new BlockException(result, result.RedirectInfo ?? result.BlockInfo!);
9068
}
9169

92-
TryReport(result, result.ShouldBlock);
70+
Reporter.TryReport(result, result.ShouldBlock);
9371
}
9472
}
9573

9674
internal void ReportAndBlock(IResult? result)
9775
{
9876
if (result is not null)
9977
{
100-
TryReport(result, result.ShouldBlock);
78+
Reporter.TryReport(result, result.ShouldBlock);
10179

10280
if (result.ShouldBlock)
10381
{
@@ -128,7 +106,13 @@ private Dictionary<string, object> GetBasicRequestArgsForWaf()
128106
}
129107
}
130108

131-
var addressesDictionary = new Dictionary<string, object> { { AddressesConstants.RequestMethod, request.Method }, { AddressesConstants.ResponseStatus, request.HttpContext.Response.StatusCode.ToString() }, { AddressesConstants.RequestUriRaw, request.GetUrlForWaf() }, { AddressesConstants.RequestClientIp, _localRootSpan.GetTag(Tags.HttpClientIp) } };
109+
var addressesDictionary = new Dictionary<string, object>
110+
{
111+
{ AddressesConstants.RequestMethod, request.Method },
112+
{ AddressesConstants.ResponseStatus, request.HttpContext.Response.StatusCode.ToString() },
113+
{ AddressesConstants.RequestUriRaw, request.GetUrlForWaf() },
114+
{ AddressesConstants.RequestClientIp, _localRootSpan.GetTag(Tags.HttpClientIp) }
115+
};
132116

133117
var userId = _localRootSpan.Context?.TraceContext?.Tags.GetTag(Tags.User.Id);
134118
if (!string.IsNullOrEmpty(userId))

‎tracer/src/Datadog.Trace/AppSec/Coordinator/SecurityCoordinator.Framework.cs‎

Lines changed: 8 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -27,33 +27,15 @@ internal readonly partial struct SecurityCoordinator
2727
private const string WebApiControllerHandlerTypeFullname = "System.Web.Http.WebHost.HttpControllerHandler";
2828
private static readonly Lazy<Action<IResult, HttpStatusCode, string>?> _throwHttpResponseRedirectException = new(CreateThrowHttpResponseExceptionDynMethForRedirect);
2929
private static readonly Lazy<Action<IResult, HttpStatusCode, string, string>?> _throwHttpResponseException = new(CreateThrowHttpResponseExceptionDynMeth);
30-
private static readonly bool? UsingIntegratedPipeline;
31-
32-
static SecurityCoordinator()
33-
{
34-
if (UsingIntegratedPipeline == null)
35-
{
36-
try
37-
{
38-
UsingIntegratedPipeline = TryGetUsingIntegratedPipelineBool();
39-
}
40-
catch (Exception ex)
41-
{
42-
UsingIntegratedPipeline = false;
43-
Log.Error(ex, "Unable to query the IIS pipeline. Request and response information may be limited.");
44-
}
45-
}
46-
}
4730

4831
private SecurityCoordinator(Security security, Span span, HttpTransport transport)
4932
{
5033
_security = security;
5134
_localRootSpan = TryGetRoot(span);
5235
_httpTransport = transport;
36+
Reporter = new SecurityReporter(_localRootSpan, transport, true);
5337
}
5438

55-
private bool CanAccessHeaders => UsingIntegratedPipeline is true or null;
56-
5739
internal static SecurityCoordinator? TryGet(Security security, Span span)
5840
{
5941
if (HttpContext.Current is not { } current)
@@ -288,7 +270,7 @@ internal void BlockAndReport(Dictionary<string, object> args, bool lastWafCall =
288270
var result = RunWaf(args, lastWafCall);
289271
if (result is not null)
290272
{
291-
var reporting = MakeReportingFunction(result);
273+
var reporting = Reporter.MakeReportingFunction(result);
292274

293275
if (result.ShouldBlock)
294276
{
@@ -304,7 +286,7 @@ internal void ReportAndBlock(IResult? result)
304286
{
305287
if (result is not null)
306288
{
307-
var reporting = MakeReportingFunction(result);
289+
var reporting = Reporter.MakeReportingFunction(result);
308290
reporting(null, result.ShouldBlock);
309291

310292
if (result.ShouldBlock)
@@ -323,20 +305,6 @@ internal void ReportAndBlock(IResult? result)
323305
}
324306
}
325307

326-
private Action<int?, bool> MakeReportingFunction(IResult result)
327-
{
328-
var securityCoordinator = this;
329-
return (status, blocked) =>
330-
{
331-
if (result.ShouldBlock)
332-
{
333-
securityCoordinator._httpTransport.MarkBlocked();
334-
}
335-
336-
securityCoordinator.TryReport(result, blocked, status);
337-
};
338-
}
339-
340308
private void ChooseBlockingMethodAndBlock(IResult result, Action<int?, bool> reporting, Dictionary<string, object?>? blockInfo, Dictionary<string, object?>? redirectInfo)
341309
{
342310
var headers = RequestDataHelper.GetHeaders(_httpTransport.Context.Request) ?? new NameValueCollection();
@@ -371,7 +339,7 @@ private void WriteAndEndResponse(BlockingAction blockingAction)
371339
httpResponse.Cookies.Clear();
372340

373341
// cant clear headers, on some iis version we get a platform not supported exception
374-
if (CanAccessHeaders)
342+
if (Reporter.CanAccessHeaders)
375343
{
376344
var keys = httpResponse.Headers.Keys.Cast<string>().ToList();
377345
foreach (var key in keys)
@@ -400,8 +368,8 @@ private void WriteAndEndResponse(BlockingAction blockingAction)
400368
public Dictionary<string, object> GetBasicRequestArgsForWaf()
401369
{
402370
var request = _httpTransport.Context.Request;
403-
var headers = RequestDataHelper.GetHeaders(request);
404-
var headersDic = ExtractHeadersFromRequest(request.Headers);
371+
var headers = request.Headers;
372+
var headersDic = ExtractHeaders(headers.AllKeys, key => GetHeaderValueForWaf(headers, key));
405373
var cookiesDic = ExtractCookiesFromRequest(request);
406374

407375
var queryString = RequestDataHelper.GetQueryString(request);
@@ -473,8 +441,6 @@ public Dictionary<string, object> GetBasicRequestArgsForWaf()
473441
return dict;
474442
}
475443

476-
internal static Dictionary<string, object> ExtractHeadersFromRequest(NameValueCollection headers) => ExtractHeaders(headers.AllKeys, key => GetHeaderValueForWaf(headers, key));
477-
478444
private static object GetHeaderAsArray(string[] value) => value.Length == 1 ? value[0] : value;
479445

480446
private static object GetHeaderValueForWaf(NameValueCollection headers, string currentKey) => GetHeaderAsArray(RequestDataHelper.GetNameValueCollectionValues(headers, currentKey) ?? []);
@@ -511,35 +477,19 @@ public Dictionary<string, object> GetResponseHeadersForWaf()
511477
return headersDic;
512478
}
513479

514-
internal static void CollectHeaders(Span internalSpan)
515-
{
516-
var context = HttpContext.Current;
517-
518-
if (context != null)
519-
{
520-
var headers = new NameValueHeadersCollection(context.Request.Headers);
521-
AddRequestHeaders(internalSpan, headers);
522-
}
523-
}
524-
525-
internal class HttpTransport : HttpTransportBase
480+
internal class HttpTransport(HttpContext context) : HttpTransportBase
526481
{
527482
private const string WafKey = "waf";
528483

529484
private static readonly IDatadogLogger Log = DatadogLogging.GetLoggerFor<HttpTransport>();
530485

531486
private static bool _canReadHttpResponseHeaders = true;
532487

533-
public HttpTransport(HttpContext context)
534-
{
535-
Context = context;
536-
}
537-
538488
internal override bool IsBlocked => Context.Items[BlockingAction.BlockDefaultActionName] is true;
539489

540490
internal override int StatusCode => Context.Response.StatusCode;
541491

542-
public override HttpContext Context { get; }
492+
public override HttpContext Context { get; } = context;
543493

544494
internal override IDictionary<string, object>? RouteData => Context.Request.RequestContext.RouteData?.Values;
545495

‎tracer/src/Datadog.Trace/AppSec/Coordinator/SecurityCoordinator.cs‎

Lines changed: 6 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -7,17 +7,12 @@
77
#pragma warning disable CS0282
88
using System;
99
using System.Collections.Generic;
10-
using System.Text;
1110
using Datadog.Trace.AppSec.Waf;
1211
using Datadog.Trace.Logging;
13-
using Datadog.Trace.Telemetry;
14-
using Datadog.Trace.Telemetry.Metrics;
1512
using Datadog.Trace.Util;
16-
using Datadog.Trace.Vendors.Serilog.Events;
1713
#if !NETFRAMEWORK
1814
using Microsoft.AspNetCore.Http;
1915
#else
20-
using System.Collections.Specialized;
2116
using System.Web;
2217
#endif
2318

@@ -36,32 +31,9 @@ internal readonly partial struct SecurityCoordinator
3631

3732
public bool IsBlocked => _httpTransport.IsBlocked;
3833

39-
public void MarkBlocked() => _httpTransport.MarkBlocked();
34+
public SecurityReporter Reporter { get; }
4035

41-
private static void LogMatchesIfDebugEnabled(IReadOnlyCollection<object>? results, bool blocked)
42-
{
43-
if (Log.IsEnabled(LogEventLevel.Debug) && results != null)
44-
{
45-
foreach (var result in results)
46-
{
47-
if (result is Dictionary<string, object?> match)
48-
{
49-
if (blocked)
50-
{
51-
Log.Debug("DDAS-0012-02: Blocking current transaction (rule: {RuleId})", match["rule"]);
52-
}
53-
else
54-
{
55-
Log.Debug("DDAS-0012-01: Detecting an attack from rule {RuleId}", match["rule"]);
56-
}
57-
}
58-
else
59-
{
60-
Log.Debug("{Result} not of expected type", result);
61-
}
62-
}
63-
}
64-
}
36+
public void MarkBlocked() => _httpTransport.MarkBlocked();
6537

6638
public IResult? Scan(bool lastTime = false)
6739
{
@@ -73,7 +45,7 @@ private static void LogMatchesIfDebugEnabled(IReadOnlyCollection<object>? result
7345

7446
public IResult? RunWaf(Dictionary<string, object> args, bool lastWafCall = false, bool runWithEphemeral = false, bool isRasp = false)
7547
{
76-
LogAddressIfDebugEnabled(args);
48+
SecurityReporter.LogAddressIfDebugEnabled(args);
7749
IResult? result = null;
7850
try
7951
{
@@ -108,7 +80,7 @@ private static void LogMatchesIfDebugEnabled(IReadOnlyCollection<object>? result
10880
result = additiveContext.Run(args, _security.Settings.WafTimeoutMicroSeconds);
10981
}
11082

111-
RecordTelemetry(result);
83+
SecurityReporter.RecordTelemetry(result);
11284
}
11385
}
11486
catch (Exception ex) when (ex is not BlockException)
@@ -130,46 +102,13 @@ private static void LogMatchesIfDebugEnabled(IReadOnlyCollection<object>? result
130102
return result;
131103
}
132104

133-
private static void RecordTelemetry(IResult? result)
134-
{
135-
if (result == null)
136-
{
137-
return;
138-
}
139-
140-
if (result.Timeout)
141-
{
142-
TelemetryFactory.Metrics.RecordCountWafRequests(MetricTags.WafAnalysis.WafTimeout);
143-
}
144-
else if (result.ShouldBlock)
145-
{
146-
TelemetryFactory.Metrics.RecordCountWafRequests(MetricTags.WafAnalysis.RuleTriggeredAndBlocked);
147-
}
148-
else if (result.ShouldReportSecurityResult)
149-
{
150-
TelemetryFactory.Metrics.RecordCountWafRequests(MetricTags.WafAnalysis.RuleTriggered);
151-
}
152-
else
153-
{
154-
TelemetryFactory.Metrics.RecordCountWafRequests(MetricTags.WafAnalysis.Normal);
155-
}
156-
}
157-
158-
public void AddResponseHeadersToSpanAndCleanup()
159-
{
160-
if (_localRootSpan.IsAppsecEvent())
161-
{
162-
AddResponseHeaderTags(CanAccessHeaders);
163-
}
164-
165-
_httpTransport.DisposeAdditiveContext();
166-
}
105+
internal static Span TryGetRoot(Span span) => span.Context.TraceContext?.RootSpan ?? span;
167106

168107
internal static Dictionary<string, object>? ExtractCookiesFromRequest(HttpRequest request)
169108
{
170109
var cookies = RequestDataHelper.GetCookies(request);
171110

172-
if (cookies is not null && cookies.Count is > 0)
111+
if (cookies is { Count: > 0 })
173112
{
174113
var cookiesCount = cookies.Count;
175114
var cookiesDic = new Dictionary<string, object>(cookiesCount);
@@ -234,6 +173,4 @@ private static Dictionary<string, object> ExtractHeaders(ICollection<string> key
234173

235174
return headersDic;
236175
}
237-
238-
private static Span TryGetRoot(Span span) => span.Context.TraceContext?.RootSpan ?? span;
239176
}

‎tracer/src/Datadog.Trace/AppSec/Coordinator/SecurityReporter.Core.cs‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
// <copyright file="SecurityReporter.Core.cs" company="Datadog">
2+
// Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License.
3+
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc.
4+
// </copyright>
5+
6+
#nullable enable
7+
#if !NETFRAMEWORK
8+
using System.Runtime.CompilerServices;
9+
using Datadog.Trace.Headers;
10+
11+
namespace Datadog.Trace.AppSec.Coordinator;
12+
13+
internal partial class SecurityReporter
14+
{
15+
private bool CanAccessHeaders => true;
16+
17+
/// <summary>
18+
/// Outside of a web context this can't work and there are no web assemblies to load so without the no inlining, this would cause a load assembly exception
19+
/// </summary>
20+
/// <param name="span">the span to report on</param>
21+
/// <param name="searchRootSpan">should we fetch the root span for you</param>
22+
internal static void SafeCollectHeaders(Span span, bool searchRootSpan = true)
23+
{
24+
if (AspNetCoreAvailabilityChecker.IsAspNetCoreAvailable())
25+
{
26+
CollectHeadersSafe(searchRootSpan);
27+
}
28+
29+
[MethodImpl(MethodImplOptions.NoInlining)]
30+
void CollectHeadersSafe(bool searchRootSpanImpl)
31+
{
32+
var context = CoreHttpContextStore.Instance.Get();
33+
if (context is not null)
34+
{
35+
var securityReporter = new SecurityReporter(span, new SecurityCoordinator.HttpTransport(context), !searchRootSpanImpl);
36+
securityReporter.CollectHeaders();
37+
}
38+
}
39+
}
40+
41+
internal void CollectHeaders()
42+
{
43+
var headers = new HeadersCollectionAdapter(_httpTransport.Context.Request.Headers);
44+
AddRequestHeaders(headers);
45+
}
46+
}
47+
#endif

0 commit comments

Comments
 (0)