Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Appsec support for Play 2.5+ #5968

Merged
merged 2 commits into from
Oct 24, 2023
Merged

Appsec support for Play 2.5+ #5968

merged 2 commits into from
Oct 24, 2023

Conversation

cataphract
Copy link
Contributor

@cataphract cataphract commented Oct 2, 2023 •
edited
Loading

Description

Add support for ASM WAF (AKA AppSec) on Play 2.5+

Additional notes

Added the option dd.http.server.decoded.resource.preserve-spaces. This defaults to true. If dd.http.server.raw.resource is set to false, setting it to false, the old behavior is restored, where whitespace in the resource name is suppressed. This shows up when the resource name is created from an endpoint with spaces, like /path%20%with%20spaces. With raw.resource set to true, in the resource name the spaces will show up as %20. With raw.resource set false, now the resource name will include /path with spaces. The old behavior, which can be restored with preserve-spaces set to false, will result in a resource name including /pathwithspaces.

APPSEC-10445 and APPSEC-10446

@cataphract cataphract requested review from a team as code owners October 2, 2023 00:15
@cataphract cataphract changed the title Appsec support for akka Appsec support for Play 2.5+ Oct 2, 2023
@cataphract cataphract force-pushed the glopes/play-appsec branch 2 times, most recently from 9d971de to 13fe51a Compare October 2, 2023 15:45
@pr-commenter
Copy link

pr-commenter bot commented Oct 2, 2023 •
edited
Loading

Benchmarks

Startup

Parameters

Baseline Candidate
commit 1.22.0-SNAPSHOT~61ab1df9ae 1.22.0-SNAPSHOT~6e523456cc
config baseline candidate
See matching parameters
Baseline Candidate
application insecure-bank insecure-bank
module Agent Agent
parent None None
variant iast iast

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 54 cases.

Startup time reports for insecure-bank 
gantt
    title insecure-bank - global startup overhead: candidate=1.22.0-SNAPSHOT~6e523456cc, baseline=1.22.0-SNAPSHOT~61ab1df9ae

    dateFormat X
    axisFormat %s
section tracing
Agent [baseline] (1.012 s) : 0, 1012317
Total [baseline] (8.677 s) : 0, 8677442
Agent [candidate] (1.019 s) : 0, 1018517
Total [candidate] (8.694 s) : 0, 8694051
section iast
Agent [baseline] (1.138 s) : 0, 1137876
Total [baseline] (9.209 s) : 0, 9209394
Agent [candidate] (1.144 s) : 0, 1144266
Total [candidate] (9.216 s) : 0, 9216442
section iast_TELEMETRY_OFF
Agent [baseline] (1.143 s) : 0, 1142544
Total [baseline] (9.187 s) : 0, 9187222
Agent [candidate] (1.144 s) : 0, 1144283
Total [candidate] (9.229 s) : 0, 9229341
Loading
  • baseline results
Module Variant Duration Δ tracing
Agent tracing 1.012 s -
Agent iast 1.138 s 125.559 ms (12.4%)
Agent iast_TELEMETRY_OFF 1.143 s 130.228 ms (12.9%)
Total tracing 8.677 s -
Total iast 9.209 s 531.952 ms (6.1%)
Total iast_TELEMETRY_OFF 9.187 s 509.78 ms (5.9%)
  • candidate results
Module Variant Duration Δ tracing
Agent tracing 1.019 s -
Agent iast 1.144 s 125.749 ms (12.3%)
Agent iast_TELEMETRY_OFF 1.144 s 125.765 ms (12.3%)
Total tracing 8.694 s -
Total iast 9.216 s 522.391 ms (6.0%)
Total iast_TELEMETRY_OFF 9.229 s 535.29 ms (6.2%) 
gantt
    title insecure-bank - break down per module: candidate=1.22.0-SNAPSHOT~6e523456cc, baseline=1.22.0-SNAPSHOT~61ab1df9ae

    dateFormat X
    axisFormat %s
