From b11aaadcba04792f6edfe37c78ec8c3b7f4ef10c Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Tue, 22 Apr 2025 16:45:21 +0200 Subject: [PATCH 01/37] WIP --- package.json | 7 +- packages/dd-trace/src/appsec/rule_manager.js | 64 ++++++++++++++++ packages/dd-trace/src/appsec/waf/index.js | 1 + .../dd-trace/src/appsec/waf/waf_manager.js | 74 ++++++++++++++++++- packages/dd-trace/src/remote_config/index.js | 7 +- .../dd-trace/src/remote_config/manager.js | 4 + 6 files changed, 151 insertions(+), 6 deletions(-) diff --git a/package.json b/package.json index b16b1cbec9b..4a9f6d47b02 100644 --- a/package.json +++ b/package.json @@ -85,7 +85,7 @@ }, "dependencies": { "@datadog/libdatadog": "0.7.0", - "@datadog/native-appsec": "8.5.2", + "@datadog/native-appsec": "file:datadog-native-appsec-8.5.2.tgz", "@datadog/native-iast-taint-tracking": "4.0.0", "@datadog/native-metrics": "3.1.1", "@datadog/pprof": "5.9.0", @@ -156,5 +156,8 @@ "tiktoken": "^1.0.21", "yaml": "^2.8.0", "yarn-deduplicate": "^6.0.2" - } + }, + "bundledDependencies": [ + "@datadog/native-appsec" + ] } diff --git a/packages/dd-trace/src/appsec/rule_manager.js b/packages/dd-trace/src/appsec/rule_manager.js index 10a66755d49..9937e352b4d 100644 --- a/packages/dd-trace/src/appsec/rule_manager.js +++ b/packages/dd-trace/src/appsec/rule_manager.js @@ -26,6 +26,67 @@ function loadRules (config) { } function updateWafFromRC ({ toUnapply, toApply, toModify }) { + console.log('\n\nupdateWafFromRC') + //console.log('toUnapply', toUnapply) + //console.log('toApply', toApply) + //console.log('toModify', toModify) + + + for (const item of toUnapply) { + // TODO Maybe item.product.startsWith('ASM') ?? or setting product handler properly + console.log('toUnapply', item.product, item.path) + if (!['ASM_DD', 'ASM_DATA', 'ASM'].includes(item.product)) continue + + waf.wafManager.remove(item.path) + console.log(item.path, item.apply_state, item.apply_error) + } + + // TODO refactor and add try catch + for (const item of toApply) { + try { + // TODO Maybe item.product.startsWith('ASM') ?? or setting product handler properly + console.log('toApply', item.product, item.path) + if (!['ASM_DD', 'ASM_DATA', 'ASM'].includes(item.product)) continue + + let updateResult + if (item.product === 'ASM_DD') { + updateResult = waf.wafManager.updateASMDD(item.file, item.path) + } else { + updateResult = waf.wafManager.update(item.file, item.path) + } + + if (updateResult.success) { + item.apply_state = ACKNOWLEDGED + } else { + item.apply_state = ERROR + item.apply_error = updateResult.error + } + console.log(item.path, item.apply_state, item.apply_error) + } catch (e) { + console.log(e) + } + } + + for (const item of toModify) { + try { + // TODO Maybe item.product.startsWith('ASM') ?? or setting product handler properly + console.log('toModify', item.product, item.path) + if (!['ASM_DD', 'ASM_DATA', 'ASM'].includes(item.product)) continue + + const updateResult = waf.wafManager.update(item.file, item.path) + if (updateResult.success) { + item.apply_state = ACKNOWLEDGED + } else { + item.apply_state = ERROR + item.apply_error = updateResult.error + } + console.log(item.path, item.apply_state, item.apply_error) + } catch (e) { + console.log(e) + } + } + + /* const batch = new Set() const newRulesData = new SpyMap(appliedRulesData) @@ -155,9 +216,12 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { } for (const config of batch) { + console.log('@@ Rule Manager applying states before', config.apply_state, config.apply_error) config.apply_state = newApplyState if (newApplyError) config.apply_error = newApplyError + console.log('@@ Rule Manager applying states', config.apply_state, config.apply_error) } + */ } // A Map with a new prop `modified`, a bool that indicates if the Map was modified diff --git a/packages/dd-trace/src/appsec/waf/index.js b/packages/dd-trace/src/appsec/waf/index.js index b025a123f46..94095acb148 100644 --- a/packages/dd-trace/src/appsec/waf/index.js +++ b/packages/dd-trace/src/appsec/waf/index.js @@ -35,6 +35,7 @@ function destroy () { } function update (newRules) { + console.log('call update with'/*, newRules*/) // TODO: check race conditions between Appsec enable/disable and WAF updates, the whole RC state management in general if (!waf.wafManager) throw new Error('Cannot update disabled WAF') diff --git a/packages/dd-trace/src/appsec/waf/waf_manager.js b/packages/dd-trace/src/appsec/waf/waf_manager.js index e745818aa9e..6f6b73bcfc6 100644 --- a/packages/dd-trace/src/appsec/waf/waf_manager.js +++ b/packages/dd-trace/src/appsec/waf/waf_manager.js @@ -6,12 +6,15 @@ const WAFContextWrapper = require('./waf_context_wrapper') const contexts = new WeakMap() +const DEFAULT_WAF_CONFIG_PATH = 'datadog/00/ASM_DD/default/config' + class WAFManager { constructor (rules, config) { this.config = config this.wafTimeout = config.wafTimeout this.ddwaf = this._loadDDWAF(rules) this.rulesVersion = this.ddwaf.diagnostics.ruleset_version + this.defaultRules = rules Reporter.reportWafInit(this.ddwafVersion, this.rulesVersion, this.ddwaf.diagnostics.rules, true) } @@ -23,8 +26,9 @@ class WAFManager { this.ddwafVersion = DDWAF.version() const { obfuscatorKeyRegex, obfuscatorValueRegex } = this.config - return new DDWAF(rules, { obfuscatorKeyRegex, obfuscatorValueRegex }) + return new DDWAF(rules, DEFAULT_WAF_CONFIG_PATH, { obfuscatorKeyRegex, obfuscatorValueRegex }) } catch (err) { + console.log('what happened', err) this.ddwafVersion = this.ddwafVersion || 'unknown' Reporter.reportWafInit(this.ddwafVersion, 'unknown') @@ -51,6 +55,7 @@ class WAFManager { return wafContext } + /* update (newRules) { try { this.ddwaf.update(newRules) @@ -66,6 +71,73 @@ class WAFManager { throw error } } + */ + + updateASMDD(config, path) { + console.log(this.ddwaf.configPaths) + if (this.ddwaf.configPaths.includes(DEFAULT_WAF_CONFIG_PATH)) { + console.log('Removing default config') + this.ddwaf.removeConfig(DEFAULT_WAF_CONFIG_PATH) + } + + const success = this.ddwaf.createOrUpdateConfig(config, path) + + if (!this.ddwaf.configPaths.some(cp => cp.includes('ASM_DD'))) { + console.log('Reverting default rules') + this.ddwaf.createOrUpdateConfig(this.defaultRules, DEFAULT_WAF_CONFIG_PATH) + } + + if (this.ddwaf.diagnostics.ruleset_version) { + this.rulesVersion = this.ddwaf.diagnostics.ruleset_version + } + + Reporter.reportWafUpdate(this.ddwafVersion, this.rulesVersion, success) + return { success } + } + + update (rules, path) { + try { + console.log('removeConfig path', path) + this.ddwaf.removeConfig(path) + console.log('createOrUpdateConfig path', path) + const success = this.ddwaf.createOrUpdateConfig(rules, path) + + if (this.ddwaf.diagnostics.ruleset_version) { + this.rulesVersion = this.ddwaf.diagnostics.ruleset_version + } + + Reporter.reportWafUpdate(this.ddwafVersion, this.rulesVersion, success) + return { success } + } catch (error) { + Reporter.reportWafUpdate(this.ddwafVersion, 'unknown', false) + + throw error + } + + } + + remove (path) { + try { + console.log('removeConfig path', path) + this.ddwaf.removeConfig(path) + + if (!this.ddwaf.configPaths.some(cp => cp.includes('ASM_DD'))) { + console.log('Reverting default rules') + this.ddwaf.createOrUpdateConfig(this.defaultRules, DEFAULT_WAF_CONFIG_PATH) + } + + if (this.ddwaf.diagnostics.ruleset_version) { + this.rulesVersion = this.ddwaf.diagnostics.ruleset_version + } + + Reporter.reportWafUpdate(this.ddwafVersion, this.rulesVersion, true) + } catch (error) { + Reporter.reportWafUpdate(this.ddwafVersion, 'unknown', false) + + throw error + } + } + destroy () { if (this.ddwaf) { diff --git a/packages/dd-trace/src/remote_config/index.js b/packages/dd-trace/src/remote_config/index.js index ab149bdf000..55702586227 100644 --- a/packages/dd-trace/src/remote_config/index.js +++ b/packages/dd-trace/src/remote_config/index.js @@ -6,6 +6,7 @@ const RemoteConfigManager = require('./manager') const RemoteConfigCapabilities = require('./capabilities') const { setCollectionMode } = require('../appsec/user_tracking') const log = require('../log') +const RuleManager = require("../appsec/rule_manager"); let rc @@ -135,9 +136,9 @@ function disableWafUpdate () { rc.updateCapabilities(RemoteConfigCapabilities.ASM_RASP_SHI, false) rc.updateCapabilities(RemoteConfigCapabilities.ASM_RASP_CMDI, false) - rc.removeProductHandler('ASM_DATA') - rc.removeProductHandler('ASM_DD') - rc.removeProductHandler('ASM') + //rc.removeProductHandler('ASM_DATA') + //rc.removeProductHandler('ASM_DD') + //rc.removeProductHandler('ASM') rc.off(RemoteConfigManager.kPreUpdate, RuleManager.updateWafFromRC) } diff --git a/packages/dd-trace/src/remote_config/manager.js b/packages/dd-trace/src/remote_config/manager.js index a95f281fb86..dbff5a96c0e 100644 --- a/packages/dd-trace/src/remote_config/manager.js +++ b/packages/dd-trace/src/remote_config/manager.js @@ -270,6 +270,7 @@ class RemoteConfigManager extends EventEmitter { for (const item of list) { // TODO: we need a way to tell if unapply configs were handled by kPreUpdate or not, because they're always // emitted unlike the apply and modify configs + console.log('\n\n@@ Dispatching item - before', item.apply_state, item.apply_error) this._callHandlerFor(action, item) @@ -278,6 +279,7 @@ class RemoteConfigManager extends EventEmitter { } else { this.appliedConfigs.set(item.path, item) } + console.log('@@ Dispatching item - after', item.apply_state, item.apply_error) } } @@ -305,7 +307,9 @@ class RemoteConfigManager extends EventEmitter { // If the handler doesn't accept an `ack` callback, assume `apply_state` is `ACKNOWLEDGED`, // unless it returns a promise, in which case we wait for the promise to be resolved or rejected. // TODO: do we want to pass old and new config ? + console.log('@@ _callHandlerFor before handler', item.apply_state, item.apply_error) const result = handler(action, item.file, item.id) + console.log('@@ _callHandlerFor after handler', item.apply_state, item.apply_error) if (result instanceof Promise) { result.then( () => { item.apply_state = ACKNOWLEDGED }, From 6afc32a16d847a2b39f5ca5715eb72e3a0741ce7 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Thu, 24 Apr 2025 17:03:34 +0200 Subject: [PATCH 02/37] Clean up + apply_error --- packages/dd-trace/src/appsec/rule_manager.js | 266 ++---------------- .../dd-trace/src/appsec/waf/waf_manager.js | 66 ++--- 2 files changed, 41 insertions(+), 291 deletions(-) diff --git a/packages/dd-trace/src/appsec/rule_manager.js b/packages/dd-trace/src/appsec/rule_manager.js index 9937e352b4d..1194a87c12a 100644 --- a/packages/dd-trace/src/appsec/rule_manager.js +++ b/packages/dd-trace/src/appsec/rule_manager.js @@ -6,17 +6,10 @@ const { ACKNOWLEDGED, ERROR } = require('../remote_config/apply_states') const blocking = require('./blocking') -let defaultRules - -let appliedRulesData = new Map() -let appliedRulesetId -let appliedRulesOverride = new Map() -let appliedExclusions = new Map() -let appliedCustomRules = new Map() let appliedActions = new Map() function loadRules (config) { - defaultRules = config.rules + const defaultRules = config.rules ? JSON.parse(fs.readFileSync(config.rules)) : require('./recommended.json') @@ -26,202 +19,45 @@ function loadRules (config) { } function updateWafFromRC ({ toUnapply, toApply, toModify }) { - console.log('\n\nupdateWafFromRC') - //console.log('toUnapply', toUnapply) - //console.log('toApply', toApply) - //console.log('toModify', toModify) - + const newActions = new SpyMap(appliedActions) for (const item of toUnapply) { - // TODO Maybe item.product.startsWith('ASM') ?? or setting product handler properly - console.log('toUnapply', item.product, item.path) if (!['ASM_DD', 'ASM_DATA', 'ASM'].includes(item.product)) continue - waf.wafManager.remove(item.path) - console.log(item.path, item.apply_state, item.apply_error) - } - - // TODO refactor and add try catch - for (const item of toApply) { try { - // TODO Maybe item.product.startsWith('ASM') ?? or setting product handler properly - console.log('toApply', item.product, item.path) - if (!['ASM_DD', 'ASM_DATA', 'ASM'].includes(item.product)) continue - - let updateResult - if (item.product === 'ASM_DD') { - updateResult = waf.wafManager.updateASMDD(item.file, item.path) - } else { - updateResult = waf.wafManager.update(item.file, item.path) - } - - if (updateResult.success) { - item.apply_state = ACKNOWLEDGED - } else { - item.apply_state = ERROR - item.apply_error = updateResult.error - } - console.log(item.path, item.apply_state, item.apply_error) + waf.wafManager.remove(item.path) } catch (e) { - console.log(e) - } - } - - for (const item of toModify) { - try { - // TODO Maybe item.product.startsWith('ASM') ?? or setting product handler properly - console.log('toModify', item.product, item.path) - if (!['ASM_DD', 'ASM_DATA', 'ASM'].includes(item.product)) continue - - const updateResult = waf.wafManager.update(item.file, item.path) - if (updateResult.success) { - item.apply_state = ACKNOWLEDGED - } else { - item.apply_state = ERROR - item.apply_error = updateResult.error - } - console.log(item.path, item.apply_state, item.apply_error) - } catch (e) { - console.log(e) - } - } - - /* - const batch = new Set() - - const newRulesData = new SpyMap(appliedRulesData) - let newRuleset - let newRulesetId - const newRulesOverride = new SpyMap(appliedRulesOverride) - const newExclusions = new SpyMap(appliedExclusions) - const newCustomRules = new SpyMap(appliedCustomRules) - const newActions = new SpyMap(appliedActions) - - for (const item of toUnapply) { - const { product, id } = item - - if (product === 'ASM_DATA') { - newRulesData.delete(id) - } else if (product === 'ASM_DD') { - if (appliedRulesetId === id) { - newRuleset = defaultRules - } - } else if (product === 'ASM') { - newRulesOverride.delete(id) - newExclusions.delete(id) - newCustomRules.delete(id) - newActions.delete(id) + item.apply_state = ERROR + item.apply_error = e.toString() } } for (const item of [...toApply, ...toModify]) { - const { product, id, file } = item - - if (product === 'ASM_DATA') { - if (file && file.rules_data && file.rules_data.length) { - newRulesData.set(id, file.rules_data) - } - - batch.add(item) - } else if (product === 'ASM_DD') { - if (appliedRulesetId && appliedRulesetId !== id && newRuleset !== defaultRules) { - item.apply_state = ERROR - item.apply_error = 'Multiple ruleset received in ASM_DD' - } else { - if (file?.rules?.length) { - const { version, metadata, rules, processors, scanners } = file - - newRuleset = { version, metadata, rules, processors, scanners } - newRulesetId = id - } - - batch.add(item) - } - } else if (product === 'ASM') { - if (file?.rules_override?.length) { - newRulesOverride.set(id, file.rules_override) - } - - if (file?.exclusions?.length) { - newExclusions.set(id, file.exclusions) - } - - if (file?.custom_rules?.length) { - newCustomRules.set(id, file.custom_rules) - } - - if (file?.actions?.length) { - newActions.set(id, file.actions) - } - - batch.add(item) - } - } - - let newApplyState = ACKNOWLEDGED - let newApplyError - - if (newRulesData.modified || - newRuleset || - newRulesOverride.modified || - newExclusions.modified || - newCustomRules.modified || - newActions.modified - ) { - const payload = newRuleset || {} - - if (newRulesData.modified) { - payload.rules_data = mergeRulesData(newRulesData) - } - if (newRulesOverride.modified) { - payload.rules_override = concatArrays(newRulesOverride) - } - if (newExclusions.modified) { - payload.exclusions = concatArrays(newExclusions) - } - if (newCustomRules.modified) { - payload.custom_rules = concatArrays(newCustomRules) - } - if (newActions.modified) { - payload.actions = concatArrays(newActions) - } + if (!['ASM_DD', 'ASM_DATA', 'ASM'].includes(item.product)) continue try { - waf.update(payload) - - if (newRulesData.modified) { - appliedRulesData = newRulesData - } - if (newRuleset) { - appliedRulesetId = newRulesetId - } - if (newRulesOverride.modified) { - appliedRulesOverride = newRulesOverride - } - if (newExclusions.modified) { - appliedExclusions = newExclusions - } - if (newCustomRules.modified) { - appliedCustomRules = newCustomRules + const updateResult = waf.wafManager.update(item.product, item.file, item.path) + item.apply_state = updateResult.success ? ACKNOWLEDGED : ERROR + item.apply_error = updateResult.error + + // check asm actions + if (updateResult.success && item.product === 'ASM') { + if (item.file?.actions?.length) { + newActions.set(item.id, item.file.actions) + } } - if (newActions.modified) { - appliedActions = newActions - blocking.setDefaultBlockingActionParameters(concatArrays(newActions)) - } - } catch (err) { - newApplyState = ERROR - newApplyError = err.toString() + } catch (e) { + item.apply_state = ERROR + item.apply_error = e.toString() } } - for (const config of batch) { - console.log('@@ Rule Manager applying states before', config.apply_state, config.apply_error) - config.apply_state = newApplyState - if (newApplyError) config.apply_error = newApplyError - console.log('@@ Rule Manager applying states', config.apply_state, config.apply_error) + // Manage blocking actions + if (newActions.modified) { + appliedActions = newActions + blocking.setDefaultBlockingActionParameters(concatArrays(newActions)) } - */ } // A Map with a new prop `modified`, a bool that indicates if the Map was modified @@ -252,67 +88,9 @@ function concatArrays (files) { return [...files.values()].flat() } -/* - ASM_DATA Merge strategy: - The merge should be based on the id and type. For any duplicate items, the longer expiration should be taken. - As a result, multiple Rule Data may use the same DATA_ID and DATA_TYPE. In this case, all values are considered part - of a set and are merged. For instance, a denylist customized by environment may use a global Rule Data for all - environments and a Rule Data per environment -*/ - -function mergeRulesData (files) { - const mergedRulesData = new Map() - for (const [, file] of files) { - for (const ruleData of file) { - const key = `${ruleData.id}+${ruleData.type}` - if (mergedRulesData.has(key)) { - const existingRulesData = mergedRulesData.get(key) - ruleData.data.reduce(rulesReducer, existingRulesData.data) - } else { - mergedRulesData.set(key, copyRulesData(ruleData)) - } - } - } - return [...mergedRulesData.values()] -} - -function rulesReducer (existingEntries, rulesDataEntry) { - const existingEntry = existingEntries.find((entry) => entry.value === rulesDataEntry.value) - if (existingEntry && !('expiration' in existingEntry)) return existingEntries - if (existingEntry && 'expiration' in rulesDataEntry && rulesDataEntry.expiration > existingEntry.expiration) { - existingEntry.expiration = rulesDataEntry.expiration - } else if (existingEntry && !('expiration' in rulesDataEntry)) { - delete existingEntry.expiration - } else if (!existingEntry) { - existingEntries.push({ ...rulesDataEntry }) - } - return existingEntries -} - -function copyRulesData (rulesData) { - const copy = { ...rulesData } - if (copy.data) { - const data = [] - copy.data.forEach(item => { - data.push({ ...item }) - }) - copy.data = data - } - return copy -} - function clearAllRules () { waf.destroy() - - defaultRules = undefined - - appliedRulesData.clear() - appliedRulesetId = undefined - appliedRulesOverride.clear() - appliedExclusions.clear() - appliedCustomRules.clear() appliedActions.clear() - blocking.setDefaultBlockingActionParameters(undefined) } diff --git a/packages/dd-trace/src/appsec/waf/waf_manager.js b/packages/dd-trace/src/appsec/waf/waf_manager.js index 6f6b73bcfc6..60700033f74 100644 --- a/packages/dd-trace/src/appsec/waf/waf_manager.js +++ b/packages/dd-trace/src/appsec/waf/waf_manager.js @@ -7,6 +7,10 @@ const WAFContextWrapper = require('./waf_context_wrapper') const contexts = new WeakMap() const DEFAULT_WAF_CONFIG_PATH = 'datadog/00/ASM_DD/default/config' +const DIAGNOSTICS_KEYS_TO_KEEP_APPLY_ERROR = [ + "error", "errors", + "exclusions", "rules", "processors", "rules_override", "rules_data", "custom_rules", "actions", "scanners" +] class WAFManager { constructor (rules, config) { @@ -19,6 +23,10 @@ class WAFManager { Reporter.reportWafInit(this.ddwafVersion, this.rulesVersion, this.ddwaf.diagnostics.rules, true) } + static computeUpdateWafError (diagnostics) { + return JSON.stringify(diagnostics, DIAGNOSTICS_KEYS_TO_KEEP_APPLY_ERROR) + } + _loadDDWAF (rules) { try { // require in `try/catch` because this can throw at require time @@ -28,7 +36,6 @@ class WAFManager { const { obfuscatorKeyRegex, obfuscatorValueRegex } = this.config return new DDWAF(rules, DEFAULT_WAF_CONFIG_PATH, { obfuscatorKeyRegex, obfuscatorValueRegex }) } catch (err) { - console.log('what happened', err) this.ddwafVersion = this.ddwafVersion || 'unknown' Reporter.reportWafInit(this.ddwafVersion, 'unknown') @@ -55,59 +62,26 @@ class WAFManager { return wafContext } - /* - update (newRules) { + update (product, rules, path) { try { - this.ddwaf.update(newRules) - - if (this.ddwaf.diagnostics.ruleset_version) { - this.rulesVersion = this.ddwaf.diagnostics.ruleset_version + if (product === 'ASM_DD' && this.ddwaf.configPaths.includes(DEFAULT_WAF_CONFIG_PATH)) { + this.ddwaf.removeConfig(DEFAULT_WAF_CONFIG_PATH) } - Reporter.reportWafUpdate(this.ddwafVersion, this.rulesVersion, true) - } catch (error) { - Reporter.reportWafUpdate(this.ddwafVersion, 'unknown', false) - - throw error - } - } - */ - - updateASMDD(config, path) { - console.log(this.ddwaf.configPaths) - if (this.ddwaf.configPaths.includes(DEFAULT_WAF_CONFIG_PATH)) { - console.log('Removing default config') - this.ddwaf.removeConfig(DEFAULT_WAF_CONFIG_PATH) - } - - const success = this.ddwaf.createOrUpdateConfig(config, path) - - if (!this.ddwaf.configPaths.some(cp => cp.includes('ASM_DD'))) { - console.log('Reverting default rules') - this.ddwaf.createOrUpdateConfig(this.defaultRules, DEFAULT_WAF_CONFIG_PATH) - } - - if (this.ddwaf.diagnostics.ruleset_version) { - this.rulesVersion = this.ddwaf.diagnostics.ruleset_version - } - - Reporter.reportWafUpdate(this.ddwafVersion, this.rulesVersion, success) - return { success } - } - - update (rules, path) { - try { - console.log('removeConfig path', path) - this.ddwaf.removeConfig(path) - console.log('createOrUpdateConfig path', path) const success = this.ddwaf.createOrUpdateConfig(rules, path) + const updateDiagnostics = this.ddwaf.diagnostics - if (this.ddwaf.diagnostics.ruleset_version) { + if (updateDiagnostics.ruleset_version) { this.rulesVersion = this.ddwaf.diagnostics.ruleset_version } Reporter.reportWafUpdate(this.ddwafVersion, this.rulesVersion, success) - return { success } + + if (product === 'ASM_DD' && !success && !this.ddwaf.configPaths.some(cp => cp.includes('ASM_DD'))) { + this.ddwaf.createOrUpdateConfig(this.defaultRules, DEFAULT_WAF_CONFIG_PATH) + } + + return { success, error: !success ? WAFManager.computeUpdateWafError(updateDiagnostics) : undefined } } catch (error) { Reporter.reportWafUpdate(this.ddwafVersion, 'unknown', false) @@ -118,7 +92,6 @@ class WAFManager { remove (path) { try { - console.log('removeConfig path', path) this.ddwaf.removeConfig(path) if (!this.ddwaf.configPaths.some(cp => cp.includes('ASM_DD'))) { @@ -138,7 +111,6 @@ class WAFManager { } } - destroy () { if (this.ddwaf) { this.ddwaf.dispose() From c1e6dc9d9b508a5c3d59cd356b93d473e83ade9a Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Wed, 30 Apr 2025 13:06:50 +0200 Subject: [PATCH 03/37] WAF update diagnostics propagation through RC and telemetry --- packages/dd-trace/src/appsec/reporter.js | 54 ++++++++++++++++++- packages/dd-trace/src/appsec/rule_manager.js | 13 ++++- .../dd-trace/src/appsec/waf/waf_manager.js | 16 ++---- 3 files changed, 69 insertions(+), 14 deletions(-) diff --git a/packages/dd-trace/src/appsec/reporter.js b/packages/dd-trace/src/appsec/reporter.js index 3aa9e43a480..e9220b1feeb 100644 --- a/packages/dd-trace/src/appsec/reporter.js +++ b/packages/dd-trace/src/appsec/reporter.js @@ -1,5 +1,8 @@ 'use strict' +const dc = require('dc-polyfill') +const zlib = require('zlib') + const Limiter = require('../rate_limiter') const { storage } = require('../../../datadog-core') const web = require('../plugins/util/web') @@ -14,7 +17,6 @@ const { updateRateLimitedMetric, getRequestMetrics } = require('./telemetry') -const zlib = require('zlib') const { keepTrace } = require('../priority_sampler') const { ASM } = require('../standalone/product') @@ -25,6 +27,20 @@ const COLLECTED_REQUEST_BODY_MAX_STRING_LENGTH = 4096 const COLLECTED_REQUEST_BODY_MAX_DEPTH = 20 const COLLECTED_REQUEST_BODY_MAX_ELEMENTS_PER_NODE = 256 +const telemetryLogCh = dc.channel('datadog:telemetry:log') + +const WAF_DIAGNOSTICS_CONFIG_KEYS_TO_REPORT = [ + 'rules', + 'custom_rules', + 'exclusions', + 'actions', + 'processors', + 'scanners', + 'rules_override', + 'rules_data', + 'exclusion_data' +] + // default limiter, configurable with setRateLimit() let limiter = new Limiter(100) @@ -227,6 +243,41 @@ function reportWafInit (wafVersion, rulesVersion, diagnosticsRules = {}, success incrementWafInitMetric(wafVersion, rulesVersion, success) } +function reportSuccessfulWafUpdate (product, rcConfigId, diagnostics) { + for (const configKey of WAF_DIAGNOSTICS_CONFIG_KEYS_TO_REPORT) { + if (!diagnostics[configKey]) continue + + if (diagnostics[configKey].error) { + telemetryLogCh.publish({ + message: diagnostics[configKey].error, + level: 'ERROR', + tags: `log_type:rc::${product}::diagnostic, appsec_config_key:${configKey}, rc_config_id:${rcConfigId}` + }) + continue + } + + if (diagnostics[configKey].errors) { + for (const [errorMessage, errorIds] of Object.entries(diagnostics[configKey].errors)) { + telemetryLogCh.publish({ + message: `"${errorMessage}": ${JSON.stringify(errorIds)}`, + level: 'ERROR', + tags: `log_type:rc::${product}::diagnostic, appsec_config_key:${configKey}, rc_config_id:${rcConfigId}` + }) + } + } + + if (diagnostics[configKey].warnings) { + for (const [warningMessage, warningIds] of Object.entries(diagnostics[configKey].warnings)) { + telemetryLogCh.publish({ + message: `"${warningMessage}": ${JSON.stringify(warningIds)}`, + level: 'ERROR', + tags: `log_type:rc::${product}::diagnostic, appsec_config_key:${configKey}, rc_config_id:${rcConfigId}` + }) + } + } + } +} + function reportMetrics (metrics, raspRule) { const store = storage('legacy').getStore() const rootSpan = store?.req && web.root(store.req) @@ -485,6 +536,7 @@ module.exports = { filterExtendedHeaders, formatHeaderName, reportWafInit, + reportSuccessfulWafUpdate, reportMetrics, reportAttack, reportWafUpdate: incrementWafUpdatesMetric, diff --git a/packages/dd-trace/src/appsec/rule_manager.js b/packages/dd-trace/src/appsec/rule_manager.js index 1194a87c12a..c8594770831 100644 --- a/packages/dd-trace/src/appsec/rule_manager.js +++ b/packages/dd-trace/src/appsec/rule_manager.js @@ -3,9 +3,15 @@ const fs = require('fs') const waf = require('./waf') const { ACKNOWLEDGED, ERROR } = require('../remote_config/apply_states') +const Reporter = require('./reporter') const blocking = require('./blocking') +const DIAGNOSTICS_KEYS_TO_KEEP_APPLY_ERROR = [ + "error", "errors", + "exclusions", "rules", "processors", "rules_override", "rules_data", "custom_rules", "actions", "scanners" +] + let appliedActions = new Map() function loadRules (config) { @@ -38,7 +44,12 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { try { const updateResult = waf.wafManager.update(item.product, item.file, item.path) item.apply_state = updateResult.success ? ACKNOWLEDGED : ERROR - item.apply_error = updateResult.error + + if (updateResult.success) { + Reporter.reportSuccessfulWafUpdate(item.product, item.id, updateResult.diagnostics) + } else { + item.apply_error = JSON.stringify(updateResult.diagnostics, DIAGNOSTICS_KEYS_TO_KEEP_APPLY_ERROR) + } // check asm actions if (updateResult.success && item.product === 'ASM') { diff --git a/packages/dd-trace/src/appsec/waf/waf_manager.js b/packages/dd-trace/src/appsec/waf/waf_manager.js index 60700033f74..38bb5c0d0b0 100644 --- a/packages/dd-trace/src/appsec/waf/waf_manager.js +++ b/packages/dd-trace/src/appsec/waf/waf_manager.js @@ -7,10 +7,6 @@ const WAFContextWrapper = require('./waf_context_wrapper') const contexts = new WeakMap() const DEFAULT_WAF_CONFIG_PATH = 'datadog/00/ASM_DD/default/config' -const DIAGNOSTICS_KEYS_TO_KEEP_APPLY_ERROR = [ - "error", "errors", - "exclusions", "rules", "processors", "rules_override", "rules_data", "custom_rules", "actions", "scanners" -] class WAFManager { constructor (rules, config) { @@ -23,10 +19,6 @@ class WAFManager { Reporter.reportWafInit(this.ddwafVersion, this.rulesVersion, this.ddwaf.diagnostics.rules, true) } - static computeUpdateWafError (diagnostics) { - return JSON.stringify(diagnostics, DIAGNOSTICS_KEYS_TO_KEEP_APPLY_ERROR) - } - _loadDDWAF (rules) { try { // require in `try/catch` because this can throw at require time @@ -69,10 +61,10 @@ class WAFManager { } const success = this.ddwaf.createOrUpdateConfig(rules, path) - const updateDiagnostics = this.ddwaf.diagnostics + const diagnostics = this.ddwaf.diagnostics - if (updateDiagnostics.ruleset_version) { - this.rulesVersion = this.ddwaf.diagnostics.ruleset_version + if (diagnostics.ruleset_version) { + this.rulesVersion = diagnostics.ruleset_version } Reporter.reportWafUpdate(this.ddwafVersion, this.rulesVersion, success) @@ -81,7 +73,7 @@ class WAFManager { this.ddwaf.createOrUpdateConfig(this.defaultRules, DEFAULT_WAF_CONFIG_PATH) } - return { success, error: !success ? WAFManager.computeUpdateWafError(updateDiagnostics) : undefined } + return { success, diagnostics } } catch (error) { Reporter.reportWafUpdate(this.ddwafVersion, 'unknown', false) From 237d6456f64a21fb293b62f7dd50adcdcb4c6d3d Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Fri, 2 May 2025 14:45:24 +0200 Subject: [PATCH 04/37] Change metrics for waf update and config errors --- packages/dd-trace/src/appsec/reporter.js | 2 + packages/dd-trace/src/appsec/rule_manager.js | 11 ++++ .../dd-trace/src/appsec/telemetry/index.js | 12 ++++- packages/dd-trace/src/appsec/telemetry/waf.js | 10 ++-- .../dd-trace/src/appsec/waf/waf_manager.js | 52 ++++++------------- 5 files changed, 46 insertions(+), 41 deletions(-) diff --git a/packages/dd-trace/src/appsec/reporter.js b/packages/dd-trace/src/appsec/reporter.js index e9220b1feeb..d30852ab70d 100644 --- a/packages/dd-trace/src/appsec/reporter.js +++ b/packages/dd-trace/src/appsec/reporter.js @@ -10,6 +10,7 @@ const { ipHeaderList } = require('../plugins/util/ip_extractor') const { incrementWafInitMetric, incrementWafUpdatesMetric, + incrementWafConfigErrorsMetric, incrementWafRequestsMetric, updateWafRequestsMetricTags, updateRaspRequestsMetricTags, @@ -540,6 +541,7 @@ module.exports = { reportMetrics, reportAttack, reportWafUpdate: incrementWafUpdatesMetric, + reportWafConfigError: incrementWafConfigErrorsMetric, reportRaspRuleSkipped: updateRaspRuleSkippedMetricTags, reportDerivatives, finishRequest, diff --git a/packages/dd-trace/src/appsec/rule_manager.js b/packages/dd-trace/src/appsec/rule_manager.js index c8594770831..83f745e9dec 100644 --- a/packages/dd-trace/src/appsec/rule_manager.js +++ b/packages/dd-trace/src/appsec/rule_manager.js @@ -27,14 +27,18 @@ function loadRules (config) { function updateWafFromRC ({ toUnapply, toApply, toModify }) { const newActions = new SpyMap(appliedActions) + let wafUpdated = false + for (const item of toUnapply) { if (!['ASM_DD', 'ASM_DATA', 'ASM'].includes(item.product)) continue try { waf.wafManager.remove(item.path) + wafUpdated = true } catch (e) { item.apply_state = ERROR item.apply_error = e.toString() + Reporter.reportWafConfigError(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion) } } @@ -46,9 +50,11 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { item.apply_state = updateResult.success ? ACKNOWLEDGED : ERROR if (updateResult.success) { + wafUpdated = true Reporter.reportSuccessfulWafUpdate(item.product, item.id, updateResult.diagnostics) } else { item.apply_error = JSON.stringify(updateResult.diagnostics, DIAGNOSTICS_KEYS_TO_KEEP_APPLY_ERROR) + Reporter.reportWafConfigError(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion) } // check asm actions @@ -61,9 +67,14 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { } catch (e) { item.apply_state = ERROR item.apply_error = e.toString() + Reporter.reportWafConfigError(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion) } } + if (wafUpdated) { + Reporter.reportWafUpdate(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion) + } + // Manage blocking actions if (newActions.modified) { appliedActions = newActions diff --git a/packages/dd-trace/src/appsec/telemetry/index.js b/packages/dd-trace/src/appsec/telemetry/index.js index 4926f1fd960..095dceb59e1 100644 --- a/packages/dd-trace/src/appsec/telemetry/index.js +++ b/packages/dd-trace/src/appsec/telemetry/index.js @@ -13,6 +13,7 @@ const { trackWafMetrics, incrementWafInit, incrementWafUpdates, + incrementWafConfigErrors, incrementWafRequests } = require('./waf') const telemetryMetrics = require('../../telemetry/metrics') @@ -145,10 +146,16 @@ function incrementWafInitMetric (wafVersion, rulesVersion, success) { incrementWafInit(wafVersion, rulesVersion, success) } -function incrementWafUpdatesMetric (wafVersion, rulesVersion, success) { +function incrementWafUpdatesMetric (wafVersion, rulesVersion) { if (!enabled) return - incrementWafUpdates(wafVersion, rulesVersion, success) + incrementWafUpdates(wafVersion, rulesVersion) +} + +function incrementWafConfigErrorsMetric (wafVersion, rulesVersion) { + if (!enabled) return + + incrementWafConfigErrors(wafVersion, rulesVersion) } function incrementWafRequestsMetric (req) { @@ -197,6 +204,7 @@ module.exports = { updateRaspRuleSkippedMetricTags, incrementWafInitMetric, incrementWafUpdatesMetric, + incrementWafConfigErrorsMetric, incrementWafRequestsMetric, incrementMissingUserLoginMetric, incrementMissingUserIdMetric, diff --git a/packages/dd-trace/src/appsec/telemetry/waf.js b/packages/dd-trace/src/appsec/telemetry/waf.js index bf8d0e07eae..5e2f2e694b5 100644 --- a/packages/dd-trace/src/appsec/telemetry/waf.js +++ b/packages/dd-trace/src/appsec/telemetry/waf.js @@ -100,13 +100,14 @@ function incrementWafInit (wafVersion, rulesVersion, success) { } } -function incrementWafUpdates (wafVersion, rulesVersion, success) { +function incrementWafUpdates (wafVersion, rulesVersion) { const versionsTags = getVersionsTags(wafVersion, rulesVersion) appsecMetrics.count('waf.updates', { ...versionsTags, success }).inc() +} - if (!success) { - appsecMetrics.count('waf.config_errors', versionsTags).inc() - } +function incrementWafConfigErrors (wafVersion, rulesVersion) { + const versionsTags = getVersionsTags(wafVersion, rulesVersion) + appsecMetrics.count('waf.config_errors', versionsTags).inc() } function incrementWafRequests (store) { @@ -137,5 +138,6 @@ module.exports = { trackWafMetrics, incrementWafInit, incrementWafUpdates, + incrementWafConfigErrors, incrementWafRequests } diff --git a/packages/dd-trace/src/appsec/waf/waf_manager.js b/packages/dd-trace/src/appsec/waf/waf_manager.js index 38bb5c0d0b0..a70f10374a9 100644 --- a/packages/dd-trace/src/appsec/waf/waf_manager.js +++ b/packages/dd-trace/src/appsec/waf/waf_manager.js @@ -55,51 +55,33 @@ class WAFManager { } update (product, rules, path) { - try { - if (product === 'ASM_DD' && this.ddwaf.configPaths.includes(DEFAULT_WAF_CONFIG_PATH)) { - this.ddwaf.removeConfig(DEFAULT_WAF_CONFIG_PATH) - } - - const success = this.ddwaf.createOrUpdateConfig(rules, path) - const diagnostics = this.ddwaf.diagnostics - - if (diagnostics.ruleset_version) { - this.rulesVersion = diagnostics.ruleset_version - } - - Reporter.reportWafUpdate(this.ddwafVersion, this.rulesVersion, success) + if (product === 'ASM_DD' && this.ddwaf.configPaths.includes(DEFAULT_WAF_CONFIG_PATH)) { + this.ddwaf.removeConfig(DEFAULT_WAF_CONFIG_PATH) + } - if (product === 'ASM_DD' && !success && !this.ddwaf.configPaths.some(cp => cp.includes('ASM_DD'))) { - this.ddwaf.createOrUpdateConfig(this.defaultRules, DEFAULT_WAF_CONFIG_PATH) - } + const success = this.ddwaf.createOrUpdateConfig(rules, path) + const diagnostics = this.ddwaf.diagnostics - return { success, diagnostics } - } catch (error) { - Reporter.reportWafUpdate(this.ddwafVersion, 'unknown', false) + if (diagnostics.ruleset_version) { + this.rulesVersion = diagnostics.ruleset_version + } - throw error + if (product === 'ASM_DD' && !success && !this.ddwaf.configPaths.some(cp => cp.includes('ASM_DD'))) { + this.ddwaf.createOrUpdateConfig(this.defaultRules, DEFAULT_WAF_CONFIG_PATH) } + return { success, diagnostics } } remove (path) { - try { - this.ddwaf.removeConfig(path) - - if (!this.ddwaf.configPaths.some(cp => cp.includes('ASM_DD'))) { - console.log('Reverting default rules') - this.ddwaf.createOrUpdateConfig(this.defaultRules, DEFAULT_WAF_CONFIG_PATH) - } + this.ddwaf.removeConfig(path) - if (this.ddwaf.diagnostics.ruleset_version) { - this.rulesVersion = this.ddwaf.diagnostics.ruleset_version - } - - Reporter.reportWafUpdate(this.ddwafVersion, this.rulesVersion, true) - } catch (error) { - Reporter.reportWafUpdate(this.ddwafVersion, 'unknown', false) + if (!this.ddwaf.configPaths.some(cp => cp.includes('ASM_DD'))) { + this.ddwaf.createOrUpdateConfig(this.defaultRules, DEFAULT_WAF_CONFIG_PATH) + } - throw error + if (this.ddwaf.diagnostics.ruleset_version) { + this.rulesVersion = this.ddwaf.diagnostics.ruleset_version } } From 602e0bf3288822798bf6d700b6138e85061e73df Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Fri, 2 May 2025 15:25:02 +0200 Subject: [PATCH 05/37] Remove deprecated span tags --- packages/dd-trace/src/appsec/reporter.js | 8 +------- packages/dd-trace/src/appsec/waf/waf_manager.js | 2 +- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/packages/dd-trace/src/appsec/reporter.js b/packages/dd-trace/src/appsec/reporter.js index d30852ab70d..ae68610afdd 100644 --- a/packages/dd-trace/src/appsec/reporter.js +++ b/packages/dd-trace/src/appsec/reporter.js @@ -230,15 +230,9 @@ function getCollectedHeaders (req, res, shouldCollectEventHeaders, storedRespons return headersTags } -function reportWafInit (wafVersion, rulesVersion, diagnosticsRules = {}, success = false) { +function reportWafInit (wafVersion, rulesVersion, success = false) { if (success) { metricsQueue.set('_dd.appsec.waf.version', wafVersion) - - metricsQueue.set('_dd.appsec.event_rules.loaded', diagnosticsRules.loaded?.length || 0) - metricsQueue.set('_dd.appsec.event_rules.error_count', diagnosticsRules.failed?.length || 0) - if (diagnosticsRules.failed?.length) { - metricsQueue.set('_dd.appsec.event_rules.errors', JSON.stringify(diagnosticsRules.errors)) - } } incrementWafInitMetric(wafVersion, rulesVersion, success) diff --git a/packages/dd-trace/src/appsec/waf/waf_manager.js b/packages/dd-trace/src/appsec/waf/waf_manager.js index a70f10374a9..6a0ff1bec4e 100644 --- a/packages/dd-trace/src/appsec/waf/waf_manager.js +++ b/packages/dd-trace/src/appsec/waf/waf_manager.js @@ -16,7 +16,7 @@ class WAFManager { this.rulesVersion = this.ddwaf.diagnostics.ruleset_version this.defaultRules = rules - Reporter.reportWafInit(this.ddwafVersion, this.rulesVersion, this.ddwaf.diagnostics.rules, true) + Reporter.reportWafInit(this.ddwafVersion, this.rulesVersion, true) } _loadDDWAF (rules) { From c09b37e39adb124798e6f5d30bd23e117e01ed58 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Fri, 2 May 2025 15:42:31 +0200 Subject: [PATCH 06/37] Fix report waf update --- packages/dd-trace/src/appsec/rule_manager.js | 6 +++++- packages/dd-trace/src/appsec/telemetry/index.js | 4 ++-- packages/dd-trace/src/appsec/telemetry/waf.js | 2 +- packages/dd-trace/src/appsec/waf/index.js | 4 ++-- 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/packages/dd-trace/src/appsec/rule_manager.js b/packages/dd-trace/src/appsec/rule_manager.js index 83f745e9dec..3a90792a517 100644 --- a/packages/dd-trace/src/appsec/rule_manager.js +++ b/packages/dd-trace/src/appsec/rule_manager.js @@ -28,6 +28,7 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { const newActions = new SpyMap(appliedActions) let wafUpdated = false + let wafUpdatedSuccess = true for (const item of toUnapply) { if (!['ASM_DD', 'ASM_DATA', 'ASM'].includes(item.product)) continue @@ -36,6 +37,7 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { waf.wafManager.remove(item.path) wafUpdated = true } catch (e) { + wafUpdatedSuccess = false item.apply_state = ERROR item.apply_error = e.toString() Reporter.reportWafConfigError(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion) @@ -53,6 +55,7 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { wafUpdated = true Reporter.reportSuccessfulWafUpdate(item.product, item.id, updateResult.diagnostics) } else { + wafUpdatedSuccess = false item.apply_error = JSON.stringify(updateResult.diagnostics, DIAGNOSTICS_KEYS_TO_KEEP_APPLY_ERROR) Reporter.reportWafConfigError(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion) } @@ -65,6 +68,7 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { } } catch (e) { + wafUpdatedSuccess = false item.apply_state = ERROR item.apply_error = e.toString() Reporter.reportWafConfigError(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion) @@ -72,7 +76,7 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { } if (wafUpdated) { - Reporter.reportWafUpdate(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion) + Reporter.reportWafUpdate(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion, wafUpdatedSuccess) } // Manage blocking actions diff --git a/packages/dd-trace/src/appsec/telemetry/index.js b/packages/dd-trace/src/appsec/telemetry/index.js index 095dceb59e1..a0cd5b5d852 100644 --- a/packages/dd-trace/src/appsec/telemetry/index.js +++ b/packages/dd-trace/src/appsec/telemetry/index.js @@ -146,10 +146,10 @@ function incrementWafInitMetric (wafVersion, rulesVersion, success) { incrementWafInit(wafVersion, rulesVersion, success) } -function incrementWafUpdatesMetric (wafVersion, rulesVersion) { +function incrementWafUpdatesMetric (wafVersion, rulesVersion, success) { if (!enabled) return - incrementWafUpdates(wafVersion, rulesVersion) + incrementWafUpdates(wafVersion, rulesVersion, success) } function incrementWafConfigErrorsMetric (wafVersion, rulesVersion) { diff --git a/packages/dd-trace/src/appsec/telemetry/waf.js b/packages/dd-trace/src/appsec/telemetry/waf.js index 5e2f2e694b5..fa720d2d487 100644 --- a/packages/dd-trace/src/appsec/telemetry/waf.js +++ b/packages/dd-trace/src/appsec/telemetry/waf.js @@ -100,7 +100,7 @@ function incrementWafInit (wafVersion, rulesVersion, success) { } } -function incrementWafUpdates (wafVersion, rulesVersion) { +function incrementWafUpdates (wafVersion, rulesVersion, success) { const versionsTags = getVersionsTags(wafVersion, rulesVersion) appsecMetrics.count('waf.updates', { ...versionsTags, success }).inc() } diff --git a/packages/dd-trace/src/appsec/waf/index.js b/packages/dd-trace/src/appsec/waf/index.js index 94095acb148..bf92a903e8c 100644 --- a/packages/dd-trace/src/appsec/waf/index.js +++ b/packages/dd-trace/src/appsec/waf/index.js @@ -7,7 +7,6 @@ const waf = { wafManager: null, init, destroy, - update, run: noop, disposeContext: noop } @@ -34,8 +33,8 @@ function destroy () { waf.disposeContext = noop } +/* function update (newRules) { - console.log('call update with'/*, newRules*/) // TODO: check race conditions between Appsec enable/disable and WAF updates, the whole RC state management in general if (!waf.wafManager) throw new Error('Cannot update disabled WAF') @@ -46,6 +45,7 @@ function update (newRules) { throw err } } +*/ function run (data, req, raspRule) { if (!req) { From b87b2ccd9874e5b6277ceb064edd86dc08d3a922 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Mon, 5 May 2025 15:20:42 +0200 Subject: [PATCH 07/37] Tests --- packages/dd-trace/test/appsec/index.spec.js | 3 ++ .../dd-trace/test/appsec/reporter.spec.js | 18 +++------- .../test/appsec/telemetry/waf.spec.js | 33 ++++++++++++------- 3 files changed, 29 insertions(+), 25 deletions(-) diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index d92d89a8c4f..2d314ad31c6 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -1312,6 +1312,7 @@ describe('IP blocking', function () { const toModify = [{ product: 'ASM_DATA', id: '1', + path: 'datadog/00/ASM_DATA/test/IP blocking', file: ruleData }] const htmlDefaultContent = blockedTemplate.html @@ -1434,6 +1435,7 @@ describe('IP blocking', function () { const toModifyCustomActions = [{ product: 'ASM', id: 'custom-actions', + path: 'datadog/00/ASM/test/Custom actions/Default content with custom status', file: { actions: [ { @@ -1494,6 +1496,7 @@ describe('IP blocking', function () { const toModifyCustomActions = [{ product: 'ASM', id: 'custom-actions', + path: 'datadog/00/ASM/test/Custom actions/Redirect on error', file: { actions: [ { diff --git a/packages/dd-trace/test/appsec/reporter.spec.js b/packages/dd-trace/test/appsec/reporter.spec.js index 76cd1949d98..c5d4d0c6211 100644 --- a/packages/dd-trace/test/appsec/reporter.spec.js +++ b/packages/dd-trace/test/appsec/reporter.spec.js @@ -138,26 +138,19 @@ describe('reporter', () => { } it('should add some entries to metricsQueue', () => { - Reporter.reportWafInit(wafVersion, rulesVersion, diagnosticsRules, true) + Reporter.reportWafInit(wafVersion, rulesVersion, true) expect(Reporter.metricsQueue.get('_dd.appsec.waf.version')).to.be.eq(wafVersion) - expect(Reporter.metricsQueue.get('_dd.appsec.event_rules.loaded')).to.be.eq(3) - expect(Reporter.metricsQueue.get('_dd.appsec.event_rules.error_count')).to.be.eq(1) - expect(Reporter.metricsQueue.get('_dd.appsec.event_rules.errors')) - .to.be.eq(JSON.stringify(diagnosticsRules.errors)) }) it('should not add entries to metricsQueue with success false', () => { - Reporter.reportWafInit(wafVersion, rulesVersion, diagnosticsRules, false) + Reporter.reportWafInit(wafVersion, rulesVersion, false) expect(Reporter.metricsQueue.get('_dd.appsec.waf.version')).to.be.undefined - expect(Reporter.metricsQueue.get('_dd.appsec.event_rules.loaded')).to.be.undefined - expect(Reporter.metricsQueue.get('_dd.appsec.event_rules.error_count')).to.be.undefined - expect(Reporter.metricsQueue.get('_dd.appsec.event_rules.errors')).to.be.undefined }) it('should call incrementWafInitMetric', () => { - Reporter.reportWafInit(wafVersion, rulesVersion, diagnosticsRules, true) + Reporter.reportWafInit(wafVersion, rulesVersion, true) expect(telemetry.incrementWafInitMetric).to.have.been.calledOnceWithExactly(wafVersion, rulesVersion, true) }) @@ -167,10 +160,7 @@ describe('reporter', () => { const rulesVersion = undefined const diagnosticsRules = undefined - Reporter.reportWafInit(wafVersion, rulesVersion, diagnosticsRules, true) - - expect(Reporter.metricsQueue.get('_dd.appsec.event_rules.loaded')).to.be.eq(0) - expect(Reporter.metricsQueue.get('_dd.appsec.event_rules.error_count')).to.be.eq(0) + Reporter.reportWafInit(wafVersion, rulesVersion, true) expect(telemetry.incrementWafInitMetric).to.have.been.calledOnceWithExactly(wafVersion, rulesVersion, true) }) diff --git a/packages/dd-trace/test/appsec/telemetry/waf.spec.js b/packages/dd-trace/test/appsec/telemetry/waf.spec.js index d562357147a..ba5eedab456 100644 --- a/packages/dd-trace/test/appsec/telemetry/waf.spec.js +++ b/packages/dd-trace/test/appsec/telemetry/waf.spec.js @@ -269,22 +269,33 @@ describe('Appsec Waf Telemetry metrics', () => { expect(metrics.series[1].tags).to.include('event_rules_version:0.0.2') expect(metrics.series[1].tags).to.include('success:true') }) + }) + + describe('incrementWafConfigErrors', () => { + it('should increment waf.config_errors metric', () => { + appsecTelemetry.incrementWafConfigErrorsMetric(wafVersion, rulesVersion) - it('should increment waf.updates and waf.config_errors on failed update', () => { + expect(count).to.have.been.calledOnceWithExactly('waf.config_errors', { + waf_version: wafVersion, + event_rules_version: rulesVersion, + }) + expect(inc).to.have.been.calledOnce + }) + + it('should increment waf.config_errors metric multiple times', () => { sinon.restore() - appsecTelemetry.incrementWafUpdatesMetric(wafVersion, rulesVersion, false) + appsecTelemetry.incrementWafConfigErrorsMetric(wafVersion, rulesVersion, true) + appsecTelemetry.incrementWafConfigErrorsMetric(wafVersion, rulesVersion, true) + appsecTelemetry.incrementWafConfigErrorsMetric(wafVersion, rulesVersion, true) const { metrics } = appsecNamespace.toJSON() - expect(metrics.series.length).to.be.eq(3) - expect(metrics.series[1].metric).to.be.eq('waf.updates') - expect(metrics.series[1].tags).to.include('waf_version:0.0.1') - expect(metrics.series[1].tags).to.include('event_rules_version:0.0.2') - expect(metrics.series[1].tags).to.include('success:false') - - expect(metrics.series[2].metric).to.be.eq('waf.config_errors') - expect(metrics.series[2].tags).to.include('waf_version:0.0.1') - expect(metrics.series[2].tags).to.include('event_rules_version:0.0.2') + expect(metrics.series.length).to.be.eq(1) + expect(metrics.series[0].metric).to.be.eq('waf.config_errors') + expect(metrics.series[0].points.length).to.be.eq(1) + expect(metrics.series[0].points[0][1]).to.be.eq(3) + expect(metrics.series[0].tags).to.include('waf_version:0.0.1') + expect(metrics.series[0].tags).to.include('event_rules_version:0.0.2') }) }) From 88f3c116397b675b3932d324dbd660090f47cbd1 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Mon, 5 May 2025 17:28:15 +0200 Subject: [PATCH 08/37] Tests for Rule Manager --- packages/dd-trace/src/appsec/rule_manager.js | 32 +- .../dd-trace/test/appsec/rule_manager.spec.js | 701 +++++++----------- 2 files changed, 274 insertions(+), 459 deletions(-) diff --git a/packages/dd-trace/src/appsec/rule_manager.js b/packages/dd-trace/src/appsec/rule_manager.js index 3a90792a517..5abbe152606 100644 --- a/packages/dd-trace/src/appsec/rule_manager.js +++ b/packages/dd-trace/src/appsec/rule_manager.js @@ -7,11 +7,6 @@ const Reporter = require('./reporter') const blocking = require('./blocking') -const DIAGNOSTICS_KEYS_TO_KEEP_APPLY_ERROR = [ - "error", "errors", - "exclusions", "rules", "processors", "rules_override", "rules_data", "custom_rules", "actions", "scanners" -] - let appliedActions = new Map() function loadRules (config) { @@ -35,7 +30,12 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { try { waf.wafManager.remove(item.path) + item.apply_state = ACKNOWLEDGED wafUpdated = true + + if (item.product === 'ASM') { + newActions.delete(item.id) + } } catch (e) { wafUpdatedSuccess = false item.apply_state = ERROR @@ -56,7 +56,7 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { Reporter.reportSuccessfulWafUpdate(item.product, item.id, updateResult.diagnostics) } else { wafUpdatedSuccess = false - item.apply_error = JSON.stringify(updateResult.diagnostics, DIAGNOSTICS_KEYS_TO_KEEP_APPLY_ERROR) + item.apply_error = JSON.stringify(extractErrors(updateResult.diagnostics)) Reporter.reportWafConfigError(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion) } @@ -66,7 +66,6 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { newActions.set(item.id, item.file.actions) } } - } catch (e) { wafUpdatedSuccess = false item.apply_state = ERROR @@ -114,6 +113,25 @@ function concatArrays (files) { return [...files.values()].flat() } +function extractErrors (obj) { + if (typeof obj !== 'object' || obj === null) return null + + const result = {} + + for (const [key, value] of Object.entries(obj)) { + if (key === 'error' || key === 'errors') { + result[key] = value + } else if (typeof value === 'object' && value !== null) { + const child = extractErrors(value) + if (child && Object.keys(child).length > 0) { + result[key] = child + } + } + } + + return Object.keys(result).length > 0 ? result : null +} + function clearAllRules () { waf.destroy() appliedActions.clear() diff --git a/packages/dd-trace/test/appsec/rule_manager.spec.js b/packages/dd-trace/test/appsec/rule_manager.spec.js index 0da5e7dbec9..22e9a4802b5 100644 --- a/packages/dd-trace/test/appsec/rule_manager.spec.js +++ b/packages/dd-trace/test/appsec/rule_manager.spec.js @@ -1,13 +1,15 @@ 'use strict' -const path = require('path') +const { assert } = require('chai') const fs = require('fs') -const { loadRules, clearAllRules, updateWafFromRC } = require('../../src/appsec/rule_manager') +const path = require('path') +const { loadRules, clearAllRules } = require('../../src/appsec/rule_manager') const Config = require('../../src/config') -const { ACKNOWLEDGED } = require('../../src/remote_config/apply_states') +const { ACKNOWLEDGED, UNACKNOWLEDGED, ERROR } = require('../../src/remote_config/apply_states') const rules = require('../../src/appsec/recommended.json') const waf = require('../../src/appsec/waf') +const WAFManager = require('../../src/appsec/waf/waf_manager') const blocking = require('../../src/appsec/blocking') describe('AppSec Rule Manager', () => { @@ -19,7 +21,6 @@ describe('AppSec Rule Manager', () => { sinon.stub(waf, 'init') sinon.stub(waf, 'destroy') - sinon.stub(waf, 'update') sinon.stub(blocking, 'setDefaultBlockingActionParameters') }) @@ -72,437 +73,265 @@ describe('AppSec Rule Manager', () => { }) describe('updateWafFromRC', () => { - describe('ASM_DATA', () => { - it('should call update with modified rules', () => { - const rulesData = { - rules_data: [{ - data: [ - { value: '1.2.3.4' } - ], - id: 'blocked_ips', - type: 'data_with_expiration' - }] - } - - const toModify = [{ + function getRcConfigs () { + return { + toUnapply: [{ + id: 'test.toUnapply', + product: 'ASM_DD', + path: 'test/rule_manager/updateWafFromRC/ASM_DD/01', + file: {}, + apply_state: UNACKNOWLEDGED + }], + toModify: [{ + id: 'test.toModify', product: 'ASM_DATA', - id: '1', - file: rulesData + path: 'test/rule_manager/updateWafFromRC/ASM_DATA/01', + file: { + rules_data: [{ + data: [ + { value: '1.2.3.4' } + ], + id: 'blocked_ips', + type: 'data_with_expiration' + }] + }, + apply_state: UNACKNOWLEDGED + }], + toApply: [{ + id: 'test.toApply', + product: 'ASM', + path: 'test/rule_manager/updateWafFromRC/ASM/01', + file: { + exclusions: [{ + ekey: 'eValue' + }], + rules_override: [{ + roKey: 'roValue' + }], + custom_rules: [{ + piKey: 'piValue' + }] + }, + apply_state: UNACKNOWLEDGED }] - - updateWafFromRC({ toUnapply: [], toApply: [], toModify }) - expect(waf.update).to.have.been.calledOnceWithExactly(rulesData) + } + } + + let RuleManager + let reportWafConfigError, reportSuccessfulWafUpdate, reportWafUpdate + let setDefaultBlockingActionParameters + + beforeEach(() => { + reportWafUpdate = sinon.stub() + reportSuccessfulWafUpdate = sinon.stub() + reportWafConfigError = sinon.stub() + setDefaultBlockingActionParameters = sinon.stub() + + RuleManager = proxyquire.noCallThru()('../src/appsec/rule_manager', { + './reporter': { + reportWafUpdate, + reportSuccessfulWafUpdate, + reportWafConfigError + }, + './blocking': { + setDefaultBlockingActionParameters + } }) - it('should apply/modify the last rule with same id', () => { - const rulesDataFirst = { - rules_data: [{ - data: [ - { value: '1.2.3.4' } - ], - id: 'blocked_ips', - type: 'data_with_expiration' - }] - } + waf.init.callThrough() - const rulesDataSecond = { - rules_data: [{ - data: [ - { value: '4.3.2.1' } - ], - id: 'blocked_ips', - type: 'data_with_expiration' - }] - } + WAFManager.prototype.update = sinon.stub() + WAFManager.prototype.remove = sinon.stub() - const toModify = [ - { - product: 'ASM_DATA', - id: '1', - file: rulesDataFirst - }, - { - product: 'ASM_DATA', - id: '1', - file: rulesDataSecond - } - ] + RuleManager.clearAllRules() + config = new Config() + RuleManager.loadRules(config.appsec) + sinon.resetHistory() + }) - const expectedPayload = { - rules_data: [ - { data: [{ value: '4.3.2.1' }], id: 'blocked_ips', type: 'data_with_expiration' } - ] - } + it('should not apply configs from non ASM products', () => { + const rcConfigsForNonAsmProducts = { + toUnapply: [{ + id: 'test.toUnapply', + product: 'NON_ASM_PRODUCT', + path: 'test/rule_manager/updateWafFromRC/NON_ASM_PRODUCT/01', + file: {}, + apply_state: UNACKNOWLEDGED + }], + toModify: [{ + id: 'test.toModify', + product: 'NON_ASM_PRODUCT', + path: 'test/rule_manager/updateWafFromRC/NON_ASM_PRODUCT/02', + file: {}, + apply_state: UNACKNOWLEDGED + }], + toApply: [{ + id: 'test.toApply', + product: 'NON_ASM_PRODUCT', + path: 'test/rule_manager/updateWafFromRC/NON_ASM_PRODUCT/03', + file: {}, + apply_state: UNACKNOWLEDGED + }] + } - updateWafFromRC({ toUnapply: [], toApply: [], toModify }) - expect(waf.update).to.have.been.calledOnce - expect(waf.update).calledWithExactly(expectedPayload) + assert.doesNotThrow(() => { + RuleManager.updateWafFromRC(rcConfigsForNonAsmProducts) }) - it('should merge all apply/modify rules', () => { - const toModify = [ - { - product: 'ASM_DATA', - id: '1', - file: { - rules_data: [{ - data: [ - { value: '1.2.3.4' } - ], - id: 'blocked_ips', - type: 'data_with_expiration' - }] - } - }, - { - product: 'ASM_DATA', - id: '2', - file: { - rules_data: [{ - data: [ - { value: '4.3.2.1' } - ], - id: 'blocked_ips', - type: 'data_with_expiration' - }] - } - } - ] + sinon.assert.notCalled(waf.wafManager.update) + sinon.assert.notCalled(waf.wafManager.remove) + assert.strictEqual(rcConfigsForNonAsmProducts.toUnapply[0].apply_state, UNACKNOWLEDGED) + assert.notProperty(rcConfigsForNonAsmProducts.toUnapply[0], 'apply_error') + assert.strictEqual(rcConfigsForNonAsmProducts.toModify[0].apply_state, UNACKNOWLEDGED) + assert.notProperty(rcConfigsForNonAsmProducts.toModify[0], 'apply_error') + assert.strictEqual(rcConfigsForNonAsmProducts.toApply[0].apply_state, UNACKNOWLEDGED) + assert.notProperty(rcConfigsForNonAsmProducts.toApply[0], 'apply_error') + }) - const expectedPayload = { - rules_data: [ - { data: [{ value: '1.2.3.4' }, { value: '4.3.2.1' }], id: 'blocked_ips', type: 'data_with_expiration' } - ] - } + it('should apply configs from ASM products', () => { + const rcConfigs = getRcConfigs() + + RuleManager.updateWafFromRC(rcConfigs) + + sinon.assert.calledOnceWithExactly(waf.wafManager.remove, rcConfigs.toUnapply[0].path) + sinon.assert.calledTwice(waf.wafManager.update) + sinon.assert.calledWith( + waf.wafManager.update.getCall(0), + rcConfigs.toApply[0].product, + rcConfigs.toApply[0].file, + rcConfigs.toApply[0].path + ) + sinon.assert.calledWith( + waf.wafManager.update.getCall(1), + rcConfigs.toModify[0].product, + rcConfigs.toModify[0].file, + rcConfigs.toModify[0].path + ) + }) - updateWafFromRC({ toUnapply: [], toApply: [], toModify }) - expect(waf.update).to.have.been.calledOnce - expect(waf.update).calledWithExactly(expectedPayload) - }) + it('should update apply_state and apply_error on successful apply', () => { + WAFManager.prototype.update.returns({ success: true }) - it('should merge all apply/modify and unapply rules', () => { - const toModify = [ - { - product: 'ASM_DATA', - id: '1', - file: { - rules_data: [{ - data: [ - { value: '4.3.2.1' } - ], - id: 'blocked_ips', - type: 'data_with_expiration' - }] - } - }, - { - product: 'ASM_DATA', - id: '2', - file: { - rules_data: [{ - data: [ - { value: '4.3.2.1' } - ], - id: 'blocked_ips', - type: 'data_with_expiration' - }] - } - } - ] + const rcConfigs = getRcConfigs() - const toUnapply = [ - { - product: 'ASM_DATA', - id: '2', - file: { - rules_data: [{ - data: [ - { value: '1.2.3.4' } - ], - id: 'blocked_ips', - type: 'data_with_expiration' - }] - } - } - ] + RuleManager.updateWafFromRC(rcConfigs) - const expectedPayload = { - rules_data: [ - { data: [{ value: '4.3.2.1' }], id: 'blocked_ips', type: 'data_with_expiration' } - ] - } + assert.strictEqual(rcConfigs.toUnapply[0].apply_state, ACKNOWLEDGED) + assert.notProperty(rcConfigs.toUnapply[0], 'apply_error') + assert.strictEqual(rcConfigs.toModify[0].apply_state, ACKNOWLEDGED) + assert.notProperty(rcConfigs.toModify[0], 'apply_error') + assert.strictEqual(rcConfigs.toApply[0].apply_state, ACKNOWLEDGED) + assert.notProperty(rcConfigs.toApply[0], 'apply_error') + }) - updateWafFromRC({ toUnapply, toApply: [], toModify }) - expect(waf.update).to.have.been.calledOnce - expect(waf.update).calledWithExactly(expectedPayload) - }) + it('should update apply_state and apply_error on failed config remove', () => { + const removeConfigError = new Error('Error remove config') + WAFManager.prototype.remove.throws(removeConfigError) - it('should merge all apply/modify rules with different expiration', () => { - // TODO: use case from previous tests, not sure if this can happen. - const toApply = [ - { - product: 'ASM_DATA', - id: '1', - file: { - rules_data: [{ - data: [ - { value: '1.2.3.4', expiration: 200 } - ], - id: 'blocked_ips', - type: 'data_with_expiration' - }] - } - }, - { - product: 'ASM_DATA', - id: '2', - file: { - rules_data: [{ - data: [ - { value: '1.2.3.4', expiration: 100 } - ], - id: 'blocked_ips', - type: 'data_with_expiration' - }] - } - } - ] + const { toUnapply } = getRcConfigs() - const expectedPayload = { - rules_data: [ - { data: [{ value: '1.2.3.4', expiration: 200 }], id: 'blocked_ips', type: 'data_with_expiration' } - ] - } + RuleManager.updateWafFromRC({ toUnapply, toApply: [], toModify: [] }) - updateWafFromRC({ toUnapply: [], toApply, toModify: [] }) - expect(waf.update).to.have.been.calledOnce - expect(waf.update).calledWithExactly(expectedPayload) - }) + assert.strictEqual(toUnapply[0].apply_state, ERROR) + assert.strictEqual(toUnapply[0].apply_error, removeConfigError.toString()) }) - describe('ASM_DD', () => { - beforeEach(() => { - loadRules(config.appsec) - }) - - it('should apply new rules', () => { - const testRules = { - version: '2.2', - metadata: { rules_version: '1.5.0' }, - rules: [{ - id: 'test-id', - name: 'test-name', - tags: { - type: 'security_scanner', - category: 'attack_attempt', - confidence: '1' - }, - conditions: [] - }], - processors: [{ - id: 'test-processor-id', - generator: 'test-generator', - evaluate: false, - output: true - }], - scanners: [{ - id: 'test-scanner-id', - name: 'Test name', - key: { - operator: 'match_regex', - parameters: { - regex: 'test-regex' - } - }, - value: { - operator: 'match_regex', - parameters: { - regex: 'test-regex-2' - } - }, - tags: { - type: 'card', - card_type: 'test', - category: 'payment' - } - }] - } - - const toApply = [ - { - product: 'ASM_DD', - id: '1', - file: testRules + it('should update apply_state and apply_error on failed config update', () => { + const diagnostics = { + rules: { + loaded: [], + failed: ['blk-001-001'], + skipped: [], + errors: { + 'missing key operator': [ + 'blk-001-001' + ] + }, + warnings: { + 'invalid tag': [ + 'blk-001-001' + ] + } + }, + processors: { + loaded: ['http-endpoint-fingerprint'], + failed: [], + skipped: [], + warnings: { + 'no mappings defined': [ + 'http-endpoint-fingerprint' + ] } - ] - - updateWafFromRC({ toUnapply: [], toApply, toModify: [] }) - expect(waf.update).to.have.been.calledOnceWithExactly(testRules) - }) - - it('should maintain previously added exclusions and rules_overrides', () => { - const asm = { - exclusions: [{ - ekey: 'eValue' - }] - } - const testRules = { - version: '2.2', - metadata: { rules_version: '1.5.0' }, - rules: [{ - id: 'test-id', - name: 'test-name', - tags: { - type: 'security_scanner', - category: 'attack_attempt', - confidence: '1' - }, - conditions: [] - }], - processors: [{ - id: 'test-processor-id', - generator: 'test-generator', - evaluate: false, - output: true - }], - scanners: [{ - id: 'test-scanner-id', - name: 'Test name', - key: { - operator: 'match_regex', - parameters: { - regex: 'test-regex' - } - }, - value: { - operator: 'match_regex', - parameters: { - regex: 'test-regex-2' - } - }, - tags: { - type: 'card', - card_type: 'test', - category: 'payment' - } - }] } + } + WAFManager.prototype.update.returns({ success: false, diagnostics }) - const toApply = [ - { - product: 'ASM', - id: '1', - file: asm - }, - { - product: 'ASM_DD', - id: '2', - file: testRules - } - ] + const { toModify, toApply } = getRcConfigs() - updateWafFromRC({ toUnapply: [], toApply, toModify: [] }) - expect(waf.update).to.have.been.calledWithExactly({ ...testRules, ...asm }) - }) + RuleManager.updateWafFromRC({ toUnapply: [], toApply, toModify }) - it('should support hotswapping ruleset in same batch', () => { - const rules1 = { - product: 'ASM_DD', - id: 'rules1', - file: { - version: '2.2', - metadata: { rules_version: '1.5.0' }, - rules: [{ - id: 'test-id', - name: 'test-name', - tags: { - type: 'security_scanner', - category: 'attack_attempt', - confidence: '1' - }, - conditions: [ - { - parameters: { - inputs: [ - { address: 'http.test' } - ], - data: 'blocked_ips' - }, - operator: 'ip_match' - } - ] - }] - } - } + assert.strictEqual(toApply[0].apply_state, ERROR) + assert.strictEqual(toApply[0].apply_error, JSON.stringify({ rules: { errors: diagnostics.rules.errors } })) + assert.strictEqual(toModify[0].apply_state, ERROR) + assert.strictEqual(toModify[0].apply_error, JSON.stringify({ rules: { errors: diagnostics.rules.errors } })) + }) - const rules2 = { - product: 'ASM_DD', - id: 'rules2', - file: { - version: '2.2', - metadata: { rules_version: '1.5.0' }, - rules: [{ - id: 'test-id', - name: 'test-name', - tags: { - type: 'security_scanner', - category: 'attack_attempt', - confidence: '1' - }, - conditions: [ - { - parameters: { - inputs: [ - { address: 'http.test' } - ], - data: 'blocked_ips' - }, - operator: 'ip_match' - } - ] - }] - } - } + it('should report successful waf update', () => { + WAFManager.prototype.update.returns({ success: true, diagnostics: {} }) - updateWafFromRC({ toUnapply: [], toApply: [rules1], toModify: [] }) + const rcConfigs = getRcConfigs() - updateWafFromRC({ toUnapply: [rules1], toApply: [rules2], toModify: [] }) + RuleManager.updateWafFromRC(rcConfigs) - expect(rules1.apply_state).to.equal(ACKNOWLEDGED) - expect(rules1.apply_error).to.equal(undefined) - expect(rules2.apply_state).to.equal(ACKNOWLEDGED) - expect(rules2.apply_error).to.equal(undefined) + sinon.assert.calledOnceWithExactly( + reportWafUpdate, + waf.wafManager.ddwafVersion, + waf.wafManager.rulesVersion, + true + ) + sinon.assert.calledTwice(reportSuccessfulWafUpdate) + sinon.assert.calledWith(reportSuccessfulWafUpdate, rcConfigs.toModify[0].product, rcConfigs.toModify[0].id, {}) + sinon.assert.calledWith(reportSuccessfulWafUpdate, rcConfigs.toApply[0].product, rcConfigs.toApply[0].id, {}) + }) + + it('should report waf config error', () => { + WAFManager.prototype.remove.throws(new Error('Error removing config')) + WAFManager.prototype.update.returns({ success: false, diagnostics: {} }) + + const rcConfigs = getRcConfigs() + + RuleManager.updateWafFromRC(rcConfigs) + sinon.assert.notCalled(reportWafUpdate) + sinon.assert.calledThrice(reportWafConfigError) + reportWafConfigError.getCalls().forEach((spyCall) => { + sinon.assert.calledWithExactly( + spyCall, + waf.wafManager.ddwafVersion, + waf.wafManager.rulesVersion + ) }) }) - describe('ASM', () => { - it('should apply both rules_override and exclusions', () => { - const asm = { - exclusions: [{ - ekey: 'eValue' - }], - rules_override: [{ - roKey: 'roValue' - }], - custom_rules: [{ - piKey: 'piValue' - }] - } + it('should report waf update', () => { + WAFManager.prototype.update.onFirstCall().returns({ success: false, diagnostics: {} }) + WAFManager.prototype.update.onSecondCall().returns({ success: true, diagnostics: {} }) - const toApply = [ - { - product: 'ASM', - id: '1', - file: asm - } - ] + const rcConfigs = getRcConfigs() - updateWafFromRC({ toUnapply: [], toApply, toModify: [] }) + RuleManager.updateWafFromRC(rcConfigs) - expect(waf.update).to.have.been.calledOnceWithExactly(asm) - }) + sinon.assert.calledOnceWithExactly( + reportWafUpdate, + waf.wafManager.ddwafVersion, + waf.wafManager.rulesVersion, + false + ) + }) + describe('ASM', () => { it('should apply blocking actions', () => { + WAFManager.prototype.update.returns({ success: true }) + const toApply = [ { product: 'ASM', @@ -536,32 +365,31 @@ describe('AppSec Rule Manager', () => { } ] - updateWafFromRC({ toUnapply: [], toApply, toModify: [] }) + RuleManager.updateWafFromRC({ toUnapply: [], toApply, toModify: [] }) - const expectedPayload = { - actions: [ - { - id: 'notblock', - parameters: { - location: '/notfound', - status_code: 404 - } - }, - { - id: 'block', - parameters: { - location: '/redirected', - status_code: 302 - } + const expectedActions = [ + { + id: 'notblock', + parameters: { + location: '/notfound', + status_code: 404 } - ] - } + }, + { + id: 'block', + parameters: { + location: '/redirected', + status_code: 302 + } + } + ] - expect(waf.update).to.have.been.calledOnceWithExactly(expectedPayload) - expect(blocking.setDefaultBlockingActionParameters).to.have.been.calledOnceWithExactly(expectedPayload.actions) + sinon.assert.calledOnceWithExactly(setDefaultBlockingActionParameters, expectedActions) }) it('should unapply blocking actions', () => { + WAFManager.prototype.update.returns({ success: true }) + const asm = { actions: [ { @@ -581,11 +409,10 @@ describe('AppSec Rule Manager', () => { file: asm } ] - updateWafFromRC({ toUnapply: [], toApply, toModify: [] }) - expect(waf.update).to.have.been.calledOnceWithExactly(asm) - expect(blocking.setDefaultBlockingActionParameters).to.have.been.calledOnceWithExactly(asm.actions) + RuleManager.updateWafFromRC({ toUnapply: [], toApply, toModify: [] }) + sinon.assert.calledOnceWithExactly(setDefaultBlockingActionParameters, asm.actions) sinon.resetHistory() const toUnapply = [ @@ -595,39 +422,9 @@ describe('AppSec Rule Manager', () => { } ] - updateWafFromRC({ toUnapply, toApply: [], toModify: [] }) - - expect(waf.update).to.have.been.calledOnceWithExactly({ actions: [] }) - expect(blocking.setDefaultBlockingActionParameters).to.have.been.calledOnceWithExactly([]) - }) - - it('should ignore other properties', () => { - const asm = { - exclusions: [{ - ekey: 'eValue' - }], - rules_override: [{ - roKey: 'roValue' - }], - not_supported: [{ - nsKey: 'nsValue' - }] - } - - const toApply = [ - { - product: 'ASM', - id: '1', - file: asm - } - ] - - updateWafFromRC({ toUnapply: [], toApply, toModify: [] }) + RuleManager.updateWafFromRC({ toUnapply, toApply: [], toModify: [] }) - expect(waf.update).to.have.been.calledOnceWithExactly({ - exclusions: asm.exclusions, - rules_override: asm.rules_override - }) + sinon.assert.calledOnceWithExactly(setDefaultBlockingActionParameters, []) }) }) }) From 9fa7aedb9b837e2cc9d42dcd1eb67edfe5f5d5a0 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Tue, 6 May 2025 12:10:05 +0200 Subject: [PATCH 09/37] More tests --- packages/dd-trace/src/appsec/reporter.js | 20 ++++- .../dd-trace/test/appsec/reporter.spec.js | 80 +++++++++++++++++-- 2 files changed, 89 insertions(+), 11 deletions(-) diff --git a/packages/dd-trace/src/appsec/reporter.js b/packages/dd-trace/src/appsec/reporter.js index ae68610afdd..275f51b412a 100644 --- a/packages/dd-trace/src/appsec/reporter.js +++ b/packages/dd-trace/src/appsec/reporter.js @@ -246,7 +246,11 @@ function reportSuccessfulWafUpdate (product, rcConfigId, diagnostics) { telemetryLogCh.publish({ message: diagnostics[configKey].error, level: 'ERROR', - tags: `log_type:rc::${product}::diagnostic, appsec_config_key:${configKey}, rc_config_id:${rcConfigId}` + tags: { + log_type: `rc::${product.toLowerCase()}::diagnostic`, + appsec_config_key: configKey, + rc_config_id: rcConfigId + } }) continue } @@ -256,7 +260,11 @@ function reportSuccessfulWafUpdate (product, rcConfigId, diagnostics) { telemetryLogCh.publish({ message: `"${errorMessage}": ${JSON.stringify(errorIds)}`, level: 'ERROR', - tags: `log_type:rc::${product}::diagnostic, appsec_config_key:${configKey}, rc_config_id:${rcConfigId}` + tags: { + log_type: `rc::${product.toLowerCase()}::diagnostic`, + appsec_config_key: configKey, + rc_config_id: rcConfigId + } }) } } @@ -265,8 +273,12 @@ function reportSuccessfulWafUpdate (product, rcConfigId, diagnostics) { for (const [warningMessage, warningIds] of Object.entries(diagnostics[configKey].warnings)) { telemetryLogCh.publish({ message: `"${warningMessage}": ${JSON.stringify(warningIds)}`, - level: 'ERROR', - tags: `log_type:rc::${product}::diagnostic, appsec_config_key:${configKey}, rc_config_id:${rcConfigId}` + level: 'WARN', + tags: { + log_type: `rc::${product.toLowerCase()}::diagnostic`, + appsec_config_key: configKey, + rc_config_id: rcConfigId + } }) } } diff --git a/packages/dd-trace/test/appsec/reporter.spec.js b/packages/dd-trace/test/appsec/reporter.spec.js index c5d4d0c6211..b5c5723d260 100644 --- a/packages/dd-trace/test/appsec/reporter.spec.js +++ b/packages/dd-trace/test/appsec/reporter.spec.js @@ -1,8 +1,10 @@ 'use strict' +const dc = require('dc-polyfill') const proxyquire = require('proxyquire') -const { storage } = require('../../../datadog-core') const zlib = require('zlib') + +const { storage } = require('../../../datadog-core') const { ASM } = require('../../src/standalone/product') const { USER_KEEP } = require('../../../../ext/priority') @@ -131,11 +133,6 @@ describe('reporter', () => { describe('reportWafInit', () => { const wafVersion = '0.0.1' const rulesVersion = '0.0.2' - const diagnosticsRules = { - loaded: ['1', '3', '4'], - failed: ['2'], - errors: { error: 'error parsing rule 2' } - } it('should add some entries to metricsQueue', () => { Reporter.reportWafInit(wafVersion, rulesVersion, true) @@ -158,7 +155,6 @@ describe('reporter', () => { it('should not fail with undefined arguments', () => { const wafVersion = undefined const rulesVersion = undefined - const diagnosticsRules = undefined Reporter.reportWafInit(wafVersion, rulesVersion, true) @@ -289,6 +285,76 @@ describe('reporter', () => { }) }) + describe('reportSuccessfulWafUpdate', () => { + it('should send diagnostics using telemetry logs', () => { + const telemetryLogHandlerAssert = sinon.stub() + + const telemetryLogCh = dc.channel('datadog:telemetry:log') + telemetryLogCh.subscribe(telemetryLogHandlerAssert) + + const product = 'ASM_DD' + const rcConfigId = '1' + const diagnostics = { + rules: { + loaded: [], + failed: ['blk-001-001'], + skipped: [], + errors: { + 'missing key operator': [ + 'blk-001-001' + ] + }, + warnings: { + 'invalid tag': [ + 'blk-001-001' + ] + } + }, + processors: { + loaded: ['http-endpoint-fingerprint'], + failed: [], + skipped: [], + warnings: { + 'no mappings defined': [ + 'http-endpoint-fingerprint' + ] + } + } + } + + Reporter.reportSuccessfulWafUpdate(product, rcConfigId, diagnostics) + + expect(telemetryLogHandlerAssert).to.have.been.calledThrice + expect(telemetryLogHandlerAssert.getCall(0)).to.have.been.calledWithExactly({ + message: '"missing key operator": ["blk-001-001"]', + level: 'ERROR', + tags: { + log_type: 'rc::asm_dd::diagnostic', + appsec_config_key: 'rules', + rc_config_id: '1' + } + }, 'datadog:telemetry:log') + expect(telemetryLogHandlerAssert.getCall(1)).to.have.been.calledWithExactly({ + message: '"invalid tag": ["blk-001-001"]', + level: 'WARN', + tags: { + log_type: 'rc::asm_dd::diagnostic', + appsec_config_key: 'rules', + rc_config_id: '1' + } + }, 'datadog:telemetry:log') + expect(telemetryLogHandlerAssert.getCall(2)).to.have.been.calledWithExactly({ + message: '"no mappings defined": ["http-endpoint-fingerprint"]', + level: 'WARN', + tags: { + log_type: 'rc::asm_dd::diagnostic', + appsec_config_key: 'processors', + rc_config_id: '1' + } + }, 'datadog:telemetry:log') + }) + }) + describe('reportAttack', () => { let req From aadcc2fd3c82622f5496fd5b16686ab8c9d91abd Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Tue, 6 May 2025 15:01:45 +0200 Subject: [PATCH 10/37] Improve assert --- packages/dd-trace/test/appsec/rule_manager.spec.js | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/packages/dd-trace/test/appsec/rule_manager.spec.js b/packages/dd-trace/test/appsec/rule_manager.spec.js index 22e9a4802b5..d4f524a7cc5 100644 --- a/packages/dd-trace/test/appsec/rule_manager.spec.js +++ b/packages/dd-trace/test/appsec/rule_manager.spec.js @@ -303,13 +303,11 @@ describe('AppSec Rule Manager', () => { RuleManager.updateWafFromRC(rcConfigs) sinon.assert.notCalled(reportWafUpdate) sinon.assert.calledThrice(reportWafConfigError) - reportWafConfigError.getCalls().forEach((spyCall) => { - sinon.assert.calledWithExactly( - spyCall, - waf.wafManager.ddwafVersion, - waf.wafManager.rulesVersion - ) - }) + sinon.assert.alwaysCalledWithExactly( + reportWafConfigError, + waf.wafManager.ddwafVersion, + waf.wafManager.rulesVersion + ) }) it('should report waf update', () => { From 64319b64fc1701d5005bc3dc9378636daef1c81f Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Tue, 6 May 2025 16:30:52 +0200 Subject: [PATCH 11/37] Tests --- .../test/appsec/waf/waf_manager.spec.js | 164 ++++++++++++++++++ 1 file changed, 164 insertions(+) diff --git a/packages/dd-trace/test/appsec/waf/waf_manager.spec.js b/packages/dd-trace/test/appsec/waf/waf_manager.spec.js index 9093f43d921..ee7615e3b59 100644 --- a/packages/dd-trace/test/appsec/waf/waf_manager.spec.js +++ b/packages/dd-trace/test/appsec/waf/waf_manager.spec.js @@ -12,6 +12,9 @@ describe('WAFManager', () => { DDWAF.prototype.knownAddresses = knownAddresses DDWAF.prototype.diagnostics = {} DDWAF.prototype.createContext = sinon.stub() + DDWAF.prototype.createOrUpdateConfig = sinon.stub() + DDWAF.prototype.removeConfig = sinon.stub() + DDWAF.prototype.configPaths = [] WAFContextWrapper = sinon.stub() WAFManager = proxyquire('../../../src/appsec/waf/waf_manager', { @@ -30,4 +33,165 @@ describe('WAFManager', () => { sinon.assert.calledOnceWithMatch(WAFContextWrapper, any, any, any, any, knownAddresses) }) }) + + describe('WAF configurations handling', () => { + let wafManager + + const DEFAULT_RULES = { + version: '2.2', + metadata: { + rules_version: '1.14.2' + }, + rules: [ + { + id: 'blk-001-001', + name: 'Block IP Addresses', + tags: { + type: 'block_ip', + category: 'security_response', + module: 'network-acl' + }, + conditions: [ + { + parameters: { + inputs: [ + { + address: 'http.client_ip' + } + ], + data: 'blocked_ips' + }, + operator: 'ip_match' + } + ], + transformers: [], + on_match: [ + 'block' + ] + } + ] + } + + const ASM_CONFIG = { + rules_override: [], + actions: [], + exclusions: [], + custom_rules: [] + } + + const ASM_DATA_CONFIG = { + rules_data: [ + { + data: [ + { + expiration: 1661848350, + value: '188.243.182.156' + }, + { + expiration: 1661848350, + value: '51.222.158.205' + } + ], + id: 'blocked_ips', + type: 'ip_with_expiration' + } + ] + } + + const ASM_DD_CONFIG = { + version: '2.2', + metadata: { + rules_version: '1.42.11' + }, + rules: [] + } + + beforeEach(() => { + wafManager = new WAFManager(DEFAULT_RULES, {}) + }) + + afterEach(() => { + sinon.restore() + }) + + describe('update config', () => { + it('should update WAF config - ASM / ASM_DATA', () => { + DDWAF.prototype.configPaths = ['datadog/00/ASM_DD/default/config'] + + wafManager.update( + 'ASM', + ASM_CONFIG, + 'datadog/00/ASM/test/update_config' + ) + + wafManager.update( + 'ASM_DATA', + ASM_DATA_CONFIG, + 'datadog/00/ASM_DATA/test/update_config' + ) + + sinon.assert.calledWithExactly( + DDWAF.prototype.createOrUpdateConfig.getCall(0), + ASM_CONFIG, + 'datadog/00/ASM/test/update_config' + ) + sinon.assert.calledWithExactly( + DDWAF.prototype.createOrUpdateConfig.getCall(1), + ASM_DATA_CONFIG, + 'datadog/00/ASM_DATA/test/update_config' + ) + }) + + it('should remove default rules on ASM_DD update', () => { + DDWAF.prototype.configPaths = ['datadog/00/ASM_DD/default/config'] + + wafManager.update('ASM_DD', ASM_DD_CONFIG, 'datadog/00/ASM_DD/test/update_config') + + sinon.assert.calledOnceWithExactly( + DDWAF.prototype.removeConfig, + 'datadog/00/ASM_DD/default/config' + ) + sinon.assert.calledOnceWithExactly( + DDWAF.prototype.createOrUpdateConfig, + ASM_DD_CONFIG, + 'datadog/00/ASM_DD/test/update_config' + ) + }) + + it('should apply default rules when no ASM config is present after config update fail', () => { + DDWAF.prototype.configPaths = [] + DDWAF.prototype.createOrUpdateConfig.returns(false) + + wafManager.update('ASM_DD', ASM_DD_CONFIG, 'datadog/00/ASM_DD/test/update_config') + + sinon.assert.calledWithExactly( + DDWAF.prototype.createOrUpdateConfig.getCall(1), + DEFAULT_RULES, + 'datadog/00/ASM_DD/default/config' + ) + }) + }) + + describe('remove config', () => { + it('should remove WAF config', () => { + DDWAF.prototype.configPaths = ['datadog/00/ASM_DD/default/config'] + + wafManager.remove('path/to/remove') + + sinon.assert.calledOnceWithExactly(DDWAF.prototype.removeConfig, 'path/to/remove') + }) + + it('should apply default rules when no ASM config is present after config removal', () => { + DDWAF.prototype.configPaths = [] + + wafManager.remove('path/to/remove') + + sinon.assert.calledOnceWithExactly( + DDWAF.prototype.createOrUpdateConfig, + DEFAULT_RULES, + 'datadog/00/ASM_DD/default/config' + ) + }) + }) + }) }) From c92f1949c006bd27560f87a8c99bcce7b93060e6 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Tue, 6 May 2025 16:38:59 +0200 Subject: [PATCH 12/37] Clean up --- packages/dd-trace/src/remote_config/index.js | 7 +++---- packages/dd-trace/src/remote_config/manager.js | 4 ---- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/packages/dd-trace/src/remote_config/index.js b/packages/dd-trace/src/remote_config/index.js index 55702586227..ab149bdf000 100644 --- a/packages/dd-trace/src/remote_config/index.js +++ b/packages/dd-trace/src/remote_config/index.js @@ -6,7 +6,6 @@ const RemoteConfigManager = require('./manager') const RemoteConfigCapabilities = require('./capabilities') const { setCollectionMode } = require('../appsec/user_tracking') const log = require('../log') -const RuleManager = require("../appsec/rule_manager"); let rc @@ -136,9 +135,9 @@ function disableWafUpdate () { rc.updateCapabilities(RemoteConfigCapabilities.ASM_RASP_SHI, false) rc.updateCapabilities(RemoteConfigCapabilities.ASM_RASP_CMDI, false) - //rc.removeProductHandler('ASM_DATA') - //rc.removeProductHandler('ASM_DD') - //rc.removeProductHandler('ASM') + rc.removeProductHandler('ASM_DATA') + rc.removeProductHandler('ASM_DD') + rc.removeProductHandler('ASM') rc.off(RemoteConfigManager.kPreUpdate, RuleManager.updateWafFromRC) } diff --git a/packages/dd-trace/src/remote_config/manager.js b/packages/dd-trace/src/remote_config/manager.js index dbff5a96c0e..a95f281fb86 100644 --- a/packages/dd-trace/src/remote_config/manager.js +++ b/packages/dd-trace/src/remote_config/manager.js @@ -270,7 +270,6 @@ class RemoteConfigManager extends EventEmitter { for (const item of list) { // TODO: we need a way to tell if unapply configs were handled by kPreUpdate or not, because they're always // emitted unlike the apply and modify configs - console.log('\n\n@@ Dispatching item - before', item.apply_state, item.apply_error) this._callHandlerFor(action, item) @@ -279,7 +278,6 @@ class RemoteConfigManager extends EventEmitter { } else { this.appliedConfigs.set(item.path, item) } - console.log('@@ Dispatching item - after', item.apply_state, item.apply_error) } } @@ -307,9 +305,7 @@ class RemoteConfigManager extends EventEmitter { // If the handler doesn't accept an `ack` callback, assume `apply_state` is `ACKNOWLEDGED`, // unless it returns a promise, in which case we wait for the promise to be resolved or rejected. // TODO: do we want to pass old and new config ? - console.log('@@ _callHandlerFor before handler', item.apply_state, item.apply_error) const result = handler(action, item.file, item.id) - console.log('@@ _callHandlerFor after handler', item.apply_state, item.apply_error) if (result instanceof Promise) { result.then( () => { item.apply_state = ACKNOWLEDGED }, From 3d5348069fec4781e635c4f538e95a250410a830 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Tue, 6 May 2025 16:56:40 +0200 Subject: [PATCH 13/37] Clean up --- .../dd-trace/test/appsec/waf/index.spec.js | 107 ------------------ 1 file changed, 107 deletions(-) diff --git a/packages/dd-trace/test/appsec/waf/index.spec.js b/packages/dd-trace/test/appsec/waf/index.spec.js index c7ff8682705..8f7b717ca6d 100644 --- a/packages/dd-trace/test/appsec/waf/index.spec.js +++ b/packages/dd-trace/test/appsec/waf/index.spec.js @@ -27,11 +27,6 @@ describe('WAF Manager', () => { DDWAF.version = sinon.stub().returns('1.2.3') DDWAF.prototype.dispose = sinon.stub() DDWAF.prototype.createContext = sinon.stub() - DDWAF.prototype.update = sinon.stub().callsFake(function (newRules) { - if (newRules?.metadata?.rules_version) { - this.diagnostics.ruleset_version = newRules?.metadata?.rules_version - } - }) DDWAF.prototype.diagnostics = { ruleset_version: '1.0.0', rules: { @@ -51,7 +46,6 @@ describe('WAF Manager', () => { sinon.stub(Reporter.metricsQueue, 'set') sinon.stub(Reporter, 'reportMetrics') sinon.stub(Reporter, 'reportAttack') - sinon.stub(Reporter, 'reportWafUpdate') sinon.stub(Reporter, 'reportDerivatives') sinon.spy(Reporter, 'reportWafInit') @@ -74,7 +68,6 @@ describe('WAF Manager', () => { expect(Reporter.reportWafInit).to.have.been.calledWithExactly( '1.2.3', '1.0.0', - { loaded: ['rule_1'], failed: [] }, true ) }) @@ -97,31 +90,6 @@ describe('WAF Manager', () => { waf.init(rules, config.appsec) expect(Reporter.metricsQueue.set).to.have.been.calledWithExactly('_dd.appsec.waf.version', '1.2.3') - expect(Reporter.metricsQueue.set).to.have.been.calledWithExactly('_dd.appsec.event_rules.loaded', 1) - expect(Reporter.metricsQueue.set).to.have.been.calledWithExactly('_dd.appsec.event_rules.error_count', 0) - expect(Reporter.metricsQueue.set).not.to.have.been.calledWith('_dd.appsec.event_rules.errors') - }) - - it('should set init metrics with errors', () => { - DDWAF.version.returns('2.3.4') - DDWAF.prototype.diagnostics = { - rules: { - loaded: ['rule_1'], - failed: ['rule_2', 'rule_3'], - errors: { - error_1: ['invalid_1'], - error_2: ['invalid_2', 'invalid_3'] - } - } - } - - waf.init(rules, config.appsec) - - expect(Reporter.metricsQueue.set).to.have.been.calledWithExactly('_dd.appsec.waf.version', '2.3.4') - expect(Reporter.metricsQueue.set).to.have.been.calledWithExactly('_dd.appsec.event_rules.loaded', 1) - expect(Reporter.metricsQueue.set).to.have.been.calledWithExactly('_dd.appsec.event_rules.error_count', 2) - expect(Reporter.metricsQueue.set).to.have.been.calledWithExactly('_dd.appsec.event_rules.errors', - '{"error_1":["invalid_1"],"error_2":["invalid_2","invalid_3"]}') }) }) @@ -170,81 +138,6 @@ describe('WAF Manager', () => { }) }) - describe('wafManager.update', () => { - const wafVersion = '2.3.4' - - beforeEach(() => { - DDWAF.version.returns(wafVersion) - - waf.init(rules, config.appsec) - }) - - it('should call ddwaf.update', () => { - const rules = { - rules_data: [ - { - id: 'blocked_users', - type: 'data_with_expiration', - data: [ - { - expiration: 9999999999, - value: 'user1' - } - ] - } - ] - } - - waf.update(rules) - - expect(DDWAF.prototype.update).to.be.calledOnceWithExactly(rules) - expect(Reporter.reportWafUpdate).to.be.calledOnceWithExactly(wafVersion, '1.0.0', true) - }) - - it('should call Reporter.reportWafUpdate on successful update', () => { - const rules = { - metadata: { - rules_version: '4.2.0' - }, - rules_data: [ - { - id: 'blocked_users', - type: 'data_with_expiration', - data: [ - { - expiration: 9999999999, - value: 'user1' - } - ] - } - ] - } - - waf.update(rules) - - expect(Reporter.reportWafUpdate).to.be.calledOnceWithExactly(wafVersion, '4.2.0', true) - }) - - it('should call Reporter.reportWafUpdate on failed update', () => { - const rules = { - metadata: { - rules_version: '4.2.0' - } - } - const error = new Error('Failed to update rules') - - DDWAF.prototype.update = sinon.stub().throws(error) - - try { - waf.update(rules) - expect.fail('waf.update should have thrown an error') - } catch (err) { - expect(err).to.equal(error) - expect(Reporter.reportWafUpdate).to.be.calledOnceWithExactly(wafVersion, 'unknown', false) - } - }) - }) - describe('WAFContextWrapper', () => { let ddwafContext, wafContextWrapper, req From fb4ec09302123fda15026a1fe94ef62528acab52 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Wed, 28 May 2025 08:03:58 +0200 Subject: [PATCH 14/37] Native appsec package bump --- package.json | 7 ++----- yarn.lock | 8 ++++---- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/package.json b/package.json index 4a9f6d47b02..5d369583f4c 100644 --- a/package.json +++ b/package.json @@ -85,7 +85,7 @@ }, "dependencies": { "@datadog/libdatadog": "0.7.0", - "@datadog/native-appsec": "file:datadog-native-appsec-8.5.2.tgz", + "@datadog/native-appsec": "9.0.0", "@datadog/native-iast-taint-tracking": "4.0.0", "@datadog/native-metrics": "3.1.1", "@datadog/pprof": "5.9.0", @@ -156,8 +156,5 @@ "tiktoken": "^1.0.21", "yaml": "^2.8.0", "yarn-deduplicate": "^6.0.2" - }, - "bundledDependencies": [ - "@datadog/native-appsec" - ] + } } diff --git a/yarn.lock b/yarn.lock index 9ca3807fd63..75c21bef36e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -211,10 +211,10 @@ resolved "https://registry.yarnpkg.com/@datadog/libdatadog/-/libdatadog-0.7.0.tgz#81e07d3040c628892db697ccd01ae3c4d2a76315" integrity sha512-VVZLspzQcfEU47gmGCVoRkngn7RgFRR4CHjw4YaX8eWT+xz4Q4l6PvA45b7CMk9nlt3MNN5MtGdYttYMIpo6Sg== -"@datadog/native-appsec@8.5.2": - version "8.5.2" - resolved "https://registry.yarnpkg.com/@datadog/native-appsec/-/native-appsec-8.5.2.tgz#93a2c15c71c2a90e19e12506fbbdec9ccbc91541" - integrity sha512-lETBaVhBk+9o0pc+LDnXvp2ImDyT8K2deuqLf8A6q4/QjzCCXyR/yZO9R5+Kdoc93jZMRTWV9Pr4pBwHEdJSVA== +"@datadog/native-appsec@9.0.0": + version "9.0.0" + resolved "https://registry.yarnpkg.com/@datadog/native-appsec/-/native-appsec-9.0.0.tgz#3ac854d8597ca75af0aa534aae28fa7f13057d2a" + integrity sha512-C7v16pP4p4Y+Cx0jcxTYmhZTptfVs8TYbn6LH/aQgTkwx2tWsWN5lss7fjBYjWyZoPMwVh2UX/yDm4ES25hJnQ== dependencies: node-gyp-build "^3.9.0" From e1b155f9dbde83e155824857253f8d340487ceab Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Fri, 30 May 2025 02:55:48 +0200 Subject: [PATCH 15/37] Revert "Remove deprecated span tags" This reverts commit f8d8805062447edb1bb13b633ed41fbab477021d. --- packages/dd-trace/src/appsec/reporter.js | 8 +++++++- packages/dd-trace/src/appsec/waf/waf_manager.js | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/src/appsec/reporter.js b/packages/dd-trace/src/appsec/reporter.js index 275f51b412a..8d5f6e8db33 100644 --- a/packages/dd-trace/src/appsec/reporter.js +++ b/packages/dd-trace/src/appsec/reporter.js @@ -230,9 +230,15 @@ function getCollectedHeaders (req, res, shouldCollectEventHeaders, storedRespons return headersTags } -function reportWafInit (wafVersion, rulesVersion, success = false) { +function reportWafInit (wafVersion, rulesVersion, diagnosticsRules = {}, success = false) { if (success) { metricsQueue.set('_dd.appsec.waf.version', wafVersion) + + metricsQueue.set('_dd.appsec.event_rules.loaded', diagnosticsRules.loaded?.length || 0) + metricsQueue.set('_dd.appsec.event_rules.error_count', diagnosticsRules.failed?.length || 0) + if (diagnosticsRules.failed?.length) { + metricsQueue.set('_dd.appsec.event_rules.errors', JSON.stringify(diagnosticsRules.errors)) + } } incrementWafInitMetric(wafVersion, rulesVersion, success) diff --git a/packages/dd-trace/src/appsec/waf/waf_manager.js b/packages/dd-trace/src/appsec/waf/waf_manager.js index 6a0ff1bec4e..a70f10374a9 100644 --- a/packages/dd-trace/src/appsec/waf/waf_manager.js +++ b/packages/dd-trace/src/appsec/waf/waf_manager.js @@ -16,7 +16,7 @@ class WAFManager { this.rulesVersion = this.ddwaf.diagnostics.ruleset_version this.defaultRules = rules - Reporter.reportWafInit(this.ddwafVersion, this.rulesVersion, true) + Reporter.reportWafInit(this.ddwafVersion, this.rulesVersion, this.ddwaf.diagnostics.rules, true) } _loadDDWAF (rules) { From 208a5a2952fcc2955e174466885cee85b63b6775 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Fri, 30 May 2025 07:15:32 +0200 Subject: [PATCH 16/37] Revert tag deprecation --- .../dd-trace/test/appsec/reporter.spec.js | 21 ++++++++++++--- .../dd-trace/test/appsec/waf/index.spec.js | 26 +++++++++++++++++++ 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/packages/dd-trace/test/appsec/reporter.spec.js b/packages/dd-trace/test/appsec/reporter.spec.js index b5c5723d260..b8ee741b9ea 100644 --- a/packages/dd-trace/test/appsec/reporter.spec.js +++ b/packages/dd-trace/test/appsec/reporter.spec.js @@ -133,21 +133,33 @@ describe('reporter', () => { describe('reportWafInit', () => { const wafVersion = '0.0.1' const rulesVersion = '0.0.2' + const diagnosticsRules = { + loaded: ['1', '3', '4'], + failed: ['2'], + errors: { error: 'error parsing rule 2' } + } it('should add some entries to metricsQueue', () => { - Reporter.reportWafInit(wafVersion, rulesVersion, true) + Reporter.reportWafInit(wafVersion, rulesVersion, diagnosticsRules, true) expect(Reporter.metricsQueue.get('_dd.appsec.waf.version')).to.be.eq(wafVersion) + expect(Reporter.metricsQueue.get('_dd.appsec.event_rules.loaded')).to.be.eq(3) + expect(Reporter.metricsQueue.get('_dd.appsec.event_rules.error_count')).to.be.eq(1) + expect(Reporter.metricsQueue.get('_dd.appsec.event_rules.errors')) + .to.be.eq(JSON.stringify(diagnosticsRules.errors)) }) it('should not add entries to metricsQueue with success false', () => { Reporter.reportWafInit(wafVersion, rulesVersion, false) expect(Reporter.metricsQueue.get('_dd.appsec.waf.version')).to.be.undefined + expect(Reporter.metricsQueue.get('_dd.appsec.event_rules.loaded')).to.be.undefined + expect(Reporter.metricsQueue.get('_dd.appsec.event_rules.error_count')).to.be.undefined + expect(Reporter.metricsQueue.get('_dd.appsec.event_rules.errors')).to.be.undefined }) it('should call incrementWafInitMetric', () => { - Reporter.reportWafInit(wafVersion, rulesVersion, true) + Reporter.reportWafInit(wafVersion, rulesVersion, diagnosticsRules, true) expect(telemetry.incrementWafInitMetric).to.have.been.calledOnceWithExactly(wafVersion, rulesVersion, true) }) @@ -155,10 +167,13 @@ describe('reporter', () => { it('should not fail with undefined arguments', () => { const wafVersion = undefined const rulesVersion = undefined + const diagnosticsRules = undefined - Reporter.reportWafInit(wafVersion, rulesVersion, true) + Reporter.reportWafInit(wafVersion, rulesVersion, diagnosticsRules, true) expect(telemetry.incrementWafInitMetric).to.have.been.calledOnceWithExactly(wafVersion, rulesVersion, true) + expect(Reporter.metricsQueue.get('_dd.appsec.event_rules.loaded')).to.be.eq(0) + expect(Reporter.metricsQueue.get('_dd.appsec.event_rules.error_count')).to.be.eq(0) }) }) diff --git a/packages/dd-trace/test/appsec/waf/index.spec.js b/packages/dd-trace/test/appsec/waf/index.spec.js index 8f7b717ca6d..00885fcc322 100644 --- a/packages/dd-trace/test/appsec/waf/index.spec.js +++ b/packages/dd-trace/test/appsec/waf/index.spec.js @@ -68,6 +68,7 @@ describe('WAF Manager', () => { expect(Reporter.reportWafInit).to.have.been.calledWithExactly( '1.2.3', '1.0.0', + { loaded: ['rule_1'], failed: [] }, true ) }) @@ -90,6 +91,31 @@ describe('WAF Manager', () => { waf.init(rules, config.appsec) expect(Reporter.metricsQueue.set).to.have.been.calledWithExactly('_dd.appsec.waf.version', '1.2.3') + expect(Reporter.metricsQueue.set).to.have.been.calledWithExactly('_dd.appsec.event_rules.loaded', 1) + expect(Reporter.metricsQueue.set).to.have.been.calledWithExactly('_dd.appsec.event_rules.error_count', 0) + expect(Reporter.metricsQueue.set).not.to.have.been.calledWith('_dd.appsec.event_rules.errors') + }) + + it('should set init metrics with errors', () => { + DDWAF.version.returns('2.3.4') + DDWAF.prototype.diagnostics = { + rules: { + loaded: ['rule_1'], + failed: ['rule_2', 'rule_3'], + errors: { + error_1: ['invalid_1'], + error_2: ['invalid_2', 'invalid_3'] + } + } + } + + waf.init(rules, config.appsec) + + expect(Reporter.metricsQueue.set).to.have.been.calledWithExactly('_dd.appsec.waf.version', '2.3.4') + expect(Reporter.metricsQueue.set).to.have.been.calledWithExactly('_dd.appsec.event_rules.loaded', 1) + expect(Reporter.metricsQueue.set).to.have.been.calledWithExactly('_dd.appsec.event_rules.error_count', 2) + expect(Reporter.metricsQueue.set).to.have.been.calledWithExactly('_dd.appsec.event_rules.errors', + '{"error_1":["invalid_1"],"error_2":["invalid_2","invalid_3"]}') }) }) From 414398b826f5d06417e628a4ecf66079cf4f08b9 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Fri, 30 May 2025 07:47:45 +0200 Subject: [PATCH 17/37] Clean up --- packages/dd-trace/src/appsec/waf/index.js | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/packages/dd-trace/src/appsec/waf/index.js b/packages/dd-trace/src/appsec/waf/index.js index bf92a903e8c..630d6aaedca 100644 --- a/packages/dd-trace/src/appsec/waf/index.js +++ b/packages/dd-trace/src/appsec/waf/index.js @@ -33,20 +33,6 @@ function destroy () { waf.disposeContext = noop } -/* -function update (newRules) { - // TODO: check race conditions between Appsec enable/disable and WAF updates, the whole RC state management in general - if (!waf.wafManager) throw new Error('Cannot update disabled WAF') - - try { - waf.wafManager.update(newRules) - } catch (err) { - log.error('[ASM] Could not apply rules from remote config') - throw err - } -} -*/ - function run (data, req, raspRule) { if (!req) { const store = storage('legacy').getStore() From 3baee907ed8c64586ff17a90d702874e3317d76b Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Fri, 30 May 2025 07:59:43 +0200 Subject: [PATCH 18/37] Lint --- packages/dd-trace/src/appsec/rule_manager.js | 6 ++---- packages/dd-trace/test/appsec/telemetry/waf.spec.js | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/dd-trace/src/appsec/rule_manager.js b/packages/dd-trace/src/appsec/rule_manager.js index 5abbe152606..7cb784c5231 100644 --- a/packages/dd-trace/src/appsec/rule_manager.js +++ b/packages/dd-trace/src/appsec/rule_manager.js @@ -61,10 +61,8 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { } // check asm actions - if (updateResult.success && item.product === 'ASM') { - if (item.file?.actions?.length) { - newActions.set(item.id, item.file.actions) - } + if (updateResult.success && item.product === 'ASM' && item.file?.actions?.length) { + newActions.set(item.id, item.file.actions) } } catch (e) { wafUpdatedSuccess = false diff --git a/packages/dd-trace/test/appsec/telemetry/waf.spec.js b/packages/dd-trace/test/appsec/telemetry/waf.spec.js index ba5eedab456..3cb6c99cd50 100644 --- a/packages/dd-trace/test/appsec/telemetry/waf.spec.js +++ b/packages/dd-trace/test/appsec/telemetry/waf.spec.js @@ -277,7 +277,7 @@ describe('Appsec Waf Telemetry metrics', () => { expect(count).to.have.been.calledOnceWithExactly('waf.config_errors', { waf_version: wafVersion, - event_rules_version: rulesVersion, + event_rules_version: rulesVersion }) expect(inc).to.have.been.calledOnce }) From ab973810237947cf57ac06cf3989b3b36d0c733d Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Fri, 30 May 2025 09:49:47 +0200 Subject: [PATCH 19/37] Fix assertion --- integration-tests/appsec/graphql.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration-tests/appsec/graphql.spec.js b/integration-tests/appsec/graphql.spec.js index b018b511d61..9ce71b82221 100644 --- a/integration-tests/appsec/graphql.spec.js +++ b/integration-tests/appsec/graphql.spec.js @@ -109,7 +109,7 @@ describe('graphql', () => { assert.property(payload[1][0].metrics, '_dd.appsec.waf.duration') assert.propertyVal(payload[1][0].meta, 'appsec.event', 'true') assert.property(payload[1][0].meta, '_dd.appsec.json') - assert.propertyVal(payload[1][0].meta, '_dd.appsec.json', JSON.stringify(result)) + assert.deepStrictEqual(JSON.parse(payload[1][0].meta['_dd.appsec.json']), result) }) await axios({ From 50de4d4989f08ef1c1c0e0590261a0d2e039a4c6 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Sun, 1 Jun 2025 10:37:52 +0200 Subject: [PATCH 20/37] Improve RC product check --- packages/dd-trace/src/appsec/rule_manager.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/src/appsec/rule_manager.js b/packages/dd-trace/src/appsec/rule_manager.js index 7cb784c5231..1c456d962dc 100644 --- a/packages/dd-trace/src/appsec/rule_manager.js +++ b/packages/dd-trace/src/appsec/rule_manager.js @@ -7,6 +7,8 @@ const Reporter = require('./reporter') const blocking = require('./blocking') +const ASM_PRODUCTS = new Set(['ASM', 'ASM_DD', 'ASM_DATA']) + let appliedActions = new Map() function loadRules (config) { @@ -26,7 +28,7 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { let wafUpdatedSuccess = true for (const item of toUnapply) { - if (!['ASM_DD', 'ASM_DATA', 'ASM'].includes(item.product)) continue + if (!ASM_PRODUCTS.has(item.product)) continue try { waf.wafManager.remove(item.path) @@ -45,7 +47,7 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { } for (const item of [...toApply, ...toModify]) { - if (!['ASM_DD', 'ASM_DATA', 'ASM'].includes(item.product)) continue + if (!ASM_PRODUCTS.has(item.product)) continue try { const updateResult = waf.wafManager.update(item.product, item.file, item.path) From 3688d7c052a4915894f4a062542c2bb83088d5d7 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Sun, 1 Jun 2025 10:49:27 +0200 Subject: [PATCH 21/37] Avoid checking obj keys in rule manager extract errors --- packages/dd-trace/src/appsec/rule_manager.js | 7 ++++-- .../dd-trace/test/appsec/rule_manager.spec.js | 25 +++++++++++++++++-- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/packages/dd-trace/src/appsec/rule_manager.js b/packages/dd-trace/src/appsec/rule_manager.js index 1c456d962dc..cd5ef881d5c 100644 --- a/packages/dd-trace/src/appsec/rule_manager.js +++ b/packages/dd-trace/src/appsec/rule_manager.js @@ -117,19 +117,22 @@ function extractErrors (obj) { if (typeof obj !== 'object' || obj === null) return null const result = {} + let isResultPopulated = false for (const [key, value] of Object.entries(obj)) { if (key === 'error' || key === 'errors') { result[key] = value + isResultPopulated = true } else if (typeof value === 'object' && value !== null) { const child = extractErrors(value) - if (child && Object.keys(child).length > 0) { + if (child) { + isResultPopulated = true result[key] = child } } } - return Object.keys(result).length > 0 ? result : null + return isResultPopulated ? result : null } function clearAllRules () { diff --git a/packages/dd-trace/test/appsec/rule_manager.spec.js b/packages/dd-trace/test/appsec/rule_manager.spec.js index d4f524a7cc5..4d6c656a467 100644 --- a/packages/dd-trace/test/appsec/rule_manager.spec.js +++ b/packages/dd-trace/test/appsec/rule_manager.spec.js @@ -257,13 +257,34 @@ describe('AppSec Rule Manager', () => { loaded: ['http-endpoint-fingerprint'], failed: [], skipped: [], + errors: { + 'missing mapping definition': [ + 'http-endpoint-fingerprint' + ] + }, warnings: { 'no mappings defined': [ 'http-endpoint-fingerprint' ] } + }, + scanners: { + error: 'Fatal error' } } + + const expectedApplyError = { + rules: { + errors: diagnostics.rules.errors + }, + processors: { + errors: diagnostics.processors.errors + }, + scanners: { + error: diagnostics.scanners.error + } + } + WAFManager.prototype.update.returns({ success: false, diagnostics }) const { toModify, toApply } = getRcConfigs() @@ -271,9 +292,9 @@ describe('AppSec Rule Manager', () => { RuleManager.updateWafFromRC({ toUnapply: [], toApply, toModify }) assert.strictEqual(toApply[0].apply_state, ERROR) - assert.strictEqual(toApply[0].apply_error, JSON.stringify({ rules: { errors: diagnostics.rules.errors } })) + assert.strictEqual(toApply[0].apply_error, JSON.stringify(expectedApplyError)) assert.strictEqual(toModify[0].apply_state, ERROR) - assert.strictEqual(toModify[0].apply_error, JSON.stringify({ rules: { errors: diagnostics.rules.errors } })) + assert.strictEqual(toModify[0].apply_error, JSON.stringify(expectedApplyError)) }) it('should report successful waf update', () => { From f86b5e7fd33607f0686f40696e5187d2ac84e6b1 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Sun, 1 Jun 2025 11:03:47 +0200 Subject: [PATCH 22/37] Abstract log message publish in reporter --- packages/dd-trace/src/appsec/reporter.js | 69 ++++++++++++------------ 1 file changed, 35 insertions(+), 34 deletions(-) diff --git a/packages/dd-trace/src/appsec/reporter.js b/packages/dd-trace/src/appsec/reporter.js index 8d5f6e8db33..4132e6bd6de 100644 --- a/packages/dd-trace/src/appsec/reporter.js +++ b/packages/dd-trace/src/appsec/reporter.js @@ -244,48 +244,49 @@ function reportWafInit (wafVersion, rulesVersion, diagnosticsRules = {}, success incrementWafInitMetric(wafVersion, rulesVersion, success) } +function logWafDiagnosticMessage (product, rcConfigId, configKey, message, level) { + telemetryLogCh.publish({ + message, + level, + tags: { + log_type: `rc::${product.toLowerCase()}::diagnostic`, + appsec_config_key: configKey, + rc_config_id: rcConfigId + } + }) +} + function reportSuccessfulWafUpdate (product, rcConfigId, diagnostics) { for (const configKey of WAF_DIAGNOSTICS_CONFIG_KEYS_TO_REPORT) { - if (!diagnostics[configKey]) continue - - if (diagnostics[configKey].error) { - telemetryLogCh.publish({ - message: diagnostics[configKey].error, - level: 'ERROR', - tags: { - log_type: `rc::${product.toLowerCase()}::diagnostic`, - appsec_config_key: configKey, - rc_config_id: rcConfigId - } - }) + const configDiagnostics = diagnostics[configKey] + if (!configDiagnostics) continue + + if (configDiagnostics.error) { + logWafDiagnosticMessage(product, rcConfigId, configKey, configDiagnostics.error, 'ERROR') continue } - if (diagnostics[configKey].errors) { - for (const [errorMessage, errorIds] of Object.entries(diagnostics[configKey].errors)) { - telemetryLogCh.publish({ - message: `"${errorMessage}": ${JSON.stringify(errorIds)}`, - level: 'ERROR', - tags: { - log_type: `rc::${product.toLowerCase()}::diagnostic`, - appsec_config_key: configKey, - rc_config_id: rcConfigId - } - }) + if (configDiagnostics.errors) { + for (const [errorMessage, errorIds] of Object.entries(configDiagnostics.errors)) { + logWafDiagnosticMessage( + product, + rcConfigId, + configKey, + `"${errorMessage}": ${JSON.stringify(errorIds)}`, + 'ERROR' + ) } } - if (diagnostics[configKey].warnings) { - for (const [warningMessage, warningIds] of Object.entries(diagnostics[configKey].warnings)) { - telemetryLogCh.publish({ - message: `"${warningMessage}": ${JSON.stringify(warningIds)}`, - level: 'WARN', - tags: { - log_type: `rc::${product.toLowerCase()}::diagnostic`, - appsec_config_key: configKey, - rc_config_id: rcConfigId - } - }) + if (configDiagnostics.warnings) { + for (const [warningMessage, warningIds] of Object.entries(configDiagnostics.warnings)) { + logWafDiagnosticMessage( + product, + rcConfigId, + configKey, + `"${warningMessage}": ${JSON.stringify(warningIds)}`, + 'WARN' + ) } } } From 9952262235e282657a296c097aee647c12cd1c6f Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Thu, 12 Jun 2025 15:01:58 +0200 Subject: [PATCH 23/37] Set ruleset version after applying default --- packages/dd-trace/src/appsec/waf/waf_manager.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/dd-trace/src/appsec/waf/waf_manager.js b/packages/dd-trace/src/appsec/waf/waf_manager.js index a70f10374a9..b59d4f43967 100644 --- a/packages/dd-trace/src/appsec/waf/waf_manager.js +++ b/packages/dd-trace/src/appsec/waf/waf_manager.js @@ -60,16 +60,17 @@ class WAFManager { } const success = this.ddwaf.createOrUpdateConfig(rules, path) + + if (product === 'ASM_DD' && !success && !this.ddwaf.configPaths.some(cp => cp.includes('ASM_DD'))) { + this.ddwaf.createOrUpdateConfig(this.defaultRules, DEFAULT_WAF_CONFIG_PATH) + } + const diagnostics = this.ddwaf.diagnostics if (diagnostics.ruleset_version) { this.rulesVersion = diagnostics.ruleset_version } - if (product === 'ASM_DD' && !success && !this.ddwaf.configPaths.some(cp => cp.includes('ASM_DD'))) { - this.ddwaf.createOrUpdateConfig(this.defaultRules, DEFAULT_WAF_CONFIG_PATH) - } - return { success, diagnostics } } From 971f42e2f9fb3abeaccc444b7b556d697cfa5052 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Thu, 12 Jun 2025 15:05:16 +0200 Subject: [PATCH 24/37] Add comment for ASM actions tracking --- packages/dd-trace/src/appsec/rule_manager.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/dd-trace/src/appsec/rule_manager.js b/packages/dd-trace/src/appsec/rule_manager.js index cd5ef881d5c..2a1284766d8 100644 --- a/packages/dd-trace/src/appsec/rule_manager.js +++ b/packages/dd-trace/src/appsec/rule_manager.js @@ -9,6 +9,10 @@ const blocking = require('./blocking') const ASM_PRODUCTS = new Set(['ASM', 'ASM_DD', 'ASM_DATA']) +/* + ASM Actions must be tracked in order to update the defaultBlockingActions in blocking. These actions are used + by blockRequest method exposed in the user blocking SDK (see packages/dd-trace/src/appsec/sdk/user_blocking.js) + */ let appliedActions = new Map() function loadRules (config) { @@ -62,7 +66,7 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { Reporter.reportWafConfigError(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion) } - // check asm actions + // check ASM actions if (updateResult.success && item.product === 'ASM' && item.file?.actions?.length) { newActions.set(item.id, item.file.actions) } From 133cf7bc358813b93f9ae6ea063d8fcc1bb163ff Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Thu, 12 Jun 2025 15:10:34 +0200 Subject: [PATCH 25/37] Put some sense in wap updated result flag --- packages/dd-trace/src/appsec/rule_manager.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/dd-trace/src/appsec/rule_manager.js b/packages/dd-trace/src/appsec/rule_manager.js index 2a1284766d8..66d244ca1b2 100644 --- a/packages/dd-trace/src/appsec/rule_manager.js +++ b/packages/dd-trace/src/appsec/rule_manager.js @@ -29,7 +29,7 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { const newActions = new SpyMap(appliedActions) let wafUpdated = false - let wafUpdatedSuccess = true + let wafUpdatedFailed = false for (const item of toUnapply) { if (!ASM_PRODUCTS.has(item.product)) continue @@ -43,7 +43,7 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { newActions.delete(item.id) } } catch (e) { - wafUpdatedSuccess = false + wafUpdatedFailed = true item.apply_state = ERROR item.apply_error = e.toString() Reporter.reportWafConfigError(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion) @@ -61,7 +61,7 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { wafUpdated = true Reporter.reportSuccessfulWafUpdate(item.product, item.id, updateResult.diagnostics) } else { - wafUpdatedSuccess = false + wafUpdatedFailed = true item.apply_error = JSON.stringify(extractErrors(updateResult.diagnostics)) Reporter.reportWafConfigError(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion) } @@ -71,7 +71,7 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { newActions.set(item.id, item.file.actions) } } catch (e) { - wafUpdatedSuccess = false + wafUpdatedFailed = true item.apply_state = ERROR item.apply_error = e.toString() Reporter.reportWafConfigError(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion) @@ -79,7 +79,7 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { } if (wafUpdated) { - Reporter.reportWafUpdate(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion, wafUpdatedSuccess) + Reporter.reportWafUpdate(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion, !wafUpdatedFailed) } // Manage blocking actions From bd75bfc25370b22d863c0a0c637573f8360ad09e Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Thu, 12 Jun 2025 15:43:06 +0200 Subject: [PATCH 26/37] Refactor extract errors from diagnostics --- packages/dd-trace/src/appsec/rule_manager.js | 33 +++++++++++++------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/packages/dd-trace/src/appsec/rule_manager.js b/packages/dd-trace/src/appsec/rule_manager.js index 66d244ca1b2..46ebea3e20e 100644 --- a/packages/dd-trace/src/appsec/rule_manager.js +++ b/packages/dd-trace/src/appsec/rule_manager.js @@ -8,6 +8,16 @@ const Reporter = require('./reporter') const blocking = require('./blocking') const ASM_PRODUCTS = new Set(['ASM', 'ASM_DD', 'ASM_DATA']) +const DIAGNOSTIC_KEYS = [ + 'exclusions', + 'rules', + 'processors', + 'rules_override', + 'rules_data', + 'custom_rules', + 'actions', + 'scanners' +] /* ASM Actions must be tracked in order to update the defaultBlockingActions in blocking. These actions are used @@ -117,22 +127,23 @@ function concatArrays (files) { return [...files.values()].flat() } -function extractErrors (obj) { - if (typeof obj !== 'object' || obj === null) return null +function extractErrors (diagnostics) { + if (!diagnostics) return + + if (diagnostics.error) return diagnostics const result = {} let isResultPopulated = false - for (const [key, value] of Object.entries(obj)) { - if (key === 'error' || key === 'errors') { - result[key] = value + for (const diagnosticKey of DIAGNOSTIC_KEYS) { + if (diagnostics[diagnosticKey]?.error) { + (result[diagnosticKey] ??= {}).error = diagnostics[diagnosticKey]?.error + isResultPopulated = true + } + + if (diagnostics[diagnosticKey]?.errors) { + (result[diagnosticKey] ??= {}).errors = diagnostics[diagnosticKey]?.errors isResultPopulated = true - } else if (typeof value === 'object' && value !== null) { - const child = extractErrors(value) - if (child) { - isResultPopulated = true - result[key] = child - } } } From 1a6274ccd74e83b17a1d7bf9a6c06c5609de554c Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Tue, 17 Jun 2025 10:41:18 +0200 Subject: [PATCH 27/37] Move logic from rules manager to waf index --- packages/dd-trace/src/appsec/rule_manager.js | 37 ++-- packages/dd-trace/src/appsec/waf/index.js | 63 +++++- .../dd-trace/src/appsec/waf/waf_manager.js | 44 ++-- .../dd-trace/test/appsec/rule_manager.spec.js | 65 ++---- .../dd-trace/test/appsec/waf/index.spec.js | 202 ++++++++++++++++++ .../test/appsec/waf/waf_manager.spec.js | 164 -------------- 6 files changed, 322 insertions(+), 253 deletions(-) diff --git a/packages/dd-trace/src/appsec/rule_manager.js b/packages/dd-trace/src/appsec/rule_manager.js index 46ebea3e20e..045e029a251 100644 --- a/packages/dd-trace/src/appsec/rule_manager.js +++ b/packages/dd-trace/src/appsec/rule_manager.js @@ -45,18 +45,19 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { if (!ASM_PRODUCTS.has(item.product)) continue try { - waf.wafManager.remove(item.path) + waf.removeConfig(item.path) + item.apply_state = ACKNOWLEDGED wafUpdated = true + // ASM actions if (item.product === 'ASM') { newActions.delete(item.id) } } catch (e) { - wafUpdatedFailed = true item.apply_state = ERROR item.apply_error = e.toString() - Reporter.reportWafConfigError(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion) + wafUpdatedFailed = true } } @@ -64,27 +65,25 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { if (!ASM_PRODUCTS.has(item.product)) continue try { - const updateResult = waf.wafManager.update(item.product, item.file, item.path) - item.apply_state = updateResult.success ? ACKNOWLEDGED : ERROR - - if (updateResult.success) { - wafUpdated = true - Reporter.reportSuccessfulWafUpdate(item.product, item.id, updateResult.diagnostics) - } else { - wafUpdatedFailed = true - item.apply_error = JSON.stringify(extractErrors(updateResult.diagnostics)) - Reporter.reportWafConfigError(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion) - } + const resultDiagnostics = item.product === 'ASM_DD' + ? waf.updateAsmDdConfig(item.path, item.file) + : waf.updateConfig(item.path, item.file) - // check ASM actions - if (updateResult.success && item.product === 'ASM' && item.file?.actions?.length) { + item.apply_state = ACKNOWLEDGED + wafUpdated = true + + Reporter.reportSuccessfulWafUpdate(item.product, item.id, resultDiagnostics) + + // ASM actions + if (item.product === 'ASM' && item.file?.actions?.length) { newActions.set(item.id, item.file.actions) } } catch (e) { - wafUpdatedFailed = true item.apply_state = ERROR - item.apply_error = e.toString() - Reporter.reportWafConfigError(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion) + item.apply_error = e instanceof waf.WafUpdateError + ? JSON.stringify(extractErrors(e.diagnosticErrors)) + : e.toString() + wafUpdatedFailed = true } } diff --git a/packages/dd-trace/src/appsec/waf/index.js b/packages/dd-trace/src/appsec/waf/index.js index 630d6aaedca..7e70e5cd8d5 100644 --- a/packages/dd-trace/src/appsec/waf/index.js +++ b/packages/dd-trace/src/appsec/waf/index.js @@ -2,13 +2,26 @@ const { storage } = require('../../../../datadog-core') const log = require('../../log') +const Reporter = require('../reporter') + +class WafUpdateError extends Error { + constructor (diagnosticErrors) { + super('WafUpdateError') + this.name = 'WafUpdateError' + this.diagnosticErrors = diagnosticErrors + } +} const waf = { wafManager: null, init, destroy, + updateAsmDdConfig, + updateConfig, + removeConfig, run: noop, - disposeContext: noop + disposeContext: noop, + WafUpdateError } function init (rules, config) { @@ -33,6 +46,54 @@ function destroy () { waf.disposeContext = noop } +function updateAsmDdConfig (configPath, asmDdConfig) { + if (!waf.wafManager) throw new Error('Cannot update disabled WAF') + + try { + waf.wafManager.removeConfig(waf.wafManager.constructor.defaultWafConfigPath) + if (waf.wafManager.updateConfig(configPath, asmDdConfig)) { + return waf.wafManager.ddwaf.diagnostics + } else { + waf.wafManager.setAsmDdFallbackConfig() + throw new WafUpdateError(waf.wafManager.ddwaf.diagnostics) + } + } catch (err) { + Reporter.reportWafConfigError(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion) + log.error('[ASM] Could not update ASM DD config from RC') + throw err + } +} + +function updateConfig (configPath, config) { + if (!waf.wafManager) throw new Error('Cannot update disabled WAF') + + try { + if (waf.wafManager.updateConfig(configPath, config)) { + return waf.wafManager.ddwaf.diagnostics + } else { + waf.wafManager.setAsmDdFallbackConfig() + throw new WafUpdateError(waf.wafManager.ddwaf.diagnostics) + } + } catch (err) { + Reporter.reportWafConfigError(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion) + log.error('[ASM] Could not update config from RC') + throw err + } +} + +function removeConfig (configPath) { + if (!waf.wafManager) throw new Error('Cannot update disabled WAF') + + try { + waf.wafManager.removeConfig(configPath) + waf.wafManager.setAsmDdFallbackConfig() + } catch (err) { + Reporter.reportWafConfigError(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion) + log.error('[ASM] Could not remove config from RC') + throw err + } +} + function run (data, req, raspRule) { if (!req) { const store = storage('legacy').getStore() diff --git a/packages/dd-trace/src/appsec/waf/waf_manager.js b/packages/dd-trace/src/appsec/waf/waf_manager.js index b59d4f43967..f3e5989c266 100644 --- a/packages/dd-trace/src/appsec/waf/waf_manager.js +++ b/packages/dd-trace/src/appsec/waf/waf_manager.js @@ -6,9 +6,9 @@ const WAFContextWrapper = require('./waf_context_wrapper') const contexts = new WeakMap() -const DEFAULT_WAF_CONFIG_PATH = 'datadog/00/ASM_DD/default/config' - class WAFManager { + static get defaultWafConfigPath () { return 'datadog/00/ASM_DD/default/config' } + constructor (rules, config) { this.config = config this.wafTimeout = config.wafTimeout @@ -26,7 +26,7 @@ class WAFManager { this.ddwafVersion = DDWAF.version() const { obfuscatorKeyRegex, obfuscatorValueRegex } = this.config - return new DDWAF(rules, DEFAULT_WAF_CONFIG_PATH, { obfuscatorKeyRegex, obfuscatorValueRegex }) + return new DDWAF(rules, WAFManager.defaultWafConfigPath, { obfuscatorKeyRegex, obfuscatorValueRegex }) } catch (err) { this.ddwafVersion = this.ddwafVersion || 'unknown' Reporter.reportWafInit(this.ddwafVersion, 'unknown') @@ -54,36 +54,28 @@ class WAFManager { return wafContext } - update (product, rules, path) { - if (product === 'ASM_DD' && this.ddwaf.configPaths.includes(DEFAULT_WAF_CONFIG_PATH)) { - this.ddwaf.removeConfig(DEFAULT_WAF_CONFIG_PATH) - } - - const success = this.ddwaf.createOrUpdateConfig(rules, path) - - if (product === 'ASM_DD' && !success && !this.ddwaf.configPaths.some(cp => cp.includes('ASM_DD'))) { - this.ddwaf.createOrUpdateConfig(this.defaultRules, DEFAULT_WAF_CONFIG_PATH) + setRulesVersion () { + if (this.ddwaf.diagnostics.ruleset_version) { + this.rulesVersion = this.ddwaf.diagnostics.ruleset_version } + } - const diagnostics = this.ddwaf.diagnostics - - if (diagnostics.ruleset_version) { - this.rulesVersion = diagnostics.ruleset_version + setAsmDdFallbackConfig () { + if (!this.ddwaf.configPaths.some(cp => cp.includes('ASM_DD'))) { + this.ddwaf.createOrUpdateConfig(this.defaultRules, WAFManager.defaultWafConfigPath) + this.setRulesVersion() } + } - return { success, diagnostics } + updateConfig (path, rules) { + const updateResult = this.ddwaf.createOrUpdateConfig(rules, path) + this.setRulesVersion() + return updateResult } - remove (path) { + removeConfig (path) { this.ddwaf.removeConfig(path) - - if (!this.ddwaf.configPaths.some(cp => cp.includes('ASM_DD'))) { - this.ddwaf.createOrUpdateConfig(this.defaultRules, DEFAULT_WAF_CONFIG_PATH) - } - - if (this.ddwaf.diagnostics.ruleset_version) { - this.rulesVersion = this.ddwaf.diagnostics.ruleset_version - } + this.setRulesVersion() } destroy () { diff --git a/packages/dd-trace/test/appsec/rule_manager.spec.js b/packages/dd-trace/test/appsec/rule_manager.spec.js index 4d6c656a467..334ca32b869 100644 --- a/packages/dd-trace/test/appsec/rule_manager.spec.js +++ b/packages/dd-trace/test/appsec/rule_manager.spec.js @@ -9,7 +9,6 @@ const { ACKNOWLEDGED, UNACKNOWLEDGED, ERROR } = require('../../src/remote_config const rules = require('../../src/appsec/recommended.json') const waf = require('../../src/appsec/waf') -const WAFManager = require('../../src/appsec/waf/waf_manager') const blocking = require('../../src/appsec/blocking') describe('AppSec Rule Manager', () => { @@ -118,20 +117,18 @@ describe('AppSec Rule Manager', () => { } let RuleManager - let reportWafConfigError, reportSuccessfulWafUpdate, reportWafUpdate + let reportSuccessfulWafUpdate, reportWafUpdate let setDefaultBlockingActionParameters beforeEach(() => { reportWafUpdate = sinon.stub() reportSuccessfulWafUpdate = sinon.stub() - reportWafConfigError = sinon.stub() setDefaultBlockingActionParameters = sinon.stub() RuleManager = proxyquire.noCallThru()('../src/appsec/rule_manager', { './reporter': { reportWafUpdate, - reportSuccessfulWafUpdate, - reportWafConfigError + reportSuccessfulWafUpdate }, './blocking': { setDefaultBlockingActionParameters @@ -140,8 +137,8 @@ describe('AppSec Rule Manager', () => { waf.init.callThrough() - WAFManager.prototype.update = sinon.stub() - WAFManager.prototype.remove = sinon.stub() + sinon.stub(waf, 'updateConfig') + sinon.stub(waf, 'removeConfig') RuleManager.clearAllRules() config = new Config() @@ -178,8 +175,8 @@ describe('AppSec Rule Manager', () => { RuleManager.updateWafFromRC(rcConfigsForNonAsmProducts) }) - sinon.assert.notCalled(waf.wafManager.update) - sinon.assert.notCalled(waf.wafManager.remove) + sinon.assert.notCalled(waf.updateConfig) + sinon.assert.notCalled(waf.removeConfig) assert.strictEqual(rcConfigsForNonAsmProducts.toUnapply[0].apply_state, UNACKNOWLEDGED) assert.notProperty(rcConfigsForNonAsmProducts.toUnapply[0], 'apply_error') assert.strictEqual(rcConfigsForNonAsmProducts.toModify[0].apply_state, UNACKNOWLEDGED) @@ -193,24 +190,22 @@ describe('AppSec Rule Manager', () => { RuleManager.updateWafFromRC(rcConfigs) - sinon.assert.calledOnceWithExactly(waf.wafManager.remove, rcConfigs.toUnapply[0].path) - sinon.assert.calledTwice(waf.wafManager.update) + sinon.assert.calledOnceWithExactly(waf.removeConfig, rcConfigs.toUnapply[0].path) + sinon.assert.calledTwice(waf.updateConfig) sinon.assert.calledWith( - waf.wafManager.update.getCall(0), - rcConfigs.toApply[0].product, - rcConfigs.toApply[0].file, - rcConfigs.toApply[0].path + waf.updateConfig, + rcConfigs.toApply[0].path, + rcConfigs.toApply[0].file ) sinon.assert.calledWith( - waf.wafManager.update.getCall(1), - rcConfigs.toModify[0].product, - rcConfigs.toModify[0].file, - rcConfigs.toModify[0].path + waf.updateConfig, + rcConfigs.toModify[0].path, + rcConfigs.toModify[0].file ) }) it('should update apply_state and apply_error on successful apply', () => { - WAFManager.prototype.update.returns({ success: true }) + waf.updateConfig.returns({}) const rcConfigs = getRcConfigs() @@ -226,7 +221,7 @@ describe('AppSec Rule Manager', () => { it('should update apply_state and apply_error on failed config remove', () => { const removeConfigError = new Error('Error remove config') - WAFManager.prototype.remove.throws(removeConfigError) + waf.removeConfig.throws(removeConfigError) const { toUnapply } = getRcConfigs() @@ -285,7 +280,7 @@ describe('AppSec Rule Manager', () => { } } - WAFManager.prototype.update.returns({ success: false, diagnostics }) + waf.updateConfig.throws(new waf.WafUpdateError(diagnostics)) const { toModify, toApply } = getRcConfigs() @@ -298,7 +293,7 @@ describe('AppSec Rule Manager', () => { }) it('should report successful waf update', () => { - WAFManager.prototype.update.returns({ success: true, diagnostics: {} }) + waf.updateConfig.returns({}) const rcConfigs = getRcConfigs() @@ -315,25 +310,9 @@ describe('AppSec Rule Manager', () => { sinon.assert.calledWith(reportSuccessfulWafUpdate, rcConfigs.toApply[0].product, rcConfigs.toApply[0].id, {}) }) - it('should report waf config error', () => { - WAFManager.prototype.remove.throws(new Error('Error removing config')) - WAFManager.prototype.update.returns({ success: false, diagnostics: {} }) - - const rcConfigs = getRcConfigs() - - RuleManager.updateWafFromRC(rcConfigs) - sinon.assert.notCalled(reportWafUpdate) - sinon.assert.calledThrice(reportWafConfigError) - sinon.assert.alwaysCalledWithExactly( - reportWafConfigError, - waf.wafManager.ddwafVersion, - waf.wafManager.rulesVersion - ) - }) - it('should report waf update', () => { - WAFManager.prototype.update.onFirstCall().returns({ success: false, diagnostics: {} }) - WAFManager.prototype.update.onSecondCall().returns({ success: true, diagnostics: {} }) + waf.updateConfig.onFirstCall().throws(new waf.WafUpdateError({ error: 'Waf update error' })) + waf.updateConfig.onSecondCall().returns({}) const rcConfigs = getRcConfigs() @@ -349,7 +328,7 @@ describe('AppSec Rule Manager', () => { describe('ASM', () => { it('should apply blocking actions', () => { - WAFManager.prototype.update.returns({ success: true }) + waf.updateConfig.returns({}) const toApply = [ { @@ -407,7 +386,7 @@ describe('AppSec Rule Manager', () => { }) it('should unapply blocking actions', () => { - WAFManager.prototype.update.returns({ success: true }) + waf.updateConfig.returns({}) const asm = { actions: [ diff --git a/packages/dd-trace/test/appsec/waf/index.spec.js b/packages/dd-trace/test/appsec/waf/index.spec.js index 00885fcc322..cc336bdf32d 100644 --- a/packages/dd-trace/test/appsec/waf/index.spec.js +++ b/packages/dd-trace/test/appsec/waf/index.spec.js @@ -1,5 +1,6 @@ 'use strict' +const assert = require('node:assert') const proxyquire = require('proxyquire') const Config = require('../../../src/config') const rules = require('../../../src/appsec/recommended.json') @@ -33,6 +34,8 @@ describe('WAF Manager', () => { loaded: ['rule_1'], failed: [] } } + DDWAF.prototype.createOrUpdateConfig = sinon.stub().returns(true) + DDWAF.prototype.removeConfig = sinon.stub() DDWAF.prototype.knownAddresses = knownAddresses WAFManager = proxyquire('../../../src/appsec/waf/waf_manager', { @@ -48,6 +51,7 @@ describe('WAF Manager', () => { sinon.stub(Reporter, 'reportAttack') sinon.stub(Reporter, 'reportDerivatives') sinon.spy(Reporter, 'reportWafInit') + sinon.spy(Reporter, 'reportWafConfigError') webContext = {} sinon.stub(web, 'getContext').returns(webContext) @@ -145,6 +149,204 @@ describe('WAF Manager', () => { }) }) + describe('waf disabled check', () => { + it('should fail when updating configs on disabled waf', () => { + assert.throws( + () => { + waf.updateConfig('path', {}) + }, + { + name: 'Error', + message: 'Cannot update disabled WAF' + } + ) + }) + + it('should fail when updating ASM_DD configs on disabled waf', () => { + assert.throws( + () => { + waf.updateAsmDdConfig('path', {}) + }, + { + name: 'Error', + message: 'Cannot update disabled WAF' + } + ) + }) + + it('should fail when removing configs on disabled waf', () => { + assert.throws( + () => { + waf.updateAsmDdConfig('path', {}) + }, + { + name: 'Error', + message: 'Cannot update disabled WAF' + } + ) + }) + }) + + describe('configurations handling', () => { + const ASM_CONFIG = { + rules_override: [], + actions: [], + exclusions: [], + custom_rules: [] + } + + const ASM_DATA_CONFIG = { + rules_data: [ + { + data: [ + { + expiration: 1661848350, + value: '188.243.182.156' + }, + { + expiration: 1661848350, + value: '51.222.158.205' + } + ], + id: 'blocked_ips', + type: 'ip_with_expiration' + } + ] + } + + const ASM_DD_CONFIG = { + version: '2.2', + metadata: { + rules_version: '1.42.11' + }, + rules: [] + } + + beforeEach(() => { + waf.init(rules, config.appsec) + }) + + afterEach(() => { + sinon.restore() + }) + + describe('update config', () => { + it('should update WAF config - ASM / ASM_DATA', () => { + DDWAF.prototype.configPaths = ['datadog/00/ASM_DD/default/config'] + + waf.updateConfig('datadog/00/ASM/test/update_config', ASM_CONFIG) + waf.updateConfig('datadog/00/ASM_DATA/test/update_config', ASM_DATA_CONFIG) + + sinon.assert.calledWithExactly( + DDWAF.prototype.createOrUpdateConfig.getCall(0), + ASM_CONFIG, + 'datadog/00/ASM/test/update_config' + ) + sinon.assert.calledWithExactly( + DDWAF.prototype.createOrUpdateConfig.getCall(1), + ASM_DATA_CONFIG, + 'datadog/00/ASM_DATA/test/update_config' + ) + }) + + it('should remove default rules on ASM_DD update', () => { + DDWAF.prototype.configPaths = ['datadog/00/ASM_DD/default/config'] + + waf.updateAsmDdConfig('datadog/00/ASM_DD/test/update_config', ASM_DD_CONFIG) + + sinon.assert.calledOnceWithExactly( + DDWAF.prototype.removeConfig, + 'datadog/00/ASM_DD/default/config' + ) + sinon.assert.calledOnceWithExactly( + DDWAF.prototype.createOrUpdateConfig, + ASM_DD_CONFIG, + 'datadog/00/ASM_DD/test/update_config' + ) + }) + + it('should apply default rules when no ASM config is present after config update fail', () => { + DDWAF.prototype.configPaths = [] + DDWAF.prototype.createOrUpdateConfig.returns(false) + + assert.throws( + () => { + waf.updateAsmDdConfig('datadog/00/ASM_DD/test/update_config', ASM_DD_CONFIG) + }, + { + name: 'WafUpdateError' + } + ) + + sinon.assert.calledWithExactly( + DDWAF.prototype.createOrUpdateConfig.getCall(1), + rules, + 'datadog/00/ASM_DD/default/config' + ) + }) + }) + + describe('remove config', () => { + it('should remove WAF config', () => { + DDWAF.prototype.configPaths = ['datadog/00/ASM_DD/default/config'] + + waf.removeConfig('path/to/remove') + + sinon.assert.calledOnceWithExactly(DDWAF.prototype.removeConfig, 'path/to/remove') + }) + + it('should apply default rules when no ASM config is present after config removal', () => { + DDWAF.prototype.configPaths = [] + + waf.removeConfig('path/to/remove') + + sinon.assert.calledOnceWithExactly( + DDWAF.prototype.createOrUpdateConfig, + rules, + 'datadog/00/ASM_DD/default/config' + ) + }) + }) + + it('should report waf config error on config update fail', () => { + DDWAF.prototype.configPaths = [] + DDWAF.prototype.createOrUpdateConfig.returns(false) + assert.throws( + () => { + waf.updateConfig('path', {}) + }, + { + name: 'WafUpdateError' + } + ) + + sinon.assert.calledOnceWithExactly( + Reporter.reportWafConfigError, + '1.2.3', + '1.0.0' + ) + }) + + it('should report waf config error on config remove fail', () => { + DDWAF.prototype.configPaths = [] + DDWAF.prototype.removeConfig.throws(new Error('Error removing WAF config')) + assert.throws( + () => { + waf.removeConfig('path') + }, + { + name: 'Error' + } + ) + + sinon.assert.calledOnceWithExactly( + Reporter.reportWafConfigError, + '1.2.3', + '1.0.0' + ) + }) + }) + describe('wafManager.createDDWAFContext', () => { beforeEach(() => { DDWAF.version.returns('4.5.6') diff --git a/packages/dd-trace/test/appsec/waf/waf_manager.spec.js b/packages/dd-trace/test/appsec/waf/waf_manager.spec.js index ee7615e3b59..9093f43d921 100644 --- a/packages/dd-trace/test/appsec/waf/waf_manager.spec.js +++ b/packages/dd-trace/test/appsec/waf/waf_manager.spec.js @@ -12,9 +12,6 @@ describe('WAFManager', () => { DDWAF.prototype.knownAddresses = knownAddresses DDWAF.prototype.diagnostics = {} DDWAF.prototype.createContext = sinon.stub() - DDWAF.prototype.createOrUpdateConfig = sinon.stub() - DDWAF.prototype.removeConfig = sinon.stub() - DDWAF.prototype.configPaths = [] WAFContextWrapper = sinon.stub() WAFManager = proxyquire('../../../src/appsec/waf/waf_manager', { @@ -33,165 +30,4 @@ describe('WAFManager', () => { sinon.assert.calledOnceWithMatch(WAFContextWrapper, any, any, any, any, knownAddresses) }) }) - - describe('WAF configurations handling', () => { - let wafManager - - const DEFAULT_RULES = { - version: '2.2', - metadata: { - rules_version: '1.14.2' - }, - rules: [ - { - id: 'blk-001-001', - name: 'Block IP Addresses', - tags: { - type: 'block_ip', - category: 'security_response', - module: 'network-acl' - }, - conditions: [ - { - parameters: { - inputs: [ - { - address: 'http.client_ip' - } - ], - data: 'blocked_ips' - }, - operator: 'ip_match' - } - ], - transformers: [], - on_match: [ - 'block' - ] - } - ] - } - - const ASM_CONFIG = { - rules_override: [], - actions: [], - exclusions: [], - custom_rules: [] - } - - const ASM_DATA_CONFIG = { - rules_data: [ - { - data: [ - { - expiration: 1661848350, - value: '188.243.182.156' - }, - { - expiration: 1661848350, - value: '51.222.158.205' - } - ], - id: 'blocked_ips', - type: 'ip_with_expiration' - } - ] - } - - const ASM_DD_CONFIG = { - version: '2.2', - metadata: { - rules_version: '1.42.11' - }, - rules: [] - } - - beforeEach(() => { - wafManager = new WAFManager(DEFAULT_RULES, {}) - }) - - afterEach(() => { - sinon.restore() - }) - - describe('update config', () => { - it('should update WAF config - ASM / ASM_DATA', () => { - DDWAF.prototype.configPaths = ['datadog/00/ASM_DD/default/config'] - - wafManager.update( - 'ASM', - ASM_CONFIG, - 'datadog/00/ASM/test/update_config' - ) - - wafManager.update( - 'ASM_DATA', - ASM_DATA_CONFIG, - 'datadog/00/ASM_DATA/test/update_config' - ) - - sinon.assert.calledWithExactly( - DDWAF.prototype.createOrUpdateConfig.getCall(0), - ASM_CONFIG, - 'datadog/00/ASM/test/update_config' - ) - sinon.assert.calledWithExactly( - DDWAF.prototype.createOrUpdateConfig.getCall(1), - ASM_DATA_CONFIG, - 'datadog/00/ASM_DATA/test/update_config' - ) - }) - - it('should remove default rules on ASM_DD update', () => { - DDWAF.prototype.configPaths = ['datadog/00/ASM_DD/default/config'] - - wafManager.update('ASM_DD', ASM_DD_CONFIG, 'datadog/00/ASM_DD/test/update_config') - - sinon.assert.calledOnceWithExactly( - DDWAF.prototype.removeConfig, - 'datadog/00/ASM_DD/default/config' - ) - sinon.assert.calledOnceWithExactly( - DDWAF.prototype.createOrUpdateConfig, - ASM_DD_CONFIG, - 'datadog/00/ASM_DD/test/update_config' - ) - }) - - it('should apply default rules when no ASM config is present after config update fail', () => { - DDWAF.prototype.configPaths = [] - DDWAF.prototype.createOrUpdateConfig.returns(false) - - wafManager.update('ASM_DD', ASM_DD_CONFIG, 'datadog/00/ASM_DD/test/update_config') - - sinon.assert.calledWithExactly( - DDWAF.prototype.createOrUpdateConfig.getCall(1), - DEFAULT_RULES, - 'datadog/00/ASM_DD/default/config' - ) - }) - }) - - describe('remove config', () => { - it('should remove WAF config', () => { - DDWAF.prototype.configPaths = ['datadog/00/ASM_DD/default/config'] - - wafManager.remove('path/to/remove') - - sinon.assert.calledOnceWithExactly(DDWAF.prototype.removeConfig, 'path/to/remove') - }) - - it('should apply default rules when no ASM config is present after config removal', () => { - DDWAF.prototype.configPaths = [] - - wafManager.remove('path/to/remove') - - sinon.assert.calledOnceWithExactly( - DDWAF.prototype.createOrUpdateConfig, - DEFAULT_RULES, - 'datadog/00/ASM_DD/default/config' - ) - }) - }) - }) }) From b423f7853d26e3958b752b06c298dd0b0dad1973 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Wed, 18 Jun 2025 07:53:31 +0200 Subject: [PATCH 28/37] Refactor conditional --- packages/dd-trace/src/appsec/waf/index.js | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/packages/dd-trace/src/appsec/waf/index.js b/packages/dd-trace/src/appsec/waf/index.js index 7e70e5cd8d5..e9c0ee14365 100644 --- a/packages/dd-trace/src/appsec/waf/index.js +++ b/packages/dd-trace/src/appsec/waf/index.js @@ -51,12 +51,14 @@ function updateAsmDdConfig (configPath, asmDdConfig) { try { waf.wafManager.removeConfig(waf.wafManager.constructor.defaultWafConfigPath) - if (waf.wafManager.updateConfig(configPath, asmDdConfig)) { - return waf.wafManager.ddwaf.diagnostics - } else { + const updateSucceed = waf.wafManager.updateConfig(configPath, asmDdConfig) + + if (!updateSucceed) { waf.wafManager.setAsmDdFallbackConfig() throw new WafUpdateError(waf.wafManager.ddwaf.diagnostics) } + + return waf.wafManager.ddwaf.diagnostics } catch (err) { Reporter.reportWafConfigError(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion) log.error('[ASM] Could not update ASM DD config from RC') @@ -68,12 +70,14 @@ function updateConfig (configPath, config) { if (!waf.wafManager) throw new Error('Cannot update disabled WAF') try { - if (waf.wafManager.updateConfig(configPath, config)) { - return waf.wafManager.ddwaf.diagnostics - } else { + const updateSucceed = waf.wafManager.updateConfig(configPath, config) + + if (!updateSucceed) { waf.wafManager.setAsmDdFallbackConfig() throw new WafUpdateError(waf.wafManager.ddwaf.diagnostics) } + + return waf.wafManager.ddwaf.diagnostics } catch (err) { Reporter.reportWafConfigError(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion) log.error('[ASM] Could not update config from RC') From 4d5a38d6294622696cc44b5a9ec852e02fec6c75 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Wed, 18 Jun 2025 07:53:49 +0200 Subject: [PATCH 29/37] Fix test after enabled metric implementation --- packages/dd-trace/test/appsec/telemetry/waf.spec.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/dd-trace/test/appsec/telemetry/waf.spec.js b/packages/dd-trace/test/appsec/telemetry/waf.spec.js index 3cb6c99cd50..c985a75280b 100644 --- a/packages/dd-trace/test/appsec/telemetry/waf.spec.js +++ b/packages/dd-trace/test/appsec/telemetry/waf.spec.js @@ -290,12 +290,12 @@ describe('Appsec Waf Telemetry metrics', () => { appsecTelemetry.incrementWafConfigErrorsMetric(wafVersion, rulesVersion, true) const { metrics } = appsecNamespace.toJSON() - expect(metrics.series.length).to.be.eq(1) - expect(metrics.series[0].metric).to.be.eq('waf.config_errors') - expect(metrics.series[0].points.length).to.be.eq(1) - expect(metrics.series[0].points[0][1]).to.be.eq(3) - expect(metrics.series[0].tags).to.include('waf_version:0.0.1') - expect(metrics.series[0].tags).to.include('event_rules_version:0.0.2') + expect(metrics.series.length).to.be.eq(2) + expect(metrics.series[1].metric).to.be.eq('waf.config_errors') + expect(metrics.series[1].points.length).to.be.eq(1) + expect(metrics.series[1].points[0][1]).to.be.eq(3) + expect(metrics.series[1].tags).to.include('waf_version:0.0.1') + expect(metrics.series[1].tags).to.include('event_rules_version:0.0.2') }) }) From 461ad1a64ca845910931f99513715bacf46cd7c2 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Wed, 18 Jun 2025 22:30:14 +0200 Subject: [PATCH 30/37] Move reporting logic to waf index --- packages/dd-trace/src/appsec/reporter.js | 20 ++-- packages/dd-trace/src/appsec/rule_manager.js | 8 +- packages/dd-trace/src/appsec/waf/index.js | 33 +++---- .../dd-trace/src/appsec/waf/waf_manager.js | 3 +- .../dd-trace/test/appsec/reporter.spec.js | 97 +++++++++---------- .../dd-trace/test/appsec/rule_manager.spec.js | 91 ++++++++++++++--- .../dd-trace/test/appsec/waf/index.spec.js | 85 +++------------- 7 files changed, 168 insertions(+), 169 deletions(-) diff --git a/packages/dd-trace/src/appsec/reporter.js b/packages/dd-trace/src/appsec/reporter.js index 4132e6bd6de..31de16246d2 100644 --- a/packages/dd-trace/src/appsec/reporter.js +++ b/packages/dd-trace/src/appsec/reporter.js @@ -245,24 +245,28 @@ function reportWafInit (wafVersion, rulesVersion, diagnosticsRules = {}, success } function logWafDiagnosticMessage (product, rcConfigId, configKey, message, level) { + const tags = + `log_type:rc::${product.toLowerCase()}::diagnostic,appsec_config_key:${configKey},rc_config_id:${rcConfigId}` telemetryLogCh.publish({ message, level, - tags: { - log_type: `rc::${product.toLowerCase()}::diagnostic`, - appsec_config_key: configKey, - rc_config_id: rcConfigId - } + tags }) } -function reportSuccessfulWafUpdate (product, rcConfigId, diagnostics) { +function reportWafConfigUpdate (product, rcConfigId, diagnostics, wafVersion) { + if (diagnostics.error) { + logWafDiagnosticMessage(product, rcConfigId, '', diagnostics.error, 'ERROR') + incrementWafConfigErrorsMetric(wafVersion, diagnostics.ruleset_version || 'unknown') + } + for (const configKey of WAF_DIAGNOSTICS_CONFIG_KEYS_TO_REPORT) { const configDiagnostics = diagnostics[configKey] if (!configDiagnostics) continue if (configDiagnostics.error) { logWafDiagnosticMessage(product, rcConfigId, configKey, configDiagnostics.error, 'ERROR') + incrementWafConfigErrorsMetric(wafVersion, diagnostics.ruleset_version || 'unknown') continue } @@ -275,6 +279,7 @@ function reportSuccessfulWafUpdate (product, rcConfigId, diagnostics) { `"${errorMessage}": ${JSON.stringify(errorIds)}`, 'ERROR' ) + incrementWafConfigErrorsMetric(wafVersion, diagnostics.ruleset_version || 'unknown') } } @@ -550,11 +555,10 @@ module.exports = { filterExtendedHeaders, formatHeaderName, reportWafInit, - reportSuccessfulWafUpdate, + reportWafConfigUpdate, reportMetrics, reportAttack, reportWafUpdate: incrementWafUpdatesMetric, - reportWafConfigError: incrementWafConfigErrorsMetric, reportRaspRuleSkipped: updateRaspRuleSkippedMetricTags, reportDerivatives, finishRequest, diff --git a/packages/dd-trace/src/appsec/rule_manager.js b/packages/dd-trace/src/appsec/rule_manager.js index 045e029a251..41e7c211983 100644 --- a/packages/dd-trace/src/appsec/rule_manager.js +++ b/packages/dd-trace/src/appsec/rule_manager.js @@ -65,15 +65,11 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { if (!ASM_PRODUCTS.has(item.product)) continue try { - const resultDiagnostics = item.product === 'ASM_DD' - ? waf.updateAsmDdConfig(item.path, item.file) - : waf.updateConfig(item.path, item.file) + waf.updateConfig(item.product, item.id, item.path, item.file) item.apply_state = ACKNOWLEDGED wafUpdated = true - Reporter.reportSuccessfulWafUpdate(item.product, item.id, resultDiagnostics) - // ASM actions if (item.product === 'ASM' && item.file?.actions?.length) { newActions.set(item.id, item.file.actions) @@ -87,6 +83,8 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { } } + waf.checkAsmDdFallback() + if (wafUpdated) { Reporter.reportWafUpdate(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion, !wafUpdatedFailed) } diff --git a/packages/dd-trace/src/appsec/waf/index.js b/packages/dd-trace/src/appsec/waf/index.js index e9c0ee14365..45833373bf1 100644 --- a/packages/dd-trace/src/appsec/waf/index.js +++ b/packages/dd-trace/src/appsec/waf/index.js @@ -16,9 +16,9 @@ const waf = { wafManager: null, init, destroy, - updateAsmDdConfig, updateConfig, removeConfig, + checkAsmDdFallback, run: noop, disposeContext: noop, WafUpdateError @@ -46,40 +46,31 @@ function destroy () { waf.disposeContext = noop } -function updateAsmDdConfig (configPath, asmDdConfig) { +function checkAsmDdFallback () { if (!waf.wafManager) throw new Error('Cannot update disabled WAF') try { - waf.wafManager.removeConfig(waf.wafManager.constructor.defaultWafConfigPath) - const updateSucceed = waf.wafManager.updateConfig(configPath, asmDdConfig) - - if (!updateSucceed) { - waf.wafManager.setAsmDdFallbackConfig() - throw new WafUpdateError(waf.wafManager.ddwaf.diagnostics) - } - - return waf.wafManager.ddwaf.diagnostics - } catch (err) { - Reporter.reportWafConfigError(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion) - log.error('[ASM] Could not update ASM DD config from RC') - throw err + waf.wafManager.setAsmDdFallbackConfig() + } catch { + log.error('[ASM] Could not apply default ruleset back as fallback') } } -function updateConfig (configPath, config) { +function updateConfig (product, configId, configPath, config) { if (!waf.wafManager) throw new Error('Cannot update disabled WAF') try { + if (product === 'ASM_DD') { + waf.wafManager.removeConfig(waf.wafManager.constructor.defaultWafConfigPath) + } + const updateSucceed = waf.wafManager.updateConfig(configPath, config) + Reporter.reportWafConfigUpdate(product, configId, waf.wafManager.ddwaf.diagnostics, waf.wafManager.ddwafVersion) if (!updateSucceed) { - waf.wafManager.setAsmDdFallbackConfig() throw new WafUpdateError(waf.wafManager.ddwaf.diagnostics) } - - return waf.wafManager.ddwaf.diagnostics } catch (err) { - Reporter.reportWafConfigError(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion) log.error('[ASM] Could not update config from RC') throw err } @@ -90,9 +81,7 @@ function removeConfig (configPath) { try { waf.wafManager.removeConfig(configPath) - waf.wafManager.setAsmDdFallbackConfig() } catch (err) { - Reporter.reportWafConfigError(waf.wafManager.ddwafVersion, waf.wafManager.rulesVersion) log.error('[ASM] Could not remove config from RC') throw err } diff --git a/packages/dd-trace/src/appsec/waf/waf_manager.js b/packages/dd-trace/src/appsec/waf/waf_manager.js index f3e5989c266..76385c82b65 100644 --- a/packages/dd-trace/src/appsec/waf/waf_manager.js +++ b/packages/dd-trace/src/appsec/waf/waf_manager.js @@ -62,8 +62,7 @@ class WAFManager { setAsmDdFallbackConfig () { if (!this.ddwaf.configPaths.some(cp => cp.includes('ASM_DD'))) { - this.ddwaf.createOrUpdateConfig(this.defaultRules, WAFManager.defaultWafConfigPath) - this.setRulesVersion() + this.updateConfig(WAFManager.defaultWafConfigPath, this.defaultRules) } } diff --git a/packages/dd-trace/test/appsec/reporter.spec.js b/packages/dd-trace/test/appsec/reporter.spec.js index b8ee741b9ea..253d8f9179d 100644 --- a/packages/dd-trace/test/appsec/reporter.spec.js +++ b/packages/dd-trace/test/appsec/reporter.spec.js @@ -48,11 +48,12 @@ describe('reporter', () => { telemetry = { incrementWafInitMetric: sinon.stub(), + incrementWafConfigErrorsMetric: sinon.stub(), + incrementWafUpdatesMetric: sinon.stub(), + incrementWafRequestsMetric: sinon.stub(), updateWafRequestsMetricTags: sinon.stub(), updateRaspRequestsMetricTags: sinon.stub(), updateRaspRuleSkippedMetricTags: sinon.stub(), - incrementWafUpdatesMetric: sinon.stub(), - incrementWafRequestsMetric: sinon.stub(), updateRateLimitedMetric: sinon.stub(), getRequestMetrics: sinon.stub() } @@ -300,74 +301,70 @@ describe('reporter', () => { }) }) - describe('reportSuccessfulWafUpdate', () => { + describe('reportWafConfigUpdate', () => { + const product = 'ASM_DD' + const rcConfigId = '1' + const diagnostics = { + ruleset_version: '1.42.11', + rules: { + loaded: [], + failed: ['blk-001-001'], + skipped: [], + errors: { + 'missing key operator': [ + 'blk-001-001' + ] + }, + warnings: { + 'invalid tag': [ + 'blk-001-001' + ] + } + }, + processors: { + loaded: ['http-endpoint-fingerprint'], + failed: [], + skipped: [], + errors: { + 'no mappings defined': [ + 'http-endpoint-fingerprint' + ] + } + } + } + it('should send diagnostics using telemetry logs', () => { const telemetryLogHandlerAssert = sinon.stub() const telemetryLogCh = dc.channel('datadog:telemetry:log') telemetryLogCh.subscribe(telemetryLogHandlerAssert) - const product = 'ASM_DD' - const rcConfigId = '1' - const diagnostics = { - rules: { - loaded: [], - failed: ['blk-001-001'], - skipped: [], - errors: { - 'missing key operator': [ - 'blk-001-001' - ] - }, - warnings: { - 'invalid tag': [ - 'blk-001-001' - ] - } - }, - processors: { - loaded: ['http-endpoint-fingerprint'], - failed: [], - skipped: [], - warnings: { - 'no mappings defined': [ - 'http-endpoint-fingerprint' - ] - } - } - } - - Reporter.reportSuccessfulWafUpdate(product, rcConfigId, diagnostics) + Reporter.reportWafConfigUpdate(product, rcConfigId, diagnostics) expect(telemetryLogHandlerAssert).to.have.been.calledThrice expect(telemetryLogHandlerAssert.getCall(0)).to.have.been.calledWithExactly({ message: '"missing key operator": ["blk-001-001"]', level: 'ERROR', - tags: { - log_type: 'rc::asm_dd::diagnostic', - appsec_config_key: 'rules', - rc_config_id: '1' - } + tags: 'log_type:rc::asm_dd::diagnostic,appsec_config_key:rules,rc_config_id:1' }, 'datadog:telemetry:log') expect(telemetryLogHandlerAssert.getCall(1)).to.have.been.calledWithExactly({ message: '"invalid tag": ["blk-001-001"]', level: 'WARN', - tags: { - log_type: 'rc::asm_dd::diagnostic', - appsec_config_key: 'rules', - rc_config_id: '1' - } + tags: 'log_type:rc::asm_dd::diagnostic,appsec_config_key:rules,rc_config_id:1' }, 'datadog:telemetry:log') expect(telemetryLogHandlerAssert.getCall(2)).to.have.been.calledWithExactly({ message: '"no mappings defined": ["http-endpoint-fingerprint"]', - level: 'WARN', - tags: { - log_type: 'rc::asm_dd::diagnostic', - appsec_config_key: 'processors', - rc_config_id: '1' - } + level: 'ERROR', + tags: 'log_type:rc::asm_dd::diagnostic,appsec_config_key:processors,rc_config_id:1' }, 'datadog:telemetry:log') }) + + it('should increment waf.config_errors metric', () => { + Reporter.reportWafConfigUpdate(product, rcConfigId, diagnostics, '1.24.1') + + expect(telemetry.incrementWafConfigErrorsMetric).to.have.been.calledTwice + expect(telemetry.incrementWafConfigErrorsMetric).to.always.have.been.calledWithExactly('1.24.1', '1.42.11') + }) }) describe('reportAttack', () => { diff --git a/packages/dd-trace/test/appsec/rule_manager.spec.js b/packages/dd-trace/test/appsec/rule_manager.spec.js index 334ca32b869..69808455175 100644 --- a/packages/dd-trace/test/appsec/rule_manager.spec.js +++ b/packages/dd-trace/test/appsec/rule_manager.spec.js @@ -108,7 +108,28 @@ describe('AppSec Rule Manager', () => { roKey: 'roValue' }], custom_rules: [{ - piKey: 'piValue' + id: 'custom_rule1', + name: 'custom_rule1', + tags: { + tag1: 'flow1', + type: 'type1' + }, + conditions: [ + { + operator: 'match_regex', + parameters: { + inputs: [ + { + address: 'server.request.headers.no_cookies' + }, + { + address: 'server.request.query' + } + ], + regex: 'custom_rule1' + } + } + ] }] }, apply_state: UNACKNOWLEDGED @@ -125,7 +146,7 @@ describe('AppSec Rule Manager', () => { reportSuccessfulWafUpdate = sinon.stub() setDefaultBlockingActionParameters = sinon.stub() - RuleManager = proxyquire.noCallThru()('../src/appsec/rule_manager', { + RuleManager = proxyquire('../src/appsec/rule_manager', { './reporter': { reportWafUpdate, reportSuccessfulWafUpdate @@ -141,6 +162,7 @@ describe('AppSec Rule Manager', () => { sinon.stub(waf, 'removeConfig') RuleManager.clearAllRules() + config = new Config() RuleManager.loadRules(config.appsec) sinon.resetHistory() @@ -175,17 +197,23 @@ describe('AppSec Rule Manager', () => { RuleManager.updateWafFromRC(rcConfigsForNonAsmProducts) }) - sinon.assert.notCalled(waf.updateConfig) - sinon.assert.notCalled(waf.removeConfig) assert.strictEqual(rcConfigsForNonAsmProducts.toUnapply[0].apply_state, UNACKNOWLEDGED) assert.notProperty(rcConfigsForNonAsmProducts.toUnapply[0], 'apply_error') assert.strictEqual(rcConfigsForNonAsmProducts.toModify[0].apply_state, UNACKNOWLEDGED) assert.notProperty(rcConfigsForNonAsmProducts.toModify[0], 'apply_error') assert.strictEqual(rcConfigsForNonAsmProducts.toApply[0].apply_state, UNACKNOWLEDGED) assert.notProperty(rcConfigsForNonAsmProducts.toApply[0], 'apply_error') + + sinon.assert.notCalled(waf.updateConfig) + sinon.assert.notCalled(waf.removeConfig) + + assert.deepEqual(waf.wafManager.ddwaf.configPaths, [waf.wafManager.constructor.defaultWafConfigPath]) }) it('should apply configs from ASM products', () => { + waf.updateConfig.callThrough() + waf.removeConfig.callThrough() + const rcConfigs = getRcConfigs() RuleManager.updateWafFromRC(rcConfigs) @@ -194,18 +222,32 @@ describe('AppSec Rule Manager', () => { sinon.assert.calledTwice(waf.updateConfig) sinon.assert.calledWith( waf.updateConfig, + rcConfigs.toApply[0].product, + rcConfigs.toApply[0].id, rcConfigs.toApply[0].path, rcConfigs.toApply[0].file ) sinon.assert.calledWith( waf.updateConfig, + rcConfigs.toModify[0].product, + rcConfigs.toModify[0].id, rcConfigs.toModify[0].path, rcConfigs.toModify[0].file ) + + assert.deepEqual( + waf.wafManager.ddwaf.configPaths, + [ + rcConfigs.toModify[0].path, + rcConfigs.toApply[0].path, + waf.wafManager.constructor.defaultWafConfigPath + ] + ) }) it('should update apply_state and apply_error on successful apply', () => { - waf.updateConfig.returns({}) + waf.updateConfig.callThrough() + waf.removeConfig.callThrough() const rcConfigs = getRcConfigs() @@ -293,8 +335,6 @@ describe('AppSec Rule Manager', () => { }) it('should report successful waf update', () => { - waf.updateConfig.returns({}) - const rcConfigs = getRcConfigs() RuleManager.updateWafFromRC(rcConfigs) @@ -305,14 +345,10 @@ describe('AppSec Rule Manager', () => { waf.wafManager.rulesVersion, true ) - sinon.assert.calledTwice(reportSuccessfulWafUpdate) - sinon.assert.calledWith(reportSuccessfulWafUpdate, rcConfigs.toModify[0].product, rcConfigs.toModify[0].id, {}) - sinon.assert.calledWith(reportSuccessfulWafUpdate, rcConfigs.toApply[0].product, rcConfigs.toApply[0].id, {}) }) - it('should report waf update', () => { - waf.updateConfig.onFirstCall().throws(new waf.WafUpdateError({ error: 'Waf update error' })) - waf.updateConfig.onSecondCall().returns({}) + it('should report failed waf update', () => { + waf.updateConfig.throws(new waf.WafUpdateError({ error: 'Update failed' })) const rcConfigs = getRcConfigs() @@ -326,6 +362,35 @@ describe('AppSec Rule Manager', () => { ) }) + describe('ASM_DD Fallback', () => { + it('should fallback to default ruleset if no ASM_DD has been loaded successfully', () => { + waf.updateConfig.callThrough() + waf.removeConfig.callThrough() + + const rcConfigs = { + toApply: [ + { + id: 'asm_dd.test.failed', + product: 'ASM_DD', + path: 'test/rule_manager/updateWafFromRC/ASM_DD/01', + file: {} + } + ], + toModify: [], + toUnapply: [] + } + + RuleManager.updateWafFromRC(rcConfigs) + + assert.strictEqual(rcConfigs.toApply[0].apply_state, ERROR) + + assert.deepEqual( + waf.wafManager.ddwaf.configPaths, + [waf.wafManager.constructor.defaultWafConfigPath] + ) + }) + }) + describe('ASM', () => { it('should apply blocking actions', () => { waf.updateConfig.returns({}) diff --git a/packages/dd-trace/test/appsec/waf/index.spec.js b/packages/dd-trace/test/appsec/waf/index.spec.js index cc336bdf32d..d171a9b7143 100644 --- a/packages/dd-trace/test/appsec/waf/index.spec.js +++ b/packages/dd-trace/test/appsec/waf/index.spec.js @@ -51,7 +51,7 @@ describe('WAF Manager', () => { sinon.stub(Reporter, 'reportAttack') sinon.stub(Reporter, 'reportDerivatives') sinon.spy(Reporter, 'reportWafInit') - sinon.spy(Reporter, 'reportWafConfigError') + sinon.spy(Reporter, 'reportWafConfigUpdate') webContext = {} sinon.stub(web, 'getContext').returns(webContext) @@ -162,22 +162,10 @@ describe('WAF Manager', () => { ) }) - it('should fail when updating ASM_DD configs on disabled waf', () => { - assert.throws( - () => { - waf.updateAsmDdConfig('path', {}) - }, - { - name: 'Error', - message: 'Cannot update disabled WAF' - } - ) - }) - it('should fail when removing configs on disabled waf', () => { assert.throws( () => { - waf.updateAsmDdConfig('path', {}) + waf.removeConfig('path', {}) }, { name: 'Error', @@ -234,8 +222,8 @@ describe('WAF Manager', () => { it('should update WAF config - ASM / ASM_DATA', () => { DDWAF.prototype.configPaths = ['datadog/00/ASM_DD/default/config'] - waf.updateConfig('datadog/00/ASM/test/update_config', ASM_CONFIG) - waf.updateConfig('datadog/00/ASM_DATA/test/update_config', ASM_DATA_CONFIG) + waf.updateConfig('ASM', 'config_id_1', 'datadog/00/ASM/test/update_config', ASM_CONFIG) + waf.updateConfig('ASM_DATA', 'config_id_2', 'datadog/00/ASM_DATA/test/update_config', ASM_DATA_CONFIG) sinon.assert.calledWithExactly( DDWAF.prototype.createOrUpdateConfig.getCall(0), @@ -252,7 +240,7 @@ describe('WAF Manager', () => { it('should remove default rules on ASM_DD update', () => { DDWAF.prototype.configPaths = ['datadog/00/ASM_DD/default/config'] - waf.updateAsmDdConfig('datadog/00/ASM_DD/test/update_config', ASM_DD_CONFIG) + waf.updateConfig('ASM_DD', 'config_id_1', 'datadog/00/ASM_DD/test/update_config', ASM_DD_CONFIG) sinon.assert.calledOnceWithExactly( DDWAF.prototype.removeConfig, @@ -264,26 +252,6 @@ describe('WAF Manager', () => { 'datadog/00/ASM_DD/test/update_config' ) }) - - it('should apply default rules when no ASM config is present after config update fail', () => { - DDWAF.prototype.configPaths = [] - DDWAF.prototype.createOrUpdateConfig.returns(false) - - assert.throws( - () => { - waf.updateAsmDdConfig('datadog/00/ASM_DD/test/update_config', ASM_DD_CONFIG) - }, - { - name: 'WafUpdateError' - } - ) - - sinon.assert.calledWithExactly( - DDWAF.prototype.createOrUpdateConfig.getCall(1), - rules, - 'datadog/00/ASM_DD/default/config' - ) - }) }) describe('remove config', () => { @@ -294,23 +262,12 @@ describe('WAF Manager', () => { sinon.assert.calledOnceWithExactly(DDWAF.prototype.removeConfig, 'path/to/remove') }) - - it('should apply default rules when no ASM config is present after config removal', () => { - DDWAF.prototype.configPaths = [] - - waf.removeConfig('path/to/remove') - - sinon.assert.calledOnceWithExactly( - DDWAF.prototype.createOrUpdateConfig, - rules, - 'datadog/00/ASM_DD/default/config' - ) - }) }) - it('should report waf config error on config update fail', () => { + it('should throw WafUpdateError on failed update', () => { DDWAF.prototype.configPaths = [] DDWAF.prototype.createOrUpdateConfig.returns(false) + assert.throws( () => { waf.updateConfig('path', {}) @@ -319,30 +276,20 @@ describe('WAF Manager', () => { name: 'WafUpdateError' } ) - - sinon.assert.calledOnceWithExactly( - Reporter.reportWafConfigError, - '1.2.3', - '1.0.0' - ) }) - it('should report waf config error on config remove fail', () => { + it('should report waf config update', () => { DDWAF.prototype.configPaths = [] - DDWAF.prototype.removeConfig.throws(new Error('Error removing WAF config')) - assert.throws( - () => { - waf.removeConfig('path') - }, - { - name: 'Error' - } - ) + DDWAF.prototype.createOrUpdateConfig.returns(true) + + waf.updateConfig('ASM', 'configId', 'path', {}) sinon.assert.calledOnceWithExactly( - Reporter.reportWafConfigError, - '1.2.3', - '1.0.0' + Reporter.reportWafConfigUpdate, + 'ASM', + 'configId', + DDWAF.prototype.diagnostics, + '1.2.3' ) }) }) From e0006a6825f708ceee74c1a2f1a731c6a6e715f5 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Wed, 18 Jun 2025 23:00:53 +0200 Subject: [PATCH 31/37] Remove deprecated tags --- packages/dd-trace/src/appsec/reporter.js | 6 ----- .../dd-trace/test/appsec/reporter.spec.js | 9 ------- .../dd-trace/test/appsec/waf/index.spec.js | 25 ------------------- 3 files changed, 40 deletions(-) diff --git a/packages/dd-trace/src/appsec/reporter.js b/packages/dd-trace/src/appsec/reporter.js index 31de16246d2..978356bcff9 100644 --- a/packages/dd-trace/src/appsec/reporter.js +++ b/packages/dd-trace/src/appsec/reporter.js @@ -233,12 +233,6 @@ function getCollectedHeaders (req, res, shouldCollectEventHeaders, storedRespons function reportWafInit (wafVersion, rulesVersion, diagnosticsRules = {}, success = false) { if (success) { metricsQueue.set('_dd.appsec.waf.version', wafVersion) - - metricsQueue.set('_dd.appsec.event_rules.loaded', diagnosticsRules.loaded?.length || 0) - metricsQueue.set('_dd.appsec.event_rules.error_count', diagnosticsRules.failed?.length || 0) - if (diagnosticsRules.failed?.length) { - metricsQueue.set('_dd.appsec.event_rules.errors', JSON.stringify(diagnosticsRules.errors)) - } } incrementWafInitMetric(wafVersion, rulesVersion, success) diff --git a/packages/dd-trace/test/appsec/reporter.spec.js b/packages/dd-trace/test/appsec/reporter.spec.js index 253d8f9179d..79b1050e50d 100644 --- a/packages/dd-trace/test/appsec/reporter.spec.js +++ b/packages/dd-trace/test/appsec/reporter.spec.js @@ -144,19 +144,12 @@ describe('reporter', () => { Reporter.reportWafInit(wafVersion, rulesVersion, diagnosticsRules, true) expect(Reporter.metricsQueue.get('_dd.appsec.waf.version')).to.be.eq(wafVersion) - expect(Reporter.metricsQueue.get('_dd.appsec.event_rules.loaded')).to.be.eq(3) - expect(Reporter.metricsQueue.get('_dd.appsec.event_rules.error_count')).to.be.eq(1) - expect(Reporter.metricsQueue.get('_dd.appsec.event_rules.errors')) - .to.be.eq(JSON.stringify(diagnosticsRules.errors)) }) it('should not add entries to metricsQueue with success false', () => { Reporter.reportWafInit(wafVersion, rulesVersion, false) expect(Reporter.metricsQueue.get('_dd.appsec.waf.version')).to.be.undefined - expect(Reporter.metricsQueue.get('_dd.appsec.event_rules.loaded')).to.be.undefined - expect(Reporter.metricsQueue.get('_dd.appsec.event_rules.error_count')).to.be.undefined - expect(Reporter.metricsQueue.get('_dd.appsec.event_rules.errors')).to.be.undefined }) it('should call incrementWafInitMetric', () => { @@ -173,8 +166,6 @@ describe('reporter', () => { Reporter.reportWafInit(wafVersion, rulesVersion, diagnosticsRules, true) expect(telemetry.incrementWafInitMetric).to.have.been.calledOnceWithExactly(wafVersion, rulesVersion, true) - expect(Reporter.metricsQueue.get('_dd.appsec.event_rules.loaded')).to.be.eq(0) - expect(Reporter.metricsQueue.get('_dd.appsec.event_rules.error_count')).to.be.eq(0) }) }) diff --git a/packages/dd-trace/test/appsec/waf/index.spec.js b/packages/dd-trace/test/appsec/waf/index.spec.js index d171a9b7143..8b643d273a3 100644 --- a/packages/dd-trace/test/appsec/waf/index.spec.js +++ b/packages/dd-trace/test/appsec/waf/index.spec.js @@ -95,31 +95,6 @@ describe('WAF Manager', () => { waf.init(rules, config.appsec) expect(Reporter.metricsQueue.set).to.have.been.calledWithExactly('_dd.appsec.waf.version', '1.2.3') - expect(Reporter.metricsQueue.set).to.have.been.calledWithExactly('_dd.appsec.event_rules.loaded', 1) - expect(Reporter.metricsQueue.set).to.have.been.calledWithExactly('_dd.appsec.event_rules.error_count', 0) - expect(Reporter.metricsQueue.set).not.to.have.been.calledWith('_dd.appsec.event_rules.errors') - }) - - it('should set init metrics with errors', () => { - DDWAF.version.returns('2.3.4') - DDWAF.prototype.diagnostics = { - rules: { - loaded: ['rule_1'], - failed: ['rule_2', 'rule_3'], - errors: { - error_1: ['invalid_1'], - error_2: ['invalid_2', 'invalid_3'] - } - } - } - - waf.init(rules, config.appsec) - - expect(Reporter.metricsQueue.set).to.have.been.calledWithExactly('_dd.appsec.waf.version', '2.3.4') - expect(Reporter.metricsQueue.set).to.have.been.calledWithExactly('_dd.appsec.event_rules.loaded', 1) - expect(Reporter.metricsQueue.set).to.have.been.calledWithExactly('_dd.appsec.event_rules.error_count', 2) - expect(Reporter.metricsQueue.set).to.have.been.calledWithExactly('_dd.appsec.event_rules.errors', - '{"error_1":["invalid_1"],"error_2":["invalid_2","invalid_3"]}') }) }) From 992eca386d1fbee634901fb826d001f5fe67542c Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Wed, 18 Jun 2025 23:17:34 +0200 Subject: [PATCH 32/37] Fix assertion to check unsortered array --- packages/dd-trace/test/appsec/rule_manager.spec.js | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/packages/dd-trace/test/appsec/rule_manager.spec.js b/packages/dd-trace/test/appsec/rule_manager.spec.js index 69808455175..dc854d042de 100644 --- a/packages/dd-trace/test/appsec/rule_manager.spec.js +++ b/packages/dd-trace/test/appsec/rule_manager.spec.js @@ -235,14 +235,10 @@ describe('AppSec Rule Manager', () => { rcConfigs.toModify[0].file ) - assert.deepEqual( - waf.wafManager.ddwaf.configPaths, - [ - rcConfigs.toModify[0].path, - rcConfigs.toApply[0].path, - waf.wafManager.constructor.defaultWafConfigPath - ] - ) + assert.strictEqual(waf.wafManager.ddwaf.configPaths.length, 3) + assert.include(waf.wafManager.ddwaf.configPaths, waf.wafManager.constructor.defaultWafConfigPath) + assert.include(waf.wafManager.ddwaf.configPaths, rcConfigs.toApply[0].path) + assert.include(waf.wafManager.ddwaf.configPaths, rcConfigs.toModify[0].path) }) it('should update apply_state and apply_error on successful apply', () => { From 610912246317cf7a698e87773171e276bb1f1a2e Mon Sep 17 00:00:00 2001 From: Carles Capell <107924659+CarlesDD@users.noreply.github.com> Date: Fri, 20 Jun 2025 06:48:23 +0200 Subject: [PATCH 33/37] Update packages/dd-trace/src/appsec/waf/index.js Co-authored-by: simon-id --- packages/dd-trace/src/appsec/waf/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dd-trace/src/appsec/waf/index.js b/packages/dd-trace/src/appsec/waf/index.js index 45833373bf1..8c37b2893d6 100644 --- a/packages/dd-trace/src/appsec/waf/index.js +++ b/packages/dd-trace/src/appsec/waf/index.js @@ -64,7 +64,7 @@ function updateConfig (product, configId, configPath, config) { waf.wafManager.removeConfig(waf.wafManager.constructor.defaultWafConfigPath) } - const updateSucceed = waf.wafManager.updateConfig(configPath, config) + const updateSucceeded = waf.wafManager.updateConfig(configPath, config) Reporter.reportWafConfigUpdate(product, configId, waf.wafManager.ddwaf.diagnostics, waf.wafManager.ddwafVersion) if (!updateSucceed) { From dbbc08d000803c9cb7a43499326a21adab06e33b Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Fri, 20 Jun 2025 06:49:05 +0200 Subject: [PATCH 34/37] Fix var name --- packages/dd-trace/src/appsec/waf/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dd-trace/src/appsec/waf/index.js b/packages/dd-trace/src/appsec/waf/index.js index 8c37b2893d6..3d08fee257d 100644 --- a/packages/dd-trace/src/appsec/waf/index.js +++ b/packages/dd-trace/src/appsec/waf/index.js @@ -67,7 +67,7 @@ function updateConfig (product, configId, configPath, config) { const updateSucceeded = waf.wafManager.updateConfig(configPath, config) Reporter.reportWafConfigUpdate(product, configId, waf.wafManager.ddwaf.diagnostics, waf.wafManager.ddwafVersion) - if (!updateSucceed) { + if (!updateSucceeded) { throw new WafUpdateError(waf.wafManager.ddwaf.diagnostics) } } catch (err) { From 1d38586f25a89fe3fee6297b37c6934e89a75b68 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Fri, 20 Jun 2025 10:17:07 +0200 Subject: [PATCH 35/37] set unknown event rules version in telemetry --- packages/dd-trace/src/appsec/reporter.js | 6 +++--- packages/dd-trace/src/appsec/telemetry/common.js | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/dd-trace/src/appsec/reporter.js b/packages/dd-trace/src/appsec/reporter.js index 978356bcff9..88bfb020088 100644 --- a/packages/dd-trace/src/appsec/reporter.js +++ b/packages/dd-trace/src/appsec/reporter.js @@ -251,7 +251,7 @@ function logWafDiagnosticMessage (product, rcConfigId, configKey, message, level function reportWafConfigUpdate (product, rcConfigId, diagnostics, wafVersion) { if (diagnostics.error) { logWafDiagnosticMessage(product, rcConfigId, '', diagnostics.error, 'ERROR') - incrementWafConfigErrorsMetric(wafVersion, diagnostics.ruleset_version || 'unknown') + incrementWafConfigErrorsMetric(wafVersion, diagnostics.ruleset_version) } for (const configKey of WAF_DIAGNOSTICS_CONFIG_KEYS_TO_REPORT) { @@ -260,7 +260,7 @@ function reportWafConfigUpdate (product, rcConfigId, diagnostics, wafVersion) { if (configDiagnostics.error) { logWafDiagnosticMessage(product, rcConfigId, configKey, configDiagnostics.error, 'ERROR') - incrementWafConfigErrorsMetric(wafVersion, diagnostics.ruleset_version || 'unknown') + incrementWafConfigErrorsMetric(wafVersion, diagnostics.ruleset_version) continue } @@ -273,7 +273,7 @@ function reportWafConfigUpdate (product, rcConfigId, diagnostics, wafVersion) { `"${errorMessage}": ${JSON.stringify(errorIds)}`, 'ERROR' ) - incrementWafConfigErrorsMetric(wafVersion, diagnostics.ruleset_version || 'unknown') + incrementWafConfigErrorsMetric(wafVersion, diagnostics.ruleset_version) } } diff --git a/packages/dd-trace/src/appsec/telemetry/common.js b/packages/dd-trace/src/appsec/telemetry/common.js index d91cd9988b7..a63e4b133ff 100644 --- a/packages/dd-trace/src/appsec/telemetry/common.js +++ b/packages/dd-trace/src/appsec/telemetry/common.js @@ -17,7 +17,7 @@ const tags = { function getVersionsTags (wafVersion, rulesVersion) { return { [tags.WAF_VERSION]: wafVersion, - [tags.EVENT_RULES_VERSION]: rulesVersion + [tags.EVENT_RULES_VERSION]: rulesVersion || 'unknown' } } From 5d0c05968d47c42638bd84b4e5a2d75bdd8acdcf Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Fri, 20 Jun 2025 12:14:29 +0200 Subject: [PATCH 36/37] Clean up things --- packages/dd-trace/test/appsec/rule_manager.spec.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/dd-trace/test/appsec/rule_manager.spec.js b/packages/dd-trace/test/appsec/rule_manager.spec.js index dc854d042de..0fcf05fb4ce 100644 --- a/packages/dd-trace/test/appsec/rule_manager.spec.js +++ b/packages/dd-trace/test/appsec/rule_manager.spec.js @@ -138,18 +138,16 @@ describe('AppSec Rule Manager', () => { } let RuleManager - let reportSuccessfulWafUpdate, reportWafUpdate + let reportWafUpdate let setDefaultBlockingActionParameters beforeEach(() => { reportWafUpdate = sinon.stub() - reportSuccessfulWafUpdate = sinon.stub() setDefaultBlockingActionParameters = sinon.stub() RuleManager = proxyquire('../src/appsec/rule_manager', { './reporter': { - reportWafUpdate, - reportSuccessfulWafUpdate + reportWafUpdate }, './blocking': { setDefaultBlockingActionParameters From ba4181968bbe3d65fc45fd7a1de299f8f9f8e7d2 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Fri, 20 Jun 2025 14:21:39 +0200 Subject: [PATCH 37/37] Extract diagnostic keys to have only one source of truth --- packages/dd-trace/src/appsec/reporter.js | 15 ++------------- packages/dd-trace/src/appsec/rule_manager.js | 11 +---------- packages/dd-trace/src/appsec/waf/diagnostics.js | 15 +++++++++++++++ 3 files changed, 18 insertions(+), 23 deletions(-) create mode 100644 packages/dd-trace/src/appsec/waf/diagnostics.js diff --git a/packages/dd-trace/src/appsec/reporter.js b/packages/dd-trace/src/appsec/reporter.js index 88bfb020088..2e9c8ac6927 100644 --- a/packages/dd-trace/src/appsec/reporter.js +++ b/packages/dd-trace/src/appsec/reporter.js @@ -20,6 +20,7 @@ const { } = require('./telemetry') const { keepTrace } = require('../priority_sampler') const { ASM } = require('../standalone/product') +const { DIAGNOSTIC_KEYS } = require('./waf/diagnostics') const REQUEST_HEADER_TAG_PREFIX = 'http.request.headers.' const RESPONSE_HEADER_TAG_PREFIX = 'http.response.headers.' @@ -30,18 +31,6 @@ const COLLECTED_REQUEST_BODY_MAX_ELEMENTS_PER_NODE = 256 const telemetryLogCh = dc.channel('datadog:telemetry:log') -const WAF_DIAGNOSTICS_CONFIG_KEYS_TO_REPORT = [ - 'rules', - 'custom_rules', - 'exclusions', - 'actions', - 'processors', - 'scanners', - 'rules_override', - 'rules_data', - 'exclusion_data' -] - // default limiter, configurable with setRateLimit() let limiter = new Limiter(100) @@ -254,7 +243,7 @@ function reportWafConfigUpdate (product, rcConfigId, diagnostics, wafVersion) { incrementWafConfigErrorsMetric(wafVersion, diagnostics.ruleset_version) } - for (const configKey of WAF_DIAGNOSTICS_CONFIG_KEYS_TO_REPORT) { + for (const configKey of DIAGNOSTIC_KEYS) { const configDiagnostics = diagnostics[configKey] if (!configDiagnostics) continue diff --git a/packages/dd-trace/src/appsec/rule_manager.js b/packages/dd-trace/src/appsec/rule_manager.js index 41e7c211983..f50e71d708f 100644 --- a/packages/dd-trace/src/appsec/rule_manager.js +++ b/packages/dd-trace/src/appsec/rule_manager.js @@ -2,22 +2,13 @@ const fs = require('fs') const waf = require('./waf') +const { DIAGNOSTIC_KEYS } = require('./waf/diagnostics') const { ACKNOWLEDGED, ERROR } = require('../remote_config/apply_states') const Reporter = require('./reporter') const blocking = require('./blocking') const ASM_PRODUCTS = new Set(['ASM', 'ASM_DD', 'ASM_DATA']) -const DIAGNOSTIC_KEYS = [ - 'exclusions', - 'rules', - 'processors', - 'rules_override', - 'rules_data', - 'custom_rules', - 'actions', - 'scanners' -] /* ASM Actions must be tracked in order to update the defaultBlockingActions in blocking. These actions are used diff --git a/packages/dd-trace/src/appsec/waf/diagnostics.js b/packages/dd-trace/src/appsec/waf/diagnostics.js new file mode 100644 index 00000000000..d949240c6e7 --- /dev/null +++ b/packages/dd-trace/src/appsec/waf/diagnostics.js @@ -0,0 +1,15 @@ +'use strict' + +module.exports = { + DIAGNOSTIC_KEYS: [ + 'rules', + 'custom_rules', + 'exclusions', + 'actions', + 'processors', + 'scanners', + 'rules_override', + 'rules_data', + 'exclusion_data' + ] +}