Fix harcoded password rds rule (#79)

* update rule

* Update changelong and requirements

* apply comments from pr

CHANGELOG.md

@@ -1,6 +1,12 @@
 # Changelog
 All notable changes to this project will be documented in this file.
 
+## [0.11.1] - 2019-11-25
+### Changed
+- `HardcodedRDSPasswordRule` now reports two different messages when there is a missing echo or a readable password.
+### Fixes
+- `HardcodedRDSPasswordRule` was wrongly adding an error when a value is provided.
+
 ## [0.11.0] - 2019-11-20
 ### Breaking changes
 - Moved some files from model to rules, renamed rules to match pythonic style. Moved tons of classes around

cfripper/rules/hardcoded_RDS_password.py

@@ -19,29 +19,22 @@
 
 class HardcodedRDSPasswordRule(Rule):
 
-    REASON = "Default RDS {} password parameter or missing NoEcho for {}."
+    REASON_DEFAULT = "Default RDS {} password parameter (readable in plain-text) for {}."
+    REASON_MISSING_NOECHO = "RDS {} password parameter missing NoEcho for {}."
 
     def invoke(self, cfmodel):
         password_protected_cluster_ids = []
         instances_to_check = []
 
         for logical_id, resource in cfmodel.Resources.items():
             # flag insecure RDS Clusters.
-            if (
-                resource.Type == "AWS::RDS::DBCluster"
-                and resource.Properties.get("MasterUserPassword", Parameter.NO_ECHO_NO_DEFAULT)
-                != Parameter.NO_ECHO_NO_DEFAULT
-            ):
-                self.add_failure(type(self).__name__, self.REASON.format("Cluster", logical_id))
-                continue
-
-            # keep track of secure RDS Clusters.
             if resource.Type == "AWS::RDS::DBCluster":
-                password_protected_cluster_ids.append(logical_id)
-                continue
+                failure_added = self._failure_added(logical_id, resource)
+                if not failure_added:
+                    password_protected_cluster_ids.append(logical_id)
 
             # keep track of RDS instances so they can be examined in the code below.
-            if resource.Type == "AWS::RDS::DBInstance":
+            elif resource.Type == "AWS::RDS::DBInstance":
                 instances_to_check.append((logical_id, resource))
 
         # check each instance with the context of clusters.
@@ -52,8 +45,16 @@ def invoke(self, cfmodel):
             ):
                 continue
 
-            if (
-                resource.Properties.get("MasterUserPassword", Parameter.NO_ECHO_NO_DEFAULT)
-                != Parameter.NO_ECHO_NO_DEFAULT
-            ):
-                self.add_failure(type(self).__name__, self.REASON.format("Instance", logical_id))
+            self._failure_added(logical_id, resource)
+
+    def _failure_added(self, logical_id, resource) -> bool:
+        master_user_password = resource.Properties.get("MasterUserPassword", Parameter.NO_ECHO_NO_DEFAULT)
+        resource_type = resource.Type.replace("AWS::RDS::DB", "")
+        if master_user_password == Parameter.NO_ECHO_WITH_DEFAULT:
+            self.add_failure(type(self).__name__, self.REASON_DEFAULT.format(resource_type, logical_id))
+            return True
+        elif master_user_password not in (Parameter.NO_ECHO_NO_DEFAULT, Parameter.NO_ECHO_WITH_VALUE):
+            self.add_failure(type(self).__name__, self.REASON_MISSING_NOECHO.format(resource_type, logical_id))
+            return True
+
+        return False

setup.py

@@ -3,7 +3,7 @@
 with open("README.md", "r") as fh:
     long_description = fh.read()
 
-install_requires = ["boto3>=1.4.7,<2", "PyYAML>=4.2b1", "pycfmodel>=0.5.0", "cfn_flip>=1.2.0"]
+install_requires = ["boto3>=1.4.7,<2", "PyYAML>=4.2b1", "pycfmodel>=0.5.1", "cfn_flip>=1.2.0"]
 
 dev_requires = [
     "black==19.10b0",
@@ -17,7 +17,7 @@
 
 setup(
     name="cfripper",
-    version="0.11.0",
+    version="0.11.1",
     author="Skyscanner Product Security",
     author_email="security@skyscanner.net",
     long_description=long_description,

tests/rules/test_HardcodedRDSPasswordRule.py

@@ -53,9 +53,11 @@ def test_failures_are_raised_for_instances(bad_template_instances):
     assert len(result.failed_rules) == 2
     assert len(result.failed_monitored_rules) == 0
     assert result.failed_rules[0].rule == "HardcodedRDSPasswordRule"
-    assert result.failed_rules[0].reason == "Default RDS Instance password parameter or missing NoEcho for BadDb3."
+    assert result.failed_rules[0].reason == "RDS Instance password parameter missing NoEcho for BadDb3."
     assert result.failed_rules[1].rule == "HardcodedRDSPasswordRule"
-    assert result.failed_rules[1].reason == "Default RDS Instance password parameter or missing NoEcho for BadDb5."
+    assert (
+        result.failed_rules[1].reason == "Default RDS Instance password parameter (readable in plain-text) for BadDb5."
+    )
 
 
 def test_failures_are_raised_for_clusters(bad_template_clusters):
@@ -67,7 +69,7 @@ def test_failures_are_raised_for_clusters(bad_template_clusters):
     assert len(result.failed_rules) == 1
     assert len(result.failed_monitored_rules) == 0
     assert result.failed_rules[0].rule == "HardcodedRDSPasswordRule"
-    assert result.failed_rules[0].reason == "Default RDS Cluster password parameter or missing NoEcho for BadCluster1."
+    assert result.failed_rules[0].reason == "RDS Cluster password parameter missing NoEcho for BadCluster1."
 
 
 def test_passed_cluster_pw_protected(good_template_clusters_and_instances):
@@ -87,7 +89,9 @@ def test_failures_are_raised_for_instances_without_protected_clusters(bad_templa
     assert len(result.failed_rules) == 1
     assert len(result.failed_monitored_rules) == 0
     assert result.failed_rules[0].rule == "HardcodedRDSPasswordRule"
-    assert result.failed_rules[0].reason == "Default RDS Instance password parameter or missing NoEcho for BadDb5."
+    assert (
+        result.failed_rules[0].reason == "Default RDS Instance password parameter (readable in plain-text) for BadDb5."
+    )
 
 
 def test_failures_are_raised_for_bad_instances_and_bad_clusters(bad_template_clusters_with_bad_instances):
@@ -99,6 +103,9 @@ def test_failures_are_raised_for_bad_instances_and_bad_clusters(bad_template_clu
     assert len(result.failed_rules) == 2
     assert len(result.failed_monitored_rules) == 0
     assert result.failed_rules[0].rule == "HardcodedRDSPasswordRule"
-    assert result.failed_rules[0].reason == "Default RDS Cluster password parameter or missing NoEcho for BadCluster99."
+    assert (
+        result.failed_rules[0].reason
+        == "Default RDS Cluster password parameter (readable in plain-text) for BadCluster99."
+    )
     assert result.failed_rules[1].rule == "HardcodedRDSPasswordRule"
-    assert result.failed_rules[1].reason == "Default RDS Instance password parameter or missing NoEcho for BadDb33."
+    assert result.failed_rules[1].reason == "RDS Instance password parameter missing NoEcho for BadDb33."