[APPSEC-8112] Appsec block request when user ID matches rules data #2642

GustavoCaso · 2023-02-23T12:37:43Z

What does this PR do?

This PR works on top of the previous work done to introduce the AppSec::Component and the Appsec::Monitor

The new code added does the following:

When calling Kit::Identity.set_user if appsec is enabled, we would propagate the user information to the appsec reactive engine
If the reactive engine considers that we should react based on the information provided, we are going to throw
The appsec rack middleware listens for the throw from the reactive engine if it encounters it. We would interrupt the request and return a 403

I had to add Datadog::AppSec.settings.send(:reset!) after all the integration contrib tests to avoid having leaky tests.

Motivation

Additional Notes

How to test the change?

CI should be 🟢

GustavoCaso · 2023-02-23T14:35:51Z

lib/datadog/kit/identity.rb

@@ -56,6 +56,11 @@ def self.set_user(trace, id:, email: nil, name: nil, session_id: nil, role: nil,
        others.each do |k, v|
          trace.set_tag("usr.#{k}", v) unless v.nil?
        end
+
+        if ::Datadog::AppSec.enabled?
+          user = OpenStruct.new(id: id)


@lloeki I'm creating a dummy object for now. I would work on having the set of objects we discussed on this other PR on a separate PR.

For now OpenStruct allows to move fast 💨

OpenStruct is very slow, let's replace it quickly! (on a separate PR)

Ref: Struct vs Class vs Hash vs OpenStruct

Absolutely. I'm not a fun of OpenStruct but for getting out of the way is very handy

…ky state test results

codecov-commenter · 2023-02-23T16:24:20Z

Codecov Report

Merging #2642 (3b09f87) into master (923f2a9) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2642      +/-   ##
==========================================
+ Coverage   98.05%   98.08%   +0.03%     
==========================================
  Files        1159     1160       +1     
  Lines       63607    63695      +88     
  Branches     2846     2849       +3     
==========================================
+ Hits        62369    62477     +108     
+ Misses       1238     1218      -20

Impacted Files	Coverage Δ
.../datadog/appsec/contrib/rack/request_middleware.rb	`95.94% <100.00%> (+0.11%)`	⬆️
lib/datadog/appsec/ext.rb	`100.00% <100.00%> (ø)`
lib/datadog/appsec/monitor/gateway/watcher.rb	`92.85% <100.00%> (+48.12%)`	⬆️
lib/datadog/kit/identity.rb	`100.00% <100.00%> (ø)`
...tadog/appsec/contrib/rack/integration_test_spec.rb	`99.25% <100.00%> (+0.06%)`	⬆️
...adog/appsec/contrib/rails/integration_test_spec.rb	`99.21% <100.00%> (+0.06%)`	⬆️
...og/appsec/contrib/sinatra/integration_test_spec.rb	`99.22% <100.00%> (+0.05%)`	⬆️
spec/datadog/kit/identity_spec.rb	`100.00% <100.00%> (ø)`
...ng/contrib/active_support/cache/instrumentation.rb	`86.30% <0.00%> (+0.09%)`	⬆️
lib/datadog/core/diagnostics/environment_logger.rb	`98.48% <0.00%> (+0.79%)`	⬆️
... and 2 more

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

lloeki

LGTM!

lloeki · 2023-02-23T15:37:16Z

lib/datadog/appsec/ext.rb

+module Datadog
+  module AppSec
+    module Ext
+      INTERRUPT = :datadog_appsec_interrupt


I guess you genericised the name from REQUEST_INTERRUPT to INTERRUPT so that it's context agnostic (request or otherwise)?

Exactly. Would you like to change it? I thought about BLOCK which is also context agnostic

lloeki · 2023-02-23T15:38:57Z

lib/datadog/appsec/monitor/gateway/watcher.rb

@@ -48,7 +49,7 @@ def watch_user_id(gateway = Instrumentation.gateway)
                  _result, block = Monitor::Reactive::SetUser.publish(op, user)
                end

-                next [nil, [[:block, event]]] if block
+                throw(Datadog::AppSec::Ext::INTERRUPT, [nil, [:block, event]]) if block


As mentioned in other discussions, we will have to see if this is still correct when multiple watchers are present on the same watch point.

Absolutely.

I'm eager to have a use case for multiple watchers and see how the reactive engine works

lloeki · 2023-02-23T15:41:03Z

lib/datadog/kit/identity.rb

@@ -56,6 +56,11 @@ def self.set_user(trace, id:, email: nil, name: nil, session_id: nil, role: nil,
        others.each do |k, v|
          trace.set_tag("usr.#{k}", v) unless v.nil?
        end
+
+        if ::Datadog::AppSec.enabled?
+          user = OpenStruct.new(id: id)


OpenStruct is very slow, let's replace it quickly! (on a separate PR)

Ref: Struct vs Class vs Hash vs OpenStruct

GustavoCaso mentioned this pull request Feb 23, 2023

Initial attempt to block request base on user ID #2627

Closed

github-actions bot added appsec Application Security monitoring product integrations Involves tracing integrations labels Feb 23, 2023

GustavoCaso force-pushed the appsec-block-user-when-ID-matches branch 3 times, most recently from f643f15 to c978ae2 Compare February 23, 2023 12:44

GustavoCaso changed the title ~~block request when user ID matches~~ [APPSEC-8112] Appsec block request when user ID matches rules data Feb 23, 2023

GustavoCaso commented Feb 23, 2023

View reviewed changes

GustavoCaso marked this pull request as ready for review February 23, 2023 14:42

GustavoCaso requested a review from a team February 23, 2023 14:42

GustavoCaso force-pushed the appsec-block-user-when-ID-matches branch from 5442da0 to b6f7afe Compare February 23, 2023 14:58

GustavoCaso requested a review from lloeki February 23, 2023 14:58

GustavoCaso added 2 commits February 23, 2023 16:05

block request when user ID matches

93cf714

make sure to reset AppSec.settings state after each test to avoid fla…

3b09f87

…ky state test results

GustavoCaso force-pushed the appsec-block-user-when-ID-matches branch from b6f7afe to 3b09f87 Compare February 23, 2023 15:05

lloeki approved these changes Feb 23, 2023

View reviewed changes

GustavoCaso merged commit 4ea0c70 into master Feb 24, 2023

GustavoCaso deleted the appsec-block-user-when-ID-matches branch February 24, 2023 08:18

github-actions bot added this to the 1.10.0 milestone Feb 24, 2023

TonyCTHsu mentioned this pull request Mar 6, 2023

Bump to version 1.10.0 #2667

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APPSEC-8112] Appsec block request when user ID matches rules data #2642

[APPSEC-8112] Appsec block request when user ID matches rules data #2642

GustavoCaso commented Feb 23, 2023 •

edited

GustavoCaso Feb 23, 2023

lloeki Feb 23, 2023

GustavoCaso Feb 23, 2023

codecov-commenter commented Feb 23, 2023 •

edited

lloeki left a comment

lloeki Feb 23, 2023

GustavoCaso Feb 23, 2023

lloeki Feb 23, 2023

GustavoCaso Feb 23, 2023

GustavoCaso Feb 23, 2023

lloeki Feb 23, 2023

[APPSEC-8112] Appsec block request when user ID matches rules data #2642

[APPSEC-8112] Appsec block request when user ID matches rules data #2642

Conversation

GustavoCaso commented Feb 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 23, 2023 • edited

Codecov Report

lloeki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GustavoCaso commented Feb 23, 2023 •

edited

codecov-commenter commented Feb 23, 2023 •

edited