sonic: Restrict SSH to the OOB management network

osism/tasks/conductor/sonic/config_generator.py

@@ -431,7 +431,7 @@ def generate_sonic_config(device, hwsku, device_as_mapping=None, config_version=
         config["MGMT_INTERFACE"][f"eth0|{oob_ip}/{prefix_len}"] = {}
         metalbox_ip = _get_metalbox_ip_for_device(device)
         config["STATIC_ROUTE"]["mgmt|0.0.0.0/0"] = {"nexthop": metalbox_ip}
-        # Restrict control-plane services (SNMP, gNMI) to the OOB network
+        # Restrict control-plane services (SSH, SNMP, gNMI) to the OOB network
         _add_ctrlplane_acls(config, oob_ip, prefix_len)
     else:
         oob_ip = None
@@ -2369,15 +2369,15 @@ def _get_gnmi_port(config):
 
 
 def _add_ctrlplane_acls(config, oob_ip, prefix_len):
-    """Add control-plane ACLs restricting SNMP and gNMI to the OOB network.
+    """Add control-plane ACLs restricting SSH, SNMP and gNMI to the OOB network.
 
-    Emits ACL_TABLE entries of type CTRLPLANE bound to the SNMP and
+    Emits ACL_TABLE entries of type CTRLPLANE bound to the SSH, SNMP and
     EXTERNAL_CLIENT (gNMI/telemetry) caclmgrd services, plus one ACL_RULE
     per table accepting only the device's OOB management subnet (the
     network-normalised oob_ip/prefix_len). caclmgrd installs an implicit
     default-drop for every service bound in a CTRLPLANE table, so sources
