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?

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 ?

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

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 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 5 times, most recently from f96b4f2 to 3db6d44 Compare January 14, 2025 17:54

nginx/appsec support

9c6ce99

cataphract force-pushed the glopes/nginx-appsec branch from 3db6d44 to 9c6ce99 Compare January 15, 2025 12:07

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 Jun 3, 2024

cbeauchesne Jun 3, 2024

cataphract Jan 13, 2025

cbeauchesne commented Jul 1, 2024

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

cbeauchesne commented Jul 1, 2024