Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 1 addition & 24 deletions src/cel-engine/src/rules/resources_extra.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,7 @@ use template_model::message::{primary_identifier_conflict_message, render_str_li
use template_model::resolver::{RefKind, ResolvedValue};
use template_model::route_table::duplicate_subnet_associations;
use template_model::vpc_cidr::subnets_outside_vpc;
use template_model::{
CAA_RECORD_PATTERN, IAM_ROLE_ARN_RULE_PATTERN, MARKER_DYNAMIC, MARKER_PARAM_TYPE, MX_RECORD_PATTERN, SourceSpan,
};
use template_model::{CAA_RECORD_PATTERN, MARKER_DYNAMIC, MARKER_PARAM_TYPE, MX_RECORD_PATTERN, SourceSpan};
use template_model::{ListMemberScenario, SemanticModel};
use template_model::{hardcoded_az, region_enums, schedule_expression_errors};
use validation_engine::{make_resource_diagnostic, make_resource_diagnostic_at_source};
Expand All @@ -34,9 +32,6 @@ static DOMAIN_RE: LazyLock<regex::Regex> = LazyLock::new(|| {
.expect("Invalid DOMAIN_RE pattern")
});

static ARN_RE: LazyLock<regex::Regex> =
LazyLock::new(|| regex::Regex::new(IAM_ROLE_ARN_RULE_PATTERN).expect("Invalid ARN_RE pattern"));

static W1030_VPC_RE: LazyLock<regex::Regex> = LazyLock::new(|| {
regex::Regex::new(r"^vpc-(([0-9A-Fa-f]{8})|([0-9A-Fa-f]{17}))$").expect("Invalid W1030_VPC_RE pattern")
});
Expand Down Expand Up @@ -2115,24 +2110,6 @@ pub fn eval_extra_resources(ctx: &EvalContext) -> Vec<Diagnostic> {
out.extend(snapstart_without_published_version(m, name));
}

for rpp in &ctx.cached_data.rule_tables.iam_role_arn_property_paths {
let path = full_property_path(rpp);
for name in m.resources_of_type(&rpp.resource_type) {
if let Some(serde_json::Value::String(val)) = resolve_concrete(m, name, &path)
&& !ARN_RE.is_match(&val)
{
out.push(make_resource_diagnostic(
"E3511",
&format!("IAM Role ARN '{}' does not match expected pattern", val),
m,
name,
&path,
None,
));
}
}
}

for (name, res) in &m.resources {
for (prop, val) in &res.properties {
if !prop.ends_with("Arn") {
Expand Down
59 changes: 26 additions & 33 deletions src/cfn-validate/tests/github_issues.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1297,32 +1297,22 @@ fn w9007_still_fires_when_two_entries_read_one_deploy_time_value() {
}

// ---------------------------------------------------------------------------
// Issue #36 - the IAM-role-ARN checks use a future-proof `arn:aws[a-zA-Z-]*`
// partition prefix, so ADC-partition ARNs no longer false-positive and the two
// engines agree. The committed fixture uses `arn:aws-iso:` (snapshot-compatible);
// the previously-divergent E3511 path is exercised here with inline bytes using
// `arn:aws-isob:`, which both engines must now accept identically.
// Issue #36 - IAM role ARN formats accept `aws`-prefixed partitions without a fixed allowlist.
// ---------------------------------------------------------------------------

/// Issue #36: the schema-validator's `AWS::IAM::Role.Arn` format check (E1156)
/// must not fire on an `arn:aws-iso:` ADC-partition ARN in either engine - the
/// partition list is no longer hardcoded.
/// An `arn:aws-iso:` role ARN must be accepted without a partition allowlist.
/// https://github.com/aws-cloudformation/cloudformation-validate/issues/36
#[test]
fn issue_36_no_e1156_on_iso_partition_arn() {
let diags = validate_all("issue-36.yaml");
assert_absent(&diags, "E1156");
assert_count(&diags, "E1156", 0);
fn issue_36_no_role_arn_format_error_on_iso_partition() {
let diagnostics = validate_all("issue-36.yaml");
assert_absent(&diagnostics, "E1156");
assert_count(&diagnostics, "E1156", 0);
}

/// Issue #36 (continued): the engine ARN rule (E3511) and the schema-validator
/// format check (E1156) both accept an `arn:aws-isob:` ARN, and the two engines
/// agree - the prior rego/cel divergence (CEL hardcoded the partition list) is
/// gone. A genuinely malformed ARN still fires E3511 in both engines, so the
/// future-proof prefix did not become a catch-all.
/// An AWS partition outside a fixed allowlist is accepted while a malformed role ARN remains rejected at engine parity.
/// https://github.com/aws-cloudformation/cloudformation-validate/issues/36
#[test]
fn issue_36_isob_partition_arn_accepted_at_parity() {
fn issue_36_future_partition_arn_accepted_at_parity() {
const VALID: &[u8] = br#"
AWSTemplateFormatVersion: "2010-09-09"
Resources:
Expand All @@ -1331,18 +1321,18 @@ Resources:
Properties:
ExecutionRoleArn: arn:aws-isob:iam::123456789012:role/my-task-role
"#;
let sv = SchemaValidator::default();
let rego = validate_bytes(&*REGO, &sv, VALID, debug_config()).unwrap().diagnostics;
let cel = validate_bytes(&*CEL, &sv, VALID, debug_config()).unwrap().diagnostics;
let composite = validate_bytes(&*COMPOSITE, &sv, VALID, debug_config()).unwrap().diagnostics;
let schema_validator = SchemaValidator::default();
let rego = validate_bytes(&*REGO, &schema_validator, VALID, debug_config()).unwrap().diagnostics;
let cel = validate_bytes(&*CEL, &schema_validator, VALID, debug_config()).unwrap().diagnostics;
let composite = validate_bytes(&*COMPOSITE, &schema_validator, VALID, debug_config()).unwrap().diagnostics;

for (name, d) in [("rego", &rego), ("cel", &cel), ("composite", &composite)] {
assert!(!d.iter().any(|x| x.rule_id == "E1156"), "[{name}] E1156 must not fire on an aws-isob ARN");
assert!(!d.iter().any(|x| x.rule_id == "E3511"), "[{name}] E3511 must not fire on an aws-isob ARN");
for (engine_name, diagnostics) in [("rego", &rego), ("cel", &cel), ("composite", &composite)] {
assert!(
!diagnostics.iter().any(|diagnostic| diagnostic.rule_id == "E1156"),
"[{engine_name}] role ARN format must accept an aws-isob ARN"
);
}

// A malformed ARN (no partition at all) must still be rejected by E3511 in
// both engines - the future-proof prefix is not a catch-all.
const MALFORMED: &[u8] = br#"
AWSTemplateFormatVersion: "2010-09-09"
Resources:
Expand All @@ -1351,12 +1341,15 @@ Resources:
Properties:
ExecutionRoleArn: not-an-arn
"#;
let rego_bad = validate_bytes(&*REGO, &sv, MALFORMED, debug_config()).unwrap().diagnostics;
let cel_bad = validate_bytes(&*CEL, &sv, MALFORMED, debug_config()).unwrap().diagnostics;
let composite_bad = validate_bytes(&*COMPOSITE, &sv, MALFORMED, debug_config()).unwrap().diagnostics;
assert!(rego_bad.iter().any(|d| d.rule_id == "E3511"), "rego must still fire E3511 on a malformed ARN");
assert!(cel_bad.iter().any(|d| d.rule_id == "E3511"), "cel must still fire E3511 on a malformed ARN");
assert!(composite_bad.iter().any(|d| d.rule_id == "E3511"), "composite must still fire E3511 on a malformed ARN");
let rego_bad = validate_bytes(&*REGO, &schema_validator, MALFORMED, debug_config()).unwrap().diagnostics;
let cel_bad = validate_bytes(&*CEL, &schema_validator, MALFORMED, debug_config()).unwrap().diagnostics;
let composite_bad = validate_bytes(&*COMPOSITE, &schema_validator, MALFORMED, debug_config()).unwrap().diagnostics;
for (engine_name, diagnostics) in [("rego", &rego_bad), ("cel", &cel_bad), ("composite", &composite_bad)] {
assert!(
diagnostics.iter().any(|diagnostic| diagnostic.rule_id == "E1156"),
"[{engine_name}] malformed role ARN must be rejected"
);
}
}

// ---------------------------------------------------------------------------
Expand Down
2 changes: 0 additions & 2 deletions src/data-source/scripts/sync_cfnlint_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,6 @@ def extract_rule_tables(cfn_lint_root: Path) -> dict:
from cfnlint.rules.resources.apigateway.RestApiMixingDefinitions import RestApiMixingDefinitions
from cfnlint.rules.resources.ectwo.EbsIopsIgnored import EbsIopsIgnored, _no_iops_volume_types
from cfnlint.rules.resources.iam.ResourcePolicy import ResourcePolicy
from cfnlint.rules.resources.iam.RoleArnPattern import RoleArnPattern
from cfnlint.rules.resources.lmbd.SnapStartEnabled import SnapStartEnabled
from cfnlint.rules.resources.lmbd.SnapStartSupported import SnapStartSupported
from cfnlint.rules.resources.lmbd.ZipPackageRequiredProperties import ZipPackageRequiredProperties
Expand All @@ -302,7 +301,6 @@ def extract_rule_tables(cfn_lint_root: Path) -> dict:
"api_gateway_mixing_resource_types": api_gateway_rule._mix_types,
"ebs_iops_ignored_volume_types": sorted(_no_iops_volume_types),
"ebs_iops_property_paths": EbsIopsIgnored().keywords,
"iam_role_arn_property_paths": RoleArnPattern().keywords,
"image_id_parameter_types": extract_enum_list(image_id_rule.validate),
"image_id_property_paths": image_id_rule.keywords,
"lambda_zip_required_properties": extract_local_list(
Expand Down
10 changes: 1 addition & 9 deletions src/data-source/src/rule_data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ pub struct NormalizedRuleTablesDocument {
pub rule_tables: RuleTablesRaw,
}

/// Raw deserialized rule tables with all 22 fields exactly as stored in JSON.
/// Raw deserialized rule tables as stored in JSON.
///
/// Every field uses `deny_unknown_fields` on the outer struct to reject unknown keys.
/// Path fields are stored as raw strings here; [`RuleTables`] holds the parsed forms.
Expand All @@ -194,7 +194,6 @@ pub struct RuleTablesRaw {
pub api_gateway_mixing_resource_types: Vec<String>,
pub ebs_iops_ignored_volume_types: Vec<String>,
pub ebs_iops_property_paths: Vec<String>,
pub iam_role_arn_property_paths: Vec<String>,
pub image_id_parameter_types: Vec<String>,
pub image_id_property_paths: Vec<String>,
pub lambda_zip_required_properties: Vec<String>,
Expand Down Expand Up @@ -225,7 +224,6 @@ pub struct RuleTables {
pub api_gateway_mixing_resource_types: Vec<String>,
pub ebs_iops_ignored_volume_types: Vec<String>,
pub ebs_iops_property_paths: Vec<ResourcePropertyPath>,
pub iam_role_arn_property_paths: Vec<ResourcePropertyPath>,
pub image_id_parameter_types: Vec<String>,
pub image_id_property_paths: Vec<ResourcePropertyPath>,
pub lambda_zip_required_properties: Vec<String>,
Expand Down Expand Up @@ -260,7 +258,6 @@ impl RuleTables {
validate_nonempty_unique(&raw.api_gateway_mixing_resource_types, "api_gateway_mixing_resource_types")?;
validate_nonempty_unique(&raw.ebs_iops_ignored_volume_types, "ebs_iops_ignored_volume_types")?;
validate_nonempty_unique(&raw.ebs_iops_property_paths, "ebs_iops_property_paths")?;
validate_nonempty_unique(&raw.iam_role_arn_property_paths, "iam_role_arn_property_paths")?;
validate_nonempty_unique(&raw.image_id_parameter_types, "image_id_parameter_types")?;
validate_nonempty_unique(&raw.image_id_property_paths, "image_id_property_paths")?;
validate_nonempty_unique(&raw.lambda_zip_required_properties, "lambda_zip_required_properties")?;
Expand Down Expand Up @@ -300,7 +297,6 @@ impl RuleTables {

let ebs_iops_property_paths = parse_paths(&raw.ebs_iops_property_paths, "ebs_iops_property_paths")?;
validate_ebs_iops_shape(&ebs_iops_property_paths)?;
let iam_role_arn_property_paths = parse_paths(&raw.iam_role_arn_property_paths, "iam_role_arn_property_paths")?;
let image_id_property_paths = parse_paths(&raw.image_id_property_paths, "image_id_property_paths")?;
let package_property_paths = parse_paths(&raw.package_property_paths, "package_property_paths")?;
let previous_generation_instance_property_paths = parse_paths(
Expand All @@ -315,7 +311,6 @@ impl RuleTables {
api_gateway_mixing_resource_types: raw.api_gateway_mixing_resource_types,
ebs_iops_ignored_volume_types: raw.ebs_iops_ignored_volume_types,
ebs_iops_property_paths,
iam_role_arn_property_paths,
image_id_parameter_types: raw.image_id_parameter_types,
image_id_property_paths,
lambda_zip_required_properties: raw.lambda_zip_required_properties,
Expand Down Expand Up @@ -740,9 +735,6 @@ mod tests {
api_gateway_mixing_resource_types: vec!["AWS::ApiGateway::Method".into()],
ebs_iops_ignored_volume_types: vec!["gp2".into()],
ebs_iops_property_paths: vec!["Resources/AWS::EC2::Instance/Properties/BlockDeviceMappings/*/Ebs".into()],
iam_role_arn_property_paths: vec![
"Resources/AWS::Backup::BackupSelection/Properties/BackupSelection/IamRoleArn".into(),
],
image_id_parameter_types: vec!["AWS::EC2::Image::Id".into()],
image_id_property_paths: vec!["Resources/AWS::AutoScaling::LaunchConfiguration/Properties/ImageId".into()],
lambda_zip_required_properties: vec!["Handler".into()],
Expand Down

This file was deleted.

8 changes: 4 additions & 4 deletions src/resources/templates/gh-issues/issue-36.yaml
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
AWSTemplateFormatVersion: "2010-09-09"
Resources:
# ExecutionRoleArn uses the `aws-iso` ADC partition. The schema-validator's
# AWS::IAM::Role.Arn format pattern hardcodes the partition list and omits the
# ADC partitions, so it raises a false-positive E1156 (in BOTH engines) even
# though this is a deployable ARN.
# ExecutionRoleArn uses the `aws-iso` partition.
# The role ARN format accepts every `aws`-prefixed partition rather than a
# hardcoded subset, so this deployable partition must not raise a
# format diagnostic in any engine.
# See https://github.com/aws-cloudformation/cloudformation-validate/issues/36
TaskDef:
Type: AWS::ECS::TaskDefinition
Expand Down
2 changes: 2 additions & 0 deletions src/rules/src/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ pub fn format_rule_for_format(format: &str) -> Option<&'static str> {
"AWS::EC2::Subnet.Id" => Some("E1154"),
"AWS::Logs::LogGroup.Name" => Some("E1155"),
"AWS::IAM::Role.Arn" => Some("E1156"),
"AWS::IAM::Role.Name" => Some("E1165"),
_ => None,
}
}
Expand Down Expand Up @@ -231,6 +232,7 @@ mod tests {
assert_eq!(format_rule_for_format("AWS::EC2::Subnet.Id"), Some("E1154"));
assert_eq!(format_rule_for_format("AWS::Logs::LogGroup.Name"), Some("E1155"));
assert_eq!(format_rule_for_format("AWS::IAM::Role.Arn"), Some("E1156"));
assert_eq!(format_rule_for_format("AWS::IAM::Role.Name"), Some("E1165"));
}

#[test]
Expand Down
12 changes: 6 additions & 6 deletions src/rules/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -716,6 +716,12 @@ pub const RULE_REGISTRY: &[RuleDefinition] = &[
description: "Validate IAM role ARN format",
origin: RuleOrigin::CfnLint,
},
RuleDefinition {
id: "E1165",
category: Category::Intrinsic,
description: "Validate IAM role name format",
origin: RuleOrigin::CfnLint,
},
RuleDefinition {
id: "I1022",
category: Category::Intrinsic,
Expand Down Expand Up @@ -1028,12 +1034,6 @@ pub const RULE_REGISTRY: &[RuleDefinition] = &[
description: "Validate identity based IAM policies",
origin: RuleOrigin::CfnLint,
},
RuleDefinition {
id: "E3511",
category: Category::Resource,
description: "Validate IAM role arn pattern",
origin: RuleOrigin::CfnLint,
},
RuleDefinition {
id: "E3512",
category: Category::Resource,
Expand Down
Loading
Loading