Merge pull request #114 from Skyscanner/improve_docs_cidr_rules

Improve message and documentation for security group rules

CHANGELOG.md

@@ -1,6 +1,11 @@
 # Changelog
 All notable changes to this project will be documented in this file.
 
+## [0.17.2] - 2020-04-01
+### Improvements
+- Improved message for users when failing the `SecurityGroupOpenToWorldRule` and `SecurityGroupIngressOpenToWorldRule` rules.
+- Improved documentation for the above rules, including styling fixes which have now been tested.
+
 ## [0.17.1] - 2020-03-30
 ### Improvements
 - Add `exists` and `empty` functions to filters

cfripper/__version__.py

@@ -1,3 +1,3 @@
-VERSION = (0, 17, 1)
+VERSION = (0, 17, 2)
 
 __version__ = ".".join(map(str, VERSION))

cfripper/rules/security_group.py

@@ -14,13 +14,23 @@
 
 class SecurityGroupOpenToWorldRule(Rule):
     """
-    Checks if security groups have an ingress rule of /0 for ports other than 80 and 443.
+    Checks if security groups have an ingress IP that is open to the world for ports other than 80 and 443.
     All other ports should be closed off from public access to prevent a serious security misconfiguration.
 
     Fix:
-        Most security groups only need to be access privately, and this can typically be done by specifying
+        Most security groups only need to be accessed privately, and this can typically be done by specifying
         the CIDR of a Security Group's ingress to `10.0.0.0/8` or similar (https://en.wikipedia.org/wiki/Private_network).
 
+        Unless required, do not use the following IP ranges in your Security Group Ingress:
+
+          - `0.0.0.0/0`.
+
+          - Any `/8` that does not start with 10.
+
+          - `172/8` or `192/8` (use `172.16/12` and `192.168/16` ranges, per RFC1918 specification).
+
+        As per RFC4193, fd00::/8 IPv6 addresses should be used to define a private network.
+
     Code for fix:
         ````json
         {
@@ -54,7 +64,7 @@ class SecurityGroupOpenToWorldRule(Rule):
     """
 
     GRANULARITY = RuleGranularity.RESOURCE
-    REASON = "Port {} open to the world in security group '{}'"
+    REASON = "Port {} open to public IPs: ({}) in security group '{}'"
 
     def invoke(self, cfmodel: CFModel, extras: Optional[Dict] = None) -> Result:
         result = Result()
@@ -68,7 +78,9 @@ def invoke(self, cfmodel: CFModel, extras: Optional[Dict] = None) -> Result:
                         for port in range(ingress.FromPort, ingress.ToPort + 1):
                             if str(port) not in self._config.allowed_world_open_ports:
                                 self.add_failure_to_result(
-                                    result, self.REASON.format(port, logical_id), resource_ids={logical_id}
+                                    result,
+                                    self.REASON.format(port, (ingress.CidrIp or ingress.CidrIpv6), logical_id),
+                                    resource_ids={logical_id},
                                 )
         return result
 
@@ -85,27 +97,18 @@ def non_compliant_ip_range(self, item) -> bool:
 
 
 class SecurityGroupIngressOpenToWorldRule(SecurityGroupOpenToWorldRule):
-    """
-    Checks if a security group has a CIDR open to world on ingress.
-    This is a security risk as your resource may be publicly available.
-
-    Fix:
-        Unless required, do not use the following IP ranges in your Security Group Ingress:
-          - 0.0.0.0/0.
-          - Any `/8` that does not start with 10.
-          - 172/8 or 192/8 (use 172.16/12 and 192.168/16 ranges, per RFC1918 specification).
-
-        As per RFC4193, fd00::/8 IPv6 addresses should be used to define a private network.
-    """
-
     def invoke(self, cfmodel: CFModel, extras: Optional[Dict] = None) -> Result:
         result = Result()
         for logical_id, resource in cfmodel.Resources.items():
             if isinstance(resource, SecurityGroupIngress) and self.non_compliant_ip_range(item=resource):
                 for port in range(resource.Properties.FromPort, resource.Properties.ToPort + 1):
                     if str(port) not in self._config.allowed_world_open_ports:
                         self.add_failure_to_result(
-                            result, self.REASON.format(port, logical_id), resource_ids={logical_id}
+                            result,
+                            self.REASON.format(
+                                port, (resource.Properties.CidrIp or resource.Properties.CidrIpv6), logical_id
+                            ),
+                            resource_ids={logical_id},
                         )
         return result
 

