Nginx with AppSec support #2455

cataphract · 2024-05-15T13:53:43Z

Motivation

Nginx now supports AppSec. Enable those tesets

Changes

Workflow

⚠️ Create your PR as draft ⚠️
Work on you PR until the CI passes (if something not related to your task is failing, you can ignore it)
Mark it as ready for review
- Test logic is modified? -> Get a review from RFC owner. We're working on refining the codeowners file quickly.
- Framework is modified, or non obvious usage of it -> get a review from R&P team

🚀 Once your PR is reviewed, you can merge it!

🛟 #apm-shared-testing 🛟

Reviewer checklist

Relevant label (run-parametric-scenario, run-profiling-scenario...) are presents
If PR title starts with [<language>], double-check that only <language> is impacted by the change
No system-tests internal is modified. Otherwise, I have the approval from R&P team
CI is green, or failing jobs are not related to this change (and you are 100% sure about this statement)
A docker base image is modified?
- the relevant build-XXX-image label is present
A scenario is added (or removed)?
- Get a review from R&P team

robertomonteromiguel · 2024-05-23T15:10:41Z

.github/workflows/run-end-to-end.yml

+      if: ${{ matrix.version == 'dev' && inputs.library == 'cpp'}}
+      run: ./utils/scripts/load-binary.sh nginx
+      env:
+        CIRCLECI_TOKEN: ${{ secrets.CIRCLECI_TOKEN }}


We can use the previous step "Load Library binary" we only need to add the "env"

The previous step does:

./utils/scripts/load-binary.sh ${{ inputs.library }}

inputs.library will be cpp. However, I need to call it with nginx.

robertomonteromiguel · 2024-05-23T15:13:26Z

.github/workflows/run-end-to-end.yml

@@ -230,7 +235,9 @@ jobs:
        DD_API_KEY: ${{ secrets.DD_API_KEY }}
    - name: Run APPSEC_MISSING_RULES scenario
      if: always() && steps.build.outcome == 'success' && contains(inputs.scenarios, '"APPSEC_MISSING_RULES"')
-      run: ./run.sh APPSEC_MISSING_RULES
+      run: |
+        # nginx does not start with missing rules file, thereby stalling the job


could we add this condition to "if:"?
if: always() && steps.build.outcome == 'success' && contains(inputs.scenarios, '"APPSEC_MISSING_RULES"') && matrix.weblog != 'nginx'

robertomonteromiguel · 2024-05-23T15:13:49Z

.github/workflows/run-end-to-end.yml

-      run: ./run.sh APPSEC_CORRUPTED_RULES
+      run: |
+        # nginx does not start with corrupted rules file, thereby stalling the job
+        if [[ $WEBLOG_VARIANT != nginx ]]; then ./run.sh APPSEC_CORRUPTED_RULES; fi


tests/appsec/test_blocking_addresses.py

tests/appsec/test_traces.py

utils/_context/containers.py

utils/_context/_scenarios.py

cbeauchesne · 2024-06-03T15:15:16Z

manifests/cpp.yml

-      test_schemas.py: irrelevant (ASM is not implemented in C++)
+      test_schemas.py:
+        Test_Scanners:
+          "*": irrelevant (ASM is not implemented in C++)


As ASM is actually implemented, could you replace irrelevant by missing_feature ?

So far, as nginx is the only weblog, you can probably do a simple search and replace

Test_Scanners: missing_feature

ASM is implemented on the nginx module, not on dd-trace-cpp. If we're collapsing the distinction because the only supported weblog is nginx, then we should stop treating nginx separately in all the places in this file, no?

then we should stop treating nginx separately in all the places in this file, no?

as long as it's the only supported weblog, yes, it'll be easier for you to maintain

Upon reflection, I think the simplification might not be a good idea, as there are other dd-trace-cpp based projects, like for Apache httpd.

In light of that, the annotation may be correct insofar as dd-trace-cpp itself is not meant to implement ASM. It's only the nginx module that in addition to including dd-trace-cpp also has an ASM implementation. But you tell me

