From 0a931391d8990f3e654b4bfda24ae4119c609bbf Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Wed, 7 Apr 2021 13:16:21 +0000 Subject: [PATCH] Make ARP protection commands compatible with "ebtables-nft" "nftables" compatible binary, "ebtables-nft", is not 100% compatible with the legacy API, as reported in LP#1922892. This patch fixes the following issues when using "ebtables-nft" (while keeping compatibility with legacy binary): - When a new chain is created, a default DROP rule is added at the end of the chain (append). This will prevent the error code 4 when the chain is listed. - The chain rules are added at the begining of the chain (insert), before the default DROP rule. This will prioritize the port rules. - The MAC rules are cleaned before the new ones are added. That will prevent the deletion of any new needed rule, now added after the deletion. - The "ebtables" command will retry on error code 4. This is the error returned when the chains are listed and no rule is present in a new created chain (reporeted in LP#1922892). This code is backwards compatible, that means it works with the legacy "ebtables" binary; this is currently installed in the Neutron CI [1]. In order to test with the new binary, "ebtables-nft", two new CI jobs are added to the periodic queue [2]. [1]https://github.com/openstack/neutron/blob/1ad9ca56b07ffdc9f7e0bc6a62af61961b9128eb/roles/legacy_ebtables/tasks/main.yaml [2]https://review.opendev.org/c/openstack/neutron/+/785144 Closes-Bug: #1922892 Related-Bug: #1508155 Change-Id: I9463b000f6f63e65aaf91d60b30f6c92c01e3baf --- .../drivers/linuxbridge/agent/arp_protect.py | 54 +++++++++---------- .../linuxbridge/agent/test_arp_protect.py | 51 +++++------------- 2 files changed, 36 insertions(+), 69 deletions(-) diff --git a/neutron/plugins/ml2/drivers/linuxbridge/agent/arp_protect.py b/neutron/plugins/ml2/drivers/linuxbridge/agent/arp_protect.py index 600e63c2183..fede638cb41 100644 --- a/neutron/plugins/ml2/drivers/linuxbridge/agent/arp_protect.py +++ b/neutron/plugins/ml2/drivers/linuxbridge/agent/arp_protect.py @@ -73,12 +73,6 @@ def delete_arp_spoofing_protection(vifs): _delete_arp_spoofing_protection(vifs, current_rules, table='nat', chain='PREROUTING') - # TODO(haleyb) this can go away in "R" cycle, it's here to cleanup - # old chains in the filter table - current_rules = ebtables(['-L'], table='filter').splitlines() - _delete_arp_spoofing_protection(vifs, current_rules, table='filter', - chain='FORWARD') - def _delete_arp_spoofing_protection(vifs, current_rules, table, chain): # delete the jump rule and then delete the whole chain @@ -92,10 +86,11 @@ def _delete_arp_spoofing_protection(vifs, current_rules, table, chain): chain=chain) -def _delete_unreferenced_arp_protection(current_vifs, table, chain): +@lockutils.synchronized('ebtables') +def delete_unreferenced_arp_protection(current_vifs): # deletes all jump rules and chains that aren't in current_vifs but match # the spoof prefix - current_rules = ebtables(['-L'], table=table).splitlines() + current_rules = ebtables(['-L'], table='nat').splitlines() to_delete = [] for line in current_rules: # we're looking to find and turn the following: @@ -107,19 +102,8 @@ def _delete_unreferenced_arp_protection(current_vifs, table, chain): to_delete.append(devname) LOG.info("Clearing orphaned ARP spoofing entries for devices %s", to_delete) - _delete_arp_spoofing_protection(to_delete, current_rules, table=table, - chain=chain) - - -@lockutils.synchronized('ebtables') -def delete_unreferenced_arp_protection(current_vifs): - _delete_unreferenced_arp_protection(current_vifs, - table='nat', chain='PREROUTING') - - # TODO(haleyb) this can go away in "R" cycle, it's here to cleanup - # old chains in the filter table - _delete_unreferenced_arp_protection(current_vifs, - table='filter', chain='FORWARD') + _delete_arp_spoofing_protection(to_delete, current_rules, table='nat', + chain='PREROUTING') @lockutils.synchronized('ebtables') @@ -133,12 +117,17 @@ def _install_arp_spoofing_protection(vif, addresses, current_rules): vif_chain = chain_name(vif) if not chain_exists(vif_chain, current_rules): ebtables(['-N', vif_chain, '-P', 'DROP']) - # flush the chain to clear previous accepts. this will cause dropped ARP - # packets until the allows are installed, but that's better than leaked - # spoofed packets and ARP can handle losses. - ebtables(['-F', vif_chain]) + # Append a default DROP rule at the end of the chain. This will + # avoid "ebtables-nft" error when listing the chain. + ebtables(['-A', vif_chain, '-j', 'DROP']) + else: + # Flush the chain to clear previous accepts. This will cause dropped + # ARP packets until the allows are installed, but that's better than + # leaked spoofed packets and ARP can handle losses. + ebtables(['-F', vif_chain]) + ebtables(['-A', vif_chain, '-j', 'DROP']) for addr in sorted(addresses): - ebtables(['-A', vif_chain, '-p', 'ARP', '--arp-ip-src', addr, + ebtables(['-I', vif_chain, '-p', 'ARP', '--arp-ip-src', addr, '-j', 'ACCEPT']) # check if jump rule already exists, if not, install it if not vif_jump_present(vif, current_rules): @@ -178,17 +167,22 @@ def _install_mac_spoofing_protection(vif, port_details, current_rules): # mac filter chain for each vif which has a default deny if not chain_exists(vif_chain, current_rules): ebtables(['-N', vif_chain, '-P', 'DROP']) + # Append a default DROP rule at the end of the chain. This will + # avoid "ebtables-nft" error when listing the chain. + ebtables(['-A', vif_chain, '-j', 'DROP']) + # check if jump rule already exists, if not, install it if not _mac_vif_jump_present(vif, current_rules): - ebtables(['-A', 'PREROUTING', '-i', vif, '-j', vif_chain]) + ebtables(['-I', 'PREROUTING', '-i', vif, '-j', vif_chain]) + + _delete_vif_mac_rules(vif, current_rules) # we can't just feed all allowed macs at once because we can exceed # the maximum argument size. limit to 500 per rule. for chunk in (mac_addresses[i:i + 500] for i in range(0, len(mac_addresses), 500)): - new_rule = ['-A', vif_chain, '-i', vif, + new_rule = ['-I', vif_chain, '-i', vif, '--among-src', ','.join(sorted(chunk)), '-j', 'RETURN'] ebtables(new_rule) - _delete_vif_mac_rules(vif, current_rules) def _mac_vif_jump_present(vif, current_rules): @@ -227,7 +221,7 @@ NAMESPACE = None @tenacity.retry( wait=tenacity.wait_exponential(multiplier=0.02), - retry=tenacity.retry_if_exception(lambda e: e.returncode == 255), + retry=tenacity.retry_if_exception(lambda e: e.returncode in [255, 4]), reraise=True ) def ebtables(comm, table='nat'): diff --git a/neutron/tests/unit/plugins/ml2/drivers/linuxbridge/agent/test_arp_protect.py b/neutron/tests/unit/plugins/ml2/drivers/linuxbridge/agent/test_arp_protect.py index c30956251c4..bcd8d44b9d9 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/linuxbridge/agent/test_arp_protect.py +++ b/neutron/tests/unit/plugins/ml2/drivers/linuxbridge/agent/test_arp_protect.py @@ -75,13 +75,19 @@ class TestLinuxBridgeARPSpoofing(base.BaseTestCase): check_exit_code=True, extra_ok_codes=None, log_fail_as_error=True, run_as_root=True, privsep_exec=True), - mock.ANY, mock.call(['ebtables', '-t', 'nat', '--concurrent', '-A', + 'neutronMAC-%s' % vif, '-j', 'DROP'], + check_exit_code=True, extra_ok_codes=None, + log_fail_as_error=True, run_as_root=True, + privsep_exec=True), + mock.ANY, + mock.call(['ebtables', '-t', 'nat', '--concurrent', '-I', 'PREROUTING', '-i', vif, '-j', mac_chain], check_exit_code=True, extra_ok_codes=None, log_fail_as_error=True, run_as_root=True, privsep_exec=True), - mock.call(['ebtables', '-t', 'nat', '--concurrent', '-A', + mock.ANY, + mock.call(['ebtables', '-t', 'nat', '--concurrent', '-I', mac_chain, '-i', vif, '--among-src', '%s' % ','.join(sorted(mac_addresses)), '-j', 'RETURN'], @@ -89,21 +95,20 @@ class TestLinuxBridgeARPSpoofing(base.BaseTestCase): log_fail_as_error=True, run_as_root=True, privsep_exec=True), mock.ANY, - mock.ANY, mock.call(['ebtables', '-t', 'nat', '--concurrent', '-N', spoof_chain, '-P', 'DROP'], check_exit_code=True, extra_ok_codes=None, log_fail_as_error=True, run_as_root=True, privsep_exec=True), - mock.call(['ebtables', '-t', 'nat', '--concurrent', '-F', - spoof_chain], + mock.call(['ebtables', '-t', 'nat', '--concurrent', '-A', + spoof_chain, '-j', 'DROP'], check_exit_code=True, extra_ok_codes=None, log_fail_as_error=True, run_as_root=True, - privsep_exec=True), + privsep_exec=True) ] for addr in sorted(ip_addresses): expected.extend([ - mock.call(['ebtables', '-t', 'nat', '--concurrent', '-A', + mock.call(['ebtables', '-t', 'nat', '--concurrent', '-I', spoof_chain, '-p', 'ARP', '--arp-ip-src', addr, '-j', 'ACCEPT'], check_exit_code=True, extra_ok_codes=None, @@ -167,38 +172,6 @@ class TestLinuxBridgeARPSpoofing(base.BaseTestCase): check_exit_code=True, extra_ok_codes=None, log_fail_as_error=True, run_as_root=True, privsep_exec=True), - mock.call(['ebtables', '-t', 'filter', '--concurrent', '-L'], - check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True, - privsep_exec=True), - mock.ANY, - mock.call(['ebtables', '-t', 'filter', '--concurrent', '-D', - 'FORWARD', '-i', VIF, '-j', spoof_chain, - '-p', 'ARP'], - check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True, - privsep_exec=True), - mock.call(['ebtables', '-t', 'filter', '--concurrent', '-F', - spoof_chain], - check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True, - privsep_exec=True), - mock.call(['ebtables', '-t', 'filter', '--concurrent', '-X', - spoof_chain], - check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True, - privsep_exec=True), - mock.ANY, - mock.call(['ebtables', '-t', 'filter', '--concurrent', '-F', - mac_chain], - check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True, - privsep_exec=True), - mock.call(['ebtables', '-t', 'filter', '--concurrent', '-X', - mac_chain], - check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True, - privsep_exec=True), ] arp_protect.delete_arp_spoofing_protection([VIF])