-    outside the OOB subnet can no longer reach SNMP or gNMI. The SSH_ONLY
-    table (#2329) belongs here as well once implemented.
+    outside the OOB subnet -- e.g. front-panel interfaces with IPs in the
+    default VRF -- can no longer reach SSH, SNMP or gNMI.
 
     caclmgrd's EXTERNAL_CLIENT service has no built-in destination port
     (verified against sonic-host-services 202211 through master): the rule
@@ -2387,13 +2387,12 @@ def _add_ctrlplane_acls(config, oob_ip, prefix_len):
     ACL_TABLE and ACL_RULE are generator-owned (ON_DEMAND_OWNED_TABLE_KEYS),
     so they are rebuilt from scratch on every regen and stay absent when the
     device has no OOB IP. They are also multi-owner
-    (MULTI_OWNER_OWNED_TABLE_KEYS): the SSH_ONLY table (#2329) belongs here as
-    well once implemented, so this helper merges only its own SNMP_ONLY /
-    GNMI_ONLY keys per key rather than rebinding the table wholesale -- the
-    central owned-table drop in generate_sonic_config clears stale entries up
-    front, and per-key merge lets coexisting control-plane helpers compose. The
-    rules are IPv4 (SRC_IP); a non-IPv4 OOB IP logs a warning and emits nothing
-    rather than failing the whole config generation.
+    (MULTI_OWNER_OWNED_TABLE_KEYS): this helper merges only its own SSH_ONLY /
+    SNMP_ONLY / GNMI_ONLY keys per key rather than rebinding the table
+    wholesale -- the central owned-table drop in generate_sonic_config clears
+    stale entries up front, and per-key merge lets coexisting control-plane
+    helpers compose. The rules are IPv4 (SRC_IP); a non-IPv4 OOB IP logs a
+    warning and emits nothing rather than failing the whole config generation.
     """
     network = ipaddress.ip_network(f"{oob_ip}/{prefix_len}", strict=False)
     if network.version != 4:
@@ -2409,6 +2408,11 @@ def _add_ctrlplane_acls(config, oob_ip, prefix_len):
         "IP_TYPE": "IP",
     }
     config.setdefault("ACL_TABLE", {})
+    config["ACL_TABLE"]["SSH_ONLY"] = {
+        "policy_desc": "SSH_ONLY",
+        "type": "CTRLPLANE",
+        "services": ["SSH"],
+    }
     config["ACL_TABLE"]["SNMP_ONLY"] = {
         "policy_desc": "SNMP_ONLY",
         "type": "CTRLPLANE",
@@ -2420,6 +2424,7 @@ def _add_ctrlplane_acls(config, oob_ip, prefix_len):
         "services": ["EXTERNAL_CLIENT"],
     }
     config.setdefault("ACL_RULE", {})
+    config["ACL_RULE"]["SSH_ONLY|RULE_1"] = dict(accept_from_oob)
     config["ACL_RULE"]["SNMP_ONLY|RULE_1"] = dict(accept_from_oob)
     config["ACL_RULE"]["GNMI_ONLY|RULE_1"] = {
         **accept_from_oob,

tests/unit/tasks/conductor/sonic/test_config_generator_ctrlplane_acls.py

@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: Apache-2.0
 
-"""Unit tests for ``_add_ctrlplane_acls`` in ``config_generator`` (#2330)."""
+"""Unit tests for ``_add_ctrlplane_acls`` in ``config_generator`` (#2329, #2330)."""
 
 import pytest
 
@@ -11,12 +11,17 @@
 )
 
 
-def test_add_ctrlplane_acls_emits_snmp_and_gnmi_ctrlplane_tables():
+def test_add_ctrlplane_acls_emits_ssh_snmp_and_gnmi_ctrlplane_tables():
     config = {}
 
     _add_ctrlplane_acls(config, "10.42.0.5", 24)
 
     assert config["ACL_TABLE"] == {
+        "SSH_ONLY": {
+            "policy_desc": "SSH_ONLY",
+            "type": "CTRLPLANE",
+            "services": ["SSH"],
+        },
         "SNMP_ONLY": {
             "policy_desc": "SNMP_ONLY",
             "type": "CTRLPLANE",
@@ -36,6 +41,12 @@ def test_add_ctrlplane_acls_rules_accept_network_normalised_oob_subnet():
 
     _add_ctrlplane_acls(config, "10.42.0.5", 24)
 
+    assert config["ACL_RULE"]["SSH_ONLY|RULE_1"] == {
+        "PRIORITY": "9999",
+        "PACKET_ACTION": "ACCEPT",
+        "SRC_IP": "10.42.0.0/24",
+        "IP_TYPE": "IP",
+    }
     assert config["ACL_RULE"]["SNMP_ONLY|RULE_1"] == {
         "PRIORITY": "9999",
         "PACKET_ACTION": "ACCEPT",
@@ -51,16 +62,38 @@ def test_add_ctrlplane_acls_rules_accept_network_normalised_oob_subnet():
     }
 
 
-def test_add_ctrlplane_acls_gnmi_rule_requires_dst_port_snmp_does_not():
+@pytest.mark.parametrize(
+    "oob_ip,prefix_len,expected_src",
+    [
+        ("10.42.0.5", 24, "10.42.0.0/24"),  # host bits stripped
+        ("192.168.45.123", 26, "192.168.45.64/26"),  # non-octet boundary
+        ("10.42.0.0", 24, "10.42.0.0/24"),  # already the network address
+    ],
+)
+def test_add_ctrlplane_acls_normalises_src_ip_to_network_address(
+    oob_ip, prefix_len, expected_src
+):
+    """The OOB IP is a host address — every rule must carry its subnet."""
+    config = {}
+
+    _add_ctrlplane_acls(config, oob_ip, prefix_len)
+
+    for rule_key in ("SSH_ONLY|RULE_1", "SNMP_ONLY|RULE_1", "GNMI_ONLY|RULE_1"):
+        assert config["ACL_RULE"][rule_key]["SRC_IP"] == expected_src
+
+
+def test_add_ctrlplane_acls_gnmi_rule_requires_dst_port_ssh_and_snmp_do_not():
     """caclmgrd's EXTERNAL_CLIENT service has no built-in destination port;
     without L4_DST_PORT in the rule it skips the whole table and the gNMI
-    restriction would silently not exist. SNMP has a fixed service port
-    (161) in caclmgrd's ACL_SERVICES, so its rule must not pin one."""
+    restriction would silently not exist. SSH and SNMP have fixed service
+    ports (22, 161) in caclmgrd's ACL_SERVICES, so their rules must not pin
+    one."""
     config = {}
 
     _add_ctrlplane_acls(config, "10.42.0.5", 24)
 
     assert config["ACL_RULE"]["GNMI_ONLY|RULE_1"]["L4_DST_PORT"] == "8080"
+    assert "L4_DST_PORT" not in config["ACL_RULE"]["SSH_ONLY|RULE_1"]
     assert "L4_DST_PORT" not in config["ACL_RULE"]["SNMP_ONLY|RULE_1"]
 
 
@@ -102,24 +135,30 @@ def test_add_ctrlplane_acls_unusable_gnmi_port_falls_back_to_default(port):
 
 
 def test_add_ctrlplane_acls_merges_into_co_owned_tables_per_key():
-    """ACL_TABLE / ACL_RULE are multi-owner (SSH, SNMP, gNMI): the helper must
-    merge only its own keys, never rebind the table wholesale, so a sibling
-    control-plane helper's entries survive. Stale carry-over from a prior regen
-    is cleared by the central owned-table drop in generate_sonic_config, not
-    here (see the ownership model and MULTI_OWNER_OWNED_TABLE_KEYS)."""
+    """ACL_TABLE / ACL_RULE are multi-owner: the helper must merge only its
+    own keys, never rebind the table wholesale, so a sibling control-plane
+    helper's entries survive. Stale carry-over from a prior regen is cleared
+    by the central owned-table drop in generate_sonic_config, not here (see
+    the ownership model and MULTI_OWNER_OWNED_TABLE_KEYS)."""
     config = {
-        "ACL_TABLE": {"SSH_ONLY": {"type": "CTRLPLANE", "services": ["SSH"]}},
-        "ACL_RULE": {"SSH_ONLY|RULE_1": {"PRIORITY": "9999"}},
+        "ACL_TABLE": {"NTP_ONLY": {"type": "CTRLPLANE", "services": ["NTP"]}},
+        "ACL_RULE": {"NTP_ONLY|RULE_1": {"PRIORITY": "9999"}},
     }
 
     _add_ctrlplane_acls(config, "10.42.0.5", 24)
 
     # A sibling helper's entries are left untouched ...
-    assert config["ACL_TABLE"]["SSH_ONLY"] == {"type": "CTRLPLANE", "services": ["SSH"]}
-    assert config["ACL_RULE"]["SSH_ONLY|RULE_1"] == {"PRIORITY": "9999"}
+    assert config["ACL_TABLE"]["NTP_ONLY"] == {"type": "CTRLPLANE", "services": ["NTP"]}
+    assert config["ACL_RULE"]["NTP_ONLY|RULE_1"] == {"PRIORITY": "9999"}
     # ... and this helper's own entries are added alongside them.
-    assert set(config["ACL_TABLE"]) == {"SSH_ONLY", "SNMP_ONLY", "GNMI_ONLY"}
+    assert set(config["ACL_TABLE"]) == {
+        "NTP_ONLY",
+        "SSH_ONLY",
+        "SNMP_ONLY",
+        "GNMI_ONLY",
+    }
     assert set(config["ACL_RULE"]) == {
+        "NTP_ONLY|RULE_1",
         "SSH_ONLY|RULE_1",
         "SNMP_ONLY|RULE_1",
         "GNMI_ONLY|RULE_1",