Merge pull request #141 from Skyscanner/ip-objs-extra-string-field-in…

…-extras

Modify ingress filter field to split into `ingress_obj` and `ingress_ip`

CHANGELOG.md

@@ -1,6 +1,12 @@
 # Changelog
 All notable changes to this project will be documented in this file.
 
+## [0.23.0] - 2021-01-19
+### Breaking changes
+- Rule config files using filters must now use `ingress_obj` and not `ingress`.
+### Additions
+- Rules using IP Address Ranges now export both `ingress_obj` and `ingress_ip` filter fields.
+
 ## [0.22.0] - 2020-12-11
 ### Breaking changes
 - Classes inheriting from `ResourceSpecificRule` now must allow an `extra` field in the `resource_invoke` function

cfripper/__version__.py

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

cfripper/rules/ec2_security_group.py

@@ -40,17 +40,15 @@ def analyse_ingress(
             non_allowed_open_ports = sorted(set(open_ports) - set(self._config.allowed_world_open_ports))
 
             if non_allowed_open_ports:
-                filters_available_context["ingress"] = ingress
+                ip_range = ingress.CidrIp or ingress.CidrIpv6
+                filters_available_context["ingress_obj"] = ingress
+                filters_available_context["ingress_ip"] = str(ip_range)
                 filters_available_context["open_ports"] = open_ports
                 filters_available_context["non_allowed_open_ports"] = non_allowed_open_ports
 
                 self.add_failure_to_result(
                     result,
-                    self.REASON.format(
-                        self.get_open_ports_wording(non_allowed_open_ports),
-                        (ingress.CidrIp or ingress.CidrIpv6),
-                        logical_id,
-                    ),
+                    self.REASON.format(self.get_open_ports_wording(non_allowed_open_ports), ip_range, logical_id,),
                     resource_ids={logical_id},
                     context=filters_available_context,
                 )
@@ -141,7 +139,8 @@ class EC2SecurityGroupOpenToWorldRule(SecurityGroupOpenToWorldRule):
         |`extras`                 | str                        | `extras` variable available inside the rule                    |
         |`logical_id`             | str                        | ID used in Cloudformation to refer the resource being analysed |
         |`resource`               | `SecurityGroup`            | Resource that is being addressed                               |
-        |`ingress`                | `SecurityGroupIngressProp` | SecurityGroupIngressProp being checked found in the Resource   |
+        |`ingress_ip`             | str                        | IP Address range (IpV4 or IpV6) of the ingress object          |
+        |`ingress_obj`            | `SecurityGroupIngressProp` | SecurityGroupIngressProp being checked found in the Resource   |
         |`open_ports`             | `List[int]`                | List of all open ports defined                                 |
         |`non_allowed_open_ports` | `List[int]`                | List of all non allowed open ports defined                     |
     """
@@ -234,7 +233,8 @@ class EC2SecurityGroupIngressOpenToWorldRule(SecurityGroupOpenToWorldRule):
         |`extras`                 | str                              | `extras` variable available inside the rule                    |
         |`logical_id`             | str                              | ID used in Cloudformation to refer the resource being analysed |
         |`resource`               | `SecurityGroupIngress`           | Resource that is being addressed                               |
-        |`ingress`                | `SecurityGroupIngressProperties` | SecurityGroupIngress being checked found in the Resource       |
+        |`ingress_ip`             | str                              | IP Address range (IpV4 or IpV6) of the ingress object          |
+        |`ingress_obj`            | `SecurityGroupIngressProperties` | SecurityGroupIngress being checked found in the Resource       |
         |`open_ports`             | `List[int]`                      | List of all open ports defined                                 |
         |`non_allowed_open_ports` | `List[int]`                      | List of all non allowed open ports defined                     |
     """

tests/rules/test_EC2SecurityGroupIngressOpenToWorld.py

@@ -1,4 +1,8 @@
-from pytest import fixture
+from ipaddress import IPv4Network, IPv6Network
+from unittest import mock
+
+from pycfmodel.model.resources.security_group_ingress import SecurityGroupIngressProperties
+from pytest import fixture, mark
 
 from cfripper.config.config import Config
 from cfripper.config.filter import Filter
@@ -44,24 +48,26 @@ def test_valid_security_group_ingress(good_template):
     assert result.valid
 
 
-def test_filter_do_not_report_anything(bad_template):
+@mark.parametrize(
+    "filter_eval_object",
+    [
+        {"and": [{"eq": [{"ref": "config.stack_name"}, "mockstack"]}, {"eq": [{"ref": "ingress_obj.FromPort"}, 46]}]},
+        {
+            "and": [
+                {"eq": [{"ref": "config.stack_name"}, "mockstack"]},
+                {"in": [{"ref": "ingress_ip"}, ["11.0.0.0/8", "::/0"]]},
+            ]
+        },
+    ],
+)
+def test_filter_do_not_report_anything(filter_eval_object, bad_template):
     mock_config = Config(
         rules=["EC2SecurityGroupIngressOpenToWorldRule"],
         aws_account_id="123456789",
         stack_name="mockstack",
         rules_config={
             "EC2SecurityGroupIngressOpenToWorldRule": RuleConfig(
-                filters=[
-                    Filter(
-                        rule_mode=RuleMode.WHITELISTED,
-                        eval={
-                            "and": [
-                                {"eq": [{"ref": "config.stack_name"}, "mockstack"]},
-                                {"eq": [{"ref": "ingress.FromPort"}, 46]},
-                            ]
-                        },
-                    )
-                ],
+                filters=[Filter(rule_mode=RuleMode.WHITELISTED, eval=filter_eval_object)],
             )
         },
     )
@@ -72,6 +78,28 @@ def test_filter_do_not_report_anything(bad_template):
     assert result.valid
 
 
+@mock.patch("cfripper.rules.base_rules.Rule.add_failure_to_result")
+def test_filter_context_set_correctly(mock_failure_to_result, bad_template):
+    mock_failure_to_result.side_effect = [None, None]
+    mock_config = Config(
+        rules=["EC2SecurityGroupIngressOpenToWorldRule"],
+        aws_account_id="123456789",
+        stack_name="mockstack",
+        rules_config={},
+    )
+    rules = [DEFAULT_RULES.get(rule)(mock_config) for rule in mock_config.rules]
+    processor = RuleProcessor(*rules)
+    processor.process_cf_template(bad_template, mock_config)
+    assert mock_failure_to_result.mock_calls[0][2]["context"]["ingress_ip"] == "11.0.0.0/8"
+    assert mock_failure_to_result.mock_calls[1][2]["context"]["ingress_ip"] == "::/0"
+    assert mock_failure_to_result.mock_calls[0][2]["context"]["ingress_obj"] == SecurityGroupIngressProperties(
+        CidrIp=IPv4Network("11.0.0.0/8"), FromPort=46, IpProtocol="tcp", ToPort=46, GroupId="sg-12341234",
+    )
+    assert mock_failure_to_result.mock_calls[1][2]["context"]["ingress_obj"] == SecurityGroupIngressProperties(
+        CidrIpv6=IPv6Network("::/0"), FromPort=46, IpProtocol="tcp", ToPort=46, GroupId="sg-12341234",
+    )
+
+
 def test_non_matching_filters_are_reported_normally(bad_template):
     mock_config = Config(
         rules=["EC2SecurityGroupIngressOpenToWorldRule"],