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

#335 Adding warning if a KMS key allows wildcarded principals in its policy #338

Merged
5 commits merged into from
Jan 14, 2020
Merged

#335 Adding warning if a KMS key allows wildcarded principals in its policy #338

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a6bd24e
#335 Adding warning if a KMS key allows wildcarded principals in its …
Jan 8, 2020
fea1e80
Merge branch 'master' into feature/kms-wildcard-principal-rule
Jan 8, 2020
b0f0428
#335 Changing to failure, and adding logic to catch when AWS subkey i…
Jan 8, 2020
2a5ec58
#335 Modifying KMS key wildcard principal rule to use new KMS key mod…
Jan 10, 2020
9d34d99
#335 Updating cfn-model version.
Jan 13, 2020
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
33 changes: 33 additions & 0 deletions lib/cfn-nag/custom_rules/KMSKeyWildcardPrincipalRule.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# frozen_string_literal: true

require 'cfn-nag/violation'
require_relative 'base'

class KMSKeyWildcardPrincipalRule < BaseRule
def rule_text
'KMS key should not allow * principal'
end

def rule_type
Violation::WARNING
end

def rule_id
'W55'
end

def audit_impl(cfn_model)
# Select all AWS::KMS::Key resources to audit
violating_keys = cfn_model.resources_by_type('AWS::KMS::Key').select do |key|
# Select all key policy 'Statement' objects to audit
violating_statements = key.keyPolicy['Statement'].select do |statement|
# Add statement as violating if allowing wildcard principal
statement['Principal'] == '*' && statement['Effect'] == 'Allow'
Copy link

@ghost ghost Jan 8, 2020 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

i believe {"AWS":"*"} is also legal and equivalent to "*"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call! Latest commit adds logic for that contingency and changes this to a failure instead of warning.

end

!violating_statements.empty?
end

violating_keys.map(&:logical_resource_id)
end
end
24 changes: 24 additions & 0 deletions spec/custom_rules/KMSKeyWildcardPrincipalRule_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
require 'spec_helper'
require 'cfn-nag/custom_rules/KMSKeyWildcardPrincipalRule'
require 'cfn-model'

describe KMSKeyWildcardPrincipalRule do
describe 'AWS::KMS::Key' do
context 'when a principal is not set to a wildcard' do
it 'does not return an offending logical resource id' do
cfn_model = CfnParser.new.parse read_test_template('json/kms/kms_key_without_wildcard_principal.json')
actual_logical_resource_ids = KMSKeyWildcardPrincipalRule.new.audit_impl cfn_model

expect(actual_logical_resource_ids).to eq []
end
end
context 'when a principal is set to a wildcard' do
it 'returns an offending logical resource id' do
cfn_model = CfnParser.new.parse read_test_template('json/kms/kms_key_with_wildcard_principal.json')
actual_logical_resource_ids = KMSKeyWildcardPrincipalRule.new.audit_impl cfn_model

expect(actual_logical_resource_ids).to eq ['myKeyWildcardPrincipal']
end
end
end
end
44 changes: 44 additions & 0 deletions spec/test_templates/json/kms/kms_key_with_wildcard_principal.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
{
"Resources": {
"myKeyWildcardPrincipal" : {
"Type" : "AWS::KMS::Key",
"Properties" : {
"Description" : "An example CMK",
"EnableKeyRotation": "true",
"KeyPolicy" : {
"Version": "2012-10-17",
"Id": "key-default-1",
"Statement": [
{
"Sid": "Enable IAM User Permissions",
"Effect": "Allow",
"Principal": {"AWS": "arn:aws:iam::111122223333:root"},
"Action": "kms:*",
"Resource": "*"
},
{
"Sid": "Allow administration of the key",
"Effect": "Allow",
"Principal": "*",
"Action": [
"kms:Create*",
"kms:CancelKeyDeletion"
],
"Resource": "*"
},
{
"Sid": "Allow use of the key",
"Effect": "Allow",
"Principal": "*",
"Action": [
"kms:GenerateDataKey",
"kms:GenerateDataKeyWithoutPlaintext"
],
"Resource": "*"
}
]
}
}
}
}
}
44 changes: 44 additions & 0 deletions spec/test_templates/json/kms/kms_key_without_wildcard_principal.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
{
"Resources": {
"myKeyNoWildcardPrincipal" : {
"Type" : "AWS::KMS::Key",
"Properties" : {
"Description" : "An example CMK",
"EnableKeyRotation": "true",
"KeyPolicy" : {
"Version": "2012-10-17",
"Id": "key-default-1",
"Statement": [
{
"Sid": "Enable IAM User Permissions",
"Effect": "Allow",
"Principal": {"AWS": "arn:aws:iam::111122223333:root"},
"Action": "kms:*",
"Resource": "*"
},
{
"Sid": "Allow administration of the key",
"Effect": "Allow",
"Principal": { "AWS": "arn:aws:iam::123456789012:user/Alice" },
"Action": [
"kms:Create*",
"kms:CancelKeyDeletion"
],
"Resource": "*"
},
{
"Sid": "Allow use of the key",
"Effect": "Allow",
"Principal": { "AWS": "arn:aws:iam::123456789012:user/Bob" },
"Action": [
"kms:GenerateDataKey",
"kms:GenerateDataKeyWithoutPlaintext"
],
"Resource": "*"
}
]
}
}
}
}
}
32 changes: 32 additions & 0 deletions spec/test_templates/yaml/kms/kms_key_with_wildcard_principal.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
---
Resources:
myKeyWildcardPrincipal:
Type: AWS::KMS::Key
Properties:
Description: An example CMK
EnableKeyRotation: true
KeyPolicy:
Version: 2012-10-17
Id: key-default-1
Statement:
- Sid: Enable IAM User Permissions
Effect: Allow
Principal:
AWS: arn:aws:iam::111122223333:root
Action: kms:*
Resource: '*'
- Sid: Allow administration of the key
Effect: Allow
Principal: '*'
Action:
- kms:Create*
- kms:CancelKeyDeletion
Resource: '*'
- Sid: Allow use of the key
Effect: Allow
Principal:
AWS: arn:aws:iam::123456789012:user/Alice
Action:
- kms:GenerateDataKey
- kms:GenerateDataKeyWithoutPlaintext
Resource: '*'
33 changes: 33 additions & 0 deletions spec/test_templates/yaml/kms/kms_key_without_wildcard_principal.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
---
Resources:
myKeyNoWildcardPrincipal:
Type: AWS::KMS::Key
Properties:
Description: An example CMK
EnableKeyRotation: true
KeyPolicy:
Version: 2012-10-17
Id: key-default-1
Statement:
- Sid: Enable IAM User Permissions
Effect: Allow
Principal:
AWS: arn:aws:iam::111122223333:root
Action: kms:*
Resource: '*'
- Sid: Allow administration of the key
Effect: Allow
Principal:
AWS: arn:aws:iam::123456789012:user/Alice
Action:
- kms:Create*
- kms:CancelKeyDeletion
Resource: '*'
- Sid: Allow use of the key
Effect: Allow
Principal:
AWS: arn:aws:iam::123456789012:user/Bob
Action:
- kms:GenerateDataKey
- kms:GenerateDataKeyWithoutPlaintext
Resource: '*'