ovsfw: Fix overlapping MAC addresses on integration bridge

The patch relies on the fact that traffic not going from instance
(and thus port not managed by firewall) is tagged. Traffic coming from
the instance is not tagged and thus net register is used for marking
such traffic. These two approaches make matching rules unique even if
two ports from different networks share its' mac addressess.

Traffic coming from trusted ports is marked with network in registry
so firewall can decide later to which network traffic belongs.

Closes-bug: #1626010

Change-Id: Ia05d75a01b0469a0eaa82ada67b16a9481c50f1c

doc/source/contributor/internals/openvswitch_firewall.rst

@@ -146,8 +146,8 @@ connections into separate conntrack zones.
 
  table=0,  priority=100,in_port=1 actions=load:0x1->NXM_NX_REG5[],load:0x284->NXM_NX_REG6[],resubmit(,71)
  table=0,  priority=100,in_port=2 actions=load:0x2->NXM_NX_REG5[],load:0x284->NXM_NX_REG6[],resubmit(,71)
- table=0,  priority=90,dl_dst=fa:16:3e:a4:22:10 actions=load:0x1->NXM_NX_REG5[],load:0x284->NXM_NX_REG6[],resubmit(,81)
- table=0,  priority=90,dl_dst=fa:16:3e:24:57:c7 actions=load:0x2->NXM_NX_REG5[],load:0x284->NXM_NX_REG6[],resubmit(,81)
+ table=0,  priority=90,dl_vlan=0x284,dl_dst=fa:16:3e:a4:22:10 actions=load:0x1->NXM_NX_REG5[],load:0x284->NXM_NX_REG6[],resubmit(,81)
+ table=0,  priority=90,dl_vlan=0x284,dl_dst=fa:16:3e:24:57:c7 actions=load:0x2->NXM_NX_REG5[],load:0x284->NXM_NX_REG6[],resubmit(,81)
  table=0,  priority=0 actions=NORMAL
 
 Following ``table 71`` implements arp spoofing protection, ip spoofing
@@ -264,8 +264,8 @@ remaining egress connections are sent to normal switching.
 
 ::
 
- table=73, priority=100,dl_dst=fa:16:3e:a4:22:10 actions=load:0x1->NXM_NX_REG5[],resubmit(,81)
- table=73, priority=100,dl_dst=fa:16:3e:24:57:c7 actions=load:0x2->NXM_NX_REG5[],resubmit(,81)
+ table=73, priority=100,reg6=0x284,dl_dst=fa:16:3e:a4:22:10 actions=load:0x1->NXM_NX_REG5[],resubmit(,81)
+ table=73, priority=100,reg6=0x284,dl_dst=fa:16:3e:24:57:c7 actions=load:0x2->NXM_NX_REG5[],resubmit(,81)
  table=73, priority=90,ct_state=+new-est,reg5=0x1 actions=ct(commit,zone=NXM_NX_REG6[0..15]),NORMAL
  table=73, priority=90,ct_state=+new-est,reg5=0x2 actions=ct(commit,zone=NXM_NX_REG6[0..15]),NORMAL
  table=73, priority=80,reg5=0x1 actions=NORMAL

neutron/agent/linux/openvswitch_firewall/firewall.py

@@ -408,6 +408,15 @@ class OVSFirewallDriver(firewall.FirewallDriver):
         for table in ovs_consts.OVS_FIREWALL_TABLES:
             self.int_br.br.add_flow(table=table, priority=0, actions='drop')
 
+    def get_ovs_port(self, port_id):
+        ovs_port = self.int_br.br.get_vif_port_by_id(port_id)
+        if not ovs_port:
+            raise exceptions.OVSFWPortNotFound(port_id=port_id)
+        return ovs_port
+
+    def _get_port_vlan_tag(self, port_name):
+        return get_tag_from_other_config(self.int_br.br, port_name)
+
     def get_ofport(self, port):
         port_id = port['device']
         return self.sg_port_map.ports.get(port_id)
