From 00298fe6e84cd7610b39af50e9517885a182f47c 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 --- 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 d3caf59784f..49b236f48dd 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 bd59ad617bd..246f5eef382 100644 --- a/neutron/agent/linux/iptables_firewall.py +++ b/neutron/agent/linux/iptables_firewall.py @@ -601,7 +601,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 32ebdac2799..d7ed32d06f9 100644 --- a/neutron/api/rpc/handlers/securitygroups_rpc.py +++ b/neutron/api/rpc/handlers/securitygroups_rpc.py @@ -327,8 +327,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 eeab00f6ffb..534db6cf017 100644 --- a/neutron/db/securitygroups_rpc_base.py +++ b/neutron/db/securitygroups_rpc_base.py @@ -228,7 +228,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 @@ -257,7 +257,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() @@ -408,9 +409,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 @@ -422,10 +425,14 @@ 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 @db_api.retry_if_session_inactive() diff --git a/neutron/tests/functional/agent/test_firewall.py b/neutron/tests/functional/agent/test_firewall.py index 3f0d96ce7c5..27a90cd41a3 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 229e603f960..e01db756fed 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 34140ad264a..d2b8daa5110 100644 --- a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_rules.py +++ b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_rules.py @@ -349,7 +349,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 698996f1e8e..070700fd017 100644 --- a/neutron/tests/unit/agent/linux/test_iptables_firewall.py +++ b/neutron/tests/unit/agent/linux/test_iptables_firewall.py @@ -1527,12 +1527,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}): @@ -2304,10 +2308,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) @@ -2316,7 +2322,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 503af21834a..3d6e5daf03a 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) @@ -525,7 +527,7 @@ class SGServerRpcCallBackTestCase(test_sg.SecurityGroupDBTestCase): 'stateful': True} ]}, 'sg_member_ips': {sg2_id: { - 'IPv4': set([port_ip2]), + 'IPv4': set([(port_ip2, None), ]), 'IPv6': set(), }} } @@ -2922,7 +2924,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', @@ -2937,13 +2941,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