Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Feb 24, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
93cf714
block request when user ID matches
GustavoCaso Feb 23, 2023
3b09f87
make sure to reset AppSec.settings state after each test to avoid fla…
GustavoCaso Feb 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions lib/datadog/appsec/contrib/rack/request_middleware.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

require 'json'

require_relative '../../ext'
require_relative '../../instrumentation/gateway'
require_relative '../../processor'
require_relative '../../response'
Expand Down Expand Up @@ -38,8 +39,10 @@ def call(env)

add_appsec_tags(processor, active_trace, active_span, env)

request_return, request_response = Instrumentation.gateway.push('rack.request', request) do
@app.call(env)
request_return, request_response = catch(::Datadog::AppSec::Ext::INTERRUPT) do
Instrumentation.gateway.push('rack.request', request) do
@app.call(env)
end
end

if request_response && request_response.any? { |action, _event| action == :block }
Expand Down
10 changes: 10 additions & 0 deletions lib/datadog/appsec/ext.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# typed: false
# frozen_string_literal: true

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

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

end
end
end
3 changes: 2 additions & 1 deletion lib/datadog/appsec/monitor/gateway/watcher.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# typed: false
# frozen_string_literal: true

require_relative '../../ext'
require_relative '../../instrumentation/gateway'
require_relative '../../reactive/operation'
require_relative '../reactive/set_user'
Expand Down Expand Up @@ -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
Member

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


ret, res = stack.call(user)

Expand Down
5 changes: 5 additions & 0 deletions lib/datadog/kit/identity.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.configuration.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
Member

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

::Datadog::AppSec::Instrumentation.gateway.push('identity.set_user', user)
end
end
# rubocop:enable Metrics/PerceivedComplexity
# rubocop:enable Metrics/CyclomaticComplexity
Expand Down
7 changes: 7 additions & 0 deletions sig/datadog/appsec/ext.rbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
module Datadog
module AppSec
module Ext
INTERRUPT: Symbol
end
end
end
36 changes: 35 additions & 1 deletion spec/datadog/appsec/contrib/rack/integration_test_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
let(:appsec_enabled) { true }
let(:tracing_enabled) { true }
let(:appsec_ip_denylist) { nil }
let(:appsec_user_id_denylist) { nil }
let(:appsec_ruleset) { :recommended }

let(:crs_942_100) do
Expand Down Expand Up @@ -133,11 +134,15 @@
c.appsec.enabled = appsec_enabled
c.appsec.instrument :rack
c.appsec.ip_denylist = appsec_ip_denylist
c.appsec.user_id_denylist = appsec_user_id_denylist
c.appsec.ruleset = appsec_ruleset
end
end

after { Datadog.registry[:rack].reset_configuration! }
after do
Datadog::AppSec.settings.send(:reset!)
Datadog.registry[:rack].reset_configuration!
end

context 'for an application' do
# TODO: also test without Tracing: it should run without trace transport
Expand Down Expand Up @@ -293,6 +298,15 @@
end
)
end

map '/set_user' do
run(
proc do |_env|
Datadog::Kit::Identity.set_user(Datadog::Tracing.active_trace, id: 'blocked-user-id')
[200, { 'Content-Type' => 'text/html' }, ['OK']]
end
)
end
end
end

Expand Down Expand Up @@ -382,6 +396,26 @@
it_behaves_like 'a trace with AppSec events'
end
end

context 'with user blocking ID' do
let(:url) { '/set_user' }

it { is_expected.to be_ok }

it_behaves_like 'a GET 200 span'
it_behaves_like 'a trace with AppSec tags'
it_behaves_like 'a trace without AppSec events'

context 'with an event-triggering user ID' do
let(:appsec_user_id_denylist) { ['blocked-user-id'] }

it { is_expected.to be_forbidden }

it_behaves_like 'a GET 403 span'
it_behaves_like 'a trace with AppSec tags'
it_behaves_like 'a trace with AppSec events'
end
end
end

describe 'POST request' do
Expand Down
33 changes: 33 additions & 0 deletions spec/datadog/appsec/contrib/rails/integration_test_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
let(:appsec_enabled) { true }
let(:tracing_enabled) { true }
let(:appsec_ip_denylist) { nil }
let(:appsec_user_id_denylist) { nil }
let(:appsec_ruleset) { :recommended }

let(:crs_942_100) do
Expand Down Expand Up @@ -89,12 +90,18 @@
c.appsec.enabled = appsec_enabled
c.appsec.instrument :rails
c.appsec.ip_denylist = appsec_ip_denylist
c.appsec.user_id_denylist = appsec_user_id_denylist
c.appsec.ruleset = appsec_ruleset

