From a8ca1352086a09fcfc0d0ae373656f21f88d40e5 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Tue, 13 Jan 2026 15:53:14 +0100 Subject: [PATCH 1/8] WIP --- .../config/AppSecConfigServiceImpl.java | 93 ++++-- .../ddwaf/WAFModuleSpecification.groovy | 8 +- ...RemoteConfigDuplicateRulesSmokeTest.groovy | 292 ++++++++++++++++++ .../remoteconfig/state/ProductState.java | 36 +-- .../state/ProductStateSpecification.groovy | 76 +---- 5 files changed, 371 insertions(+), 134 deletions(-) create mode 100644 dd-smoke-tests/appsec/springboot/src/test/groovy/datadog/smoketest/appsec/RemoteConfigDuplicateRulesSmokeTest.groovy diff --git a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/config/AppSecConfigServiceImpl.java b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/config/AppSecConfigServiceImpl.java index 95ff767f2f6..ffeed822bb8 100644 --- a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/config/AppSecConfigServiceImpl.java +++ b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/config/AppSecConfigServiceImpl.java @@ -64,6 +64,7 @@ import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -220,6 +221,11 @@ public String getCurrentRuleVersion() { } private class AppSecConfigChangesListener implements ProductListener { + + // Deferred operations - collected during accept/remove, executed during commit + protected final Map> configsToApply = new HashMap<>(); + protected final Set configsToRemove = new HashSet<>(); + @Override public void accept(ConfigKey configKey, byte[] content, PollingRateHinter pollingRateHinter) throws IOException { @@ -235,13 +241,14 @@ public void accept(ConfigKey configKey, byte[] content, PollingRateHinter pollin ignoredConfigKeys.add(key); } else { ignoredConfigKeys.remove(key); - try { - beforeApply(key, contentMap); - maybeInitializeDefaultConfig(); - handleWafUpdateResultReport(key, contentMap); - } catch (AppSecModule.AppSecModuleActivationException e) { - throw new RuntimeException(e); - } + beforeApply(key, contentMap); + maybeInitializeDefaultConfig(); + + // DEFER: Store config to apply in commit phase + configsToApply.put(key, contentMap); + + // Cancel any pending remove for this key (accept overrides remove) + configsToRemove.remove(key); } } @@ -252,18 +259,46 @@ public void remove(ConfigKey configKey, PollingRateHinter pollingRateHinter) if (ignoredConfigKeys.remove(key)) { return; } - try { - maybeInitializeDefaultConfig(); - wafBuilder.removeConfig(key); - afterRemove(key); - } catch (UnclassifiedWafException e) { - throw new RuntimeException(e); - } + + // DEFER: Mark config for removal in commit phase (don't execute immediately) + configsToRemove.add(key); + + // Cancel any pending apply for this key (remove overrides apply) + configsToApply.remove(key); + + // Notify subclass that removal is pending + afterRemove(key); } @Override public void commit(PollingRateHinter pollingRateHinter) { - // no action needed + // Execute deferred operations atomically: + // 1. FIRST: Remove obsolete configs + // 2. THEN: Apply new/updated configs + // This ensures no duplicate rules in memory and no inconsistent state + + // Phase 1: Execute all pending removes + for (String key : configsToRemove) { + try { + maybeInitializeDefaultConfig(); + wafBuilder.removeConfig(key); + } catch (UnclassifiedWafException e) { + throw new RuntimeException(e); + } + } + + // Phase 2: Execute all pending applies + for (Map.Entry> entry : configsToApply.entrySet()) { + try { + handleWafUpdateResultReport(entry.getKey(), entry.getValue()); + } catch (AppSecModule.AppSecModuleActivationException e) { + throw new RuntimeException(e); + } + } + + // Clear deferred operations after successful commit + configsToRemove.clear(); + configsToApply.clear(); } protected void beforeApply(final String key, final Map contentMap) {} @@ -274,21 +309,33 @@ protected void afterRemove(final String key) {} private class AppSecConfigChangesDDListener extends AppSecConfigChangesListener { @Override protected void beforeApply(final String key, final Map config) { - if (defaultConfigActivated) { // if we get any config, remove the default one - log.debug("Removing default config ASM_DD/default"); + // Track that we're using this DD config key (for accounting) + usedDDWafConfigKeys.add(key); + } + + @Override + protected void afterRemove(final String key) { + // Track removal from DD config keys (for accounting) + usedDDWafConfigKeys.remove(key); + } + + @Override + public void commit(PollingRateHinter pollingRateHinter) { + // Special handling for ASM_DD: Remove default config before applying remote configs + // This must happen atomically with the other operations to avoid duplicate rules + if (defaultConfigActivated && !configsToApply.isEmpty()) { + log.debug("Removing default config ASM_DD/default before applying remote configs"); try { + maybeInitializeDefaultConfig(); wafBuilder.removeConfig(DEFAULT_WAF_CONFIG_RULE); + defaultConfigActivated = false; } catch (UnclassifiedWafException e) { throw new RuntimeException(e); } - defaultConfigActivated = false; } - usedDDWafConfigKeys.add(key); - } - @Override - protected void afterRemove(final String key) { - usedDDWafConfigKeys.remove(key); + // Now execute the standard commit flow (removes first, then applies) + super.commit(pollingRateHinter); } } diff --git a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/ddwaf/WAFModuleSpecification.groovy b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/ddwaf/WAFModuleSpecification.groovy index 2483d5af410..61a837b1791 100644 --- a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/ddwaf/WAFModuleSpecification.groovy +++ b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/ddwaf/WAFModuleSpecification.groovy @@ -145,10 +145,12 @@ class WAFModuleSpecification extends DDSpecification { ConfigKey config = new ParsedConfigKey(configKey, 'null', 1, 'null', 'null') if(map == null) { listener.remove(config, null) - return + } else { + def json = ADAPTER.toJson(map) + listener.accept(config, json.getBytes(), null) } - def json = ADAPTER.toJson(map) - listener.accept(config, json.getBytes(), null) + // Trigger commit to execute deferred operations (P2 deferred commit pattern) + listener.commit(null) } diff --git a/dd-smoke-tests/appsec/springboot/src/test/groovy/datadog/smoketest/appsec/RemoteConfigDuplicateRulesSmokeTest.groovy b/dd-smoke-tests/appsec/springboot/src/test/groovy/datadog/smoketest/appsec/RemoteConfigDuplicateRulesSmokeTest.groovy new file mode 100644 index 00000000000..5ac4bb2b156 --- /dev/null +++ b/dd-smoke-tests/appsec/springboot/src/test/groovy/datadog/smoketest/appsec/RemoteConfigDuplicateRulesSmokeTest.groovy @@ -0,0 +1,292 @@ +package datadog.smoketest.appsec + +import datadog.trace.agent.test.utils.OkHttpUtils +import okhttp3.Request +import spock.lang.Shared + +import java.util.concurrent.TimeUnit + +/** + * Smoke test that simulates the real production scenario where: + * 1. default_config.json is loaded with base rules + * 2. Remote Config sends an "update" with: + * - Rules that ALREADY EXIST (should REPLACE, not duplicate) + * - New rules that weren't in default + * 3. libddwaf detects DUPLICATES and generates warnings + * + * In production we saw: + * - 159 rules in default_config.json + * - 172 duplicate warnings + * - 158 rules duplicated (existed in both) + * - 14 new rules (only in Remote Config) + */ +class RemoteConfigDuplicateRulesSmokeTest extends AbstractAppSecServerSmokeTest { + + @Override + def logLevel() { + 'DEBUG' + } + + @Shared + String buildDir = new File(System.getProperty("datadog.smoketest.builddir")).absolutePath + + @Shared + String customRulesPath = "${buildDir}/appsec_remoteconfig_base_rules.json" + + @Shared + List capturedLogs = [] + + @Override + void beforeProcessBuilders() { + // Set Remote Config BEFORE creating the process + prepareRemoteConfigWithDuplicates() + super.beforeProcessBuilders() + } + + def prepareRemoteConfigWithDuplicates() { + // This simulates Remote Config sending updates with same rule IDs + def remoteConfigRules = [ + // DUPLICATED RULES - Same IDs as in base config + // In production there were 158 duplicated rules + [ + id: 'blk-001-001', // DUPLICATED + name: 'Block IP Addresses - Updated via Remote Config', + tags: [ + type: 'block_ip', + category: 'security_scanner', + updated: 'true' // Marker to identify it came from RC + ], + conditions: [ + [ + parameters: [ + inputs: [[address: 'http.client_ip']], + data: 'blocked_ips' + ], + operator: 'ip_match' + ] + ], + transformers: ['removeNulls'], + on_match: ['block'] + ], + [ + id: 'blk-001-002', // DUPLICATED + name: 'Block User Addresses - Updated', + tags: [type: 'block_user', category: 'security_scanner'], + conditions: [ + [ + parameters: [ + inputs: [[address: 'usr.id']], + data: 'blocked_users' + ], + operator: 'exact_match' + ] + ], + transformers: [], + on_match: ['block'] + ], + [ + id: 'crs-913-110', // DUPLICATED + name: 'Acunetix - Updated', + tags: [type: 'security_scanner', category: 'attack_attempt'], + conditions: [ + [ + parameters: [ + inputs: [[address: 'server.request.headers.no_cookies']], + key_path: ['user-agent'], + regex: 'Acunetix' + ], + operator: 'match_regex' + ] + ], + transformers: ['lowercase'], + on_match: [] + ], + [ + id: 'crs-921-110', // DUPLICATED + name: 'HTTP Request Smuggling Attack - Updated', + tags: [type: 'http_protocol_violation', category: 'attack_attempt'], + conditions: [ + [ + parameters: [ + inputs: [[address: 'server.request.headers.no_cookies']], + key_path: ['content-length'], + regex: '.*,.*' + ], + operator: 'match_regex' + ] + ], + transformers: [], + on_match: [] + ], + [ + id: 'crs-942-100', // DUPLICATED + name: 'SQL Injection Attack - Updated', + tags: [type: 'sql_injection', category: 'attack_attempt'], + conditions: [ + [ + parameters: [ + inputs: [[address: 'server.request.query']] + ], + operator: 'is_sqli' + ] + ], + transformers: [], + on_match: [] + ], + // NEW RULES - Simulate the 14 rules that only exist in Remote Config + [ + id: 'api-001-100', // NEW + name: 'API Security Rule 100', + tags: [type: 'api_security', category: 'attack_attempt'], + conditions: [ + [ + parameters: [ + inputs: [[address: 'server.request.uri.raw']], + regex: '/api/.*' + ], + operator: 'match_regex' + ] + ], + transformers: [], + on_match: [] + ], + [ + id: 'api-001-110', // NEW + name: 'API Security Rule 110', + tags: [type: 'api_security', category: 'attack_attempt'], + conditions: [ + [ + parameters: [ + inputs: [[address: 'server.request.method']], + list: ['PUT', 'DELETE'] + ], + operator: 'match' + ] + ], + transformers: [], + on_match: [] + ] + ] + + def remoteConfigProcessors = [ + // DUPLICATED PROCESSOR + [ + id: "http-network-fingerprint", // DUPLICATED + generator: "http_network_fingerprint", + conditions: [ + [ + operator: "exists", + parameters: [ + inputs: [[address: "waf.context.event"]] + ] + ] + ], + parameters: [ + mappings: [ + [ + headers: [[address: "server.request.headers.no_cookies"]], + output: "_dd.appsec.fp.http.network.updated" // Different output + ] + ] + ], + evaluate: false, + output: true + ] + ] + + def remoteConfigData = [ + version: "2.2", + metadata: [ + rules_version: "1.1.0-remoteconfig" + ], + rules: remoteConfigRules, + processors: remoteConfigProcessors + ] + + // Set Remote Config using the inherited infrastructure + setRemoteConfig("datadog/2/ASM_DD/config_id/config", groovy.json.JsonOutput.toJson(remoteConfigData)) + } + + def prepareBaseConfiguration() { + // DON'T provide a custom rules file - let the agent use its default bundled config + // This way Remote Config will be applied (agent refuses RC when custom rules are provided) + + // Enable Remote Config (parent class disables it by default) + defaultAppSecProperties += "-Ddd.remote_config.enabled=true" as String + defaultAppSecProperties += "-Ddd.remote_config.url=http://localhost:${server.address.port}/v0.7/config" as String + defaultAppSecProperties += "-Ddd.remote_config.poll_interval.seconds=1" as String + defaultAppSecProperties += "-Ddd.remote_config.integrity_check.enabled=false" as String + } + + @Override + ProcessBuilder createProcessBuilder() { + prepareBaseConfiguration() + + String springBootShadowJar = System.getProperty("datadog.smoketest.appsec.springboot.shadowJar.path") + + List command = new ArrayList<>() + command.add(javaPath()) + command.addAll(defaultJavaProperties) + command.addAll(defaultAppSecProperties) + command.addAll((String[]) ["-jar", springBootShadowJar, "--server.port=${httpPort}"]) + + ProcessBuilder processBuilder = new ProcessBuilder(command) + processBuilder.directory(new File(buildDirectory)) + processBuilder.redirectErrorStream(true) + + return processBuilder + } + + void 'test Remote Config applies without duplicate warnings'() { + when: 'we wait for the agent to start and apply Remote Config' + // Remote Config was already set in beforeProcessBuilders() BEFORE process started + // Now we just need to wait for agent to start and apply it + Thread.sleep(5000) // Give time for agent to initialize and apply RC + + then: 'we make a request to ensure the app is running' + def url = "http://localhost:${httpPort}/greeting" + def client = OkHttpUtils.clientBuilder() + .readTimeout(30, TimeUnit.SECONDS) + .build() + def request = new Request.Builder() + .url(url) + .get() + .build() + def response = client.newCall(request).execute() + + response.code() == 200 + + and: 'Remote Config is applied successfully WITHOUT duplicate warnings' + Thread.sleep(2000) // Give time for all logs to be written + + // Read logs from the log file (process output is captured to file by ProcessManager) + capturedLogs.clear() + def logFile = new File(logFilePath) + if (logFile.exists()) { + capturedLogs.addAll(logFile.readLines()) + } + + // Verify Remote Config was applied + def remoteConfigApplied = capturedLogs.find { + it.contains('AppSec loaded') && it.contains('rules from file datadog/2/ASM_DD/config_id/config') + } + + def defaultConfigRemoved = capturedLogs.find { + it.contains('Removing default config ASM_DD/default') + } + + // Check for NO duplicate warnings (proving the fix works) + def duplicateRuleWarnings = capturedLogs.findAll { + it.contains('WARN') && it.contains('ddwaf_native') && it.contains('Duplicate rule') + } + def duplicateProcessorWarnings = capturedLogs.findAll { + it.contains('WARN') && it.contains('ddwaf_native') && it.contains('Duplicate processor') + } + + // Verifications - The fix should work, so NO duplicates expected + assert defaultConfigRemoved != null, "Default config should be removed before applying Remote Config" + assert remoteConfigApplied != null, "Remote Config should be applied successfully" + assert duplicateRuleWarnings.isEmpty(), "Expected NO duplicate rule warnings (fix is working), but found: ${duplicateRuleWarnings.size()}" + assert duplicateProcessorWarnings.isEmpty(), "Expected NO duplicate processor warnings (fix is working), but found: ${duplicateProcessorWarnings.size()}" + } +} diff --git a/remote-config/remote-config-core/src/main/java/datadog/remoteconfig/state/ProductState.java b/remote-config/remote-config-core/src/main/java/datadog/remoteconfig/state/ProductState.java index 91a70c7be7b..56763b4b976 100644 --- a/remote-config/remote-config-core/src/main/java/datadog/remoteconfig/state/ProductState.java +++ b/remote-config/remote-config-core/src/main/java/datadog/remoteconfig/state/ProductState.java @@ -58,10 +58,8 @@ public boolean apply( errors = null; List configBeenUsedByProduct = new ArrayList<>(); - List changedKeys = new ArrayList<>(); boolean changesDetected = false; - // Step 1: Detect all changes for (ParsedConfigKey configKey : relevantKeys) { try { RemoteConfigResponse.Targets.ConfigTarget target = @@ -70,28 +68,14 @@ public boolean apply( if (isTargetChanged(configKey, target)) { changesDetected = true; - changedKeys.add(configKey); - } - } catch (ReportableException e) { - recordError(e); - } - } - - // Step 2: For products other than ASM_DD, apply changes immediately - if (product != Product.ASM_DD) { - for (ParsedConfigKey configKey : changedKeys) { - try { byte[] content = getTargetFileContent(fleetResponse, configKey); callListenerApplyTarget(fleetResponse, hinter, configKey, content); - } catch (ReportableException e) { - recordError(e); } + } catch (ReportableException e) { + recordError(e); } } - // Step 3: Remove obsolete configurations (for all products) - // For ASM_DD, this is critical: removes MUST happen before applies to prevent - // duplicate rule warnings from the ddwaf rule parser and causing memory spikes. List keysToRemove = cachedTargetFiles.keySet().stream() .filter(configKey -> !configBeenUsedByProduct.contains(configKey)) @@ -102,22 +86,6 @@ public boolean apply( callListenerRemoveTarget(hinter, configKey); } - // Step 4: For ASM_DD, apply changes AFTER removes - // TODO: This is a temporary solution. The proper fix requires better synchronization - // between remove and add/update operations. This should be discussed - // with the guild to determine the best long-term design approach. - if (product == Product.ASM_DD) { - for (ParsedConfigKey configKey : changedKeys) { - try { - byte[] content = getTargetFileContent(fleetResponse, configKey); - callListenerApplyTarget(fleetResponse, hinter, configKey, content); - } catch (ReportableException e) { - recordError(e); - } - } - } - - // Step 5: Commit if there were changes if (changesDetected) { try { callListenerCommit(hinter); diff --git a/remote-config/remote-config-core/src/test/groovy/datadog/remoteconfig/state/ProductStateSpecification.groovy b/remote-config/remote-config-core/src/test/groovy/datadog/remoteconfig/state/ProductStateSpecification.groovy index ab81e3f3c81..4d986ad96a4 100644 --- a/remote-config/remote-config-core/src/test/groovy/datadog/remoteconfig/state/ProductStateSpecification.groovy +++ b/remote-config/remote-config-core/src/test/groovy/datadog/remoteconfig/state/ProductStateSpecification.groovy @@ -10,8 +10,8 @@ class ProductStateSpecification extends Specification { PollingRateHinter hinter = Mock() - void 'test apply for non-ASM_DD product applies changes before removes'() { - given: 'a ProductState for ASM_DATA' + void 'test apply with new and updated configs'() { + given: 'a ProductState' def productState = new ProductState(Product.ASM_DATA) def listener = new OrderRecordingListener() productState.addProductListener(listener) @@ -45,78 +45,6 @@ class ProductStateSpecification extends Specification { ] } - void 'test apply for ASM_DD product applies changes after removes'() { - given: 'a ProductState for ASM_DD' - def productState = new ProductState(Product.ASM_DD) - def listener = new OrderRecordingListener() - productState.addProductListener(listener) - - and: 'first apply with config1 and config2 to cache them' - def response1 = buildResponse([ - 'org/ASM_DD/config1/foo': [version: 1, length: 8, hash: 'oldhash1'], - 'org/ASM_DD/config2/foo': [version: 1, length: 8, hash: 'hash2'] - ]) - def key1 = ParsedConfigKey.parse('org/ASM_DD/config1/foo') - def key2 = ParsedConfigKey.parse('org/ASM_DD/config2/foo') - productState.apply(response1, [key1, key2], hinter) - listener.operations.clear() // Clear for the actual test - - and: 'a new response with only config1 (changed hash) - config2 will be removed' - def response2 = buildResponse([ - 'org/ASM_DD/config1/foo': [version: 2, length: 8, hash: 'newhash1'] - ]) - - when: 'apply is called' - def changed = productState.apply(response2, [key1], hinter) - - then: 'changes are detected' - changed - - and: 'operations happen in order: remove config2 FIRST, then apply config1, then commit' - listener.operations == ['remove:org/ASM_DD/config2/foo', 'accept:org/ASM_DD/config1/foo', 'commit'] - } - - void 'test ASM_DD with multiple new configs removes before applies all'() { - given: 'a ProductState for ASM_DD' - def productState = new ProductState(Product.ASM_DD) - def listener = new OrderRecordingListener() - productState.addProductListener(listener) - - and: 'first apply with old configs' - def response1 = buildResponse([ - 'org/ASM_DD/old1/foo': [version: 1, length: 8, hash: 'hash_old1'], - 'org/ASM_DD/old2/foo': [version: 1, length: 8, hash: 'hash_old2'] - ]) - def oldKey1 = ParsedConfigKey.parse('org/ASM_DD/old1/foo') - def oldKey2 = ParsedConfigKey.parse('org/ASM_DD/old2/foo') - productState.apply(response1, [oldKey1, oldKey2], hinter) - listener.operations.clear() // Clear for the actual test - - and: 'a response with completely new configs' - def response2 = buildResponse([ - 'org/ASM_DD/new1/foo': [version: 1, length: 8, hash: 'hash_new1'], - 'org/ASM_DD/new2/foo': [version: 1, length: 8, hash: 'hash_new2'] - ]) - def newKey1 = ParsedConfigKey.parse('org/ASM_DD/new1/foo') - def newKey2 = ParsedConfigKey.parse('org/ASM_DD/new2/foo') - - when: 'apply is called' - def changed = productState.apply(response2, [newKey1, newKey2], hinter) - - then: 'changes are detected' - changed - - and: 'all removes happen before all applies' - listener.operations.size() == 5 // 2 removes + 2 accepts + 1 commit - listener.operations.findAll { it.startsWith('remove:') }.size() == 2 - listener.operations.findAll { it.startsWith('accept:') }.size() == 2 - - and: 'removes come before accepts' - def lastRemoveIdx = listener.operations.findLastIndexOf { it.startsWith('remove:') } - def firstAcceptIdx = listener.operations.findIndexOf { it.startsWith('accept:') } - lastRemoveIdx < firstAcceptIdx - } - void 'test no changes detected when config hashes match'() { given: 'a ProductState' def productState = new ProductState(Product.ASM_DATA) From 734a58869595d40414cd9a5b966acf3aa2e03cdc Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Wed, 14 Jan 2026 13:50:26 +0100 Subject: [PATCH 2/8] WIP --- .../config/AppSecConfigServiceImpl.java | 13 +- ...ppSecConfigServiceImplSpecification.groovy | 115 ++++++++++++++++++ 2 files changed, 123 insertions(+), 5 deletions(-) diff --git a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/config/AppSecConfigServiceImpl.java b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/config/AppSecConfigServiceImpl.java index ffeed822bb8..f159f213bf6 100644 --- a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/config/AppSecConfigServiceImpl.java +++ b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/config/AppSecConfigServiceImpl.java @@ -279,11 +279,14 @@ public void commit(PollingRateHinter pollingRateHinter) { // Phase 1: Execute all pending removes for (String key : configsToRemove) { - try { - maybeInitializeDefaultConfig(); - wafBuilder.removeConfig(key); - } catch (UnclassifiedWafException e) { - throw new RuntimeException(e); + // Only remove if the config was actually added to the WAF + if (usedDDWafConfigKeys.contains(key)) { + try { + maybeInitializeDefaultConfig(); + wafBuilder.removeConfig(key); + } catch (UnclassifiedWafException e) { + throw new RuntimeException(e); + } } } diff --git a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/config/AppSecConfigServiceImplSpecification.groovy b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/config/AppSecConfigServiceImplSpecification.groovy index e42b82a4b7d..6f819c89a93 100644 --- a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/config/AppSecConfigServiceImplSpecification.groovy +++ b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/config/AppSecConfigServiceImplSpecification.groovy @@ -705,6 +705,7 @@ class AppSecConfigServiceImplSpecification extends DDSpecification { void 'config keys are added and removed to the set when receiving ASM_DD payloads'() { setup: + AppSecSystem.active = true final key = new ParsedConfigKey('Test', '1234', 1, 'ASM_DD', 'ID') final service = new AppSecConfigServiceImpl(config, poller, reconf) config.getAppSecActivation() >> ProductActivation.ENABLED_INACTIVE @@ -731,12 +732,14 @@ class AppSecConfigServiceImplSpecification extends DDSpecification { when: listeners.savedWafDataChangesListener.accept(key, '''{"rules_override": [{"rules_target": [{"rule_id": "foo"}], "enabled": false}]}'''.getBytes(), NOOP) + listeners.savedWafDataChangesListener.commit(NOOP) then: service.usedDDWafConfigKeys.toList() == [key.toString()] when: listeners.savedWafDataChangesListener.remove(key, NOOP) + listeners.savedWafDataChangesListener.commit(NOOP) then: service.usedDDWafConfigKeys.empty @@ -776,6 +779,118 @@ class AppSecConfigServiceImplSpecification extends DDSpecification { noExceptionThrown() } + void 'deferred commit: operations are tracked and executed via commit'() { + setup: + AppSecSystem.active = true + final key = new ParsedConfigKey('Test', '1234', 1, 'ASM_DD', 'ID') + final service = new AppSecConfigServiceImpl(config, poller, reconf) + config.getAppSecActivation() >> ProductActivation.ENABLED_INACTIVE + + when: + service.maybeSubscribeConfigPolling() + + then: + 1 * poller.addListener(Product.ASM_DD, _) >> { + listeners.savedWafDataChangesListener = it[1] + } + 1 * poller.addListener(Product.ASM_FEATURES, _, _) >> { + listeners.savedFeaturesDeserializer = it[1] + listeners.savedFeaturesListener = it[2] + } + + when: 'activate AppSec' + listeners.savedFeaturesListener.accept('asm_features conf', + listeners.savedFeaturesDeserializer.deserialize('{"asm":{"enabled": true}}'.bytes), + NOOP) + + then: + service.usedDDWafConfigKeys.empty + + when: 'accept and commit' + listeners.savedWafDataChangesListener.accept(key, '''{"rules_override": [{"rules_target": [{"rule_id": "foo"}], "enabled": false}]}'''.getBytes(), NOOP) + listeners.savedWafDataChangesListener.commit(NOOP) + + then: + service.usedDDWafConfigKeys.toList() == [key.toString()] + + when: 'remove and commit' + listeners.savedWafDataChangesListener.remove(key, NOOP) + listeners.savedWafDataChangesListener.commit(NOOP) + + then: + service.usedDDWafConfigKeys.empty + } + + void 'deferred commit: accept after remove cancels the remove'() { + setup: + AppSecSystem.active = true + final key = new ParsedConfigKey('Test', '1234', 1, 'ASM_DD', 'ID') + final service = new AppSecConfigServiceImpl(config, poller, reconf) + config.getAppSecActivation() >> ProductActivation.ENABLED_INACTIVE + + when: + service.maybeSubscribeConfigPolling() + + then: + 1 * poller.addListener(Product.ASM_DD, _) >> { + listeners.savedWafDataChangesListener = it[1] + } + 1 * poller.addListener(Product.ASM_FEATURES, _, _) >> { + listeners.savedFeaturesDeserializer = it[1] + listeners.savedFeaturesListener = it[2] + } + + when: 'activate AppSec and add config' + listeners.savedFeaturesListener.accept('asm_features conf', + listeners.savedFeaturesDeserializer.deserialize('{"asm":{"enabled": true}}'.bytes), + NOOP) + listeners.savedWafDataChangesListener.accept(key, '''{"rules_override": [{"rules_target": [{"rule_id": "foo"}], "enabled": false}]}'''.getBytes(), NOOP) + listeners.savedWafDataChangesListener.commit(NOOP) + + then: + service.usedDDWafConfigKeys.toList() == [key.toString()] + + when: 'remove then accept (cancel remove), then commit' + listeners.savedWafDataChangesListener.remove(key, NOOP) + listeners.savedWafDataChangesListener.accept(key, '''{"rules_override": [{"rules_target": [{"rule_id": "bar"}], "enabled": false}]}'''.getBytes(), NOOP) + listeners.savedWafDataChangesListener.commit(NOOP) + + then: 'config updated not removed' + service.usedDDWafConfigKeys.toList() == [key.toString()] + } + + void 'deferred commit: remove after accept cancels the accept'() { + setup: + AppSecSystem.active = true + final key = new ParsedConfigKey('Test', '1234', 1, 'ASM_DD', 'ID') + final service = new AppSecConfigServiceImpl(config, poller, reconf) + config.getAppSecActivation() >> ProductActivation.ENABLED_INACTIVE + + when: + service.maybeSubscribeConfigPolling() + + then: + 1 * poller.addListener(Product.ASM_DD, _) >> { + listeners.savedWafDataChangesListener = it[1] + } + 1 * poller.addListener(Product.ASM_FEATURES, _, _) >> { + listeners.savedFeaturesDeserializer = it[1] + listeners.savedFeaturesListener = it[2] + } + + when: 'activate AppSec' + listeners.savedFeaturesListener.accept('asm_features conf', + listeners.savedFeaturesDeserializer.deserialize('{"asm":{"enabled": true}}'.bytes), + NOOP) + + and: 'accept then remove (cancel accept), then commit' + listeners.savedWafDataChangesListener.accept(key, '''{"rules_override": [{"rules_target": [{"rule_id": "foo"}], "enabled": false}]}'''.getBytes(), NOOP) + listeners.savedWafDataChangesListener.remove(key, NOOP) + listeners.savedWafDataChangesListener.commit(NOOP) + + then: 'config removed (never applied)' + service.usedDDWafConfigKeys.empty + } private static AppSecFeatures autoUserInstrum(String mode) { return new AppSecFeatures().tap { features -> From 152fed531827fc135aeac221aec55fba5f6d44f7 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Wed, 14 Jan 2026 13:51:06 +0100 Subject: [PATCH 3/8] WIP --- ...RemoteConfigDuplicateRulesSmokeTest.groovy | 292 ------------------ 1 file changed, 292 deletions(-) delete mode 100644 dd-smoke-tests/appsec/springboot/src/test/groovy/datadog/smoketest/appsec/RemoteConfigDuplicateRulesSmokeTest.groovy diff --git a/dd-smoke-tests/appsec/springboot/src/test/groovy/datadog/smoketest/appsec/RemoteConfigDuplicateRulesSmokeTest.groovy b/dd-smoke-tests/appsec/springboot/src/test/groovy/datadog/smoketest/appsec/RemoteConfigDuplicateRulesSmokeTest.groovy deleted file mode 100644 index 5ac4bb2b156..00000000000 --- a/dd-smoke-tests/appsec/springboot/src/test/groovy/datadog/smoketest/appsec/RemoteConfigDuplicateRulesSmokeTest.groovy +++ /dev/null @@ -1,292 +0,0 @@ -package datadog.smoketest.appsec - -import datadog.trace.agent.test.utils.OkHttpUtils -import okhttp3.Request -import spock.lang.Shared - -import java.util.concurrent.TimeUnit - -/** - * Smoke test that simulates the real production scenario where: - * 1. default_config.json is loaded with base rules - * 2. Remote Config sends an "update" with: - * - Rules that ALREADY EXIST (should REPLACE, not duplicate) - * - New rules that weren't in default - * 3. libddwaf detects DUPLICATES and generates warnings - * - * In production we saw: - * - 159 rules in default_config.json - * - 172 duplicate warnings - * - 158 rules duplicated (existed in both) - * - 14 new rules (only in Remote Config) - */ -class RemoteConfigDuplicateRulesSmokeTest extends AbstractAppSecServerSmokeTest { - - @Override - def logLevel() { - 'DEBUG' - } - - @Shared - String buildDir = new File(System.getProperty("datadog.smoketest.builddir")).absolutePath - - @Shared - String customRulesPath = "${buildDir}/appsec_remoteconfig_base_rules.json" - - @Shared - List capturedLogs = [] - - @Override - void beforeProcessBuilders() { - // Set Remote Config BEFORE creating the process - prepareRemoteConfigWithDuplicates() - super.beforeProcessBuilders() - } - - def prepareRemoteConfigWithDuplicates() { - // This simulates Remote Config sending updates with same rule IDs - def remoteConfigRules = [ - // DUPLICATED RULES - Same IDs as in base config - // In production there were 158 duplicated rules - [ - id: 'blk-001-001', // DUPLICATED - name: 'Block IP Addresses - Updated via Remote Config', - tags: [ - type: 'block_ip', - category: 'security_scanner', - updated: 'true' // Marker to identify it came from RC - ], - conditions: [ - [ - parameters: [ - inputs: [[address: 'http.client_ip']], - data: 'blocked_ips' - ], - operator: 'ip_match' - ] - ], - transformers: ['removeNulls'], - on_match: ['block'] - ], - [ - id: 'blk-001-002', // DUPLICATED - name: 'Block User Addresses - Updated', - tags: [type: 'block_user', category: 'security_scanner'], - conditions: [ - [ - parameters: [ - inputs: [[address: 'usr.id']], - data: 'blocked_users' - ], - operator: 'exact_match' - ] - ], - transformers: [], - on_match: ['block'] - ], - [ - id: 'crs-913-110', // DUPLICATED - name: 'Acunetix - Updated', - tags: [type: 'security_scanner', category: 'attack_attempt'], - conditions: [ - [ - parameters: [ - inputs: [[address: 'server.request.headers.no_cookies']], - key_path: ['user-agent'], - regex: 'Acunetix' - ], - operator: 'match_regex' - ] - ], - transformers: ['lowercase'], - on_match: [] - ], - [ - id: 'crs-921-110', // DUPLICATED - name: 'HTTP Request Smuggling Attack - Updated', - tags: [type: 'http_protocol_violation', category: 'attack_attempt'], - conditions: [ - [ - parameters: [ - inputs: [[address: 'server.request.headers.no_cookies']], - key_path: ['content-length'], - regex: '.*,.*' - ], - operator: 'match_regex' - ] - ], - transformers: [], - on_match: [] - ], - [ - id: 'crs-942-100', // DUPLICATED - name: 'SQL Injection Attack - Updated', - tags: [type: 'sql_injection', category: 'attack_attempt'], - conditions: [ - [ - parameters: [ - inputs: [[address: 'server.request.query']] - ], - operator: 'is_sqli' - ] - ], - transformers: [], - on_match: [] - ], - // NEW RULES - Simulate the 14 rules that only exist in Remote Config - [ - id: 'api-001-100', // NEW - name: 'API Security Rule 100', - tags: [type: 'api_security', category: 'attack_attempt'], - conditions: [ - [ - parameters: [ - inputs: [[address: 'server.request.uri.raw']], - regex: '/api/.*' - ], - operator: 'match_regex' - ] - ], - transformers: [], - on_match: [] - ], - [ - id: 'api-001-110', // NEW - name: 'API Security Rule 110', - tags: [type: 'api_security', category: 'attack_attempt'], - conditions: [ - [ - parameters: [ - inputs: [[address: 'server.request.method']], - list: ['PUT', 'DELETE'] - ], - operator: 'match' - ] - ], - transformers: [], - on_match: [] - ] - ] - - def remoteConfigProcessors = [ - // DUPLICATED PROCESSOR - [ - id: "http-network-fingerprint", // DUPLICATED - generator: "http_network_fingerprint", - conditions: [ - [ - operator: "exists", - parameters: [ - inputs: [[address: "waf.context.event"]] - ] - ] - ], - parameters: [ - mappings: [ - [ - headers: [[address: "server.request.headers.no_cookies"]], - output: "_dd.appsec.fp.http.network.updated" // Different output - ] - ] - ], - evaluate: false, - output: true - ] - ] - - def remoteConfigData = [ - version: "2.2", - metadata: [ - rules_version: "1.1.0-remoteconfig" - ], - rules: remoteConfigRules, - processors: remoteConfigProcessors - ] - - // Set Remote Config using the inherited infrastructure - setRemoteConfig("datadog/2/ASM_DD/config_id/config", groovy.json.JsonOutput.toJson(remoteConfigData)) - } - - def prepareBaseConfiguration() { - // DON'T provide a custom rules file - let the agent use its default bundled config - // This way Remote Config will be applied (agent refuses RC when custom rules are provided) - - // Enable Remote Config (parent class disables it by default) - defaultAppSecProperties += "-Ddd.remote_config.enabled=true" as String - defaultAppSecProperties += "-Ddd.remote_config.url=http://localhost:${server.address.port}/v0.7/config" as String - defaultAppSecProperties += "-Ddd.remote_config.poll_interval.seconds=1" as String - defaultAppSecProperties += "-Ddd.remote_config.integrity_check.enabled=false" as String - } - - @Override - ProcessBuilder createProcessBuilder() { - prepareBaseConfiguration() - - String springBootShadowJar = System.getProperty("datadog.smoketest.appsec.springboot.shadowJar.path") - - List command = new ArrayList<>() - command.add(javaPath()) - command.addAll(defaultJavaProperties) - command.addAll(defaultAppSecProperties) - command.addAll((String[]) ["-jar", springBootShadowJar, "--server.port=${httpPort}"]) - - ProcessBuilder processBuilder = new ProcessBuilder(command) - processBuilder.directory(new File(buildDirectory)) - processBuilder.redirectErrorStream(true) - - return processBuilder - } - - void 'test Remote Config applies without duplicate warnings'() { - when: 'we wait for the agent to start and apply Remote Config' - // Remote Config was already set in beforeProcessBuilders() BEFORE process started - // Now we just need to wait for agent to start and apply it - Thread.sleep(5000) // Give time for agent to initialize and apply RC - - then: 'we make a request to ensure the app is running' - def url = "http://localhost:${httpPort}/greeting" - def client = OkHttpUtils.clientBuilder() - .readTimeout(30, TimeUnit.SECONDS) - .build() - def request = new Request.Builder() - .url(url) - .get() - .build() - def response = client.newCall(request).execute() - - response.code() == 200 - - and: 'Remote Config is applied successfully WITHOUT duplicate warnings' - Thread.sleep(2000) // Give time for all logs to be written - - // Read logs from the log file (process output is captured to file by ProcessManager) - capturedLogs.clear() - def logFile = new File(logFilePath) - if (logFile.exists()) { - capturedLogs.addAll(logFile.readLines()) - } - - // Verify Remote Config was applied - def remoteConfigApplied = capturedLogs.find { - it.contains('AppSec loaded') && it.contains('rules from file datadog/2/ASM_DD/config_id/config') - } - - def defaultConfigRemoved = capturedLogs.find { - it.contains('Removing default config ASM_DD/default') - } - - // Check for NO duplicate warnings (proving the fix works) - def duplicateRuleWarnings = capturedLogs.findAll { - it.contains('WARN') && it.contains('ddwaf_native') && it.contains('Duplicate rule') - } - def duplicateProcessorWarnings = capturedLogs.findAll { - it.contains('WARN') && it.contains('ddwaf_native') && it.contains('Duplicate processor') - } - - // Verifications - The fix should work, so NO duplicates expected - assert defaultConfigRemoved != null, "Default config should be removed before applying Remote Config" - assert remoteConfigApplied != null, "Remote Config should be applied successfully" - assert duplicateRuleWarnings.isEmpty(), "Expected NO duplicate rule warnings (fix is working), but found: ${duplicateRuleWarnings.size()}" - assert duplicateProcessorWarnings.isEmpty(), "Expected NO duplicate processor warnings (fix is working), but found: ${duplicateProcessorWarnings.size()}" - } -} From e062604f9010e2b674712b49fd3e9269b1789047 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Thu, 15 Jan 2026 11:22:22 +0100 Subject: [PATCH 4/8] WIP --- .../com/datadog/appsec/config/AppSecConfigServiceImpl.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/config/AppSecConfigServiceImpl.java b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/config/AppSecConfigServiceImpl.java index f159f213bf6..32f36968bcc 100644 --- a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/config/AppSecConfigServiceImpl.java +++ b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/config/AppSecConfigServiceImpl.java @@ -241,7 +241,6 @@ public void accept(ConfigKey configKey, byte[] content, PollingRateHinter pollin ignoredConfigKeys.add(key); } else { ignoredConfigKeys.remove(key); - beforeApply(key, contentMap); maybeInitializeDefaultConfig(); // DEFER: Store config to apply in commit phase @@ -265,9 +264,6 @@ public void remove(ConfigKey configKey, PollingRateHinter pollingRateHinter) // Cancel any pending apply for this key (remove overrides apply) configsToApply.remove(key); - - // Notify subclass that removal is pending - afterRemove(key); } @Override @@ -288,10 +284,12 @@ public void commit(PollingRateHinter pollingRateHinter) { throw new RuntimeException(e); } } + afterRemove(key); } // Phase 2: Execute all pending applies for (Map.Entry> entry : configsToApply.entrySet()) { + beforeApply(entry.getKey(), entry.getValue()); try { handleWafUpdateResultReport(entry.getKey(), entry.getValue()); } catch (AppSecModule.AppSecModuleActivationException e) { From 397fb66a44c3db4aed084a140cbbe44247522473 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Thu, 15 Jan 2026 11:30:35 +0100 Subject: [PATCH 5/8] WIP --- .../appsec/config/AppSecConfigServiceImplSpecification.groovy | 1 - 1 file changed, 1 deletion(-) diff --git a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/config/AppSecConfigServiceImplSpecification.groovy b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/config/AppSecConfigServiceImplSpecification.groovy index 6f819c89a93..a51d59c4584 100644 --- a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/config/AppSecConfigServiceImplSpecification.groovy +++ b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/config/AppSecConfigServiceImplSpecification.groovy @@ -750,7 +750,6 @@ class AppSecConfigServiceImplSpecification extends DDSpecification { final key = new ParsedConfigKey('Test', '1234', 1, 'ASM_DD', 'ID') when: - AppSecSystem.active = true config.getAppSecActivation() >> ProductActivation.FULLY_ENABLED final service = new AppSecConfigServiceImpl(config, poller, reconf) service.init() From 9e63ed4c490236df8e862621680ec38edbd73229 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Thu, 15 Jan 2026 11:30:47 +0100 Subject: [PATCH 6/8] WIP --- .../com/datadog/appsec/ddwaf/WAFModuleSpecification.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/ddwaf/WAFModuleSpecification.groovy b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/ddwaf/WAFModuleSpecification.groovy index 61a837b1791..b9fa2ecb751 100644 --- a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/ddwaf/WAFModuleSpecification.groovy +++ b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/ddwaf/WAFModuleSpecification.groovy @@ -149,7 +149,7 @@ class WAFModuleSpecification extends DDSpecification { def json = ADAPTER.toJson(map) listener.accept(config, json.getBytes(), null) } - // Trigger commit to execute deferred operations (P2 deferred commit pattern) + // Trigger commit to execute deferred operations listener.commit(null) } From 2149edfa07b5647c6de47166b78e15c6dc39cd6d Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Thu, 15 Jan 2026 13:20:58 +0100 Subject: [PATCH 7/8] WIP --- .../config/AppSecConfigServiceImpl.java | 22 ++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/config/AppSecConfigServiceImpl.java b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/config/AppSecConfigServiceImpl.java index 32f36968bcc..f436a26361f 100644 --- a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/config/AppSecConfigServiceImpl.java +++ b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/config/AppSecConfigServiceImpl.java @@ -275,8 +275,7 @@ public void commit(PollingRateHinter pollingRateHinter) { // Phase 1: Execute all pending removes for (String key : configsToRemove) { - // Only remove if the config was actually added to the WAF - if (usedDDWafConfigKeys.contains(key)) { + if (shouldRemoveConfig(key)) { try { maybeInitializeDefaultConfig(); wafBuilder.removeConfig(key); @@ -305,6 +304,17 @@ public void commit(PollingRateHinter pollingRateHinter) { protected void beforeApply(final String key, final Map contentMap) {} protected void afterRemove(final String key) {} + + /** + * Determines whether a config should be removed from the WAF. Override this method to add + * custom logic for specific product listeners. + * + * @param key the config key to check + * @return true if the config should be removed, false otherwise + */ + protected boolean shouldRemoveConfig(final String key) { + return true; // Default: always remove + } } private class AppSecConfigChangesDDListener extends AppSecConfigChangesListener { @@ -320,6 +330,12 @@ protected void afterRemove(final String key) { usedDDWafConfigKeys.remove(key); } + @Override + protected boolean shouldRemoveConfig(final String key) { + // For ASM_DD, only remove if the config was actually added (tracked in usedDDWafConfigKeys) + return usedDDWafConfigKeys.contains(key); + } + @Override public void commit(PollingRateHinter pollingRateHinter) { // Special handling for ASM_DD: Remove default config before applying remote configs @@ -335,7 +351,7 @@ public void commit(PollingRateHinter pollingRateHinter) { } } - // Now execute the standard commit flow (removes first, then applies) + // Execute the standard deferred commit flow (removes first, then applies) super.commit(pollingRateHinter); } } From 4ccde35fead45189a4bb74daaef976d672f19417 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Thu, 15 Jan 2026 14:02:01 +0100 Subject: [PATCH 8/8] WIP --- .../appsec/config/AppSecConfigServiceImplSpecification.groovy | 1 - 1 file changed, 1 deletion(-) diff --git a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/config/AppSecConfigServiceImplSpecification.groovy b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/config/AppSecConfigServiceImplSpecification.groovy index a51d59c4584..8e3fca14784 100644 --- a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/config/AppSecConfigServiceImplSpecification.groovy +++ b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/config/AppSecConfigServiceImplSpecification.groovy @@ -705,7 +705,6 @@ class AppSecConfigServiceImplSpecification extends DDSpecification { void 'config keys are added and removed to the set when receiving ASM_DD payloads'() { setup: - AppSecSystem.active = true final key = new ParsedConfigKey('Test', '1234', 1, 'ASM_DD', 'ID') final service = new AppSecConfigServiceImpl(config, poller, reconf) config.getAppSecActivation() >> ProductActivation.ENABLED_INACTIVE