AWS NGW: Tweak NGW filtering criteria (#6286)

* AWS NGW: change NGW filtering criteria * remove cruft
batfish · Oct 7, 2020 · 67ef9ef · 67ef9ef
1 parent 7d349c9
commit 67ef9ef
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 12 deletions.
diff --git a/projects/batfish/src/main/java/org/batfish/representation/aws/NatGateway.java b/projects/batfish/src/main/java/org/batfish/representation/aws/NatGateway.java
@@ -197,7 +197,7 @@ Configuration toConfigurationNode(
 
     // post transformation filter on the interface to the subnet
     IpAccessList postTransformationFilter =
-        computePostTransformationIllegalPacketFilter(getPrivateIp(), subnet.getCidrBlock());
+        computePostTransformationIllegalPacketFilter(getPrivateIp());
     cfgNode.getIpAccessLists().put(postTransformationFilter.getName(), postTransformationFilter);
     ifaceToSubnet.setPostTransformationIncomingFilter(postTransformationFilter);
 
@@ -310,15 +310,16 @@ static Transformation computeOutgoingNatTransformation(Ip privateIp) {
   }
 
   @VisibleForTesting
-  static IpAccessList computePostTransformationIllegalPacketFilter(
-      Ip privateIp, Prefix subnetPrefix) {
+  static IpAccessList computePostTransformationIllegalPacketFilter(Ip privateIp) {
     return IpAccessList.builder()
         .setName(ILLEGAL_PACKET_FILTER_NAME)
         .setLines(
             ExprAclLine.rejecting(
-                TraceElement.of("Denied packets from sources within the subnet"),
+                TraceElement.of("Denied packets where source IP is the NAT gateway's private IP"),
                 new MatchHeaderSpace(
-                    HeaderSpace.builder().setSrcIps(subnetPrefix.toIpSpace()).build())),
+                    HeaderSpace.builder()
+                        .setSrcIps(ImmutableList.of(IpWildcard.create(privateIp)))
+                        .build())),
             ExprAclLine.rejecting(
                 TraceElement.of("Denied packets that did NOT match an active NAT session"),
                 new MatchHeaderSpace(

diff --git a/projects/batfish/src/test/java/org/batfish/representation/aws/NatGatewayTest.java b/projects/batfish/src/test/java/org/batfish/representation/aws/NatGatewayTest.java
@@ -233,19 +233,18 @@ public void testConnectToVpc() {
   @Test
   public void testComputePostTransformationIllegalPacketFilter() {
     Ip privateIp = Ip.parse("10.10.10.10");
-    IpAccessList filter =
-        computePostTransformationIllegalPacketFilter(privateIp, Prefix.parse("10.10.10.0/24"));
+    IpAccessList filter = computePostTransformationIllegalPacketFilter(privateIp);
 
-    // is in the NAT's subnet
+    // denied: it has the same ip has the nat
     assertThat(
         filter
             .filter(
                 Flow.builder()
                     .setIngressNode("a")
                     .setIpProtocol(IpProtocol.TCP)
-                    .setSrcIp(Ip.parse("10.10.10.11"))
+                    .setSrcIp(privateIp)
                     .setSrcPort(345)
-                    .setDstIp(privateIp)
+                    .setDstIp(Ip.parse("2.2.2.2"))
                     .setDstPort(80)
                     .build(),
                 "a",
@@ -254,13 +253,14 @@ public void testComputePostTransformationIllegalPacketFilter() {
             .getAction(),
         equalTo(LineAction.DENY));
 
-    // has the same ip has the nat
+    // denied: it wasn't transformed (as its dst IP is that of the NAT)
     assertThat(
         filter
             .filter(
                 Flow.builder()
                     .setIngressNode("a")
                     .setIpProtocol(IpProtocol.TCP)
+                    .setSrcIp(Ip.parse("1.1.1.11"))
                     .setSrcPort(345)
                     .setDstIp(privateIp)
                     .setDstPort(80)
@@ -271,7 +271,25 @@ public void testComputePostTransformationIllegalPacketFilter() {
             .getAction(),
         equalTo(LineAction.DENY));
 
-    // legal packet
+    // allowed: it is in the NAT's subnet
+    assertThat(
+        filter
+            .filter(
+                Flow.builder()
+                    .setIngressNode("a")
+                    .setIpProtocol(IpProtocol.TCP)
+                    .setSrcIp(Ip.parse("10.10.10.11"))
+                    .setSrcPort(345)
+                    .setDstIp(Ip.parse("2.2.2.2"))
+                    .setDstPort(80)
+                    .build(),
+                "a",
+                ImmutableMap.of(),
+                ImmutableMap.of())
+            .getAction(),
+        equalTo(LineAction.PERMIT));
+
+    // allowed: coming from outside
     assertThat(
         filter
             .filter(