From 516cef129e4292575c116a97a7e764a7dc29c75a Mon Sep 17 00:00:00 2001 From: Satyaki Ghosh Date: Tue, 22 Sep 2026 11:56:52 -0400 Subject: [PATCH] Keep resolve() returning the target logical ID for custom Rego rules while built-in policies see the __ref marker --- src/CUSTOM_RULES.md | 4 +- src/cfn-validate/tests/cross_engine.rs | 55 ++++++ src/composite-engine/src/engine.rs | 52 ++++++ src/rego-engine/README.md | 18 +- src/rego-engine/src/builtins.rs | 172 ++++++++++++++---- src/rego-engine/src/engine.rs | 231 ++++++++++++++++++++++++- src/rego-engine/src/eval_context.rs | 88 +++++++++- 7 files changed, 572 insertions(+), 48 deletions(-) diff --git a/src/CUSTOM_RULES.md b/src/CUSTOM_RULES.md index bdfc8b4f..e8b5fed8 100644 --- a/src/CUSTOM_RULES.md +++ b/src/CUSTOM_RULES.md @@ -257,9 +257,9 @@ v := { | Builtin | Signature | Behavior | |-----------------------------------|----------------------------------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------| -| `resolve` | `(resource_id, path) -> value` | Deep-resolves a property. Chooses the first enum value and true conditional branch; a `Ref`/`Fn::GetAtt` becomes the marker object `{"__ref": target}` rather than a string, so guard literal checks with `is_string`; unresolved dynamic values are undefined. Use `follow_ref` for the target ID. | +| `resolve` | `(resource_id, path) -> value` | Deep-resolves a property. Chooses the first enum value and true conditional branch; a `Ref`/`Fn::GetAtt` to a template resource becomes that resource's logical ID string (also inside a resolved list); unresolved dynamic values are undefined. A rule that validates literal content should exclude a logical ID with `not input.resources[value]`, or read the reference explicitly with `follow_ref`/`authored_form`. | | `resolve_preserving_conditionals` | `(resource_id, path) -> value` | Resolves while retaining each conditional as `{"Fn::If": [condition, true_value, false_value]}`. | -| `resolve_all` | `(resource_id, path) -> [value]` | Returns all concrete enum and conditional outcomes. References and dynamic values contribute no values. | +| `resolve_all` | `(resource_id, path) -> [value]` | Returns all concrete enum and conditional outcomes. A property that is itself a reference or dynamic value contributes no values; a reference inside a resolved list is rendered as in `resolve`. | | `resolve_scenarios` | `(resource_id, path) -> [{value, conditions, path?}]` | Returns values with their condition assignments. A `.{}.` path segment expands array indices and adds the concrete `path`. | | `properties_scenarios` | `(resource_id, [property_name]) -> [{properties, conditions}]` | Returns satisfiable property scenarios projected to the requested top-level fields; null fields are omitted. | | `is_dynamic` | `(resource_id, path) -> bool` | Whether the value or a nested value contains a dynamic value or unresolved reference; missing paths return `false`. | diff --git a/src/cfn-validate/tests/cross_engine.rs b/src/cfn-validate/tests/cross_engine.rs index 97112a95..ae0307eb 100644 --- a/src/cfn-validate/tests/cross_engine.rs +++ b/src/cfn-validate/tests/cross_engine.rs @@ -228,6 +228,61 @@ fn custom_rule_list_rules_and_validate_match_between_engines() { } } +/// A custom Rego rule reads `resolve()` under the contract it was written +/// against - a `Ref`/`Fn::GetAtt` to a template resource comes back as that +/// resource's logical ID string - while the built-in policies evaluated beside it +/// must keep treating such a reference as a non-literal. The template is the +/// built-in regression fixture for that second half, so a single run proves both +/// halves on every engine that hosts custom Rego. +#[test] +fn custom_rego_resolve_keeps_the_target_logical_id_while_builtins_stay_silent_on_references() { + const TEMPLATE: &str = "good/reference_values_are_not_literals.yaml"; + let legacy_reference_rule = ExternalRuleSource { + name: "legacy_reference.rego".into(), + content: r#" +package legacy_reference +import rego.v1 + +violation contains make_diag("LEGACY_ROLE_TARGET", "warn", name, sprintf("role comes from %s", [target])) if { + some name in resources_of_type("AWS::Lambda::Function") + target := resolve(name, "Properties.Role") + is_string(target) + input.resources[target].resourceType == "AWS::SSM::Parameter" +} +"# + .into(), + }; + let rego = RegoEngine::new(EngineConfig { + custom_rules: vec![legacy_reference_rule.clone()], + guard_rules: vec![], + ..Default::default() + }) + .unwrap(); + let composite = + CompositeEngine::new(CompositeEngineConfig::new().with_rego_rules([legacy_reference_rule])).unwrap(); + let builtin_baseline: Vec = + validate_template(&*COMPOSITE, TEMPLATE).into_iter().map(|d| d.rule_id).collect(); + + for (engine_name, diags) in + [("rego", validate_template(®o, TEMPLATE)), ("composite", validate_template(&composite, TEMPLATE))] + { + let legacy = diags + .iter() + .find(|d| d.rule_id == "LEGACY_ROLE_TARGET") + .unwrap_or_else(|| panic!("[{engine_name}] the custom rule must see the referenced logical ID")); + assert_eq!(legacy.message, "role comes from Store", "[{engine_name}] resolve() yields the target logical ID"); + assert_eq!(legacy.resource_logical_id(), Some("Function"), "[{engine_name}] resource_id"); + assert_eq!(legacy.source, RuleOrigin::Custom, "[{engine_name}] origin"); + + let builtins: Vec = + diags.iter().filter(|d| d.source != RuleOrigin::Custom).map(|d| d.rule_id.clone()).collect(); + assert_eq!( + builtins, builtin_baseline, + "[{engine_name}] loading a custom rule must not change what the built-in rules report on a reference" + ); + } +} + #[test] fn arbitrary_f_prefixed_custom_id_keeps_declared_severity_in_both_engines() { // A custom rule ID is arbitrary (here: `Firewall.check-1`, WARN). The built-in diff --git a/src/composite-engine/src/engine.rs b/src/composite-engine/src/engine.rs index 9241b83b..c474f9b5 100644 --- a/src/composite-engine/src/engine.rs +++ b/src/composite-engine/src/engine.rs @@ -458,4 +458,56 @@ Resources: "the composite init metric must span the external engine's construction, not replace it" ); } + + /// Custom Rego rules were written against `resolve` returning the logical ID + /// of a `Ref`/`Fn::GetAtt` target as a string. The external-only engine the + /// composite layers on must honor that contract, so a rule pack that resolves + /// a reference into a resource lookup keeps firing after an engine upgrade. + #[test] + fn custom_rego_resolve_yields_the_referenced_target_logical_id_as_a_string() { + let legacy_reference_rule = ExternalRuleSource { + name: "legacy_reference.rego".into(), + content: r#" +package legacy_reference +import rego.v1 + +violation contains make_diag("LEGACY_TARGET_LOOKUP", "error", name, sprintf("code bucket is %s", [target])) if { + some name in resources_of_type("AWS::Lambda::Function") + target := resolve(name, "Properties.Code.S3Bucket") + is_string(target) + input.resources[target].resourceType == "AWS::S3::Bucket" +} +"# + .into(), + }; + let composite = CompositeEngine::new(CompositeEngineConfig::new().with_rego_rules([legacy_reference_rule])) + .expect("composite builds"); + let model = model( + r#" +AWSTemplateFormatVersion: "2010-09-09" +Resources: + ArtifactsBucket: + Type: AWS::S3::Bucket + Handler: + Type: AWS::Lambda::Function + Properties: + Runtime: python3.12 + Handler: index.handler + Role: arn:aws:iam::123456789012:role/lambda-role + Code: + S3Bucket: !Ref ArtifactsBucket + S3Key: code.zip +"#, + ); + + let diags = composite.evaluate_rules(&model, &ValidateConfig::default()).expect("composite evaluates"); + + let legacy = diags + .iter() + .find(|d| d.rule_id == "LEGACY_TARGET_LOOKUP") + .expect("a custom rule resolving a Ref into a resource lookup must still fire"); + assert_eq!(legacy.message, "code bucket is ArtifactsBucket"); + assert_eq!(legacy.source, RuleOrigin::Custom); + assert_eq!(legacy.resource_logical_id(), Some("Handler")); + } } diff --git a/src/rego-engine/README.md b/src/rego-engine/README.md index 7a0331d7..23187c13 100644 --- a/src/rego-engine/README.md +++ b/src/rego-engine/README.md @@ -31,7 +31,7 @@ namespace prefix. For example, a policy calls `resolve(name, "Properties.BucketN | Builtin | Signature | Purpose | |----------------------|---------------------------------------------------------------|------------------------------------------------------| -| `resolve` | `(resource_id, path) → value` | Resolve a property value through intrinsic functions; a reference is a `{"__ref": target}` marker, never a bare string | +| `resolve` | `(resource_id, path) → value` | Resolve a property value through intrinsic functions; a reference is rendered per the evaluating package, see below | | `resolve_all` | `(resource_id, path) → [values]` | Resolve all scenario values for a property | | `resolve_scenarios` | `(resource_id, path) → [{value, conditions}]` | Resolve all (value, condition_map) pairs | | `resolve_ref_target` | `(resource_id, path) → {resourceType, condition, properties}` | Resolve the target of a reference | @@ -42,6 +42,22 @@ namespace prefix. For example, a policy calls `resolve(name, "Properties.BucketN | `follow_ref` | `(resource_id, path) → target_id` | Follow a Ref/GetAtt to its target resource | | `flatten_list` | `(resource_id, path) → [{value, index}]` | Flatten nested arrays | +#### Reference rendering + +A `Ref`/`Fn::GetAtt` to a template resource has no literal before deployment, and `resolve` (and a reference inside a +list returned by `resolve_all`) renders it according to the package being evaluated: + +- **Handwritten built-in policies** receive the `{"__ref": target}` marker object, the same shape the `input` document + uses. It is never a bare string, so a format or enum check that guards with `is_string` skips the reference instead of + judging a logical ID as if it were the value, while a presence check still sees a value. +- **Custom rules** receive the target's logical ID as a plain string, the contract they were written against; a custom + rule may look the target up in `input.resources` or compare it with another logical ID. Rules that validate literal + content should exclude a logical ID with `not input.resources[value]`, or read the reference explicitly with + `follow_ref` or `authored_form`. + +The rendering is selected per package around each `eval_rule` query, so the two kinds evaluate on one engine instance +without observing each other's rendering. Guard rules are not affected: they never evaluate through Rego. + ### Resource Queries | Builtin | Signature | Purpose | diff --git a/src/rego-engine/src/builtins.rs b/src/rego-engine/src/builtins.rs index cfd6e85a..3028af6f 100644 --- a/src/rego-engine/src/builtins.rs +++ b/src/rego-engine/src/builtins.rs @@ -1,4 +1,6 @@ -use crate::eval_context::{current_model, current_region, is_builtin_rule_suppressed}; +use crate::eval_context::{ + ReferenceRendering, current_model, current_reference_rendering, current_region, is_builtin_rule_suppressed, +}; use data_source::types::{ArtifactCountEntry, CodepipelineArtifactCounts, GetattData, SchemaMetadataCatalog}; use regex::Regex; use regorus::Value; @@ -211,11 +213,11 @@ fn register_cfn_rule_active(rego: &mut regorus::Engine) -> anyhow::Result<()> { ) } -fn resolved_to_rego(rv: &ResolvedValue) -> Value { +fn resolved_to_rego(rv: &ResolvedValue, rendering: ReferenceRendering) -> Value { match rv { ResolvedValue::Concrete { value: v } => json_to_value(v), ResolvedValue::List { items } => { - let vals: Vec = items.iter().map(resolved_to_rego).collect(); + let vals: Vec = items.iter().map(|item| resolved_to_rego(item, rendering)).collect(); Value::from(vals) } ResolvedValue::Map { entries } => { @@ -233,21 +235,27 @@ fn resolved_to_rego(rv: &ResolvedValue) -> Value { } Value::Undefined } - ResolvedValue::Conditional { if_true: t, .. } => resolved_to_rego(t), - // A reference has no literal before deployment. Rendering it as the same - // marker object the `input` document uses keeps it distinct from an - // authored string, so a rule that validates literal content skips it - // while a presence check still sees a value. - ResolvedValue::Reference { target, .. } => json_to_value(&serde_json::json!({MARKER_REF: target})), + ResolvedValue::Conditional { if_true: t, .. } => resolved_to_rego(t, rendering), + ResolvedValue::Reference { target, .. } => reference_to_rego(target, rendering), ResolvedValue::Dynamic { .. } | ResolvedValue::TypedDynamic { .. } => Value::Undefined, } } -fn resolved_all_to_rego(rv: &ResolvedValue) -> Vec { +/// A reference has no literal before deployment; how it is rendered is the +/// contract the evaluating package was written against (see +/// [`ReferenceRendering`]). +fn reference_to_rego(target: &str, rendering: ReferenceRendering) -> Value { + match rendering { + ReferenceRendering::Marker => json_to_value(&serde_json::json!({MARKER_REF: target})), + ReferenceRendering::TargetId => Value::from(target), + } +} + +fn resolved_all_to_rego(rv: &ResolvedValue, rendering: ReferenceRendering) -> Vec { match rv { ResolvedValue::Concrete { value: v } => vec![json_to_value(v)], ResolvedValue::List { items } => { - let vals: Vec = items.iter().map(resolved_to_rego).collect(); + let vals: Vec = items.iter().map(|item| resolved_to_rego(item, rendering)).collect(); vec![Value::from(vals)] } ResolvedValue::Map { entries } => { @@ -257,10 +265,12 @@ fn resolved_all_to_rego(rv: &ResolvedValue) -> Vec { } vec![json_to_value(&serde_json::Value::Object(map))] } - ResolvedValue::Enum { variants: vals } => vals.iter().flat_map(resolved_all_to_rego).collect(), + ResolvedValue::Enum { variants: vals } => { + vals.iter().flat_map(|v| resolved_all_to_rego(v, rendering)).collect() + } ResolvedValue::Conditional { if_true: t, if_false: f, .. } => { - let mut r = resolved_all_to_rego(t); - r.extend(resolved_all_to_rego(f)); + let mut r = resolved_all_to_rego(t, rendering); + r.extend(resolved_all_to_rego(f, rendering)); r } // Unresolved references and dynamic values have no concrete literal to return. @@ -377,11 +387,12 @@ fn register_resolve(rego: &mut regorus::Engine) { }; let rid = params[0].as_string()?; let path = params[1].as_string()?; + let rendering = current_reference_rendering(); if let Some(val) = model.resolve_deep(rid, path) { - return Ok(resolved_to_rego(&val)); + return Ok(resolved_to_rego(&val, rendering)); } if let Some(val) = model.resolve(rid, path) { - return Ok(resolved_to_rego(val)); + return Ok(resolved_to_rego(val, rendering)); } // `Properties` wrapped in `Fn::If` stores values only under the // synthetic branch path - fall back to scenario resolution so the @@ -432,11 +443,12 @@ fn register_resolve_all(rego: &mut regorus::Engine) { }; let rid = params[0].as_string()?; let path = params[1].as_string()?; + let rendering = current_reference_rendering(); if let Some(val) = model.resolve_deep(rid, path) { - return Ok(Value::from(resolved_all_to_rego(&val))); + return Ok(Value::from(resolved_all_to_rego(&val, rendering))); } if let Some(val) = model.resolve(rid, path) { - return Ok(Value::from(resolved_all_to_rego(val))); + return Ok(Value::from(resolved_all_to_rego(val, rendering))); } // `Properties` wrapped in `Fn::If` stores values under a synthetic // branch path. Fall back to scenario resolution so rules that walk @@ -2825,13 +2837,13 @@ mod tests { #[test] fn resolved_to_rego_concrete_string() { let rv = ResolvedValue::Concrete { value: serde_json::json!("test").into() }; - assert_eq!(resolved_to_rego(&rv), Value::from("test")); + assert_eq!(resolved_to_rego(&rv, ReferenceRendering::Marker), Value::from("test")); } #[test] fn resolved_to_rego_concrete_number() { let rv = ResolvedValue::Concrete { value: serde_json::json!(99).into() }; - assert_eq!(resolved_to_rego(&rv), Value::from(99i64)); + assert_eq!(resolved_to_rego(&rv, ReferenceRendering::Marker), Value::from(99i64)); } #[test] @@ -2842,7 +2854,7 @@ mod tests { ResolvedValue::Concrete { value: serde_json::json!(2).into() }, ], }; - let v = resolved_to_rego(&rv); + let v = resolved_to_rego(&rv, ReferenceRendering::Marker); let arr = v.as_array().expect("should be array"); assert_eq!(arr.len(), 2); } @@ -2855,7 +2867,7 @@ mod tests { value: ResolvedValue::Concrete { value: serde_json::json!("val").into() }, }], }; - let v = resolved_to_rego(&rv); + let v = resolved_to_rego(&rv, ReferenceRendering::Marker); v.as_object().expect("resolved_to_rego should produce a valid object"); } @@ -2867,13 +2879,13 @@ mod tests { ResolvedValue::Concrete { value: serde_json::json!("second").into() }, ], }; - assert_eq!(resolved_to_rego(&rv), Value::from("first")); + assert_eq!(resolved_to_rego(&rv, ReferenceRendering::Marker), Value::from("first")); } #[test] fn resolved_to_rego_enum_empty_returns_undefined() { let rv = ResolvedValue::Enum { variants: vec![] }; - assert_eq!(resolved_to_rego(&rv), Value::Undefined); + assert_eq!(resolved_to_rego(&rv, ReferenceRendering::Marker), Value::Undefined); } #[test] @@ -2883,33 +2895,96 @@ mod tests { if_true: Box::new(ResolvedValue::Concrete { value: serde_json::json!("yes").into() }), if_false: Box::new(ResolvedValue::Concrete { value: serde_json::json!("no").into() }), }; - assert_eq!(resolved_to_rego(&rv), Value::from("yes")); + assert_eq!(resolved_to_rego(&rv, ReferenceRendering::Marker), Value::from("yes")); } #[test] - fn resolved_to_rego_reference_is_a_marker_object_not_the_target_string() { + fn resolved_to_rego_reference_under_marker_rendering_is_a_marker_object_not_the_target_string() { let rv = ResolvedValue::Reference { target: "MyBucket".to_string(), kind: RefKind::Ref }; - let rendered = resolved_to_rego(&rv); + let rendered = resolved_to_rego(&rv, ReferenceRendering::Marker); assert_ne!(rendered, Value::from("MyBucket"), "a logical ID must never masquerade as a literal string"); assert_eq!(rendered, json_to_value(&serde_json::json!({MARKER_REF: "MyBucket"}))); } + #[test] + fn resolved_to_rego_reference_under_target_id_rendering_is_the_target_string() { + let rv = ResolvedValue::Reference { target: "MyBucket".to_string(), kind: RefKind::Ref }; + assert_eq!(resolved_to_rego(&rv, ReferenceRendering::TargetId), Value::from("MyBucket")); + } + + #[test] + fn resolved_to_rego_getatt_reference_renders_like_a_ref_under_both_renderings() { + let rv = ResolvedValue::Reference { target: "Store".to_string(), kind: RefKind::GetAtt { attr: "Arn".into() } }; + assert_eq!( + resolved_to_rego(&rv, ReferenceRendering::Marker), + json_to_value(&serde_json::json!({MARKER_REF: "Store"})) + ); + assert_eq!(resolved_to_rego(&rv, ReferenceRendering::TargetId), Value::from("Store")); + } + + #[test] + fn resolved_to_rego_list_items_follow_the_rendering_of_the_whole_value() { + let rv = ResolvedValue::List { + items: vec![ + ResolvedValue::Concrete { value: serde_json::json!("literal.example.com").into() }, + ResolvedValue::Reference { + target: "Store".to_string(), + kind: RefKind::GetAtt { attr: "Value".into() }, + }, + ], + }; + let marker = json_to_value(&serde_json::json!(["literal.example.com", {MARKER_REF: "Store"}])); + assert_eq!(resolved_to_rego(&rv, ReferenceRendering::Marker), marker); + let target_ids = json_to_value(&serde_json::json!(["literal.example.com", "Store"])); + assert_eq!(resolved_to_rego(&rv, ReferenceRendering::TargetId), target_ids); + } + + #[test] + fn resolved_to_rego_conditional_reference_follows_the_rendering() { + let rv = ResolvedValue::Conditional { + condition: "UseSharedBucket".to_string(), + if_true: Box::new(ResolvedValue::Reference { target: "SharedBucket".to_string(), kind: RefKind::Ref }), + if_false: Box::new(ResolvedValue::Concrete { value: serde_json::json!("literal-bucket").into() }), + }; + assert_eq!( + resolved_to_rego(&rv, ReferenceRendering::Marker), + json_to_value(&serde_json::json!({MARKER_REF: "SharedBucket"})) + ); + assert_eq!(resolved_to_rego(&rv, ReferenceRendering::TargetId), Value::from("SharedBucket")); + } + + #[test] + fn resolved_to_rego_map_values_keep_the_marker_under_both_renderings() { + // A map is rendered as a document fragment, where a reference has always + // been the marker; the rendering only decides how a reference that *is* + // the resolved value comes back. + let rv = ResolvedValue::Map { + entries: vec![MapEntry { + key: "S3Bucket".to_string(), + value: ResolvedValue::Reference { target: "ArtifactsBucket".to_string(), kind: RefKind::Ref }, + }], + }; + let expected = json_to_value(&serde_json::json!({"S3Bucket": {MARKER_REF: "ArtifactsBucket"}})); + assert_eq!(resolved_to_rego(&rv, ReferenceRendering::Marker), expected); + assert_eq!(resolved_to_rego(&rv, ReferenceRendering::TargetId), expected); + } + #[test] fn resolved_to_rego_dynamic_returns_undefined() { let rv = ResolvedValue::Dynamic { reason: "param".to_string() }; - assert_eq!(resolved_to_rego(&rv), Value::Undefined); + assert_eq!(resolved_to_rego(&rv, ReferenceRendering::Marker), Value::Undefined); } #[test] fn resolved_to_rego_typed_dynamic_returns_undefined() { let rv = ResolvedValue::TypedDynamic { reason: "param".to_string(), param_type: "String".to_string() }; - assert_eq!(resolved_to_rego(&rv), Value::Undefined); + assert_eq!(resolved_to_rego(&rv, ReferenceRendering::Marker), Value::Undefined); } #[test] fn resolved_all_concrete_returns_single() { let rv = ResolvedValue::Concrete { value: serde_json::json!("x").into() }; - let vals = resolved_all_to_rego(&rv); + let vals = resolved_all_to_rego(&rv, ReferenceRendering::Marker); assert_eq!(vals.len(), 1); assert_eq!(vals[0], Value::from("x")); } @@ -2922,7 +2997,7 @@ mod tests { ResolvedValue::Concrete { value: serde_json::json!("b").into() }, ], }; - let vals = resolved_all_to_rego(&rv); + let vals = resolved_all_to_rego(&rv, ReferenceRendering::Marker); assert_eq!(vals.len(), 2); } @@ -2933,14 +3008,14 @@ mod tests { if_true: Box::new(ResolvedValue::Concrete { value: serde_json::json!("t").into() }), if_false: Box::new(ResolvedValue::Concrete { value: serde_json::json!("f").into() }), }; - let vals = resolved_all_to_rego(&rv); + let vals = resolved_all_to_rego(&rv, ReferenceRendering::Marker); assert_eq!(vals.len(), 2); } #[test] fn resolved_all_dynamic_returns_empty() { let rv = ResolvedValue::Dynamic { reason: "x".to_string() }; - assert!(resolved_all_to_rego(&rv).is_empty()); + assert!(resolved_all_to_rego(&rv, ReferenceRendering::Marker).is_empty()); } #[test] @@ -3400,7 +3475,7 @@ mod tests { ResolvedValue::Concrete { value: serde_json::json!(2).into() }, ], }; - let vals = resolved_all_to_rego(&rv); + let vals = resolved_all_to_rego(&rv, ReferenceRendering::Marker); assert_eq!(vals.len(), 1, "List wraps into a single array value"); vals[0].as_array().expect("first element should be an array"); } @@ -3413,7 +3488,7 @@ mod tests { value: ResolvedValue::Concrete { value: serde_json::json!("v").into() }, }], }; - let vals = resolved_all_to_rego(&rv); + let vals = resolved_all_to_rego(&rv, ReferenceRendering::Marker); assert_eq!(vals.len(), 1, "Map wraps into a single object value"); } @@ -3422,13 +3497,36 @@ mod tests { // References are omitted so format-validation rules don't mistake a logical ID // for a literal value. let rv = ResolvedValue::Reference { target: "Target".to_string(), kind: RefKind::Ref }; - assert!(resolved_all_to_rego(&rv).is_empty()); + assert!(resolved_all_to_rego(&rv, ReferenceRendering::Marker).is_empty()); + } + + #[test] + fn resolved_all_reference_stays_omitted_under_target_id_rendering() { + // The rendering decides how a reference looks, not whether `resolve_all` + // reports one: a bare reference has never contributed a scenario value. + let rv = ResolvedValue::Reference { target: "Target".to_string(), kind: RefKind::Ref }; + assert!(resolved_all_to_rego(&rv, ReferenceRendering::TargetId).is_empty()); + } + + #[test] + fn resolved_all_list_items_follow_the_rendering() { + let rv = ResolvedValue::List { + items: vec![ResolvedValue::Reference { target: "Cert".to_string(), kind: RefKind::Ref }], + }; + assert_eq!( + resolved_all_to_rego(&rv, ReferenceRendering::Marker), + vec![json_to_value(&serde_json::json!([{MARKER_REF: "Cert"}]))] + ); + assert_eq!( + resolved_all_to_rego(&rv, ReferenceRendering::TargetId), + vec![json_to_value(&serde_json::json!(["Cert"]))] + ); } #[test] fn resolved_all_typed_dynamic_returns_empty() { let rv = ResolvedValue::TypedDynamic { reason: "p".to_string(), param_type: "String".to_string() }; - assert!(resolved_all_to_rego(&rv).is_empty()); + assert!(resolved_all_to_rego(&rv, ReferenceRendering::Marker).is_empty()); } #[test] diff --git a/src/rego-engine/src/engine.rs b/src/rego-engine/src/engine.rs index 5c6be5b1..26866645 100644 --- a/src/rego-engine/src/engine.rs +++ b/src/rego-engine/src/engine.rs @@ -1,3 +1,4 @@ +use crate::eval_context::{EvaluationContext, EvaluationScope, ReferenceRendering, ReferenceRenderingScope}; use crate::policies; use data_source::embedded; use data_source::types::KnownResourceTypes; @@ -189,6 +190,45 @@ enum BuiltinRuleMode { ExternalOnly, } +/// Who authored a Rego package, which fixes the contract its rules were written +/// against: how its diagnostics are attributed and how the resolution builtins +/// render a reference while it evaluates. +#[derive(Clone, Copy)] +enum PolicyPackageKind { + /// A handwritten built-in policy shipped with the engine. + BuiltIn, + /// A caller-supplied custom rule. + Custom, +} + +impl PolicyPackageKind { + fn source_label(self) -> &'static str { + match self { + Self::BuiltIn => "Core", + Self::Custom => "Custom", + } + } + + /// The origin stamped on the package's diagnostics; a built-in rule keeps the + /// origin the registry records for it. + fn rule_origin_override(self) -> Option<&'static RuleOrigin> { + match self { + Self::BuiltIn => None, + Self::Custom => Some(&RuleOrigin::Custom), + } + } + + /// Built-in policies validate literal content and must never see a logical ID + /// where a value belongs. Custom rules keep the logical-ID string they were + /// written against, so upgrading the engine does not change what they report. + fn reference_rendering(self) -> ReferenceRendering { + match self { + Self::BuiltIn => ReferenceRendering::Marker, + Self::Custom => ReferenceRendering::TargetId, + } + } +} + pub struct RegoEngine { base_rego: regorus::Engine, /// Whether the handwritten built-in policies are evaluated, or only the @@ -396,6 +436,10 @@ impl RegoEngine { /// Evaluates a single Rego package and appends its diagnostics to `out`. /// + /// The package's [`ReferenceRendering`] is in effect only while its query + /// runs: Regorus discards memoized rule and builtin results between queries, + /// so a value rendered for one package never reaches the next. + /// /// Any evaluation or serialization failure is returned as a structured /// [`ValidationError`] - an exception the caller can handle - and is never /// converted into a diagnostic. A rule that fails to run must surface as an @@ -404,11 +448,12 @@ impl RegoEngine { &self, rego: &mut regorus::Engine, package: &str, - source_label: &str, + kind: PolicyPackageKind, model: &SemanticModel, - origin: Option<&RuleOrigin>, out: &mut Vec, ) -> Result<(), ValidationError> { + let source_label = kind.source_label(); + let _rendering = ReferenceRenderingScope::enter(kind.reference_rendering()); let value = rego.eval_rule(package.to_string()).map_err(|e| { ValidationError::Engine(format!("{source_label} rule package '{package}' failed to evaluate: {e}")) })?; @@ -418,7 +463,8 @@ impl RegoEngine { serialized to JSON: {e}" )) })?; - extract_diagnostics_from_value(&diagnostics_json, model, out, origin).map_err(ValidationError::from) + extract_diagnostics_from_value(&diagnostics_json, model, out, kind.rule_origin_override()) + .map_err(ValidationError::from) } /// The built-in rule IDs that global filtering proves cannot survive under @@ -454,12 +500,12 @@ impl ValidationEngine for RegoEngine { model: &Arc, config: &ValidateConfig, ) -> Result, ValidationError> { - let context = crate::eval_context::EvaluationContext::new( + let context = EvaluationContext::new( model.clone(), config.pseudo_parameter_overrides.region.clone(), self.globally_suppressed_builtin_rules(config), ); - let _scope = crate::eval_context::EvaluationScope::enter(context); + let _scope = EvaluationScope::enter(context); let mut rego = self.base_rego.clone(); @@ -484,12 +530,12 @@ impl ValidationEngine for RegoEngine { // excluded rule costs one builtin call. The packages share no rules, // so one query per package costs the same as one aggregated query. for package in CORE_PACKAGES { - self.eval_package_into(&mut rego, package, "Core", model, None, &mut diagnostics)?; + self.eval_package_into(&mut rego, package, PolicyPackageKind::BuiltIn, model, &mut diagnostics)?; } } for pkg in &self.custom_packages { - self.eval_package_into(&mut rego, pkg, "Custom", model, Some(&RuleOrigin::Custom), &mut diagnostics)?; + self.eval_package_into(&mut rego, pkg, PolicyPackageKind::Custom, model, &mut diagnostics)?; } if let Some(guard_rules) = &self.guard_rules { @@ -925,6 +971,21 @@ Resources: Code: S3Bucket: !Ref MyBucket S3Key: code.zip + VpcConfig: + SecurityGroupIds: + - sg-0123456789abcdef0 + SubnetIds: + - !Ref MySubnet + - subnet-0123456789abcdef0 + MyVpc: + Type: AWS::EC2::VPC + Properties: + CidrBlock: 10.0.0.0/16 + MySubnet: + Type: AWS::EC2::Subnet + Properties: + VpcId: !Ref MyVpc + CidrBlock: 10.0.0.0/24 MyQueue: Type: AWS::SQS::Queue Properties: @@ -978,6 +1039,162 @@ violation contains v if { assert_eq!(diags.len(), 1, "resolve_all should return at least one value"); } + /// The contract custom rules were written against: `resolve` on a `Ref` or + /// `Fn::GetAtt` yields the target's logical ID as a string, so a rule can look + /// the target up in `input.resources` or compare it with another logical ID. + const LEGACY_REFERENCE_CUSTOM_RULE: &str = r#" +package legacy_reference_test +import rego.v1 + +violation contains make_diag("LEGACY_LOOKUP", "error", name, sprintf("code bucket is %s", [target])) if { + some name in resources_of_type("AWS::Lambda::Function") + target := resolve(name, "Properties.Code.S3Bucket") + is_string(target) + input.resources[target].resourceType == "AWS::S3::Bucket" +} + +violation contains make_diag("LEGACY_EQUALS", "error", name, "code lives in MyBucket") if { + some name in resources_of_type("AWS::Lambda::Function") + resolve(name, "Properties.Code.S3Bucket") == "MyBucket" +} + +violation contains make_diag("LEGACY_MARKER_SEEN", "error", name, "resolve returned the marker object") if { + some name in resources_of_type("AWS::Lambda::Function") + is_object(resolve(name, "Properties.Code.S3Bucket")) +} +"#; + + fn assert_legacy_reference_contract(diags: &[Diagnostic]) { + let lookup = diags + .iter() + .find(|d| d.rule_id == "LEGACY_LOOKUP") + .expect("resolve() must hand back a logical ID a rule can look up in input.resources"); + assert_eq!(lookup.message, "code bucket is MyBucket"); + assert_eq!(lookup.resource_logical_id(), Some("MyFunc")); + assert!( + diags.iter().any(|d| d.rule_id == "LEGACY_EQUALS"), + "resolve() must compare equal to the target's logical ID" + ); + assert!( + !diags.iter().any(|d| d.rule_id == "LEGACY_MARKER_SEEN"), + "a custom rule must never see the marker object from resolve()" + ); + } + + #[test] + fn custom_rule_resolve_keeps_the_legacy_target_id_string_for_a_reference() { + let engine = RegoEngine::new(EngineConfig { + custom_rules: vec![ExternalRuleSource { + name: "legacy_reference_test.rego".into(), + content: LEGACY_REFERENCE_CUSTOM_RULE.into(), + }], + ..Default::default() + }) + .unwrap(); + let model = make_model_from_yaml(BUILTIN_TEST_TEMPLATE); + + let diags = engine.evaluate_rules(&model, &ValidateConfig::default()).unwrap(); + + assert_legacy_reference_contract(&diags); + } + + #[test] + fn external_only_engine_keeps_the_legacy_target_id_string_for_a_reference() { + let engine = RegoEngine::new_external_only(EngineConfig { + custom_rules: vec![ExternalRuleSource { + name: "legacy_reference_test.rego".into(), + content: LEGACY_REFERENCE_CUSTOM_RULE.into(), + }], + ..Default::default() + }) + .unwrap(); + let model = make_model_from_yaml(BUILTIN_TEST_TEMPLATE); + + let diags = engine.evaluate_rules(&model, &ValidateConfig::default()).unwrap(); + + assert_legacy_reference_contract(&diags); + } + + #[test] + fn custom_rule_resolve_all_list_items_keep_the_legacy_target_id_string() { + let diags = eval_builtin_policy( + r#" +package builtin_test +import rego.v1 +violation contains make_diag("LEGACY_LIST_ITEM", "error", "MyFunc", sprintf("subnets: %v", [subnets])) if { + some subnets in resolve_all("MyFunc", "Properties.VpcConfig.SubnetIds") + subnets == ["MySubnet", "subnet-0123456789abcdef0"] +} +"#, + "LEGACY_LIST_ITEM", + ); + assert_eq!(diags.len(), 1, "a referenced list item must come back as the target's logical ID"); + } + + /// Both kinds of package evaluate on one Regorus engine instance, so the + /// rendering must switch per package: the built-in policies fixed to skip a + /// reference they would otherwise judge as a literal must keep doing so while + /// the custom package beside them still receives the legacy string. + #[test] + fn builtin_policies_keep_the_marker_rendering_while_a_custom_rule_gets_the_legacy_string() { + let engine = RegoEngine::new(EngineConfig { + custom_rules: vec![ExternalRuleSource { + name: "legacy_reference_test.rego".into(), + content: LEGACY_REFERENCE_CUSTOM_RULE.into(), + }], + ..Default::default() + }) + .unwrap(); + let model = make_model_from_yaml( + r#" +AWSTemplateFormatVersion: "2010-09-09" +Resources: + Store: + Type: AWS::SSM::Parameter + Properties: + Type: String + Value: placeholder + MyBucket: + Type: AWS::S3::Bucket + Distribution: + Type: AWS::CloudFront::Distribution + Properties: + DistributionConfig: + Enabled: true + Aliases: + - !GetAtt Store.Value + DefaultCacheBehavior: + TargetOriginId: primary + ViewerProtocolPolicy: redirect-to-https + ForwardedValues: + QueryString: false + Origins: + - Id: primary + DomainName: origin.example.com + CustomOriginConfig: + OriginProtocolPolicy: https-only + MyFunc: + Type: AWS::Lambda::Function + Properties: + Runtime: python3.12 + Handler: index.handler + Role: arn:aws:iam::123456789012:role/lambda-role + Code: + S3Bucket: !Ref MyBucket + S3Key: code.zip +"#, + ); + + let diags = engine.evaluate_rules(&model, &ValidateConfig::default()).unwrap(); + + assert!( + !diags.iter().any(|d| d.rule_id == "E3013"), + "the built-in alias check must not judge the logical ID 'Store' as a domain name: {:?}", + diags.iter().map(|d| (&d.rule_id, &d.message)).collect::>() + ); + assert_legacy_reference_contract(&diags); + } + #[test] fn builtin_is_dynamic_on_conditional_property() { let diags = eval_builtin_policy( diff --git a/src/rego-engine/src/eval_context.rs b/src/rego-engine/src/eval_context.rs index 8a87f2a4..df228272 100644 --- a/src/rego-engine/src/eval_context.rs +++ b/src/rego-engine/src/eval_context.rs @@ -1,8 +1,29 @@ -use std::cell::RefCell; +use std::cell::{Cell, RefCell}; use std::collections::HashSet; use std::sync::Arc; use template_model::SemanticModel; +/// How `resolve` and `resolve_all` render a `Ref`/`Fn::GetAtt` to a template +/// resource, a value that has no literal before deployment. +/// +/// The two renderings exist because two audiences read the same builtins. The +/// handwritten built-in policies need a reference to stay distinct from an +/// authored string so a format or enum check never judges a logical ID as if it +/// were the value. Custom rules were written against the older contract, where +/// the target's logical ID came back as a plain string that a rule could look up +/// in `input.resources` or compare with another logical ID; keeping that contract +/// for them means upgrading the engine does not silently change what their rules +/// report. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum ReferenceRendering { + /// The `{"__ref": target}` marker object the `input` document uses, so a + /// literal check skips the reference with `is_string` while a presence check + /// still sees a value. + Marker, + /// The target's logical ID as a bare string. + TargetId, +} + /// Per-evaluation state the Rego builtins read while a single `evaluate_rules` /// call runs on the current thread: the model under validation, the region /// override the region-scoped rules resolve against, and the built-in rule IDs @@ -32,6 +53,13 @@ thread_local! { /// the enclosing context when it finishes instead of clearing the state the /// outer evaluation still depends on. static CONTEXT_STACK: RefCell> = const { RefCell::new(Vec::new()) }; + + /// The rendering of the package being evaluated on this thread. Separate from + /// [`CONTEXT_STACK`] because it changes per package inside one evaluation, + /// while the context above is fixed for the whole evaluation. Outside any + /// package scope it holds the built-in policies' rendering, so engine warm-up + /// and unit tests of the builtins see the documented default. + static REFERENCE_RENDERING: Cell = const { Cell::new(ReferenceRendering::Marker) }; } /// Installs an [`EvaluationContext`] for the current thread until it is dropped. @@ -59,6 +87,30 @@ impl Drop for EvaluationScope { } } +/// Selects the [`ReferenceRendering`] for the current thread until it is dropped, +/// then restores the rendering that was in effect before. +/// +/// Installed around the evaluation of one Rego package. Restoring rather than +/// resetting keeps a nested evaluation on the same thread from clobbering the +/// rendering of the package that triggered it. +#[must_use = "the rendering is only selected while the scope is held"] +pub(crate) struct ReferenceRenderingScope { + previous: ReferenceRendering, +} + +impl ReferenceRenderingScope { + pub(crate) fn enter(rendering: ReferenceRendering) -> Self { + let previous = REFERENCE_RENDERING.with(|slot| slot.replace(rendering)); + Self { previous } + } +} + +impl Drop for ReferenceRenderingScope { + fn drop(&mut self) { + REFERENCE_RENDERING.with(|slot| slot.set(self.previous)); + } +} + fn with_current_context(read: impl FnOnce(&EvaluationContext) -> R) -> Option { CONTEXT_STACK.with(|stack| stack.borrow().last().map(read)) } @@ -82,6 +134,12 @@ pub(crate) fn is_builtin_rule_suppressed(rule_id: &str) -> bool { with_current_context(|context| context.suppressed_builtin_rules.contains(rule_id)).unwrap_or(false) } +/// The rendering the package being evaluated on this thread was written against; +/// the built-in policies' [`ReferenceRendering::Marker`] outside any package scope. +pub(crate) fn current_reference_rendering() -> ReferenceRendering { + REFERENCE_RENDERING.with(Cell::get) +} + #[cfg(test)] mod tests { use super::*; @@ -136,4 +194,32 @@ mod tests { assert!(is_builtin_rule_suppressed("OUTER")); assert!(!is_builtin_rule_suppressed("INNER")); } + + #[test] + fn no_rendering_scope_uses_the_marker_rendering_of_the_builtin_policies() { + assert_eq!(current_reference_rendering(), ReferenceRendering::Marker); + } + + #[test] + fn rendering_scope_selects_its_rendering_and_restores_the_previous_one_on_exit() { + { + let _custom = ReferenceRenderingScope::enter(ReferenceRendering::TargetId); + assert_eq!(current_reference_rendering(), ReferenceRendering::TargetId); + } + assert_eq!(current_reference_rendering(), ReferenceRendering::Marker); + } + + #[test] + fn nested_rendering_scope_restores_the_enclosing_rendering_not_the_default() { + let _custom = ReferenceRenderingScope::enter(ReferenceRendering::TargetId); + { + let _builtin = ReferenceRenderingScope::enter(ReferenceRendering::Marker); + assert_eq!(current_reference_rendering(), ReferenceRendering::Marker); + } + assert_eq!( + current_reference_rendering(), + ReferenceRendering::TargetId, + "leaving the inner scope must hand back the outer package's rendering" + ); + } }