commit 7aed4a7a2fcfe5621dae4aeacf3d9b07d152e7fc Author: Aaron Rosen Date: Tue Mar 18 18:07:33 2014 -0700 Prevent cross plugging router ports from other tenants Previously, a tenant could plug an interface into another tenant's router if he knew their router_id by creating a port with the correct device_id and device_owner. This patch prevents this from occuring by preventing non-admin users from creating ports with device_owner network:router_interface with a device_id that matches another tenants router. In addition, it prevents one from updating a ports device_owner and device_id so that the device_id won't match another tenants router with device_owner being network:router_interface. NOTE: with this change it does open up the possiblity for a tenant to discover router_id's of another tenant's by guessing them and updating a port till a conflict occurs. That said, randomly guessing the router id would be hard and in theory should not matter if exposed. We also need to allow a tenant to update the device_id on network:router_interface ports as this would be used for by anyone using a vm as a service router. This issue will be fixed in another patch upstream as a db migration is required but since this needs to be backported to all stable branches this is not possible. NOTE: The only plugins affect by this are the ones that use the l3-agent. NOTE: **One should perform and audit of the ports that are already attached to routers after applying this patch and remove ports that a tenant may have cross plugged.** Closes-bug: #1243327 Conflicts: neutron/common/exceptions.py neutron/db/db_base_plugin_v2.py diff --git a/neutron/common/exceptions.py b/neutron/common/exceptions.py index 7b02647..88fa6e4 100644 --- a/neutron/common/exceptions.py +++ b/neutron/common/exceptions.py @@ -279,25 +279,30 @@ class InvalidConfigurationOption(NeutronException): "%(opt_value)s") class GatewayConflictWithAllocationPools(InUse): message = _("Gateway ip %(ip_address)s conflicts with " "allocation pool %(pool)s") class GatewayIpInUse(InUse): message = _("Current gateway ip %(ip_address)s already in use " "by port %(port_id)s. Unable to update.") class NetworkVlanRangeError(NeutronException): message = _("Invalid network VLAN range: '%(vlan_range)s' - '%(error)s'") def __init__(self, **kwargs): # Convert vlan_range tuple to 'start:end' format for display if isinstance(kwargs['vlan_range'], tuple): kwargs['vlan_range'] = "%d:%d" % kwargs['vlan_range'] super(NetworkVlanRangeError, self).__init__(**kwargs) class NetworkVxlanPortRangeError(object): message = _("Invalid network VXLAN port range: '%(vxlan_range)s'") + + +class DeviceIDNotOwnedByTenant(Conflict): + message = _("The following device_id %(device_id)s is not owned by your " + "tenant or matches another tenants router.") diff --git a/neutron/db/db_base_plugin_v2.py b/neutron/db/db_base_plugin_v2.py index 2afbac5..872463f 100644 --- a/neutron/db/db_base_plugin_v2.py +++ b/neutron/db/db_base_plugin_v2.py @@ -5,58 +5,62 @@ # # Licensed under the Apache License, Version 2.0 (the "License"); you may # not use this file except in compliance with the License. You may obtain # a copy of the License at # # http://www.apache.org/licenses/LICENSE-2.0 # # Unless required by applicable law or agreed to in writing, software # distributed under the License is distributed on an "AS IS" BASIS, WITHOUT # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the # License for the specific language governing permissions and limitations # under the License. import datetime import itertools import random import netaddr from oslo.config import cfg from sqlalchemy import orm from sqlalchemy.orm import exc from neutron.api.v2 import attributes from neutron.common import constants from neutron.common import exceptions as q_exc +from neutron import context as ctx from neutron.db import api as db from neutron.db import models_v2 from neutron.db import sqlalchemyutils +from neutron.extensions import l3 +from neutron import manager from neutron import neutron_plugin_base_v2 from neutron.openstack.common import excutils from neutron.openstack.common import log as logging from neutron.openstack.common import timeutils from neutron.openstack.common import uuidutils +from neutron.plugins.common import constants as service_constants LOG = logging.getLogger(__name__) AGENT_OWNER_PREFIX = 'network:' # Ports with the following 'device_owner' values will not prevent # network deletion. If delete_network() finds that all ports on a # network have these owners, it will explicitly delete each port # and allow network deletion to continue. Similarly, if delete_subnet() # finds out that all existing IP Allocations are associated with ports # with these owners, it will allow subnet deletion to proceed with the # IP allocations being cleaned up by cascade. AUTO_DELETE_PORT_OWNERS = ['network:dhcp'] class CommonDbMixin(object): """Common methods used in core and service plugins.""" # Plugins, mixin classes implementing extension will register # hooks into the dict below for "augmenting" the "core way" of # building a query for retrieving objects from a model class. # To this aim, the register_model_query_hook and unregister_query_hook # from this class should be invoked _model_query_hooks = {} @@ -1289,50 +1293,53 @@ class NeutronDbPluginV2(neutron_plugin_base_v2.NeutronPluginBaseV2, page_reverse=False): marker_obj = self._get_marker_obj(context, 'subnet', limit, marker) return self._get_collection(context, models_v2.Subnet, self._make_subnet_dict, filters=filters, fields=fields, sorts=sorts, limit=limit, marker_obj=marker_obj, page_reverse=page_reverse) def get_subnets_count(self, context, filters=None): return self._get_collection_count(context, models_v2.Subnet, filters=filters) def create_port_bulk(self, context, ports): return self._create_bulk('port', context, ports) def create_port(self, context, port): p = port['port'] port_id = p.get('id') or uuidutils.generate_uuid() network_id = p['network_id'] mac_address = p['mac_address'] # NOTE(jkoelker) Get the tenant_id outside of the session to avoid # unneeded db action if the operation raises tenant_id = self._get_tenant_id_for_create(context, p) + if p.get('device_owner') == constants.DEVICE_OWNER_ROUTER_INTF: + self._enforce_device_owner_not_router_intf_or_device_id(context, p, + tenant_id) with context.session.begin(subtransactions=True): network = self._get_network(context, network_id) # Ensure that a MAC address is defined and it is unique on the # network if mac_address is attributes.ATTR_NOT_SPECIFIED: mac_address = NeutronDbPluginV2._generate_mac(context, network_id) else: # Ensure that the mac on the network is unique if not NeutronDbPluginV2._check_unique_mac(context, network_id, mac_address): raise q_exc.MacAddressInUse(net_id=network_id, mac=mac_address) # Returns the IP's for the port ips = self._allocate_ips_for_port(context, network, port) if 'status' not in p: status = constants.PORT_STATUS_ACTIVE else: status = p['status'] @@ -1352,50 +1359,67 @@ class NeutronDbPluginV2(neutron_plugin_base_v2.NeutronPluginBaseV2, for ip in ips: ip_address = ip['ip_address'] subnet_id = ip['subnet_id'] LOG.debug(_("Allocated IP %(ip_address)s " "(%(network_id)s/%(subnet_id)s/%(port_id)s)"), {'ip_address': ip_address, 'network_id': network_id, 'subnet_id': subnet_id, 'port_id': port_id}) allocated = models_v2.IPAllocation( network_id=network_id, port_id=port_id, ip_address=ip_address, subnet_id=subnet_id, ) context.session.add(allocated) return self._make_port_dict(port, process_extensions=False) def update_port(self, context, id, port): p = port['port'] changed_ips = False with context.session.begin(subtransactions=True): port = self._get_port(context, id) + if 'device_owner' in p: + current_device_owner = p['device_owner'] + changed_device_owner = True + else: + current_device_owner = port['device_owner'] + changed_device_owner = False + if p.get('device_id') != port['device_id']: + changed_device_id = True + + # if the current device_owner is ROUTER_INF and the device_id or + # device_owner changed check device_id is not another tenants + # router + if ((current_device_owner == constants.DEVICE_OWNER_ROUTER_INTF) + and (changed_device_id or changed_device_owner)): + self._enforce_device_owner_not_router_intf_or_device_id( + context, p, port['tenant_id'], port) + # Check if the IPs need to be updated if 'fixed_ips' in p: changed_ips = True original = self._make_port_dict(port, process_extensions=False) added_ips, prev_ips = self._update_ips_for_port( context, port["network_id"], id, original["fixed_ips"], p['fixed_ips']) # Update ips if necessary for ip in added_ips: allocated = models_v2.IPAllocation( network_id=port['network_id'], port_id=port.id, ip_address=ip['ip_address'], subnet_id=ip['subnet_id']) context.session.add(allocated) # Remove all attributes in p which are not in the port DB model # and then update the port port.update(self._filter_non_model_columns(p, models_v2.Port)) result = self._make_port_dict(port) # Keep up with fields that changed if changed_ips: result['fixed_ips'] = prev_ips + added_ips return result def delete_port(self, context, id): @@ -1461,25 +1485,63 @@ class NeutronDbPluginV2(neutron_plugin_base_v2.NeutronPluginBaseV2, if subnet_ids: query = query.filter(IPAllocation.subnet_id.in_(subnet_ids)) query = self._apply_filters_to_query(query, Port, filters) if limit and page_reverse and sorts: sorts = [(s[0], not s[1]) for s in sorts] query = sqlalchemyutils.paginate_query(query, Port, limit, sorts, marker_obj) return query def get_ports(self, context, filters=None, fields=None, sorts=None, limit=None, marker=None, page_reverse=False): marker_obj = self._get_marker_obj(context, 'port', limit, marker) query = self._get_ports_query(context, filters=filters, sorts=sorts, limit=limit, marker_obj=marker_obj, page_reverse=page_reverse) items = [self._make_port_dict(c, fields) for c in query] if limit and page_reverse: items.reverse() return items def get_ports_count(self, context, filters=None): return self._get_ports_query(context, filters).count() + + def _enforce_device_owner_not_router_intf_or_device_id(self, context, + port_request, + tenant_id, + db_port=None): + if not context.is_admin: + # find the device_id. If the call was update_port and the + # device_id was not passed in we use the device_id from the + # db. + device_id = port_request.get('device_id') + if not device_id and db_port: + device_id = db_port.get('device_id') + # check to make sure device_id does not match another tenants + # router. + if device_id: + if hasattr(self, 'get_router'): + try: + ctx_admin = ctx.get_admin_context() + router = self.get_router(ctx_admin, device_id) + except l3.RouterNotFound: + return + else: + l3plugin = ( + manager.NeutronManager.get_service_plugins().get( + service_constants.L3_ROUTER_NAT)) + if l3plugin: + try: + ctx_admin = ctx.get_admin_context() + router = l3plugin.get_router(ctx_admin, + device_id) + except l3.RouterNotFound: + return + else: + # raise as extension doesn't support L3 anyways. + raise q_exc.DeviceIDNotOwnedByTenant( + device_id=device_id) + if tenant_id != router['tenant_id']: + raise q_exc.DeviceIDNotOwnedByTenant(device_id=device_id) diff --git a/neutron/tests/unit/test_l3_plugin.py b/neutron/tests/unit/test_l3_plugin.py index 4f75b57..9cc5cf9 100644 --- a/neutron/tests/unit/test_l3_plugin.py +++ b/neutron/tests/unit/test_l3_plugin.py @@ -357,59 +357,64 @@ class L3NatTestCaseMixin(object): res = self._create_router(fmt, tenant_id, name, admin_state_up, set_context, arg_list=arg_list, external_gateway_info=external_gateway_info, **kwargs) return self.deserialize(fmt, res) def _add_external_gateway_to_router(self, router_id, network_id, expected_code=exc.HTTPOk.code, neutron_context=None): return self._update('routers', router_id, {'router': {'external_gateway_info': {'network_id': network_id}}}, expected_code=expected_code, neutron_context=neutron_context) def _remove_external_gateway_from_router(self, router_id, network_id, expected_code=exc.HTTPOk.code): return self._update('routers', router_id, {'router': {'external_gateway_info': {}}}, expected_code=expected_code) def _router_interface_action(self, action, router_id, subnet_id, port_id, expected_code=exc.HTTPOk.code, - expected_body=None): + expected_body=None, + tenant_id=None): interface_data = {} if subnet_id: interface_data.update({'subnet_id': subnet_id}) if port_id and (action != 'add' or not subnet_id): interface_data.update({'port_id': port_id}) req = self.new_action_request('routers', interface_data, router_id, "%s_router_interface" % action) + # if tenant_id was specified, create a tenant context for this request + if tenant_id: + req.environ['neutron.context'] = context.Context( + '', tenant_id) res = req.get_response(self.ext_api) self.assertEqual(res.status_int, expected_code) response = self.deserialize(self.fmt, res) if expected_body: self.assertEqual(response, expected_body) return response @contextlib.contextmanager def router(self, name='router1', admin_state_up=True, fmt=None, tenant_id=_uuid(), external_gateway_info=None, set_context=False, **kwargs): router = self._make_router(fmt or self.fmt, tenant_id, name, admin_state_up, external_gateway_info, set_context, **kwargs) try: yield router finally: self._delete('routers', router['router']['id']) def _set_net_external(self, net_id): self._update('networks', net_id, {'network': {external_net.EXTERNAL: True}}) def _create_floatingip(self, fmt, network_id, port_id=None, @@ -946,50 +951,106 @@ class L3NatTestCaseBase(L3NatTestCaseMixin): s['subnet']['network_id']) body = self._show('routers', r['router']['id']) gw_info = body['router']['external_gateway_info'] self.assertEqual(gw_info, None) def test_router_add_gateway_tenant_ctx(self): with self.router(tenant_id='noadmin', set_context=True) as r: with self.subnet() as s: self._set_net_external(s['subnet']['network_id']) ctx = context.Context('', 'noadmin') self._add_external_gateway_to_router( r['router']['id'], s['subnet']['network_id'], neutron_context=ctx) body = self._show('routers', r['router']['id']) net_id = body['router']['external_gateway_info']['network_id'] self.assertEqual(net_id, s['subnet']['network_id']) self._remove_external_gateway_from_router( r['router']['id'], s['subnet']['network_id']) body = self._show('routers', r['router']['id']) gw_info = body['router']['external_gateway_info'] self.assertEqual(gw_info, None) + def test_create_router_port_with_device_id_of_other_teants_router(self): + with self.router() as admin_router: + with self.network(tenant_id='tenant_a', + set_context=True) as n: + with self.subnet(network=n): + self._create_port( + self.fmt, n['network']['id'], + tenant_id='tenant_a', + device_id=admin_router['router']['id'], + device_owner='network:router_interface', + set_context=True, + expected_res_status=exc.HTTPConflict.code) + + def test_create_non_router_port_device_id_of_other_teants_router_update( + self): + # This tests that HTTPConflict is raised if we create a non-router + # port that matches the device_id of another tenants router and then + # we change the device_owner to be network:router_interface. + with self.router() as admin_router: + with self.network(tenant_id='tenant_a', + set_context=True) as n: + with self.subnet(network=n): + port_res = self._create_port( + self.fmt, n['network']['id'], + tenant_id='tenant_a', + device_id=admin_router['router']['id'], + set_context=True) + port = self.deserialize(self.fmt, port_res) + neutron_context = context.Context('', 'tenant_a') + data = {'port': {'device_owner': + 'network:router_interface'}} + self._update('ports', port['port']['id'], data, + neutron_context=neutron_context, + expected_code=exc.HTTPConflict.code) + self._delete('ports', port['port']['id']) + + def test_update_port_device_id_to_different_tenants_router(self): + with self.router() as admin_router: + with self.router(tenant_id='tenant_a', + set_context=True) as tenant_router: + with self.network(tenant_id='tenant_a', + set_context=True) as n: + with self.subnet(network=n) as s: + port = self._router_interface_action( + 'add', tenant_router['router']['id'], + s['subnet']['id'], None, tenant_id='tenant_a') + neutron_context = context.Context('', 'tenant_a') + data = {'port': + {'device_id': admin_router['router']['id']}} + self._update('ports', port['port_id'], data, + neutron_context=neutron_context, + expected_code=exc.HTTPConflict.code) + self._router_interface_action( + 'remove', tenant_router['router']['id'], + s['subnet']['id'], None, tenant_id='tenant_a') + def test_router_add_gateway_invalid_network_returns_404(self): with self.router() as r: self._add_external_gateway_to_router( r['router']['id'], "foobar", expected_code=exc.HTTPNotFound.code) def test_router_add_gateway_net_not_external_returns_400(self): with self.router() as r: with self.subnet() as s: # intentionally do not set net as external self._add_external_gateway_to_router( r['router']['id'], s['subnet']['network_id'], expected_code=exc.HTTPBadRequest.code) def test_router_add_gateway_no_subnet_returns_400(self): with self.router() as r: with self.network() as n: self._set_net_external(n['network']['id']) self._add_external_gateway_to_router( r['router']['id'], n['network']['id'], expected_code=exc.HTTPBadRequest.code) def test_router_remove_interface_inuse_returns_409(self): with self.router() as r: