Merged PR 539: ACL rules don't apply to ARP

## Summary Updates to `router.py` to allow ARP layer-2 traffic to not be held up by ACL rules, as they now only handle layer-3 traffic. An explicit ACL rule to permit ARP traffic is no longer needed. Documentation updates to reflect change. ## Test process All tests still pass. ## Checklist - [X] PR is linked to a **work item** - [X] **acceptance criteria** of linked ticket are met - [X] performed **self-review** of the code - [X] written **tests** for any new functionality added with this PR - [X] updated the **documentation** if this PR changes or adds functionality - [X] written/updated **design docs** if this PR implements new functionality - [X] updated the **change log** - [X] ran **pre-commit** checks for code style - [X] attended to any **TO-DOs** left in the code Related work items: #2775
2024-09-12 08:17:45 +00:00
parent 1cdf9edffb f908f9b23e
commit 31a820f021
14 changed files with 45 additions and 40 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased]
 ### Added
 -   Log observation space data by episode and step.
+-   ACL's are no longer applied to layer-2 traffic.

 ## [3.3.0] - 2024-09-04
 ### Added
--- a/docs/source/configuration/simulation/nodes/router.rst
+++ b/docs/source/configuration/simulation/nodes/router.rst
@@ -74,7 +74,7 @@ The subnet mask setting for the port.
 ``acl``
 -------

-Sets up the ACL rules for the router.
+Sets up the ACL rules for the router to apply to layer-3 traffic. These are not applied to layer-2 traffic such as ARP.

 e.g.

@@ -85,10 +85,6 @@ e.g.
        ...
        acl:
            1:
-                action: PERMIT
-                src_port: ARP
-                dst_port: ARP
-            2:
                action: PERMIT
                protocol: ICMP

--- a/docs/source/simulation_components/network/network.rst
+++ b/docs/source/simulation_components/network/network.rst
@@ -97,17 +97,10 @@ we'll use the following Network that has a client, server, two switches, and a r
    network.connect(endpoint_a=switch_2.network_interface[1], endpoint_b=client_1.network_interface[1])
    network.connect(endpoint_a=switch_1.network_interface[1], endpoint_b=server_1.network_interface[1])

-8. Add ACL rules on the Router to allow ARP and ICMP traffic.
+8. Add an ACL rule on the Router to allow ICMP traffic.

 .. code-block:: python

-    router_1.acl.add_rule(
-        action=ACLAction.PERMIT,
-        src_port=Port.ARP,
-        dst_port=Port.ARP,
-        position=22
-    )
-
    router_1.acl.add_rule(
        action=ACLAction.PERMIT,
        protocol=IPProtocol.ICMP,
--- a/docs/source/simulation_components/network/nodes/wireless_router.rst
+++ b/docs/source/simulation_components/network/nodes/wireless_router.rst
@@ -102,7 +102,6 @@ ICMP traffic, ensuring basic network connectivity and ping functionality.
    network.connect(pc_a.network_interface[1], router_1.router_interface)

    # Configure Router 1 ACLs
-    router_1.acl.add_rule(action=ACLAction.PERMIT, src_port=Port.ARP, dst_port=Port.ARP, position=22)
    router_1.acl.add_rule(action=ACLAction.PERMIT, protocol=IPProtocol.ICMP, position=23)

    # Configure PC B
--- a/src/primaite/simulator/network/creation.py
+++ b/src/primaite/simulator/network/creation.py
@@ -7,7 +7,6 @@ from primaite.simulator.network.hardware.nodes.host.computer import Computer
 from primaite.simulator.network.hardware.nodes.network.router import ACLAction, Router
 from primaite.simulator.network.hardware.nodes.network.switch import Switch
 from primaite.simulator.network.transmission.network_layer import IPProtocol
-from primaite.simulator.network.transmission.transport_layer import Port


 def num_of_switches_required(num_nodes: int, max_network_interface: int = 24) -> int:
@@ -98,7 +97,6 @@ def create_office_lan(
        default_gateway = IPv4Address(f"192.168.{subnet_base}.1")
        router = Router(hostname=f"router_{lan_name}", start_up_duration=0)
        router.power_on()
-        router.acl.add_rule(action=ACLAction.PERMIT, src_port=Port.ARP, dst_port=Port.ARP, position=22)
        router.acl.add_rule(action=ACLAction.PERMIT, protocol=IPProtocol.ICMP, position=23)
        network.add_node(router)
        router.configure_port(port=1, ip_address=default_gateway, subnet_mask="255.255.255.0")
--- a/src/primaite/simulator/network/hardware/nodes/network/router.py
+++ b/src/primaite/simulator/network/hardware/nodes/network/router.py
@@ -467,6 +467,7 @@ class AccessControlList(SimComponent):
        """Check if a packet with the given properties is permitted through the ACL."""
        permitted = False
        rule: ACLRule = None
+
        for _rule in self._acl:
            if not _rule:
                continue
@@ -1257,7 +1258,6 @@ class Router(NetworkNode):
        Initializes the router's ACL (Access Control List) with default rules, permitting essential protocols like ARP
        and ICMP, which are necessary for basic network operations and diagnostics.
        """
-        self.acl.add_rule(action=ACLAction.PERMIT, src_port=Port.ARP, dst_port=Port.ARP, position=22)
        self.acl.add_rule(action=ACLAction.PERMIT, protocol=IPProtocol.ICMP, position=23)

    def setup_for_episode(self, episode: int):
@@ -1369,6 +1369,12 @@ class Router(NetworkNode):

        return False

+    def subject_to_acl(self, frame: Frame) -> bool:
+        """Check that frame is subject to ACL rules."""
+        if frame.ip.protocol == IPProtocol.UDP and frame.is_arp:
+            return False
+        return True
+
    def receive_frame(self, frame: Frame, from_network_interface: RouterInterface):
        """
        Processes an incoming frame received on one of the router's interfaces.
@@ -1382,8 +1388,12 @@ class Router(NetworkNode):
        if self.operating_state != NodeOperatingState.ON:
            return

-        # Check if it's permitted
-        permitted, rule = self.acl.is_permitted(frame)
+        if self.subject_to_acl(frame=frame):
+            # Check if it's permitted
+            permitted, rule = self.acl.is_permitted(frame)
+        else:
+            permitted = True
+            rule = None

        if not permitted:
            at_port = self._get_port_of_nic(from_network_interface)
--- a/src/primaite/simulator/network/networks.py
+++ b/src/primaite/simulator/network/networks.py
@@ -79,8 +79,6 @@ def client_server_routed() -> Network:
    server_1.power_on()
    network.connect(endpoint_b=server_1.network_interface[1], endpoint_a=switch_1.network_interface[1])

-    router_1.acl.add_rule(action=ACLAction.PERMIT, src_port=Port.ARP, dst_port=Port.ARP, position=22)
-
    router_1.acl.add_rule(action=ACLAction.PERMIT, protocol=IPProtocol.ICMP, position=23)

    return network
@@ -271,8 +269,6 @@ def arcd_uc2_network() -> Network:
    security_suite.connect_nic(NIC(ip_address="192.168.10.110", subnet_mask="255.255.255.0"))
    network.connect(endpoint_b=security_suite.network_interface[2], endpoint_a=switch_2.network_interface[7])

-    router_1.acl.add_rule(action=ACLAction.PERMIT, src_port=Port.ARP, dst_port=Port.ARP, position=22)
-
    router_1.acl.add_rule(action=ACLAction.PERMIT, protocol=IPProtocol.ICMP, position=23)

    # Allow PostgreSQL requests
--- a/src/primaite/simulator/network/transmission/data_link_layer.py
+++ b/src/primaite/simulator/network/transmission/data_link_layer.py
@@ -161,11 +161,11 @@ class Frame(BaseModel):
        """
        Checks if the Frame is an ARP (Address Resolution Protocol) packet.

-        This is determined by checking if the destination port of the TCP header is equal to the ARP port.
+        This is determined by checking if the destination and source port of the UDP header is equal to the ARP port.

        :return: True if the Frame is an ARP packet, otherwise False.
        """
-        return self.udp.dst_port == Port.ARP
+        return self.udp.dst_port == Port.ARP and self.udp.src_port == Port.ARP

    @property
    def is_icmp(self) -> bool:
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -350,7 +350,6 @@ def install_stuff_to_sim(sim: Simulation):
    network.connect(endpoint_a=server_2.network_interface[1], endpoint_b=switch_2.network_interface[2])

    # 2: Configure base ACL
-    router.acl.add_rule(action=ACLAction.PERMIT, src_port=Port.ARP, dst_port=Port.ARP, position=22)
    router.acl.add_rule(action=ACLAction.PERMIT, protocol=IPProtocol.ICMP, position=23)
    router.acl.add_rule(action=ACLAction.PERMIT, src_port=Port.DNS, dst_port=Port.DNS, position=1)
    router.acl.add_rule(action=ACLAction.PERMIT, src_port=Port.HTTP, dst_port=Port.HTTP, position=3)
@@ -382,8 +381,6 @@ def install_stuff_to_sim(sim: Simulation):
            assert acl_rule.src_port == acl_rule.dst_port == Port.DNS
        elif i == 3:
            assert acl_rule.src_port == acl_rule.dst_port == Port.HTTP
-        elif i == 22:
-            assert acl_rule.src_port == acl_rule.dst_port == Port.ARP
        elif i == 23:
            assert acl_rule.protocol == IPProtocol.ICMP
        elif i == 24:
--- a/tests/integration_tests/game_layer/test_actions.py
+++ b/tests/integration_tests/game_layer/test_actions.py
@@ -106,7 +106,7 @@ def test_router_acl_addrule_integration(game_and_agent: Tuple[PrimaiteGame, Prox
    """
    Test that the RouterACLAddRuleAction can form a request and that it is accepted by the simulation.

-    The ACL starts off with 4 rules, and we add a rule, and check that the ACL now has 5 rules.
+    The ACL starts off with 3 rules, and we add a rule, and check that the ACL now has 4 rules.
    """
    game, agent = game_and_agent

@@ -115,7 +115,7 @@ def test_router_acl_addrule_integration(game_and_agent: Tuple[PrimaiteGame, Prox
    server_1 = game.simulation.network.get_node_by_hostname("server_1")
    server_2 = game.simulation.network.get_node_by_hostname("server_2")
    router = game.simulation.network.get_node_by_hostname("router")
-    assert router.acl.num_rules == 4
+    assert router.acl.num_rules == 3
    assert client_1.ping("10.0.2.3")  # client_1 can ping server_2
    assert server_2.ping("10.0.1.2")  # server_2 can ping client_1

@@ -138,8 +138,8 @@ def test_router_acl_addrule_integration(game_and_agent: Tuple[PrimaiteGame, Prox
    agent.store_action(action)
    game.step()

-    # 3: Check that the ACL now has 5 rules, and that client 1 cannot ping server 2
-    assert router.acl.num_rules == 5
+    # 3: Check that the ACL now has 4 rules, and that client 1 cannot ping server 2
+    assert router.acl.num_rules == 4
    assert not client_1.ping("10.0.2.3")  # Cannot ping server_2
    assert client_1.ping("10.0.2.2")  # Can ping server_1
    assert not server_2.ping(
@@ -165,8 +165,8 @@ def test_router_acl_addrule_integration(game_and_agent: Tuple[PrimaiteGame, Prox
    agent.store_action(action)
    game.step()

-    # 5: Check that the ACL now has 6 rules, but that server_1 can still ping server_2
-    assert router.acl.num_rules == 6
+    # 5: Check that the ACL now has 5 rules, but that server_1 can still ping server_2
+    assert router.acl.num_rules == 5
    assert server_1.ping("10.0.2.3")  # Can ping server_2


@@ -195,8 +195,8 @@ def test_router_acl_removerule_integration(game_and_agent: Tuple[PrimaiteGame, P
    agent.store_action(action)
    game.step()

-    # 3: Check that the ACL now has 3 rules, and that client 1 cannot access example.com
-    assert router.acl.num_rules == 3
+    # 3: Check that the ACL now has 2 rules, and that client 1 cannot access example.com
+    assert router.acl.num_rules == 2
    assert not browser.get_webpage()
    client_1.software_manager.software.get("DNSClient").dns_cache.clear()
    assert client_1.ping("10.0.2.2")  # pinging still works because ICMP is allowed
--- a/tests/integration_tests/network/test_routing.py
+++ b/tests/integration_tests/network/test_routing.py
@@ -73,7 +73,6 @@ def multi_hop_network() -> Network:
    router_1.enable_port(2)

    # Configure Router 1 ACLs
-    router_1.acl.add_rule(action=ACLAction.PERMIT, src_port=Port.ARP, dst_port=Port.ARP, position=22)
    router_1.acl.add_rule(action=ACLAction.PERMIT, protocol=IPProtocol.ICMP, position=23)

    # Configure PC B
--- a/tests/integration_tests/network/test_wireless_router.py
+++ b/tests/integration_tests/network/test_wireless_router.py
@@ -37,7 +37,6 @@ def wireless_wan_network():
    network.connect(pc_a.network_interface[1], router_1.network_interface[2])

    # Configure Router 1 ACLs
-    router_1.acl.add_rule(action=ACLAction.PERMIT, src_port=Port.ARP, dst_port=Port.ARP, position=22)
    router_1.acl.add_rule(action=ACLAction.PERMIT, protocol=IPProtocol.ICMP, position=23)

    # Configure PC B
--- a/tests/integration_tests/system/test_arp.py
+++ b/tests/integration_tests/system/test_arp.py
@@ -1,5 +1,7 @@
 # © Crown-owned copyright 2024, Defence Science and Technology Laboratory UK
-from primaite.simulator.network.hardware.nodes.network.router import RouterARP
+from primaite.simulator.network.hardware.nodes.network.router import ACLAction, Router, RouterARP
+from primaite.simulator.network.transmission.network_layer import IPProtocol
+from primaite.simulator.network.transmission.transport_layer import Port
 from primaite.simulator.system.services.arp.arp import ARP
 from tests.integration_tests.network.test_routing import multi_hop_network

@@ -48,3 +50,19 @@ def test_arp_fails_for_network_address_between_routers(multi_hop_network):
    actual_result = router_1_arp.get_arp_cache_mac_address(router_1.network_interface[1].ip_network.network_address)

    assert actual_result == expected_result
+
+
+def test_arp_not_affected_by_acl(multi_hop_network):
+    pc_a = multi_hop_network.get_node_by_hostname("pc_a")
+    router_1: Router = multi_hop_network.get_node_by_hostname("router_1")
+
+    # Add explicit rule to block ARP traffic. This shouldn't actually stop ARP traffic
+    # as it operates a different layer within the network.
+    router_1.acl.add_rule(action=ACLAction.DENY, src_port=Port.ARP, dst_port=Port.ARP, position=23)
+
+    pc_a_arp: ARP = pc_a.software_manager.arp
+
+    expected_result = router_1.network_interface[2].mac_address
+    actual_result = pc_a_arp.get_arp_cache_mac_address(router_1.network_interface[2].ip_address)
+
+    assert actual_result == expected_result
--- a/tests/unit_tests/_primaite/_simulator/_system/_services/test_terminal.py
+++ b/tests/unit_tests/_primaite/_simulator/_system/_services/test_terminal.py
@@ -77,7 +77,6 @@ def wireless_wan_network():
    network.connect(pc_a.network_interface[1], router_1.network_interface[2])

    # Configure Router 1 ACLs
-    router_1.acl.add_rule(action=ACLAction.PERMIT, src_port=Port.ARP, dst_port=Port.ARP, position=22)
    router_1.acl.add_rule(action=ACLAction.PERMIT, protocol=IPProtocol.ICMP, position=23)

    # add ACL rule to allow SSH traffic