ci: security guardian changes by kumvprat · Pull Request #36110 · aws/aws-cdk

kumvprat · 2025-11-19T17:29:23Z

Issue # (if applicable)

N/A - Enhancement and bug fixes for Security Guardian tool.

Reason for this change

Security guardian is first line of defense for scanning policy, roles and permissions vended by CDK for customers. This GH Action is critical to determine if a PR change is providing secure-by-default policies for any changes introduced in CDK.

The Security Guardian tool needed several critical improvements to function properly:

CFN Intrinsic Resolution: Missing comprehensive support for CloudFormation intrinsic functions like Fn::Sub, Fn::Select, Fn::Contains, etc.
Rule Coverage: Incomplete security rule coverage across AWS services
Create noise: Current setup of security guardian does not provide benefit as it fails to exclude cases where the generic cfn-guard rule might not work. Like with KMS secrets or secret manager secrets

Description of changes

Added template preprocessing pipeline with intrinsic resolution and policy normalization, details can be found below

Major Enhancements:

Complete CFN Intrinsic Function Resolver: Added comprehensive support for all CloudFormation intrinsic functions including Fn::Sub with literal escaping, Fn::Select with bounds checking, Fn::Contains, Fn::Split, Fn::Cidr, Fn::Base64, and shorthand forms (!Ref, !GetAtt, etc.)
Cross-Stack Resolution: Implemented resource registry for resolving Fn::ImportValue and cross-template references
Policy Normalization: Added IAM policy normalizer to resolve intrinsics within policy documents before validation
Output presentation: Integrated JUnit XML output with GitHub Actions using pinned mikepenz/action-junit-report for rich PR feedback ( suggested by cfn-guard here)
Security report generation workflow : Separated out report generation into another workflow due to 2 reasons :
- Gives us flexibility to change the trigger type for security-guardian to lower permissive trigger likepull_request or pull_request_review
- Allows fork PRs to run : Fork PRs trigger workflow runs with read permissions, and the separate workflow can execute separate from the fork PR call chain with full permissions needed to update checks and annotation on the PR

Security Rule Expansion:

Added comprehensive guard rules for 13 AWS services: CodePipeline, DataTrace, DocumentDB, EC2, IAM, Kinesis, Neptune, Redshift, S3, SNS, SQS, and trust scope validation
Enhanced existing rules with better coverage for broad principals, wildcard actions, and cross-account access patterns

Describe any new or updated permissions being added

No new IAM permissions required. All changes are to the static analysis tool and GitHub Actions workflow.

Description of how you validated changes

Unit Testing: via

Guard Rule Syntax: All 13 guard rule files pass cfn-guard v3 parser validation
Cross-Stack Testing: Validated resolution of Fn::ImportValue and cross-template references
Example cases to test that the current set of rules PASS and FAIL for different scenarios

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

…y default policy from scans

…nsic scanner code to exempt some services from root principal check

…ner by default till it runs consistenly and changing workflow to run on pull_request_target

…summary and output as provided in the tool invocation arguments + Removed s3 bucket versioning requirement + data trace checks only have private key and aws access key id checks

…ons + added tests to cover that logic

…on : Not and contains with tests + made the resolution function modular

…being resolved by the custom resolver + removed malformed template checks as cdk should always produced a valid template + added policy resolution tests

…prehensive tests + Fix Fn::Sub literal escaping and parameter resolution + Add shorthand form support (etc.) + Improve Fn::Select bounds checking + Add comprehensive test coverage for guard rules and intrinsic functions

…results + changes in security guardian workflow to parse the junit files + removed test.sh + fix faulty guard rules + added action to consume and publish junit result

… of scfn guard runs

… fields in a policy are targeted for normalization

…guardian

…cy validation

…it action report GH action looks for file attribute

…tead oif implementing a reversible function => Works for unresolved cfn temapates

… filed + added tests to coer this

…pe being processed

… a PR rather than cehckout action based merge commit

…ecific object inside Properties and only triggers rule when the changes exists + test changes

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

Abogical · 2025-12-04T11:25:20Z

.github/workflows/security-report.yml

+    permissions:
+      checks: write
+      pull-requests: write
+      contents: write


Why does it need contents: write permissions?

I think this is something we can remove, will tell this first and see if it's not being used

Removed the permissions as it's not needed.

Thanks, was a good catch

mergify · 2025-12-04T13:35:11Z

mergify · 2025-12-04T13:35:12Z

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 · 2025-12-04T13:39:13Z

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).

kumvprat · 2025-12-04T16:51:26Z

@Mergifyio dequeue

mergify · 2025-12-04T16:51:51Z

dequeue

✅ The pull request has been removed from the queue

mergify · 2025-12-05T10:05:45Z

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).

kumvprat · 2025-12-05T10:07:51Z

@Mergifyio requeue

mergify · 2025-12-05T10:07:57Z

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

mergify · 2025-12-05T10:08:30Z

mergify · 2025-12-05T10:47:56Z

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).

github-actions · 2025-12-05T10:48:12Z

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.

kumvprat added 30 commits September 18, 2025 15:48

feat: Changed intrinsic scanner in security guardian to exempt kms ke…

fe1e34e

…y default policy from scans

feat: Modified rules for catching more broad policies + changed intri…

bd14fd3

…nsic scanner code to exempt some services from root principal check

feat: Adding security guardian new rules and disabling intrinsic scan…

9f0e606

…ner by default till it runs consistenly and changing workflow to run on pull_request_target

Merge branch 'main' into security_guardian_changes

648b4e1

feat: Removed ecr image checks + reverted back for cfn guard to show …

ae8fc97

…summary and output as provided in the tool invocation arguments + Removed s3 bucket versioning requirement + data trace checks only have private key and aws access key id checks

feat: Added cfn template resolution logic to resolve intrinsic functi…

d4893bd

…ons + added tests to cover that logic

feat: Added cfn template resolution logic to resolve intrinsic functi…

6f872d5

…on : Not and contains with tests + made the resolution function modular

feat: Added code comments to explain the various intrinsic functions …

2250e2f

…being resolved by the custom resolver + removed malformed template checks as cdk should always produced a valid template + added policy resolution tests

feat: using junit format of cfn-guard to generate junit related test …

9bfee59

…results + changes in security guardian workflow to parse the junit files + removed test.sh + fix faulty guard rules + added action to consume and publish junit result

feat: Fixed with proper use of result piping from cfn guard output

02ae206

chore: deleting yarn.lock file

3adada6

Merge branch 'main' into security_guardian_changes

250e5b9

feat: Added a complient cfn template + tests to check successful runs…

f92685f

… of scfn guard runs

feat: Using unnormalized check of policy normalizer to ensure all the…

23977ae

… fields in a policy are targeted for normalization

fix: config changes for junit publishing action(group by suites)

51733b3

fix: Fixing testsuite name in the junit report generated by security …

f3d4ad1

…guardian

feat: abstract out postProcessXml function

f379db6

feat: restore security-critical resource attributes for accurate poli…

2a13551

…cy validation

fix: copy name attribute to file attribute in testsuite since the jun…

e86ada3

…it action report GH action looks for file attribute

fix: Logic to handled file name mapping and returning the mapping ins…

948717a

…tead oif implementing a reversible function => Works for unresolved cfn temapates

fix: Logic to handled file name mappings and resolving testsuite name…

eaeb476

… filed + added tests to coer this

feat: Added logic to convert failure message based on cfn template ty…

b249b28

…pe being processed

feat: Using proper summary table in GH workflow

4baf2a1

feat: Eanble annotation update rather than new ones

3145e90

feat: Added logic to creat 2 set ofsumaaries and use exact chagens in…

b594d29

… a PR rather than cehckout action based merge commit

fix: Added checks in no-root-principals guard rule that checks for sp…

fcb058b

…ecific object inside Properties and only triggers rule when the changes exists + test changes

fix: Added checks in no-root-principals guard rule that checks for sp…

5ede549

…ecific object inside Properties and only triggers rule when the changes exists + test changes

feat: Add example for suppressing a guard rule

0309483

fix: Removing datatrace.guard as it's source of lot of false positives

8af6924

Merge branch 'main' into security_guardian_changes

d82b62b

github-actions bot mentioned this pull request Dec 1, 2025

Monthly PR metrics report #36251

Open

Abogical added pr-linter/exempt-integ-test The PR linter will not require integ test changes and removed pr-linter/exempt-integ-test The PR linter will not require integ test changes labels Dec 4, 2025

Abogical changed the title ~~feat(ci): security guardian changes~~ ci: security guardian changes Dec 4, 2025

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 4, 2025

Abogical reviewed Dec 4, 2025

View reviewed changes

kumvprat added 2 commits December 4, 2025 13:37

Merge branch 'main' into security_guardian_changes

c4f44e4

feat: remove contents write permission for security-report workflow

a2ec1b8

Abogical approved these changes Dec 4, 2025

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 4, 2025

Merge branch 'main' into security_guardian_changes

2965c05

kumvprat added pr/do-not-merge This PR should not be merged at this time. and removed pr/do-not-merge This PR should not be merged at this time. labels Dec 4, 2025

Merge branch 'main' into security_guardian_changes

a6a0d3f

mergify bot merged commit 87b7e4a into main Dec 5, 2025
17 of 18 checks passed

mergify bot deleted the security_guardian_changes branch December 5, 2025 10:48

github-actions bot locked as resolved and limited conversation to collaborators Dec 5, 2025

Conversation

kumvprat commented Nov 19, 2025

Issue # (if applicable)

Reason for this change

Description of changes

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

Uh oh!

Abogical Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

kumvprat Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

kumvprat Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Reason

Hint

Uh oh!

mergify bot commented Dec 4, 2025

Uh oh!

mergify bot commented Dec 4, 2025

Uh oh!

kumvprat commented Dec 4, 2025

Uh oh!

mergify bot commented Dec 4, 2025

✅ The pull request has been removed from the queue

Uh oh!

mergify bot commented Dec 5, 2025

Uh oh!

kumvprat commented Dec 5, 2025

Uh oh!

mergify bot commented Dec 5, 2025

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

Uh oh!

mergify bot commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Uh oh!

mergify bot commented Dec 5, 2025

Uh oh!

Uh oh!

github-actions bot commented Dec 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mergify bot commented Dec 4, 2025 •

edited

Loading

mergify bot commented Dec 5, 2025 •

edited

Loading