commit 2e55cc6d989e158a7f5e75b6cdf112d35e47d17c Author: Aaron Rosen Date: Wed Dec 18 13:46:39 2013 -0800 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 create/updating_ports and passing in a device_owner value set to network:router_interface. The device_owner is an internal field that tenants should never set anyways so this works as a stop gap measure to prevent this (without requiring a db migration). Some of the tests that were added were mocked out for the cisco and nvp plugin as they do not support this workflow currently though patches to support this will follow upstream after this merges. Notes: The only plugins affect by this are the ones that use the l3-agent. 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 diff --git a/neutron/common/exceptions.py b/neutron/common/exceptions.py index 9e77832..271b4a1 100644 --- a/neutron/common/exceptions.py +++ b/neutron/common/exceptions.py @@ -288,25 +288,30 @@ 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(NeutronException): message = _("Invalid network VXLAN port range: '%(vxlan_range)s'") class DuplicatedExtension(NeutronException): message = _("Found duplicate extension: %(alias)s") + + +class DeviceOwnerNotAllowed(BadRequest): + message = _("Setting device_owner to %(device_owner)s requires admin " + "credentials") diff --git a/neutron/db/db_base_plugin_v2.py b/neutron/db/db_base_plugin_v2.py index d871c17..6aa471f 100644 --- a/neutron/db/db_base_plugin_v2.py +++ b/neutron/db/db_base_plugin_v2.py @@ -1284,50 +1284,52 @@ class NeutronDbPluginV2(neutron_plugin_base_v2.NeutronPluginBaseV2, def get_subnet(self, context, id, fields=None): subnet = self._get_subnet(context, id) return self._make_subnet_dict(subnet, fields) def get_subnets(self, context, filters=None, fields=None, sorts=None, limit=None, marker=None, 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'] + + self._enforce_device_owner_not_router_intf(context, p) 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) 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) @@ -1353,50 +1355,56 @@ 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) + # Ensure that the device_owner does not change to + # router_intf by normal tenant if it was not already + # router_intf. + if p.get('device_owner') != port['device_owner']: + self._enforce_device_owner_not_router_intf(context, p) + # 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): @@ -1475,25 +1483,31 @@ 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(self, context, port): + if (not context.is_admin and + port.get('device_owner') == constants.DEVICE_OWNER_ROUTER_INTF): + raise q_exc.DeviceOwnerNotAllowed( + device_owner=port.get('device_owner')) diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py index ab6017f..b4e19ba 100644 --- a/neutron/db/l3_db.py +++ b/neutron/db/l3_db.py @@ -322,51 +322,53 @@ class L3_NAT_db_mixin(l3.RouterPluginBase): fixed_ips = [ip for ip in port['fixed_ips']] if len(fixed_ips) != 1: msg = _('Router port must have exactly one fixed IP') raise q_exc.BadRequest(resource='router', msg=msg) subnet_id = fixed_ips[0]['subnet_id'] subnet = self._core_plugin._get_subnet(context, subnet_id) self._check_for_dup_router_subnet(context, router_id, port['network_id'], subnet['id'], subnet['cidr']) port.update({'device_id': router_id, 'device_owner': DEVICE_OWNER_ROUTER_INTF}) elif 'subnet_id' in interface_info: subnet_id = interface_info['subnet_id'] subnet = self._core_plugin._get_subnet(context, subnet_id) # Ensure the subnet has a gateway if not subnet['gateway_ip']: msg = _('Subnet for router interface must have a gateway IP') raise q_exc.BadRequest(resource='router', msg=msg) self._check_for_dup_router_subnet(context, router_id, subnet['network_id'], subnet_id, subnet['cidr']) fixed_ip = {'ip_address': subnet['gateway_ip'], 'subnet_id': subnet['id']} - port = self._core_plugin.create_port(context, { + # elevate context here as it is required to set device_owner + # to ROUTER_INTF + port = self._core_plugin.create_port(context.elevated(), { 'port': {'tenant_id': subnet['tenant_id'], 'network_id': subnet['network_id'], 'fixed_ips': [fixed_ip], 'mac_address': attributes.ATTR_NOT_SPECIFIED, 'admin_state_up': True, 'device_id': router_id, 'device_owner': DEVICE_OWNER_ROUTER_INTF, 'name': ''}}) self.l3_rpc_notifier.routers_updated( context, [router_id], 'add_router_interface') info = {'id': router_id, 'tenant_id': subnet['tenant_id'], 'port_id': port['id'], 'subnet_id': port['fixed_ips'][0]['subnet_id']} notifier_api.notify(context, notifier_api.publisher_id('network'), 'router.interface.create', notifier_api.CONF.default_notification_level, {'router.interface': info}) return info def _confirm_router_interface_not_in_use(self, context, router_id, subnet_id): diff --git a/neutron/tests/unit/cisco/n1kv/test_n1kv_plugin.py b/neutron/tests/unit/cisco/n1kv/test_n1kv_plugin.py index 298074c..78220fd 100644 --- a/neutron/tests/unit/cisco/n1kv/test_n1kv_plugin.py +++ b/neutron/tests/unit/cisco/n1kv/test_n1kv_plugin.py @@ -325,50 +325,58 @@ class TestN1kvPorts(test_plugin.TestPortsV2, def test_create_port_with_n1kv_profile_id(self): """Test port create with policy profile id.""" profile_obj = self._make_test_policy_profile(name='test_profile') with self.network() as network: data = {'port': {n1kv_profile.PROFILE_ID: profile_obj.id, 'tenant_id': self.tenant_id, 'network_id': network['network']['id']}} port_req = self.new_create_request('ports', data) port = self.deserialize(self.fmt, port_req.get_response(self.api)) self.assertEqual(port['port'][n1kv_profile.PROFILE_ID], profile_obj.id) self._delete('ports', port['port']['id']) def test_update_port_with_n1kv_profile_id(self): """Test port update failure while updating policy profile id.""" with self.port() as port: data = {'port': {n1kv_profile.PROFILE_ID: 'some-profile-uuid'}} port_req = self.new_update_request('ports', data, port['port']['id']) res = port_req.get_response(self.api) # Port update should fail to update policy profile id. self.assertEqual(res.status_int, 400) + def test_create_port_device_owner_router_intf(self): + # The cisco plugin does not support this workflow. + pass + + def test_update_port_device_owner_on_router_inf_port(self): + # The cisco plugin does not support this workflow. + pass + class TestN1kvNetworks(test_plugin.TestNetworksV2, N1kvPluginTestCase): def _prepare_net_data(self, net_profile_id): return {'network': {'name': 'net1', n1kv_profile.PROFILE_ID: net_profile_id, 'tenant_id': self.tenant_id}} def test_create_network_with_default_n1kv_profile_id(self): """Test network create without passing network profile id.""" with self.network() as network: db_session = db.get_session() np = n1kv_db_v2.get_network_profile( db_session, network['network'][n1kv_profile.PROFILE_ID]) self.assertEqual(np['name'], 'default_network_profile') def test_create_network_with_n1kv_profile_id(self): """Test network create with network profile id.""" profile_obj = self._make_test_profile(name='test_profile') data = self._prepare_net_data(profile_obj.id) network_req = self.new_create_request('networks', data) network = self.deserialize(self.fmt, network_req.get_response(self.api)) self.assertEqual(network['network'][n1kv_profile.PROFILE_ID], diff --git a/neutron/tests/unit/nicira/test_nicira_plugin.py b/neutron/tests/unit/nicira/test_nicira_plugin.py index bccd9aa..e65a4ef 100644 --- a/neutron/tests/unit/nicira/test_nicira_plugin.py +++ b/neutron/tests/unit/nicira/test_nicira_plugin.py @@ -245,50 +245,57 @@ class TestNiciraPortsV2(NiciraPluginV2TestCase, def test_create_port_neutron_error_no_orphan_left(self): with mock.patch.object(nicira_db, 'add_neutron_nsx_port_mapping', side_effect=ntn_exc.NeutronException): with self.network() as net: net_id = net['network']['id'] self._create_port(self.fmt, net_id, webob.exc.HTTPInternalServerError.code) self._verify_no_orphan_left(net_id) def test_create_port_maintenance_returns_503(self): with self.network() as net: with mock.patch.object(nvplib, 'do_request', side_effect=nvp_exc.MaintenanceInProgress): data = {'port': {'network_id': net['network']['id'], 'admin_state_up': False, 'fixed_ips': [], 'tenant_id': self._tenant_id}} plugin = manager.NeutronManager.get_plugin() with mock.patch.object(plugin, 'get_network', return_value=net['network']): port_req = self.new_create_request('ports', data, self.fmt) res = port_req.get_response(self.api) self.assertEqual(webob.exc.HTTPServiceUnavailable.code, res.status_int) + def test_update_port_device_owner_on_router_inf_port(self): + #TODO(arosen): nvp plugin does not allow one to + # create a port with device_owner network:interface_router + # as this should only happen via router_interface_attach. + # We should support this anyways as the db_base_class supports this. + pass + class TestNiciraNetworksV2(test_plugin.TestNetworksV2, NiciraPluginV2TestCase): def _test_create_bridge_network(self, vlan_id=None): net_type = vlan_id and 'vlan' or 'flat' name = 'bridge_net' expected = [('subnets', []), ('name', name), ('admin_state_up', True), ('status', 'ACTIVE'), ('shared', False), (pnet.NETWORK_TYPE, net_type), (pnet.PHYSICAL_NETWORK, 'tzuuid'), (pnet.SEGMENTATION_ID, vlan_id)] providernet_args = {pnet.NETWORK_TYPE: net_type, pnet.PHYSICAL_NETWORK: 'tzuuid'} if vlan_id: providernet_args[pnet.SEGMENTATION_ID] = vlan_id with self.network(name=name, providernet_args=providernet_args, arg_list=(pnet.NETWORK_TYPE, pnet.PHYSICAL_NETWORK, pnet.SEGMENTATION_ID)) as net: for k, v in expected: self.assertEqual(net['network'][k], v) def test_create_bridge_network(self): diff --git a/neutron/tests/unit/test_db_plugin.py b/neutron/tests/unit/test_db_plugin.py index 1025a80..e279666 100644 --- a/neutron/tests/unit/test_db_plugin.py +++ b/neutron/tests/unit/test_db_plugin.py @@ -1735,50 +1735,111 @@ fixed_ips=ip_address%%3D%s&fixed_ips=ip_address%%3D%s&fixed_ips=subnet_id%%3D%s {'subnet_id': subnet['subnet']['id']}]} net_id = subnet['subnet']['network_id'] res = self._create_port(self.fmt, net_id=net_id, **kwargs) self.assertEqual(res.status_int, webob.exc.HTTPClientError.code) def test_update_max_fixed_ips_exceeded(self): with self.subnet(gateway_ip='10.0.0.3', cidr='10.0.0.0/24') as subnet: with self.port(subnet) as port: data = {'port': {'fixed_ips': [{'subnet_id': subnet['subnet']['id'], 'ip_address': '10.0.0.2'}, {'subnet_id': subnet['subnet']['id'], 'ip_address': '10.0.0.4'}, {'subnet_id': subnet['subnet']['id']}, {'subnet_id': subnet['subnet']['id']}, {'subnet_id': subnet['subnet']['id']}, {'subnet_id': subnet['subnet']['id']}]}} req = self.new_update_request('ports', data, port['port']['id']) res = req.get_response(self.api) self.assertEqual(res.status_int, webob.exc.HTTPClientError.code) + def test_create_port_device_owner_router_intf(self): + network_res = self._create_network( + self.fmt, 'some_net', True, + tenant_id='non_admin', set_context=True) + network = self.deserialize(self.fmt, network_res) + port_res = self._create_port( + self.fmt, network['network']['id'], + tenant_id='non_admin', + device_owner='network:router_interface', + set_context=True) + + self.assertEqual(port_res.status_int, webob.exc.HTTPBadRequest.code) + self._delete('networks', network['network']['id']) + + def test_update_port_device_owner_router_intf(self): + network_res = self._create_network(self.fmt, + 'some_net', + True, + tenant_id='non_admin', + set_context=True) + network = self.deserialize(self.fmt, network_res) + port_res = self._create_port( + self.fmt, network['network']['id'], + tenant_id='non_admin', + set_context=True) + port = self.deserialize(self.fmt, port_res) + + data = {'port': {'device_owner': 'network:router_interface'}} + neutron_context = context.Context('', 'non_admin') + self._update('ports', port['port']['id'], data, + neutron_context=neutron_context, + expected_code=webob.exc.HTTPBadRequest.code) + self._delete('ports', port['port']['id']) + self._delete('networks', network['network']['id']) + + def test_update_port_device_owner_on_router_inf_port(self): + network_res = self._create_network(self.fmt, + 'some_net', + True, + tenant_id='non_admin', + set_context=True) + network = self.deserialize(self.fmt, network_res) + + # ports can be created with network:router_interface device_owner + # if done as an admin. + port_res = self._create_port( + self.fmt, network['network']['id'], + tenant_id='non_admin', + device_owner='network:router_interface') + port = self.deserialize(self.fmt, port_res) + + data = {'port': {'device_owner': 'network:router_interface'}} + neutron_context = context.Context('', 'non_admin') + + # ports with this device_owner can be updated to the + # same device_owner. + self._update('ports', port['port']['id'], data, + neutron_context=neutron_context, + expected_code=webob.exc.HTTPOk.code) + self._delete('ports', port['port']['id']) + self._delete('networks', network['network']['id']) class TestNetworksV2(NeutronDbPluginV2TestCase): # NOTE(cerberus): successful network update and delete are # effectively tested above def test_create_network(self): name = 'net1' keys = [('subnets', []), ('name', name), ('admin_state_up', True), ('status', self.net_create_status), ('shared', False)] with self.network(name=name) as net: for k, v in keys: self.assertEqual(net['network'][k], v) def test_create_public_network(self): name = 'public_net' keys = [('subnets', []), ('name', name), ('admin_state_up', True), ('status', self.net_create_status), ('shared', True)] with self.network(name=name, shared=True) as net: for k, v in keys: self.assertEqual(net['network'][k], v) def test_create_public_network_no_admin_tenant(self): name = 'public_net' with testlib_api.ExpectedException( webob.exc.HTTPClientError) as ctx_manager: with self.network(name=name,