From 778a1d162bbb8032e319d2bc2ae99c20339e1a47 Mon Sep 17 00:00:00 2001 From: Chris Behrens Date: Wed, 21 Sep 2011 06:40:52 +0000 Subject: [PATCH 1/4] floating_ip_get_by_address should check user's project_id --- nova/db/sqlalchemy/api.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index 52325884149f..a968e1f4bda8 100644 --- a/nova/db/sqlalchemy/api.py +++ b/nova/db/sqlalchemy/api.py @@ -532,7 +532,6 @@ def floating_ip_count_by_project(context, project_id): def floating_ip_fixed_ip_associate(context, floating_address, fixed_address): session = get_session() with session.begin(): - # TODO(devcamcar): How to ensure floating_id belongs to user? floating_ip_ref = floating_ip_get_by_address(context, floating_address, session=session) @@ -547,7 +546,6 @@ def floating_ip_fixed_ip_associate(context, floating_address, fixed_address): def floating_ip_deallocate(context, address): session = get_session() with session.begin(): - # TODO(devcamcar): How to ensure floating id belongs to user? floating_ip_ref = floating_ip_get_by_address(context, address, session=session) @@ -561,7 +559,6 @@ def floating_ip_deallocate(context, address): def floating_ip_destroy(context, address): session = get_session() with session.begin(): - # TODO(devcamcar): Ensure address belongs to user. floating_ip_ref = floating_ip_get_by_address(context, address, session=session) @@ -572,8 +569,6 @@ def floating_ip_destroy(context, address): def floating_ip_disassociate(context, address): session = get_session() with session.begin(): - # TODO(devcamcar): Ensure address belongs to user. - # Does get_floating_ip_by_address handle this? floating_ip_ref = floating_ip_get_by_address(context, address, session=session) @@ -641,15 +636,20 @@ def floating_ip_get_all_by_project(context, project_id): @require_context def floating_ip_get_by_address(context, address, session=None): - # TODO(devcamcar): Ensure the address belongs to user. if not session: session = get_session() - result = session.query(models.FloatingIp).\ + query = session.query(models.FloatingIp).\ options(joinedload_all('fixed_ip.network')).\ - filter_by(address=address).\ - filter_by(deleted=can_read_deleted(context)).\ - first() + filter_by(address=address) + + if is_admin_context(context): + query = query.filter_by(deleted=can_read_deleted(context))) + elif is_user_context(context): + query = query.filter_by(project_id=context.project_id).\ + filter_by(deleted=False) + + result = query.first() if not result: raise exception.FloatingIpNotFoundForAddress(address=address) return result From f752e712b7710b921f332c5c8459a29e064e8681 Mon Sep 17 00:00:00 2001 From: Chris Behrens Date: Wed, 21 Sep 2011 08:27:48 +0000 Subject: [PATCH 2/4] actions on floating IPs in other projects for non-admins should not be allowed. --- nova/api/ec2/__init__.py | 4 ++++ nova/db/sqlalchemy/api.py | 2 +- nova/tests/test_network.py | 46 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 1 deletion(-) diff --git a/nova/api/ec2/__init__.py b/nova/api/ec2/__init__.py index 3b217e62e5ea..144aa3811379 100644 --- a/nova/api/ec2/__init__.py +++ b/nova/api/ec2/__init__.py @@ -437,6 +437,10 @@ class Executor(wsgi.Application): LOG.debug(_('InvalidPortRange raised: %s'), unicode(ex), context=context) return self._error(req, context, type(ex).__name__, unicode(ex)) + except exception.NotAuthorized as ex: + LOG.info(_('NotAuthorized raised: %s'), unicode(ex), + context=context) + return self._error(req, context, type(ex).__name__, unicode(ex)) except Exception as ex: extra = {'environment': req.environ} LOG.exception(_('Unexpected error raised: %s'), unicode(ex), diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index a968e1f4bda8..dfdf136e2150 100644 --- a/nova/db/sqlalchemy/api.py +++ b/nova/db/sqlalchemy/api.py @@ -644,7 +644,7 @@ def floating_ip_get_by_address(context, address, session=None): filter_by(address=address) if is_admin_context(context): - query = query.filter_by(deleted=can_read_deleted(context))) + query = query.filter_by(deleted=can_read_deleted(context)) elif is_user_context(context): query = query.filter_by(project_id=context.project_id).\ filter_by(deleted=False) diff --git a/nova/tests/test_network.py b/nova/tests/test_network.py index 3bdcdbb65264..06c6b2f66968 100644 --- a/nova/tests/test_network.py +++ b/nova/tests/test_network.py @@ -446,6 +446,52 @@ class VlanNetworkTestCase(test.TestCase): self.network.add_fixed_ip_to_instance(self.context, 1, HOST, networks[0]['id']) + def test_ip_association_and_allocation_of_other_project(self): + """Makes sure that we cannot deallocaate or disassociate + a public ip of other project""" + + context1 = context.RequestContext('user', 'project1') + context2 = context.RequestContext('user', 'project2') + + address = '1.2.3.4' + float_addr = db.floating_ip_create(context1.elevated(), + {'address': address, + 'project_id': context1.project_id}) + + instance = db.instance_create(context1, + {'project_id': 'project1'}) + + fix_addr = db.fixed_ip_associate_pool(context1.elevated(), + 1, instance['id']) + + # Associate the IP with non-admin user context + self.assertRaises(exception.FloatingIpNotFoundForAddress, + self.network.associate_floating_ip, + context2, + float_addr, + fix_addr) + + # Deallocate address from other project + self.assertRaises(exception.FloatingIpNotFoundForAddress, + self.network.deallocate_floating_ip, + context2, + float_addr) + + # Now Associates the address to the actual project + self.network.associate_floating_ip(context1, float_addr, fix_addr) + + # Now try dis-associating from other project + self.assertRaises(exception.FloatingIpNotFoundForAddress, + self.network.disassociate_floating_ip, + context2, + float_addr) + + # Clean up the ip addresses + self.network.deallocate_floating_ip(context1, float_addr) + self.network.deallocate_fixed_ip(context1, fix_addr) + db.floating_ip_destroy(context1.elevated(), float_addr) + db.fixed_ip_disassociate(context1.elevated(), fix_addr) + class CommonNetworkTestCase(test.TestCase): From aff43d206a679c1b81904a72cb2e4fb6dadbd515 Mon Sep 17 00:00:00 2001 From: Chris Behrens Date: Wed, 21 Sep 2011 08:37:54 +0000 Subject: [PATCH 3/4] floating ip could have no project and we should allow access --- nova/db/sqlalchemy/api.py | 21 +++++++++++---------- nova/tests/test_network.py | 6 +++--- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index dfdf136e2150..6aebe22f6663 100644 --- a/nova/db/sqlalchemy/api.py +++ b/nova/db/sqlalchemy/api.py @@ -639,19 +639,20 @@ def floating_ip_get_by_address(context, address, session=None): if not session: session = get_session() - query = session.query(models.FloatingIp).\ - options(joinedload_all('fixed_ip.network')).\ - filter_by(address=address) + result = session.query(models.FloatingIp).\ + options(joinedload_all('fixed_ip.network')).\ + filter_by(address=address).\ + filter_by(deleted=can_read_deleted(context)).\ + first() - if is_admin_context(context): - query = query.filter_by(deleted=can_read_deleted(context)) - elif is_user_context(context): - query = query.filter_by(project_id=context.project_id).\ - filter_by(deleted=False) - - result = query.first() if not result: raise exception.FloatingIpNotFoundForAddress(address=address) + + # If the floating IP has a project ID set, check to make sure + # the non-admin user has access. + if result.project_id and is_user_context(context): + authorize_project_context(context, result.project_id) + return result diff --git a/nova/tests/test_network.py b/nova/tests/test_network.py index 06c6b2f66968..e4fa3a4175ac 100644 --- a/nova/tests/test_network.py +++ b/nova/tests/test_network.py @@ -465,14 +465,14 @@ class VlanNetworkTestCase(test.TestCase): 1, instance['id']) # Associate the IP with non-admin user context - self.assertRaises(exception.FloatingIpNotFoundForAddress, + self.assertRaises(exception.NotAuthorized, self.network.associate_floating_ip, context2, float_addr, fix_addr) # Deallocate address from other project - self.assertRaises(exception.FloatingIpNotFoundForAddress, + self.assertRaises(exception.NotAuthorized, self.network.deallocate_floating_ip, context2, float_addr) @@ -481,7 +481,7 @@ class VlanNetworkTestCase(test.TestCase): self.network.associate_floating_ip(context1, float_addr, fix_addr) # Now try dis-associating from other project - self.assertRaises(exception.FloatingIpNotFoundForAddress, + self.assertRaises(exception.NotAuthorized, self.network.disassociate_floating_ip, context2, float_addr) From 4d0bb8730a076b44d0a37fd0770c743b834e5751 Mon Sep 17 00:00:00 2001 From: Chris Behrens Date: Wed, 21 Sep 2011 08:47:33 +0000 Subject: [PATCH 4/4] update floating ips tests --- nova/api/openstack/contrib/floating_ips.py | 7 ++++++- nova/tests/api/openstack/contrib/test_floating_ips.py | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/nova/api/openstack/contrib/floating_ips.py b/nova/api/openstack/contrib/floating_ips.py index d078b26c6527..8b5b19c2120f 100644 --- a/nova/api/openstack/contrib/floating_ips.py +++ b/nova/api/openstack/contrib/floating_ips.py @@ -144,6 +144,8 @@ class Floating_ips(extensions.ExtensionDescriptor): address) except exception.ApiError, e: raise webob.exc.HTTPBadRequest(explanation=e.message) + except exception.NotAuthorized, e: + raise webob.exc.HTTPUnauthorized() return webob.Response(status_int=202) @@ -162,7 +164,10 @@ class Floating_ips(extensions.ExtensionDescriptor): floating_ip = self.network_api.get_floating_ip_by_ip(context, address) if floating_ip.get('fixed_ip'): - self.network_api.disassociate_floating_ip(context, address) + try: + self.network_api.disassociate_floating_ip(context, address) + except exception.NotAuthorized, e: + raise webob.exc.HTTPUnauthorized() return webob.Response(status_int=202) diff --git a/nova/tests/api/openstack/contrib/test_floating_ips.py b/nova/tests/api/openstack/contrib/test_floating_ips.py index 0744f0a11bbd..d4e08b30307d 100644 --- a/nova/tests/api/openstack/contrib/test_floating_ips.py +++ b/nova/tests/api/openstack/contrib/test_floating_ips.py @@ -278,7 +278,7 @@ class FloatingIpTest(test.TestCase): req.body = json.dumps(body) req.headers["content-type"] = "application/json" resp = req.get_response(fakes.wsgi_app()) - self.assertEqual(resp.status_int, 400) + self.assertEqual(resp.status_int, 401) def test_associate_floating_ip_to_instance_no_project_id(self): def fake_fixed_ip_get_by_address(ctx, address, session=None):