Upgrade libddwaf native bindings #5792

CarlesDD · 2025-05-30T05:48:12Z

What does this PR do?

Introduces changes to use the latest version of native appsec which contains the libddwaf changes up to version 1.24.1.

This new libddwaf brings a new interface for instance creation as well as instance configuration management, moving the logic of merging configurations received by RC to libddwaf.

Also, diagnostics have been updated and should now be propagated by RC and telemetry.

Additional Notes

APPSEC-55565
RFC-1025

github-actions · 2025-05-30T05:48:43Z

Overall package size

Self size: 9.58 MB
Deduped: 104.06 MB
No deduping: 104.58 MB

Dependency sizes

| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.5.1 | 29.73 MB | 29.73 MB | | @datadog/native-appsec | 9.0.0 | 19.6 MB | 19.61 MB | | @datadog/pprof | 5.8.0 | 12.55 MB | 12.92 MB | | @datadog/native-iast-taint-tracking | 4.0.0 | 11.72 MB | 11.73 MB | | @opentelemetry/core | 1.30.1 | 908.66 kB | 7.16 MB | | protobufjs | 7.5.3 | 2.95 MB | 5.6 MB | | @datadog/wasm-js-rewriter | 4.0.1 | 2.85 MB | 3.58 MB | | @datadog/native-metrics | 3.1.1 | 1.02 MB | 1.43 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.14.0 | 120.58 kB | 842.2 kB | | source-map | 0.7.4 | 226 kB | 226 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | lru-cache | 7.18.3 | 133.92 kB | 133.92 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.1 | 109.9 kB | 109.9 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.2 | 53.63 kB | 53.63 kB | | istanbul-lib-coverage | 3.2.2 | 34.37 kB | 34.37 kB | | rfdc | 1.4.1 | 27.15 kB | 27.15 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | dc-polyfill | 0.1.9 | 25.11 kB | 25.11 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | shell-quote | 1.8.2 | 23.54 kB | 23.54 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | semifies | 1.0.0 | 15.84 kB | 15.84 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | ttl-set | 1.0.0 | 4.61 kB | 9.69 kB | | mutexify | 1.4.0 | 5.71 kB | 8.74 kB | | path-to-regexp | 0.1.12 | 6.6 kB | 6.6 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | module-details-from-path | 1.0.4 | 3.96 kB | 3.96 kB |

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

codecov · 2025-05-30T05:49:38Z

Codecov Report

Attention: Patch coverage is 95.45455% with 4 lines in your changes missing coverage. Please review.

Project coverage is 79.15%. Comparing base (569c46d) to head (079af0e).

Files with missing lines	Patch %	Lines
packages/dd-trace/src/appsec/reporter.js	88.23%	2 Missing ⚠️
packages/dd-trace/src/appsec/waf/waf_manager.js	88.23%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5792   +/-   ##
=======================================
  Coverage   79.15%   79.15%           
=======================================
  Files         522      522           
  Lines       24010    23973   -37     
=======================================
- Hits        19005    18976   -29     
+ Misses       5005     4997    -8

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

datadog-datadog-prod-us1 · 2025-05-30T06:00:39Z

Datadog Report

Branch report: ccapell/new-waf-builder
Commit report: d5eb034
Test service: dd-trace-js-integration-tests

✅ 0 Failed, 1257 Passed, 0 Skipped, 25m 36.12s Total Time

pr-commenter · 2025-05-30T06:01:27Z

Benchmarks

Benchmark execution time: 2025-06-02 13:38:41

Comparing candidate commit 079af0e in PR branch ccapell/new-waf-builder with baseline commit 569c46d in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 1273 metrics, 50 unstable metrics.

BridgeAR

I can't tell anything about the domain logic in this case, I just had a brief look at the overall code and left a few questions and suggestions :)

BridgeAR · 2025-05-30T10:17:17Z

packages/dd-trace/src/appsec/reporter.js

+      telemetryLogCh.publish({
+        message: diagnostics[configKey].error,
+        level: 'ERROR',
+        tags: {
+          log_type: `rc::${product.toLowerCase()}::diagnostic`,
+          appsec_config_key: configKey,
+          rc_config_id: rcConfigId
+        }
+      })


I would abstract this in a small helper with the rule of three

New helper has been implemented to publish these diagnostics log messages.

BridgeAR · 2025-05-30T10:19:10Z

packages/dd-trace/src/appsec/rule_manager.js

  for (const item of toUnapply) {
-    const { product, id } = item
+    if (!['ASM_DD', 'ASM_DATA', 'ASM'].includes(item.product)) continue


Right now this array is created in each loop iteration. Maybe move it outside of the method and make it a set so that there is only a single lookup happening per iteration? :)

Suggested change

if (!['ASM_DD', 'ASM_DATA', 'ASM'].includes(item.product)) continue

if (PRODUCTS.has(item.product)) continue

The same applies in other places.

Suggestion has been applied.

BridgeAR · 2025-05-30T10:21:52Z

packages/dd-trace/src/appsec/rule_manager.js

+    } catch (e) {
+      wafUpdatedSuccess = false
+      item.apply_state = ERROR
+      item.apply_error = e.toString()


Is it intentional to only care about the error type and message? Calling .toString() is pretty bad for Node.js errors such that the stack and other properties are all lost. In case the error should be inspected, I would just use util.inspect(error).

The same for other spots.

Yes, it is. Tracer only reports error type and message in apply_error for RC items.

https://github.com/DataDog/dd-trace-js/blob/master/packages/dd-trace/src/remote_config/manager.js#L299

https://github.com/DataDog/dd-trace-js/blob/master/packages/dd-trace/src/remote_config/manager.js#L314

https://github.com/DataDog/dd-trace-js/blob/master/packages/dd-trace/src/remote_config/manager.js#L323

BridgeAR · 2025-05-30T10:26:25Z

packages/dd-trace/src/appsec/rule_manager.js

-      if (file?.custom_rules?.length) {
-        newCustomRules.set(id, file.custom_rules)
+        wafUpdatedSuccess = false
+        item.apply_error = JSON.stringify(extractErrors(updateResult.diagnostics))


The errors are handled very differently depending on where it happens while the overall logical seems to almost be identical. Should they not be handled all the same? :)

BridgeAR · 2025-05-30T10:30:15Z

packages/dd-trace/src/appsec/rule_manager.js

-    copy.data = data
-  }
-  return copy
+  return Object.keys(result).length > 0 ? result : null