section tracing
BytebuddyAgent [baseline] (631.044 ms) : 0, 631044
BytebuddyAgent [candidate] (636.58 ms) : 0, 636580
GlobalTracer [baseline] (291.467 ms) : 0, 291467
GlobalTracer [candidate] (291.768 ms) : 0, 291768
AppSec [baseline] (48.918 ms) : 0, 48918
AppSec [candidate] (49.119 ms) : 0, 49119
Remote Config [baseline] (662.994 µs) : 0, 663
Remote Config [candidate] (676.402 µs) : 0, 676
Telemetry [baseline] (5.973 ms) : 0, 5973
Telemetry [candidate] (6.044 ms) : 0, 6044
section iast
BytebuddyAgent [baseline] (758.915 ms) : 0, 758915
BytebuddyAgent [candidate] (765.407 ms) : 0, 765407
GlobalTracer [baseline] (271.46 ms) : 0, 271460
GlobalTracer [candidate] (272.523 ms) : 0, 272523
AppSec [baseline] (46.185 ms) : 0, 46185
AppSec [candidate] (46.056 ms) : 0, 46056
IAST [baseline] (17.626 ms) : 0, 17626
IAST [candidate] (17.26 ms) : 0, 17260
Remote Config [baseline] (566.74 µs) : 0, 567
Remote Config [candidate] (586.703 µs) : 0, 587
Telemetry [baseline] (8.882 ms) : 0, 8882
Telemetry [candidate] (8.209 ms) : 0, 8209
section iast_TELEMETRY_OFF
BytebuddyAgent [baseline] (760.082 ms) : 0, 760082
BytebuddyAgent [candidate] (764.058 ms) : 0, 764058
GlobalTracer [baseline] (274.385 ms) : 0, 274385
GlobalTracer [candidate] (273.351 ms) : 0, 273351
AppSec [baseline] (46.462 ms) : 0, 46462
AppSec [candidate] (46.513 ms) : 0, 46513
IAST [baseline] (18.749 ms) : 0, 18749
IAST [candidate] (17.079 ms) : 0, 17079
Remote Config [baseline] (606.717 µs) : 0, 607
Remote Config [candidate] (561.302 µs) : 0, 561
Telemetry [baseline] (7.692 ms) : 0, 7692
Telemetry [candidate] (8.197 ms) : 0, 8197
Loading
Startup time reports for petclinic 
gantt
    title petclinic - global startup overhead: candidate=1.22.0-SNAPSHOT~6e523456cc, baseline=1.22.0-SNAPSHOT~61ab1df9ae

    dateFormat X
    axisFormat %s
section tracing
Agent [baseline] (1.012 s) : 0, 1012109
Total [baseline] (9.188 s) : 0, 9188406
Agent [candidate] (1.026 s) : 0, 1025988
Total [candidate] (9.223 s) : 0, 9222898
section appsec
Agent [baseline] (1.108 s) : 0, 1108424
Total [baseline] (9.302 s) : 0, 9301860
Agent [candidate] (1.108 s) : 0, 1108001
Total [candidate] (9.276 s) : 0, 9276030
section iast
Agent [baseline] (1.142 s) : 0, 1141548
Total [baseline] (9.464 s) : 0, 9464381
Agent [candidate] (1.144 s) : 0, 1144049
Total [candidate] (9.43 s) : 0, 9430442
section profiling
Agent [baseline] (1.201 s) : 0, 1201169
Total [baseline] (9.448 s) : 0, 9447809
Agent [candidate] (1.207 s) : 0, 1206971
Total [candidate] (9.512 s) : 0, 9512101
Loading
  • baseline results
Module Variant Duration Δ tracing
Agent tracing 1.012 s -
Agent appsec 1.108 s 96.315 ms (9.5%)
Agent iast 1.142 s 129.438 ms (12.8%)
Agent profiling 1.201 s 189.06 ms (18.7%)
Total tracing 9.188 s -
Total appsec 9.302 s 113.454 ms (1.2%)
Total iast 9.464 s 275.975 ms (3.0%)
Total profiling 9.448 s 259.403 ms (2.8%)
  • candidate results