@@ -418,15 +427,11 @@ class OVSFirewallDriver(firewall.FirewallDriver):
         If ofport is nonexistent, create and return one.
         """
         port_id = port['device']
-        ovs_port = self.int_br.br.get_vif_port_by_id(port_id)
-        if not ovs_port:
-            raise exceptions.OVSFWPortNotFound(port_id=port_id)
-
+        ovs_port = self.get_ovs_port(port_id)
         try:
             of_port = self.sg_port_map.ports[port_id]
         except KeyError:
-            port_vlan_id = get_tag_from_other_config(
-                self.int_br.br, ovs_port.port_name)
+            port_vlan_id = self._get_port_vlan_tag(ovs_port.port_name)
             of_port = OFPort(port, ovs_port, port_vlan_id)
             self.sg_port_map.create_port(of_port, port)
         else:
@@ -442,6 +447,7 @@ class OVSFirewallDriver(firewall.FirewallDriver):
 
     def prepare_port_filter(self, port):
         if not firewall.port_sec_enabled(port):
+            self._initialize_egress_no_port_security(port['device'])
             return
         old_of_port = self.get_ofport(port)
         of_port = self.get_or_create_ofport(port)
@@ -462,8 +468,10 @@ class OVSFirewallDriver(firewall.FirewallDriver):
         """
         if not firewall.port_sec_enabled(port):
             self.remove_port_filter(port)
+            self._initialize_egress_no_port_security(port['device'])
             return
         elif not self.is_port_managed(port):
+            self._remove_egress_no_port_security(port['device'])
             self.prepare_port_filter(port)
             return
         old_of_port = self.get_ofport(port)
@@ -519,6 +527,15 @@ class OVSFirewallDriver(firewall.FirewallDriver):
             self.conj_ip_manager.sg_removed(sg_id)
             self.sg_port_map.delete_sg(sg_id)
 
+    def process_trusted_ports(self, port_ids):
+        """Pass packets from these ports directly to ingress pipeline."""
+        for port_id in port_ids:
+            self._initialize_egress_no_port_security(port_id)
+
+    def remove_trusted_ports(self, port_ids):
+        for port_id in port_ids:
+            self._remove_egress_no_port_security(port_id)
+
     def filter_defer_apply_on(self):
         self._deferred = True
 
@@ -559,6 +576,7 @@ class OVSFirewallDriver(firewall.FirewallDriver):
             table=ovs_consts.TRANSIENT_TABLE,
             priority=90,
             dl_dst=port.mac,
+            dl_vlan='0x%x' % port.vlan_tag,
             actions='set_field:{:d}->reg{:d},'
                     'set_field:{:d}->reg{:d},'
                     'strip_vlan,resubmit(,{:d})'.format(
@@ -585,6 +603,50 @@ class OVSFirewallDriver(firewall.FirewallDriver):
                 actions='normal'
             )
 
+    def _initialize_egress_no_port_security(self, port_id):
+        ovs_port = self.get_ovs_port(port_id)
+        try:
+            vlan_tag = self._get_port_vlan_tag(ovs_port.port_name)
+        except exceptions.OVSFWTagNotFound:
+            # It's a patch port, don't set anything
+            return
+        self._add_flow(
+            table=ovs_consts.TRANSIENT_TABLE,
+            priority=100,
+            in_port=ovs_port.ofport,
+            actions='set_field:%d->reg%d,'
+                    'set_field:%d->reg%d,'
+                    'resubmit(,%d)' % (
+                        ovs_port.ofport,
+                        ovsfw_consts.REG_PORT,
+                        vlan_tag,
+                        ovsfw_consts.REG_NET,
+                        ovs_consts.ACCEPT_OR_INGRESS_TABLE)
+        )
+        self._add_flow(
+            table=ovs_consts.ACCEPT_OR_INGRESS_TABLE,
+            priority=80,
+            reg_port=ovs_port.ofport,
+            actions='normal'
+        )
+
+    def _remove_egress_no_port_security(self, port_id):
+        ovs_port = self.get_ovs_port(port_id)
+        try:
+            # Test if it's a patch port
+            self._get_port_vlan_tag(ovs_port.port_name)
+        except exceptions.OVSFWTagNotFound:
+            # It's a patch port, don't do anything
+            return
+        self._delete_flows(
+            table=ovs_consts.TRANSIENT_TABLE,
+            in_port=ovs_port.ofport
+        )
+        self._delete_flows(
+            table=ovs_consts.ACCEPT_OR_INGRESS_TABLE,
+            reg_port=ovs_port.ofport
+        )
+
     def _initialize_egress(self, port):
         """Identify egress traffic and send it to egress base"""
         self._initialize_egress_ipv6_icmp(port)
@@ -695,6 +757,7 @@ class OVSFirewallDriver(firewall.FirewallDriver):
             table=ovs_consts.ACCEPT_OR_INGRESS_TABLE,
             priority=100,
             dl_dst=port.mac,
+            reg_net=port.vlan_tag,
             actions='set_field:{:d}->reg{:d},resubmit(,{:d})'.format(
                 port.ofport,
                 ovsfw_consts.REG_PORT,
@@ -935,14 +998,15 @@ class OVSFirewallDriver(firewall.FirewallDriver):
         self._strict_delete_flow(
             priority=90,
             table=ovs_consts.TRANSIENT_TABLE,
-            dl_dst=port.mac)
+            dl_dst=port.mac,
+            dl_vlan=port.vlan_tag)
         self._strict_delete_flow(
             priority=100,
             table=ovs_consts.TRANSIENT_TABLE,
             in_port=port.ofport)
         self._delete_flows(reg_port=port.ofport)
         self._delete_flows(table=ovs_consts.ACCEPT_OR_INGRESS_TABLE,
-                           dl_dst=port.mac)
+                           dl_dst=port.mac, reg_net=port.vlan_tag)
 
     def delete_flows_for_ip_addresses(
             self, ip_addresses, direction, ethertype, vlan_tag):

neutron/tests/common/conn_testers.py

@@ -22,6 +22,8 @@ from oslo_utils import uuidutils
 from neutron.agent import firewall
 from neutron.common import constants as n_consts
 from neutron.common import utils as common_utils
+from neutron.plugins.ml2.drivers.openvswitch.agent.common import (
+    constants as ovs_consts)
 from neutron.plugins.ml2.drivers.openvswitch.agent.openflow.ovs_ofctl import (
     br_int)
 from neutron.tests.common import machine_fixtures
@@ -360,6 +362,12 @@ class ConnectionTester(fixtures.Fixture):
                 "At least one packet got reply from %s namespace to %s "
                 "address." % self._get_namespace_and_address(direction)))
 
+    def set_peer_port_as_patch_port(self):
+        pass
+
+    def set_peer_port_as_vm_port(self):
+        pass
+
 
 class OVSBaseConnectionTester(ConnectionTester):
 
@@ -419,9 +427,47 @@ class OVSConnectionTester(OVSBaseConnectionTester):
 
     def set_vm_tag(self, tag):
         self.set_tag(self._vm.port.name, self.bridge, tag)
+        self._vm.port.vlan_tag = tag
 
     def set_peer_tag(self, tag):
         self.set_tag(self._peer.port.name, self.bridge, tag)
+        self._peer.port.vlan_tag = tag
+
+    def set_peer_port_as_patch_port(self):
+        """As packets coming from tunneling bridges are always tagged with
+        local VLAN tag, this flows will simulate the behavior.
+        """
+        self.bridge.add_flow(
+            table=ovs_consts.LOCAL_SWITCHING,
+            priority=110,
+            vlan_tci=0,
+            in_port=self.bridge.get_port_ofport(self._peer.port.name),
+            actions='mod_vlan_vid:0x%x,'
+                    'resubmit(,%d)' % (
+                        self._peer.port.vlan_tag,
+                        ovs_consts.LOCAL_SWITCHING)
+        )
+        self.bridge.add_flow(
+            table=ovs_consts.TRANSIENT_TABLE,
+            priority=4,
+            dl_vlan='0x%x' % self._peer.port.vlan_tag,
+            actions='strip_vlan,normal'
+        )
+
+    def set_peer_port_as_vm_port(self):
+        """Remove flows simulating traffic from tunneling bridges.
+
+        This method is opposite to set_peer_port_as_patch_port().
+        """
+        self.bridge.delete_flows(
+            table=ovs_consts.LOCAL_SWITCHING,
+            vlan_tci=0,
+            in_port=self.bridge.get_port_ofport(self._peer.port.name),
+        )
+        self.bridge.delete_flows(
+            table=ovs_consts.TRANSIENT_TABLE,
+            dl_vlan='0x%x' % self._peer.port.vlan_tag,
+        )
 
 
 class OVSTrunkConnectionTester(OVSBaseConnectionTester):

neutron/tests/common/net_helpers.py

@@ -737,6 +737,7 @@ class OVSPortFixture(PortFixture):
                  hybrid_plug=False):
         super(OVSPortFixture, self).__init__(bridge, namespace, mac, port_id)
         self.hybrid_plug = hybrid_plug
+        self.vlan_tag = None
 
     def _create_bridge_fixture(self):
         return OVSBridgeFixture()

neutron/tests/fullstack/test_securitygroup.py

@@ -111,6 +111,8 @@ class TestSecurityGroupsSameNetwork(BaseSecurityGroupsSameNetworkTest):
              5. a remote security group member addition works, and
              6. an established connection stops by deleting a SG rule.
              7. test other protocol functionality by using SCTP protocol
