caclmgrd clarification

[justindthomas]

I'm trying to understand the iptables rules that get applied to the host addresses. This section in calcmgrd has me a little befuddled: https://github.com/sonic-net/sonic-host-services/blob/e8ae2afd612ef7fd08b7d855c48c78fe54b34ec4/scripts/caclmgrd

def generate_block_ip2me_traffic_iptables_commands(self, namespace, config_db_connector):
    INTERFACE_TABLE_NAME_LIST = [
        "LOOPBACK_INTERFACE",
        "VLAN_INTERFACE",
        "PORTCHANNEL_INTERFACE",
        "INTERFACE"
    ]

    block_ip2me_cmds = []

    # Add iptables rules to drop all packets destined for peer-to-peer interface IP addresses
    for iface_table_name in INTERFACE_TABLE_NAME_LIST:
        iface_table = config_db_connector.get_table(iface_table_name)
        if iface_table:
            for key, _ in iface_table.items():
                if not _ip_prefix_in_key(key):
                    continue

                iface_name, iface_cidr = key
                ip_ntwrk = ipaddress.ip_network(iface_cidr, strict=False)

                # For VLAN interfaces, the IP address we want to block is the default gateway (i.e.,
                # the first available host IP address of the VLAN subnet)
                ip_addr = next(ip_ntwrk.hosts()) if iface_table_name == "VLAN_INTERFACE" else ip_ntwrk.network_address

                if isinstance(ip_ntwrk, ipaddress.IPv4Network):
                    block_ip2me_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['iptables', '-A', 'INPUT', '-d', '{}/{}'.format(ip_addr, ip_ntwrk.max_prefixlen), '-j', 'DROP'])
                elif isinstance(ip_ntwrk, ipaddress.IPv6Network):
                    block_ip2me_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['ip6tables', '-A', 'INPUT', '-d', '{}/{}'.format(ip_addr, ip_ntwrk.max_prefixlen), '-j', 'DROP'])
                else:
                    self.log_warning("Unrecognized IP address type on interface '{}': {}".format(iface_name, ip_ntwrk))

    return block_ip2me_cmds

This comment in particular seems problematic:

# For VLAN interfaces, the IP address we want to block is the default gateway (i.e.,
# the first available host IP address of the VLAN subnet)
ip_addr = next(ip_ntwrk.hosts()) if iface_table_name == "VLAN_INTERFACE" else ip_ntwrk.network_address

It's not clear to my why we want to block the default gateway. But even if we do, selecting the first address is naive and potentially incorrect (i.e., there's no reason a default gateway must be the first address in the subnet).

This line also has the side-effect of blocking SSH to Loopback addresses (which often - maybe always - occupy the "network" address of a single-address subnet). As an example, here's my current LOOPBACK_INTERFACE table (which may be from the default configuration - I don't recall intentionally setting up that address):

"LOOPBACK_INTERFACE": {
    "Loopback0": {},
    "Loopback0|10.1.0.1/32": {}
},

If I understand the logic correctly, we loop through that list of relevant tables here:

for iface_table_name in INTERFACE_TABLE_NAME_LIST:

That matches on LOOPBACK_INTERFACE for my table. Then we loop through the table items here:

for key, _ in iface_table.items():

Then we skip the Loopback0 definition without an address:

if not _ip_prefix_in_key(key):
    continue

We build a network object from the relevant entry:

iface_name, iface_cidr = key
    ip_ntwrk = ipaddress.ip_network(iface_cidr, strict=False)

Then we get the first address if this is a VLAN_INTERFACE (which it is not) or just the network_address otherwise.

ip_addr = next(ip_ntwrk.hosts()) if iface_table_name == "VLAN_INTERFACE" else ip_ntwrk.network_address

I assume this will return 10.1.0.1/32 since this is a single-address subnet (although it seems like it should be undefined, I suspect both the network and broadcast addresses are 10.1.0.1/32).

Then we check if it's IPv4 and configure a block:

block_ip2me_cmds.append(self.iptables_cmd_ns_prefix[namespace] + ['iptables', '-A', 'INPUT', '-d', '{}/{}'.format(ip_addr, ip_ntwrk.max_prefixlen), '-j', 'DROP'])

This whole block seems ill-conceived. Is there documentation somewhere that explains the reasoning better? Or am I correct that this needs to be re-designed?

[prabhataravind]

@prsunny might have context on this. @justindthomas please sync up with Prince on this.