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 diff --git a/unit_tests/engine/test_rule_loader.cpp b/unit_tests/engine/test_rule_loader.cpp index d369d5c7798..a6b7d1da2eb 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: @@ -43,6 +44,66 @@ 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()) + { + return false; + } + + 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; + } + } + + 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 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; @@ -76,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) @@ -104,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) @@ -134,6 +187,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)); + 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"); @@ -145,7 +201,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( @@ -160,14 +215,13 @@ 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; - 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)"); -} + // We should have at least one warning because the 'append' flag is deprecated. + ASSERT_TRUE(check_warning_message(WARNING_APPEND)); + ASSERT_EQ(get_compiled_rule_condition("legit_rule"),"(evt.type = open and proc.name = cat)"); +} TEST_F(engine_loader_test, rule_override_replace) { @@ -258,7 +312,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); } @@ -283,24 +337,373 @@ 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); + + // We should have at least one warning because the 'append' flag is deprecated. + ASSERT_TRUE(check_warning_message(WARNING_APPEND)); + + ASSERT_TRUE(check_error_message(ERROR_OVERRIDE_APPEND)); } -TEST_F(engine_loader_test, rule_override_without_rule) +TEST_F(engine_loader_test, macro_override_append_before_macro_definition) { std::string rules_content = R"END( -- rule: failing_rule - desc: an appended field + +- 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(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")); + ASSERT_EQ(get_compiled_rule_condition("test_rule"),"evt.type in (open, openat)"); +} + +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(ERROR_NO_PREVIOUS_MACRO)); +} + +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")); + 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) +{ + std::string rules_content = R"END( + +- macro: open_simple + condition: evt.type in (open,openat) + +- macro: open_simple + condition: or evt.type = openat2 + append: true + +- 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")); + 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) +{ + std::string rules_content = R"END( +- rule: test_rule condition: and proc.name = cat override: - desc: replace condition: append + +- rule: test_rule + desc: simple rule + condition: evt.type in (open,openat) + output: command=%proc.cmdline + priority: INFO + )END"; - std::string rule_name = "failing_rule"; + ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); + 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) +{ + 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(ERROR_NO_PREVIOUS_RULE_APPEND)); +} + +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")); + 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) +{ + 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")); + 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_wrong_key) +{ + // todo: maybe we want to manage some non-existent keys + // 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"] + override_written_wrong: + 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 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")); + 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) +{ + std::string rules_content = R"END( +- list: dev_creation_binaries + items: ["csi-provisioner", "csi-attacher"] + override: + 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"; + + // We cannot define a list override before the list definition. + ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); + 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")); + 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) +{ + 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(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(ERROR_NO_PREVIOUS_LIST)); +} + +TEST_F(engine_loader_test, list_override_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"] + 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")); + 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) +{ + 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")); + 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) @@ -322,7 +725,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) @@ -346,5 +749,195 @@ 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) +{ + 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)); + // 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); + ASSERT_EQ(get_compiled_rule_condition("test_rule"),"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_FALSE(has_warnings()); +} + +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/falco_engine.cpp b/userspace/engine/falco_engine.cpp index af73cec7251..b9254ba38f6 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); @@ -538,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 @@ -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; }; - 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_collector.cpp b/userspace/engine/rule_loader_collector.cpp index a3e699f8f1a..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)); } } @@ -190,10 +191,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 +204,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 +240,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_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 @@ -330,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_loader_reader.cpp b/userspace/engine/rule_loader_reader.cpp index c7aec794fd2..abf23143707 100644 --- a/userspace/engine/rule_loader_reader.cpp +++ b/userspace/engine/rule_loader_reader.cpp @@ -22,7 +22,7 @@ limitations under the License. #include "rule_loader_reader.h" #include "falco_engine_version.h" -#include "logger.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)); } } @@ -440,16 +440,17 @@ static void read_item( decode_items(item, v.items, ctx); decode_optional_val(item, "append", append, ctx); - + if(append) + { + cfg.res->add_warning(falco::load_result::LOAD_DEPRECATED_ITEM, WARNING_APPEND, 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, "Keys 'override' and 'append: true' cannot be used together.", 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. @@ -482,16 +483,17 @@ static void read_item( v.cond_ctx = rule_loader::context(item["condition"], rule_loader::context::MACRO_CONDITION, "", ctx); decode_optional_val(item, "append", append, ctx); + if(append) + { + cfg.res->add_warning(falco::load_result::LOAD_DEPRECATED_ITEM, WARNING_APPEND, 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, "Keys 'override' and 'append: true' cannot be used together.", 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. @@ -518,6 +520,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, ctx); + } std::set override_append, override_replace; std::set overridable_append {"condition", "output", "desc", "tags", "exceptions"}; @@ -528,8 +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), - "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), ERROR_OVERRIDE_APPEND, ctx); if(has_overrides) { @@ -689,6 +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, ctx); collector.enable(cfg, v); } else diff --git a/userspace/engine/rule_loading_messages.h b/userspace/engine/rule_loading_messages.h new file mode 100644 index 00000000000..5962fb0a406 --- /dev/null +++ b/userspace/engine/rule_loading_messages.h @@ -0,0 +1,23 @@ +#pragma once + +//////////////// +// Warnings +//////////////// + +#define WARNING_APPEND "'append' key is deprecated. Add an 'append' entry (e.g. 'condition: append') under 'override' instead." + +#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" + +#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_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" 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"); } }