Module Variant Duration Δ tracing
Agent tracing 1.026 s -
Agent appsec 1.108 s 82.013 ms (8.0%)
Agent iast 1.144 s 118.062 ms (11.5%)
Agent profiling 1.207 s 180.983 ms (17.6%)
Total tracing 9.223 s -
Total appsec 9.276 s 53.132 ms (0.6%)
Total iast 9.43 s 207.544 ms (2.3%)
Total profiling 9.512 s 289.204 ms (3.1%) 
gantt
    title petclinic - break down per module: candidate=1.22.0-SNAPSHOT~6e523456cc, baseline=1.22.0-SNAPSHOT~61ab1df9ae

    dateFormat X
    axisFormat %s
section tracing
BytebuddyAgent [baseline] (630.297 ms) : 0, 630297
BytebuddyAgent [candidate] (640.905 ms) : 0, 640905
GlobalTracer [baseline] (291.746 ms) : 0, 291746
GlobalTracer [candidate] (294.521 ms) : 0, 294521
AppSec [baseline] (49.094 ms) : 0, 49094
AppSec [candidate] (49.303 ms) : 0, 49303
Remote Config [baseline] (661.473 µs) : 0, 661
Remote Config [candidate] (693.36 µs) : 0, 693
Telemetry [baseline] (6.094 ms) : 0, 6094
Telemetry [candidate] (6.083 ms) : 0, 6083
section appsec
BytebuddyAgent [baseline] (636.685 ms) : 0, 636685
BytebuddyAgent [candidate] (637.726 ms) : 0, 637726
GlobalTracer [baseline] (293.252 ms) : 0, 293252
GlobalTracer [candidate] (291.889 ms) : 0, 291889
AppSec [baseline] (137.493 ms) : 0, 137493
AppSec [candidate] (137.663 ms) : 0, 137663
Remote Config [baseline] (643.87 µs) : 0, 644
Remote Config [candidate] (638.076 µs) : 0, 638
Telemetry [baseline] (5.747 ms) : 0, 5747
Telemetry [candidate] (5.688 ms) : 0, 5688
section iast
BytebuddyAgent [baseline] (761.561 ms) : 0, 761561
BytebuddyAgent [candidate] (765.161 ms) : 0, 765161
GlobalTracer [baseline] (272.297 ms) : 0, 272297
GlobalTracer [candidate] (271.639 ms) : 0, 271639
AppSec [baseline] (46.291 ms) : 0, 46291
AppSec [candidate] (47.538 ms) : 0, 47538
IAST [baseline] (18.232 ms) : 0, 18232
IAST [candidate] (17.204 ms) : 0, 17204
Remote Config [baseline] (586.07 µs) : 0, 586
Remote Config [candidate] (584.924 µs) : 0, 585
Telemetry [baseline] (8.271 ms) : 0, 8271
Telemetry [candidate] (7.525 ms) : 0, 7525
section profiling
BytebuddyAgent [baseline] (651.079 ms) : 0, 651079
BytebuddyAgent [candidate] (656.607 ms) : 0, 656607
GlobalTracer [baseline] (358.14 ms) : 0, 358140
GlobalTracer [candidate] (358.834 ms) : 0, 358834
AppSec [baseline] (49.537 ms) : 0, 49537
AppSec [candidate] (49.348 ms) : 0, 49348
Remote Config [baseline] (664.943 µs) : 0, 665
Remote Config [candidate] (656.362 µs) : 0, 656
Telemetry [baseline] (6.185 ms) : 0, 6185
Telemetry [candidate] (6.112 ms) : 0, 6112
ProfilingAgent [baseline] (81.615 ms) : 0, 81615
ProfilingAgent [candidate] (81.5 ms) : 0, 81500
Profiling [baseline] (81.639 ms) : 0, 81639
Profiling [candidate] (81.527 ms) : 0, 81527
Loading

