[ASM] Blocking a request on .NET Core#3066
Merged
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
anna-git
force-pushed
the
anna/asm/blocking-request
branch
2 times, most recently
from
August 12, 2022 09:55
be51f16 to
5cffe62
Compare
This comment has been minimized.
This comment has been minimized.
anna-git
force-pushed
the
anna/asm/blocking-request
branch
from
August 22, 2022 09:18
5cffe62 to
4352d8d
Compare
This comment has been minimized.
This comment has been minimized.
anna-git
force-pushed
the
anna/asm/blocking-request
branch
from
August 22, 2022 14:49
4352d8d to
c35bd44
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
anna-git
force-pushed
the
anna/asm/blocking-request
branch
from
August 23, 2022 13:29
6afb73c to
1cd9af9
Compare
This comment has been minimized.
This comment has been minimized.
anna-git
force-pushed
the
anna/asm/blocking-request
branch
from
August 23, 2022 14:53
1cd9af9 to
aec6ec6
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
anna-git
force-pushed
the
anna/asm/blocking-request
branch
from
August 24, 2022 14:58
2d57db2 to
eba9d56
Compare
anna-git
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
anna-git
force-pushed
the
anna/asm/blocking-request
branch
from
August 25, 2022 13:10
b637e1b to
f24f19b
Compare
This comment has been minimized.
This comment has been minimized.
anna-git
force-pushed
the
anna/asm/blocking-request
branch
from
August 25, 2022 14:49
fa2b1d0 to
9da4752
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
anna-git
force-pushed
the
anna/asm/blocking-request
branch
from
August 26, 2022 08:49
e48152f to
b4a80ef
Compare
anna-git
marked this pull request as ready for review
August 26, 2022 08:50
little fixes comment
fixing aspnet core 2 addin snapshot files
fix incorrect merge fix incorrect header fix accidentaly broken framework integration tests Fix integration tests regex scrubber because different bytes on linux
uncomment code that was commented for testing purposes remove verbose if test
anna-git
force-pushed
the
anna/asm/blocking-request
branch
from
August 31, 2022 08:48
af3f957 to
f437137
Compare
Member
Benchmarks Report 🐌Benchmarks for #3066 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AppSecBodyBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AspNetCoreBenchmark - 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 - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️Raw results
|
Member
Code Coverage Report 📊✔️ Merging #3066 into master will not change line coverage
View the full report for further details: Datadog.Trace Breakdown ✔️
The following classes have significant coverage changes.
The following classes were added in #3066:
View the full reports for further details: |
andrewlock
reviewed
Aug 31, 2022
andrewlock
left a comment
andrewlock
left a comment
Member
There was a problem hiding this comment.
Nice, I have a few concerns about the use of the synchronous APIs, but otherwise looks good. Either way, I'm apparently too slow as this is merged 😂
Comment on lines
+59
to
+60
| httpResponse.Clear(); | ||
| httpResponse.Headers.Clear(); |
Member
There was a problem hiding this comment.
FYI, this may throw if the response is already streaming
Comment on lines
+70
to
+78
| if (_context.Request.Headers["Accept"] == "application/json") | ||
| { | ||
| httpResponse.ContentType = "application/json"; | ||
| } | ||
| else | ||
| { | ||
| httpResponse.ContentType = "text/html"; | ||
| template = templateHtml; | ||
| } |
Member
There was a problem hiding this comment.
Personally, I would probably reverse this condition and use "contains" e.g. if the Accept header contains "text/html", use that, otherwise return json. Otherwise, if they're using a json derivative (e.g. applicataion/hal+json) we incorrectly send them text/html.
| if (syncIOFeature != null) | ||
| { | ||
| // allow synchronous operatoin for net core >=3.1 otherwise invalidoperation exception | ||
| syncIOFeature.AllowSynchronousIO = true; |
Member
There was a problem hiding this comment.
This is kinda horrible, is there no way we can make this async up the stack? 🙁 Last thing we really want is thread starvation and hangs 😬
Comment on lines
-890
to
+911
| span.SetHeaderTags(new HeadersCollectionAdapter(httpContext.Response.Headers), tracer.Settings.HeaderTags, defaultTagPrefix: SpanContextPropagator.HttpResponseHeadersTagPrefix); | ||
| scope.Dispose(); | ||
| DoBeforeRequestStops(httpContext, scope, tracer.Settings); |
Member
There was a problem hiding this comment.
AFAIK, this code is called even when there's an exception, so I don't think you need to call this function everywhere 🤔
| @@ -1,4 +1,5 @@ | |||
| Microsoft.AspNetCore.Http.Abstractions, Version=2.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60 | |||
| Microsoft.AspNetCore.Http.Extensions, Version=2.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60 | |||
Member
There was a problem hiding this comment.
😬 Do we have to add this? We generally don't want to add references (we really need to look at removing these in future if anything)
| http.request.headers.host: localhost:00000, | ||
| http.request.headers.user-agent: Mistake Not..., | ||
| http.request.headers.x-forwarded: 86.242.244.246, | ||
| http.response.headers.content-length: 0, |
Member
There was a problem hiding this comment.
Is this correct? You send content when blocking don't you, so this shouldn't be 0?
robertpi
added a commit
that referenced
this pull request
Sep 29, 2022
## Summary of changes This builds on #3066 to bring blocking capabilities to the .NET Framework as well as core. This provides the ability to block a request but is not linked to any real mechanism to block requests, only test blocking rules exist for now. ## Reason for change We want to support blocking on both .NET Framework and Core. ## Implementation details Most of the blocking mechanism was put in place in #3066, this just fills in the missing parts for .NET Framework. ## Test coverage Covered by integration tests.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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
This PR is part of a set of ASM PRs to implement blocking in conjunction with remote config to activate ASM and configure security rules remotely.
It only implements the blocking mechanism that will be used in NET Core subsequently. For now, it's testable on a dummy rule which will be replaced with the waf update and remote config available
Reason for change
Implementing actual blocking
Implementation details
We report everything before blocking and finally throw an exception to stop the request thread from continuing.
Waf is ran multiple times during the request, as data becomes more and more available (request headers and url, path paramters, body..)
Blocking templates are configurable for json and html for now.
Adding address to the waf http.client_ip for later when waf is updated in subsequent PR
Test coverage
Tests will be implemented in conjunction with the new waf's version.
Other details