sonic: Restrict SSH to the OOB management network#2337
Open
berendt wants to merge 1 commit into
Open
Conversation
e0ff1f4 to
7b01e69
Compare
Member
Author
|
Has to be revisited after merge of #2338. |
This was referenced Jun 10, 2026
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
_add_ssh_acl_configurationyou overwriteconfig["ACL_TABLE"]andconfig["ACL_RULE"]wholesale; consider usingsetdefault/updateso additional ACL emitters (e.g. future SNMP/GNMI CTRLPLANE ACLs) can coexist without being clobbered. - The orchestrator test
test_generate_sonic_config_no_oob_ip_drops_stale_acl_tablesasserts thatACL_TABLE/ACL_RULEare absent entirely; this may become too strict once other features legitimately emit ACLs without an OOB IP, so you might narrow the assertion to the SSH-only entries instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_add_ssh_acl_configuration` you overwrite `config["ACL_TABLE"]` and `config["ACL_RULE"]` wholesale; consider using `setdefault`/`update` so additional ACL emitters (e.g. future SNMP/GNMI CTRLPLANE ACLs) can coexist without being clobbered.
- The orchestrator test `test_generate_sonic_config_no_oob_ip_drops_stale_acl_tables` asserts that `ACL_TABLE`/`ACL_RULE` are absent entirely; this may become too strict once other features legitimately emit ACLs without an OOB IP, so you might narrow the assertion to the SSH-only entries instead.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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 #2329 AI-assisted: Claude Code Signed-off-by: Christian Berendt <berendt@osism.tech>
7b01e69 to
5239e98
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2329
What this does
Generates, per switch, a SONiC control-plane ACL (handled by
caclmgrd) that permits SSH only from the device's OOB management subnet. Once aCTRLPLANEtable binds theSSHservice, caclmgrd installs an implicit default-drop for it, so front-panel / in-band interfaces with IPs in the default VRF can no longer reach TCP/22.Walkthrough (commit order)
sonic: Take ownership of ACL_TABLE and ACL_RULE on regen— registers both tables inON_DEMAND_OWNED_TABLE_KEYS(feedingOWNED_TABLE_KEYS), so base-config content is dropped up front and the tables are rebuilt from scratch on every regen. The on-demand registry was chosen overTOP_LEVEL_SCAFFOLD_KEYSso the tables are entirely absent when nothing emits them (no empty{}scaffold entries). Includes an orchestrator test pinning that stale ACL entries carried over fromconfig_db.jsondo not survive a regen without an OOB IP. The existing exhaustive ownership test (test_generate_sonic_config_every_owned_table_drops_stale_entries) picks the new keys up automatically.sonic: Restrict SSH to the OOB management network— adds_add_ssh_acl_configuration(config, device, oob_ip_result), called fromgenerate_sonic_configin the management-interface block, only when an OOB IP exists. The permitted subnet is derived from the sameget_device_oob_ipresult used forMGMT_INTERFACE, normalised to the network address viaIPv4Network(..., strict=False). Emits exactly the shape from the issue:ACL_TABLE["SSH_ONLY"]—type: CTRLPLANE,services: ["SSH"]ACL_RULE["SSH_ONLY|RULE_1"]—PRIORITY 9999,PACKET_ACTION ACCEPT,SRC_IP <oob_network>/<prefix>,IP_TYPE IPTests: helper-level (table/rule shape, network normalisation incl. non-octet boundary, wholesale replacement of pre-existing entries) plus orchestrator glue (called with the full OOB result when present, not called and tables absent when not).
Notes for the reviewer
ACL_TABLE/ACL_RULEare not in the generated pydantic schema set (_generated/_schemas.pyonly modelsDASH_ACL_*), so the validator reports them as warnings, not errors. Adding thesonic-aclYANG model is deferred (relates to Cover Enterprise SONiC tables in the YANG model set #2258).AI-assisted: Claude Code