Load

Parameters

Baseline Candidate
commit 1.22.0-SNAPSHOT~61ab1df9ae 1.22.0-SNAPSHOT~6e523456cc
config baseline candidate
end_time 2023-10-24T14:24:20 2023-10-24T14:40:34
start_time 2023-10-24T14:24:07 2023-10-24T14:40:21
See matching parameters
Baseline Candidate
application insecure-bank insecure-bank
variant iast iast

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 22 cases.

Request duration reports for insecure-bank 
gantt
    title insecure-bank - request duration [CI 0.99] : candidate=1.22.0-SNAPSHOT~6e523456cc, baseline=1.22.0-SNAPSHOT~61ab1df9ae
    dateFormat X
    axisFormat %s
section baseline
no_agent (361.905 µs) : 340, 384
.   : milestone, 362,
iast (464.422 µs) : 443, 486
.   : milestone, 464,
iast_FULL (513.289 µs) : 493, 534
.   : milestone, 513,
iast_INACTIVE (435.086 µs) : 414, 456
.   : milestone, 435,
iast_TELEMETRY_OFF (458.595 µs) : 438, 479
.   : milestone, 459,
tracing (422.594 µs) : 402, 444
.   : milestone, 423,
section candidate
no_agent (357.933 µs) : 337, 378
.   : milestone, 358,
iast (451.851 µs) : 431, 473
.   : milestone, 452,
iast_FULL (514.741 µs) : 494, 535
.   : milestone, 515,
iast_INACTIVE (423.332 µs) : 403, 444
.   : milestone, 423,
iast_TELEMETRY_OFF (455.554 µs) : 434, 477
.   : milestone, 456,
tracing (422.534 µs) : 402, 443
.   : milestone, 423,
Loading
  • baseline results
Variant Request duration [CI 0.99] Δ no_agent
no_agent 361.905 µs [339.79 µs, 384.02 µs] -
iast 464.422 µs [443.263 µs, 485.582 µs] 102.517 µs (28.3%)
iast_FULL 513.289 µs [492.826 µs, 533.751 µs] 151.384 µs (41.8%)
iast_INACTIVE 435.086 µs [413.771 µs, 456.401 µs] 73.181 µs (20.2%)
iast_TELEMETRY_OFF 458.595 µs [437.769 µs, 479.42 µs] 96.69 µs (26.7%)
tracing 422.594 µs [401.683 µs, 443.505 µs] 60.689 µs (16.8%)
  • candidate results
Variant Request duration [CI 0.99] Δ no_agent
no_agent 357.933 µs [337.403 µs, 378.463 µs] -
iast 451.851 µs [431.157 µs, 472.545 µs] 93.918 µs (26.2%)
iast_FULL 514.741 µs [494.207 µs, 535.276 µs] 156.808 µs (43.8%)
iast_INACTIVE 423.332 µs [402.76 µs, 443.904 µs] 65.398 µs (18.3%)
iast_TELEMETRY_OFF 455.554 µs [433.85 µs, 477.257 µs] 97.62 µs (27.3%)
tracing 422.534 µs [401.692 µs, 443.376 µs] 64.601 µs (18.0%)
Request duration reports for petclinic 
gantt
    title petclinic - request duration [CI 0.99] : candidate=1.22.0-SNAPSHOT~6e523456cc, baseline=1.22.0-SNAPSHOT~61ab1df9ae
    dateFormat X
    axisFormat %s
section baseline
no_agent (1.34 ms) : 1321, 1359
.   : milestone, 1340,
appsec (1.683 ms) : 1657, 1708
.   : milestone, 1683,
iast (1.442 ms) : 1418, 1466
.   : milestone, 1442,
profiling (1.469 ms) : 1445, 1494
.   : milestone, 1469,
tracing (1.458 ms) : 1434, 1482
.   : milestone, 1458,
section candidate
no_agent (1.315 ms) : 1296, 1334
.   : milestone, 1315,
appsec (1.668 ms) : 1643, 1693
.   : milestone, 1668,
iast (1.444 ms) : 1420, 1468
.   : milestone, 1444,
profiling (1.436 ms) : 1411, 1461
.   : milestone, 1436,
tracing (1.431 ms) : 1406, 1455
.   : milestone, 1431,
Loading
  • baseline results
