From c167bd08d4c2fb3bf9c9de0aa972be42d12d351f Mon Sep 17 00:00:00 2001 From: Yury Lebedev Date: Thu, 28 Nov 2024 16:34:28 +0100 Subject: [PATCH] Fix AppSec libddwaf actions handling after update to 1.17.0.0.0 In version 1.17.0.0.0 libddwaf changes the format of the actions that are returned from an array of actions IDs to an array of actions objects. This means that we no longer have to look up actions in the ruleset and can just use actions that libddwaf returns. --- lib/datadog/appsec.rb | 4 +- lib/datadog/appsec/component.rb | 9 +- lib/datadog/appsec/processor/actions.rb | 49 --------- lib/datadog/appsec/remote.rb | 4 +- lib/datadog/appsec/response.rb | 11 +- sig/datadog/appsec.rbs | 1 - sig/datadog/appsec/processor/actions.rbs | 18 --- sig/datadog/appsec/response.rbs | 2 +- spec/datadog/appsec/component_spec.rb | 101 +---------------- spec/datadog/appsec/processor/actions_spec.rb | 104 ------------------ spec/datadog/appsec/remote_spec.rb | 39 +------ spec/datadog/appsec/response_spec.rb | 91 ++++----------- 12 files changed, 37 insertions(+), 396 deletions(-) delete mode 100644 lib/datadog/appsec/processor/actions.rb delete mode 100644 sig/datadog/appsec/processor/actions.rbs delete mode 100644 spec/datadog/appsec/processor/actions_spec.rb 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))