+             8. test two vms with same mac on the same host in different
+                networks
         """
         index_to_sg = [0, 0, 1]
         if self.firewall_driver == 'iptables_hybrid':
@@ -272,3 +274,100 @@ class TestSecurityGroupsSameNetwork(BaseSecurityGroupsSameNetworkTest):
         self.assert_connection(
             vms[1].namespace, vms[0].namespace, vms[0].ip, 3366,
             net_helpers.NetcatTester.SCTP)
+
+        # 8. test two vms with same mac on the same host in different networks
+        self._test_overlapping_mac_addresses()
+
+    def _create_vm_on_host(
+            self, project_id, network_id, sg_id, host, mac_address=None):
+        if mac_address:
+            port = self.safe_client.create_port(
+                project_id, network_id, host.hostname,
+                security_groups=[sg_id], mac_address=mac_address)
+        else:
+            port = self.safe_client.create_port(
+                project_id, network_id, host.hostname,
+                security_groups=[sg_id])
+
+        return self.useFixture(
+            machine.FakeFullstackMachine(
+                host, network_id, project_id, self.safe_client,
+                neutron_port=port))
+
+    def _create_three_vms_first_has_static_mac(
+            self, project_id, allowed_port, subnet_cidr):
+        """Create three vms.
+
+        First VM has a static mac and is placed on first host. Second VM is
+        placed on the first host and third VM is placed on second host.
+        """
+        network = self.safe_client.create_network(project_id)
+        self.safe_client.create_subnet(
+            project_id, network['id'], subnet_cidr)
+        sg = self.safe_client.create_security_group(project_id)
+
+        self.safe_client.create_security_group_rule(
+            project_id, sg['id'],
+            direction='ingress',
+            ethertype=constants.IPv4,
+            protocol=constants.PROTO_NAME_TCP,
+            port_range_min=allowed_port, port_range_max=allowed_port)
+
+        vms = [self._create_vm_on_host(
+            project_id, network['id'], sg['id'], self.environment.hosts[0],
+            mac_address="fa:16:3e:de:ad:fe")]
+
+        if self.firewall_driver == 'iptables_hybrid':
+            # iptables lack isolation between agents, use only a single host
+            vms.extend([
+                self._create_vm_on_host(
+                    project_id, network['id'], sg['id'],
+                    self.environment.hosts[0])
+                for _ in range(2)])
+        else:
+            vms.extend([
+                self._create_vm_on_host(
+                    project_id, network['id'], sg['id'], host)
+                for host in self.environment.hosts[:2]])
+
+        map(lambda vm: vm.block_until_boot(), vms)
+        return vms
+
+    def verify_connectivity_between_vms(self, src_vm, dst_vm, protocol, port):
+        self.assert_connection(
+            src_vm.namespace, dst_vm.namespace, dst_vm.ip, port,
+            protocol)
+
+    def verify_no_connectivity_between_vms(
+            self, src_vm, dst_vm, protocol, port):
+        self.assert_no_connection(
+            src_vm.namespace, dst_vm.namespace, dst_vm.ip, port, protocol)
+
+    def _test_overlapping_mac_addresses(self):
+        project1 = uuidutils.generate_uuid()
+        p1_allowed = 4444
+
+        project2 = uuidutils.generate_uuid()
+        p2_allowed = 4445
+
+        p1_vms = self._create_three_vms_first_has_static_mac(
+            project1, p1_allowed, '20.0.2.0/24')
+        p2_vms = self._create_three_vms_first_has_static_mac(
+            project2, p2_allowed, '20.0.3.0/24')
+
+        have_connectivity = [
+            (p1_vms[0], p1_vms[1], p1_allowed),
+            (p1_vms[1], p1_vms[2], p1_allowed),
+            (p2_vms[0], p2_vms[1], p2_allowed),
+            (p2_vms[1], p2_vms[2], p2_allowed),
+        ]
+
+        for vm1, vm2, port in have_connectivity:
+            self.verify_connectivity_between_vms(
+                vm1, vm2, net_helpers.NetcatTester.TCP, port)
+            self.verify_connectivity_between_vms(
+                vm2, vm1, net_helpers.NetcatTester.TCP, port)
+            self.verify_no_connectivity_between_vms(
+                vm1, vm2, net_helpers.NetcatTester.TCP, port + 1)
+            self.verify_no_connectivity_between_vms(
+                vm2, vm1, net_helpers.NetcatTester.TCP, port + 1)

neutron/tests/functional/agent/test_firewall.py

@@ -111,6 +111,8 @@ class BaseFirewallTestCase(base.BaseSudoTestCase):
         # FIXME(jlibosva): We should consider to call prepare_port_filter with
         # deferred bridge depending on its performance
         self.firewall.prepare_port_filter(self.src_port_desc)
+        # Traffic coming from patch-port is always VLAN tagged
+        self.tester.set_peer_port_as_patch_port()
 
     def initialize_iptables(self):
         cfg.CONF.set_override('enable_ipset', self.enable_ipset,
@@ -542,6 +544,8 @@ class FirewallTestCase(BaseFirewallTestCase):
         self.assertEqual(packets_received, 0)
 
     def test_remote_security_groups(self):
+        self.tester.set_peer_port_as_vm_port()
+
         remote_sg_id = 'remote_sg_id'
         peer_port_desc = self._create_port_description(
             self.tester.peer_port_id,

neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py

@@ -476,6 +476,7 @@ class TestOVSFirewallDriver(base.BaseTestCase):
                         self.port_ofport, TESTING_VLAN_TAG,
                         ovs_consts.BASE_INGRESS_TABLE),
             dl_dst=self.port_mac,
+            dl_vlan='0x%x' % TESTING_VLAN_TAG,
             priority=90,
             table=ovs_consts.TRANSIENT_TABLE)
         filter_rule = mock.call(
@@ -498,8 +499,10 @@ class TestOVSFirewallDriver(base.BaseTestCase):
                      'security_groups': [1],
                      'port_security_enabled': False}
         self._prepare_security_group()
-        self.firewall.prepare_port_filter(port_dict)
-        self.assertFalse(self.mock_bridge.br.add_flow.called)
+        with mock.patch.object(
+                self.firewall, 'initialize_port_flows') as m_init_flows:
+            self.firewall.prepare_port_filter(port_dict)
+        self.assertFalse(m_init_flows.called)
 
     def test_prepare_port_filter_initialized_port(self):
         port_dict = {'device': 'port-id',
@@ -602,3 +605,51 @@ class TestOVSFirewallDriver(base.BaseTestCase):
             self.firewall._cleanup_stale_sg()
             sg_removed_mock.assert_called_once_with(1)
             delete_sg_mock.assert_called_once_with(1)
+
+    def test_get_ovs_port(self):
+        ovs_port = self.firewall.get_ovs_port('port_id')
+        self.assertEqual(self.fake_ovs_port, ovs_port)
+
+    def test_get_ovs_port_non_existent(self):
+        self.mock_bridge.br.get_vif_port_by_id.return_value = None
+        with testtools.ExpectedException(exceptions.OVSFWPortNotFound):
+            self.firewall.get_ovs_port('port_id')
+
+    def test__initialize_egress_no_port_security_sends_to_egress(self):
+        self.mock_bridge.br.db_get_val.return_value = {'tag': TESTING_VLAN_TAG}
+        self.firewall._initialize_egress_no_port_security('port_id')
+        expected_call = mock.call(
+            table=ovs_consts.TRANSIENT_TABLE,
+            priority=100,
+            in_port=self.fake_ovs_port.ofport,
+            actions='set_field:%d->reg%d,'
+                    'set_field:%d->reg%d,'
+                    'resubmit(,%d)' % (
+                        self.fake_ovs_port.ofport,
+                        ovsfw_consts.REG_PORT,
+                        TESTING_VLAN_TAG,
+                        ovsfw_consts.REG_NET,
+                        ovs_consts.ACCEPT_OR_INGRESS_TABLE)
+        )
+        calls = self.mock_bridge.br.add_flow.call_args_list
+        self.assertIn(expected_call, calls)
+
+    def test__initialize_egress_no_port_security_no_tag(self):
+        self.mock_bridge.br.db_get_val.return_value = {}
+        self.firewall._initialize_egress_no_port_security('port_id')
+        self.assertFalse(self.mock_bridge.br.add_flow.called)
+
+    def test__remove_egress_no_port_security_deletes_flow(self):
+        self.mock_bridge.br.db_get_val.return_value = {'tag': TESTING_VLAN_TAG}
+        self.firewall._remove_egress_no_port_security('port_id')
+        expected_call = mock.call(
+            table=ovs_consts.TRANSIENT_TABLE,
+            in_port=self.fake_ovs_port.ofport,
+        )
+        calls = self.mock_bridge.br.delete_flows.call_args_list
+        self.assertIn(expected_call, calls)
+
+    def test__remove_egress_no_port_security_no_tag(self):
+        self.mock_bridge.br.db_get_val.return_value = {}
+        self.firewall._remove_egress_no_port_security('port_id')
+        self.assertFalse(self.mock_bridge.br.delete_flows.called)