Variant Request duration [CI 0.99] Δ no_agent
no_agent 1.34 ms [1.321 ms, 1.359 ms] -
appsec 1.683 ms [1.657 ms, 1.708 ms] 342.711 µs (25.6%)
iast 1.442 ms [1.418 ms, 1.466 ms] 102.16 µs (7.6%)
profiling 1.469 ms [1.445 ms, 1.494 ms] 129.476 µs (9.7%)
tracing 1.458 ms [1.434 ms, 1.482 ms] 118.351 µs (8.8%)
  • candidate results
Variant Request duration [CI 0.99] Δ no_agent
no_agent 1.315 ms [1.296 ms, 1.334 ms] -
appsec 1.668 ms [1.643 ms, 1.693 ms] 352.789 µs (26.8%)
iast 1.444 ms [1.42 ms, 1.468 ms] 128.627 µs (9.8%)
profiling 1.436 ms [1.411 ms, 1.461 ms] 120.998 µs (9.2%)
tracing 1.431 ms [1.406 ms, 1.455 ms] 115.58 µs (8.8%)

An error occurred while trying to automatically change base from glopes/akka-appsec to master October 2, 2023 20:59
An error occurred while trying to automatically change base from glopes/akka-appsec to master October 2, 2023 20:59
@cataphract cataphract changed the base branch from glopes/akka-appsec to master October 2, 2023 21:00
@smola smola added the comp: asm waf Application Security Management (WAF) label Oct 4, 2023
bantonsson
bantonsson reviewed Oct 6, 2023
View reviewed changes
...test/groovy/datadog/trace/bootstrap/instrumentation/decorator/HttpServerDecoratorTest.groovy
@@ -145,9 +145,9 @@ class HttpServerDecoratorTest extends ServerDecoratorTest {

where:
rawQuery | rawResource | url | expectedUrl | expectedQuery | expectedResource
false | false | "http://host/p%20ath?query%3F?" | "http://host/p ath" | "query??" | "/path"
false | false | "http://host/p%20ath?query%3F?" | "http://host/p ath" | "query??" | "/p ath"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change scares me. Does this imply that the resource name for some customer will change? That is not something that can be rolled out in a minor version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, potentially, yes. My thought was that this was a bug. Even if this was intentional, the result of removing the spaces is that multiple different endpoints will end up with the same resource name, if they only differ by the spaces in them (a%20b and ab will result in ab).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I understand what the changes are, and I agree that the old behavior is not what one would expect, but the fact remains that this is a change that can potentially break current customer dashboards and monitors since the resource changes.
There should at least be a feature flag to revert to the old behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a feature flag.

@jandro996
Copy link
Member

@cataphract I have added the JIRA tickets to the description, could you confirm if they are correct? thanks!

@ygree ygree removed their request for review October 13, 2023 20:17
@cataphract
Appsec support for play 2.5+
26e9761
@cataphract
Add cfg option http.server.decoded.resource.preserve-spaces
6e52345 
Disable to get back the old behavior of suppressing spaces in resource
names when using non-raw resource names.
@cataphract cataphract merged commit 85efb35 into master Oct 24, 2023
10 checks passed
@cataphract cataphract deleted the glopes/play-appsec branch October 24, 2023 14:58
@github-actions github-actions bot added this to the 1.22.0 milestone Oct 24, 2023
@smola smola added the tag: breaking change Breaking changes label Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bantonsson bantonsson bantonsson left review comments

@jandro996 jandro996 jandro996 approved these changes

Assignees
No one assigned
Labels
comp: asm waf Application Security Management (WAF) tag: breaking change Breaking changes
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.
4 participants
@cataphract @jandro996 @bantonsson @smola