tests/rules/test_SecurityGroupIngressOpenToWorld.py

@@ -23,9 +23,14 @@ def test_failures_are_raised(bad_template):
     assert len(result.failed_rules) == 2
     assert len(result.failed_monitored_rules) == 0
     assert result.failed_rules[0].rule == "SecurityGroupIngressOpenToWorldRule"
-    assert result.failed_rules[0].reason == "Port 46 open to the world in security group 'securityGroupIngress1'"
+    assert (
+        result.failed_rules[0].reason
+        == "Port 46 open to public IPs: (11.0.0.0/8) in security group 'securityGroupIngress1'"
+    )
     assert result.failed_rules[1].rule == "SecurityGroupIngressOpenToWorldRule"
-    assert result.failed_rules[1].reason == "Port 46 open to the world in security group 'securityGroupIngress2'"
+    assert (
+        result.failed_rules[1].reason == "Port 46 open to public IPs: (::/0) in security group 'securityGroupIngress2'"
+    )
 
 
 def test_valid_security_group_ingress(good_template):

tests/rules/test_SecurityGroupOpenToWorldRule.py

@@ -47,7 +47,7 @@ def test_security_group_type_slash0(security_group_type_slash0):
 
     assert not result.valid
     assert result.failed_rules[0].rule == "SecurityGroupOpenToWorldRule"
-    assert result.failed_rules[0].reason == "Port 22 open to the world in security group 'SecurityGroup'"
+    assert result.failed_rules[0].reason == "Port 22 open to public IPs: (0.0.0.0/0) in security group 'SecurityGroup'"
 
 
 def test_valid_security_group_not_slash0(valid_security_group_not_slash0):
@@ -83,7 +83,7 @@ def test_invalid_security_group_cidripv6(invalid_security_group_cidripv6):
 
     assert not result.valid
     assert result.failed_rules[0].rule == "SecurityGroupOpenToWorldRule"
-    assert result.failed_rules[0].reason == "Port 22 open to the world in security group 'SecurityGroup'"
+    assert result.failed_rules[0].reason == "Port 22 open to public IPs: (::/0) in security group 'SecurityGroup'"
 
 
 def test_invalid_security_group_range(invalid_security_group_range):
@@ -92,7 +92,7 @@ def test_invalid_security_group_range(invalid_security_group_range):
 
     assert not result.valid
     assert result.failed_rules[0].rule == "SecurityGroupOpenToWorldRule"
-    assert result.failed_rules[0].reason == "Port 0 open to the world in security group 'SecurityGroup'"
+    assert result.failed_rules[0].reason == "Port 0 open to public IPs: (11.0.0.0/8) in security group 'SecurityGroup'"
 
 
 def test_invalid_security_group_multiple_statements(invalid_security_group_multiple_statements):
@@ -101,4 +101,6 @@ def test_invalid_security_group_multiple_statements(invalid_security_group_multi
 
     assert not result.valid
     assert result.failed_rules[0].rule == "SecurityGroupOpenToWorldRule"
-    assert result.failed_rules[0].reason == "Port 9090 open to the world in security group 'SecurityGroup'"
+    assert (
+        result.failed_rules[0].reason == "Port 9090 open to public IPs: (172.0.0.0/8) in security group 'SecurityGroup'"
+    )