diff --git a/.github/workflows/system-tests.yml b/.github/workflows/system-tests.yml index 28dd9f79fe7..929541ba8e6 100644 --- a/.github/workflows/system-tests.yml +++ b/.github/workflows/system-tests.yml @@ -220,6 +220,7 @@ jobs: - APPSEC_BLOCKING_FULL_DENYLIST - APPSEC_REQUEST_BLOCKING - APPSEC_STANDALONE + - APPSEC_BLOCKING include: - library: ruby app: rack diff --git a/lib/datadog/appsec.rb b/lib/datadog/appsec.rb index a00403bc5f2..940bbc4b4b6 100644 --- a/lib/datadog/appsec.rb +++ b/lib/datadog/appsec.rb @@ -24,12 +24,12 @@ def processor appsec_component.processor if appsec_component end - def reconfigure(ruleset:, actions:, telemetry:) + def reconfigure(ruleset:, telemetry:) appsec_component = components.appsec return unless appsec_component - appsec_component.reconfigure(ruleset: ruleset, actions: actions, telemetry: telemetry) + appsec_component.reconfigure(ruleset: ruleset, telemetry: telemetry) end def reconfigure_lock(&block) diff --git a/lib/datadog/appsec/component.rb b/lib/datadog/appsec/component.rb index ce877db721a..7eb46adb141 100644 --- a/lib/datadog/appsec/component.rb +++ b/lib/datadog/appsec/component.rb @@ -3,7 +3,6 @@ require_relative 'processor' require_relative 'processor/rule_merger' require_relative 'processor/rule_loader' -require_relative 'processor/actions' module Datadog module AppSec @@ -52,10 +51,6 @@ def create_processor(settings, telemetry) ) return nil unless rules - actions = rules['actions'] - - AppSec::Processor::Actions.merge(actions) if actions - data = AppSec::Processor::RuleLoader.load_data( ip_denylist: settings.appsec.ip_denylist, user_id_denylist: settings.appsec.user_id_denylist, @@ -84,10 +79,8 @@ def initialize(processor:) @mutex = Mutex.new end - def reconfigure(ruleset:, actions:, telemetry:) + def reconfigure(ruleset:, telemetry:) @mutex.synchronize do - AppSec::Processor::Actions.merge(actions) - new = Processor.new(ruleset: ruleset, telemetry: telemetry) if new && new.ready? diff --git a/lib/datadog/appsec/processor/actions.rb b/lib/datadog/appsec/processor/actions.rb deleted file mode 100644 index 5f022d812d8..00000000000 --- a/lib/datadog/appsec/processor/actions.rb +++ /dev/null @@ -1,49 +0,0 @@ -# frozen_string_literal: true - -module Datadog - module AppSec - class Processor - # Actions store the actions information in memory - # Also, takes care of merging when RC send new information - module Actions - class << self - def actions - @actions ||= [] - end - - def fetch_configuration(action) - actions.find { |action_configuration| action_configuration['id'] == action } - end - - def merge(actions_to_merge) - return if actions_to_merge.empty? - - if actions.empty? - @actions = actions_to_merge - else - merged_actions = [] - actions_dup = actions.dup - - actions_to_merge.each do |new_action| - existing_action = actions_dup.find { |action| new_action['id'] == action['id'] } - - # the old action is discard and the new kept - actions_dup.delete(existing_action) if existing_action - merged_actions << new_action - end - - @actions = merged_actions.concat(actions_dup) - end - end - - private - - # Used in tests - def reset - @actions = [] - end - end - end - end - end -end diff --git a/lib/datadog/appsec/remote.rb b/lib/datadog/appsec/remote.rb index 29ef57bbd3f..163ebac352b 100644 --- a/lib/datadog/appsec/remote.rb +++ b/lib/datadog/appsec/remote.rb @@ -67,7 +67,6 @@ def receivers(telemetry) data = [] overrides = [] exclusions = [] - actions = [] repository.contents.each do |content| parsed_content = parse_content(content) @@ -81,7 +80,6 @@ def receivers(telemetry) overrides << parsed_content['rules_override'] if parsed_content['rules_override'] exclusions << parsed_content['exclusions'] if parsed_content['exclusions'] custom_rules << parsed_content['custom_rules'] if parsed_content['custom_rules'] - actions.concat(parsed_content['actions']) if parsed_content['actions'] end end @@ -105,7 +103,7 @@ def receivers(telemetry) telemetry: telemetry ) - Datadog::AppSec.reconfigure(ruleset: ruleset, actions: actions, telemetry: telemetry) + Datadog::AppSec.reconfigure(ruleset: ruleset, telemetry: telemetry) end [receiver] diff --git a/lib/datadog/appsec/response.rb b/lib/datadog/appsec/response.rb index acad8573a9d..03ae79dbac7 100644 --- a/lib/datadog/appsec/response.rb +++ b/lib/datadog/appsec/response.rb @@ -31,19 +31,16 @@ class << self def negotiate(env, actions) # @type var configured_response: Response? configured_response = nil - actions.each do |action| + actions.each do |type, parameters| # Need to use next to make steep happy :( # I rather use break to stop the execution next if configured_response - action_configuration = AppSec::Processor::Actions.fetch_configuration(action) - next unless action_configuration - - configured_response = case action_configuration['type'] + configured_response = case type when 'block_request' - block_response(env, action_configuration['parameters']) + block_response(env, parameters) when 'redirect_request' - redirect_response(env, action_configuration['parameters']) + redirect_response(env, parameters) end end diff --git a/sig/datadog/appsec.rbs b/sig/datadog/appsec.rbs index a39a0a33e3e..c4adceb753a 100644 --- a/sig/datadog/appsec.rbs +++ b/sig/datadog/appsec.rbs @@ -6,7 +6,6 @@ module Datadog def self.reconfigure: ( ruleset: ::Hash[untyped, untyped], - actions: Array[Hash[String, untyped]], telemetry: Datadog::Core::Telemetry::Component ) -> void diff --git a/sig/datadog/appsec/processor/actions.rbs b/sig/datadog/appsec/processor/actions.rbs deleted file mode 100644 index 5d689a11b60..00000000000 --- a/sig/datadog/appsec/processor/actions.rbs +++ /dev/null @@ -1,18 +0,0 @@ -module Datadog - module AppSec - class Processor - module Actions - self.@actions: Array[Hash[String, untyped]] - - def self.actions: () -> Array[Hash[String, untyped]] - - def self.fetch_configuration: (String action) -> Hash[String, untyped]? - - def self.merge: (Array[Hash[String, untyped]] actions_to_merge) -> Array[Hash[String, untyped]]? - - private - def self.reset: () -> void - end - end - end -end diff --git a/sig/datadog/appsec/response.rbs b/sig/datadog/appsec/response.rbs index d74f4c47128..3d00dedc5a9 100644 --- a/sig/datadog/appsec/response.rbs +++ b/sig/datadog/appsec/response.rbs @@ -11,7 +11,7 @@ module Datadog def to_sinatra_response: () -> ::Sinatra::Response def to_action_dispatch_response: () -> ::ActionDispatch::Response - def self.negotiate: (::Hash[untyped, untyped] env, Array[String] actions) -> Response + def self.negotiate: (::Hash[untyped, untyped] env, ::Hash[String, untyped] actions) -> Response def self.graphql_response: (Datadog::AppSec::Contrib::GraphQL::Gateway::Multiplex gateway_multiplex) -> Array[::GraphQL::Query::Result] private diff --git a/spec/datadog/appsec/component_spec.rb b/spec/datadog/appsec/component_spec.rb index 84e3a4e569d..78167642aa2 100644 --- a/spec/datadog/appsec/component_spec.rb +++ b/spec/datadog/appsec/component_spec.rb @@ -73,86 +73,6 @@ expect(component.processor).to be_nil end end - - context 'when static rules have actions defined' do - it 'calls Datadog::AppSec::Processor::Actions.merge' do - actions = [ - { - 'id' => 'block', - 'type' => 'block_request', - 'parameters' => { - 'type' => 'auto', - 'status_code' => 403, - - } - } - ] - ruleset = - { - 'version' => '2.2', - 'rules' => [{ - 'conditions' => [{ - 'operator' => 'ip_match', - 'parameters' => { - 'data' => 'blocked_ips', - 'inputs' => [{ - 'address' => 'http.client_ip' - }] - } - }], - 'id' => 'blk-001-001', - 'name' => 'Block IP Addresses', - 'on_match' => ['block'], - 'tags' => { - 'category' => 'security_response', 'type' => 'block_ip' - }, - 'transformers' => [] - }], - 'actions' => actions - } - - expect(Datadog::AppSec::Processor::Actions).to receive(:merge).with(actions) - expect(Datadog::AppSec::Processor::RuleLoader).to receive(:load_rules).and_return(ruleset) - - component = described_class.build_appsec_component(settings, telemetry: telemetry) - - expect(component.processor).to be_a(Datadog::AppSec::Processor) - end - end - - context 'when static rules do not have actions defined' do - it 'calls Datadog::AppSec::Processor::Actions.merge' do - ruleset = - { - 'version' => '2.2', - 'rules' => [{ - 'conditions' => [{ - 'operator' => 'ip_match', - 'parameters' => { - 'data' => 'blocked_ips', - 'inputs' => [{ - 'address' => 'http.client_ip' - }] - } - }], - 'id' => 'blk-001-001', - 'name' => 'Block IP Addresses', - 'on_match' => ['block'], - 'tags' => { - 'category' => 'security_response', 'type' => 'block_ip' - }, - 'transformers' => [] - }], - } - - expect(Datadog::AppSec::Processor::Actions).to_not receive(:merge) - expect(Datadog::AppSec::Processor::RuleLoader).to receive(:load_rules).and_return(ruleset) - - component = described_class.build_appsec_component(settings, telemetry: telemetry) - - expect(component.processor).to be_a(Datadog::AppSec::Processor) - end - end end context 'when appsec is not enabled' do @@ -220,8 +140,6 @@ } end - let(:actions) { [] } - context 'lock' do it 'makes sure to synchronize' do mutex = Mutex.new @@ -229,18 +147,7 @@ component = described_class.new(processor: processor) component.instance_variable_set(:@mutex, mutex) expect(mutex).to receive(:synchronize) - component.reconfigure(ruleset: {}, actions: actions, telemetry: telemetry) - end - end - - context 'actions' do - it 'merges the actions' do - processor = instance_double(Datadog::AppSec::Processor) - expect(processor).to receive(:finalize) - component = described_class.new(processor: processor) - - expect(Datadog::AppSec::Processor::Actions).to receive(:merge).with(actions) - component.reconfigure(ruleset: ruleset, actions: actions, telemetry: telemetry) + component.reconfigure(ruleset: {}, telemetry: telemetry) end end @@ -252,7 +159,7 @@ old_processor = component.processor expect(old_processor).to receive(:finalize) - component.reconfigure(ruleset: ruleset, actions: actions, telemetry: telemetry) + component.reconfigure(ruleset: ruleset, telemetry: telemetry) new_processor = component.processor expect(new_processor).to_not eq(old_processor) new_processor.finalize @@ -267,7 +174,7 @@ old_processor = component.processor expect(old_processor).to_not receive(:finalize) - component.reconfigure(ruleset: ruleset, actions: actions, telemetry: telemetry) + component.reconfigure(ruleset: ruleset, telemetry: telemetry) new_processor = component.processor expect(new_processor).to_not eq(old_processor) new_processor.finalize @@ -284,7 +191,7 @@ ruleset = { 'invalid_one' => true } expect(old_processor).to_not receive(:finalize) - component.reconfigure(ruleset: ruleset, actions: actions, telemetry: telemetry) + component.reconfigure(ruleset: ruleset, telemetry: telemetry) expect(component.processor).to eq(old_processor) end end diff --git a/spec/datadog/appsec/processor/actions_spec.rb b/spec/datadog/appsec/processor/actions_spec.rb deleted file mode 100644 index fd20e9efef3..00000000000 --- a/spec/datadog/appsec/processor/actions_spec.rb +++ /dev/null @@ -1,104 +0,0 @@ -require 'datadog/appsec/spec_helper' -require 'datadog/appsec/processor/actions' - -RSpec.describe Datadog::AppSec::Processor::Actions do - after do - described_class.send(:reset) - end - - let(:actions) do - [ - { - 'id' => 'block', - 'parameters' => { - 'type' => 'auto', - 'status_code' => 403, - - } - } - ] - end - - describe '.merge' do - context 'empty actions' do - it 'stores the actions' do - described_class.merge(actions) - expect(described_class.actions).to eq(actions) - end - end - - context 'no empty actions' do - it 'merges' do - described_class.merge(actions) - expect(described_class.actions).to eq(actions) - - actions_to_merge = [ - { - 'id' => 'redirect_request', - 'parameters' => { - 'location' => 'foo', - 'status_code' => 303, - - } - } - ] - - expectd_result = [] - expectd_result.concat(actions) - expectd_result.concat(actions_to_merge) - - described_class.merge(actions_to_merge) - - expect(described_class.actions).to match_array(expectd_result) - end - - it 'merges and updates exiting ones' do - described_class.merge(actions) - expect(described_class.actions).to eq(actions) - - actions_to_merge = [ - { - 'id' => 'block', - 'parameters' => { - 'type' => 'html', - 'status_code' => 403, - - } - }, - { - 'id' => 'redirect_request', - 'parameters' => { - 'location' => 'foo', - 'status_code' => 303, - - } - } - ] - - described_class.merge(actions_to_merge) - expect(described_class.actions).to eq(actions_to_merge) - end - end - end - - describe '.fetch_configuration' do - it 'returns the existing configuration' do - described_class.merge(actions) - expect(described_class.fetch_configuration('block')).to eq( - { - 'id' => 'block', - 'parameters' => { - 'type' => 'auto', - 'status_code' => 403, - - } - } - ) - end - - it 'returns nil if no configuration matches' do - described_class.merge(actions) - expect(described_class.fetch_configuration('fake')).to be_nil - end - end -end diff --git a/spec/datadog/appsec/remote_spec.rb b/spec/datadog/appsec/remote_spec.rb index acf53ba9abd..4b1db36be98 100644 --- a/spec/datadog/appsec/remote_spec.rb +++ b/spec/datadog/appsec/remote_spec.rb @@ -167,7 +167,7 @@ } expect(Datadog::AppSec).to receive(:reconfigure).with( - ruleset: expected_ruleset, actions: [], telemetry: telemetry + ruleset: expected_ruleset, telemetry: telemetry ).and_return(nil) changes = transaction receiver.call(repository, changes) @@ -288,19 +288,6 @@ ] end - let(:actions) do - [ - { - 'id' => 'block', - 'type' => 'block_request', - 'parameters' => { - 'status_code' => 418, - 'type' => 'auto' - } - } - ] - end - context 'ASM' do let(:path) { 'datadog/603646/ASM/whatevername/config' } @@ -370,28 +357,6 @@ end end - context 'actions' do - let(:data) do - { - 'actions' => actions - } - end - - it 'pass the actions to reconfigure' do - ruleset = Datadog::AppSec::Processor::RuleMerger.merge(rules: default_ruleset, telemetry: telemetry) - - expect(Datadog::AppSec).to receive(:reconfigure).with( - ruleset: ruleset, - actions: actions, - telemetry: telemetry - ) - .and_return(nil) - - changes = transaction - receiver.call(repository, changes) - end - end - context 'multiple keys' do let(:data) do { @@ -494,7 +459,7 @@ ruleset = Datadog::AppSec::Processor::RuleMerger.merge(rules: default_ruleset, telemetry: telemetry) changes = transaction - expect(Datadog::AppSec).to receive(:reconfigure).with(ruleset: ruleset, actions: [], telemetry: telemetry) + expect(Datadog::AppSec).to receive(:reconfigure).with(ruleset: ruleset, telemetry: telemetry) .and_return(nil) receiver.call(repository, changes) end diff --git a/spec/datadog/appsec/response_spec.rb b/spec/datadog/appsec/response_spec.rb index 655ff6800b2..e4d2d786fc1 100644 --- a/spec/datadog/appsec/response_spec.rb +++ b/spec/datadog/appsec/response_spec.rb @@ -3,39 +3,28 @@ RSpec.describe Datadog::AppSec::Response do describe '.negotiate' do let(:env) { double } - let(:actions) { [] } before do allow(env).to receive(:key?).with('HTTP_ACCEPT').and_return(true) allow(env).to receive(:[]).with('HTTP_ACCEPT').and_return('text/html') - Datadog::AppSec::Processor::Actions.merge(actions) - end - - after do - Datadog::AppSec::Processor::Actions.send(:reset) end describe 'configured actions' do describe 'block' do let(:actions) do - [ - { - 'id' => 'block', - 'type' => 'block_request', - 'parameters' => { - 'type' => type, - 'status_code' => status_code, - - } + { + 'block_request' => { + 'type' => type, + 'status_code' => status_code } - ] + } end let(:type) { 'html' } let(:status_code) { 100 } context 'status_code' do - subject(:status) { described_class.negotiate(env, ['block']).status } + subject(:status) { described_class.negotiate(env, actions).status } it { is_expected.to eq 100 } @@ -47,7 +36,7 @@ end context 'body' do - subject(:body) { described_class.negotiate(env, ['block']).body } + subject(:body) { described_class.negotiate(env, actions).body } it { is_expected.to eq [Datadog::AppSec::Assets.blocked(format: :html)] } @@ -64,7 +53,7 @@ end context 'headers' do - subject(:header) { described_class.negotiate(env, ['block']).headers['Content-Type'] } + subject(:header) { described_class.negotiate(env, actions).headers['Content-Type'] } it { is_expected.to eq 'text/html' } @@ -80,19 +69,9 @@ end end - context 'no specify action' do - subject(:response) { described_class.negotiate(env, []) } - - it 'uses default response' do - expect(response.status).to eq 403 - expect(response.body).to eq [Datadog::AppSec::Assets.blocked(format: :html)] - expect(response.headers['Content-Type']).to eq 'text/html' - end - end - context 'no configured actions' do - let(:actions) { [] } - subject(:response) { described_class.negotiate(env, []) } + let(:actions) { {} } + subject(:response) { described_class.negotiate(env, actions) } it 'uses default response' do expect(response.status).to eq 403 @@ -104,24 +83,19 @@ describe 'redirect_request' do let(:actions) do - [ - { - 'id' => 'redirect_request', - 'type' => 'redirect_request', - 'parameters' => { - 'location' => location, - 'status_code' => status_code, - - } + { + 'redirect_request' => { + 'location' => location, + 'status_code' => status_code } - ] + } end let(:location) { 'foo' } let(:status_code) { 303 } context 'status_code' do - subject(:status) { described_class.negotiate(env, ['redirect_request']).status } + subject(:status) { described_class.negotiate(env, actions).status } it { is_expected.to eq 303 } @@ -133,13 +107,13 @@ end context 'body' do - subject(:body) { described_class.negotiate(env, ['redirect_request']).body } + subject(:body) { described_class.negotiate(env, actions).body } it { is_expected.to eq [] } end context 'headers' do - subject(:headers) { described_class.negotiate(env, ['redirect_request']).headers } + subject(:headers) { described_class.negotiate(env, actions).headers } context 'Content-Type' do before do @@ -159,31 +133,10 @@ end end - context 'no specify action' do - subject(:response) { described_class.negotiate(env, []) } - - it 'uses default response' do - expect(response.status).to eq 403 - expect(response.body).to eq [Datadog::AppSec::Assets.blocked(format: :html)] - expect(response.headers['Content-Type']).to eq 'text/html' - end - end - context 'location is empty' do let(:location) { '' } - subject(:response) { described_class.negotiate(env, []) } - - it 'uses default response' do - expect(response.status).to eq 403 - expect(response.body).to eq [Datadog::AppSec::Assets.blocked(format: :html)] - expect(response.headers['Content-Type']).to eq 'text/html' - end - end - - context 'no configured actions' do - let(:actions) { [] } - subject(:response) { described_class.negotiate(env, []) } + subject(:response) { described_class.negotiate(env, actions) } it 'uses default response' do expect(response.status).to eq 403 @@ -195,13 +148,13 @@ end describe '.status' do - subject(:status) { described_class.negotiate(env, []).status } + subject(:status) { described_class.negotiate(env, {}).status } it { is_expected.to eq 403 } end describe '.body' do - subject(:body) { described_class.negotiate(env, []).body } + subject(:body) { described_class.negotiate(env, {}).body } before do expect(env).to receive(:key?).with('HTTP_ACCEPT').and_return(true) @@ -254,7 +207,7 @@ end describe ".headers['Content-Type']" do - subject(:content_type) { described_class.negotiate(env, []).headers['Content-Type'] } + subject(:content_type) { described_class.negotiate(env, {}).headers['Content-Type'] } before do expect(env).to receive(:key?).with('HTTP_ACCEPT').and_return(respond_to?(:accept))