[ASM][ATO] user id collection on authenticated request #6431
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Snapshots difference summaryThe following differences have been observed in committed snapshots. It is meant to help the reviewer. 6 occurrences of : + _dd.appsec.fp.http.header: hdr-0000000100-3626b5f8-2-da57b738,
+ _dd.appsec.fp.http.network: net-1-1000000000,
+ _dd.appsec.fp.session: ssn-<fingerprint>,
|
Datadog ReportBranch report: ✅ 0 Failed, 555193 Passed, 4591 Skipped, 47h 17m 17.27s Total Time |
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing the following branches/commits: Execution-time benchmarks measure the whole time it takes to execute a program. And are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are shown in red. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6431) - mean (69ms) : 66, 72
. : milestone, 69,
master - mean (69ms) : 64, 75
. : milestone, 69,
section CallTarget+Inlining+NGEN
This PR (6431) - mean (981ms) : 960, 1001
. : milestone, 981,
master - mean (978ms) : 954, 1001
. : milestone, 978,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6431) - mean (108ms) : 106, 110
. : milestone, 108,
master - mean (107ms) : 105, 110
. : milestone, 107,
section CallTarget+Inlining+NGEN
This PR (6431) - mean (679ms) : 662, 696
. : milestone, 679,
master - mean (681ms) : 667, 695
. : milestone, 681,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6431) - mean (91ms) : 89, 93
. : milestone, 91,
master - mean (91ms) : 87, 95
. : milestone, 91,
section CallTarget+Inlining+NGEN
This PR (6431) - mean (636ms) : 623, 650
. : milestone, 636,
master - mean (632ms) : 615, 650
. : milestone, 632,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6431) - mean (190ms) : 186, 194
. : milestone, 190,
master - mean (192ms) : 184, 200
. : milestone, 192,
section CallTarget+Inlining+NGEN
This PR (6431) - mean (1,094ms) : 1064, 1123
. : milestone, 1094,
master - mean (1,094ms) : 1058, 1130
. : milestone, 1094,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6431) - mean (278ms) : 273, 282
. : milestone, 278,
master - mean (276ms) : 272, 280
. : milestone, 276,
section CallTarget+Inlining+NGEN
This PR (6431) - mean (872ms) : 836, 907
. : milestone, 872,
master - mean (872ms) : 844, 900
. : milestone, 872,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6431) - mean (265ms) : 260, 270
. : milestone, 265,
master - mean (264ms) : 260, 268
. : milestone, 264,
section CallTarget+Inlining+NGEN
This PR (6431) - mean (848ms) : 814, 883
. : milestone, 848,
master - mean (849ms) : 811, 886
. : milestone, 849,
|
Benchmarks Report for appsec 🐌Benchmarks for #6431 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.Asm.AppSecBodyBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Asm.AppSecEncoderBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Asm.AppSecWafBenchmark - Same speed ✔️ More allocations
|
| Benchmark | Base Allocated | Diff Allocated | Change | Change % |
|---|---|---|---|---|
| Benchmarks.Trace.Asm.AppSecWafBenchmark.RunWafRealisticBenchmarkWithAttack‑netcoreapp3.1 | 1.46 KB | 1.53 KB | 72 B | 4.95% |
| Benchmarks.Trace.Asm.AppSecWafBenchmark.RunWafRealisticBenchmarkWithAttack‑net472 | 1.48 KB | 1.56 KB | 73 B | 4.92% |
| Benchmarks.Trace.Asm.AppSecWafBenchmark.RunWafRealisticBenchmarkWithAttack‑net6.0 | 1.47 KB | 1.54 KB | 72 B | 4.89% |
| Benchmarks.Trace.Asm.AppSecWafBenchmark.RunWafRealisticBenchmark‑netcoreapp3.1 | 2.39 KB | 2.46 KB | 72 B | 3.01% |
| Benchmarks.Trace.Asm.AppSecWafBenchmark.RunWafRealisticBenchmark‑net472 | 2.46 KB | 2.53 KB | 73 B | 2.97% |
| Benchmarks.Trace.Asm.AppSecWafBenchmark.RunWafRealisticBenchmark‑net6.0 | 2.44 KB | 2.51 KB | 72 B | 2.95% |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | RunWafRealisticBenchmark |
net6.0 | 175μs | 67.6ns | 253ns | 0 | 0 | 0 | 2.44 KB |
| master | RunWafRealisticBenchmark |
netcoreapp3.1 | 186μs | 241ns | 935ns | 0 | 0 | 0 | 2.39 KB |
| master | RunWafRealisticBenchmark |
net472 | 201μs | 157ns | 609ns | 0.302 | 0 | 0 | 2.46 KB |
| master | RunWafRealisticBenchmarkWithAttack |
net6.0 | 116μs | 82.5ns | 320ns | 0 | 0 | 0 | 1.47 KB |
| master | RunWafRealisticBenchmarkWithAttack |
netcoreapp3.1 | 123μs | 101ns | 349ns | 0 | 0 | 0 | 1.46 KB |
| master | RunWafRealisticBenchmarkWithAttack |
net472 | 133μs | 83.6ns | 324ns | 0.199 | 0 | 0 | 1.48 KB |
| #6431 | RunWafRealisticBenchmark |
net6.0 | 175μs | 310ns | 1.2μs | 0 | 0 | 0 | 2.51 KB |
| #6431 | RunWafRealisticBenchmark |
netcoreapp3.1 | 188μs | 189ns | 708ns | 0 | 0 | 0 | 2.46 KB |
| #6431 | RunWafRealisticBenchmark |
net472 | 200μs | 69.6ns | 260ns | 0.401 | 0 | 0 | 2.53 KB |
| #6431 | RunWafRealisticBenchmarkWithAttack |
net6.0 | 115μs | 146ns | 567ns | 0 | 0 | 0 | 1.54 KB |
| #6431 | RunWafRealisticBenchmarkWithAttack |
netcoreapp3.1 | 125μs | 144ns | 539ns | 0 | 0 | 0 | 1.53 KB |
| #6431 | RunWafRealisticBenchmarkWithAttack |
net472 | 133μs | 77.9ns | 291ns | 0.199 | 0 | 0 | 1.56 KB |
Benchmarks.Trace.Iast.StringAspectsBenchmark - Slower ⚠️ More allocations ⚠️
Slower ⚠️ in #6431
Benchmark
diff/base
Base Median (ns)
Diff Median (ns)
Modality
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net6.0
1.193
51,900.00
61,900.00
bimodal
More allocations ⚠️ in #6431
Benchmark
Base Allocated
Diff Allocated
Change
Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑netcoreapp3.1
252.53 KB
265.08 KB
12.55 KB
4.97%
Fewer allocations 🎉 in #6431
Benchmark
Base Allocated
Diff Allocated
Change
Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net6.0
264.31 KB
254.55 KB
-9.76 KB
-3.69%
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472
61.98 KB
59.12 KB
-2.86 KB
-4.62%
| Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net6.0 | 1.193 | 51,900.00 | 61,900.00 | bimodal |
| Benchmark | Base Allocated | Diff Allocated | Change | Change % |
|---|---|---|---|---|
| Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑netcoreapp3.1 | 252.53 KB | 265.08 KB | 12.55 KB | 4.97% |
| Benchmark | Base Allocated | Diff Allocated | Change | Change % |
|---|---|---|---|---|
| Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net6.0 | 264.31 KB | 254.55 KB | -9.76 KB | -3.69% |
| Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472 | 61.98 KB | 59.12 KB | -2.86 KB | -4.62% |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | StringConcatBenchmark |
net6.0 | 54.2μs | 498ns | 4.88μs | 0 | 0 | 0 | 43.44 KB |
| master | StringConcatBenchmark |
netcoreapp3.1 | 53.9μs | 280ns | 1.37μs | 0 | 0 | 0 | 42.64 KB |
| master | StringConcatBenchmark |
net472 | 36.9μs | 93.2ns | 323ns | 0 | 0 | 0 | 61.98 KB |
| master | StringConcatAspectBenchmark |
net6.0 | 319μs | 1.58μs | 10.1μs | 0 | 0 | 0 | 264.31 KB |
| master | StringConcatAspectBenchmark |
netcoreapp3.1 | 336μs | 1.85μs | 10.7μs | 0 | 0 | 0 | 252.53 KB |
| master | StringConcatAspectBenchmark |
net472 | 269μs | 4.94μs | 47.7μs | 0 | 0 | 0 | 278.53 KB |
| #6431 | StringConcatBenchmark |
net6.0 | 61.7μs | 683ns | 6.62μs | 0 | 0 | 0 | 43.44 KB |
| #6431 | StringConcatBenchmark |
netcoreapp3.1 | 54.5μs | 258ns | 1.34μs | 0 | 0 | 0 | 42.64 KB |
| #6431 | StringConcatBenchmark |
net472 | 37.4μs | 109ns | 409ns | 0 | 0 | 0 | 59.12 KB |
| #6431 | StringConcatAspectBenchmark |
net6.0 | 310μs | 1.69μs | 9.55μs | 0 | 0 | 0 | 254.55 KB |
| #6431 | StringConcatAspectBenchmark |
netcoreapp3.1 | 351μs | 1.91μs | 15.1μs | 0 | 0 | 0 | 265.08 KB |
| #6431 | StringConcatAspectBenchmark |
net472 | 280μs | 5.49μs | 53μs | 0 | 0 | 0 | 278.53 KB |
Throughput/Crank Report ⚡Throughput results for AspNetCoreSimpleController comparing the following branches/commits: Cases where throughput results for the PR are worse than latest master (5% drop or greater), results are shown in red. Note that these results are based on a single point-in-time result for each branch. For full results, see one of the many, many dashboards! gantt
title Throughput Linux x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6431) (11.158M) : 0, 11158228
master (11.434M) : 0, 11433823
benchmarks/2.9.0 (11.033M) : 0, 11032866
section Automatic
This PR (6431) (7.318M) : 0, 7318390
master (7.329M) : 0, 7329326
benchmarks/2.9.0 (7.786M) : 0, 7785853
section Trace stats
master (7.611M) : 0, 7611113
section Manual
master (11.108M) : 0, 11107912
section Manual + Automatic
This PR (6431) (6.842M) : 0, 6842127
master (6.845M) : 0, 6844945
section DD_TRACE_ENABLED=0
master (10.329M) : 0, 10328733
gantt
title Throughput Linux arm64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6431) (9.512M) : 0, 9511726
master (9.534M) : 0, 9533585
benchmarks/2.9.0 (9.495M) : 0, 9494821
section Automatic
This PR (6431) (6.335M) : 0, 6334952
master (6.293M) : 0, 6293263
section Trace stats
master (6.541M) : 0, 6541247
section Manual
master (9.502M) : 0, 9502053
section Manual + Automatic
This PR (6431) (5.936M) : 0, 5936063
master (5.976M) : 0, 5976365
section DD_TRACE_ENABLED=0
master (8.806M) : 0, 8806055
gantt
title Throughput Windows x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6431) (9.679M) : 0, 9678866
master (9.968M) : 0, 9968345
benchmarks/2.9.0 (10.020M) : 0, 10019592
section Automatic
This PR (6431) (6.254M) : 0, 6254041
master (6.506M) : 0, 6506205
benchmarks/2.9.0 (7.255M) : 0, 7255257
section Trace stats
master (7.120M) : 0, 7119839
section Manual
master (10.011M) : 0, 10010780
section Manual + Automatic
This PR (6431) (5.825M) : 0, 5825023
master (5.923M) : 0, 5922704
section DD_TRACE_ENABLED=0
master (9.290M) : 0, 9290361
|
Benchmarks Report for tracer 🐌Benchmarks for #6431 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.ActivityBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SpanBenchmark - Slower
|
| Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net6.0 | 1.134 | 409.69 | 464.64 | |
| Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑netcoreapp3.1 | 1.133 | 553.12 | 626.83 |
| Benchmark | base/diff | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net472 | 1.140 | 733.50 | 643.37 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | StartFinishSpan |
net6.0 | 410ns | 0.344ns | 1.33ns | 0.00805 | 0 | 0 | 576 B |
| master | StartFinishSpan |
netcoreapp3.1 | 553ns | 0.481ns | 1.86ns | 0.00772 | 0 | 0 | 576 B |
| master | StartFinishSpan |
net472 | 733ns | 0.207ns | 0.802ns | 0.0918 | 0 | 0 | 578 B |
| master | StartFinishScope |
net6.0 | 478ns | 0.228ns | 0.885ns | 0.00985 | 0 | 0 | 696 B |
| master | StartFinishScope |
netcoreapp3.1 | 729ns | 0.517ns | 2ns | 0.00945 | 0 | 0 | 696 B |
| master | StartFinishScope |
net472 | 896ns | 0.369ns | 1.38ns | 0.104 | 0 | 0 | 658 B |
| #6431 | StartFinishSpan |
net6.0 | 461ns | 1.7ns | 6.57ns | 0.00806 | 0 | 0 | 576 B |
| #6431 | StartFinishSpan |
netcoreapp3.1 | 627ns | 0.279ns | 1.04ns | 0.00788 | 0 | 0 | 576 B |
| #6431 | StartFinishSpan |
net472 | 643ns | 0.354ns | 1.37ns | 0.0918 | 0 | 0 | 578 B |
| #6431 | StartFinishScope |
net6.0 | 473ns | 0.245ns | 0.948ns | 0.0097 | 0 | 0 | 696 B |
| #6431 | StartFinishScope |
netcoreapp3.1 | 673ns | 0.434ns | 1.68ns | 0.00947 | 0 | 0 | 696 B |
| #6431 | StartFinishScope |
net472 | 824ns | 0.472ns | 1.77ns | 0.104 | 0 | 0 | 658 B |
Benchmarks.Trace.TraceAnnotationsBenchmark - Slower ⚠️ Same allocations ✔️
Slower ⚠️ in #6431
Benchmark
diff/base
Base Median (ns)
Diff Median (ns)
Modality
Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑netcoreapp3.1
1.130
853.29
964.34
| Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑netcoreapp3.1 | 1.130 | 853.29 | 964.34 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | RunOnMethodBegin |
net6.0 | 617ns | 0.573ns | 2.22ns | 0.00965 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
netcoreapp3.1 | 853ns | 0.454ns | 1.76ns | 0.0094 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
net472 | 1.11μs | 0.583ns | 2.26ns | 0.105 | 0 | 0 | 658 B |
| #6431 | RunOnMethodBegin |
net6.0 | 642ns | 0.445ns | 1.72ns | 0.00964 | 0 | 0 | 696 B |
| #6431 | RunOnMethodBegin |
netcoreapp3.1 | 964ns | 0.919ns | 3.56ns | 0.00953 | 0 | 0 | 696 B |
| #6431 | RunOnMethodBegin |
net472 | 1.12μs | 0.563ns | 2.18ns | 0.104 | 0 | 0 | 658 B |
anna-git
force-pushed
the
anna/asm/userid-monitoring
branch
3 times, most recently
from
cab8f96 to
10a0655
Compare
anna-git
force-pushed
the
anna/asm/userid-monitoring
branch
9 times, most recently
from
1fd1357 to
587d23b
Compare
anna-git
commented
Jan 15, 2025
| @@ -12,6 +12,7 @@ | |||
| "tracer\\test\\Datadog.Trace.ClrProfiler.IntegrationTests\\Datadog.Trace.ClrProfiler.IntegrationTests.csproj", | |||
| "tracer\\test\\Datadog.Trace.Security.IntegrationTests\\Datadog.Trace.Security.IntegrationTests.csproj", | |||
| "tracer\\test\\Datadog.Trace.Security.Unit.Tests\\Datadog.Trace.Security.Unit.Tests.csproj", | |||
| "tracer\\test\\Datadog.Trace.TestHelpers.AutoInstrumentation\\Datadog.Trace.TestHelpers.AutoInstrumentation.csproj", | |||
There was a problem hiding this comment.
we need it in order to rebuild and play integration tests for the IDE
anna-git
commented
Jan 15, 2025
| @@ -114,12 +128,6 @@ private Dictionary<string, object> GetBasicRequestArgsForWaf() | |||
| { AddressesConstants.RequestClientIp, _localRootSpan.GetTag(Tags.HttpClientIp) } | |||
| }; | |||
|
|
|||
| var userId = _localRootSpan.Context?.TraceContext?.Tags.GetTag(Tags.User.Id); | |||
There was a problem hiding this comment.
Dont do this anymore: this could override the sdk, we now need a more precise approach with values from the sdk not overriden and others yes
| @@ -8,6 +8,7 @@ | |||
| using System.Collections.Generic; | |||
| using Datadog.Trace.AppSec.Waf; | |||
| using Datadog.Trace.Headers; | |||
| using Datadog.Trace.Vendors.Serilog; | |||
There was a problem hiding this comment.
NIT: Guess that this is not needed.
| return null; | ||
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| SecurityCoordinator? GetSecurityCoordinatorImpl(Security securityImpl, Span spanImpl) => TryGet(securityImpl, spanImpl); |
There was a problem hiding this comment.
Do we need to define GetSecurityCoordinatorImpl instead of just writing return TryGet(security, span); in line 48?
There was a problem hiding this comment.
yes because of [MethodImpl(MethodImplOptions.NoInlining)] , we wanna make sure the method is not inlined, as it would cause a crash, trying to load a web assembly in a console app execution context
| @@ -630,38 +630,30 @@ internal bool IsMetaStructSupported() | |||
| return _spanMetaStructs; | |||
| } | |||
|
|
|||
| internal void UpdateActiveAddresses() | |||
| private void UpdateActiveAddresses() | |||
| { | |||
| // So far, RASP is the only one that uses this | |||
There was a problem hiding this comment.
If RASP is not the only one using this, we should delete this comment, right?
| } | ||
| else | ||
| { | ||
| _activeAddresses = null; | ||
| } | ||
| } | ||
|
|
||
| internal bool AddressEnabled(string address) | ||
| public bool AddressEnabled(string address) |
There was a problem hiding this comment.
NIT: Is public required?
There was a problem hiding this comment.
yes as it's now mocked in the unit tests ContextUserEventTests , cant seem to find another way 🤔
There was a problem hiding this comment.
yes as it's now mocked in the unit tests ContextUserEventTests , cant seem to find another way
IMO, requiring mocking like this is a strong indication that we need an additional layer of abstraction and/or dependency injection. never mind, I didn't spot that it's because there's an interface now, all good 🙂
| SecurityReporter.LogAddressIfDebugEnabled(addresses); | ||
|
|
||
| // run the WAF and execute the results | ||
| result = additiveContext.Run(addresses, _security.Settings.WafTimeoutMicroSeconds); |
There was a problem hiding this comment.
I guess that this is modifying the session fingerprints, right? I have not seen any test related to session fingerprints, though. Should we test in this PR that the fingerprints are generated correctly?
There was a problem hiding this comment.
good point, I added it in the snapshots here
If there is no numbers as per the replacement logic here, this wouldnt print but just ---
anna-git
force-pushed
the
anna/asm/userid-monitoring
branch
7 times, most recently
from
6b790c0 to
87255a9
Compare
adapt squash! Adapt sample
…d to play integration tests
anna-git
force-pushed
the
anna/asm/userid-monitoring
branch
from
87255a9 to
a064bf5
Compare
andrewlock
reviewed
Jan 17, 2025
| @@ -42,6 +43,20 @@ private SecurityCoordinator(Security security, Span span, HttpTransport transpor | |||
| return new SecurityCoordinator(security, span, new(context)); | |||
| } | |||
|
|
|||
| internal static SecurityCoordinator? TryGetSafe(Security security, Span span) | |||
There was a problem hiding this comment.
I'm still not entirely sure how "safe" this really is 😅 but until we see crashes related to it I guess we'll carry on! 😄
| @@ -49,6 +49,8 @@ private SecurityCoordinator(Security security, Span span, HttpTransport transpor | |||
| return new SecurityCoordinator(security, span, transport); | |||
| } | |||
|
|
|||
| internal static SecurityCoordinator? TryGetSafe(Security security, Span span) => TryGet(security, span); | |||
There was a problem hiding this comment.
I'm not actually sure if this is always safe, for similar reasons to .NET Core 🤔 e.g. in Owin, you don't use System.Web... maybe we should test that scenario (manually I mean, we probably don't need integration tests for it at this stage)
| @@ -282,6 +284,24 @@ internal void BlockAndReport(Dictionary<string, object> args, bool lastWafCall = | |||
| } | |||
| } | |||
|
|
|||
| internal void BlockAndReport(IResult? result) | |||
There was a problem hiding this comment.
How is this different to the ReportAndBlock() bellow, and how do you know which one to choose? 😅
There was a problem hiding this comment.
yes I must say it's a bit confusing, ReportAndBlock is used by rasp, as the blocking middleware is not always here to catch the exception, so to make sure it's reported before blocking it.
But I think the preferred method should always be BlockAndReport as we're reporting the actual http status code and other stuff, we want to make sure we report what currently is and not what will be theoretically after blocking...
| result = additiveContext.Run(args, _security.Settings.WafTimeoutMicroSeconds); | ||
| } | ||
| // run the WAF and execute the results | ||
| result = runWithEphemeral |
There was a problem hiding this comment.
Is it safe to assume additiveContext is not null then, seeing as we removed the check?
| Dictionary<string, object>? addresses = null; | ||
| try | ||
| { | ||
| var additiveContext = GetOrCreateAdditiveContext(); |
There was a problem hiding this comment.
Can this return null? Should it?
| var additiveContext = GetOrCreateAdditiveContext(); | ||
| if (additiveContext?.ShouldRunWith(_security, userId, userLogin, userSessionId, fromSdk) is { Count: > 0 } userAddresses) | ||
| { | ||
| addresses = userAddresses.ToDictionary(k => k.Key, object (v) => v.Value); |
There was a problem hiding this comment.
userAddresses is already a Dictionary<>, right, and isn't shared anywhere afaict? Can we avoid creating another dictionary here, and just use the one provided instead?
| { | ||
| var addresses = _waf.GetKnownAddresses(); | ||
| Log.Debug("Updating WAF active addresses to {Addresses}", addresses); | ||
| _activeAddresses = addresses is null ? null : new HashSet<string>(addresses); | ||
| _activeAddresses = [..addresses]; |
There was a problem hiding this comment.
Nice 👍 but can addresses still be null? If so you'll still need a similar pattern, right?
| /// <summary> | ||
| /// Gets or sets a string for the value and bool for if it came from sdk | ||
| /// </summary> | ||
| internal UserRecord Id { get; set; } = new(null, false); |
There was a problem hiding this comment.
Is it worth creating "null" versions of the UserRecord? Are the types mutable? If not, that could save a bunch of allocations - currently we're allocating four objects every time, and then potentially replacing them later?
| MethodName = "set_User", | ||
| ReturnTypeName = ClrNames.Task, | ||
| MinimumVersion = Major2, | ||
| CallTargetIntegrationKind = CallTargetKind.Derived, |
There was a problem hiding this comment.
Derived integration are much more expensive in general - IIRC, we have to analyze the class hierarchy for every loaded type to see if it derives from the type we're interested in. What I don't know is if the incremental perf hit is significant, or if it's just "as soon as we have a derived instrumentation we pay the cost".
I wonder if it would be sufficient to just instrument the "known" public HttpContext implementations e.g. DefaultHttpContext - realistically no-one outside of Microsoft is implementing their own HttpContext is going to be implementing their own HttpContext, so will probably be good enough? 🤔
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of changes
Reason for change
https://docs.google.com/document/d/1RT38U6dTTcB-8muiYV4-aVDCsT_XrliyakjtAPyjUpw
Implementation details
Test coverage
Other details