From 4e522420024efe96403be62e17501b85fe8f4098 Mon Sep 17 00:00:00 2001 From: Andrea Terzolo Date: Wed, 3 Jan 2024 12:40:19 +0100 Subject: [PATCH 01/19] cleanup(rule_loader): add a common log message Signed-off-by: Andrea Terzolo --- unit_tests/engine/test_rule_loader.cpp | 2 +- userspace/engine/rule_loader_reader.cpp | 7 +++---- userspace/engine/rule_loader_reader.h | 3 +++ 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/unit_tests/engine/test_rule_loader.cpp b/unit_tests/engine/test_rule_loader.cpp index d369d5c7798..22ab01efd83 100644 --- a/unit_tests/engine/test_rule_loader.cpp +++ b/unit_tests/engine/test_rule_loader.cpp @@ -283,7 +283,7 @@ TEST_F(engine_loader_test, rule_incorrect_append_override) std::string rule_name = "failing_rule"; ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); - ASSERT_TRUE(std::string(m_load_result_json["errors"][0]["message"]).find("'override' and 'append: true' cannot be used together") != std::string::npos); + ASSERT_TRUE(std::string(m_load_result_json["errors"][0]["message"]).find(OVERRIDE_APPEND_ERROR_MESSAGE) != std::string::npos); } TEST_F(engine_loader_test, rule_override_without_rule) diff --git a/userspace/engine/rule_loader_reader.cpp b/userspace/engine/rule_loader_reader.cpp index c7aec794fd2..591ac8f5799 100644 --- a/userspace/engine/rule_loader_reader.cpp +++ b/userspace/engine/rule_loader_reader.cpp @@ -448,7 +448,7 @@ static void read_item( if(append == true && has_overrides) { - THROW(true, "Keys 'override' and 'append: true' cannot be used together.", ctx); + THROW(true, OVERRIDE_APPEND_ERROR_MESSAGE, ctx); } // Since a list only has items, if we have chosen to append them we can append the entire object @@ -490,7 +490,7 @@ static void read_item( if(append == true && has_overrides) { - THROW(true, "Keys 'override' and 'append: true' cannot be used together.", ctx); + THROW(true, OVERRIDE_APPEND_ERROR_MESSAGE, ctx); } // Since a macro only has a condition, if we have chosen to append to it we can append the entire object @@ -528,8 +528,7 @@ static void read_item( bool has_overrides_replace = !override_replace.empty(); bool has_overrides = has_overrides_append || has_overrides_replace; - THROW((has_append_flag && has_overrides), - "Keys 'override' and 'append: true' cannot be used together. Add an append entry (e.g. 'condition: append') under override instead.", ctx); + THROW((has_append_flag && has_overrides), OVERRIDE_APPEND_ERROR_MESSAGE, ctx); if(has_overrides) { diff --git a/userspace/engine/rule_loader_reader.h b/userspace/engine/rule_loader_reader.h index 378556c1dbf..50825300bcb 100644 --- a/userspace/engine/rule_loader_reader.h +++ b/userspace/engine/rule_loader_reader.h @@ -23,6 +23,9 @@ limitations under the License. #include "version.h" #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." + namespace rule_loader { From ad964c02d066b5938c543166e3171420c12e630b Mon Sep 17 00:00:00 2001 From: Andrea Terzolo Date: Wed, 3 Jan 2024 13:04:02 +0100 Subject: [PATCH 02/19] cleanup(rule_loader): remove useless include Signed-off-by: Andrea Terzolo --- userspace/engine/rule_loader_reader.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/userspace/engine/rule_loader_reader.cpp b/userspace/engine/rule_loader_reader.cpp index 591ac8f5799..e4535d33308 100644 --- a/userspace/engine/rule_loader_reader.cpp +++ b/userspace/engine/rule_loader_reader.cpp @@ -22,7 +22,6 @@ limitations under the License. #include "rule_loader_reader.h" #include "falco_engine_version.h" -#include "logger.h" #define THROW(cond, err, ctx) { if ((cond)) { throw rule_loader::rule_load_exception(falco::load_result::LOAD_ERR_YAML_VALIDATE, (err), (ctx)); } } From 55b1c02cff94460d0ef4ea1151926dc0466e5b58 Mon Sep 17 00:00:00 2001 From: Andrea Terzolo Date: Wed, 3 Jan 2024 15:40:46 +0100 Subject: [PATCH 03/19] update(rule_loader): deprecate `append` key and add a warning Signed-off-by: Andrea Terzolo --- unit_tests/engine/test_rule_loader.cpp | 29 ++++++++++++++++++ userspace/engine/falco_load_result.cpp | 9 ++++-- userspace/engine/falco_load_result.h | 3 +- userspace/engine/rule_loader_reader.cpp | 40 ++++++++++++++----------- userspace/engine/rule_loader_reader.h | 5 +++- 5 files changed, 64 insertions(+), 22 deletions(-) diff --git a/unit_tests/engine/test_rule_loader.cpp b/unit_tests/engine/test_rule_loader.cpp index 22ab01efd83..94f97f77ee4 100644 --- a/unit_tests/engine/test_rule_loader.cpp +++ b/unit_tests/engine/test_rule_loader.cpp @@ -43,6 +43,25 @@ class engine_loader_test : public ::testing::Test { return ret; } + bool check_warning_message(std::string warning_msg) + { + if(!m_load_result->has_warnings()) + { + return false; + } + + for(auto &warn : m_load_result_json["warnings"]) + { + std::string msg = warn["message"]; + if(msg.find(warning_msg) != std::string::npos) + { + return true; + } + } + + return false; + } + std::string m_sample_ruleset; std::string m_sample_source; sinsp_filter_check_list m_filterlist; @@ -134,6 +153,9 @@ TEST_F(engine_loader_test, rule_override_append) std::string rule_name = "legit_rule"; ASSERT_TRUE(load_rules(rules_content, "legit_rules.yaml")) << m_load_result_string; + // Here we don't use the deprecated `append` flag, so we don't expect the warning. + ASSERT_FALSE(check_warning_message(WARNING_APPEND_MESSAGE)); + auto rule_description = m_engine->describe_rule(&rule_name, {}); ASSERT_EQ(rule_description["rules"][0]["info"]["condition"].template get(), "evt.type=open and proc.name = cat"); @@ -163,6 +185,9 @@ TEST_F(engine_loader_test, rule_append) std::string rule_name = "legit_rule"; ASSERT_TRUE(load_rules(rules_content, "legit_rules.yaml")) << m_load_result_string; + // We should have at least one warning because the 'append' flag is deprecated. + ASSERT_TRUE(check_warning_message(WARNING_APPEND_MESSAGE)); + auto rule_description = m_engine->describe_rule(&rule_name, {}); ASSERT_EQ(rule_description["rules"][0]["details"]["condition_compiled"].template get(), "(evt.type = open and proc.name = cat)"); @@ -283,6 +308,10 @@ TEST_F(engine_loader_test, rule_incorrect_append_override) std::string rule_name = "failing_rule"; ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); + + // We should have at least one warning because the 'append' flag is deprecated. + ASSERT_TRUE(check_warning_message(WARNING_APPEND_MESSAGE)); + ASSERT_TRUE(std::string(m_load_result_json["errors"][0]["message"]).find(OVERRIDE_APPEND_ERROR_MESSAGE) != std::string::npos); } diff --git a/userspace/engine/falco_load_result.cpp b/userspace/engine/falco_load_result.cpp index fec4b051af9..11b14cf9b5b 100644 --- a/userspace/engine/falco_load_result.cpp +++ b/userspace/engine/falco_load_result.cpp @@ -66,7 +66,8 @@ static const std::string warning_codes[] = { "LOAD_UNKNOWN_FILTER", "LOAD_UNUSED_MACRO", "LOAD_UNUSED_LIST", - "LOAD_UNKNOWN_ITEM" + "LOAD_UNKNOWN_ITEM", + "LOAD_DEPRECATED_ITEM" }; const std::string& falco::load_result::warning_code_str(warning_code wc) @@ -81,7 +82,8 @@ static const std::string warning_strings[] = { "Unknown field or event-type in condition or output", "Unused macro", "Unused list", - "Unknown rules file item" + "Unknown rules file item", + "Used deprecated item" }; const std::string& falco::load_result::warning_str(warning_code wc) @@ -96,7 +98,8 @@ static const std::string warning_descs[] = { "A rule condition or output refers to a field or evt.type that does not exist. This is normally an error, but if a rule has a skip-if-unknown-filter property, the error is downgraded to a warning.", "A macro is defined in the rules content but is not used by any other macro or rule.", "A list is defined in the rules content but is not used by any other list, macro, or rule.", - "An unknown top-level object is in the rules content. It will be ignored." + "An unknown top-level object is in the rules content. It will be ignored.", + "A deprecated item is employed by lists, macros, or rules." }; const std::string& falco::load_result::warning_desc(warning_code wc) diff --git a/userspace/engine/falco_load_result.h b/userspace/engine/falco_load_result.h index a2072be9187..2b4b43c8343 100644 --- a/userspace/engine/falco_load_result.h +++ b/userspace/engine/falco_load_result.h @@ -54,7 +54,8 @@ class load_result { LOAD_UNKNOWN_FILTER, LOAD_UNUSED_MACRO, LOAD_UNUSED_LIST, - LOAD_UNKNOWN_ITEM + LOAD_UNKNOWN_ITEM, + LOAD_DEPRECATED_ITEM }; virtual ~load_result() = default; diff --git a/userspace/engine/rule_loader_reader.cpp b/userspace/engine/rule_loader_reader.cpp index e4535d33308..6d149b8e4bd 100644 --- a/userspace/engine/rule_loader_reader.cpp +++ b/userspace/engine/rule_loader_reader.cpp @@ -434,27 +434,28 @@ static void read_item( rule_loader::context ctx(item, rule_loader::context::LIST, name, parent); rule_loader::list_info v(ctx); - bool append = false; + bool has_append_flag = false; decode_val(item, "list", v.name, ctx); decode_items(item, v.items, ctx); - decode_optional_val(item, "append", append, ctx); - + decode_optional_val(item, "append", has_append_flag, ctx); + if(has_append_flag) + { + cfg.res->add_warning(falco::load_result::LOAD_DEPRECATED_ITEM, WARNING_APPEND_MESSAGE, ctx); + } + std::set override_append, override_replace; std::set overridable {"items"}; decode_overrides(item, overridable, overridable, override_append, override_replace, ctx); bool has_overrides = !override_append.empty() || !override_replace.empty(); - if(append == true && has_overrides) - { - THROW(true, OVERRIDE_APPEND_ERROR_MESSAGE, ctx); - } + THROW(has_append_flag && has_overrides, OVERRIDE_APPEND_ERROR_MESSAGE, ctx); // Since a list only has items, if we have chosen to append them we can append the entire object // otherwise we just want to redefine the list. - append |= override_append.find("items") != override_append.end(); + has_append_flag |= override_append.find("items") != override_append.end(); - if(append) + if(has_append_flag) { collector.append(cfg, v); } @@ -474,29 +475,30 @@ static void read_item( rule_loader::macro_info v(ctx); v.name = name; - bool append = false; + bool has_append_flag = false; decode_val(item, "condition", v.cond, ctx); // Now set the proper context for the condition now that we know it exists v.cond_ctx = rule_loader::context(item["condition"], rule_loader::context::MACRO_CONDITION, "", ctx); - decode_optional_val(item, "append", append, ctx); + decode_optional_val(item, "append", has_append_flag, ctx); + if(has_append_flag) + { + cfg.res->add_warning(falco::load_result::LOAD_DEPRECATED_ITEM, WARNING_APPEND_MESSAGE, ctx); + } std::set override_append, override_replace; std::set overridable {"condition"}; decode_overrides(item, overridable, overridable, override_append, override_replace, ctx); bool has_overrides = !override_append.empty() || !override_replace.empty(); - if(append == true && has_overrides) - { - THROW(true, OVERRIDE_APPEND_ERROR_MESSAGE, ctx); - } + THROW((has_append_flag && has_overrides), OVERRIDE_APPEND_ERROR_MESSAGE, ctx); // Since a macro only has a condition, if we have chosen to append to it we can append the entire object // otherwise we just want to redefine the macro. - append |= override_append.find("condition") != override_append.end(); + has_append_flag |= override_append.find("condition") != override_append.end(); - if(append) + if(has_append_flag) { collector.append(cfg, v); } @@ -517,6 +519,10 @@ static void read_item( bool has_append_flag = false; decode_optional_val(item, "append", has_append_flag, ctx); + if(has_append_flag) + { + cfg.res->add_warning(falco::load_result::LOAD_DEPRECATED_ITEM, WARNING_APPEND_MESSAGE, ctx); + } std::set override_append, override_replace; std::set overridable_append {"condition", "output", "desc", "tags", "exceptions"}; diff --git a/userspace/engine/rule_loader_reader.h b/userspace/engine/rule_loader_reader.h index 50825300bcb..ef0225ac39b 100644 --- a/userspace/engine/rule_loader_reader.h +++ b/userspace/engine/rule_loader_reader.h @@ -24,7 +24,10 @@ 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." namespace rule_loader { From 7484bde99a3a89b8cede0b3fe8eb66a065fe234a Mon Sep 17 00:00:00 2001 From: Andrea Terzolo Date: Thu, 4 Jan 2024 16:35:27 +0100 Subject: [PATCH 04/19] 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 { From 66cea672662edfd2a8011530fbcd614d66d5ba63 Mon Sep 17 00:00:00 2001 From: Andrea Terzolo Date: Thu, 4 Jan 2024 16:40:07 +0100 Subject: [PATCH 05/19] cleanup(tests): use new `check_error_message` helper Signed-off-by: Andrea Terzolo --- unit_tests/engine/test_rule_loader.cpp | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/unit_tests/engine/test_rule_loader.cpp b/unit_tests/engine/test_rule_loader.cpp index 48231f33e51..3ed5aaa01c3 100644 --- a/unit_tests/engine/test_rule_loader.cpp +++ b/unit_tests/engine/test_rule_loader.cpp @@ -202,7 +202,6 @@ TEST_F(engine_loader_test, rule_override_append) "legit rule description with append"); } - TEST_F(engine_loader_test, rule_append) { std::string rules_content = R"END( @@ -228,7 +227,6 @@ TEST_F(engine_loader_test, rule_append) "(evt.type = open and proc.name = cat)"); } - TEST_F(engine_loader_test, rule_override_replace) { std::string rules_content = R"END( @@ -318,7 +316,7 @@ TEST_F(engine_loader_test, rule_incorrect_override_type) std::string rule_name = "failing_rule"; ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); - ASSERT_EQ(m_load_result_json["errors"][0]["message"], "Key 'priority' cannot be appended to, use 'replace' instead"); + ASSERT_TRUE(check_error_message("Key 'priority' cannot be appended to, use 'replace' instead")); ASSERT_TRUE(std::string(m_load_result_json["errors"][0]["context"]["snippet"]).find("priority: append") != std::string::npos); } @@ -347,7 +345,7 @@ TEST_F(engine_loader_test, rule_incorrect_append_override) // We should have at least one warning because the 'append' flag is deprecated. ASSERT_TRUE(check_warning_message(WARNING_APPEND_MESSAGE)); - ASSERT_TRUE(std::string(m_load_result_json["errors"][0]["message"]).find(OVERRIDE_APPEND_ERROR_MESSAGE) != std::string::npos); + ASSERT_TRUE(check_error_message(OVERRIDE_APPEND_ERROR_MESSAGE)); } TEST_F(engine_loader_test, rule_override_without_rule) @@ -364,7 +362,7 @@ TEST_F(engine_loader_test, rule_override_without_rule) std::string rule_name = "failing_rule"; ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); - ASSERT_TRUE(std::string(m_load_result_json["errors"][0]["message"]).find("no rule by that name already exists") != std::string::npos); + ASSERT_TRUE(check_error_message("no rule by that name already exists")); } TEST_F(engine_loader_test, rule_override_without_field) @@ -386,7 +384,7 @@ TEST_F(engine_loader_test, rule_override_without_field) std::string rule_name = "failing_rule"; ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); - ASSERT_EQ(m_load_result_json["errors"][0]["message"], "An append override for 'condition' was specified but 'condition' is not defined"); + ASSERT_TRUE(check_error_message("An append override for 'condition' was specified but 'condition' is not defined")); } TEST_F(engine_loader_test, rule_override_extra_field) @@ -410,7 +408,7 @@ TEST_F(engine_loader_test, rule_override_extra_field) std::string rule_name = "failing_rule"; 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); + ASSERT_TRUE(check_error_message("Unexpected key 'priority'")); } TEST_F(engine_loader_test, missing_enabled_key_with_override) From 4a1fe2f77443ebb419203fba68dc69817133b230 Mon Sep 17 00:00:00 2001 From: Andrea Terzolo Date: Thu, 4 Jan 2024 17:06:52 +0100 Subject: [PATCH 06/19] update(rule_loader): deprecate all non-SemVer compatible values Signed-off-by: Andrea Terzolo --- unit_tests/engine/test_rule_loader.cpp | 54 +++++++++++++++++++++++++ userspace/engine/rule_loader_reader.cpp | 1 + userspace/engine/rule_loader_reader.h | 3 ++ 3 files changed, 58 insertions(+) diff --git a/unit_tests/engine/test_rule_loader.cpp b/unit_tests/engine/test_rule_loader.cpp index 3ed5aaa01c3..e4c5d70f85a 100644 --- a/unit_tests/engine/test_rule_loader.cpp +++ b/unit_tests/engine/test_rule_loader.cpp @@ -549,3 +549,57 @@ TEST_F(engine_loader_test, rewrite_rule) auto rule_description = m_engine->describe_rule(&rule_name, {}); ASSERT_EQ(rule_description["rules"][0]["details"]["condition_compiled"].template get(), "proc.name = cat"); } + +TEST_F(engine_loader_test, required_engine_version_semver) +{ + std::string rules_content = R"END( +- required_engine_version: 0.26.0 + +- 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 + +)END"; + + ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); + ASSERT_FALSE(has_warnings()); +} + +TEST_F(engine_loader_test, required_engine_version_not_semver) +{ + std::string rules_content = R"END( +- required_engine_version: 26 + +- 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 + +)END"; + + ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); + ASSERT_TRUE(check_warning_message(WARNING_ENGINE_VERSION_NOT_SEMVER)); +} + +TEST_F(engine_loader_test, required_engine_version_invalid) +{ + std::string rules_content = R"END( +- required_engine_version: seven + +- 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 + +)END"; + + ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); + ASSERT_TRUE(check_error_message("Unable to parse engine version")); +} diff --git a/userspace/engine/rule_loader_reader.cpp b/userspace/engine/rule_loader_reader.cpp index 0b7e1a64c49..8258b13b20a 100644 --- a/userspace/engine/rule_loader_reader.cpp +++ b/userspace/engine/rule_loader_reader.cpp @@ -368,6 +368,7 @@ static void read_item( // Build proper semver representation v.version = rule_loader::reader::get_implicit_engine_version(ver); + cfg.res->add_warning(falco::load_result::LOAD_DEPRECATED_ITEM, WARNING_ENGINE_VERSION_NOT_SEMVER, ctx); } catch(std::exception& e) { diff --git a/userspace/engine/rule_loader_reader.h b/userspace/engine/rule_loader_reader.h index 877f9d451b5..768f7e4ead0 100644 --- a/userspace/engine/rule_loader_reader.h +++ b/userspace/engine/rule_loader_reader.h @@ -32,6 +32,9 @@ limitations under the License. // 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')." +// Warning message used when the `required_engine_version` is not semver compatible. +#define WARNING_ENGINE_VERSION_NOT_SEMVER "The 'required_engine_version' should be SemVer compatible. All non-SemVer compatible values are deprecated." + namespace rule_loader { From e930f40b5df66f4c6608d8b70a61c3f008971ea7 Mon Sep 17 00:00:00 2001 From: Andrea Terzolo Date: Thu, 4 Jan 2024 18:04:46 +0100 Subject: [PATCH 07/19] cleanup(falco_engine): remove unused methods Signed-off-by: Andrea Terzolo --- userspace/engine/falco_engine.cpp | 70 ------------------------------- userspace/engine/falco_engine.h | 17 +------- 2 files changed, 1 insertion(+), 86 deletions(-) diff --git a/userspace/engine/falco_engine.cpp b/userspace/engine/falco_engine.cpp index af73cec7251..189efa41adc 100644 --- a/userspace/engine/falco_engine.cpp +++ b/userspace/engine/falco_engine.cpp @@ -177,15 +177,6 @@ void falco_engine::list_fields(std::string &source, bool verbose, bool names_onl } } -void falco_engine::load_rules(const std::string &rules_content, bool verbose, bool all_events) -{ - static const std::string no_name = "N/A"; - - std::unique_ptr res = load_rules(rules_content, no_name); - - interpret_load_result(res, no_name, rules_content, verbose); -} - std::unique_ptr falco_engine::load_rules(const std::string &rules_content, const std::string &name) { rule_loader::configuration cfg(rules_content, m_sources, name); @@ -257,44 +248,6 @@ std::unique_ptr falco_engine::load_rules(const std::string &rules_c return std::move(cfg.res); } -void falco_engine::load_rules_file(const std::string &rules_filename, bool verbose, bool all_events) -{ - std::string rules_content; - - read_file(rules_filename, rules_content); - - std::unique_ptr res = load_rules(rules_content, rules_filename); - - interpret_load_result(res, rules_filename, rules_content, verbose); -} - -std::unique_ptr falco_engine::load_rules_file(const std::string &rules_filename) -{ - std::string rules_content; - - try { - read_file(rules_filename, rules_content); - } - catch (falco_exception &e) - { - rule_loader::context ctx(rules_filename); - - std::unique_ptr res(new rule_loader::result(rules_filename)); - - res->add_error(load_result::LOAD_ERR_FILE_READ, e.what(), ctx); - -// Old gcc versions (e.g. 4.8.3) won't allow move elision but newer versions -// (e.g. 10.2.1) would complain about the redundant move. -#if __GNUC__ > 4 - return res; -#else - return std::move(res); -#endif - } - - return load_rules(rules_content, rules_filename); -} - void falco_engine::enable_rule(const std::string &substring, bool enabled, const std::string &ruleset) { uint16_t ruleset_id = find_ruleset_id(ruleset); @@ -955,29 +908,6 @@ void falco_engine::read_file(const std::string& filename, std::string& contents) std::istreambuf_iterator()); } -void falco_engine::interpret_load_result(std::unique_ptr& res, - const std::string& rules_filename, - const std::string& rules_content, - bool verbose) -{ - falco::load_result::rules_contents_t rc = {{rules_filename, rules_content}}; - - if(!res->successful()) - { - // The output here is always the full e.g. "verbose" output. - throw falco_exception(res->as_string(true, rc).c_str()); - } - - if(verbose && res->has_warnings()) - { - // Here, verbose controls whether to additionally - // "log" e.g. print to stderr. What's logged is always - // non-verbose so it fits on a single line. - // todo(jasondellaluce): introduce a logging callback in Falco - fprintf(stderr, "%s\n", res->as_string(false, rc).c_str()); - } -} - static bool check_plugin_requirement_alternatives( const std::vector& plugins, const rule_loader::plugin_version_info::requirement_alternatives& alternatives, diff --git a/userspace/engine/falco_engine.h b/userspace/engine/falco_engine.h index 1ca7ec67157..a13e58ebe00 100644 --- a/userspace/engine/falco_engine.h +++ b/userspace/engine/falco_engine.h @@ -74,15 +74,8 @@ class falco_engine void list_fields(std::string &source, bool verbose, bool names_only, bool markdown) const; // - // Load rules either directly or from a filename. + // Load rules and returns a result object. // - void load_rules_file(const std::string &rules_filename, bool verbose, bool all_events); - void load_rules(const std::string &rules_content, bool verbose, bool all_events); - - // - // Identical to above, but returns a result object instead of - // throwing exceptions on error. - std::unique_ptr load_rules_file(const std::string &rules_filename); std::unique_ptr load_rules(const std::string &rules_content, const std::string &name); // @@ -296,13 +289,6 @@ class falco_engine // Throws falco_exception if the file can not be read void read_file(const std::string& filename, std::string& contents); - // For load_rules methods that throw exceptions on error, - // interpret a load_result and throw an exception if needed. - void interpret_load_result(std::unique_ptr& res, - const std::string& rules_filename, - const std::string& rules_content, - bool verbose); - indexed_vector m_sources; const falco_source* find_source(std::size_t index) const; @@ -387,4 +373,3 @@ class falco_engine std::string m_extra; bool m_replace_container_info; }; - From 468897bfd4a3d8efac5a5f7e078eb074315e70a8 Mon Sep 17 00:00:00 2001 From: Andrea Terzolo Date: Thu, 4 Jan 2024 18:11:20 +0100 Subject: [PATCH 08/19] update(falco): always enable rules warnings Signed-off-by: Andrea Terzolo --- userspace/falco/app/actions/load_rules_files.cpp | 5 ++--- userspace/falco/app/actions/validate_rules_files.cpp | 7 +------ 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/userspace/falco/app/actions/load_rules_files.cpp b/userspace/falco/app/actions/load_rules_files.cpp index cde74ba5bf4..9f62234766a 100644 --- a/userspace/falco/app/actions/load_rules_files.cpp +++ b/userspace/falco/app/actions/load_rules_files.cpp @@ -79,10 +79,9 @@ falco::app::run_result falco::app::actions::load_rules_files(falco::app::state& break; } - // If verbose is true, also print any warnings - if(s.options.verbose && res->has_warnings()) + if(res->has_warnings()) { - fprintf(stderr, "%s\n", res->as_string(true, rc).c_str()); + falco_logger::log(falco_logger::level::WARNING,res->as_string(true, rc) + "\n"); } } diff --git a/userspace/falco/app/actions/validate_rules_files.cpp b/userspace/falco/app/actions/validate_rules_files.cpp index 67cfdcd7249..cf55f3f53bf 100644 --- a/userspace/falco/app/actions/validate_rules_files.cpp +++ b/userspace/falco/app/actions/validate_rules_files.cpp @@ -114,12 +114,7 @@ falco::app::run_result falco::app::actions::validate_rules_files(falco::app::sta // file was ok with warnings, without actually // printing the warnings. summary += filename + ": Ok, with warnings"; - - // If verbose is true, print the warnings now. - if(s.options.verbose) - { - fprintf(stderr, "%s\n", res->as_string(true, rc).c_str()); - } + falco_logger::log(falco_logger::level::WARNING, res->as_string(true, rc) + "\n"); } } From 8befe94ec1a79c4e23ca6b4151cc843ad6f2e12d Mon Sep 17 00:00:00 2001 From: Andrea Terzolo Date: Tue, 9 Jan 2024 12:33:58 +0100 Subject: [PATCH 09/19] update(rule_loader): remove the warning on the required_engine_version Signed-off-by: Andrea Terzolo --- unit_tests/engine/test_rule_loader.cpp | 2 +- userspace/engine/rule_loader_reader.cpp | 1 - userspace/engine/rule_loader_reader.h | 3 --- 3 files changed, 1 insertion(+), 5 deletions(-) diff --git a/unit_tests/engine/test_rule_loader.cpp b/unit_tests/engine/test_rule_loader.cpp index e4c5d70f85a..1b11a2aed4d 100644 --- a/unit_tests/engine/test_rule_loader.cpp +++ b/unit_tests/engine/test_rule_loader.cpp @@ -583,7 +583,7 @@ TEST_F(engine_loader_test, required_engine_version_not_semver) )END"; ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); - ASSERT_TRUE(check_warning_message(WARNING_ENGINE_VERSION_NOT_SEMVER)); + ASSERT_FALSE(has_warnings()); } TEST_F(engine_loader_test, required_engine_version_invalid) diff --git a/userspace/engine/rule_loader_reader.cpp b/userspace/engine/rule_loader_reader.cpp index 8258b13b20a..0b7e1a64c49 100644 --- a/userspace/engine/rule_loader_reader.cpp +++ b/userspace/engine/rule_loader_reader.cpp @@ -368,7 +368,6 @@ static void read_item( // Build proper semver representation v.version = rule_loader::reader::get_implicit_engine_version(ver); - cfg.res->add_warning(falco::load_result::LOAD_DEPRECATED_ITEM, WARNING_ENGINE_VERSION_NOT_SEMVER, ctx); } catch(std::exception& e) { diff --git a/userspace/engine/rule_loader_reader.h b/userspace/engine/rule_loader_reader.h index 768f7e4ead0..877f9d451b5 100644 --- a/userspace/engine/rule_loader_reader.h +++ b/userspace/engine/rule_loader_reader.h @@ -32,9 +32,6 @@ limitations under the License. // 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')." -// Warning message used when the `required_engine_version` is not semver compatible. -#define WARNING_ENGINE_VERSION_NOT_SEMVER "The 'required_engine_version' should be SemVer compatible. All non-SemVer compatible values are deprecated." - namespace rule_loader { From 50c8be6dc2ed62f2347b285e58f67d6971a35745 Mon Sep 17 00:00:00 2001 From: Andrea Terzolo Date: Wed, 10 Jan 2024 11:43:30 +0100 Subject: [PATCH 10/19] doc: typo in the exception Signed-off-by: Andrea Terzolo --- userspace/engine/falco_engine.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/userspace/engine/falco_engine.cpp b/userspace/engine/falco_engine.cpp index 189efa41adc..b9254ba38f6 100644 --- a/userspace/engine/falco_engine.cpp +++ b/userspace/engine/falco_engine.cpp @@ -491,7 +491,7 @@ nlohmann::json falco_engine::describe_rule(std::string *rule, const std::vector< // output of rules, macros, and lists. if (m_last_compile_output == nullptr) { - throw falco_exception("rules most be loaded before describing them"); + throw falco_exception("rules must be loaded before describing them"); } // use collected and compiled info to print a json output From 616b8a01f4747fc2bb3c6307b98d836fa97261a6 Mon Sep 17 00:00:00 2001 From: Andrea Terzolo Date: Wed, 10 Jan 2024 11:49:07 +0100 Subject: [PATCH 11/19] tests: add some new tests about list order Signed-off-by: Andrea Terzolo --- unit_tests/engine/test_rule_loader.cpp | 159 +++++++++++++++++++++++-- 1 file changed, 151 insertions(+), 8 deletions(-) diff --git a/unit_tests/engine/test_rule_loader.cpp b/unit_tests/engine/test_rule_loader.cpp index 1b11a2aed4d..c129261b7ee 100644 --- a/unit_tests/engine/test_rule_loader.cpp +++ b/unit_tests/engine/test_rule_loader.cpp @@ -348,21 +348,164 @@ TEST_F(engine_loader_test, rule_incorrect_append_override) ASSERT_TRUE(check_error_message(OVERRIDE_APPEND_ERROR_MESSAGE)); } -TEST_F(engine_loader_test, rule_override_without_rule) +/// todo: MACRO order + +/// todo: RULES order + + +// TEST_F(engine_loader_test, rule_append_before_rule) +// { +// std::string rules_content = R"END( +// - rule: test_rule +// condition: and proc.name = cat +// append: true +// output: user=%user.name command=%proc.cmdline priority: INFO + +// - rule: test_rule +// desc: simple rule +// condition: evt.type=clone +// )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, list_override_typo) { + // todo: maybe we want to manage in someway not existent keys + // Please note the typo `overridde` in the first list definition. std::string rules_content = R"END( -- rule: failing_rule - desc: an appended field - condition: and proc.name = cat +- list: dev_creation_binaries + items: ["csi-provisioner", "csi-attacher"] + overridde: + items: append + +- list: dev_creation_binaries + items: [blkid] + +- rule: test_rule + desc: simple rule + condition: evt.type = execve and proc.name in (dev_creation_binaries) + output: command=%proc.cmdline + priority: INFO + +)END"; + + // Since there is a typo in the first list definition the `override` is not + // considered. so in this situation, we are defining the list 2 times. The + // second one overrides the first one. + ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); + std::string rule_name = "test_rule"; + auto rule_description = m_engine->describe_rule(&rule_name, {}); + + ASSERT_EQ(rule_description["rules"][0]["info"]["condition"].template get(), + "evt.type = execve and proc.name in (dev_creation_binaries)"); + + ASSERT_EQ(rule_description["rules"][0]["details"]["condition_compiled"].template get(), + "(evt.type = execve and proc.name in (blkid))"); +} + +TEST_F(engine_loader_test, list_override_before_list_definition) +{ + std::string rules_content = R"END( +- list: dev_creation_binaries + items: ["csi-provisioner", "csi-attacher"] override: - desc: replace - condition: append + items: append + +- list: dev_creation_binaries + items: [blkid] + +- rule: test_rule + desc: simple rule + condition: evt.type = execve and proc.name in (dev_creation_binaries) + output: command=%proc.cmdline + priority: INFO + )END"; - std::string rule_name = "failing_rule"; + // We cannot define a list override before the list definition. + ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); + ASSERT_TRUE(check_error_message("List has 'append' key but no list by that name already exists")); +} + +TEST_F(engine_loader_test, list_append_before_list_definition) +{ + std::string rules_content = R"END( +- list: dev_creation_binaries + items: ["csi-provisioner", "csi-attacher"] + append: true + +- list: dev_creation_binaries + items: [blkid] + +- rule: test_rule + desc: simple rule + condition: evt.type = execve and proc.name in (dev_creation_binaries) + output: command=%proc.cmdline + priority: INFO +)END"; + + // We cannot define a list append before the list definition. ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); - ASSERT_TRUE(check_error_message("no rule by that name already exists")); + ASSERT_TRUE(check_error_message("List has 'append' key but no list by that name already exists")); +} + +TEST_F(engine_loader_test, list_override_after_list_definition) +{ + std::string rules_content = R"END( +- list: dev_creation_binaries + items: [blkid] + +- list: dev_creation_binaries + items: ["csi-provisioner", "csi-attacher"] + override: + items: append + +- rule: test_rule + desc: simple rule + condition: evt.type = execve and proc.name in (dev_creation_binaries) + output: command=%proc.cmdline + priority: INFO + +)END"; + + ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); + + 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(), + "(evt.type = execve and proc.name in (blkid, csi-provisioner, csi-attacher))"); +} + +TEST_F(engine_loader_test, list_append_after_list_definition) +{ + std::string rules_content = R"END( +- list: dev_creation_binaries + items: [blkid] + +- list: dev_creation_binaries + items: ["csi-provisioner", "csi-attacher"] + append: true + +- rule: test_rule + desc: simple rule + condition: evt.type = execve and proc.name in (dev_creation_binaries) + output: command=%proc.cmdline + priority: INFO +)END"; + + ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); + + 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(), + "(evt.type = execve and proc.name in (blkid, csi-provisioner, csi-attacher))"); } TEST_F(engine_loader_test, rule_override_without_field) From 867eea569e97b8cf71488b38a1f3e27defb89698 Mon Sep 17 00:00:00 2001 From: Andrea Terzolo Date: Wed, 10 Jan 2024 15:01:56 +0100 Subject: [PATCH 12/19] tests: add test on the order for macro and rules Signed-off-by: Andrea Terzolo --- unit_tests/engine/test_rule_loader.cpp | 203 ++++++++++++++++++++++--- 1 file changed, 182 insertions(+), 21 deletions(-) diff --git a/unit_tests/engine/test_rule_loader.cpp b/unit_tests/engine/test_rule_loader.cpp index c129261b7ee..34595218cb0 100644 --- a/unit_tests/engine/test_rule_loader.cpp +++ b/unit_tests/engine/test_rule_loader.cpp @@ -348,32 +348,193 @@ TEST_F(engine_loader_test, rule_incorrect_append_override) ASSERT_TRUE(check_error_message(OVERRIDE_APPEND_ERROR_MESSAGE)); } -/// todo: MACRO order +TEST_F(engine_loader_test, macro_override_append_before_macro_definition) +{ + std::string rules_content = R"END( + +- macro: open_simple + condition: or evt.type = openat2 + override: + condition: append + +- macro: open_simple + condition: evt.type in (open,openat) + +- rule: test_rule + desc: simple rule + condition: open_simple + output: command=%proc.cmdline + priority: INFO + +)END"; + + // We cannot define a macro override before the macro definition. + ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); + ASSERT_TRUE(check_error_message("Macro has 'append' key but no macro by that name already exists")); +} + +TEST_F(engine_loader_test, macro_append_before_macro_definition) +{ + std::string rules_content = R"END( + +- macro: open_simple + condition: or evt.type = openat2 + append: true + +- macro: open_simple + condition: evt.type in (open,openat) + +- rule: test_rule + desc: simple rule + condition: open_simple + output: command=%proc.cmdline + priority: INFO + +)END"; + + // We cannot define a macro override before the macro definition. + ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); + ASSERT_TRUE(check_error_message("Macro has 'append' key but no macro by that name already exists")); +} + +TEST_F(engine_loader_test, macro_override_append_after_macro_definition) +{ + std::string rules_content = R"END( + +- macro: open_simple + condition: evt.type in (open,openat) + +- macro: open_simple + condition: or evt.type = openat2 + override: + condition: append + +- rule: test_rule + desc: simple rule + condition: open_simple + output: command=%proc.cmdline + priority: INFO + +)END"; + + // We cannot define a macro override before the macro definition. + ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); + 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(), + "(evt.type in (open, openat) or evt.type = openat2)"); +} -/// todo: RULES order +TEST_F(engine_loader_test, macro_append_after_macro_definition) +{ + std::string rules_content = R"END( +- macro: open_simple + condition: evt.type in (open,openat) -// TEST_F(engine_loader_test, rule_append_before_rule) -// { -// std::string rules_content = R"END( -// - rule: test_rule -// condition: and proc.name = cat -// append: true -// output: user=%user.name command=%proc.cmdline priority: INFO +- macro: open_simple + condition: or evt.type = openat2 + append: true -// - rule: test_rule -// desc: simple rule -// condition: evt.type=clone -// )END"; +- rule: test_rule + desc: simple rule + condition: open_simple + output: command=%proc.cmdline + priority: INFO -// 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); -// } +)END"; + // We cannot define a macro override before the macro definition. + ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); + 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(), + "(evt.type in (open, openat) or evt.type = openat2)"); +} + +TEST_F(engine_loader_test, rule_override_append_before_rule_definition) +{ + std::string rules_content = R"END( +- rule: test_rule + condition: and proc.name = cat + override: + condition: append + +- rule: test_rule + desc: simple rule + condition: evt.type in (open,openat) + output: command=%proc.cmdline + priority: INFO + +)END"; + + ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); + ASSERT_TRUE(check_error_message("Rule has 'append' key but no rule by that name already exists")); +} + +TEST_F(engine_loader_test, rule_append_before_rule_definition) +{ + std::string rules_content = R"END( +- rule: test_rule + condition: and proc.name = cat + append: true + +- rule: test_rule + desc: simple rule + condition: evt.type in (open,openat) + output: command=%proc.cmdline + priority: INFO + +)END"; + + ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); + ASSERT_TRUE(check_error_message("Rule has 'append' key but no rule by that name already exists")); +} + +TEST_F(engine_loader_test, rule_override_append_after_rule_definition) +{ + std::string rules_content = R"END( +- rule: test_rule + desc: simple rule + condition: evt.type in (open,openat) + output: command=%proc.cmdline + priority: INFO + +- rule: test_rule + condition: and proc.name = cat + override: + condition: append +)END"; + + ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); + 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(), + "(evt.type in (open, openat) and proc.name = cat)"); +} + +TEST_F(engine_loader_test, rule_append_after_rule_definition) +{ + std::string rules_content = R"END( +- rule: test_rule + desc: simple rule + condition: evt.type in (open,openat) + output: command=%proc.cmdline + priority: INFO + +- rule: test_rule + condition: and proc.name = cat + append: true +)END"; + + ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); + 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(), + "(evt.type in (open, openat) and proc.name = cat)"); +} -TEST_F(engine_loader_test, list_override_typo) +TEST_F(engine_loader_test, list_override_append_typo) { // todo: maybe we want to manage in someway not existent keys // Please note the typo `overridde` in the first list definition. @@ -408,7 +569,7 @@ TEST_F(engine_loader_test, list_override_typo) "(evt.type = execve and proc.name in (blkid))"); } -TEST_F(engine_loader_test, list_override_before_list_definition) +TEST_F(engine_loader_test, list_override_append_before_list_definition) { std::string rules_content = R"END( - list: dev_creation_binaries @@ -455,7 +616,7 @@ TEST_F(engine_loader_test, list_append_before_list_definition) ASSERT_TRUE(check_error_message("List has 'append' key but no list by that name already exists")); } -TEST_F(engine_loader_test, list_override_after_list_definition) +TEST_F(engine_loader_test, list_override_append_after_list_definition) { std::string rules_content = R"END( - list: dev_creation_binaries From e537cd8a0e7778511843f74ce922532665cd7d3a Mon Sep 17 00:00:00 2001 From: Andrea Terzolo Date: Wed, 10 Jan 2024 15:17:14 +0100 Subject: [PATCH 13/19] cleanup: use macros for default error messages Signed-off-by: Andrea Terzolo --- unit_tests/engine/test_rule_loader.cpp | 13 +++++++------ userspace/engine/rule_loader_collector.cpp | 14 +++----------- userspace/engine/rule_loader_reader.cpp | 1 + userspace/engine/rule_loader_reader.h | 9 --------- userspace/engine/rule_loading_messages.h | 16 ++++++++++++++++ 5 files changed, 27 insertions(+), 26 deletions(-) create mode 100644 userspace/engine/rule_loading_messages.h diff --git a/unit_tests/engine/test_rule_loader.cpp b/unit_tests/engine/test_rule_loader.cpp index 34595218cb0..f5fb40a4b3e 100644 --- a/unit_tests/engine/test_rule_loader.cpp +++ b/unit_tests/engine/test_rule_loader.cpp @@ -3,6 +3,7 @@ #include "falco_engine.h" #include "rule_loader_reader.h" #include "rule_loader_compiler.h" +#include "rule_loading_messages.h" class engine_loader_test : public ::testing::Test { protected: @@ -370,7 +371,7 @@ TEST_F(engine_loader_test, macro_override_append_before_macro_definition) // We cannot define a macro override before the macro definition. ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); - ASSERT_TRUE(check_error_message("Macro has 'append' key but no macro by that name already exists")); + ASSERT_TRUE(check_error_message(ERROR_NO_PREVIOUS_MACRO)); } TEST_F(engine_loader_test, macro_append_before_macro_definition) @@ -394,7 +395,7 @@ TEST_F(engine_loader_test, macro_append_before_macro_definition) // We cannot define a macro override before the macro definition. ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); - ASSERT_TRUE(check_error_message("Macro has 'append' key but no macro by that name already exists")); + ASSERT_TRUE(check_error_message(ERROR_NO_PREVIOUS_MACRO)); } TEST_F(engine_loader_test, macro_override_append_after_macro_definition) @@ -469,7 +470,7 @@ TEST_F(engine_loader_test, rule_override_append_before_rule_definition) )END"; ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); - ASSERT_TRUE(check_error_message("Rule has 'append' key but no rule by that name already exists")); + ASSERT_TRUE(check_error_message(ERROR_NO_PREVIOUS_RULE)); } TEST_F(engine_loader_test, rule_append_before_rule_definition) @@ -488,7 +489,7 @@ TEST_F(engine_loader_test, rule_append_before_rule_definition) )END"; ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); - ASSERT_TRUE(check_error_message("Rule has 'append' key but no rule by that name already exists")); + ASSERT_TRUE(check_error_message(ERROR_NO_PREVIOUS_RULE)); } TEST_F(engine_loader_test, rule_override_append_after_rule_definition) @@ -590,7 +591,7 @@ TEST_F(engine_loader_test, list_override_append_before_list_definition) // We cannot define a list override before the list definition. ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); - ASSERT_TRUE(check_error_message("List has 'append' key but no list by that name already exists")); + ASSERT_TRUE(check_error_message(ERROR_NO_PREVIOUS_LIST)); } TEST_F(engine_loader_test, list_append_before_list_definition) @@ -613,7 +614,7 @@ TEST_F(engine_loader_test, list_append_before_list_definition) // We cannot define a list append before the list definition. ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); - ASSERT_TRUE(check_error_message("List has 'append' key but no list by that name already exists")); + ASSERT_TRUE(check_error_message(ERROR_NO_PREVIOUS_LIST)); } TEST_F(engine_loader_test, list_override_append_after_list_definition) diff --git a/userspace/engine/rule_loader_collector.cpp b/userspace/engine/rule_loader_collector.cpp index a3e699f8f1a..9c8b31c9868 100644 --- a/userspace/engine/rule_loader_collector.cpp +++ b/userspace/engine/rule_loader_collector.cpp @@ -190,10 +190,7 @@ void rule_loader::collector::define(configuration& cfg, list_info& info) void rule_loader::collector::append(configuration& cfg, list_info& info) { auto prev = m_list_infos.at(info.name); - THROW(!prev, - // "List has 'append' key or an append override but no list by that name already exists", // TODO update this error and update testing - "List has 'append' key but no list by that name already exists", - info.ctx); + THROW(!prev, ERROR_NO_PREVIOUS_LIST, info.ctx); prev->items.insert(prev->items.end(), info.items.begin(), info.items.end()); append_info(prev, info, m_cur_index++); } @@ -206,9 +203,7 @@ void rule_loader::collector::define(configuration& cfg, macro_info& info) void rule_loader::collector::append(configuration& cfg, macro_info& info) { auto prev = m_macro_infos.at(info.name); - THROW(!prev, - "Macro has 'append' key but no macro by that name already exists", - info.ctx); + THROW(!prev, ERROR_NO_PREVIOUS_MACRO, info.ctx); prev->cond += " "; prev->cond += info.cond; append_info(prev, info, m_cur_index++); @@ -244,10 +239,7 @@ void rule_loader::collector::append(configuration& cfg, rule_update_info& info) { auto prev = m_rule_infos.at(info.name); - THROW(!prev, - // "Rule has 'append' key or an append override but no rule by that name already exists", // TODO replace with this and update testing - "Rule has 'append' key but no rule by that name already exists", - info.ctx); + THROW(!prev, ERROR_NO_PREVIOUS_RULE, info.ctx); THROW(!info.has_any_value(), "Appended rule must have exceptions or condition property", // "Appended rule must have at least one field that can be appended to", // TODO replace with this and update testing diff --git a/userspace/engine/rule_loader_reader.cpp b/userspace/engine/rule_loader_reader.cpp index 0b7e1a64c49..3210e878cfd 100644 --- a/userspace/engine/rule_loader_reader.cpp +++ b/userspace/engine/rule_loader_reader.cpp @@ -22,6 +22,7 @@ limitations under the License. #include "rule_loader_reader.h" #include "falco_engine_version.h" +#include "rule_loading_messages.h" #define THROW(cond, err, ctx) { if ((cond)) { throw rule_loader::rule_load_exception(falco::load_result::LOAD_ERR_YAML_VALIDATE, (err), (ctx)); } } diff --git a/userspace/engine/rule_loader_reader.h b/userspace/engine/rule_loader_reader.h index 877f9d451b5..378556c1dbf 100644 --- a/userspace/engine/rule_loader_reader.h +++ b/userspace/engine/rule_loader_reader.h @@ -23,15 +23,6 @@ limitations under the License. #include "version.h" #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." - -// 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." - -// 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 { diff --git a/userspace/engine/rule_loading_messages.h b/userspace/engine/rule_loading_messages.h new file mode 100644 index 00000000000..a0e62ac4051 --- /dev/null +++ b/userspace/engine/rule_loading_messages.h @@ -0,0 +1,16 @@ +#pragma once + +// Error message used when both 'override' and 'append' keys 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." + +// Warning message used when 'append' key is used. +#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' key. +#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')." + +#define ERROR_NO_PREVIOUS_MACRO "Macro uses 'append' or 'override.condition: append' but no macro by that name already exists" + +#define ERROR_NO_PREVIOUS_LIST "List uses 'append' or 'override.items: append' but no list by that name already exists" + +#define ERROR_NO_PREVIOUS_RULE "Rule uses 'append' or 'override.: append' but no rule by that name already exists" From 04bd07886ed90a0165af9efc8580c05fcbd2c5ab Mon Sep 17 00:00:00 2001 From: Andrea Terzolo Date: Wed, 10 Jan 2024 15:36:38 +0100 Subject: [PATCH 14/19] tests: add some new tests on override replace Signed-off-by: Andrea Terzolo --- unit_tests/engine/test_rule_loader.cpp | 79 +++++++++++++++++++++- userspace/engine/rule_loader_collector.cpp | 7 +- userspace/engine/rule_loading_messages.h | 8 +-- 3 files changed, 84 insertions(+), 10 deletions(-) diff --git a/unit_tests/engine/test_rule_loader.cpp b/unit_tests/engine/test_rule_loader.cpp index f5fb40a4b3e..042f55d09c7 100644 --- a/unit_tests/engine/test_rule_loader.cpp +++ b/unit_tests/engine/test_rule_loader.cpp @@ -374,6 +374,34 @@ TEST_F(engine_loader_test, macro_override_append_before_macro_definition) ASSERT_TRUE(check_error_message(ERROR_NO_PREVIOUS_MACRO)); } +TEST_F(engine_loader_test, macro_override_replace_before_macro_definition) +{ + std::string rules_content = R"END( + +- macro: open_simple + condition: or evt.type = openat2 + override: + condition: replace + +- macro: open_simple + condition: evt.type in (open,openat) + +- rule: test_rule + desc: simple rule + condition: open_simple + output: command=%proc.cmdline + priority: INFO + +)END"; + + // The first override defines a macro that is overridden by the second macro definition + ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); + 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(), + "evt.type in (open, openat)"); +} + TEST_F(engine_loader_test, macro_append_before_macro_definition) { std::string rules_content = R"END( @@ -470,7 +498,27 @@ TEST_F(engine_loader_test, rule_override_append_before_rule_definition) )END"; ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); - ASSERT_TRUE(check_error_message(ERROR_NO_PREVIOUS_RULE)); + ASSERT_TRUE(check_error_message(ERROR_NO_PREVIOUS_RULE_APPEND)); +} + +TEST_F(engine_loader_test, rule_override_replace_before_rule_definition) +{ + std::string rules_content = R"END( +- rule: test_rule + condition: and proc.name = cat + override: + condition: replace + +- rule: test_rule + desc: simple rule + condition: evt.type in (open,openat) + output: command=%proc.cmdline + priority: INFO + +)END"; + + ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); + ASSERT_TRUE(check_error_message(ERROR_NO_PREVIOUS_RULE_REPLACE)); } TEST_F(engine_loader_test, rule_append_before_rule_definition) @@ -489,7 +537,7 @@ TEST_F(engine_loader_test, rule_append_before_rule_definition) )END"; ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); - ASSERT_TRUE(check_error_message(ERROR_NO_PREVIOUS_RULE)); + ASSERT_TRUE(check_error_message(ERROR_NO_PREVIOUS_RULE_APPEND)); } TEST_F(engine_loader_test, rule_override_append_after_rule_definition) @@ -594,6 +642,33 @@ TEST_F(engine_loader_test, list_override_append_before_list_definition) ASSERT_TRUE(check_error_message(ERROR_NO_PREVIOUS_LIST)); } +TEST_F(engine_loader_test, list_override_replace_before_list_definition) +{ + std::string rules_content = R"END( +- list: dev_creation_binaries + items: ["csi-provisioner", "csi-attacher"] + override: + items: replace + +- list: dev_creation_binaries + items: [blkid] + +- rule: test_rule + desc: simple rule + condition: evt.type = execve and proc.name in (dev_creation_binaries) + output: command=%proc.cmdline + priority: INFO + +)END"; + + // With override replace we define a first list that then is overridden by the second one. + ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); + 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(), + "(evt.type = execve and proc.name in (blkid))"); +} + TEST_F(engine_loader_test, list_append_before_list_definition) { std::string rules_content = R"END( diff --git a/userspace/engine/rule_loader_collector.cpp b/userspace/engine/rule_loader_collector.cpp index 9c8b31c9868..fdeef81e308 100644 --- a/userspace/engine/rule_loader_collector.cpp +++ b/userspace/engine/rule_loader_collector.cpp @@ -20,6 +20,7 @@ limitations under the License. #include "falco_engine.h" #include "rule_loader_collector.h" +#include "rule_loading_messages.h" #define THROW(cond, err, ctx) { if ((cond)) { throw rule_loader::rule_load_exception(falco::load_result::LOAD_ERR_VALIDATE, (err), (ctx)); } } @@ -239,7 +240,7 @@ void rule_loader::collector::append(configuration& cfg, rule_update_info& info) { auto prev = m_rule_infos.at(info.name); - THROW(!prev, ERROR_NO_PREVIOUS_RULE, info.ctx); + THROW(!prev, ERROR_NO_PREVIOUS_RULE_APPEND, info.ctx); THROW(!info.has_any_value(), "Appended rule must have exceptions or condition property", // "Appended rule must have at least one field that can be appended to", // TODO replace with this and update testing @@ -322,9 +323,7 @@ void rule_loader::collector::selective_replace(configuration& cfg, rule_update_i { auto prev = m_rule_infos.at(info.name); - THROW(!prev, - "An replace to a rule was requested but no rule by that name already exists", - info.ctx); + THROW(!prev, ERROR_NO_PREVIOUS_RULE_REPLACE, info.ctx); THROW(!info.has_any_value(), "The rule must have at least one field that can be replaced", info.ctx); diff --git a/userspace/engine/rule_loading_messages.h b/userspace/engine/rule_loading_messages.h index a0e62ac4051..18476b64f78 100644 --- a/userspace/engine/rule_loading_messages.h +++ b/userspace/engine/rule_loading_messages.h @@ -1,16 +1,16 @@ #pragma once -// Error message used when both 'override' and 'append' keys are specified. +// todo: rename putting error at the beginning #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' key is used. #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' key. #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')." #define ERROR_NO_PREVIOUS_MACRO "Macro uses 'append' or 'override.condition: append' but no macro by that name already exists" #define ERROR_NO_PREVIOUS_LIST "List uses 'append' or 'override.items: append' but no list by that name already exists" -#define ERROR_NO_PREVIOUS_RULE "Rule uses 'append' or 'override.: append' but no rule by that name already exists" +#define ERROR_NO_PREVIOUS_RULE_APPEND "Rule uses 'append' or 'override.: append' but no rule by that name already exists" + +#define ERROR_NO_PREVIOUS_RULE_REPLACE "An 'override.: replace' to a rule was requested but no rule by that name already exists" From 723dfd7319916ea187fe768aeeff133203de739a Mon Sep 17 00:00:00 2001 From: Andrea Terzolo Date: Wed, 10 Jan 2024 15:40:14 +0100 Subject: [PATCH 15/19] cleanup: rename some error messages Signed-off-by: Andrea Terzolo --- unit_tests/engine/test_rule_loader.cpp | 10 +++++----- userspace/engine/rule_loader_reader.cpp | 14 +++++++------- userspace/engine/rule_loading_messages.h | 15 +++++++++++---- 3 files changed, 23 insertions(+), 16 deletions(-) diff --git a/unit_tests/engine/test_rule_loader.cpp b/unit_tests/engine/test_rule_loader.cpp index 042f55d09c7..74d1696d4cb 100644 --- a/unit_tests/engine/test_rule_loader.cpp +++ b/unit_tests/engine/test_rule_loader.cpp @@ -190,7 +190,7 @@ TEST_F(engine_loader_test, rule_override_append) ASSERT_TRUE(load_rules(rules_content, "legit_rules.yaml")) << m_load_result_string; // Here we don't use the deprecated `append` flag, so we don't expect the warning. - ASSERT_FALSE(check_warning_message(WARNING_APPEND_MESSAGE)); + ASSERT_FALSE(check_warning_message(WARNING_APPEND)); auto rule_description = m_engine->describe_rule(&rule_name, {}); ASSERT_EQ(rule_description["rules"][0]["info"]["condition"].template get(), @@ -221,7 +221,7 @@ TEST_F(engine_loader_test, rule_append) ASSERT_TRUE(load_rules(rules_content, "legit_rules.yaml")) << m_load_result_string; // We should have at least one warning because the 'append' flag is deprecated. - ASSERT_TRUE(check_warning_message(WARNING_APPEND_MESSAGE)); + ASSERT_TRUE(check_warning_message(WARNING_APPEND)); auto rule_description = m_engine->describe_rule(&rule_name, {}); ASSERT_EQ(rule_description["rules"][0]["details"]["condition_compiled"].template get(), @@ -344,9 +344,9 @@ TEST_F(engine_loader_test, rule_incorrect_append_override) ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); // We should have at least one warning because the 'append' flag is deprecated. - ASSERT_TRUE(check_warning_message(WARNING_APPEND_MESSAGE)); + ASSERT_TRUE(check_warning_message(WARNING_APPEND)); - ASSERT_TRUE(check_error_message(OVERRIDE_APPEND_ERROR_MESSAGE)); + ASSERT_TRUE(check_error_message(ERROR_OVERRIDE_APPEND)); } TEST_F(engine_loader_test, macro_override_append_before_macro_definition) @@ -872,7 +872,7 @@ TEST_F(engine_loader_test, rule_enabled_warning) )END"; ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); - ASSERT_TRUE(check_warning_message(WARNING_ENABLED_MESSAGE)); + ASSERT_TRUE(check_warning_message(WARNING_ENABLED)); // The rule should be enabled at the end. EXPECT_EQ(num_rules_for_ruleset(), 1); } diff --git a/userspace/engine/rule_loader_reader.cpp b/userspace/engine/rule_loader_reader.cpp index 3210e878cfd..57ab8455a52 100644 --- a/userspace/engine/rule_loader_reader.cpp +++ b/userspace/engine/rule_loader_reader.cpp @@ -442,7 +442,7 @@ static void read_item( decode_optional_val(item, "append", has_append_flag, ctx); if(has_append_flag) { - cfg.res->add_warning(falco::load_result::LOAD_DEPRECATED_ITEM, WARNING_APPEND_MESSAGE, ctx); + cfg.res->add_warning(falco::load_result::LOAD_DEPRECATED_ITEM, WARNING_APPEND, ctx); } std::set override_append, override_replace; @@ -450,7 +450,7 @@ static void read_item( decode_overrides(item, overridable, overridable, override_append, override_replace, ctx); bool has_overrides = !override_append.empty() || !override_replace.empty(); - THROW(has_append_flag && has_overrides, OVERRIDE_APPEND_ERROR_MESSAGE, ctx); + THROW(has_append_flag && has_overrides, ERROR_OVERRIDE_APPEND, ctx); // Since a list only has items, if we have chosen to append them we can append the entire object // otherwise we just want to redefine the list. @@ -485,7 +485,7 @@ static void read_item( decode_optional_val(item, "append", has_append_flag, ctx); if(has_append_flag) { - cfg.res->add_warning(falco::load_result::LOAD_DEPRECATED_ITEM, WARNING_APPEND_MESSAGE, ctx); + cfg.res->add_warning(falco::load_result::LOAD_DEPRECATED_ITEM, WARNING_APPEND, ctx); } std::set override_append, override_replace; @@ -493,7 +493,7 @@ static void read_item( decode_overrides(item, overridable, overridable, override_append, override_replace, ctx); bool has_overrides = !override_append.empty() || !override_replace.empty(); - THROW((has_append_flag && has_overrides), OVERRIDE_APPEND_ERROR_MESSAGE, ctx); + THROW((has_append_flag && has_overrides), ERROR_OVERRIDE_APPEND, ctx); // Since a macro only has a condition, if we have chosen to append to it we can append the entire object // otherwise we just want to redefine the macro. @@ -522,7 +522,7 @@ static void read_item( decode_optional_val(item, "append", has_append_flag, ctx); if(has_append_flag) { - cfg.res->add_warning(falco::load_result::LOAD_DEPRECATED_ITEM, WARNING_APPEND_MESSAGE, ctx); + cfg.res->add_warning(falco::load_result::LOAD_DEPRECATED_ITEM, WARNING_APPEND, ctx); } std::set override_append, override_replace; @@ -534,7 +534,7 @@ static void read_item( bool has_overrides_replace = !override_replace.empty(); bool has_overrides = has_overrides_append || has_overrides_replace; - THROW((has_append_flag && has_overrides), OVERRIDE_APPEND_ERROR_MESSAGE, ctx); + THROW((has_append_flag && has_overrides), ERROR_OVERRIDE_APPEND, ctx); if(has_overrides) { @@ -694,7 +694,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); + cfg.res->add_warning(falco::load_result::LOAD_DEPRECATED_ITEM, WARNING_ENABLED, ctx); collector.enable(cfg, v); } else diff --git a/userspace/engine/rule_loading_messages.h b/userspace/engine/rule_loading_messages.h index 18476b64f78..5962fb0a406 100644 --- a/userspace/engine/rule_loading_messages.h +++ b/userspace/engine/rule_loading_messages.h @@ -1,11 +1,18 @@ #pragma once -// todo: rename putting error at the beginning -#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." +//////////////// +// Warnings +//////////////// -#define WARNING_APPEND_MESSAGE "'append' key is deprecated. Add an 'append' entry (e.g. 'condition: append') under 'override' instead." +#define WARNING_APPEND "'append' key is deprecated. Add an 'append' entry (e.g. 'condition: append') under 'override' instead." -#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')." +#define WARNING_ENABLED "The standalone 'enabled' key usage is deprecated. The correct approach requires also a 'replace' entry under the 'override' key (i.e. 'enabled: replace')." + +//////////////// +// Errors +//////////////// + +#define ERROR_OVERRIDE_APPEND "Keys 'override' and 'append: true' cannot be used together. Add an 'append' entry (e.g. 'condition: append') under 'override' instead." #define ERROR_NO_PREVIOUS_MACRO "Macro uses 'append' or 'override.condition: append' but no macro by that name already exists" From 7f634360954f93b199b4e5e1d5dc086e450bad98 Mon Sep 17 00:00:00 2001 From: Andrea Terzolo Date: Wed, 10 Jan 2024 15:55:27 +0100 Subject: [PATCH 16/19] cleanup: adopt a new helper method in tests Signed-off-by: Andrea Terzolo --- unit_tests/engine/test_rule_loader.cpp | 84 +++++++------------------- 1 file changed, 21 insertions(+), 63 deletions(-) diff --git a/unit_tests/engine/test_rule_loader.cpp b/unit_tests/engine/test_rule_loader.cpp index 74d1696d4cb..037455aa074 100644 --- a/unit_tests/engine/test_rule_loader.cpp +++ b/unit_tests/engine/test_rule_loader.cpp @@ -98,6 +98,12 @@ class engine_loader_test : public ::testing::Test { return false; } + std::string get_compiled_rule_condition(std::string rule_name = "") + { + auto rule_description = m_engine->describe_rule(&rule_name, {}); + return rule_description["rules"][0]["details"]["condition_compiled"].template get(); + } + std::string m_sample_ruleset; std::string m_sample_source; sinsp_filter_check_list m_filterlist; @@ -131,12 +137,8 @@ TEST_F(engine_loader_test, list_append) items: append )END"; - std::string rule_name = "legit_rule"; ASSERT_TRUE(load_rules(rules_content, "legit_rules.yaml")) << m_load_result_string; - - auto rule_description = m_engine->describe_rule(&rule_name, {}); - ASSERT_EQ(rule_description["rules"][0]["details"]["condition_compiled"].template get(), - "(evt.type = open and proc.name in (ash, bash, csh, ksh, sh, tcsh, zsh, dash, pwsh))"); + ASSERT_EQ(get_compiled_rule_condition("legit_rule"),"(evt.type = open and proc.name in (ash, bash, csh, ksh, sh, tcsh, zsh, dash, pwsh))"); } TEST_F(engine_loader_test, condition_append) @@ -159,12 +161,8 @@ TEST_F(engine_loader_test, condition_append) condition: append )END"; - std::string rule_name = "legit_rule"; ASSERT_TRUE(load_rules(rules_content, "legit_rules.yaml")) << m_load_result_string; - - auto rule_description = m_engine->describe_rule(&rule_name, {}); - ASSERT_EQ(rule_description["rules"][0]["details"]["condition_compiled"].template get(), - "(evt.type = open and (((proc.aname = sshd and proc.name != sshd) or proc.name = systemd-logind or proc.name = login) or proc.name = ssh))"); + ASSERT_EQ(get_compiled_rule_condition("legit_rule"),"(evt.type = open and (((proc.aname = sshd and proc.name != sshd) or proc.name = systemd-logind or proc.name = login) or proc.name = ssh))"); } TEST_F(engine_loader_test, rule_override_append) @@ -217,15 +215,12 @@ TEST_F(engine_loader_test, rule_append) append: true )END"; - std::string rule_name = "legit_rule"; ASSERT_TRUE(load_rules(rules_content, "legit_rules.yaml")) << m_load_result_string; // We should have at least one warning because the 'append' flag is deprecated. ASSERT_TRUE(check_warning_message(WARNING_APPEND)); - auto rule_description = m_engine->describe_rule(&rule_name, {}); - ASSERT_EQ(rule_description["rules"][0]["details"]["condition_compiled"].template get(), - "(evt.type = open and proc.name = cat)"); + ASSERT_EQ(get_compiled_rule_condition("legit_rule"),"(evt.type = open and proc.name = cat)"); } TEST_F(engine_loader_test, rule_override_replace) @@ -396,10 +391,7 @@ TEST_F(engine_loader_test, macro_override_replace_before_macro_definition) // The first override defines a macro that is overridden by the second macro definition ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); - 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(), - "evt.type in (open, openat)"); + ASSERT_EQ(get_compiled_rule_condition("test_rule"),"evt.type in (open, openat)"); } TEST_F(engine_loader_test, macro_append_before_macro_definition) @@ -448,10 +440,7 @@ TEST_F(engine_loader_test, macro_override_append_after_macro_definition) // We cannot define a macro override before the macro definition. ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); - 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(), - "(evt.type in (open, openat) or evt.type = openat2)"); + ASSERT_EQ(get_compiled_rule_condition("test_rule"),"(evt.type in (open, openat) or evt.type = openat2)"); } TEST_F(engine_loader_test, macro_append_after_macro_definition) @@ -475,10 +464,7 @@ TEST_F(engine_loader_test, macro_append_after_macro_definition) // We cannot define a macro override before the macro definition. ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); - 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(), - "(evt.type in (open, openat) or evt.type = openat2)"); + ASSERT_EQ(get_compiled_rule_condition("test_rule"),"(evt.type in (open, openat) or evt.type = openat2)"); } TEST_F(engine_loader_test, rule_override_append_before_rule_definition) @@ -556,10 +542,7 @@ TEST_F(engine_loader_test, rule_override_append_after_rule_definition) )END"; ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); - 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(), - "(evt.type in (open, openat) and proc.name = cat)"); + ASSERT_EQ(get_compiled_rule_condition("test_rule"),"(evt.type in (open, openat) and proc.name = cat)"); } TEST_F(engine_loader_test, rule_append_after_rule_definition) @@ -577,16 +560,13 @@ TEST_F(engine_loader_test, rule_append_after_rule_definition) )END"; ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); - 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(), - "(evt.type in (open, openat) and proc.name = cat)"); + ASSERT_EQ(get_compiled_rule_condition("test_rule"),"(evt.type in (open, openat) and proc.name = cat)"); } TEST_F(engine_loader_test, list_override_append_typo) { - // todo: maybe we want to manage in someway not existent keys - // Please note the typo `overridde` in the first list definition. + // todo: maybe we want to manage some non-existent keys + // Please note the typo in `override` in the first list definition. std::string rules_content = R"END( - list: dev_creation_binaries items: ["csi-provisioner", "csi-attacher"] @@ -608,14 +588,7 @@ TEST_F(engine_loader_test, list_override_append_typo) // considered. so in this situation, we are defining the list 2 times. The // second one overrides the first one. ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); - std::string rule_name = "test_rule"; - auto rule_description = m_engine->describe_rule(&rule_name, {}); - - ASSERT_EQ(rule_description["rules"][0]["info"]["condition"].template get(), - "evt.type = execve and proc.name in (dev_creation_binaries)"); - - ASSERT_EQ(rule_description["rules"][0]["details"]["condition_compiled"].template get(), - "(evt.type = execve and proc.name in (blkid))"); + ASSERT_EQ(get_compiled_rule_condition("test_rule"),"(evt.type = execve and proc.name in (blkid))"); } TEST_F(engine_loader_test, list_override_append_before_list_definition) @@ -663,10 +636,7 @@ TEST_F(engine_loader_test, list_override_replace_before_list_definition) // With override replace we define a first list that then is overridden by the second one. ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); - 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(), - "(evt.type = execve and proc.name in (blkid))"); + ASSERT_EQ(get_compiled_rule_condition("test_rule"),"(evt.type = execve and proc.name in (blkid))"); } TEST_F(engine_loader_test, list_append_before_list_definition) @@ -712,11 +682,7 @@ TEST_F(engine_loader_test, list_override_append_after_list_definition) )END"; ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); - - 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(), - "(evt.type = execve and proc.name in (blkid, csi-provisioner, csi-attacher))"); + ASSERT_EQ(get_compiled_rule_condition("test_rule"),"(evt.type = execve and proc.name in (blkid, csi-provisioner, csi-attacher))"); } TEST_F(engine_loader_test, list_append_after_list_definition) @@ -737,12 +703,7 @@ TEST_F(engine_loader_test, list_append_after_list_definition) )END"; ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); - - 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(), - "(evt.type = execve and proc.name in (blkid, csi-provisioner, csi-attacher))"); + ASSERT_EQ(get_compiled_rule_condition("test_rule"),"(evt.type = execve and proc.name in (blkid, csi-provisioner, csi-attacher))"); } TEST_F(engine_loader_test, rule_override_without_field) @@ -924,10 +885,7 @@ TEST_F(engine_loader_test, rewrite_rule) 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"); + ASSERT_EQ(get_compiled_rule_condition("test_rule"),"proc.name = cat"); } TEST_F(engine_loader_test, required_engine_version_semver) From a9d3334ed5510e54b5e01a9deccc628e6e838acb Mon Sep 17 00:00:00 2001 From: Andrea Terzolo Date: Wed, 10 Jan 2024 16:06:04 +0100 Subject: [PATCH 17/19] fix codespell Signed-off-by: Andrea Terzolo --- unit_tests/engine/test_rule_loader.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/unit_tests/engine/test_rule_loader.cpp b/unit_tests/engine/test_rule_loader.cpp index 037455aa074..a6b7d1da2eb 100644 --- a/unit_tests/engine/test_rule_loader.cpp +++ b/unit_tests/engine/test_rule_loader.cpp @@ -563,14 +563,14 @@ TEST_F(engine_loader_test, rule_append_after_rule_definition) ASSERT_EQ(get_compiled_rule_condition("test_rule"),"(evt.type in (open, openat) and proc.name = cat)"); } -TEST_F(engine_loader_test, list_override_append_typo) +TEST_F(engine_loader_test, list_override_append_wrong_key) { // todo: maybe we want to manage some non-existent keys - // Please note the typo in `override` in the first list definition. + // Please note how the non-existent key 'non-existent keys' is ignored. std::string rules_content = R"END( - list: dev_creation_binaries items: ["csi-provisioner", "csi-attacher"] - overridde: + override_written_wrong: items: append - list: dev_creation_binaries @@ -584,7 +584,7 @@ TEST_F(engine_loader_test, list_override_append_typo) )END"; - // Since there is a typo in the first list definition the `override` is not + // Since there is a wrong key in the first list definition the `override` is not // considered. so in this situation, we are defining the list 2 times. The // second one overrides the first one. ASSERT_TRUE(load_rules(rules_content, "rules.yaml")); From 9db9ecb5f060b751ab2fbec1e98b4162d2e0ca5c Mon Sep 17 00:00:00 2001 From: Andrea Terzolo Date: Wed, 10 Jan 2024 17:20:05 +0100 Subject: [PATCH 18/19] chore: bump testing submodule manually to fix e2e tests Signed-off-by: Andrea Terzolo --- submodules/falcosecurity-testing | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/submodules/falcosecurity-testing b/submodules/falcosecurity-testing index 9b9630e2d80..ae3950acf0d 160000 --- a/submodules/falcosecurity-testing +++ b/submodules/falcosecurity-testing @@ -1 +1 @@ -Subproject commit 9b9630e2d80396dc35aa61277f9252bbdb99926e +Subproject commit ae3950acf0da01836d8412c9d1e499b6bce5f042 From 0dcf972e9cf31ab26742dd729a38121cf2e3041e Mon Sep 17 00:00:00 2001 From: Andrea Terzolo Date: Thu, 11 Jan 2024 15:02:48 +0100 Subject: [PATCH 19/19] cleanup: restore the name of a variable Signed-off-by: Andrea Terzolo Co-authored-by: Luca Guerra --- userspace/engine/rule_loader_reader.cpp | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/userspace/engine/rule_loader_reader.cpp b/userspace/engine/rule_loader_reader.cpp index 57ab8455a52..abf23143707 100644 --- a/userspace/engine/rule_loader_reader.cpp +++ b/userspace/engine/rule_loader_reader.cpp @@ -435,12 +435,12 @@ static void read_item( rule_loader::context ctx(item, rule_loader::context::LIST, name, parent); rule_loader::list_info v(ctx); - bool has_append_flag = false; + bool append = false; decode_val(item, "list", v.name, ctx); decode_items(item, v.items, ctx); - decode_optional_val(item, "append", has_append_flag, ctx); - if(has_append_flag) + decode_optional_val(item, "append", append, ctx); + if(append) { cfg.res->add_warning(falco::load_result::LOAD_DEPRECATED_ITEM, WARNING_APPEND, ctx); } @@ -450,13 +450,13 @@ static void read_item( decode_overrides(item, overridable, overridable, override_append, override_replace, ctx); bool has_overrides = !override_append.empty() || !override_replace.empty(); - THROW(has_append_flag && has_overrides, ERROR_OVERRIDE_APPEND, ctx); + THROW(append && has_overrides, ERROR_OVERRIDE_APPEND, ctx); // Since a list only has items, if we have chosen to append them we can append the entire object // otherwise we just want to redefine the list. - has_append_flag |= override_append.find("items") != override_append.end(); + append |= override_append.find("items") != override_append.end(); - if(has_append_flag) + if(append) { collector.append(cfg, v); } @@ -476,14 +476,14 @@ static void read_item( rule_loader::macro_info v(ctx); v.name = name; - bool has_append_flag = false; + bool append = false; decode_val(item, "condition", v.cond, ctx); // Now set the proper context for the condition now that we know it exists v.cond_ctx = rule_loader::context(item["condition"], rule_loader::context::MACRO_CONDITION, "", ctx); - decode_optional_val(item, "append", has_append_flag, ctx); - if(has_append_flag) + decode_optional_val(item, "append", append, ctx); + if(append) { cfg.res->add_warning(falco::load_result::LOAD_DEPRECATED_ITEM, WARNING_APPEND, ctx); } @@ -493,13 +493,13 @@ static void read_item( decode_overrides(item, overridable, overridable, override_append, override_replace, ctx); bool has_overrides = !override_append.empty() || !override_replace.empty(); - THROW((has_append_flag && has_overrides), ERROR_OVERRIDE_APPEND, ctx); + THROW((append && has_overrides), ERROR_OVERRIDE_APPEND, ctx); // Since a macro only has a condition, if we have chosen to append to it we can append the entire object // otherwise we just want to redefine the macro. - has_append_flag |= override_append.find("condition") != override_append.end(); + append |= override_append.find("condition") != override_append.end(); - if(has_append_flag) + if(append) { collector.append(cfg, v); }