From 43820c8797aa2aac71e1e9a8f2a69f2e98231039 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 2 Aug 2024 13:51:24 +0200 Subject: [PATCH] fix(asm): switch back to default if remote config stop sending the security rule file [backport 2.9] (#10053) Backport e3f90045c677465ad132daf55abd5f4776f3f856 from #10052 to 2.9. backporting https://github.com/DataDog/dd-trace-py/pull/10030 APPSEC-54105 (cherry picked from commit bc50e9cd69c4a21a101e11bf250a7904dc6b6937) ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) Co-authored-by: Christophe Papazian <114495376+christophe-papazian@users.noreply.github.com> --- ddtrace/appsec/_processor.py | 25 ++++++++++++------- ddtrace/appsec/_remoteconfiguration.py | 17 ++++++++----- ...ix_rc_asm_dd_no_file-37e6f733583e334c.yaml | 4 +++ tests/appsec/appsec/test_processor.py | 2 +- .../appsec/appsec/test_remoteconfiguration.py | 12 +++++++++ 5 files changed, 44 insertions(+), 16 deletions(-) create mode 100644 releasenotes/notes/fix_rc_asm_dd_no_file-37e6f733583e334c.yaml diff --git a/ddtrace/appsec/_processor.py b/ddtrace/appsec/_processor.py index 3af6f18547d..588d6a378ba 100644 --- a/ddtrace/appsec/_processor.py +++ b/ddtrace/appsec/_processor.py @@ -140,7 +140,7 @@ def _get_rate_limiter() -> RateLimiter: @dataclasses.dataclass(eq=False) class AppSecSpanProcessor(SpanProcessor): - rules: str = dataclasses.field(default_factory=get_rules) + rule_filename: str = dataclasses.field(default_factory=get_rules) obfuscation_parameter_key_regexp: bytes = dataclasses.field( default_factory=get_appsec_obfuscation_parameter_key_regexp ) @@ -159,28 +159,35 @@ def __post_init__(self) -> None: from ddtrace.appsec._ddwaf import DDWaf try: - with open(self.rules, "r") as f: - rules = json.load(f) + with open(self.rule_filename, "r") as f: + self._rules = json.load(f) except EnvironmentError as err: if err.errno == errno.ENOENT: - log.error("[DDAS-0001-03] ASM could not read the rule file %s. Reason: file does not exist", self.rules) + log.error( + "[DDAS-0001-03] ASM could not read the rule file %s. Reason: file does not exist", + self.rule_filename, + ) else: # TODO: try to log reasons - log.error("[DDAS-0001-03] ASM could not read the rule file %s.", self.rules) + log.error("[DDAS-0001-03] ASM could not read the rule file %s.", self.rule_filename) raise except JSONDecodeError: - log.error("[DDAS-0001-03] ASM could not read the rule file %s. Reason: invalid JSON file", self.rules) + log.error( + "[DDAS-0001-03] ASM could not read the rule file %s. Reason: invalid JSON file", self.rule_filename + ) raise except Exception: # TODO: try to log reasons - log.error("[DDAS-0001-03] ASM could not read the rule file %s.", self.rules) + log.error("[DDAS-0001-03] ASM could not read the rule file %s.", self.rule_filename) raise try: - self._ddwaf = DDWaf(rules, self.obfuscation_parameter_key_regexp, self.obfuscation_parameter_value_regexp) + self._ddwaf = DDWaf( + self._rules, self.obfuscation_parameter_key_regexp, self.obfuscation_parameter_value_regexp + ) if not self._ddwaf._handle or self._ddwaf.info.failed: stack_trace = "DDWAF.__init__: invalid rules\n ruleset: %s\nloaded:%s\nerrors:%s\n" % ( - rules, + self._rules, self._ddwaf.info.loaded, self._ddwaf.info.errors, ) diff --git a/ddtrace/appsec/_remoteconfiguration.py b/ddtrace/appsec/_remoteconfiguration.py index ca2db4f6710..23f30b335c4 100644 --- a/ddtrace/appsec/_remoteconfiguration.py +++ b/ddtrace/appsec/_remoteconfiguration.py @@ -91,8 +91,9 @@ def _add_rules_to_list(features: Mapping[str, Any], feature: str, message: str, if rules is not None: try: if ruleset.get(feature) is None: - ruleset[feature] = [] - ruleset[feature] += rules + ruleset[feature] = rules + else: + ruleset[feature] = ruleset[feature] + rules log.debug("Reloading Appsec %s: %s", message, str(rules)[:20]) except json.JSONDecodeError: log.error("ERROR Appsec %s: invalid JSON content from remote configuration", message) @@ -114,14 +115,18 @@ def _appsec_rules_data(features: Mapping[str, Any], test_tracer: Optional[Tracer if features and tracer._appsec_processor: ruleset = {} # type: dict[str, Optional[list[Any]]] - _add_rules_to_list(features, "rules_data", "rules data", ruleset) + if features.get("rules", None) == []: + # if rules is empty, we need to switch back to the default rules + ruleset = tracer._appsec_processor._rules.copy() or {} + _add_rules_to_list(features, "actions", "actions", ruleset) _add_rules_to_list(features, "custom_rules", "custom rules", ruleset) - _add_rules_to_list(features, "rules", "Datadog rules", ruleset) _add_rules_to_list(features, "exclusions", "exclusion filters", ruleset) + _add_rules_to_list(features, "exclusion_data", "exclusion data", ruleset) + _add_rules_to_list(features, "processors", "processors", ruleset) + _add_rules_to_list(features, "rules", "Datadog rules", ruleset) + _add_rules_to_list(features, "rules_data", "rules data", ruleset) _add_rules_to_list(features, "rules_override", "rules override", ruleset) _add_rules_to_list(features, "scanners", "scanners", ruleset) - _add_rules_to_list(features, "processors", "processors", ruleset) - _add_rules_to_list(features, "actions", "actions", ruleset) if ruleset: return tracer._appsec_processor._update_rules({k: v for k, v in ruleset.items() if v is not None}) diff --git a/releasenotes/notes/fix_rc_asm_dd_no_file-37e6f733583e334c.yaml b/releasenotes/notes/fix_rc_asm_dd_no_file-37e6f733583e334c.yaml new file mode 100644 index 00000000000..a3224a1df96 --- /dev/null +++ b/releasenotes/notes/fix_rc_asm_dd_no_file-37e6f733583e334c.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - | + ASM: This fix resolves an issue where the WAF could be disabled if the ASM_DD rule file was not found in Remote Config. diff --git a/tests/appsec/appsec/test_processor.py b/tests/appsec/appsec/test_processor.py index f01314037ca..967cbcebc34 100644 --- a/tests/appsec/appsec/test_processor.py +++ b/tests/appsec/appsec/test_processor.py @@ -79,7 +79,7 @@ def test_enable_custom_rules(): processor = AppSecSpanProcessor() assert processor.enabled - assert processor.rules == rules.RULES_GOOD_PATH + assert processor.rule_filename == rules.RULES_GOOD_PATH def test_ddwaf_ctx(tracer_appsec): diff --git a/tests/appsec/appsec/test_remoteconfiguration.py b/tests/appsec/appsec/test_remoteconfiguration.py index 223d9813468..91bf42fb638 100644 --- a/tests/appsec/appsec/test_remoteconfiguration.py +++ b/tests/appsec/appsec/test_remoteconfiguration.py @@ -1033,3 +1033,15 @@ def test_rc_rules_data_error_ddwaf(tracer): "rules": [{"invalid": mock.MagicMock()}], } assert not _appsec_rules_data(config, tracer) + + +def test_rules_never_empty(tracer): + with override_global_config(dict(_asm_enabled=True)): + tracer.configure(appsec_enabled=True, api_version="v0.4") + with mock.patch("ddtrace.appsec._processor.AppSecSpanProcessor._update_rules", autospec=True) as mock_update: + mock_update.reset_mock() + _appsec_rules_data({"rules": []}, tracer) + call = mock_update.mock_calls + args = call[-1][1][1] + assert "rules" in args + assert args["rules"], "empty rules should not be possible, it must switch to default."