Block based on response status or headers #171

cataphract · 2025-02-19T17:03:14Z

Significantly changes how the hooking mechanism for executing the final WAF run. The purpose is to allow blocking based on the response headers and status that would otherwise be sent.

Previously, we would install only an output filter. By the time it was invoked, the headers would already have been committed. It would not be possible to fully replace the response with another one.

So this PR installs also a header filter. When invoked, this header filter prevents the final filter from committing the response by changing the send_chain function pointer, continuing the filter chain and saving the buffers instead (because the filter may not invoke send_chain, it needs also to look at where this data may be buffered: this needs different code for http 1/3 and http/2).

Afterwards, it launches the WAF task. While this task is running, any data send to the output buffer is consumed as saved (copied), up to a certain limit. Once this limit is reached, the buffers are not consumed anymore, therefore stalling the output chain with busy buffers.

Once the final WAF task finishes, either a blocking response is committed, fully replacing the headers and body, or, if we're not to block, the buffered headers and body are sent forward (with special logic wrt the headers in http/2, as ngx_http_write_filter() can't send header frames in http/2).

Other changes:

Blocking tests in http/2 and /3
Fix read out-of-bounds in ngx_header_writer.h

datadog-datadog-prod-us1 · 2025-02-19T17:03:51Z

test/cases/orchestration.py

+    elif http_version == 3:
+        version_arg = "--http3-only"
+    else:
+        raise Exception(f"Unknown HTTP version: {http_version}")


⚪ Code Quality Violation

Exception is too generic (...read more)

Do not raise Exception and BaseException. These are too generic. Having generic exceptions makes it difficult to differentiate errors in a program. Use a specific exception, for example, ValueError, or create your own instead of using generic ones.

Learn More

Stop using raise Exception

datadog-datadog-prod-us1 · 2025-02-19T17:03:51Z

test/services/client/Dockerfile

@@ -1,9 +1,10 @@
-from alpine:3.19
+FROM alpine/curl-http3


🟠 Code Quality Violation

always pin version to an image spec (...read more)

This rule dictates that Docker images should always be tagged with a specific version number. In Docker, an image tag represents a particular version of an image. The use of tags allows developers to have better control over which versions of an image are being used in their projects.

This is crucial because it ensures the consistency and reliability of the Docker environment. If an image is not tagged, Docker defaults to using the 'latest' version of the image. However, the 'latest' tag does not guarantee that the same version of an image will be used every time, which can lead to unexpected behavior or compatibility issues.

To comply with this rule, always specify a version number when pulling a Docker image. Instead of FROM debian, write FROM debian:unstable or FROM debian:10.3. This ensures that you are using a specific version of the image, providing a more predictable and stable environment for your project.

Looks unnecessary once the full header filter chain is run.

dmehala · 2025-02-20T09:32:46Z

@cataphract We're not part of the same team or even the same organization. Given that, providing some context upfront can help set the stage for those reviewing your PR. A bit of background on what it does and why it’s needed would go a long way in ensuring a smoother review process. Could you, please, share more details?

Fixes refcount management and use-after-frees. Prefers to call ngx_http_finalize_request() with NGX_OK rather than NGX_DONE so that filter finalization, subrequest post actions, lingering close, and keepalive logic are respected.

codecov-commenter · 2025-02-21T16:09:14Z

Codecov Report

Attention: Patch coverage is 65.01767% with 198 lines in your changes missing coverage. Please review.

Project coverage is 71.25%. Comparing base (f1e5aff) to head (7cfce28).

Files with missing lines	Patch %	Lines
src/security/context.cpp	62.88%	166 Missing and 27 partials ⚠️
src/security/blocking.cpp	77.27%	3 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #171      +/-   ##
==========================================
- Coverage   71.87%   71.25%   -0.62%     
==========================================
  Files          44       44              
  Lines        6190     6683     +493     
  Branches      899      948      +49     
==========================================
+ Hits         4449     4762     +313     
- Misses       1305     1466     +161     
- Partials      436      455      +19

