From 952ab277d52fd648bfcc098d247a8d273f98b986 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Vidal=20Dom=C3=ADnguez?= Date: Fri, 24 Jan 2025 11:47:34 +0100 Subject: [PATCH 1/2] Update metrics to have success value --- .../appsec/powerwaf/PowerWAFModule.java | 19 ++++-- .../PowerWAFModuleSpecification.groovy | 68 +++++++++++++++++-- .../api/telemetry/WafMetricCollector.java | 28 +++++--- .../telemetry/WafMetricCollectorTest.groovy | 18 ++--- 4 files changed, 104 insertions(+), 29 deletions(-) diff --git a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/powerwaf/PowerWAFModule.java b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/powerwaf/PowerWAFModule.java index 4c8e04978e0..ef20df366f1 100644 --- a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/powerwaf/PowerWAFModule.java +++ b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/powerwaf/PowerWAFModule.java @@ -245,12 +245,6 @@ private void initializeNewWafCtx( currentRulesVersion = initReport.rulesetVersion; } - if (prevContextAndAddresses == null) { - WafMetricCollector.get().wafInit(Powerwaf.LIB_VERSION, currentRulesVersion); - } else { - WafMetricCollector.get().wafUpdates(currentRulesVersion); - } - if (initReport != null) { log.info( "Created {} WAF context with rules ({} OK, {} BAD), version {}", @@ -273,11 +267,13 @@ private void initializeNewWafCtx( } } catch (InvalidRuleSetException irse) { initReport = irse.ruleSetInfo; + sendWafMetrics(prevContextAndAddresses, false); throw new AppSecModuleActivationException("Error creating WAF rules", irse); } catch (RuntimeException | AbstractPowerwafException e) { if (newPwafCtx != null) { newPwafCtx.close(); } + sendWafMetrics(prevContextAndAddresses, false); throw new AppSecModuleActivationException("Error creating WAF rules", e); } finally { if (initReport != null) { @@ -287,9 +283,12 @@ private void initializeNewWafCtx( if (!this.ctxAndAddresses.compareAndSet(prevContextAndAddresses, newContextAndAddresses)) { newPwafCtx.close(); + sendWafMetrics(prevContextAndAddresses, false); throw new AppSecModuleActivationException("Concurrent update of WAF configuration"); } + sendWafMetrics(prevContextAndAddresses, true); + if (prevContextAndAddresses != null) { prevContextAndAddresses.ctx.close(); } @@ -297,6 +296,14 @@ private void initializeNewWafCtx( reconf.reloadSubscriptions(); } + private void sendWafMetrics(CtxAndAddresses prevContextAndAddresses, boolean success) { + if (prevContextAndAddresses == null) { + WafMetricCollector.get().wafInit(Powerwaf.LIB_VERSION, currentRulesVersion, success); + } else { + WafMetricCollector.get().wafUpdates(currentRulesVersion, success); + } + } + private Map calculateEffectiveActions( CtxAndAddresses prevContextAndAddresses, AppSecConfig ruleConfig) { Map actionInfoMap; diff --git a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/powerwaf/PowerWAFModuleSpecification.groovy b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/powerwaf/PowerWAFModuleSpecification.groovy index 11e1c3ad899..a2759333328 100644 --- a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/powerwaf/PowerWAFModuleSpecification.groovy +++ b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/powerwaf/PowerWAFModuleSpecification.groovy @@ -69,7 +69,10 @@ class PowerWAFModuleSpecification extends DDSpecification { Additive pwafAdditive PowerwafMetrics metrics + WafMetricCollector wafMetricCollector = Mock(WafMetricCollector) + void setup() { + WafMetricCollector.INSTANCE = wafMetricCollector AgentTracer.forceRegister(tracer) } @@ -205,6 +208,8 @@ class PowerWAFModuleSpecification extends DDSpecification { ctx.closeAdditive() then: + 1 * wafMetricCollector.wafInit(Powerwaf.LIB_VERSION, _, true) + 1 * wafMetricCollector.wafUpdates(_, true) 1 * reconf.reloadSubscriptions() 1 * flow.setAction({ Flow.Action.RequestBlockingAction rba -> rba.statusCode == 501 && @@ -240,6 +245,7 @@ class PowerWAFModuleSpecification extends DDSpecification { ctx.closeAdditive() then: + 1 * wafMetricCollector.wafInit(Powerwaf.LIB_VERSION, _, true) 1 * flow.setAction({ Flow.Action.RequestBlockingAction rba -> rba.statusCode == 403 && rba.blockingContentType == BlockingContentType.AUTO @@ -281,6 +287,7 @@ class PowerWAFModuleSpecification extends DDSpecification { ctx.closeAdditive() then: + 1 * wafMetricCollector.wafUpdates(_, true) 1 * reconf.reloadSubscriptions() 1 * flow.setAction({ Flow.Action.RequestBlockingAction rba -> rba.statusCode == 403 && @@ -363,6 +370,7 @@ class PowerWAFModuleSpecification extends DDSpecification { ctx.closeAdditive() then: + 1 * wafMetricCollector.wafUpdates(_, true) 1 * reconf.reloadSubscriptions() 1 * flow.setAction({ Flow.Action.RequestBlockingAction rba -> rba.statusCode == 403 && @@ -439,7 +447,10 @@ class PowerWAFModuleSpecification extends DDSpecification { } then: + 1 * wafMetricCollector.wafInit(Powerwaf.LIB_VERSION, _, true) + 1 * wafMetricCollector.wafUpdates(_, true) 1 * reconf.reloadSubscriptions() + 0 * _ when: dataListener.onDataAvailable(flow, ctx, ATTACK_BUNDLE, gwCtx) @@ -521,7 +532,10 @@ class PowerWAFModuleSpecification extends DDSpecification { } then: + 1 * wafMetricCollector.wafInit(Powerwaf.LIB_VERSION, _, true) + 1 * wafMetricCollector.wafUpdates(_, true) 1 * reconf.reloadSubscriptions() + 0 * _ when: dataListener.onDataAvailable(flow, ctx, ATTACK_BUNDLE, gwCtx) @@ -596,7 +610,10 @@ class PowerWAFModuleSpecification extends DDSpecification { } then: + 1 * wafMetricCollector.wafInit(Powerwaf.LIB_VERSION, _, true) + 2 * wafMetricCollector.wafUpdates(_, true) 2 * reconf.reloadSubscriptions() + 0 * _ when: dataListener.onDataAvailable(flow, ctx, ATTACK_BUNDLE, gwCtx) @@ -971,9 +988,6 @@ class PowerWAFModuleSpecification extends DDSpecification { TraceSegment segment = Mock() TraceSegmentPostProcessor pp = service.traceSegmentPostProcessors.last() - def mockWafMetricCollector = Mock(WafMetricCollector) - WafMetricCollector.INSTANCE = mockWafMetricCollector - when: dataListener.onDataAvailable(flow, ctx, db, gwCtx) @@ -982,7 +996,7 @@ class PowerWAFModuleSpecification extends DDSpecification { pwafAdditive = it[0].openAdditive() } assert !flow.blocking 1 * ctx.increaseTimeouts() - 1 * mockWafMetricCollector.get().wafRequestTimeout() + 1 * wafMetricCollector.get().wafRequestTimeout() when: pp.processTraceSegment(segment, ctx, []) @@ -1006,6 +1020,7 @@ class PowerWAFModuleSpecification extends DDSpecification { then: thrown AppSecModule.AppSecModuleActivationException + 0 * _ when: cfgService.listeners['waf'].onNewSubconfig(defaultConfig['waf'], reconf) @@ -1017,7 +1032,14 @@ class PowerWAFModuleSpecification extends DDSpecification { 1 * ctx.getOrCreateAdditive(_, true, false) >> { pwafAdditive = it[0].openAdditive() } 1 * ctx.reportEvents(_ as Collection) + 1 * ctx.isAdditiveClosed() + 1 * ctx.getWafMetrics() + 1 * ctx.isThrottled(null) + 1 * ctx.closeAdditive() + 2 * tracer.activeSpan() + 1 * wafMetricCollector.wafInit(Powerwaf.LIB_VERSION, _, true) 1 * reconf.reloadSubscriptions() + 0 * _ } void 'rule data given through configuration'() { @@ -1049,6 +1071,7 @@ class PowerWAFModuleSpecification extends DDSpecification { ctx.closeAdditive() then: + 1 * wafMetricCollector.wafUpdates(_, true) 1 * reconf.reloadSubscriptions() 1 * ctx.getOrCreateAdditive(_, true, false) >> { pwafAdditive = it[0].openAdditive() } 2 * tracer.activeSpan() @@ -1106,6 +1129,7 @@ class PowerWAFModuleSpecification extends DDSpecification { ctx.closeAdditive() then: 'no match; rule is disabled' + 1 * wafMetricCollector.wafUpdates(_, true) 1 * reconf.reloadSubscriptions() 1 * ctx.getOrCreateAdditive(_, true, false) >> { pwafAdditive = it[0].openAdditive() } @@ -1135,6 +1159,7 @@ class PowerWAFModuleSpecification extends DDSpecification { 1 * ctx.getWafMetrics() 1 * ctx.isAdditiveClosed() >> false 1 * ctx.closeAdditive() >> {pwafAdditive.close()} + 1 * wafMetricCollector.wafUpdates(_, true) 1 * reconf.reloadSubscriptions() _ * ctx.increaseTimeouts() 0 * _ @@ -1151,6 +1176,7 @@ class PowerWAFModuleSpecification extends DDSpecification { ctx.closeAdditive() then: 'now we have match' + 1 * wafMetricCollector.wafUpdates(_, true) 1 * reconf.reloadSubscriptions() 1 * ctx.getOrCreateAdditive(_, true, false) >> { pwafAdditive = it[0].openAdditive() } @@ -1179,6 +1205,7 @@ class PowerWAFModuleSpecification extends DDSpecification { ctx.closeAdditive() then: 'nothing again; we disabled the rule' + 1 * wafMetricCollector.wafUpdates(_, true) 1 * reconf.reloadSubscriptions() 1 * ctx.getOrCreateAdditive(_, true, false) >> { pwafAdditive = it[0].openAdditive() } 1 * ctx.getWafMetrics() @@ -1208,6 +1235,7 @@ class PowerWAFModuleSpecification extends DDSpecification { ctx.closeAdditive() then: + 1 * wafMetricCollector.wafUpdates(_, true) 1 * reconf.reloadSubscriptions() // no attack 1 * ctx.getOrCreateAdditive(_, true, false) >> { pwafAdditive = it[0].openAdditive() } @@ -1231,6 +1259,7 @@ class PowerWAFModuleSpecification extends DDSpecification { ctx.closeAdditive() then: + 1 * wafMetricCollector.wafUpdates(_, true) 1 * reconf.reloadSubscriptions() // no attack 1 * ctx.getOrCreateAdditive(_, true, false) >> { @@ -1255,6 +1284,7 @@ class PowerWAFModuleSpecification extends DDSpecification { ctx.closeAdditive() then: + 1 * wafMetricCollector.wafUpdates(_, true) 1 * reconf.reloadSubscriptions() // attack found 1 * ctx.getOrCreateAdditive(_, true, false) >> { @@ -1282,6 +1312,7 @@ class PowerWAFModuleSpecification extends DDSpecification { ctx.closeAdditive() then: + 1 * wafMetricCollector.wafUpdates(_, true) 1 * reconf.reloadSubscriptions() // no attack 1 * ctx.getOrCreateAdditive(_, true, false) >> { @@ -1329,7 +1360,9 @@ class PowerWAFModuleSpecification extends DDSpecification { pwafModule.config(cfgService) then: + 1 * wafMetricCollector.wafInit(Powerwaf.LIB_VERSION, _, true) !pwafModule.dataSubscriptions.first().subscribedAddresses.contains(doesNotExistAddress) + 0 * _ } void 'bad initial configuration is given results in no subscriptions'() { @@ -1342,6 +1375,7 @@ class PowerWAFModuleSpecification extends DDSpecification { then: thrown AppSecModule.AppSecModuleActivationException pwafModule.dataSubscriptions.empty + 0 * _ } void 'rule data not a config'() { @@ -1353,9 +1387,8 @@ class PowerWAFModuleSpecification extends DDSpecification { then: thrown AppSecModule.AppSecModuleActivationException - - then: pwafModule.ctxAndAddresses.get() == null + 0 * _ } void 'bad ResultWithData - empty list'() { @@ -1493,7 +1526,18 @@ class PowerWAFModuleSpecification extends DDSpecification { ctx.closeAdditive() then: + 1 * ctx.isAdditiveClosed() + 1 * ctx.getOrCreateAdditive(_ as PowerwafContext, true, false) >> { + pwafAdditive = it[0].openAdditive() + } + 1 * ctx.getWafMetrics() + 1 * ctx.isThrottled(null) + 1 * ctx.reportEvents(_ as Collection) + 1 * ctx.closeAdditive() + 2 * tracer.activeSpan() + 1 * flow.isBlocking() 0 * flow.setAction(_) + 0 * _ when: final ipData = new AppSecData(exclusion: [ @@ -1515,11 +1559,22 @@ class PowerWAFModuleSpecification extends DDSpecification { ctx.closeAdditive() then: + 1 * wafMetricCollector.wafUpdates(_, true) 1 * reconf.reloadSubscriptions() 1 * flow.setAction({ Flow.Action.RequestBlockingAction rba -> rba.statusCode == 402 && rba.blockingContentType == BlockingContentType.AUTO }) + 1 * flow.isBlocking() 1 * ctx.isAdditiveClosed() >> false + 1 * ctx.getOrCreateAdditive(_ as PowerwafContext, true, false) >> { + pwafAdditive = it[0].openAdditive() + } + 1 * ctx.getWafMetrics() + 1 * ctx.isThrottled(null) + 1 * ctx.reportEvents(_ as Collection) + 1 * ctx.closeAdditive() + 2 * tracer.activeSpan() + 0 * _ } void 'http endpoint fingerprint support'() { @@ -1607,6 +1662,7 @@ class PowerWAFModuleSpecification extends DDSpecification { then: 1 * ctx.closeAdditive() 1 * ctx.isAdditiveClosed() >> true + 1 * wafMetricCollector.wafInit(Powerwaf.LIB_VERSION, _, true) 0 * _ } diff --git a/internal-api/src/main/java/datadog/trace/api/telemetry/WafMetricCollector.java b/internal-api/src/main/java/datadog/trace/api/telemetry/WafMetricCollector.java index 824412fbf24..2567cc6c200 100644 --- a/internal-api/src/main/java/datadog/trace/api/telemetry/WafMetricCollector.java +++ b/internal-api/src/main/java/datadog/trace/api/telemetry/WafMetricCollector.java @@ -55,16 +55,17 @@ private WafMetricCollector() { */ private static String rulesVersion = ""; - public void wafInit(final String wafVersion, final String rulesVersion) { + public void wafInit(final String wafVersion, final String rulesVersion, final boolean success) { WafMetricCollector.wafVersion = wafVersion; WafMetricCollector.rulesVersion = rulesVersion; rawMetricsQueue.offer( - new WafInitRawMetric(wafInitCounter.incrementAndGet(), wafVersion, rulesVersion)); + new WafInitRawMetric(wafInitCounter.incrementAndGet(), wafVersion, rulesVersion, success)); } - public void wafUpdates(final String rulesVersion) { + public void wafUpdates(final String rulesVersion, final boolean success) { rawMetricsQueue.offer( - new WafUpdatesRawMetric(wafUpdatesCounter.incrementAndGet(), wafVersion, rulesVersion)); + new WafUpdatesRawMetric( + wafUpdatesCounter.incrementAndGet(), wafVersion, rulesVersion, success)); // Flush request metrics to get the new version. if (rulesVersion != null @@ -249,20 +250,31 @@ public WafMetric(String metricName, long counter, String... tags) { public static class WafInitRawMetric extends WafMetric { public WafInitRawMetric( - final long counter, final String wafVersion, final String rulesVersion) { + final long counter, + final String wafVersion, + final String rulesVersion, + final boolean success) { super( - "waf.init", counter, "waf_version:" + wafVersion, "event_rules_version:" + rulesVersion); + "waf.init", + counter, + "waf_version:" + wafVersion, + "event_rules_version:" + rulesVersion, + "success:" + success); } } public static class WafUpdatesRawMetric extends WafMetric { public WafUpdatesRawMetric( - final long counter, final String wafVersion, final String rulesVersion) { + final long counter, + final String wafVersion, + final String rulesVersion, + final boolean success) { super( "waf.updates", counter, "waf_version:" + wafVersion, - "event_rules_version:" + rulesVersion); + "event_rules_version:" + rulesVersion, + "success:" + success); } } diff --git a/internal-api/src/test/groovy/datadog/trace/api/telemetry/WafMetricCollectorTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/telemetry/WafMetricCollectorTest.groovy index e039b2598f8..1e1159dcfa7 100644 --- a/internal-api/src/test/groovy/datadog/trace/api/telemetry/WafMetricCollectorTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/api/telemetry/WafMetricCollectorTest.groovy @@ -18,9 +18,9 @@ class WafMetricCollectorTest extends DDSpecification { def "put-get waf/rasp metrics"() { when: - WafMetricCollector.get().wafInit('waf_ver1', 'rules.1') - WafMetricCollector.get().wafUpdates('rules.2') - WafMetricCollector.get().wafUpdates('rules.3') + WafMetricCollector.get().wafInit('waf_ver1', 'rules.1', true) + WafMetricCollector.get().wafUpdates('rules.2', true) + WafMetricCollector.get().wafUpdates('rules.3', false) WafMetricCollector.get().wafRequest() WafMetricCollector.get().wafRequest() WafMetricCollector.get().wafRequest() @@ -43,21 +43,21 @@ class WafMetricCollectorTest extends DDSpecification { initMetric.value == 1 initMetric.namespace == 'appsec' initMetric.metricName == 'waf.init' - initMetric.tags.toSet() == ['waf_version:waf_ver1', 'event_rules_version:rules.1'].toSet() + initMetric.tags.toSet() == ['waf_version:waf_ver1', 'event_rules_version:rules.1', 'success:true'].toSet() def updateMetric1 = (WafMetricCollector.WafUpdatesRawMetric)metrics[1] updateMetric1.type == 'count' updateMetric1.value == 1 updateMetric1.namespace == 'appsec' updateMetric1.metricName == 'waf.updates' - updateMetric1.tags.toSet() == ['waf_version:waf_ver1', 'event_rules_version:rules.2'].toSet() + updateMetric1.tags.toSet() == ['waf_version:waf_ver1', 'event_rules_version:rules.2', 'success:true'].toSet() def updateMetric2 = (WafMetricCollector.WafUpdatesRawMetric)metrics[2] updateMetric2.type == 'count' updateMetric2.value == 2 updateMetric2.namespace == 'appsec' updateMetric2.metricName == 'waf.updates' - updateMetric2.tags.toSet() == ['waf_version:waf_ver1', 'event_rules_version:rules.3'].toSet() + updateMetric2.tags.toSet() == ['waf_version:waf_ver1', 'event_rules_version:rules.3', 'success:false'].toSet() def requestMetric = (WafMetricCollector.WafRequestsRawMetric)metrics[3] requestMetric.namespace == 'appsec' @@ -140,7 +140,7 @@ class WafMetricCollectorTest extends DDSpecification { when: (0..limit*2).each { - collector.wafInit("foo", "bar") + collector.wafInit("foo", "bar", true) } then: @@ -149,7 +149,7 @@ class WafMetricCollectorTest extends DDSpecification { when: (0..limit*2).each { - collector.wafUpdates("bar") + collector.wafUpdates("bar", true) } then: @@ -298,7 +298,7 @@ class WafMetricCollectorTest extends DDSpecification { def "test Rasp #ruleType metrics"() { when: - WafMetricCollector.get().wafInit('waf_ver1', 'rules.1') + WafMetricCollector.get().wafInit('waf_ver1', 'rules.1', true) WafMetricCollector.get().raspRuleEval(ruleType) WafMetricCollector.get().raspRuleEval(ruleType) WafMetricCollector.get().raspRuleMatch(ruleType) From b19e7ef2ef2020f28ed6f8b2039abe1224d83df8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Vidal=20Dom=C3=ADnguez?= Date: Mon, 27 Jan 2025 10:59:48 +0100 Subject: [PATCH 2/2] Fix tests --- .../WafMetricPeriodicActionSpecification.groovy | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/telemetry/src/test/groovy/datadog/telemetry/metric/WafMetricPeriodicActionSpecification.groovy b/telemetry/src/test/groovy/datadog/telemetry/metric/WafMetricPeriodicActionSpecification.groovy index 375a09d2244..329dcb18c14 100644 --- a/telemetry/src/test/groovy/datadog/telemetry/metric/WafMetricPeriodicActionSpecification.groovy +++ b/telemetry/src/test/groovy/datadog/telemetry/metric/WafMetricPeriodicActionSpecification.groovy @@ -11,9 +11,9 @@ class WafMetricPeriodicActionSpecification extends DDSpecification { void 'push waf metrics into the telemetry service'() { setup: - WafMetricCollector.get().wafInit('0.0.0', 'rules_ver_1') - WafMetricCollector.get().wafUpdates('rules_ver_2') - WafMetricCollector.get().wafUpdates('rules_ver_3') + WafMetricCollector.get().wafInit('0.0.0', 'rules_ver_1', true) + WafMetricCollector.get().wafUpdates('rules_ver_2', true) + WafMetricCollector.get().wafUpdates('rules_ver_3', true) when: periodicAction.doIteration(telemetryService) @@ -23,26 +23,26 @@ class WafMetricPeriodicActionSpecification extends DDSpecification { metric.namespace == 'appsec' && metric.metric == 'waf.init' && metric.points[0][1] == 1 && - metric.tags == ['waf_version:0.0.0', 'event_rules_version:rules_ver_1'] + metric.tags == ['waf_version:0.0.0', 'event_rules_version:rules_ver_1', 'success:true'] } ) 1 * telemetryService.addMetric( { Metric metric -> metric.namespace == 'appsec' && metric.metric == 'waf.updates' && metric.points[0][1] == 1 && - metric.tags == ['waf_version:0.0.0', 'event_rules_version:rules_ver_2'] + metric.tags == ['waf_version:0.0.0', 'event_rules_version:rules_ver_2', 'success:true'] } ) 1 * telemetryService.addMetric( { Metric metric -> metric.namespace == 'appsec' && metric.metric == 'waf.updates' && metric.points[0][1] == 2 && - metric.tags == ['waf_version:0.0.0', 'event_rules_version:rules_ver_3'] + metric.tags == ['waf_version:0.0.0', 'event_rules_version:rules_ver_3', 'success:true'] } ) 0 * _._ } void 'push waf request metrics and push into the telemetry'() { when: - WafMetricCollector.get().wafInit('0.0.0', 'rules_ver_1') + WafMetricCollector.get().wafInit('0.0.0', 'rules_ver_1', true) WafMetricCollector.get().wafRequest() WafMetricCollector.get().wafRequestTriggered() WafMetricCollector.get().wafRequest() @@ -108,7 +108,7 @@ class WafMetricPeriodicActionSpecification extends DDSpecification { 0 * _._ when: 'waf.updates happens' - WafMetricCollector.get().wafUpdates('rules_ver_2') + WafMetricCollector.get().wafUpdates('rules_ver_2', true) WafMetricCollector.get().wafRequest() WafMetricCollector.get().wafRequestTriggered() WafMetricCollector.get().wafRequestBlocked()