Skip to content

Commit

Permalink
fix(asm): switch back to default if remote config stop sending the se…
Browse files Browse the repository at this point in the history
…curity rule file [backport 2.10] (#10052)

backporting #10030

APPSEC-54105

(cherry picked from commit bc50e9c)

## 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)
  • Loading branch information
christophe-papazian authored Aug 2, 2024
1 parent 6de96d3 commit e3f9004
Show file tree
Hide file tree
Showing 5 changed files with 44 additions and 16 deletions.
25 changes: 16 additions & 9 deletions ddtrace/appsec/_processor.py
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,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
)
Expand All @@ -158,28 +158,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,
)
Expand Down
17 changes: 11 additions & 6 deletions ddtrace/appsec/_remoteconfiguration.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,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)
Expand All @@ -110,14 +111,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})

Expand Down
Original file line number Diff line number Diff line change
@@ -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.
2 changes: 1 addition & 1 deletion tests/appsec/appsec/test_processor.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
12 changes: 12 additions & 0 deletions tests/appsec/appsec/test_remoteconfiguration.py
Original file line number Diff line number Diff line change
Expand Up @@ -1032,3 +1032,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."

0 comments on commit e3f9004

Please sign in to comment.