Files with missing lines	Coverage Δ
src/datadog_context.cpp	`61.79% <100.00%> (+4.22%)`	⬆️
src/ngx_header_writer.h	`93.02% <100.00%> (+0.16%)`	⬆️
src/ngx_http_datadog_module.cpp	`60.51% <ø> (ø)`
src/security/blocking.h	`100.00% <ø> (ø)`
src/security/context.h	`82.35% <ø> (ø)`
src/security/library.cpp	`61.75% <100.00%> (+0.93%)`	⬆️
src/security/library.h	`100.00% <ø> (ø)`
src/security/blocking.cpp	`80.98% <77.27%> (-0.12%)`	⬇️
src/security/context.cpp	`65.83% <62.88%> (-3.26%)`	⬇️

cataphract · 2025-02-21T17:16:17Z

@cataphract We're not part of the same team or even the same organization. Given that, providing some context upfront can help set the stage for those reviewing your PR. A bit of background on what it does and why it’s needed would go a long way in ensuring a smoother review process. Could you, please, share more details?

@dmehala I've added a description now. It was meant to be a draft PR; I opened it to run the pipelines.

datadog-datadog-prod-us1 · 2025-02-24T12:11:34Z

test/services/http/http.js

+  try {
+    const urlObj = new URL(request.url, `http://${request.headers.host}`);
+    const card = parseInt(urlObj.searchParams.get("card"));
+    const numBouts = parseInt(urlObj.searchParams.get("num_bouts"));


⚪ Code Quality Violation

Missing radix parameter. (...read more)

When utilizing the parseInt() function, many often skip the second parameter (the radix), allowing the function to deduce the number type based on the initial argument. By default, parseInt() can recognize both decimal and hexadecimal numbers, the latter through the 0x prefix. However, before ECMAScript 5, the function also mistakenly recognized octal numbers, leading to issues as many developers presumed a starting 0 would be disregarded.

datadog-datadog-prod-us1 · 2025-02-24T12:11:34Z

test/services/http/http.js

+    const urlObj = new URL(request.url, `http://${request.headers.host}`);
+    const card = parseInt(urlObj.searchParams.get("card"));
+    const numBouts = parseInt(urlObj.searchParams.get("num_bouts"));
+    const delay = parseInt(urlObj.searchParams.get("delay"));


⚪ Code Quality Violation

Missing radix parameter. (...read more)

When utilizing the parseInt() function, many often skip the second parameter (the radix), allowing the function to deduce the number type based on the initial argument. By default, parseInt() can recognize both decimal and hexadecimal numbers, the latter through the 0x prefix. However, before ECMAScript 5, the function also mistakenly recognized octal numbers, leading to issues as many developers presumed a starting 0 would be disregarded.

datadog-datadog-prod-us1 · 2025-02-24T12:11:34Z

test/services/http/http.js

+const sendRepeatResponse = (request, response) => {
+  try {
+    const urlObj = new URL(request.url, `http://${request.headers.host}`);
+    const card = parseInt(urlObj.searchParams.get("card"));


⚪ Code Quality Violation

Missing radix parameter. (...read more)

When utilizing the parseInt() function, many often skip the second parameter (the radix), allowing the function to deduce the number type based on the initial argument. By default, parseInt() can recognize both decimal and hexadecimal numbers, the latter through the 0x prefix. However, before ECMAScript 5, the function also mistakenly recognized octal numbers, leading to issues as many developers presumed a starting 0 would be disregarded.

src/security/context.cpp

Anilm3

From reading the PR it's quite hard to ascertain if this will work as intended, at least not without spending a similar amount of time it took to write it reviewing it... However aside from a few comments, I don't see any critical issues although some mistakes have been highlighted and should be fixed.

src/security/context.cpp

Anilm3 · 2025-02-26T09:51:48Z

src/security/context.cpp

+    assert(req != nullptr);
+
+    auto *ctx = instance.current_ctx_;
+    assert(ctx != nullptr);


How certain are we about these assertions...? An explicit check may be more prudent otherwise.

What's not clear is that c->data is the request in http3.

Seeing as this function is very difficult to exercise and the tests don't reach it (the data for the headers is small enough that it isn't sent just yet), I'm changing this to not rely on c->data.

cataphract requested review from a team as code owners February 19, 2025 17:03

cataphract requested review from dubloom and removed request for a team February 19, 2025 17:03

datadog-datadog-prod-us1 bot reviewed Feb 19, 2025

View reviewed changes

cataphract added 2 commits February 19, 2025 17:08

WAF: block responses

25a875b

Remove temp file file conversion

2820009

Looks unnecessary once the full header filter chain is run.

cataphract force-pushed the glopes/block-resp branch from c0cbf4c to 2820009 Compare February 19, 2025 17:08

cataphract added 10 commits February 21, 2025 10:23

Fix out-of-bounds read in ngx_header_writer.h

3ea7ba0

Fix blocking paths

579a70a

Fixes refcount management and use-after-frees. Prefers to call ngx_http_finalize_request() with NGX_OK rather than NGX_DONE so that filter finalization, subrequest post actions, lingering close, and keepalive logic are respected.

Fix Context::buffer_chain() with mixed memory and file buffers

7ba5b00

More tests of blocking with http2/3

e5a8d05

Add valgrind suppressions file

c260438

Makefile test target fix

b2c7184

Don't run tests with --failfast

91f84a7

Fix tests locking up

358dd45

lint

308d1fd

Fix nginx skip in openresty

d5c11ae

cataphract added 2 commits February 24, 2025 12:09

Fix configuration appsec_max_saved_output_data

154b0c0

Remove debugging functions

5971688

datadog-datadog-prod-us1 bot reviewed Feb 24, 2025

View reviewed changes

Increase test coverage

e9066a7

cataphract force-pushed the glopes/block-resp branch from bf9ab76 to e9066a7 Compare February 24, 2025 14:23

cataphract added 2 commits February 25, 2025 12:57

Bump version to 1.6.0

11a952d

Block: reset err_status and headers_out.status_line too

d1235a2

Anilm3 reviewed Feb 26, 2025

View reviewed changes

src/security/context.cpp Outdated Show resolved Hide resolved

Anilm3 reviewed Feb 26, 2025

View reviewed changes

src/security/context.cpp Outdated Show resolved Hide resolved

Anilm3 approved these changes Feb 26, 2025

View reviewed changes

Address PR comments

7cfce28

cataphract force-pushed the glopes/block-resp branch from 0b6544e to 7cfce28 Compare February 26, 2025 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block based on response status or headers #171

Block based on response status or headers #171

cataphract commented Feb 19, 2025 •

edited

Loading

datadog-datadog-prod-us1 bot Feb 19, 2025

datadog-datadog-prod-us1 bot Feb 19, 2025

dmehala commented Feb 20, 2025 •

edited

Loading

codecov-commenter commented Feb 21, 2025 •

edited

Loading

cataphract commented Feb 21, 2025 •

edited

Loading

datadog-datadog-prod-us1 bot Feb 24, 2025

datadog-datadog-prod-us1 bot Feb 24, 2025

datadog-datadog-prod-us1 bot Feb 24, 2025

Anilm3 left a comment

Anilm3 Feb 26, 2025

cataphract Feb 26, 2025

Block based on response status or headers #171

Are you sure you want to change the base?

Block based on response status or headers #171

Conversation

cataphract commented Feb 19, 2025 • edited Loading

datadog-datadog-prod-us1 bot Feb 19, 2025

Choose a reason for hiding this comment

⚪ Code Quality Violation

Learn More

datadog-datadog-prod-us1 bot Feb 19, 2025

Choose a reason for hiding this comment

🟠 Code Quality Violation

dmehala commented Feb 20, 2025 • edited Loading

codecov-commenter commented Feb 21, 2025 • edited Loading

Codecov Report

cataphract commented Feb 21, 2025 • edited Loading

datadog-datadog-prod-us1 bot Feb 24, 2025

Choose a reason for hiding this comment

⚪ Code Quality Violation

datadog-datadog-prod-us1 bot Feb 24, 2025

Choose a reason for hiding this comment

⚪ Code Quality Violation

datadog-datadog-prod-us1 bot Feb 24, 2025

Choose a reason for hiding this comment

⚪ Code Quality Violation

Anilm3 left a comment

Choose a reason for hiding this comment

Anilm3 Feb 26, 2025

Choose a reason for hiding this comment

cataphract Feb 26, 2025

Choose a reason for hiding this comment

cataphract commented Feb 19, 2025 •

edited

Loading

dmehala commented Feb 20, 2025 •

edited

Loading

codecov-commenter commented Feb 21, 2025 •

edited

Loading

cataphract commented Feb 21, 2025 •

edited

Loading