From 7484bde99a3a89b8cede0b3fe8eb66a065fe234a Mon Sep 17 00:00:00 2001 From: Andrea Terzolo Date: Thu, 4 Jan 2024 16:35:27 +0100 Subject: [PATCH] update(rule_loader): deprecate `enabled` usage Signed-off-by: Andrea Terzolo --- unit_tests/engine/test_rule_loader.cpp | 174 ++++++++++++++++++++++++ userspace/engine/rule_loader_reader.cpp | 1 + userspace/engine/rule_loader_reader.h | 7 +- 3 files changed, 180 insertions(+), 2 deletions(-) diff --git a/unit_tests/engine/test_rule_loader.cpp b/unit_tests/engine/test_rule_loader.cpp index 94f97f77ee4..48231f33e51 100644 --- a/unit_tests/engine/test_rule_loader.cpp +++ b/unit_tests/engine/test_rule_loader.cpp @@ -43,6 +43,17 @@ class engine_loader_test : public ::testing::Test { return ret; } + // This must be kept in line with the (private) falco_engine::s_default_ruleset + uint64_t num_rules_for_ruleset(std::string ruleset = "falco-default-ruleset") + { + return m_engine->num_rules_for_ruleset(ruleset); + } + + bool has_warnings() + { + return m_load_result->has_warnings(); + } + bool check_warning_message(std::string warning_msg) { if(!m_load_result->has_warnings()) @@ -53,6 +64,8 @@ class engine_loader_test : public ::testing::Test { for(auto &warn : m_load_result_json["warnings"]) { std::string msg = warn["message"]; + // Debug: + // printf("msg: %s\n", msg.c_str()); if(msg.find(warning_msg) != std::string::npos) { return true; @@ -62,6 +75,28 @@ class engine_loader_test : public ::testing::Test { return false; } + bool check_error_message(std::string error_msg) + { + // if the loading is successful there are no errors + if(m_load_result->successful()) + { + return false; + } + + for(auto &err : m_load_result_json["errors"]) + { + std::string msg = err["message"]; + // Debug: + // printf("msg: %s\n", msg.c_str()); + if(msg.find(error_msg) != std::string::npos) + { + return true; + } + } + + return false; + } + std::string m_sample_ruleset; std::string m_sample_source; sinsp_filter_check_list m_filterlist; @@ -377,3 +412,142 @@ TEST_F(engine_loader_test, rule_override_extra_field) ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); ASSERT_TRUE(std::string(m_load_result_json["errors"][0]["message"]).find("Unexpected key 'priority'") != std::string::npos); } + +TEST_F(engine_loader_test, missing_enabled_key_with_override) +{ + std::string rules_content = R"END( +- rule: test_rule + desc: test rule description + condition: evt.type = close + output: user=%user.name command=%proc.cmdline file=%fd.name + priority: INFO + enabled: false + +- rule: test_rule + desc: missing enabled key + condition: and proc.name = cat + override: + desc: replace + condition: append + enabled: replace +)END"; + + // In the rule override we miss `enabled: true` + ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); + ASSERT_TRUE(check_error_message("'enabled' was specified but 'enabled' is not defined")); +} + +TEST_F(engine_loader_test, rule_override_with_enabled) +{ + std::string rules_content = R"END( +- rule: test_rule + desc: test rule description + condition: evt.type = close + output: user=%user.name command=%proc.cmdline file=%fd.name + priority: INFO + enabled: false + +- rule: test_rule + desc: correct override + condition: and proc.name = cat + enabled: true + override: + desc: replace + condition: append + enabled: replace +)END"; + + ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); + ASSERT_FALSE(has_warnings()); + // The rule should be enabled at the end. + EXPECT_EQ(num_rules_for_ruleset(), 1); +} + +TEST_F(engine_loader_test, rule_not_enabled) +{ + std::string rules_content = R"END( +- rule: test_rule + desc: rule not enabled + condition: evt.type = close + output: user=%user.name command=%proc.cmdline file=%fd.name + priority: INFO + enabled: false +)END"; + + ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); + ASSERT_FALSE(has_warnings()); + EXPECT_EQ(num_rules_for_ruleset(), 0); +} + +TEST_F(engine_loader_test, rule_enabled_warning) +{ + std::string rules_content = R"END( +- rule: test_rule + desc: test rule description + condition: evt.type = close + output: user=%user.name command=%proc.cmdline file=%fd.name + priority: INFO + enabled: false + +- rule: test_rule + enabled: true +)END"; + + ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); + ASSERT_TRUE(check_warning_message(WARNING_ENABLED_MESSAGE)); + // The rule should be enabled at the end. + EXPECT_EQ(num_rules_for_ruleset(), 1); +} + +// todo!: Probably we shouldn't allow this syntax +TEST_F(engine_loader_test, rule_enabled_is_ignored_by_append) +{ + std::string rules_content = R"END( +- rule: test_rule + desc: test rule description + condition: evt.type = close + output: user=%user.name command=%proc.cmdline file=%fd.name + priority: INFO + enabled: false + +- rule: test_rule + condition: and proc.name = cat + append: true + enabled: true +)END"; + + // 'enabled' is ignored by the append, this syntax is not supported + // so the rule is not enabled. + ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); + EXPECT_EQ(num_rules_for_ruleset(), 0); +} + +// todo!: Probably we shouldn't allow this syntax +TEST_F(engine_loader_test, rewrite_rule) +{ + std::string rules_content = R"END( +- rule: test_rule + desc: test rule description + condition: evt.type = close + output: user=%user.name command=%proc.cmdline file=%fd.name + priority: INFO + enabled: false + +- rule: test_rule + desc: redefined rule syntax + condition: proc.name = cat + output: user=%user.name command=%proc.cmdline file=%fd.name + priority: WARNING + enabled: true +)END"; + + // The above syntax is not supported, we cannot override the content + // of a rule in this way. + ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); + // In this case the rule is completely overridden but this syntax is not supported. + EXPECT_EQ(num_rules_for_ruleset(), 1); + + std::string rule_name = "test_rule"; + auto rule_description = m_engine->describe_rule(&rule_name, {}); + ASSERT_EQ(rule_description["rules"][0]["details"]["condition_compiled"].template get(), "proc.name = cat"); +} diff --git a/userspace/engine/rule_loader_reader.cpp b/userspace/engine/rule_loader_reader.cpp index 6d149b8e4bd..0b7e1a64c49 100644 --- a/userspace/engine/rule_loader_reader.cpp +++ b/userspace/engine/rule_loader_reader.cpp @@ -693,6 +693,7 @@ static void read_item( !item["priority"].IsDefined()) { decode_val(item, "enabled", v.enabled, ctx); + cfg.res->add_warning(falco::load_result::LOAD_DEPRECATED_ITEM, WARNING_ENABLED_MESSAGE, ctx); collector.enable(cfg, v); } else diff --git a/userspace/engine/rule_loader_reader.h b/userspace/engine/rule_loader_reader.h index ef0225ac39b..877f9d451b5 100644 --- a/userspace/engine/rule_loader_reader.h +++ b/userspace/engine/rule_loader_reader.h @@ -24,10 +24,13 @@ limitations under the License. #include "falco_engine_version.h" // Error message used when both 'override' and 'append' are specified. -#define OVERRIDE_APPEND_ERROR_MESSAGE "Keys 'override' and 'append: true' cannot be used together. Add an append entry (e.g. 'condition: append') under 'override' instead." +#define OVERRIDE_APPEND_ERROR_MESSAGE "Keys 'override' and 'append: true' cannot be used together. Add an 'append' entry (e.g. 'condition: append') under 'override' instead." // Warning message used when `append` is used. -#define WARNING_APPEND_MESSAGE "'append' key is deprecated. Add an append entry (e.g. 'condition: append') under 'override' instead." +#define WARNING_APPEND_MESSAGE "'append' key is deprecated. Add an 'append' entry (e.g. 'condition: append') under 'override' instead." + +// Warning message used when `enabled` is used without override. +#define WARNING_ENABLED_MESSAGE "The standalone 'enabled' key usage is deprecated. The correct approach requires also a 'replace' entry under the 'override' key (i.e. 'enabled: replace')." namespace rule_loader {