# TODO: test with c.appsec.instrument :rack
end
end

after do
Datadog::AppSec.settings.send(:reset!)
Datadog.registry[:rails].reset_configuration!
end

context 'for an application' do
include_context 'Rails test application'

Expand All @@ -118,6 +125,11 @@
def success
head :ok
end

def set_user
Datadog::Kit::Identity.set_user(Datadog::Tracing.active_trace, id: 'blocked-user-id')
head :ok
end
end
)
end
Expand Down Expand Up @@ -244,6 +256,7 @@ def success
{
'/success' => 'test#success',
[:post, '/success'] => 'test#success',
'/set_user' => 'test#set_user',
}
end

Expand Down Expand Up @@ -347,6 +360,26 @@ def success
it_behaves_like 'a trace with AppSec tags'
it_behaves_like 'a trace with AppSec events'
end

context 'with user blocking ID' do
let(:url) { '/set_user' }

it { is_expected.to be_ok }

it_behaves_like 'a GET 200 span'
it_behaves_like 'a trace with AppSec tags'
it_behaves_like 'a trace without AppSec events'

context 'with an event-triggering user ID' do
let(:appsec_user_id_denylist) { ['blocked-user-id'] }

it { is_expected.to be_forbidden }

it_behaves_like 'a GET 403 span'
it_behaves_like 'a trace with AppSec tags'
it_behaves_like 'a trace with AppSec events'
end
end
end

describe 'POST request' do
Expand Down
28 changes: 28 additions & 0 deletions spec/datadog/appsec/contrib/sinatra/integration_test_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
let(:appsec_enabled) { true }
let(:tracing_enabled) { true }
let(:appsec_ip_denylist) { nil }
let(:appsec_user_id_denylist) { nil }
let(:appsec_ruleset) { :recommended }

let(:crs_942_100) do
Expand Down Expand Up @@ -101,13 +102,15 @@
c.appsec.enabled = appsec_enabled
c.appsec.instrument :sinatra
c.appsec.ip_denylist = appsec_ip_denylist
c.appsec.user_id_denylist = appsec_user_id_denylist
c.appsec.ruleset = appsec_ruleset

# TODO: test with c.appsec.instrument :rack
end
end

after do
Datadog::AppSec.settings.send(:reset!)
Datadog.registry[:rack].reset_configuration!
Datadog.registry[:sinatra].reset_configuration!
end
Expand Down Expand Up @@ -254,6 +257,11 @@
post '/success' do
'ok'
end

get '/set_user' do
Datadog::Kit::Identity.set_user(Datadog::Tracing.active_trace, id: 'blocked-user-id')
'ok'
end
end
end

Expand Down Expand Up @@ -359,6 +367,26 @@
it_behaves_like 'a trace with AppSec tags'
it_behaves_like 'a trace with AppSec events'
end

context 'with user blocking ID' do
let(:url) { '/set_user' }

it { is_expected.to be_ok }

it_behaves_like 'a GET 200 span'
it_behaves_like 'a trace with AppSec tags'
it_behaves_like 'a trace without AppSec events'

context 'with an event-triggering user ID' do
let(:appsec_user_id_denylist) { ['blocked-user-id'] }

it { is_expected.to be_forbidden }

it_behaves_like 'a GET 403 span'
it_behaves_like 'a trace with AppSec tags'
it_behaves_like 'a trace with AppSec events'
end
end
end

describe 'POST request' do
Expand Down
24 changes: 24 additions & 0 deletions spec/datadog/kit/identity_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,5 +230,29 @@
expect { described_class.set_user(trace_op, id: 42, foo: 42) }.to raise_error(TypeError)
end
end

context 'appsec' do
after { Datadog.configuration.appsec.send(:reset!) }

context 'when is enabled' do
it 'instruments the user information to appsec' do
Datadog.configuration.appsec.enabled = true
user = OpenStruct.new(id: '42')
expect_any_instance_of(Datadog::AppSec::Instrumentation::Gateway).to receive(:push).with(
'identity.set_user',
user
)
described_class.set_user(trace_op, id: '42')
end
end

context 'when is disabled' do
it 'does not instrument the user information to appsec' do
Datadog.configuration.appsec.enabled = false
expect_any_instance_of(Datadog::AppSec::Instrumentation::Gateway).to_not receive(:push).with('identity.set_user')
described_class.set_user(trace_op, id: '42')
end
end
end
end
end