From 18a171808ba8c5e4806feda43f571fb8cfc0b9a1 Mon Sep 17 00:00:00 2001 From: LIU Yulong Date: Fri, 13 Mar 2020 18:18:04 +0800 Subject: [PATCH] [Security] fix allowed-address-pair 0.0.0.0/0 issue When add allowed-address-pair 0.0.0.0/0 to one port, it will unexpectedly open all others' protocol under same security group. IPv6 has the same problem. The root cause is the openflow rules calculation of the security group, it will unexpectedly allow all IP(4&6) traffic to get through. For openvswitch openflow firewall, this patch adds a source mac address match for the allowed-address-pair which has prefix lenght 0, that means all ethernet packets from this mac will be accepted. It exactly will meet the request of accepting any IP address from the configured VM. Test result shows that the remote security group and allowed address pair works: 1. Port has 0.0.0.0/0 allowed-address-pair clould send any IP (src) packet out. 2. Port has x.x.x.x/y allowed-address-pair could be accepted for those VMs under same security group. 3. Ports under same network can reach each other (remote security group). 4. Protocol port number could be accessed only when there has related rule. Closes-bug: #1867119 Change-Id: I2e3aa7c400d7bb17cc117b65faaa160b41013dde (cherry picked from commit 00298fe6e84cd7610b39af50e9517885a182f47c) --- neutron/agent/linux/ipset_manager.py | 2 +- neutron/agent/linux/iptables_firewall.py | 2 +- .../agent/linux/openvswitch_firewall/rules.py | 6 +++++ .../api/rpc/handlers/securitygroups_rpc.py | 6 +++-- neutron/db/securitygroups_rpc_base.py | 23 ++++++++++++------- .../tests/functional/agent/test_firewall.py | 3 ++- .../openvswitch_firewall/test_firewall.py | 7 +++--- .../linux/openvswitch_firewall/test_rules.py | 3 ++- .../unit/agent/linux/test_ipset_manager.py | 18 ++++++++++----- .../agent/linux/test_iptables_firewall.py | 22 +++++++++++------- .../unit/agent/test_securitygroups_rpc.py | 22 +++++++++++++----- .../rpc/handlers/test_securitygroups_rpc.py | 22 ++++++++++++++---- 12 files changed, 94 insertions(+), 42 deletions(-) diff --git a/neutron/agent/linux/ipset_manager.py b/neutron/agent/linux/ipset_manager.py index 0fd81cf45fe..eb43e62c645 100644 --- a/neutron/agent/linux/ipset_manager.py +++ b/neutron/agent/linux/ipset_manager.py @@ -44,7 +44,7 @@ class IpsetManager(object): /1's to represent the /0. """ sanitized_addresses = [] - for ip in addresses: + for ip, _mac in addresses: ip = netaddr.IPNetwork(ip) if ip.prefixlen == 0: if ip.version == 4: diff --git a/neutron/agent/linux/iptables_firewall.py b/neutron/agent/linux/iptables_firewall.py index 3bbaeaab215..2f1458d7245 100644 --- a/neutron/agent/linux/iptables_firewall.py +++ b/neutron/agent/linux/iptables_firewall.py @@ -535,7 +535,7 @@ class IptablesFirewallDriver(firewall.FirewallDriver): ethertype = rule['ethertype'] port_ips = port.get('fixed_ips', []) - for ip in self.sg_members[remote_group_id][ethertype]: + for ip, _mac in self.sg_members[remote_group_id][ethertype]: if ip not in port_ips: ip_rule = rule.copy() direction_ip_prefix = firewall.DIRECTION_IP_PREFIX[ diff --git a/neutron/agent/linux/openvswitch_firewall/rules.py b/neutron/agent/linux/openvswitch_firewall/rules.py index 86f2ad70d61..94bbd874de3 100644 --- a/neutron/agent/linux/openvswitch_firewall/rules.py +++ b/neutron/agent/linux/openvswitch_firewall/rules.py @@ -297,6 +297,9 @@ def create_flows_for_ip_address(ip_address, direction, ethertype, """Create flows from a rule and an ip_address derived from remote_group_id """ + ip_address, mac_address = ip_address + net = netaddr.IPNetwork(str(ip_address)) + any_src_ip = net.prefixlen == 0 # Group conj_ids per priority. conj_id_lists = [[] for i in range(4)] @@ -321,6 +324,9 @@ def create_flows_for_ip_address(ip_address, direction, ethertype, flow_template[FLOW_FIELD_FOR_IPVER_AND_DIRECTION[( ip_ver, direction)]] = ip_prefix + if any_src_ip: + flow_template['dl_src'] = mac_address + result = [] for offset, conj_id_list in enumerate(conj_id_lists): if not conj_id_list: diff --git a/neutron/api/rpc/handlers/securitygroups_rpc.py b/neutron/api/rpc/handlers/securitygroups_rpc.py index 7b03e5adcf5..f97653c75da 100644 --- a/neutron/api/rpc/handlers/securitygroups_rpc.py +++ b/neutron/api/rpc/handlers/securitygroups_rpc.py @@ -326,8 +326,10 @@ class SecurityGroupServerAPIShim(sg_rpc_base.SecurityGroupInfoAPIMixin): filters = {'security_group_ids': tuple(remote_group_ids)} for p in self.rcache.get_resources('Port', filters): - port_ips = [str(addr.ip_address) - for addr in p.fixed_ips + p.allowed_address_pairs] + allowed_ips = [(str(addr.ip_address), str(addr.mac_address)) + for addr in p.allowed_address_pairs] + port_ips = [(str(addr.ip_address), str(p.mac_address)) + for addr in p.fixed_ips] + allowed_ips for sg_id in p.security_group_ids: if sg_id in ips_by_group: ips_by_group[sg_id].update(set(port_ips)) diff --git a/neutron/db/securitygroups_rpc_base.py b/neutron/db/securitygroups_rpc_base.py index 858a9461264..369c15fd0f8 100644 --- a/neutron/db/securitygroups_rpc_base.py +++ b/neutron/db/securitygroups_rpc_base.py @@ -224,7 +224,7 @@ class SecurityGroupInfoAPIMixin(object): context, sg_info['sg_member_ips'].keys()) for sg_id, member_ips in ips.items(): for ip in member_ips: - ethertype = 'IPv%d' % netaddr.IPNetwork(ip).version + ethertype = 'IPv%d' % netaddr.IPNetwork(ip[0]).version if ethertype in sg_info['sg_member_ips'][sg_id]: sg_info['sg_member_ips'][sg_id][ethertype].add(ip) return sg_info @@ -253,7 +253,8 @@ class SecurityGroupInfoAPIMixin(object): port['security_group_source_groups'].append(remote_group_id) base_rule = rule - for ip in ips[remote_group_id]: + ip_list = [ip[0] for ip in ips[remote_group_id]] + for ip in ip_list: if ip in port.get('fixed_ips', []): continue ip_rule = base_rule.copy() @@ -396,9 +397,11 @@ class SecurityGroupServerRpcMixin(SecurityGroupInfoAPIMixin, # Join the security group binding table directly to the IP allocation # table instead of via the Port table skip an unnecessary intermediary - query = context.session.query(sg_binding_sgid, - models_v2.IPAllocation.ip_address, - aap_models.AllowedAddressPair.ip_address) + query = context.session.query( + sg_binding_sgid, + models_v2.IPAllocation.ip_address, + aap_models.AllowedAddressPair.ip_address, + aap_models.AllowedAddressPair.mac_address) query = query.join(models_v2.IPAllocation, ip_port == sg_binding_port) # Outerjoin because address pairs may be null and we still want the @@ -410,8 +413,12 @@ class SecurityGroupServerRpcMixin(SecurityGroupInfoAPIMixin, # Each allowed address pair IP record for a port beyond the 1st # will have a duplicate regular IP in the query response since # the relationship is 1-to-many. Dedup with a set - for security_group_id, ip_address, allowed_addr_ip in query: - ips_by_group[security_group_id].add(ip_address) + for security_group_id, ip_address, allowed_addr_ip, mac in query: + # Since port mac will not be used further, but in order to align + # the data structure we directly set None to it to avoid bother + # the ports table. + ips_by_group[security_group_id].add((ip_address, None)) if allowed_addr_ip: - ips_by_group[security_group_id].add(allowed_addr_ip) + ips_by_group[security_group_id].add( + (allowed_addr_ip, mac)) return ips_by_group diff --git a/neutron/tests/functional/agent/test_firewall.py b/neutron/tests/functional/agent/test_firewall.py index f3e9168405c..2f23d824418 100644 --- a/neutron/tests/functional/agent/test_firewall.py +++ b/neutron/tests/functional/agent/test_firewall.py @@ -581,7 +581,8 @@ class FirewallTestCase(BaseFirewallTestCase): [remote_sg_id], self.net_id) - vm_sg_members = {'IPv4': [self.tester.peer_ip_address]} + vm_sg_members = {'IPv4': [ + (self.tester.peer_ip_address, self.tester.peer_mac_address)]} peer_sg_rules = [{'ethertype': 'IPv4', 'direction': 'egress', 'protocol': 'icmp'}] self.firewall.update_security_group_rules(remote_sg_id, peer_sg_rules) 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 a7b353a3f17..4c2dcfa81e5 100644 --- a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py +++ b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py @@ -323,9 +323,10 @@ class TestConjIPFlowManager(base.BaseTestCase): def test_update_flows_for_vlan_no_ports_but_members(self): remote_group = self.driver.sg_port_map.get_sg.return_value remote_group.ports = set() - remote_group.members = {constants.IPv4: ['10.22.3.4']} + remote_group.members = {constants.IPv4: [ + ('10.22.3.4', 'fa:16:3e:aa:bb:cc'), ]} remote_group.get_ethertype_filtered_addresses.return_value = [ - '10.22.3.4'] + ('10.22.3.4', 'fa:16:3e:aa:bb:cc'), ] with mock.patch.object(self.manager.conj_id_map, 'get_conj_id') as get_conj_id_mock: get_conj_id_mock.return_value = self.conj_id @@ -338,7 +339,7 @@ class TestConjIPFlowManager(base.BaseTestCase): def test_update_flows_for_vlan(self): remote_group = self.driver.sg_port_map.get_sg.return_value remote_group.get_ethertype_filtered_addresses.return_value = [ - '10.22.3.4'] + ('10.22.3.4', 'fa:16:3e:aa:bb:cc'), ] with mock.patch.object(self.manager.conj_id_map, 'get_conj_id') as get_conj_id_mock: get_conj_id_mock.return_value = self.conj_id diff --git a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_rules.py b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_rules.py index 5f0dec8a6d6..59cbbb07913 100644 --- a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_rules.py +++ b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_rules.py @@ -350,7 +350,8 @@ class TestCreateFlowsForIpAddress(base.BaseTestCase): conj_ids = [12, 20] flows = rules.create_flows_for_ip_address( - '192.168.0.1', constants.EGRESS_DIRECTION, constants.IPv4, + ('192.168.0.1', 'fa:16:3e:aa:bb:cc'), + constants.EGRESS_DIRECTION, constants.IPv4, 0x123, conj_ids) self.assertEqual(2, len(flows)) diff --git a/neutron/tests/unit/agent/linux/test_ipset_manager.py b/neutron/tests/unit/agent/linux/test_ipset_manager.py index bb7984638dd..5eca4949284 100644 --- a/neutron/tests/unit/agent/linux/test_ipset_manager.py +++ b/neutron/tests/unit/agent/linux/test_ipset_manager.py @@ -20,8 +20,12 @@ TEST_SET_ID = 'fake_sgid' ETHERTYPE = 'IPv4' TEST_SET_NAME = ipset_manager.IpsetManager.get_name(TEST_SET_ID, ETHERTYPE) TEST_SET_NAME_NEW = TEST_SET_NAME + ipset_manager.SWAP_SUFFIX -FAKE_IPS = ['10.0.0.1', '10.0.0.2', '10.0.0.3', '10.0.0.4', - '10.0.0.5', '10.0.0.6'] +FAKE_IPS = [('10.0.0.1', 'fa:16:3e:aa:bb:c1'), + ('10.0.0.2', 'fa:16:3e:aa:bb:c2'), + ('10.0.0.3', 'fa:16:3e:aa:bb:c3'), + ('10.0.0.4', 'fa:16:3e:aa:bb:c4'), + ('10.0.0.5', 'fa:16:3e:aa:bb:c5'), + ('10.0.0.6', 'fa:16:3e:aa:bb:c6')] class BaseIpsetManagerTest(base.BaseTestCase): @@ -149,13 +153,15 @@ class IpsetManagerTestCase(BaseIpsetManagerTest): self.verify_mock_calls() def test_set_members_adding_all_zero_ipv4(self): - self.expect_set(['0.0.0.0/0']) - self.ipset.set_members(TEST_SET_ID, ETHERTYPE, ['0.0.0.0/0']) + self.expect_set([('0.0.0.0/0', 'fa:16:3e:aa:bb:c1'), ]) + self.ipset.set_members(TEST_SET_ID, ETHERTYPE, + [('0.0.0.0/0', 'fa:16:3e:aa:bb:c1'), ]) self.verify_mock_calls() def test_set_members_adding_all_zero_ipv6(self): - self.expect_set(['::/0']) - self.ipset.set_members(TEST_SET_ID, ETHERTYPE, ['::/0']) + self.expect_set([('::/0', 'fa:16:3e:aa:bb:c1'), ]) + self.ipset.set_members(TEST_SET_ID, ETHERTYPE, + [('::/0', 'fa:16:3e:aa:bb:c1'), ]) self.verify_mock_calls() def test_destroy(self): diff --git a/neutron/tests/unit/agent/linux/test_iptables_firewall.py b/neutron/tests/unit/agent/linux/test_iptables_firewall.py index 2400c181400..45713034595 100644 --- a/neutron/tests/unit/agent/linux/test_iptables_firewall.py +++ b/neutron/tests/unit/agent/linux/test_iptables_firewall.py @@ -1518,12 +1518,16 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase): if ethertype == "IPv4": ethertype = "ipv4" - members_add = {'IPv4': ['10.0.0.2', '10.0.0.3']} - members_after_delete = {'IPv4': ['10.0.0.3']} + members_add = {'IPv4': [('10.0.0.2', 'fa:16:3e:aa:bb:c1'), + ('10.0.0.3', 'fa:16:3e:aa:bb:c2')]} + members_after_delete = { + 'IPv4': [('10.0.0.3', 'fa:16:3e:aa:bb:c2'), ]} else: ethertype = "ipv6" - members_add = {'IPv6': ['fe80::2', 'fe80::3']} - members_after_delete = {'IPv6': ['fe80::3']} + members_add = {'IPv6': [('fe80::2', 'fa:16:3e:aa:bb:c3'), + ('fe80::3', 'fa:16:3e:aa:bb:c4')]} + members_after_delete = { + 'IPv6': [('fe80::3', 'fa:16:3e:aa:bb:c4'), ]} with mock.patch.dict(self.firewall.ipconntrack._device_zone_map, {port['network_id']: ct_zone}): @@ -2295,10 +2299,12 @@ class IptablesFirewallEnhancedIpsetTestCase(BaseIptablesFirewallTestCase): self.firewall.ipset.assert_has_calls(calls, True) def test_sg_rule_expansion_with_remote_ips(self): - other_ips = ['10.0.0.2', '10.0.0.3', '10.0.0.4'] + other_ips = [('10.0.0.2', 'fa:16:3e:aa:bb:c1'), + ('10.0.0.3', 'fa:16:3e:aa:bb:c2'), + ('10.0.0.4', 'fa:16:3e:aa:bb:c3')] self.firewall.sg_members = {'fake_sgid': { - 'IPv4': [FAKE_IP['IPv4']] + other_ips, - 'IPv6': [FAKE_IP['IPv6']]}} + 'IPv4': [(FAKE_IP['IPv4'], 'fa:16:3e:aa:bb:c4'), ] + other_ips, + 'IPv6': [(FAKE_IP['IPv6'], 'fa:16:3e:aa:bb:c5'), ]}} port = self._fake_port() rule = self._fake_sg_rule_for_ethertype(_IPv4, FAKE_SGID) @@ -2307,7 +2313,7 @@ class IptablesFirewallEnhancedIpsetTestCase(BaseIptablesFirewallTestCase): self.assertEqual(list(rules), [dict(list(rule.items()) + [('source_ip_prefix', '%s/32' % ip)]) - for ip in other_ips]) + for ip, _mac in other_ips]) def test_build_ipv4v6_mac_ip_list(self): mac_oth = 'ffff-ff0f-ffff' diff --git a/neutron/tests/unit/agent/test_securitygroups_rpc.py b/neutron/tests/unit/agent/test_securitygroups_rpc.py index e87d41ae74a..dd67b3669ac 100644 --- a/neutron/tests/unit/agent/test_securitygroups_rpc.py +++ b/neutron/tests/unit/agent/test_securitygroups_rpc.py @@ -317,8 +317,10 @@ class SGServerRpcCallBackTestCase(test_sg.SecurityGroupDBTestCase): sg_member_ips = self.rpc.security_group_info_for_devices( ctx, devices=devices)['sg_member_ips'] expected_member_ips = [ - '10.0.1.0/24', '11.0.0.1', - port['port']['fixed_ips'][0]['ip_address']] + ('10.0.1.0/24', '00:00:00:00:00:01'), + ('11.0.0.1', '00:00:00:00:00:01'), + (port['port']['fixed_ips'][0]['ip_address'], + None)] self.assertEqual(sorted(expected_member_ips), sorted(sg_member_ips[sg_id]['IPv4'])) self._delete('ports', port_id) @@ -522,7 +524,7 @@ class SGServerRpcCallBackTestCase(test_sg.SecurityGroupDBTestCase): 'remote_group_id': sg2_id} ]}, 'sg_member_ips': {sg2_id: { - 'IPv4': set([port_ip2]), + 'IPv4': set([(port_ip2, None), ]), 'IPv6': set(), }} } @@ -2947,7 +2949,9 @@ class TestSecurityGroupAgentEnhancedRpcWithIptables( self.devices_info1 = {'security_groups': {'security_group1': rule1}, 'sg_member_ips': { 'security_group1': { - 'IPv4': ['10.0.0.3/32'], 'IPv6': []}}, + 'IPv4': [('10.0.0.3/32', + 'fa:16:3e:aa:bb:c1'), ], + 'IPv6': []}}, 'devices': devices_info1} devices_info2 = collections.OrderedDict([ ('tap_port1', self._device('tap_port1', @@ -2962,13 +2966,19 @@ class TestSecurityGroupAgentEnhancedRpcWithIptables( self.devices_info2 = {'security_groups': {'security_group1': rule1}, 'sg_member_ips': { 'security_group1': { - 'IPv4': ['10.0.0.3/32', '10.0.0.4/32'], + 'IPv4': [('10.0.0.3/32', + 'fa:16:3e:aa:bb:c1'), + ('10.0.0.4/32', + 'fa:16:3e:aa:bb:c2')], 'IPv6': []}}, 'devices': devices_info2} self.devices_info3 = {'security_groups': {'security_group1': rule2}, 'sg_member_ips': { 'security_group1': { - 'IPv4': ['10.0.0.3/32', '10.0.0.4/32'], + 'IPv4': [('10.0.0.3/32', + 'fa:16:3e:aa:bb:c1'), + ('10.0.0.4/32', + 'fa:16:3e:aa:bb:c2')], 'IPv6': []}}, 'devices': devices_info2} diff --git a/neutron/tests/unit/api/rpc/handlers/test_securitygroups_rpc.py b/neutron/tests/unit/api/rpc/handlers/test_securitygroups_rpc.py index 6099327b367..339c80ca2a6 100644 --- a/neutron/tests/unit/api/rpc/handlers/test_securitygroups_rpc.py +++ b/neutron/tests/unit/api/rpc/handlers/test_securitygroups_rpc.py @@ -13,6 +13,7 @@ # under the License. import mock +import netaddr from neutron_lib import context from oslo_utils import uuidutils @@ -128,19 +129,30 @@ class SecurityGroupServerAPIShimTestCase(base.BaseTestCase): def test_security_group_info_for_devices(self): s1 = self._make_security_group_ovo() - p1 = self._make_port_ovo(ip='1.1.1.1', security_group_ids={s1.id}) + mac_1 = 'fa:16:3e:aa:bb:c1' + p1 = self._make_port_ovo(ip='1.1.1.1', + mac_address=netaddr.EUI(mac_1), + security_group_ids={s1.id}) + mac_2 = 'fa:16:3e:aa:bb:c2' p2 = self._make_port_ovo( ip='2.2.2.2', + mac_address=netaddr.EUI(mac_2), security_group_ids={s1.id}, security=psec.PortSecurity(port_security_enabled=False)) - p3 = self._make_port_ovo(ip='3.3.3.3', security_group_ids={s1.id}, + mac_3 = 'fa:16:3e:aa:bb:c3' + p3 = self._make_port_ovo(ip='3.3.3.3', + mac_address=netaddr.EUI(mac_3), + security_group_ids={s1.id}, device_owner='network:dhcp') ids = [p1.id, p2.id, p3.id] info = self.shim.security_group_info_for_devices(self.ctx, ids) - self.assertIn('1.1.1.1', info['sg_member_ips'][s1.id]['IPv4']) - self.assertIn('2.2.2.2', info['sg_member_ips'][s1.id]['IPv4']) - self.assertIn('3.3.3.3', info['sg_member_ips'][s1.id]['IPv4']) + self.assertIn(('1.1.1.1', str(netaddr.EUI(mac_1))), + info['sg_member_ips'][s1.id]['IPv4']) + self.assertIn(('2.2.2.2', str(netaddr.EUI(mac_2))), + info['sg_member_ips'][s1.id]['IPv4']) + self.assertIn(('3.3.3.3', str(netaddr.EUI(mac_3))), + info['sg_member_ips'][s1.id]['IPv4']) self.assertIn(p1.id, info['devices'].keys()) self.assertIn(p2.id, info['devices'].keys()) # P3 is a trusted port so it doesn't have rules