From 49b4dd3478d782aee4260033825aa6b47eaf644a Mon Sep 17 00:00:00 2001 From: Kevin Benton Date: Fri, 19 Feb 2016 03:34:27 -0800 Subject: [PATCH] Use network RBAC feature for external access This allows access to external networks to be controlled via the RBAC framework added during Liberty with a new 'access_as_external' action. A migration adds all current external networks to the RBAC policies table with a wildcard indicating that all tenants can access the network as RBAC. Unlike the conversion of shared networks to RBAC, the external table is left in the DB to avoid invasive changes throughout the codebase to calculate the flag relative to the caller. So the current 'external' flag is used throughout the code base as it previously was for wiring up floating IPs, router gateway ports, etc. Then the RBAC entries are only referenced when determining what networks to show the tenants. API Behavior: * Marking a network as 'external' will automatically create a wildcard entry that allows that network to be accessed by all tenants. * An external network may have all of its RBAC entries deleted and then only an admin will be able to attach to it. * An RBAC 'access_as_external' entry cannot be deleted if it is required for a tenant that currently has a router attached to that network. * Creating an 'access_as_external' RBAC entry will automatically convert the network into an external network. (This is to enable a workflow where a private external network is never visible to everyone.) * The default policy.json will prevent a non-admin from creating wildcard 'access_as_external' RBAC entries to align with the current default policy we have on setting the 'external' field on the network to prevent poluting everyone else's network lists. * The default policy.json will allow a tenant to create an 'access_as_external' RBAC entry to allow specific tenants (including itself) the ability to use its network as an external network. Closes-Bug: #1547985 DocImpact: External networks can now have access restricted to small subsets of tenants APIImpact: 'access_as_external' will be allowed as an action in the RBAC API for networks Change-Id: I4d8ee78a9763c58884e4fd3d7b40133da659cd61 --- neutron/db/db_base_plugin_v2.py | 4 +- neutron/db/external_net_db.py | 103 ++++++++++- .../alembic_migrations/versions/CONTRACT_HEAD | 2 +- .../5ffceebfada_rbac_network_external.py | 73 ++++++++ neutron/db/rbac_db_models.py | 8 +- neutron/extensions/rbac.py | 5 +- .../admin/test_external_network_extension.py | 169 ++++++++++++++++++ .../unit/extensions/test_external_net.py | 9 +- ...ess_as_external_rbac-455dc74b9fa22761.yaml | 19 ++ 9 files changed, 382 insertions(+), 10 deletions(-) create mode 100644 neutron/db/migration/alembic_migrations/versions/mitaka/contract/5ffceebfada_rbac_network_external.py create mode 100644 releasenotes/notes/access_as_external_rbac-455dc74b9fa22761.yaml diff --git a/neutron/db/db_base_plugin_v2.py b/neutron/db/db_base_plugin_v2.py index 6cf0ed1f64c..147c5bc4722 100644 --- a/neutron/db/db_base_plugin_v2.py +++ b/neutron/db/db_base_plugin_v2.py @@ -124,8 +124,8 @@ class NeutronDbPluginV2(db_base_plugin_common.DbBasePluginCommon, On update and delete, make sure the tenant losing access does not have resources that depend on that access. """ - if object_type != 'network': - # we only care about network policies + if object_type != 'network' or policy['action'] != 'access_as_shared': + # we only care about shared network policies return # The object a policy targets cannot be changed so we can look # at the original network for the update event as well. diff --git a/neutron/db/external_net_db.py b/neutron/db/external_net_db.py index d23f60ea94e..594040e5999 100644 --- a/neutron/db/external_net_db.py +++ b/neutron/db/external_net_db.py @@ -26,9 +26,12 @@ from neutron.callbacks import resources from neutron.common import constants as l3_constants from neutron.common import exceptions as n_exc from neutron.db import db_base_plugin_v2 +from neutron.db import l3_db from neutron.db import model_base from neutron.db import models_v2 +from neutron.db import rbac_db_models as rbac_db from neutron.extensions import external_net +from neutron.extensions import rbac as rbac_ext from neutron import manager from neutron.plugins.common import constants as service_constants @@ -65,8 +68,14 @@ class External_net_db_mixin(object): # Apply the external network filter only in non-admin and non-advsvc # context if self.model_query_scope(context, original_model): - conditions = expr.or_(ExternalNetwork.network_id != expr.null(), - *conditions) + # the table will already be joined to the rbac entries for the + # shared check so we don't need to worry about ensuring that + rbac_model = original_model.rbac_entries.property.mapper.class_ + tenant_allowed = ( + (rbac_model.action == 'access_as_external') & + (rbac_model.target_tenant == context.tenant_id) | + (rbac_model.target_tenant == '*')) + conditions = expr.or_(tenant_allowed, *conditions) return conditions def _network_result_filter_hook(self, query, filters): @@ -124,13 +133,16 @@ class External_net_db_mixin(object): # raise the underlying exception raise e.errors[0].error context.session.add(ExternalNetwork(network_id=net_data['id'])) + context.session.add(rbac_db.NetworkRBAC( + object_id=net_data['id'], action='access_as_external', + target_tenant='*', tenant_id=net_data['tenant_id'])) registry.notify( resources.EXTERNAL_NETWORK, events.AFTER_CREATE, self, context=context, request=req_data, network=net_data) net_data[external_net.EXTERNAL] = external - def _process_l3_update(self, context, net_data, req_data): + def _process_l3_update(self, context, net_data, req_data, allow_all=True): try: registry.notify( resources.EXTERNAL_NETWORK, events.BEFORE_UPDATE, @@ -151,6 +163,10 @@ class External_net_db_mixin(object): if new_value: context.session.add(ExternalNetwork(network_id=net_id)) net_data[external_net.EXTERNAL] = True + if allow_all: + context.session.add(rbac_db.NetworkRBAC( + object_id=net_id, action='access_as_external', + target_tenant='*', tenant_id=net_data['tenant_id'])) else: # must make sure we do not have any external gateway ports # (and thus, possible floating IPs) on this network before @@ -163,6 +179,8 @@ class External_net_db_mixin(object): context.session.query(ExternalNetwork).filter_by( network_id=net_id).delete() + context.session.query(rbac_db.NetworkRBAC).filter_by( + object_id=net_id, action='access_as_external').delete() net_data[external_net.EXTERNAL] = False def _process_l3_delete(self, context, network_id): @@ -177,3 +195,82 @@ class External_net_db_mixin(object): raise n_exc.TooManyExternalNetworks() else: return nets[0]['id'] if nets else None + + def _process_ext_policy_create(self, resource, event, trigger, context, + object_type, policy, **kwargs): + if (object_type != 'network' or + policy['action'] != 'access_as_external'): + return + net = self.get_network(context, policy['object_id']) + if not context.is_admin and net['tenant_id'] != context.tenant_id: + msg = _("Only admins can manipulate policies on networks they " + "do not own.") + raise n_exc.InvalidInput(error_message=msg) + if not self._network_is_external(context, policy['object_id']): + # we automatically convert the network into an external network + self._process_l3_update(context, net, + {external_net.EXTERNAL: True}, + allow_all=False) + + def _validate_ext_not_in_use_by_tenant(self, resource, event, trigger, + context, object_type, policy, + **kwargs): + if (object_type != 'network' or + policy['action'] != 'access_as_external'): + return + if event == events.BEFORE_UPDATE: + new_tenant = kwargs['policy_tenant']['target_tenant'] + if new_tenant == policy['target_tenant']: + # nothing to validate if the tenant didn't change + return + ports = context.session.query(models_v2.Port.id).filter_by( + device_owner=DEVICE_OWNER_ROUTER_GW, + network_id=policy['object_id']) + router = context.session.query(l3_db.Router).filter( + l3_db.Router.gw_port_id.in_(ports)) + rbac = rbac_db.NetworkRBAC + if policy['target_tenant'] != '*': + router = router.filter( + l3_db.Router.tenant_id == policy['target_tenant']) + # if there is a wildcard entry we can safely proceed without the + # router lookup because they will have access either way + if context.session.query(rbac_db.NetworkRBAC).filter( + rbac.object_id == policy['object_id'], + rbac.action == 'access_as_external', + rbac.target_tenant == '*').count(): + return + else: + # deleting the wildcard is okay as long as the tenants with + # attached routers have their own entries and the network is + # not the default external network. + is_default = context.session.query(ExternalNetwork).filter_by( + network_id=policy['object_id'], is_default=True).count() + if is_default: + msg = _("Default external networks must be shared to " + "everyone.") + raise rbac_ext.RbacPolicyInUse(object_id=policy['object_id'], + details=msg) + tenants_with_entries = ( + context.session.query(rbac.target_tenant). + filter(rbac.object_id == policy['object_id'], + rbac.action == 'access_as_external', + rbac.target_tenant != '*')) + router = router.filter( + ~l3_db.Router.tenant_id.in_(tenants_with_entries)) + if router.count(): + msg = _("There are routers attached to this network that " + "depend on this policy for access.") + raise rbac_ext.RbacPolicyInUse(object_id=policy['object_id'], + details=msg) + + def _register_external_net_rbac_hooks(self): + registry.subscribe(self._process_ext_policy_create, + 'rbac-policy', events.BEFORE_CREATE) + for e in (events.BEFORE_UPDATE, events.BEFORE_DELETE): + registry.subscribe(self._validate_ext_not_in_use_by_tenant, + 'rbac-policy', e) + + def __new__(cls, *args, **kwargs): + new = super(External_net_db_mixin, cls).__new__(cls, *args, **kwargs) + new._register_external_net_rbac_hooks() + return new diff --git a/neutron/db/migration/alembic_migrations/versions/CONTRACT_HEAD b/neutron/db/migration/alembic_migrations/versions/CONTRACT_HEAD index 46d3ecf98bd..d1b0015881f 100644 --- a/neutron/db/migration/alembic_migrations/versions/CONTRACT_HEAD +++ b/neutron/db/migration/alembic_migrations/versions/CONTRACT_HEAD @@ -1 +1 @@ -c6c112992c9 +5ffceebfada diff --git a/neutron/db/migration/alembic_migrations/versions/mitaka/contract/5ffceebfada_rbac_network_external.py b/neutron/db/migration/alembic_migrations/versions/mitaka/contract/5ffceebfada_rbac_network_external.py new file mode 100644 index 00000000000..b2b35225cf4 --- /dev/null +++ b/neutron/db/migration/alembic_migrations/versions/mitaka/contract/5ffceebfada_rbac_network_external.py @@ -0,0 +1,73 @@ +# Copyright 2015 OpenStack Foundation +# +# 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. +# + +"""network_rbac_external + +Revision ID: 5ffceebfada +Revises: c6c112992c9 +Create Date: 2015-06-14 13:12:04.012457 + +""" + +# revision identifiers, used by Alembic. +revision = '5ffceebfada' +down_revision = 'c6c112992c9' +depends_on = () + +from alembic import op +from oslo_utils import uuidutils +import sqlalchemy as sa + +from neutron.api.v2 import attributes + + +# A simple model of the external network table with only the fields needed for +# the migration. +external = sa.Table('externalnetworks', sa.MetaData(), + sa.Column('network_id', sa.String(length=36), + nullable=False)) +TENANT_ID_MAX_LEN = attributes.TENANT_ID_MAX_LEN +network = sa.Table('networks', sa.MetaData(), + sa.Column('id', sa.String(length=36), nullable=False), + sa.Column('tenant_id', sa.String(length=TENANT_ID_MAX_LEN))) + +networkrbacs = sa.Table( + 'networkrbacs', sa.MetaData(), + sa.Column('id', sa.String(length=36), nullable=False), + sa.Column('object_id', sa.String(length=36), nullable=False), + sa.Column('tenant_id', sa.String(length=TENANT_ID_MAX_LEN), nullable=True, + index=True), + sa.Column('target_tenant', sa.String(length=TENANT_ID_MAX_LEN), + nullable=False), + sa.Column('action', sa.String(length=255), nullable=False)) + + +def upgrade(): + op.bulk_insert(networkrbacs, get_values()) + + +def get_values(): + session = sa.orm.Session(bind=op.get_bind()) + values = [] + net_to_tenant_id = {} + for row in session.query(network).all(): + net_to_tenant_id[row[0]] = row[1] + for row in session.query(external).all(): + values.append({'id': uuidutils.generate_uuid(), 'object_id': row[0], + 'tenant_id': net_to_tenant_id[row[0]], + 'target_tenant': '*', 'action': 'access_as_external'}) + # this commit appears to be necessary to allow further operations + session.commit() + return values diff --git a/neutron/db/rbac_db_models.py b/neutron/db/rbac_db_models.py index ef0c8adf9da..c7f176db299 100644 --- a/neutron/db/rbac_db_models.py +++ b/neutron/db/rbac_db_models.py @@ -23,9 +23,11 @@ from neutron._i18n import _ from neutron.api.v2 import attributes as attr from neutron.common import exceptions as n_exc from neutron.db import model_base +from neutron import manager ACCESS_SHARED = 'access_as_shared' +ACCESS_EXTERNAL = 'access_as_external' class InvalidActionForType(n_exc.InvalidInput): @@ -94,7 +96,11 @@ class NetworkRBAC(RBACColumns, model_base.BASEV2): object_type = 'network' def get_valid_actions(self): - return (ACCESS_SHARED,) + actions = (ACCESS_SHARED,) + pl = manager.NeutronManager.get_plugin() + if 'external-net' in pl.supported_extension_aliases: + actions += (ACCESS_EXTERNAL,) + return actions class QosPolicyRBAC(RBACColumns, model_base.BASEV2): diff --git a/neutron/extensions/rbac.py b/neutron/extensions/rbac.py index fb3b86a0fff..c74d794af7d 100644 --- a/neutron/extensions/rbac.py +++ b/neutron/extensions/rbac.py @@ -70,7 +70,10 @@ RESOURCE_ATTRIBUTE_MAP = { # action depends on type so validation has to occur in # the extension 'validate': {'type:string': attr.DESCRIPTION_MAX_LEN}, - 'is_visible': True}, + # we set enforce_policy so operators can define policies + # that restrict actions + 'is_visible': True, 'enforce_policy': True, + 'default': None}, } } diff --git a/neutron/tests/api/admin/test_external_network_extension.py b/neutron/tests/api/admin/test_external_network_extension.py index 8dc236bf298..87fa50982d9 100644 --- a/neutron/tests/api/admin/test_external_network_extension.py +++ b/neutron/tests/api/admin/test_external_network_extension.py @@ -10,8 +10,11 @@ # License for the specific language governing permissions and limitations # under the License. +from oslo_config import cfg from tempest.lib.common.utils import data_utils +from tempest.lib import exceptions as lib_exc from tempest import test +import testtools from neutron.tests.api import base @@ -125,3 +128,169 @@ class ExternalNetworksTestJSON(base.BaseAdminNetworkTest): (s['id'] for s in subnet_list)) # Removes subnet from the cleanup list self.subnets.remove(subnet) + + +class ExternalNetworksRBACTestJSON(base.BaseAdminNetworkTest): + + credentials = ['primary', 'alt', 'admin'] + + @classmethod + def resource_setup(cls): + if not test.is_extension_enabled('rbac_policies', 'network'): + msg = "rbac extension not enabled." + raise cls.skipException(msg) + super(ExternalNetworksRBACTestJSON, cls).resource_setup() + cls.client2 = cls.alt_manager.network_client + + def _create_network(self, external=True): + post_body = {'name': data_utils.rand_name('network-')} + if external: + post_body['router:external'] = external + body = self.admin_client.create_network(**post_body) + network = body['network'] + self.addCleanup(self.admin_client.delete_network, network['id']) + return network + + @test.attr(type='smoke') + @test.idempotent_id('afd8f1b7-a81e-4629-bca8-a367b3a144bb') + def test_regular_client_shares_with_another(self): + net = self.create_network() + self.client.create_rbac_policy( + object_type='network', object_id=net['id'], + action='access_as_external', + target_tenant=self.client2.tenant_id) + body = self.client2.list_networks() + networks_list = [n['id'] for n in body['networks']] + self.assertIn(net['id'], networks_list) + r = self.client2.create_router( + data_utils.rand_name('router-'), + external_gateway_info={'network_id': net['id']})['router'] + self.addCleanup(self.admin_client.delete_router, r['id']) + + @test.idempotent_id('afd8f1b7-a81e-4629-bca8-a367b3a144bb') + def test_regular_client_blocked_from_creating_external_wild_policies(self): + net = self.create_network() + with testtools.ExpectedException(lib_exc.Forbidden): + self.client.create_rbac_policy( + object_type='network', object_id=net['id'], + action='access_as_external', + target_tenant='*') + + @test.attr(type='smoke') + @test.idempotent_id('a2e19f06-48a9-4e4c-b717-08cb2008707d') + def test_wildcard_policy_created_from_external_network_api(self): + # create external makes wildcard + net_id = self._create_network(external=True)['id'] + self.assertEqual(1, len(self.admin_client.list_rbac_policies( + object_id=net_id, action='access_as_external', + target_tenant='*')['rbac_policies'])) + # update to non-external clears wildcard + self.admin_client.update_network(net_id, **{'router:external': False}) + self.assertEqual(0, len(self.admin_client.list_rbac_policies( + object_id=net_id, action='access_as_external', + target_tenant='*')['rbac_policies'])) + # create non-external has no wildcard + net_id = self._create_network(external=False)['id'] + self.assertEqual(0, len(self.admin_client.list_rbac_policies( + object_id=net_id, action='access_as_external', + target_tenant='*')['rbac_policies'])) + # update to external makes wildcard + self.admin_client.update_network(net_id, **{'router:external': True}) + self.assertEqual(1, len(self.admin_client.list_rbac_policies( + object_id=net_id, action='access_as_external', + target_tenant='*')['rbac_policies'])) + + @test.idempotent_id('a5539002-5bdb-48b5-b124-e9eedd5975e6') + def test_external_conversion_on_policy_create(self): + net_id = self._create_network(external=False)['id'] + self.admin_client.create_rbac_policy( + object_type='network', object_id=net_id, + action='access_as_external', + target_tenant=self.client2.tenant_id) + body = self.admin_client.show_network(net_id)['network'] + self.assertTrue(body['router:external']) + + @test.idempotent_id('01364c50-bfb6-46c4-b44c-edc4564d61cf') + def test_policy_allows_tenant_to_allocate_floatingip(self): + net = self._create_network(external=False) + # share to the admin client so it gets converted to external but + # not shared to everyone + self.admin_client.create_rbac_policy( + object_type='network', object_id=net['id'], + action='access_as_external', + target_tenant=self.admin_client.tenant_id) + self.create_subnet(net, client=self.admin_client, enable_dhcp=False) + with testtools.ExpectedException(lib_exc.NotFound): + self.client2.create_floatingip( + floating_network_id=net['id']) + self.admin_client.create_rbac_policy( + object_type='network', object_id=net['id'], + action='access_as_external', + target_tenant=self.client2.tenant_id) + self.client2.create_floatingip( + floating_network_id=net['id']) + + @test.idempotent_id('476be1e0-f72e-47dc-9a14-4435926bbe82') + def test_policy_allows_tenant_to_attach_ext_gw(self): + net = self._create_network(external=False) + self.create_subnet(net, client=self.admin_client, enable_dhcp=False) + self.admin_client.create_rbac_policy( + object_type='network', object_id=net['id'], + action='access_as_external', + target_tenant=self.client2.tenant_id) + r = self.client2.create_router( + data_utils.rand_name('router-'), + external_gateway_info={'network_id': net['id']})['router'] + self.addCleanup(self.admin_client.delete_router, r['id']) + + @test.idempotent_id('d54decee-4203-4ced-91a2-ea42ca63e154') + def test_delete_policies_while_tenant_attached_to_net(self): + net = self._create_network(external=False) + self.create_subnet(net, client=self.admin_client, enable_dhcp=False) + wildcard = self.admin_client.create_rbac_policy( + object_type='network', object_id=net['id'], + action='access_as_external', + target_tenant='*')['rbac_policy'] + r = self.client2.create_router( + data_utils.rand_name('router-'), + external_gateway_info={'network_id': net['id']})['router'] + # delete should fail because the wildcard is required for the tenant's + # access + with testtools.ExpectedException(lib_exc.Conflict): + self.admin_client.delete_rbac_policy(wildcard['id']) + tenant = self.admin_client.create_rbac_policy( + object_type='network', object_id=net['id'], + action='access_as_external', + target_tenant=self.client2.tenant_id)['rbac_policy'] + # now we can delete the policy because the tenant has its own policy + # to allow it access + self.admin_client.delete_rbac_policy(wildcard['id']) + # but now we can't delete the tenant's policy without the wildcard + with testtools.ExpectedException(lib_exc.Conflict): + self.admin_client.delete_rbac_policy(tenant['id']) + wildcard = self.admin_client.create_rbac_policy( + object_type='network', object_id=net['id'], + action='access_as_external', + target_tenant='*')['rbac_policy'] + # with the wildcard added back we can delete the tenant's policy + self.admin_client.delete_rbac_policy(tenant['id']) + self.admin_client.delete_router(r['id']) + # now without the tenant attached, the wildcard can be deleted + self.admin_client.delete_rbac_policy(wildcard['id']) + # finally we ensure that the tenant can't attach to the network since + # there are no policies allowing it + with testtools.ExpectedException(lib_exc.NotFound): + self.client2.create_router( + data_utils.rand_name('router-'), + external_gateway_info={'network_id': net['id']}) + + @test.idempotent_id('7041cec7-d8fe-4c78-9b04-b51b2fd49dc9') + def test_wildcard_policy_delete_blocked_on_default_ext(self): + public_net_id = cfg.CONF.network.public_network_id + # ensure it is default before so we don't wipe out the policy + self.admin_client.update_network(public_net_id, is_default=True) + policy = self.admin_client.list_rbac_policies( + object_id=public_net_id, action='access_as_external', + target_tenant='*')['rbac_policies'][0] + with testtools.ExpectedException(lib_exc.Conflict): + self.admin_client.delete_rbac_policy(policy['id']) diff --git a/neutron/tests/unit/extensions/test_external_net.py b/neutron/tests/unit/extensions/test_external_net.py index ef862d248ba..0e706836060 100644 --- a/neutron/tests/unit/extensions/test_external_net.py +++ b/neutron/tests/unit/extensions/test_external_net.py @@ -141,12 +141,17 @@ class ExtNetDBTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): plugin = manager.NeutronManager.get_plugin() ctx = context.Context('edinson', 'cavani') model = models_v2.Network - txt = "externalnetworks.network_id IS NOT NULL" + txt = ("networkrbacs.action = :action_1 AND " + "networkrbacs.target_tenant = :target_tenant_1 OR " + "networkrbacs.target_tenant = :target_tenant_2") conditions = plugin._network_filter_hook(ctx, model, []) self.assertEqual(conditions.__str__(), txt) # Try to concatenate conditions + txt2 = (txt.replace('tenant_1', 'tenant_3'). + replace('tenant_2', 'tenant_4'). + replace('action_1', 'action_2')) conditions = plugin._network_filter_hook(ctx, model, conditions) - self.assertEqual(conditions.__str__(), "%s OR %s" % (txt, txt)) + self.assertEqual(conditions.__str__(), "%s OR %s" % (txt, txt2)) def test_create_port_external_network_non_admin_fails(self): with self.network(router__external=True) as ext_net: diff --git a/releasenotes/notes/access_as_external_rbac-455dc74b9fa22761.yaml b/releasenotes/notes/access_as_external_rbac-455dc74b9fa22761.yaml new file mode 100644 index 00000000000..0fdfe804bbf --- /dev/null +++ b/releasenotes/notes/access_as_external_rbac-455dc74b9fa22761.yaml @@ -0,0 +1,19 @@ +--- +prelude: > + External networks can now be controlled using the + RBAC framework that was added in Liberty. This allows + networks to be made available to specific tenants + (as opposed to all tenants) to be used as an external + gateway for routers and floating IPs. +features: + - External networks can now be controlled using the + RBAC framework that was added in Liberty. This allows + networks to be made available to specific tenants + (as opposed to all tenants) to be used as an external + gateway for routers and floating IPs. + By default this feature will also allow regular tenants + to make their networks available as external networks + to other individual tenants (or even themselves), but + they are prevented from using the wildcard to share to + all tenants. This behavior can be adjusted via + policy.json by the operator if desired.