From bd6203b2c7e1e4af63813b307bc4ec1b49516bd5 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. Conflicts: neutron/tests/unit/agent/linux/openvswitch_firewall/test_rules.py 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 798fde8b4b8..19bdf92289b 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 3eb7a5f1570..c0a16aa4cae 100644 --- a/neutron/agent/linux/iptables_firewall.py +++ b/neutron/agent/linux/iptables_firewall.py @@ -540,7 +540,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 9a44f64316e..0d57115a3b7 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 cc9fbb60422..dec334d8114 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 fd2f3aa2e64..704a405d76c 100644 --- a/neutron/db/securitygroups_rpc_base.py +++ b/neutron/db/securitygroups_rpc_base.py @@ -219,7 +219,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 @@ -248,7 +248,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() @@ -391,9 +392,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 @@ -405,8 +408,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 dc7b0edf147..b9bab95e6f5 100644 --- a/neutron/tests/functional/agent/test_firewall.py +++ b/neutron/tests/functional/agent/test_firewall.py @@ -585,7 +585,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 0a84b10bb70..6aeb7a45341 100644 --- a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py +++ b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py @@ -324,9 +324,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 @@ -339,7 +340,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 e360b8ac4fc..c659ff51460 100644 --- a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_rules.py +++ b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_rules.py @@ -351,7 +351,8 @@ class TestCreateFlowsForIpAddress(base.BaseTestCase): conj_ids = [12, 20] flows = rules.create_flows_for_ip_address( - '192.168.0.1', firewall.EGRESS_DIRECTION, constants.IPv4, + ('192.168.0.1', 'fa:16:3e:aa:bb:cc'), + firewall.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 8cb068c9e90..a46f67ffa10 100644 --- a/neutron/tests/unit/agent/linux/test_iptables_firewall.py +++ b/neutron/tests/unit/agent/linux/test_iptables_firewall.py @@ -1492,12 +1492,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}): @@ -2269,10 +2273,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) @@ -2281,7 +2287,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 451aefb6728..d4f75c594e7 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 d7ea4e8887f..c36160af39b 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 @@ -126,19 +127,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