From 5239e9849d3ad8160edacf3db1537fa329a44856 Mon Sep 17 00:00:00 2001 From: Christian Berendt Date: Wed, 10 Jun 2026 08:53:19 +0200 Subject: [PATCH] sonic: Restrict SSH to the OOB management network The generated ConfigDB did not restrict which networks can reach the switch's SSH service: front-panel / in-band interfaces with IPs in the default VRF could still reach TCP/22. Emit a per-device SONiC control-plane ACL handled by caclmgrd: an ACL_TABLE of type CTRLPLANE bound to the SSH service plus an ACL_RULE that ACCEPTs the device's OOB management subnet. Once a CTRLPLANE table binds a service, caclmgrd installs an implicit default-drop for it, so SSH is reachable only from the management network. The permitted subnet is derived from the same OOB data already used for MGMT_INTERFACE (get_device_oob_ip), normalised to the network address. Only the device's own OOB subnet is permitted in this iteration. When no OOB IP is present, no ACL is emitted and SSH is not locked down. ACL_TABLE / ACL_RULE are not yet covered by the generated pydantic schema set, so the validator reports them as warnings, not errors. Closes osism/python-osism#2329 AI-assisted: Claude Code Signed-off-by: Christian Berendt --- .../tasks/conductor/sonic/config_generator.py | 29 ++++---- .../test_config_generator_ctrlplane_acls.py | 69 +++++++++++++++---- 2 files changed, 71 insertions(+), 27 deletions(-) diff --git a/osism/tasks/conductor/sonic/config_generator.py b/osism/tasks/conductor/sonic/config_generator.py index 24f05ef96..4f5ab7308 100644 --- a/osism/tasks/conductor/sonic/config_generator.py +++ b/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, diff --git a/tests/unit/tasks/conductor/sonic/test_config_generator_ctrlplane_acls.py b/tests/unit/tasks/conductor/sonic/test_config_generator_ctrlplane_acls.py index c44251776..92ea39505 100644 --- a/tests/unit/tasks/conductor/sonic/test_config_generator_ctrlplane_acls.py +++ b/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",