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.

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-8112] Appsec block request when user ID matches rules data #2642

Merged
merged 2 commits into from
Feb 24, 2023

Conversation

GustavoCaso
Copy link
Member

@GustavoCaso GustavoCaso commented Feb 23, 2023

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 🟢

@github-actions github-actions bot added appsec Application Security monitoring product integrations Involves tracing integrations labels Feb 23, 2023
@GustavoCaso 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 GustavoCaso changed the title block request when user ID matches [APPSEC-8112] Appsec block request when user ID matches rules data Feb 23, 2023
@@ -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)
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Ref: Struct vs Class vs Hash vs OpenStruct

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@GustavoCaso GustavoCaso marked this pull request as ready for review February 23, 2023 14:42
@GustavoCaso GustavoCaso requested a review from a team February 23, 2023 14:42
@GustavoCaso GustavoCaso force-pushed the appsec-block-user-when-ID-matches branch from 5442da0 to b6f7afe Compare February 23, 2023 14:58
@GustavoCaso GustavoCaso force-pushed the appsec-block-user-when-ID-matches branch from b6f7afe to 3b09f87 Compare February 23, 2023 15:05
@codecov-commenter
Copy link

codecov-commenter commented Feb 23, 2023

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

Copy link
Contributor

@lloeki lloeki left a comment

Choose a reason for hiding this comment

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

LGTM!

module Datadog
module AppSec
module Ext
INTERRUPT = :datadog_appsec_interrupt
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Ref: Struct vs Class vs Hash vs OpenStruct

@GustavoCaso GustavoCaso merged commit 4ea0c70 into master Feb 24, 2023
@GustavoCaso GustavoCaso deleted the appsec-block-user-when-ID-matches branch February 24, 2023 08:18
@github-actions github-actions bot added this to the 1.10.0 milestone Feb 24, 2023
@TonyCTHsu TonyCTHsu mentioned this pull request Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
appsec Application Security monitoring product integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants