From 8f68987a4a63f18d2f0ee6548243cb876036f03e Mon Sep 17 00:00:00 2001 From: Luca Guerra Date: Wed, 13 Dec 2023 14:09:47 +0000 Subject: [PATCH 1/7] new(engine): add selective overrides Signed-off-by: Luca Guerra --- unit_tests/engine/test_rule_loader.cpp | 235 ++++++++++++++++++ userspace/engine/rule_loader.cpp | 8 +- userspace/engine/rule_loader.h | 38 ++- userspace/engine/rule_loader_collector.cpp | 163 ++++++++++--- userspace/engine/rule_loader_collector.h | 7 +- userspace/engine/rule_loader_reader.cpp | 266 ++++++++++++++++++++- 6 files changed, 670 insertions(+), 47 deletions(-) create mode 100644 unit_tests/engine/test_rule_loader.cpp diff --git a/unit_tests/engine/test_rule_loader.cpp b/unit_tests/engine/test_rule_loader.cpp new file mode 100644 index 00000000000..b12bda09699 --- /dev/null +++ b/unit_tests/engine/test_rule_loader.cpp @@ -0,0 +1,235 @@ +#include + +#include "falco_engine.h" +#include "rule_loader_reader.h" +#include "rule_loader_compiler.h" + +class engine_loader_test : public ::testing::Test { +protected: + void SetUp() override + { + m_sample_ruleset = "sample-ruleset"; + m_sample_source = falco_common::syscall_source; + + // create a falco engine ready to load the ruleset + m_inspector.reset(new sinsp()); + m_engine.reset(new falco_engine()); + m_filter_factory = std::shared_ptr( + new sinsp_filter_factory(m_inspector.get(), m_filterlist)); + m_formatter_factory = std::shared_ptr( + new sinsp_evt_formatter_factory(m_inspector.get(), m_filterlist)); + m_engine->add_source(m_sample_source, m_filter_factory, m_formatter_factory); + } + + void TearDown() override + { + + } + + bool load_rules(std::string rules_content, std::string rules_filename) + { + bool ret = false; + falco::load_result::rules_contents_t rc = {{rules_filename, rules_content}}; + m_load_result = m_engine->load_rules(rules_content, rules_filename); + m_load_result_string = m_load_result->as_string(true, rc); + ret = m_load_result->successful(); + + if (ret) + { + m_engine->enable_rule("", true, m_sample_ruleset); + } + + return ret; + } + + std::string m_sample_ruleset; + std::string m_sample_source; + sinsp_filter_check_list m_filterlist; + std::shared_ptr m_filter_factory; + std::shared_ptr m_formatter_factory; + std::unique_ptr m_engine; + std::unique_ptr m_load_result; + std::string m_load_result_string; + std::unique_ptr m_inspector; +}; + +std::string s_sample_ruleset = "sample-ruleset"; +std::string s_sample_source = falco_common::syscall_source; + +TEST_F(engine_loader_test, list_append) +{ + std::string rules_content = R"END( +- list: shell_binaries + items: [ash, bash, csh, ksh, sh, tcsh, zsh, dash] + +- rule: legit_rule + desc: legit rule description + condition: evt.type=open and proc.name in (shell_binaries) + output: user=%user.name command=%proc.cmdline file=%fd.name + priority: INFO + +- list: shell_binaries + items: [pwsh] + override: + 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))"); +} + +TEST_F(engine_loader_test, condition_append) +{ + std::string rules_content = R"END( +- macro: interactive + condition: > + ((proc.aname=sshd and proc.name != sshd) or + proc.name=systemd-logind or proc.name=login) + +- rule: legit_rule + desc: legit rule description + condition: evt.type=open and interactive + output: user=%user.name command=%proc.cmdline file=%fd.name + priority: INFO + +- macro: interactive + condition: or proc.name = ssh + override: + 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))"); +} + +TEST_F(engine_loader_test, rule_override_append) +{ + std::string rules_content = R"END( +- rule: legit_rule + desc: legit rule description + condition: evt.type=open + output: user=%user.name command=%proc.cmdline file=%fd.name + priority: INFO + +- rule: legit_rule + desc: with append + condition: and proc.name = cat + output: proc=%proc.name + override: + desc: append + condition: append + output: 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]["info"]["condition"].template get(), + "evt.type=open and proc.name = cat"); + + ASSERT_EQ(rule_description["rules"][0]["info"]["output"].template get(), + "user=%user.name command=%proc.cmdline file=%fd.name proc=%proc.name"); + + ASSERT_EQ(rule_description["rules"][0]["info"]["description"].template get(), + "legit rule description with append"); +} + + +TEST_F(engine_loader_test, rule_append) +{ + std::string rules_content = R"END( +- rule: legit_rule + desc: legit rule description + condition: evt.type=open + output: user=%user.name command=%proc.cmdline file=%fd.name + priority: INFO + +- rule: legit_rule + condition: and proc.name = cat + 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)"); +} + + +TEST_F(engine_loader_test, rule_override_replace) +{ + std::string rules_content = R"END( +- rule: legit_rule + desc: legit rule description + condition: evt.type=open + output: user=%user.name command=%proc.cmdline file=%fd.name + priority: INFO + +- rule: legit_rule + desc: a replaced legit description + condition: evt.type = close + override: + desc: replace + condition: replace +)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]["info"]["condition"].template get(), + "evt.type = close"); + + ASSERT_EQ(rule_description["rules"][0]["info"]["output"].template get(), + "user=%user.name command=%proc.cmdline file=%fd.name"); + + ASSERT_EQ(rule_description["rules"][0]["info"]["description"].template get(), + "a replaced legit description"); +} + +TEST_F(engine_loader_test, rule_override_append_replace) +{ + std::string rules_content = R"END( +- rule: legit_rule + desc: legit rule description + condition: evt.type = close + output: user=%user.name command=%proc.cmdline file=%fd.name + priority: INFO + +- rule: legit_rule + desc: a replaced legit description + condition: and proc.name = cat + priority: WARNING + override: + desc: replace + condition: append + priority: replace +)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]["info"]["condition"].template get(), + "evt.type = close and proc.name = cat"); + + ASSERT_EQ(rule_description["rules"][0]["info"]["output"].template get(), + "user=%user.name command=%proc.cmdline file=%fd.name"); + + ASSERT_EQ(rule_description["rules"][0]["info"]["description"].template get(), + "a replaced legit description"); + + ASSERT_EQ(rule_description["rules"][0]["info"]["priority"].template get(), + "Warning"); +} diff --git a/userspace/engine/rule_loader.cpp b/userspace/engine/rule_loader.cpp index ddfc74b30f6..9ea32a90540 100644 --- a/userspace/engine/rule_loader.cpp +++ b/userspace/engine/rule_loader.cpp @@ -41,7 +41,8 @@ static const std::string item_type_strings[] = { "condition expression", "rule output", "rule output expression", - "rule priority" + "rule priority", + "overrides" }; const std::string& rule_loader::context::item_type_as_string(enum item_type it) @@ -553,6 +554,11 @@ rule_loader::rule_info::rule_info(context &ctx) { } +rule_loader::rule_update_info::rule_update_info(context &ctx) + : ctx(ctx), cond_ctx(ctx) +{ +} + rule_loader::rule_load_exception::rule_load_exception(falco::load_result::error_code ec, const std::string& msg, const context& ctx) : ec(ec), msg(msg), ctx(ctx) { diff --git a/userspace/engine/rule_loader.h b/userspace/engine/rule_loader.h index ac00cfb94be..2401b1492d5 100644 --- a/userspace/engine/rule_loader.h +++ b/userspace/engine/rule_loader.h @@ -19,6 +19,7 @@ limitations under the License. #include #include +#include #include #include #include "falco_source.h" @@ -56,7 +57,8 @@ namespace rule_loader CONDITION_EXPRESSION, RULE_OUTPUT, RULE_OUTPUT_EXPRESSION, - RULE_PRIORITY + RULE_PRIORITY, + OVERRIDE }; static const std::string& item_type_as_string(enum item_type it); @@ -451,4 +453,38 @@ namespace rule_loader bool warn_evttypes; bool skip_if_unknown_filter; }; + + /*! + \brief Represents infos about a rule update (append or replace) request + */ + + struct rule_update_info + { + rule_update_info(context &ctx); + ~rule_update_info() = default; + rule_update_info(rule_update_info&&) = default; + rule_update_info& operator = (rule_update_info&&) = default; + rule_update_info(const rule_update_info&) = default; + rule_update_info& operator = (const rule_update_info&) = default; + + bool has_any_value() + { + return cond.has_value() || output.has_value() || desc.has_value() || tags.has_value() || + exceptions.has_value() || priority.has_value() || enabled.has_value() || + warn_evttypes.has_value() || skip_if_unknown_filter.has_value(); + } + + context ctx; + context cond_ctx; + std::string name; + std::optional cond; + std::optional output; + std::optional desc; + std::optional> tags; + std::optional> exceptions; + std::optional priority; + std::optional enabled; + std::optional warn_evttypes; + std::optional skip_if_unknown_filter; + }; }; diff --git a/userspace/engine/rule_loader_collector.cpp b/userspace/engine/rule_loader_collector.cpp index 5ec5e5795af..5e84a93e0ab 100644 --- a/userspace/engine/rule_loader_collector.cpp +++ b/userspace/engine/rule_loader_collector.cpp @@ -48,8 +48,14 @@ static inline void define_info(indexed_vector& infos, T& info, uint32_t id) } } -template -static inline void append_info(T* prev, T& info, uint32_t id) +template +static inline void append_info(T* prev, U& info, uint32_t id) +{ + prev->visibility = id; +} + +template +static inline void replace_info(T* prev, U& info, uint32_t id) { prev->visibility = id; } @@ -185,7 +191,7 @@ void rule_loader::collector::append(configuration& cfg, list_info& info) { auto prev = m_list_infos.at(info.name); THROW(!prev, - "List has 'append' key but no list by that name already exists", + "List has 'append' key or an append override but no list by that name already exists", info.ctx); prev->items.insert(prev->items.end(), info.items.begin(), info.items.end()); append_info(prev, info, m_cur_index++); @@ -233,15 +239,15 @@ void rule_loader::collector::define(configuration& cfg, rule_info& info) define_info(m_rule_infos, info, m_cur_index++); } -void rule_loader::collector::append(configuration& cfg, rule_info& info) +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 but no rule by that name already exists", + "Rule has 'append' key or an append override but no rule by that name already exists", info.ctx); - THROW(info.cond.empty() && info.exceptions.empty(), - "Appended rule must have exceptions or condition property", + THROW(!info.has_any_value(), + "Appended rule must have at least one field that can be appended to", info.ctx); // note: source can be nullptr in case we've collected a @@ -256,43 +262,138 @@ void rule_loader::collector::append(configuration& cfg, rule_info& info) info.ctx); } - if (!info.cond.empty()) + if (info.cond.has_value() && !info.cond->empty()) { prev->cond += " "; - prev->cond += info.cond; + prev->cond += *info.cond; } - for (auto &ex : info.exceptions) + if (info.output.has_value() && !info.output->empty()) + { + prev->output += " "; + prev->output += *info.output; + } + + if (info.desc.has_value() && !info.desc->empty()) { - auto prev_ex = find_if(prev->exceptions.begin(), prev->exceptions.end(), - [&ex](const rule_loader::rule_exception_info& i) - { return i.name == ex.name; }); - if (prev_ex == prev->exceptions.end()) + prev->desc += " "; + prev->desc += *info.desc; + } + + if (info.tags.has_value()) + { + for (auto &tag: *info.tags) { - THROW(!ex.fields.is_valid(), - "Rule exception must have fields property with a list of fields", - ex.ctx); - THROW(ex.values.empty(), - "Rule exception must have values property with a list of values", - ex.ctx); - validate_exception_info(source, ex); - prev->exceptions.push_back(ex); + prev->tags.insert(tag); } - else + } + + if (info.exceptions.has_value()) + { + for (auto &ex : *info.exceptions) { - THROW(ex.fields.is_valid(), - "Can not append exception fields to existing exception, only values", - ex.ctx); - THROW(ex.comps.is_valid(), - "Can not append exception comps to existing exception, only values", - ex.ctx); - prev_ex->values.insert( - prev_ex->values.end(), ex.values.begin(), ex.values.end()); + auto prev_ex = find_if(prev->exceptions.begin(), prev->exceptions.end(), + [&ex](const rule_loader::rule_exception_info& i) + { return i.name == ex.name; }); + if (prev_ex == prev->exceptions.end()) + { + THROW(!ex.fields.is_valid(), + "Rule exception must have fields property with a list of fields", + ex.ctx); + THROW(ex.values.empty(), + "Rule exception must have values property with a list of values", + ex.ctx); + validate_exception_info(source, ex); + prev->exceptions.push_back(ex); + } + else + { + THROW(ex.fields.is_valid(), + "Can not append exception fields to existing exception, only values", + ex.ctx); + THROW(ex.comps.is_valid(), + "Can not append exception comps to existing exception, only values", + ex.ctx); + prev_ex->values.insert( + prev_ex->values.end(), ex.values.begin(), ex.values.end()); + } } } + append_info(prev, info, m_cur_index++); } +void rule_loader::collector::selective_replace(configuration& cfg, rule_update_info& info) +{ + 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(!info.has_any_value(), + "The rule must have at least one field that can be replaced", + info.ctx); + + // note: source can be nullptr in case we've collected a + // rule for which the source is unknown + falco_source* source = nullptr; + if (!prev->unknown_source) + { + // note: if the source is not unknown, this should not return nullptr + source = cfg.sources.at(prev->source); + THROW(!source, + std::string("Unknown source ") + prev->source, + info.ctx); + } + + if (info.cond.has_value()) + { + prev->cond = *info.cond; + } + + if (info.output.has_value()) + { + prev->output = *info.output; + } + + if (info.desc.has_value()) + { + prev->desc = *info.desc; + } + + if (info.tags.has_value()) + { + prev->tags = *info.tags; + } + + if (info.exceptions.has_value()) + { + prev->exceptions = *info.exceptions; + } + + if (info.priority.has_value()) + { + prev->priority = *info.priority; + } + + if (info.enabled.has_value()) + { + prev->enabled = *info.enabled; + } + + if (info.warn_evttypes.has_value()) + { + prev->warn_evttypes = *info.warn_evttypes; + } + + if (info.skip_if_unknown_filter.has_value()) + { + prev->skip_if_unknown_filter = *info.skip_if_unknown_filter; + } + + replace_info(prev, info, m_cur_index++); +} + void rule_loader::collector::enable(configuration& cfg, rule_info& info) { auto prev = m_rule_infos.at(info.name); diff --git a/userspace/engine/rule_loader_collector.h b/userspace/engine/rule_loader_collector.h index d5e3402c264..d95bb8e6966 100644 --- a/userspace/engine/rule_loader_collector.h +++ b/userspace/engine/rule_loader_collector.h @@ -85,13 +85,18 @@ class collector */ virtual void append(configuration& cfg, list_info& info); virtual void append(configuration& cfg, macro_info& info); - virtual void append(configuration& cfg, rule_info& info); + virtual void append(configuration& cfg, rule_update_info& info); /*! \brief Updates the 'enabled' flag of an existing definition */ virtual void enable(configuration& cfg, rule_info& info); + /*! + \brief Selectively replaces some fields of an existing definition + */ + virtual void selective_replace(configuration& cfg, rule_update_info& info); + private: uint32_t m_cur_index; indexed_vector m_rule_infos; diff --git a/userspace/engine/rule_loader_reader.cpp b/userspace/engine/rule_loader_reader.cpp index 1983583c635..e87e8c27016 100644 --- a/userspace/engine/rule_loader_reader.cpp +++ b/userspace/engine/rule_loader_reader.cpp @@ -17,6 +17,8 @@ limitations under the License. #include #include +#include +#include #include "rule_loader_reader.h" #include "falco_engine_version.h" @@ -45,6 +47,14 @@ static void decode_val_generic(const YAML::Node& item, const char *key, T& out, THROW(!YAML::convert::decode(val, out), "Can't decode YAML scalar value", valctx); } +template +static void decode_val_generic(const YAML::Node& item, const char *key, std::optional& out, const rule_loader::context& ctx, bool optional) +{ + T decoded; + decode_val_generic(item, key, decoded, ctx, optional); + out = decoded; +} + template static void decode_val(const YAML::Node& item, const char *key, T& out, const rule_loader::context& ctx) { @@ -115,6 +125,73 @@ static void decode_tags(const YAML::Node& item, std::set& out, decode_seq(item, "tags", inserter, ctx, optional); } +template +static void decode_tags(const YAML::Node& item, std::optional>& out, + const rule_loader::context& ctx) +{ + std::set decoded; + decode_tags(item, decoded, ctx); + out = decoded; +} + +static void decode_overrides(const YAML::Node& item, + std::set& overridable_append, + std::set& overridable_replace, + std::set& out_append, + std::set& out_replace, + const rule_loader::context& ctx) +{ + const YAML::Node& val = item["override"]; + + if(!val.IsDefined()) + { + return; + } + + rule_loader::context overridectx(item, rule_loader::context::OVERRIDE, "", ctx); + + for(YAML::const_iterator it=val.begin();it!=val.end();++it) + { + std::string key = it->first.as(); + std::string operation = it->second.as(); + + bool is_overridable_append = overridable_append.find(it->first.as()) != overridable_append.end(); + bool is_overridable_replace = overridable_replace.find(it->first.as()) != overridable_replace.end(); + + if (operation == "append") + { + rule_loader::context keyctx(it->first, rule_loader::context::OVERRIDE, key, overridectx); + THROW(!is_overridable_append, std::string("Key '") + key + std::string("' cannot be appended to"), keyctx); + + out_append.insert(key); + } + else if (operation == "replace") + { + rule_loader::context keyctx(it->first, rule_loader::context::OVERRIDE, key, overridectx); + THROW(!is_overridable_replace, std::string("Key '") + key + std::string("' cannot be replaced"), keyctx); + + out_replace.insert(key); + } + else + { + rule_loader::context operationctx(it->second, rule_loader::context::VALUE_FOR, key, overridectx); + std::stringstream err_ss; + err_ss << "Invalid override operation for key '" << key << "': '" << operation << "'. " + << "Allowed values are: "; + if (is_overridable_append) + { + err_ss << "append "; + } + if (is_overridable_replace) + { + err_ss << "replace "; + } + + THROW(true, err_ss.str(), operationctx); + } + } +} + // Don't call this directly, call decode_exception_{fields,comps,values} instead static void decode_exception_info_entry( const YAML::Node& item, @@ -187,7 +264,7 @@ static void decode_exception_values( static void read_rule_exceptions( const YAML::Node& item, - rule_loader::rule_info& v, + std::vector& exceptions, const rule_loader::context& parent, bool append) { @@ -239,8 +316,31 @@ static void read_rule_exceptions( v_ex.values.push_back(v_ex_val); } } - v.exceptions.push_back(v_ex); + exceptions.push_back(v_ex); + } +} + +static void read_rule_exceptions( + const YAML::Node& item, + std::optional>& exceptions, + const rule_loader::context& parent, + bool append) +{ + std::vector decoded; + read_rule_exceptions(item, decoded, parent, append); + exceptions = decoded; +} + +inline static bool check_override_defined(const YAML::Node& item, const std::set& overrides, const std::string& override_type, const std::string& key, const rule_loader::context& ctx) +{ + if (overrides.find(key) == overrides.end()) + { + return false; } + + THROW(!item[key].IsDefined(), std::string("An ") + override_type + " override for '" + key + "' was specified but '" + key + "' is not defined", ctx); + + return true; } static void read_item( @@ -337,6 +437,19 @@ static void read_item( decode_items(item, v.items, ctx); decode_optional_val(item, "append", append, ctx); + + std::set override_append, override_replace; + std::set overridable {"items"}; + decode_overrides(item, overridable, overridable, override_append, override_replace, ctx); + + if(append == true && !override_replace.empty()) + { + THROW(true, "Cannot specify a replace override when 'append: true' is set", 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(); if(append) { @@ -366,6 +479,19 @@ static void read_item( decode_optional_val(item, "append", append, ctx); + std::set override_append, override_replace; + std::set overridable {"condition"}; + decode_overrides(item, overridable, overridable, override_append, override_replace, ctx); + + if(append == true && !override_replace.empty()) + { + THROW(true, "Cannot specify a replace override when 'append: true' is set", 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(); + if(append) { collector.append(cfg, v); @@ -384,28 +510,142 @@ static void read_item( decode_val(item, "rule", name, tmp); rule_loader::context ctx(item, rule_loader::context::RULE, name, parent); - rule_loader::rule_info v(ctx); - v.name = name; - bool append = false; - v.enabled = true; - v.warn_evttypes = true; - v.skip_if_unknown_filter = false; + bool has_append_flag = false; + decode_optional_val(item, "append", has_append_flag, ctx); - decode_optional_val(item, "append", append, ctx); + std::set override_append, override_replace; + std::set overridable_append {"condition", "output", "desc", "tags", "exceptions"}; + std::set overridable_replace { + "condition", "output", "desc", "priority", "tags", "exceptions", "enabled", "warn_evttypes", "skip-if-unknown-filter"}; + decode_overrides(item, overridable_append, overridable_replace, override_append, override_replace, ctx); + bool has_overrides_append = !override_append.empty(); + bool has_overrides_replace = !override_replace.empty(); + bool has_overrides = has_overrides_append || has_overrides_replace; - if(append) + THROW((has_append_flag && has_overrides), "Keys 'override' and 'append' cannot be used together.", ctx); + + if(has_overrides) { - decode_optional_val(item, "condition", v.cond, ctx); + if (has_overrides_append) + { + rule_loader::rule_update_info v(ctx); + v.name = name; + if (check_override_defined(item, override_append, "append", "condition", ctx)) + { + decode_val(item, "condition", v.cond, ctx); + } + + if (check_override_defined(item, override_append, "append", "exceptions", ctx)) + { + read_rule_exceptions(item, v.exceptions, ctx, true); + } + + if (check_override_defined(item, override_append, "append", "output", ctx)) + { + decode_val(item, "output", v.output, ctx); + } + + if (check_override_defined(item, override_append, "append", "desc", ctx)) + { + decode_val(item, "desc", v.desc, ctx); + } + + if (check_override_defined(item, override_append, "append", "tags", ctx)) + { + decode_tags(item, v.tags, ctx); + } + + collector.append(cfg, v); + } + + if (has_overrides_replace) + { + rule_loader::rule_update_info v(ctx); + v.name = name; + if (check_override_defined(item, override_replace, "replace", "condition", ctx)) + { + decode_val(item, "condition", v.cond, ctx); + } + + if (check_override_defined(item, override_replace, "replace", "exceptions", ctx)) + { + read_rule_exceptions(item, v.exceptions, ctx, true); + } + + if (check_override_defined(item, override_replace, "replace", "output", ctx)) + { + decode_val(item, "output", v.output, ctx); + } + + if (check_override_defined(item, override_replace, "replace", "desc", ctx)) + { + decode_val(item, "desc", v.desc, ctx); + } + + if (check_override_defined(item, override_replace, "replace", "tags", ctx)) + { + decode_tags(item, v.tags, ctx); + } + + if (check_override_defined(item, override_replace, "replace", "priority", ctx)) + { + std::string priority; + decode_val(item, "priority", priority, ctx); + rule_loader::context prictx(item["priority"], rule_loader::context::RULE_PRIORITY, "", ctx); + falco_common::priority_type parsed_priority; + THROW(!falco_common::parse_priority(priority, parsed_priority), "Invalid priority", prictx); + v.priority = parsed_priority; + } + + if (check_override_defined(item, override_replace, "replace", "enabled", ctx)) + { + decode_val(item, "enabled", v.enabled, ctx); + } + + if (check_override_defined(item, override_replace, "replace", "warn_evttypes", ctx)) + { + decode_val(item, "warn_evttypes", v.warn_evttypes, ctx); + } + + if (check_override_defined(item, override_replace, "replace", "skip-if-unknown-filter", ctx)) + { + decode_val(item, "skip-if-unknown-filter", v.skip_if_unknown_filter, ctx); + } + + collector.selective_replace(cfg, v); + } + } + else if(has_append_flag) + { + rule_loader::rule_update_info v(ctx); + v.name = name; + if(item["condition"].IsDefined()) { v.cond_ctx = rule_loader::context(item["condition"], rule_loader::context::RULE_CONDITION, "", ctx); + decode_val(item, "condition", v.cond, ctx); + } + + if(item["exceptions"].IsDefined()) + { + read_rule_exceptions(item, v.exceptions, ctx, true); } - read_rule_exceptions(item, v, ctx, append); + + THROW((!v.cond.has_value() && !v.exceptions.has_value()), + "Appended rule must have exceptions or condition property", + v.ctx); + collector.append(cfg, v); } else { + rule_loader::rule_info v(ctx); + v.name = name; + v.enabled = true; + v.warn_evttypes = true; + v.skip_if_unknown_filter = false; + // If the rule does *not* have any of // condition/output/desc/priority, it *must* // have an enabled property. Use the enabled @@ -443,7 +683,7 @@ static void read_item( decode_optional_val(item, "warn_evttypes", v.warn_evttypes, ctx); decode_optional_val(item, "skip-if-unknown-filter", v.skip_if_unknown_filter, ctx); decode_tags(item, v.tags, ctx); - read_rule_exceptions(item, v, ctx, append); + read_rule_exceptions(item, v.exceptions, ctx, has_append_flag); collector.define(cfg, v); } } From 2b328584e27fc819a0226efb010b3cab9487536b Mon Sep 17 00:00:00 2001 From: Luca Guerra Date: Wed, 20 Dec 2023 10:50:58 +0000 Subject: [PATCH 2/7] update(engine): clarify override error messages Signed-off-by: Luca Guerra --- userspace/engine/rule_loader_reader.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/userspace/engine/rule_loader_reader.cpp b/userspace/engine/rule_loader_reader.cpp index e87e8c27016..b489b0f2944 100644 --- a/userspace/engine/rule_loader_reader.cpp +++ b/userspace/engine/rule_loader_reader.cpp @@ -441,10 +441,11 @@ static void read_item( 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 && !override_replace.empty()) + if(append == true && has_overrides) { - THROW(true, "Cannot specify a replace override when 'append: true' is set", ctx); + THROW(true, "Keys 'override' and 'append: true' cannot be used together.", ctx); } // Since a list only has items, if we have chosen to append them we can append the entire object @@ -482,10 +483,11 @@ static void read_item( 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 && !override_replace.empty()) + if(append == true && has_overrides) { - THROW(true, "Cannot specify a replace override when 'append: true' is set", ctx); + THROW(true, "Keys 'override' and 'append: true' cannot be used together.", ctx); } // Since a macro only has a condition, if we have chosen to append to it we can append the entire object @@ -523,7 +525,8 @@ 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' cannot be used together.", ctx); + 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); if(has_overrides) { From 81e956864302caf82504c6f6c62c53a356aeb08b Mon Sep 17 00:00:00 2001 From: Luca Guerra Date: Wed, 20 Dec 2023 11:32:24 +0000 Subject: [PATCH 3/7] chore(engine): bump engine version Signed-off-by: Luca Guerra --- userspace/engine/falco_engine_version.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/userspace/engine/falco_engine_version.h b/userspace/engine/falco_engine_version.h index 96b77658ec5..60a99aaad5e 100644 --- a/userspace/engine/falco_engine_version.h +++ b/userspace/engine/falco_engine_version.h @@ -20,7 +20,7 @@ limitations under the License. // The version of this Falco engine #define FALCO_ENGINE_VERSION_MAJOR 0 -#define FALCO_ENGINE_VERSION_MINOR 30 +#define FALCO_ENGINE_VERSION_MINOR 31 #define FALCO_ENGINE_VERSION_PATCH 0 #define FALCO_ENGINE_VERSION \ From 5162bc0ed9dc2fb73d493343dbf9af48054e8c5e Mon Sep 17 00:00:00 2001 From: Luca Guerra Date: Fri, 22 Dec 2023 15:53:53 +0000 Subject: [PATCH 4/7] update(engine): throw an error if an unexpected top level key is found in an override Signed-off-by: Luca Guerra --- userspace/engine/rule_loader_reader.cpp | 60 ++++++++++++++++++------- 1 file changed, 44 insertions(+), 16 deletions(-) diff --git a/userspace/engine/rule_loader_reader.cpp b/userspace/engine/rule_loader_reader.cpp index b489b0f2944..9fbca762330 100644 --- a/userspace/engine/rule_loader_reader.cpp +++ b/userspace/engine/rule_loader_reader.cpp @@ -331,14 +331,17 @@ static void read_rule_exceptions( exceptions = decoded; } -inline static bool check_override_defined(const YAML::Node& item, const std::set& overrides, const std::string& override_type, const std::string& key, const rule_loader::context& ctx) +inline static bool check_update_expected(std::set& expected_keys, const std::set& overrides, const std::string& override_type, const std::string& key, const rule_loader::context& ctx) { if (overrides.find(key) == overrides.end()) { return false; } - THROW(!item[key].IsDefined(), std::string("An ") + override_type + " override for '" + key + "' was specified but '" + key + "' is not defined", ctx); + THROW(expected_keys.find(key) == expected_keys.end(), + std::string("An ") + override_type + " override for '" + key + "' was specified but '" + key + "' is not defined", ctx); + + expected_keys.erase(key); return true; } @@ -530,31 +533,50 @@ static void read_item( if(has_overrides) { + std::set expected_keys; + for (auto& key : overridable_append) + { + if (item[key].IsDefined()) + { + expected_keys.insert(key); + } + } + + for (auto& key : overridable_replace) + { + if (item[key].IsDefined()) + { + expected_keys.insert(key); + } + } + + // expected_keys is (appendable U replaceable) ^ (defined) + if (has_overrides_append) { rule_loader::rule_update_info v(ctx); v.name = name; - if (check_override_defined(item, override_append, "append", "condition", ctx)) + if (check_update_expected(expected_keys, override_append, "append", "condition", ctx)) { decode_val(item, "condition", v.cond, ctx); } - if (check_override_defined(item, override_append, "append", "exceptions", ctx)) + if (check_update_expected(expected_keys, override_append, "append", "exceptions", ctx)) { read_rule_exceptions(item, v.exceptions, ctx, true); } - if (check_override_defined(item, override_append, "append", "output", ctx)) + if (check_update_expected(expected_keys, override_append, "append", "output", ctx)) { decode_val(item, "output", v.output, ctx); } - if (check_override_defined(item, override_append, "append", "desc", ctx)) + if (check_update_expected(expected_keys, override_append, "append", "desc", ctx)) { decode_val(item, "desc", v.desc, ctx); } - if (check_override_defined(item, override_append, "append", "tags", ctx)) + if (check_update_expected(expected_keys, override_append, "append", "tags", ctx)) { decode_tags(item, v.tags, ctx); } @@ -566,32 +588,32 @@ static void read_item( { rule_loader::rule_update_info v(ctx); v.name = name; - if (check_override_defined(item, override_replace, "replace", "condition", ctx)) + if (check_update_expected(expected_keys, override_replace, "replace", "condition", ctx)) { decode_val(item, "condition", v.cond, ctx); } - if (check_override_defined(item, override_replace, "replace", "exceptions", ctx)) + if (check_update_expected(expected_keys, override_replace, "replace", "exceptions", ctx)) { read_rule_exceptions(item, v.exceptions, ctx, true); } - if (check_override_defined(item, override_replace, "replace", "output", ctx)) + if (check_update_expected(expected_keys, override_replace, "replace", "output", ctx)) { decode_val(item, "output", v.output, ctx); } - if (check_override_defined(item, override_replace, "replace", "desc", ctx)) + if (check_update_expected(expected_keys, override_replace, "replace", "desc", ctx)) { decode_val(item, "desc", v.desc, ctx); } - if (check_override_defined(item, override_replace, "replace", "tags", ctx)) + if (check_update_expected(expected_keys, override_replace, "replace", "tags", ctx)) { decode_tags(item, v.tags, ctx); } - if (check_override_defined(item, override_replace, "replace", "priority", ctx)) + if (check_update_expected(expected_keys, override_replace, "replace", "priority", ctx)) { std::string priority; decode_val(item, "priority", priority, ctx); @@ -601,23 +623,29 @@ static void read_item( v.priority = parsed_priority; } - if (check_override_defined(item, override_replace, "replace", "enabled", ctx)) + if (check_update_expected(expected_keys, override_replace, "replace", "enabled", ctx)) { decode_val(item, "enabled", v.enabled, ctx); } - if (check_override_defined(item, override_replace, "replace", "warn_evttypes", ctx)) + if (check_update_expected(expected_keys, override_replace, "replace", "warn_evttypes", ctx)) { decode_val(item, "warn_evttypes", v.warn_evttypes, ctx); } - if (check_override_defined(item, override_replace, "replace", "skip-if-unknown-filter", ctx)) + if (check_update_expected(expected_keys, override_replace, "replace", "skip-if-unknown-filter", ctx)) { decode_val(item, "skip-if-unknown-filter", v.skip_if_unknown_filter, ctx); } collector.selective_replace(cfg, v); } + + // if any expected key has not been defined throw an error + for (auto &key : expected_keys) { + rule_loader::context keyctx(item[key], rule_loader::context::OVERRIDE, key, ctx); + THROW(true, "Unexpected key '" + key + "': no corresponding entry under 'override' is defined.", keyctx); + } } else if(has_append_flag) { From 471870edcc0021f31fe2aaa644570b981d415380 Mon Sep 17 00:00:00 2001 From: Luca Guerra Date: Fri, 22 Dec 2023 16:16:54 +0000 Subject: [PATCH 5/7] update(engine): temporary replace for error messages Signed-off-by: Luca Guerra --- userspace/engine/rule_loader_collector.cpp | 9 ++++++--- userspace/engine/rule_loader_reader.cpp | 7 ++++--- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/userspace/engine/rule_loader_collector.cpp b/userspace/engine/rule_loader_collector.cpp index 5e84a93e0ab..a3e699f8f1a 100644 --- a/userspace/engine/rule_loader_collector.cpp +++ b/userspace/engine/rule_loader_collector.cpp @@ -191,7 +191,8 @@ 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", + // "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); prev->items.insert(prev->items.end(), info.items.begin(), info.items.end()); append_info(prev, info, m_cur_index++); @@ -244,10 +245,12 @@ 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", + // "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(!info.has_any_value(), - "Appended rule must have at least one field that can be appended to", + "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 info.ctx); // note: source can be nullptr in case we've collected a diff --git a/userspace/engine/rule_loader_reader.cpp b/userspace/engine/rule_loader_reader.cpp index 9fbca762330..cb665f0a28a 100644 --- a/userspace/engine/rule_loader_reader.cpp +++ b/userspace/engine/rule_loader_reader.cpp @@ -663,9 +663,10 @@ static void read_item( read_rule_exceptions(item, v.exceptions, ctx, true); } - THROW((!v.cond.has_value() && !v.exceptions.has_value()), - "Appended rule must have exceptions or condition property", - v.ctx); + // TODO restore this error and update testing + //THROW((!v.cond.has_value() && !v.exceptions.has_value()), + // "Appended rule must have exceptions or condition property", + // v.ctx); collector.append(cfg, v); } From 37bf76b9c3501ed214eeb004f0794c0388e06535 Mon Sep 17 00:00:00 2001 From: Luca Guerra Date: Fri, 22 Dec 2023 16:17:28 +0000 Subject: [PATCH 6/7] new(tests): add error testing for rule overrides Signed-off-by: Luca Guerra --- unit_tests/engine/test_rule_loader.cpp | 116 +++++++++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/unit_tests/engine/test_rule_loader.cpp b/unit_tests/engine/test_rule_loader.cpp index b12bda09699..5670ea1081f 100644 --- a/unit_tests/engine/test_rule_loader.cpp +++ b/unit_tests/engine/test_rule_loader.cpp @@ -32,6 +32,7 @@ class engine_loader_test : public ::testing::Test { falco::load_result::rules_contents_t rc = {{rules_filename, rules_content}}; m_load_result = m_engine->load_rules(rules_content, rules_filename); m_load_result_string = m_load_result->as_string(true, rc); + m_load_result_json = m_load_result->as_json(rc); ret = m_load_result->successful(); if (ret) @@ -50,6 +51,7 @@ class engine_loader_test : public ::testing::Test { std::unique_ptr m_engine; std::unique_ptr m_load_result; std::string m_load_result_string; + nlohmann::json m_load_result_json; std::unique_ptr m_inspector; }; @@ -233,3 +235,117 @@ TEST_F(engine_loader_test, rule_override_append_replace) ASSERT_EQ(rule_description["rules"][0]["info"]["priority"].template get(), "Warning"); } + +TEST_F(engine_loader_test, rule_incorrect_override_type) +{ + std::string rules_content = R"END( +- rule: failing_rule + desc: legit rule description + condition: evt.type = close + output: user=%user.name command=%proc.cmdline file=%fd.name + priority: INFO + +- rule: failing_rule + desc: an appended incorrect field + condition: and proc.name = cat + priority: WARNING + override: + desc: replace + condition: append + priority: append +)END"; + + 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"); + ASSERT_TRUE(std::string(m_load_result_json["errors"][0]["context"]["snippet"]).find("priority: append") != std::string::npos); +} + +TEST_F(engine_loader_test, rule_incorrect_append_override) +{ + std::string rules_content = R"END( +- rule: failing_rule + desc: legit rule description + condition: evt.type = close + output: user=%user.name command=%proc.cmdline file=%fd.name + priority: INFO + +- rule: failing_rule + desc: an appended incorrect field + condition: and proc.name = cat + append: true + override: + desc: replace + condition: append +)END"; + + 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); +} + +TEST_F(engine_loader_test, rule_override_without_rule) +{ + std::string rules_content = R"END( +- rule: failing_rule + desc: an appended field + condition: and proc.name = cat + override: + desc: replace + condition: append +)END"; + + std::string rule_name = "failing_rule"; + + ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); + // std::cout << m_load_result_json.dump(4) << std::endl; + ASSERT_TRUE(std::string(m_load_result_json["errors"][0]["message"]).find("no rule by that name already exists") != std::string::npos); +} + +TEST_F(engine_loader_test, rule_override_without_field) +{ + std::string rules_content = R"END( +- rule: failing_rule + desc: legit rule description + condition: evt.type = close + output: user=%user.name command=%proc.cmdline file=%fd.name + priority: INFO + +- rule: failing_rule + desc: an appended incorrect field + override: + desc: replace + condition: append +)END"; + + 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"); +} + +TEST_F(engine_loader_test, rule_override_extra_field) +{ + std::string rules_content = R"END( +- rule: failing_rule + desc: legit rule description + condition: evt.type = close + output: user=%user.name command=%proc.cmdline file=%fd.name + priority: INFO + +- rule: failing_rule + desc: an appended incorrect field + condition: and proc.name = cat + priority: WARNING + override: + desc: replace + condition: append +)END"; + + 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); +} From ca6d3835b917d555ad1b876a0e08c770aa34c457 Mon Sep 17 00:00:00 2001 From: Luca Guerra Date: Fri, 22 Dec 2023 16:56:44 +0000 Subject: [PATCH 7/7] fix(engine): clarify error message for invalid append Signed-off-by: Luca Guerra --- unit_tests/engine/test_rule_loader.cpp | 3 +-- userspace/engine/rule_loader_reader.cpp | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/unit_tests/engine/test_rule_loader.cpp b/unit_tests/engine/test_rule_loader.cpp index 5670ea1081f..d369d5c7798 100644 --- a/unit_tests/engine/test_rule_loader.cpp +++ b/unit_tests/engine/test_rule_loader.cpp @@ -258,7 +258,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"); + ASSERT_EQ(m_load_result_json["errors"][0]["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); } @@ -300,7 +300,6 @@ TEST_F(engine_loader_test, rule_override_without_rule) std::string rule_name = "failing_rule"; ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); - // std::cout << m_load_result_json.dump(4) << std::endl; ASSERT_TRUE(std::string(m_load_result_json["errors"][0]["message"]).find("no rule by that name already exists") != std::string::npos); } diff --git a/userspace/engine/rule_loader_reader.cpp b/userspace/engine/rule_loader_reader.cpp index cb665f0a28a..c7aec794fd2 100644 --- a/userspace/engine/rule_loader_reader.cpp +++ b/userspace/engine/rule_loader_reader.cpp @@ -161,7 +161,7 @@ static void decode_overrides(const YAML::Node& item, if (operation == "append") { rule_loader::context keyctx(it->first, rule_loader::context::OVERRIDE, key, overridectx); - THROW(!is_overridable_append, std::string("Key '") + key + std::string("' cannot be appended to"), keyctx); + THROW(!is_overridable_append, std::string("Key '") + key + std::string("' cannot be appended to, use 'replace' instead"), keyctx); out_append.insert(key); }