From 860f7eceaf6ecbc5701a78da2f37bacfd0d413b2 Mon Sep 17 00:00:00 2001 From: Eric Kascic Date: Wed, 29 Apr 2020 11:17:54 -0400 Subject: [PATCH] #443 Warn on missing KmsKeyId for a Secret (#444) * #440 fix rubocop and use the common not_truth? method instead * #443 Add rule for explicit kms key in a secret. Make all booleans check for NoValue --- Dockerfile | 4 +- Dockerfile.local | 12 +++++ .../EbsVolumeEncryptionKeyRule.rb | 17 ++++--- .../SecretsManagerSecretKmsKeyIdRule.rb | 26 ++++++++++ .../SnsTopicKmsMasterKeyIdRule.rb | 16 +++---- .../SqsQueueKmsMasterKeyIdRule.rb | 16 +++---- lib/cfn-nag/custom_rules/boolean_base_rule.rb | 8 +++- scripts/publish.sh | 9 +--- .../SecretsManagerSecretKmsKeyIdRule_spec.rb | 26 ++++++++++ .../json/redshift/cluster_with_kms.json | 21 +++++++++ .../yaml/secretsmanager/explicit_key.yml | 47 +++++++++++++++++++ 11 files changed, 165 insertions(+), 37 deletions(-) create mode 100644 Dockerfile.local create mode 100644 lib/cfn-nag/custom_rules/SecretsManagerSecretKmsKeyIdRule.rb create mode 100644 spec/custom_rules/SecretsManagerSecretKmsKeyIdRule_spec.rb create mode 100644 spec/test_templates/json/redshift/cluster_with_kms.json create mode 100644 spec/test_templates/yaml/secretsmanager/explicit_key.yml diff --git a/Dockerfile b/Dockerfile index a6fc03a2..10b59f5e 100644 --- a/Dockerfile +++ b/Dockerfile @@ -6,9 +6,7 @@ LABEL org.opencontainers.image.authors="eric.kascic@stelligent.com" # override here for a real build process ARG version='' -RUN gem install cfn-nag --version "$version" || true -RUN sleep 30; gem install cfn-nag --version "$version" || true -RUN sleep 30; gem install cfn-nag --version "$version" || true +RUN gem install cfn-nag --version "$version" ENTRYPOINT ["cfn_nag"] CMD ["--help"] diff --git a/Dockerfile.local b/Dockerfile.local new file mode 100644 index 00000000..c6656381 --- /dev/null +++ b/Dockerfile.local @@ -0,0 +1,12 @@ +FROM ruby:2.5-alpine3.9@sha256:f33782620b363575ad95d19d0f0f07f7d197e9ccfee51f20df39dd33d408cdb4 + +LABEL org.opencontainers.image.authors="eric.kascic@stelligent.com" + +ARG version + +ADD cfn-nag-${version}.gem + +RUN gem install cfn-nag-${version}.gem + +ENTRYPOINT ["cfn_nag"] +CMD ["--help"] diff --git a/lib/cfn-nag/custom_rules/EbsVolumeEncryptionKeyRule.rb b/lib/cfn-nag/custom_rules/EbsVolumeEncryptionKeyRule.rb index 009baa2b..1b46e9dc 100644 --- a/lib/cfn-nag/custom_rules/EbsVolumeEncryptionKeyRule.rb +++ b/lib/cfn-nag/custom_rules/EbsVolumeEncryptionKeyRule.rb @@ -1,13 +1,17 @@ # frozen_string_literal: true require 'cfn-nag/violation' -require_relative 'base' +require_relative 'boolean_base_rule' -class EbsVolumeEncryptionKeyRule < BaseRule +class EbsVolumeEncryptionKeyRule < BooleanBaseRule def rule_text 'EBS Volume should specify a KmsKeyId value' end + def resource_type + 'AWS::EC2::Volume' + end + def rule_type Violation::WARNING end @@ -16,12 +20,7 @@ def rule_id 'W37' end - def audit_impl(cfn_model) - violating_volumes = cfn_model.resources_by_type('AWS::EC2::Volume') - .select do |volume| - volume.kmsKeyId.nil? || volume.kmsKeyId == { 'Ref' => 'AWS::NoValue' } - end - - violating_volumes.map(&:logical_resource_id) + def boolean_property + :kmsKeyId end end diff --git a/lib/cfn-nag/custom_rules/SecretsManagerSecretKmsKeyIdRule.rb b/lib/cfn-nag/custom_rules/SecretsManagerSecretKmsKeyIdRule.rb new file mode 100644 index 00000000..32a1349e --- /dev/null +++ b/lib/cfn-nag/custom_rules/SecretsManagerSecretKmsKeyIdRule.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require 'cfn-nag/violation' +require_relative 'boolean_base_rule' + +class SecretsManagerSecretKmsKeyIdRule < BooleanBaseRule + def rule_text + 'Secrets Manager Secret should explicitly specify KmsKeyId' + end + + def rule_type + Violation::FAILING_VIOLATION + end + + def rule_id + 'F81' + end + + def resource_type + 'AWS::SecretsManager::Secret' + end + + def boolean_property + :kmsKeyId + end +end diff --git a/lib/cfn-nag/custom_rules/SnsTopicKmsMasterKeyIdRule.rb b/lib/cfn-nag/custom_rules/SnsTopicKmsMasterKeyIdRule.rb index 8f9d3318..05321c43 100644 --- a/lib/cfn-nag/custom_rules/SnsTopicKmsMasterKeyIdRule.rb +++ b/lib/cfn-nag/custom_rules/SnsTopicKmsMasterKeyIdRule.rb @@ -1,13 +1,17 @@ # frozen_string_literal: true require 'cfn-nag/violation' -require_relative 'base' +require_relative 'boolean_base_rule' -class SnsTopicKmsMasterKeyIdRule < BaseRule +class SnsTopicKmsMasterKeyIdRule < BooleanBaseRule def rule_text 'SNS Topic should specify KmsMasterKeyId property' end + def resource_type + 'AWS::SNS::Topic' + end + def rule_type Violation::WARNING end @@ -16,11 +20,7 @@ def rule_id 'W47' end - def audit_impl(cfn_model) - violating_sns_topics = cfn_model.resources_by_type('AWS::SNS::Topic').select do |topic| - topic.kmsMasterKeyId.nil? - end - - violating_sns_topics.map(&:logical_resource_id) + def boolean_property + :kmsMasterKeyId end end diff --git a/lib/cfn-nag/custom_rules/SqsQueueKmsMasterKeyIdRule.rb b/lib/cfn-nag/custom_rules/SqsQueueKmsMasterKeyIdRule.rb index 55c703f8..3a88b2fc 100644 --- a/lib/cfn-nag/custom_rules/SqsQueueKmsMasterKeyIdRule.rb +++ b/lib/cfn-nag/custom_rules/SqsQueueKmsMasterKeyIdRule.rb @@ -1,13 +1,17 @@ # frozen_string_literal: true require 'cfn-nag/violation' -require_relative 'base' +require_relative 'boolean_base_rule' -class SqsQueueKmsMasterKeyIdRule < BaseRule +class SqsQueueKmsMasterKeyIdRule < BooleanBaseRule def rule_text 'SQS Queue should specify KmsMasterKeyId property' end + def resource_type + 'AWS::SQS::Queue' + end + def rule_type Violation::WARNING end @@ -16,11 +20,7 @@ def rule_id 'W48' end - def audit_impl(cfn_model) - violating_sqs_queues = cfn_model.resources_by_type('AWS::SQS::Queue').select do |sqs_queue| - sqs_queue.kmsMasterKeyId.nil? - end - - violating_sqs_queues.map(&:logical_resource_id) + def boolean_property + :kmsMasterKeyId end end diff --git a/lib/cfn-nag/custom_rules/boolean_base_rule.rb b/lib/cfn-nag/custom_rules/boolean_base_rule.rb index 10ec56bd..f2f7d84d 100644 --- a/lib/cfn-nag/custom_rules/boolean_base_rule.rb +++ b/lib/cfn-nag/custom_rules/boolean_base_rule.rb @@ -4,6 +4,11 @@ require_relative 'base' require 'cfn-nag/util/truthy.rb' +## +# Derive from this rule to ensure that a resource +# always has a given property declared, and if it does, it's not set to false +# this does double duty for existence and being boolean/not false... strictly speaking +# it could be broken out but it does work this way class BooleanBaseRule < BaseRule def resource_type raise 'must implement in subclass' @@ -17,7 +22,8 @@ def audit_impl(cfn_model) resources = cfn_model.resources_by_type(resource_type) violating_resources = resources.select do |resource| - not_truthy?(resource.send(boolean_property)) + boolean_property_value = resource.send(boolean_property) + not_truthy?(boolean_property_value) || boolean_property_value == { 'Ref' => 'AWS::NoValue' } end violating_resources.map(&:logical_resource_id) diff --git a/scripts/publish.sh b/scripts/publish.sh index 8d0d9e7d..c5899c71 100755 --- a/scripts/publish.sh +++ b/scripts/publish.sh @@ -51,15 +51,8 @@ sed -i.bak "s/0\.0\.0/${new_version}/g" cfn-nag.gemspec gem build cfn-nag.gemspec gem push cfn-nag-${new_version}.gem -latest_version=0.0.0 -until [ ${latest_version} == ${new_version} ] -do - latest_version=$(curl -s https://rubygems.org/api/v1/versions/cfn-nag/latest.json | ruby -e 'require "json"' -e 'puts JSON.parse(STDIN.read)["version"]') - sleep 90 -done - # publish docker image to DockerHub, https://hub.docker.com/r/stelligent/cfn_nag -docker build -t $docker_org/cfn_nag:${new_version} . --build-arg version=${new_version} +docker build -t $docker_org/cfn_nag:${new_version} . --build-arg version=${new_version} -f Dockerfile.local set +x echo $docker_password | docker login -u $docker_user --password-stdin set -x diff --git a/spec/custom_rules/SecretsManagerSecretKmsKeyIdRule_spec.rb b/spec/custom_rules/SecretsManagerSecretKmsKeyIdRule_spec.rb new file mode 100644 index 00000000..696a514b --- /dev/null +++ b/spec/custom_rules/SecretsManagerSecretKmsKeyIdRule_spec.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'cfn-model' +require 'cfn-nag/custom_rules/SecretsManagerSecretKmsKeyIdRule' + +describe SecretsManagerSecretKmsKeyIdRule do + context 'missing kms key id' do + it 'returns offending logical resource id' do + cfn_model = CfnParser.new.parse read_test_template('yaml/secretsmanager/conditional_properties.yml') + + actual_logical_resource_ids = SecretsManagerSecretKmsKeyIdRule.new.audit_impl cfn_model + expected_logical_resource_ids = %w[AppDbSecret] + + expect(actual_logical_resource_ids).to eq expected_logical_resource_ids + end + end + + context 'has explicit kms key id' do + it 'returns no logical resource ids' do + cfn_model = CfnParser.new.parse read_test_template('yaml/secretsmanager/explicit_key.yml') + + expect(SecretsManagerSecretKmsKeyIdRule.new.audit(cfn_model)).to be nil + end + end +end diff --git a/spec/test_templates/json/redshift/cluster_with_kms.json b/spec/test_templates/json/redshift/cluster_with_kms.json new file mode 100644 index 00000000..62e70756 --- /dev/null +++ b/spec/test_templates/json/redshift/cluster_with_kms.json @@ -0,0 +1,21 @@ +{ + "Resources": { + "myCluster": { + "Type": "AWS::Redshift::Cluster", + "Properties": { + "DBName": "mydb", + "MasterUsername": "master", + "NodeType": "ds2.xlarge", + "ClusterType": "single-node", + "Encrypted": "true", + "KmsKeyId": "arn:aws:kms:us-east-1:111122223333:key/1234abcd-12ab-34cd-56ef-1234567890ab", + "Tags": [ + { + "Key": "foo", + "Value": "bar" + } + ] + } + } + } +} diff --git a/spec/test_templates/yaml/secretsmanager/explicit_key.yml b/spec/test_templates/yaml/secretsmanager/explicit_key.yml new file mode 100644 index 00000000..45b878b4 --- /dev/null +++ b/spec/test_templates/yaml/secretsmanager/explicit_key.yml @@ -0,0 +1,47 @@ +AWSTemplateFormatVersion: 2010-09-09 +Parameters: + RestoreSecretString: + Type: String + NoEcho: true + Default: none + +Resources: + Key: + 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: '*' + + AppDbSecret: + Type: AWS::SecretsManager::Secret + DeletionPolicy: Retain + Properties: + Description: 'Restore' + SecretString: !Ref 'RestoreSecretString' + KmsKeyId: !Ref Key \ No newline at end of file