Activate AVM OSS via environment variable #4119

CarlesDD · 2024-02-29T08:20:34Z

What does this PR do?

Adds a new configuration DD_APPSEC_SCA_ENABLED, used to enable AVM OSS.
The value of this new configuration is sent via telemetry to the backend in the configuration payload for the following telemetry messages: app-started, app-extended-heartbeat and app-client-configuration-change.
If DD_APPSEC_SCA_ENABLED is not set explicitly, then the value is not sent in telemetry messages.

Motivation

Customers need a way to enable AVM OSS via an environment variable, just as they enable APM or other products or features, instead of having to do it exclusively through the UI.

Plugin Checklist

Additional Notes

System Test PR

Security

Datadog employees:

If this PR touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

APPSEC-17141

github-actions · 2024-02-29T08:21:16Z

Overall package size

Self size: 6.45 MB
Deduped: 60.95 MB
No deduping: 61.23 MB

Dependency sizes

name	version	self size	total size
@datadog/native-iast-taint-tracking	1.7.0	16.71 MB	16.72 MB
@datadog/native-appsec	7.1.1	14.39 MB	14.4 MB
@datadog/pprof	5.2.0	8.84 MB	9.21 MB
protobufjs	7.2.5	2.77 MB	6.56 MB
@datadog/native-iast-rewriter	2.3.0	2.15 MB	2.24 MB
@opentelemetry/core	1.14.0	872.87 kB	1.47 MB
@datadog/native-metrics	2.0.0	898.77 kB	1.3 MB
@opentelemetry/api	1.4.1	780.32 kB	780.32 kB
import-in-the-middle	1.7.3	67.62 kB	731.01 kB
msgpack-lite	0.1.26	201.16 kB	281.59 kB
opentracing	0.14.7	194.81 kB	194.81 kB
semver	7.5.4	93.4 kB	123.8 kB
pprof-format	2.1.0	111.69 kB	111.69 kB
@datadog/sketches-js	2.1.0	109.9 kB	109.9 kB
lodash.sortby	4.7.0	75.76 kB	75.76 kB
lru-cache	7.14.0	74.95 kB	74.95 kB
ipaddr.js	2.1.0	60.23 kB	60.23 kB
ignore	5.2.4	51.22 kB	51.22 kB
int64-buffer	0.1.10	49.18 kB	49.18 kB
shell-quote	1.8.1	44.96 kB	44.96 kB
istanbul-lib-coverage	3.2.0	29.34 kB	29.34 kB
tlhunter-sorted-set	0.1.0	24.94 kB	24.94 kB
limiter	1.1.5	23.17 kB	23.17 kB
dc-polyfill	0.1.4	23.1 kB	23.1 kB
retry	0.13.1	18.85 kB	18.85 kB
node-abort-controller	3.1.1	16.89 kB	16.89 kB
jest-docblock	29.7.0	8.99 kB	12.76 kB
crypto-randomuuid	1.0.0	11.18 kB	11.18 kB
path-to-regexp	0.1.7	6.78 kB	6.78 kB
koalas	1.0.2	6.47 kB	6.47 kB
methods	1.1.2	5.29 kB	5.29 kB
module-details-from-path	1.0.3	4.47 kB	4.47 kB

_{🤖 This report was automatically generated by heaviest-objects-in-the-universe}

codecov · 2024-02-29T08:21:46Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.76%. Comparing base (c11fcfd) to head (7b97662).
Report is 11 commits behind head on master.

❗ Current head 7b97662 differs from pull request most recent head 6ec04c5. Consider uploading reports for the commit 6ec04c5 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #4119       +/-   ##
===========================================
+ Coverage   83.11%   95.76%   +12.65%     
===========================================
  Files         244       97      -147     
  Lines       10255     3259     -6996     
  Branches       33       33               
===========================================
- Hits         8523     3121     -5402     
+ Misses       1732      138     -1594

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pr-commenter · 2024-02-29T08:27:04Z

Benchmarks

Benchmark execution time: 2024-05-03 07:59:56

Comparing candidate commit 6ec04c5 in PR branch ccapell/activate-oss-vm-via-env-var with baseline commit 3031b6e in branch master.

Found 1 performance improvements and 0 performance regressions! Performance is the same for 260 metrics, 5 unstable metrics.

scenario:plugin-graphql-with-depth-on-max-18

🟩 max_rss_usage [-116.988MB; -73.936MB] or [-12.550%; -7.931%]

packages/dd-trace/src/config.js

iunanua · 2024-04-11T13:14:19Z

packages/dd-trace/src/config.js

