From 6dc98ba170ad2a44c4a41f5a0958b0f5592ef8d2 Mon Sep 17 00:00:00 2001 From: Madison Steiner Date: Wed, 12 Aug 2026 14:15:09 +0000 Subject: [PATCH 01/10] Retire the superseded evaluator and its query engine guard/src/rules/evaluate.rs was the pre-eval.rs evaluator. Nothing on the CLI path reached it: the compiler reported 15 items in it as never used, RootScope::new was called only from evaluate_tests.rs, and its one apparent outside consumer -- path_value.rs's QueryResolver::select -- takes &dyn EvaluationContext, the old context trait. select's only non-test callers were itself, evaluate.rs, and MetadataAppender, which is constructed only in its own test. One connected dead component, not a scattering. Removed: - evaluate.rs (1342) and evaluate_tests.rs (2345) - QueryResolver and PathAwareValue::select, plus the four helpers only select used: map_error_or_empty, map_some_or_error_all, retrieve_index, accumulate. eval_context.rs already has its own retrieve_index and accumulate as free functions, so the new engine loses nothing. - aws_meta_appender.rs and its test Kept: EvaluationType and StatusContext, which live reporter code still reads -- GenericSummary is constructed at helper.rs:66 and validate.rs:704. Display for GuardNamedRuleClause moved to exprs.rs beside the Display impls for its sibling clause types. It lived in evaluate.rs but eval.rs formats it, so it was the one piece of that file the live evaluator depended on. evaluate_tests.rs was also a near-duplicate suite: of five test names sampled, all five exist in eval_tests.rs against the live evaluator. The 39 tests dropped here (32 evaluate_tests, 4 path_value_tests, 2 values_tests, 1 appender) all exercised only the deleted engine; the same semantics stay covered by eval_context_tests for query retrieval, eval_tests::filter_based_* and test_map_keys_function for filters, and rule_test_type_blocks for type blocks. 358 -> 319 lib tests, 0 failed. 4693 lines deleted. --- guard/src/commands/aws_meta_appender.rs | 76 - guard/src/commands/aws_meta_appender_tests.rs | 86 - guard/src/commands/mod.rs | 1 - guard/src/rules/evaluate.rs | 1342 ---------- guard/src/rules/evaluate_tests.rs | 2345 ----------------- guard/src/rules/exprs.rs | 6 + guard/src/rules/mod.rs | 1 - guard/src/rules/path_value.rs | 351 --- guard/src/rules/path_value_tests.rs | 259 -- guard/src/rules/values_tests.rs | 234 +- 10 files changed, 8 insertions(+), 4693 deletions(-) delete mode 100644 guard/src/commands/aws_meta_appender.rs delete mode 100644 guard/src/commands/aws_meta_appender_tests.rs delete mode 100644 guard/src/rules/evaluate.rs delete mode 100644 guard/src/rules/evaluate_tests.rs diff --git a/guard/src/commands/aws_meta_appender.rs b/guard/src/commands/aws_meta_appender.rs deleted file mode 100644 index e72d3a0a1..000000000 --- a/guard/src/commands/aws_meta_appender.rs +++ /dev/null @@ -1,76 +0,0 @@ -use crate::rules::exprs::AccessQuery; -use crate::rules::path_value::{PathAwareValue, QueryResolver}; -use crate::rules::values::CmpOperator; -use crate::rules::{EvaluationContext, EvaluationType, Result, Status}; -use std::convert::TryFrom; - -pub(super) struct MetadataAppender<'d> { - pub(super) delegate: &'d dyn EvaluationContext, - pub(super) root_context: &'d PathAwareValue, -} - -impl<'d> EvaluationContext for MetadataAppender<'d> { - fn resolve_variable(&self, variable: &str) -> Result> { - self.delegate.resolve_variable(variable) - } - - fn rule_status(&self, rule_name: &str) -> Result { - self.delegate.rule_status(rule_name) - } - - #[allow(clippy::never_loop)] - fn end_evaluation( - &self, - eval_type: EvaluationType, - context: &str, - msg: String, - from: Option, - to: Option, - status: Option, - cmp: Option<(CmpOperator, bool)>, - ) { - let msg = if eval_type == EvaluationType::Clause { - match status { - Some(status) => loop { - if status == Status::FAIL { - if let Some(value) = &from { - let path = value.self_path(); - if path.0.starts_with("/Resources") { - let parts = path.0.splitn(4, '/').collect::>(); - if parts.len() == 4 { - let query = format!( - "Resources['{}'].Metadata[ keys == /^aws/ ]", - parts[2] - ); - let AccessQuery { - query, - match_all: all, - } = AccessQuery::try_from(query.as_str()).unwrap(); - if let Ok(selected) = - self.root_context.select(all, &query, self) - { - break format!("{}\nMetadata: {:?}", msg, selected); - } - } - } - } - } - break msg; - }, - None => msg, - } - } else { - msg - }; - self.delegate - .end_evaluation(eval_type, context, msg, from, to, status, cmp) - } - - fn start_evaluation(&self, eval_type: EvaluationType, context: &str) { - self.delegate.start_evaluation(eval_type, context) - } -} - -#[cfg(test)] -#[path = "aws_meta_appender_tests.rs"] -mod aws_meta_appender_tests; diff --git a/guard/src/commands/aws_meta_appender_tests.rs b/guard/src/commands/aws_meta_appender_tests.rs deleted file mode 100644 index 5e4649703..000000000 --- a/guard/src/commands/aws_meta_appender_tests.rs +++ /dev/null @@ -1,86 +0,0 @@ -use super::*; - -#[test] -fn append_cdk_metadata_test() -> Result<()> { - let resources = r#"{ - "Resources": { - "table1F1EAFA30": { - "Type": "AWS::DynamoDB::Table", - "Properties": { - "KeySchema": [ - { - "AttributeName": "table1", - "KeyType": "HASH" - } - ], - "AttributeDefinitions": [ - { - "AttributeName": "table1", - "AttributeType": "S" - } - ], - "ProvisionedThroughput": { - "ReadCapacityUnits": 5, - "WriteCapacityUnits": 5 - } - }, - "UpdateReplacePolicy": "Retain", - "DeletionPolicy": "Retain", - "Metadata": { - "aws:cdk:path": "FtCdkDynamoDBStack/table1/Resource" - } - } - } - }"#; - let root = PathAwareValue::try_from(resources)?; - let query = AccessQuery::try_from( - "Resources['table1F1EAFA30'].Properties.ProvisionedThroughput.ReadCapacityUnits", - )?; - struct Capture {} - impl EvaluationContext for Capture { - fn resolve_variable(&self, _: &str) -> Result> { - unimplemented!() - } - - fn rule_status(&self, _: &str) -> Result { - unimplemented!() - } - - fn end_evaluation( - &self, - _: EvaluationType, - _: &str, - msg: String, - _: Option, - _: Option, - _: Option, - _cmp: Option<(CmpOperator, bool)>, - ) { - assert_ne!(msg.as_str(), ""); - assert!(msg.starts_with("FIRST PART")); - assert!(msg.len() > "FIRST PART".len()); - println!("{}", msg); - } - - fn start_evaluation(&self, _: EvaluationType, _: &str) { - unimplemented!() - } - } - let capture = Capture {}; - let appender = MetadataAppender { - root_context: &root, - delegate: &capture, - }; - let value = root.select(true, &query.query, &appender)?[0]; - println!("{:?}", value); - appender.end_evaluation( - EvaluationType::Clause, - "Clause", - "FIRST PART".to_string(), - Some(value.clone()), - None, - Some(Status::FAIL), - None, - ); - Ok(()) -} diff --git a/guard/src/commands/mod.rs b/guard/src/commands/mod.rs index 3cb8dda5a..bae10680d 100644 --- a/guard/src/commands/mod.rs +++ b/guard/src/commands/mod.rs @@ -15,7 +15,6 @@ pub mod rulegen; pub mod test; pub mod validate; -mod aws_meta_appender; mod common_test_helpers; pub mod completions; pub mod reporters; diff --git a/guard/src/rules/evaluate.rs b/guard/src/rules/evaluate.rs deleted file mode 100644 index df45063a8..000000000 --- a/guard/src/rules/evaluate.rs +++ /dev/null @@ -1,1342 +0,0 @@ -use std::collections::HashMap; -use std::convert::TryFrom; -use std::fmt::Formatter; - -use crate::rules::errors::Error; -use crate::rules::exprs::{ - AccessQuery, Block, Conjunctions, GuardAccessClause, LetExpr, LetValue, Rule, RulesFile, - SliceDisplay, -}; -use crate::rules::exprs::{ - BlockGuardClause, GuardClause, GuardNamedRuleClause, QueryPart, RuleClause, TypeBlock, - WhenGuardClause, -}; -use crate::rules::path_value::{PathAwareValue, QueryResolver}; -use crate::rules::values::*; -use crate::rules::{Evaluate, EvaluationContext, EvaluationType, Result, Status}; - -////////////////////////////////////////////////////////////////////////////////////////////////// -// // -// Implementation for Guard Evaluations // -// // -////////////////////////////////////////////////////////////////////////////////////////////////// - -pub(super) fn resolve_variable_query<'s>( - all: bool, - variable: &str, - query: &[QueryPart<'_>], - var_resolver: &'s dyn EvaluationContext, -) -> Result> { - let retrieved = var_resolver.resolve_variable(variable)?; - let index: usize = if query.len() > 1 { - match &query[1] { - QueryPart::AllIndices(_) => 2, - _ => 1, - } - } else { - 1 - }; - let mut acc = Vec::with_capacity(retrieved.len()); - for each in retrieved { - if query.len() > index { - acc.extend(each.select(all, &query[index..], var_resolver)?) - } else { - acc.push(each); - } - } - Ok(acc) -} - -pub(super) fn resolve_query<'s>( - all: bool, - query: &[QueryPart<'_>], - context: &'s PathAwareValue, - var_resolver: &'s dyn EvaluationContext, -) -> Result> { - match query[0].variable() { - Some(var) => resolve_variable_query(all, var, query, var_resolver), - None => context.select(all, query, var_resolver), - } -} - -fn invert_status(status: Status, not: bool) -> Status { - if not { - return match status { - Status::FAIL => Status::PASS, - Status::PASS => Status::FAIL, - Status::SKIP => Status::SKIP, - }; - } - status -} - -fn negation_status(r: bool, clause_not: bool, not: bool) -> Status { - let status = if clause_not { !r } else { r }; - let status = if not { !status } else { status }; - if status { - Status::PASS - } else { - Status::FAIL - } -} - -#[allow(clippy::type_complexity)] -fn compare_loop_all( - lhs: &Vec<&PathAwareValue>, - rhs: &Vec<&PathAwareValue>, - compare: F, - any_one_rhs: bool, -) -> Result<( - bool, - Vec<(bool, Option, Option)>, -)> -where - F: Fn(&PathAwareValue, &PathAwareValue) -> Result, -{ - let mut lhs_cmp = true; - let mut results = Vec::with_capacity(lhs.len()); - 'lhs: for lhs_value in lhs { - let mut acc = Vec::with_capacity(lhs.len()); - for rhs_value in rhs { - let check = compare(lhs_value, rhs_value)?; - if check { - if any_one_rhs { - acc.clear(); - results.push((true, None, None)); - continue 'lhs; - } - acc.push((true, None, None)); - } else { - acc.push(( - false, - Some((*lhs_value).clone()), - Some((*rhs_value).clone()), - )); - if !any_one_rhs { - lhs_cmp = false; - } - } - } - if any_one_rhs { - lhs_cmp = false; - } - results.extend(acc) - } - Ok((lhs_cmp, results)) -} - -#[allow(clippy::never_loop, clippy::type_complexity)] -fn compare_loop( - lhs: &Vec<&PathAwareValue>, - rhs: &Vec<&PathAwareValue>, - compare: F, - any_one_rhs: bool, - atleast_one: bool, -) -> Result<( - bool, - Vec<(bool, Option, Option)>, -)> -where - F: Fn(&PathAwareValue, &PathAwareValue) -> Result, -{ - let (overall, results) = compare_loop_all(lhs, rhs, compare, any_one_rhs)?; - let overall = 'outer: loop { - if !overall { - for (each, _, _) in results.iter() { - if atleast_one { - if *each { - break 'outer true; - } - } else if !*each { - break 'outer false; - } - } - if atleast_one { - break 'outer false; - } else { - break 'outer true; - } - } else { - break true; - } - }; - Ok((overall, results)) -} - -fn elevate_inner<'a>( - list_of_list: &'a Vec<&PathAwareValue>, -) -> Result>> { - let mut elevated = Vec::with_capacity(list_of_list.len()); - for each_list_elem in list_of_list { - match *each_list_elem { - PathAwareValue::List((_path, list)) => { - let inner_lhs = list.iter().collect::>(); - elevated.push(inner_lhs); - } - - rest => elevated.push(vec![rest]), - } - } - Ok(elevated) -} - -fn is_mixed_values_results(incoming: &[&PathAwareValue]) -> bool { - let mut non_list_elem = false; - let mut list_elem_present = false; - for each in incoming { - match each { - PathAwareValue::List((_, _)) => { - list_elem_present = true; - continue; - } - - _ => { - non_list_elem = true; - continue; - } - } - } - non_list_elem && list_elem_present -} - -fn merge_mixed_results<'a>(incoming: &'a [&PathAwareValue]) -> Vec<&'a PathAwareValue> { - let mut merged = Vec::with_capacity(incoming.len()); - for each in incoming { - match each { - PathAwareValue::List((_, l)) => { - for inner in l { - merged.push(inner); - } - } - - rest => { - merged.push(rest); - } - } - } - merged -} - -#[allow(clippy::type_complexity)] -fn compare( - lhs: &[&PathAwareValue], - _lhs_query: &[QueryPart<'_>], - rhs: &[&PathAwareValue], - _rhs_query: Option<&[QueryPart<'_>]>, - compare: F, - any: bool, - atleast_one: bool, -) -> Result<( - Status, - Vec<(bool, Option, Option)>, -)> -where - F: Fn(&PathAwareValue, &PathAwareValue) -> Result, -{ - if lhs.is_empty() || rhs.is_empty() { - return Ok((Status::FAIL, vec![])); - } - - let lhs = if is_mixed_values_results(lhs) { - merge_mixed_results(lhs) - } else { - lhs.to_vec() - }; - let rhs = if is_mixed_values_results(rhs) { - merge_mixed_results(rhs) - } else { - rhs.to_vec() - }; - - let lhs_elem_has_list = lhs[0].is_list(); - let rhs_elem_has_list = rhs[0].is_list(); - - // - // What are possible comparisons - // - if !lhs_elem_has_list && !rhs_elem_has_list { - match compare_loop(&lhs, &rhs, compare, any, atleast_one) { - Ok((true, outcomes)) => Ok((Status::PASS, outcomes)), - Ok((false, outcomes)) => Ok((Status::FAIL, outcomes)), - Err(e) => Err(e), - } - } else if lhs_elem_has_list && !rhs_elem_has_list { - for elevated in elevate_inner(&lhs)? { - if let Ok((cmp, outcomes)) = - compare_loop(&elevated, &rhs, |f, s| compare(f, s), any, atleast_one) - { - if !cmp { - return Ok((Status::FAIL, outcomes)); - } - } - } - Ok((Status::PASS, vec![])) - } else if (!lhs_elem_has_list || any) && rhs_elem_has_list { - for elevated in elevate_inner(&rhs)? { - if let Ok((cmp, outcomes)) = - compare_loop(&lhs, &elevated, |f, s| compare(f, s), any, atleast_one) - { - if !cmp { - return Ok((Status::FAIL, outcomes)); - } - } - } - Ok((Status::PASS, vec![])) - } else { - match compare_loop(&lhs, &rhs, compare, any, atleast_one)? { - (true, _) => Ok((Status::PASS, vec![])), - (false, outcomes) => Ok((Status::FAIL, outcomes)), - } - } -} - -pub(super) fn invert_closure( - f: F, - clause_not: bool, - not: bool, -) -> impl Fn(&PathAwareValue, &PathAwareValue) -> Result -where - F: Fn(&PathAwareValue, &PathAwareValue) -> Result, -{ - move |first, second| { - let r = f(first, second)?; - let r = if clause_not { !r } else { r }; - let r = if not { !r } else { r }; - Ok(r) - } -} - -impl<'loc> Evaluate for GuardAccessClause<'loc> { - #[allow(clippy::never_loop)] - fn evaluate<'s>( - &self, - context: &'s PathAwareValue, - var_resolver: &'s dyn EvaluationContext, - ) -> Result { - //var_resolver.start_evaluation(EvaluationType::Clause, &guard_loc); - let clause = self; - - let all = self.access_clause.query.match_all; - - let (lhs, retrieve_error) = match resolve_query( - clause.access_clause.query.match_all, - &clause.access_clause.query.query, - context, - var_resolver, - ) { - Ok(v) => (Some(v), None), - Err(Error::RetrievalError(e)) | Err(Error::IncompatibleRetrievalError(e)) => { - (None, Some(e)) - } - Err(e) => return Err(e), - }; - - let result = match clause.access_clause.comparator { - (CmpOperator::Empty, not) => - // - // Retrieval Error is considered the same as an empty or !exists - // When using "SOME" keyword in the clause, then IncompatibleError is trapped to be none - // This is okay as long as the checks are for empty, exists - // - { - match &lhs { - None => Some(negation_status(true, not, clause.negation)), - Some(l) => Some(if !l.is_empty() { - if l[0].is_list() || l[0].is_map() { - 'all_empty: loop { - for element in l { - let status = match *element { - PathAwareValue::List((_, v)) => { - negation_status(v.is_empty(), not, clause.negation) - } - PathAwareValue::Map((_, m)) => { - negation_status(m.is_empty(), not, clause.negation) - } - _ => continue, - }; - - if status == Status::FAIL { - break 'all_empty Status::FAIL; - } - } - break Status::PASS; - } - } else { - negation_status(false, not, clause.negation) - } - } else { - negation_status(true, not, clause.negation) - }), - } - } - - (CmpOperator::Exists, not) => match &lhs { - None => Some(negation_status(false, not, clause.negation)), - Some(_) => Some(negation_status(true, not, clause.negation)), - }, - - (CmpOperator::Eq, not) => match &clause.access_clause.compare_with { - Some(LetValue::Value(PathAwareValue::Null(_))) => match &lhs { - None => Some(negation_status(true, not, clause.negation)), - Some(_) => Some(negation_status(false, not, clause.negation)), - }, - _ => None, - }, - - (CmpOperator::IsString, not) => match &lhs { - None => Some(negation_status(false, not, clause.negation)), - Some(l) => Some(negation_status( - l.iter() - .find(|p| !matches!(**p, PathAwareValue::String(_))) - .map_or(true, |_i| false), - not, - clause.negation, - )), - }, - - (CmpOperator::IsList, not) => match &lhs { - None => Some(negation_status(false, not, clause.negation)), - Some(l) => Some(negation_status( - l.iter() - .find(|p| !matches!(**p, PathAwareValue::List(_))) - .map_or(true, |_i| false), - not, - clause.negation, - )), - }, - - (CmpOperator::IsMap, not) => match &lhs { - None => Some(negation_status(false, not, clause.negation)), - Some(l) => Some(negation_status( - l.iter() - .find(|p| !matches!(**p, PathAwareValue::Map(_))) - .map_or(true, |_i| false), - not, - clause.negation, - )), - }, - - _ => None, - }; - - if let Some(r) = result { - let guard_loc = format!("{}", self); - let mut auto_reporter = - AutoReport::new(EvaluationType::Clause, var_resolver, &guard_loc); - let message = match &clause.access_clause.custom_message { - Some(msg) => msg, - None => "(DEFAULT: NO_MESSAGE)", - }; - auto_reporter - .cmp(self.access_clause.comparator) - .status(r) - .from(match &lhs { - None => Some(context.clone()), - Some(l) => { - if !l.is_empty() { - Some(l[0].clone()) - } else { - Some(context.clone()) - } - } - }); - if r == Status::FAIL { - auto_reporter.message(message.to_string()); - } - return Ok(r); - } - - let lhs = match lhs { - None => { - let guard_loc = format!("{}", self); - let mut auto_reporter = - AutoReport::new(EvaluationType::Clause, var_resolver, &guard_loc); - if all { - return Ok(auto_reporter - .status(Status::FAIL) - .message(retrieve_error.map_or("".to_string(), |e| e)) - .get_status()); - } else { - return Ok(auto_reporter - .status(Status::FAIL) - .message(retrieve_error.map_or("".to_string(), |e| e)) - .get_status()); - } - } - Some(l) => l, - }; - - let rhs_local = match &clause.access_clause.compare_with { - None => { - return Err(Error::IncompatibleRetrievalError(format!( - "Expecting a RHS for comparison and did not find one, clause@{}", - clause.access_clause.location - ))) - } - - Some(expr) => match expr { - LetValue::Value(v) => Some(vec![v]), - - _ => None, - }, - }; - - let (rhs_resolved, rhs_query) = if let Some(expr) = &clause.access_clause.compare_with { - match expr { - LetValue::AccessClause(query) => ( - Some(resolve_query( - query.match_all, - &query.query, - context, - var_resolver, - )?), - Some(query.query.as_slice()), - ), - _ => (None, None), - } - } else { - (None, None) - }; - - let rhs = match rhs_local { - Some(local) => local, - None => match rhs_resolved { - Some(resolved) => resolved, - None => unreachable!(), - }, - }; - - let (result, outcomes) = match &clause.access_clause.comparator.0 { - // - // ==, != - // - CmpOperator::Eq => compare( - &lhs, - &clause.access_clause.query.query, - &rhs, - rhs_query, - invert_closure( - super::path_value::compare_eq, - clause.access_clause.comparator.1, - clause.negation, - ), - false, - !all, - )?, - - // - // > - // - CmpOperator::Gt => compare( - &lhs, - &clause.access_clause.query.query, - &rhs, - rhs_query, - invert_closure( - super::path_value::compare_gt, - clause.access_clause.comparator.1, - clause.negation, - ), - false, - !all, - )?, - - // - // >= - // - CmpOperator::Ge => compare( - &lhs, - &clause.access_clause.query.query, - &rhs, - rhs_query, - invert_closure( - super::path_value::compare_ge, - clause.access_clause.comparator.1, - clause.negation, - ), - false, - !all, - )?, - - // - // < - // - CmpOperator::Lt => compare( - &lhs, - &clause.access_clause.query.query, - &rhs, - rhs_query, - invert_closure( - super::path_value::compare_lt, - clause.access_clause.comparator.1, - clause.negation, - ), - false, - !all, - )?, - - // - // <= - // - CmpOperator::Le => compare( - &lhs, - &clause.access_clause.query.query, - &rhs, - rhs_query, - invert_closure( - super::path_value::compare_le, - clause.access_clause.comparator.1, - clause.negation, - ), - false, - !all, - )?, - - // - // IN, !IN - // - CmpOperator::In => { - let mut result = if clause.access_clause.comparator.1 { - // - // ! IN operator - // - compare( - &lhs, - &clause.access_clause.query.query, - &rhs, - rhs_query, - |lhs, rhs| Ok(!super::path_value::compare_eq(lhs, rhs)?), - false, - !all, - )? - } else { - compare( - &lhs, - &clause.access_clause.query.query, - &rhs, - rhs_query, - super::path_value::compare_eq, - true, - !all, - )? - }; - result.0 = invert_status(result.0, clause.negation); - result - } - - _ => unreachable!(), - }; - - for (outcome, from, to) in outcomes { - let guard_loc = format!("{}", self); - let mut auto_reporter = - AutoReport::new(EvaluationType::Clause, var_resolver, &guard_loc); - auto_reporter.status(if outcome { Status::PASS } else { Status::FAIL }); - auto_reporter.cmp(clause.access_clause.comparator); - if !outcome { - auto_reporter.from(from).to(to).message( - match &clause.access_clause.custom_message { - Some(msg) => msg.clone(), - None => "DEFAULT MESSAGE(FAIL)".to_string(), - }, - ); - } - } - Ok(result) - } -} - -impl<'loc> std::fmt::Display for GuardNamedRuleClause<'loc> { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - write!(f, "Rule({}@{})", self.dependent_rule, self.location) - } -} - -impl<'loc> Evaluate for GuardNamedRuleClause<'loc> { - fn evaluate<'s>( - &self, - _context: &'s PathAwareValue, - var_resolver: &'s dyn EvaluationContext, - ) -> Result { - let guard_loc = format!("{}", self); - let mut auto_reporter = AutoReport::new(EvaluationType::Clause, var_resolver, &guard_loc); - let status = invert_status( - match var_resolver.rule_status(&self.dependent_rule)? { - Status::PASS => Status::PASS, - _ => Status::FAIL, - }, - self.negation, - ); - - Ok(if status == Status::FAIL { - let msg = if let Some(msg) = &self.custom_message { - msg - } else { - "DEFAULT FAIL" - }; - auto_reporter - .status(status) - .message(msg.to_string()) - .get_status() - } else { - auto_reporter.status(status).get_status() - }) - } -} - -impl<'loc> Evaluate for GuardClause<'loc> { - #[allow(clippy::never_loop)] - fn evaluate<'s>( - &self, - context: &'s PathAwareValue, - var_resolver: &'s dyn EvaluationContext, - ) -> Result { - match self { - GuardClause::Clause(gac) => gac.evaluate(context, var_resolver), - GuardClause::NamedRule(nr) => nr.evaluate(context, var_resolver), - GuardClause::BlockClause(bc) => bc.evaluate(context, var_resolver), - GuardClause::WhenBlock(conditions, clauses) => { - let status = loop { - let mut when_conditions = - AutoReport::new(EvaluationType::Condition, var_resolver, ""); - break when_conditions - .status(conditions.evaluate(context, var_resolver)?) - .get_status(); - }; - match status { - Status::PASS => { - let mut auto_block = - AutoReport::new(EvaluationType::ConditionBlock, var_resolver, ""); - Ok(auto_block - .status(clauses.evaluate(context, var_resolver)?) - .get_status()) - } - _ => { - let mut skip_block = - AutoReport::new(EvaluationType::ConditionBlock, var_resolver, ""); - Ok(skip_block.status(Status::SKIP).get_status()) - } - } - } - GuardClause::ParameterizedNamedRule(_) => unimplemented!(), - } - } -} - -impl<'loc, T: Evaluate + 'loc> Evaluate for Block<'loc, T> { - fn evaluate<'s>( - &self, - context: &'s PathAwareValue, - var_resolver: &'s dyn EvaluationContext, - ) -> Result { - let block = BlockScope::new(self, context, var_resolver)?; - self.conjunctions.evaluate(context, &block) - } -} - -impl<'loc, T: Evaluate + 'loc> Evaluate for Conjunctions { - #[allow(clippy::never_loop)] - fn evaluate<'s>( - &self, - context: &'s PathAwareValue, - var_resolver: &'s dyn EvaluationContext, - ) -> Result { - Ok(loop { - let mut num_passes = 0; - let mut num_fails = 0; - let item_name = std::any::type_name::(); - 'conjunction: for conjunction in self { - let mut num_of_disjunction_fails = 0; - let mut report = if "cfn_guard::rules::exprs::GuardClause" == item_name { - Some(AutoReport::new( - EvaluationType::Conjunction, - var_resolver, - item_name, - )) - } else { - None - }; - for disjunction in conjunction { - match disjunction.evaluate(context, var_resolver)? { - Status::PASS => { - let _ = report - .as_mut() - .map(|r| Some(r.status(Status::PASS).get_status())); - num_passes += 1; - continue 'conjunction; - } - Status::SKIP => {} - Status::FAIL => { - num_of_disjunction_fails += 1; - } - } - } - - if num_of_disjunction_fails > 0 { - let _ = report - .as_mut() - .map(|r| Some(r.status(Status::FAIL).get_status())); - num_fails += 1; - continue; - //break 'outer Status::FAIL - } - } - if num_fails > 0 { - break Status::FAIL; - } - if num_passes > 0 { - break Status::PASS; - } - break Status::SKIP; - }) - } -} - -impl<'loc> Evaluate for BlockGuardClause<'loc> { - #[allow(clippy::never_loop)] - fn evaluate<'s>( - &self, - context: &'s PathAwareValue, - var_resolver: &'s dyn EvaluationContext, - ) -> Result { - let blk_context = format!("Block[{}]", self.location); - let mut report = AutoReport::new(EvaluationType::BlockClause, var_resolver, &blk_context); - let all = self.query.match_all; - let block_values = match resolve_query(all, &self.query.query, context, var_resolver) { - Err(Error::RetrievalError(e)) | Err(Error::IncompatibleRetrievalError(e)) => { - return Ok(report.message(e).status(Status::FAIL).get_status()) - } - - Ok(v) => { - if v.is_empty() { - // one or more - return Ok(report - .from(Some(context.clone())) - .message(format!( - "Query {} returned no results", - SliceDisplay(&self.query.query) - )) - .status(Status::FAIL) - .get_status()); - } else { - v - } - } - - Err(e) => return Err(e), - }; - - Ok(report - .status(loop { - let mut num_fail = 0; - let mut num_pass = 0; - for each in block_values { - match self.block.evaluate(each, var_resolver)? { - Status::FAIL => { - num_fail += 1; - } - Status::SKIP => {} - Status::PASS => { - num_pass += 1; - } - } - } - - if all { - if num_fail > 0 { - break Status::FAIL; - } - if num_pass > 0 { - break Status::PASS; - } - break Status::SKIP; - } else { - if num_pass > 0 { - break Status::PASS; - } - if num_fail > 0 { - break Status::FAIL; - } - break Status::SKIP; - } - }) - .get_status()) - } -} - -impl<'loc> Evaluate for WhenGuardClause<'loc> { - fn evaluate<'s>( - &self, - context: &'s PathAwareValue, - var_resolver: &'s dyn EvaluationContext, - ) -> Result { - match self { - WhenGuardClause::Clause(gac) => gac.evaluate(context, var_resolver), - WhenGuardClause::NamedRule(nr) => nr.evaluate(context, var_resolver), - WhenGuardClause::ParameterizedNamedRule(_) => todo!(), - } - } -} - -impl<'loc> Evaluate for TypeBlock<'loc> { - #[allow(clippy::never_loop)] - fn evaluate<'s>( - &self, - context: &'s PathAwareValue, - var_resolver: &'s dyn EvaluationContext, - ) -> Result { - let mut type_report = AutoReport::new(EvaluationType::Type, var_resolver, &self.type_name); - - if let Some(conditions) = &self.conditions { - let mut type_conds = AutoReport::new(EvaluationType::Condition, var_resolver, ""); - match type_conds - .status(conditions.evaluate(context, var_resolver)?) - .get_status() - { - Status::PASS => {} - _ => return Ok(type_report.status(Status::SKIP).get_status()), - } - } - - let query = format!("Resources.*[ Type == \"{}\" ]", self.type_name); - let cfn_query = AccessQuery::try_from(query.as_str())?; - let values = match context.select(cfn_query.match_all, &cfn_query.query, var_resolver) { - Ok(v) => { - if v.is_empty() { - return Ok(type_report - .message(format!( - "There are no {} types present in context", - self.type_name - )) - .status(Status::SKIP) - .get_status()); - } else { - v - } - } - Err(_) => vec![context], - }; - - let overall = loop { - let mut num_fail = 0; - let mut num_pass = 0; - for (index, each) in values.iter().enumerate() { - let type_context = format!("{}#{}({})", self.type_name, index, (*each).self_path()); - let mut each_type_report = - AutoReport::new(EvaluationType::Type, var_resolver, &type_context); - match each_type_report - .status(self.block.evaluate(each, var_resolver)?) - .get_status() - { - Status::PASS => { - num_pass += 1; - } - Status::FAIL => { - num_fail += 1; - } - Status::SKIP => {} - } - } - if num_fail > 0 { - break Status::FAIL; - } - if num_pass > 0 { - break Status::PASS; - } - break Status::SKIP; - }; - Ok(match overall { - Status::SKIP => type_report - .status(Status::SKIP) - .message(format!( - "ALL Clauses for all types {} was SKIPPED. This can be an error", - self.type_name - )) - .get_status(), - rest => type_report.status(rest).get_status(), - }) - } -} - -impl<'loc> Evaluate for RuleClause<'loc> { - fn evaluate<'s>( - &self, - context: &'s PathAwareValue, - var_resolver: &'s dyn EvaluationContext, - ) -> Result { - Ok(match self { - RuleClause::Clause(gc) => gc.evaluate(context, var_resolver)?, - RuleClause::TypeBlock(tb) => tb.evaluate(context, var_resolver)?, - RuleClause::WhenBlock(conditions, block) => { - let status = { - let mut auto_cond = - AutoReport::new(EvaluationType::Condition, var_resolver, ""); - auto_cond - .status(conditions.evaluate(context, var_resolver)?) - .get_status() - }; - - match status { - Status::PASS => { - let mut auto_block = - AutoReport::new(EvaluationType::ConditionBlock, var_resolver, ""); - auto_block - .status(block.evaluate(context, var_resolver)?) - .get_status() - } - _ => { - let mut skip_block = - AutoReport::new(EvaluationType::ConditionBlock, var_resolver, ""); - skip_block.status(Status::SKIP).get_status() - } - } - } - }) - } -} - -impl<'loc> Evaluate for Rule<'loc> { - fn evaluate<'s>( - &self, - context: &'s PathAwareValue, - var_resolver: &'s dyn EvaluationContext, - ) -> Result { - let mut auto = AutoReport::new(EvaluationType::Rule, var_resolver, &self.rule_name); - if let Some(conds) = &self.conditions { - let mut cond = - AutoReport::new(EvaluationType::Condition, var_resolver, &self.rule_name); - match cond - .status(conds.evaluate(context, var_resolver)?) - .get_status() - { - Status::PASS => {} - _ => return Ok(auto.status(Status::SKIP).get_status()), - } - } - Ok(auto - .status(self.block.evaluate(context, var_resolver)?) - .get_status()) - } -} - -impl<'loc> Evaluate for RulesFile<'loc> { - fn evaluate<'s>( - &self, - context: &'s PathAwareValue, - var_resolver: &'s dyn EvaluationContext, - ) -> Result { - let mut overall = Status::PASS; - let mut auto_report = AutoReport::new(EvaluationType::File, var_resolver, ""); - for rule in &self.guard_rules { - if Status::FAIL == rule.evaluate(context, var_resolver)? { - overall = Status::FAIL - } - } - auto_report.status(overall); - Ok(overall) - } -} - -////////////////////////////////////////////////////////////////////////////////////////////////// -// // -// Evaluation Context implementations for scoped variables // -// // -////////////////////////////////////////////////////////////////////////////////////////////////// - -fn extract_variables<'s, 'loc>( - expressions: &'s Vec>, - vars: &mut HashMap<&'s str, &'s PathAwareValue>, - queries: &mut HashMap<&'s str, &'s AccessQuery<'loc>>, -) -> Result<()> { - for each in expressions { - match &each.value { - LetValue::Value(v) => { - vars.insert(&each.var, v); - } - - LetValue::AccessClause(query) => { - queries.insert(&each.var, query); - } - LetValue::FunctionCall(_) => {} - } - } - Ok(()) -} - -#[derive(Debug)] -#[deprecated] -#[allow(dead_code)] -pub(crate) struct RootScope<'s, 'loc> { - rules: &'s RulesFile<'loc>, - input_context: &'s PathAwareValue, - pending_queries: HashMap<&'s str, &'s AccessQuery<'loc>>, - variables: std::cell::RefCell>>, - literals: HashMap<&'s str, &'s PathAwareValue>, - rule_by_name: HashMap<&'s str, &'s Rule<'loc>>, - rule_statues: std::cell::RefCell>, -} - -#[cfg(test)] -impl<'s, 'loc> RootScope<'s, 'loc> { - pub(crate) fn new(rules: &'s RulesFile<'loc>, value: &'s PathAwareValue) -> Result { - let mut literals = HashMap::new(); - let mut pending = HashMap::new(); - extract_variables(&rules.assignments, &mut literals, &mut pending)?; - let mut lookup_cache = HashMap::with_capacity(rules.guard_rules.len()); - for rule in &rules.guard_rules { - lookup_cache.insert(rule.rule_name.as_str(), rule); - } - - Ok(RootScope { - rules, - input_context: value, - pending_queries: pending, - literals, - variables: std::cell::RefCell::new(HashMap::new()), - rule_by_name: lookup_cache, - rule_statues: std::cell::RefCell::new(HashMap::with_capacity(rules.guard_rules.len())), - }) - } -} - -impl<'s, 'loc> EvaluationContext for RootScope<'s, 'loc> { - fn resolve_variable(&self, variable: &str) -> Result> { - if let Some(literal) = self.literals.get(variable) { - return Ok(vec![literal]); - } - - if let Some(value) = self.variables.borrow().get(variable) { - return Ok(value.clone()); - } - return if let Some((key, query)) = self.pending_queries.get_key_value(variable) { - let all = query.match_all; - let query = &query.query; - let values = match query[0].variable() { - Some(var) => resolve_variable_query(all, var, query, self)?, - None => { - let values = self.input_context.select(all, query, self)?; - self.variables.borrow_mut().insert(*key, values.clone()); - values - } - }; - Ok(values) - } else { - Err(Error::MissingVariable(format!( - "Could not resolve variable {}", - variable - ))) - }; - } - - fn rule_status(&self, rule_name: &str) -> Result { - if let Some(status) = self.rule_statues.borrow().get(rule_name) { - return Ok(*status); - } - - if let Some((name, rule)) = self.rule_by_name.get_key_value(rule_name) { - let status = (*rule).evaluate(self.input_context, self)?; - self.rule_statues.borrow_mut().insert(*name, status); - return Ok(status); - } - - Err(Error::MissingValue(format!( - "Attempting to resolve rule_status for rule = {}, rule not found", - rule_name - ))) - } - - fn end_evaluation( - &self, - eval_type: EvaluationType, - context: &str, - _msg: String, - _from: Option, - _to: Option, - status: Option, - _cmp: Option<(CmpOperator, bool)>, - ) { - if EvaluationType::Rule == eval_type { - let (name, _rule) = self.rule_by_name.get_key_value(context).unwrap(); - if let Some(status) = status { - self.rule_statues.borrow_mut().insert(*name, status); - } - } - } - - fn start_evaluation(&self, _eval_type: EvaluationType, _context: &str) {} -} - -#[allow(dead_code)] -pub(crate) struct BlockScope<'s, T> { - block_type: &'s Block<'s, T>, - input_context: &'s PathAwareValue, - pending_queries: HashMap<&'s str, &'s AccessQuery<'s>>, - literals: HashMap<&'s str, &'s PathAwareValue>, - variables: std::cell::RefCell>>, - parent: &'s dyn EvaluationContext, -} - -impl<'s, T> BlockScope<'s, T> { - pub(crate) fn new( - block_type: &'s Block<'s, T>, - context: &'s PathAwareValue, - parent: &'s dyn EvaluationContext, - ) -> Result { - let mut literals = HashMap::new(); - let mut pending = HashMap::new(); - extract_variables(&block_type.assignments, &mut literals, &mut pending)?; - Ok(BlockScope { - block_type, - input_context: context, - literals, - parent, - variables: std::cell::RefCell::new(HashMap::new()), - pending_queries: pending, - }) - } -} - -impl<'s, T> EvaluationContext for BlockScope<'s, T> { - fn resolve_variable(&self, variable: &str) -> Result> { - if let Some(literal) = self.literals.get(variable) { - return Ok(vec![literal]); - } - - if let Some(value) = self.variables.borrow().get(variable) { - return Ok(value.clone()); - } - return if let Some((key, query)) = self.pending_queries.get_key_value(variable) { - let all = query.match_all; - let query = &query.query; - let values = match query[0].variable() { - Some(var) => resolve_variable_query(all, var, query, self)?, - None => { - let values = self.input_context.select(all, query, self)?; - self.variables.borrow_mut().insert(*key, values.clone()); - values - } - }; - Ok(values) - } else { - self.parent.resolve_variable(variable) - }; - } - - fn rule_status(&self, rule_name: &str) -> Result { - self.parent.rule_status(rule_name) - } - - fn end_evaluation( - &self, - eval_type: EvaluationType, - context: &str, - msg: String, - from: Option, - to: Option, - status: Option, - cmp: Option<(CmpOperator, bool)>, - ) { - self.parent - .end_evaluation(eval_type, context, msg, from, to, status, cmp) - } - - fn start_evaluation(&self, eval_type: EvaluationType, context: &str) { - self.parent.start_evaluation(eval_type, context); - } -} - -#[derive(Clone)] -pub(super) struct AutoReport<'s> { - context: &'s dyn EvaluationContext, - type_context: &'s str, - eval_type: EvaluationType, - status: Option, - from: Option, - to: Option, - cmp: Option<(CmpOperator, bool)>, - message: Option, -} - -impl<'s> std::fmt::Debug for AutoReport<'s> { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - f.write_fmt(format_args!( - "Context = {}, Type = {}, Status = {:?}", - self.type_context, self.eval_type, self.status - ))?; - Ok(()) - } -} - -impl<'s> AutoReport<'s> { - pub(super) fn new( - eval_type: EvaluationType, - context: &'s dyn EvaluationContext, - type_context: &'s str, - ) -> Self { - context.start_evaluation(eval_type, type_context); - AutoReport { - eval_type, - type_context, - context, - status: None, - from: None, - to: None, - cmp: None, - message: None, - } - } - - pub(super) fn status(&mut self, status: Status) -> &mut Self { - self.status = Some(status); - self - } - - pub(super) fn from(&mut self, from: Option) -> &mut Self { - self.from = from; - self - } - - pub(super) fn to(&mut self, to: Option) -> &mut Self { - self.to = to; - self - } - - pub(super) fn cmp(&mut self, cmp: (CmpOperator, bool)) -> &mut Self { - self.cmp = Some(cmp); - self - } - - pub(super) fn message(&mut self, msg: String) -> &mut Self { - self.message = Some(msg); - self - } - - pub(super) fn get_status(&self) -> Status { - self.status.unwrap() - } -} - -impl<'s> Drop for AutoReport<'s> { - fn drop(&mut self) { - let status = match self.status { - Some(status) => status, - None => Status::SKIP, - }; - self.context.end_evaluation( - self.eval_type, - self.type_context, - match &self.message { - Some(message) => message.clone(), - None => format!("DEFAULT MESSAGE({})", status), - }, - self.from.clone(), - self.to.clone(), - Some(status), - self.cmp, - ) - } -} - -#[cfg(test)] -#[path = "evaluate_tests.rs"] -mod evaluate_tests; diff --git a/guard/src/rules/evaluate_tests.rs b/guard/src/rules/evaluate_tests.rs deleted file mode 100644 index b79e62c37..000000000 --- a/guard/src/rules/evaluate_tests.rs +++ /dev/null @@ -1,2345 +0,0 @@ -use super::super::path_value; -use super::super::path_value::Path; -use super::*; -use crate::commands::files::read_file_content; -use crate::rules::parser::{rules_file, Span}; -use pretty_assertions::assert_eq; -use std::convert::TryFrom; -use std::fs::File; - -const RULES_FILES_EXAMPLE: &str = r#" -rule iam_role_exists { - Resources.*[ Type == "AWS::IAM::Role" ] EXISTS -} - -rule iam_role_lambda_compliance when iam_role_exists { - let roles = Resources.*[ Type == "AWS::IAM::Role" ] - let select_lambda_service = %roles.Properties.AssumeRolePolicyDocument.Statement[ Principal.Service EXISTS - Principal.Service.* == /^lambda/ ] - - %select_lambda_service EMPTY or - %select_lambda_service.Action.* == /sts:AssumeRole/ -} -"#; - -fn parse_rules<'c>(rules: &'c str, name: &'c str) -> Result> { - let span = Span::new_extra(rules, name); - Ok(rules_file(span)?.unwrap()) -} - -fn read_data(file: File) -> Result { - let context = read_file_content(file)?; - match serde_json::from_str::(&context) { - Ok(value) => Value::try_from(value), - Err(_) => { - let value = serde_yaml::from_str::(&context)?; - Value::try_from(value) - } - } -} - -#[test] -fn guard_access_clause_test_all_up() -> Result<()> { - let _rules = parse_rules(RULES_FILES_EXAMPLE, "iam-rules.gr")?; - let _root = read_data(File::open("assets/cfn-lambda.yaml")?)?; - Ok(()) -} - -struct DummyEval {} -impl EvaluationContext for DummyEval { - fn resolve_variable(&self, _variable: &str) -> Result> { - unimplemented!() - } - - fn rule_status(&self, _rule_name: &str) -> Result { - unimplemented!() - } - - fn end_evaluation( - &self, - _eval_type: EvaluationType, - _context: &str, - _msg: String, - _from: Option, - _to: Option, - _status: Option, - _cmp: Option<(CmpOperator, bool)>, - ) { - } - - fn start_evaluation(&self, _eval_type: EvaluationType, _context: &str) {} -} - -#[test] -fn guard_access_clause_tests() -> Result<()> { - let dummy = DummyEval {}; - let root = read_data(File::open("assets/cfn-lambda.yaml")?)?; - let root = PathAwareValue::try_from(root)?; - let clause = GuardClause::try_from( - r#"Resources.*[ Type == "AWS::IAM::Role" ].Properties.AssumeRolePolicyDocument.Statement[ - Principal.Service EXISTS - Principal.Service == /^lambda/ ].Action == "sts:AssumeRole""#, - )?; - let status = clause.evaluate(&root, &dummy)?; - println!("Status = {:?}", status); - assert_eq!(Status::PASS, status); - - let clause = GuardClause::try_from( - r#"Resources.*[ Type == "AWS::IAM::Role" ].Properties.AssumeRolePolicyDocument.Statement[ - Principal.Service EXISTS - Principal.Service == /^notexists/ ].Action == "sts:AssumeRole""#, - )?; - - assert!(matches!(clause.evaluate(&root, &dummy)?, Status::FAIL)); - - Ok(()) -} - -#[test] -fn rule_clause_tests() -> Result<()> { - let dummy = DummyEval {}; - let r = r###" - rule check_all_resources_have_tags_present { - let all_resources = Resources.*.Properties - - %all_resources.Tags EXISTS - %all_resources.Tags !EMPTY -} - "###; - let rule = Rule::try_from(r)?; - - let v = r#" - { - "Resources": { - "vpc": { - "Type": "AWS::EC2::VPC", - "Properties": { - "CidrBlock": "10.0.0.0/25", - "Tags": [ - { - "Key": "my-vpc", - "Value": "my-vpc" - } - ] - } - } - } - } - "#; - - let value = Value::try_from(v)?; - let value = PathAwareValue::try_from(value)?; - let status = rule.evaluate(&value, &dummy)?; - assert_eq!(Status::PASS, status); - - let r = r" - rule iam_basic_checks { - AWS::IAM::Role { - Properties.AssumeRolePolicyDocument.Version == /(\d{4})-(\d{2})-(\d{2})/ - Properties.PermissionsBoundary == /arn:aws:iam::(\d{12}):policy/ - Properties.Tags[*].Value == /[a-zA-Z0-9]+/ - Properties.Tags[*].Key == /[a-zA-Z0-9]+/ - } -}"; - let _rule = Rule::try_from(r)?; - Ok(()) -} - -struct Reporter<'r>(&'r dyn EvaluationContext); -impl<'r> EvaluationContext for Reporter<'r> { - fn resolve_variable(&self, variable: &str) -> Result> { - self.0.resolve_variable(variable) - } - - fn rule_status(&self, rule_name: &str) -> Result { - self.0.rule_status(rule_name) - } - - fn end_evaluation( - &self, - eval_type: EvaluationType, - context: &str, - msg: String, - from: Option, - to: Option, - status: Option, - cmp: Option<(CmpOperator, bool)>, - ) { - println!("{} {} {:?}", eval_type, context, status); - self.0 - .end_evaluation(eval_type, context, msg, from, to, status, cmp) - } - - fn start_evaluation(&self, eval_type: EvaluationType, context: &str) { - println!("{} {}", eval_type, context); - } -} - -#[test] -fn rules_file_tests() -> Result<()> { - let file = r#" -let iam_resources = Resources.*[ Type == "AWS::IAM::Role" ] -rule iam_resources_exists { - %iam_resources !EMPTY -} - -rule iam_basic_checks when iam_resources_exists { - %iam_resources.Properties.AssumeRolePolicyDocument.Version == /(\d{4})-(\d{2})-(\d{2})/ - %iam_resources.Properties.PermissionsBoundary == /arn:aws:iam::(\d{12}):policy/ - when %iam_resources.Properties.Tags EXISTS - %iam_resources.Properties.Tags !EMPTY { - - %iam_resources.Properties.Tags.Value == /[a-zA-Z0-9]+/ - %iam_resources.Properties.Tags.Key == /[a-zA-Z0-9]+/ - } -}"#; - - let value = r#" - { - "Resources": { - "iamrole": { - "Type": "AWS::IAM::Role", - "Properties": { - "PermissionsBoundary": "arn:aws:iam::123456789012:policy/permboundary", - "AssumeRolePolicyDocument": { - "Version": "2021-01-10", - "Statement": { - "Effect": "Allow", - "Principal": "*", - "Action": "*", - "Resource": "*" - } - } - } - } - } - } - "#; - - let root = Value::try_from(value)?; - let root = PathAwareValue::try_from(root)?; - let rules_file = RulesFile::try_from(file)?; - let root_context = RootScope::new(&rules_file, &root)?; - let reporter = Reporter(&root_context); - let status = rules_file.evaluate(&root, &reporter)?; - assert_eq!(Status::PASS, status); - Ok(()) -} - -#[test] -fn rules_not_in_tests() -> Result<()> { - let clause = "Resources.*.Type NOT IN [/AWS::IAM/, /AWS::S3/]"; - let parsed = GuardClause::try_from(clause)?; - let value = "{ Resources: { iam: { Type: 'AWS::IAM::Role' } } }"; - let parsed_value = PathAwareValue::try_from(value)?; - let dummy = DummyEval {}; - let status = parsed.evaluate(&parsed_value, &dummy)?; - assert_eq!(status, Status::FAIL); - Ok(()) -} - -const SAMPLE: &str = r#" - { - "Statement": [ - { - "Sid": "PrincipalPutObjectIfIpAddress", - "Effect": "Allow", - "Action": "s3:PutObject", - "Resource": "arn:aws:s3:::my-service-bucket/*", - "Condition": { - "Bool": {"aws:ViaAWSService": "false"} - } - }, - { - "Sid": "ServicePutObject", - "Effect": "Allow", - "Action": "s3:PutObject", - "Resource": "arn:aws:s3:::my-service-bucket/*", - "Condition": { - "Bool": {"aws:ViaAWSService": "true"} - } - } - ] - } - "#; - -#[test] -fn test_iam_statement_clauses() -> Result<()> { - let sample = r#" - { - "Statement": [ - { - "Sid": "PrincipalPutObjectIfIpAddress", - "Effect": "Allow", - "Action": "s3:PutObject", - "Resource": "arn:aws:s3:::my-service-bucket/*", - "Condition": { - "Bool": {"aws:ViaAWSService": "false"}, - "StringEquals": {"aws:SourceVpc": "vpc-12243sc"} - } - }, - { - "Sid": "ServicePutObject", - "Effect": "Allow", - "Action": "s3:PutObject", - "Resource": "arn:aws:s3:::my-service-bucket/*", - "Condition": { - "Bool": {"aws:ViaAWSService": "true"} - } - } - ] - } - "#; - let value = Value::try_from(sample)?; - let value = PathAwareValue::try_from(value)?; - - let dummy = DummyEval {}; - let reporter = Reporter(&dummy); - - let clause = "Statement[ Condition EXISTS ].Condition.*[ KEYS == /aws:[sS]ource(Vpc|VPC|Vpce|VPCE)/ ] NOT EMPTY"; - // let clause = "Condition.*[ KEYS == /aws:[sS]ource(Vpc|VPC|Vpce|VPCE)/ ]"; - let parsed = GuardClause::try_from(clause)?; - let status = parsed.evaluate(&value, &reporter)?; - println!("Status {:?}", status); - assert_eq!(Status::PASS, status); - - let clause = r#"Statement[ Condition EXISTS - Condition.*[ KEYS == /aws:[sS]ource(Vpc|VPC|Vpce|VPCE)/ ] !EMPTY ] NOT EMPTY - "#; - let parsed = GuardClause::try_from(clause)?; - let status = parsed.evaluate(&value, &reporter)?; - println!("Status {:?}", status); - assert_eq!(Status::PASS, status); - - let value = Value::try_from(SAMPLE)?; - let value = PathAwareValue::try_from(value)?; - let parsed = GuardClause::try_from(clause)?; - let status = parsed.evaluate(&value, &reporter)?; - println!("Status {:?}", status); - assert_eq!(Status::FAIL, status); - - Ok(()) -} - -#[test] -fn test_api_gateway() -> Result<()> { - let rule = r#" -rule check_rest_api_private { - AWS::ApiGateway::RestApi { - # Endpoint configuration must only be private - Properties.EndpointConfiguration == ["PRIVATE"] - - # At least one statement in the resource policy must contain a condition with the key of "aws:sourceVpc" or "aws:sourceVpce" - Properties.Policy.Statement[ Condition.*[ KEYS == /aws:[sS]ource(Vpc|VPC|Vpce|VPCE)/ ] !EMPTY ] !EMPTY - } -} - "#; - - let rule = Rule::try_from(rule)?; - - let resources = r#" - { - "Resources": { - "apigatewayapi": { - "Type": "AWS::ApiGateway::RestApi", - "Properties": { - "Policy": { - "Version": "2012-10-17", - "Statement": [ - { - "Sid": "PrincipalPutObjectIfIpAddress", - "Effect": "Allow", - "Action": "s3:PutObject", - "Resource": "arn:aws:s3:::my-service-bucket/*", - "Condition": { - "Bool": {"aws:ViaAWSService": "false"}, - "StringEquals": {"aws:SourceVpc": "vpc-12243sc"} - } - }, - { - "Sid": "ServicePutObject", - "Effect": "Allow", - "Action": "s3:PutObject", - "Resource": "arn:aws:s3:::my-service-bucket/*", - "Condition": { - "Bool": {"aws:ViaAWSService": "true"} - } - } - ] - }, - "EndpointConfiguration": ["PRIVATE"] - } - } - } - }"#; - - let value = Value::try_from(resources)?; - let value = PathAwareValue::try_from(value)?; - let dummy = DummyEval {}; - let reporter = Reporter(&dummy); - let status = rule.evaluate(&value, &reporter)?; - println!("{}", status); - Ok(()) -} - -#[test] -fn testing_iam_role_prov_serve() -> Result<()> { - let resources = r#" - { - "Resources": { - "CounterTaskDefExecutionRole5959CB2D": { - "Type": "AWS::IAM::Role", - "Properties": { - "AssumeRolePolicyDocument": { - "Statement": [ - { - "Action": "sts:AssumeRole", - "Effect": "Allow", - "Principal": { - "Service": "ecs-tasks.amazonaws.com" - } - }], - "Version": "2012-10-17" - }, - "PermissionBoundary": {"Fn::Sub" : "arn::aws::iam::${AWS::AccountId}:policy/my-permission-boundary"}, - "Tags": [{ "Key": "TestRole", "Value": ""}] - }, - "Metadata": { - "aws:cdk:path": "foo/Counter/TaskDef/ExecutionRole/Resource" - } - } - } - } - "#; - - let rules = r#" -let iam_roles = Resources.*[ Type == "AWS::IAM::Role" ] -let ecs_tasks = Resources.*[ Type == "AWS::ECS::TaskDefinition" ] - -rule deny_permissions_boundary_iam_role when %iam_roles !EMPTY { - # atleast one Tags contains a Key "TestRole" - %iam_roles.Properties.Tags[ Key == "TestRole" ] NOT EMPTY - %iam_roles.Properties.PermissionBoundary !EXISTS -} - -rule deny_task_role_no_permission_boundary when %ecs_tasks !EMPTY { - let task_role = %ecs_tasks.Properties.TaskRoleArn - - when %task_role.'Fn::GetAtt' EXISTS { - let role_name = %task_role.'Fn::GetAtt'[0] - let iam_roles_by_name = Resources.*[ KEYS == %role_name ] - %iam_roles_by_name !EMPTY - iam_roles_by_name.Properties.Tags !EMPTY - } or - %task_role == /aws:arn/ # either a direct string or -} - "#; - - let rules_file = RulesFile::try_from(rules)?; - let value = PathAwareValue::try_from(resources)?; - - // let dummy = DummyEval{}; - let root_context = RootScope::new(&rules_file, &value)?; - let reporter = Reporter(&root_context); - let status = rules_file.evaluate(&value, &reporter)?; - println!("{}", status); - Ok(()) -} - -#[test] -fn testing_sg_rules_pro_serve() -> Result<()> { - let sgs = r#" - [{ - "Resources": { - "CounterServiceSecurityGroupF41A3908": { - "Type": "AWS::EC2::SecurityGroup", - "Properties": { - "GroupDescription": "foo/Counter/Service/SecurityGroup", - "SecurityGroupEgress": [ - { - "CidrIp": "0.0.0.0/0", - "Description": "Allow all outbound traffic by default", - "IpProtocol": "-1" - } - ], - "VpcId": { - "Ref": "Vpc8378EB38" - } - }, - "Metadata": { - "aws:cdk:path": "foo/Counter/Service/SecurityGroup/Resource" - } - } - } -}, - { - "Resources": { - "CounterServiceSecurityGroupF41A3908": { - "Type": "AWS::EC2::SecurityGroup", - "Properties": { - "GroupDescription": "foo/Counter/Service/SecurityGroup", - "SecurityGroupEgress": [ - { - "CidrIpv6": "::/0", - "Description": "Allow all outbound traffic by default", - "IpProtocol": "-1" - } - ], - "VpcId": { - "Ref": "Vpc8378EB38" - } - }, - "Metadata": { - "aws:cdk:path": "foo/Counter/Service/SecurityGroup/Resource" - } - } - } -}, { - "Resources": { - "CounterServiceSecurityGroupF41A3908": { - "Type": "AWS::EC2::SecurityGroup", - "Properties": { - "GroupDescription": "foo/Counter/Service/SecurityGroup", - "SecurityGroupEgress": [ - { - "CidrIp": "10.0.0.0/16", - "Description": "", - "IpProtocol": "-1" - } - ], - "VpcId": { - "Ref": "Vpc8378EB38" - } - }, - "Metadata": { - "aws:cdk:path": "foo/Counter/Service/SecurityGroup/Resource" - } - } - } -}, -{ "Resources": { - "CounterServiceSecurityGroupF41A3908": { - "Type": "AWS::EC2::SecurityGroup", - "Properties": { - "GroupDescription": "foo/Counter/Service/SecurityGroup", - "VpcId": { - "Ref": "Vpc8378EB38" - } - }, - "Metadata": { - "aws:cdk:path": "foo/Counter/Service/SecurityGroup/Resource" - } - } - } -}] - - "#; - - let rules = r#" -let sgs = Resources.*[ Type == "AWS::EC2::SecurityGroup" ] - -rule deny_egress when %sgs NOT EMPTY { - # Ensure that none of the security group contain a rule - # that has Cidr Ip set to any - %sgs.Properties.SecurityGroupEgress[ CidrIp == "0.0.0.0/0" or - CidrIpv6 == "::/0" ] EMPTY -} - - "#; - - let rules_file = RulesFile::try_from(rules)?; - - let values = PathAwareValue::try_from(sgs)?; - let samples = match values { - PathAwareValue::List((_p, v)) => v, - _ => unreachable!(), - }; - - for (index, each) in samples.iter().enumerate() { - let root_context = RootScope::new(&rules_file, each)?; - let reporter = Reporter(&root_context); - let status = rules_file.evaluate(each, &reporter)?; - println!("Status {} = {}", index, status); - } - - let sample = r#"{ "Resources": {} }"#; - let value = PathAwareValue::try_from(sample)?; - let rule = r#" -rule deny_egress { - # Ensure that none of the security group contain a rule - # that has Cidr Ip set to any - Resources.*[ Type == "AWS::EC2::SecurityGroup" ] - .Properties.SecurityGroupEgress[ CidrIp == "0.0.0.0/0" or - CidrIpv6 == "::/0" ] EMPTY -} - "#; - - let dummy = DummyEval {}; - let rule_parsed = Rule::try_from(rule)?; - let status = rule_parsed.evaluate(&value, &dummy)?; - println!("Status {:?}", status); - - Ok(()) -} - -#[test] -fn test_s3_bucket_pro_serv() -> Result<()> { - let values = r#" - [ -{ - "Resources": { - "S3Bucket": { - "Type": "AWS::S3::Bucket", - "Properties": { - "BlockPublicAcls" : true, - "BlockPublicPolicy" : true, - "IgnorePublicAcls" : true, - "RestrictPublicBuckets" : true - }, - "Metadata": { - "aws:cdk:path": "foo/Counter/S3/Resource" - } - } - } -}, - -{ "Resources": { - "S3Bucket": { - "Type": "AWS::S3::Bucket", - "Properties": { - "BlockPublicAcls" : false, - "BlockPublicPolicy" : true, - "IgnorePublicAcls" : true, - "RestrictPublicBuckets" : true - }, - "Metadata": { - "aws:cdk:path": "foo/Counter/S3/Resource" - } - } - } -}, - -{ "Resources": { - "S3Bucket": { - "Type": "AWS::S3::Bucket", - "Properties": { - "BlockPublicAcls" : true, - "BlockPublicPolicy" : false, - "IgnorePublicAcls" : true, - "RestrictPublicBuckets" : true - }, - "Metadata": { - "aws:cdk:path": "foo/Counter/S3/Resource" - } - } - } -}, - -{ "Resources": { - "S3Bucket": { - "Type": "AWS::S3::Bucket", - "Properties": { - "BlockPublicAcls" : true, - "BlockPublicPolicy" : true, - "IgnorePublicAcls" : false, - "RestrictPublicBuckets" : true - }, - "Metadata": { - "aws:cdk:path": "foo/Counter/S3/Resource" - } - } - } -}, - -{ "Resources": { - "S3Bucket": { - "Type": "AWS::S3::Bucket", - "Properties": { - "BlockPublicAcls" : true, - "BlockPublicPolicy" : true, - "IgnorePublicAcls" : true, - "RestrictPublicBuckets" : false - }, - "Metadata": { - "aws:cdk:path": "foo/Counter/S3/Resource" - } - } - } -}, - -{ "Resources": { - "S3Bucket": { - "Type": "AWS::S3::Bucket", - "Properties": { - "BlockPublicAcls" : false, - "BlockPublicPolicy" : false, - "IgnorePublicAcls" : false, - "RestrictPublicBuckets" : false - }, - "Metadata": { - "aws:cdk:path": "foo/Counter/S3/Resource" - } - } - } -}, - -{ "Resources": { - "S3Bucket": { - "Type": "AWS::S3::Bucket", - "Metadata": { - "aws:cdk:path": "foo/Counter/S3/Resource" - } - } - } -}, - -{ "Resources": { - "S3Bucket": { - "Type": "AWS::S3::Bucket", - "Properties": { - "BlockPublicAcls" : true - }, - "Metadata": { - "aws:cdk:path": "foo/Counter/S3/Resource" - } - } - } -}, - -{ "Resources": { - "S3Bucket": { - "Type": "AWS::S3::Bucket", - "Properties": { - "BlockPublicAcls" : true, - "BlockPublicPolicy" : true - }, - "Metadata": { - "aws:cdk:path": "foo/Counter/S3/Resource" - } - } - } -}, - -{ "Resources": { - "S3Bucket": { - "Type": "AWS::S3::Bucket", - "Properties": { - "BlockPublicAcls" : true, - "BlockPublicPolicy" : true, - "RestrictPublicBuckets" : true - }, - "Metadata": { - "aws:cdk:path": "foo/Counter/S3/Resource" - } - } - } -}] - - "#; - - let parsed_values = match PathAwareValue::try_from(values)? { - PathAwareValue::List((_, v)) => v, - _ => unreachable!(), - }; - - let rule = r#" - rule deny_s3_public_bucket { - AWS::S3::Bucket { # this is just a short form notation for Resources.*[ Type == "AWS::S3::Bucket" ] - Properties.BlockPublicAcls NOT EXISTS or - Properties.BlockPublicPolicy NOT EXISTS or - Properties.IgnorePublicAcls NOT EXISTS or - Properties.RestrictPublicBuckets NOT EXISTS or - - Properties.BlockPublicAcls == false or - Properties.BlockPublicPolicy == false or - Properties.IgnorePublicAcls == false or - Properties.RestrictPublicBuckets == false - } -} - - "#; - - let s3_rule = Rule::try_from(rule)?; - let dummy = DummyEval {}; - let reported = Reporter(&dummy); - for (idx, each) in parsed_values.iter().enumerate() { - let status = s3_rule.evaluate(each, &reported)?; - println!("Status#{} = {}", idx, status); - } - Ok(()) -} - -#[test] -fn ecs_iam_role_relationship_assetions() -> Result<()> { - let _template = r#" - # deny_task_role_no_permission_boundary is expected to be false so negate it to pass test -{ "Resources": { - "CounterTaskDef1468734E": { - "Type": "AWS::ECS::TaskDefinition", - "Properties": { - "ContainerDefinitions": [ - { - "Environment": [ - { - "Name": "COUNTER_TABLE_NAME", - "Value": { - "Ref": "CounterTableFE2C0268" - } - } - ], - "Essential": true, - "Image": { - "Fn::Sub": "${AWS::AccountId}.dkr.ecr.${AWS::Region}.${AWS::URLSuffix}/cdk-hnb659fds-container-assets-${AWS::AccountId}-${AWS::Region}:9a4832ed07fabf889e6df624dc8a8170008880d8db629312f85dba129920e0b1" - }, - "LogConfiguration": { - "LogDriver": "awslogs", - "Options": { - "awslogs-group": { - "Ref": "CounterTaskDefwebLogGroup437F46A3" - }, - "awslogs-stream-prefix": "Counter", - "awslogs-region": { - "Ref": "AWS::Region" - } - } - }, - "Name": "web", - "PortMappings": [ - { - "ContainerPort": 8080, - "Protocol": "tcp" - } - ] - } - ], - "Cpu": "256", - "ExecutionRoleArn": { - "Fn::GetAtt": [ - "CounterTaskDefExecutionRole5959CB2D", - "Arn" - ] - }, - "Family": "fooCounterTaskDef49BA9021", - "Memory": "512", - "NetworkMode": "awsvpc", - "RequiresCompatibilities": [ - "FARGATE" - ], - "TaskRoleArn": { - "Fn::GetAtt": [ - "CounterTaskRole71EBC3F8", - "Arn" - ] - } - }, - "Metadata": { - "aws:cdk:path": "foo/Counter/TaskDef/Resource" - } - }, - "CounterTaskRole71EBC3F8": { - "Type": "AWS::IAM::Role", - "Properties": { - "AssumeRolePolicyDocument": { - "Statement": [ - { - "Action": "sts:AssumeRole", - "Effect": "Allow", - "Principal": { - "Service": "ecs-tasks.amazonaws.com" - } - } - ], - "Version": "2012-10-17" - }, - "Tags": [{"Key": "TestRole", "Value": ""}], - "PermissionBoundary": "arn:aws:iam...", - "Policies": [ - { - "PolicyDocument": { - "Statement": [ - { - "Action": [ - "dynamodb:BatchGet*", - "dynamodb:DescribeStream", - "dynamodb:DescribeTable", - "dynamodb:Get*", - "dynamodb:Query", - "dynamodb:Scan", - "dynamodb:BatchWrite*", - "dynamodb:CreateTable", - "dynamodb:Delete*", - "dynamodb:Update*", - "dynamodb:PutItem" - ], - "Effect": "Allow", - "Resource": { - "Fn::GetAtt": [ - "CounterTableFE2C0268", - "Arn" - ] - } - } - ], - "Version": "2012-10-17" - }, - "PolicyName": "DynamoDBTableRWAccess" - } - ] - }, - "Metadata": { - "aws:cdk:path": "foo/CounterTaskRole/Default/Resource" - } - } - } -} - "#; - Ok(()) -} - -struct VariableResolver<'a, 'b>( - &'a dyn EvaluationContext, - HashMap>, -); - -impl<'a, 'b> EvaluationContext for VariableResolver<'a, 'b> { - fn resolve_variable(&self, variable: &str) -> Result> { - if let Some(value) = self.1.get(variable) { - Ok(value.clone()) - } else { - self.0.resolve_variable(variable) - } - } - - fn rule_status(&self, rule_name: &str) -> Result { - self.0.rule_status(rule_name) - } - - fn end_evaluation( - &self, - eval_type: EvaluationType, - context: &str, - msg: String, - from: Option, - to: Option, - status: Option, - cmp: Option<(CmpOperator, bool)>, - ) { - self.0 - .end_evaluation(eval_type, context, msg, from, to, status, cmp); - } - - fn start_evaluation(&self, eval_type: EvaluationType, context: &str) { - self.0.start_evaluation(eval_type, context); - } -} - -#[test] -fn test_iam_subselections() -> Result<()> { - let template = r#" - { - Resources: { - # NOT SELECTED - one: { - Type: "AWS::IAM::Role", - Properties: { - Tags: [ - { - Key: "TestRole", - Value: "" - } - ], - PermissionsBoundary: "aws:arn" - } - }, - # SELECTED - two: - { - Type: "AWS::IAM::Role", - Properties: { - Tags: [ - { - Key: "TestRole", - Value: "" - } - ] - } - }, - # NOT SELECTED - three: { - Type: "AWS::IAM::Role", - Properties: { - Tags: [], - PermissionsBoundary: "aws:arn" - } - }, - # NOT SELECTED #1, SELECTED #2 - four: - { - Type: "AWS::IAM::Role", - Properties: { - Tags: [ - { - Key: "Prod", - Value: "" - } - ] - } - } - } - } - "#; - - let value = Value::try_from(template)?; - let value = PathAwareValue::try_from(value)?; - let query = AccessQuery::try_from( - r#"Resources.*[ - Type == "AWS::IAM::Role" - Properties.Tags[ Key == "TestRole" ] !EMPTY - Properties.PermissionsBoundary !EXISTS - ]"#, - )?; - let dummy = DummyEval {}; - let selected = value.select(query.match_all, &query.query, &dummy)?; - println!("Selected {:?}", selected); - assert_eq!(selected.len(), 1); - assert_eq!(selected[0].self_path(), &Path::try_from("/Resources/two")?); - let expected = PathAwareValue::try_from(( - r#" - { - Type: "AWS::IAM::Role", - Properties: { - Tags: [ - { - Key: "TestRole", - Value: "" - } - ] - } - } - "#, - Path::try_from("/Resources/two")?, - ))?; - assert_eq!(selected[0], &expected); - - let query = AccessQuery::try_from( - r#"Resources.*[ - Type == "AWS::IAM::Role" - Properties.Tags[ Key == "TestRole" or Key == "Prod" ] !EMPTY - Properties.PermissionsBoundary !EXISTS - ]"#, - )?; - let selected = value.select(query.match_all, &query.query, &dummy)?; - println!("Selected {:?}", selected); - assert_eq!(selected.len(), 2); - let expected2 = PathAwareValue::try_from(( - r#" - { - Type: "AWS::IAM::Role", - Properties: { - Tags: [ - { - Key: "Prod", - Value: "" - } - ] - } - } - "#, - Path::try_from("/Resources/four")?, - ))?; - assert_eq!(selected[0], &expected); - assert_eq!(selected[1], &expected2); - - let rules_file = r#" -let iam_roles = Resources.*[ Type == "AWS::IAM::Role" ] - -rule deny_permissions_boundary_iam_role when %iam_roles !EMPTY { - # atleast one Tags contains a Key "TestRole" - %iam_roles[ - # Properties.Tags !EMPTY - Properties.Tags[ Key == "TestRole" ] !EMPTY - Properties.PermissionsBoundary !EXISTS - ] !EMPTY -} - "#; - - let rules = RulesFile::try_from(rules_file)?; - let root_scope = RootScope::new(&rules, &value)?; - let reporter = Reporter(&root_scope); - let status = rules.evaluate(&value, &reporter)?; - println!("Status = {}", status); - assert_eq!(status, Status::PASS); - let fail_value = PathAwareValue::try_from(( - r#" - { Resources: { - one: { - Type: "AWS::IAM::Role", - Properties: { - Tags: [ - { - Key: "Prod", - Value: "" - } - ] - } - } - } - } - "#, - Path::try_from("/Resources/four")?, - ))?; - let root_scope = RootScope::new(&rules, &fail_value)?; - let reporter = Reporter(&root_scope); - let status = rules.evaluate(&fail_value, &reporter)?; - println!("Status = {}", status); - assert_eq!(status, Status::FAIL); - - Ok(()) -} - -#[test] -fn test_rules_with_some_clauses() -> Result<()> { - let query = r#"some Resources.*[ Type == 'AWS::IAM::Role' ].Properties.Tags[ Key == /[A-Za-z0-9]+Role/ ]"#; - let resources = r#" { - "Resources": { - "CounterTaskDefExecutionRole5959CB2D": { - "Type": "AWS::IAM::Role", - "Properties": { - "AssumeRolePolicyDocument": { - "Statement": [ - { - "Action": "sts:AssumeRole", - "Effect": "Allow", - "Principal": { - "Service": "ecs-tasks.amazonaws.com" - } - }], - "Version": "2012-10-17" - }, - "PermissionsBoundary": {"Fn::Sub" : "arn::aws::iam::${AWS::AccountId}:policy/my-permission-boundary"}, - "Tags": [{ "Key": "TestRole", "Value": ""}] - }, - "Metadata": { - "aws:cdk:path": "foo/Counter/TaskDef/ExecutionRole/Resource" - } - }, - "BlankRole001": { - "Type": "AWS::IAM::Role", - "Properties": { - "AssumeRolePolicyDocument": { - "Statement": [ - { - "Action": "sts:AssumeRole", - "Effect": "Allow", - "Principal": { - "Service": "ecs-tasks.amazonaws.com" - } - }], - "Version": "2012-10-17" - }, - "Tags": [{ "Key": "FooBar", "Value": ""}] - }, - "Metadata": { - "aws:cdk:path": "foo/Counter/TaskDef/ExecutionRole/Resource" - } - }, - "BlankRole002": { - "Type": "AWS::IAM::Role", - "Properties": { - "AssumeRolePolicyDocument": { - "Statement": [ - { - "Action": "sts:AssumeRole", - "Effect": "Allow", - "Principal": { - "Service": "ecs-tasks.amazonaws.com" - } - }], - "Version": "2012-10-17" - } - }, - "Metadata": { - "aws:cdk:path": "foo/Counter/TaskDef/ExecutionRole/Resource" - } - } - } - } - "#; - let value = PathAwareValue::try_from(resources)?; - let parsed = AccessQuery::try_from(query)?; - let dummy = DummyEval {}; - let selected = value.select(parsed.match_all, &parsed.query, &dummy)?; - println!("{:?}", selected); - assert_eq!(selected.len(), 1); - Ok(()) -} - -#[test] -fn test_support_for_atleast_one_match_clause() -> Result<()> { - let clause_some_str = r#"some Tags[*].Key == /PROD/"#; - let clause_some = GuardClause::try_from(clause_some_str)?; - - let clause_str = r#"Tags[*].Key == /PROD/"#; - let clause = GuardClause::try_from(clause_str)?; - - let values_str = r#"{ - Tags: [ - { - Key: "InPROD", - Value: "ProdApp" - }, - { - Key: "NoP", - Value: "NoQ" - } - ] - } - "#; - let values = PathAwareValue::try_from(values_str)?; - let dummy = DummyEval {}; - - let status = clause_some.evaluate(&values, &dummy)?; - assert_eq!(status, Status::PASS); - let status = clause.evaluate(&values, &dummy)?; - assert_eq!(status, Status::FAIL); - - let values_str = r#"{ Tags: [] }"#; - let values = PathAwareValue::try_from(values_str)?; - let status = clause_some.evaluate(&values, &dummy)?; - assert_eq!(status, Status::FAIL); - let status = clause.evaluate(&values, &dummy)?; - assert_eq!(status, Status::FAIL); - - let values_str = r#"{ }"#; - let values = PathAwareValue::try_from(values_str)?; - let status = clause.evaluate(&values, &dummy)?; - assert_eq!(status, Status::FAIL); - - let r = clause_some.evaluate(&values, &dummy); - assert!(r.is_ok()); - assert_eq!(r.unwrap(), Status::FAIL); - - // - // Trying out the selection filters - // - let selection_str = r#"Resources.*[ - Type == 'AWS::DynamoDB::Table' - some Properties.Tags[*].Key == /PROD/ - ]"#; - let _query = AccessQuery::try_from(selection_str)?; - let resources_str = r#"{ - Resources: { - ddbSelected: { - Type: 'AWS::DynamoDB::Table', - Properties: { - Tags: [ - { - Key: "PROD", - Value: "ProdApp" - } - ] - } - }, - ddbNotSelected: { - Type: 'AWS::DynamoDB::Table' - } - } - }"#; - let resources = PathAwareValue::try_from(resources_str)?; - let selection_query = AccessQuery::try_from(selection_str)?; - let selected = resources.select(selection_query.match_all, &selection_query.query, &dummy)?; - println!("Selected = {:?}", selected); - assert_eq!(selected.len(), 1); - - Ok(()) -} - -#[test] -fn double_projection_tests() -> Result<()> { - let rule_str = r###" - rule check_ecs_against_local_or_metadata { - let ecs_tasks = Resources.*[ - Type == 'AWS::ECS::TaskDefinition' - Properties.TaskRoleArn exists - ] - - let iam_references = some %ecs_tasks.Properties.TaskRoleArn.'Fn::GetAtt'[0] - when %iam_references !empty { - let iam_local = Resources.%iam_references - %iam_local.Type == 'AWS::IAM::Role' - %iam_local.Properties.PermissionsBoundary exists - } - - let ecs_task_role_is_string = %ecs_tasks[ - Properties.TaskRoleArn is_string - ] - when %ecs_task_role_is_string !empty { - %ecs_task_role_is_string.Metadata.NotRestricted exists - } - } - "###; - - let resources_str = r#" - { - Resources: { - ecs: { - Type: 'AWS::ECS::TaskDefinition', - Metadata: { - NotRestricted: true - }, - Properties: { - TaskRoleArn: "aws:arn..." - } - }, - ecs2: { - Type: 'AWS::ECS::TaskDefinition', - Properties: { - TaskRoleArn: { 'Fn::GetAtt': ["iam", "arn"] } - } - }, - iam: { - Type: 'AWS::IAM::Role', - Properties: { - PermissionsBoundary: "aws:arn" - } - } - } - } - "#; - let value = PathAwareValue::try_from(resources_str)?; - let dummy = DummyEval {}; - let rule = Rule::try_from(rule_str)?; - let status = rule.evaluate(&value, &dummy)?; - assert_eq!(status, Status::PASS); - - let resources_str = r#" - { - Resources: { - ecs2: { - Type: 'AWS::ECS::TaskDefinition', - Properties: { - TaskRoleArn: { 'Fn::GetAtt': ["iam", "arn"] } - } - } - } - } - "#; - let value = PathAwareValue::try_from(resources_str)?; - let status = rule.evaluate(&value, &dummy)?; - println!("{}", status); - assert_eq!(status, Status::FAIL); - - Ok(()) -} - -#[test] -fn test_map_keys_function() -> Result<()> { - let value_str = r#" - Resources: - apiGw: - Type: 'AWS::ApiGateway::RestApi' - Properties: - EndpointConfiguration: ["PRIVATE"] - Policy: - Statement: - - Action: Allow - Resource: ['*', "aws:"] - Condition: - 'aws:IsSecure': true - - "#; - let value = serde_yaml::from_str::(value_str)?; - let value = PathAwareValue::try_from(value)?; - - let rule_str = r#" -let api_gws = Resources.*[ Type == 'AWS::ApiGateway::RestApi' ] -rule check_rest_api_is_private_and_has_access when %api_gws !empty { - %api_gws.Properties.EndpointConfiguration == ["PRIVATE"] - some %api_gws.Properties.Policy.Statement[*].Condition[ keys == /aws:[sS]ource(Vpc|VPC|Vpce|VPCE)/ ] !empty -}"#; - let rule = RulesFile::try_from(rule_str)?; - let root = RootScope::new(&rule, &value)?; - let status = rule.evaluate(&value, &root)?; - assert_eq!(status, Status::FAIL); - - let value_str = r#" - Resources: - apiGw: - Type: 'AWS::ApiGateway::RestApi' - Properties: - EndpointConfiguration: ["PRIVATE"] - Policy: - Statement: - - Action: Allow - Resource: ['*', "aws:"] - Condition: - 'aws:IsSecure': true - 'aws:sourceVpc': ['vpc-1234'] - - "#; - let value = serde_yaml::from_str::(value_str)?; - let value = PathAwareValue::try_from(value)?; - let root = RootScope::new(&rule, &value)?; - let status = rule.evaluate(&value, &root)?; - assert_eq!(status, Status::PASS); - - Ok(()) -} - -#[test] -fn test_compare_loop_atleast_one_eq() -> Result<()> { - let root = Path::root(); - let lhs = [ - PathAwareValue::String((root.clone(), "aws:isSecure".to_string())), - PathAwareValue::String((root.clone(), "aws:sourceVpc".to_string())), - ]; - let rhs = [PathAwareValue::Regex(( - root, - "aws:[sS]ource(Vpc|VPC|Vpce|VPCE)".to_string(), - ))]; - - let lhs_values = lhs.iter().collect::>(); - let rhs_values = rhs.iter().collect::>(); - - // - // match any one rhs = false, at-least-one = false - // - let (result, _results) = compare_loop( - &lhs_values, - &rhs_values, - path_value::compare_eq, - false, - false, - )?; - assert!(!result); - - // - // match any one rhs = false, at-least-one = true - // - let (result, _results) = compare_loop( - &lhs_values, - &rhs_values, - path_value::compare_eq, - false, - true, - )?; - assert!(result); - - // - // match any one rhs = true, at-least-one = false - // - let (result, _results) = compare_loop( - &lhs_values, - &rhs_values, - path_value::compare_eq, - true, - false, - )?; - assert!(!result); - - Ok(()) -} - -#[test] -fn test_compare_loop_all() -> Result<()> { - let root = Path::root(); - let lhs = [ - PathAwareValue::String((root.clone(), "aws:isSecure".to_string())), - PathAwareValue::String((root.clone(), "aws:sourceVpc".to_string())), - ]; - let rhs = [PathAwareValue::Regex(( - root, - "aws:[sS]ource(Vpc|VPC|Vpce|VPCE)".to_string(), - ))]; - - let lhs_values = lhs.iter().collect::>(); - let rhs_values = rhs.iter().collect::>(); - - let results = super::compare_loop_all(&lhs_values, &rhs_values, path_value::compare_eq, false)?; - // - // One result for each LHS value - // - assert_eq!(results.1.len(), 2); - let (outcome, from, to) = &results.1[0]; - assert!(!*outcome); - assert_eq!(from, &Some(lhs[0].clone())); - assert_eq!(to, &Some(rhs[0].clone())); - - let (outcome, from, to) = &results.1[1]; - assert!(*outcome); - assert_eq!(from, &None); - assert_eq!(to, &None); - - Ok(()) -} - -#[test] -fn test_compare_lists() -> Result<()> { - let root = Path::root(); - let value = PathAwareValue::List(( - root.clone(), - vec![ - PathAwareValue::Int((root.clone(), 1)), - PathAwareValue::Int((root, 2)), - ], - )); - let lhs = vec![&value]; - let rhs = vec![&value]; - - let query = []; - let r = super::compare( - &lhs, - &query, - &rhs, - None, - super::super::path_value::compare_eq, - false, - false, - )?; - assert_eq!(r.0, Status::PASS); - Ok(()) -} - -#[test] -fn test_compare_rulegen() -> Result<()> { - let rulegen_created = r#" -let aws_ec2_securitygroup_resources = Resources.*[ Type == 'AWS::EC2::SecurityGroup' ] -rule aws_ec2_securitygroup when %aws_ec2_securitygroup_resources !empty { - %aws_ec2_securitygroup_resources.Properties.SecurityGroupEgress == [{"CidrIp":"0.0.0.0/0","IpProtocol":-1},{"CidrIpv6":"::/0","IpProtocol":-1}] -}"#; - let template = r#" -Resources: - - # SecurityGroups - ## Alb Security Groups - - rFrontendAppSpecificSg: - Type: AWS::EC2::SecurityGroup - Properties: - GroupDescription: Frontend Security Group - GroupName: secgrp-frontend - SecurityGroupEgress: - - CidrIp: "0.0.0.0/0" - IpProtocol: -1 - - CidrIpv6: "::/0" - IpProtocol: -1 - VpcId: vpc-123abc - "#; - let rules = RulesFile::try_from(rulegen_created)?; - let value = PathAwareValue::try_from(serde_yaml::from_str::(template)?)?; - let root = RootScope::new(&rules, &value)?; - let status = rules.evaluate(&value, &root)?; - assert_eq!(status, Status::PASS); - Ok(()) -} - -#[test] -fn test_guard_10_compatibility_and_diff() -> Result<()> { - let value_str = r###" - Statement: - - Principal: ['*', 's3:*'] - "###; - let value = PathAwareValue::try_from(serde_yaml::from_str::(value_str)?)?; - let dummy = DummyEval {}; - - // - // Evaluation differences with 1.0 for Statement.*.Principal == '*' - // - // Guard 1.0 this would PASS with at-least one semantics for the payload above. This is where docs - // need to be consulted to understand that == is at-least-one and != is ALL. Due to this decision certain - // expressions like ensure that ALL AWS::EC2::Volume Encrypted == true, could not be specified - // - // In Guard 2.0 this would FAIL. The reason being that Guard 2.0 goes for explicitness in specifying - // clauses. By default it asserts for ALL semantics. If you expecting to match at-least one or more - // you must use SOME keyword that would evaluate correctly. With this support in 2.0 we can - // support ALL expressions like - // - // AWS::EC2::Volume Properties.Encrypted == true - // - // At the same time, one can explicitly express at-least-one or more semantics using SOME - // - // AWS::EC2::Volume SOME Properties.Encrypted == true - // - // And finally - // - // AWS::EC2::Volume Properties { - // Encrypted !EXISTS or - // Encrypted == true - // } - // - // can be correctly specified. This also makes the intent clear to both the rule author and - // auditor what was acceptable. Here, it is okay that accept Encrypted was not specified - // as an attribute or when specified it must be true. This makes it clear to the reader/auditor - // rather than guess at how Guard engine evaluates. - // - // The evaluation engine is purposefully dumb and stupid, defaults to working - // one way consistently enforcing ALL semantics. Needs to told explicitly to do otherwise - // - - let clause_str = r#"Statement.*.Principal == '*'"#; - let clause = GuardClause::try_from(clause_str)?; - let status = clause.evaluate(&value, &dummy)?; - assert_eq!(status, Status::FAIL); - - let clause_str = r#"SOME Statement.*.Principal == '*'"#; - let clause = GuardClause::try_from(clause_str)?; - let dummy = DummyEval {}; - let status = clause.evaluate(&value, &dummy)?; - assert_eq!(status, Status::PASS); - - let value_str = r###" - Statement: - - Principal: aws - - Principal: ['*', 's3:*'] - "###; - let value = PathAwareValue::try_from(serde_yaml::from_str::(value_str)?)?; - // - // Evaluate the SOME clause again, it must pass with the value as well - // - let status = clause.evaluate(&value, &dummy)?; - assert_eq!(status, Status::PASS); - - Ok(()) -} - -#[test] -fn block_evaluation() -> Result<()> { - let value_str = r#" - Resources: - apiGw: - Type: 'AWS::ApiGateway::RestApi' - Properties: - EndpointConfiguration: ["PRIVATE"] - Policy: - Statement: - - Action: Allow - Resource: ['*', "aws:"] - Condition: - 'aws:IsSecure': true - 'aws:sourceVpc': ['vpc-1234'] - - Action: Allow - Resource: ['*', "aws:"] - - "#; - let value = serde_yaml::from_str::(value_str)?; - let value = PathAwareValue::try_from(value)?; - let clause_str = r#"Resources.*[ Type == 'AWS::ApiGateway::RestApi' ].Properties { - EndpointConfiguration == ["PRIVATE"] - some Policy.Statement[*] { - Action == 'Allow' - Condition[ keys == 'aws:IsSecure' ] !empty - } - } - "#; - let clause = GuardClause::try_from(clause_str)?; - let dummy = DummyEval {}; - let status = clause.evaluate(&value, &dummy)?; - assert_eq!(status, Status::PASS); - Ok(()) -} - -#[test] -fn block_evaluation_fail() -> Result<()> { - let value_str = r#" - Resources: - apiGw: - Type: 'AWS::ApiGateway::RestApi' - Properties: - EndpointConfiguration: ["PRIVATE"] - Policy: - Statement: - - Action: Allow - Resource: ['*', "aws:"] - Condition: - 'aws:IsSecure': true - 'aws:sourceVpc': ['vpc-1234'] - - Action: Allow - Resource: ['*', "aws:"] - apiGw2: - Type: 'AWS::ApiGateway::RestApi' - Properties: - EndpointConfiguration: ["PRIVATE"] - Policy: - Statement: - - Action: Allow - Resource: ['*', "aws:"] - - "#; - let value = serde_yaml::from_str::(value_str)?; - let value = PathAwareValue::try_from(value)?; - let clause_str = r#"Resources.*[ Type == 'AWS::ApiGateway::RestApi' ].Properties { - EndpointConfiguration == ["PRIVATE"] - some Policy.Statement[*] { - Action == 'Allow' - Condition[ keys == 'aws:IsSecure' ] !empty - } - } - "#; - let clause = GuardClause::try_from(clause_str)?; - let dummy = DummyEval {}; - let status = clause.evaluate(&value, &dummy)?; - assert_eq!(status, Status::FAIL); - Ok(()) -} - -#[test] -fn embedded_when_clause_redshift_use_case_test() -> Result<()> { - let rule = r###" -# -# Find all Redshift subnet group resource and extract all subnet Ids that are referenced -# -let local_subnet_refs = Resources.*[ Type == /Redshift::ClusterSubnetGroup/ ].Properties.SubnetIds[ Ref exists ].Ref - -rule redshift_is_not_internet_accessible when %local_subnet_refs !empty { - - # - # check that all local references where indeed subnet type. FAIL otherwise - # - Resources.%local_subnet_refs.Type == 'AWS::EC2::Subnet' - - # - # Find out all Subnet Route Associations with the set of subnets and extract the - # Route Table references that they have - # - let route_tables = some Resources.*[ - Type == 'AWS::EC2::SubnetRouteTableAssociation' - Properties.SubnetId.Ref in %local_subnet_refs - ].Properties.RouteTableId.Ref - - # - # If no associations are present in the template then we SKIP the check - # - when %route_tables !empty { - # - # Ensure that all of these references where indeed RouteTable references - # - Resources.%route_tables.Type == 'AWS::EC2::RouteTable' - - # - # Find all routes that have a gateways associated with the route table and extract - # all their references - # - let gws_ids = some Resources.*[ - Type == 'AWS::EC2::Route' - Properties.GatewayId.Ref exists - Properties.RouteTableId.Ref in %route_tables - ].Properties.GatewayId.Ref - - # - # if no gateways or route association were found then we skip the check - # - when %gws_ids !empty { - Resources.%gws_ids.Type != 'AWS::EC2::InternetGateway' - } - } - -}"###; - - let value_str = r#" - Resources: - rcsg: - Type: 'AWS::Redshift::ClusterSubnetGroup' - Properties: - SubnetIds: [{Ref: subnet}, "subnet-2"] - subnet: - Type: 'AWS::EC2::Subnet' - subRtAssoc: - Type: 'AWS::EC2::SubnetRouteTableAssociation' - Properties: - SubnetId: { Ref: subnet } - RouteTableId: { Ref: rt } - rt: - Type: 'AWS::EC2::RouteTable' - route1: - Type: 'AWS::EC2::Route' - Properties: - GatewayId: { Ref: gw } - RouteTableId: { Ref: rt } - gw: - Type: 'AWS::EC2::InternetGateway' - "#; - - let rules_files = RulesFile::try_from(rule)?; - let value = serde_yaml::from_str::(value_str)?; - let value = PathAwareValue::try_from(value)?; - let root = RootScope::new(&rules_files, &value)?; - let status = rules_files.evaluate(&value, &root)?; - assert_eq!(status, Status::FAIL); - - let value_str = r#" - Resources: - rcsg: - Type: 'AWS::Redshift::ClusterSubnetGroup' - Properties: - SubnetIds: [{Ref: subnet}, "subnet-2"] - subnet: - Type: 'AWS::EC2::Subnet' - subRtAssoc: - Type: 'AWS::EC2::SubnetRouteTableAssociation' - Properties: - SubnetId: { Ref: subnet } - RouteTableId: { Ref: rt } - rt: - Type: 'AWS::EC2::RouteTable' - route1: - Type: 'AWS::EC2::Route' - Properties: - GatewayId: { Ref: gw } - RouteTableId: { Ref: rt } - gw: - Type: 'AWS::EC2::TransitGateway' - "#; - - let rules_files = RulesFile::try_from(rule)?; - let value = serde_yaml::from_str::(value_str)?; - let value = PathAwareValue::try_from(value)?; - let root = RootScope::new(&rules_files, &value)?; - let status = rules_files.evaluate(&value, &root)?; - assert_eq!(status, Status::PASS); - - let value_str = r#" - Resources: - rcsg: - Type: 'AWS::Redshift::ClusterSubnetGroup' - Properties: - SubnetIds: [{Ref: subnet}, "subnet-2"] - subnet: - Type: 'AWS::EC2::Subnet' - subRtAssoc: - Type: 'AWS::EC2::SubnetRouteTableAssociation' - Properties: - SubnetId: { Ref: subnet } - RouteTableId: { Ref: rt } - rt: - Type: 'AWS::EC2::RouteTable' - route1: - Type: 'AWS::EC2::Route' - Properties: - GatewayId: { Ref: gw } - RouteTableId: { Ref: rt } - "#; - - let rules_files = RulesFile::try_from(rule)?; - let value = serde_yaml::from_str::(value_str)?; - let value = PathAwareValue::try_from(value)?; - let root = RootScope::new(&rules_files, &value)?; - let status = rules_files.evaluate(&value, &root)?; - assert_eq!(status, Status::FAIL); - Ok(()) -} - -struct Tracker<'a> { - root: &'a dyn EvaluationContext, - expected: HashMap, -} - -impl<'a> EvaluationContext for Tracker<'a> { - fn resolve_variable(&self, variable: &str) -> Result> { - self.root.resolve_variable(variable) - } - - fn rule_status(&self, rule_name: &str) -> Result { - self.root.rule_status(rule_name) - } - - fn end_evaluation( - &self, - eval_type: EvaluationType, - context: &str, - msg: String, - from: Option, - to: Option, - status: Option, - cmp: Option<(CmpOperator, bool)>, - ) { - self.root - .end_evaluation(eval_type, context, msg, from, to, status, cmp); - if eval_type == EvaluationType::Rule { - match self.expected.get(context) { - Some(e) => { - assert_eq!(*e, status.unwrap()); - } - _ => unreachable!(), - } - } - } - - fn start_evaluation(&self, eval_type: EvaluationType, context: &str) { - self.root.start_evaluation(eval_type, context) - } -} - -#[test] -fn rule_clause_when_check() -> Result<()> { - let rules_skipped = r#" - rule skipped when skip !exists { - Resources.*.Properties.Tags !empty - } - - rule dependent_on_skipped when skipped { - Resources.*.Properties exists - } - - rule dependent_on_dependent when dependent_on_skipped { - Resources.*.Properties exists - } - - rule dependent_on_not_skipped when !skipped { - Resources.*.Properties exists - } - "#; - - let input = r#" - { - skip: true, - Resources: { - first: { - Type: 'WhackWhat', - Properties: { - Tags: [{ hi: "there" }, { right: "way" }] - } - } - } - } - "#; - - let resources = PathAwareValue::try_from(input)?; - let rules = RulesFile::try_from(rules_skipped)?; - let root = RootScope::new(&rules, &resources)?; - let mut expectations = HashMap::with_capacity(3); - expectations.insert("skipped".to_string(), Status::SKIP); - expectations.insert("dependent_on_skipped".to_string(), Status::SKIP); - expectations.insert("dependent_on_dependent".to_string(), Status::SKIP); - expectations.insert("dependent_on_not_skipped".to_string(), Status::PASS); - let tracker = Tracker { - root: &root, - expected: expectations, - }; - - let status = rules.evaluate(&resources, &tracker)?; - assert_eq!(status, Status::PASS); - - let input = r#" - { - Resources: { - first: { - Type: 'WhackWhat', - Properties: { - Tags: [{ hi: "there" }, { right: "way" }] - } - } - } - } - "#; - - let resources = PathAwareValue::try_from(input)?; - let rules = RulesFile::try_from(rules_skipped)?; - let root = RootScope::new(&rules, &resources)?; - let mut expectations = HashMap::with_capacity(3); - expectations.insert("skipped".to_string(), Status::PASS); - expectations.insert("dependent_on_skipped".to_string(), Status::PASS); - expectations.insert("dependent_on_dependent".to_string(), Status::PASS); - expectations.insert("dependent_on_not_skipped".to_string(), Status::SKIP); - let tracker = Tracker { - root: &root, - expected: expectations, - }; - - let status = rules.evaluate(&resources, &tracker)?; - assert_eq!(status, Status::PASS); - Ok(()) -} - -#[test] -fn test_field_type_array_or_single() -> Result<()> { - let statements = r#"{ - Statement: [{ - Action: '*', - Effect: 'Allow', - Resources: '*' - }, { - Action: ['api:Get', 'api2:Set'], - Effect: 'Allow', - Resources: '*' - }] - } - "#; - let path_value = PathAwareValue::try_from(statements)?; - let clause = GuardClause::try_from(r#"Statement[*].Action != '*'"#)?; - let dummy = DummyEval {}; - let status = clause.evaluate(&path_value, &dummy)?; - assert_eq!(status, Status::FAIL); - - let statements = r#"{ - Statement: { - Action: '*', - Effect: 'Allow', - Resources: '*' - } - } - "#; - let path_value = PathAwareValue::try_from(statements)?; - let status = clause.evaluate(&path_value, &dummy)?; - assert_eq!(status, Status::FAIL); - - let clause = GuardClause::try_from(r#"Statement[*].Action[*] != '*'"#)?; - let status = clause.evaluate(&path_value, &dummy)?; - assert_eq!(status, Status::FAIL); - - // Test old format - let clause = GuardClause::try_from(r#"Statement.*.Action.* != '*'"#)?; - let status = clause.evaluate(&path_value, &dummy)?; - assert_eq!(status, Status::FAIL); - Ok(()) -} - -#[test] -fn test_for_not_in() -> Result<()> { - let statements = r#" - { - "mainSteps": [ - { - "action": "aws:updateAgent" - }, - { - "action": "aws:configurePackage" - } - ] - }"#; - - let clause = GuardClause::try_from( - r#"mainSteps[*].action !IN ["aws:updateSsmAgent", "aws:updateAgent"]"#, - )?; - let value = PathAwareValue::try_from(serde_yaml::from_str::(statements)?)?; - let dummy = DummyEval {}; - let status = clause.evaluate(&value, &dummy)?; - assert_eq!(status, Status::FAIL); - Ok(()) -} - -#[test] -fn test_rule_with_range_test() -> Result<()> { - let rule_str = r#"rule check_parameter_validity { - InputParameter.TcpBlockedPorts[*] { - this in r[0, 65535] <<[NON_COMPLIANT] Parameter TcpBlockedPorts has invalid value.>> - } - }"#; - - let rule = Rule::try_from(rule_str)?; - - let value_str = r#" - InputParameter: - TcpBlockedPorts: - - 21 - - 22 - - 101 - "#; - let value = PathAwareValue::try_from(serde_yaml::from_str::(value_str)?)?; - let dummy = DummyEval {}; - let status = rule.evaluate(&value, &dummy)?; - assert_eq!(status, Status::PASS); - - Ok(()) -} - -#[test] -fn test_inner_when_skipped() -> Result<()> { - let rule_str = r#" - rule no_wild_card_in_managed_policy { - Resources.*[ Type == /ManagedPolicy/ ] { - when Properties.ManagedPolicyName != /Admin/ { - Properties.PolicyDocument.Statement[*].Action[*] != '*' - } - } - } - "#; - - let rule = Rule::try_from(rule_str)?; - let dummy = DummyEval {}; - - let value_str = r#" - Resources: - ReadOnlyAdminPolicy: - Type: 'AWS::IAM::ManagedPolicy' - Properties: - PolicyDocument: - Statement: - - Action: '*' - Effect: Allow - Resource: '*' - Version: 2012-10-17 - Description: '' - ManagedPolicyName: AdminPolicy - ReadOnlyPolicy: - Type: 'AWS::IAM::ManagedPolicy' - Properties: - PolicyDocument: - Statement: - - Action: - - 'cloudwatch:*' - - '*' - Effect: Allow - Resource: '*' - Version: 2013-10-17 - Description: '' - ManagedPolicyName: OperatorPolicy - "#; - let value = PathAwareValue::try_from(serde_yaml::from_str::(value_str)?)?; - - let status = rule.evaluate(&value, &dummy)?; - assert_eq!(status, Status::FAIL); - - let value_str = r#" - Resources: - ReadOnlyAdminPolicy: - Type: 'AWS::IAM::ManagedPolicy' - Properties: - PolicyDocument: - Statement: - - Action: '*' - Effect: Allow - Resource: '*' - Version: 2012-10-17 - Description: '' - ManagedPolicyName: AdminPolicy - "#; - let value = PathAwareValue::try_from(serde_yaml::from_str::(value_str)?)?; - let status = rule.evaluate(&value, &dummy)?; - assert_eq!(status, Status::SKIP); - - let value_str = r#" - Resources: {} - "#; - let value = PathAwareValue::try_from(serde_yaml::from_str::(value_str)?)?; - let status = rule.evaluate(&value, &dummy)?; - assert_eq!(status, Status::FAIL); - - let value_str = r#"{}"#; - let value = PathAwareValue::try_from(serde_yaml::from_str::(value_str)?)?; - let status = rule.evaluate(&value, &dummy)?; - assert_eq!(status, Status::FAIL); - - Ok(()) -} - -#[test] -fn test_multiple_valued_clause_reporting() -> Result<()> { - let rule = r###" - rule name_check { Resources.*.Properties.Name == /NAME/ } - "###; - - let value = r###" - Resources: - second: - Properties: - Name: FAILEDMatch - first: - Properties: - Name: MatchNAME - matches: - Properties: - Name: MatchNAME - failed: - Properties: - Name: FAILEDMatch - "###; - - #[derive(Debug, Clone)] - struct Reporter {} - impl EvaluationContext for Reporter { - fn resolve_variable(&self, _: &str) -> Result> { - todo!() - } - - fn rule_status(&self, _: &str) -> Result { - todo!() - } - - fn end_evaluation( - &self, - eval_type: EvaluationType, - _: &str, - msg: String, - from: Option, - to: Option, - status: Option, - _cmp: Option<(CmpOperator, bool)>, - ) { - if eval_type == EvaluationType::Clause { - match &status { - Some(Status::FAIL) => { - assert!(from.is_some()); - assert!(to.is_some()); - let path_val = from.unwrap(); - let path = path_val.self_path(); - assert!(path.0.contains("/second") || path.0.contains("/failed")); - } - Some(Status::PASS) => { - assert_eq!(from, None); - assert_eq!(to, None); - assert!(msg.contains("DEFAULT")); - } - _ => {} - } - } - } - - fn start_evaluation(&self, _: EvaluationType, _: &str) {} - } - - let rules = Rule::try_from(rule)?; - let values = PathAwareValue::try_from(serde_yaml::from_str::(value)?)?; - let reporter = Reporter {}; - let status = rules.evaluate(&values, &reporter)?; - assert_eq!(status, Status::FAIL); - Ok(()) -} - -#[test] -fn test_multiple_valued_clause_reporting_var_access() -> Result<()> { - let rule = r###" - let resources = Resources.* - rule name_check { %resources.Properties.Name == /NAME/ } - "###; - - let value = r###" - Resources: - second: - Properties: - Name: FAILEDMatch - first: - Properties: - Name: MatchNAME - matches: - Properties: - Name: MatchNAME - failed: - Properties: - Name: FAILEDMatch - "###; - - struct Reporter<'a> { - root: &'a dyn EvaluationContext, - } - - impl<'a> EvaluationContext for Reporter<'a> { - fn resolve_variable(&self, variable: &str) -> Result> { - self.root.resolve_variable(variable) - } - - fn rule_status(&self, rule_name: &str) -> Result { - self.root.rule_status(rule_name) - } - - fn end_evaluation( - &self, - eval_type: EvaluationType, - context: &str, - msg: String, - from: Option, - to: Option, - status: Option, - cmp: Option<(CmpOperator, bool)>, - ) { - if eval_type == EvaluationType::Clause { - match &status { - Some(Status::FAIL) => { - assert!(from.is_some()); - assert!(to.is_some()); - let path_val = from.as_ref().unwrap(); - let path = path_val.self_path(); - assert!(path.0.contains("/second") || path.0.contains("/failed")); - } - Some(Status::PASS) => { - assert_eq!(from, None); - assert_eq!(to, None); - assert!(msg.contains("DEFAULT")); - } - _ => {} - } - } - self.root - .end_evaluation(eval_type, context, msg, from, to, status, cmp) - } - - fn start_evaluation(&self, eval_type: EvaluationType, context: &str) { - self.root.start_evaluation(eval_type, context) - } - } - - let rules = RulesFile::try_from(rule)?; - let values = PathAwareValue::try_from(serde_yaml::from_str::(value)?)?; - let root = RootScope::new(&rules, &values)?; - let reporter = Reporter { root: &root }; - let status = rules.evaluate(&values, &reporter)?; - assert_eq!(status, Status::FAIL); - Ok(()) -} - -#[test] -fn test_in_comparison_operator_for_list_of_lists() -> Result<()> { - let template = r###" - Resources: - MasterRecord: - Type: AWS::Route53::RecordSet - Properties: - HostedZoneName: !Ref 'HostedZoneName' - Comment: DNS name for my instance. - Name: !Join ['', [!Ref 'SubdomainMaster', ., !Ref 'HostedZoneName']] - Type: A - TTL: '900' - ResourceRecords: - - !GetAtt Master.PrivateIp - InternalRecord: - Type: AWS::Route53::RecordSet - Properties: - HostedZoneName: !Ref 'HostedZoneName' - Comment: DNS name for my instance. - Name: !Join ['', [!Ref 'SubdomainInternal', ., !Ref 'HostedZoneName']] - Type: A - TTL: '900' - ResourceRecords: - - !GetAtt Master.PrivateIp - SubdomainRecord: - Type: AWS::Route53::RecordSet - Properties: - HostedZoneName: !Ref 'HostedZoneName' - Comment: DNS name for my instance. - Name: !Join ['', [!Ref 'SubdomainDefault', ., !Ref 'HostedZoneName']] - Type: A - TTL: '900' - ResourceRecords: - - !GetAtt Infra1.PrivateIp - WildcardRecord: - Type: AWS::Route53::RecordSet - Properties: - HostedZoneName: !Ref 'HostedZoneName' - Comment: DNS name for my instance. - Name: !Join ['', [!Ref 'SubdomainWild', ., !Ref 'HostedZoneName']] - Type: A - TTL: '900' - ResourceRecords: - - !GetAtt Infra1.PrivateIp - "###; - - let rules = r#" - let aws_route53_recordset_resources = Resources.*[ Type == 'AWS::Route53::RecordSet' ] - rule aws_route53_recordset when %aws_route53_recordset_resources !empty { - %aws_route53_recordset_resources.Properties.Comment == "DNS name for my instance." - let targets = [["",["SubdomainWild",".","HostedZoneName"]], ["",["SubdomainInternal",".","HostedZoneName"]], ["",["SubdomainMaster",".","HostedZoneName"]], ["",["SubdomainDefault",".","HostedZoneName"]]] - %aws_route53_recordset_resources.Properties.Name IN %targets - %aws_route53_recordset_resources.Properties.Type == "A" - %aws_route53_recordset_resources.Properties.ResourceRecords IN [["Master.PrivateIp"], ["Infra1.PrivateIp"]] - %aws_route53_recordset_resources.Properties.TTL == "900" - %aws_route53_recordset_resources.Properties.HostedZoneName == "HostedZoneName" - } - "#; - - let value = PathAwareValue::try_from(serde_yaml::from_str::(template)?)?; - let rule_eval = RulesFile::try_from(rules)?; - let context = RootScope::new(&rule_eval, &value)?; - let status = rule_eval.evaluate(&value, &context)?; - assert_eq!(status, Status::PASS); - - Ok(()) -} diff --git a/guard/src/rules/exprs.rs b/guard/src/rules/exprs.rs index d44fd5d9e..2da087be6 100644 --- a/guard/src/rules/exprs.rs +++ b/guard/src/rules/exprs.rs @@ -341,6 +341,12 @@ impl<'loc> std::fmt::Display for GuardAccessClause<'loc> { } } +impl<'loc> std::fmt::Display for GuardNamedRuleClause<'loc> { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + write!(f, "Rule({}@{})", self.dependent_rule, self.location) + } +} + impl<'loc> std::fmt::Display for AccessClause<'loc> { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { write!( diff --git a/guard/src/rules/mod.rs b/guard/src/rules/mod.rs index 0bcbee8fd..1f4ae5206 100644 --- a/guard/src/rules/mod.rs +++ b/guard/src/rules/mod.rs @@ -3,7 +3,6 @@ pub(crate) mod display; pub(crate) mod errors; pub(crate) mod eval; pub(crate) mod eval_context; -pub(crate) mod evaluate; pub(crate) mod exprs; pub(crate) mod functions; mod libyaml; diff --git a/guard/src/rules/path_value.rs b/guard/src/rules/path_value.rs index 5b99e7564..887240eb1 100644 --- a/guard/src/rules/path_value.rs +++ b/guard/src/rules/path_value.rs @@ -8,17 +8,11 @@ use std::convert::{TryFrom, TryInto}; use serde::{Deserialize, Serialize, Serializer}; use std::fmt::Formatter; -use crate::rules::evaluate::{resolve_query, AutoReport}; -use crate::rules::EvaluationType; - use super::errors::Error; -use super::exprs::{QueryPart, SliceDisplay}; -use super::{Evaluate, EvaluationContext, Status}; // // Local mod // use super::values::*; -use crate::rules::exprs::LetValue; use fancy_regex::Regex; use serde::ser::{SerializeMap, SerializeStruct}; use std::hash::{Hash, Hasher}; @@ -587,280 +581,6 @@ impl<'a> TryInto<(String, serde_json::Value)> for &'a PathAwareValue { } } -pub(crate) trait QueryResolver { - fn select( - &self, - all: bool, - query: &[QueryPart<'_>], - eval: &dyn EvaluationContext, - ) -> Result, Error>; -} - -impl QueryResolver for PathAwareValue { - fn select( - &self, - all: bool, - query: &[QueryPart<'_>], - resolver: &dyn EvaluationContext, - ) -> Result, Error> { - if query.is_empty() { - return Ok(vec![self]); - } - - match &query[0] { - QueryPart::This => self.select(all, &query[1..], resolver), - - QueryPart::Key(key) => { - match key.parse::() { - Ok(index) => match self { - PathAwareValue::List((_, list)) => { - PathAwareValue::retrieve_index(self, index, list, query).map_or_else( - |e| self.map_error_or_empty(all, e), - |val| val.select(all, &query[1..], resolver), - ) - } - - _ => self.map_some_or_error_all(all, query), - }, - - Err(_) => match self { - PathAwareValue::Map((path, map)) => { - // - // Variable interpolation support. - // - if query[0].is_variable() { - let var = query[0].variable().unwrap(); - let keys = resolver.resolve_variable(var)?; - let mut acc = Vec::with_capacity(keys.len()); - let keys = if query.len() > 1 { - match query[1] { - QueryPart::AllIndices(_) | QueryPart::Key(_) => keys, - QueryPart::Index(index) => { - let check = if index >= 0 { index } else { -index } as usize; - if check < keys.len() { - vec![keys[check]] - } else { - self.map_some_or_error_all(all, query)? - } - }, - - _ => return Err(Error::IncompatibleError( - format!("THIS type of variable interpolation is not supported {}, {}", self.type_info(), SliceDisplay(query)) - )) - } - } else { - keys - }; - for each_key in keys { - if let PathAwareValue::String((_, k)) = each_key { - if let Some(next) = map.values.get(k) { - acc.extend(next.select(all, &query[1..], resolver)?); - } else if all { - return Err(Error:: - RetrievalError( - format!("Could not locate key = {} inside object/map = {:?}, Path = {}, remaining query = {}", - key, self, path, SliceDisplay(query)) - )); - } - } else { - return Err(Error - ::NotComparable( - format!("Variable projections inside Query {}, is returning a non-string value for key {}, {:?}", - SliceDisplay(query), - each_key.type_info(), - each_key.self_value() - ) - - )); - } - } - Ok(acc) - } else if let Some(next) = map.values.get(key) { - next.select(all, &query[1..], resolver) - } else { - self.map_some_or_error_all(all, query) - } - } - - _ => self.map_some_or_error_all(all, query), - }, - } - } - - QueryPart::Index(array_idx) => match self { - PathAwareValue::List((_path, vec)) => { - PathAwareValue::retrieve_index(self, *array_idx, vec, query).map_or_else( - |e| self.map_error_or_empty(all, e), - |val| val.select(all, &query[1..], resolver), - ) - } - - _ => self.map_some_or_error_all(all, query), - }, - - QueryPart::AllIndices(_name) => { - match self { - PathAwareValue::List((_path, elements)) => { - PathAwareValue::accumulate(self, all, &query[1..], elements, resolver) - } - - // - // Often in the place where a list of values is accepted - // single values often are accepted. So proceed to the next - // part of your query - // - rest => rest.select(all, &query[1..], resolver), - } - } - - QueryPart::AllValues(_name) => { - match self { - // - // Supporting old format - // - PathAwareValue::List((_path, elements)) => { - PathAwareValue::accumulate(self, all, &query[1..], elements, resolver) - } - - PathAwareValue::Map((_path, map)) => { - let values: Vec<&PathAwareValue> = map.values.values().collect(); - let mut resolved = Vec::with_capacity(values.len()); - for each in values { - resolved.extend(each.select(all, &query[1..], resolver)?); - } - Ok(resolved) - } - - // - // Often in the place where a list of values is accepted - // single values often are accepted. So proceed to the next - // part of your query - // - rest => rest.select(all, &query[1..], resolver), - } - } - - QueryPart::MapKeyFilter(_name, filter) => match self { - PathAwareValue::Map((_, map)) => { - let mut selected = Vec::with_capacity(map.values.len()); - match &filter.compare_with { - LetValue::AccessClause(query) => { - let values = resolve_query(false, &query.query, self, resolver)?; - for key in map.keys.iter() { - if values.contains(&key) { - match key { - PathAwareValue::String((_, v)) => { - selected.push(map.values.get(v).unwrap()); - } - _ => unreachable!(), - } - } - } - } - - LetValue::Value(path_value) => { - for key in map.keys.iter() { - if key == path_value { - match key { - PathAwareValue::String((_, v)) => { - selected.push(map.values.get(v).unwrap()); - } - _ => unreachable!(), - } - } - } - } - - LetValue::FunctionCall(_) => unreachable!(), - }; - if query.len() > 1 { - let mut acc = Vec::with_capacity(selected.len()); - for each in selected { - acc.extend(each.select(all, &query[1..], resolver)?) - } - Ok(acc) - } else { - Ok(selected) - } - } - - _ => self.map_some_or_error_all(all, query), - }, - - QueryPart::Filter(_name, conjunctions) => { - match self { - PathAwareValue::List((path, vec)) => { - let mut selected = Vec::with_capacity(vec.len()); - let context = format!("Path={},Type=Array", path); - for each in vec { - let mut filter = - AutoReport::new(EvaluationType::Filter, resolver, &context); - match conjunctions.evaluate(each, resolver) { - Err(Error::RetrievalError(e)) => { - if all { - return Err(Error::RetrievalError(e)); - } - // Else treat is like a filter - } - Err(Error::IncompatibleRetrievalError(e)) => { - if all { - return Err(Error::IncompatibleRetrievalError(e)); - } - // Else treat is like a filter - } - Err(e) => return Err(e), - Ok(status) => match status { - Status::PASS => { - filter.status(Status::PASS); - let index: usize = if query.len() > 1 { - match &query[1] { - QueryPart::AllIndices(_) => 2, - _ => 1, - } - } else { - 1 - }; - selected.extend(each.select( - all, - &query[index..], - resolver, - )?); - } - rest => { - filter.status(rest); - } - }, - } - } - Ok(selected) - } - - PathAwareValue::Map((path, _map)) => { - let context = format!("Path={},Type=MapElement", path); - let mut filter = - AutoReport::new(EvaluationType::Filter, resolver, &context); - conjunctions.evaluate(self, resolver).map_or_else( - |e| self.map_error_or_empty(all, e), - |status| match status { - Status::PASS => { - filter.status(Status::PASS); - self.select(all, &query[1..], resolver) - } - rest => { - filter.status(rest); - Ok(vec![]) - } - }, - ) - } - - _ => self.map_some_or_error_all(all, query), - } - } - } - } -} - impl Serialize for PathAwareValue { fn serialize(&self, serializer: S) -> Result where @@ -930,33 +650,6 @@ impl PathAwareValue { matches!(self, PathAwareValue::Null(_)) } - fn map_error_or_empty(&self, all: bool, e: Error) -> Result, Error> { - if !all { - match e { - Error::IncompatibleRetrievalError(_) | Error::RetrievalError(_) => Ok(vec![]), - - rest => Err(rest), - } - } else { - Err(e) - } - } - - fn map_some_or_error_all( - &self, - all: bool, - query: &[QueryPart<'_>], - ) -> Result, Error> { - if all { - Err(Error::IncompatibleRetrievalError( - format!("Attempting to retrieve array index or key from map at path = {} , Type was not an array/object {}, Remaining Query = {}", - self.self_value().0, self.type_info(), SliceDisplay(query)) - )) - } else { - Ok(vec![]) - } - } - pub(crate) fn is_scalar(&self) -> bool { !self.is_list() && !self.is_map() } @@ -998,50 +691,6 @@ impl PathAwareValue { PathAwareValue::RangeChar((_path, _)) => "range(char, char)", } } - - pub(crate) fn retrieve_index<'v>( - parent: &PathAwareValue, - index: i32, - list: &'v Vec, - query: &[QueryPart<'_>], - ) -> Result<&'v PathAwareValue, Error> { - let check = if index >= 0 { index } else { -index } as usize; - if check < list.len() { - Ok(&list[check]) - } else { - Err(Error:: - RetrievalError( - format!("Array Index out of bounds for path = {} on index = {} inside Array = {:?}, remaining query = {}", - parent.self_path(), index, list, SliceDisplay(query)) - )) - } - } - - pub(crate) fn accumulate<'v>( - parent: &PathAwareValue, - all: bool, - query: &[QueryPart<'_>], - elements: &'v Vec, - resolver: &dyn EvaluationContext, - ) -> Result, Error> { - if elements.is_empty() && !query.is_empty() && all { - return Err(Error::RetrievalError(format!( - "No entries for path = {} . Remaining Query {}", - parent.self_path(), - SliceDisplay(query) - ))); - } - - let mut accumulated = Vec::with_capacity(elements.len()); - for each in elements { - if !query.is_empty() { - accumulated.extend(each.select(all, query, resolver)?); - } else { - accumulated.push(each); - } - } - Ok(accumulated) - } } fn compare_values(first: &PathAwareValue, other: &PathAwareValue) -> Result { diff --git a/guard/src/rules/path_value_tests.rs b/guard/src/rules/path_value_tests.rs index 623a19cbd..67a9406c7 100644 --- a/guard/src/rules/path_value_tests.rs +++ b/guard/src/rules/path_value_tests.rs @@ -1,6 +1,3 @@ -use crate::rules::exprs::{ - AccessClause, AccessQuery, FileLocation, GuardAccessClause, GuardClause, LetExpr, LetValue, -}; use pretty_assertions::assert_eq; use super::*; @@ -114,262 +111,6 @@ fn path_value_equivalent() -> Result<(), Error> { Ok(()) } -struct DummyEval {} -impl EvaluationContext for DummyEval { - fn resolve_variable(&self, _variable: &str) -> crate::rules::Result> { - unimplemented!() - } - - fn rule_status(&self, _rule_name: &str) -> crate::rules::Result { - unimplemented!() - } - - fn end_evaluation( - &self, - _eval_type: EvaluationType, - _context: &str, - _msg: String, - _from: Option, - _to: Option, - _status: Option, - _cmp: Option<(CmpOperator, bool)>, - ) { - } - - fn start_evaluation(&self, _eval_type: EvaluationType, _context: &str) {} -} - -#[test] -fn path_value_queries() -> Result<(), Error> { - let resources = r#"{ - "Resources": { - "NewSecurityGroupACA21D0A": { - "Type": "AWS::EC2::SecurityGroup", - "Properties": { - "GroupDescription": "Allow ssh access to ec2 instances", - "SecurityGroupEgress": [ - { - "CidrIp": "0.0.0.0/0", - "Description": "Allow all outbound traffic by default", - "IpProtocol": "-1" - } - ], - "SecurityGroupIngress": [ - { - "CidrIp": "0.0.0.0/0", - "Description": "allow ssh access from the world", - "FromPort": 22, - "IpProtocol": "tcp", - "ToPort": 22 - } - ], - "VpcId": { - "Ref": "TheVPC92636AB0" - } - }, - "Metadata": { - "aws:cdk:path": "FtCdkSecurityGroupStack/NewSecurityGroup/Resource" - } - }, - "myInstanceUsingNewSG": { - "Type": "AWS::EC2::Instance", - "Properties": { - "ImageId": " ami-0f5dbc86dd9cbf7a8", - "InstanceType": "t2.micro", - "NetworkInterfaces": [ - { - "DeviceIndex": "0", - "SubnetId": { - "Ref": "TheVPCapplicationSubnet1Subnet2149DB21" - } - } - ], - "SecurityGroupIds": [ - { - "Fn::GetAtt": [ - "NewSecurityGroupACA21D0A", - "GroupId" - ] - } - ], - "Tags": [ - { - "Key": "Name", - "Value": "my-new-ec2-myInstanceUsingNewSG" - } - ] - }, - "Metadata": { - "aws:cdk:path": "FtCdkSecurityGroupStack/myInstanceUsingNewSG" - } - } - } - } - "#; - - let incoming = PathAwareValue::try_from(resources)?; - let eval = DummyEval {}; - // - // Select all resources that have security groups present as a property - // - let resources_with_sgs = - AccessQuery::try_from("Resources.*[ Properties.SecurityGroups EXISTS ]")?; - let selected = incoming.select( - resources_with_sgs.match_all, - &resources_with_sgs.query, - &eval, - )?; - assert!(selected.is_empty()); - - let resources_with_sgs = - AccessQuery::try_from("Resources.*[ Properties.SecurityGroupIds EXISTS ]")?; - let selected = incoming.select( - resources_with_sgs.match_all, - &resources_with_sgs.query, - &eval, - )?; - assert!(!selected.is_empty()); - - let get_att_refs = r#"Resources.*[ Properties.SecurityGroupIds EXISTS ].Properties.SecurityGroupIds[ 'Fn::GetAtt' EXISTS ].'Fn::GetAtt'.*"#; - let resources_with_sgs = AccessQuery::try_from(get_att_refs)?; - let selected = incoming.select( - resources_with_sgs.match_all, - &resources_with_sgs.query, - &eval, - )?; - assert_eq!(selected.len(), 2); - - let get_att_refs = r#"SOME Resources.*.Properties.SecurityGroupIds[*].'Fn::GetAtt'.*"#; - let resources_with_sgs = AccessQuery::try_from(get_att_refs)?; - let selected = incoming.select( - resources_with_sgs.match_all, - &resources_with_sgs.query, - &eval, - )?; - assert_eq!(selected.len(), 2); - println!("{:?}", selected); - - // - // Assignments - // - let assignment = r#"let var = ANY Resources.*.Properties.SecurityGroupIds[*].'Fn::GetAtt'.*"#; - let let_statement = LetExpr::try_from(assignment)?; - println!("{:?}", let_statement); - - // - // Clauses - // - let clause = - "SOME Resources.*.Properties.SecurityGroupIds[*].'Fn::GetAtt'.* IN [/aa/, /bb/] #;"; - let clause_statement = GuardClause::try_from(clause)?; - println!("{:?}", clause_statement); - let expected = GuardClause::Clause(GuardAccessClause { - negation: false, - access_clause: AccessClause { - query: AccessQuery { - query: vec![ - QueryPart::Key(String::from("Resources")), - QueryPart::AllValues(None), - QueryPart::Key("Properties".to_string()), - QueryPart::Key("SecurityGroupIds".to_string()), - QueryPart::AllIndices(None), - QueryPart::Key("Fn::GetAtt".to_string()), - QueryPart::AllValues(None), - ], - match_all: false, - }, - compare_with: Some(LetValue::Value(PathAwareValue::try_from("[/aa/, /bb/]")?)), - location: FileLocation { - line: 1, - column: 1, - file_name: "", - }, - comparator: (CmpOperator::In, false), - custom_message: None, - }, - }); - assert_eq!(expected, clause_statement); - - Ok(()) -} - -#[test] -fn some_filter_tests() -> Result<(), Error> { - let query_str = r#"some Resources.*.Properties.SecurityGroups[*].'Fn::GetAtt'"#; - let resources_str = r#"{ - Resources: { - ec2: { - Properties: { - SecurityGroups: ["sg-1234"] - } - }, - ec22: { - Properties: { - SecurityGroups: [{ 'Fn::GetAtt': ["sg", "GroupId"] }] - } - } - } - }"#; - let query = AccessQuery::try_from(query_str)?; - let resources = PathAwareValue::try_from(resources_str)?; - let dummy = DummyEval {}; - let selected = resources.select(query.match_all, &query.query, &dummy)?; - assert_eq!(selected.len(), 1); - Ok(()) -} - -#[test] -fn it_support_evaluation_tests() -> Result<(), Error> { - let tags = r#"Tags[ this == { Key: "Hi", Value: "There" } ]"#; - let parsed_tags = AccessQuery::try_from(tags)?; - let values = r#"{ - Tags: [ - { Key: "Hi", Value: "There" }, - { Key: "NotHi", Value: "NotThere" } - ] - }"#; - let parsed_values = PathAwareValue::try_from(values)?; - let dummy = DummyEval {}; - let selected = parsed_values.select(parsed_tags.match_all, &parsed_tags.query, &dummy)?; - println!("Selected = {:?}", selected); - assert_eq!(selected.len(), 1); - match selected[0] { - PathAwareValue::Map((p, _map)) => { - assert_eq!(p, &Path::try_from("/Tags/0")?); - } - _ => unreachable!(), - } - Ok(()) -} - -#[test] -fn map_keys_filter_test() -> Result<(), Error> { - let condition_str = r#"{ - Condition: { - 'aws:SourceVpc': ['vpc-123454'], - 'aws:IsSecure': false - } - }"#; - let value = PathAwareValue::try_from(condition_str)?; - let selection_str = r#"Condition[ keys == /aws:[Ss]ource(Vpc|VPC|VpcE|VPCE)/ ]"#; - let access = AccessQuery::try_from(selection_str)?; - let dummy = DummyEval {}; - let selected = value.select(access.match_all, &access.query, &dummy)?; - println!("Selected = {:?}", selected); - assert_eq!(selected.len(), 1); - let inner = selected[0]; - if let PathAwareValue::List((p, l)) = inner { - assert_eq!(p, &Path::try_from("/Condition/aws:SourceVpc")?); - assert_eq!(l.len(), 1); - let inner = &l[0]; - if let PathAwareValue::String((p, v)) = inner { - assert_eq!(p, &Path::try_from("/Condition/aws:SourceVpc/0")?); - assert_eq!(v, "vpc-123454"); - } - } - Ok(()) -} - #[test] fn merge_values_test() -> Result<(), Error> { let resources = PathAwareValue::try_from(serde_yaml::from_str::( diff --git a/guard/src/rules/values_tests.rs b/guard/src/rules/values_tests.rs index 4fd53cf87..74010a559 100644 --- a/guard/src/rules/values_tests.rs +++ b/guard/src/rules/values_tests.rs @@ -1,14 +1,10 @@ use super::*; -use crate::rules::exprs::{AccessQuery, GuardClause}; -use crate::rules::exprs::{Rule, TypeBlock}; use pretty_assertions::assert_eq; -use std::collections::HashMap; use std::convert::{TryFrom, TryInto}; -use std::fs::read_to_string; use crate::rules::path_value::traversal::{Traversal, TraversalResult}; -use crate::rules::path_value::{PathAwareValue, QueryResolver}; -use crate::rules::{Error, Evaluate, EvaluationContext, EvaluationType, Result, Status}; +use crate::rules::path_value::PathAwareValue; +use crate::rules::Result; #[test] fn test_convert_from_to_value() -> Result<()> { @@ -129,232 +125,6 @@ fn test_convert_into_json() -> Result<()> { Ok(()) } -#[test] -fn test_query_on_value() -> Result<()> { - let content = read_to_string("assets/cfn-template.json")?; - let value = PathAwareValue::try_from(content.as_str())?; - - struct DummyResolver<'a> { - cache: HashMap<&'a str, Vec<&'a PathAwareValue>>, - } - impl<'a> EvaluationContext for DummyResolver<'a> { - fn resolve_variable(&self, variable: &str) -> Result> { - if let Some(v) = self.cache.get(variable) { - return Ok(v.clone()); - } - Err(Error::MissingVariable(format!("Not found {}", variable))) - } - - fn rule_status(&self, _rule_name: &str) -> Result { - unimplemented!() - } - - fn end_evaluation( - &self, - _eval_type: EvaluationType, - _context: &str, - _msg: String, - _from: Option, - _to: Option, - _status: Option, - _cmp: Option<(CmpOperator, bool)>, - ) { - } - - fn start_evaluation(&self, _eval_type: EvaluationType, _context: &str) {} - } - let dummy = DummyResolver { - cache: HashMap::new(), - }; - - // - // Select all resources inside a template - // - let query = AccessQuery::try_from("Resources.*")?; - let selected = value.select(query.match_all, &query.query, &dummy)?; - assert_eq!(selected.len(), 17); - for each in selected { - if let PathAwareValue::Map(_index) = each { - continue; - } - unreachable!() - } - - // - // Select all IAM::Role resources inside the template - // - let query = AccessQuery::try_from("Resources.*[ Type == \"AWS::IAM::Role\" ]")?; - let selected = value.select(query.match_all, &query.query, &dummy)?; - assert_eq!(selected.len(), 1); - - println!("{:?}", selected[0]); - let iam_role = selected[0]; - - // - // Select all policies that has Effect "allow" - // - let query = AccessQuery::try_from( - "Properties.Policies.*.PolicyDocument.Statement[ Effect == \"Allow\" ]", - )?; - let selected = iam_role.select(query.match_all, &query.query, &dummy)?; - assert_eq!(selected.len(), 2); - - // - // This is the case with IAM roles where Action can be either a single value or array - // - // let clause = GuardClause::try_from( - // "Properties.Policies.*.PolicyDocument.Statement[ Effect == \"Allow\" ].Action != \"*\"")?; - // let status = clause.evaluate(iam_role, &dummy)?; - // assert_eq!(status, Status::FAIL); - - let clause = GuardClause::try_from( - "Properties.Policies.*.PolicyDocument.Statement[ Effect == \"Allow\" ].Action.* != \"*\"", - )?; - let status = clause.evaluate(iam_role, &dummy)?; - assert_eq!(status, Status::FAIL); - - // - // Making it work with variable references - // - let block = r#" - AWS::IAM::Role { - let statements = Properties.Policies.*.PolicyDocument.Statement[ Effect == "Allow" ] - - # %statements.Action != "*" OR - %statements.Action.* != "*" - - %statements.Resource != "*" # OR - # %statements.Resource.* != "*" - } - "#; - let type_block = TypeBlock::try_from(block)?; - let status = type_block.evaluate(&value, &dummy)?; - assert_eq!(status, Status::FAIL); - - Ok(()) -} - -#[test] -fn test_type_block_with_var_query_evaluation() -> Result<()> { - let content = read_to_string("assets/cfn-template.json")?; - let value = PathAwareValue::try_from(content.as_str())?; - - struct DummyResolver {} - impl EvaluationContext for DummyResolver { - fn resolve_variable(&self, _variable: &str) -> Result> { - unimplemented!() - } - - fn rule_status(&self, _rule_name: &str) -> Result { - unimplemented!() - } - - fn end_evaluation( - &self, - _eval_type: EvaluationType, - _context: &str, - _msg: String, - _from: Option, - _to: Option, - _status: Option, - _cmp: Option<(CmpOperator, bool)>, - ) { - } - - fn start_evaluation(&self, _eval_type: EvaluationType, _context: &str) {} - } - let dummy = DummyResolver {}; - - let block = r#" - rule check_subnets when Resources.*[ Type == "AWS::EC2::VPC" ] !EMPTY { - # Ensure that Zone is always set - AWS::EC2::Subnet Properties.AvailabilityZone NOT EMPTY - - # Check if either IPv6 is correctly on or IPv4 - AWS::EC2::Subnet { - Properties.AssignIpv6AddressOnCreation EXISTS - Properties.AssignIpv6AddressOnCreation == true - Properties.Ipv6CidrBlock EXISTS - Properties.CidrBlock NOT EXISTS - } OR - AWS::EC2::Subnet { - Properties.AssignIpv6AddressOnCreation !EXISTS or - Properties.AssignIpv6AddressOnCreation == false - Properties.CidrBlock EXISTS - Properties.Ipv6CidrBlock NOT EXISTS - } - } - "#; - let rule = Rule::try_from(block)?; - let status = rule.evaluate(&value, &dummy)?; - println!("Status = {:?}", status); - assert_eq!(status, Status::PASS); - - let block = r###" - rule check_subnets { - # Ensure that Zone is always set - AWS::EC2::Subnet Properties.AvailabilityZone NOT EMPTY - - # Check if either IPv6 is correctly on or IPv4 - AWS::EC2::Subnet { - Properties.AssignIpv6AddressOnCreation EXISTS - Properties.AssignIpv6AddressOnCreation == true - Properties.Ipv6CidrBlock EXISTS - Properties.CidrBlock NOT EXISTS - } OR - AWS::EC2::Subnet { - Properties.AssignIpv6AddressOnCreation !EXISTS or - Properties.AssignIpv6AddressOnCreation == false - Properties.CidrBlock EXISTS - Properties.Ipv6CidrBlock NOT EXISTS - } - } - "###; - let rule = Rule::try_from(block)?; - let status = rule.evaluate(&value, &dummy)?; - println!("Status = {:?}", status); - assert_eq!(status, Status::PASS); - - let content = r#" - { - "Resources": { - "subnet": { - "Type": "AWS::EC2::Subnet", - "Properties": { - "AvailabilityZone": "us-east-2a", - "AssignIpv6AddressOnCreation": true, - "CidrBlock": "10.0.0.0/12" - } - } - } - } - "#; - let value = PathAwareValue::try_from(content)?; - let status = rule.evaluate(&value, &dummy)?; - println!("Status = {:?}", status); - assert_eq!(status, Status::FAIL); - - let content = r#" - { - "Resources": { - "subnet": { - "Type": "AWS::EC2::Subnet", - "Properties": { - "AvailabilityZone": "us-east-2a", - "CidrBlock": "10.0.0.0/12" - } - } - } - } - "#; - let value = PathAwareValue::try_from(content)?; - let status = rule.evaluate(&value, &dummy)?; - println!("Status = {:?}", status); - assert_eq!(status, Status::PASS); - - Ok(()) -} - #[test] fn test_parse_string_with_colon() -> Result<()> { // let s = r#"'aws:AssumeRole'"#; From 144baac0b74299edc1d9fbced751d17a645c0f5e Mon Sep 17 00:00:00 2001 From: Madison Steiner Date: Wed, 12 Aug 2026 15:09:00 +0000 Subject: [PATCH 02/10] Make verbose output independent of the compiler that built it eval_conjunction_clauses built a user-visible string from std::any::type_name::(): `context` becomes the `Context=` label on the Disjunction node in --verbose output, and four fixtures under guard/resources pin it exactly. type_name's output is explicitly unspecified -- std documents that it "must not be considered to uniquely identify a type" and may change between compiler versions, and it did. Newer rustc renders the elided lifetime, so cfn_guard::rules::exprs::GuardClause became ...::GuardClause<'_>, and test_data_file_verbose and test_with_rules_dir_verbose fail on any toolchain other than the 1.77.2 pinned in rust-toolchain.toml. Verified failing at both 320251c and 57bbdbf (upstream main) under rustc 1.97, so it is pre-existing and not introduced by this branch. Truncating at the first `<` restores the pre-change spelling for every T and is stable under further rendering changes, since only the generic/lifetime portion varies. Deliberately not changing the strings themselves: that a Rust module path is user-facing output at all is a real wart, but rewriting it is a visible output change that belongs with the fixtures. Also removes the EvaluationContext and Evaluate traits, which the previous commit left with no production implementations. `cargo clippy -- -D warnings` -- the gate at pr.yml:94 -- rejects `trait EvaluationContext is never used`, so leaving them would have failed CI. Differential clippy against upstream main showed this as the only new lint the branch introduces. Removing them takes with them: - StackTracker, the old evaluator's recorder, and StatusContext::new, its only caller. StatusContext itself stays: the validate reporters still destructure it and generic_summary.rs is live. - common_test_helpers.rs, whose only content was a DummyEval implementing the trait - a DummyEval in parser_tests.rs, constructed once into `let _dummy` and never used 324 lib tests, 0 failed, 3 ignored. test_command 19/19 -- both *_verbose tests now pass, where they were 17/2 before. --- guard/src/commands/common_test_helpers.rs | 27 ------- guard/src/commands/mod.rs | 1 - guard/src/commands/tracker.rs | 97 +++-------------------- guard/src/rules/eval.rs | 24 +++++- guard/src/rules/mod.rs | 27 ------- guard/src/rules/parser_tests.rs | 27 ------- 6 files changed, 33 insertions(+), 170 deletions(-) delete mode 100644 guard/src/commands/common_test_helpers.rs diff --git a/guard/src/commands/common_test_helpers.rs b/guard/src/commands/common_test_helpers.rs deleted file mode 100644 index b717205d9..000000000 --- a/guard/src/commands/common_test_helpers.rs +++ /dev/null @@ -1,27 +0,0 @@ -use crate::rules::values::CmpOperator; -use crate::rules::{path_value::PathAwareValue, EvaluationContext, EvaluationType, Result, Status}; - -pub(super) struct DummyEval {} -impl EvaluationContext for DummyEval { - fn resolve_variable(&self, _variable: &str) -> Result> { - unimplemented!() - } - - fn rule_status(&self, _rule_name: &str) -> Result { - unimplemented!() - } - - fn end_evaluation( - &self, - _eval_type: EvaluationType, - _context: &str, - _msg: String, - _from: Option, - _to: Option, - _status: Option, - _cmp: Option<(CmpOperator, bool)>, - ) { - } - - fn start_evaluation(&self, _eval_type: EvaluationType, _context: &str) {} -} diff --git a/guard/src/commands/mod.rs b/guard/src/commands/mod.rs index bae10680d..d2cdbd71a 100644 --- a/guard/src/commands/mod.rs +++ b/guard/src/commands/mod.rs @@ -15,7 +15,6 @@ pub mod rulegen; pub mod test; pub mod validate; -mod common_test_helpers; pub mod completions; pub mod reporters; mod tracker; diff --git a/guard/src/commands/tracker.rs b/guard/src/commands/tracker.rs index 462c462e2..f264d9c06 100644 --- a/guard/src/commands/tracker.rs +++ b/guard/src/commands/tracker.rs @@ -1,8 +1,16 @@ use crate::rules::values::CmpOperator; -use crate::rules::{path_value::PathAwareValue, EvaluationContext, EvaluationType, Result, Status}; -use nom::lib::std::fmt::Formatter; +use crate::rules::{path_value::PathAwareValue, EvaluationType, Status}; use serde::Serialize; +/// The per-clause record the validate reporters read. +/// +/// Nothing constructs one any more -- `StackTracker`, the `EvaluationContext` implementation +/// that built these, was the old evaluator's recorder and went with it. The type stays +/// because the reporters still destructure it: `common.rs`, `cfn_reporter.rs` and +/// `generic_summary.rs` match on `eval_type` and walk `children`, and `generic_summary.rs` is +/// live (constructed at `helper.rs` and `validate.rs`). The new evaluator records through +/// `RecordType`/`EventRecord` in `eval_context.rs` instead, so those reporter branches are +/// unreachable rather than wrong. #[derive(Serialize, Debug)] pub(crate) struct StatusContext { pub(crate) eval_type: EvaluationType, @@ -14,88 +22,3 @@ pub(crate) struct StatusContext { pub(crate) comparator: Option<(CmpOperator, bool)>, pub(crate) children: Vec, } - -impl StatusContext { - fn new(eval_type: EvaluationType, context: &str) -> Self { - StatusContext { - eval_type, - context: context.to_string(), - status: None, - msg: None, - from: None, - to: None, - comparator: None, - children: vec![], - } - } -} - -pub(crate) struct StackTracker<'r> { - root_context: &'r dyn EvaluationContext, - stack: std::cell::RefCell>, -} - -impl<'r> std::fmt::Debug for StackTracker<'r> { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - self.stack.borrow().fmt(f) - } -} - -impl<'r> EvaluationContext for StackTracker<'r> { - fn resolve_variable(&self, variable: &str) -> Result> { - self.root_context.resolve_variable(variable) - } - - fn rule_status(&self, rule_name: &str) -> Result { - self.root_context.rule_status(rule_name) - } - - fn end_evaluation( - &self, - eval_type: EvaluationType, - context: &str, - msg: String, - from: Option, - to: Option, - status: Option, - cmp: Option<(CmpOperator, bool)>, - ) { - if self.stack.borrow().len() == 1 { - match self.stack.borrow_mut().get_mut(0) { - Some(top) => { - top.status = status; - top.from = from; - top.to = to; - top.msg = Some(msg); - top.comparator = cmp; - } - None => unreachable!(), - } - return; - } - - let stack = self.stack.borrow_mut().pop(); - if let Some(mut stack) = stack { - stack.status = status; - stack.from = from.clone(); - stack.to = to.clone(); - stack.msg = Some(msg.clone()); - stack.comparator = cmp; - - match self.stack.borrow_mut().last_mut() { - Some(cxt) => cxt.children.push(stack), - None => unreachable!(), - } - } - self.root_context - .end_evaluation(eval_type, context, msg, from, to, status, cmp); - } - - fn start_evaluation(&self, eval_type: EvaluationType, context: &str) { - let _indent = self.stack.borrow().len(); - self.stack - .borrow_mut() - .push(StatusContext::new(eval_type, context)); - self.root_context.start_evaluation(eval_type, context); - } -} diff --git a/guard/src/rules/eval.rs b/guard/src/rules/eval.rs index ad5d47ba0..18214debe 100644 --- a/guard/src/rules/eval.rs +++ b/guard/src/rules/eval.rs @@ -1979,7 +1979,29 @@ where Ok(loop { let mut num_passes = 0; let mut num_fails = 0; - let context = format!("{}#disjunction", std::any::type_name::()); + // `std::any::type_name` is the source of a user-visible string here: `context` + // becomes the `Context=` label on the Disjunction node in `--verbose` output, and + // four fixtures under `guard/resources` pin it exactly. + // + // Its output is explicitly unspecified -- std documents that it "must not be + // considered to uniquely identify a type" and may change between compiler versions, + // and it did. Newer rustc renders the elided lifetime, so + // `cfn_guard::rules::exprs::GuardClause` became `...::GuardClause<'_>` and both + // `*_verbose` fixture tests fail on any toolchain other than the 1.77.2 pinned in + // rust-toolchain.toml. That made cfn-guard's own output depend on which compiler + // built it. + // + // Truncating at the first `<` restores the pre-change spelling for every `T` and is + // stable under further rendering changes, since only the generic/lifetime portion + // varies. The deeper problem -- that a Rust module path is user-facing output at all, + // which no rule author can act on -- is left alone deliberately: changing these + // strings is a visible output change and belongs with the fixtures, not in a fix for + // a build-environment-dependent failure. + let type_name = std::any::type_name::(); + let type_name = type_name + .split_once('<') + .map_or(type_name, |(bare, _generics)| bare); + let context = format!("{}#disjunction", type_name); 'conjunction: for conjunction in conjunctions { let mut num_of_disjunction_fails = 0; let multiple_ors_present = conjunction.len() > 1; diff --git a/guard/src/rules/mod.rs b/guard/src/rules/mod.rs index 1f4ae5206..48f265c81 100644 --- a/guard/src/rules/mod.rs +++ b/guard/src/rules/mod.rs @@ -378,33 +378,6 @@ pub(crate) trait EvalContext<'value, 'loc: 'value>: RecordTracer<'value> { } } -pub(crate) trait EvaluationContext { - fn resolve_variable(&self, variable: &str) -> Result>; - - fn rule_status(&self, rule_name: &str) -> Result; - - #[allow(clippy::too_many_arguments)] - fn end_evaluation( - &self, - eval_type: EvaluationType, - context: &str, - msg: String, - from: Option, - to: Option, - status: Option, - comparator: Option<(CmpOperator, bool)>, - ); - - fn start_evaluation(&self, eval_type: EvaluationType, context: &str); -} - -pub(crate) trait Evaluate { - fn evaluate<'s>( - &self, - context: &'s PathAwareValue, - var_resolver: &'s dyn EvaluationContext, - ) -> Result; -} pub fn short_form_to_long(fn_ref: &str) -> &'static str { match SHORT_FORM_TO_LONG_MAPPING.get(fn_ref) { diff --git a/guard/src/rules/parser_tests.rs b/guard/src/rules/parser_tests.rs index 334720a7b..65432d963 100644 --- a/guard/src/rules/parser_tests.rs +++ b/guard/src/rules/parser_tests.rs @@ -1,6 +1,5 @@ use crate::rules::path_value::PathAwareValue; use crate::rules::values::WithinRange; -use crate::rules::{EvaluationContext, EvaluationType, Status}; use pretty_assertions::assert_eq; use std::vec; @@ -3317,31 +3316,6 @@ fn test_complex_predicate_clauses() -> Result<(), Error> { Ok(()) } -struct DummyEval {} -impl EvaluationContext for DummyEval { - fn resolve_variable(&self, _variable: &str) -> crate::rules::Result> { - unimplemented!() - } - - fn rule_status(&self, _rule_name: &str) -> crate::rules::Result { - unimplemented!() - } - - fn end_evaluation( - &self, - _eval_type: EvaluationType, - _context: &str, - _msg: String, - _from: Option, - _to: Option, - _status: Option, - _cmp: Option<(CmpOperator, bool)>, - ) { - } - - fn start_evaluation(&self, _eval_type: EvaluationType, _context: &str) {} -} - #[test] fn select_any_one_from_list_clauses() -> Result<(), Error> { let clause = "this == /\\{\\{resolve:secretsmanager/"; @@ -3406,7 +3380,6 @@ fn select_any_one_from_list_clauses() -> Result<(), Error> { "#, ]; - let _dummy = DummyEval {}; let _clause = GuardClause::try_from( r#"Resources.*[ this.Type == "AWS::RDS::DBInstance" ].Properties.MasterUserPassword.'Fn::Join'[1][ this == /\{\{resolve:secretsmanager/ ] !EMPTY"#, )?; From e3fea728ce7297689cf51f6ba04c83f90f5a992f Mon Sep 17 00:00:00 2001 From: Madison Steiner Date: Wed, 12 Aug 2026 15:11:49 +0000 Subject: [PATCH 03/10] Apply rustfmt Formatting only, no behaviour change. `cargo fmt --check` is a CI gate (pr.yml:46-54, actions-rust-lang/rustfmt@v1) and this branch was failing it. Verified this is genuinely unformatted branch code and not a rustfmt version artifact: upstream main at 57bbdbf passes `cargo fmt --check` cleanly under the same rustfmt 1.97, so the only files it can rewrite are ones this stack changed. Two of the six files -- eval_context.rs and outcome_tests.rs -- come from feat/status-type-migration rather than from this branch, so that branch is failing the same gate on its own. 324 lib tests, 0 failed. cargo fmt --check exit 0. --- guard/src/rules/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/guard/src/rules/mod.rs b/guard/src/rules/mod.rs index 48f265c81..5fac73231 100644 --- a/guard/src/rules/mod.rs +++ b/guard/src/rules/mod.rs @@ -378,7 +378,6 @@ pub(crate) trait EvalContext<'value, 'loc: 'value>: RecordTracer<'value> { } } - pub fn short_form_to_long(fn_ref: &str) -> &'static str { match SHORT_FORM_TO_LONG_MAPPING.get(fn_ref) { Some(fn_ref) => fn_ref, From 4cab3728141d99895482c159dcf77657b4052ddf Mon Sep 17 00:00:00 2001 From: Madison Steiner Date: Wed, 12 Aug 2026 16:33:46 +0000 Subject: [PATCH 04/10] Remove the dead Reporter::report method `Reporter` had two methods: `report`, taking `&[&StatusContext]`, and `report_eval`, taking an `EventRecord`. Only the second is called. `report` was the old evaluator's reporting entry point and nothing has invoked it since the new recorder replaced it -- several implementations were already `_`-prefixed stubs returning `Ok(())`, which is what a vestigial required method decays into. Removed the trait method and its eight implementations. Two traits in the same files also have a `report`, and both are live: `GenericReporter::report` and the `report(&mut self) -> Result` on the structured reporters. They are kept, and the removal was filtered on whether the parameter list mentions `StatusContext` rather than on the method name, because name-matching removed live impls twice while writing this. This orphans the legacy reporting cluster rather than removing it: StatusContext and EvaluationType are now unreferenced except by each other, along with find_all_failing_clauses, extract_name_info, print_partition, print_compliant_skipped_info, pprint_failed_sub_tree, and the CfnReporter / SingleLineReporter / ConsoleReporter / StructuredSummary / DataOutput / DataOutputNewForm / StructureType / SarifRule set. Deleting that cluster is the follow-up; splitting it out keeps this commit to one reviewable question. 329 lib tests, 0 failed, 1 ignored. cargo fmt --check clean. Zero new clippy lints against upstream main. --- guard/src/commands/files.rs | 2 +- .../src/commands/reporters/test/structured.rs | 4 +- guard/src/commands/reporters/validate/cfn.rs | 18 +-- .../reporters/validate/cfn_reporter.rs | 109 +----------------- .../reporters/validate/console_reporter.rs | 16 --- .../reporters/validate/generic_summary.rs | 92 +-------------- .../reporters/validate/summary_table.rs | 74 ------------ guard/src/commands/reporters/validate/tf.rs | 18 +-- guard/src/commands/validate.rs | 14 --- guard/src/rules/parser.rs | 12 +- guard/src/rules/path_value/traversal.rs | 4 +- guard/src/utils/mod.rs | 2 +- 12 files changed, 16 insertions(+), 349 deletions(-) diff --git a/guard/src/commands/files.rs b/guard/src/commands/files.rs index 372539df4..81ab893ee 100644 --- a/guard/src/commands/files.rs +++ b/guard/src/commands/files.rs @@ -68,7 +68,7 @@ where } } -pub(crate) fn iterate_over(files: &[PathBuf], converter: C) -> Iter +pub(crate) fn iterate_over(files: &[PathBuf], converter: C) -> Iter<'_, T, C> where C: Fn(String, &PathBuf) -> Result, { diff --git a/guard/src/commands/reporters/test/structured.rs b/guard/src/commands/reporters/test/structured.rs index 03f9d3f1a..712150faa 100644 --- a/guard/src/commands/reporters/test/structured.rs +++ b/guard/src/commands/reporters/test/structured.rs @@ -66,7 +66,7 @@ impl TestResult { } } - pub fn build_test_suite(&self) -> TestSuite { + pub fn build_test_suite(&self) -> TestSuite<'_> { match self { TestResult::Err(Err { rule_file, @@ -136,7 +136,7 @@ impl TestCase { self.failed_rules.len() } - fn build_junit_test_cases(&self) -> Vec { + fn build_junit_test_cases(&self) -> Vec> { let mut test_cases = vec![]; for test_case in &self.passed_rules { diff --git a/guard/src/commands/reporters/validate/cfn.rs b/guard/src/commands/reporters/validate/cfn.rs index 81af2e25e..afbf654a9 100644 --- a/guard/src/commands/reporters/validate/cfn.rs +++ b/guard/src/commands/reporters/validate/cfn.rs @@ -14,7 +14,6 @@ use crate::{ reporters::validate::common::{ populate_hierarchy_path_trees, IdentityHash, LocalResourceAggr, PathTree, RuleHierarchy, }, - tracker::StatusContext, validate::{OutputFormatType, Reporter}, }, rules::{ @@ -46,27 +45,12 @@ pub(crate) struct CfnAware<'reporter> { } impl<'reporter> CfnAware<'reporter> { - pub(crate) fn new_with(next: &'reporter dyn Reporter) -> CfnAware { + pub(crate) fn new_with(next: &'reporter dyn Reporter) -> CfnAware<'reporter> { CfnAware { next: Some(next) } } } impl<'reporter> Reporter for CfnAware<'reporter> { - fn report( - &self, - _writer: &mut dyn Write, - _status: Option, - _failed_rules: &[&StatusContext], - _passed_or_skipped: &[&StatusContext], - _longest_rule_name: usize, - _rules_file: &str, - _data_file: &str, - _data: &Traversal<'_>, - _output_format_type: OutputFormatType, - ) -> rules::Result<()> { - Ok(()) - } - fn report_eval<'value>( &self, write: &mut dyn Write, diff --git a/guard/src/commands/reporters/validate/cfn_reporter.rs b/guard/src/commands/reporters/validate/cfn_reporter.rs index 384d13682..8a6aa4bc1 100644 --- a/guard/src/commands/reporters/validate/cfn_reporter.rs +++ b/guard/src/commands/reporters/validate/cfn_reporter.rs @@ -6,15 +6,12 @@ use fancy_regex::Regex; use lazy_static::*; use crate::commands::reporters::validate::common::{ - find_all_failing_clauses, GenericReporter, NameInfo, StructureType, StructuredSummary, + GenericReporter, NameInfo, StructureType, StructuredSummary, }; -use crate::commands::tracker::StatusContext; use crate::commands::validate::{OutputFormatType, Reporter}; -use crate::rules::errors::Error; use crate::rules::eval_context::EventRecord; use crate::rules::path_value::traversal::Traversal; -use crate::rules::EvaluationType; use crate::rules::Status; lazy_static! { @@ -27,110 +24,6 @@ lazy_static! { pub(crate) struct CfnReporter {} impl Reporter for CfnReporter { - fn report( - &self, - writer: &mut dyn Write, - _status: Option, - failed_rules: &[&StatusContext], - passed_or_skipped: &[&StatusContext], - longest_rule_name: usize, - rules_file: &str, - data_file: &str, - _data: &Traversal<'_>, - output_format_type: OutputFormatType, - ) -> crate::rules::Result<()> { - let renderer = - match output_format_type { - OutputFormatType::SingleLineSummary => { - Box::new(SingleLineReporter {}) as Box - } - OutputFormatType::JSON => Box::new(StructuredSummary::new(StructureType::JSON)) - as Box, - OutputFormatType::YAML => Box::new(StructuredSummary::new(StructureType::YAML)) - as Box, - OutputFormatType::Junit => unreachable!(), - OutputFormatType::Sarif => unreachable!(), - }; - let failed = if !failed_rules.is_empty() { - let mut by_resource_name = HashMap::new(); - for (idx, each_failed_rule) in failed_rules.iter().enumerate() { - let failed = find_all_failing_clauses(each_failed_rule); - for (clause_idx, each_failing_clause) in failed.iter().enumerate() { - match each_failing_clause.eval_type { - EvaluationType::Clause | EvaluationType::BlockClause => { - if each_failing_clause.eval_type == EvaluationType::BlockClause { - match &each_failing_clause.msg { - Some(msg) => { - if msg.contains("DEFAULT") { - continue; - } - } - - None => { - continue; - } - } - } - let mut resource_info = super::common::extract_name_info( - &each_failed_rule.context, - each_failing_clause, - )?; - let (resource_name, property_path) = - match CFN_RESOURCES.captures(&resource_info.path) { - Ok(Some(caps)) => { - (caps["name"].to_string(), caps["rest"].replace('/', ".")) - } - Ok(None) => ( - format!( - "Rule {} Resource {} {}", - each_failed_rule.context, idx, clause_idx - ), - "".to_string(), - ), - Err(e) => return Err(Error::from(Box::new(e))), - }; - resource_info.path = property_path; - by_resource_name - .entry(resource_name) - .or_insert(Vec::new()) - .push(resource_info); - } - - _ => unreachable!(), - } - } - } - by_resource_name - } else { - HashMap::new() - }; - let as_vec = passed_or_skipped.to_vec(); - let (skipped, passed): (Vec<&StatusContext>, Vec<&StatusContext>) = - as_vec.iter().partition(|status| match status.status { - // This uses the dereference deep trait of Rust - Some(Status::SKIP) => true, - _ => false, - }); - let skipped = skipped - .iter() - .map(|s| s.context.clone()) - .collect::>(); - let passed = passed - .iter() - .map(|s| s.context.clone()) - .collect::>(); - renderer.report( - writer, - rules_file, - data_file, - failed, - passed, - skipped, - longest_rule_name, - )?; - Ok(()) - } - fn report_eval<'value>( &self, _write: &mut dyn Write, diff --git a/guard/src/commands/reporters/validate/console_reporter.rs b/guard/src/commands/reporters/validate/console_reporter.rs index 1021dc255..e129a1108 100644 --- a/guard/src/commands/reporters/validate/console_reporter.rs +++ b/guard/src/commands/reporters/validate/console_reporter.rs @@ -1,4 +1,3 @@ -use crate::commands::tracker::StatusContext; use crate::commands::validate::{OutputFormatType, Reporter}; use crate::rules::eval_context::EventRecord; use crate::rules::path_value::traversal::Traversal; @@ -309,21 +308,6 @@ fn pprint_failed_sub_tree( } impl Reporter for ConsoleReporter { - fn report( - &self, - _writer: &mut dyn Write, - _status: Option, - _failed_rules: &[&StatusContext], - _passed_or_skipped: &[&StatusContext], - _longest_rule_name: usize, - _rules_file: &str, - _data_file: &str, - _data: &Traversal<'_>, - _output_format_type: OutputFormatType, - ) -> crate::rules::Result<()> { - Ok(()) - } - fn report_eval<'value>( &self, _write: &mut dyn Write, diff --git a/guard/src/commands/reporters/validate/generic_summary.rs b/guard/src/commands/reporters/validate/generic_summary.rs index 5a4dd2b77..f114fc88b 100644 --- a/guard/src/commands/reporters/validate/generic_summary.rs +++ b/guard/src/commands/reporters/validate/generic_summary.rs @@ -4,9 +4,8 @@ use std::io::Write; use enumflags2::BitFlags; -use crate::commands::tracker::StatusContext; use crate::commands::validate::{OutputFormatType, Reporter}; -use crate::rules::{EvaluationType, Status}; +use crate::rules::Status; use super::common::*; use super::summary_table::SummaryType; @@ -26,95 +25,6 @@ impl GenericSummary { } impl Reporter for GenericSummary { - fn report( - &self, - writer: &mut dyn Write, - _: Option, - failed_rules: &[&StatusContext], - passed_or_skipped: &[&StatusContext], - longest_rule_name: usize, - rules_file: &str, - data_file: &str, - _: &Traversal<'_>, - output_format_type: OutputFormatType, - ) -> crate::rules::Result<()> { - let renderer = match output_format_type { - OutputFormatType::SingleLineSummary => Box::new(SingleLineSummary { - summary_table: self.summary_table, - }) as Box, - OutputFormatType::JSON => { - Box::new(StructuredSummary::new(StructureType::JSON)) as Box - } - OutputFormatType::YAML => { - Box::new(StructuredSummary::new(StructureType::YAML)) as Box - } - OutputFormatType::Junit => unreachable!(), - OutputFormatType::Sarif => unreachable!(), - }; - let failed = if !failed_rules.is_empty() { - let mut by_rule = HashMap::with_capacity(failed_rules.len()); - for each_failed_rule in failed_rules { - for each_failed_clause in find_all_failing_clauses(each_failed_rule) { - match each_failed_clause.eval_type { - EvaluationType::Clause | EvaluationType::BlockClause => { - if each_failed_clause.eval_type == EvaluationType::BlockClause { - match &each_failed_clause.msg { - Some(msg) => { - if msg.contains("DEFAULT") { - continue; - } - } - - None => { - continue; - } - } - } - by_rule - .entry(each_failed_rule.context.clone()) - .or_insert(Vec::new()) - .push(extract_name_info( - &each_failed_rule.context, - each_failed_clause, - )?); - } - - _ => {} - } - } - } - by_rule - } else { - HashMap::new() - }; - - let as_vec = passed_or_skipped.to_vec(); - let (skipped, passed): (Vec<&StatusContext>, Vec<&StatusContext>) = - as_vec.iter().partition(|status| match status.status { - // This uses the dereference deep trait of Rust - Some(Status::SKIP) => true, - _ => false, - }); - let skipped = skipped - .iter() - .map(|s| s.context.clone()) - .collect::>(); - let passed = passed - .iter() - .map(|s| s.context.clone()) - .collect::>(); - renderer.report( - writer, - rules_file, - data_file, - failed, - passed, - skipped, - longest_rule_name, - )?; - Ok(()) - } - fn report_eval<'value>( &self, writer: &mut dyn Write, diff --git a/guard/src/commands/reporters/validate/summary_table.rs b/guard/src/commands/reporters/validate/summary_table.rs index 315c1a96f..104229354 100644 --- a/guard/src/commands/reporters/validate/summary_table.rs +++ b/guard/src/commands/reporters/validate/summary_table.rs @@ -8,7 +8,6 @@ use crate::rules::RecordType; use crate::rules::{NamedStatus, Status}; use colored::*; use enumflags2::{bitflags, BitFlags}; -use itertools::Itertools; use std::io::Write; #[bitflags] @@ -75,79 +74,6 @@ fn print_summary( } impl<'r> Reporter for SummaryTable<'r> { - fn report( - &self, - writer: &mut dyn Write, - status: Option, - failed_rules: &[&StatusContext], - passed_or_skipped: &[&StatusContext], - longest_rule_name: usize, - rules_file_name: &str, - data_file_name: &str, - _data: &Traversal<'_>, - _output_format_type: OutputFormatType, - ) -> crate::rules::Result<()> { - let as_vec = passed_or_skipped.iter().copied().collect_vec(); - let (skipped, passed): (Vec<&StatusContext>, Vec<&StatusContext>) = - as_vec.iter().partition(|status| match status.status { - // This uses the dereference deep trait of Rust - Some(Status::SKIP) => true, - _ => false, - }); - - let mut wrote_header_line = false; - if self.summary_type.contains(SummaryType::SKIP) && !skipped.is_empty() { - writeln!( - writer, - "{} Status = {}", - data_file_name, - colored_string(status) - )?; - wrote_header_line = true; - writeln!(writer, "{}", "SKIP rules".bold())?; - print_partition(writer, rules_file_name, &skipped, longest_rule_name)?; - } - - if self.summary_type.contains(SummaryType::PASS) && !passed.is_empty() { - writeln!( - writer, - "{} Status = {}", - data_file_name, - colored_string(status) - )?; - wrote_header_line = true; - writeln!(writer, "{}", "PASS rules".bold())?; - print_partition(writer, rules_file_name, &passed, longest_rule_name)?; - } - - if self.summary_type.contains(SummaryType::FAIL) && !failed_rules.is_empty() { - writeln!( - writer, - "{} Status = {}", - data_file_name, - colored_string(status) - )?; - wrote_header_line = true; - writeln!(writer, "{}", "FAILED rules".bold())?; - print_partition(writer, rules_file_name, failed_rules, longest_rule_name)?; - } - - if wrote_header_line { - writeln!(writer, "---")?; - } - self.next.report( - writer, - status, - failed_rules, - passed_or_skipped, - longest_rule_name, - rules_file_name, - data_file_name, - _data, - _output_format_type, - ) - } - fn report_eval<'value>( &self, _write: &mut dyn Write, diff --git a/guard/src/commands/reporters/validate/tf.rs b/guard/src/commands/reporters/validate/tf.rs index a704e1a98..08e14a29f 100644 --- a/guard/src/commands/reporters/validate/tf.rs +++ b/guard/src/commands/reporters/validate/tf.rs @@ -1,4 +1,3 @@ -use crate::commands::tracker::StatusContext; use crate::commands::validate::{OutputFormatType, Reporter}; use crate::rules::eval_context::{ simplified_json_from_root, BinaryComparison, ClauseReport, EventRecord, FileReport, @@ -18,27 +17,12 @@ pub(crate) struct TfAware<'reporter> { } impl<'reporter> TfAware<'reporter> { - pub(crate) fn new_with(next: &'reporter dyn Reporter) -> TfAware { + pub(crate) fn new_with(next: &'reporter dyn Reporter) -> TfAware<'reporter> { TfAware { next: Some(next) } } } impl<'reporter> Reporter for TfAware<'reporter> { - fn report( - &self, - _writer: &mut dyn Write, - _status: Option, - _failed_rules: &[&StatusContext], - _passed_or_skipped: &[&StatusContext], - _longest_rule_name: usize, - _rules_file: &str, - _data_file: &str, - _data: &Traversal<'_>, - _output_type: OutputFormatType, - ) -> crate::rules::Result<()> { - Ok(()) - } - fn report_eval<'value>( &self, write: &mut dyn Write, diff --git a/guard/src/commands/validate.rs b/guard/src/commands/validate.rs index 2f453a84c..8c8cf550f 100644 --- a/guard/src/commands/validate.rs +++ b/guard/src/commands/validate.rs @@ -17,7 +17,6 @@ use crate::commands::reporters::validate::structured::StructuredEvaluator; use crate::commands::reporters::validate::summary_table::{self, SummaryType}; use crate::commands::reporters::validate::tf::TfAware; use crate::commands::reporters::validate::{cfn, generic_summary}; -use crate::commands::tracker::StatusContext; use crate::commands::{ Executable, ALPHABETICAL, DATA_FILE_SUPPORTED_EXTENSIONS, ERROR_STATUS_CODE, FAILURE_STATUS_CODE, LAST_MODIFIED, PAYLOAD, PRINT_JSON, REQUIRED_FLAGS, RULES, @@ -112,19 +111,6 @@ impl From<&str> for OutputFormatType { #[allow(clippy::too_many_arguments)] pub(crate) trait Reporter: Debug { - fn report( - &self, - writer: &mut dyn Write, - status: Option, - failed_rules: &[&StatusContext], - passed_or_skipped: &[&StatusContext], - longest_rule_name: usize, - rules_file: &str, - data_file: &str, - data: &Traversal<'_>, - output_type: OutputFormatType, - ) -> Result<()>; - fn report_eval<'value>( &self, _write: &mut dyn Write, diff --git a/guard/src/rules/parser.rs b/guard/src/rules/parser.rs index dcda3b063..b76d66a8f 100644 --- a/guard/src/rules/parser.rs +++ b/guard/src/rules/parser.rs @@ -32,7 +32,7 @@ use crate::rules::values::*; pub(crate) type Span<'a> = LocatedSpan<&'a str, &'a str>; const DEFAULT_RULE_NAME: &str = "default"; -pub(crate) fn from_str2(in_str: &str) -> Span { +pub(crate) fn from_str2(in_str: &str) -> Span<'_> { Span::new_extra(in_str, "") } @@ -693,7 +693,7 @@ pub(crate) fn value_cmp(input: Span) -> IResult { ))(input) } -fn extract_message(input: Span) -> IResult { +fn extract_message(input: Span<'_>) -> IResult<'_, Span<'_>, &str> { match input.find_substring(">>") { None => Err(nom::Err::Failure(ParserError { span: input, @@ -707,7 +707,7 @@ fn extract_message(input: Span) -> IResult { } } -fn custom_message(input: Span) -> IResult { +fn custom_message(input: Span<'_>) -> IResult<'_, Span<'_>, &str> { delimited(tag("<<"), extract_message, tag(">>"))(input) } @@ -955,7 +955,7 @@ fn clause_with_map<'loc, A, M, T: 'loc>( input: Span<'loc>, mut access: A, mut mapper: M, -) -> IResult, T> +) -> IResult<'loc, Span<'loc>, T> where A: FnMut(Span<'loc>) -> IResult, AccessQuery<'loc>>, M: FnMut(GuardAccessClause<'loc>) -> T + 'loc, @@ -1286,7 +1286,7 @@ fn cnf_clauses<'loc, T, E, F, M>( mut f: F, _m: M, _non_empty: bool, -) -> IResult, Conjunctions> +) -> IResult<'loc, Span<'loc>, Conjunctions> where F: FnMut(Span<'loc>) -> IResult, E>, M: FnMut(Vec) -> T, @@ -1328,7 +1328,7 @@ fn disjunction_clauses<'loc, E, F>( input: Span<'loc>, mut parser: F, non_empty: bool, -) -> IResult, Disjunctions> +) -> IResult<'loc, Span<'loc>, Disjunctions> where F: FnMut(Span<'loc>) -> IResult, E>, E: Clone + 'loc, diff --git a/guard/src/rules/path_value/traversal.rs b/guard/src/rules/path_value/traversal.rs index dd8554cb7..d2980d8b4 100644 --- a/guard/src/rules/path_value/traversal.rs +++ b/guard/src/rules/path_value/traversal.rs @@ -101,7 +101,7 @@ fn from_value<'value>( } impl<'value> Traversal<'value> { - pub(crate) fn root(&self) -> Option<&Node> { + pub(crate) fn root(&self) -> Option<&Node<'_>> { self.nodes.get("/") } @@ -109,7 +109,7 @@ impl<'value> Traversal<'value> { &'traverse self, pointer: &str, node: &'traverse Node, - ) -> crate::rules::Result { + ) -> crate::rules::Result> { if pointer.is_empty() || pointer == "0" { return Ok(TraversalResult::Value(node)); } diff --git a/guard/src/utils/mod.rs b/guard/src/utils/mod.rs index c72ea67ec..96e0da793 100644 --- a/guard/src/utils/mod.rs +++ b/guard/src/utils/mod.rs @@ -11,7 +11,7 @@ pub(crate) struct ReadCursor<'buffer> { } impl<'buffer> ReadCursor<'buffer> { - pub(crate) fn new(buffer: &str) -> ReadCursor { + pub(crate) fn new(buffer: &str) -> ReadCursor<'_> { ReadCursor { line_num: 0, line_buffer: buffer.lines(), From 448317b84731b449013e2d6b3169e5cd92de87ef Mon Sep 17 00:00:00 2001 From: Madison Steiner Date: Wed, 12 Aug 2026 16:35:45 +0000 Subject: [PATCH 05/10] Delete the two unreachable validate reporters cfn_reporter.rs and console_reporter.rs existed only to implement the Reporter::report method removed in the previous commit. Neither CfnReporter, SingleLineReporter nor ConsoleReporter was ever constructed, and after that removal the files were referenced by nothing but their own `pub mod` declarations in reporters/validate/mod.rs -- verified by search before deleting rather than inferred from the dead-code warnings, since those warnings say an item is unused and not that its file is unreferenced. The live validate reporting path is unaffected: GenericSummary is constructed at helper.rs and validate.rs and implements report_eval, which is the method the evaluator actually calls. Dead-code warnings 20 -> 15. What remains is the second half of the same cluster -- StructureType, StructuredSummary, DataOutput and DataOutputNewForm in common.rs, SarifRule, the print_partition / print_compliant_skipped_info / pprint_failed_sub_tree / extract_name_info / find_all_failing_clauses helpers, and finally StatusContext and EvaluationType once nothing names them. Left for a follow-up because each needs its own check that no live reporter destructures it, and this commit is already one reviewable question. 329 lib tests, 0 failed, 1 ignored. cargo fmt --check clean. Zero new clippy lints against upstream main. --- .../reporters/validate/cfn_reporter.rs | 139 -------- .../reporters/validate/console_reporter.rs | 331 ------------------ guard/src/commands/reporters/validate/mod.rs | 2 - 3 files changed, 472 deletions(-) delete mode 100644 guard/src/commands/reporters/validate/cfn_reporter.rs delete mode 100644 guard/src/commands/reporters/validate/console_reporter.rs diff --git a/guard/src/commands/reporters/validate/cfn_reporter.rs b/guard/src/commands/reporters/validate/cfn_reporter.rs deleted file mode 100644 index 8a6aa4bc1..000000000 --- a/guard/src/commands/reporters/validate/cfn_reporter.rs +++ /dev/null @@ -1,139 +0,0 @@ -use std::collections::{HashMap, HashSet}; -use std::fmt::Debug; -use std::io::Write; - -use fancy_regex::Regex; -use lazy_static::*; - -use crate::commands::reporters::validate::common::{ - GenericReporter, NameInfo, StructureType, StructuredSummary, -}; -use crate::commands::validate::{OutputFormatType, Reporter}; - -use crate::rules::eval_context::EventRecord; -use crate::rules::path_value::traversal::Traversal; -use crate::rules::Status; - -lazy_static! { - static ref CFN_RESOURCES: Regex = Regex::new(r"^/Resources/(?P[^/]+)/(?P.*$)") - .ok() - .unwrap(); -} - -#[derive(Debug)] -pub(crate) struct CfnReporter {} - -impl Reporter for CfnReporter { - fn report_eval<'value>( - &self, - _write: &mut dyn Write, - _status: Status, - _root_record: &EventRecord<'value>, - _rules_file: &str, - _data_file: &str, - _data_file_bytes: &str, - _data: &Traversal<'value>, - _output_type: OutputFormatType, - ) -> crate::rules::Result<()> { - let renderer = - match _output_type { - OutputFormatType::SingleLineSummary => { - Box::new(SingleLineReporter {}) as Box - } - OutputFormatType::JSON => Box::new(StructuredSummary::new(StructureType::JSON)) - as Box, - OutputFormatType::YAML => Box::new(StructuredSummary::new(StructureType::YAML)) - as Box, - OutputFormatType::Junit => unreachable!(), - OutputFormatType::Sarif => unreachable!(), - }; - super::common::report_from_events( - _root_record, - _write, - _data_file, - _rules_file, - renderer.as_ref(), - ) - } -} - -#[derive(Debug)] -struct SingleLineReporter {} - -impl super::common::GenericReporter for SingleLineReporter { - fn report( - &self, - writer: &mut dyn Write, - rules_file_name: &str, - data_file_name: &str, - by_resource_name: HashMap>>, - passed: HashSet, - skipped: HashSet, - longest_rule_len: usize, - ) -> crate::rules::Result<()> { - writeln!( - writer, - "Evaluation of rules {} for template {}, number of resource failures = {}", - rules_file_name, - data_file_name, - by_resource_name.len() - )?; - if !by_resource_name.is_empty() { - writeln!(writer, "--")?; - } - // - // Agreed on text - // Resource [NewVolume2] property [Properties.Encrypted] in template [template.json] is not compliant with [sg.guard/aws_ec2_volume_checks] because provided value [false] does not match with expected value [true]. Error Message [[EC2-008] : EC2 volumes should be encrypted] - // - for (resource, info) in by_resource_name.iter() { - super::common::print_name_info( - writer, - info, - longest_rule_len, - rules_file_name, - data_file_name, - |_, _, info| { - Ok(format!("Resource [{}] traversed until [{}] for template [{}] wasn't compliant with [{}] due to retrieval error. Error Message [{}]", - resource, - info.path, - data_file_name, - info.rule, - info.message.replace('\n', ";") - )) - }, - |_, _, op_msg, info| { - Ok(format!("Resource [{resource}] property [{property}] in template [{template}] is not compliant with [{rule}] because needed value at [{provided}] {op_msg}. Error message [{msg}]", - resource=resource, - property=info.path, - provided=info.provided.as_ref().map_or(&serde_json::Value::Null, std::convert::identity), - op_msg=op_msg, - template=data_file_name, - rule= info.rule, - msg=info.message.replace('\n', ";") - )) - }, - |_, _, msg, info| { - Ok(format!("Resource [{resource}] property [{property}] in template [{template}] is not compliant with [{rule}] because provided value [{provided}] {op_msg} match with expected value [{expected}]. Error message [{msg}]", - resource=resource, - property=info.path, - provided=info.provided.as_ref().map_or(&serde_json::Value::Null, std::convert::identity), - op_msg=msg, - expected=info.expected.as_ref().map_or(&serde_json::Value::Null, std::convert::identity), - template=data_file_name, - rule=info.rule, - msg=info.message.replace('\n', ";") - )) - }, - )?; - } - super::common::print_compliant_skipped_info( - writer, - &passed, - &skipped, - rules_file_name, - data_file_name, - )?; - writeln!(writer, "--")?; - Ok(()) - } -} diff --git a/guard/src/commands/reporters/validate/console_reporter.rs b/guard/src/commands/reporters/validate/console_reporter.rs deleted file mode 100644 index e129a1108..000000000 --- a/guard/src/commands/reporters/validate/console_reporter.rs +++ /dev/null @@ -1,331 +0,0 @@ -use crate::commands::validate::{OutputFormatType, Reporter}; -use crate::rules::eval_context::EventRecord; -use crate::rules::path_value::traversal::Traversal; -use crate::rules::values::CmpOperator; -use crate::rules::{ - BlockCheck, ClauseCheck, ComparisonClauseCheck, NamedStatus, QueryResult, RecordType, Status, - TypeBlockCheck, UnaryValueCheck, ValueCheck, -}; -use std::io::Write; -use std::rc::Rc; - -#[derive(Debug)] -pub(crate) struct ConsoleReporter {} - -// -// https://vallentin.dev/2019/05/14/pretty-print-tree -// -fn pprint_failed_sub_tree( - current: &EventRecord<'_>, - prefix: String, - last: bool, - rules_file_name: &str, - data_file_name: &str, - writer: &mut dyn Write, -) -> crate::rules::Result<()> { - let prefix_current = if last { "`- " } else { "|- " }; - let increment_prefix = match ¤t.container { - Some(RecordType::TypeBlock(Status::FAIL)) - | Some(RecordType::BlockGuardCheck(BlockCheck { - status: Status::FAIL, - .. - })) - | Some(RecordType::GuardClauseBlockCheck(BlockCheck { - status: Status::FAIL, - .. - })) - | Some(RecordType::TypeCheck(TypeBlockCheck { - block: - BlockCheck { - status: Status::FAIL, - .. - }, - .. - })) - | Some(RecordType::WhenCheck(BlockCheck { - status: Status::FAIL, - .. - })) => false, - Some(RecordType::FileCheck(NamedStatus { - status: Status::FAIL, - .. - })) - | Some(RecordType::RuleCheck(NamedStatus { - status: Status::FAIL, - .. - })) - | Some(RecordType::Disjunction(BlockCheck { - status: Status::FAIL, - .. - })) => { - writeln!(writer, "{}{}{}", prefix, prefix_current, current)?; - true - } - - Some(RecordType::ClauseValueCheck(check)) => { - match check { - ClauseCheck::NoValueForEmptyCheck(msg) => { - let custom_message = msg - .as_ref() - .map_or("".to_string(), |s| s.replace('\n', ";")); - - writeln!( - writer, - "{}{}Check was not compliant as variable in context [{}] was not empty. Message [{}]", - prefix, - prefix_current, - current.context, - custom_message - )?; - } - - ClauseCheck::Success => {} - - ClauseCheck::DependentRule(missing) => { - writeln!( - writer, - "{prefix}{prefix_current}Check was not compliant as dependent rule [{rule}] evaluated to FAIL in [{file}]. Context [{cxt}]", - prefix=prefix, - prefix_current=prefix_current, - rule=missing.rule, - file=rules_file_name, - cxt=current.context - )?; - } - - ClauseCheck::MissingBlockValue(missing) => { - let (property, far) = match &missing.from { - QueryResult::UnResolved(ur) => { - (ur.remaining_query.as_str(), Rc::clone(&ur.traversed_to)) - } - _ => unreachable!(), - }; - writeln!( - writer, - "{}{}Check was not compliant as property [{}] is missing in data [{}]. Value traversed to [{}]", - prefix, - prefix_current, - property, - data_file_name, - far - )?; - } - - ClauseCheck::Unary(UnaryValueCheck { - comparison: (cmp, not), - value: - ValueCheck { - status: Status::FAIL, - from, - message, - custom_message, - }, - }) => { - let cmp_msg = match cmp { - CmpOperator::Exists => { - if *not { - "existed" - } else { - "did not exist" - } - } - CmpOperator::Empty => { - if *not { - "was empty" - } else { - "was not empty" - } - } - CmpOperator::IsList => { - if *not { - "was a list " - } else { - "was not list" - } - } - CmpOperator::IsMap => { - if *not { - "was a struct" - } else { - "was not struct" - } - } - CmpOperator::IsString => { - if *not { - "was a string " - } else { - "was not string" - } - } - _ => unreachable!(), - }; - - let custom_message = custom_message.as_ref().map_or("".to_string(), |s| { - format!(" Message = [{}]", s.replace('\n', ";")) - }); - - let error_message = message - .as_ref() - .map_or("".to_string(), |s| format!(" Error = [{}]", s)); - - match from { - QueryResult::Literal(_) => unreachable!(), - QueryResult::Resolved(res) => { - writeln!( - writer, - "{}{}Check was not compliant as property [{prop}] {cmp_msg}.{err}{msg}", - prefix, - prefix_current, - prop=res.self_path(), - cmp_msg=cmp_msg, - err=error_message, - msg=custom_message - )?; - } - - QueryResult::UnResolved(unres) => { - writeln!( - writer, - "{}{}Check was not compliant as property [{remain}] is missing. Value traversed to [{tr}].{err}{msg}", - prefix, - prefix_current, - remain=unres.remaining_query, - tr=unres.traversed_to, - err=error_message, - msg=custom_message - )?; - } - } - } - - ClauseCheck::Comparison(ComparisonClauseCheck { - custom_message, - message, - comparison: (cmp, not), - from, - status: Status::FAIL, - to, - }) => { - let custom_message = custom_message.as_ref().map_or("".to_string(), |s| { - format!(" Message = [{}]", s.replace('\n', ";")) - }); - - let error_message = message - .as_ref() - .map_or("".to_string(), |s| format!(" Error = [{}]", s)); - - let to_result = match to { - Some(to) => match to { - QueryResult::Literal(_) => unreachable!(), - QueryResult::Resolved(to_res) => Some(Rc::clone(to_res)), - - QueryResult::UnResolved(to_unres) => { - writeln!( - writer, - "{}{}Check was not compliant as property [{remain}] to compare to is missing. Value traversed to [{to}].{err}{msg}", - prefix, - prefix_current, - remain=to_unres.remaining_query, - to=to_unres.traversed_to, - err=error_message, - msg=custom_message - )?; - return Ok(()); - } - }, - - None => None, - }; - - match from { - QueryResult::Literal(_) => unreachable!(), - QueryResult::UnResolved(to_unres) => { - writeln!( - writer, - "{}{}Check was not compliant as property [{remain}] to compare from is missing. Value traversed to [{to}].{err}{msg}", - prefix, - prefix_current, - remain=to_unres.remaining_query, - to=to_unres.traversed_to, - err=error_message, - msg=custom_message - )?; - } - - QueryResult::Resolved(res) => { - writeln!( - writer, - "{}{}Check was not compliant as property value [{from}] {op_msg} value [{to}].{err}{msg}", - prefix, - prefix_current, - from=res, - to=to_result.map_or("NULL".to_string(), |t| format!("{}", t)), - op_msg=match cmp { - CmpOperator::Eq => if *not { "equal to" } else { "not equal to" }, - CmpOperator::Le => if *not { "less than equal to" } else { "not less than equal to" }, - CmpOperator::Lt => if *not { "less than" } else { "not less than" }, - CmpOperator::Ge => if *not { "greater than equal to" } else { "not greater than equal" }, - CmpOperator::Gt => if *not { "greater than" } else { "not greater than" }, - CmpOperator::In => if *not { "in" } else { "not in" }, - _ => unreachable!() - }, - err=error_message, - msg=custom_message - )?; - } - } - } - - _ => return Ok(()), // Success skip - } - false - } - - _ => return Ok(()), - }; - - let prefix = if increment_prefix { - let prefix_child = if last { " " } else { "| " }; - prefix + prefix_child - } else { - prefix - }; - - if !current.children.is_empty() { - let last_child = current.children.len() - 1; - for (i, child) in current.children.iter().enumerate() { - pprint_failed_sub_tree( - child, - prefix.clone(), - i == last_child, - rules_file_name, - data_file_name, - writer, - )?; - } - } - Ok(()) -} - -impl Reporter for ConsoleReporter { - fn report_eval<'value>( - &self, - _write: &mut dyn Write, - _status: Status, - _root_record: &EventRecord<'value>, - _rules_file: &str, - _data_file: &str, - _data_file_bytes: &str, - _data: &Traversal<'value>, - _output_type: OutputFormatType, - ) -> crate::rules::Result<()> { - pprint_failed_sub_tree( - _root_record, - "".to_string(), - true, - _rules_file, - _data_file, - _write, - ) - } -} diff --git a/guard/src/commands/reporters/validate/mod.rs b/guard/src/commands/reporters/validate/mod.rs index b6d5a9eb5..593d39110 100644 --- a/guard/src/commands/reporters/validate/mod.rs +++ b/guard/src/commands/reporters/validate/mod.rs @@ -1,7 +1,5 @@ pub mod cfn; -pub mod cfn_reporter; pub mod common; -pub mod console_reporter; pub mod generic_summary; pub mod sarif; pub mod structured; From 2e0fc4cf528ead9b5d9f37772301f762cf45b8e4 Mon Sep 17 00:00:00 2001 From: Madison Steiner Date: Wed, 12 Aug 2026 17:06:52 +0000 Subject: [PATCH 06/10] Delete the rest of the legacy reporting cluster; zero dead-code warnings Finishes what the previous two commits started. Removing Reporter::report left a connected set of items reachable only from each other, and deleting them in dependency order collapses it entirely: common.rs extract_name_info, find_all_failing_clauses, print_compliant_skipped_info, StructuredSummary and its impls, StructureType, DataOutput, DataOutputNewForm summary_table print_partition sarif.rs SarifRule tracker.rs deleted -- StatusContext was the whole file once StackTracker went mod.rs EvaluationType and its Display impl, freed by StatusContext going; add_variable_capture_index, a never-called default trait method operators.rs UnaryComparator, a trait with no implementors exprs.rs WhenGuardBlockClause, never constructed by the parser Dead-code warnings 15 -> 0. Upstream main has 43, so this branch and its two parents account for all of them. Order mattered and was followed rather than guessed: the four dead functions were the last things naming StatusContext, StatusContext was the last thing naming EvaluationType, and each step was verified by search before deleting rather than inferred from the warning list -- a warning says an item is unused, not that removing it is safe. The live validate path is untouched. GenericSummary is still constructed at helper.rs and validate.rs, GenericReporter and the structured reporters keep their own `report` methods, and report_eval remains the evaluator's entry point. 329 lib tests, 0 failed, 1 ignored. All other targets green except validate, which fails 81/15 identically on upstream main -- a path artifact where the fixture comparison strips a directory prefix and a /local/... checkout defeats it. cargo fmt --check clean. Clippy 85 -> 20 errors under 1.97, zero of them new against main. --- guard/src/commands/mod.rs | 1 - .../src/commands/reporters/validate/common.rs | 188 +----------------- .../src/commands/reporters/validate/sarif.rs | 5 - .../reporters/validate/summary_table.rs | 20 -- guard/src/commands/tracker.rs | 24 --- guard/src/rules/eval/operators.rs | 4 - guard/src/rules/exprs.rs | 6 - guard/src/rules/mod.rs | 33 --- 8 files changed, 3 insertions(+), 278 deletions(-) delete mode 100644 guard/src/commands/tracker.rs diff --git a/guard/src/commands/mod.rs b/guard/src/commands/mod.rs index d2cdbd71a..ef0d22833 100644 --- a/guard/src/commands/mod.rs +++ b/guard/src/commands/mod.rs @@ -17,7 +17,6 @@ pub mod validate; pub mod completions; pub mod reporters; -mod tracker; // // Constants diff --git a/guard/src/commands/reporters/validate/common.rs b/guard/src/commands/reporters/validate/common.rs index 7ed39243b..97d749c0d 100644 --- a/guard/src/commands/reporters/validate/common.rs +++ b/guard/src/commands/reporters/validate/common.rs @@ -1,16 +1,13 @@ use colored::*; use serde::Serialize; -use crate::commands::tracker::StatusContext; use crate::rules::eval_context::{ - BinaryCheck, BinaryComparison, ClauseReport, EventRecord, FileReport, GuardClauseReport, - InComparison, UnaryCheck, UnaryComparison, ValueComparisons, ValueUnResolved, + BinaryCheck, BinaryComparison, ClauseReport, EventRecord, GuardClauseReport, InComparison, + UnaryCheck, UnaryComparison, ValueComparisons, ValueUnResolved, }; use crate::rules::values::CmpOperator; -use crate::rules::{ - ClauseCheck, EvaluationType, NamedStatus, QueryResult, RecordType, Status, UnResolved, -}; +use crate::rules::{ClauseCheck, NamedStatus, QueryResult, RecordType, Status, UnResolved}; use fancy_regex::Regex; use lazy_static::*; use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; @@ -73,60 +70,6 @@ pub(super) trait GenericReporter: Debug { ) -> crate::rules::Result<()>; } -#[derive(Debug)] -#[allow(clippy::upper_case_acronyms)] -pub(super) enum StructureType { - JSON, - YAML, -} - -#[derive(Debug)] -pub(super) struct StructuredSummary { - hierarchy_type: StructureType, -} - -impl StructuredSummary { - pub(super) fn new(hierarchy_type: StructureType) -> Self { - StructuredSummary { hierarchy_type } - } -} - -#[derive(Debug, Serialize)] -struct DataOutput<'a> { - data_from: &'a str, - rules_from: &'a str, - not_compliant: HashMap>>, - not_applicable: HashSet, - compliant: HashSet, -} - -impl GenericReporter for StructuredSummary { - fn report( - &self, - writer: &mut dyn Write, - rules_file_name: &str, - data_file_name: &str, - failed: HashMap>>, - passed: HashSet, - skipped: HashSet, - _: usize, - ) -> crate::rules::Result<()> { - let value = DataOutput { - rules_from: rules_file_name, - data_from: data_file_name, - not_compliant: failed, - compliant: passed, - not_applicable: skipped, - }; - - match &self.hierarchy_type { - StructureType::JSON => writeln!(writer, "{}", serde_json::to_string(&value)?), - StructureType::YAML => writeln!(writer, "{}", serde_yaml::to_string(&value)?), - }?; - Ok(()) - } -} - lazy_static! { static ref PATH_FROM_MSG: Regex = Regex::new(r"path\s+=\s+(?P[^ ]+)").ok().unwrap(); } @@ -379,71 +322,6 @@ pub(super) fn report_from_events( Ok(()) } -pub(super) fn extract_name_info<'a>( - rule_name: &'a str, - each_failing_clause: &StatusContext, -) -> crate::rules::Result> { - if each_failing_clause.from.is_some() { - let value = each_failing_clause.from.as_ref().unwrap(); - let (path, from): (String, serde_json::Value) = value.try_into()?; - Ok(NameInfo { - rule: rule_name, - path, - provided: Some(from), - expected: match &each_failing_clause.to { - Some(to) => { - let (_, val): (String, serde_json::Value) = to.try_into()?; - Some(val) - } - None => None, - }, - comparison: each_failing_clause.comparator.map(|input| input.into()), - message: each_failing_clause - .msg - .as_ref() - .map_or("".to_string(), |e| { - if !e.contains("DEFAULT") { - e.clone() - } else { - "".to_string() - } - }), - error: None, - }) - } else { - // - // This is crappy, but we are going to extract information from the retrieval error message - // see path_value.rs for retrieval error messages. - // TODO merge the query interface to retrieve partial results along with errored one ones and then - // change this logic based on the reporting changes. Today we bail out for the first - // retrieval error, fast fail semantics - // - - // - // No from is how we indicate retrieval errors. - // - let (path, error) = - each_failing_clause - .msg - .as_ref() - .map_or( - ("".to_string(), "".to_string()), - |msg| match PATH_FROM_MSG.captures(msg) { - Ok(Some(cap)) => (cap["path"].to_string(), msg.clone()), - Ok(None) => ("".to_string(), msg.clone()), - Err(_) => panic!("Error while parsing retrieval errors"), - }, - ); - - Ok(NameInfo { - rule: rule_name, - path, - error: Some(error), - ..Default::default() - }) - } -} - pub(super) fn colored_string(status: Option) -> ColoredString { let status = match status { Some(s) => s, @@ -456,59 +334,6 @@ pub(super) fn colored_string(status: Option) -> ColoredString { } } -pub(super) fn find_all_failing_clauses(context: &StatusContext) -> Vec<&StatusContext> { - let mut failed = Vec::with_capacity(context.children.len()); - for each in &context.children { - if each.status.map_or(false, |s| s == Status::FAIL) { - match each.eval_type { - EvaluationType::Clause | EvaluationType::BlockClause => { - failed.push(each); - if each.eval_type == EvaluationType::BlockClause { - failed.extend(find_all_failing_clauses(each)); - } - } - - EvaluationType::Filter | EvaluationType::Condition => { - continue; - } - - _ => failed.extend(find_all_failing_clauses(each)), - } - } - } - failed -} - -pub(super) fn print_compliant_skipped_info( - writer: &mut dyn Write, - passed: &HashSet, - skipped: &HashSet, - _: &str, - data_file_name: &str, -) -> crate::rules::Result<()> { - if !passed.is_empty() { - writeln!(writer, "--")?; - } - for pass in passed { - writeln!( - writer, - "Rule [{}] is compliant for template [{}]", - pass, data_file_name - )?; - } - if !skipped.is_empty() { - writeln!(writer, "--")?; - } - for skip in skipped { - writeln!( - writer, - "Rule [{}] is not applicable for template [{}]", - skip, data_file_name - )?; - } - Ok(()) -} - #[allow(clippy::too_many_arguments)] pub(super) fn print_name_info( writer: &mut dyn Write, @@ -645,13 +470,6 @@ where Ok(()) } -#[derive(Debug, Serialize)] -struct DataOutputNewForm<'a, 'v> { - data_from: &'a str, - rules_from: &'a str, - report: FileReport<'v>, -} - #[derive(Clone, Debug)] pub(super) struct LocalResourceAggr<'record, 'value: 'record> { pub(super) name: String, diff --git a/guard/src/commands/reporters/validate/sarif.rs b/guard/src/commands/reporters/validate/sarif.rs index 7dd9be9c4..d4917c7b7 100644 --- a/guard/src/commands/reporters/validate/sarif.rs +++ b/guard/src/commands/reporters/validate/sarif.rs @@ -179,11 +179,6 @@ struct SarifLocation { physical_location: SarifPhysicalLocation, } -#[derive(Debug, Deserialize, Serialize, Clone)] -struct SarifRule { - id: String, -} - #[derive(Debug, Deserialize, Serialize, Clone)] pub struct SarifReport { #[serde(rename = "$schema")] diff --git a/guard/src/commands/reporters/validate/summary_table.rs b/guard/src/commands/reporters/validate/summary_table.rs index 104229354..fb563dc29 100644 --- a/guard/src/commands/reporters/validate/summary_table.rs +++ b/guard/src/commands/reporters/validate/summary_table.rs @@ -1,5 +1,4 @@ use crate::commands::reporters::validate::common::colored_string; -use crate::commands::tracker::StatusContext; use crate::commands::validate::{OutputFormatType, Reporter}; use crate::rules::eval_context::EventRecord; use crate::rules::parser::get_rule_name; @@ -35,25 +34,6 @@ impl<'a> SummaryTable<'a> { } } -fn print_partition( - writer: &mut dyn Write, - rules_file_name: &str, - part: &[&StatusContext], - longest: usize, -) -> crate::rules::Result<()> { - for container in part { - writeln!( - writer, - "{filename}/{context:<0$}{status}", - longest + 4, - filename = rules_file_name, - context = get_rule_name(rules_file_name, &container.context), - status = super::common::colored_string(container.status) - )?; - } - Ok(()) -} - fn print_summary( writer: &mut dyn Write, rules_file_name: &str, diff --git a/guard/src/commands/tracker.rs b/guard/src/commands/tracker.rs deleted file mode 100644 index f264d9c06..000000000 --- a/guard/src/commands/tracker.rs +++ /dev/null @@ -1,24 +0,0 @@ -use crate::rules::values::CmpOperator; -use crate::rules::{path_value::PathAwareValue, EvaluationType, Status}; -use serde::Serialize; - -/// The per-clause record the validate reporters read. -/// -/// Nothing constructs one any more -- `StackTracker`, the `EvaluationContext` implementation -/// that built these, was the old evaluator's recorder and went with it. The type stays -/// because the reporters still destructure it: `common.rs`, `cfn_reporter.rs` and -/// `generic_summary.rs` match on `eval_type` and walk `children`, and `generic_summary.rs` is -/// live (constructed at `helper.rs` and `validate.rs`). The new evaluator records through -/// `RecordType`/`EventRecord` in `eval_context.rs` instead, so those reporter branches are -/// unreachable rather than wrong. -#[derive(Serialize, Debug)] -pub(crate) struct StatusContext { - pub(crate) eval_type: EvaluationType, - pub(crate) context: String, - pub(crate) msg: Option, - pub(crate) from: Option, - pub(crate) to: Option, - pub(crate) status: Option, - pub(crate) comparator: Option<(CmpOperator, bool)>, - pub(crate) children: Vec, -} diff --git a/guard/src/rules/eval/operators.rs b/guard/src/rules/eval/operators.rs index 926a135aa..092c42036 100644 --- a/guard/src/rules/eval/operators.rs +++ b/guard/src/rules/eval/operators.rs @@ -102,10 +102,6 @@ pub(crate) trait Comparator { -> crate::rules::Result; } -pub(crate) trait UnaryComparator { - fn compare(&self, lhs: &[QueryResult]) -> crate::rules::Result; -} - struct CommonOperator { comparator: fn(&PathAwareValue, &PathAwareValue) -> crate::rules::Result, } diff --git a/guard/src/rules/exprs.rs b/guard/src/rules/exprs.rs index 2da087be6..cea04aefc 100644 --- a/guard/src/rules/exprs.rs +++ b/guard/src/rules/exprs.rs @@ -202,12 +202,6 @@ pub(crate) struct BlockGuardClause<'loc> { pub(crate) not_empty: bool, } -#[derive(Eq, PartialEq, Debug, Clone, Serialize, Deserialize, Hash)] -pub(crate) struct WhenGuardBlockClause<'loc> { - pub(crate) conditions: WhenConditions<'loc>, - pub(crate) block: Block<'loc, GuardClause<'loc>>, -} - #[derive(Eq, PartialEq, Debug, Clone, Serialize, Deserialize, Hash)] pub(crate) struct ParameterizedNamedRuleClause<'loc> { pub(crate) parameters: Vec>, diff --git a/guard/src/rules/mod.rs b/guard/src/rules/mod.rs index 5fac73231..7ff68cba0 100644 --- a/guard/src/rules/mod.rs +++ b/guard/src/rules/mod.rs @@ -131,36 +131,6 @@ impl Status { } } -#[derive(Debug, Clone, PartialEq, Copy, Serialize)] -pub(crate) enum EvaluationType { - File, - Rule, - Type, - Condition, - ConditionBlock, - Filter, - Conjunction, - BlockClause, - Clause, -} - -impl std::fmt::Display for EvaluationType { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - match self { - EvaluationType::File => f.write_str("File")?, - EvaluationType::Rule => f.write_str("Rule")?, - EvaluationType::Type => f.write_str("Type")?, - EvaluationType::Condition => f.write_str("Condition")?, - EvaluationType::ConditionBlock => f.write_str("ConditionBlock")?, - EvaluationType::Filter => f.write_str("Filter")?, - EvaluationType::Conjunction => f.write_str("Conjunction")?, - EvaluationType::BlockClause => f.write_str("BlockClause")?, - EvaluationType::Clause => f.write_str("Clause")?, - } - Ok(()) - } -} - #[derive(Debug, Clone, PartialEq, Serialize)] pub(crate) struct UnResolved { pub(crate) traversed_to: Rc, @@ -373,9 +343,6 @@ pub(crate) trait EvalContext<'value, 'loc: 'value>: RecordTracer<'value> { variable_name: &'value str, key: Rc, ) -> Result<()>; - fn add_variable_capture_index(&mut self, _: &str, _: Rc) -> Result<()> { - Ok(()) - } } pub fn short_form_to_long(fn_ref: &str) -> &'static str { From 3445508cd62ebccea9a224bba4163359c09ee24e Mon Sep 17 00:00:00 2001 From: Madison Steiner Date: Wed, 12 Aug 2026 17:22:40 +0000 Subject: [PATCH 07/10] Anchor test path sanitisation on the crate dir, not $HOME MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit compare_write_buffer_with_file normalises captured output before comparing it to a fixture, and did so by rewriting $HOME to ~ and then reducing ~/…/file.yaml to the bare filename. Two bugs in that: - $HOME was substituted by plain substring match, so a checkout whose path merely *contains* $HOME was corrupted rather than normalised. With HOME=/home/u, the path /local/home/u/repo/tests/resources/x.yaml became /local~/repo/tests/resources/x.yaml, and reducing the tail then left /localx.yaml welded together. /local/home is a real layout, not a hypothetical. - a checkout outside $HOME produced no ~ at all, so the reduction never fired and every comparison saw a full absolute path. Either way the failures looked like product bugs. On this checkout 15 of the 96 validate tests failed for this reason alone, which is why that whole target had been written off as environmental. Now anchored on CARGO_MANIFEST_DIR, which is where get_full_path_for_resource_file roots every resource path, so no assumption about the checkout location survives. Anchoring there rather than matching bare absolute paths is deliberate: the SARIF fixtures contain //docs.oasis-open.org/…/sarif-schema-2.1.0.json, and a regex over absolute paths ending in .json would reduce that URL to its basename. It is the only slash-bearing file reference in guard/resources, so the distinction matters for exactly one fixture and is easy to lose. replace_home_directory_with_tilde is deleted rather than fixed. Nothing else called it, and no fixture in guard/resources contains a tilde, so the substitution existed only as a sentinel for the regex that no longer needs one. validate target 81 passed/15 failed -> 95 passed/1 failed. The remaining failure, test_validate_with_failing_complex_rule, is a real output difference that the sanitisation bug was masking, not a path artifact; it is diagnosed separately. --- guard/tests/utils.rs | 48 ++++++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/guard/tests/utils.rs b/guard/tests/utils.rs index 69d38b67e..906206644 100644 --- a/guard/tests/utils.rs +++ b/guard/tests/utils.rs @@ -102,35 +102,43 @@ pub fn get_full_path_for_resource_file(path: &str) -> String { return resource.display().to_string(); } -pub fn replace_home_directory_with_tilde(text: String) -> String { - let home_dir_string = match env::var("HOME") { - Ok(home_path) => home_path, - Err(_) => panic!("HOME variable required for tests!"), - }; - - text.replace(&home_dir_string, "~") -} - +/// Reduce resource paths in captured output to bare filenames, so expected-output fixtures do +/// not depend on where the repository is checked out. +/// +/// Anchored on `CARGO_MANIFEST_DIR`. Every resource path that reaches test output is rooted +/// there, because `get_full_path_for_resource_file` builds them from it. +/// +/// This replaced a `$HOME`-based version that first rewrote the home directory to `~` and then +/// matched `~/…`, which had two bugs: +/// +/// - `$HOME` was substituted by plain substring match, so a checkout whose path merely +/// *contains* `$HOME` was corrupted rather than normalised. With `HOME=/home/u`, the path +/// `/local/home/u/repo/tests/resources/x.yaml` became `/local~/repo/tests/resources/x.yaml`, +/// and reducing the tail then left `/localx.yaml` welded together. `/local/home` is a real +/// layout rather than a hypothetical, and it failed 15 of the 96 `validate` tests while +/// leaving them looking like product failures. +/// - a checkout *outside* `$HOME` produced no `~` at all, so the reduction never fired and every +/// comparison saw a full absolute path. +/// +/// Anchoring on the crate directory also leaves URLs alone by construction, which a regex over +/// bare absolute paths would not: the SARIF fixtures contain +/// `//docs.oasis-open.org/…/sarif-schema-2.1.0.json`, and reducing that to its basename would +/// break them. It is the only slash-bearing file reference in `guard/resources`, so the +/// distinction is load-bearing for exactly one fixture and easy to lose. pub fn replace_path_with_filenames(text: String) -> String { let extensions = ["yaml", "yml", "json"]; - // pattern to match anything between "~/" and any of the extensions + // Any path rooted at the crate directory, reduced to its final component. let pattern = format!( - r#"~/(?:[\w/\-]+/)?([\w/\-]+\.(?:{}))"#, + r#"{}[\w/.\-]*/([\w.\-]+\.(?:{}))"#, + fancy_regex::escape(env!("CARGO_MANIFEST_DIR")), extensions.join("|") ); let re = Regex::new(&pattern).unwrap(); - // replace the entire match with match group 1 (the file name) - let replaced_filenames = re.replace_all(&text, "$1"); - - replaced_filenames.to_string() + re.replace_all(&text, "$1").to_string() } pub fn sanitize_path(string_to_sanitize: String) -> String { - // replace the home directory to avoid regex issues with path matches beyond the - // leading forward slash for example '[/Users/...' or 'name="/User...' - let replaced_home_directory = replace_home_directory_with_tilde(string_to_sanitize); - // return the blob of text with full path replaced with just the filename - replace_path_with_filenames(replaced_home_directory) + replace_path_with_filenames(string_to_sanitize) } pub fn compare_write_buffer_with_file( From 0df9eae6ba5d27d5c9a11869e042d77c4ce35f13 Mon Sep 17 00:00:00 2001 From: Madison Steiner Date: Mon, 17 Aug 2026 19:13:34 +0000 Subject: [PATCH 08/10] Stop the install scripts failing on an anonymous GitHub API quota The Windows install job failed on an unrelated pull request with {"message": "API rate limit exceeded for 20.9.183.48."} from install-guard.ps1's release lookup. That address is a shared GitHub Actions runner, and the anonymous API allows 60 requests an hour per source IP, counted across everyone behind it. The same limit reaches real users: a corporate NAT, a VPN, or a second person installing from the same office is enough. Resolution now prefers whatever needs least from the caller. The gh CLI first, when it is installed and authenticated, because it reuses credentials the caller already has. Then the REST API with GITHUB_TOKEN when the environment supplies one. Then anonymously, which is the only path the limit applies to. An explicit version skips the lookup entirely, and install-guard.ps1 gains -Version for that, matching -v in install-guard.sh. Retries honour what the API says rather than guessing: retry-after on a secondary limit, x-ratelimit-reset when the primary one is exhausted, exponential backoff only when neither header is readable. Total waiting is capped at five minutes, after which it stops and names GITHUB_TOKEN, gh auth login and -Version as the ways out. Waiting for a primary reset can mean an hour, and an installer that looks hung for an hour is worse than one that explains itself. The token is passed to curl through a config file on stdin rather than argv. An Authorization header on a command line is readable from ps by anyone else on the host for the life of the request. It is only ever sent to api.github.com; the release archive redirects to a separate download host. install-guard.sh could not fail. Its err() exits, but it was reached from the left side of a pipeline feeding a while-read loop, so exit 1 ended only that subshell and the pipeline took its status from the loop, which had read nothing. A failed lookup left the script exiting 0 with nothing installed -- which is why only the Windows job went red when all three platforms hit the same wall. get_version's result is assigned now, so the status propagates. Get-ArchType read Win32_Processor through Get-WmiObject, a cmdlet PowerShell 6 removed; this workflow runs pwsh 7. It reads OSArchitecture from RuntimeInformation instead, which is part of the framework, present in every supported host, and exercisable outside Windows -- the WMI and CIM cmdlets are both Windows-only, so neither could be tested before CI ran. The install jobs now build cfn-guard from the branch under test, package it into the release layout, and install that, asserting the installed binary's checksum matches the one just built. They previously resolved the latest release and installed it, so they tested the installer against a binary unrelated to the change under review, and depended on the API that failed above. --- .github/workflows/pr.yml | 89 ++++++++++++-- install-guard.ps1 | 234 +++++++++++++++++++++++++++++++++---- install-guard.sh | 245 +++++++++++++++++++++++++++++++++------ 3 files changed, 504 insertions(+), 64 deletions(-) diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 352079c1a..c230876c3 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -53,35 +53,110 @@ jobs: - name: Rustfmt Check uses: actions-rust-lang/rustfmt@v1 + # These jobs install a binary built from the branch under test, not the last published release. + # + # They used to resolve "latest release" from the GitHub API and install that, which tested the + # installer against a binary having nothing to do with the change under review, and made the job + # depend on an API whose anonymous limit is 60 requests per hour per source IP -- shared across + # every job on a runner. That limit is what failed the Windows job on an unrelated pull request. + # + # Packaging the local build into the release layout and pointing the installer at it with + # GUARD_DOWNLOAD_BASE_URL removes the network from the critical path and makes the assertion + # meaningful: the checksum of the installed binary is compared against the one just built. + # + # GITHUB_TOKEN is still exported. Nothing in the pinned path needs it, but any future step that + # resolves a version should be authenticated by default rather than discover the limit in CI. installScript: strategy: matrix: os: [ubuntu-latest, macos-latest] runs-on: ${{ matrix.os }} name: Testing Install Script (install-guard.sh) + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} steps: - uses: actions/checkout@v3 name: Checkout cfn-guard with: path: cloudformation-guard - - name: Test install script on ${{ matrix.os }} + - uses: actions-rust-lang/setup-rust-toolchain@v1 + - name: Build cfn-guard from this branch + working-directory: cloudformation-guard + run: cargo build --release --bin cfn-guard + - name: Package the build into the release layout and install it + working-directory: cloudformation-guard run: | - set -e - cd cloudformation-guard - sh install-guard.sh + set -eu + VERSION=$(./target/release/cfn-guard --version | awk '{ print $2 }') + MAJOR=${VERSION%%.*} + case "$(uname -s)" in + Darwin) OS_TYPE=macos ;; + *) OS_TYPE=ubuntu ;; + esac + ARCH_TYPE=$(uname -m) + [ "$ARCH_TYPE" = "arm64" ] && ARCH_TYPE=aarch64 + NAME="cfn-guard-v${MAJOR}-${ARCH_TYPE}-${OS_TYPE}-latest" + + mkdir -p "stage/${NAME}" "artifacts/${VERSION}" + cp target/release/cfn-guard "stage/${NAME}/cfn-guard" + tar -czf "artifacts/${VERSION}/${NAME}.tar.gz" -C stage "${NAME}" + + GUARD_DOWNLOAD_BASE_URL="file://${PWD}/artifacts" sh install-guard.sh -v "${VERSION}" + + # The installer reports success on its own terms; this checks it installed the binary we + # just built rather than something already on the runner. + "${HOME}/.guard/bin/cfn-guard" --version + BUILT=$(shasum -a 256 target/release/cfn-guard | awk '{ print $1 }') + INSTALLED=$(shasum -a 256 "${HOME}/.guard/${MAJOR}/${NAME}/cfn-guard" | awk '{ print $1 }') + if [ "$BUILT" != "$INSTALLED" ]; then + echo "installed binary does not match the one built from this branch" >&2 + echo " built: $BUILT" >&2 + echo " installed: $INSTALLED" >&2 + exit 1 + fi + echo "installed binary matches this branch's build ($BUILT)" installScriptWindows: runs-on: windows-latest name: Testing Windows Install Script (install-guard.ps1) + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} steps: - uses: actions/checkout@v3 name: Checkout cfn-guard with: path: cloudformation-guard - - name: Test install script on windows-latest + - uses: actions-rust-lang/setup-rust-toolchain@v1 + - name: Build cfn-guard from this branch + working-directory: cloudformation-guard + run: cargo build --release --bin cfn-guard + - name: Package the build into the release layout and install it + working-directory: cloudformation-guard + shell: pwsh run: | - cd cloudformation-guard - ./install-guard.ps1 + $ErrorActionPreference = 'Stop' + $version = (& ./target/release/cfn-guard.exe --version).Split(' ')[1] + $major = $version.Split('.')[0] + $arch = switch ([System.Runtime.InteropServices.RuntimeInformation]::OSArchitecture) { + 'Arm64' { 'aarch64' } 'X64' { 'x86_64' } 'X86' { 'i686' } + } + $name = "cfn-guard-v$major-$arch-windows-latest" + + New-Item -ItemType Directory -Force -Path "stage/$name", "artifacts/$version" | Out-Null + Copy-Item target/release/cfn-guard.exe "stage/$name/cfn-guard.exe" + tar -czf "artifacts/$version/$name.tar.gz" -C stage $name + + $env:GUARD_DOWNLOAD_BASE_URL = "file://$($PWD.Path -replace '\\','/')/artifacts" + ./install-guard.ps1 -Version $version + + # The installer reports success on its own terms; this checks it installed the binary we + # just built rather than something already on the runner. + $built = (Get-FileHash target/release/cfn-guard.exe -Algorithm SHA256).Hash + $installed = (Get-FileHash "$env:USERPROFILE\.guard\$major\$name\cfn-guard.exe" -Algorithm SHA256).Hash + if ($built -ne $installed) { + throw "installed binary ($installed) does not match this branch's build ($built)" + } + Write-Host "installed binary matches this branch's build ($built)" linting: name: Linting check (clippy) diff --git a/install-guard.ps1 b/install-guard.ps1 index f3032bf0a..42429b0fc 100644 --- a/install-guard.ps1 +++ b/install-guard.ps1 @@ -1,10 +1,45 @@ +# Downloads and installs cfn-guard from GitHub releases on Windows. +# +# Parameters: +# -Version install this exact release tag instead of resolving the latest one. Skips the +# GitHub API entirely, which is the only part of this script that can be rate +# limited. Mirrors -v in install-guard.sh. +# +# Environment: +# GITHUB_TOKEN when set, authenticates the release lookup. The anonymous GitHub API +# allows 60 requests per hour per source IP, shared by everyone behind +# the same address, so a corporate NAT, a VPN or a CI runner can exhaust +# it through no fault of the caller. The `gh` CLI, if installed and +# logged in, is preferred over this and needs no setup. +# GUARD_DOWNLOAD_BASE_URL overrides where release archives are fetched from. Defaults to the +# GitHub releases URL. Set it to a file:// or http:// prefix to install +# an archive built locally, which is how this script is tested against +# the code under review rather than against the last release, and what +# makes an air-gapped install possible. +param( + [string]$Version +) + +# Total seconds we are willing to spend waiting across all retries. A primary rate limit can be up +# to an hour from reset, and an installer that appears to hang for an hour is worse than one that +# fails with an explanation, so past this we stop and say what to do about it. +$script:MaxTotalWaitSeconds = 300 +# Attempts per request, and the first backoff delay when the server tells us nothing more specific. +$script:MaxAttempts = 5 +$script:BaseDelaySeconds = 2 + +$script:GitHubApi = "https://api.github.com/repos/aws-cloudformation/cloudformation-guard" +$script:DefaultDownloadBaseUrl = "https://github.com/aws-cloudformation/cloudformation-guard/releases/download" + function main { + param([string]$RequestedVersion) + # Check for deps and if the user is in an admin shell check_requirements # Log to the user what version and archType we're trying to install $archType = Get-ArchType - $majorVersion, $version = Get-Versions + $majorVersion, $version = Get-GuardVersion -RequestedVersion $RequestedVersion Write-Host "Installing cfn-guard version $version for $archType architecture" # Create the guard directory & bin directory @@ -15,8 +50,9 @@ function main { # Are already present mkdir $guardDir, $binDir -ErrorAction SilentlyContinue | Out-Null - # Download the latest release into the temp directory - $downloadUrl = "https://github.com/aws-cloudformation/cloudformation-guard/releases/download/$version/cfn-guard-v$majorVersion-$archType-windows-latest.tar.gz" + # Download the release into the temp directory + $baseUrl = if ($env:GUARD_DOWNLOAD_BASE_URL) { $env:GUARD_DOWNLOAD_BASE_URL } else { $script:DefaultDownloadBaseUrl } + $downloadUrl = "$baseUrl/$version/cfn-guard-v$majorVersion-$archType-windows-latest.tar.gz" $tmpFile = "$env:TEMP\guard.tar.gz" download_file_to_path $downloadUrl $tmpFile @@ -41,24 +77,161 @@ function main { Write-Host "Done." } +# Architecture from .NET rather than WMI. Get-WmiObject was removed in PowerShell 6, and +# Get-CimInstance, its documented replacement, exists only on Windows -- neither can be exercised +# outside a Windows PowerShell host, so neither is testable before CI runs. RuntimeInformation is +# part of the framework, is present in every supported host, and reports the OS architecture +# directly, which is what the release archive name needs. function Get-ArchType { - $archtype = (Get-WmiObject -Class Win32_Processor).Architecture + $archtype = [System.Runtime.InteropServices.RuntimeInformation]::OSArchitecture switch ($archtype) { - 12 { "aarch64" } - 9 { "x86_64" } - 0 { "i686" } + "Arm64" { "aarch64" } + "X64" { "x86_64" } + "X86" { "i686" } default { err "Unsupported architecture type $archtype" } } } -function Get-Versions { +# Resolve the release tag to install, preferring whichever mechanism needs the least from the +# caller. +# +# 1. An explicit -Version, which skips the API and so cannot be rate limited. +# 2. `gh`, if installed and authenticated. It reuses credentials the caller already has, so it is +# both authenticated and free of any setup on our part. +# 3. The REST API, authenticated when GITHUB_TOKEN is present and anonymous otherwise. The +# anonymous path is the one subject to the 60/hour per-IP limit. +function Get-GuardVersion { + param([string]$RequestedVersion) + + if ($RequestedVersion) { + Write-Host "Using the requested version $RequestedVersion" + return $RequestedVersion.Split('.')[0], $RequestedVersion + } + Write-Host "Getting the latest release version online" - $latestRelease = Invoke-RestMethod -Uri "https://api.github.com/repos/aws-cloudformation/cloudformation-guard/releases/latest" - $tag_name = $latestRelease.tag_name - $majorVersion = $tag_name.Split('.')[0] - $version = $tag_name - Write-Host "Latest release is $version" - return $majorVersion, $version + + $tag = Get-TagFromGhCli + if (-not $tag) { + $latestRelease = Invoke-GitHubApiWithBackoff -Uri "$script:GitHubApi/releases/latest" + $tag = $latestRelease.tag_name + } + if (-not $tag) { + err "unable to determine which cfn-guard version to install" + } + + Write-Host "Latest release is $tag" + return $tag.Split('.')[0], $tag +} + +# The tag according to the gh CLI, or $null when gh is absent, unauthenticated, or unhappy. Never +# fatal on its own: the REST paths are still worth trying. +function Get-TagFromGhCli { + if (-not (Get-Command gh -ErrorAction SilentlyContinue)) { + return $null + } + gh auth status *> $null + if ($LASTEXITCODE -ne 0) { + return $null + } + $tag = gh release view --repo aws-cloudformation/cloudformation-guard --json tagName --jq ".tagName" 2>$null + if ($LASTEXITCODE -ne 0 -or -not $tag) { + Write-Host "gh was available but did not return a release; falling back to the REST API" + return $null + } + return $tag.Trim() +} + +# GET a GitHub API URL, honouring the API's own backoff signals. +# +# The API tells us how long to wait and we listen, rather than guessing: retry-after on a +# secondary limit, and x-ratelimit-reset when the primary limit is exhausted. Blind exponential +# backoff would retry straight into an empty quota and report a network error for what is really a +# quota problem. +function Invoke-GitHubApiWithBackoff { + param([string]$Uri) + + $headers = @{ "Accept" = "application/vnd.github+json"; "User-Agent" = "install-guard" } + if ($env:GITHUB_TOKEN) { + $headers["Authorization"] = "Bearer $env:GITHUB_TOKEN" + } + + $attempt = 1 + $delay = $script:BaseDelaySeconds + $waited = 0 + + while ($true) { + try { + return Invoke-RestMethod -Uri $Uri -Headers $headers -ErrorAction Stop + } catch { + $response = $_.Exception.Response + $status = 0 + if ($response) { $status = [int]$response.StatusCode } + $sleep = Get-BackoffDelay -Response $response -Fallback $delay + + if ($attempt -ge $script:MaxAttempts -or ($waited + $sleep) -gt $script:MaxTotalWaitSeconds) { + Write-Host "GitHub API request failed with HTTP $status after $attempt attempt(s)." + if ($status -eq 403 -or $status -eq 429) { + Write-Host "This is a rate limit rather than a problem with the release." + Write-Host "Authenticate to raise it: set GITHUB_TOKEN, or run 'gh auth login'," + Write-Host "or pass -Version to skip the lookup entirely." + } + err "unable to reach the GitHub API: $($_.Exception.Message)" + } + + Write-Host "attempt $attempt of $($script:MaxAttempts) got HTTP $status; retrying in $sleep s" + Start-Sleep -Seconds $sleep + $waited = $waited + $sleep + $attempt = $attempt + 1 + $delay = $delay * 2 + } + } +} + +# Seconds to wait before the next attempt, from the response headers when they say, else $Fallback. +function Get-BackoffDelay { + param($Response, [int]$Fallback) + + # retry-after is authoritative and is what a secondary limit returns. + $retryAfter = Get-HeaderValue -Response $Response -Name "Retry-After" + if ($retryAfter -and [int]::TryParse($retryAfter, [ref]$null)) { + $seconds = [int]$retryAfter + if ($seconds -gt 0) { return $seconds } + } + + # A primary limit is exhausted when remaining is 0; reset is an epoch second. + $remaining = Get-HeaderValue -Response $Response -Name "X-RateLimit-Remaining" + $reset = Get-HeaderValue -Response $Response -Name "X-RateLimit-Reset" + if ($remaining -eq "0" -and $reset) { + $now = [int][double]::Parse((Get-Date -UFormat %s)) + $until = [int]$reset - $now + 1 + if ($until -gt 0) { return $until } + } + + return $Fallback +} + +# One header value, read defensively. Windows PowerShell hands back a WebHeaderCollection with a +# string indexer while PowerShell 7 hands back HttpResponseHeaders with TryGetValues, and this +# script has to work under both. An unreadable header is not an error; it just means we fall back +# to exponential backoff. +function Get-HeaderValue { + param($Response, [string]$Name) + + if (-not $Response) { return $null } + try { + $headers = $Response.Headers + if ($null -eq $headers) { return $null } + if ($headers -is [System.Net.WebHeaderCollection]) { + return $headers[$Name] + } + $values = $null + if ($headers.TryGetValues($Name, [ref]$values)) { + return ($values | Select-Object -First 1) + } + } catch { + return $null + } + return $null } function extract_tar { @@ -72,7 +245,7 @@ function extract_tar { function err { param($message) Write-Host $message -ForegroundColor Red - throw + throw $message } function check_cmd_present { @@ -82,14 +255,31 @@ function check_cmd_present { } } +# Fetch the release archive, retried. Never authenticated: the archive redirects to a separate +# download host and a credential has no business travelling there. function download_file_to_path { param($url, $outputFile) - try { - Write-Host "Downloading $url to $outputFile" - $webClient = New-Object System.Net.WebClient - $webClient.DownloadFile($url, $outputFile) - } catch { - err "Failed to download cfn-guard release. Please try again." + + $attempt = 1 + $delay = $script:BaseDelaySeconds + $waited = 0 + + while ($true) { + try { + Write-Host "Downloading $url to $outputFile" + $webClient = New-Object System.Net.WebClient + $webClient.DownloadFile($url, $outputFile) + return + } catch { + if ($attempt -ge $script:MaxAttempts -or ($waited + $delay) -gt $script:MaxTotalWaitSeconds) { + err "Failed to download cfn-guard release from $url after $attempt attempt(s)." + } + Write-Host "attempt $attempt of $($script:MaxAttempts) failed; retrying in $delay s" + Start-Sleep -Seconds $delay + $waited = $waited + $delay + $attempt = $attempt + 1 + $delay = $delay * 2 + } } } @@ -127,4 +317,4 @@ function update_path { } } -main +main -RequestedVersion $Version diff --git a/install-guard.sh b/install-guard.sh index 50751ae48..259fe310e 100644 --- a/install-guard.sh +++ b/install-guard.sh @@ -4,6 +4,30 @@ # It detects platforms, downloads the pre-built binary for the specified version (default latest), installs # it in the ~/.guard/$MAJOR_VER/cfn-guard-v$MAJOR_VER-$OS_TYPE-latest/cfn-guard and symlinks ~/.guard/bin # to the last installed binary. +# +# Environment: +# GITHUB_TOKEN when set, authenticates the release lookup. The anonymous GitHub API +# allows 60 requests per hour per source IP, shared by everyone behind the +# same address, so a corporate NAT, a VPN or a CI runner can exhaust it +# through no fault of the caller. An authenticated request is counted +# against the token instead. The `gh` CLI, if installed and logged in, is +# preferred over this and needs no setup. +# GUARD_DOWNLOAD_BASE_URL overrides where release archives are fetched from. Defaults to the +# GitHub releases URL. Set it to a file:// or http:// prefix to install an +# archive built locally, which is how the install scripts are tested +# against the code under review rather than against the last release, and +# what makes an air-gapped install possible. + +# Total seconds we are willing to spend waiting across all retries. A primary rate limit can be up +# to an hour from reset, and an installer that appears to hang for an hour is worse than one that +# fails with an explanation, so past this we stop and say what to do about it. +MAX_TOTAL_WAIT=300 +# Attempts per request, and the first backoff delay when the server tells us nothing more specific. +MAX_ATTEMPTS=5 +BASE_DELAY=2 + +GITHUB_API="https://api.github.com/repos/aws-cloudformation/cloudformation-guard" +DEFAULT_DOWNLOAD_BASE_URL="https://github.com/aws-cloudformation/cloudformation-guard/releases/download" main() { if ! (check_cmd curl || check_cmd wget); then @@ -18,25 +42,34 @@ main() { get_os_type get_arch_type - get_version "$@" | - while - read -r VERSION - do - echo "Installing cfn-guard version '${VERSION}'..." - MAJOR_VER=$(echo "$VERSION" | awk -F '.' '{ print $1 }') - mkdir -p ~/.guard/"$MAJOR_VER" ~/.guard/bin || - err "unable to make directories ~/.guard/$MAJOR_VER, ~/.guard/bin" - get_os_type - download https://github.com/aws-cloudformation/cloudformation-guard/releases/download/"$VERSION"/cfn-guard-v"$MAJOR_VER"-"$ARCH_TYPE"-"$OS_TYPE"-latest.tar.gz >/tmp/guard.tar.gz || - err "unable to download https://github.com/aws-cloudformation/cloudformation-guard/releases/download/$VERSION/cfn-guard-v$MAJOR_VER-$ARCH_TYPE-$OS_TYPE-latest.tar.gz" - tar -C ~/.guard/"$MAJOR_VER" -xzf /tmp/guard.tar.gz || - err "unable to untar /tmp/guard.tar.gz" - ln -sf ~/.guard/"$MAJOR_VER"/cfn-guard-v"$MAJOR_VER"-"$ARCH_TYPE"-"$OS_TYPE"-latest/cfn-guard ~/.guard/bin || - err "unable to symlink to ~/.guard/bin directory" - ~/.guard/bin/cfn-guard help || - err "cfn-guard was not installed properly" - echo "Remember to SET PATH include PATH=\${PATH}:~/.guard/bin" - done + + # Assigned rather than piped into a `while read` loop. err() exits, but when it was reached + # from the left side of a pipeline it only exited that subshell: the pipeline's status came + # from the loop, which had simply read nothing, so a failed release lookup left this script + # exiting 0 with nothing installed. Command substitution propagates the status instead. + VERSION=$(get_version "$@") || exit 1 + if [ -z "$VERSION" ]; then + err "unable to determine which cfn-guard version to install" + fi + + echo "Installing cfn-guard version '${VERSION}'..." + MAJOR_VER=$(echo "$VERSION" | awk -F '.' '{ print $1 }') + mkdir -p ~/.guard/"$MAJOR_VER" ~/.guard/bin || + err "unable to make directories ~/.guard/$MAJOR_VER, ~/.guard/bin" + + _base_url="${GUARD_DOWNLOAD_BASE_URL:-$DEFAULT_DOWNLOAD_BASE_URL}" + _archive="cfn-guard-v${MAJOR_VER}-${ARCH_TYPE}-${OS_TYPE}-latest.tar.gz" + _url="${_base_url}/${VERSION}/${_archive}" + + download "$_url" >/tmp/guard.tar.gz || + err "unable to download $_url" + tar -C ~/.guard/"$MAJOR_VER" -xzf /tmp/guard.tar.gz || + err "unable to untar /tmp/guard.tar.gz" + ln -sf ~/.guard/"$MAJOR_VER"/cfn-guard-v"$MAJOR_VER"-"$ARCH_TYPE"-"$OS_TYPE"-latest/cfn-guard ~/.guard/bin || + err "unable to symlink to ~/.guard/bin directory" + ~/.guard/bin/cfn-guard help || + err "cfn-guard was not installed properly" + echo "Remember to SET PATH include PATH=\${PATH}:~/.guard/bin" } get_os_type() { @@ -62,25 +95,155 @@ get_version() { # Get the version from the -v option, if provided. while getopts 'v:' OPTION; do case "$OPTION" in - v) - VERSION="$OPTARG" - ;; - ?) - err "Usage: install-guard.sh [-v ]" - ;; + v) + VERSION="$OPTARG" + ;; + ?) + err "Usage: install-guard.sh [-v ]" + ;; esac done # If version is not provided default to the latest version. - if [ -z "$VERSION" ] ; then + if [ -z "$VERSION" ]; then get_latest_release else echo "$VERSION" fi } +# Resolve the latest release tag, preferring whichever mechanism needs the least from the caller. +# +# 1. `gh`, if installed and authenticated. It reuses credentials the caller already has, so it is +# both authenticated and free of any setup on our part. +# 2. The REST API with GITHUB_TOKEN, when one is in the environment. +# 3. The REST API anonymously, which is the path subject to the 60/hour per-IP limit. get_latest_release() { - download https://api.github.com/repos/aws-cloudformation/cloudformation-guard/releases/latest | - awk -F '"' '/tag_name/ { print $4 }' + if check_cmd gh && gh auth status >/dev/null 2>&1; then + if _tag=$(gh release view --repo aws-cloudformation/cloudformation-guard \ + --json tagName --jq '.tagName' 2>/dev/null) && [ -n "$_tag" ]; then + echo "$_tag" + return 0 + fi + # Fall through rather than fail: gh being present does not guarantee it can reach the + # API, and the plain HTTP paths below may still work. + echo "gh was available but did not return a release; falling back to the REST API" >&2 + fi + + github_api "${GITHUB_API}/releases/latest" | + awk -F '"' '/tag_name/ { print $4; exit }' +} + +# GET a GitHub API URL to stdout, honouring the API's own backoff signals. +# +# The API tells us how long to wait and we listen, rather than guessing: `retry-after` on a +# secondary limit, and `x-ratelimit-reset` when the primary limit is exhausted. Blind exponential +# backoff would retry straight into an empty quota and report a network error for what is really a +# quota problem. +github_api() { + _url="$1" + _attempt=1 + _delay="$BASE_DELAY" + _waited=0 + + # Header inspection needs curl. With only wget available we still retry, just without the + # server's guidance, which is strictly better than one attempt. + if ! check_cmd curl; then + _body=$(retry_wget "$_url") || return 1 + echo "$_body" + return 0 + fi + + _hdr=$(mktemp) || err "unable to create a temporary file" + _body=$(mktemp) || err "unable to create a temporary file" + + while :; do + # The token goes in a config file on stdin rather than on the command line. An + # Authorization header in argv is readable from `ps` by anyone else on the host for + # the life of the request, which matters on shared build machines. Only ever sent to + # api.github.com: the release archive redirects to a separate download host and a + # credential has no business travelling there. + if [ -n "${GITHUB_TOKEN:-}" ]; then + _code=$(printf 'header = "Authorization: Bearer %s"\n' "$GITHUB_TOKEN" | + curl -sS -K - -o "$_body" -D "$_hdr" -w '%{http_code}' "$_url" 2>/dev/null) + else + _code=$(curl -sS -o "$_body" -D "$_hdr" -w '%{http_code}' "$_url" 2>/dev/null) + fi + + if [ "$_code" = "200" ]; then + cat "$_body" + rm -f "$_hdr" "$_body" + return 0 + fi + + _sleep=$(backoff_seconds "$_hdr" "$_delay") + + if [ "$_attempt" -ge "$MAX_ATTEMPTS" ] || + [ $((_waited + _sleep)) -gt "$MAX_TOTAL_WAIT" ]; then + echo "GitHub API request failed with HTTP $_code after ${_attempt} attempt(s)." >&2 + if [ "$_code" = "403" ] || [ "$_code" = "429" ]; then + echo "This is a rate limit rather than a problem with the release." >&2 + echo "Authenticate to raise it: set GITHUB_TOKEN, or run 'gh auth login'," >&2 + echo "or pass an explicit version with -v to skip the lookup entirely." >&2 + fi + rm -f "$_hdr" "$_body" + return 1 + fi + + echo "attempt ${_attempt} of ${MAX_ATTEMPTS} got HTTP ${_code}; retrying in ${_sleep}s" >&2 + sleep "$_sleep" + _waited=$((_waited + _sleep)) + _attempt=$((_attempt + 1)) + _delay=$((_delay * 2)) + done +} + +# Seconds to wait before the next attempt, from the response headers when they say, else $2. +backoff_seconds() { + _hdrfile="$1" + _fallback="$2" + + # retry-after is authoritative and is what a secondary limit returns. + _retry_after=$(awk 'tolower($1) ~ /^retry-after:/ { gsub(/\r/, "", $2); print $2; exit }' "$_hdrfile") + if [ -n "$_retry_after" ] && [ "$_retry_after" -gt 0 ] 2>/dev/null; then + echo "$_retry_after" + return 0 + fi + + # A primary limit is exhausted when remaining is 0; reset is an epoch second. + _remaining=$(awk 'tolower($1) ~ /^x-ratelimit-remaining:/ { gsub(/\r/, "", $2); print $2; exit }' "$_hdrfile") + _reset=$(awk 'tolower($1) ~ /^x-ratelimit-reset:/ { gsub(/\r/, "", $2); print $2; exit }' "$_hdrfile") + if [ "$_remaining" = "0" ] && [ -n "$_reset" ]; then + _now=$(date +%s) + _until=$((_reset - _now + 1)) + if [ "$_until" -gt 0 ]; then + echo "$_until" + return 0 + fi + fi + + echo "$_fallback" +} + +retry_wget() { + _url="$1" + _attempt=1 + _delay="$BASE_DELAY" + _waited=0 + while :; do + if _out=$(wget -qO- "$_url" 2>/dev/null); then + echo "$_out" + return 0 + fi + if [ "$_attempt" -ge "$MAX_ATTEMPTS" ] || [ $((_waited + _delay)) -gt "$MAX_TOTAL_WAIT" ]; then + echo "unable to fetch $_url after ${_attempt} attempt(s)" >&2 + return 1 + fi + echo "attempt ${_attempt} of ${MAX_ATTEMPTS} failed; retrying in ${_delay}s" >&2 + sleep "$_delay" + _waited=$((_waited + _delay)) + _attempt=$((_attempt + 1)) + _delay=$((_delay * 2)) + done } err() { @@ -98,16 +261,28 @@ check_cmd() { command -v "$1" >/dev/null 2>&1 } +# Fetch a release archive to stdout. Retried, but never authenticated: see auth_header_args. download() { - if check_cmd curl; then - if ! (curl -fsSL "$1"); then - err "error attempting to download from the github repository" + _url="$1" + _attempt=1 + _delay="$BASE_DELAY" + _waited=0 + while :; do + if check_cmd curl; then + curl -fsSL "$_url" && return 0 + else + wget -qO- "$_url" && return 0 fi - else - if ! (wget -qO- "$1"); then - err "error attempting to download from the github repository" + if [ "$_attempt" -ge "$MAX_ATTEMPTS" ] || [ $((_waited + _delay)) -gt "$MAX_TOTAL_WAIT" ]; then + echo "error attempting to download from the github repository: $_url" >&2 + return 1 fi - fi + echo "attempt ${_attempt} of ${MAX_ATTEMPTS} failed; retrying in ${_delay}s" >&2 + sleep "$_delay" + _waited=$((_waited + _delay)) + _attempt=$((_attempt + 1)) + _delay=$((_delay * 2)) + done } get_arch_type() { From 583c3b86168f8b7ba60d9e16a8af1c8336e22bcf Mon Sep 17 00:00:00 2001 From: Madison Steiner Date: Mon, 17 Aug 2026 19:21:22 +0000 Subject: [PATCH 09/10] Gate install-guard.ps1 on PSScriptAnalyzer install-guard.sh has been guarded by shellcheck since it was written; install-guard.ps1 had no static analysis at all. That is how a Get-WmiObject call survived in it: PowerShell 6 removed the WMI cmdlets and this workflow runs pwsh 7, so the script referenced a cmdlet its own CI shell does not have, and nothing in the repository was looking. PSScriptAnalyzer is the PowerShell counterpart, and it reports that call as PSAvoidUsingWMICmdlet. Run against the script as it stood before the preceding commit it finds two real problems and fourteen instances of one deliberate choice; run against it now it finds none, so the gate starts clean rather than with a backlog to grandfather in. Configured by .github/PSScriptAnalyzerSettings.psd1 at Error, Warning and Information, so a new finding fails the build. PSAvoidUsingWriteHost is the one exclusion, with the reasoning recorded next to it: Get-ArchType and Get-GuardVersion return their values through the pipeline, so routing progress commentary to Write-Output as the rule advises would mix that text into their return values and break them. Runs on ubuntu rather than windows: the analyser is platform independent, and a Linux runner is cheaper. Ordering: this depends on the script fixes in the preceding commit. Applied to main as it stands today the gate fails, on the WMI cmdlet and on Get-Versions using a plural noun. --- .github/PSScriptAnalyzerSettings.psd1 | 27 +++++++++++++++++++++++++++ .github/workflows/pr.yml | 24 ++++++++++++++++++++++++ 2 files changed, 51 insertions(+) create mode 100644 .github/PSScriptAnalyzerSettings.psd1 diff --git a/.github/PSScriptAnalyzerSettings.psd1 b/.github/PSScriptAnalyzerSettings.psd1 new file mode 100644 index 000000000..e5bd15875 --- /dev/null +++ b/.github/PSScriptAnalyzerSettings.psd1 @@ -0,0 +1,27 @@ +# PSScriptAnalyzer configuration for the CI lint gate. +# +# This is the PowerShell counterpart to the shellcheck job that already guards install-guard.sh. +# The Windows installer had no static analysis at all, which is how a WMI cmdlet removed in +# PowerShell 6 sat in it unnoticed -- the CI shell is pwsh 7, so nothing in the repository would +# have caught it before a user did. +# +# The gate runs with every default rule enabled except the exclusion below, so a new finding fails +# the build rather than accumulating. +@{ + Severity = @('Error', 'Warning', 'Information') + + ExcludeRules = @( + # PSAvoidUsingWriteHost objects to Write-Host because its output cannot be captured or + # redirected, which is the right call for a module or a library returning data to a caller. + # install-guard.ps1 is neither: it is an interactive installer whose output is progress + # commentary for a human watching a terminal, and the README documents piping it straight + # into Invoke-Expression. Write-Output would put that commentary into the pipeline, where + # it would be mistaken for the return value of the functions that emit it -- Get-ArchType + # and Get-GuardVersion both return values by writing to the pipeline, so mixing progress + # text into it would actively break them. + # + # Suppressed here rather than per-line: it applies to every Write-Host in the file for the + # same reason, and twenty-odd inline suppressions would obscure the code they annotate. + 'PSAvoidUsingWriteHost' + ) +} diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index c230876c3..a966ec764 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -42,6 +42,30 @@ jobs: - name: Shellcheck run: shellcheck install-guard.sh + # The PowerShell counterpart to shellcheck. install-guard.ps1 had no static analysis, which is + # how a WMI cmdlet that PowerShell 6 removed sat in it unnoticed -- this workflow runs pwsh 7, + # so nothing here would have caught it before a user did. Configured by + # .github/PSScriptAnalyzerSettings.psd1; runs on ubuntu because the analyser is + # platform independent and a Linux runner is cheaper than a Windows one. + psscriptanalyzer: + name: PSScriptAnalyzer + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + - name: Install PSScriptAnalyzer + shell: pwsh + run: Install-Module -Name PSScriptAnalyzer -Scope CurrentUser -Force -AcceptLicense + - name: Analyze install-guard.ps1 + shell: pwsh + run: | + $findings = Invoke-ScriptAnalyzer -Path install-guard.ps1 ` + -Settings .github/PSScriptAnalyzerSettings.psd1 + if ($findings) { + $findings | Format-Table -AutoSize Severity, Line, RuleName, Message | Out-String -Width 200 | Write-Host + throw "PSScriptAnalyzer reported $($findings.Count) finding(s)" + } + Write-Host "PSScriptAnalyzer: no findings" + formatting: name: Formatting check (cargo fmt) runs-on: ubuntu-latest From c953a258f914ebf48586c64dc4ea747a68b69589 Mon Sep 17 00:00:00 2001 From: Madison Steiner Date: Wed, 19 Aug 2026 18:02:33 +0000 Subject: [PATCH 10/10] Keep the new comments ASCII, like the rest of the crate Two horizontal-ellipsis characters in comments this branch added, in a file whose merge-base has no non-ASCII byte anywhere. `...` says the same thing and does not depend on the reader's terminal or the diff viewer's encoding. Found by scanning for non-ASCII rather than by reading, which is the only way this class shows up: it does not fail a build, `typos` does not flag it, and a diff renders it as an ordinary character. The same scan found 53 em dashes and a stray CJK character on the sibling PR. --- guard/tests/utils.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/guard/tests/utils.rs b/guard/tests/utils.rs index 906206644..51d62a21d 100644 --- a/guard/tests/utils.rs +++ b/guard/tests/utils.rs @@ -109,7 +109,7 @@ pub fn get_full_path_for_resource_file(path: &str) -> String { /// there, because `get_full_path_for_resource_file` builds them from it. /// /// This replaced a `$HOME`-based version that first rewrote the home directory to `~` and then -/// matched `~/…`, which had two bugs: +/// matched `~/...`, which had two bugs: /// /// - `$HOME` was substituted by plain substring match, so a checkout whose path merely /// *contains* `$HOME` was corrupted rather than normalised. With `HOME=/home/u`, the path @@ -122,7 +122,7 @@ pub fn get_full_path_for_resource_file(path: &str) -> String { /// /// Anchoring on the crate directory also leaves URLs alone by construction, which a regex over /// bare absolute paths would not: the SARIF fixtures contain -/// `//docs.oasis-open.org/…/sarif-schema-2.1.0.json`, and reducing that to its basename would +/// `//docs.oasis-open.org/.../sarif-schema-2.1.0.json`, and reducing that to its basename would /// break them. It is the only slash-bearing file reference in `guard/resources`, so the /// distinction is load-bearing for exactly one fixture and easy to lose. pub fn replace_path_with_filenames(text: String) -> String {