[Appsec] Call waf only once, add path params address by anna-git · Pull Request #2676 · DataDog/dd-trace-dotnet

anna-git · 2022-04-12T17:44:34Z

Summary of changes

Building up all the addresses and calling the waf only in the end
Adding an address that was accidentally removed to the waf, the path parameters of an mvc route.

Reason for change

Improvement on performances only calling waf once
Some path parameters attacks arent caught because route data params werent forwarded

Implementation details

Change instrumentation for asp.net, take advantage of the same instrumentation as the body one
Adapt integration tests with names per scenario

Test coverage

Other details

andrewlock · 2022-04-13T05:01:32Z

Benchmarks Report 🐌

Benchmarks for #2676 compared to master:

1 benchmarks are faster, with geometric mean 1.123
4 benchmarks have more allocations

The following thresholds were used for comparing the benchmark speeds:

Mann–Whitney U test with statistical test for significance of 5%
Only results indicating a difference greater than 10% and 0.3 ns are considered.

Allocation changes below 0.5% are ignored.

Benchmark details

Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Allocated
master	`WriteAndFlushEnrichedTraces`	net472	929μs	4.56μs	18.8μs	3.16 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	719μs	4.03μs	27.9μs	2.57 KB
#2676	`WriteAndFlushEnrichedTraces`	net472	921μs	4.2μs	16.3μs	3.17 KB
#2676	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	689μs	3.05μs	11.4μs	2.57 KB

Benchmarks.Trace.AppSecBodyBenchmark - Same speed ✔️ More allocations ⚠️

More allocations ⚠️ in #2676

Benchmark	Base Allocated	Diff Allocated	Change	Change %
Benchmarks.Trace.AppSecBodyBenchmark.AllCycleMoreComplexBody‑netcoreapp3.1	304 B	392 B	88 B	28.95%
Benchmarks.Trace.AppSecBodyBenchmark.AllCycleMoreComplexBody‑net472	305 B	393 B	88 B	28.85%
Benchmarks.Trace.AppSecBodyBenchmark.AllCycleSimpleBody‑netcoreapp3.1	328 B	416 B	88 B	26.83%
Benchmarks.Trace.AppSecBodyBenchmark.AllCycleSimpleBody‑net472	329 B	417 B	88 B	26.75%

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net472	287ns	1.39ns	5.55ns	0.0519	0	329 B
master	`AllCycleSimpleBody`	netcoreapp3.1	373ns	1.48ns	5.72ns	0.00444	0	328 B
master	`AllCycleMoreComplexBody`	net472	288ns	1.54ns	8.59ns	0.0481	0	305 B
master	`AllCycleMoreComplexBody`	netcoreapp3.1	377ns	1.8ns	8.45ns	0.00427	0	304 B
master	`BodyExtractorSimpleBody`	net472	424ns	1.92ns	6.94ns	0.0569	0	361 B
master	`BodyExtractorSimpleBody`	netcoreapp3.1	434ns	1.8ns	7.2ns	0.00361	0	272 B
master	`BodyExtractorMoreComplexBody`	net472	23.9μs	97.7ns	378ns	1.19	0.0118	7.62 KB
master	`BodyExtractorMoreComplexBody`	netcoreapp3.1	21.8μs	102ns	396ns	0.0945	0	6.75 KB
#2676	`AllCycleSimpleBody`	net472	310ns	1.11ns	4.17ns	0.066	0	417 B
#2676	`AllCycleSimpleBody`	netcoreapp3.1	405ns	1.28ns	4.6ns	0.00575	0	416 B
#2676	`AllCycleMoreComplexBody`	net472	305ns	1.6ns	7.49ns	0.0623	0	393 B
#2676	`AllCycleMoreComplexBody`	netcoreapp3.1	393ns	1.61ns	6.24ns	0.00537	0	392 B
#2676	`BodyExtractorSimpleBody`	net472	440ns	2.29ns	11.9ns	0.057	0	361 B
#2676	`BodyExtractorSimpleBody`	netcoreapp3.1	443ns	1.58ns	6.12ns	0.00369	0	272 B
#2676	`BodyExtractorMoreComplexBody`	net472	24.1μs	128ns	653ns	1.19	0.0116	7.62 KB
#2676	`BodyExtractorMoreComplexBody`	netcoreapp3.1	20.4μs	88.3ns	342ns	0.0919	0	6.75 KB

Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
master	`SendRequest`	netcoreapp3.1	289μs	1.37μs	5.29μs	0.144	19.74 KB
#2676	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#2676	`SendRequest`	netcoreapp3.1	300μs	1.58μs	8.38μs	0.147	19.74 KB

Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`ExecuteNonQuery`	net472	1.92μs	10.8ns	74.9ns	0.0944	0.001	594 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.55μs	8.38ns	59.8ns	0.00904	0	632 B
#2676	`ExecuteNonQuery`	net472	1.81μs	9.76ns	51.7ns	0.0944	0	594 B
#2676	`ExecuteNonQuery`	netcoreapp3.1	1.54μs	7.18ns	26.9ns	0.00844	0	632 B

