From 68d464fa82cdd52cfabbdcbd3d94c86eb1453a3e Mon Sep 17 00:00:00 2001 From: Gaudenz Steinlin Date: Wed, 1 Mar 2023 09:52:00 +0100 Subject: [PATCH] allowed_cidr validation for additional_vips The validation for the allowed_cidr parameter did not take into account the IP version of additional vIPs. The parameter was rejected if the IP version did not match the IP version of the primary vIP. As additional vIPs can have a different IP version from the primary vIP all vIPs must be checked during validation. Change-Id: I3706fd5e12e17be37edce974563c6806d4f09709 (cherry picked from commit 60f579b64a8f603e5a67860a90cd413a0e9ca381) --- octavia/api/v2/controllers/listener.py | 33 +++++++---- .../drivers/neutron/allowed_address_pairs.py | 18 ++++-- .../tests/functional/api/v2/test_listener.py | 55 +++++++++++++------ .../neutron/test_allowed_address_pairs.py | 31 ++++++++++- ...-for-additional_vips-175c32824cc7ee95.yaml | 7 +++ 5 files changed, 107 insertions(+), 37 deletions(-) create mode 100644 releasenotes/notes/allowed_cidr-validation-for-additional_vips-175c32824cc7ee95.yaml diff --git a/octavia/api/v2/controllers/listener.py b/octavia/api/v2/controllers/listener.py index 0bca3b2c6e..bda4c3eff5 100644 --- a/octavia/api/v2/controllers/listener.py +++ b/octavia/api/v2/controllers/listener.py @@ -156,14 +156,18 @@ class ListenersController(base.BaseController): value=headers, option=('%s protocol listener.' % listener_protocol)) - def _validate_cidr_compatible_with_vip(self, vip, allowed_cidrs): + def _validate_cidr_compatible_with_vip(self, vips, allowed_cidrs): for cidr in allowed_cidrs: - # Check if CIDR IP version matches VIP IP version - if common_utils.is_cidr_ipv6(cidr) != common_utils.is_ipv6(vip): - msg = _("CIDR %(cidr)s IP version incompatible with VIP " - "%(vip)s IP version.") + for vip in vips: + # Check if CIDR IP version matches VIP IP version + if (common_utils.is_cidr_ipv6(cidr) == + common_utils.is_ipv6(vip)): + break + else: + msg = _("CIDR %(cidr)s IP version incompatible with all VIPs " + "%(vips)s IP version.") raise exceptions.ValidationException( - detail=msg % {'cidr': cidr, 'vip': vip}) + detail=msg % {'cidr': cidr, 'vips': vips}) def _validate_create_listener(self, lock_session, listener_dict): """Validate listener for wrong protocol or duplicate listeners @@ -306,10 +310,11 @@ class ListenersController(base.BaseController): # Validate allowed CIDRs allowed_cidrs = listener_dict.get('allowed_cidrs', []) or [] lb_id = listener_dict.get('load_balancer_id') - vip_db = self.repositories.vip.get( - lock_session, load_balancer_id=lb_id) - vip_address = vip_db.ip_address - self._validate_cidr_compatible_with_vip(vip_address, allowed_cidrs) + lb_db = self.repositories.load_balancer.get( + lock_session, id=lb_id) + vip_addresses = [lb_db.vip.ip_address] + vip_addresses.extend([vip.ip_address for vip in lb_db.additional_vips]) + self._validate_cidr_compatible_with_vip(vip_addresses, allowed_cidrs) if _can_tls_offload: # Validate TLS version list @@ -525,9 +530,13 @@ class ListenersController(base.BaseController): # Validate allowed CIDRs if (listener.allowed_cidrs and listener.allowed_cidrs != wtypes.Unset): - vip_address = db_listener.load_balancer.vip.ip_address + vip_addresses = [db_listener.load_balancer.vip.ip_address] + vip_addresses.extend( + [vip.ip_address + for vip in db_listener.load_balancer.additional_vips] + ) self._validate_cidr_compatible_with_vip( - vip_address, listener.allowed_cidrs) + vip_addresses, listener.allowed_cidrs) # Check TLS cipher prohibit list if listener.tls_ciphers: diff --git a/octavia/network/drivers/neutron/allowed_address_pairs.py b/octavia/network/drivers/neutron/allowed_address_pairs.py index 61618b82be..fbaacd3c55 100644 --- a/octavia/network/drivers/neutron/allowed_address_pairs.py +++ b/octavia/network/drivers/neutron/allowed_address_pairs.py @@ -147,6 +147,10 @@ class AllowedAddressPairsDriver(neutron_base.BaseNeutronDriver): address = ipaddress.ip_address(ip) return 'IPv6' if address.version == 6 else 'IPv4' + def _get_ethertype_for_cidr(self, cidr): + net = ipaddress.ip_network(cidr) + return 'IPv6' if net.version == 6 else 'IPv4' + def _update_security_group_rules(self, load_balancer, sec_grp_id): rules = self.neutron_client.list_security_group_rules( security_group_id=sec_grp_id) @@ -224,11 +228,15 @@ class AllowedAddressPairsDriver(neutron_base.BaseNeutronDriver): ethertypes.add(self._get_ethertype_for_ip(add_vip.ip_address)) for port_protocol in add_ports: for ethertype in ethertypes: - self._create_security_group_rule(sec_grp_id, port_protocol[1], - port_min=port_protocol[0], - port_max=port_protocol[0], - ethertype=ethertype, - cidr=port_protocol[2]) + cidr = port_protocol[2] + if not cidr or self._get_ethertype_for_cidr(cidr) == ethertype: + self._create_security_group_rule( + sec_grp_id, port_protocol[1], + port_min=port_protocol[0], + port_max=port_protocol[0], + ethertype=ethertype, + cidr=cidr, + ) # Currently we are using the VIP network for VRRP # so we need to open up the protocols for it diff --git a/octavia/tests/functional/api/v2/test_listener.py b/octavia/tests/functional/api/v2/test_listener.py index 6b5db1a777..476640aad8 100644 --- a/octavia/tests/functional/api/v2/test_listener.py +++ b/octavia/tests/functional/api/v2/test_listener.py @@ -1199,9 +1199,9 @@ class TestListener(base.BaseAPITest): "It must be a valid x509 PEM format certificate.", response['faultstring']) - def _test_create_with_allowed_cidrs(self, allowed_cidrs): + def _test_create_with_allowed_cidrs(self, allowed_cidrs, lb_id): listener = self.create_listener(constants.PROTOCOL_TCP, - 80, self.lb_id, + 80, lb_id, allowed_cidrs=allowed_cidrs) listener_path = self.LISTENER_PATH.format( listener_id=listener['listener']['id']) @@ -1210,14 +1210,17 @@ class TestListener(base.BaseAPITest): def test_create_with_allowed_cidrs_ipv4(self): allowed_cidrs = ['10.0.1.0/24', '172.16.55.0/25'] - self._test_create_with_allowed_cidrs(allowed_cidrs) + self._test_create_with_allowed_cidrs(allowed_cidrs, self.lb_id) def test_create_with_allowed_cidrs_ipv6(self): + lb_ipv6 = self.create_load_balancer( + uuidutils.generate_uuid(), + vip_address='2001:db9:a1b:13f0::1', + ) + lb_id = lb_ipv6.get('loadbalancer').get('id') + self.set_lb_status(lb_id) allowed_cidrs = ['2001:db8:a0b:12f0::/64', '2a02:8071:69e::/64'] - with mock.patch('octavia.db.repositories.VipRepository.' - 'get') as repo_mock: - repo_mock.return_value.ip_address = "2001:db9:a1b:13f0::1" - self._test_create_with_allowed_cidrs(allowed_cidrs) + self._test_create_with_allowed_cidrs(allowed_cidrs, lb_id) def test_create_with_bad_allowed_cidrs(self): allowed_cidrs = [u'10.0.1.0/33', u'172.16.55.1.0/25'] @@ -1243,24 +1246,42 @@ class TestListener(base.BaseAPITest): body = self._build_body(lb_listener) response = self.post(self.LISTENERS_PATH, body, status=400).json self.assertIn("Validation failure: CIDR 2001:db8:a0b:12f0::/64 IP " - "version incompatible with VIP 198.0.2.5 IP version.", + "version incompatible with all VIPs ['198.0.2.5'] IP " + "version.", response['faultstring']) def test_create_with_incompatible_allowed_cidrs_ipv4(self): + lb_ipv6 = self.create_load_balancer( + uuidutils.generate_uuid(), + vip_address='2001:db9:a1b:13f0::1', + ) + lb_id = lb_ipv6.get('loadbalancer').get('id') + self.set_lb_status(lb_id) lb_listener = { 'protocol': constants.PROTOCOL_TCP, 'protocol_port': 80, 'project_id': self.project_id, - 'loadbalancer_id': self.lb_id, + 'loadbalancer_id': lb_id, 'allowed_cidrs': ['10.0.1.0/24']} - with mock.patch('octavia.db.repositories.VipRepository.' - 'get') as repo_mock: - repo_mock.return_value.ip_address = "2001:db9:a1b:13f0::1" - body = self._build_body(lb_listener) - response = self.post(self.LISTENERS_PATH, body, status=400).json - self.assertIn("Validation failure: CIDR 10.0.1.0/24 IP version " - "incompatible with VIP 2001:db9:a1b:13f0::1 IP " - "version.", response['faultstring']) + body = self._build_body(lb_listener) + response = self.post(self.LISTENERS_PATH, body, status=400).json + self.assertIn("Validation failure: CIDR 10.0.1.0/24 IP version " + "incompatible with all VIPs " + "['2001:db9:a1b:13f0::1'] IP version.", + response['faultstring']) + + def test_create_with_mixed_version_allowed_cidrs(self): + lb_dualstack = self.create_load_balancer( + uuidutils.generate_uuid(), + additional_vips=[{'subnet_id': uuidutils.generate_uuid(), + 'ip_address': '2001:db9:a1b:13f0::1', + }], + ) + lb_id = lb_dualstack.get('loadbalancer').get('id') + self.set_lb_status(lb_id) + self._test_create_with_allowed_cidrs(['10.0.1.0/24', + '2001:db9:a1b:13f0::/64'], + lb_id) def test_create_with_duplicated_allowed_cidrs(self): allowed_cidrs = ['10.0.1.0/24', '10.0.2.0/24', '10.0.2.0/24'] diff --git a/octavia/tests/unit/network/drivers/neutron/test_allowed_address_pairs.py b/octavia/tests/unit/network/drivers/neutron/test_allowed_address_pairs.py index 49994a99ac..c5dc513f3a 100644 --- a/octavia/tests/unit/network/drivers/neutron/test_allowed_address_pairs.py +++ b/octavia/tests/unit/network/drivers/neutron/test_allowed_address_pairs.py @@ -1099,12 +1099,13 @@ class TestAllowedAddressPairsDriver(base.TestCase): lc_1 = data_models.ListenerCidr('l1', '10.0.101.0/24') lc_2 = data_models.ListenerCidr('l2', '10.0.102.0/24') lc_3 = data_models.ListenerCidr('l2', '10.0.103.0/24') + lc_4 = data_models.ListenerCidr('l2', '2001:0DB8::/32') listeners = [data_models.Listener(protocol_port=80, peer_port=1024, protocol=constants.PROTOCOL_TCP, allowed_cidrs=[lc_1]), data_models.Listener(protocol_port=443, peer_port=1025, protocol=constants.PROTOCOL_TCP, - allowed_cidrs=[lc_2, lc_3]), + allowed_cidrs=[lc_2, lc_3, lc_4]), data_models.Listener(protocol_port=50, peer_port=1026, protocol=constants.PROTOCOL_UDP)] vip = data_models.Vip(ip_address='10.0.0.2') @@ -1183,7 +1184,18 @@ class TestAllowedAddressPairsDriver(base.TestCase): 'remote_ip_prefix': '10.0.103.0/24' } } - expected_create_rule_udp = { + expected_create_rule_5 = { + 'security_group_rule': { + 'security_group_id': 'secgrp-1', + 'direction': 'ingress', + 'protocol': 'tcp', + 'port_range_min': 443, + 'port_range_max': 443, + 'ethertype': 'IPv6', + 'remote_ip_prefix': '2001:0DB8::/32' + } + } + expected_create_rule_udp_1 = { 'security_group_rule': { 'security_group_id': 'secgrp-1', 'direction': 'ingress', @@ -1194,13 +1206,26 @@ class TestAllowedAddressPairsDriver(base.TestCase): 'remote_ip_prefix': None } } + expected_create_rule_udp_2 = { + 'security_group_rule': { + 'security_group_id': 'secgrp-1', + 'direction': 'ingress', + 'protocol': 'udp', + 'port_range_min': 50, + 'port_range_max': 50, + 'ethertype': 'IPv6', + 'remote_ip_prefix': None + } + } create_rule.assert_has_calls([mock.call(expected_create_rule_1), mock.call(expected_create_rule_udp_peer), mock.call(expected_create_rule_2), mock.call(expected_create_rule_3), mock.call(expected_create_rule_4), - mock.call(expected_create_rule_udp)], + mock.call(expected_create_rule_5), + mock.call(expected_create_rule_udp_1), + mock.call(expected_create_rule_udp_2)], any_order=True) def test_update_vip_when_protocol_and_peer_ports_overlap(self): diff --git a/releasenotes/notes/allowed_cidr-validation-for-additional_vips-175c32824cc7ee95.yaml b/releasenotes/notes/allowed_cidr-validation-for-additional_vips-175c32824cc7ee95.yaml new file mode 100644 index 0000000000..456d9d1806 --- /dev/null +++ b/releasenotes/notes/allowed_cidr-validation-for-additional_vips-175c32824cc7ee95.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + The validation for the allowed_cidr parameter only took into account the + IP version of the primary VIP. CIDRs which only matched the version of an + additonal VIP were rejected. This if fixed and CIDRs are now matched + against the IP version of all VIPs.