Activate and expose the WAF's obfuscator by robertpi · Pull Request #2696 · DataDog/dd-trace-dotnet

robertpi · 2022-04-19T09:38:06Z

Summary of changes

The WAF now comes with a build in obfuscator based on RE2 patterns to
scan the output of the WAF for possible sensitive.

This PR activates that functionality by setting the patterns to the
standard values.

It also allows users to control the obfuscation via two environment
variables: DD_APPSEC_OBFUSCATION_PARAMETER_KEY_REGEXP and
DD_APPSEC_OBFUSCATION_PARAMETER_VALUE_REGEXP (and by user all the
other standard ways of setting config in .NET)

Test coverage

Includes unit tests, this functionality is also covered by the system-tests.

andrewlock · 2022-04-19T09:55:53Z

+        [InlineData(true, "pwd", "select pg_sleep", "select pg_sleep")]
+        public void AttacksWithSecrets(bool obfuscate, string key, string fullAttack, string highlight)
+        {
+            Environment.SetEnvironmentVariable("DD_TRACE_DEBUG", "true");


I don't think we should set this here if possible, as this is changing the global environment variables, and could conflict with other tests? Doesn't look required AFAICT

To be honest, I just copied and pasted it from another WAF test. I agree, it probably going to slow things and could create issues with other tests, so I the line from both places in 15eff67.

The debug variable is still being set here. Maybe you committed before saving this file?

Okay, really fixed in c66cff5. Yes, can't have saved the file.

zacharycmontoya · 2022-04-19T21:19:26Z

+            using var waf =
+                    obfuscate
+                        ? Waf.Create(SecuritySettings.ObfuscationParameterKeyRegexDefault, SecuritySettings.ObfuscationParameterValueRegexDefault)
+                        : Waf.Create(string.Empty, string.Empty);


I could see Waf.Create making the obfuscation parameters optional and defaulting to "" if not provided

I did think about making the new parameters optional, but it seemed to make the meaning of the API clearer to keep them mandatory. I think I'll leave them as they are for now, and come back to this, as I'm planning to start on a refactoring of the Security class soon. Having three string parameters with different meanings isn't great, so maybe I'll wrap them in a class / struct.

andrewlock · 2022-04-19T21:53:10Z

Benchmarks Report 🐌

Benchmarks for #2696 compared to master:

3 benchmarks are slower, with geometric mean 1.216
All benchmarks have the same 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	1.03ms	5.33μs	24.4μs	3.16 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	718μs	3.5μs	18.5μs	2.57 KB
#2696	`WriteAndFlushEnrichedTraces`	net472	936μs	3.83μs	14.3μs	3.16 KB
#2696	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	738μs	4.32μs	40.1μs	2.57 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net472	313ns	1.59ns	6.93ns	0.066	0	417 B
master	`AllCycleSimpleBody`	netcoreapp3.1	415ns	1.55ns	6.01ns	0.0058	0	416 B
master	`AllCycleMoreComplexBody`	net472	307ns	1.38ns	5.18ns	0.0622	0	393 B
master	`AllCycleMoreComplexBody`	netcoreapp3.1	396ns	1.29ns	4.84ns	0.00535	0	392 B
master	`BodyExtractorSimpleBody`	net472	454ns	2.31ns	11.1ns	0.0569	0	361 B
master	`BodyExtractorSimpleBody`	netcoreapp3.1	447ns	1.97ns	7.62ns	0.00375	0	272 B
master	`BodyExtractorMoreComplexBody`	net472	24μs	87.5ns	316ns	1.18	0.0122	7.62 KB
master	`BodyExtractorMoreComplexBody`	netcoreapp3.1	21μs	75ns	291ns	0.0952	0	6.75 KB
#2696	`AllCycleSimpleBody`	net472	325ns	1.73ns	9.29ns	0.066	0	417 B
#2696	`AllCycleSimpleBody`	netcoreapp3.1	415ns	2.03ns	8.11ns	0.00577	0	416 B
#2696	`AllCycleMoreComplexBody`	net472	317ns	1.68ns	8.39ns	0.0621	0	393 B
#2696	`AllCycleMoreComplexBody`	netcoreapp3.1	400ns	1.57ns	6.1ns	0.00545	0	392 B
#2696	`BodyExtractorSimpleBody`	net472	463ns	2.57ns	15.4ns	0.057	0	361 B
#2696	`BodyExtractorSimpleBody`	netcoreapp3.1	450ns	2.35ns	11.3ns	0.00384	0	272 B
#2696	`BodyExtractorMoreComplexBody`	net472	25.2μs	134ns	849ns	1.19	0.012	7.62 KB
#2696	`BodyExtractorMoreComplexBody`	netcoreapp3.1	21.4μs	102ns	420ns	0.094	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	295μs	1.4μs	5.44μs	0.293	19.74 KB
#2696	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#2696	`SendRequest`	netcoreapp3.1	309μs	1.64μs	8.67μs	0.15	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.7μs	8.34ns	33.4ns	0.0941	0	594 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.6μs	9.29ns	73.7ns	0.00849	0	632 B
#2696	`ExecuteNonQuery`	net472	1.78μs	9.97ns	62.3ns	0.0934	0.000963	594 B
#2696	`ExecuteNonQuery`	netcoreapp3.1	1.54μs	8.27ns	42.1ns	0.00897	0	632 B

Benchmarks.Trace.ElasticsearchBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #2696

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearch‑net472	1.307	2,506.26	3,275.09

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`CallElasticsearch`	net472	2.51μs	12.2ns	48.8ns	0.127	0	803 B
master	`CallElasticsearch`	netcoreapp3.1	1.68μs	6.15ns	23ns	0.0109	0	792 B
master	`CallElasticsearchAsync`	net472	2.76μs	11.8ns	45.7ns	0.148	0.00135	939 B
master	`CallElasticsearchAsync`	netcoreapp3.1	2.11μs	10.9ns	50ns	0.0124	0	912 B
#2696	`CallElasticsearch`	net472	3.29μs	13.2ns	49.4ns	0.127	0	803 B
#2696	`CallElasticsearch`	netcoreapp3.1	1.73μs	9.89ns	70ns	0.0108	0	792 B
#2696	`CallElasticsearchAsync`	net472	3.01μs	17.4ns	132ns	0.146	0	939 B
#2696	`CallElasticsearchAsync`	netcoreapp3.1	1.92μs	9.66ns	52ns	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μs	14.2ns	60.3ns	0.168	0.00147	1.06 KB
master	`ExecuteAsync`	netcoreapp3.1	2μs	11.3ns	75.1ns	0.0143	0	1.03 KB
#2696	`ExecuteAsync`	net472	3.04μs	10.1ns	36.3ns	0.168	0.0015	1.06 KB
#2696	`ExecuteAsync`	netcoreapp3.1	2.03μs	9.76ns	42.5ns	0.0152	0	1.03 KB

Benchmarks.Trace.HttpClientBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #2696

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.HttpClientBenchmark.SendAsync‑net472	1.145	6,813.95	7,803.63

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net472	6.79μs	33.6ns	138ns	0.361	2.28 KB
master	`SendAsync`	netcoreapp3.1	4.68μs	16.8ns	65.2ns	0.0301	2.21 KB
#2696	`SendAsync`	net472	7.83μs	40.4ns	258ns	0.356	2.28 KB
#2696	`SendAsync`	netcoreapp3.1	5.18μs	27.4ns	134ns	0.0303	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.59μs	19.8ns	121ns	0.227	1.45 KB
master	`EnrichedLog`	netcoreapp3.1	3.15μs	17.4ns	99.9ns	0.0213	1.53 KB
#2696	`EnrichedLog`	net472	3.38μs	16.4ns	71.6ns	0.228	1.45 KB
#2696	`EnrichedLog`	netcoreapp3.1	3.29μs	18.5ns	123ns	0.0215	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	284μs	1.33μs	4.97μs	0.441	0.147	4.33 KB
master	`EnrichedLog`	netcoreapp3.1	227μs	745ns	2.79μs	0	0	4.21 KB
#2696	`EnrichedLog`	net472	287μs	1.44μs	7.21μs	0.445	0.148	4.33 KB
#2696	`EnrichedLog`	netcoreapp3.1	235μs	1.28μs	7.68μs	0	0	4.21 KB

Benchmarks.Trace.NLogBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #2696

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.NLogBenchmark.EnrichedLog‑net472	1.202	7,306.12	8,783.99

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	7.32μs	30.3ns	109ns	0.506	3.23 KB
master	`EnrichedLog`	netcoreapp3.1	6.4μs	30.2ns	117ns	0.0507	3.6 KB
#2696	`EnrichedLog`	net472	8.92μs	51.9ns	437ns	0.502	3.23 KB
#2696	`EnrichedLog`	netcoreapp3.1	6.52μs	17.8ns	64.2ns	0.0494	3.6 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`SendReceive`	net472	2.42μs	9.33ns	37.3ns	0.16	0.00118	1.01 KB
master	`SendReceive`	netcoreapp3.1	1.96μs	6.33ns	24.5ns	0.0136	0	1.01 KB
#2696	`SendReceive`	net472	2.41μs	9.43ns	35.3ns	0.161	0	1.01 KB
#2696	`SendReceive`	netcoreapp3.1	2.08μs	11.2ns	60.1ns	0.014	0	1.01 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	6.61μs	37.2ns	244ns	0.292	1.87 KB
master	`EnrichedLog`	netcoreapp3.1	5.19μs	22.7ns	84.8ns	0.0207	1.49 KB
#2696	`EnrichedLog`	net472	6.16μs	35ns	245ns	0.291	1.87 KB
#2696	`EnrichedLog`	netcoreapp3.1	5.21μs	27ns	133ns	0.0205	1.49 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net472	924ns	4.42ns	18.7ns	0.0719	457 B
master	`StartFinishSpan`	netcoreapp3.1	932ns	5.31ns	39.1ns	0.00618	456 B
master	`StartFinishScope`	net472	1.06μs	5.42ns	24.8ns	0.0845	538 B
master	`StartFinishScope`	netcoreapp3.1	1.08μs	6.26ns	53.5ns	0.00805	576 B
#2696	`StartFinishSpan`	net472	986ns	4.65ns	20.8ns	0.0719	457 B
#2696	`StartFinishSpan`	netcoreapp3.1	884ns	3.05ns	10.6ns	0.00603	456 B
#2696	`StartFinishScope`	net472	1.18μs	6.65ns	52.3ns	0.0841	538 B
#2696	`StartFinishScope`	netcoreapp3.1	1.03μs	4.96ns	20.4ns	0.00768	576 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net472	1.36μs	7.31ns	42ns	0.0843	538 B
master	`RunOnMethodBegin`	netcoreapp3.1	1.26μs	6.62ns	31.7ns	0.0075	576 B
#2696	`RunOnMethodBegin`	net472	1.27μs	6.05ns	30.2ns	0.084	538 B
#2696	`RunOnMethodBegin`	netcoreapp3.1	1.18μs	5.54ns	22.9ns	0.00769	576 B

The WAF now comes with a build in obfuscator based on RE2 patterns to scan the output of the WAF for possible sensitive. This PR activates that functionality by setting the patterns to the standard values. It also allows users to control the obfuscation via two environment variables: `DD_APPSEC_OBFUSCATION_PARAMETER_KEY_REGEXP` and `DD_APPSEC_OBFUSCATION_PARAMETER_VALUE_REGEXP` (and by user all the other standard ways of setting config in .NET)

andrewlock · 2022-04-20T15:04:39Z

Code Coverage Report 📊

⚠️ Merging #2696 into master will will decrease line coverage by 1%
⚠️ Merging #2696 into master will will decrease branch coverage by 1%
⛔ Merging #2696 into master will will increase complexity by 250

	master	#2696	Change
Lines	`13549` / `18651`	`13654` / `18936`
Lines %	`73%`	`72%`	`-1%` ⚠️
Branches	`7821` / `11131`	`7882` / `11355`
Branches %	`70%`	`69%`	`-1%` ⚠️
Complexity	`12416`	`12666`	`250` ⛔

View the full report for further details:

Datadog.Trace Breakdown ⚠️

	master	#2696	Change
Lines %	`73%`	`72%`	`-1%` ⚠️
Branches %	`70%`	`69%`	`-1%` ⚠️
Complexity	`12416`	`12666`	`250` ⛔

The following classes have significant coverage changes.

File	Line coverage change	Branch coverage change	Complexity change
Datadog.Trace.TracerManager	`-30%` ⛔	`-34%` ⛔	`88` ⛔
Datadog.Trace.TracerManagerFactory	`-28%` ⛔	`-37%` ⛔	`60` ⛔
Datadog.Trace.AppSec.Waf.Waf	`-17%` ⛔	`-12%` ⛔	`9` ⛔
Datadog.Trace.AppSec.SecuritySettings	`-16%` ⛔	`-13%` ⛔	`46` ⛔
Datadog.Trace.AppSec.Waf.Initialization.WafConfigurator	`-16%` ⛔	`-12%` ⛔	`40` ⛔
Datadog.Trace.SpanExtensions	`-5%` ⛔	`-8%` ⛔	`0` ✔️
Datadog.Trace.Ci.GitInfo	`5%` ✔️	`7%` ✔️	`0` ✔️
Datadog.Trace.Ci.CIVisibility	`7%` ✔️	`7%` ✔️	`0` ✔️

View the full reports for further details:

robertpi requested a review from a team as a code owner April 19, 2022 09:38

andrewlock approved these changes Apr 19, 2022

View reviewed changes

robertpi force-pushed the appsec/robert/obfuscation branch from 3d880e9 to 97561d1 Compare April 19, 2022 11:14

This comment has been minimized.

Sign in to view

zacharycmontoya reviewed Apr 19, 2022

View reviewed changes

Comment thread tracer/src/Datadog.Trace/AppSec/Waf/Waf.cs

zacharycmontoya reviewed Apr 19, 2022

View reviewed changes

Comment thread tracer/src/Datadog.Trace/AppSec/Waf/Waf.cs

zacharycmontoya reviewed Apr 19, 2022

View reviewed changes

Comment thread tracer/src/Datadog.Trace/AppSec/SecuritySettings.cs

zacharycmontoya reviewed Apr 19, 2022

View reviewed changes

Comment thread tracer/src/Datadog.Trace/AppSec/SecuritySettings.cs

zacharycmontoya approved these changes Apr 19, 2022

View reviewed changes

robertpi added 4 commits April 20, 2022 14:12

Correction required after rebase on master

10cc4c5

Stop setting DD_TRACE_DEBUG

cdec6ba

Remove Environment.SetEnvironmentVariable("DD_TRACE_DEBUG", "true");

5024f6b

robertpi force-pushed the appsec/robert/obfuscation branch from 93a3b44 to 5024f6b Compare April 20, 2022 12:13

Correct build issue caused by rebase

c1d2851

robertpi merged commit 9024d66 into master Apr 20, 2022

robertpi deleted the appsec/robert/obfuscation branch April 20, 2022 15:23

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

andrewlock added the area:app-sec label Apr 22, 2022

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

Uh oh!

Conversation

robertpi commented Apr 19, 2022

Summary of changes

Test coverage

Uh oh!

Uh oh!

Uh oh!

andrewlock Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

robertpi Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

zacharycmontoya Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

robertpi Apr 20, 2022

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

zacharycmontoya Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

robertpi Apr 20, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

andrewlock commented Apr 19, 2022

Benchmarks Report 🐌

Benchmark details

Raw results

Raw results

Raw results

Raw results

Slower ⚠️ in #2696

Raw results

Raw results

Slower ⚠️ in #2696

Raw results

Raw results

Raw results

Slower ⚠️ in #2696

Raw results

Raw results

Raw results

Raw results

Raw results

Uh oh!

andrewlock commented Apr 20, 2022

Code Coverage Report 📊

Datadog.Trace Breakdown ⚠️

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants