Pass HTTP client IP to WAF #2316

lloeki · 2022-10-13T22:41:00Z

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:

# with Datadog.configure { |c| c.appsec.ip_denylist = ['1.2.3.4'] }
curl -vv -H 'X-Forwarded-For: 1.2.3.4' -H 'Accept: text/html' http://127.0.0.1:9292/
curl -vv -H 'X-Forwarded-For: 1.2.3.4' -H 'Accept: application/json' http://127.0.0.1:9292/
curl -vv -H 'X-Forwarded-For: 1.2.3.4' -H 'Accept: text/plain' http://127.0.0.1:9292/
curl -vv -H 'X-Forwarded-For: 1.2.3.4' http://127.0.0.1:9292/

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

spec/datadog/appsec/contrib/rack/integration_test_spec.rb

lib/datadog/appsec/configuration/settings.rb

lib/datadog/appsec/response.rb

spec/datadog/appsec/contrib/sinatra/integration_test_spec.rb

lib/datadog/appsec/assets/blocked.text

Co-authored-by: Marco Costa <marco.costa@datadoghq.com>

JRuby seems to have issues with memory / GC in these cases

marcotc · 2022-11-14T20:40:46Z

Rakefile

-  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'


Did you mean to disable JRuby for Tracing tests? bundle exec appraisal rails32-mysql2 rake spec:rails

Hmm, good catch @marcotc, I think I fat-fingered that one.

I think the fat finger was in the original PR that added it? This actually seems duplicated from the non-appsec section 😱

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.

lloeki · 2022-11-15T18:23:34Z

Alright, I fixed the 3.2 crash in specs using head instead of render.

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:

runner           | XPASSED tests/appsec/waf/test_addresses.py::Test_PathParams::test_security_scanner - Expected to fail, but all is ok

Sadly I don't have those enforced yet (hence XPASSED), otherwise we would have detected the issue before.

Root cause is strange, the following is properly called but afterwards ProcessActionPatch is conspicuously missing from the TestController ancestry.

          def patch_process_action
            binding.pry
            ::ActionController::Instrumentation.prepend(ProcessActionPatch)
          end

…ation The former patch point appears to be ineffective on Rails 3.2. In addition this aligns with Tracing's ActionPack patch.

codecov-commenter · 2022-11-15T22:08:16Z

Codecov Report

Merging #2316 (ba2147c) into master (97885d7) will increase coverage by 0.02%.
The diff coverage is 98.26%.

@@            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

Impacted Files	Coverage Δ
.../datadog/appsec/contrib/sinatra/gateway/watcher.rb	`100.00% <ø> (ø)`
lib/datadog/appsec/extensions.rb	`88.05% <83.33%> (-0.47%)`	⬇️
lib/datadog/appsec/response.rb	`88.46% <88.46%> (ø)`
lib/datadog/appsec/assets.rb	`100.00% <100.00%> (+5.88%)`	⬆️
lib/datadog/appsec/configuration.rb	`100.00% <100.00%> (ø)`
lib/datadog/appsec/configuration/settings.rb	`81.39% <100.00%> (+0.44%)`	⬆️
...ib/datadog/appsec/contrib/rack/reactive/request.rb	`92.50% <100.00%> (+0.39%)`	⬆️
lib/datadog/appsec/contrib/rack/request.rb	`97.14% <100.00%> (+1.14%)`	⬆️
...dog/appsec/contrib/rack/request_body_middleware.rb	`100.00% <100.00%> (+5.55%)`	⬆️
.../datadog/appsec/contrib/rack/request_middleware.rb	`100.00% <100.00%> (+1.47%)`	⬆️
... and 17 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

github-actions bot added appsec Application Security monitoring product integrations Involves tracing integrations labels Oct 13, 2022

lloeki mentioned this pull request Oct 14, 2022

Update to libddwaf 1.5.1 #2306

Merged

lloeki added 6 commits November 9, 2022 16:24

Pass HTTP client IP to WAF

ca4d708

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.

Add basic content negotiation support for blocking

e3d11c7

Also updates blocking assets to be spec compliant.

Change expected 403 to 200 when appsec is disabled

042c2f0

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

Change reset with around to after

8e226dc

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.

Enable AppSec IP blocking test for Sinatra and Rails

0328405

Improve blocking code

bfe1f0a

