-
Notifications
You must be signed in to change notification settings - Fork 375
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
Pass HTTP client IP to WAF #2316
Conversation
A configurable was added as well to ease testing. As a consequence, since the blocking process was implemented, the expected 403 response is sent upon match.
Also updates blocking assets to be spec compliant.
This is a 403 FORBIDDEN span, but in these tests it's used for the blocking case so the no-appsec case should be a 200 OK
Presumably after is enough, around was carried over from tracing specs, where it was probably added because other tests were leaking configuration. This is not the case in the appsec spec scope.
- factor out response generation - support blocking for Sinatra route events
4123115
to
bfe1f0a
Compare
78f7d41
to
1f68f94
Compare
Co-authored-by: Marco Costa <marco.costa@datadoghq.com>
fb5d99f
to
5f25e24
Compare
JRuby seems to have issues with memory / GC in these cases
Rakefile
Outdated
declare '❌ 2.1 / ✅ 2.2 / ✅ 2.3 / ✅ 2.4 / ✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ❌ 3.0 / ❌ 3.1 / ❌ 3.2 / ✅ jruby' => 'bundle exec appraisal rails5-mysql2 rake spec:appsec:rails' | ||
declare '❌ 2.1 / ❌ 2.2 / ❌ 2.3 / ❌ 2.4 / ✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ❌ 3.0 / ❌ 3.1 / ❌ 3.2 / ✅ jruby' => 'bundle exec appraisal rails6-mysql2 rake spec:appsec:rails' | ||
declare '❌ 2.1 / ❌ 2.2 / ❌ 2.3 / ❌ 2.4 / ✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ✅ 3.0 / ✅ 3.1 / ❌ 3.2 / ✅ jruby' => 'bundle exec appraisal rails61-mysql2 rake spec:appsec:rails' | ||
declare '✅ 2.1 / ✅ 2.2 / ✅ 2.3 / ❌ 2.4 / ❌ 2.5 / ❌ 2.6 / ❌ 2.7 / ❌ 3.0 / ❌ 3.1 / ❌ 3.2 / ❌ jruby' => 'bundle exec appraisal rails32-mysql2 rake spec:rails' |
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.
Did you mean to disable JRuby for Tracing tests? bundle exec appraisal rails32-mysql2 rake spec:rails
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.
Hmm, good catch @marcotc, I think I fat-fingered that one.
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 think the fat finger was in the original PR that added it? This actually seems duplicated from the non-appsec section 😱
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.
Oh, actually:
diff --git a/Rakefile b/Rakefile
index 9b854bd151..c0b07de3d3 100644
--- a/Rakefile
+++ b/Rakefile
@@ -388,7 +388,6 @@ task :ci do
# AppSec contrib specs
declare '✅ 2.1 / ✅ 2.2 / ✅ 2.3 / ✅ 2.4 / ✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ✅ 3.0 / ✅ 3.1 / ✅ 3.2 / ✅ jruby' => 'bundle exec appraisal contrib rake spec:appsec:rack'
declare '✅ 2.1 / ✅ 2.2 / ✅ 2.3 / ✅ 2.4 / ✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ✅ 3.0 / ✅ 3.1 / ✅ 3.2 / ✅ jruby' => 'bundle exec appraisal contrib rake spec:appsec:sinatra'
- declare '✅ 2.1 / ✅ 2.2 / ✅ 2.3 / ❌ 2.4 / ❌ 2.5 / ❌ 2.6 / ❌ 2.7 / ❌ 3.0 / ❌ 3.1 / ❌ 3.2 / ✅ jruby' => 'bundle exec appraisal rails4-mysql2 rake spec:rails'
# AppSec Rails specs
declare '✅ 2.1 / ✅ 2.2 / ✅ 2.3 / ❌ 2.4 / ❌ 2.5 / ❌ 2.6 / ❌ 2.7 / ❌ 3.0 / ❌ 3.1 / ❌ 3.2 / ✅ jruby' => 'bundle exec appraisal rails32-mysql2 rake spec:rails'
☝️ This diff removed an extra line that was duplicated.
diff --git a/Rakefile b/Rakefile
index c0b07de3d3..30fd9257e6 100644
--- a/Rakefile
+++ b/Rakefile
@@ -390,11 +390,11 @@ task :ci do
declare '✅ 2.1 / ✅ 2.2 / ✅ 2.3 / ✅ 2.4 / ✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ✅ 3.0 / ✅ 3.1 / ✅ 3.2 / ✅ jruby' => 'bundle exec appraisal contrib rake spec:appsec:sinatra'
# AppSec Rails specs
- declare '✅ 2.1 / ✅ 2.2 / ✅ 2.3 / ❌ 2.4 / ❌ 2.5 / ❌ 2.6 / ❌ 2.7 / ❌ 3.0 / ❌ 3.1 / ❌ 3.2 / ✅ jruby' => 'bundle exec appraisal rails32-mysql2 rake spec:rails'
- declare '✅ 2.1 / ✅ 2.2 / ✅ 2.3 / ❌ 2.4 / ❌ 2.5 / ❌ 2.6 / ❌ 2.7 / ❌ 3.0 / ❌ 3.1 / ❌ 3.2 / ✅ jruby' => 'bundle exec appraisal rails4-mysql2 rake spec:appsec:rails'
- declare '❌ 2.1 / ✅ 2.2 / ✅ 2.3 / ✅ 2.4 / ✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ❌ 3.0 / ❌ 3.1 / ❌ 3.2 / ✅ jruby' => 'bundle exec appraisal rails5-mysql2 rake spec:appsec:rails'
- declare '❌ 2.1 / ❌ 2.2 / ❌ 2.3 / ❌ 2.4 / ✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ❌ 3.0 / ❌ 3.1 / ❌ 3.2 / ✅ jruby' => 'bundle exec appraisal rails6-mysql2 rake spec:appsec:rails'
- declare '❌ 2.1 / ❌ 2.2 / ❌ 2.3 / ❌ 2.4 / ✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ✅ 3.0 / ✅ 3.1 / ❌ 3.2 / ✅ jruby' => 'bundle exec appraisal rails61-mysql2 rake spec:appsec:rails'
+ declare '✅ 2.1 / ✅ 2.2 / ✅ 2.3 / ❌ 2.4 / ❌ 2.5 / ❌ 2.6 / ❌ 2.7 / ❌ 3.0 / ❌ 3.1 / ❌ 3.2 / ❌ jruby' => 'bundle exec appraisal rails32-mysql2 rake spec:rails'
+ declare '✅ 2.1 / ✅ 2.2 / ✅ 2.3 / ❌ 2.4 / ❌ 2.5 / ❌ 2.6 / ❌ 2.7 / ❌ 3.0 / ❌ 3.1 / ❌ 3.2 / ❌ jruby' => 'bundle exec appraisal rails4-mysql2 rake spec:appsec:rails'
+ declare '❌ 2.1 / ✅ 2.2 / ✅ 2.3 / ✅ 2.4 / ✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ❌ 3.0 / ❌ 3.1 / ❌ 3.2 / ❌ jruby' => 'bundle exec appraisal rails5-mysql2 rake spec:appsec:rails'
+ declare '❌ 2.1 / ❌ 2.2 / ❌ 2.3 / ❌ 2.4 / ✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ❌ 3.0 / ❌ 3.1 / ❌ 3.2 / ❌ jruby' => 'bundle exec appraisal rails6-mysql2 rake spec:appsec:rails'
+ declare '❌ 2.1 / ❌ 2.2 / ❌ 2.3 / ❌ 2.4 / ✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ✅ 3.0 / ✅ 3.1 / ❌ 3.2 / ❌ jruby' => 'bundle exec appraisal rails61-mysql2 rake spec:appsec:rails'
end
namespace :coverage do
☝️ This diff had the corresponding line that was duplicated. Notice though that both are spec:rails
... instead of spec:appsec:rails
, which is the actual mistake!
There appears to have been a mistake here, either progressively slipping in, or an automerge failure.
Using head eschews view lookup, which failed for Rails 3 since `render plain: 'ok'` is not supported.
Alright, I fixed the 3.2 crash in specs using Last spec failures are about Rails 3.2 not instrumenting correctly for AppSec. This is detected by system tests as well, as the following result is absent from 3.2 variants:
Sadly I don't have those enforced yet (hence Root cause is strange, the following is properly called but afterwards
|
…ation The former patch point appears to be ineffective on Rails 3.2. In addition this aligns with Tracing's ActionPack patch.
Codecov Report
@@ Coverage Diff @@
## master #2316 +/- ##
==========================================
+ Coverage 98.34% 98.36% +0.02%
==========================================
Files 1102 1103 +1
Lines 58942 59149 +207
==========================================
+ Hits 57967 58183 +216
+ Misses 975 966 -9
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
What does this PR do?
Pass HTTP client IP to WAF, which in turn makes it blockable.
Motivation
IP blocking.
How to test the change?
Specs, or manually: