-
Notifications
You must be signed in to change notification settings - Fork 277
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.
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
[AppSec] Added smoke test to validate WAF #3010
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm interested in opinions from others on the spring-boot version.
As far as a smoke test goes, at some point it would probably be good to have an app that includes spring-security to ensure there aren't interoperability issues
8ecba41
to
11c4b29
Compare
47b9aa2
to
36d05c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Only minor things.
dd-java-agent/appsec/src/main/java/com/datadog/appsec/report/ReportStrategy.java
Outdated
Show resolved
Hide resolved
class SpringBootSmokeTest extends AbstractAppSecServerSmokeTest { | ||
|
||
// Estimate for the amount of time instrumentation, plus request, plus some extra | ||
public static final int TIMEOUT_SECS = 30 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment makes no sense to me, since it's the total time it would take from the request is done to the AppSec attack has been reported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I'm sorry, blind copy-paste. Removed redundant code.
@@ -858,6 +861,10 @@ private Config(final ConfigProvider configProvider) { | |||
appSecEnabled = configProvider.getBoolean(APPSEC_ENABLED, DEFAULT_APPSEC_ENABLED); | |||
appSecRulesFile = configProvider.getString(APPSEC_RULES_FILE, null); | |||
|
|||
// Default AppSec report timeout min=5, max=60 | |||
appSecReportMaxTimeout = configProvider.getInteger(APPSEC_REPORT_TIMEOUT_SEC, 30); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment says 60, but code says 30
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Old comment - fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the comment might be old, but the tests seem to think that it should be 60 seconds since this fails subsequent event of same type should wait 1 min
607dddd
to
fc23b75
Compare
0eda189
to
c82c814
Compare
c82c814
to
2fd53d8
Compare
AppSec smoke test implemented based on SpringBoot smoke test.
Added AppSec report simulator to track if the malicious request reported correctly.
Thу test is a sequence of 200 malicious requests. Each request will be detected by AppSec and should be sent 200 events to the backend with corresponding information about the attack. To validate AppSec we are checking events reported by AppSec.
Since the event dispatch mechanism has a timeout (minimum and maximum time between dispatches), I added the
appsec.report.timeout
configuration option to force the maximum interval between dispatches. Thus, for tests, we set the value-Dappsec.report.timeout=0
, which allows perform tests fast as possible (event commitment happens almost instantly).To use the additional option, I had to modify
Config.java
, but after switching to AppSec Span Interface, I need to get rid of this option.