APPSEC-55830 - SCA + Appsec Standalone

[iunanua]

Motivation

Check that SCA is still working when STANDALONE mode is on.

Changes

• Add new SCA_STANDALONE scenario: SCA on, STANDALONE on, APPSEC off, IAST off
• Add the Test_SCAStandalone_Telemetry class with two new tests:

  • test_telemetry_sca_enabled_propagated: testing that DD_APPSEC_SCA_ENABLED config property is sent when STANDALONE mode is on. It is 'inspired' by Test_TelemetrySCAEnvVar::test_telemetry_sca_enabled_propagated parametric test.
    It gets the configuration from telemetry data and looks for the proper DD_APPSEC_SCA_ENABLED property taking into account the different languages.

  • test_app_dependencies_loaded: testing that dependencies are still sent via the app-dependencies-loaded telemetry request. It uses the same logic and endpoint than Test_Telemetry::test_app_dependencies_loaded.
    It makes a weblog request to /load_dependency endpoint which loads a dependency and gets the app-dependencies-loaded payload from telemetry trying to find the corresponding entry for the previously loaded dependency

Workflow

1. ⚠️ Create your PR as draft ⚠️
2. Work on you PR until the CI passes (if something not related to your task is failing, you can ignore it)
3. 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

• 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

[iunanua]

@cbeauchesne any clues as to why these Test_TelemetrySCAEnvVar parametric tests fail? In theory I haven't modified them...
All languages are failing

       def test_telemetry_sca_enabled_not_propagated(self, library_env, test_agent, test_library):
E       fixture 'test_agent' not found

[cbeauchesne]

Becasue you imported some test classes : https://github.com/DataDog/system-tests/pull/3436/files#diff-b85606db77dfc3d56861c57eea9ce4af4d9e93bd4b21df3367f2ed8ef8fdd214R5

Test classes must not be imported from one folder to another. If some of them shares methods, the common bit must be placed in a utils.py file.

[iunanua]

Becasue you imported some test classes : https://github.com/DataDog/system-tests/pull/3436/files#diff-b85606db77dfc3d56861c57eea9ce4af4d9e93bd4b21df3367f2ed8ef8fdd214R5

Test classes must not be imported from one folder to another. If some of them shares methods, the common bit must be placed in a utils.py file.

what about now?

[cbeauchesne]

what about now?

Good, (failures are not related see #3498)

[vpellan]

Is there a way to ensure that DD_EXPERIMENTAL_APPSEC_STANDALONE_ENABLED is actually set to true (if that's our goal) ? Because I tried these tests on a version of dd-trace-rb that did not implemented Standalone AppSec yet (v2.4.0) and it did passed the tests, which surprised me because that means that DD_EXPERIMENTAL_APPSEC_STANDALONE_ENABLED is simply not used in this context ?

[cbeauchesne]

Is there a way to ensure that DD_EXPERIMENTAL_APPSEC_STANDALONE_ENABLED is actually set to true

You mean, checking the env var value inside the weblog container ?

[vpellan]

More like checking that the env var actually activates standalone appsec. The env var can still be in the container even when standalone appsec is not implemented (as this is just an env var after all). I know this is the whole point of the APPSEC_STANDALONE scenario, but here, changing the value of DD_EXPERIMENTAL_APPSEC_STANDALONE_ENABLED does not impact the result (as weblogs that don't even implements Standalone Appsec will still pass the tests, meaning that this env var is basically useless in these tests).

My question was, if the behaviour we want is really to pass these tests ONLY if Standalone Appsec is activated, is there anyway to know if it active, through telemetry for example ? If so, we should add a check for that in these tests.
If we do not care about Standalone Appsec being active for Standalone SCA to work, then we should remove that env var.
And if we do care but cannot verify it through telemetry, then I believe that we should add a section on the Standalone ASM RFC about that and change the current system-tests accordingly (both Standalone ASM Threats and Standalone SCA)

[iunanua]

changing the value of DD_EXPERIMENTAL_APPSEC_STANDALONE_ENABLED does not impact the result

That's one aspect we want to test, that SCA behaviour is not affected by standalone

But for what you say, we could check if traces contain _dd.p.appsec tag to ensure standalone is ON. The only problem with that is we need to enable appsec or iast and generate appsec events so that the trace is kept.

[vpellan]

That's one aspect we want to test, that SCA behaviour is not affected by standalone

I understand better now, thanks !

The problem is that, for versions of libraries that have not implemented Standalone Appsec yet (like dd-trace-rb v2.4.0 and lower) these tests will pass but not for the correct reason. It will not test that Standalone ASM does not affects SCA, as there is no Standalone Appsec implementation, so we are sure that it will not impact it.

So these tests will be equivalent to Test_TelemetrySCAEnvVar::test_telemetry_sca_enabled_propagated and Test_Telemetry::test_app_dependencies_loaded.

The fact that these tests ensures that Standalone ASM does not affects SCA behaviour reinforce my belief that they should not pass on versions/libraries where Standalone ASM is not implemented, thus we should check that they correctly implemented it (using _dd.p.appsec for example, or maybe adding a field in "app-started" telemetry event ? (But that would require to change the RFC and update current implementations of Standalone ASM))

[vpellan]

Another example is the go tracer. I believe that they implemented SCA but not Standalone ASM yet. And yet the test_telemetry_sca_enabled_propagated test is XPASS-ing, which I believe is not correct. ( test_app_dependencies_loaded is irrelevant for golang, thus xfailing)

[iunanua]

what do you think @vpellan?

[iunanua]

I have included the check in both tests

[vpellan]

In Ruby, when the sampler decides to not sample a trace, it sets it to MANUAL_REJECT. So span["metrics"]["_sampling_priority_v1"] will be equal to -1 (MANUAL_REJECT) instead of 0 (AUTO_REJECT). Maybe it is an error from our side (we still pass all the standalone ASM threats system-tests though) but if not, we should replace this line with:
assert span["metrics"]["_sampling_priority_v1"] <= 0. Otherwise I think it's good !

[iunanua]

In node 0 is used for automatic resolutions (the agent decides it) and -1 when manual.drop has been found or a rule has dropped the trace. But is OK for me to change the assert!

[vpellan]

LGTM!