diff --git a/policy/BUILD b/policy/BUILD index fe5d0d341..2c00dcf4d 100644 --- a/policy/BUILD +++ b/policy/BUILD @@ -122,6 +122,7 @@ cc_library( ":cel_policy_parser", ":cel_policy_validation_result", "//checker:type_check_issue", + "//checker:type_checker", "//checker:validation_result", "//common:ast", "//common:ast_rewrite", diff --git a/policy/compiler.cc b/policy/compiler.cc index 555610efc..0f3b7b20b 100644 --- a/policy/compiler.cc +++ b/policy/compiler.cc @@ -37,6 +37,7 @@ #include "absl/strings/string_view.h" #include "absl/types/span.h" #include "checker/type_check_issue.h" +#include "checker/type_checker.h" #include "checker/validation_result.h" #include "common/ast.h" #include "common/ast_rewrite.h" @@ -64,8 +65,8 @@ namespace { constexpr absl::string_view kCelBlock = "cel.@block"; enum class RuleSemantics { - // TODO(b/506179116): will also need "aggregate" or similar concept. kFirstMatch, + kAggregate, kNotForUseWithExhaustiveSwitchStatements, }; @@ -76,6 +77,9 @@ void AbslStringify(Sink& s, RuleSemantics semantics) { case RuleSemantics::kFirstMatch: s.Append("first_match"); return; + case RuleSemantics::kAggregate: + s.Append("aggregate"); + return; default: s.Append(""); return; @@ -116,6 +120,7 @@ struct CompiledRule { std::vector matches; // Not set if cannot be determined. std::optional result_type; + RuleSemantics semantics = RuleSemantics::kFirstMatch; }; std::optional GetOutputType( @@ -155,9 +160,6 @@ class IntermediateCompiledPolicy { } absl::string_view description() const { return description_; } - void set_semantics(RuleSemantics semantics) { semantics_ = semantics; } - RuleSemantics semantics() const { return semantics_; } - void set_policy_source(const CelPolicySource* absl_nullable src) { policy_source_ = src; } @@ -169,7 +171,6 @@ class IntermediateCompiledPolicy { std::string name_; std::string display_name_; std::string description_; - RuleSemantics semantics_ = RuleSemantics::kFirstMatch; const CelPolicySource* absl_nullable policy_source_ = nullptr; CompiledRule root_rule_; @@ -454,10 +455,9 @@ class PolicyCompiler { absl::Status CompileRule(const Rule& rule, const cel::Compiler* env, CompiledRule* out) { - if (rule.is_aggregate()) { - return absl::UnimplementedError("Aggregate rules are not yet supported"); - } out->id = rule.id(); + out->semantics = rule.is_aggregate() ? RuleSemantics::kAggregate + : RuleSemantics::kFirstMatch; std::unique_ptr buf; absl::flat_hash_set seen_variables; @@ -531,7 +531,6 @@ class PolicyCompiler { IntermediateCompiledPolicy* out) { src_ = policy.source(); out->set_policy_source(src_); - out->set_semantics(RuleSemantics::kFirstMatch); out->set_name(policy.name().value()); out->set_display_name( policy.display_name().value_or(ValueString{}).value()); @@ -550,12 +549,11 @@ class PolicyCompiler { bool IsExhaustive(const CompiledRule& rule); -class FirstMatchComposer { +class PolicyComposer { public: - FirstMatchComposer(const IntermediateCompiledPolicy& icp, - const Compiler& compiler, - const CompilePolicyOptions& options, - policy_internal::IssueReporter& issues) + PolicyComposer(const IntermediateCompiledPolicy& icp, + const Compiler& compiler, const CompilePolicyOptions& options, + policy_internal::IssueReporter& issues) : issues_(issues), icp_(icp), compiler_(compiler), options_(options) {} absl::Status Compose(); @@ -586,11 +584,16 @@ class FirstMatchComposer { absl::flat_hash_map ResolveBlockIndexes(const Ast& ast); - bool CheckMatchStructure(const CompiledRule& rule); + bool CheckMatchStructure(const CompiledRule& rule, + bool has_aggregate_ancestor = false); // Returns true if already optional wrapped. absl::StatusOr ComposeRule(const CompiledRule& rule, Expr& init, Expr& insertion_expr); + absl::StatusOr ComposeFirstMatchRule(const CompiledRule& rule, + Expr& init, Expr& insertion_expr); + absl::Status ComposeAggregateRule(const CompiledRule& rule, Expr& init, + Expr& insertion_expr); // returns true if already optional wrapped. absl::StatusOr ComposeProduction( @@ -612,9 +615,7 @@ class FirstMatchComposer { std::unique_ptr ast_; }; -absl::Status FirstMatchComposer::Compose() { - ABSL_DCHECK(icp_.semantics() == RuleSemantics::kFirstMatch); - +absl::Status PolicyComposer::Compose() { if (icp_.policy_source() != nullptr) { ABSL_DCHECK(icp_.policy_source()->content() != nullptr); absl::Span line_offsets = @@ -674,6 +675,20 @@ bool IsTriviallyTrueCondition(const CompiledMatch& match) { return false; } +bool IsLiterallyFalseCondition(const CompiledMatch& match) { + if (!match.condition.has_value() || match.condition->ast == nullptr) { + return false; + } + const cel::Expr& expr = match.condition->ast->root_expr(); + if (expr.has_const_expr()) { + const cel::Constant& const_expr = expr.const_expr(); + if (const_expr.has_bool_value() && !const_expr.bool_value()) { + return true; + } + } + return false; +} + bool IsExhaustive(const CompiledRule& rule); bool IsExhaustive(const CompiledMatch& match) { @@ -689,6 +704,9 @@ bool IsExhaustive(const CompiledMatch& match) { } bool IsExhaustive(const CompiledRule& rule) { + if (rule.semantics == RuleSemantics::kAggregate) { + return true; + } if (rule.matches.empty()) { // Validation should fail, but generalization would be false. return false; @@ -711,17 +729,23 @@ bool IsExhaustive(const CompiledRule& rule) { return has_default; } -bool FirstMatchComposer::CheckMatchStructure(const CompiledRule& rule) { +bool PolicyComposer::CheckMatchStructure(const CompiledRule& rule, + bool has_aggregate_ancestor) { + bool valid = true; + if (has_aggregate_ancestor && rule.semantics == RuleSemantics::kAggregate) { + issues_.ReportError(rule.id, "nested aggregate rules are not allowed"); + valid = false; + } + if (rule.matches.empty()) { issues_.ReportError(rule.id, "rule does not specify match conditions"); return false; } - bool valid = true; bool seen_trivially_true = false; for (const auto& match : rule.matches) { - if (seen_trivially_true) { + if (seen_trivially_true && rule.semantics == RuleSemantics::kFirstMatch) { if (std::holds_alternative(match.production)) { issues_.ReportError(match.id, "match creates unreachable outputs"); } else if (std::holds_alternative>( @@ -735,11 +759,18 @@ bool FirstMatchComposer::CheckMatchStructure(const CompiledRule& rule) { seen_trivially_true = true; } + if (IsLiterallyFalseCondition(match)) { + issues_.ReportError(match.id, "condition is always false"); + valid = false; + } + if (auto* nested_rule = std::get_if>(&match.production); nested_rule != nullptr) { ABSL_DCHECK(*nested_rule != nullptr); - if (!CheckMatchStructure(**nested_rule)) { + const bool next_has_aggregate_ancestor = + has_aggregate_ancestor || rule.semantics == RuleSemantics::kAggregate; + if (!CheckMatchStructure(**nested_rule, next_has_aggregate_ancestor)) { valid = false; } } @@ -748,7 +779,7 @@ bool FirstMatchComposer::CheckMatchStructure(const CompiledRule& rule) { return valid; } -std::optional FirstMatchComposer::ResolvePolicyVariable( +std::optional PolicyComposer::ResolvePolicyVariable( absl::string_view reference) { for (auto scope_iter = scopes_.rbegin(); scope_iter != scopes_.rend(); ++scope_iter) { @@ -780,9 +811,20 @@ class IndexRewrite : public AstRewriterBase { OptimizerExprFactory& factory_; }; -absl::StatusOr FirstMatchComposer::ComposeRule(const CompiledRule& rule, - Expr& init, - Expr& insertion_expr) { +absl::StatusOr PolicyComposer::ComposeRule(const CompiledRule& rule, + Expr& init, + Expr& insertion_expr) { + if (rule.semantics == RuleSemantics::kAggregate) { + CEL_RETURN_IF_ERROR(ComposeAggregateRule(rule, init, insertion_expr)); + return false; + } + ABSL_DCHECK(rule.semantics == RuleSemantics::kFirstMatch); + return ComposeFirstMatchRule(rule, init, insertion_expr); +} + +absl::StatusOr PolicyComposer::ComposeFirstMatchRule( + const CompiledRule& rule, Expr& init, Expr& insertion_expr) { + ABSL_DCHECK(rule.semantics == RuleSemantics::kFirstMatch); scopes_.emplace_back(); auto pop_scope = absl::MakeCleanup([this]() { scopes_.pop_back(); }); ComposeRuleVariables(rule, init, insertion_expr); @@ -852,7 +894,14 @@ absl::StatusOr FirstMatchComposer::ComposeRule(const CompiledRule& rule, return true; } -absl::StatusOr FirstMatchComposer::ComposeProduction( +absl::Status PolicyComposer::ComposeAggregateRule(const CompiledRule& rule, + Expr& init, + Expr& insertion_expr) { + return absl::UnimplementedError( + "Aggregate rules are not supported in C++ policy engine."); +} + +absl::StatusOr PolicyComposer::ComposeProduction( const CompiledRule& rule, const CompiledMatch::Production& production, Expr& init, Expr& insertion_expr) { if (auto* nested_rule = @@ -877,7 +926,7 @@ absl::StatusOr FirstMatchComposer::ComposeProduction( return false; } -absl::flat_hash_map FirstMatchComposer::ResolveBlockIndexes( +absl::flat_hash_map PolicyComposer::ResolveBlockIndexes( const Ast& ast) { absl::flat_hash_map out; for (auto it = ast.reference_map().begin(); it != ast.reference_map().end(); @@ -896,15 +945,14 @@ absl::flat_hash_map FirstMatchComposer::ResolveBlockIndexes( return out; } -void FirstMatchComposer::MapVariables(Ast& ast) { +void PolicyComposer::MapVariables(Ast& ast) { absl::flat_hash_map edit_map = ResolveBlockIndexes(ast); IndexRewrite rewriter(std::move(edit_map), factory_); AstRewrite(ast.mutable_root_expr(), rewriter); } -void FirstMatchComposer::ComposeRuleVariables(const CompiledRule& rule, - Expr& init, - Expr& insertion_expr) { +void PolicyComposer::ComposeRuleVariables(const CompiledRule& rule, Expr& init, + Expr& insertion_expr) { for (const auto& variable : rule.variables) { Ast ast = *variable.ast.ast; MapVariables(ast); @@ -1131,34 +1179,26 @@ absl::StatusOr CompilePolicy( } CEL_ASSIGN_OR_RETURN(base_compiler, builder->Build()); - switch (icp.semantics()) { - case RuleSemantics::kFirstMatch: { - FirstMatchComposer composer(icp, *base_compiler, options, issues); - CEL_RETURN_IF_ERROR(composer.Compose()); - if (!composer.success()) { - return CelPolicyValidationResult(issues.ReleaseIssues(), - policy.source_ptr()); - } - - auto ast = composer.ReleaseAst(); - Unnester unnester(base_compiler->GetTypeChecker(), std::move(*ast), - options, issues); - CEL_ASSIGN_OR_RETURN(Ast unnested_ast, unnester.Unnest()); + PolicyComposer composer(icp, *base_compiler, options, issues); + CEL_RETURN_IF_ERROR(composer.Compose()); + if (!composer.success()) { + return CelPolicyValidationResult(issues.ReleaseIssues(), + policy.source_ptr()); + } - if (HasErrors(issues)) { - return CelPolicyValidationResult(issues.ReleaseIssues(), - policy.source_ptr()); - } + auto ast = composer.ReleaseAst(); + Unnester unnester(base_compiler->GetTypeChecker(), std::move(*ast), options, + issues); + CEL_ASSIGN_OR_RETURN(Ast unnested_ast, unnester.Unnest()); - CelPolicyValidationResult result = CelPolicyValidationResult( - std::make_unique(std::move(unnested_ast)), {}, - policy.source_ptr()); - return result; - } - default: - return absl::UnimplementedError( - absl::StrCat("Unsupported RuleSemantics: ", icp.semantics())); + if (HasErrors(issues)) { + return CelPolicyValidationResult(issues.ReleaseIssues(), + policy.source_ptr()); } + + CelPolicyValidationResult result = CelPolicyValidationResult( + std::make_unique(std::move(unnested_ast)), {}, policy.source_ptr()); + return result; } } // namespace cel diff --git a/policy/compiler_test.cc b/policy/compiler_test.cc index 31831931a..1b75cbe11 100644 --- a/policy/compiler_test.cc +++ b/policy/compiler_test.cc @@ -116,7 +116,10 @@ absl::StatusOr> ParsePolicyFromYaml( return parse_result.ReleasePolicy(); } -const Ast* GetAstPointer(const std::unique_ptr& ast) { return ast.get(); } +[[maybe_unused]] +const Ast* GetAstPointer(const std::unique_ptr& ast) { + return ast.get(); +} MATCHER(MetadataMapValid, negation ? "has stale metadata map entries" : "has valid metadata maps") { @@ -433,6 +436,90 @@ name: dead_branch testing::HasSubstr("match creates unreachable outputs")); } +TEST(CompilerTest, UnreachableRuleAfterTriviallyTrueCondition) { + absl::string_view yaml = R"yaml( +name: dead_rule_branch +rule: + match: + - condition: true + output: 1 + - rule: + match: + - output: 2 +)yaml"; + ASSERT_OK_AND_ASSIGN(std::unique_ptr policy, + ParsePolicyFromYaml(yaml)); + ASSERT_OK_AND_ASSIGN(std::unique_ptr compiler, BuildTestCompiler()); + ASSERT_OK_AND_ASSIGN(CelPolicyValidationResult result, + CompilePolicy(*compiler, *policy)); + EXPECT_FALSE(result.IsValid()); + EXPECT_THAT(result.FormatIssues(), + testing::HasSubstr("rule creates unreachable outputs")); +} + +TEST(CompilerTest, UnreachableMatchAfterUnconditionalAggregateSubRule) { + absl::string_view yaml = R"yaml( +name: dead_branch_after_aggregate +rule: + match: + - rule: + aggregate: + - condition: x > 0 + output: 1 + - output: 2 +)yaml"; + ASSERT_OK_AND_ASSIGN(std::unique_ptr policy, + ParsePolicyFromYaml(yaml)); + ASSERT_OK_AND_ASSIGN(std::unique_ptr compiler, BuildTestCompiler()); + ASSERT_OK_AND_ASSIGN(CelPolicyValidationResult result, + CompilePolicy(*compiler, *policy)); + EXPECT_FALSE(result.IsValid()); + EXPECT_THAT(result.FormatIssues(), + testing::HasSubstr("match creates unreachable outputs")); +} + +TEST(CompilerTest, LiterallyFalseConditionReportsError) { + absl::string_view yaml = R"yaml( +name: cel_policy +rule: + id: test_rule + match: + - condition: false + output: dyn(1) + - condition: x > 5 + output: 2 + - output: '"default"' +)yaml"; + ASSERT_OK_AND_ASSIGN(std::unique_ptr policy, + ParsePolicyFromYaml(yaml)); + ASSERT_OK_AND_ASSIGN(std::unique_ptr compiler, BuildTestCompiler()); + ASSERT_OK_AND_ASSIGN(CelPolicyValidationResult result, + CompilePolicy(*compiler, *policy)); + EXPECT_FALSE(result.IsValid()); + EXPECT_THAT(result.GetIssues(), testing::SizeIs(1)); + EXPECT_THAT(result.FormatIssues(), + testing::HasSubstr("condition is always false")); +} + +TEST(CompilerTest, AggregateRuleLiterallyFalseConditionReportsError) { + absl::string_view yaml = R"yaml( +name: aggregate_false_condition_policy +rule: + aggregate: + - condition: false + output: '"unreachable"' + - output: '"default"' +)yaml"; + ASSERT_OK_AND_ASSIGN(std::unique_ptr policy, + ParsePolicyFromYaml(yaml)); + ASSERT_OK_AND_ASSIGN(std::unique_ptr compiler, BuildTestCompiler()); + ASSERT_OK_AND_ASSIGN(CelPolicyValidationResult result, + CompilePolicy(*compiler, *policy)); + EXPECT_FALSE(result.IsValid()); + EXPECT_THAT(result.FormatIssues(), + testing::HasSubstr("condition is always false")); +} + TEST(CompilerTest, RuleWithoutMatchesReportsError) { absl::string_view yaml = R"yaml( name: cel_policy @@ -1323,10 +1410,11 @@ TEST(CompilerTest, AggregateRuleReturnsUnimplementedError) { name: aggregate_policy rule: aggregate: - - condition: x > 10 - output: '"first"' - - condition: y > 5 + - output: '"first"' + - condition: true output: '"second"' + - condition: y > 5 + output: '"third"' )yaml"; ASSERT_OK_AND_ASSIGN(std::unique_ptr policy, ParsePolicyFromYaml(yaml)); @@ -1352,5 +1440,48 @@ name: nested_aggregate_policy StatusIs(absl::StatusCode::kUnimplemented)); } +TEST(CompilerTest, NestedAggregateUnderAggregateReportsError) { + absl::string_view yaml = R"yaml( +name: nested_aggregate_policy +rule: + aggregate: + - condition: x > 10 + rule: + aggregate: + - output: '"first"' +)yaml"; + ASSERT_OK_AND_ASSIGN(std::unique_ptr policy, + ParsePolicyFromYaml(yaml)); + ASSERT_OK_AND_ASSIGN(std::unique_ptr compiler, BuildTestCompiler()); + ASSERT_OK_AND_ASSIGN(CelPolicyValidationResult result, + CompilePolicy(*compiler, *policy)); + EXPECT_FALSE(result.IsValid()); + EXPECT_THAT(result.FormatIssues(), + testing::HasSubstr("nested aggregate rules are not allowed")); +} + +TEST(CompilerTest, NestedAggregateUnderMatchUnderAggregateReportsError) { + absl::string_view yaml = R"yaml( +name: nested_aggregate_mixed_policy +rule: + aggregate: + - condition: x > 10 + rule: + match: + - condition: y > 5 + rule: + aggregate: + - output: '"first"' +)yaml"; + ASSERT_OK_AND_ASSIGN(std::unique_ptr policy, + ParsePolicyFromYaml(yaml)); + ASSERT_OK_AND_ASSIGN(std::unique_ptr compiler, BuildTestCompiler()); + ASSERT_OK_AND_ASSIGN(CelPolicyValidationResult result, + CompilePolicy(*compiler, *policy)); + EXPECT_FALSE(result.IsValid()); + EXPECT_THAT(result.FormatIssues(), + testing::HasSubstr("nested aggregate rules are not allowed")); +} + } // namespace } // namespace cel