From 07abb89f36b9e2b83fad2cac873f0b6925a087be Mon Sep 17 00:00:00 2001 From: Mark Stemm Date: Thu, 17 Sep 2020 16:02:42 -0700 Subject: [PATCH] Pass back warnings when loading rules Add the notion of warnings when loading rules, which are printed if verbose is true: - load_rules now returns a tuple (success, required engine version, error array, warnings array) instead of (true, required engine version) or (false, error string) - build_error/build_error_with_context now returns an array instead of string value. - warnings are combined across calls to load_rules_doc - Current warnings include: - a rule that contains an unknown filter - a macro not referred to by any rule - a list not referred to by any rule/macro/list Any errors/warnings are concatenated into the exception if success was false. Any errors/warnings will be printed if verbose is true. Signed-off-by: Mark Stemm --- userspace/engine/lua/rule_loader.lua | 99 ++++++++++++++++------------ userspace/engine/rules.cpp | 79 ++++++++++++++++++---- userspace/falco/falco.cpp | 18 +++-- 3 files changed, 136 insertions(+), 60 deletions(-) diff --git a/userspace/engine/lua/rule_loader.lua b/userspace/engine/lua/rule_loader.lua index d6d2427a..0712bede 100644 --- a/userspace/engine/lua/rule_loader.lua +++ b/userspace/engine/lua/rule_loader.lua @@ -256,16 +256,18 @@ end function build_error(rules_lines, row, num_lines, err) local ret = err.."\n---\n"..get_lines(rules_lines, row, num_lines).."---" - return ret + return {ret} end function build_error_with_context(ctx, err) local ret = err.."\n---\n"..ctx.."---" - return ret + return {ret} end function load_rules_doc(rules_mgr, doc, load_state) + local warnings = {} + -- Iterate over yaml list. In this pass, all we're doing is -- populating the set of rules, macros, and lists. We're not -- expanding/compiling anything yet. All that will happen in a @@ -279,7 +281,7 @@ function load_rules_doc(rules_mgr, doc, load_state) load_state.indices[load_state.cur_item_idx]) if (not (type(v) == "table")) then - return false, build_error_with_context(context, "Unexpected element of type " ..type(v)..". Each element should be a yaml associative array.") + return false, build_error_with_context(context, "Unexpected element of type " ..type(v)..". Each element should be a yaml associative array."), warnings end v['context'] = context @@ -291,13 +293,13 @@ function load_rules_doc(rules_mgr, doc, load_state) end if falco_rules.engine_version(rules_mgr) < v['required_engine_version'] then - return false, build_error_with_context(v['context'], "Rules require engine version "..v['required_engine_version']..", but engine version is "..falco_rules.engine_version(rules_mgr)) + return false, build_error_with_context(v['context'], "Rules require engine version "..v['required_engine_version']..", but engine version is "..falco_rules.engine_version(rules_mgr)), warnings end elseif (v['macro']) then if (v['macro'] == nil or type(v['macro']) == "table") then - return false, build_error_with_context(v['context'], "Macro name is empty") + return false, build_error_with_context(v['context'], "Macro name is empty"), warnings end if v['source'] == nil then @@ -310,7 +312,7 @@ function load_rules_doc(rules_mgr, doc, load_state) for j, field in ipairs({'condition'}) do if (v[field] == nil) then - return false, build_error_with_context(v['context'], "Macro must have property "..field) + return false, build_error_with_context(v['context'], "Macro must have property "..field), warnings end end @@ -323,7 +325,7 @@ function load_rules_doc(rules_mgr, doc, load_state) if append then if state.macros_by_name[v['macro']] == nil then - return false, build_error_with_context(v['context'], "Macro " ..v['macro'].. " has 'append' key but no macro by that name already exists") + return false, build_error_with_context(v['context'], "Macro " ..v['macro'].. " has 'append' key but no macro by that name already exists"), warnings end state.macros_by_name[v['macro']]['condition'] = state.macros_by_name[v['macro']]['condition'] .. " " .. v['condition'] @@ -338,7 +340,7 @@ function load_rules_doc(rules_mgr, doc, load_state) elseif (v['list']) then if (v['list'] == nil or type(v['list']) == "table") then - return false, build_error_with_context(v['context'], "List name is empty") + return false, build_error_with_context(v['context'], "List name is empty"), warnings end if state.lists_by_name[v['list']] == nil then @@ -347,7 +349,7 @@ function load_rules_doc(rules_mgr, doc, load_state) for j, field in ipairs({'items'}) do if (v[field] == nil) then - return false, build_error_with_context(v['context'], "List must have property "..field) + return false, build_error_with_context(v['context'], "List must have property "..field), warnings end end @@ -360,7 +362,7 @@ function load_rules_doc(rules_mgr, doc, load_state) if append then if state.lists_by_name[v['list']] == nil then - return false, build_error_with_context(v['context'], "List " ..v['list'].. " has 'append' key but no list by that name already exists") + return false, build_error_with_context(v['context'], "List " ..v['list'].. " has 'append' key but no list by that name already exists"), warnings end for j, elem in ipairs(v['items']) do @@ -373,7 +375,7 @@ function load_rules_doc(rules_mgr, doc, load_state) elseif (v['rule']) then if (v['rule'] == nil or type(v['rule']) == "table") then - return false, build_error_with_context(v['context'], "Rule name is empty") + return false, build_error_with_context(v['context'], "Rule name is empty"), warnings end -- By default, if a rule's condition refers to an unknown @@ -398,13 +400,13 @@ function load_rules_doc(rules_mgr, doc, load_state) -- For append rules, all you need is the condition for j, field in ipairs({'condition'}) do if (v[field] == nil) then - return false, build_error_with_context(v['context'], "Rule must have property "..field) + return false, build_error_with_context(v['context'], "Rule must have property "..field), warnings end end if state.rules_by_name[v['rule']] == nil then if state.skipped_rules_by_name[v['rule']] == nil then - return false, build_error_with_context(v['context'], "Rule " ..v['rule'].. " has 'append' key but no rule by that name already exists") + return false, build_error_with_context(v['context'], "Rule " ..v['rule'].. " has 'append' key but no rule by that name already exists"), warnings end else state.rules_by_name[v['rule']]['condition'] = state.rules_by_name[v['rule']]['condition'] .. " " .. v['condition'] @@ -417,7 +419,7 @@ function load_rules_doc(rules_mgr, doc, load_state) for j, field in ipairs({'condition', 'output', 'desc', 'priority'}) do if (v[field] == nil) then - return false, build_error_with_context(v['context'], "Rule must have property "..field) + return false, build_error_with_context(v['context'], "Rule must have property "..field), warnings end end @@ -449,13 +451,18 @@ function load_rules_doc(rules_mgr, doc, load_state) -- Remove the context from the table, so the table is exactly what was parsed local context = v['context'] v['context'] = nil - return false, build_error_with_context(context, "Unknown rule object: "..table.tostring(v)) + return false, build_error_with_context(context, "Unknown rule object: "..table.tostring(v)), warnings end end - return true, "" + return true, {}, {} end +-- Returns: +-- - Load Result: bool +-- - required engine version. will be nil when load result is false +-- - List of Errors +-- - List of Warnings function load_rules(sinsp_lua_parser, json_lua_parser, rules_content, @@ -466,6 +473,8 @@ function load_rules(sinsp_lua_parser, replace_container_info, min_priority) + local warnings = {} + local load_state = {lines={}, indices={}, cur_item_idx=0, min_priority=min_priority, required_engine_version=0} load_state.lines, load_state.indices = split_lines(rules_content) @@ -487,36 +496,42 @@ function load_rules(sinsp_lua_parser, row = tonumber(row) col = tonumber(col) - return false, build_error(load_state.lines, row, 3, docs) + return false, nil, build_error(load_state.lines, row, 3, docs), warnings end if docs == nil then -- An empty rules file is acceptable - return true, load_state.required_engine_version + return true, load_state.required_engine_version, {}, warnings end if type(docs) ~= "table" then - return false, build_error(load_state.lines, 1, 1, "Rules content is not yaml") + return false, nil, build_error(load_state.lines, 1, 1, "Rules content is not yaml"), warnings end for docidx, doc in ipairs(docs) do if type(doc) ~= "table" then - return false, build_error(load_state.lines, 1, 1, "Rules content is not yaml") + return false, nil, build_error(load_state.lines, 1, 1, "Rules content is not yaml"), warnings end -- Look for non-numeric indices--implies that document is not array -- of objects. for key, val in pairs(doc) do if type(key) ~= "number" then - return false, build_error(load_state.lines, 1, 1, "Rules content is not yaml array of objects") + return false, nil, build_error(load_state.lines, 1, 1, "Rules content is not yaml array of objects"), warnings end end - res, errstr = load_rules_doc(rules_mgr, doc, load_state) + res, errors, doc_warnings = load_rules_doc(rules_mgr, doc, load_state) + + if (doc_warnings ~= nil) then + for idx, warning in pairs(doc_warnings) do + table.insert(warnings, warning) + end + end if not res then - return res, errstr + return res, nil, errors, warnings end end @@ -556,7 +571,7 @@ function load_rules(sinsp_lua_parser, local status, ast = compiler.compile_macro(v['condition'], state.macros, state.lists) if status == false then - return false, build_error_with_context(v['context'], ast) + return false, nil, build_error_with_context(v['context'], ast), warnings end if v['source'] == "syscall" then @@ -581,7 +596,7 @@ function load_rules(sinsp_lua_parser, state.macros, state.lists) if status == false then - return false, build_error_with_context(v['context'], filter_ast) + return false, nil, build_error_with_context(v['context'], filter_ast), warnings end local evtttypes = {} @@ -631,12 +646,10 @@ function load_rules(sinsp_lua_parser, end if not found then - if v['skip-if-unknown-filter'] then - if verbose then - print("Skipping rule \""..v['rule'].."\" that contains unknown filter "..filter) - end - goto next_rule - else + msg = "rule \""..v['rule'].."\" contains unknown filter "..filter + warnings[#warnings + 1] = msg + + if not v['skip-if-unknown-filter'] then error("Rule \""..v['rule'].."\" contains unknown filter "..filter) end end @@ -719,30 +732,30 @@ function load_rules(sinsp_lua_parser, formatter = formats.formatter(v['source'], v['output']) formats.free_formatter(v['source'], formatter) else - return false, build_error_with_context(v['context'], "Unexpected type in load_rule: "..filter_ast.type) + return false, nil, build_error_with_context(v['context'], "Unexpected type in load_rule: "..filter_ast.type), warnings end ::next_rule:: end - if verbose then - -- Print info on any dangling lists or macros that were not used anywhere - for name, macro in pairs(state.macros) do - if macro.used == false then - print("Warning: macro "..name.." not refered to by any rule/macro") - end + -- Print info on any dangling lists or macros that were not used anywhere + for name, macro in pairs(state.macros) do + if macro.used == false then + msg = "macro "..name.." not refered to by any rule/macro" + warnings[#warnings + 1] = msg end + end - for name, list in pairs(state.lists) do - if list.used == false then - print("Warning: list "..name.." not refered to by any rule/macro/list") - end + for name, list in pairs(state.lists) do + if list.used == false then + msg = "list "..name.." not refered to by any rule/macro/list" + warnings[#warnings + 1] = msg end end io.flush() - return true, load_state.required_engine_version + return true, load_state.required_engine_version, {}, warnings end local rule_fmt = "%-50s %s" diff --git a/userspace/engine/rules.cpp b/userspace/engine/rules.cpp index 97394e09..14c71d8d 100644 --- a/userspace/engine/rules.cpp +++ b/userspace/engine/rules.cpp @@ -14,8 +14,9 @@ See the License for the specific language governing permissions and limitations under the License. */ +#include + #include "rules.h" -#include "logger.h" extern "C" { #include "lua.h" @@ -218,6 +219,31 @@ int falco_rules::engine_version(lua_State *ls) return 1; } +static std::list get_lua_table_values(lua_State *ls, int idx) +{ + std::list ret; + + if (lua_isnil(ls, idx)) { + return ret; + } + + lua_pushnil(ls); /* first key */ + while (lua_next(ls, idx-1) != 0) { + // key is at index -2, value is at index + // -1. We want the values. + if (! lua_isstring(ls, -1)) { + std::string err = "Non-string value in table of strings"; + throw falco_exception(err); + } + ret.push_back(string(lua_tostring(ls, -1))); + + // Remove value, keep key for next iteration + lua_pop(ls, 1); + } + + return ret; +} + void falco_rules::load_rules(const string &rules_content, bool verbose, bool all_events, string &extra, bool replace_container_info, @@ -423,7 +449,7 @@ void falco_rules::load_rules(const string &rules_content, lua_pushstring(m_ls, extra.c_str()); lua_pushboolean(m_ls, (replace_container_info ? 1 : 0)); lua_pushnumber(m_ls, min_priority); - if(lua_pcall(m_ls, 9, 2, 0) != 0) + if(lua_pcall(m_ls, 9, 4, 0) != 0) { const char* lerr = lua_tostring(m_ls, -1); @@ -432,20 +458,49 @@ void falco_rules::load_rules(const string &rules_content, throw falco_exception(err); } - // Either returns (true, required_engine_version), or (false, error string) - bool successful = lua_toboolean(m_ls, -2); + // Returns: + // Load result: bool + // required engine version: will be nil when load result is false + // array of errors + // array of warnings + bool successful = lua_toboolean(m_ls, -4); + required_engine_version = lua_tonumber(m_ls, -3); + std::list errors = get_lua_table_values(m_ls, -2); + std::list warnings = get_lua_table_values(m_ls, -1); - if(successful) + // Concatenate errors/warnings + std::ostringstream os; + if (errors.size() > 0) { - required_engine_version = lua_tonumber(m_ls, -1); - } - else - { - std::string err = lua_tostring(m_ls, -1); - throw falco_exception(err); + os << errors.size() << " errors:" << std::endl; + for(auto err : errors) + { + os << err << std::endl; + } } - lua_pop(m_ls, 2); + if (warnings.size() > 0) + { + os << warnings.size() << " warnings:" << std::endl; + for(auto warn : warnings) + { + os << warn << std::endl; + } + } + + if(!successful) + { + throw falco_exception(os.str()); + } + + if (verbose && os.str() != "") { + // We don't really have a logging callback + // from the falco engine, but this would be a + // good place to use it. + fprintf(stderr, "When reading rules content: %s", os.str().c_str()); + } + + lua_pop(m_ls, 4); } else { throw falco_exception("No function " + m_lua_load_rules + " found in lua rule module"); diff --git a/userspace/falco/falco.cpp b/userspace/falco/falco.cpp index 1ab4bd61..85947810 100644 --- a/userspace/falco/falco.cpp +++ b/userspace/falco/falco.cpp @@ -813,7 +813,7 @@ int falco_init(int argc, char **argv) } catch(falco_exception &e) { - printf("%s%s\n", prefix.c_str(), e.what()); + printf("%s%s", prefix.c_str(), e.what()); throw; } printf("%sOk\n", prefix.c_str()); @@ -865,7 +865,15 @@ int falco_init(int argc, char **argv) falco_logger::log(LOG_INFO, "Loading rules from file " + filename + ":\n"); uint64_t required_engine_version; - engine->load_rules_file(filename, verbose, all_events, required_engine_version); + try { + engine->load_rules_file(filename, verbose, all_events, required_engine_version); + } + catch(falco_exception &e) + { + std::string prefix = "Could not load rules file " + filename + ": "; + + throw falco_exception(prefix + e.what()); + } required_engine_versions[filename] = required_engine_version; } @@ -1177,8 +1185,8 @@ int falco_init(int argc, char **argv) falco_logger::log(LOG_ERR, "Unable to load the driver.\n"); } open_f(inspector); - } - else + } + else { rethrow_exception(current_exception()); } @@ -1287,7 +1295,7 @@ int falco_init(int argc, char **argv) if(!trace_filename.empty() && !trace_is_scap) { -#ifndef MINIMAL_BUILD +#ifndef MINIMAL_BUILD read_k8s_audit_trace_file(engine, outputs, trace_filename);