caclmgrd: correct IP2ME address logic

[bluecmd]

Why I did it

Fixes #7008.

Ensure that the destination IP that is permitted is the interface IP.

Currently the logic does not handle IP interfaces correctly and only produces correct results for /32 or /128 subnets.

{
    "LOOPBACK_INTERFACE": {
        "Loopback0": {},
        "Loopback0|10.10.10.10/32": {}
    },
    "MGMT_INTERFACE": {
        "eth0|172.18.0.249/24": {
            "gwaddr": "172.18.0.1"
        }
    },
    "VLAN_INTERFACE": {
        "Vlan110": {},
        "Vlan110|172.18.0.250/24": {},
    }
}

See iptables -vnL

    0     0 DROP       all  --  *      *       0.0.0.0/0            10.10.10.10         
    0     0 DROP       all  --  *      *       0.0.0.0/0            172.18.0.0          
    0     0 DROP       all  --  *      *       0.0.0.0/0            172.18.0.1         

How I did it

Corrected the logic and the iptables commands executed.

It is likely that the initial implementation was only ever tested on L3 interfaces with /32 and VLAN-interfaces where the switch had the first IP in the network (e.g. 192.168.0.1/24).

How to verify it

1. Apply patch
2. Configure some interfaces and VLAN interfaces
3. Execute iptables -vnL

You will see something like this:

    0     0 DROP       all  --  *      *       0.0.0.0/0            10.10.10.10          /* Block IP2ME on interface Loopback0 */
    0     0 DROP       all  --  *      *       0.0.0.0/0            172.18.0.250         /* Block IP2ME on interface Vlan110 */

Note that MGMT_INTERFACEs are not blocked by default.

Which release branch to backport (provide reason below if selected)

• 201811
• 201911
• 202006
• 202012
• 202106
• 202111

Description for the changelog

caclmgrd: handle IP2ME subnets

A picture of a cute animal (not mandatory but encouraged)

Note: The original code was added in #4412

[mssonicbld]

/AzurePipelines run

[azure-pipelines]

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

[azure-pipelines]

Commenter does not have sufficient privileges for PR 9826 in repo Azure/sonic-buildimage

[prsunny]

PR is missing the unit tests

[bluecmd]

PR is missing the unit tests

Tests added

[bluecmd]

@prsunny @yxieca Ready for review, this time with tests! :-)

[yxieca]

@prsunny the change looks good to me. The only thing is that I am not sure if relaxing mgmt cacl aligns with our design?

[bluecmd]

@prsunny the change looks good to me. The only thing is that I am not sure if relaxing mgmt cacl aligns with our design?

To be clear, this PR does not relax the cacls for MGMT in practice - unless I am missing something. My assessment is that the previous MGMT cacls never worked as intended and wouldn't block any traffic unless these very specific scenarios were met:

• You used a /32 management address.
• You used the network identity as the MGMT address, e.g. 192.168.0.0/24 as opposed to e.g. 192.168.0.1/24.

I would be very surprised if anyone used SONiC in these scenarios - and if they did, they would have no way to SSH into the switch as the default factory configuration would block SSH on the MGMT interface. It stands to reason they would have reported an issue.

[yxieca]

@prsunny the change looks good to me. The only thing is that I am not sure if relaxing mgmt cacl aligns with our design?

To be clear, this PR does not relax the cacls for MGMT in practice - unless I am missing something. My assessment is that the previous MGMT cacls never worked as intended and wouldn't block any traffic unless these very specific scenarios were met:

• You used a /32 management address.
• You used the network identity as the MGMT address, e.g. 192.168.0.0/24 as opposed to e.g. 192.168.0.1/24.

I would be very surprised if anyone used SONiC in these scenarios - and if they did, they would have no way to SSH into the switch as the default factory configuration would block SSH on the MGMT interface. It stands to reason they would have reported an issue.

Thanks for the clarifications!

[yxieca]

Let's merge #9903 before this PR.

[prsunny]

@prsunny the change looks good to me. The only thing is that I am not sure if relaxing mgmt cacl aligns with our design?

To be clear, this PR does not relax the cacls for MGMT in practice - unless I am missing something. My assessment is that the previous MGMT cacls never worked as intended and wouldn't block any traffic unless these very specific scenarios were met:

• You used a /32 management address.
• You used the network identity as the MGMT address, e.g. 192.168.0.0/24 as opposed to e.g. 192.168.0.1/24.

I would be very surprised if anyone used SONiC in these scenarios - and if they did, they would have no way to SSH into the switch as the default factory configuration would block SSH on the MGMT interface. It stands to reason they would have reported an issue.

Thanks for the clarifications!

@prsunny the change looks good to me. The only thing is that I am not sure if relaxing mgmt cacl aligns with our design?

To be clear, this PR does not relax the cacls for MGMT in practice - unless I am missing something. My assessment is that the previous MGMT cacls never worked as intended and wouldn't block any traffic unless these very specific scenarios were met:

• You used a /32 management address.
• You used the network identity as the MGMT address, e.g. 192.168.0.0/24 as opposed to e.g. 192.168.0.1/24.

I would be very surprised if anyone used SONiC in these scenarios - and if they did, they would have no way to SSH into the switch as the default factory configuration would block SSH on the MGMT interface. It stands to reason they would have reported an issue.

Thanks for the clarifications!

I'm not sure I understand the case here. Currently we install drop rules as below:

1. For Vlan interface, the drop shall be added to the default gw or the first IP address in the subnet
2. For Mgmt interface, it shall be to the first ip (gw) with /32. This will not block anyone from ssh'ing to the device.

Our requirement is to block kernel accepting any packet to the gw IPs.

[prsunny]

please check comment

[bluecmd]

Sure,

I'm not sure I understand the case here. Currently we install drop rules as below:

1. For Vlan interface, the drop shall be added to the default gw or the first IP address in the subnet

2. For Mgmt interface, it shall be to the first ip (gw) with /32. This will not block anyone from ssh'ing to the device.

Our requirement is to block kernel accepting any packet to the gw IPs.

Can you explain why these cases are formulated this way? I suspect these are not the true requirements, but rather a misstaken way to block the IPs configured on the switch. As I explained above, the current behavior does not make any sense as it would be almost impossible for it to block packets in real life scenarios. It does not protect the control plane currently in practice, so the scenario does not make sense as currently formulated in the code.

To avoid talking in circles, we can break it down if we look at the original PR (#4412) these are the relevant scenarios:

• Add iptables rules to drop all packets destined for loopback interface IP addresses
• Add iptables rules to drop all packets destined for management interface IP addresses
• Add iptables rules to drop all packets destined for point-to-point interface IP addresses

These were broken before this PR and only supported /32 - with this PR it fixes to work with any subnet mask. Do we agree that this PR fixes these issues?

• Add iptables rules to drop all packets destined for our VLAN interface gateway IP addresses

This one is a bit more difficult. The only interpretation I can make of this sentence that make sense is that the assumption that the SONiC switch is the gateway. Thus we can simplify the sentence to be like the other interfaces above; "Add iptables rules to drop all packets destined for VLAN interface IP addresses". In the case where we are not the gateway, the packets would never enter the INPUT table anyway in the controlplane, so any other interpretation does in my honest opinion seems moot.

Is that answer to your question @prsunny ?

[bluecmd]

It seems the current tests on master is a bit unstable. I rebased against latest master in an attempt to see if the tests were fixed but now they appear to be even more broken.

Before the rebase the test results can be found here. It appears that issue happens on other PRs as well (like this one) so it seems to have nothing to do with the code here. Earlier test runs of this PR were fine before adding the .1 logic, FWIW.

[bluecmd]

@prsunny What are the next steps here? The tests seems to be failing in the current master branch already, so I cannot provide a PR with all tests working. Do you want to wait until somebody fixes the master branch before merging these, or are you fine accepting this patch with the tests broken in master?

[prsunny]

@bluecmd , would you please rebase to latest?

[bluecmd]

@bluecmd , would you please rebase to latest?

Done!

EDIT: Whoops, accidentally clicked "comment and close", Reopened it again.

[prsunny]

/azp run Azure.sonic-buildimage

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[bluecmd]

I will move this PR to https://github.com/sonic-net/sonic-host-services now when this code has moved.

[bluecmd]

See sonic-net/sonic-host-services#4 for the first part of this "version 2" attempt :-)