diff --git a/osism/tasks/conductor/sonic/config_generator.py b/osism/tasks/conductor/sonic/config_generator.py index cc7658a5..7b9746bc 100644 --- a/osism/tasks/conductor/sonic/config_generator.py +++ b/osism/tasks/conductor/sonic/config_generator.py @@ -56,6 +56,10 @@ # VXLAN VTEP name used for VXLAN tunnel configuration VXLAN_VTEP_NAME = "vtepServ" +# Default listen port of the SONiC telemetry/gNMI container, used for the +# GNMI_ONLY control-plane ACL rule when TELEMETRY|gnmi does not set a port. +DEFAULT_GNMI_PORT = "8080" + # Top-level scaffold keys that the orchestrator and downstream helpers index # into directly. Kept as a single source of truth so that the production code # and the test helpers cannot drift apart when keys are added or removed. @@ -105,11 +109,11 @@ # difference being only that the generator depends on this one as an input. # Distinct from inherited tables, which the generator also writes selected # fields into; the distinction is read-only vs. read-and-update, not the access -# syntax (inherited tables are read via config.get() too). Empty for now; the -# first consumer is the gNMI listen port, read from TELEMETRY. Must stay +# syntax (inherited tables are read via config.get() too). The first (and so +# far only) consumer is the gNMI listen port, read from TELEMETRY. Must stay # disjoint from OWNED_TABLE_KEYS -- an image-consumed table dropped up front # would always read back empty. -IMAGE_CONSUMED_TABLE_KEYS = () +IMAGE_CONSUMED_TABLE_KEYS = ("TELEMETRY",) # Owned tables that are also scaffolded: every scaffold key except the # inherited ones. The orchestrator setdefault-creates these up front, so @@ -126,6 +130,8 @@ # defaults (location "Data Center", contact "info@example.com") even when the # device has no SNMP data in NetBox. ON_DEMAND_OWNED_TABLE_KEYS = ( + "ACL_RULE", + "ACL_TABLE", "ROUTE_REDISTRIBUTE", "SNMP_SERVER", "SNMP_AGENT_ADDRESS_CONFIG", @@ -424,6 +430,8 @@ 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 + _add_ctrlplane_acls(config, oob_ip, prefix_len) else: oob_ip = None @@ -2326,3 +2334,73 @@ def _add_snmp_configuration(config, device, oob_ip): counter += 1 logger.debug(f"Added snmp_server_target {host}") + + +def _get_gnmi_port(config): + """Return the gNMI server port for the GNMI_ONLY control-plane ACL rule. + + The telemetry/gNMI container reads its listen port from + TELEMETRY|gnmi|port and falls back to 8080 when unset, so the ACL rule + follows the same lookup against the (image-consumed) TELEMETRY table. + """ + gnmi_config = config.get("TELEMETRY", {}).get("gnmi", {}) + return str(gnmi_config.get("port", DEFAULT_GNMI_PORT)) + + +def _add_ctrlplane_acls(config, oob_ip, prefix_len): + """Add control-plane ACLs restricting SNMP and gNMI to the OOB network. + + Emits ACL_TABLE entries of type CTRLPLANE bound to the 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. + + caclmgrd's EXTERNAL_CLIENT service has no built-in destination port + (verified against sonic-host-services 202211 through master): the rule + must carry it in L4_DST_PORT, otherwise caclmgrd skips the whole table + and the gNMI restriction would silently not exist. + + 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. + """ + network = ipaddress.ip_network(f"{oob_ip}/{prefix_len}", strict=False) + if network.version != 4: + logger.warning( + f"OOB IP {oob_ip}/{prefix_len} is not IPv4; skipping control-plane ACLs" + ) + return + + accept_from_oob = { + "PRIORITY": "9999", + "PACKET_ACTION": "ACCEPT", + "SRC_IP": str(network), + "IP_TYPE": "IP", + } + config.setdefault("ACL_TABLE", {}) + config["ACL_TABLE"]["SNMP_ONLY"] = { + "policy_desc": "SNMP_ONLY", + "type": "CTRLPLANE", + "services": ["SNMP"], + } + config["ACL_TABLE"]["GNMI_ONLY"] = { + "policy_desc": "GNMI_ONLY", + "type": "CTRLPLANE", + "services": ["EXTERNAL_CLIENT"], + } + config.setdefault("ACL_RULE", {}) + config["ACL_RULE"]["SNMP_ONLY|RULE_1"] = dict(accept_from_oob) + config["ACL_RULE"]["GNMI_ONLY|RULE_1"] = { + **accept_from_oob, + "L4_DST_PORT": _get_gnmi_port(config), + } 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 new file mode 100644 index 00000000..3f8689de --- /dev/null +++ b/tests/unit/tasks/conductor/sonic/test_config_generator_ctrlplane_acls.py @@ -0,0 +1,119 @@ +# SPDX-License-Identifier: Apache-2.0 + +"""Unit tests for ``_add_ctrlplane_acls`` in ``config_generator`` (#2330).""" + +from osism.tasks.conductor.sonic.config_generator import ( + ON_DEMAND_OWNED_TABLE_KEYS, + TOP_LEVEL_SCAFFOLD_KEYS, + _add_ctrlplane_acls, +) + + +def test_add_ctrlplane_acls_emits_snmp_and_gnmi_ctrlplane_tables(): + config = {} + + _add_ctrlplane_acls(config, "10.42.0.5", 24) + + assert config["ACL_TABLE"] == { + "SNMP_ONLY": { + "policy_desc": "SNMP_ONLY", + "type": "CTRLPLANE", + "services": ["SNMP"], + }, + "GNMI_ONLY": { + "policy_desc": "GNMI_ONLY", + "type": "CTRLPLANE", + "services": ["EXTERNAL_CLIENT"], + }, + } + + +def test_add_ctrlplane_acls_rules_accept_network_normalised_oob_subnet(): + """SRC_IP must carry the network address, not the device's host IP.""" + config = {} + + _add_ctrlplane_acls(config, "10.42.0.5", 24) + + assert config["ACL_RULE"]["SNMP_ONLY|RULE_1"] == { + "PRIORITY": "9999", + "PACKET_ACTION": "ACCEPT", + "SRC_IP": "10.42.0.0/24", + "IP_TYPE": "IP", + } + assert config["ACL_RULE"]["GNMI_ONLY|RULE_1"] == { + "PRIORITY": "9999", + "PACKET_ACTION": "ACCEPT", + "SRC_IP": "10.42.0.0/24", + "IP_TYPE": "IP", + "L4_DST_PORT": "8080", + } + + +def test_add_ctrlplane_acls_gnmi_rule_requires_dst_port_snmp_does_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.""" + 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"]["SNMP_ONLY|RULE_1"] + + +def test_add_ctrlplane_acls_gnmi_port_follows_telemetry_table(): + """A TELEMETRY|gnmi|port from the base config wins over the 8080 default + (and non-string values are normalised to the string ConfigDB expects).""" + config = {"TELEMETRY": {"gnmi": {"port": 50051}}} + + _add_ctrlplane_acls(config, "10.42.0.5", 24) + + assert config["ACL_RULE"]["GNMI_ONLY|RULE_1"]["L4_DST_PORT"] == "50051" + + +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).""" + config = { + "ACL_TABLE": {"SSH_ONLY": {"type": "CTRLPLANE", "services": ["SSH"]}}, + "ACL_RULE": {"SSH_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"} + # ... 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_RULE"]) == { + "SSH_ONLY|RULE_1", + "SNMP_ONLY|RULE_1", + "GNMI_ONLY|RULE_1", + } + + +def test_add_ctrlplane_acls_non_ipv4_oob_ip_emits_nothing(): + """The rules are IPv4-only (SRC_IP); an IPv6 OOB IP must not produce + half-correct tables — and must not break config generation either.""" + config = {} + + _add_ctrlplane_acls(config, "2001:db8::5", 64) + + assert "ACL_TABLE" not in config + assert "ACL_RULE" not in config + + +def test_ctrlplane_acl_tables_are_on_demand_owned(): + """#2330 requires ACL_TABLE/ACL_RULE to be rebuilt from scratch on every + regen and absent without an OOB IP. Both guarantees hang on the keys + being on-demand owned: owned tables are dropped from the base config up + front, and on-demand (unlike scaffolded) tables are not re-created as + empty dicts.""" + for key in ("ACL_TABLE", "ACL_RULE"): + assert key in ON_DEMAND_OWNED_TABLE_KEYS + assert key not in TOP_LEVEL_SCAFFOLD_KEYS diff --git a/tests/unit/tasks/conductor/sonic/test_config_generator_orchestrator.py b/tests/unit/tasks/conductor/sonic/test_config_generator_orchestrator.py index be149e67..bb6a8264 100644 --- a/tests/unit/tasks/conductor/sonic/test_config_generator_orchestrator.py +++ b/tests/unit/tasks/conductor/sonic/test_config_generator_orchestrator.py @@ -95,6 +95,7 @@ def patch(name, **kw): _add_loopback_configuration=patch("_add_loopback_configuration"), _add_log_server_configuration=patch("_add_log_server_configuration"), _add_snmp_configuration=patch("_add_snmp_configuration"), + _add_ctrlplane_acls=patch("_add_ctrlplane_acls"), _add_vrf_configuration=patch("_add_vrf_configuration"), _add_portchannel_configuration=patch("_add_portchannel_configuration"), get_cached_device_interfaces=patch( @@ -395,6 +396,43 @@ def test_generate_sonic_config_no_oob_ip_leaves_mgmt_empty_and_passes_none( assert snmp_oob is None +def test_generate_sonic_config_oob_ip_wires_ctrlplane_acls( + mocker, patch_orchestrator_helpers, make_orchestrator_device +): + """With an OOB IP the orchestrator delegates the control-plane ACLs + (#2330) to ``_add_ctrlplane_acls`` with the raw OOB IP and prefix — + network normalisation is the helper's job.""" + patch_base_config(mocker) + patch_orchestrator_helpers.get_device_oob_ip.return_value = ("10.42.0.5", 24) + device = make_orchestrator_device() + + config = generate_sonic_config(device, "HWSKU") + + patch_orchestrator_helpers._add_ctrlplane_acls.assert_called_once_with( + config, "10.42.0.5", 24 + ) + + +def test_generate_sonic_config_no_oob_ip_skips_ctrlplane_acls( + mocker, patch_orchestrator_helpers, make_orchestrator_device +): + """Without an OOB IP no control-plane ACLs are wired and the owned + ACL_TABLE / ACL_RULE tables stay absent — stale base-config content is + removed by the up-front owned-table drop, not re-created.""" + base = make_base_config() + base["ACL_TABLE"] = {"SNMP_ONLY": {"type": "CTRLPLANE"}} + base["ACL_RULE"] = {"SNMP_ONLY|RULE_1": {"PRIORITY": "9999"}} + patch_base_config(mocker, base_config=base) + patch_orchestrator_helpers.get_device_oob_ip.return_value = None + device = make_orchestrator_device() + + config = generate_sonic_config(device, "HWSKU") + + patch_orchestrator_helpers._add_ctrlplane_acls.assert_not_called() + assert "ACL_TABLE" not in config + assert "ACL_RULE" not in config + + # --------------------------------------------------------------------------- # generate_sonic_config — breakout merge # ---------------------------------------------------------------------------