From 8d1582aa2b7b55374e77a2c27c275b17091c4ea3 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Tue, 7 Jul 2020 17:46:14 -0400 Subject: [PATCH 01/39] Move final "Success" logging statement out to main --- cfn-guard/src/lib.rs | 3 --- cfn-guard/src/main.rs | 2 ++ 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/cfn-guard/src/lib.rs b/cfn-guard/src/lib.rs index 3418addf2..d0684927c 100644 --- a/cfn-guard/src/lib.rs +++ b/cfn-guard/src/lib.rs @@ -163,9 +163,6 @@ fn check_resources( } } } - if result.is_empty() { - info!("All CloudFormation resources passed"); - } result } diff --git a/cfn-guard/src/main.rs b/cfn-guard/src/main.rs index 1a19391b7..772b2f440 100644 --- a/cfn-guard/src/main.rs +++ b/cfn-guard/src/main.rs @@ -87,5 +87,7 @@ fn main() { } else { process::exit(exit_code as i32); } + } else { + info!("All CloudFormation resources passed"); } } From dc44b5a70244aa471f3787f9f70ebc72b7c4ba51 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Thu, 2 Jul 2020 15:47:37 -0400 Subject: [PATCH 02/39] Update parser and guard_types for conditional rules --- cfn-guard/src/guard_types.rs | 19 +++++++-- cfn-guard/src/lib.rs | 75 +++++++++++++++++++----------------- cfn-guard/src/parser.rs | 49 +++++++++++++++++------ 3 files changed, 93 insertions(+), 50 deletions(-) diff --git a/cfn-guard/src/guard_types.rs b/cfn-guard/src/guard_types.rs index 9cb0932f5..fb1ed1ae5 100644 --- a/cfn-guard/src/guard_types.rs +++ b/cfn-guard/src/guard_types.rs @@ -6,6 +6,7 @@ pub mod enums { pub enum LineType { Assignment, Comment, + Conditional, Rule, WhiteSpace, } @@ -34,6 +35,12 @@ pub mod enums { OR, AND, } + + #[derive(Debug, Clone)] + pub enum RuleType { + CompoundRule(super::structs::CompoundRule), + ConditionalRule(super::structs::ConditionalRule), + } } pub mod structs { @@ -49,15 +56,21 @@ pub mod structs { pub(crate) custom_msg: Option, } - #[derive(Debug, Eq, PartialEq)] + #[derive(Debug, Clone, Eq, PartialEq)] pub struct CompoundRule { pub(crate) compound_type: super::enums::CompoundType, pub(crate) rule_list: Vec, } - #[derive(Debug, Eq, PartialEq)] + #[derive(Debug, Clone, Eq, PartialEq)] + pub struct ConditionalRule { + pub(crate) condition: CompoundRule, + pub(crate) consequent: CompoundRule, + } + + #[derive(Debug)] pub struct ParsedRuleSet { pub(crate) variables: HashMap, - pub(crate) rule_set: Vec, + pub(crate) rule_set: Vec, } } diff --git a/cfn-guard/src/lib.rs b/cfn-guard/src/lib.rs index d0684927c..001cff1bf 100644 --- a/cfn-guard/src/lib.rs +++ b/cfn-guard/src/lib.rs @@ -118,49 +118,52 @@ fn check_resources( let mut result: Vec = vec![]; for c_rule in parsed_rule_set.rule_set.iter() { info!("Applying rule '{:#?}'", &c_rule); - match c_rule.compound_type { - enums::CompoundType::OR => { - for (name, cfn_resource) in cfn_resources { - trace!("OR'ing [{}] against {:?}", name, c_rule); - let mut pass_fail = HashSet::new(); - let mut temp_results: Vec = vec![]; - let mut cfn_resource_map: HashMap = HashMap::new(); - cfn_resource_map.insert(name.clone(), cfn_resource.clone()); - for rule in &c_rule.rule_list { - match apply_rule( - &cfn_resource_map, + match c_rule { + enums::RuleType::ConditionalRule(r) => {} + enums::RuleType::CompoundRule(r) => match r.compound_type { + enums::CompoundType::OR => { + for (name, cfn_resource) in cfn_resources { + trace!("OR'ing [{}] against {:?}", name, r); + let mut pass_fail = HashSet::new(); + let mut temp_results: Vec = vec![]; + let mut cfn_resource_map: HashMap = HashMap::new(); + cfn_resource_map.insert(name.clone(), cfn_resource.clone()); + for rule in &r.rule_list { + match apply_rule( + &cfn_resource_map, + &rule, + &parsed_rule_set.variables, + strict_checks, + ) { + Some(rule_result) => { + pass_fail.insert("fail"); + temp_results.extend(rule_result); + } + None => { + pass_fail.insert("pass"); + } + } + } + trace! {"pass_fail set is {:?}", &pass_fail}; + trace! {"temp_results are {:?}", &temp_results}; + if !pass_fail.contains("pass") { + result.extend(temp_results); + } + } + } + enums::CompoundType::AND => { + for rule in &r.rule_list { + if let Some(rule_result) = apply_rule( + &cfn_resources, &rule, &parsed_rule_set.variables, strict_checks, ) { - Some(rule_result) => { - pass_fail.insert("fail"); - temp_results.extend(rule_result); - } - None => { - pass_fail.insert("pass"); - } + result.extend(rule_result); } } - trace! {"pass_fail set is {:?}", &pass_fail}; - trace! {"temp_results are {:?}", &temp_results}; - if !pass_fail.contains("pass") { - result.extend(temp_results); - } - } - } - enums::CompoundType::AND => { - for rule in &c_rule.rule_list { - if let Some(rule_result) = apply_rule( - &cfn_resources, - &rule, - &parsed_rule_set.variables, - strict_checks, - ) { - result.extend(rule_result); - } } - } + }, } } result diff --git a/cfn-guard/src/parser.rs b/cfn-guard/src/parser.rs index a332dc7fa..0a41b9db1 100644 --- a/cfn-guard/src/parser.rs +++ b/cfn-guard/src/parser.rs @@ -8,8 +8,8 @@ use log::{self, debug, error, trace}; use regex::{Captures, Regex}; use serde_json::Value; -use crate::guard_types::enums::{CompoundType, LineType, OpCode, RValueType}; -use crate::guard_types::structs::{CompoundRule, ParsedRuleSet, Rule}; +use crate::guard_types::enums::{CompoundType, LineType, OpCode, RValueType, RuleType}; +use crate::guard_types::structs::{CompoundRule, ConditionalRule, ParsedRuleSet, Rule}; use crate::util; use lazy_static::lazy_static; @@ -23,6 +23,7 @@ lazy_static! { static ref RULE_WITH_OPTIONAL_MESSAGE_REG: Regex = Regex::new( r"(?P\S+) +(?P[\w\.\*]+) +(?P\S+) +(?P[^\n\r]+) +<{2} *(?P.*)").unwrap(); static ref WHITE_SPACE_REG: Regex = Regex::new(r"\s+").unwrap(); + static ref CONDITIONAL_RULE_REG: Regex = Regex::new(r"(?P\S+) +if +(?P.+) +then +(?P.*)").unwrap(); } pub(crate) fn parse_rules( @@ -39,7 +40,7 @@ pub(crate) fn parse_rules( &cfn_resources ); - let mut rule_set: Vec = vec![]; + let mut rule_set: Vec = vec![]; let mut variables = HashMap::new(); let lines = rules_file_contents.lines(); @@ -78,15 +79,14 @@ pub(crate) fn parse_rules( } LineType::Comment => (), LineType::Rule => { - let compound_rule: CompoundRule = if is_or_rule(l) { - debug!("Line is an |OR| rule"); - process_or_rule(l, &cfn_resources) - } else { - debug!("Line is an 'AND' rule"); - process_and_rule(l, &cfn_resources) - }; + let compound_rule = parse_rule_line(l, &cfn_resources); debug!("Parsed rule is: {:#?}", &compound_rule); - rule_set.push(compound_rule); + rule_set.push(RuleType::CompoundRule(compound_rule)); + } + LineType::Conditional => { + let conditional_rule: ConditionalRule = process_conditional(l, &cfn_resources); + debug!("Parsed conditional is {:#?}", &conditional_rule); + rule_set.push(RuleType::ConditionalRule(conditional_rule)); } LineType::WhiteSpace => { debug!("Line is white space"); @@ -107,6 +107,30 @@ pub(crate) fn parse_rules( } } +fn parse_rule_line(l: &str, cfn_resources: &HashMap) -> CompoundRule { + let compound_rule: CompoundRule = if is_or_rule(l) { + debug!("Line is an |OR| rule"); + process_or_rule(l, &cfn_resources) + } else { + debug!("Line is an 'AND' rule"); + process_and_rule(l, &cfn_resources) + }; + compound_rule +} + +fn process_conditional(line: &str, cfn_resources: &HashMap) -> ConditionalRule { + let caps = CONDITIONAL_RULE_REG.captures(line).unwrap(); + trace!("ConditionalRule regex captures are {:#?}", &caps); + let conjd_caps_conditional = format!("{} {}", &caps["resource_type"], &caps["condition"]); + let conjd_caps_consequent = format!("{} {}", &caps["resource_type"], &caps["consequent"]); + let condition = parse_rule_line(&conjd_caps_conditional, cfn_resources); + let consequent = parse_rule_line(&conjd_caps_consequent, cfn_resources); + ConditionalRule { + condition: condition, + consequent: consequent, + } +} + fn find_line_type(line: &str) -> LineType { if COMMENT_REG.is_match(line) { return LineType::Comment; @@ -114,6 +138,9 @@ fn find_line_type(line: &str) -> LineType { if ASSIGN_REG.is_match(line) { return LineType::Assignment; }; + if CONDITIONAL_RULE_REG.is_match(line) { + return LineType::Conditional; + }; if RULE_REG.is_match(line) { return LineType::Rule; }; From b7e0adc1eaadc51fd2bc34a56c60a8b9484eeb14 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Tue, 7 Jul 2020 17:48:26 -0400 Subject: [PATCH 03/39] Add field addressing from "root" aka `.` of resources rather than just properties --- cfn-guard/src/lib.rs | 19 ++++++++++++-- cfn-guard/src/util.rs | 61 +++++++++++++++++++++++-------------------- 2 files changed, 50 insertions(+), 30 deletions(-) diff --git a/cfn-guard/src/lib.rs b/cfn-guard/src/lib.rs index 001cff1bf..51693e684 100644 --- a/cfn-guard/src/lib.rs +++ b/cfn-guard/src/lib.rs @@ -183,8 +183,23 @@ fn apply_rule( "Checking [{}] which is of type {}", &name, &cfn_resource["Type"] ); - let target_field: Vec<&str> = rule.field.split('.').collect(); - match util::get_resource_prop_value(&cfn_resource["Properties"], &target_field) { + let mut target_field: Vec<&str> = rule.field.split('.').collect(); + let (property_root, address) = match target_field.first() { + Some(x) => { + if *x == "" { + target_field.remove(0); + target_field.insert(0, "."); + (cfn_resource, target_field) + } else { + (&cfn_resource["Properties"], target_field) + } + } + None => { + error!("Invalid property address: {}", rule.field); + return None; + } + }; + match util::get_resource_prop_value(property_root, &address) { Err(e) => { if strict_checks { rule_result.push(match &rule.custom_msg { diff --git a/cfn-guard/src/util.rs b/cfn-guard/src/util.rs index e72cb80bc..925d736e6 100644 --- a/cfn-guard/src/util.rs +++ b/cfn-guard/src/util.rs @@ -70,6 +70,7 @@ pub fn convert_list_var_to_vec(rule_val: &str) -> Vec { fn match_props<'a>(props: &'a Value, n: &'a dyn serde_json::value::Index) -> Result<&'a Value, ()> { trace!("props are {:#?}", props); + match props.get(n) { Some(v) => Ok(v), None => Err(()), @@ -84,40 +85,44 @@ pub fn get_resource_prop_value(props: &Value, field: &[&str]) -> Result() { - Ok(n) => { - trace!( - "next_field is {:?} and field_list is now {:?}", - &n, - &field_list - ); + if next_field == "." { + get_resource_prop_value(&props, &field_list) + } else { + match next_field.parse::() { + Ok(n) => { + trace!( + "next_field is {:?} and field_list is now {:?}", + &n, + &field_list + ); - match match_props(props, &n) { - Ok(v) => { - if !field_list.is_empty() { - get_resource_prop_value(&v, &field_list) - } else { - Ok(v.clone()) + match match_props(props, &n) { + Ok(v) => { + if !field_list.is_empty() { + get_resource_prop_value(&v, &field_list) + } else { + Ok(v.clone()) + } } + Err(_) => Err(n.to_string()), } - Err(_) => Err(n.to_string()), } - } - Err(_) => { - trace!( - "next_field is {:?} and field_list is now {:?}", - &next_field, - &field_list - ); - match match_props(props, &next_field) { - Ok(v) => { - if !field_list.is_empty() { - get_resource_prop_value(&v, &field_list) - } else { - Ok(v.clone()) + Err(_) => { + trace!( + "next_field is {:?} and field_list is now {:?}", + &next_field, + &field_list + ); + match match_props(props, &next_field) { + Ok(v) => { + if !field_list.is_empty() { + get_resource_prop_value(&v, &field_list) + } else { + Ok(v.clone()) + } } + Err(_) => Err(next_field.to_string()), } - Err(_) => Err(next_field.to_string()), } } } From 9f33df0d1bd5ce6a67c2b8e1b7d21e789e088dbe Mon Sep 17 00:00:00 2001 From: nathanmc Date: Tue, 7 Jul 2020 17:51:03 -0400 Subject: [PATCH 04/39] Initial cut at checking `condition` and `consequent` using existing field interrogation semantics. Result output still rough. --- cfn-guard/src/lib.rs | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/cfn-guard/src/lib.rs b/cfn-guard/src/lib.rs index 51693e684..5c60a5273 100644 --- a/cfn-guard/src/lib.rs +++ b/cfn-guard/src/lib.rs @@ -114,12 +114,38 @@ fn check_resources( parsed_rule_set: &structs::ParsedRuleSet, strict_checks: bool, ) -> Vec { + // TODO: Change this to a Result info!("Checking resources"); let mut result: Vec = vec![]; for c_rule in parsed_rule_set.rule_set.iter() { info!("Applying rule '{:#?}'", &c_rule); match c_rule { - enums::RuleType::ConditionalRule(r) => {} + enums::RuleType::ConditionalRule(r) => { + trace!("Conditional rule is {:#?}", r); + for (name, cfn_resource) in cfn_resources { + trace!("Checking condition: {:?}", r.condition); + let mut cfn_resource_map: HashMap = HashMap::new(); + cfn_resource_map.insert(name.clone(), cfn_resource.clone()); + let condition_rule_set = structs::ParsedRuleSet { + variables: parsed_rule_set.variables.clone(), + rule_set: vec![enums::RuleType::CompoundRule(r.clone().condition)], + }; + let condition = check_resources(&cfn_resource_map, &condition_rule_set, true); + if condition.is_empty() { + // TODO: Change this to a Result + let consequent_rule_set = structs::ParsedRuleSet { + variables: parsed_rule_set.variables.clone(), + rule_set: vec![enums::RuleType::CompoundRule(r.clone().consequent)], + }; + let postscript = format!("when {:?}", r.condition.rule_list); + let temp_result = + check_resources(&cfn_resource_map, &consequent_rule_set, strict_checks) + .into_iter() + .map(|x| format!("{} {}", x, postscript)); + result.extend(temp_result); + } + } + } enums::RuleType::CompoundRule(r) => match r.compound_type { enums::CompoundType::OR => { for (name, cfn_resource) in cfn_resources { From 9dc0ce489fee61c9f78a58cd8d1c67f54808af5b Mon Sep 17 00:00:00 2001 From: nathanmc Date: Wed, 8 Jul 2020 13:45:42 -0400 Subject: [PATCH 05/39] Add raw_rule to CompoundRule struct to make it available for output in result messages --- cfn-guard/src/guard_types.rs | 1 + cfn-guard/src/lib.rs | 2 +- cfn-guard/src/parser.rs | 2 ++ 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/cfn-guard/src/guard_types.rs b/cfn-guard/src/guard_types.rs index fb1ed1ae5..03e2219fb 100644 --- a/cfn-guard/src/guard_types.rs +++ b/cfn-guard/src/guard_types.rs @@ -59,6 +59,7 @@ pub mod structs { #[derive(Debug, Clone, Eq, PartialEq)] pub struct CompoundRule { pub(crate) compound_type: super::enums::CompoundType, + pub(crate) raw_rule: String, pub(crate) rule_list: Vec, } diff --git a/cfn-guard/src/lib.rs b/cfn-guard/src/lib.rs index 5c60a5273..e62336f52 100644 --- a/cfn-guard/src/lib.rs +++ b/cfn-guard/src/lib.rs @@ -137,7 +137,7 @@ fn check_resources( variables: parsed_rule_set.variables.clone(), rule_set: vec![enums::RuleType::CompoundRule(r.clone().consequent)], }; - let postscript = format!("when {:?}", r.condition.rule_list); + let postscript = format!("when {}", r.condition.raw_rule); let temp_result = check_resources(&cfn_resource_map, &consequent_rule_set, strict_checks) .into_iter() diff --git a/cfn-guard/src/parser.rs b/cfn-guard/src/parser.rs index 0a41b9db1..794d03992 100644 --- a/cfn-guard/src/parser.rs +++ b/cfn-guard/src/parser.rs @@ -173,6 +173,7 @@ fn process_or_rule(line: &str, cfn_resources: &HashMap) -> Compou } CompoundRule { compound_type: CompoundType::OR, + raw_rule: line.to_string(), rule_list: rules, } } @@ -180,6 +181,7 @@ fn process_or_rule(line: &str, cfn_resources: &HashMap) -> Compou fn process_and_rule(line: &str, cfn_resources: &HashMap) -> CompoundRule { CompoundRule { compound_type: CompoundType::AND, + raw_rule: line.to_string(), rule_list: destructure_rule(line, cfn_resources), } } From 1e8cc465e68145b69b8fa9718b8cc0ca1a0f1394 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Thu, 9 Jul 2020 12:13:47 -0400 Subject: [PATCH 06/39] Move check_resources to return Option --- cfn-guard/src/lib.rs | 63 +++++++++++++++++++++++++++++++------------- 1 file changed, 45 insertions(+), 18 deletions(-) diff --git a/cfn-guard/src/lib.rs b/cfn-guard/src/lib.rs index e62336f52..104ee8f01 100644 --- a/cfn-guard/src/lib.rs +++ b/cfn-guard/src/lib.rs @@ -99,7 +99,13 @@ pub fn run_check( info!("Parsing rule set"); let parsed_rule_set = parser::parse_rules(&cleaned_rules_file_contents, &cfn_resources); - let mut outcome = check_resources(&cfn_resources, &parsed_rule_set, strict_checks); + let mut outcome: Vec = vec![]; + match check_resources(&cfn_resources, &parsed_rule_set, strict_checks) { + Some(x) => { + outcome.extend(x); + } + None => (), + } outcome.sort(); let exit_code = match outcome.len() { @@ -113,8 +119,7 @@ fn check_resources( cfn_resources: &HashMap, parsed_rule_set: &structs::ParsedRuleSet, strict_checks: bool, -) -> Vec { - // TODO: Change this to a Result +) -> Option> { info!("Checking resources"); let mut result: Vec = vec![]; for c_rule in parsed_rule_set.rule_set.iter() { @@ -124,26 +129,44 @@ fn check_resources( trace!("Conditional rule is {:#?}", r); for (name, cfn_resource) in cfn_resources { trace!("Checking condition: {:?}", r.condition); + let mut cfn_resource_map: HashMap = HashMap::new(); cfn_resource_map.insert(name.clone(), cfn_resource.clone()); + trace!("Temporary resource map is {:#?}", cfn_resource_map); + let condition_rule_set = structs::ParsedRuleSet { variables: parsed_rule_set.variables.clone(), rule_set: vec![enums::RuleType::CompoundRule(r.clone().condition)], }; - let condition = check_resources(&cfn_resource_map, &condition_rule_set, true); - if condition.is_empty() { - // TODO: Change this to a Result - let consequent_rule_set = structs::ParsedRuleSet { - variables: parsed_rule_set.variables.clone(), - rule_set: vec![enums::RuleType::CompoundRule(r.clone().consequent)], - }; - let postscript = format!("when {}", r.condition.raw_rule); - let temp_result = - check_resources(&cfn_resource_map, &consequent_rule_set, strict_checks) - .into_iter() - .map(|x| format!("{} {}", x, postscript)); - result.extend(temp_result); - } + trace!( + "condition_rule_set is {{variables: {:#?}, rule_set: {:#?}}}", + util::filter_for_env_vars(&condition_rule_set.variables), + condition_rule_set.rule_set + ); + + // Use the existing rules logic to see if there's a hit on the Condition clause + match check_resources(&cfn_resource_map, &condition_rule_set, true) { + Some(_) => (), // A result from a condition check means that it *wasn't* met (by def) + None => { + let consequent_rule_set = structs::ParsedRuleSet { + variables: parsed_rule_set.variables.clone(), + rule_set: vec![enums::RuleType::CompoundRule(r.clone().consequent)], + }; + let postscript = format!("when {}", r.condition.raw_rule); + match check_resources( + &cfn_resource_map, + &consequent_rule_set, + strict_checks, + ) { + Some(x) => { + let temp_result = + x.into_iter().map(|x| format!("{} {}", x, postscript)); + result.extend(temp_result); + } + None => (), + }; + } + }; } } enums::RuleType::CompoundRule(r) => match r.compound_type { @@ -192,7 +215,11 @@ fn check_resources( }, } } - result + if !result.is_empty() { + Some(result) + } else { + None + } } fn apply_rule( From bd671e8738dc8df45ce018b09ef5bfe9e90d3619 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Thu, 9 Jul 2020 17:35:42 -0400 Subject: [PATCH 07/39] Trim redundant ConditionalRule fields --- cfn-guard/src/parser.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cfn-guard/src/parser.rs b/cfn-guard/src/parser.rs index 794d03992..20ad0230c 100644 --- a/cfn-guard/src/parser.rs +++ b/cfn-guard/src/parser.rs @@ -126,8 +126,8 @@ fn process_conditional(line: &str, cfn_resources: &HashMap) -> Co let condition = parse_rule_line(&conjd_caps_conditional, cfn_resources); let consequent = parse_rule_line(&conjd_caps_consequent, cfn_resources); ConditionalRule { - condition: condition, - consequent: consequent, + condition, + consequent, } } From 9c3eb70eb66b71392e4f20be585600f7f9c95239 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Fri, 10 Jul 2020 13:36:34 -0400 Subject: [PATCH 08/39] Add test coverage output files to .gitignore --- .gitignore | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 3970208c6..dd68378fd 100644 --- a/.gitignore +++ b/.gitignore @@ -8,4 +8,6 @@ cloudformation-guard.tar.gz *# .idea/ *~ -cmake-build-debug \ No newline at end of file +cmake-build-debug +*gcno +*gcda From cbc25e22982a127e50c5b76a75c57071293cae7e Mon Sep 17 00:00:00 2001 From: nathanmc Date: Fri, 10 Jul 2020 13:37:40 -0400 Subject: [PATCH 09/39] Add test for conditional pass/fail to functional tests --- .../tests/conditional-ddb-template.ruleset | 3 + cfn-guard/tests/conditional-ddb-template.yaml | 64 +++++++++++++++++++ cfn-guard/tests/functional.rs | 19 ++++++ 3 files changed, 86 insertions(+) create mode 100644 cfn-guard/tests/conditional-ddb-template.ruleset create mode 100644 cfn-guard/tests/conditional-ddb-template.yaml diff --git a/cfn-guard/tests/conditional-ddb-template.ruleset b/cfn-guard/tests/conditional-ddb-template.ruleset new file mode 100644 index 000000000..977519e8f --- /dev/null +++ b/cfn-guard/tests/conditional-ddb-template.ruleset @@ -0,0 +1,3 @@ +# https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-product-attribute-reference.html +AWS::DynamoDB::Table if Tags == /.*PROD.*/ then .DeletionPolicy == Retain +AWS::DynamoDB::Table if Tags == /.*PROD.*/ then .DeletionPolicy != Retain diff --git a/cfn-guard/tests/conditional-ddb-template.yaml b/cfn-guard/tests/conditional-ddb-template.yaml new file mode 100644 index 000000000..402497092 --- /dev/null +++ b/cfn-guard/tests/conditional-ddb-template.yaml @@ -0,0 +1,64 @@ +{ + "Resources": { + "DDBTable": { + "Type": "AWS::DynamoDB::Table", + "UpdateReplacePolicy": "Abort", + "DeletionPolicy": "Retain", + "Properties": { + "AttributeDefinitions": [ + { + "AttributeName": "ArtistId", + "AttributeType": "S" + }, + { + "AttributeName": "Concert", + "AttributeType": "S" + }, + { + "AttributeName": "TicketSales", + "AttributeType": "S" + } + ], + "KeySchema": [ + { + "AttributeName": "ArtistId", + + "KeyType": "HASH" + }, + { + "AttributeName": "Concert", + "KeyType": "RANGE" + } + ], + "GlobalSecondaryIndexes": [ + { + + "IndexName": "GSI", + "KeySchema": [ + + { + + "AttributeName": "TicketSales", + "KeyType": "HASH" + } + ], + "Projection": { + "ProjectionType": "KEYS_ONLY" + }, + "ProvisionedThroughput": { + "ReadCapacityUnits": 5, + "WriteCapacityUnits": 5 + } + } + ], + "ProvisionedThroughput": { + "ReadCapacityUnits": 5, + "WriteCapacityUnits": 5 + }, + "Tags": [ + {"Key": "ENV", "Value": "PROD"} + ] + } + } + } +} diff --git a/cfn-guard/tests/functional.rs b/cfn-guard/tests/functional.rs index c24b6f977..41a56c86e 100644 --- a/cfn-guard/tests/functional.rs +++ b/cfn-guard/tests/functional.rs @@ -1476,4 +1476,23 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, ) ); } + + #[test] + fn test_conditional_check() { + let template_contents = fs::read_to_string("tests/conditional-ddb-template.yaml") + .unwrap_or_else(|err| format!("{}", err)); + + let mut rules_file_contents = String::from("AWS::DynamoDB::Table if Tags == /.*PROD.*/ then .DeletionPolicy == Retain"); + assert_eq!( + cfn_guard::run_check(&template_contents, &rules_file_contents, false), + (vec![], 0) + ); + + rules_file_contents = String::from("AWS::DynamoDB::Table if Tags == /.*PROD.*/ then .DeletionPolicy != Retain"); + assert_eq!( + cfn_guard::run_check(&template_contents, &rules_file_contents, false), + (vec![String::from("[DDBTable] failed because [.DeletionPolicy] is [Retain] and that value is not permitted when AWS::DynamoDB::Table Tags == /.*PROD.*/")], + 2) + ) + } } From ffda014fac52b9921680228b70e06e606bf30357 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Fri, 10 Jul 2020 16:30:07 -0400 Subject: [PATCH 10/39] cargo bump to 0.6.0 --- cfn-guard-lambda/Cargo.lock | 4 ++-- cfn-guard-lambda/Cargo.toml | 2 +- cfn-guard-rulegen-lambda/Cargo.lock | 6 +++--- cfn-guard-rulegen-lambda/Cargo.toml | 2 +- cfn-guard-rulegen/Cargo.lock | 2 +- cfn-guard-rulegen/Cargo.toml | 2 +- cfn-guard/Cargo.lock | 2 +- cfn-guard/Cargo.toml | 2 +- 8 files changed, 11 insertions(+), 11 deletions(-) diff --git a/cfn-guard-lambda/Cargo.lock b/cfn-guard-lambda/Cargo.lock index d7cf68f09..8f9091765 100644 --- a/cfn-guard-lambda/Cargo.lock +++ b/cfn-guard-lambda/Cargo.lock @@ -88,7 +88,7 @@ checksum = "4785bdd1c96b2a846b2bd7cc02e86b6b3dbf14e7e53446c4f54c92a361040822" [[package]] name = "cfn-guard" -version = "0.5.2" +version = "0.6.0" dependencies = [ "clap", "lazy_static", @@ -102,7 +102,7 @@ dependencies = [ [[package]] name = "cfn-guard-lambda" -version = "0.5.2" +version = "0.6.0" dependencies = [ "cfn-guard", "lambda_runtime", diff --git a/cfn-guard-lambda/Cargo.toml b/cfn-guard-lambda/Cargo.toml index 1912eb919..a9130376f 100644 --- a/cfn-guard-lambda/Cargo.toml +++ b/cfn-guard-lambda/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "cfn-guard-lambda" -version = "0.5.2" +version = "0.6.0" edition = "2018" [dependencies] diff --git a/cfn-guard-rulegen-lambda/Cargo.lock b/cfn-guard-rulegen-lambda/Cargo.lock index b2cf8bd82..6cb6134a2 100644 --- a/cfn-guard-rulegen-lambda/Cargo.lock +++ b/cfn-guard-rulegen-lambda/Cargo.lock @@ -89,7 +89,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" [[package]] name = "cfn-guard-rulegen" -version = "0.5.2" +version = "0.6.0" dependencies = [ "clap 2.33.0 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.7 (registry+https://github.com/rust-lang/crates.io-index)", @@ -101,9 +101,9 @@ dependencies = [ [[package]] name = "cfn-guard-rulegen-lambda" -version = "0.5.2" +version = "0.6.0" dependencies = [ - "cfn-guard-rulegen 0.5.2", + "cfn-guard-rulegen 0.6.0", "lambda_runtime 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.7 (registry+https://github.com/rust-lang/crates.io-index)", "serde 1.0.95 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/cfn-guard-rulegen-lambda/Cargo.toml b/cfn-guard-rulegen-lambda/Cargo.toml index 80dda92f4..d995eb978 100644 --- a/cfn-guard-rulegen-lambda/Cargo.toml +++ b/cfn-guard-rulegen-lambda/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "cfn-guard-rulegen-lambda" -version = "0.5.2" +version = "0.6.0" edition = "2018" [dependencies] diff --git a/cfn-guard-rulegen/Cargo.lock b/cfn-guard-rulegen/Cargo.lock index 4148815f7..18a368308 100644 --- a/cfn-guard-rulegen/Cargo.lock +++ b/cfn-guard-rulegen/Cargo.lock @@ -40,7 +40,7 @@ checksum = "4785bdd1c96b2a846b2bd7cc02e86b6b3dbf14e7e53446c4f54c92a361040822" [[package]] name = "cfn-guard-rulegen" -version = "0.5.2" +version = "0.6.0" dependencies = [ "clap", "log", diff --git a/cfn-guard-rulegen/Cargo.toml b/cfn-guard-rulegen/Cargo.toml index 5b987f90b..fbc22b4be 100644 --- a/cfn-guard-rulegen/Cargo.toml +++ b/cfn-guard-rulegen/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "cfn-guard-rulegen" -version = "0.5.2" +version = "0.6.0" edition = "2018" [dependencies] diff --git a/cfn-guard/Cargo.lock b/cfn-guard/Cargo.lock index 8bad63b83..a4e46b5e9 100644 --- a/cfn-guard/Cargo.lock +++ b/cfn-guard/Cargo.lock @@ -49,7 +49,7 @@ checksum = "4785bdd1c96b2a846b2bd7cc02e86b6b3dbf14e7e53446c4f54c92a361040822" [[package]] name = "cfn-guard" -version = "0.5.2" +version = "0.6.0" dependencies = [ "clap", "lazy_static", diff --git a/cfn-guard/Cargo.toml b/cfn-guard/Cargo.toml index 2302ad141..86aade01f 100644 --- a/cfn-guard/Cargo.toml +++ b/cfn-guard/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "cfn-guard" -version = "0.5.2" +version = "0.6.0" edition = "2018" [dependencies] From e7de87a203a7aa78c099cac11f83f1e0cb86a7e3 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Fri, 10 Jul 2020 16:30:50 -0400 Subject: [PATCH 11/39] Make the ebs Example easier to understand. Update README to reflect new output. --- Examples/ebs-volume-template.json | 4 ++-- Examples/ebs-volume-template.ruleset | 4 +--- README.md | 20 ++++++++------------ 3 files changed, 11 insertions(+), 17 deletions(-) diff --git a/Examples/ebs-volume-template.json b/Examples/ebs-volume-template.json index 36fe0b755..0d9d21b55 100644 --- a/Examples/ebs-volume-template.json +++ b/Examples/ebs-volume-template.json @@ -3,7 +3,7 @@ "NewVolume" : { "Type" : "AWS::EC2::Volume", "Properties" : { - "Size" : 101, + "Size" : 500, "Encrypted": false, "AvailabilityZone" : "us-west-2b" } @@ -11,7 +11,7 @@ "NewVolume2" : { "Type" : "AWS::EC2::Volume", "Properties" : { - "Size" : 99, + "Size" : 50, "Encrypted": false, "AvailabilityZone" : "us-west-2c" } diff --git a/Examples/ebs-volume-template.ruleset b/Examples/ebs-volume-template.ruleset index 2965d82d8..489afed7c 100644 --- a/Examples/ebs-volume-template.ruleset +++ b/Examples/ebs-volume-template.ruleset @@ -1,6 +1,4 @@ let encryption_flag = true -let allowed_azs = [us-east-1a,us-east-1b,us-east-1c] -AWS::EC2::Volume AvailabilityZone IN %allowed_azs AWS::EC2::Volume Encrypted == %encryption_flag -AWS::EC2::Volume Size == 100 +AWS::EC2::Volume Size <= 100 diff --git a/README.md b/README.md index ba50d7763..71e51ac59 100644 --- a/README.md +++ b/README.md @@ -19,7 +19,7 @@ For example, given a CloudFormation template: "NewVolume" : { "Type" : "AWS::EC2::Volume", "Properties" : { - "Size" : 101, + "Size" : 500, "Encrypted": false, "AvailabilityZone" : "us-west-2b" } @@ -27,7 +27,7 @@ For example, given a CloudFormation template: "NewVolume2" : { "Type" : "AWS::EC2::Volume", "Properties" : { - "Size" : 99, + "Size" : 50, "Encrypted": false, "AvailabilityZone" : "us-west-2c" } @@ -40,24 +40,20 @@ And a set of rules: ``` let encryption_flag = true -let allowed_azs = [us-east-1a,us-east-1b,us-east-1c] -AWS::EC2::Volume AvailabilityZone IN %allowed_azs AWS::EC2::Volume Encrypted == %encryption_flag -AWS::EC2::Volume Size == 100 +AWS::EC2::Volume Size <= 100 ``` You can check the template to ensure that it adheres to the rules. ``` $> cfn-guard -t Examples/ebs_volume_template.json -r Examples/ebs_volume_template.ruleset - "[NewVolume2] failed because [Encrypted] is [false] and the permitted value is [true]" - "[NewVolume2] failed because [Size] is [99] and the permitted value is [100]" - "[NewVolume2] failed because [us-west-2c] is not in [us-east-1a,us-east-1b,us-east-1c] for [AvailabilityZone]" - "[NewVolume] failed because [Encrypted] is [false] and the permitted value is [true]" - "[NewVolume] failed because [Size] is [101] and the permitted value is [100]" - "[NewVolume] failed because [us-west-2b] is not in [us-east-1a,us-east-1b,us-east-1c] for [AvailabilityZone]" - Number of failures: 6 + +[NewVolume2] failed because [Encrypted] is [false] and the permitted value is [true] +[NewVolume] failed because [Encrypted] is [false] and the permitted value is [true] +[NewVolume] failed because [Size] is [500] and the permitted value is [<= 100] +Number of failures: 3 ``` ### Evaluating Security Policies From b7c6fde8570704769585d411e336d199e5d9e9de Mon Sep 17 00:00:00 2001 From: nathanmc Date: Tue, 14 Jul 2020 17:09:27 -0400 Subject: [PATCH 12/39] Make logging more readable for conditionals --- cfn-guard/src/lib.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cfn-guard/src/lib.rs b/cfn-guard/src/lib.rs index 104ee8f01..6fd5fc600 100644 --- a/cfn-guard/src/lib.rs +++ b/cfn-guard/src/lib.rs @@ -147,7 +147,8 @@ fn check_resources( // Use the existing rules logic to see if there's a hit on the Condition clause match check_resources(&cfn_resource_map, &condition_rule_set, true) { Some(_) => (), // A result from a condition check means that it *wasn't* met (by def) - None => { + None => { + trace!("Condition met for {}", r.condition.raw_rule); let consequent_rule_set = structs::ParsedRuleSet { variables: parsed_rule_set.variables.clone(), rule_set: vec![enums::RuleType::CompoundRule(r.clone().consequent)], @@ -193,8 +194,8 @@ fn check_resources( } } } - trace! {"pass_fail set is {:?}", &pass_fail}; trace! {"temp_results are {:?}", &temp_results}; + trace! {"pass_fail set is {:?}", &pass_fail}; if !pass_fail.contains("pass") { result.extend(temp_results); } From 8fd0a7581140ddfdbc0618ce45127f817013b453 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Thu, 16 Jul 2020 06:24:50 -0400 Subject: [PATCH 13/39] Move var_value in regex to be + instead of * --- cfn-guard/src/parser.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cfn-guard/src/parser.rs b/cfn-guard/src/parser.rs index 20ad0230c..9b68bd796 100644 --- a/cfn-guard/src/parser.rs +++ b/cfn-guard/src/parser.rs @@ -16,7 +16,7 @@ use lazy_static::lazy_static; // This sets it up so the regexen only get compiled once // See: https://docs.rs/regex/1.3.9/regex/#example-avoid-compiling-the-same-regex-in-a-loop lazy_static! { - static ref ASSIGN_REG: Regex = Regex::new(r"let (?P\w+) +(?P\S+) +(?P.*)").unwrap(); + static ref ASSIGN_REG: Regex = Regex::new(r"let (?P\w+) +(?P\S+) +(?P.+)").unwrap(); static ref RULE_REG: Regex = Regex::new(r"(?P\S+) +(?P[\w\.\*]+) +(?P\S+) +(?P[^\n\r]+)").unwrap(); static ref COMMENT_REG: Regex = Regex::new(r#"#(?P.*)"#).unwrap(); static ref WILDCARD_OR_RULE_REG: Regex = Regex::new(r"(\S+) (\S+\*\S*) (==|IN) (.+)").unwrap(); From 8a3f085a647ef45e5c4a7713e863980a089bb29f Mon Sep 17 00:00:00 2001 From: nathanmc Date: Thu, 16 Jul 2020 15:31:38 -0400 Subject: [PATCH 14/39] Move functions to Result<> types to make error handling more elegant and refactoring more clear --- cfn-guard/src/lib.rs | 62 +++++++------- cfn-guard/src/parser.rs | 150 +++++++++++++++++++++++----------- cfn-guard/tests/functional.rs | 147 +++++++++++++++++---------------- 3 files changed, 210 insertions(+), 149 deletions(-) diff --git a/cfn-guard/src/lib.rs b/cfn-guard/src/lib.rs index 6fd5fc600..1287a79dc 100644 --- a/cfn-guard/src/lib.rs +++ b/cfn-guard/src/lib.rs @@ -36,16 +36,20 @@ pub fn run( rules_file_contents.to_string() ); - let (outcome, exit_code) = run_check(&template_contents, &rules_file_contents, strict_checks); - debug!("Outcome was: '{:#?}'", &outcome); - Ok((outcome, exit_code)) + match run_check(&template_contents, &rules_file_contents, strict_checks) { + Ok(res) => { + debug!("Outcome was: '{:#?}'", &res.0); + Ok(res) + } + Err(e) => Err(e.into()), + } } pub fn run_check( template_file_contents: &str, rules_file_contents: &str, strict_checks: bool, -) -> (Vec, usize) { +) -> Result<(Vec, usize), String> { info!("Loading CloudFormation Template and Rule Set"); debug!("Entered run_check"); @@ -69,13 +73,10 @@ pub fn run_check( Err(_) => match serde_yaml::from_str(&cleaned_template_file_contents) { Ok(y) => y, Err(e) => { - return ( - vec![format!( - "ERROR: Template file format was unreadable as json or yaml: {}", - e - )], - 1, - ); + return Err(format!( + "Template file format was unreadable as json or yaml: {}", + e + )); } }, }; @@ -84,12 +85,8 @@ pub fn run_check( let cfn_resources: HashMap = match cfn_template.get("Resources") { Some(r) => serde_json::from_value(r.clone()).unwrap(), None => { - return ( - vec![ - "ERROR: Template file does not contain a [Resources] section to check" - .to_string(), - ], - 1, + return Err( + "Template file does not contain a [Resources] section to check".to_string(), ); } }; @@ -97,22 +94,25 @@ pub fn run_check( trace!("CFN resources are: {:?}", cfn_resources); info!("Parsing rule set"); - let parsed_rule_set = parser::parse_rules(&cleaned_rules_file_contents, &cfn_resources); + match parser::parse_rules(&cleaned_rules_file_contents, &cfn_resources) { + Ok(pr) => { + let mut outcome: Vec = vec![]; + match check_resources(&cfn_resources, &pr, strict_checks) { + Some(x) => { + outcome.extend(x); + } + None => (), + } + outcome.sort(); - let mut outcome: Vec = vec![]; - match check_resources(&cfn_resources, &parsed_rule_set, strict_checks) { - Some(x) => { - outcome.extend(x); + let exit_code = match outcome.len() { + 0 => 0, + _ => 2, + }; + return Ok((outcome, exit_code)); } - None => (), + Err(e) => Err(e), } - outcome.sort(); - - let exit_code = match outcome.len() { - 0 => 0, - _ => 2, - }; - (outcome, exit_code) } fn check_resources( @@ -147,7 +147,7 @@ fn check_resources( // Use the existing rules logic to see if there's a hit on the Condition clause match check_resources(&cfn_resource_map, &condition_rule_set, true) { Some(_) => (), // A result from a condition check means that it *wasn't* met (by def) - None => { + None => { trace!("Condition met for {}", r.condition.raw_rule); let consequent_rule_set = structs::ParsedRuleSet { variables: parsed_rule_set.variables.clone(), diff --git a/cfn-guard/src/parser.rs b/cfn-guard/src/parser.rs index 9b68bd796..544821202 100644 --- a/cfn-guard/src/parser.rs +++ b/cfn-guard/src/parser.rs @@ -29,7 +29,7 @@ lazy_static! { pub(crate) fn parse_rules( rules_file_contents: &str, cfn_resources: &HashMap, -) -> ParsedRuleSet { +) -> Result { debug!("Entered parse_rules"); trace!( "Parse rules entered with rules_file_contents: {:#?}", @@ -56,8 +56,8 @@ pub(crate) fn parse_rules( match line_type { LineType::Assignment => { let caps = match process_assignment(l) { - Some(a) => a, - None => continue, + Ok(a) => a, + Err(e) => return Err(e), }; trace!("Parsed assignment's captures are: {:#?}", &caps); if caps["operator"] != *"=" { @@ -78,16 +78,20 @@ pub(crate) fn parse_rules( variables.insert(var_name, var_value); } LineType::Comment => (), - LineType::Rule => { - let compound_rule = parse_rule_line(l, &cfn_resources); - debug!("Parsed rule is: {:#?}", &compound_rule); - rule_set.push(RuleType::CompoundRule(compound_rule)); - } - LineType::Conditional => { - let conditional_rule: ConditionalRule = process_conditional(l, &cfn_resources); - debug!("Parsed conditional is {:#?}", &conditional_rule); - rule_set.push(RuleType::ConditionalRule(conditional_rule)); - } + LineType::Rule => match parse_rule_line(l, &cfn_resources) { + Ok(prl) => { + debug!("Parsed rule is: {:#?}", &prl); + rule_set.push(RuleType::CompoundRule(prl)) + } + Err(e) => return Err(e), + }, + LineType::Conditional => match process_conditional(l, &cfn_resources) { + Ok(c) => { + debug!("Parsed conditional is {:#?}", &c); + rule_set.push(RuleType::ConditionalRule(c)); + } + Err(e) => return Err(e), + }, LineType::WhiteSpace => { debug!("Line is white space"); continue; @@ -101,34 +105,70 @@ pub(crate) fn parse_rules( let filtered_env_vars = util::filter_for_env_vars(&variables); debug!("Variables dictionary is {:?}", &filtered_env_vars); debug!("Rule Set is {:#?}", &rule_set); - ParsedRuleSet { + Ok(ParsedRuleSet { variables, rule_set, - } + }) } -fn parse_rule_line(l: &str, cfn_resources: &HashMap) -> CompoundRule { - let compound_rule: CompoundRule = if is_or_rule(l) { - debug!("Line is an |OR| rule"); - process_or_rule(l, &cfn_resources) - } else { - debug!("Line is an 'AND' rule"); - process_and_rule(l, &cfn_resources) - }; - compound_rule +fn parse_rule_line( + l: &str, + cfn_resources: &HashMap, +) -> Result { + match is_or_rule(l) { + true => { + debug!("Line is an |OR| rule"); + match process_or_rule(l, &cfn_resources) { + Ok(r) => Ok(r), + Err(e) => Err(e), + } + } + false => { + debug!("Line is an 'AND' rule"); + match process_and_rule(l, &cfn_resources) { + Ok(r) => Ok(r), + Err(e) => return Err(e), + } + } + } } -fn process_conditional(line: &str, cfn_resources: &HashMap) -> ConditionalRule { +fn process_conditional( + line: &str, + cfn_resources: &HashMap, +) -> Result { let caps = CONDITIONAL_RULE_REG.captures(line).unwrap(); trace!("ConditionalRule regex captures are {:#?}", &caps); + let conjd_caps_conditional = format!("{} {}", &caps["resource_type"], &caps["condition"]); - let conjd_caps_consequent = format!("{} {}", &caps["resource_type"], &caps["consequent"]); - let condition = parse_rule_line(&conjd_caps_conditional, cfn_resources); - let consequent = parse_rule_line(&conjd_caps_consequent, cfn_resources); - ConditionalRule { - condition, - consequent, + trace!("conjd_caps_conditional is {:#?}", conjd_caps_conditional); + match parse_rule_line(&conjd_caps_conditional, cfn_resources) { + Ok(condition) => { + let conjd_caps_consequent = + format!("{} {}", &caps["resource_type"], &caps["consequent"]); + trace!("conjd_caps_consequent is {:#?}", conjd_caps_consequent); + match parse_rule_line(&conjd_caps_consequent, cfn_resources) { + Ok(consequent) => Ok(ConditionalRule { + condition, + consequent, + }), + Err(e) => Err(e), + } + } + Err(e) => Err(e), } + // if let condition = parse_rule_line(&conjd_caps_conditional, cfn_resources) + // .unwrap_or_else(|err| return Err(err)); + // + // let conjd_caps_consequent = format!("{} {}", &caps["resource_type"], &caps["consequent"]); + // trace!("conjd_caps_consequent is {:#?}", conjd_caps_consequent); + // if let consequent = parse_rule_line(&conjd_caps_consequent, cfn_resources) + // .unwrap_or_else(|err| return Err(err)); + // + // ok(conditionalrule { + // condition, + // consequent, + // }) } fn find_line_type(line: &str) -> LineType { @@ -152,10 +192,10 @@ fn find_line_type(line: &str) -> LineType { process::exit(1) } -fn process_assignment(line: &str) -> Option { +fn process_assignment(line: &str) -> Result { match ASSIGN_REG.captures(line) { - Some(c) => Some(c), - None => None, + Some(c) => Ok(c), + None => Err(format!("Invalid assignment statement: '{}", line)), } } @@ -163,30 +203,45 @@ fn is_or_rule(line: &str) -> bool { line.contains("|OR|") || WILDCARD_OR_RULE_REG.is_match(line) } -fn process_or_rule(line: &str, cfn_resources: &HashMap) -> CompoundRule { +fn process_or_rule( + line: &str, + cfn_resources: &HashMap, +) -> Result { trace!("Entered process_or_rule"); let branches = line.split("|OR|"); debug!("Rule branches are: {:#?}", &branches); let mut rules: Vec = vec![]; for b in branches { - rules.append(destructure_rule(b.trim(), cfn_resources).as_mut()); + match destructure_rule(b.trim(), cfn_resources) { + Ok(r) => rules.append(&mut r.clone()), + Err(e) => return Err(e), + } } - CompoundRule { + Ok(CompoundRule { compound_type: CompoundType::OR, raw_rule: line.to_string(), rule_list: rules, - } + }) } -fn process_and_rule(line: &str, cfn_resources: &HashMap) -> CompoundRule { - CompoundRule { - compound_type: CompoundType::AND, - raw_rule: line.to_string(), - rule_list: destructure_rule(line, cfn_resources), +fn process_and_rule( + line: &str, + cfn_resources: &HashMap, +) -> Result { + match destructure_rule(line, cfn_resources) { + Ok(r) => Ok(CompoundRule { + compound_type: CompoundType::AND, + raw_rule: line.to_string(), + rule_list: r, + }), + Err(e) => Err(e), } } -fn destructure_rule(rule_text: &str, cfn_resources: &HashMap) -> Vec { +fn destructure_rule( + rule_text: &str, + cfn_resources: &HashMap, +) -> Result, String> { trace!("Entered destructure_rule"); let mut rules_hash: HashSet = HashSet::new(); let caps = match RULE_WITH_OPTIONAL_MESSAGE_REG.captures(rule_text) { @@ -194,8 +249,7 @@ fn destructure_rule(rule_text: &str, cfn_resources: &HashMap) -> None => match RULE_REG.captures(rule_text) { Some(c) => c, None => { - trace!("No captures from rule regex"); - return vec![]; + return Err(format!("Invalid rule: {}", rule_text)); } }, }; @@ -279,7 +333,7 @@ fn destructure_rule(rule_text: &str, cfn_resources: &HashMap) -> let rules = rules_hash.into_iter().collect::>(); trace!("Destructured rules are: {:#?}", &rules); - rules + Ok(rules) } mod tests { @@ -305,7 +359,7 @@ mod tests { let mut var_map: HashMap = HashMap::new(); var_map.insert("var".to_string(), "[128]".to_string()); - let parsed_rules = parse_rules(assignment, &cfn_resources); + let parsed_rules = parse_rules(assignment, &cfn_resources).unwrap(); assert!(parsed_rules.variables["var"] == "[128]"); } } diff --git a/cfn-guard/tests/functional.rs b/cfn-guard/tests/functional.rs index 41a56c86e..32f0b9444 100644 --- a/cfn-guard/tests/functional.rs +++ b/cfn-guard/tests/functional.rs @@ -55,12 +55,12 @@ mod tests { ); let mut rules_file_contents = String::from("AWS::EC2::Volume Encrypted == true"); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), (vec![], 0) ); rules_file_contents = String::from("AWS::EC2::Volume Encrypted == True"); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), (vec![], 0) ); @@ -89,13 +89,13 @@ mod tests { ); rules_file_contents = String::from("AWS::EC2::Volume Encrypted == false"); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), (vec![], 0) ); rules_file_contents = String::from("AWS::EC2::Volume Encrypted == False"); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), (vec![], 0) ); @@ -124,13 +124,13 @@ mod tests { ); rules_file_contents = String::from("AWS::EC2::Volume Encrypted == false"); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), (vec![], 0) ); rules_file_contents = String::from("AWS::EC2::Volume Encrypted == False"); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), (vec![], 0) ); @@ -157,13 +157,13 @@ mod tests { ); rules_file_contents = String::from("AWS::EC2::Volume Encrypted == false"); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, false), + cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), (vec![], 0) ); rules_file_contents = String::from("AWS::EC2::Volume Encrypted == False"); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, false), + cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), (vec![], 0) ); } @@ -174,7 +174,7 @@ mod tests { .unwrap_or_else(|err| format!("{}", err)); let rules_file_content = String::from(r#"AWS::EC2::Volume Encrypted != /true/"#); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_content, true), + cfn_guard::run_check(&template_contents, &rules_file_content, true).unwrap(), ( vec![ String::from( @@ -196,7 +196,7 @@ mod tests { let rules_file_content = String::from(r#"AWS::EC2::Volume Encrypted != /true/ << lorem ipsum"#); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_content, true), + cfn_guard::run_check(&template_contents, &rules_file_content, true).unwrap(), ( vec![ String::from( @@ -218,7 +218,7 @@ mod tests { let rules_file_content = String::from(r#"AWS::EC2::Volume Encrypted != true << lorem ipsum"#); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_content, true), + cfn_guard::run_check(&template_contents, &rules_file_content, true).unwrap(), ( vec![ String::from( @@ -240,9 +240,13 @@ mod tests { let rules_file_contents = fs::read_to_string("tests/ebs_volume_rule_set_custom_msg.passing") .unwrap_or_else(|err| format!("{}", err)); + let error = match cfn_guard::run_check(&template_contents, &rules_file_contents, true) { + Ok(_) => "".to_string(), + Err(e) => e, + }; assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), - (vec![String::from("ERROR: Template file format was unreadable as json or yaml: invalid type: string \"THIS IS MEANT TO BE INVALID\", expected a map at line 1 column 1")], 1) + error, + String::from("Template file format was unreadable as json or yaml: invalid type: string \"THIS IS MEANT TO BE INVALID\", expected a map at line 1 column 1") ); } @@ -254,7 +258,7 @@ mod tests { fs::read_to_string("tests/ebs_volume_rule_set_custom_msg.passing") .unwrap_or_else(|err| format!("{}", err)); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), (vec![], 0) ); } @@ -287,7 +291,7 @@ mod tests { ]; outcome.sort(); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), (outcome, 2) ); } @@ -321,7 +325,7 @@ mod tests { let rules_file_contents = fs::read_to_string("tests/test_not_in_list_fail.ruleset") .unwrap_or_else(|err| format!("{}", err)); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), (vec![ String::from("[NewVolume2] failed because [us-west-2c] is not in [us-east-1a,us-east-1b,us-east-1c] for [AvailabilityZone]"), String::from("[NewVolume] failed because [us-west-2b] is not in [us-east-1a,us-east-1b,us-east-1c] for [AvailabilityZone]"), @@ -359,7 +363,7 @@ mod tests { fs::read_to_string("tests/test_in_list_fail_custom_message.ruleset") .unwrap_or_else(|err| format!("{}", err)); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), (vec![ String::from("[NewVolume2] failed because [AvailabilityZone] is [us-west-2c] and lorem ipsum"), String::from("[NewVolume] failed because [AvailabilityZone] is [us-west-2b] and lorem ipsum"), @@ -421,7 +425,7 @@ mod tests { env::set_var("MOTP", "ec2.amazonaws.com"); // Env vars need to be unique to each test because they're global when `cargo test` runs let empty_vec: Vec = Vec::new(); assert_eq!( - cfn_guard::run_check(&template_file_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_file_contents, &rules_file_contents, true).unwrap(), (empty_vec, 0) ); } @@ -463,7 +467,7 @@ mod tests { ); env::set_var("MOTF", "motf"); // Env vars need to be unique to each test because they're global when `cargo test` runs assert_eq!( - cfn_guard::run_check(&template_file_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_file_contents, &rules_file_contents, true).unwrap(), (vec![String::from("[LambdaRoleHelper] failed because [AssumeRolePolicyDocument.Statement.0.Principal.Service.0] is [ec2.amazonaws.com] and the permitted value is [motf]"), String::from("[LambdaRoleHelper] failed because [AssumeRolePolicyDocument.Version] is [2012-10-17] and the permitted pattern is [(\\d{5})-(\\d{2})-(\\d{2})]"), ], 2) @@ -507,7 +511,7 @@ mod tests { AWS::IAM::Role AssumeRolePolicyDocument.Statement.*.Principal.Service.* != ec2.amazonaws.com"#, ); assert_eq!( - cfn_guard::run_check(&template_file_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_file_contents, &rules_file_contents, true).unwrap(), (vec![ String::from("[LambdaRoleHelper] failed because [AssumeRolePolicyDocument.Statement.0.Principal.Service.0] is [ec2.amazonaws.com] and that value is not permitted"), String::from("[LambdaRoleHelper] failed because [AssumeRolePolicyDocument.Statement.0.Principal.Service.1] is [lambda.amazonaws.com] and that value is not permitted"), @@ -556,7 +560,7 @@ mod tests { ); let empty_vec: Vec = Vec::new(); assert_eq!( - cfn_guard::run_check(&template_file_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_file_contents, &rules_file_contents, true).unwrap(), (empty_vec, 0) ); } @@ -598,7 +602,7 @@ mod tests { ); let empty_vec: Vec = Vec::new(); assert_eq!( - cfn_guard::run_check(&template_file_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_file_contents, &rules_file_contents, true).unwrap(), (empty_vec, 0) ); } @@ -639,7 +643,7 @@ mod tests { r#"AWS::IAM::Role AssumeRolePolicyDocument.Statement.*.Principal.Service.* == wcf"#, ); assert_eq!( - cfn_guard::run_check(&template_file_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_file_contents, &rules_file_contents, true).unwrap(), (vec![String::from("[LambdaRoleHelper] failed because [AssumeRolePolicyDocument.Statement.0.Principal.Service.0] is [ec2.amazonaws.com] and the permitted value is [wcf]"), String::from("[LambdaRoleHelper] failed because [AssumeRolePolicyDocument.Statement.0.Principal.Service.1] is [lambda.amazonaws.com] and the permitted value is [wcf]"), String::from("[LambdaRoleHelper] failed because [AssumeRolePolicyDocument.Statement.1.Principal.Service.0] is [lambda.amazonaws.com] and the permitted value is [wcf]"), @@ -674,7 +678,7 @@ mod tests { env::set_var("SERV_PRIN_EVP", "lambda.amazonaws.com"); // Env vars need to be unique to each test because they're global when `cargo test` runs let empty_vec: Vec = Vec::new(); assert_eq!( - cfn_guard::run_check(&template_file_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_file_contents, &rules_file_contents, true).unwrap(), (empty_vec, 0) ); } @@ -701,7 +705,7 @@ mod tests { ); env::set_var("SERV_PRIN_EVF", "evf.amazonaws.com"); // Env vars need to be unique to each test because they're global when `cargo test` runs assert_eq!( - cfn_guard::run_check(&template_file_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_file_contents, &rules_file_contents, true).unwrap(), (vec![String::from("[LambdaRoleHelper] failed because [AssumeRolePolicyDocument.Statement.0.Principal.Service.0] is [lambda.amazonaws.com] and the permitted value is [evf.amazonaws.com]")], 2) ); } @@ -728,7 +732,7 @@ mod tests { ); let empty_vec: Vec = Vec::new(); assert_eq!( - cfn_guard::run_check(&template_file_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_file_contents, &rules_file_contents, true).unwrap(), (empty_vec, 0) ); } @@ -754,7 +758,7 @@ mod tests { r#"AWS::IAM::Role AssumeRolePolicyDocument.Version == /(\d{5})-(\d{2})-(\d{2})/"#, ); assert_eq!( - cfn_guard::run_check(&template_file_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_file_contents, &rules_file_contents, true).unwrap(), (vec![String::from("[LambdaRoleHelper] failed because [AssumeRolePolicyDocument.Version] is [2012-10-17] and the permitted pattern is [(\\d{5})-(\\d{2})-(\\d{2})]")], 2) ); } @@ -780,7 +784,7 @@ mod tests { AWS::EC2::Volume Encrypted != %require_encryption"#, ); assert_eq!( - cfn_guard::run_check(&template_file_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_file_contents, &rules_file_contents, true).unwrap(), (vec![String::from("[NewVolume] failed because it does not contain the required property of [Encrypted]")], 2) ); } @@ -807,7 +811,7 @@ AWS::EC2::Volume Encrypted != %require_encryption"#, AWS::EC2::Volume Encrypted != %require_encryption"#, ); assert_eq!( - cfn_guard::run_check(&template_file_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_file_contents, &rules_file_contents, true).unwrap(), (vec![String::from("[NewVolume] failed because there is no value defined for [%require_encryption] to check [Encrypted] against")], 2) ); } @@ -843,7 +847,7 @@ AWS::EC2::Volume Encrypted != %require_encryption"#, AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, ); assert_eq!( - cfn_guard::run_check(&template_file_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_file_contents, &rules_file_contents, true).unwrap(), (vec![String::from("[NewVolume] failed because [Size] is [100] and the permitted value is [101]"), String::from("[NewVolume] failed because [Size] is [100] and the permitted value is [99]"),], 2) @@ -881,7 +885,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, AWS::EC2::Volume Size < 101"#, ); assert_eq!( - cfn_guard::run_check(&template_file_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_file_contents, &rules_file_contents, true).unwrap(), (vec![String::from("[NewVolume2] failed because [Size] is [101] and the permitted value is [< 101]"), ], 2) ); @@ -918,7 +922,7 @@ AWS::EC2::Volume Size < 101"#, AWS::EC2::Volume Size > 100"#, ); assert_eq!( - cfn_guard::run_check(&template_file_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_file_contents, &rules_file_contents, true).unwrap(), ( vec![String::from( "[NewVolume] failed because [Size] is [100] and the permitted value is [> 100]" @@ -959,7 +963,7 @@ AWS::EC2::Volume Size > 100"#, AWS::EC2::Volume Size <= 100"#, ); assert_eq!( - cfn_guard::run_check(&template_file_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_file_contents, &rules_file_contents, true).unwrap(), (vec![String::from("[NewVolume2] failed because [Size] is [101] and the permitted value is [<= 100]"), ], 2) ); @@ -996,7 +1000,7 @@ AWS::EC2::Volume Size <= 100"#, AWS::EC2::Volume Size >= 101"#, ); assert_eq!( - cfn_guard::run_check(&template_file_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_file_contents, &rules_file_contents, true).unwrap(), (vec![String::from("[NewVolume] failed because [Size] is [100] and the permitted value is [>= 101]"), ], 2) ); @@ -1058,7 +1062,7 @@ AWS::EC2::Volume Encrypted != %require_encryption AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, ); assert_eq!( - cfn_guard::run_check(&template_file_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_file_contents, &rules_file_contents, true).unwrap(), (vec![String::from("[NewVolume] failed because [Encrypted] is [true] and that value is not permitted"), String::from("[NewVolume] failed because [Size] is [100] and the permitted value is [101]"), String::from("[NewVolume] failed because [Size] is [100] and the permitted value is [99]"), @@ -1092,7 +1096,7 @@ AWS::EC2::Volume Encrypted != %require_encryption AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, ); assert_eq!( - cfn_guard::run_check(&template_file_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_file_contents, &rules_file_contents, true).unwrap(), (vec![ String::from("[NewVolume] failed because [Encrypted] is [true] and that value is not permitted"), String::from("[NewVolume] failed because [Size] is [100] and the permitted value is [101]"), @@ -1111,7 +1115,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, AWS::EC2::SecurityGroup Tags.* == {"Key":"OwnerContact","Value":"OwnerContact"}"#, ); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), (vec![], 0) ); } @@ -1125,7 +1129,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, AWS::EC2::SecurityGroup Tags.* == {"Key":"OwnerContact","Value":"OwnerContact"}"#, ); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), ( vec![ String::from( @@ -1159,7 +1163,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, let rules_file_contents = fs::read_to_string("tests/wildcard_iam_rule_set.passing") .unwrap_or_else(|err| format!("{}", err)); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), (vec![], 0) ); } @@ -1171,7 +1175,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, let rules_file_contents = fs::read_to_string("tests/wildcard_not_in_iam_rule_set.failing") .unwrap_or_else(|err| format!("{}", err)); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), (vec![String::from("[LambdaRoleHelper] failed because [lambda.amazonaws.com] is in [lambda.amazonaws.com, ec2.amazonaws.com] which is not permitted for [AssumeRolePolicyDocument.Statement.0.Principal.Service.0]"), ], 2) ); @@ -1184,7 +1188,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, let rules_file_contents = fs::read_to_string("tests/wildcard_action.pass") .unwrap_or_else(|err| format!("{}", err)); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), (vec![], 0) ); } @@ -1196,7 +1200,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, let rules_file_contents = fs::read_to_string("tests/wildcard_action.fail") .unwrap_or_else(|err| format!("{}", err)); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), (vec![String::from("[EndpointCloudWatchRoleC3C64E0F] failed because [AssumeRolePolicyDocument.Statement.0.Action] is [sts:AssumeRole] and that value is not permitted"), String::from("[HelloHandlerServiceRole11EF7C63] failed because [AssumeRolePolicyDocument.Statement.0.Action] is [sts:AssumeRole] and that value is not permitted")], 2) @@ -1218,7 +1222,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, ) .unwrap_or_else(|err| format!("{}", err)); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), (vec![String::from("[NewVolume2] failed because it does not contain the required property of [Tags]"), String::from("[NewVolume2] failed because it does not contain the required property of [Tags]"), String::from("[NewVolume] failed because [Tags.0.Key] is [uaid] and the permitted value is [uai]"), @@ -1242,7 +1246,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, ) .unwrap_or_else(|err| format!("{}", err)); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, false), + cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), (vec![String::from("[NewVolume] failed because [Tags.0.Key] is [uaid] and the permitted value is [uai]"), String::from("[NewVolume] failed because [Tags.1.Key] is [tag2] and the permitted value is [uai]")], 2) @@ -1256,7 +1260,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, let rules_file_contents = fs::read_to_string("tests/getatt_template.ruleset") .unwrap_or_else(|err| format!("{}", err)); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, false), + cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), (vec![String::from("[EC2Instance] failed because [t3.medium] is not in [t2.nano,t2.micro,t2.small,t3.nano,t3.micro,t3.small] for [InstanceType]"), String::from("[InstanceSecurityGroup] failed because [SecurityGroupIngress] is [[{\"CidrIp\":\"SSHLocation\",\"FromPort\":22,\"IpProtocol\":\"tcp\",\"ToPort\":22}]] and the permitted value is [[{\"CidrIp\":\"SSHLocation\",\"FromPort\":33322,\"IpProtocol\":\"tcp\",\"ToPort\":33322}]]"), String::from("[NewVolume] failed because [Size] is [512] and the permitted value is [128]"), @@ -1273,7 +1277,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, let rules_file_contents = fs::read_to_string("tests/getatt_template.ruleset") .unwrap_or_else(|err| format!("{}", err)); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, false), + cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), (vec![String::from("[NewVolume2] failed because [Encrypted] is [true] and that value is not permitted"), String::from("[NewVolume2] failed because [Size] is [99] and the permitted value is [128]"), String::from("[NewVolume2] failed because [Size] is [99] and the permitted value is [256]"), @@ -1291,14 +1295,13 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, .unwrap_or_else(|err| format!("{}", err)); let rules_file_contents = fs::read_to_string("tests/no_resources_template.ruleset") .unwrap_or_else(|err| format!("{}", err)); + let error = match cfn_guard::run_check(&template_contents, &rules_file_contents, true) { + Ok(_) => "".to_string(), + Err(e) => e, + }; assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, false), - ( - vec![String::from( - "ERROR: Template file does not contain a [Resources] section to check" - )], - 1 - ) + error, + String::from("Template file does not contain a [Resources] section to check") ) } @@ -1314,7 +1317,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, "#, ); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, false), + cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), ( vec![String::from( r#"[ClusterSg] failed because [{"Key":"OwnerContact","Value":"OwnerContact"}] is in ["test", 1, ["a", "b"], {"Key":"OwnerContact","Value":"OwnerContact"},{"Key":"OwnerContact","Value":{"Ref":"OwnerContact"}}] which is not permitted for [Tags.1]"# @@ -1329,7 +1332,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, "#, ); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, false), + cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), (vec![], 0) ); template_contents = fs::read_to_string("tests/parse_lists_with_json_test-template.json") @@ -1341,7 +1344,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, "#, ); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, false), + cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), ( vec![String::from( r#"[ClusterSg] failed because [{"Key":"OwnerContact","Value":{"Ref":"OwnerContact"}}] is in ["test", 1, ["a", "b"], {"Key":"OwnerContact","Value":"OwnerContact"},{"Key":"OwnerContact","Value":{"Ref":"OwnerContact"}}] which is not permitted for [Tags.1]"# @@ -1356,7 +1359,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, "#, ); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, false), + cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), (vec![], 0) ); } @@ -1372,7 +1375,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, AWS::Lambda::Function Environment.*.*.* IN %log_stuff"#, ); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), (vec![], 0) ); @@ -1383,7 +1386,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, AWS::Lambda::Function Environment.*.*.* NOT_IN %log_stuff"#, ); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), ( vec![ String::from( @@ -1403,7 +1406,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, // Pass over-wildcarding (ie, wildcards in structures that don't extend that deeply rules_file_contents = String::from(r#"AWS::Lambda::Function MemorySize.*.* IN [128]"#); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), (vec![], 0) ); @@ -1412,7 +1415,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, r#"AWS::Lambda::Function Environment.*.* IN [["Solution", "Data", "LogLevel"]]"#, ); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), (vec![], 0) ); @@ -1421,7 +1424,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, r#"AWS::Lambda::Function Environment.*.* NOT_IN [["Solution", "Data", "LogLevel"]]"#, ); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), ( vec![String::from( r#"[LambdaWAFHelperFunction] failed because [["Solution","Data","LogLevel"]] is in [["Solution", "Data", "LogLevel"]] which is not permitted for [Environment.Variables.LOG_LEVEL]"# @@ -1435,7 +1438,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, r#"AWS::Lambda::Function Environment.*.LOG_LEVEL IN [["Solution", "Data", "LogLevel"]]"#, ); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), (vec![], 0) ); @@ -1444,7 +1447,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, r#"AWS::Lambda::Function Environment.*.LOG_LEVEL NOT_IN [["Solution", "Data", "LogLevel"]]"#, ); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, true), + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), ( vec![String::from( r#"[LambdaWAFHelperFunction] failed because [["Solution","Data","LogLevel"]] is in [["Solution", "Data", "LogLevel"]] which is not permitted for [Environment.Variables.LOG_LEVEL]"# @@ -1458,7 +1461,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, r#"AWS::Lambda::Function *.*.LOG_LEVEL IN [["Solution", "Data", "LogLevel"]]"#, ); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, false), + cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), (vec![], 0) ); @@ -1467,7 +1470,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, r#"AWS::Lambda::Function *.*.LOG_LEVEL NOT_IN [["Solution", "Data", "LogLevel"]]"#, ); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, false), + cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), ( vec![String::from( r#"[LambdaWAFHelperFunction] failed because [["Solution","Data","LogLevel"]] is in [["Solution", "Data", "LogLevel"]] which is not permitted for [Environment.Variables.LOG_LEVEL]"# @@ -1482,15 +1485,19 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, let template_contents = fs::read_to_string("tests/conditional-ddb-template.yaml") .unwrap_or_else(|err| format!("{}", err)); - let mut rules_file_contents = String::from("AWS::DynamoDB::Table if Tags == /.*PROD.*/ then .DeletionPolicy == Retain"); + let mut rules_file_contents = String::from( + "AWS::DynamoDB::Table if Tags == /.*PROD.*/ then .DeletionPolicy == Retain", + ); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, false), + cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), (vec![], 0) ); - rules_file_contents = String::from("AWS::DynamoDB::Table if Tags == /.*PROD.*/ then .DeletionPolicy != Retain"); + rules_file_contents = String::from( + "AWS::DynamoDB::Table if Tags == /.*PROD.*/ then .DeletionPolicy != Retain", + ); assert_eq!( - cfn_guard::run_check(&template_contents, &rules_file_contents, false), + cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), (vec![String::from("[DDBTable] failed because [.DeletionPolicy] is [Retain] and that value is not permitted when AWS::DynamoDB::Table Tags == /.*PROD.*/")], 2) ) From 821295599fc5bafef9a0097ced528e5a8d0d1267 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Thu, 16 Jul 2020 16:01:37 -0400 Subject: [PATCH 15/39] Removed commented-out test --- cfn-guard/tests/functional.rs | 26 -------------------------- 1 file changed, 26 deletions(-) diff --git a/cfn-guard/tests/functional.rs b/cfn-guard/tests/functional.rs index 32f0b9444..0118bec75 100644 --- a/cfn-guard/tests/functional.rs +++ b/cfn-guard/tests/functional.rs @@ -1006,32 +1006,6 @@ AWS::EC2::Volume Size >= 101"#, ); } - // TODO: Create test for clean_exit() scenarios - // #[test] - // #[should_panic] - // fn test_non_numeric_value_comparison_fail() { - // let template_file_contents = String::from( - // r#"{ - // "Resources": { - // "NewVolume" : { - // "Type" : "AWS::EC2::Volume", - // "Properties" : { - // "Size" : 100, - // "Encrypted" : true, - // "AvailabilityZone" : "us-east-1b", - // "DeletionPolicy" : "Snapshot" - // } - // } - // } - // }"#, - // ); - // let rules_file_contents = String::from( - // r#" - // AWS::EC2::Volume Size < a"#, - // ); - // cfn_guard::run_check(&template_file_contents, &rules_file_contents, true); - // } - #[test] fn test_json_results() { let template_file_contents = String::from( From 44ce2398825cd80d6bb30ab8c55399bb43444f53 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Tue, 21 Jul 2020 17:36:36 -0400 Subject: [PATCH 16/39] Update Lambda to work with new run_check() exit Result type --- cfn-guard-lambda/src/main.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cfn-guard-lambda/src/main.rs b/cfn-guard-lambda/src/main.rs index e59626ef7..442e0ecfa 100644 --- a/cfn-guard-lambda/src/main.rs +++ b/cfn-guard-lambda/src/main.rs @@ -34,7 +34,11 @@ fn my_handler(e: CustomEvent, _c: Context) -> Result //dbg!(&e); info!("Template is [{}]", &e.template); info!("Rule Set is [{}]", &e.rule_set); - let (result, exit_code) = cfn_guard::run_check(&e.template, &e.rule_set, e.strict_checks); + let (result, exit_code) = match cfn_guard::run_check(&e.template, &e.rule_set, e.strict_checks) + { + Ok(t) => t, + Err(e) => (vec![e], 1), + }; let exit_status = match exit_code { 0 => "PASS", From 734e14ccf84fb1854389c4e6a0f61171e1bc1488 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Thu, 23 Jul 2020 13:22:31 -0400 Subject: [PATCH 17/39] Force cfn-guard-rulegen output order to be deterministic (Issue #44) --- cfn-guard-rulegen/src/lib.rs | 38 ++++++++++++++++++----------------- cfn-guard-rulegen/src/main.rs | 3 ++- 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/cfn-guard-rulegen/src/lib.rs b/cfn-guard-rulegen/src/lib.rs index 44e0c82ac..4670649f9 100644 --- a/cfn-guard-rulegen/src/lib.rs +++ b/cfn-guard-rulegen/src/lib.rs @@ -29,10 +29,11 @@ pub fn run_gen(template_file_contents: &str) -> Vec { Err(_) => match serde_yaml::from_str(template_file_contents) { Ok(y) => y, Err(e) => { - let msg_string = format!("Template file format was unreadable as json or yaml: {}", e); + let msg_string = + format!("Template file format was unreadable as json or yaml: {}", e); error!("{}", &msg_string); - return vec![msg_string] - }, + return vec![msg_string]; + } }, }; trace!("CFN Template is {:#?}", &cfn_template); @@ -41,18 +42,17 @@ pub fn run_gen(template_file_contents: &str) -> Vec { None => { let msg_string = format!("Template lacks a Resources section"); error!("{}", &msg_string); - return vec![msg_string] - }, + return vec![msg_string]; + } + }; + let cfn_resources: HashMap = match serde_json::from_value(cfn_resources_clone) { + Ok(y) => y, + Err(e) => { + let msg_string = format!("Template Resources section has an invalid structure: {}", e); + error!("{}", &msg_string); + return vec![msg_string]; + } }; - let cfn_resources: HashMap = - match serde_json::from_value(cfn_resources_clone) { - Ok(y) => y, - Err(e) => { - let msg_string = format!("Template Resources section has an invalid structure: {}", e); - error!("{}", &msg_string); - return vec![msg_string] - }, - }; trace!("CFN resources are: {:?}", cfn_resources); gen_rules(cfn_resources) } @@ -65,7 +65,7 @@ fn gen_rules(cfn_resources: HashMap) -> Vec { let props: HashMap = match serde_json::from_value(cfn_resource["Properties"].clone()) { Ok(s) => s, - Err(_) => continue + Err(_) => continue, }; for (prop_name, prop_val) in props { let stripped_val = match prop_val.as_str() { @@ -76,7 +76,8 @@ fn gen_rules(cfn_resources: HashMap) -> Vec { let key_name = format!("{} {}", &cfn_resource["Type"].as_str().unwrap(), prop_name); // If the key doesn't exist, create it and set its value to a new HashSet with the rule value in it if !rule_map.contains_key(&key_name) { - let value_set: HashSet = vec![no_newline_stripped_val].into_iter().collect(); + let value_set: HashSet = + vec![no_newline_stripped_val].into_iter().collect(); rule_map.insert(key_name, value_set); } else { // If the key does exist, add the item to the HashSet @@ -88,7 +89,9 @@ fn gen_rules(cfn_resources: HashMap) -> Vec { for (key, val_set) in rule_map { let mut rule_string: String = String::from(""); let mut count = 0; - for r in val_set { + let mut sorted_val_set: Vec = val_set.into_iter().collect::>(); + sorted_val_set.sort(); + for r in sorted_val_set { let temp_rule_string = format!("{} == {}", key, r); if count > 0 { rule_string = format!("{} |OR| {}", rule_string, temp_rule_string); @@ -101,4 +104,3 @@ fn gen_rules(cfn_resources: HashMap) -> Vec { } rule_set.into_iter().collect() } - diff --git a/cfn-guard-rulegen/src/main.rs b/cfn-guard-rulegen/src/main.rs index 41a97ea99..b13499621 100644 --- a/cfn-guard-rulegen/src/main.rs +++ b/cfn-guard-rulegen/src/main.rs @@ -35,12 +35,13 @@ fn main() { info!("Generating rules from {}", &template_file); - let result = cfn_guard_rulegen::run(template_file).unwrap_or_else(|err| { + let mut result = cfn_guard_rulegen::run(template_file).unwrap_or_else(|err| { println!("Problem generating rules: {}", err); process::exit(1); }); if !result.is_empty() { + result.sort(); for res in result.iter() { println!("{}", res); } From 6d6f3e36c14bbbdb6708fd8191f2624e4ab39661 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Wed, 22 Jul 2020 18:02:00 -0400 Subject: [PATCH 18/39] Add new fundamental RuleType "SimpleRule" to provide base case for rule processing recursion --- cfn-guard/src/guard_types.rs | 11 +- cfn-guard/src/lib.rs | 153 ++++++++++++++++++--- cfn-guard/src/parser.rs | 246 ++++++++++++++++++---------------- cfn-guard/tests/functional.rs | 50 +++++++ 4 files changed, 319 insertions(+), 141 deletions(-) diff --git a/cfn-guard/src/guard_types.rs b/cfn-guard/src/guard_types.rs index 03e2219fb..541f6fb64 100644 --- a/cfn-guard/src/guard_types.rs +++ b/cfn-guard/src/guard_types.rs @@ -30,16 +30,17 @@ pub mod enums { Regex, Variable, } - #[derive(Debug, Clone, Eq, PartialEq)] + #[derive(Debug, Clone, Eq, PartialEq, Hash)] pub enum CompoundType { OR, AND, } - #[derive(Debug, Clone)] + #[derive(Debug, Clone, Eq, PartialEq, Hash)] pub enum RuleType { CompoundRule(super::structs::CompoundRule), ConditionalRule(super::structs::ConditionalRule), + SimpleRule(super::structs::Rule), } } @@ -56,14 +57,14 @@ pub mod structs { pub(crate) custom_msg: Option, } - #[derive(Debug, Clone, Eq, PartialEq)] + #[derive(Debug, Clone, Eq, PartialEq, Hash)] pub struct CompoundRule { pub(crate) compound_type: super::enums::CompoundType, pub(crate) raw_rule: String, - pub(crate) rule_list: Vec, + pub(crate) rule_list: Vec, } - #[derive(Debug, Clone, Eq, PartialEq)] + #[derive(Debug, Clone, Eq, PartialEq, Hash)] pub struct ConditionalRule { pub(crate) condition: CompoundRule, pub(crate) consequent: CompoundRule, diff --git a/cfn-guard/src/lib.rs b/cfn-guard/src/lib.rs index 1287a79dc..5f5e3541e 100644 --- a/cfn-guard/src/lib.rs +++ b/cfn-guard/src/lib.rs @@ -125,6 +125,14 @@ fn check_resources( for c_rule in parsed_rule_set.rule_set.iter() { info!("Applying rule '{:#?}'", &c_rule); match c_rule { + enums::RuleType::SimpleRule(r) => { + trace!("Simple rule is {:#?}", r); + if let Some(rule_result) = + apply_rule(&cfn_resources, r, &parsed_rule_set.variables, strict_checks) + { + result.extend(rule_result); + } + } enums::RuleType::ConditionalRule(r) => { trace!("Conditional rule is {:#?}", r); for (name, cfn_resource) in cfn_resources { @@ -160,8 +168,13 @@ fn check_resources( strict_checks, ) { Some(x) => { - let temp_result = - x.into_iter().map(|x| format!("{} {}", x, postscript)); + let temp_result = x.into_iter().map(|x| { + if !x.contains("when") { + format!("{} {}", x, postscript) + } else { + x + } + }); result.extend(temp_result); } None => (), @@ -178,19 +191,71 @@ fn check_resources( let mut temp_results: Vec = vec![]; let mut cfn_resource_map: HashMap = HashMap::new(); cfn_resource_map.insert(name.clone(), cfn_resource.clone()); - for rule in &r.rule_list { - match apply_rule( - &cfn_resource_map, - &rule, - &parsed_rule_set.variables, - strict_checks, - ) { - Some(rule_result) => { - pass_fail.insert("fail"); - temp_results.extend(rule_result); + for typed_rule in &r.rule_list { + match typed_rule { + enums::RuleType::SimpleRule(r) => { + match apply_rule( + &cfn_resource_map, + r, + &parsed_rule_set.variables, + strict_checks, + ) { + Some(rule_result) => { + pass_fail.insert("fail"); + temp_results.extend(rule_result); + } + None => { + pass_fail.insert("pass"); + } + } } - None => { - pass_fail.insert("pass"); + enums::RuleType::CompoundRule(r) => { + let rule_set = structs::ParsedRuleSet { + variables: parsed_rule_set.variables.clone(), + rule_set: vec![enums::RuleType::CompoundRule(r.clone())], + }; + let postscript = format!("when {}", r.raw_rule); + match check_resources( + &cfn_resource_map, + &rule_set, + strict_checks, + ) { + Some(x) => { + let temp_result = x.into_iter().map(|x| { + if !x.contains("when") { + format!("{} {}", x, postscript) + } else { + x + } + }); + result.extend(temp_result); + } + None => (), + }; + } + enums::RuleType::ConditionalRule(r) => { + let rule_set = structs::ParsedRuleSet { + variables: parsed_rule_set.variables.clone(), + rule_set: vec![enums::RuleType::ConditionalRule(r.clone())], + }; + let postscript = format!("when {}", r.condition.raw_rule); + match check_resources( + &cfn_resource_map, + &rule_set, + strict_checks, + ) { + Some(x) => { + let temp_result = x.into_iter().map(|x| { + if !x.contains("when") { + format!("{} {}", x, postscript) + } else { + x + } + }); + result.extend(temp_result); + } + None => (), + }; } } } @@ -202,14 +267,58 @@ fn check_resources( } } enums::CompoundType::AND => { - for rule in &r.rule_list { - if let Some(rule_result) = apply_rule( - &cfn_resources, - &rule, - &parsed_rule_set.variables, - strict_checks, - ) { - result.extend(rule_result); + for typed_rule in &r.rule_list { + match typed_rule { + enums::RuleType::SimpleRule(r) => { + if let Some(rule_result) = apply_rule( + &cfn_resources, + r, + &parsed_rule_set.variables, + strict_checks, + ) { + result.extend(rule_result); + } + } + enums::RuleType::CompoundRule(r) => { + let rule_set = structs::ParsedRuleSet { + variables: parsed_rule_set.variables.clone(), + rule_set: vec![enums::RuleType::CompoundRule(r.clone())], + }; + let postscript = format!("when {}", r.raw_rule); + match check_resources(cfn_resources, &rule_set, strict_checks) { + Some(x) => { + let temp_result = x.into_iter().map(|x| { + if !x.contains("when") { + format!("{} {}", x, postscript) + } else { + x + } + }); + result.extend(temp_result); + } + None => (), + }; + } + enums::RuleType::ConditionalRule(r) => { + let rule_set = structs::ParsedRuleSet { + variables: parsed_rule_set.variables.clone(), + rule_set: vec![enums::RuleType::ConditionalRule(r.clone())], + }; + let postscript = format!("when {}", r.condition.raw_rule); + match check_resources(cfn_resources, &rule_set, strict_checks) { + Some(x) => { + let temp_result = x.into_iter().map(|x| { + if !x.contains("when") { + format!("{} {}", x, postscript) + } else { + x + } + }); + result.extend(temp_result); + } + None => (), + }; + } } } } diff --git a/cfn-guard/src/parser.rs b/cfn-guard/src/parser.rs index 544821202..8fb5914f8 100644 --- a/cfn-guard/src/parser.rs +++ b/cfn-guard/src/parser.rs @@ -81,14 +81,14 @@ pub(crate) fn parse_rules( LineType::Rule => match parse_rule_line(l, &cfn_resources) { Ok(prl) => { debug!("Parsed rule is: {:#?}", &prl); - rule_set.push(RuleType::CompoundRule(prl)) + rule_set.push(prl) } Err(e) => return Err(e), }, - LineType::Conditional => match process_conditional(l, &cfn_resources) { + LineType::Conditional => match parse_rule_line(l, &cfn_resources) { Ok(c) => { debug!("Parsed conditional is {:#?}", &c); - rule_set.push(RuleType::ConditionalRule(c)); + rule_set.push(c); } Err(e) => return Err(e), }, @@ -111,10 +111,7 @@ pub(crate) fn parse_rules( }) } -fn parse_rule_line( - l: &str, - cfn_resources: &HashMap, -) -> Result { +fn parse_rule_line(l: &str, cfn_resources: &HashMap) -> Result { match is_or_rule(l) { true => { debug!("Line is an |OR| rule"); @@ -143,32 +140,30 @@ fn process_conditional( let conjd_caps_conditional = format!("{} {}", &caps["resource_type"], &caps["condition"]); trace!("conjd_caps_conditional is {:#?}", conjd_caps_conditional); match parse_rule_line(&conjd_caps_conditional, cfn_resources) { - Ok(condition) => { + Ok(cond) => { + let condition = match cond { + RuleType::CompoundRule(s) => s, + _ => return Err(format!("Bad destructure of conditional rule: {}", line)), + }; let conjd_caps_consequent = format!("{} {}", &caps["resource_type"], &caps["consequent"]); trace!("conjd_caps_consequent is {:#?}", conjd_caps_consequent); match parse_rule_line(&conjd_caps_consequent, cfn_resources) { - Ok(consequent) => Ok(ConditionalRule { - condition, - consequent, - }), + Ok(cons) => { + let consequent = match cons { + RuleType::CompoundRule(s) => s, + _ => return Err(format!("Bad destructure of conditional rule: {}", line)), + }; + Ok(ConditionalRule { + condition, + consequent, + }) + } Err(e) => Err(e), } } Err(e) => Err(e), } - // if let condition = parse_rule_line(&conjd_caps_conditional, cfn_resources) - // .unwrap_or_else(|err| return Err(err)); - // - // let conjd_caps_consequent = format!("{} {}", &caps["resource_type"], &caps["consequent"]); - // trace!("conjd_caps_consequent is {:#?}", conjd_caps_consequent); - // if let consequent = parse_rule_line(&conjd_caps_consequent, cfn_resources) - // .unwrap_or_else(|err| return Err(err)); - // - // ok(conditionalrule { - // condition, - // consequent, - // }) } fn find_line_type(line: &str) -> LineType { @@ -203,106 +198,111 @@ fn is_or_rule(line: &str) -> bool { line.contains("|OR|") || WILDCARD_OR_RULE_REG.is_match(line) } -fn process_or_rule( - line: &str, - cfn_resources: &HashMap, -) -> Result { +fn process_or_rule(line: &str, cfn_resources: &HashMap) -> Result { trace!("Entered process_or_rule"); let branches = line.split("|OR|"); - debug!("Rule branches are: {:#?}", &branches); - let mut rules: Vec = vec![]; + // debug!("Rule branches are: {:#?}", &branches); + let mut rules: Vec = vec![]; for b in branches { + debug!("Rule |OR| branch is '{}'", b); match destructure_rule(b.trim(), cfn_resources) { Ok(r) => rules.append(&mut r.clone()), Err(e) => return Err(e), } } - Ok(CompoundRule { + Ok(RuleType::CompoundRule(CompoundRule { compound_type: CompoundType::OR, raw_rule: line.to_string(), rule_list: rules, - }) + })) } fn process_and_rule( line: &str, cfn_resources: &HashMap, -) -> Result { - match destructure_rule(line, cfn_resources) { - Ok(r) => Ok(CompoundRule { - compound_type: CompoundType::AND, - raw_rule: line.to_string(), - rule_list: r, - }), - Err(e) => Err(e), +) -> Result { + trace!("Entered process_and_rule"); + let branches = line.split("|AND|"); + debug!("Rule branches are: {:#?}", &branches); + let mut rules: Vec = vec![]; + for b in branches { + match destructure_rule(b.trim(), cfn_resources) { + Ok(r) => rules.append(&mut r.clone()), + Err(e) => return Err(e), + } } + Ok(RuleType::CompoundRule(CompoundRule { + compound_type: CompoundType::AND, + raw_rule: line.to_string(), + rule_list: rules, + })) + // match destructure_rule(line, cfn_resources) { + // Ok(r) => Ok(CompoundRule { + // compound_type: CompoundType::AND, + // raw_rule: line.to_string(), + // rule_list: r, + // }), + // Err(e) => Err(e), + // } } fn destructure_rule( rule_text: &str, cfn_resources: &HashMap, -) -> Result, String> { +) -> Result, String> { trace!("Entered destructure_rule"); - let mut rules_hash: HashSet = HashSet::new(); - let caps = match RULE_WITH_OPTIONAL_MESSAGE_REG.captures(rule_text) { - Some(c) => c, - None => match RULE_REG.captures(rule_text) { - Some(c) => c, - None => { - return Err(format!("Invalid rule: {}", rule_text)); - } - }, - }; - - trace!("Parsed rule's captures are: {:#?}", &caps); - let mut props: Vec = vec![]; - if caps["resource_property"].contains('*') { - for (_name, value) in cfn_resources { - if caps["resource_type"] == value["Type"] { - if let Some(p) = util::expand_wildcard_props( - &value["Properties"], - caps["resource_property"].to_string(), - String::from(""), - ) { - props.append(&mut p.clone()); - trace!("Expanded props are {:#?}", &props); - } + let mut rules_hash: HashSet = HashSet::new(); + if CONDITIONAL_RULE_REG.is_match(rule_text) { + match process_conditional(rule_text, cfn_resources) { + Ok(r) => { + rules_hash.insert(RuleType::ConditionalRule(r)); } + Err(e) => return Err(e), } } else { - props.push(caps["resource_property"].to_string()); - }; + let caps = match RULE_WITH_OPTIONAL_MESSAGE_REG.captures(rule_text) { + Some(c) => c, + None => match RULE_REG.captures(rule_text) { + Some(c) => c, + None => { + return Err(format!("Invalid rule: {}", rule_text)); + } + }, + }; - for p in props { - rules_hash.insert(Rule { - resource_type: caps["resource_type"].to_string(), - field: p.to_string(), - operation: { - match &caps["operator"] { - "==" => OpCode::Require, - "!=" => OpCode::RequireNot, - "<" => OpCode::LessThan, - ">" => OpCode::GreaterThan, - "<=" => OpCode::LessThanOrEqualTo, - ">=" => OpCode::GreaterThanOrEqualTo, - "IN" => OpCode::In, - "NOT_IN" => OpCode::NotIn, - _ => { - let msg_string = format!( - "Bad Rule Operator: [{}] in '{}'", - &caps["operator"], rule_text - ); - error!("{}", &msg_string); - process::exit(1) + trace!("Parsed rule's captures are: {:#?}", &caps); + let mut props: Vec = vec![]; + if caps["resource_property"].contains('*') { + for (_name, value) in cfn_resources { + if caps["resource_type"] == value["Type"] { + if let Some(p) = util::expand_wildcard_props( + &value["Properties"], + caps["resource_property"].to_string(), + String::from(""), + ) { + props.append(&mut p.clone()); + trace!("Expanded props are {:#?}", &props); } } - }, - rule_vtype: { - let rv = caps["rule_value"].chars().next().unwrap(); - match rv { - '[' => match &caps["operator"] { - "==" | "!=" | "<=" | ">=" | "<" | ">" => RValueType::Value, - "IN" | "NOT_IN" => RValueType::List, + } + } else { + props.push(caps["resource_property"].to_string()); + }; + + for p in props { + let rule = Rule { + resource_type: caps["resource_type"].to_string(), + field: p.to_string(), + operation: { + match &caps["operator"] { + "==" => OpCode::Require, + "!=" => OpCode::RequireNot, + "<" => OpCode::LessThan, + ">" => OpCode::GreaterThan, + "<=" => OpCode::LessThanOrEqualTo, + ">=" => OpCode::GreaterThanOrEqualTo, + "IN" => OpCode::In, + "NOT_IN" => OpCode::NotIn, _ => { let msg_string = format!( "Bad Rule Operator: [{}] in '{}'", @@ -311,27 +311,45 @@ fn destructure_rule( error!("{}", &msg_string); process::exit(1) } - }, - '/' => RValueType::Regex, - '%' => RValueType::Variable, - _ => RValueType::Value, - } - }, - value: { - let rv = caps["rule_value"].chars().next().unwrap(); - match rv { - '/' => caps["rule_value"].trim_matches('/').to_string(), - _ => caps["rule_value"].to_string().trim().to_string(), - } - }, - custom_msg: match caps.name("custom_msg") { - Some(s) => Some(s.as_str().to_string()), - None => None, - }, - }); + } + }, + rule_vtype: { + let rv = caps["rule_value"].chars().next().unwrap(); + match rv { + '[' => match &caps["operator"] { + "==" | "!=" | "<=" | ">=" | "<" | ">" => RValueType::Value, + "IN" | "NOT_IN" => RValueType::List, + _ => { + let msg_string = format!( + "Bad Rule Operator: [{}] in '{}'", + &caps["operator"], rule_text + ); + error!("{}", &msg_string); + process::exit(1) + } + }, + '/' => RValueType::Regex, + '%' => RValueType::Variable, + _ => RValueType::Value, + } + }, + value: { + let rv = caps["rule_value"].chars().next().unwrap(); + match rv { + '/' => caps["rule_value"].trim_matches('/').to_string(), + _ => caps["rule_value"].to_string().trim().to_string(), + } + }, + custom_msg: match caps.name("custom_msg") { + Some(s) => Some(s.as_str().to_string()), + None => None, + }, + }; + rules_hash.insert(RuleType::SimpleRule(rule)); + } } - let rules = rules_hash.into_iter().collect::>(); + let rules = rules_hash.into_iter().collect::>(); trace!("Destructured rules are: {:#?}", &rules); Ok(rules) } diff --git a/cfn-guard/tests/functional.rs b/cfn-guard/tests/functional.rs index 0118bec75..2bf7b7dc4 100644 --- a/cfn-guard/tests/functional.rs +++ b/cfn-guard/tests/functional.rs @@ -1467,6 +1467,14 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, (vec![], 0) ); + rules_file_contents = String::from( + "AWS::DynamoDB::Table if Tags != /.*DEV.*/ then .DeletionPolicy == Retain", + ); + assert_eq!( + cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), + (vec![], 0) + ); + rules_file_contents = String::from( "AWS::DynamoDB::Table if Tags == /.*PROD.*/ then .DeletionPolicy != Retain", ); @@ -1474,6 +1482,48 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), (vec![String::from("[DDBTable] failed because [.DeletionPolicy] is [Retain] and that value is not permitted when AWS::DynamoDB::Table Tags == /.*PROD.*/")], 2) + ); + rules_file_contents = String::from( + "AWS::DynamoDB::Table if Tags != /.*DEV.*/ then .DeletionPolicy != Retain", + ); + assert_eq!( + cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), + (vec![String::from("[DDBTable] failed because [.DeletionPolicy] is [Retain] and that value is not permitted when AWS::DynamoDB::Table Tags != /.*DEV.*/")], 2) ) } + #[test] + fn test_compound_conditional_check() { + let template_contents = fs::read_to_string("tests/conditional-ddb-template.yaml") + .unwrap_or_else(|err| format!("{}", err)); + + let mut rules_file_contents = String::from( + "AWS::DynamoDB::Table if Tags == /.*PROD.*/ then .DeletionPolicy == Retain |OR|AWS::DynamoDB::Table if Tags.* == /.*DEV.*/ then .UpdateReplacePolicy == Retain", + ); + assert_eq!( + cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), + (vec![], 0) + ); + + rules_file_contents = String::from( + "AWS::DynamoDB::Table if Tags != /.*DEV.*/ then .DeletionPolicy == Retain |OR| AWS::DynamoDB::Table Tags.* != PROD", + ); + assert_eq!( + cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), + (vec![], 0) + ); + + rules_file_contents = String::from( + r#"AWS::DynamoDB::Table if Tags == /.*PROD.*/ then .DeletionPolicy != Retain\ + AWS::DynamoDB::Table Tags.* != {"Key":"ENV","Value":"PROD"}"#, + ); + assert_eq!( + cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), + ( + vec![String::from( + r#"[DDBTable] failed because [Tags.0] is [{"Key":"ENV","Value":"PROD"}] and that value is not permitted"# + )], + 2 + ) + ); + } } From 1d8b670f070cc5501a39f5174715d97b92cd686b Mon Sep 17 00:00:00 2001 From: nathanmc Date: Tue, 28 Jul 2020 17:59:53 -0400 Subject: [PATCH 19/39] Make operators explicit in regex --- cfn-guard/src/parser.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cfn-guard/src/parser.rs b/cfn-guard/src/parser.rs index 8fb5914f8..95a1658d3 100644 --- a/cfn-guard/src/parser.rs +++ b/cfn-guard/src/parser.rs @@ -17,11 +17,11 @@ use lazy_static::lazy_static; // See: https://docs.rs/regex/1.3.9/regex/#example-avoid-compiling-the-same-regex-in-a-loop lazy_static! { static ref ASSIGN_REG: Regex = Regex::new(r"let (?P\w+) +(?P\S+) +(?P.+)").unwrap(); - static ref RULE_REG: Regex = Regex::new(r"(?P\S+) +(?P[\w\.\*]+) +(?P\S+) +(?P[^\n\r]+)").unwrap(); + static ref RULE_REG: Regex = Regex::new(r"^(?P\S+) +(?P[\w\.\*]+) +(?P==|!=|<|>|<=|>=|IN|NOT_IN) +(?P[^\n\r]+)").unwrap(); static ref COMMENT_REG: Regex = Regex::new(r#"#(?P.*)"#).unwrap(); static ref WILDCARD_OR_RULE_REG: Regex = Regex::new(r"(\S+) (\S+\*\S*) (==|IN) (.+)").unwrap(); static ref RULE_WITH_OPTIONAL_MESSAGE_REG: Regex = Regex::new( - r"(?P\S+) +(?P[\w\.\*]+) +(?P\S+) +(?P[^\n\r]+) +<{2} *(?P.*)").unwrap(); + r"^(?P\S+) +(?P[\w\.\*]+) +(?P==|!=|<|>|<=|>=|IN|NOT_IN) +(?P[^\n\r]+) +<{2} *(?P.*)").unwrap(); static ref WHITE_SPACE_REG: Regex = Regex::new(r"\s+").unwrap(); static ref CONDITIONAL_RULE_REG: Regex = Regex::new(r"(?P\S+) +if +(?P.+) +then +(?P.*)").unwrap(); } From a525c4813f516e11291d99364913ede1b3e28550 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Tue, 28 Jul 2020 18:00:35 -0400 Subject: [PATCH 20/39] Clean up whitespace around rules sooner --- cfn-guard/src/parser.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/cfn-guard/src/parser.rs b/cfn-guard/src/parser.rs index 95a1658d3..cbb31f2e1 100644 --- a/cfn-guard/src/parser.rs +++ b/cfn-guard/src/parser.rs @@ -48,14 +48,15 @@ pub(crate) fn parse_rules( for l in lines { debug!("Parsing '{}'", &l); - if l.is_empty() { + let trimmed_line = l.trim(); + if trimmed_line.is_empty() { continue; }; - let line_type = find_line_type(l); + let line_type = find_line_type(trimmed_line); debug!("line_type is {:#?}", line_type); match line_type { LineType::Assignment => { - let caps = match process_assignment(l) { + let caps = match process_assignment(trimmed_line) { Ok(a) => a, Err(e) => return Err(e), }; @@ -63,7 +64,7 @@ pub(crate) fn parse_rules( if caps["operator"] != *"=" { let msg_string = format!( "Bad Assignment Operator: [{}] in '{}'", - &caps["operator"], l + &caps["operator"], trimmed_line ); error!("{}", &msg_string); process::exit(1) @@ -78,14 +79,14 @@ pub(crate) fn parse_rules( variables.insert(var_name, var_value); } LineType::Comment => (), - LineType::Rule => match parse_rule_line(l, &cfn_resources) { + LineType::Rule => match parse_rule_line(trimmed_line, &cfn_resources) { Ok(prl) => { debug!("Parsed rule is: {:#?}", &prl); rule_set.push(prl) } Err(e) => return Err(e), }, - LineType::Conditional => match parse_rule_line(l, &cfn_resources) { + LineType::Conditional => match parse_rule_line(trimmed_line, &cfn_resources) { Ok(c) => { debug!("Parsed conditional is {:#?}", &c); rule_set.push(c); From 7a52154fc4391ba9db9d48c826a72a5f6049fd0d Mon Sep 17 00:00:00 2001 From: nathanmc Date: Tue, 28 Jul 2020 18:01:11 -0400 Subject: [PATCH 21/39] change logging of branches so they aren't just the split debug construct --- cfn-guard/src/parser.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cfn-guard/src/parser.rs b/cfn-guard/src/parser.rs index cbb31f2e1..107525ab7 100644 --- a/cfn-guard/src/parser.rs +++ b/cfn-guard/src/parser.rs @@ -224,9 +224,9 @@ fn process_and_rule( ) -> Result { trace!("Entered process_and_rule"); let branches = line.split("|AND|"); - debug!("Rule branches are: {:#?}", &branches); let mut rules: Vec = vec![]; for b in branches { + debug!("AND rule branch is: {:#?}", &b); match destructure_rule(b.trim(), cfn_resources) { Ok(r) => rules.append(&mut r.clone()), Err(e) => return Err(e), From 8eefff3f95bdca4b4aa94804a05b523412c00eb6 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Tue, 28 Jul 2020 18:01:49 -0400 Subject: [PATCH 22/39] Add better field handling and error messages around parsing conditionals --- cfn-guard/src/parser.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/cfn-guard/src/parser.rs b/cfn-guard/src/parser.rs index 107525ab7..4e98764a2 100644 --- a/cfn-guard/src/parser.rs +++ b/cfn-guard/src/parser.rs @@ -138,6 +138,14 @@ fn process_conditional( let caps = CONDITIONAL_RULE_REG.captures(line).unwrap(); trace!("ConditionalRule regex captures are {:#?}", &caps); + if RULE_REG.is_match(&caps["condition"]) + || RULE_WITH_OPTIONAL_MESSAGE_REG.is_match(&caps["condition"]) + { + return Err(format!( + "Invalid condition: '{}' in '{}'", + &caps["condition"], line + )); + } let conjd_caps_conditional = format!("{} {}", &caps["resource_type"], &caps["condition"]); trace!("conjd_caps_conditional is {:#?}", conjd_caps_conditional); match parse_rule_line(&conjd_caps_conditional, cfn_resources) { @@ -146,6 +154,14 @@ fn process_conditional( RuleType::CompoundRule(s) => s, _ => return Err(format!("Bad destructure of conditional rule: {}", line)), }; + if RULE_REG.is_match(&caps["consequent"]) + || RULE_WITH_OPTIONAL_MESSAGE_REG.is_match(&caps["consequent"]) + { + return Err(format!( + "Invalid consequent: '{}' in '{}'", + &caps["consequent"], line + )); + } let conjd_caps_consequent = format!("{} {}", &caps["resource_type"], &caps["consequent"]); trace!("conjd_caps_consequent is {:#?}", conjd_caps_consequent); From 84e2f1054910f7e823764c6ad24e522d15ef3bd1 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Tue, 28 Jul 2020 18:02:19 -0400 Subject: [PATCH 23/39] Add conditional example to Examples --- Examples/ddb-template.yaml | 64 ++++++++++++++++++++++++++++++++++++++ Examples/ddb.ruleset | 1 + 2 files changed, 65 insertions(+) create mode 100644 Examples/ddb-template.yaml create mode 100644 Examples/ddb.ruleset diff --git a/Examples/ddb-template.yaml b/Examples/ddb-template.yaml new file mode 100644 index 000000000..402497092 --- /dev/null +++ b/Examples/ddb-template.yaml @@ -0,0 +1,64 @@ +{ + "Resources": { + "DDBTable": { + "Type": "AWS::DynamoDB::Table", + "UpdateReplacePolicy": "Abort", + "DeletionPolicy": "Retain", + "Properties": { + "AttributeDefinitions": [ + { + "AttributeName": "ArtistId", + "AttributeType": "S" + }, + { + "AttributeName": "Concert", + "AttributeType": "S" + }, + { + "AttributeName": "TicketSales", + "AttributeType": "S" + } + ], + "KeySchema": [ + { + "AttributeName": "ArtistId", + + "KeyType": "HASH" + }, + { + "AttributeName": "Concert", + "KeyType": "RANGE" + } + ], + "GlobalSecondaryIndexes": [ + { + + "IndexName": "GSI", + "KeySchema": [ + + { + + "AttributeName": "TicketSales", + "KeyType": "HASH" + } + ], + "Projection": { + "ProjectionType": "KEYS_ONLY" + }, + "ProvisionedThroughput": { + "ReadCapacityUnits": 5, + "WriteCapacityUnits": 5 + } + } + ], + "ProvisionedThroughput": { + "ReadCapacityUnits": 5, + "WriteCapacityUnits": 5 + }, + "Tags": [ + {"Key": "ENV", "Value": "PROD"} + ] + } + } + } +} diff --git a/Examples/ddb.ruleset b/Examples/ddb.ruleset new file mode 100644 index 000000000..d096ec6e8 --- /dev/null +++ b/Examples/ddb.ruleset @@ -0,0 +1 @@ +AWS::DynamoDB::Table if Tags == /.*PROD.*/ then .DeletionPolicy == Retain From 21d98aa12f1936ea12ab4765c2521ea6795668fe Mon Sep 17 00:00:00 2001 From: nathanmc Date: Tue, 28 Jul 2020 18:07:08 -0400 Subject: [PATCH 24/39] Add tests for conditional corner cases including absent properties and expressing conditionals across types using inverse logic constructs --- cfn-guard/tests/functional.rs | 32 ++++++++++++++++--- ...ltiple-resources-conditional-template.yaml | 32 +++++++++++++++++++ 2 files changed, 60 insertions(+), 4 deletions(-) create mode 100644 cfn-guard/tests/test-multiple-resources-conditional-template.yaml diff --git a/cfn-guard/tests/functional.rs b/cfn-guard/tests/functional.rs index 2bf7b7dc4..ecd161d1b 100644 --- a/cfn-guard/tests/functional.rs +++ b/cfn-guard/tests/functional.rs @@ -1513,17 +1513,41 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, ); rules_file_contents = String::from( - r#"AWS::DynamoDB::Table if Tags == /.*PROD.*/ then .DeletionPolicy != Retain\ + r#"AWS::DynamoDB::Table if Tags == /.*PROD.*/ then .DeletionPolicy != Retain AWS::DynamoDB::Table Tags.* != {"Key":"ENV","Value":"PROD"}"#, ); assert_eq!( cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), ( - vec![String::from( - r#"[DDBTable] failed because [Tags.0] is [{"Key":"ENV","Value":"PROD"}] and that value is not permitted"# - )], + vec![ + String::from( + r#"[DDBTable] failed because [.DeletionPolicy] is [Retain] and that value is not permitted when AWS::DynamoDB::Table Tags == /.*PROD.*/"# + ), + String::from( + r#"[DDBTable] failed because [Tags.0] is [{"Key":"ENV","Value":"PROD"}] and that value is not permitted"# + ) + ], 2 ) ); } + + #[test] + fn test_conditional_corner_cases() { + // This test ensures that missing properties _aren't_ checked and an alternative approach to expressing conditional forms across different types + let template_contents = + fs::read_to_string("tests/test-multiple-resources-conditional-template.yaml") + .unwrap_or_else(|err| format!("{}", err)); + + let rules_file_contents = String::from( + r#"AWS::Lambda::Function if madeupproperty == somevalue << test of cond message then ProvisioningArtifactName == apig_2.0 << test of cons message + AWS::Lambda::Function Runtime != /.*/ |OR| AWS::ServiceCatalog::CloudFormationProvisionedProduct ProvisioningParameters.*.Value == lambdaFunction.Arn + AWS::Lambda::Function Runtime != Java |OR| AWS::ServiceCatalog::CloudFormationProvisionedProduct ProvisioningParameters.*.Value == lambdaFunction.Arn + AWS::ServiceCatalog::CloudFormationProvisionedProduct ProvisioningParameters.*.Value == lambdaFunction.Arn"#, + ); + assert_eq!( + cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), + (vec![], 0) + ) + } } diff --git a/cfn-guard/tests/test-multiple-resources-conditional-template.yaml b/cfn-guard/tests/test-multiple-resources-conditional-template.yaml new file mode 100644 index 000000000..5b6ce8ea3 --- /dev/null +++ b/cfn-guard/tests/test-multiple-resources-conditional-template.yaml @@ -0,0 +1,32 @@ +Resources: + lambdaFunction: + Type: "AWS::Lambda::Function" + Properties: + Code: + ZipFile: | + def handler(event,context): + return { + 'body': 'Hello there {0}'.format(event['requestContext']['identity']['sourceIp']), + 'headers': { + 'Content-Type': 'text/plain' + }, + 'statusCode': 200 + } + Description: "Governance Lambda Demo" + FunctionName: !Ref "lambdaFunctionName" + Handler: "index.handler" + MemorySize: 128 + Role: !GetAtt "lambdaIAMRole.Arn" + Runtime: "python2.7" + Timeout: 10 + + APIGatewayPP: + Type: "AWS::ServiceCatalog::CloudFormationProvisionedProduct" + Properties: + AcceptLanguage: en + ProvisionedProductName: "PP" + ProductName: "PG" + ProvisioningArtifactName: "apig_1.0" + ProvisioningParameters: + - Key: "lambdaFunctionArn" + Value: !GetAtt "lambdaFunction.Arn" \ No newline at end of file From 29f9068a545f79c6dee56d2c7f9b67e2674ef6ea Mon Sep 17 00:00:00 2001 From: nathanmc Date: Thu, 6 Aug 2020 15:25:15 -0400 Subject: [PATCH 25/39] Fix for whitespace match on rule parsing --- cfn-guard/src/parser.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cfn-guard/src/parser.rs b/cfn-guard/src/parser.rs index 4e98764a2..531c65301 100644 --- a/cfn-guard/src/parser.rs +++ b/cfn-guard/src/parser.rs @@ -22,8 +22,8 @@ lazy_static! { static ref WILDCARD_OR_RULE_REG: Regex = Regex::new(r"(\S+) (\S+\*\S*) (==|IN) (.+)").unwrap(); static ref RULE_WITH_OPTIONAL_MESSAGE_REG: Regex = Regex::new( r"^(?P\S+) +(?P[\w\.\*]+) +(?P==|!=|<|>|<=|>=|IN|NOT_IN) +(?P[^\n\r]+) +<{2} *(?P.*)").unwrap(); - static ref WHITE_SPACE_REG: Regex = Regex::new(r"\s+").unwrap(); static ref CONDITIONAL_RULE_REG: Regex = Regex::new(r"(?P\S+) +if +(?P.+) +then +(?P.*)").unwrap(); + static ref WHITE_SPACE_REG: Regex = Regex::new(r"^\s+$").unwrap(); } pub(crate) fn parse_rules( From 6f41d1bc17671c6706ea21f053dcd363f4db2e76 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Thu, 6 Aug 2020 15:26:42 -0400 Subject: [PATCH 26/39] Improve logging of rules file lines --- cfn-guard/src/parser.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cfn-guard/src/parser.rs b/cfn-guard/src/parser.rs index 531c65301..b980da375 100644 --- a/cfn-guard/src/parser.rs +++ b/cfn-guard/src/parser.rs @@ -44,7 +44,10 @@ pub(crate) fn parse_rules( let mut variables = HashMap::new(); let lines = rules_file_contents.lines(); - trace!("Rules file lines: {:#?}", &lines); + trace!( + "Rules file lines: {:#?}", + lines.clone().into_iter().collect::>() + ); for l in lines { debug!("Parsing '{}'", &l); From 93e434fc47a35ff59532dd96524674d8c6aa7516 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Thu, 6 Aug 2020 15:27:04 -0400 Subject: [PATCH 27/39] Delete commented code --- cfn-guard/src/parser.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/cfn-guard/src/parser.rs b/cfn-guard/src/parser.rs index b980da375..9fe030c5c 100644 --- a/cfn-guard/src/parser.rs +++ b/cfn-guard/src/parser.rs @@ -256,14 +256,6 @@ fn process_and_rule( raw_rule: line.to_string(), rule_list: rules, })) - // match destructure_rule(line, cfn_resources) { - // Ok(r) => Ok(CompoundRule { - // compound_type: CompoundType::AND, - // raw_rule: line.to_string(), - // rule_list: r, - // }), - // Err(e) => Err(e), - // } } fn destructure_rule( From c8da1e38446752824c2d4597322fca3def95332f Mon Sep 17 00:00:00 2001 From: nathanmc Date: Thu, 6 Aug 2020 15:35:28 -0400 Subject: [PATCH 28/39] Remove process::exit's from destructure rule and replace with proper Err returns --- cfn-guard/src/parser.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cfn-guard/src/parser.rs b/cfn-guard/src/parser.rs index 9fe030c5c..8574c2d8d 100644 --- a/cfn-guard/src/parser.rs +++ b/cfn-guard/src/parser.rs @@ -321,7 +321,7 @@ fn destructure_rule( &caps["operator"], rule_text ); error!("{}", &msg_string); - process::exit(1) + return Err(msg_string); } } }, @@ -337,7 +337,7 @@ fn destructure_rule( &caps["operator"], rule_text ); error!("{}", &msg_string); - process::exit(1) + return Err(msg_string); } }, '/' => RValueType::Regex, From ff4abe6a370ccd3cf4323328c2b4e90be56a9d13 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Thu, 6 Aug 2020 15:43:12 -0400 Subject: [PATCH 29/39] Change "bad operator" return to Err from process::exit --- cfn-guard/src/parser.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cfn-guard/src/parser.rs b/cfn-guard/src/parser.rs index 8574c2d8d..ea8995238 100644 --- a/cfn-guard/src/parser.rs +++ b/cfn-guard/src/parser.rs @@ -2,7 +2,6 @@ use std::collections::{HashMap, HashSet}; use std::env; -use std::process; use log::{self, debug, error, trace}; use regex::{Captures, Regex}; @@ -70,7 +69,7 @@ pub(crate) fn parse_rules( &caps["operator"], trimmed_line ); error!("{}", &msg_string); - process::exit(1) + return Err(msg_string); } let var_name = caps["var_name"].to_string(); let var_value = caps["var_value"].to_string(); From 043004f44016c22503fa69da130ff54bab2f575a Mon Sep 17 00:00:00 2001 From: nathanmc Date: Thu, 6 Aug 2020 15:44:31 -0400 Subject: [PATCH 30/39] Improve "invalid consequent" error message --- cfn-guard/src/parser.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cfn-guard/src/parser.rs b/cfn-guard/src/parser.rs index ea8995238..67ea1a152 100644 --- a/cfn-guard/src/parser.rs +++ b/cfn-guard/src/parser.rs @@ -160,7 +160,7 @@ fn process_conditional( || RULE_WITH_OPTIONAL_MESSAGE_REG.is_match(&caps["consequent"]) { return Err(format!( - "Invalid consequent: '{}' in '{}'", + "Invalid consequent: '{}' in '{}'. Consequents cannot contain resource types.", &caps["consequent"], line )); } From 51473a39b3c54d14577060bf281a336c3c27426e Mon Sep 17 00:00:00 2001 From: nathanmc Date: Thu, 6 Aug 2020 15:45:20 -0400 Subject: [PATCH 31/39] Change find_line_type to return Result<> --- cfn-guard/src/parser.rs | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/cfn-guard/src/parser.rs b/cfn-guard/src/parser.rs index 67ea1a152..b7ce42263 100644 --- a/cfn-guard/src/parser.rs +++ b/cfn-guard/src/parser.rs @@ -54,7 +54,10 @@ pub(crate) fn parse_rules( if trimmed_line.is_empty() { continue; }; - let line_type = find_line_type(trimmed_line); + let line_type = match find_line_type(trimmed_line) { + Ok(lt) => lt, + Err(e) => return Err(e), + }; debug!("line_type is {:#?}", line_type); match line_type { LineType::Assignment => { @@ -185,25 +188,25 @@ fn process_conditional( } } -fn find_line_type(line: &str) -> LineType { +fn find_line_type(line: &str) -> Result { if COMMENT_REG.is_match(line) { - return LineType::Comment; + return Ok(LineType::Comment); }; if ASSIGN_REG.is_match(line) { - return LineType::Assignment; + return Ok(LineType::Assignment); }; if CONDITIONAL_RULE_REG.is_match(line) { - return LineType::Conditional; + return Ok(LineType::Conditional); }; if RULE_REG.is_match(line) { - return LineType::Rule; + return Ok(LineType::Rule); }; if WHITE_SPACE_REG.is_match(line) { - return LineType::WhiteSpace; + return Ok(LineType::WhiteSpace); } let msg_string = format!("BAD RULE: {:?}", line); error!("{}", &msg_string); - process::exit(1) + Err(msg_string) } fn process_assignment(line: &str) -> Result { @@ -375,10 +378,10 @@ mod tests { let assignment = find_line_type("let x = assignment"); let rule = find_line_type("AWS::EC2::Volume Encryption == true"); let white_space = find_line_type(" "); - assert_eq!(comment, crate::enums::LineType::Comment); - assert_eq!(assignment, crate::enums::LineType::Assignment); - assert_eq!(rule, crate::enums::LineType::Rule); - assert_eq!(white_space, crate::enums::LineType::WhiteSpace) + assert_eq!(comment, Ok(crate::enums::LineType::Comment)); + assert_eq!(assignment, Ok(crate::enums::LineType::Assignment)); + assert_eq!(rule, Ok(crate::enums::LineType::Rule)); + assert_eq!(white_space, Ok(crate::enums::LineType::WhiteSpace)) } #[test] From de76a6a715034f9fbe2215481405adb19730cf7b Mon Sep 17 00:00:00 2001 From: nathanmc Date: Thu, 6 Aug 2020 15:46:05 -0400 Subject: [PATCH 32/39] Change syntax from `if/then` to `when/check` for conditionals --- cfn-guard/src/parser.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cfn-guard/src/parser.rs b/cfn-guard/src/parser.rs index b7ce42263..95fad1cbd 100644 --- a/cfn-guard/src/parser.rs +++ b/cfn-guard/src/parser.rs @@ -21,8 +21,8 @@ lazy_static! { static ref WILDCARD_OR_RULE_REG: Regex = Regex::new(r"(\S+) (\S+\*\S*) (==|IN) (.+)").unwrap(); static ref RULE_WITH_OPTIONAL_MESSAGE_REG: Regex = Regex::new( r"^(?P\S+) +(?P[\w\.\*]+) +(?P==|!=|<|>|<=|>=|IN|NOT_IN) +(?P[^\n\r]+) +<{2} *(?P.*)").unwrap(); - static ref CONDITIONAL_RULE_REG: Regex = Regex::new(r"(?P\S+) +if +(?P.+) +then +(?P.*)").unwrap(); static ref WHITE_SPACE_REG: Regex = Regex::new(r"^\s+$").unwrap(); + static ref CONDITIONAL_RULE_REG: Regex = Regex::new(r"(?P\S+) +(when|WHEN) +(?P.+) +(check|CHECK) +(?P.*)").unwrap(); } pub(crate) fn parse_rules( From 7d0b5594b5e49484bd67aad545042a4d5a671198 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Thu, 6 Aug 2020 15:46:49 -0400 Subject: [PATCH 33/39] Update conditional tests to use `when/check` and add coverage for bad rule and bad assignment tests --- cfn-guard/tests/functional.rs | 98 +++++++++++++++++++++++++++++------ 1 file changed, 81 insertions(+), 17 deletions(-) diff --git a/cfn-guard/tests/functional.rs b/cfn-guard/tests/functional.rs index ecd161d1b..a745329d7 100644 --- a/cfn-guard/tests/functional.rs +++ b/cfn-guard/tests/functional.rs @@ -1454,13 +1454,29 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, ); } + #[test] + fn test_bad_rule() { + let template_contents = fs::read_to_string("tests/conditional-ddb-template.yaml") + .unwrap_or_else(|err| format!("{}", err)); + + let rules_file_contents = String::from( + "AWS::DynamoDB::Table if Tags == /.*PROD.*/ then .DeletionPolicy == Retain", + ); + assert_eq!( + cfn_guard::run_check(&template_contents, &rules_file_contents, false), + Err(String::from( + r#"BAD RULE: "AWS::DynamoDB::Table if Tags == /.*PROD.*/ then .DeletionPolicy == Retain""# + )) + ); + } + #[test] fn test_conditional_check() { let template_contents = fs::read_to_string("tests/conditional-ddb-template.yaml") .unwrap_or_else(|err| format!("{}", err)); let mut rules_file_contents = String::from( - "AWS::DynamoDB::Table if Tags == /.*PROD.*/ then .DeletionPolicy == Retain", + "AWS::DynamoDB::Table when Tags == /.*PROD.*/ check .DeletionPolicy == Retain", ); assert_eq!( cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), @@ -1468,7 +1484,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, ); rules_file_contents = String::from( - "AWS::DynamoDB::Table if Tags != /.*DEV.*/ then .DeletionPolicy == Retain", + "AWS::DynamoDB::Table when Tags != /.*DEV.*/ check .DeletionPolicy == Retain", ); assert_eq!( cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), @@ -1476,7 +1492,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, ); rules_file_contents = String::from( - "AWS::DynamoDB::Table if Tags == /.*PROD.*/ then .DeletionPolicy != Retain", + "AWS::DynamoDB::Table WHEN Tags == /.*PROD.*/ check .DeletionPolicy != Retain", ); assert_eq!( cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), @@ -1484,20 +1500,21 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, 2) ); rules_file_contents = String::from( - "AWS::DynamoDB::Table if Tags != /.*DEV.*/ then .DeletionPolicy != Retain", + "AWS::DynamoDB::Table when Tags != /.*DEV.*/ CHECK .DeletionPolicy != Retain", ); assert_eq!( cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), (vec![String::from("[DDBTable] failed because [.DeletionPolicy] is [Retain] and that value is not permitted when AWS::DynamoDB::Table Tags != /.*DEV.*/")], 2) ) } + #[test] fn test_compound_conditional_check() { let template_contents = fs::read_to_string("tests/conditional-ddb-template.yaml") .unwrap_or_else(|err| format!("{}", err)); let mut rules_file_contents = String::from( - "AWS::DynamoDB::Table if Tags == /.*PROD.*/ then .DeletionPolicy == Retain |OR|AWS::DynamoDB::Table if Tags.* == /.*DEV.*/ then .UpdateReplacePolicy == Retain", + "AWS::DynamoDB::Table when Tags == /.*PROD.*/ check .DeletionPolicy == Retain |OR|AWS::DynamoDB::Table WHEN Tags.* == /.*DEV.*/ CHECK .UpdateReplacePolicy == Retain", ); assert_eq!( cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), @@ -1505,7 +1522,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, ); rules_file_contents = String::from( - "AWS::DynamoDB::Table if Tags != /.*DEV.*/ then .DeletionPolicy == Retain |OR| AWS::DynamoDB::Table Tags.* != PROD", + "AWS::DynamoDB::Table when Tags != /.*DEV.*/ CHECK .DeletionPolicy == Retain |OR| AWS::DynamoDB::Table Tags.* != PROD", ); assert_eq!( cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), @@ -1513,20 +1530,33 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, ); rules_file_contents = String::from( - r#"AWS::DynamoDB::Table if Tags == /.*PROD.*/ then .DeletionPolicy != Retain - AWS::DynamoDB::Table Tags.* != {"Key":"ENV","Value":"PROD"}"#, + r#"AWS::DynamoDB::Table WHEN Tags == /.*PROD.*/ check .DeletionPolicy != Retain"#, ); assert_eq!( cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), ( - vec![ - String::from( - r#"[DDBTable] failed because [.DeletionPolicy] is [Retain] and that value is not permitted when AWS::DynamoDB::Table Tags == /.*PROD.*/"# - ), - String::from( - r#"[DDBTable] failed because [Tags.0] is [{"Key":"ENV","Value":"PROD"}] and that value is not permitted"# - ) - ], + vec![String::from( + r#"[DDBTable] failed because [.DeletionPolicy] is [Retain] and that value is not permitted when AWS::DynamoDB::Table Tags == /.*PROD.*/"# + )], + 2 + ) + ); + } + + #[test] + fn test_conditional_checks_with_custom_messages() { + let template_contents = fs::read_to_string("tests/conditional-ddb-template.yaml") + .unwrap_or_else(|err| format!("{}", err)); + + let rules_file_contents = String::from( + r#"AWS::DynamoDB::Table WHEN Tags == /.*PROD.*/ << custom conditional message check .DeletionPolicy != Retain << custom consequent message"#, + ); + assert_eq!( + cfn_guard::run_check(&template_contents, &rules_file_contents, false).unwrap(), + ( + vec![String::from( + r#"[DDBTable] failed because [.DeletionPolicy] is [Retain] and custom consequent message when AWS::DynamoDB::Table Tags == /.*PROD.*/ << custom conditional message"# + )], 2 ) ); @@ -1540,7 +1570,7 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, .unwrap_or_else(|err| format!("{}", err)); let rules_file_contents = String::from( - r#"AWS::Lambda::Function if madeupproperty == somevalue << test of cond message then ProvisioningArtifactName == apig_2.0 << test of cons message + r#"AWS::Lambda::Function WHEN madeupproperty == somevalue << test of cond message CHECK ProvisioningArtifactName == apig_2.0 << test of cons message AWS::Lambda::Function Runtime != /.*/ |OR| AWS::ServiceCatalog::CloudFormationProvisionedProduct ProvisioningParameters.*.Value == lambdaFunction.Arn AWS::Lambda::Function Runtime != Java |OR| AWS::ServiceCatalog::CloudFormationProvisionedProduct ProvisioningParameters.*.Value == lambdaFunction.Arn AWS::ServiceCatalog::CloudFormationProvisionedProduct ProvisioningParameters.*.Value == lambdaFunction.Arn"#, @@ -1550,4 +1580,38 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, (vec![], 0) ) } + + #[test] + fn test_conditional_bad_consequent() { + // This test ensures that missing properties _aren't_ checked and an alternative approach to expressing conditional forms across different types + let template_contents = + fs::read_to_string("tests/test-multiple-resources-conditional-template.yaml") + .unwrap_or_else(|err| format!("{}", err)); + + let rules_file_contents = String::from( + r#"AWS::Lambda::Function WHEN madeupproperty == somevalue << test of cond message CHECK AWS::EC2::Volume ProvisioningArtifactName == apig_2.0 << test of cons message"#, + ); + assert_eq!( + cfn_guard::run_check(&template_contents, &rules_file_contents, false), + Err(String::from( + r#"Invalid consequent: 'AWS::EC2::Volume ProvisioningArtifactName == apig_2.0 << test of cons message' in 'AWS::Lambda::Function WHEN madeupproperty == somevalue << test of cond message CHECK AWS::EC2::Volume ProvisioningArtifactName == apig_2.0 << test of cons message'. Consequents cannot contain resource types."# + )) + ) + } + + #[test] + fn test_bad_assignment() { + let template_contents = + fs::read_to_string("tests/test-multiple-resources-conditional-template.yaml") + .unwrap_or_else(|err| format!("{}", err)); + + let rules_file_contents = String::from(r#"let x == y"#); + + assert_eq!( + cfn_guard::run_check(&template_contents, &rules_file_contents, false), + Err(String::from( + r#"Bad Assignment Operator: [==] in 'let x == y'"# + )) + ) + } } From 5bf9fe4a0b6dcd401d6bf95f376a8c21cdd15286 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Thu, 6 Aug 2020 15:47:33 -0400 Subject: [PATCH 34/39] Update README for when checks and add ddb when-check example to Examples --- Examples/conditional-ddb-template.ruleset | 3 + Examples/conditional-ddb-template.yaml | 64 ++++++++++++++++ cfn-guard/README.md | 93 +++++++++++++++++++++-- 3 files changed, 152 insertions(+), 8 deletions(-) create mode 100644 Examples/conditional-ddb-template.ruleset create mode 100644 Examples/conditional-ddb-template.yaml diff --git a/Examples/conditional-ddb-template.ruleset b/Examples/conditional-ddb-template.ruleset new file mode 100644 index 000000000..977519e8f --- /dev/null +++ b/Examples/conditional-ddb-template.ruleset @@ -0,0 +1,3 @@ +# https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-product-attribute-reference.html +AWS::DynamoDB::Table if Tags == /.*PROD.*/ then .DeletionPolicy == Retain +AWS::DynamoDB::Table if Tags == /.*PROD.*/ then .DeletionPolicy != Retain diff --git a/Examples/conditional-ddb-template.yaml b/Examples/conditional-ddb-template.yaml new file mode 100644 index 000000000..402497092 --- /dev/null +++ b/Examples/conditional-ddb-template.yaml @@ -0,0 +1,64 @@ +{ + "Resources": { + "DDBTable": { + "Type": "AWS::DynamoDB::Table", + "UpdateReplacePolicy": "Abort", + "DeletionPolicy": "Retain", + "Properties": { + "AttributeDefinitions": [ + { + "AttributeName": "ArtistId", + "AttributeType": "S" + }, + { + "AttributeName": "Concert", + "AttributeType": "S" + }, + { + "AttributeName": "TicketSales", + "AttributeType": "S" + } + ], + "KeySchema": [ + { + "AttributeName": "ArtistId", + + "KeyType": "HASH" + }, + { + "AttributeName": "Concert", + "KeyType": "RANGE" + } + ], + "GlobalSecondaryIndexes": [ + { + + "IndexName": "GSI", + "KeySchema": [ + + { + + "AttributeName": "TicketSales", + "KeyType": "HASH" + } + ], + "Projection": { + "ProjectionType": "KEYS_ONLY" + }, + "ProvisionedThroughput": { + "ReadCapacityUnits": 5, + "WriteCapacityUnits": 5 + } + } + ], + "ProvisionedThroughput": { + "ReadCapacityUnits": 5, + "WriteCapacityUnits": 5 + }, + "Tags": [ + {"Key": "ENV", "Value": "PROD"} + ] + } + } + } +} diff --git a/cfn-guard/README.md b/cfn-guard/README.md index b8d50df55..cdd262ca5 100644 --- a/cfn-guard/README.md +++ b/cfn-guard/README.md @@ -101,10 +101,10 @@ We modeled `cfn-guard` rules on firewall rules. They're easy to write and have The most basic CloudFormation Guard rule has the form: ``` - == + ``` -The available operations are: +The available operators are: * `==` - Equal * `!=` - Not Equal @@ -115,6 +115,45 @@ The available operations are: * `IN` - In a list of form `[x, y, z]` * `NOT_IN` - Not in a list of form `[x, y, z]` +## Checking Resource Properties and Attributes + +Properties in a rule can take two forms. The basic form exists to make writing simple rules very straightforward: + +``` +AWS::EC2::Volume Encryption == true +``` + +This simple form makes the assumption that the property you're checking is in the resource's `Properties` section: + +``` + "NewVolume" : { + "Type" : "AWS::EC2::Volume", + "Properties" : { + "Size" : 101, + "Encrypted": true, + "AvailabilityZone" : "us-west-2b" + } + } +``` + +However, you may also want to write a rule that checks the resource's [Attributes](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-product-attribute-reference.html): + +``` + "NewVolume" : { + "Type" : "AWS::EC2::Volume", + "Properties" : { + "Size" : "100", + "Encrypted" : "true", + }, + "DeletionPolicy" : "Snapshot" + } +``` +In this case, let's say we want to check the `DeletionPolicy` for deployment safety reasons. We could write a rule that checks attributes at the level above `Properties` by preceding the symbol in the property position with a `.` to indicate that you want to examine a value at the root of the resource: + +``` +AWS::EC2::Volume .DeletionPolicy == Snapshot +``` + ## Comments Comments can be added to a rule set via the `#` operator: @@ -125,15 +164,41 @@ Comments can be added to a rule set via the `#` operator: ## Rule Logic +### ANDs and ORs Each rule in a given rule set is implicitly `AND`'d to every other rule. -You can `OR` rules to provide alternate acceptable values of arbitrary types using `|OR|`: +You can `OR` rules on a single line to provide alternate acceptable values of arbitrary types using `|OR|`: ``` AWS::EBS::Volume Size == 500 |OR| AWS::EBS::Volume AvailabiltyZone == us-east-1b ``` +### WHEN checks +At times, you may not want to check every resource of a particular type in a template for the same values. You can write conditional checks using the `WHEN-CHECK` syntax: + +``` + WHEN CHECK ``` +``` +As an example: +``` +AWS::DynamoDB::Table WHEN Tags.* == /.*PROD.*/ CHECK .DeletionPolicy == Retain +``` +The first section (`WHEN Tags.* == /.*PROD.*/`) is the `condition` you want to filter on. It uses the same property and value syntax and semantics as a basic rule. + +The second section (`CHECK .DeletionPolicy == Retain`) is the `consequent` that the resource must pass for the rule to pass. + +If the `condition` matches, the `consequent` is evaluated and the result of that evaluation is added to the overall ruleset results. + +Note that `WHEN` checks **can only operate on a single resource type at a time**. They can also be aggregated using `OR`'s like a regular rule: + +``` +AWS::DynamoDB::Table when Tags == /.*PROD.*/ check .DeletionPolicy == Retain |OR| AWS::DynamoDB::Table WHEN Tags.* == /.*DEV.*/ CHECK .DeletionPolicy == Delete +``` + +To see a practical example of a conditional rule, look at the `conditional-ddb-template` files in the [Examples](../Examples) directory. + ## Checking nested fields +### Using explicit paths Fields that are nested inside CloudFormation [resource properties](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-template-resource-type-ref.html) can be addressed using a dotted notation: ``` @@ -154,9 +219,7 @@ Resources: Service: - lambda.amazonaws.com ``` -## Wildcards -### Syntax - +### Using Wildcards You can also refer to template items, lists and maps as wildcards (`*`). Wildcards are a preprocessor macro that examines both the rules file and the template to expand the wildcards into lists of rules of the same length as those contained in the template that's being checked. In other words, given a template of the form: @@ -185,7 +248,7 @@ CloudFormation Guard will walk the template and internally convert the wildcard ``` AWS::IAM::Role AssumeRolePolicyDocument.Statement.0.Principal.Service.0 == lambda.amazonaws.com |OR| AWS::IAM::Role AssumeRolePolicyDocument.Statement.1.Principal.Service.0 == ec2.amazonaws.com ``` -### Semantics +#### Wildcard Semantics Note carefully the different semantic meanings between the equality (`==`) or in-a-list (`IN`) operators and the inequality (`!=`) or not-in-a-list (`NOT_IN`) ones with wildcards: ``` @@ -288,7 +351,7 @@ That will give you more information on how `cfn-guard` is processing it. (See [Troubleshooting](#troubleshooting) for more details on using the different logging levels to see how your template and rule set are being processed.) -### Environment Varibles +### Environment Variables You can even reference **environment variables** using the Makefile-style notation: `%{Name}` So you could rewrite the IAM Role rule above as: @@ -351,6 +414,20 @@ The result looks like an erroneous repeat: "[NewVolume2] failed because [AvailabilityZone] is [us-west-2c] and lorem ipsum" "[NewVolume2] failed because [AvailabilityZone] is [us-west-2c] and lorem ipsum" ``` + +Custom messages are syntactically valid on both sides of a [WHEN check](README.md#when-checks): + +``` +AWS::DynamoDB::Table WHEN Tags == /.*PROD.*/ << custom conditional message CHECK .DeletionPolicy != Retain << custom consequent message +``` + +But the `condition`'s custom message is only exposed inline as part of the raw rule included in the error message: + +``` +[DDBTable] failed because [.DeletionPolicy] is [Retain] and custom consequent message when AWS::DynamoDB::Table Tags == /.*PROD.*/ << custom conditional message +``` + + ## Working with CloudFormation Intrinsic Functions Because of the way YAML is parsed by serde_yaml, functions like `!GetAtt` are treated as comments and ignored. For example: ``` From 90ecf72593f02d2181117698962b05d2f40867d4 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Fri, 7 Aug 2020 15:48:38 -0400 Subject: [PATCH 35/39] Fix the wildcard regex match to allow for "*" by itself --- cfn-guard/src/parser.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cfn-guard/src/parser.rs b/cfn-guard/src/parser.rs index 95fad1cbd..da7b3f18d 100644 --- a/cfn-guard/src/parser.rs +++ b/cfn-guard/src/parser.rs @@ -18,7 +18,7 @@ lazy_static! { static ref ASSIGN_REG: Regex = Regex::new(r"let (?P\w+) +(?P\S+) +(?P.+)").unwrap(); static ref RULE_REG: Regex = Regex::new(r"^(?P\S+) +(?P[\w\.\*]+) +(?P==|!=|<|>|<=|>=|IN|NOT_IN) +(?P[^\n\r]+)").unwrap(); static ref COMMENT_REG: Regex = Regex::new(r#"#(?P.*)"#).unwrap(); - static ref WILDCARD_OR_RULE_REG: Regex = Regex::new(r"(\S+) (\S+\*\S*) (==|IN) (.+)").unwrap(); + static ref WILDCARD_OR_RULE_REG: Regex = Regex::new(r"(\S+) (\S*\*\S*) (==|IN) (.+)").unwrap(); static ref RULE_WITH_OPTIONAL_MESSAGE_REG: Regex = Regex::new( r"^(?P\S+) +(?P[\w\.\*]+) +(?P==|!=|<|>|<=|>=|IN|NOT_IN) +(?P[^\n\r]+) +<{2} *(?P.*)").unwrap(); static ref WHITE_SPACE_REG: Regex = Regex::new(r"^\s+$").unwrap(); From 49e74499b560da3e876706031cf716a331f2b470 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Fri, 7 Aug 2020 15:51:42 -0400 Subject: [PATCH 36/39] Fix wildcard lookups to work at the root of the resource json for things like attribute-level wildcarding. (eg ".*"). Add comments to clarify what's happening. --- cfn-guard/src/lib.rs | 7 ++++--- cfn-guard/src/parser.rs | 22 ++++++++++++++++++++-- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/cfn-guard/src/lib.rs b/cfn-guard/src/lib.rs index 5f5e3541e..3e06a911f 100644 --- a/cfn-guard/src/lib.rs +++ b/cfn-guard/src/lib.rs @@ -350,11 +350,12 @@ fn apply_rule( let (property_root, address) = match target_field.first() { Some(x) => { if *x == "" { + // If the first address segment is a '.' target_field.remove(0); - target_field.insert(0, "."); - (cfn_resource, target_field) + target_field.insert(0, "."); // Replace the empty first element with a "." + (cfn_resource, target_field) // Return the root of the Value for lookup } else { - (&cfn_resource["Properties"], target_field) + (&cfn_resource["Properties"], target_field) // Otherwise, treat it as a normal property lookup } } None => { diff --git a/cfn-guard/src/parser.rs b/cfn-guard/src/parser.rs index da7b3f18d..312750b47 100644 --- a/cfn-guard/src/parser.rs +++ b/cfn-guard/src/parser.rs @@ -289,9 +289,27 @@ fn destructure_rule( if caps["resource_property"].contains('*') { for (_name, value) in cfn_resources { if caps["resource_type"] == value["Type"] { + let target_field: Vec<&str> = caps["resource_property"].split('.').collect(); + let (property_root, address) = match target_field.first() { + Some(x) => { + if *x == "" { + // If the first address segment is a '.' + (value, target_field) // Return the root of the Value for lookup + } else { + // Otherwise, treat it as a normal property lookup + (&value["Properties"], target_field) + } + } + None => { + let msg_string = + format!("Invalid property address: {:#?}", target_field); + error!("{}", msg_string); + return Err(msg_string); + } + }; if let Some(p) = util::expand_wildcard_props( - &value["Properties"], - caps["resource_property"].to_string(), + property_root, + address.join("."), String::from(""), ) { props.append(&mut p.clone()); From 175ddb8a208f60475adf17a1df01d4c56e0a7533 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Fri, 7 Aug 2020 17:20:46 -0400 Subject: [PATCH 37/39] Add comment to GuardTypes to explain the purpose of SimpleRule --- cfn-guard/src/guard_types.rs | 3 ++- cfn-guard/tests/root-wildcard-template.json | 20 ++++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) create mode 100644 cfn-guard/tests/root-wildcard-template.json diff --git a/cfn-guard/src/guard_types.rs b/cfn-guard/src/guard_types.rs index 541f6fb64..4017b92e5 100644 --- a/cfn-guard/src/guard_types.rs +++ b/cfn-guard/src/guard_types.rs @@ -40,7 +40,8 @@ pub mod enums { pub enum RuleType { CompoundRule(super::structs::CompoundRule), ConditionalRule(super::structs::ConditionalRule), - SimpleRule(super::structs::Rule), + SimpleRule(super::structs::Rule), // SimpleRule is a rule that cannot be reduced/transformed any further + // It's the base case for recursing into rule processing } } diff --git a/cfn-guard/tests/root-wildcard-template.json b/cfn-guard/tests/root-wildcard-template.json new file mode 100644 index 000000000..c48fbf37b --- /dev/null +++ b/cfn-guard/tests/root-wildcard-template.json @@ -0,0 +1,20 @@ +{ + "Resources": { + "NewVolume" : { + "Type" : "AWS::EC2::Volume", + "Properties" : { + "Size" : 101, + "Encrypted": true, + "AvailabilityZone" : "us-west-2b" + } + }, + "NewVolume2" : { + "Type" : "AWS::EC2::Volume", + "Properties" : { + "Size" : 99, + "Encrypted": true, + "AvailabilityZone" : "us-west-2c" + } + } + } +} From e844fbd0bb474377f5181236c524d053ff4e18bd Mon Sep 17 00:00:00 2001 From: nathanmc Date: Fri, 7 Aug 2020 17:21:13 -0400 Subject: [PATCH 38/39] Add tests for root wildcard property (ie, "*") --- cfn-guard/tests/functional.rs | 34 +++++++++++++++++++++ cfn-guard/tests/root-wildcard-template.json | 1 + 2 files changed, 35 insertions(+) diff --git a/cfn-guard/tests/functional.rs b/cfn-guard/tests/functional.rs index a745329d7..88929d7ff 100644 --- a/cfn-guard/tests/functional.rs +++ b/cfn-guard/tests/functional.rs @@ -1614,4 +1614,38 @@ AWS::EC2::Volume Size == 101 |OR| AWS::EC2::Volume Size == 99"#, )) ) } + #[test] + fn test_root_wildcard() { + let template_contents = fs::read_to_string("tests/root-wildcard-template.json") + .unwrap_or_else(|err| format!("{}", err)); + + let mut rules_file_contents = String::from(r#"AWS::EC2::Volume * != true"#); + + assert_eq!( + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), + ( + vec![ + String::from( + r#"[NewVolume2] failed because [Encrypted] is [true] and that value is not permitted"# + ), + String::from( + r#"[NewVolume2] failed because it does not contain the required property of [AutoEnableIO]"# + ), + String::from( + r#"[NewVolume] failed because [AutoEnableIO] is [true] and that value is not permitted"# + ), + String::from( + r#"[NewVolume] failed because [Encrypted] is [true] and that value is not permitted"# + ) + ], + 2 + ) + ); + + rules_file_contents = String::from(r#"AWS::EC2::Volume * == true"#); + assert_eq!( + cfn_guard::run_check(&template_contents, &rules_file_contents, true).unwrap(), + (vec![], 0) + ) + } } diff --git a/cfn-guard/tests/root-wildcard-template.json b/cfn-guard/tests/root-wildcard-template.json index c48fbf37b..ac9fa7376 100644 --- a/cfn-guard/tests/root-wildcard-template.json +++ b/cfn-guard/tests/root-wildcard-template.json @@ -3,6 +3,7 @@ "NewVolume" : { "Type" : "AWS::EC2::Volume", "Properties" : { + "AutoEnableIO": true, "Size" : 101, "Encrypted": true, "AvailabilityZone" : "us-west-2b" From 3a5e5b8b3ffb16a9a6f8fde1bfbb171be8aa3a45 Mon Sep 17 00:00:00 2001 From: nathanmc Date: Fri, 7 Aug 2020 17:39:36 -0400 Subject: [PATCH 39/39] Add discussion of strict-checks and wildcards to the cfn-guard README --- cfn-guard/README.md | 158 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 158 insertions(+) diff --git a/cfn-guard/README.md b/cfn-guard/README.md index cdd262ca5..72970cec8 100644 --- a/cfn-guard/README.md +++ b/cfn-guard/README.md @@ -502,6 +502,164 @@ AWS::EC2::Volume Size == 100 |OR| AWS::EC2::Volume Size == 99 AWS::EC2::Volume Encrypted == true |OR| AWS::EC2::Volume Encrypted == false AWS::EC2::Volume AvailabilityZone == {"Fn::GetAtt":["EC2Instance","AvailabilityZone"]} ``` +# Strict Checks +The `--strict-check` flag will cause a resource to fail a check if it does not contain the property the rule is checking. This is useful to enforce the presence of optional properties like `Encryption == true`. + +Strict checks and wildcards need to be carefully thought out before being used together, however. Wildcards create rules at runtime that map to all of the values that *each* resource of that type has at the position of the wildcard. That means means that overly broad wildcards will give overly broad failures. + +As an example, let's look at the following wildcard scenario: + +Here's a template snippet: +``` +{ + "Resources": { + "NewVolume" : { + "Type" : "AWS::EC2::Volume", + "Properties" : { + "AutoEnableIO": true, + "Size" : 101, + "Encrypted": true, + "AvailabilityZone" : "us-west-2b" + } + }, + "NewVolume2" : { + "Type" : "AWS::EC2::Volume", + "Properties" : { + "Size" : 99, + "Encrypted": true, + "AvailabilityZone" : "us-west-2c" + } + } + } +} +``` +It's perfectly valid semantically (although of dubious practical value) to use a wildcard to ensure that at least one property has a value equal to true: +``` +AWS::EC2::Volume * == true +``` +As discussed above in the section about wildcards, this translates at runtime to a rule for each property being created and joined by an `|OR|`: +``` +> cfn-guard -t ~/scratch-template.yaml -r ~/scratch.ruleset -vvv +... +2020-08-07 17:25:59,000 INFO [cfn_guard] Applying rule 'CompoundRule( + CompoundRule { + compound_type: OR, + raw_rule: "AWS::EC2::Volume * == true", + rule_list: [ + SimpleRule( + Rule { + resource_type: "AWS::EC2::Volume", + field: "AvailabilityZone", + operation: Require, + value: "true", + rule_vtype: Value, + custom_msg: None, + }, + ), + SimpleRule( + Rule { + resource_type: "AWS::EC2::Volume", + field: "Size", + operation: Require, + value: "true", + rule_vtype: Value, + custom_msg: None, + }, + ), + SimpleRule( + Rule { + resource_type: "AWS::EC2::Volume", + field: "Encrypted", + operation: Require, + value: "true", + rule_vtype: Value, + custom_msg: None, + }, + ), + SimpleRule( + Rule { + resource_type: "AWS::EC2::Volume", + field: "AutoEnableIO", + operation: Require, + value: "true", + rule_vtype: Value, + custom_msg: None, + }, + ), + ], + }, +)' + +``` +And the check will pass. + +However, if you change your wildcard rule to be a `!=`: +``` +AWS::EC2::Volume * != false +``` + +The `OR` rule becomes an `AND` rule: +``` +2020-08-07 17:33:20,637 INFO [cfn_guard] Applying rule 'CompoundRule( + CompoundRule { + compound_type: AND, + raw_rule: "AWS::EC2::Volume * != false", + rule_list: [ + SimpleRule( + Rule { + resource_type: "AWS::EC2::Volume", + field: "AvailabilityZone", + operation: RequireNot, + value: "false", + rule_vtype: Value, + custom_msg: None, + }, + ), + SimpleRule( + Rule { + resource_type: "AWS::EC2::Volume", + field: "AutoEnableIO", + operation: RequireNot, + value: "false", + rule_vtype: Value, + custom_msg: None, + }, + ), + SimpleRule( + Rule { + resource_type: "AWS::EC2::Volume", + field: "Size", + operation: RequireNot, + value: "false", + rule_vtype: Value, + custom_msg: None, + }, + ), + SimpleRule( + Rule { + resource_type: "AWS::EC2::Volume", + field: "Encrypted", + operation: RequireNot, + value: "false", + rule_vtype: Value, + custom_msg: None, + }, + ), + ], + }, +)' +``` + +And if you run it with `--strict-checks` it'll fail because `NewVolume2` does not contain the `AutoEnableIO` property: + +``` +> cfn-guard -t ~/scratch-template.yaml -r ~/scratch.ruleset --strict-checks +[NewVolume2] failed because it does not contain the required property of [AutoEnableIO] +Number of failures: 1 +``` +Admittedly, this is a very contrived example, but it's an important to behavior understand. + + # Troubleshooting `cfn-guard` is meant to be used as part of a tool chain. It does not, for instance, check to see if the CloudFormation template presented to it is valid CloudFormation. The [cfn-lint](https://github.com/aws-cloudformation/cfn-python-lint) tool already does a deep and thorough inspection of template structure and provides copious feedback to help users write high-quality templates.