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-8867] Appsec reuse rules when ASM_DD is empty #2732

Merged
merged 5 commits into from
Mar 31, 2023

Conversation

GustavoCaso
Copy link
Member

@GustavoCaso GustavoCaso commented Mar 30, 2023

What does this PR do?

During Remote Client sync, there could be occasions when the rules information from ASM_DD product is not present or not available.

In those situations, we should default to the ones defined on the appsec settings. By default is :recommended

Additional Notes

I had to extract away the parsing of the ruleset from the AppSec::Processor, so I could reuse it in other places in the code base. I like it because it removes that functionality from Processor, which is only responsible for ensuring a valid processor and context are created, with the ruleset provided.

How to test the change?

CI

@GustavoCaso GustavoCaso requested a review from a team March 30, 2023 10:05
@github-actions github-actions bot added appsec Application Security monitoring product core Involves Datadog core libraries labels Mar 30, 2023
@GustavoCaso GustavoCaso force-pushed the appsec-reuse-rules-when-asm-dd-is-empty branch from 65d818d to 45dd5d0 Compare March 30, 2023 10:10
@GustavoCaso GustavoCaso requested a review from lloeki March 30, 2023 10:11
Comment on lines +210 to +220
repository.contents.each do |content|
case content.path.product
when 'ASM_DD'
rules << parse_content(content)
when 'ASM_DATA'
data << parse_content(content) if asm_data_config_types.include?(content.path.config_id)
when 'ASM'
overrides << parse_content(content) if asm_overrides_config_types.include?(content.path.config_id)
exclusions << parse_content(content) if content.path.config_id == 'exclusion_filters'
end
end
Copy link
Member Author

Choose a reason for hiding this comment

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

This reduces the time we iterate over repository.contents from 4 to 1.

@codecov-commenter
Copy link

Codecov Report

Merging #2732 (894e8a2) into master (caee66d) will increase coverage by 0.00%.
The diff coverage is 99.16%.

@@           Coverage Diff           @@
##           master    #2732   +/-   ##
=======================================
  Coverage   98.05%   98.06%           
=======================================
  Files        1212     1214    +2     
  Lines       66643    66716   +73     
  Branches     2991     3002   +11     
=======================================
+ Hits        65350    65425   +75     
+ Misses       1293     1291    -2     
Impacted Files Coverage Δ
lib/datadog/core/remote/client.rb 97.39% <95.65%> (-0.78%) ⬇️
lib/datadog/appsec/processor/rule_loader.rb 96.77% <96.77%> (ø)
lib/datadog/appsec/component.rb 100.00% <100.00%> (ø)
lib/datadog/appsec/processor.rb 96.80% <100.00%> (+0.87%) ⬆️
spec/datadog/appsec/component_spec.rb 100.00% <100.00%> (ø)
spec/datadog/appsec/processor/rule_loader_spec.rb 100.00% <100.00%> (ø)
spec/datadog/appsec/processor_spec.rb 99.04% <100.00%> (-0.27%) ⬇️
spec/datadog/core/remote/client_spec.rb 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

📣 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

@@ -64,18 +63,18 @@ class AlreadyActiveContextError < StandardError; end

attr_reader :ruleset_info, :addresses

def initialize(ruleset: nil)
def initialize(ruleset:)
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, I was thinking of getting rid of that nilable

@GustavoCaso GustavoCaso merged commit c34f727 into master Mar 31, 2023
@GustavoCaso GustavoCaso deleted the appsec-reuse-rules-when-asm-dd-is-empty branch March 31, 2023 08:18
@github-actions github-actions bot added this to the 1.11.0 milestone Mar 31, 2023
@lloeki lloeki modified the milestones: 1.11.0, 1.11.0.beta1 Apr 14, 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 core Involves Datadog core libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants