From 8799cb942aad2e95e74f18aeaa416b07382c34d5 Mon Sep 17 00:00:00 2001 From: Aaron Rosen Date: Mon, 7 Oct 2013 15:34:38 -0700 Subject: [PATCH] Add X-Tenant-ID to metadata request Previously, one could update a port's device_id to be that of another tenant's instance_id and then be able to retrieve that instance's metadata. In order to prevent this X-Tenant-ID is now passed in the metadata request to nova and nova then checks that X-Tenant-ID also matches the tenant_id for the instance against it's database to ensure it's not being spoofed. DocImpact - When upgrading OpenStack nova and neturon, neutron should be updated first (and neutron-metadata-agent restarted before nova is upgraded) in order to minimize downtime. This is because there is also a patch to nova which has checks X-Tenant-ID against it's database therefore neutron-metadata-agent needs to pass that before nova is upgraded for metadata to work. Change-Id: I2b8fa2f561a7f2914608e68133abf15efa95015a Closes-Bug: #1235450 --- neutron/agent/metadata/agent.py | 13 ++--- neutron/tests/unit/test_metadata_agent.py | 62 +++++++++++++---------- 2 files changed, 43 insertions(+), 32 deletions(-) diff --git a/neutron/agent/metadata/agent.py b/neutron/agent/metadata/agent.py index 60fcc4bc3a..4be089c6ff 100644 --- a/neutron/agent/metadata/agent.py +++ b/neutron/agent/metadata/agent.py @@ -97,9 +97,9 @@ class MetadataProxyHandler(object): try: LOG.debug(_("Request: %s"), req) - instance_id = self._get_instance_id(req) + instance_id, tenant_id = self._get_instance_and_tenant_id(req) if instance_id: - return self._proxy_request(instance_id, req) + return self._proxy_request(instance_id, tenant_id, req) else: return webob.exc.HTTPNotFound() @@ -109,7 +109,7 @@ class MetadataProxyHandler(object): 'Please try your request again.') return webob.exc.HTTPInternalServerError(explanation=unicode(msg)) - def _get_instance_id(self, req): + def _get_instance_and_tenant_id(self, req): qclient = self._get_neutron_client() remote_address = req.headers.get('X-Forwarded-For') @@ -130,14 +130,15 @@ class MetadataProxyHandler(object): fixed_ips=['ip_address=%s' % remote_address])['ports'] self.auth_info = qclient.get_auth_info() - if len(ports) == 1: - return ports[0]['device_id'] + return ports[0]['device_id'], ports[0]['tenant_id'] + return None, None - def _proxy_request(self, instance_id, req): + def _proxy_request(self, instance_id, tenant_id, req): headers = { 'X-Forwarded-For': req.headers.get('X-Forwarded-For'), 'X-Instance-ID': instance_id, + 'X-Tenant-ID': tenant_id, 'X-Instance-ID-Signature': self._sign_instance_id(instance_id) } diff --git a/neutron/tests/unit/test_metadata_agent.py b/neutron/tests/unit/test_metadata_agent.py index f944676dfc..8eb3784491 100644 --- a/neutron/tests/unit/test_metadata_agent.py +++ b/neutron/tests/unit/test_metadata_agent.py @@ -55,8 +55,9 @@ class TestMetadataProxyHandler(base.BaseTestCase): def test_call(self): req = mock.Mock() - with mock.patch.object(self.handler, '_get_instance_id') as get_id: - get_id.return_value = 'id' + with mock.patch.object(self.handler, + '_get_instance_and_tenant_id') as get_ids: + get_ids.return_value = ('instance_id', 'tenant_id') with mock.patch.object(self.handler, '_proxy_request') as proxy: proxy.return_value = 'value' @@ -65,21 +66,23 @@ class TestMetadataProxyHandler(base.BaseTestCase): def test_call_no_instance_match(self): req = mock.Mock() - with mock.patch.object(self.handler, '_get_instance_id') as get_id: - get_id.return_value = None + with mock.patch.object(self.handler, + '_get_instance_and_tenant_id') as get_ids: + get_ids.return_value = None, None retval = self.handler(req) self.assertIsInstance(retval, webob.exc.HTTPNotFound) def test_call_internal_server_error(self): req = mock.Mock() - with mock.patch.object(self.handler, '_get_instance_id') as get_id: - get_id.side_effect = Exception + with mock.patch.object(self.handler, + '_get_instance_and_tenant_id') as get_ids: + get_ids.side_effect = Exception retval = self.handler(req) self.assertIsInstance(retval, webob.exc.HTTPInternalServerError) self.assertEqual(len(self.log.mock_calls), 2) - def _get_instance_id_helper(self, headers, list_ports_retval, - networks=None, router_id=None): + def _get_instance_and_tenant_id_helper(self, headers, list_ports_retval, + networks=None, router_id=None): headers['X-Forwarded-For'] = '192.168.1.1' req = mock.Mock(headers=headers) @@ -87,8 +90,7 @@ class TestMetadataProxyHandler(base.BaseTestCase): return {'ports': list_ports_retval.pop(0)} self.qclient.return_value.list_ports.side_effect = mock_list_ports - retval = self.handler._get_instance_id(req) - + instance_id, tenant_id = self.handler._get_instance_and_tenant_id(req) expected = [ mock.call( username=FakeConf.admin_user, @@ -118,7 +120,7 @@ class TestMetadataProxyHandler(base.BaseTestCase): self.qclient.assert_has_calls(expected) - return retval + return (instance_id, tenant_id) def test_get_instance_id_router_id(self): router_id = 'the_id' @@ -129,13 +131,14 @@ class TestMetadataProxyHandler(base.BaseTestCase): networks = ['net1', 'net2'] ports = [ [{'network_id': 'net1'}, {'network_id': 'net2'}], - [{'device_id': 'device_id'}] + [{'device_id': 'device_id', 'tenant_id': 'tenant_id'}] ] self.assertEqual( - self._get_instance_id_helper(headers, ports, networks=networks, - router_id=router_id), - 'device_id' + self._get_instance_and_tenant_id_helper(headers, ports, + networks=networks, + router_id=router_id), + ('device_id', 'tenant_id') ) def test_get_instance_id_router_id_no_match(self): @@ -149,10 +152,11 @@ class TestMetadataProxyHandler(base.BaseTestCase): [{'network_id': 'net1'}, {'network_id': 'net2'}], [] ] - - self.assertIsNone( - self._get_instance_id_helper(headers, ports, networks=networks, - router_id=router_id), + self.assertEqual( + self._get_instance_and_tenant_id_helper(headers, ports, + networks=networks, + router_id=router_id), + (None, None) ) def test_get_instance_id_network_id(self): @@ -162,12 +166,14 @@ class TestMetadataProxyHandler(base.BaseTestCase): } ports = [ - [{'device_id': 'device_id'}] + [{'device_id': 'device_id', + 'tenant_id': 'tenant_id'}] ] self.assertEqual( - self._get_instance_id_helper(headers, ports, networks=['the_id']), - 'device_id' + self._get_instance_and_tenant_id_helper(headers, ports, + networks=['the_id']), + ('device_id', 'tenant_id') ) def test_get_instance_id_network_id_no_match(self): @@ -178,8 +184,10 @@ class TestMetadataProxyHandler(base.BaseTestCase): ports = [[]] - self.assertIsNone( - self._get_instance_id_helper(headers, ports, networks=['the_id']) + self.assertEqual( + self._get_instance_and_tenant_id_helper(headers, ports, + networks=['the_id']), + (None, None) ) def _proxy_request_test_helper(self, response_code=200, method='GET'): @@ -194,7 +202,8 @@ class TestMetadataProxyHandler(base.BaseTestCase): with mock.patch('httplib2.Http') as mock_http: mock_http.return_value.request.return_value = (resp, 'content') - retval = self.handler._proxy_request('the_id', req) + retval = self.handler._proxy_request('the_id', 'tenant_id', + req) mock_http.assert_has_calls([ mock.call().request( 'http://9.9.9.9:8775/the_path', @@ -202,7 +211,8 @@ class TestMetadataProxyHandler(base.BaseTestCase): headers={ 'X-Forwarded-For': '8.8.8.8', 'X-Instance-ID-Signature': 'signed', - 'X-Instance-ID': 'the_id' + 'X-Instance-ID': 'the_id', + 'X-Tenant-ID': 'tenant_id' }, body=body )]