cbeauchesne · 2024-06-03T16:02:57Z

.github/workflows/ci.yml

@@ -89,6 +89,11 @@ jobs:
        uses: actions/checkout@v4
      - name: Get library artifact
        run: ./utils/scripts/load-binary.sh ${{ matrix.library }}
+      - name: Get nginx module


I feel like ./utils/scripts/load-binary.sh cpp should load both nginx and cpp, WDYT ?

Same question :)

depends if you want to collapse cpp and nginx or if you antecipate testing dd-trace-cpp directly

tests/appsec/test_traces.py

cbeauchesne · 2024-06-03T16:43:24Z

utils/_context/containers.py

@@ -143,6 +143,10 @@ def start(self) -> Container:

        logger.info(f"Start container {self.container_name}")

+        # the whole thing is reimplemented in python...
+        if self.healthcheck is not None:


I don't get this change ? 🤔

there's a healthcheck implemented in python if the healthcheck argument is passed. In this case, there's no point also running the healthcheck defined in the docker file

interesting, do you have a dockerfile with such an healthcheck ?

some of the images used in system-tests had them; it was the point of the change IIRC. But anyway, if you just examples you can search GitHub

utils/_context/containers.py

cbeauchesne · 2024-07-01T09:18:01Z

Hi @cataphract ,

Do you need any help to fix issues/comments on this PR ?

cataphract · 2025-01-15T19:14:00Z

@cbeauchesne I've given another try to this PR. Please take a look

cbeauchesne · 2025-01-16T10:42:36Z

.github/actions/pull_images/action.yml

@@ -59,4 +59,4 @@ runs:

    - name: Pull
      shell: bash
-      run: docker compose pull
+      run: cat compose.yaml && docker compose pull


I left it on purpose because it shows what's actually going on, but i can remove it

cbeauchesne · 2025-01-16T10:42:52Z

.github/workflows/ci.yml

@@ -89,6 +89,11 @@ jobs:
        uses: actions/checkout@v4
      - name: Get library artifact
        run: ./utils/scripts/load-binary.sh ${{ matrix.library }}
+      - name: Get nginx module


Same question :)

cbeauchesne · 2025-01-16T10:45:14Z

manifests/cpp.yml

-      test_schemas.py: irrelevant (ASM is not implemented in C++)
+      test_schemas.py:
+        Test_Scanners:
+          "*": irrelevant (ASM is not implemented in C++)


then we should stop treating nginx separately in all the places in this file, no?

as long as it's the only supported weblog, yes, it'll be easier for you to maintain

cbeauchesne · 2025-01-16T10:46:02Z

tests/appsec/test_blocking_addresses.py

@@ -54,6 +57,7 @@ def test_blocking(self):
    def setup_blocking_before(self):
        self.block_req2 = weblog.get("/tag_value/tainted_value_6512/200", headers={"X-Forwarded-For": "1.1.1.1"})

+    @irrelevant(context.weblog_variant == "nginx", reason="Tag adding happens before WAF run")


out of curiosity, it means that it'll never be supported ?

I'm not sure about never, but I can't see other use cases to set tags only after and conditionally on the blocking of response, except doing that just for having these tests passing. If there is a compelling, non purely test related use case, we can look at the viability.

cbeauchesne · 2025-01-16T10:46:08Z

tests/appsec/test_blocking_addresses.py

@@ -3,6 +3,9 @@
 # Copyright 2021 Datadog, Inc.

 import json
+
+import pytest


so it seems

cbeauchesne · 2025-01-16T10:48:12Z

utils/_context/containers.py

@@ -143,6 +143,10 @@ def start(self) -> Container:

        logger.info(f"Start container {self.container_name}")

+        # the whole thing is reimplemented in python...
+        if self.healthcheck is not None:


interesting, do you have a dockerfile with such an healthcheck ?

cbeauchesne · 2025-01-16T10:48:44Z

utils/_context/containers.py

            name="mysqldb",
