#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

@@ -6,9 +6,7 @@ LABEL org.opencontainers.image.authors="[email protected]"
 # 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"]

Dockerfile.local

@@ -0,0 +1,12 @@
+FROM ruby:2.5-alpine3.9@sha256:f33782620b363575ad95d19d0f0f07f7d197e9ccfee51f20df39dd33d408cdb4
+
+LABEL org.opencontainers.image.authors="[email protected]"
+
+ARG version
+
+ADD cfn-nag-${version}.gem
+
+RUN gem install cfn-nag-${version}.gem
+
+ENTRYPOINT ["cfn_nag"]
+CMD ["--help"]

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

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

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

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

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)

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

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

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"
+          }
+        ]
+      }
+    }
+  }
+}

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