@@ -544,6 +546,8 @@ class Config {
    this._setValue(env, 'appsec.rateLimit', maybeInt(DD_APPSEC_TRACE_RATE_LIMIT))
    this._setString(env, 'appsec.rules', DD_APPSEC_RULES)
    this._setValue(env, 'appsec.wafTimeout', maybeInt(DD_APPSEC_WAF_TIMEOUT))
+    // DD_APPSEC_SCA_ENABLED is never used locally, but only sent to the backend
+    this._setBoolean(env, 'appsec.sca.enabled', DD_APPSEC_SCA_ENABLED)


Could we avoid the 384 line _setValue and the changes on telemetry/index.js if we check the env value and only set it if not undefined?
Or can there be no env config property without its correspondent default value?

Suggested change

this._setBoolean(env, 'appsec.sca.enabled', DD_APPSEC_SCA_ENABLED)

if (DD_APPSEC_SCA_ENABLED) {

this._setBoolean(env, 'appsec.sca.enabled', DD_APPSEC_SCA_ENABLED)

}

ummm there is a comment in config.js

// for _merge to work, every config value must have a default value

so I guess that answers my questions

That's right. The _merge function goes through all the defaults to extract changes, so every configuration must have a default.

simon-id · 2024-04-11T13:51:45Z

Can we include the system test enablement PR link in the description ?

packages/dd-trace/src/telemetry/index.js

simon-id · 2024-04-11T15:03:33Z

@CarlesDD sorry for the system-tests PR i meant the PR where we enable the test 👍

simon-id · 2024-04-23T14:57:49Z

packages/dd-trace/test/config.spec.js

@@ -284,6 +285,7 @@ describe('Config', () => {
      { name: 'appsec.blockedTemplateJson', value: undefined, origin: 'default' },
      { name: 'appsec.eventTracking.enabled', value: true, origin: 'default' },
      { name: 'appsec.eventTracking.mode', value: 'safe', origin: 'default' },
+      { name: 'appsec.sca.enabled', value: undefined, origin: 'default' },


isn't this supposed to be value: null then ? 🤔

and if i'm right, why is it not failing in the CI ?

Good catch.

You are right Simon. The expected value is null.

The CI is not failing because this assertion was not correctly implemented.

expect(updateConfig.getCall(0).args[0]).to.deep.include( { name: 'service', value: 'node', origin: 'default' }, { name: 'logInjection', value: false, origin: 'default' }, { name: 'headerTags', value: [], origin: 'default' }, ...

It checks that first argument of the first call to updateConfig includes { name: 'service', value: 'node', origin: 'default' } (the first argument for deep.include call), ignoring the rest. 😕

This test, and others in this file must be revisited.

With this regex (?<!assert)\.include\(($|.+,) I found 3 similar cases in the whole repo, all in this file, line 235, 503, 761.
We need to fix this before merging the current PR. Would you agree with that but in a different PR ? 😆
I think we can fix it by just adding an array around the objects, and changing to deep equal ? like done so in line 1401:

expect(updateConfig.getCall(1).args[0]).to.deep.equal([ { name: 'sampleRate', value: 0, origin: 'remote_config' } ])

…ot been set

…set.

packages/dd-trace/src/config.js

* Add SCA enabled to configuration * Add comment to new config * Fix test for telemtry after app-extended-heartbeat behaviour change * Do not send sca enabled in telemetry messages when not informed * Fix lint * Rename sca.enabled to appsec.sca.enabled * Prevent sending some configuration in telemetry message when it has not been set * Improve loggers naming * Appsec SCA default to null. Avoid stripping config entry off when no set. * Fix sca default value assertion * Fix config sorting

CarlesDD added semver-minor appsec labels Feb 29, 2024

CarlesDD force-pushed the ccapell/activate-oss-vm-via-env-var branch from 886c9ba to 9fa7c3f Compare February 29, 2024 09:57

CarlesDD marked this pull request as ready for review February 29, 2024 10:24

CarlesDD requested a review from a team as a code owner February 29, 2024 10:24

simon-id reviewed Feb 29, 2024

View reviewed changes

packages/dd-trace/src/config.js Outdated Show resolved Hide resolved

simon-id previously approved these changes Mar 1, 2024

View reviewed changes

uurien previously approved these changes Mar 7, 2024

View reviewed changes

CarlesDD dismissed stale reviews from uurien and simon-id via 18f0c63 March 27, 2024 08:23

CarlesDD force-pushed the ccapell/activate-oss-vm-via-env-var branch from 3fcb83c to 18f0c63 Compare March 27, 2024 08:23

CarlesDD force-pushed the ccapell/activate-oss-vm-via-env-var branch 2 times, most recently from de68728 to 64b35df Compare April 11, 2024 05:41

iunanua reviewed Apr 11, 2024

View reviewed changes

simon-id reviewed Apr 11, 2024

View reviewed changes

packages/dd-trace/src/telemetry/index.js Outdated Show resolved Hide resolved

simon-id reviewed Apr 11, 2024

View reviewed changes

packages/dd-trace/src/telemetry/index.js Outdated Show resolved Hide resolved

CarlesDD force-pushed the ccapell/activate-oss-vm-via-env-var branch from 1d35c6f to e7e3042 Compare April 22, 2024 11:00

uurien previously approved these changes Apr 23, 2024

View reviewed changes

simon-id reviewed Apr 23, 2024

View reviewed changes

CarlesDD added 6 commits May 3, 2024 08:36

Add SCA enabled to configuration

c4fc94a

Add comment to new config

0589bbc

Fix test for telemtry after app-extended-heartbeat behaviour change

3999f64

Do not send sca enabled in telemetry messages when not informed

08e250d

Fix lint

c0200fe

Rename sca.enabled to appsec.sca.enabled

c480532

CarlesDD added 4 commits May 3, 2024 08:39

Prevent sending some configuration in telemetry message when it has n…

40a189b

…ot been set

Improve loggers naming

4b59b11

Appsec SCA default to null. Avoid stripping config entry off when no …

395dcd4

…set.

Fix sca default value assertion

6034f1b

CarlesDD dismissed uurien’s stale review via 6034f1b May 3, 2024 07:10

CarlesDD force-pushed the ccapell/activate-oss-vm-via-env-var branch from e7e3042 to 6034f1b Compare May 3, 2024 07:10

simon-id reviewed May 3, 2024

View reviewed changes

packages/dd-trace/src/config.js Show resolved Hide resolved

Fix config sorting

6ec04c5

simon-id approved these changes May 3, 2024

View reviewed changes

CarlesDD merged commit 0e50598 into master May 3, 2024
109 of 111 checks passed

CarlesDD deleted the ccapell/activate-oss-vm-via-env-var branch May 3, 2024 10:40

erikvanbrakel mentioned this pull request Jul 5, 2024

[DEVX-3565] add neo4j plugin for v4.41 mazedesignhq/dd-trace-js#15

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Activate AVM OSS via environment variable #4119

Activate AVM OSS via environment variable #4119

CarlesDD commented Feb 29, 2024 •

edited

Loading

github-actions bot commented Feb 29, 2024 •

edited

Loading

codecov bot commented Feb 29, 2024 •

edited

Loading

pr-commenter bot commented Feb 29, 2024 •

edited

Loading

iunanua Apr 11, 2024 •

edited

Loading

iunanua Apr 11, 2024

CarlesDD Apr 11, 2024

simon-id commented Apr 11, 2024

simon-id commented Apr 11, 2024

simon-id Apr 23, 2024

simon-id Apr 23, 2024

CarlesDD Apr 24, 2024 •

edited

Loading

simon-id Apr 24, 2024

Activate AVM OSS via environment variable #4119

Activate AVM OSS via environment variable #4119

Conversation

CarlesDD commented Feb 29, 2024 • edited Loading

What does this PR do?

Motivation

Plugin Checklist

Additional Notes

Security

github-actions bot commented Feb 29, 2024 • edited Loading

Overall package size

Dependency sizes

codecov bot commented Feb 29, 2024 • edited Loading

Codecov Report

pr-commenter bot commented Feb 29, 2024 • edited Loading

Benchmarks

scenario:plugin-graphql-with-depth-on-max-18

iunanua Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

iunanua Apr 11, 2024

Choose a reason for hiding this comment

CarlesDD Apr 11, 2024

Choose a reason for hiding this comment

simon-id commented Apr 11, 2024

simon-id commented Apr 11, 2024

simon-id Apr 23, 2024

Choose a reason for hiding this comment

simon-id Apr 23, 2024

Choose a reason for hiding this comment

CarlesDD Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

simon-id Apr 24, 2024

Choose a reason for hiding this comment

CarlesDD commented Feb 29, 2024 •

edited

Loading

github-actions bot commented Feb 29, 2024 •

edited

Loading

codecov bot commented Feb 29, 2024 •

edited

Loading

pr-commenter bot commented Feb 29, 2024 •

edited

Loading

iunanua Apr 11, 2024 •

edited

Loading

CarlesDD Apr 24, 2024 •

edited

Loading