Skip to content

Conversation

@kumvprat
Copy link
Contributor

Issue # (if applicable)

Fixes false positive failures in Security Guardian for PR #36600

Reason for this change

The guardhooks-no-root-principals-except-kms-secrets.guard rule was incorrectly failing on CloudFormation templates containing unresolved intrinsic functions (like Fn::Join, Fn::Sub) in IAM policy principals. This caused false positive security violations on legitimate CDK-generated templates.

Error encountered:

Error = [PathAwareValues are not comparable map, Regex]
Value = {"Fn::Join":["",["arn:",{"Ref":"AWS::Partition"},":iam::234567890123:role/..."]]}
ComparedWith = "/(?i):root$/"

Description of changes

Root Cause: The rule had an inconsistency where:

  • AssumeRolePolicyDocument section properly checked if array items were strings before validation
  • PolicyDocument, Policy, and ResourcePolicy sections were missing this type check

This caused cfn-guard to attempt regex comparison on intrinsic function objects, resulting in comparison errors.

Fix Applied:
Changed the validation pattern from:

when Principal.AWS is_list {
    Principal.AWS[*] != /(?i):root$/  # Fails on objects
}

To:

when Principal.AWS is_list {
    Principal.AWS[*] {
        when this is_string {
            this != /(?i):root$/  # Only validates strings
        }
    }
}

This pattern:

  1. Iterates through each array item individually
  2. Only validates items that are strings
  3. Skips intrinsic function objects (avoiding false positives on static templates)
  4. Still catches real :root principals in resolved templates

Files Changed:

  • tools/@aws-cdk/security-guardian/rules/guard-hooks/guardhooks-no-root-principals-except-kms-secrets.guard

Alternatives Considered:

  • Adding when Principal.AWS[*] is_string guard: This checks if ALL items are strings, which fails when even one intrinsic function is present
  • Disabling the rule for static templates: Would miss real violations in templates without intrinsics
  • The chosen solution properly handles mixed arrays and maintains security validation

Describe any new or updated permissions being added

No IAM permissions changes.

Description of how you validated changes

Unit tests

Checklist


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@github-actions github-actions bot added the p2 label Jan 14, 2026
@aws-cdk-automation aws-cdk-automation requested a review from a team January 14, 2026 13:19
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jan 14, 2026
@aws-cdk-automation aws-cdk-automation added the pr/needs-further-review PR requires additional review from our team specialists due to the scope or complexity of changes. label Jan 14, 2026
@mergify
Copy link
Contributor

mergify bot commented Jan 14, 2026

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify
Copy link
Contributor

mergify bot commented Jan 14, 2026

Merge Queue Status

✅ The pull request has been merged at d72a2dd

This pull request spent 1 hour 3 minutes 56 seconds in the queue, including 29 minutes 4 seconds running CI.
The checks were run in-place.

Required conditions to merge

@mergify
Copy link
Contributor

mergify bot commented Jan 14, 2026

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 7da5aeb into main Jan 14, 2026
17 of 18 checks passed
@mergify mergify bot deleted the security_guardian_kms_key_rule_improvements branch January 14, 2026 15:11
@github-actions
Copy link
Contributor

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 14, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

contribution/core This is a PR that came from AWS. p2 pr/needs-further-review PR requires additional review from our team specialists due to the scope or complexity of changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants