Skip to content

Commit f78a48b

Browse files
jandro996jandro996
authored
Fix appsec.waf.requests telemetry metric (#8644)
Rework from scratch the appsec.waf.requests telemetry metric as the RFC diverges a lot from our previous implementation Update metric once when request ends Metric flags only need to be updated if we are dealing with WAF and not with RASP
1 parent 368851d commit f78a48b

File tree

9 files changed

+381
-358
lines changed

9 files changed

+381
-358
lines changed

‎dd-java-agent/appsec/src/main/java/com/datadog/appsec/ddwaf/WAFModule.java‎

Lines changed: 30 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,6 @@
4040
import datadog.trace.api.gateway.Flow;
4141
import datadog.trace.api.telemetry.LogCollector;
4242
import datadog.trace.api.telemetry.WafMetricCollector;
43-
import datadog.trace.api.telemetry.WafTruncatedType;
4443
import datadog.trace.api.time.SystemTimeSource;
4544
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
4645
import datadog.trace.bootstrap.instrumentation.api.AgentTracer;
@@ -440,20 +439,17 @@ public void onDataAvailable(
440439
WafMetricCollector.get().raspTimeout(gwCtx.raspRuleType);
441440
} else {
442441
reqCtx.increaseWafTimeouts();
443-
WafMetricCollector.get().wafRequestTimeout();
444442
log.debug(LogCollector.EXCLUDE_TELEMETRY, "Timeout calling the WAF", tpe);
445443
}
446444
return;
447445
} catch (UnclassifiedWafException e) {
448446
if (!reqCtx.isWafContextClosed()) {
449447
log.error("Error calling WAF", e);
450448
}
451-
// TODO this is wrong and will be fixed in another PR
452-
WafMetricCollector.get().wafRequestError();
453-
incrementErrorCodeMetric(gwCtx, e.code);
449+
incrementErrorCodeMetric(reqCtx, gwCtx, e.code);
454450
return;
455451
} catch (AbstractWafException e) {
456-
incrementErrorCodeMetric(gwCtx, e.code);
452+
incrementErrorCodeMetric(reqCtx, gwCtx, e.code);
457453
return;
458454
} finally {
459455
if (log.isDebugEnabled()) {
@@ -467,17 +463,8 @@ public void onDataAvailable(
467463
final long listMapTooLarge = wafMetrics.getTruncatedListMapTooLargeCount();
468464
final long objectTooDeep = wafMetrics.getTruncatedObjectTooDeepCount();
469465

470-
if (stringTooLong > 0) {
471-
WafMetricCollector.get()
472-
.wafInputTruncated(WafTruncatedType.STRING_TOO_LONG, stringTooLong);
473-
}
474-
if (listMapTooLarge > 0) {
475-
WafMetricCollector.get()
476-
.wafInputTruncated(WafTruncatedType.LIST_MAP_TOO_LARGE, listMapTooLarge);
477-
}
478-
if (objectTooDeep > 0) {
479-
WafMetricCollector.get()
480-
.wafInputTruncated(WafTruncatedType.OBJECT_TOO_DEEP, objectTooDeep);
466+
if (stringTooLong > 0 || listMapTooLarge > 0 || objectTooDeep > 0) {
467+
reqCtx.setWafTruncated();
481468
}
482469
}
483470
}
@@ -501,10 +488,12 @@ public void onDataAvailable(
501488
ActionInfo actionInfo = new ActionInfo(actionType, actionParams);
502489

503490
if ("block_request".equals(actionInfo.type)) {
504-
Flow.Action.RequestBlockingAction rba = createBlockRequestAction(actionInfo);
491+
Flow.Action.RequestBlockingAction rba =
492+
createBlockRequestAction(actionInfo, reqCtx, gwCtx.isRasp);
505493
flow.setAction(rba);
506494
} else if ("redirect_request".equals(actionInfo.type)) {
507-
Flow.Action.RequestBlockingAction rba = createRedirectRequestAction(actionInfo);
495+
Flow.Action.RequestBlockingAction rba =
496+
createRedirectRequestAction(actionInfo, reqCtx, gwCtx.isRasp);
508497
flow.setAction(rba);
509498
} else if ("generate_stack".equals(actionInfo.type)) {
510499
if (Config.get().isAppSecStackTraceEnabled()) {
@@ -516,7 +505,9 @@ public void onDataAvailable(
516505
}
517506
} else {
518507
log.info("Ignoring action with type {}", actionInfo.type);
519-
WafMetricCollector.get().wafRequestBlockFailure();
508+
if (!gwCtx.isRasp) {
509+
reqCtx.setWafRequestBlockFailure();
510+
}
520511
}
521512
}
522513
Collection<AppSecEvent> events = buildEvents(resultWithData);
@@ -543,12 +534,16 @@ public void onDataAvailable(
543534
reqCtx.reportEvents(events);
544535
} else {
545536
log.debug("Rate limited WAF events");
546-
WafMetricCollector.get().wafRequestRateLimited();
537+
if (!gwCtx.isRasp) {
538+
reqCtx.setWafRateLimited();
539+
}
547540
}
548541
}
549542

550543
if (flow.isBlocking()) {
551-
reqCtx.setBlocked();
544+
if (!gwCtx.isRasp) {
545+
reqCtx.setWafBlocked();
546+
}
552547
}
553548
}
554549

@@ -557,7 +552,8 @@ public void onDataAvailable(
557552
}
558553
}
559554