- factor out response generation - support blocking for Sinatra route events

lloeki force-pushed the lloeki/pass-client-ip-to-waf branch from 4123115 to bfe1f0a Compare November 10, 2022 12:46

github-actions bot added core Involves Datadog core libraries profiling Involves Datadog profiling tracing labels Nov 10, 2022

lloeki changed the base branch from 1.5-stable to master November 10, 2022 12:48

lloeki marked this pull request as ready for review November 10, 2022 12:49

lloeki requested a review from a team November 10, 2022 12:49

lloeki commented Nov 10, 2022

View reviewed changes

spec/datadog/appsec/contrib/rack/integration_test_spec.rb Outdated Show resolved Hide resolved

lloeki commented Nov 10, 2022

View reviewed changes

lib/datadog/appsec/configuration/settings.rb Show resolved Hide resolved

github-actions bot removed tracing core Involves Datadog core libraries profiling Involves Datadog profiling labels Nov 10, 2022

lloeki added 2 commits November 10, 2022 14:59

Fix guard clause early return on Processor init

a22e6fb

Comply with linter

1f68f94

lloeki force-pushed the lloeki/pass-client-ip-to-waf branch from 78f7d41 to 1f68f94 Compare November 10, 2022 14:00

marcotc reviewed Nov 10, 2022

View reviewed changes

lib/datadog/appsec/response.rb Outdated Show resolved Hide resolved

marcotc reviewed Nov 10, 2022

View reviewed changes

spec/datadog/appsec/contrib/sinatra/integration_test_spec.rb Show resolved Hide resolved

marcotc reviewed Nov 10, 2022

View reviewed changes

lib/datadog/appsec/assets/blocked.text Show resolved Hide resolved

Split HTTP_ACCEPT only once

5f25e24

Co-authored-by: Marco Costa <marco.costa@datadoghq.com>

lloeki force-pushed the lloeki/pass-client-ip-to-waf branch from fb5d99f to 5f25e24 Compare November 14, 2022 14:57

Add comment regarding experimental configurable

ada420e

lloeki added 2 commits November 14, 2022 16:20

Factor out SQLI rule in specs

f8de0e2

Indent class-level methods

e799022

marcotc approved these changes Nov 14, 2022

View reviewed changes

lloeki added 2 commits November 14, 2022 21:00

Remove extra spec declaration line

b4c2694

Disable AppSec rails contrib tests

f7b43e9

JRuby seems to have issues with memory / GC in these cases

marcotc reviewed Nov 14, 2022

View reviewed changes

lloeki added 2 commits November 15, 2022 11:40

Fix incorrect spec target

9ec5ec3

There appears to have been a mistake here, either progressively slipping in, or an automerge failure.

Use head instead of render for AppSec Rails specs

9314c7a

Using head eschews view lookup, which failed for Rails 3 since `render plain: 'ok'` is not supported.

Patch ActionController::Metal instead of ActionController::Instrument…

ba2147c

…ation The former patch point appears to be ineffective on Rails 3.2. In addition this aligns with Tracing's ActionPack patch.

marcotc approved these changes Nov 15, 2022

View reviewed changes

lloeki merged commit 2731a04 into master Nov 15, 2022

lloeki deleted the lloeki/pass-client-ip-to-waf branch November 15, 2022 22:16

github-actions bot added this to the 1.7.0 milestone Nov 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass HTTP client IP to WAF #2316

Pass HTTP client IP to WAF #2316

lloeki commented Oct 13, 2022

marcotc Nov 14, 2022

lloeki Nov 15, 2022

ivoanjo Nov 15, 2022

lloeki Nov 15, 2022

lloeki commented Nov 15, 2022 •

edited

codecov-commenter commented Nov 15, 2022

Pass HTTP client IP to WAF #2316

Pass HTTP client IP to WAF #2316

Conversation

lloeki commented Oct 13, 2022

marcotc Nov 14, 2022

Choose a reason for hiding this comment

lloeki Nov 15, 2022

Choose a reason for hiding this comment

ivoanjo Nov 15, 2022

Choose a reason for hiding this comment

lloeki Nov 15, 2022

Choose a reason for hiding this comment

lloeki commented Nov 15, 2022 • edited

codecov-commenter commented Nov 15, 2022

Codecov Report

lloeki commented Nov 15, 2022 •

edited