-
Notifications
You must be signed in to change notification settings - Fork 212
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.
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
Feature/issue 253 amazonmq broker user password #301
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
# frozen_string_literal: true | ||
|
||
require 'cfn-nag/violation' | ||
require 'cfn-nag/util/enforce_reference_parameter' | ||
require 'cfn-nag/util/enforce_string_or_dynamic_reference' | ||
require_relative 'base' | ||
|
||
class AmazonMQBrokerUserPasswordRule < BaseRule | ||
|
||
def rule_text | ||
'Amazon MQ Broker resource Users property should exist and its Password property value ' + | ||
'should not show password in plain text, resolve an unsecure ssm string, or have a default value for parameter.' | ||
end | ||
|
||
def rule_type | ||
Violation::FAILING_VIOLATION | ||
end | ||
|
||
def rule_id | ||
'F52' | ||
end | ||
|
||
def check_password_property(cfn_model, user) | ||
# Checks to make sure 'Users' property has the key 'Password' defined. | ||
# Also check if the value for the 'Password' key is nil | ||
if user.has_key? 'Password' | ||
if insecure_parameter?(cfn_model, user['Password']) | ||
true | ||
elsif insecure_string_or_dynamic_reference?(cfn_model, user['Password']) | ||
true | ||
elsif user['Password'].nil? | ||
true | ||
end | ||
else | ||
true | ||
end | ||
end | ||
|
||
def check_user_property(user_property) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'd nuke this method. the return values could be true or nil which is possibly confusing - but you can check the nil? inline where this is invoked imho There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. make this private |
||
# Checks to make sure that the 'Users' property that is part of the 'AWS::AmazonMQ::Broker' | ||
# resource exists and is defined. | ||
if !user_property.nil? | ||
true | ||
end | ||
end | ||
|
||
def get_violating_users(cfn_model, resource) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. call this violating_users? name the resource argument as"mq_broker There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. make this private |
||
# If the 'Users' property is defined then try to grab the violating user password keys | ||
# that are oare of the 'Users' property list. | ||
if check_user_property(resource.users) | ||
violating_users = resource.users.select do |user| | ||
check_password_property(cfn_model, user) | ||
end | ||
!violating_users.empty? | ||
else | ||
true | ||
end | ||
end | ||
|
||
def audit_impl(cfn_model) | ||
resources = cfn_model.resources_by_type('AWS::AmazonMQ::Broker') | ||
violating_resources = resources.select do |resource| | ||
get_violating_users(cfn_model, resource) | ||
end | ||
violating_resources.map(&:logical_resource_id) | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,140 @@ | ||
require 'spec_helper' | ||
require 'password_rule_spec_helper' | ||
require 'cfn-model' | ||
require 'cfn-nag/custom_rules/AmazonMQBrokerUserPasswordRule' | ||
|
||
describe AmazonMQBrokerUserPasswordRule do | ||
context 'AmazonMQBroker resource with Users password in plain text' do | ||
it 'Returns the logical resource ID of the offending AmazonMQBroker resource' do | ||
cfn_model = CfnParser.new.parse read_test_template( | ||
'yaml/amazon_mq/amazon_mq_broker_user_password_plain_text.yaml' | ||
) | ||
|
||
actual_logical_resource_ids = | ||
AmazonMQBrokerUserPasswordRule.new.audit_impl cfn_model | ||
expected_logical_resource_ids = %w[AmazonMQBroker AmazonMQBroker2] | ||
|
||
expect(actual_logical_resource_ids).to eq expected_logical_resource_ids | ||
end | ||
end | ||
|
||
context 'AmazonMQBroker resource with parameter password with NoEcho but default value' do | ||
it 'Returns the logical resource ID of the offending AmazonMQBroker resource' do | ||
cfn_model = CfnParser.new.parse read_test_template( | ||
'yaml/amazon_mq/amazon_mq_broker_user_password_parameter_noecho_with_default.yaml' | ||
) | ||
|
||
actual_logical_resource_ids = | ||
AmazonMQBrokerUserPasswordRule.new.audit_impl cfn_model | ||
expected_logical_resource_ids = %w[AmazonMQBroker AmazonMQBroker2] | ||
|
||
expect(actual_logical_resource_ids).to eq expected_logical_resource_ids | ||
end | ||
end | ||
|
||
context 'AmazonMQBroker resource with parameter password with default value' do | ||
it 'Returns the logical resource ID of the offending AmazonMQBroker resource' do | ||
cfn_model = CfnParser.new.parse read_test_template( | ||
'yaml/amazon_mq/amazon_mq_broker_user_password_parameter_default.yaml' | ||
) | ||
|
||
actual_logical_resource_ids = | ||
AmazonMQBrokerUserPasswordRule.new.audit_impl cfn_model | ||
expected_logical_resource_ids = %w[AmazonMQBroker AmazonMQBroker2] | ||
|
||
expect(actual_logical_resource_ids).to eq expected_logical_resource_ids | ||
end | ||
end | ||
|
||
context 'AmazonMQBroker resource with parameter password with NoEcho' do | ||
it 'returns empty list' do | ||
cfn_model = CfnParser.new.parse read_test_template( | ||
'yaml/amazon_mq/amazon_mq_broker_user_password_parameter_noecho.yaml' | ||
) | ||
|
||
actual_logical_resource_ids = | ||
AmazonMQBrokerUserPasswordRule.new.audit_impl cfn_model | ||
expected_logical_resource_ids = %w[] | ||
|
||
expect(actual_logical_resource_ids).to eq expected_logical_resource_ids | ||
end | ||
end | ||
|
||
context 'AmazonMQBroker has password from Secrets Manager' do | ||
it 'returns empty list' do | ||
cfn_model = CfnParser.new.parse read_test_template( | ||
'yaml/amazon_mq/amazon_mq_broker_user_password_secrets_manager.yaml' | ||
) | ||
actual_logical_resource_ids = | ||
AmazonMQBrokerUserPasswordRule.new.audit_impl cfn_model | ||
expected_logical_resource_ids = %w[] | ||
|
||
expect(actual_logical_resource_ids).to eq expected_logical_resource_ids | ||
end | ||
end | ||
|
||
context 'AmazonMQBroker has password from Secure String in Systems Manager' do | ||
it 'returns empty list' do | ||
cfn_model = CfnParser.new.parse read_test_template( | ||
'yaml/amazon_mq/amazon_mq_broker_user_password_ssm_secure.yaml' | ||
) | ||
actual_logical_resource_ids = | ||
AmazonMQBrokerUserPasswordRule.new.audit_impl cfn_model | ||
expected_logical_resource_ids = %w[] | ||
|
||
expect(actual_logical_resource_ids).to eq expected_logical_resource_ids | ||
end | ||
end | ||
|
||
context 'AmazonMQBroker has password from Not Secure String in Systems Manager' do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing password is one of the contexts to capture for coverage |
||
it 'returns offending logical resource id for offending AmazonMQBroker resource' do | ||
cfn_model = CfnParser.new.parse read_test_template( | ||
'yaml/amazon_mq/amazon_mq_broker_user_password_ssm_not_secure.yaml' | ||
) | ||
actual_logical_resource_ids = | ||
AmazonMQBrokerUserPasswordRule.new.audit_impl cfn_model | ||
expected_logical_resource_ids = %w[AmazonMQBroker AmazonMQBroker2] | ||
|
||
expect(actual_logical_resource_ids).to eq expected_logical_resource_ids | ||
end | ||
end | ||
|
||
context 'AmazonMQBroker has Users property defined' do | ||
it 'returns offending logical resource id for offending AmazonMQBroker resource' do | ||
cfn_model = CfnParser.new.parse read_test_template( | ||
'yaml/amazon_mq/amazon_mq_broker_user_property_not_defined.yaml' | ||
) | ||
actual_logical_resource_ids = | ||
AmazonMQBrokerUserPasswordRule.new.audit_impl cfn_model | ||
expected_logical_resource_ids = %w[AmazonMQBroker] | ||
|
||
expect(actual_logical_resource_ids).to eq expected_logical_resource_ids | ||
end | ||
end | ||
|
||
context 'AmazonMQBroker has Users property with Password value nil' do | ||
it 'returns offending logical resource id for offending AmazonMQBroker resource' do | ||
cfn_model = CfnParser.new.parse read_test_template( | ||
'yaml/amazon_mq/amazon_mq_broker_user_password_value_nil.yaml' | ||
) | ||
actual_logical_resource_ids = | ||
AmazonMQBrokerUserPasswordRule.new.audit_impl cfn_model | ||
expected_logical_resource_ids = %w[AmazonMQBroker] | ||
|
||
expect(actual_logical_resource_ids).to eq expected_logical_resource_ids | ||
end | ||
end | ||
|
||
context 'AmazonMQBroker has Users property with no Password key' do | ||
it 'returns offending logical resource id for offending AmazonMQBroker resource' do | ||
cfn_model = CfnParser.new.parse read_test_template( | ||
'yaml/amazon_mq/amazon_mq_broker_user_password_key_not_defined.yaml' | ||
) | ||
actual_logical_resource_ids = | ||
AmazonMQBrokerUserPasswordRule.new.audit_impl cfn_model | ||
expected_logical_resource_ids = %w[AmazonMQBroker] | ||
|
||
expect(actual_logical_resource_ids).to eq expected_logical_resource_ids | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
--- | ||
Resources: | ||
AmazonMQBroker: | ||
Type: AWS::AmazonMQ::Broker | ||
Properties: | ||
AutoMinorVersionUpgrade: true | ||
BrokerName: MyAmazonMQBroker | ||
DeploymentMode: SINGLE_INSTANCE | ||
EngineType: ACTIVEMQ | ||
EngineVersion: "5.15.0" | ||
HostInstanceType: mq.t2.micro | ||
PubliclyAccessible: false | ||
Users: | ||
- Username: admin | ||
|
||
AmazonMQBroker2: | ||
Type: AWS::AmazonMQ::Broker | ||
Properties: | ||
AutoMinorVersionUpgrade: true | ||
BrokerName: MyAmazonMQBroker | ||
DeploymentMode: SINGLE_INSTANCE | ||
EngineType: ACTIVEMQ | ||
EngineVersion: "5.15.0" | ||
HostInstanceType: mq.t2.micro | ||
PubliclyAccessible: false | ||
Users: | ||
- Password: '{{resolve:ssm-secure:SecureSecretString:1}}' | ||
Username: admin1 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
--- | ||
Parameters: | ||
AmazonMQBrokerUserPassword1: | ||
Type: String | ||
Default: BadPassword123 | ||
AmazonMQBrokerUserPassword2: | ||
Type: String | ||
Default: BadPassword123 | ||
AmazonMQBrokerUserPassword3: | ||
Type: String | ||
NoEcho: true | ||
Resources: | ||
AmazonMQBroker: | ||
Type: AWS::AmazonMQ::Broker | ||
Properties: | ||
AutoMinorVersionUpgrade: true | ||
BrokerName: MyAmazonMQBroker | ||
DeploymentMode: SINGLE_INSTANCE | ||
EngineType: ACTIVEMQ | ||
EngineVersion: "5.15.0" | ||
HostInstanceType: mq.t2.micro | ||
PubliclyAccessible: false | ||
Users: | ||
- Password: !Ref AmazonMQBrokerUserPassword1 | ||
Username: admin1 | ||
- Password: !Ref AmazonMQBrokerUserPassword1 | ||
Username: admin2 | ||
|
||
AmazonMQBroker2: | ||
Type: AWS::AmazonMQ::Broker | ||
Properties: | ||
AutoMinorVersionUpgrade: true | ||
BrokerName: MyAmazonMQBroker | ||
DeploymentMode: SINGLE_INSTANCE | ||
EngineType: ACTIVEMQ | ||
EngineVersion: "5.15.0" | ||
HostInstanceType: mq.t2.micro | ||
PubliclyAccessible: false | ||
Users: | ||
- Password: !Ref AmazonMQBrokerUserPassword2 | ||
Username: admin1 | ||
|
||
AmazonMQBroker3: | ||
Type: AWS::AmazonMQ::Broker | ||
Properties: | ||
AutoMinorVersionUpgrade: true | ||
BrokerName: MyAmazonMQBroker | ||
DeploymentMode: SINGLE_INSTANCE | ||
EngineType: ACTIVEMQ | ||
EngineVersion: "5.15.0" | ||
HostInstanceType: mq.t2.micro | ||
PubliclyAccessible: false | ||
Users: | ||
- Password: !Ref AmazonMQBrokerUserPassword3 | ||
Username: admin1 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
--- | ||
Parameters: | ||
AmazonMQBrokerUserPassword1: | ||
Type: String | ||
NoEcho: true | ||
AmazonMQBrokerUserPassword2: | ||
Type: String | ||
NoEcho: true | ||
Resources: | ||
AmazonMQBroker: | ||
Type: AWS::AmazonMQ::Broker | ||
Properties: | ||
AutoMinorVersionUpgrade: true | ||
BrokerName: MyAmazonMQBroker | ||
DeploymentMode: SINGLE_INSTANCE | ||
EngineType: ACTIVEMQ | ||
EngineVersion: "5.15.0" | ||
HostInstanceType: mq.t2.micro | ||
PubliclyAccessible: false | ||
Users: | ||
- Password: !Ref AmazonMQBrokerUserPassword1 | ||
Username: admin1 | ||
- Password: !Ref AmazonMQBrokerUserPassword1 | ||
Username: admin2 | ||
|
||
AmazonMQBroker2: | ||
Type: AWS::AmazonMQ::Broker | ||
Properties: | ||
AutoMinorVersionUpgrade: true | ||
BrokerName: MyAmazonMQBroker | ||
DeploymentMode: SINGLE_INSTANCE | ||
EngineType: ACTIVEMQ | ||
EngineVersion: "5.15.0" | ||
HostInstanceType: mq.t2.micro | ||
PubliclyAccessible: false | ||
Users: | ||
- Password: !Ref AmazonMQBrokerUserPassword2 | ||
Username: admin1 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
--- | ||
Parameters: | ||
AmazonMQBrokerUserPassword1: | ||
Type: String | ||
NoEcho: true | ||
Default: BadPassword123 | ||
AmazonMQBrokerUserPassword2: | ||
Type: String | ||
NoEcho: true | ||
Default: BadPassword123 | ||
AmazonMQBrokerUserPassword3: | ||
Type: String | ||
NoEcho: true | ||
Resources: | ||
AmazonMQBroker: | ||
Type: AWS::AmazonMQ::Broker | ||
Properties: | ||
AutoMinorVersionUpgrade: true | ||
BrokerName: MyAmazonMQBroker | ||
DeploymentMode: SINGLE_INSTANCE | ||
EngineType: ACTIVEMQ | ||
EngineVersion: "5.15.0" | ||
HostInstanceType: mq.t2.micro | ||
PubliclyAccessible: false | ||
Users: | ||
- Password: !Ref AmazonMQBrokerUserPassword1 | ||
Username: admin1 | ||
- Password: !Ref AmazonMQBrokerUserPassword1 | ||
Username: admin2 | ||
|
||
AmazonMQBroker2: | ||
Type: AWS::AmazonMQ::Broker | ||
Properties: | ||
AutoMinorVersionUpgrade: true | ||
BrokerName: MyAmazonMQBroker | ||
DeploymentMode: SINGLE_INSTANCE | ||
EngineType: ACTIVEMQ | ||
EngineVersion: "5.15.0" | ||
HostInstanceType: mq.t2.micro | ||
PubliclyAccessible: false | ||
Users: | ||
- Password: !Ref AmazonMQBrokerUserPassword2 | ||
Username: admin1 | ||
|
||
AmazonMQBroker3: | ||
Type: AWS::AmazonMQ::Broker | ||
Properties: | ||
AutoMinorVersionUpgrade: true | ||
BrokerName: MyAmazonMQBroker | ||
DeploymentMode: SINGLE_INSTANCE | ||
EngineType: ACTIVEMQ | ||
EngineVersion: "5.15.0" | ||
HostInstanceType: mq.t2.micro | ||
PubliclyAccessible: false | ||
Users: | ||
- Password: !Ref AmazonMQBrokerUserPassword3 | ||
Username: admin1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'check' is a bit opaque as a term. while i would never say comments are never necessary, imho writing them usually means something isn't obvious from the code itself.
user_has_insecure_password? is probably a better name