From a33d42fa59ada38dd5a5d3114a9a83f03a30d0b2 Mon Sep 17 00:00:00 2001 From: Lingxian Kong Date: Thu, 9 Apr 2020 13:58:30 +1200 Subject: [PATCH] Validate resource access when creating loadbalancer or member * Make sure the user has access to the subnet in the request for creating or updating pool member. * Make sure the user has access to port or subnet or network for creating load balancer Story: 2007531 Task: 39339 Change-Id: I479019a911b5a1acfc1951d1cbbc2a351089cb4d --- etc/octavia.conf | 10 +++ octavia/api/v2/controllers/load_balancer.py | 25 ++++--- octavia/api/v2/controllers/member.py | 8 +-- octavia/common/clients.py | 28 ++++++++ octavia/common/config.py | 6 ++ octavia/common/validate.py | 13 ++-- octavia/network/base.py | 9 ++- octavia/network/drivers/neutron/base.py | 21 +++--- octavia/network/drivers/noop_driver/driver.py | 6 +- .../unit/network/drivers/neutron/test_base.py | 67 +++++++++++++++++++ ...ow-invisible-subnets-e30b0b5fbd216294.yaml | 16 +++++ 11 files changed, 176 insertions(+), 33 deletions(-) create mode 100644 releasenotes/notes/allow-invisible-subnets-e30b0b5fbd216294.yaml diff --git a/etc/octavia.conf b/etc/octavia.conf index 6f85fa4784..10afbb6e11 100644 --- a/etc/octavia.conf +++ b/etc/octavia.conf @@ -156,21 +156,31 @@ [networking] # The maximum attempts to retry an action with the networking service. # max_retries = 15 + # Seconds to wait before retrying an action with the networking service. # retry_interval = 1 + # The maximum time to wait, in seconds, for a port to detach from an amphora # port_detach_timeout = 300 + # Allow/disallow specific network object types when creating VIPs. # allow_vip_network_id = True # allow_vip_subnet_id = True # allow_vip_port_id = True + # List of network_ids that are valid for VIP creation. # If this field empty, no validation is performed. # valid_vip_networks = + # List of reserved IP addresses that cannot be used for member addresses # The default is the nova metadata service address # reserved_ips = ['169.254.169.254'] +# When True, users can use network resources they cannot normally see as VIP +# or member subnets. Making this True may allow users to access resources on +# subnets they do not normally have access to via neutron RBAC policies. +# allow_invisible_resource_usage = False + [haproxy_amphora] # base_path = /var/lib/octavia # base_cert_dir = /var/lib/octavia/certs diff --git a/octavia/api/v2/controllers/load_balancer.py b/octavia/api/v2/controllers/load_balancer.py index 0173e3407d..59acd6eca8 100644 --- a/octavia/api/v2/controllers/load_balancer.py +++ b/octavia/api/v2/controllers/load_balancer.py @@ -118,10 +118,12 @@ class LoadBalancersController(base.BaseController): state=prov_status, id=id) @staticmethod - def _validate_network_and_fill_or_validate_subnet(load_balancer): + def _validate_network_and_fill_or_validate_subnet(load_balancer, + context=None): network = validate.network_exists_optionally_contains_subnet( network_id=load_balancer.vip_network_id, - subnet_id=load_balancer.vip_subnet_id) + subnet_id=load_balancer.vip_subnet_id, + context=context) if not load_balancer.vip_subnet_id: network_driver = utils.get_network_driver() if load_balancer.vip_address: @@ -169,8 +171,10 @@ class LoadBalancersController(base.BaseController): break @staticmethod - def _validate_port_and_fill_or_validate_subnet(load_balancer): - port = validate.port_exists(port_id=load_balancer.vip_port_id) + def _validate_port_and_fill_or_validate_subnet(load_balancer, + context=None): + port = validate.port_exists(port_id=load_balancer.vip_port_id, + context=context) validate.check_port_in_use(port) load_balancer.vip_network_id = port.network_id @@ -185,7 +189,8 @@ class LoadBalancersController(base.BaseController): # Identify the subnet for this port if load_balancer.vip_subnet_id: - validate.subnet_exists(subnet_id=load_balancer.vip_subnet_id) + validate.subnet_exists(subnet_id=load_balancer.vip_subnet_id, + context=context) else: if load_balancer.vip_address: for port_fixed_ip in port.fixed_ips: @@ -203,7 +208,7 @@ class LoadBalancersController(base.BaseController): "VIP port's subnet could not be determined. Please " "specify either a VIP subnet or address.")) - def _validate_vip_request_object(self, load_balancer): + def _validate_vip_request_object(self, load_balancer, context=None): allowed_network_objects = [] if CONF.networking.allow_vip_port_id: allowed_network_objects.append('vip_port_id') @@ -235,10 +240,12 @@ class LoadBalancersController(base.BaseController): # Validate the port id if load_balancer.vip_port_id: - self._validate_port_and_fill_or_validate_subnet(load_balancer) + self._validate_port_and_fill_or_validate_subnet(load_balancer, + context=context) # If no port id, validate the network id (and subnet if provided) elif load_balancer.vip_network_id: - self._validate_network_and_fill_or_validate_subnet(load_balancer) + self._validate_network_and_fill_or_validate_subnet(load_balancer, + context=context) # Validate just the subnet id elif load_balancer.vip_subnet_id: subnet = validate.subnet_exists( @@ -387,7 +394,7 @@ class LoadBalancersController(base.BaseController): self._auth_validate_action(context, load_balancer.project_id, constants.RBAC_POST) - self._validate_vip_request_object(load_balancer) + self._validate_vip_request_object(load_balancer, context=context) self._validate_flavor(context.session, load_balancer) diff --git a/octavia/api/v2/controllers/member.py b/octavia/api/v2/controllers/member.py index 9f5749e524..e76052a9a1 100644 --- a/octavia/api/v2/controllers/member.py +++ b/octavia/api/v2/controllers/member.py @@ -149,9 +149,9 @@ class MemberController(base.BaseController): validate.ip_not_reserved(member.address) # Validate member subnet - if member.subnet_id and not validate.subnet_exists(member.subnet_id): - raise exceptions.NotFound(resource='Subnet', - id=member.subnet_id) + if (member.subnet_id and + not validate.subnet_exists(member.subnet_id, context=context)): + raise exceptions.NotFound(resource='Subnet', id=member.subnet_id) pool = self.repositories.pool.get(context.session, id=self.pool_id) member.project_id, provider = self._get_lb_project_id_provider( context.session, pool.load_balancer_id) @@ -345,7 +345,7 @@ class MembersController(MemberController): # Validate member subnets for member in members: if member.subnet_id and not validate.subnet_exists( - member.subnet_id): + member.subnet_id, context=context): raise exceptions.NotFound(resource='Subnet', id=member.subnet_id) diff --git a/octavia/common/clients.py b/octavia/common/clients.py index 1b1277ca32..58795b3408 100644 --- a/octavia/common/clients.py +++ b/octavia/common/clients.py @@ -12,6 +12,8 @@ from cinderclient import client as cinder_client from glanceclient import client as glance_client +from keystoneauth1.identity.generic import token +from keystoneauth1 import session from neutronclient.neutron import client as neutron_client from novaclient import api_versions from novaclient import client as nova_client @@ -107,6 +109,32 @@ class NeutronAuth(object): LOG.exception("Error creating Neutron client.") return cls.neutron_client + @classmethod + def get_user_neutron_client(cls, context): + # get a normal session + ksession = keystone.KeystoneSession() + service_auth = ksession.get_auth() + + # make user auth and swap it in session + user_auth = token.Token(auth_url=service_auth.auth_url, + token=context.auth_token, + project_id=context.project_id) + user_session = session.Session(auth=user_auth) + + kwargs = { + 'session': user_session, + 'region_name': CONF.neutron.region_name, + 'endpoint_type': CONF.neutron.endpoint_type, + 'service_name': CONF.neutron.service_name, + 'insecure': CONF.neutron.insecure, + 'ca_cert': CONF.neutron.ca_certificates_file + } + if CONF.neutron.endpoint: + kwargs['endpoint_override'] = CONF.neutron.endpoint + + # create neutron client using user's session + return neutron_client.Client(NEUTRON_VERSION, **kwargs) + class GlanceAuth(object): glance_client = None diff --git a/octavia/common/config.py b/octavia/common/config.py index 12f560e495..5bb5f6435b 100644 --- a/octavia/common/config.py +++ b/octavia/common/config.py @@ -212,6 +212,12 @@ networking_opts = [ help=_('List of IP addresses reserved from being used for ' 'member addresses. IPv6 addresses should be in ' 'expanded, uppercase form.')), + cfg.BoolOpt('allow_invisible_resource_usage', default=False, + help=_("When True, users can use network resources they " + "cannot normally see as VIP or member subnets. Making " + "this True may allow users to access resources on " + "subnets they do not normally have access to via " + "neutron RBAC policies.")), ] healthmanager_opts = [ diff --git a/octavia/common/validate.py b/octavia/common/validate.py index 17410af293..dc8e49690f 100644 --- a/octavia/common/validate.py +++ b/octavia/common/validate.py @@ -312,11 +312,11 @@ def sanitize_l7policy_api_args(l7policy, create=False): return l7policy -def port_exists(port_id): +def port_exists(port_id, context=None): """Raises an exception when a port does not exist.""" network_driver = utils.get_network_driver() try: - port = network_driver.get_port(port_id) + port = network_driver.get_port(port_id, context=context) except Exception: raise exceptions.InvalidSubresource(resource='Port', id=port_id) return port @@ -331,11 +331,11 @@ def check_port_in_use(port): return False -def subnet_exists(subnet_id): +def subnet_exists(subnet_id, context=None): """Raises an exception when a subnet does not exist.""" network_driver = utils.get_network_driver() try: - subnet = network_driver.get_subnet(subnet_id) + subnet = network_driver.get_subnet(subnet_id, context=context) except Exception: raise exceptions.InvalidSubresource(resource='Subnet', id=subnet_id) return subnet @@ -358,14 +358,15 @@ def qos_extension_enabled(network_driver): "VIP QoS policy is not allowed in this deployment.")) -def network_exists_optionally_contains_subnet(network_id, subnet_id=None): +def network_exists_optionally_contains_subnet(network_id, subnet_id=None, + context=None): """Raises an exception when a network does not exist. If a subnet is provided, also validate the network contains that subnet. """ network_driver = utils.get_network_driver() try: - network = network_driver.get_network(network_id) + network = network_driver.get_network(network_id, context=context) except Exception: raise exceptions.InvalidSubresource(resource='Network', id=network_id) if subnet_id: diff --git a/octavia/network/base.py b/octavia/network/base.py index 6b342ab40c..9fb549f5c2 100644 --- a/octavia/network/base.py +++ b/octavia/network/base.py @@ -183,28 +183,31 @@ class AbstractNetworkDriver(object, metaclass=abc.ABCMeta): """ @abc.abstractmethod - def get_network(self, network_id): + def get_network(self, network_id, context=None): """Retrieves network from network id. :param network_id: id of an network to retrieve + :param context: A request context :return: octavia.network.data_models.Network :raises: NetworkException, NetworkNotFound """ @abc.abstractmethod - def get_subnet(self, subnet_id): + def get_subnet(self, subnet_id, context=None): """Retrieves subnet from subnet id. :param subnet_id: id of a subnet to retrieve + :param context: A request context :return: octavia.network.data_models.Subnet :raises: NetworkException, SubnetNotFound """ @abc.abstractmethod - def get_port(self, port_id): + def get_port(self, port_id, context=None): """Retrieves port from port id. :param port_id: id of a port to retrieve + :param context: A request context :return: octavia.network.data_models.Port :raises: NetworkException, PortNotFound """ diff --git a/octavia/network/drivers/neutron/base.py b/octavia/network/drivers/neutron/base.py index 6dfe0dc50b..def22f0baf 100644 --- a/octavia/network/drivers/neutron/base.py +++ b/octavia/network/drivers/neutron/base.py @@ -173,9 +173,14 @@ class BaseNeutronDriver(base.AbstractNetworkDriver): return [self._port_to_octavia_interface( compute_id, port) for port in ports['ports']] - def _get_resource(self, resource_type, resource_id): + def _get_resource(self, resource_type, resource_id, context=None): + neutron_client = self.neutron_client + if context and not CONF.networking.allow_invisible_resource_usage: + neutron_client = clients.NeutronAuth.get_user_neutron_client( + context) + try: - resource = getattr(self.neutron_client, 'show_%s' % + resource = getattr(neutron_client, 'show_%s' % resource_type)(resource_id) return getattr(utils, 'convert_%s_dict_to_model' % resource_type)(resource) @@ -225,14 +230,14 @@ class BaseNeutronDriver(base.AbstractNetworkDriver): LOG.exception(message) raise base.NetworkException(message) - def get_network(self, network_id): - return self._get_resource('network', network_id) + def get_network(self, network_id, context=None): + return self._get_resource('network', network_id, context=context) - def get_subnet(self, subnet_id): - return self._get_resource('subnet', subnet_id) + def get_subnet(self, subnet_id, context=None): + return self._get_resource('subnet', subnet_id, context=context) - def get_port(self, port_id): - return self._get_resource('port', port_id) + def get_port(self, port_id, context=None): + return self._get_resource('port', port_id, context=context) def get_network_by_name(self, network_name): return self._get_resources_by_filters( diff --git a/octavia/network/drivers/noop_driver/driver.py b/octavia/network/drivers/noop_driver/driver.py index ebac2e21f4..38ac6ecfa0 100644 --- a/octavia/network/drivers/noop_driver/driver.py +++ b/octavia/network/drivers/noop_driver/driver.py @@ -313,13 +313,13 @@ class NoopNetworkDriver(driver_base.AbstractNetworkDriver): def update_vip(self, loadbalancer, for_delete=False): self.driver.update_vip(loadbalancer, for_delete) - def get_network(self, network_id): + def get_network(self, network_id, context=None): return self.driver.get_network(network_id) - def get_subnet(self, subnet_id): + def get_subnet(self, subnet_id, context=None): return self.driver.get_subnet(subnet_id) - def get_port(self, port_id): + def get_port(self, port_id, context=None): return self.driver.get_port(port_id) def get_qos_policy(self, qos_policy_id): diff --git a/octavia/tests/unit/network/drivers/neutron/test_base.py b/octavia/tests/unit/network/drivers/neutron/test_base.py index 42e6bff795..15bbd4bb49 100644 --- a/octavia/tests/unit/network/drivers/neutron/test_base.py +++ b/octavia/tests/unit/network/drivers/neutron/test_base.py @@ -14,6 +14,8 @@ from unittest import mock from neutronclient.common import exceptions as neutron_client_exceptions +from oslo_config import cfg +from oslo_config import fixture as oslo_fixture from octavia.common import clients from octavia.common import data_models @@ -200,6 +202,9 @@ class TestBaseNeutronNetworkDriver(base.TestCase): port2['fixed_ips'][0]['ip_address']]) def test_get_network(self): + config = self.useFixture(oslo_fixture.Config(cfg.CONF)) + config.config(group="networking", allow_invisible_resource_usage=True) + show_network = self.driver.neutron_client.show_network show_network.return_value = {'network': { 'id': t_constants.MOCK_NETWORK_ID, @@ -210,7 +215,25 @@ class TestBaseNeutronNetworkDriver(base.TestCase): self.assertEqual(1, len(network.subnets)) self.assertEqual(t_constants.MOCK_SUBNET_ID, network.subnets[0]) + @mock.patch("octavia.common.clients.NeutronAuth.get_user_neutron_client") + def test_get_user_network(self, neutron_client_mock): + show_network = neutron_client_mock.return_value.show_network + show_network.return_value = {'network': { + 'id': t_constants.MOCK_NETWORK_ID, + 'subnets': [t_constants.MOCK_SUBNET_ID]}} + + network = self.driver.get_network(t_constants.MOCK_NETWORK_ID, + context=mock.ANY) + + self.assertIsInstance(network, network_models.Network) + self.assertEqual(t_constants.MOCK_NETWORK_ID, network.id) + self.assertEqual(1, len(network.subnets)) + self.assertEqual(t_constants.MOCK_SUBNET_ID, network.subnets[0]) + def test_get_subnet(self): + config = self.useFixture(oslo_fixture.Config(cfg.CONF)) + config.config(group="networking", allow_invisible_resource_usage=True) + show_subnet = self.driver.neutron_client.show_subnet show_subnet.return_value = {'subnet': { 'id': t_constants.MOCK_SUBNET_ID, @@ -222,7 +245,26 @@ class TestBaseNeutronNetworkDriver(base.TestCase): self.assertEqual(t_constants.MOCK_IP_ADDRESS, subnet.gateway_ip) self.assertEqual(t_constants.MOCK_CIDR, subnet.cidr) + @mock.patch("octavia.common.clients.NeutronAuth.get_user_neutron_client") + def test_get_user_subnet(self, neutron_client_mock): + show_subnet = neutron_client_mock.return_value.show_subnet + show_subnet.return_value = {'subnet': { + 'id': t_constants.MOCK_SUBNET_ID, + 'gateway_ip': t_constants.MOCK_IP_ADDRESS, + 'cidr': t_constants.MOCK_CIDR}} + + subnet = self.driver.get_subnet(t_constants.MOCK_SUBNET_ID, + context=mock.ANY) + + self.assertIsInstance(subnet, network_models.Subnet) + self.assertEqual(t_constants.MOCK_SUBNET_ID, subnet.id) + self.assertEqual(t_constants.MOCK_IP_ADDRESS, subnet.gateway_ip) + self.assertEqual(t_constants.MOCK_CIDR, subnet.cidr) + def test_get_port(self): + config = self.useFixture(oslo_fixture.Config(cfg.CONF)) + config.config(group="networking", allow_invisible_resource_usage=True) + show_port = self.driver.neutron_client.show_port show_port.return_value = {'port': { 'id': t_constants.MOCK_PORT_ID, @@ -244,6 +286,31 @@ class TestBaseNeutronNetworkDriver(base.TestCase): self.assertEqual(t_constants.MOCK_IP_ADDRESS, port.fixed_ips[0].ip_address) + @mock.patch("octavia.common.clients.NeutronAuth.get_user_neutron_client") + def test_get_user_port(self, neutron_client_mock): + show_port = neutron_client_mock.return_value.show_port + show_port.return_value = {'port': { + 'id': t_constants.MOCK_PORT_ID, + 'mac_address': t_constants.MOCK_MAC_ADDR, + 'network_id': t_constants.MOCK_NETWORK_ID, + 'fixed_ips': [{ + 'subnet_id': t_constants.MOCK_SUBNET_ID, + 'ip_address': t_constants.MOCK_IP_ADDRESS + }]}} + + port = self.driver.get_port(t_constants.MOCK_PORT_ID, context=mock.ANY) + + self.assertIsInstance(port, network_models.Port) + self.assertEqual(t_constants.MOCK_PORT_ID, port.id) + self.assertEqual(t_constants.MOCK_MAC_ADDR, port.mac_address) + self.assertEqual(t_constants.MOCK_NETWORK_ID, port.network_id) + self.assertEqual(1, len(port.fixed_ips)) + self.assertIsInstance(port.fixed_ips[0], network_models.FixedIP) + self.assertEqual(t_constants.MOCK_SUBNET_ID, + port.fixed_ips[0].subnet_id) + self.assertEqual(t_constants.MOCK_IP_ADDRESS, + port.fixed_ips[0].ip_address) + def test_get_network_by_name(self): list_network = self.driver.neutron_client.list_networks list_network.return_value = {'networks': [{'network': { diff --git a/releasenotes/notes/allow-invisible-subnets-e30b0b5fbd216294.yaml b/releasenotes/notes/allow-invisible-subnets-e30b0b5fbd216294.yaml new file mode 100644 index 0000000000..0663be6c28 --- /dev/null +++ b/releasenotes/notes/allow-invisible-subnets-e30b0b5fbd216294.yaml @@ -0,0 +1,16 @@ +--- +upgrade: + - | + After this upgrade, users will no longer be able use network resources they + cannot see or "show" on load balancers. Operators can revert this behavior + by setting the "allow_invisible_reourece_usage" configuration file setting + to ``True``. +security: + - | + Previously, if a user knew or could guess the UUID for a network resource, + they could use that UUID to create load balancer resources using that UUID. + Now the user must have permission to see or "show" the resource before it + can be used with a load balancer. This will be the new default, but + operators can disable this behavior via the setting the configuration file + setting "allow_invisible_resource_usage" to ``True``. This issue falls + under the "Class C1" security issue as the user would require a valid UUID.