[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 00298fe6e8)
This commit is contained in:
LIU Yulong 2020-03-13 18:18:04 +08:00 committed by Slawek Kaplonski
parent 8181c5dbfe
commit 18a171808b
12 changed files with 94 additions and 42 deletions

View File

@ -44,7 +44,7 @@ class IpsetManager(object):
/1's to represent the /0. /1's to represent the /0.
""" """
sanitized_addresses = [] sanitized_addresses = []
for ip in addresses: for ip, _mac in addresses:
ip = netaddr.IPNetwork(ip) ip = netaddr.IPNetwork(ip)
if ip.prefixlen == 0: if ip.prefixlen == 0:
if ip.version == 4: if ip.version == 4:

View File

@ -535,7 +535,7 @@ class IptablesFirewallDriver(firewall.FirewallDriver):
ethertype = rule['ethertype'] ethertype = rule['ethertype']
port_ips = port.get('fixed_ips', []) 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: if ip not in port_ips:
ip_rule = rule.copy() ip_rule = rule.copy()
direction_ip_prefix = firewall.DIRECTION_IP_PREFIX[ direction_ip_prefix = firewall.DIRECTION_IP_PREFIX[

View File

@ -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 """Create flows from a rule and an ip_address derived from
remote_group_id 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. # Group conj_ids per priority.
conj_id_lists = [[] for i in range(4)] 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[( flow_template[FLOW_FIELD_FOR_IPVER_AND_DIRECTION[(
ip_ver, direction)]] = ip_prefix ip_ver, direction)]] = ip_prefix
if any_src_ip:
flow_template['dl_src'] = mac_address
result = [] result = []
for offset, conj_id_list in enumerate(conj_id_lists): for offset, conj_id_list in enumerate(conj_id_lists):
if not conj_id_list: if not conj_id_list:

View File

@ -326,8 +326,10 @@ class SecurityGroupServerAPIShim(sg_rpc_base.SecurityGroupInfoAPIMixin):
filters = {'security_group_ids': tuple(remote_group_ids)} filters = {'security_group_ids': tuple(remote_group_ids)}
for p in self.rcache.get_resources('Port', filters): for p in self.rcache.get_resources('Port', filters):
port_ips = [str(addr.ip_address) allowed_ips = [(str(addr.ip_address), str(addr.mac_address))
for addr in p.fixed_ips + p.allowed_address_pairs] 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: for sg_id in p.security_group_ids:
if sg_id in ips_by_group: if sg_id in ips_by_group:
ips_by_group[sg_id].update(set(port_ips)) ips_by_group[sg_id].update(set(port_ips))

View File

@ -224,7 +224,7 @@ class SecurityGroupInfoAPIMixin(object):
context, sg_info['sg_member_ips'].keys()) context, sg_info['sg_member_ips'].keys())
for sg_id, member_ips in ips.items(): for sg_id, member_ips in ips.items():
for ip in member_ips: 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]: if ethertype in sg_info['sg_member_ips'][sg_id]:
sg_info['sg_member_ips'][sg_id][ethertype].add(ip) sg_info['sg_member_ips'][sg_id][ethertype].add(ip)
return sg_info return sg_info
@ -253,7 +253,8 @@ class SecurityGroupInfoAPIMixin(object):
port['security_group_source_groups'].append(remote_group_id) port['security_group_source_groups'].append(remote_group_id)
base_rule = rule 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', []): if ip in port.get('fixed_ips', []):
continue continue
ip_rule = base_rule.copy() ip_rule = base_rule.copy()
@ -396,9 +397,11 @@ class SecurityGroupServerRpcMixin(SecurityGroupInfoAPIMixin,
# Join the security group binding table directly to the IP allocation # Join the security group binding table directly to the IP allocation
# table instead of via the Port table skip an unnecessary intermediary # table instead of via the Port table skip an unnecessary intermediary
query = context.session.query(sg_binding_sgid, query = context.session.query(
models_v2.IPAllocation.ip_address, sg_binding_sgid,
aap_models.AllowedAddressPair.ip_address) models_v2.IPAllocation.ip_address,
aap_models.AllowedAddressPair.ip_address,
aap_models.AllowedAddressPair.mac_address)
query = query.join(models_v2.IPAllocation, query = query.join(models_v2.IPAllocation,
ip_port == sg_binding_port) ip_port == sg_binding_port)
# Outerjoin because address pairs may be null and we still want the # 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 # Each allowed address pair IP record for a port beyond the 1st
# will have a duplicate regular IP in the query response since # will have a duplicate regular IP in the query response since
# the relationship is 1-to-many. Dedup with a set # the relationship is 1-to-many. Dedup with a set
for security_group_id, ip_address, allowed_addr_ip in query: for security_group_id, ip_address, allowed_addr_ip, mac in query:
ips_by_group[security_group_id].add(ip_address) # 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: 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 return ips_by_group

View File

@ -581,7 +581,8 @@ class FirewallTestCase(BaseFirewallTestCase):
[remote_sg_id], [remote_sg_id],
self.net_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', peer_sg_rules = [{'ethertype': 'IPv4', 'direction': 'egress',
'protocol': 'icmp'}] 'protocol': 'icmp'}]
self.firewall.update_security_group_rules(remote_sg_id, peer_sg_rules) self.firewall.update_security_group_rules(remote_sg_id, peer_sg_rules)

View File

@ -323,9 +323,10 @@ class TestConjIPFlowManager(base.BaseTestCase):
def test_update_flows_for_vlan_no_ports_but_members(self): def test_update_flows_for_vlan_no_ports_but_members(self):
remote_group = self.driver.sg_port_map.get_sg.return_value remote_group = self.driver.sg_port_map.get_sg.return_value
remote_group.ports = set() 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 = [ 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, with mock.patch.object(self.manager.conj_id_map,
'get_conj_id') as get_conj_id_mock: 'get_conj_id') as get_conj_id_mock:
get_conj_id_mock.return_value = self.conj_id get_conj_id_mock.return_value = self.conj_id
@ -338,7 +339,7 @@ class TestConjIPFlowManager(base.BaseTestCase):
def test_update_flows_for_vlan(self): def test_update_flows_for_vlan(self):
remote_group = self.driver.sg_port_map.get_sg.return_value remote_group = self.driver.sg_port_map.get_sg.return_value
remote_group.get_ethertype_filtered_addresses.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, with mock.patch.object(self.manager.conj_id_map,
'get_conj_id') as get_conj_id_mock: 'get_conj_id') as get_conj_id_mock:
get_conj_id_mock.return_value = self.conj_id get_conj_id_mock.return_value = self.conj_id

View File

@ -350,7 +350,8 @@ class TestCreateFlowsForIpAddress(base.BaseTestCase):
conj_ids = [12, 20] conj_ids = [12, 20]
flows = rules.create_flows_for_ip_address( 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) 0x123, conj_ids)
self.assertEqual(2, len(flows)) self.assertEqual(2, len(flows))

View File

@ -20,8 +20,12 @@ TEST_SET_ID = 'fake_sgid'
ETHERTYPE = 'IPv4' ETHERTYPE = 'IPv4'
TEST_SET_NAME = ipset_manager.IpsetManager.get_name(TEST_SET_ID, ETHERTYPE) TEST_SET_NAME = ipset_manager.IpsetManager.get_name(TEST_SET_ID, ETHERTYPE)
TEST_SET_NAME_NEW = TEST_SET_NAME + ipset_manager.SWAP_SUFFIX 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', FAKE_IPS = [('10.0.0.1', 'fa:16:3e:aa:bb:c1'),
'10.0.0.5', '10.0.0.6'] ('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): class BaseIpsetManagerTest(base.BaseTestCase):
@ -149,13 +153,15 @@ class IpsetManagerTestCase(BaseIpsetManagerTest):
self.verify_mock_calls() self.verify_mock_calls()
def test_set_members_adding_all_zero_ipv4(self): def test_set_members_adding_all_zero_ipv4(self):
self.expect_set(['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']) self.ipset.set_members(TEST_SET_ID, ETHERTYPE,
[('0.0.0.0/0', 'fa:16:3e:aa:bb:c1'), ])
self.verify_mock_calls() self.verify_mock_calls()
def test_set_members_adding_all_zero_ipv6(self): def test_set_members_adding_all_zero_ipv6(self):
self.expect_set(['::/0']) self.expect_set([('::/0', 'fa:16:3e:aa:bb:c1'), ])
self.ipset.set_members(TEST_SET_ID, ETHERTYPE, ['::/0']) self.ipset.set_members(TEST_SET_ID, ETHERTYPE,
[('::/0', 'fa:16:3e:aa:bb:c1'), ])
self.verify_mock_calls() self.verify_mock_calls()
def test_destroy(self): def test_destroy(self):

View File

@ -1518,12 +1518,16 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
if ethertype == "IPv4": if ethertype == "IPv4":
ethertype = "ipv4" ethertype = "ipv4"
members_add = {'IPv4': ['10.0.0.2', '10.0.0.3']} members_add = {'IPv4': [('10.0.0.2', 'fa:16:3e:aa:bb:c1'),
members_after_delete = {'IPv4': ['10.0.0.3']} ('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: else:
ethertype = "ipv6" ethertype = "ipv6"
members_add = {'IPv6': ['fe80::2', 'fe80::3']} members_add = {'IPv6': [('fe80::2', 'fa:16:3e:aa:bb:c3'),
members_after_delete = {'IPv6': ['fe80::3']} ('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, with mock.patch.dict(self.firewall.ipconntrack._device_zone_map,
{port['network_id']: ct_zone}): {port['network_id']: ct_zone}):
@ -2295,10 +2299,12 @@ class IptablesFirewallEnhancedIpsetTestCase(BaseIptablesFirewallTestCase):
self.firewall.ipset.assert_has_calls(calls, True) self.firewall.ipset.assert_has_calls(calls, True)
def test_sg_rule_expansion_with_remote_ips(self): 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': { self.firewall.sg_members = {'fake_sgid': {
'IPv4': [FAKE_IP['IPv4']] + other_ips, 'IPv4': [(FAKE_IP['IPv4'], 'fa:16:3e:aa:bb:c4'), ] + other_ips,
'IPv6': [FAKE_IP['IPv6']]}} 'IPv6': [(FAKE_IP['IPv6'], 'fa:16:3e:aa:bb:c5'), ]}}
port = self._fake_port() port = self._fake_port()
rule = self._fake_sg_rule_for_ethertype(_IPv4, FAKE_SGID) rule = self._fake_sg_rule_for_ethertype(_IPv4, FAKE_SGID)
@ -2307,7 +2313,7 @@ class IptablesFirewallEnhancedIpsetTestCase(BaseIptablesFirewallTestCase):
self.assertEqual(list(rules), self.assertEqual(list(rules),
[dict(list(rule.items()) + [dict(list(rule.items()) +
[('source_ip_prefix', '%s/32' % ip)]) [('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): def test_build_ipv4v6_mac_ip_list(self):
mac_oth = 'ffff-ff0f-ffff' mac_oth = 'ffff-ff0f-ffff'

View File

@ -317,8 +317,10 @@ class SGServerRpcCallBackTestCase(test_sg.SecurityGroupDBTestCase):
sg_member_ips = self.rpc.security_group_info_for_devices( sg_member_ips = self.rpc.security_group_info_for_devices(
ctx, devices=devices)['sg_member_ips'] ctx, devices=devices)['sg_member_ips']
expected_member_ips = [ expected_member_ips = [
'10.0.1.0/24', '11.0.0.1', ('10.0.1.0/24', '00:00:00:00:00:01'),
port['port']['fixed_ips'][0]['ip_address']] ('11.0.0.1', '00:00:00:00:00:01'),
(port['port']['fixed_ips'][0]['ip_address'],
None)]
self.assertEqual(sorted(expected_member_ips), self.assertEqual(sorted(expected_member_ips),
sorted(sg_member_ips[sg_id]['IPv4'])) sorted(sg_member_ips[sg_id]['IPv4']))
self._delete('ports', port_id) self._delete('ports', port_id)
@ -522,7 +524,7 @@ class SGServerRpcCallBackTestCase(test_sg.SecurityGroupDBTestCase):
'remote_group_id': sg2_id} 'remote_group_id': sg2_id}
]}, ]},
'sg_member_ips': {sg2_id: { 'sg_member_ips': {sg2_id: {
'IPv4': set([port_ip2]), 'IPv4': set([(port_ip2, None), ]),
'IPv6': set(), 'IPv6': set(),
}} }}
} }
@ -2947,7 +2949,9 @@ class TestSecurityGroupAgentEnhancedRpcWithIptables(
self.devices_info1 = {'security_groups': {'security_group1': rule1}, self.devices_info1 = {'security_groups': {'security_group1': rule1},
'sg_member_ips': { 'sg_member_ips': {
'security_group1': { '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': devices_info1}
devices_info2 = collections.OrderedDict([ devices_info2 = collections.OrderedDict([
('tap_port1', self._device('tap_port1', ('tap_port1', self._device('tap_port1',
@ -2962,13 +2966,19 @@ class TestSecurityGroupAgentEnhancedRpcWithIptables(
self.devices_info2 = {'security_groups': {'security_group1': rule1}, self.devices_info2 = {'security_groups': {'security_group1': rule1},
'sg_member_ips': { 'sg_member_ips': {
'security_group1': { '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': []}}, 'IPv6': []}},
'devices': devices_info2} 'devices': devices_info2}
self.devices_info3 = {'security_groups': {'security_group1': rule2}, self.devices_info3 = {'security_groups': {'security_group1': rule2},
'sg_member_ips': { 'sg_member_ips': {
'security_group1': { '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': []}}, 'IPv6': []}},
'devices': devices_info2} 'devices': devices_info2}

View File

@ -13,6 +13,7 @@
# under the License. # under the License.
import mock import mock
import netaddr
from neutron_lib import context from neutron_lib import context
from oslo_utils import uuidutils from oslo_utils import uuidutils
@ -128,19 +129,30 @@ class SecurityGroupServerAPIShimTestCase(base.BaseTestCase):
def test_security_group_info_for_devices(self): def test_security_group_info_for_devices(self):
s1 = self._make_security_group_ovo() 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( p2 = self._make_port_ovo(
ip='2.2.2.2', ip='2.2.2.2',
mac_address=netaddr.EUI(mac_2),
security_group_ids={s1.id}, security_group_ids={s1.id},
security=psec.PortSecurity(port_security_enabled=False)) 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') device_owner='network:dhcp')
ids = [p1.id, p2.id, p3.id] ids = [p1.id, p2.id, p3.id]
info = self.shim.security_group_info_for_devices(self.ctx, ids) 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(('1.1.1.1', str(netaddr.EUI(mac_1))),
self.assertIn('2.2.2.2', info['sg_member_ips'][s1.id]['IPv4']) info['sg_member_ips'][s1.id]['IPv4'])
self.assertIn('3.3.3.3', 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(p1.id, info['devices'].keys())
self.assertIn(p2.id, info['devices'].keys()) self.assertIn(p2.id, info['devices'].keys())
# P3 is a trusted port so it doesn't have rules # P3 is a trusted port so it doesn't have rules