From ce4e6dcc89edabedc707360154835dabdd0f83f3 Mon Sep 17 00:00:00 2001
From: Kevin DeJong <kddejong@amazon.com>
Date: Tue, 9 Jul 2024 07:53:41 -0700
Subject: [PATCH] Allow ICMP to have fromport other than -1 with to port -1

---
 .../all_to_and_from_ports.json                | 143 ++++++++++++------
 .../ec2/test_sg_all_to_and_from_ports.py      |  35 ++++-
 2 files changed, 124 insertions(+), 54 deletions(-)

diff --git a/src/cfnlint/data/schemas/extensions/aws_ec2_securitygroup/all_to_and_from_ports.json b/src/cfnlint/data/schemas/extensions/aws_ec2_securitygroup/all_to_and_from_ports.json
index fd589e80a2..dcfd0da178 100644
--- a/src/cfnlint/data/schemas/extensions/aws_ec2_securitygroup/all_to_and_from_ports.json
+++ b/src/cfnlint/data/schemas/extensions/aws_ec2_securitygroup/all_to_and_from_ports.json
@@ -1,60 +1,103 @@
 {
- "allOf": [
-  {
-   "if": {
-    "properties": {
-     "ToPort": {
-      "enum": [
-       -1,
-       "-1"
-      ]
-     }
+ "else": {
+  "allOf": [
+   {
+    "if": {
+     "properties": {
+      "ToPort": {
+       "enum": [
+        -1,
+        "-1"
+       ]
+      }
+     },
+     "required": [
+      "ToPort"
+     ]
     },
-    "required": [
-     "ToPort"
-    ]
+    "then": {
+     "properties": {
+      "FromPort": {
+       "enum": [
+        -1,
+        "-1"
+       ]
+      }
+     },
+     "required": [
+      "FromPort"
+     ]
+    }
    },
-   "then": {
-    "properties": {
-     "FromPort": {
-      "enum": [
-       -1,
-       "-1"
-      ]
-     }
+   {
+    "if": {
+     "properties": {
+      "FromPort": {
+       "enum": [
+        -1,
+        "-1"
+       ]
+      }
+     },
+     "required": [
+      "FromPort"
+     ]
     },
-    "required": [
-     "FromPort"
+    "then": {
+     "properties": {
+      "ToPort": {
+       "enum": [
+        -1,
+        "-1"
+       ]
+      }
+     },
+     "required": [
+      "ToPort"
+     ]
+    }
+   }
+  ]
+ },
+ "if": {
+  "properties": {
+   "IpProtocol": {
+    "enum": [
+     "1",
+     "icmp"
     ]
    }
   },
-  {
-   "if": {
-    "properties": {
-     "FromPort": {
-      "enum": [
-       -1,
-       "-1"
-      ]
-     }
-    },
-    "required": [
-     "FromPort"
-    ]
+  "required": [
+   "IpProtocol"
+  ]
+ },
+ "then": {
+  "if": {
+   "properties": {
+    "FromPort": {
+     "enum": [
+      -1,
+      "-1"
+     ]
+    }
    },
-   "then": {
-    "properties": {
-     "ToPort": {
-      "enum": [
-       -1,
-       "-1"
-      ]
-     }
-    },
-    "required": [
-     "ToPort"
-    ]
-   }
+   "required": [
+    "FromPort"
+   ]
+  },
+  "then": {
+   "properties": {
+    "ToPort": {
+     "enum": [
+      -1,
+      "-1"
+     ]
+    }
+   },
+   "required": [
+    "ToPort"
+   ]
   }
- ]
+ }
 }
diff --git a/test/unit/rules/resources/ec2/test_sg_all_to_and_from_ports.py b/test/unit/rules/resources/ec2/test_sg_all_to_and_from_ports.py
index 792d4021b6..a82f8acf59 100644
--- a/test/unit/rules/resources/ec2/test_sg_all_to_and_from_ports.py
+++ b/test/unit/rules/resources/ec2/test_sg_all_to_and_from_ports.py
@@ -40,6 +40,14 @@ def rule():
             },
             [],
         ),
+        (
+            {
+                "IpProtocol": 1,
+                "ToPort": -1,
+                "FromPort": 8,
+            },
+            [],
+        ),
         (
             {
                 "ToPort": -1,
@@ -56,7 +64,7 @@ def rule():
                         "properties": {"FromPort": {"enum": [-1, "-1"]}},
                         "required": ["FromPort"],
                     },
-                    schema_path=deque(["allOf", 0, "then", "required"]),
+                    schema_path=deque(["else", "allOf", 0, "then", "required"]),
                 )
             ],
         ),
@@ -76,7 +84,7 @@ def rule():
                         "properties": {"ToPort": {"enum": [-1, "-1"]}},
                         "required": ["ToPort"],
                     },
-                    schema_path=deque(["allOf", 1, "then", "required"]),
+                    schema_path=deque(["else", "allOf", 1, "then", "required"]),
                 )
             ],
         ),
@@ -95,7 +103,7 @@ def rule():
                     instance=5,
                     schema={"enum": [-1, "-1"]},
                     schema_path=deque(
-                        ["allOf", 0, "then", "properties", "FromPort", "enum"]
+                        ["else", "allOf", 0, "then", "properties", "FromPort", "enum"]
                     ),
                 )
             ],
@@ -115,11 +123,30 @@ def rule():
                     instance=5,
                     schema={"enum": [-1, "-1"]},
                     schema_path=deque(
-                        ["allOf", 1, "then", "properties", "ToPort", "enum"]
+                        ["else", "allOf", 1, "then", "properties", "ToPort", "enum"]
                     ),
                 )
             ],
         ),
+        (
+            {
+                "IpProtocol": "icmp",
+                "ToPort": 8,
+                "FromPort": -1,
+            },
+            [
+                ValidationError(
+                    ("Both ['FromPort', 'ToPort'] must " "be -1 when one is -1"),
+                    rule=SecurityGroupAllToAndFromPorts(),
+                    path=deque(["ToPort"]),
+                    validator="enum",
+                    validator_value=[-1, "-1"],
+                    instance=5,
+                    schema={"enum": [-1, "-1"]},
+                    schema_path=deque(["then", "then", "properties", "ToPort", "enum"]),
+                )
+            ],
+        ),
     ],
 )
 def test_backup_lifecycle(instance, expected, rule, validator):