560-
private Flow.Action.RequestBlockingAction createBlockRequestAction(ActionInfo actionInfo) {
555+
private Flow.Action.RequestBlockingAction createBlockRequestAction(
556+
final ActionInfo actionInfo, final AppSecRequestContext reqCtx, final boolean isRasp) {
561557
try {
562558
int statusCode;
563559
Object statusCodeObj = actionInfo.parameters.get("status_code");
@@ -578,12 +574,15 @@ private Flow.Action.RequestBlockingAction createBlockRequestAction(ActionInfo ac
578574
return new Flow.Action.RequestBlockingAction(statusCode, blockingContentType);
579575
} catch (RuntimeException cce) {
580576
log.warn("Invalid blocking action data", cce);
581-
WafMetricCollector.get().wafRequestBlockFailure();
577+
if (!isRasp) {
578+
reqCtx.setWafRequestBlockFailure();
579+
}
582580
return null;
583581
}
584582
}
585583

586-
private Flow.Action.RequestBlockingAction createRedirectRequestAction(ActionInfo actionInfo) {
584+
private Flow.Action.RequestBlockingAction createRedirectRequestAction(
585+
final ActionInfo actionInfo, final AppSecRequestContext reqCtx, final boolean isRasp) {
587586
try {
588587
int statusCode;
589588
Object statusCodeObj = actionInfo.parameters.get("status_code");
@@ -604,7 +603,9 @@ private Flow.Action.RequestBlockingAction createRedirectRequestAction(ActionInfo
604603
return Flow.Action.RequestBlockingAction.forRedirect(statusCode, location);
605604
} catch (RuntimeException cce) {
606605
log.warn("Invalid blocking action data", cce);
607-
WafMetricCollector.get().wafRequestBlockFailure();
606+
if (!isRasp) {
607+
reqCtx.setWafRequestBlockFailure();
608+
}
608609
return null;
609610
}
610611
}
@@ -649,11 +650,13 @@ private Waf.ResultWithData runWafContext(
649650
}
650651
}
651652

652-
private static void incrementErrorCodeMetric(GatewayContext gwCtx, int code) {
653+
private static void incrementErrorCodeMetric(
654+
AppSecRequestContext reqCtx, GatewayContext gwCtx, int code) {
653655
if (gwCtx.isRasp) {
654656
WafMetricCollector.get().raspErrorCode(gwCtx.raspRuleType, code);
655657
} else {
656658
WafMetricCollector.get().wafErrorCode(code);
659+
reqCtx.setWafErrors();
657660
}
658661
}
659662

‎dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/AppSecRequestContext.java‎

Lines changed: 43 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,13 @@ public class AppSecRequestContext implements DataBundle, Closeable {
119119
private volatile WafMetrics wafMetrics;
120120
private volatile WafMetrics raspMetrics;
121121
private final AtomicInteger raspMetricsCounter = new AtomicInteger(0);
122-
private volatile boolean blocked;
122+
123+
private volatile boolean wafBlocked;
124+
private volatile boolean wafErrors;
125+
private volatile boolean wafTruncated;
126+
private volatile boolean wafRequestBlockFailure;
127+
private volatile boolean wafRateLimited;
128+
123129
private volatile int wafTimeouts;
124130
private volatile int raspTimeouts;
125131

@@ -174,12 +180,44 @@ public AtomicInteger getRaspMetricsCounter() {
174180
return raspMetricsCounter;
175181
}
176182

177-
public void setBlocked() {
178-
this.blocked = true;
183+
public void setWafBlocked() {
184+
this.wafBlocked = true;
185+
}
186+
187+
public boolean isWafBlocked() {
188+
return wafBlocked;
189+
}
190+
191+
public void setWafErrors() {
192+
this.wafErrors = true;
193+
}
194+
195+
public boolean hasWafErrors() {
196+
return wafErrors;
197+
}
198+
199+
public void setWafTruncated() {
200+
this.wafTruncated = true;
201+
}
202+
203+
public boolean isWafTruncated() {
204+
return wafTruncated;
205+
}
206+
207+
public void setWafRequestBlockFailure() {
208+
this.wafRequestBlockFailure = true;
209+
}
210+
211+
public boolean isWafRequestBlockFailure() {
212+
return wafRequestBlockFailure;
213+
}
214+
215+
public void setWafRateLimited() {
216+
this.wafRateLimited = true;
179217
}
180218

181-
public boolean isBlocked() {
182-
return blocked;
219+
public boolean isWafRateLimited() {
220+
return wafRateLimited;
183221
}
184222

185223
public void increaseWafTimeouts() {

‎dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -724,13 +724,16 @@ private NoopFlow onRequestEnded(RequestContext ctx_, IGSpanInfo spanInfo) {
724724
log.debug("Unable to commit, derivatives will be skipped {}", ctx.getDerivativeKeys());
725725
}
726726

727-
if (ctx.isBlocked()) {
728-
WafMetricCollector.get().wafRequestBlocked();
729-
} else if (!collectedEvents.isEmpty()) {
730-
WafMetricCollector.get().wafRequestTriggered();
731-
} else {
732-
WafMetricCollector.get().wafRequest();
733-
}
727+
WafMetricCollector.get()
728+
.wafRequest(
729+
!collectedEvents.isEmpty(), // ruleTriggered
730+
ctx.isWafBlocked(), // requestBlocked
731+
ctx.hasWafErrors(), // wafError
732+
ctx.getWafTimeouts() > 0, // wafTimeout,
733+
ctx.isWafRequestBlockFailure(), // blockFailure,
734+
ctx.isWafRateLimited(), // rateLimited,
735+
ctx.isWafTruncated() // inputTruncated
736+
);
734737
}
735738

736739
ctx.close();

‎dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/ddwaf/WAFModuleSpecification.groovy‎

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -475,7 +475,7 @@ class WAFModuleSpecification extends DDSpecification {
475475
2 * ctx.getWafMetrics()
476476
1 * ctx.isWafContextClosed() >> false
477477
1 * ctx.closeWafContext() >> { wafContext.close() }
478-
1 * ctx.setBlocked()
478+
1 * ctx.setWafBlocked()
479479
1 * ctx.isThrottled(null)
480480
0 * _
481481

@@ -560,7 +560,7 @@ class WAFModuleSpecification extends DDSpecification {
560560
2 * ctx.getWafMetrics()
561561
1 * ctx.isWafContextClosed() >> false
562562
1 * ctx.closeWafContext()
563-
1 * ctx.setBlocked()
563+
1 * ctx.setWafBlocked()
564564
1 * ctx.isThrottled(null)
565565
0 * _
566566
}
@@ -665,7 +665,7 @@ class WAFModuleSpecification extends DDSpecification {
665665
1 * ctx.isWafContextClosed() >> false
666666
1 * ctx.closeWafContext()
667667
1 * ctx.reportEvents(_)
668-
1 * ctx.setBlocked()
668+
1 * ctx.setWafBlocked()
669669
1 * ctx.isThrottled(null)
670670
0 * ctx._(*_)
671671
flow.blocking == true
@@ -731,7 +731,7 @@ class WAFModuleSpecification extends DDSpecification {
731731
1 * ctx.isWafContextClosed() >> false
732732
1 * ctx.closeWafContext()
733733
1 * ctx.reportEvents(_)
734-
1 * ctx.setBlocked()
734+
1 * ctx.setWafBlocked()
735735
1 * ctx.isThrottled(null)
736736
0 * ctx._(*_)
737737
flow.blocking == true
@@ -758,7 +758,7 @@ class WAFModuleSpecification extends DDSpecification {
758758
1 * ctx.isWafContextClosed() >> false
759759
1 * ctx.closeWafContext()
760760
1 * ctx.reportEvents(_)
761-
1 * ctx.setBlocked()
761+
1 * ctx.setWafBlocked()
762762
1 * ctx.isThrottled(null)
763763
0 * ctx._(*_)
764764
metrics == null
@@ -817,7 +817,7 @@ class WAFModuleSpecification extends DDSpecification {
817817
}
818818
2 * ctx.getWafMetrics() >> metrics
819819
1 * ctx.reportEvents(*_)
820-
1 * ctx.setBlocked()
820+
1 * ctx.setWafBlocked()
821821
1 * ctx.isThrottled(null)
822822
1 * ctx.isWafContextClosed() >> false
823823
0 * ctx._(*_)
@@ -1016,7 +1016,6 @@ class WAFModuleSpecification extends DDSpecification {
10161016
wafContext = it[0].openContext() }
10171017
2 * ctx.getWafMetrics()
10181018
1 * ctx.increaseWafTimeouts()
1019-
1 * wafMetricCollector.get().wafRequestTimeout()
10201019
0 * _
10211020
10221021
when:
@@ -1787,7 +1786,6 @@ class WAFModuleSpecification extends DDSpecification {
17871786
1 * wafMetricCollector.wafInit(Waf.LIB_VERSION, _, true)
17881787
1 * ctx.getRaspMetrics()
17891788
1 * ctx.getRaspMetricsCounter()
1790-
(0..1) * WafMetricCollector.get().wafRequestError() // TODO: remove this line when WAFModule is removed
17911789
1 * wafMetricCollector.raspErrorCode(RuleType.SQL_INJECTION, wafErrorCode.code)
17921790
0 * _
17931791

@@ -1816,8 +1814,8 @@ class WAFModuleSpecification extends DDSpecification {
18161814
1 * wafContext.run(_, _, _) >> { throw createWafException(wafErrorCode) }
18171815
1 * wafMetricCollector.wafInit(Waf.LIB_VERSION, _, true)
18181816
2 * ctx.getWafMetrics()
1819-
(0..1) * WafMetricCollector.get().wafRequestError() // TODO: remove this line when WAFModule is removed
18201817
1 * wafMetricCollector.wafErrorCode(wafErrorCode.code)
1818+
1 * ctx.setWafErrors()
18211819
0 * _
18221820

18231821
where:

‎dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/gateway/GatewayBridgeSpecification.groovy‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,12 @@ import datadog.trace.api.gateway.SubscriptionService
2020
import datadog.trace.api.http.StoredBodySupplier
2121
import datadog.trace.api.internal.TraceSegment
2222
import datadog.trace.api.telemetry.LoginEvent
23+
import datadog.trace.api.telemetry.WafMetricCollector
2324
import datadog.trace.bootstrap.instrumentation.api.AgentSpan
2425
import datadog.trace.bootstrap.instrumentation.api.URIDataAdapter
2526
import datadog.trace.bootstrap.instrumentation.api.URIDataAdapterBase
2627
import datadog.trace.test.util.DDSpecification
28+
import spock.lang.Shared
2729

2830
import java.util.function.BiConsumer
2931
import java.util.function.BiFunction
@@ -37,6 +39,9 @@ import static datadog.trace.api.telemetry.LoginEvent.SIGN_UP
3739

3840
class GatewayBridgeSpecification extends DDSpecification {
3941

42+
@Shared
43+
protected static final ORIGINAL_METRIC_COLLECTOR = WafMetricCollector.get()
44+
4045
private static final String USER_ID = 'user'
4146

4247
SubscriptionService ig = Mock()
@@ -106,12 +111,16 @@ class GatewayBridgeSpecification extends DDSpecification {
106111
BiFunction<RequestContext, String, Flow<Void>> userCB
107112
TriFunction<RequestContext, LoginEvent, String, Flow<Void>> loginEventCB
108113

114+
WafMetricCollector wafMetricCollector = Mock(WafMetricCollector)
115+
109116
void setup() {
110117
callInitAndCaptureCBs()
111118
AppSecSystem.active = true
119+
WafMetricCollector.INSTANCE = wafMetricCollector
112120
}
113121

114122
void cleanup() {
123+
WafMetricCollector.INSTANCE = ORIGINAL_METRIC_COLLECTOR
115124
bridge.stop()
116125
}
117126

@@ -169,6 +178,13 @@ class GatewayBridgeSpecification extends DDSpecification {
169178
1 * traceSegment.setTagTop('http.request.headers.accept', 'header_value')
170179
1 * traceSegment.setTagTop('http.response.headers.content-type', 'text/html; charset=UTF-8')
171180
1 * traceSegment.setTagTop('network.client.ip', '2001::1')
181+
1 * mockAppSecCtx.isWafBlocked()
182+
1 * mockAppSecCtx.hasWafErrors()
183+
1 * mockAppSecCtx.getWafTimeouts()
184+
1 * mockAppSecCtx.isWafRequestBlockFailure()
185+
1 * mockAppSecCtx.isWafRateLimited()
186+
1 * mockAppSecCtx.isWafTruncated()
187+
1 * wafMetricCollector.wafRequest(_, _, _, _, _, _, _) // call waf request metric
172188
flow.result == null
173189
flow.action == Flow.Action.Noop.INSTANCE
174190
}

‎internal-api/build.gradle‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,8 @@ excludedClassesCoverage += [
203203
'datadog.trace.api.telemetry.LogCollector.RawLogMessage',
204204
"datadog.trace.api.telemetry.MetricCollector.DistributionSeriesPoint",
205205
"datadog.trace.api.telemetry.MetricCollector",
206+
//Enum
207+
"datadog.trace.api.telemetry.WafTruncatedType",
206208
// stubs
207209
'datadog.trace.api.profiling.Timing.NoOp',
208210
'datadog.trace.api.profiling.Timer.NoOp',

0 commit comments

Comments
 (0)