caclmgrd: Don't block traffic to mgmt by default

[bluecmd]

Currently the IP2ME rules block the management interface's identity address instead of the actual host address. This logic results in a DROP rule that hits the management interface address only in the case of /32 netmask - all other netmasks will result in traffic being accepted by default. Thus, it is exceedingly likely that the current DROP rule has never worked for management interfaces given that /32 are mainly loopback addresses, not for network links.

Claim: Management addresses should not be dropped by default. This matches the current SONiC behavior in all current releases. Dropping by default would result in that the device can only be managed by serial console (physical or possibly BMC SoL).

Dropping management traffic is still possible by adding any explicit ACL rule which will install a default DROP that hits management as well, so this change is a no-op for every case except if anybody is using /32 on management interface as well as expects that traffic to be dropped (extremely unlikely, and still achievable by adding explicit ACLs).

Also see older discussion summary in sonic-net/sonic-buildimage#9826 (comment)

[bluecmd]

Ping @prsunny @bingwang-ms

[prsunny]

The expectation is that, this /32 DROP rule shall be installed at the end. If any control or other protocol (ssh/snmp etc) is required to be allowed, the user must explicitly add allow rules above the DROP rule

[bluecmd]

The expectation is that, this /32 DROP rule shall be installed at the end. If any control or other protocol (ssh/snmp etc) is required to be allowed, the user must explicitly add allow rules above the DROP rule

@prsunny On the meeting in February we agreed that this DROP should be removed (see sonic-net/sonic-buildimage#9826 (comment)) , has new information come forth or has anything else changed?

[prsunny]

The expectation is that, this /32 DROP rule shall be installed at the end. If any control or other protocol (ssh/snmp etc) is required to be allowed, the user must explicitly add allow rules above the DROP rule

@prsunny On the meeting in February we agreed that this DROP should be removed (see sonic-net/sonic-buildimage#9826 (comment)) , has new information come forth or has anything else changed?

I see, thanks for pointing out. For default ssh via mgmt, this change is required. lgtm, @yxieca to review as well.

[SuvarnaMeenakshi]

@bluecmd , @prsunny - this PR change is causing a sonic-mgmt test to fail and is blocking merge of sonic-host-services sub module update. https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fsonic-net%2Fsonic-buildimage%2Fpull%2F12195&data=05%7C01%7CSuvarna.Meenakshi%40microsoft.com%7Cb238b7ef21c0406f056108dab0a19f03%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C638016505489756783%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=gyDxl3OWFzfmMMWaIkdX8XteVTSnpXpL3E5O58TIQuw%3D&reserved=0

Is there a plan to merge this change to other branches, based on that fix should go in sonic-mgmt repo as well.

https://github.com/sonic-net/sonic-mgmt/blob/master/tests/cacl/test_cacl_application.py#L300 - this is causing test case to fail.

[bluecmd]

@SuvarnaMeenakshi Thanks for flagging that. After taking a quick look at the tests and verify_cacl it seems to me that those tests are redundant to the unit tests introduced with this PR.

As long as we have tests that depend on the exact iptables rule output, which those tests you linked do, any changes to the rule logic in this repo will be really hard to do, as is evident with the current case @SuvarnaMeenakshi is bringing up.

My take: I recommend that the linked unit tests are deleted (or significantly reduced), as we are now testing that behavior in this repo's own tests. That way we test the specifics in this repo, and we test the manifested outcome (e.g try to run SSH or just open a port) instead of exactly how is implemented.

Thoughts?

[SuvarnaMeenakshi]

@SuvarnaMeenakshi Thanks for flagging that. After taking a quick look at the tests and verify_cacl it seems to me that those tests are redundant to the unit tests introduced with this PR.

As long as we have tests that depend on the exact iptables rule output, which those tests you linked do, any changes to the rule logic in this repo will be really hard to do, as is evident with the current case @SuvarnaMeenakshi is bringing up.

My take: I recommend that the linked unit tests are deleted (or significantly reduced), as we are now testing that behavior in this repo's own tests. That way we test the specifics in this repo, and we test the manifested outcome (e.g try to run SSH or just open a port) instead of exactly how is implemented.

Thoughts?

We should keep the unit-tests and sonic-mgmt tests as sonic-mgmt tests.
You suggestion to test the outcome does seem right, but my understanding is that we compare the rules as it easier when compared to testing possible outcome for all the cacls.
In the interest of time to make sure that we are able to update sonic-host-services submodule, I have created an issue to track this: sonic-net/sonic-buildimage#12464. Will try to have a easy fix in sonic-mgmt repo to unblock and can be fine tuned later.