Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update PassRole rules to not trigger on deny statements. Fixes #584 #586

Merged
merged 1 commit into from
Jan 3, 2022
Merged

Update PassRole rules to not trigger on deny statements. Fixes #584 #586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion lib/cfn-nag/custom_rules/IamRolePassRoleWildcardResourceRule.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ def audit_impl(cfn_model)
violating_roles = cfn_model.resources_by_type('AWS::IAM::Role').select do |role|
violating_policies = role.policy_objects.select do |policy|
violating_statements = policy.policy_document.statements.select do |statement|
passrole_action?(statement) && wildcard_resource?(statement)
statement.effect == 'Allow' && passrole_action?(statement) && wildcard_resource?(statement)
end
!violating_statements.empty?
end
Expand Down
2 changes: 1 addition & 1 deletion lib/cfn-nag/custom_rules/passrole_base_rule.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def audit_impl(cfn_model)

violating_policies = policies.select do |policy|
violating_statements = policy.policy_document.statements.select do |statement|
passrole_action?(statement) && wildcard_resource?(statement)
statement.effect == 'Allow' && passrole_action?(statement) && wildcard_resource?(statement)
end
!violating_statements.empty?
end
Expand Down
2 changes: 1 addition & 1 deletion spec/custom_rules/SPCMRule_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
rule = SPCMRule.new
rule.spcm_threshold = 1
actual_logical_resource_ids = rule.audit_impl cfn_model
expected_logical_resource_ids = %w[InlinePolicyPass]
expected_logical_resource_ids = %w[InlinePolicyPass InlinePolicyDenyPass]

expect(actual_logical_resource_ids).to eq expected_logical_resource_ids
end
Expand Down
30 changes: 28 additions & 2 deletions ...yaml/iam_passrole_resource_wildcard/iam_inline_policy_passrole_resource_wildcard_pass.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ Resources:

GenericGroup:
Type: AWS::IAM::Group
Properties:
Properties:
GroupName: GenericGroup

InlinePolicyPass:
Expand All @@ -15,7 +15,7 @@ Resources:
Statement:
-
Effect: "Allow"
Action:
Action:
- "s3:ListBucket"
- "s3:GetBucketLocation"
Resource: "arn:aws:s3:::*"
Expand All @@ -29,5 +29,31 @@ Resources:
Effect: Allow
Action: "iam:PassRole"
Resource: "arn:aws:s3:::*"
Groups:
- !Ref GenericGroup

InlinePolicyDenyPass:
Type: "AWS::IAM::Policy"
Properties:
PolicyName: WildcardDenyResourcePolicy
PolicyDocument:
Version: "2012-10-17"
Statement:
-
Effect: "Allow"
Action:
- "s3:ListBucket"
- "s3:GetBucketLocation"
Resource: "arn:aws:s3:::*"
-
Effect: Allow
Action:
- "s3:ListBucket"
- "s3:GetBucketLocation"
Resource: "*"
-
Effect: Deny
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general rules that check for things like * resource, etc. should likely ignore the Deny. i.e. this will still trigger the lib/cfn-nag/custom_rules/IamPolicyWildcardResourceRule.rb rule. Otherwise we are properly following the changes requested for PassRole rules themselves.

Action: "iam:PassRole"
Resource: "*"
Groups:
- !Ref GenericGroup
19 changes: 15 additions & 4 deletions ...aml/iam_passrole_resource_wildcard/iam_managed_policy_passrole_resource_wildcard_pass.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ Resources:

GenericGroup:
Type: AWS::IAM::Group
Properties:
Properties:
GroupName: GenericGroup

ManagedPolicyPass1:
Expand All @@ -14,7 +14,7 @@ Resources:
Statement:
-
Effect: "Allow"
Action:
Action:
- "s3:ListBucket"
- "s3:GetBucketLocation"
Resource: "arn:aws:s3:::*"
Expand All @@ -31,7 +31,7 @@ Resources:
- "s3:ListBucket"
- "s3:GetBucketLocation"
Resource: "*"

ManagedPolicyPass3:
Type: "AWS::IAM::ManagedPolicy"
Properties:
Expand All @@ -43,4 +43,15 @@ Resources:
Action: "iam:PassRole"
Resource: "arn:aws:s3:::*"
Groups:
- !Ref GenericGroup
- !Ref GenericGroup

ManagedPolicyPass4:
Type: "AWS::IAM::ManagedPolicy"
Properties:
PolicyDocument:
Version: "2012-10-17"
Statement:
-
Effect: Deny
Action: "iam:PassRole"
Resource: "*"
28 changes: 25 additions & 3 deletions ...emplates/yaml/iam_passrole_resource_wildcard/iam_role_passrole_resource_wildcard_pass.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,30 @@
---
Resources:

RoleDeny:
Type: AWS::IAM::Role
Properties:
AssumeRolePolicyDocument:
Version: "2012-10-17"
Statement:
-
Effect: Allow
Principal:
Service:
- cloudformation.amazonaws.com
Action:
- sts:AssumeRole
Policies:
-
PolicyName: PolicyDeny
PolicyDocument:
Version: "2012-10-17"
Statement:
-
Effect: Deny
Action: "iam:PassRole"
Resource: "*"
RoleFail:
Type: AWS::IAM::Role
Type: AWS::IAM::Role
Properties:
AssumeRolePolicyDocument:
Version: "2012-10-17"
Expand All @@ -22,7 +44,7 @@ Resources:
Statement:
-
Effect: "Allow"
Action:
Action:
- "s3:ListBucket"
- "s3:GetBucketLocation"
Resource: "arn:aws:s3:::*"
Expand Down