Benchmarks.Trace.ElasticsearchBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #2676

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearchAsync‑netcoreapp3.1	1.123	1,939.38	1,726.43

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`CallElasticsearch`	net472	2.77μs	15ns	83.5ns	0.127	0	803 B
master	`CallElasticsearch`	netcoreapp3.1	1.66μs	8.07ns	35.2ns	0.0106	0	792 B
master	`CallElasticsearchAsync`	net472	2.91μs	15ns	67ns	0.149	0.00145	939 B
master	`CallElasticsearchAsync`	netcoreapp3.1	1.96μs	10.8ns	66.6ns	0.0124	0	912 B
#2676	`CallElasticsearch`	net472	2.71μs	12.7ns	49.2ns	0.127	0	803 B
#2676	`CallElasticsearch`	netcoreapp3.1	1.71μs	6.31ns	23.6ns	0.011	0	792 B
#2676	`CallElasticsearchAsync`	net472	3.11μs	16ns	76.9ns	0.149	0.00155	939 B
#2676	`CallElasticsearchAsync`	netcoreapp3.1	1.73μs	6.49ns	25.1ns	0.0129	0	912 B

Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`ExecuteAsync`	net472	3.04μs	15.8ns	73.9ns	0.168	0.00149	1.06 KB
master	`ExecuteAsync`	netcoreapp3.1	1.94μs	7.76ns	29ns	0.0146	0	1.03 KB
#2676	`ExecuteAsync`	net472	3.07μs	16ns	80ns	0.168	0.00152	1.06 KB
#2676	`ExecuteAsync`	netcoreapp3.1	2.09μs	11.2ns	60.2ns	0.0137	0	1.03 KB

Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net472	6.71μs	33.5ns	153ns	0.36	2.28 KB
master	`SendAsync`	netcoreapp3.1	4.75μs	23ns	94.9ns	0.0306	2.21 KB
#2676	`SendAsync`	net472	6.72μs	29.8ns	111ns	0.359	2.28 KB
#2676	`SendAsync`	netcoreapp3.1	4.82μs	19.6ns	78.6ns	0.03	2.21 KB

Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	3.43μs	19.8ns	157ns	0.228	1.45 KB
master	`EnrichedLog`	netcoreapp3.1	3.03μs	10.7ns	39.9ns	0.0206	1.53 KB
#2676	`EnrichedLog`	net472	3.48μs	19ns	113ns	0.228	1.45 KB
#2676	`EnrichedLog`	netcoreapp3.1	3.1μs	14.7ns	78.9ns	0.0219	1.53 KB

Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	293μs	1.44μs	5.93μs	0.434	0.145	4.33 KB
master	`EnrichedLog`	netcoreapp3.1	227μs	864ns	3.23μs	0	0	4.21 KB
#2676	`EnrichedLog`	net472	296μs	1.65μs	10.7μs	0.45	0.15	4.33 KB
#2676	`EnrichedLog`	netcoreapp3.1	233μs	1.1μs	4.39μs	0	0	4.21 KB

Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	7.28μs	34.4ns	154ns	0.506	3.23 KB
master	`EnrichedLog`	netcoreapp3.1	6.4μs	32ns	143ns	0.0476	3.6 KB
#2676	`EnrichedLog`	net472	7.83μs	28.4ns	106ns	0.505	3.23 KB
#2676	`EnrichedLog`	netcoreapp3.1	6.98μs	23.6ns	91.5ns	0.0482	3.6 KB

Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendReceive`	net472	2.37μs	11.1ns	41.4ns	0.16	1.01 KB
master	`SendReceive`	netcoreapp3.1	2.03μs	8.9ns	37.8ns	0.0134	1.01 KB
#2676	`SendReceive`	net472	2.27μs	10.3ns	43.6ns	0.161	1.01 KB
#2676	`SendReceive`	netcoreapp3.1	1.98μs	9.3ns	37.2ns	0.0139	1.01 KB

Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	5.91μs	24.1ns	90.2ns	0.293	1.87 KB
master	`EnrichedLog`	netcoreapp3.1	5.08μs	21.4ns	80.1ns	0.0203	1.49 KB
#2676	`EnrichedLog`	net472	6μs	31ns	152ns	0.29	1.87 KB
#2676	`EnrichedLog`	netcoreapp3.1	5.07μs	24.9ns	106ns	0.0198	1.49 KB

Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net472	887ns	4.44ns	22.7ns	0.0717	457 B
master	`StartFinishSpan`	netcoreapp3.1	927ns	5.27ns	37.3ns	0.0063	456 B
master	`StartFinishScope`	net472	1.12μs	5.97ns	31ns	0.0846	538 B
master	`StartFinishScope`	netcoreapp3.1	1.03μs	3.08ns	11.5ns	0.00799	576 B
#2676	`StartFinishSpan`	net472	867ns	2.94ns	10.2ns	0.0717	457 B
#2676	`StartFinishSpan`	netcoreapp3.1	900ns	5.19ns	42.5ns	0.00612	456 B
#2676	`StartFinishScope`	net472	1.13μs	5.78ns	25.8ns	0.0844	538 B
#2676	`StartFinishScope`	netcoreapp3.1	1.04μs	5.97ns	45.5ns	0.00765	576 B

Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net472	1.31μs	4.88ns	17.6ns	0.084	538 B
master	`RunOnMethodBegin`	netcoreapp3.1	1.16μs	3.06ns	11ns	0.00769	576 B
#2676	`RunOnMethodBegin`	net472	1.31μs	7.09ns	39.5ns	0.0841	538 B
#2676	`RunOnMethodBegin`	netcoreapp3.1	1.18μs	5.99ns	26.8ns	0.00786	576 B

robertpi · 2022-04-13T14:59:52Z

+        {
+            foreach (var item in args)
+            {
+                if (!_addresses.ContainsKey(item.Key))


Might make sense to replace the key rather than ignoring it?

yes fixed in ed56173
taking last value then

…trigger with ruleset 3.0

andrewlock · 2022-04-14T12:21:55Z

Code Coverage Report 📊

✔️ Merging #2676 into master will not change line coverage
✔️ Merging #2676 into master will not change branch coverage
✔️ Merging #2676 into master will will decrease complexity by 24

	master	#2676	Change
Lines	`13533` / `18627`	`13525` / `18633`
Lines %	`73%`	`73%`	`0%` ✔️
Branches	`7832` / `11125`	`7820` / `11123`
Branches %	`70%`	`70%`	`0%` ✔️
Complexity	`12431`	`12407`	`-24` ✔️

View the full report for further details:

Datadog.Trace Breakdown ✔️

	master	#2676	Change
Lines %	`73%`	`73%`	`0%` ✔️
Branches %	`70%`	`70%`	`0%` ✔️
Complexity	`12431`	`12407`	`-24` ✔️

The following classes have significant coverage changes.

File	Line coverage change	Branch coverage change	Complexity change
Datadog.Trace.Telemetry.JsonWebRequestTelemetryTransport	`-27%` ⛔	`-57%` ⛔	`0` ✔️
Datadog.Trace.Ci.GitInfo	`-17%` ⛔	`-11%` ⛔	`0` ✔️
Datadog.Trace.Util.Http.HttpRequestExtensions	`-3%` ⚠️	`-7%` ⛔	`-2` ✔️
Datadog.Trace.ClrProfiler.AspNetCore.DefaultModelBindingContext_SetResult_Integration	`0%` ✔️	`15%` ✔️	`0` ✔️
Datadog.Trace.AppSec.Waf.Context	`2%` ✔️	`8%` ✔️	`2` ⛔
Datadog.Trace.ClrProfiler.AutoInstrumentation.AspNet.AsyncControllerActionInvoker_BeginInvokeAction_Integration	`6%` ✔️	`8%` ✔️	`-2` ✔️
Datadog.Trace.Ci.CIVisibility	`7%` ✔️	`7%` ✔️	`0` ✔️
Datadog.Trace.AppSec.InstrumentationGateway	`18%` ✔️	`-14%` ⛔	`-15` ✔️

1 classes were removed from Datadog.Trace in #2676

View the full reports for further details:

pierotibou

LGTM

anna-git added the area:app-sec label Apr 12, 2022

github-actions Bot added the area-appsec label Apr 12, 2022

pierotibou removed the area-appsec label Apr 13, 2022

github-actions Bot added the area-appsec label Apr 13, 2022

anna-git added 5 commits April 13, 2022 11:07

asp net core path params and a first refactoring

74d1569

Add integration tests for aspnetcore

02f3d0e

Adding integration tests for path parameters

c851e95

Change logic putting value in keys

98ee92a

fix unit test

ef8f2a0

pierotibou removed the area-appsec label Apr 13, 2022

anna-git force-pushed the anna/appsec/add-path-params branch from f28fb1f to ef8f2a0 Compare April 13, 2022 09:23

github-actions Bot added the area:appsec label Apr 13, 2022

fix encoder for when logging cookies

d7cce15

anna-git mentioned this pull request Apr 13, 2022

Activate on test for .NET DataDog/system-tests#293

Merged

This comment has been minimized.

Sign in to view

anna-git marked this pull request as ready for review April 13, 2022 13:35

anna-git requested a review from a team as a code owner April 13, 2022 13:35

anna-git changed the title ~~[Appsec] Add path params address to the waf~~ [Appsec] Call waf only once, add path params address Apr 13, 2022

robertpi reviewed Apr 13, 2022

View reviewed changes

anna-git added 3 commits April 13, 2022 18:17

in a url such as ?arg&[$slice] > consider as keys

ae84077

response to comment

ed56173

fix snapshots for arg&[$slice], now that they become keys, it doesnt …

b2356ca

…trigger with ruleset 3.0

robertpi approved these changes Apr 14, 2022

View reviewed changes

pierotibou removed the area:appsec label Apr 15, 2022

pierotibou approved these changes Apr 15, 2022

View reviewed changes

Comment thread tracer/src/Datadog.Trace/Util/Http/HttpRequestExtensions.Framework.cs

Comment thread tracer/src/Datadog.Trace/Util/Http/HttpRequestExtensions.Framework.cs

robertpi merged commit ad335b3 into master Apr 19, 2022

robertpi deleted the anna/appsec/add-path-params branch April 19, 2022 07:50

github-actions Bot added this to the vNext milestone Apr 19, 2022

lucaspimentel added area:asm and removed area:app-sec labels Aug 25, 2022

Uh oh!

Conversation

anna-git commented Apr 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

Uh oh!

andrewlock commented Apr 13, 2022

Benchmarks Report 🐌

Benchmark details

Raw results

More allocations ⚠️ in #2676

Raw results

Raw results

Raw results

Faster 🎉 in #2676

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Uh oh!

This comment has been minimized.

robertpi Apr 13, 2022

Choose a reason for hiding this comment

Uh oh!

anna-git Apr 13, 2022

Choose a reason for hiding this comment

Uh oh!

andrewlock commented Apr 14, 2022

Code Coverage Report 📊

Datadog.Trace Breakdown ✔️

Uh oh!

pierotibou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

anna-git commented Apr 12, 2022 •

edited

Loading