-            command="--default-authentication-plugin=mysql_native_password",
+            command="--lc-messages-dir=/usr/share/mysql-8.0/english "


out of curiosity, why this change ?

I got an error at some point about missing messages, probably related to a different locale in my machine? I don't remember exactly.

utils/build/docker/php/common/install_ddtrace.sh

cataphract added the run-all-scenarios label May 15, 2024

cataphract force-pushed the glopes/nginx-appsec branch 12 times, most recently from f89da8a to 9bc83b3 Compare May 22, 2024 10:54

cataphract marked this pull request as ready for review May 23, 2024 15:04

cataphract requested review from a team as code owners May 23, 2024 15:04

robertomonteromiguel requested changes May 23, 2024

View reviewed changes

robertomonteromiguel requested changes May 24, 2024

View reviewed changes

utils/_context/containers.py Outdated Show resolved Hide resolved

cataphract requested a review from robertomonteromiguel May 24, 2024 10:30

cataphract force-pushed the glopes/nginx-appsec branch 2 times, most recently from ced3965 to 0df2555 Compare June 3, 2024 14:10

cataphract requested a review from cbeauchesne June 3, 2024 14:15

cbeauchesne requested changes Jun 3, 2024

View reviewed changes

cataphract force-pushed the glopes/nginx-appsec branch from 0df2555 to 98b4f66 Compare August 29, 2024 17:58

cataphract marked this pull request as draft August 29, 2024 17:59

cataphract force-pushed the glopes/nginx-appsec branch from 98b4f66 to f7dc232 Compare August 29, 2024 18:03

cataphract force-pushed the glopes/nginx-appsec branch 3 times, most recently from 2385524 to f3af09f Compare January 14, 2025 11:20

cataphract force-pushed the glopes/nginx-appsec branch 7 times, most recently from 9c6ce99 to 7414f49 Compare January 15, 2025 17:30

cataphract marked this pull request as ready for review January 15, 2025 19:12

cataphract requested review from a team as code owners January 15, 2025 19:12

cataphract requested review from dubloom and removed request for a team January 15, 2025 19:12

cbeauchesne requested changes Jan 16, 2025

View reviewed changes

cataphract force-pushed the glopes/nginx-appsec branch from 7414f49 to 1c62ce9 Compare January 17, 2025 19:44

nginx/appsec support

f0763ac

cataphract force-pushed the glopes/nginx-appsec branch from 1c62ce9 to f0763ac Compare January 17, 2025 19:45

cataphract requested a review from cbeauchesne January 17, 2025 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nginx with AppSec support #2455

Nginx with AppSec support #2455

cataphract commented May 15, 2024

robertomonteromiguel May 23, 2024

cataphract May 24, 2024

robertomonteromiguel May 23, 2024

cataphract May 24, 2024

robertomonteromiguel May 23, 2024

cbeauchesne Jun 3, 2024

cataphract Jan 13, 2025

cbeauchesne Jan 16, 2025

cataphract Jan 17, 2025

cbeauchesne Jun 3, 2024

cbeauchesne Jan 16, 2025

cataphract Jan 17, 2025

cbeauchesne Jun 3, 2024

cataphract Jan 13, 2025

cbeauchesne Jan 16, 2025

cataphract Jan 17, 2025

cbeauchesne commented Jul 1, 2024

cataphract commented Jan 15, 2025

cbeauchesne Jan 16, 2025

cataphract Jan 17, 2025

cbeauchesne Jan 16, 2025

cbeauchesne Jan 16, 2025

cbeauchesne Jan 16, 2025

cataphract Jan 17, 2025

cbeauchesne Jan 16, 2025

cataphract Jan 17, 2025

cbeauchesne Jan 16, 2025

cbeauchesne Jan 16, 2025

cataphract Jan 17, 2025

Nginx with AppSec support #2455

Are you sure you want to change the base?

Nginx with AppSec support #2455

Conversation

cataphract commented May 15, 2024

Motivation

Changes

Workflow

Reviewer checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbeauchesne commented Jul 1, 2024

cataphract commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment