Skip to content

Commit 74d1e8b

Browse files
gudipatigudipati
authored
chore(security-guardian): improve failure messages (#37542)
### Issue # (if applicable) N/A ### Reason for this change Currently, when a guard rule fails, the security guardian report only shows the rule name and a pass/fail indicator. After this change, each failure annotation will include a human-readable description of what's wrong and how to fix it. ### Description of changes 1. Updated summary message to include instructions on how to suppress rules as this is an experimental feature 2. Add custom error messages to each guard rule 3. Updated Junit XML post processing to include the custom error messages in annotations 4. Updated unit-tests ### Describe any new or updated permissions being added N/A ### Description of how you validated changes `yarn test` succeeded Validated with an incorrect template using `yarn security-guardian --enhance_xml=true` - ``` Static: <testcase name="sqs-encryption-enabled.guard" time="0"> <failure message="[Type: Static] SQS queue must have encryption enabled. Set &apos;KmsMasterKeyId&apos; to a KMS key ARN or &apos;SqsManagedSseEnabled&apos; to true."> Check was not compliant as property [Properties.KmsMasterKeyId] is missing. Value traversed to [Path=/Resources/SourceQueue6E809DF0[L:2,C:25] Value={&quot;Type&quot;:&quot;AWS::SQS::Queue&quot;,&quot;UpdateReplacePolicy&quot;:&quot;Delete&quot;,&quot;DeletionPolicy&quot;:&quot;Delete&quot;}]. Check was not compliant as property [Properties.SqsManagedSseEnabled] to compare from is missing. Value traversed to [Path=/Resources/SourceQueue6E809DF0[L:2,C:25] Value={&quot;Type&quot;:&quot;AWS::SQS::Queue&quot;,&quot;UpdateReplacePolicy&quot;:&quot;Delete&quot;,&quot;DeletionPolicy&quot;:&quot;Delete&quot;}].</failure> </testcase> Resolved: <testcase name="kinesis-firehose-encryption-enabled.guard" time="0"> <failure message="[Type: Resolved] Kinesis Firehose delivery stream must have encryption enabled. Set &apos;DeliveryStreamEncryptionConfigurationInput.KeyType&apos; to &apos;AWS_OWNED_CMK&apos; or &apos;CUSTOMER_MANAGED_CMK&apos;."> Check was not compliant as property [DeliveryStreamEncryptionConfigurationInput] is missing. Value traversed to [Path=/Resources/DeliveryStream58CF96DB/Properties[L:149,C:20] Value={&quot;DeliveryStreamType&quot;:&quot;DirectPut&quot;,&quot;ExtendedS3DestinationConfiguration&quot;:{&quot;BucketARN&quot;:&quot;arn:aws:s3:::Bucket83908E77&quot;,&quot;BufferingHints&quot;:{&quot;IntervalInSeconds&quot;:30,&quot;SizeInMBs&quot;:5},&quot;RoleARN&quot;:&quot;arn:aws:iam::123456789012:role/DeliveryStreamS3DestinationRoleD96B8345&quot;}}]. Check was not compliant as property [DeliveryStreamEncryptionConfigurationInput.KeyType] is missing. Value traversed to [Path=/Resources/DeliveryStream58CF96DB/Properties[L:149,C:20] Value={&quot;DeliveryStreamType&quot;:&quot;DirectPut&quot;,&quot;ExtendedS3DestinationConfiguration&quot;:{&quot;BucketARN&quot;:&quot;arn:aws:s3:::Bucket83908E77&quot;,&quot;BufferingHints&quot;:{&quot;IntervalInSeconds&quot;:30,&quot;SizeInMBs&quot;:5},&quot;RoleARN&quot;:&quot;arn:aws:iam::123456789012:role/DeliveryStreamS3DestinationRoleD96B8345&quot;}}]. Check was not compliant as property [DeliveryStreamEncryptionConfigurationInput.KeyType] to compare from is missing. Value traversed to [Path=/Resources/DeliveryStream58CF96DB/Properties[L:149,C:20] Value={&quot;DeliveryStreamType&quot;:&quot;DirectPut&quot;,&quot;ExtendedS3DestinationConfiguration&quot;:{&quot;BucketARN&quot;:&quot;arn:aws:s3:::Bucket83908E77&quot;,&quot;BufferingHints&quot;:{&quot;IntervalInSeconds&quot;:30,&quot;SizeInMBs&quot;:5},&quot;RoleARN&quot;:&quot;arn:aws:iam::123456789012:role/DeliveryStreamS3DestinationRoleD96B8345&quot;}}].</failure> </testcase> ``` Example PR with updated security guardian - https://github.com/gudipati/aws-cdk/pull/1/changes#diff-f21e6d7f4ef5955cb4419b155fb577b7d2f41071077c48356c0063b5d49eb85f ### Checklist - [X] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent a0588ed commit 74d1e8b

29 files changed

Lines changed: 183 additions & 52 deletions

‎.github/workflows/security-report.yml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ jobs:
7474
);
7575
7676
if (botComment) {
77-
const disclaimer = '⚠️ **Experimental Feature**: This security report is currently in experimental phase. Results may include false positives and the rules are being actively refined. \n**This security report is NOT a review blocker.** Please try `merge from main` to avoid findings unrelated to the PR.\n\n---\n\n';
77+
const disclaimer = '⚠️ **Experimental Feature**: This security report is currently in experimental phase. Results may include false positives and the rules are being actively refined. \n**This security report is NOT a review blocker.** Please try `merge from main` to avoid findings unrelated to the PR.\nTo suppress a specific rule, see [Suppressing Rules](https://github.com/aws/aws-cdk/blob/main/tools/%40aws-cdk/security-guardian/README.md#suppressing-rules).\n\n---\n\n';
7878
await github.rest.issues.updateComment({
7979
owner: context.repo.owner,
8080
repo: context.repo.repo,
@@ -126,7 +126,7 @@ jobs:
126126
);
127127
128128
if (botComment) {
129-
const disclaimer = '⚠️ **Experimental Feature**: This security report is currently in experimental phase. Results may include false positives and the rules are being actively refined. \n**This security report is NOT a review blocker.** Please try `merge from main` to avoid findings unrelated to the PR.\n\n---\n\n';
129+
const disclaimer = '⚠️ **Experimental Feature**: This security report is currently in experimental phase. Results may include false positives and the rules are being actively refined. \n**This security report is NOT a review blocker.** Please try `merge from main` to avoid findings unrelated to the PR.\nTo suppress a specific rule, see [Suppressing Rules](https://github.com/aws/aws-cdk/blob/main/tools/%40aws-cdk/security-guardian/README.md#suppressing-rules).\n\n---\n\n';
130130
await github.rest.issues.updateComment({
131131
owner: context.repo.owner,
132132
repo: context.repo.repo,

‎tools/@aws-cdk/security-guardian/README.md‎

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -160,27 +160,30 @@ Use `mikepenz/action-junit-report@e08919a3b1fb83a78393dfb775a9c37f17d8eea6` (v6.
160160

161161
### Enhanced Failure Formatting
162162

163-
The tool automatically enhances CFN Guard failure messages by:
163+
When `enhance_xml` is enabled, the tool enhances CFN Guard failure messages by:
164164

165-
- Splitting concatenated failure messages into individual violations
166-
- Extracting exact line numbers and column positions
167-
- Identifying specific CloudFormation resources and properties
168-
- Formatting output for better readability in CI/CD reports
165+
- Extracting human-readable error descriptions from custom `<<##ERROR:...##>>` annotations in guard rules
166+
- Replacing the raw rule name with an actionable message that explains what's wrong and how to fix it
167+
- Splitting concatenated failure details into individual violations
168+
- Prefixing each failure with the validation type (e.g., `[Type: Static]` or `[Type: Resolved]`)
169+
170+
Each guard rule includes a custom error annotation using cfn-guard's `<<...>>` syntax:
171+
172+
```
173+
<<##ERROR:EBS volume must have encryption enabled. Set 'Encrypted' to true.##>>
174+
```
169175

170176
**Before (Raw CFN Guard Output):**
171177

172-
```text
173-
IAM_NO_WILDCARD_ACTIONS_INLINE for Type: ResolvedCheck was not compliant as property [Policies[*].PolicyDocument.Statement[*]] is missing. Value traversed to [Path=/Resources/Role1/Properties[L:324,C:20]]Check was not compliant as property [Policies[*].PolicyDocument.Statement[*]] is missing. Value traversed to [Path=/Resources/Role2/Properties[L:485,C:20]]
178+
```xml
179+
<failure message="EBS_ENCRYPTION_ENABLED for Type: Static">Check was not compliant as property [Properties.Encrypted] is missing.</failure>
174180
```
175181

176182
**After (Enhanced Format):**
177183

178-
```text
179-
Rule: IAM_NO_WILDCARD_ACTIONS_INLINE (Type: Resolved)
180-
==================================================
181-
182-
- Check was not compliant as property [Policies[*].PolicyDocument.Statement[*]] is missing. Value traversed to [Path=/Resources/Role1/Properties[L:324,C:20]]
183-
- Check was not compliant as property [Policies[*].PolicyDocument.Statement[*]] is missing. Value traversed to [Path=/Resources/Role2/Properties[L:485,C:20]]
184+
```xml
185+
<failure message="[Type: Static] EBS volume must have encryption enabled. Set 'Encrypted' to true.">
186+
Check was not compliant as property [Properties.Encrypted] is missing.</failure>
184187
```
185188

186189
---

‎tools/@aws-cdk/security-guardian/rules/documentdb/documentdb-encryption-enabled.guard‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,5 +17,6 @@ rule DOCUMENTDB_ENCRYPTION_ENABLED when %documentdb_clusters !empty {
1717
%documentdb_clusters {
1818
Properties.StorageEncrypted exists
1919
Properties.StorageEncrypted == true
20+
<<##ERROR:DocumentDB cluster must have encryption enabled. Set 'StorageEncrypted' to true.##>>
2021
}
21-
}
22+
}

‎tools/@aws-cdk/security-guardian/rules/ec2/ec2-ebs-encryption-enabled.guard‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,5 +17,6 @@ rule EBS_ENCRYPTION_ENABLED when %ebs_volumes !empty {
1717
%ebs_volumes {
1818
Properties.Encrypted exists
1919
Properties.Encrypted == true
20+
<<##ERROR:EBS volume must have encryption enabled. Set 'Encrypted' to true.##>>
2021
}
21-
}
22+
}

‎tools/@aws-cdk/security-guardian/rules/ec2/ec2-no-open-security-groups.guard‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,11 @@ rule EC2_NO_OPEN_SECURITY_GROUPS when %security_groups !empty {
2020
Properties.SecurityGroupIngress[*] {
2121
when CidrIp exists {
2222
CidrIp != "0.0.0.0/0"
23+
<<##ERROR:Security group must not allow unrestricted ingress. Remove '0.0.0.0/0' from CidrIp and scope to specific IP ranges.##>>
2324
}
2425
when CidrIpv6 exists {
2526
CidrIpv6 != "::/0"
27+
<<##ERROR:Security group must not allow unrestricted ingress. Remove '::/0' from CidrIpv6 and scope to specific IP ranges.##>>
2628
}
2729
}
2830
}
@@ -33,4 +35,4 @@ rule EC2_NO_OPEN_SECURITY_GROUPS when %security_groups !empty {
3335
# 3. Most applications legitimately need unrestricted outbound access
3436
# 4. Restricting egress is an advanced security practice, not a default requirement
3537
}
36-
}
38+
}

‎tools/@aws-cdk/security-guardian/rules/iam/iam-no-overly-permissive-passrole.guard‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,11 @@ rule IAM_NO_OVERLY_PERMISSIVE_PASSROLE when %iam_passrole_resources !empty {
2525
when Resource exists {
2626
when Resource is_string {
2727
Resource != "*"
28+
<<##ERROR:iam:PassRole must not use wildcard resources. Replace '*' in Resource with specific role ARNs that can be passed.##>>
2829
}
2930
when Resource is_list {
3031
Resource[*] != "*"
32+
<<##ERROR:iam:PassRole must not use wildcard resources. Replace '*' in Resource with specific role ARNs that can be passed.##>>
3133
}
3234
}
3335
}
@@ -37,9 +39,11 @@ rule IAM_NO_OVERLY_PERMISSIVE_PASSROLE when %iam_passrole_resources !empty {
3739
when Resource exists {
3840
when Resource is_string {
3941
Resource != "*"
42+
<<##ERROR:iam:PassRole must not use wildcard resources. Replace '*' in Resource with specific role ARNs that can be passed.##>>
4043
}
4144
when Resource is_list {
4245
Resource[*] != "*"
46+
<<##ERROR:iam:PassRole must not use wildcard resources. Replace '*' in Resource with specific role ARNs that can be passed.##>>
4347
}
4448
}
4549
}
@@ -59,9 +63,11 @@ rule IAM_NO_OVERLY_PERMISSIVE_PASSROLE when %iam_passrole_resources !empty {
5963
when Resource exists {
6064
when Resource is_string {
6165
Resource != "*"
66+
<<##ERROR:iam:PassRole must not use wildcard resources. Replace '*' in Resource with specific role ARNs that can be passed.##>>
6267
}
6368
when Resource is_list {
6469
Resource[*] != "*"
70+
<<##ERROR:iam:PassRole must not use wildcard resources. Replace '*' in Resource with specific role ARNs that can be passed.##>>
6571
}
6672
}
6773
}
@@ -71,9 +77,11 @@ rule IAM_NO_OVERLY_PERMISSIVE_PASSROLE when %iam_passrole_resources !empty {
7177
when Resource exists {
7278
when Resource is_string {
7379
Resource != "*"
80+
<<##ERROR:iam:PassRole must not use wildcard resources. Replace '*' in Resource with specific role ARNs that can be passed.##>>
7481
}
7582
when Resource is_list {
7683
Resource[*] != "*"
84+
<<##ERROR:iam:PassRole must not use wildcard resources. Replace '*' in Resource with specific role ARNs that can be passed.##>>
7785
}
7886
}
7987
}
@@ -83,4 +91,4 @@ rule IAM_NO_OVERLY_PERMISSIVE_PASSROLE when %iam_passrole_resources !empty {
8391
}
8492
}
8593
}
86-
}
94+
}

‎tools/@aws-cdk/security-guardian/rules/iam/iam-no-wildcard-actions-inline.guard‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,11 +21,15 @@ rule IAM_NO_WILDCARD_ACTIONS_INLINE when %iam_roles !empty {
2121
when Action exists {
2222
when Action is_string {
2323
Action != "*"
24+
<<##ERROR:IAM role inline policy must not use wildcard actions. Replace '*' in Action with specific actions needed.##>>
2425
Action != /\w+:\*$/
26+
<<##ERROR:IAM role inline policy must not use wildcard actions. Replace 'service:*' in Action with specific actions needed.##>>
2527
}
2628
when Action is_list {
2729
Action[*] != "*"
30+
<<##ERROR:IAM role inline policy must not use wildcard actions. Replace '*' in Action with specific actions needed.##>>
2831
Action[*] != /\w+:\*$/
32+
<<##ERROR:IAM role inline policy must not use wildcard actions. Replace 'service:*' in Action with specific actions needed.##>>
2933
}
3034
}
3135
}
@@ -38,15 +42,19 @@ rule IAM_NO_WILDCARD_ACTIONS_INLINE when %iam_roles !empty {
3842
when Action exists {
3943
when Action is_string {
4044
Action != "*"
45+
<<##ERROR:IAM role inline policy must not use wildcard actions. Replace '*' in Action with specific actions needed.##>>
4146
Action != /\w+:\*$/
47+
<<##ERROR:IAM role inline policy must not use wildcard actions. Replace 'service:*' in Action with specific actions needed.##>>
4248
}
4349
when Action is_list {
4450
Action[*] != "*"
51+
<<##ERROR:IAM role inline policy must not use wildcard actions. Replace '*' in Action with specific actions needed.##>>
4552
Action[*] != /\w+:\*$/
53+
<<##ERROR:IAM role inline policy must not use wildcard actions. Replace 'service:*' in Action with specific actions needed.##>>
4654
}
4755
}
4856
}
4957
}
5058
}
5159
}
52-
}
60+
}

‎tools/@aws-cdk/security-guardian/rules/iam/iam-no-wildcard-actions.guard‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,14 +20,18 @@ rule IAM_NO_WILDCARD_ACTIONS when %iam_policies !empty {
2020
when Action exists {
2121
when Action is_string {
2222
Action != "*"
23+
<<##ERROR:IAM policy must not use wildcard actions. Replace '*' in Action with specific actions needed.##>>
2324
Action != /\w+:\*$/
25+
<<##ERROR:IAM policy must not use wildcard actions. Replace 'service:*' in Action with specific actions needed.##>>
2426
}
2527
when Action is_list {
2628
Action[*] != "*"
29+
<<##ERROR:IAM policy must not use wildcard actions. Replace '*' in Action with specific actions needed.##>>
2730
Action[*] != /\w+:\*$/
31+
<<##ERROR:IAM policy must not use wildcard actions. Replace 'service:*' in Action with specific actions needed.##>>
2832
}
2933
}
3034
}
3135
}
3236
}
33-
}
37+
}

‎tools/@aws-cdk/security-guardian/rules/iam/iam-no-world-accessible-trust-policy.guard‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,14 @@ let iam_roles = Resources.*[
1616
# Note: Only string values are checked - intrinsic functions (maps like Fn::GetAtt) are skipped
1717
rule IAM_NO_WORLD_ACCESSIBLE_TRUST_POLICY when %iam_roles !empty {
1818
%iam_roles.Properties.AssumeRolePolicyDocument.Statement[ Effect == "Allow" ] {
19-
when Principal is_string { Principal != "*" }
20-
when Principal.AWS is_string { Principal.AWS != "*" }
21-
when Principal.AWS is_list { Principal.AWS[*][ this is_string ] != "*" }
19+
when Principal is_string { Principal != "*"
20+
<<##ERROR:IAM role trust policy must not be world-accessible. Remove '*' from Principal and scope to specific accounts, roles, or services.##>>
21+
}
22+
when Principal.AWS is_string { Principal.AWS != "*"
23+
<<##ERROR:IAM role trust policy must not be world-accessible. Remove '*' from Principal and scope to specific accounts, roles, or services.##>>
24+
}
25+
when Principal.AWS is_list { Principal.AWS[*][ this is_string ] != "*"
26+
<<##ERROR:IAM role trust policy must not be world-accessible. Remove '*' from Principal and scope to specific accounts, roles, or services.##>>
27+
}
2228
}
23-
}
29+
}

‎tools/@aws-cdk/security-guardian/rules/iam/iam-policy-no-broad-principals.guard‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,25 +22,32 @@ rule IAM_POLICY_NO_BROAD_PRINCIPALS when %iam_policies_no_broad_principals !empt
2222
# Check for wildcard as entire principal
2323
when Principal is_string {
2424
Principal != "*"
25+
<<##ERROR:IAM policy must not use broadly scoped principals. Remove '*', root principals, or wildcard account ARNs from Principal and scope to specific accounts or roles.##>>
2526
}
2627
# Check if AWS principal exists
2728
when Principal.AWS exists {
2829
# Check if AWS is a string
2930
when Principal.AWS is_string {
3031
Principal.AWS != "*"
32+
<<##ERROR:IAM policy must not use broadly scoped principals. Remove '*', root principals, or wildcard account ARNs from Principal and scope to specific accounts or roles.##>>
3133
Principal.AWS != /(?i):root/
34+
<<##ERROR:IAM policy must not use broadly scoped principals. Remove '*', root principals, or wildcard account ARNs from Principal and scope to specific accounts or roles.##>>
3235
Principal.AWS != /arn:aws:iam::\*:/
36+
<<##ERROR:IAM policy must not use broadly scoped principals. Remove '*', root principals, or wildcard account ARNs from Principal and scope to specific accounts or roles.##>>
3337
}
3438
# Check if AWS is an array - only check string items
3539
when Principal.AWS is_list {
3640
when Principal.AWS[*] is_string {
3741
Principal.AWS[*] != "*"
42+
<<##ERROR:IAM policy must not use broadly scoped principals. Remove '*', root principals, or wildcard account ARNs from Principal and scope to specific accounts or roles.##>>
3843
Principal.AWS[*] != /(?i):root/
44+
<<##ERROR:IAM policy must not use broadly scoped principals. Remove '*', root principals, or wildcard account ARNs from Principal and scope to specific accounts or roles.##>>
3945
Principal.AWS[*] != /arn:aws:iam::\*:/
46+
<<##ERROR:IAM policy must not use broadly scoped principals. Remove '*', root principals, or wildcard account ARNs from Principal and scope to specific accounts or roles.##>>
4047
}
4148
}
4249
}
4350
}
4451
}
4552
}
46-
}
53+
}

0 commit comments

Comments
 (0)