feat(appsec): enable request blocking #630
Conversation
c3fb11f to
06c1e34
Compare
06c1e34 to
cba994e
Compare
| @@ -126,6 +130,14 @@ def asm_start_request( | |||
| span.set_tag_str("http.client_ip", request_ip) | |||
| span.set_tag_str("network.client.ip", request_ip) | |||
|
|
|||
| # Encode the parsed query and append it to reconstruct the original raw URI expected by AppSec. | |||
| if parsed_query: | |||
There was a problem hiding this comment.
Could you please explain a bit in the PR description how this part is related? Thank you
There was a problem hiding this comment.
Done
Bug Fix
The appsec waf expects the raw URI to contain the query parameters. This is an omission from the previous Appsec PRs. The fix is included in this one.
Some rules in the system-tests use the presence of query parameters in the URI to block requests. This is therefore required to pass the
APPSEC_BLOCKINGtest suite of the system tests which this PR relates to.
datadog_lambda/wrapper.py
Outdated
| @@ -155,12 +162,21 @@ def __init__(self, func): | |||
| except Exception as e: | |||
| logger.error(format_err_with_traceback(e)) | |||
|
|
|||
| def _get_blocking_response(self): | |||
| if not config.appsec_enabled: | |||
There was a problem hiding this comment.
I think we might not need this method at all. In its 3 usages, 2 actually have the config.appsec_enabled right before calling it anyway. The only place left is in the BlockingException case, which implies that appsec already enabled? Or we just add the config.appsec_enabled check there?
in short, I think we can just call get_asm_blocked_response(self.event_source) directly in the 3 usages so far to avoid some double if statements..
There was a problem hiding this comment.
Yes it makes total sense to avoid checking it twice.
For the BlockingException case, it can only be raised when the ASM context is set therefore only when appsec is already enabled. No need to do the check in this case either.
datadog_lambda/wrapper.py
Outdated
| from datadog_lambda.asm import ( | ||
| asm_set_context, | ||
| asm_start_response, | ||
| asm_start_request, | ||
| get_asm_blocked_response, | ||
| ) |
There was a problem hiding this comment.
| from datadog_lambda.asm import ( | |
| asm_set_context, | |
| asm_start_response, | |
| asm_start_request, | |
| get_asm_blocked_response, | |
| ) | |
| if config.appsec_enabled: | |
| from datadog_lambda.asm import ( | |
| asm_set_context, | |
| asm_start_response, | |
| asm_start_request, | |
| get_asm_blocked_response, | |
| ) |
Add move it below from datadog_lambda.config import config
There was a problem hiding this comment.
Done. This also implies some changes in the test_wapper.py where I have to reload the import to test appsec.
datadog_lambda/asm.py
Outdated
| blocked = get_blocked() | ||
| if not blocked: | ||
| return None | ||
| set_blocked(blocked) |
There was a problem hiding this comment.
i need some help understanding this part. Since we already got the blocked above, does the set_blocked have any effect here?
There was a problem hiding this comment.
I thought that calling set_blocked added the expected content-type calculated from the accept headers gathered by Appsec. In fact it is already done by the WAF and unnecessary outside of certain frameworks.
I removed the set_blocked line as it is useless.
| @@ -199,6 +208,30 @@ | |||
| ), | |||
| ] | |||
|
|
|||
| ASM_BLOCKED_RESPONSE_TEST_CASES = [ | |||
There was a problem hiding this comment.
Claude gave me these following cases below. Could you add them here if they make sense? Thank you.
# Default blocking configuration
{
"status_code": 403,
"type": "auto",
"content-type": "application/json" # or "text/html" based on Accept header
}
# HTML blocking response
{
"status_code": 403,
"type": "html",
"content-type": "text/html"
}
# Redirect blocking
{
"status_code": 403,
"type": "none",
"location": "https://example.com/blocked"
}
There was a problem hiding this comment.
I cleaned up the test cases by adding yours and removing duplicates.
cba994e to
29bb61d
Compare
What does this PR do?
This PR enables Appsec to block request if they are deemed dangerous by the in-app WAF.
There are three cases:
asm_start_requestBlockingExceptionasm_start_responseMotivation
Expand the capabilities of Appsec inside the datadog-lambda-python layer.
Testing Guidelines
I added unit tests to both
test_asm.pyandtest_wrapper.pyto ensure requests are blocked only if necessary and at the right stage.Integration tests will be run as part of the system-test, APPSEC_BLOCKING scenario.
Bug Fix
The appsec waf expects the raw URI to contain the query parameters. This is an omission from the previous Appsec PRs. The fix is included in this one.
Some rules in the system-tests use the presence of query parameters in the URI to block requests. This is therefore required to pass the
APPSEC_BLOCKINGtest suite of the system tests which this PR relates to.Types of Changes
Check all that apply