From 6370a0471076ccb095a90f97ffc869ae7ea2e5ed Mon Sep 17 00:00:00 2001 From: Jakub Libosvar Date: Tue, 13 Jun 2017 12:07:28 +0000 Subject: [PATCH] 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 --- .../internals/openvswitch_firewall.rst | 8 +- .../linux/openvswitch_firewall/firewall.py | 80 +++++++++++++-- neutron/tests/common/conn_testers.py | 46 +++++++++ neutron/tests/common/net_helpers.py | 1 + neutron/tests/fullstack/test_securitygroup.py | 99 +++++++++++++++++++ .../tests/functional/agent/test_firewall.py | 4 + .../openvswitch_firewall/test_firewall.py | 55 ++++++++++- 7 files changed, 279 insertions(+), 14 deletions(-) diff --git a/doc/source/contributor/internals/openvswitch_firewall.rst b/doc/source/contributor/internals/openvswitch_firewall.rst index abcdd74e8fe..406ecc441a4 100644 --- a/doc/source/contributor/internals/openvswitch_firewall.rst +++ b/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 diff --git a/neutron/agent/linux/openvswitch_firewall/firewall.py b/neutron/agent/linux/openvswitch_firewall/firewall.py index c86bab4642e..71eebe24dfb 100644 --- a/neutron/agent/linux/openvswitch_firewall/firewall.py +++ b/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): diff --git a/neutron/tests/common/conn_testers.py b/neutron/tests/common/conn_testers.py index 82d705d8496..f59dc083ab6 100644 --- a/neutron/tests/common/conn_testers.py +++ b/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): diff --git a/neutron/tests/common/net_helpers.py b/neutron/tests/common/net_helpers.py index 0258434368b..f2fd878051e 100644 --- a/neutron/tests/common/net_helpers.py +++ b/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() diff --git a/neutron/tests/fullstack/test_securitygroup.py b/neutron/tests/fullstack/test_securitygroup.py index a18754f2606..e89346ac836 100644 --- a/neutron/tests/fullstack/test_securitygroup.py +++ b/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) diff --git a/neutron/tests/functional/agent/test_firewall.py b/neutron/tests/functional/agent/test_firewall.py index 349d54eca25..aa8d8707d3e 100644 --- a/neutron/tests/functional/agent/test_firewall.py +++ b/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, diff --git a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py index f37ff7dbf3b..153965a5d00 100644 --- a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py +++ b/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)