Instead of checking for results keys, I would just use a boolean that I set to true in case the loop happened to match something, since calling Object.keys() is quite expensive to just get that information.

In general, is the object really arbitrary that we have to loop over it completely in an unknown way?

Suggestion's been applied.

Regarding the arbitrariness of the objetc, no, the object is not really arbitrary, it has a defined schema (at least, for this version of libddwaf).

IlyasShabi · 2025-06-02T07:53:34Z

packages/dd-trace/src/appsec/waf/waf_manager.js

-      if (this.ddwaf.diagnostics.ruleset_version) {
-        this.rulesVersion = this.ddwaf.diagnostics.ruleset_version
-      }
+    const success = this.ddwaf.createOrUpdateConfig(rules, path)


createOrUpdateConfig could throw an error, I think we need a try/catch block

IlyasShabi · 2025-06-02T08:00:30Z

packages/dd-trace/src/appsec/waf/waf_manager.js

+      this.ddwaf.createOrUpdateConfig(this.defaultRules, DEFAULT_WAF_CONFIG_PATH)
+    }
+
+    return { success, diagnostics }


I think that we need to update this.rulesVersion = this.ddwaf.diagnostics.ruleset_version here too

IlyasShabi · 2025-06-02T08:02:48Z

packages/dd-trace/src/appsec/rule_manager.js

+      wafUpdatedSuccess = false
+      item.apply_state = ERROR
+      item.apply_error = e.toString()
+      Reporter.reportWafConfigError(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion)


This means that we gonna report probably a wrong rulesVersion if remove fails no?

This reverts commit f8d8805.

uurien · 2025-06-02T15:06:07Z

packages/dd-trace/src/appsec/reporter.js

@@ -225,6 +242,54 @@ function reportWafInit (wafVersion, rulesVersion, diagnosticsRules = {}, success
  incrementWafInitMetric(wafVersion, rulesVersion, success)
 }

+function logWafDiagnosticMessage (product, rcConfigId, configKey, message, level) {
+  telemetryLogCh.publish({


These logs are sent via telemetry, but aren't shown in the output if the logs are enabled, is the expected behaviour?

The RFC only establishes the propagation of diagnostics by telemetry and RC. Specifically it states that:

Note that aside from through telemetry logs, none of the diagnostics presented here should be logged to a regular logger using a level above debug, as it will inevitably result in customer complaints.

CarlesDD added asm-waf semver-minor appsec labels May 30, 2025

CarlesDD marked this pull request as ready for review May 30, 2025 08:57

CarlesDD requested review from a team as code owners May 30, 2025 08:57

BridgeAR reviewed May 30, 2025

View reviewed changes

CarlesDD force-pushed the ccapell/new-waf-builder branch from c570c22 to 9872b5d Compare June 1, 2025 09:38

IlyasShabi reviewed Jun 2, 2025

View reviewed changes

CarlesDD added 17 commits June 2, 2025 15:29

WIP

ccc741a

Clean up + apply_error

f9b4242

WAF update diagnostics propagation through RC and telemetry

cf6b37c

Change metrics for waf update and config errors

bf584c8

Remove deprecated span tags

85e40fd

Fix report waf update

fb8f7dd

Tests

66712f6

Tests for Rule Manager

0417683

More tests

ec54f44

Improve assert

5174d97

Tests

00b9cdd

Clean up

efbf2a6

Clean up

50da11a

Native appsec package bump

1d79770

Revert "Remove deprecated span tags"

2ad92a6

This reverts commit f8d8805.

Revert tag deprecation

1987802

Clean up

ac9b1d6

CarlesDD added 5 commits June 2, 2025 15:29

Lint

c5841ab

Fix assertion

479c2e9

Improve RC product check

66e1b28

Avoid checking obj keys in rule manager extract errors

1a2ebc9

Abstract log message publish in reporter

079af0e

CarlesDD force-pushed the ccapell/new-waf-builder branch from 9872b5d to 079af0e Compare June 2, 2025 13:29

uurien reviewed Jun 2, 2025

View reviewed changes

	if (!['ASM_DD', 'ASM_DATA', 'ASM'].includes(item.product)) continue
	if (PRODUCTS.has(item.product)) continue

Upgrade libddwaf native bindings #5792

Are you sure you want to change the base?

Upgrade libddwaf native bindings #5792

Uh oh!

Conversation

CarlesDD commented May 30, 2025 • edited by jira bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Additional Notes

Uh oh!

github-actions bot commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overall package size

Uh oh!

codecov bot commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

datadog-datadog-prod-us1 bot commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Datadog Report

Uh oh!

pr-commenter bot commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CarlesDD Jun 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CarlesDD commented May 30, 2025 •

edited by jira bot

Loading

github-actions bot commented May 30, 2025 •

edited

Loading

codecov bot commented May 30, 2025 •

edited

Loading

datadog-datadog-prod-us1 bot commented May 30, 2025 •

edited

Loading

pr-commenter bot commented May 30, 2025 •

edited

Loading

CarlesDD Jun 1, 2025 •

edited

Loading