From 179775b839eaeaa0e3f5f5eabbf669d15139f52e Mon Sep 17 00:00:00 2001 From: Matthieu Huin Date: Thu, 14 May 2020 17:39:03 +0200 Subject: [PATCH] REST API: improve tenant scoping of autohold, authorizations Improve whitelabeling support of the REST API. * autohold-info now checks that the tenant in the REST query matches the tenant of the autohold request looked up by id. Return a 404 Not Found if the autohold request's tenant doesn't match the REST query's tenant. * autohold-delete: DELETE /api/tenant/{tenant}/autohold/{id} returns a 403 Forbidden if the autohold id does not match the tenant * authorizations: /api/user/authorizations is deprecated in favor of the scoped endpoint: GET /api/tenant/{tenant}/authorizations which returns a list of authorized tenants scoped to {tenant}, ie either [tenant,] or [] depending on the user's authorizations. Change-Id: Ibbe5e07a886d54ecd641bb64f02e28dbf8025659 --- .../scoping_REST_API-866574c4d73c577a.yaml | 11 ++ tests/unit/test_web.py | 101 +++++++++++++++--- zuul/web/__init__.py | 79 +++++++++++--- 3 files changed, 164 insertions(+), 27 deletions(-) create mode 100644 releasenotes/notes/scoping_REST_API-866574c4d73c577a.yaml diff --git a/releasenotes/notes/scoping_REST_API-866574c4d73c577a.yaml b/releasenotes/notes/scoping_REST_API-866574c4d73c577a.yaml new file mode 100644 index 0000000000..f2f3ca5959 --- /dev/null +++ b/releasenotes/notes/scoping_REST_API-866574c4d73c577a.yaml @@ -0,0 +1,11 @@ +--- +features: + - | + REST API: authorizations: add a tenant-scoped endpoint at + '/api/tenant/{tenant}/authorizations'. Calling the endpoint will return + a list of admin tenants limited to the scoped tenant, if the user has admin + privileges on it. +deprecations: + - | + REST API: authorizations: the /api/user/authorizations endpoint is deprecated + in favor of the tenant-scoped endpoint. It will be removed next release. diff --git a/tests/unit/test_web.py b/tests/unit/test_web.py index 64830e801b..b6817a84f3 100644 --- a/tests/unit/test_web.py +++ b/tests/unit/test_web.py @@ -830,6 +830,10 @@ class TestWeb(BaseTestWeb): self.assertEqual("reason text", request['reason']) self.assertEqual([], request['nodes']) + # Scope the request to tenant-two, not found + resp = self.get_url("api/tenant/tenant-two/autohold/%s" % request_id) + self.assertEqual(404, resp.status_code, resp.text) + def test_autohold_list(self): """test listing autoholds through zuul-web""" client = zuul.rpcclient.RPCClient('127.0.0.1', @@ -1513,14 +1517,7 @@ class TestTenantScopedWebApi(BaseTestWeb): self.assertEqual("some reason", request['reason']) self.assertEqual(1, request['max_count']) - def test_autohold_delete(self): - authz = {'iss': 'zuul_operator', - 'aud': 'zuul.example.com', - 'sub': 'testuser', - 'zuul': { - 'admin': ['tenant-one', ] - }, - 'exp': time.time() + 3600} + def _init_autohold_delete(self, authz): token = jwt.encode(authz, key='NoDanaOnlyZuul', algorithm='HS256').decode('utf-8') @@ -1539,13 +1536,51 @@ class TestTenantScopedWebApi(BaseTestWeb): self.assertNotEqual([], autohold_requests) self.assertEqual(1, len(autohold_requests)) request_id = autohold_requests[0]['id'] + return client, request_id, token + def test_autohold_delete_wrong_tenant(self): + """Make sure authorization rules are applied""" + authz = {'iss': 'zuul_operator', + 'aud': 'zuul.example.com', + 'sub': 'testuser', + 'zuul': { + 'admin': ['tenant-one', ] + }, + 'exp': time.time() + 3600} + client, request_id, _ = self._init_autohold_delete(authz) # now try the autohold-delete API + bad_authz = {'iss': 'zuul_operator', + 'aud': 'zuul.example.com', + 'sub': 'testuser', + 'zuul': { + 'admin': ['tenant-two', ] + }, + 'exp': time.time() + 3600} + bad_token = jwt.encode(bad_authz, key='NoDanaOnlyZuul', + algorithm='HS256').decode('utf-8') + resp = self.delete_url( + "api/tenant/tenant-one/autohold/%s" % request_id, + headers={'Authorization': 'Bearer %s' % bad_token}) + # Throw a "Forbidden" error, because user is authenticated but not + # authorized for tenant-one + self.assertEqual(403, resp.status_code, resp.text) + # clean up + r = client.autohold_delete(request_id) + self.assertTrue(r) + + def test_autohold_delete(self): + authz = {'iss': 'zuul_operator', + 'aud': 'zuul.example.com', + 'sub': 'testuser', + 'zuul': { + 'admin': ['tenant-one', ] + }, + 'exp': time.time() + 3600} + client, request_id, token = self._init_autohold_delete(authz) resp = self.delete_url( "api/tenant/tenant-one/autohold/%s" % request_id, headers={'Authorization': 'Bearer %s' % token}) self.assertEqual(204, resp.status_code, resp.text) - # autohold-list should be empty now resp = self.get_url( "api/tenant/tenant-one/autohold") @@ -1701,12 +1736,16 @@ class TestTenantScopedWebApi(BaseTestWeb): {'action': 'autohold_by_request_id', 'path': 'api/tenant/my-tenant/autohold/123', 'allowed_methods': ['GET', 'DELETE', ]}, - {'action': 'authorizations', - 'path': 'api/user/authorizations', - 'allowed_methods': ['GET', ]}, {'action': 'dequeue', 'path': 'api/tenant/my-tenant/project/my-project/enqueue', 'allowed_methods': ['POST', ]}, + {'action': 'authorizations', + 'path': 'api/tenant/my-tenant/authorizations', + 'allowed_methods': ['GET', ]}, + # TODO (mhu) deprecated, remove in next version + {'action': 'authorizations', + 'path': 'api/user/authorizations', + 'allowed_methods': ['GET', ]}, ] for endpoint in endpoints: preflight = self.options_url( @@ -1876,9 +1915,13 @@ class TestTenantScopedWebApiWithAuthRules(BaseTestWeb): 'exp': time.time() + 3600} token = jwt.encode(authz, key='NoDanaOnlyZuul', algorithm='HS256').decode('utf-8') + # TODO(mhu) deprecated, remove after next release req = self.get_url('/api/user/authorizations', headers={'Authorization': 'Bearer %s' % token}) self.assertEqual(401, req.status_code, req.text) + req = self.get_url('/api/tenant/tenant-one/authorizations', + headers={'Authorization': 'Bearer %s' % token}) + self.assertEqual(401, req.status_code, req.text) def test_user_actions(self): """Test that users get the right 'zuul.actions' trees""" @@ -1914,6 +1957,7 @@ class TestTenantScopedWebApiWithAuthRules(BaseTestWeb): authz['exp'] = time.time() + 3600 token = jwt.encode(authz, key='NoDanaOnlyZuul', algorithm='HS256').decode('utf-8') + # TODO(mhu) deprecated, remove after next release req = self.get_url('/api/user/authorizations', headers={'Authorization': 'Bearer %s' % token}) self.assertEqual(200, req.status_code, req.text) @@ -1926,8 +1970,41 @@ class TestTenantScopedWebApiWithAuthRules(BaseTestWeb): data['zuul']['admin'], "%s got %s" % (authz['sub'], data)) + req = self.get_url('/api/tenant/tenant-one/authorizations', + headers={'Authorization': 'Bearer %s' % token}) + self.assertEqual(200, req.status_code, req.text) + data = req.json() + self.assertTrue('zuul' in data, + "%s got %s" % (authz['sub'], data)) + self.assertTrue('admin' in data['zuul'], + "%s got %s" % (authz['sub'], data)) + self.assertEqual('tenant-one' in test_user['zuul.admin'], + data['zuul']['admin'], + "%s got %s" % (authz['sub'], data)) + self.assertEqual(['tenant-one', ], + data['zuul']['scope'], + "%s got %s" % (authz['sub'], data)) + + req = self.get_url('/api/tenant/tenant-two/authorizations', + headers={'Authorization': 'Bearer %s' % token}) + self.assertEqual(200, req.status_code, req.text) + data = req.json() + self.assertTrue('zuul' in data, + "%s got %s" % (authz['sub'], data)) + self.assertTrue('admin' in data['zuul'], + "%s got %s" % (authz['sub'], data)) + self.assertEqual('tenant-two' in test_user['zuul.admin'], + data['zuul']['admin'], + "%s got %s" % (authz['sub'], data)) + self.assertEqual(['tenant-two', ], + data['zuul']['scope'], + "%s got %s" % (authz['sub'], data)) + def test_authorizations_no_header(self): """Test that missing Authorization header results in HTTP 401""" + req = self.get_url('/api/tenant/tenant-one/authorizations') + self.assertEqual(401, req.status_code, req.text) + # TODO(mhu) deprecated, remove after next release req = self.get_url('/api/user/authorizations') self.assertEqual(401, req.status_code, req.text) diff --git a/zuul/web/__init__.py b/zuul/web/__init__.py index efa2166971..fe3e7dc551 100755 --- a/zuul/web/__init__.py +++ b/zuul/web/__init__.py @@ -503,13 +503,13 @@ class ZuulWebAPI(object): @cherrypy.tools.handle_options(allowed_methods=['GET', 'DELETE', ]) def autohold_by_request_id(self, tenant, request_id): if cherrypy.request.method == 'GET': - return self._autohold_info(request_id) + return self._autohold_info(tenant, request_id) elif cherrypy.request.method == 'DELETE': - return self._autohold_delete(request_id) + return self._autohold_delete(tenant, request_id) else: raise cherrypy.HTTPError(405) - def _autohold_info(self, request_id): + def _autohold_info(self, tenant, request_id): job = self.rpc.submitJob('zuul:autohold_info', {'request_id': request_id}) if job.failure: @@ -518,7 +518,11 @@ class ZuulWebAPI(object): request = json.loads(job.data[0]) if not request: raise cherrypy.HTTPError( - 404, 'Hold request %s does not exist.' % request_id) + 404, 'Hold request %s not found.' % request_id) + if tenant != request['tenant']: + # return 404 rather than 403 to avoid leaking tenant info + raise cherrypy.HTTPError( + 404, 'Hold request %s not found.' % request_id) resp = cherrypy.response resp.headers['Access-Control-Allow-Origin'] = '*' return { @@ -535,10 +539,9 @@ class ZuulWebAPI(object): 'nodes': request['nodes'] } - def _autohold_delete(self, request_id): + def _autohold_delete(self, tenant, request_id): # We need tenant info from the request for authz - request = self._autohold_info(request_id) - + request = self._autohold_info(tenant, request_id) basic_error = self._basic_auth_header_check() if basic_error is not None: return basic_error @@ -589,12 +592,16 @@ class ZuulWebAPI(object): 'buildsets': '/api/tenant/{tenant}/buildsets', 'buildset': '/api/tenant/{tenant}/buildset/{uuid}', 'config_errors': '/api/tenant/{tenant}/config-errors', + # TODO(mhu) remove after next release 'authorizations': '/api/user/authorizations', + 'tenant_authorizations': ('/api/tenant/{tenant}' + '/authorizations'), 'autohold': '/api/tenant/{tenant}/project/{project:.*}/autohold', 'autohold_list': '/api/tenant/{tenant}/autohold', - 'autohold_by_request_id': '/api/tenant/{tenant}/' - 'autohold/{request_id}', - 'autohold_delete': '/api/tenant/{tenant}/autohold/{request_id}', + 'autohold_by_request_id': ('/api/tenant/{tenant}' + '/autohold/{request_id}'), + 'autohold_delete': ('/api/tenant/{tenant}' + '/autohold/{request_id}'), 'enqueue': '/api/tenant/{tenant}/project/{project:.*}/enqueue', 'dequeue': '/api/tenant/{tenant}/project/{project:.*}/dequeue', 'promote': '/api/tenant/{tenant}/promote', @@ -638,11 +645,11 @@ class ZuulWebAPI(object): raise cherrypy.HTTPError(403) return user_authz - # TODO good candidate for caching + # TODO(mhu) deprecated, remove next version @cherrypy.expose @cherrypy.tools.json_out(content_type='application/json; charset=utf-8') @cherrypy.tools.handle_options(allowed_methods=['GET', ]) - def user_authorizations(self): + def authorizations(self): basic_error = self._basic_auth_header_check() if basic_error is not None: return basic_error @@ -650,12 +657,50 @@ class ZuulWebAPI(object): claims, token_error = self._auth_token_check() if token_error is not None: return token_error + try: + admin_tenants = self._authorizations() + except exceptions.AuthTokenException as e: + for header, contents in e.getAdditionalHeaders().items(): + cherrypy.response.headers[header] = contents + cherrypy.response.status = e.HTTPError + return {'description': e.error_description, + 'error': e.error, + 'realm': e.realm} + return {'zuul': {'admin': admin_tenants}, } + + @cherrypy.expose + @cherrypy.tools.json_out(content_type='application/json; charset=utf-8') + @cherrypy.tools.handle_options(allowed_methods=['GET', ]) + def tenant_authorizations(self, tenant): + basic_error = self._basic_auth_header_check() + if basic_error is not None: + return basic_error + # AuthN/AuthZ + claims, token_error = self._auth_token_check() + if token_error is not None: + return token_error + try: + admin_tenants = self._authorizations() + except exceptions.AuthTokenException as e: + for header, contents in e.getAdditionalHeaders().items(): + cherrypy.response.headers[header] = contents + cherrypy.response.status = e.HTTPError + return {'description': e.error_description, + 'error': e.error, + 'realm': e.realm} + return {'zuul': {'admin': tenant in admin_tenants, + 'scope': [tenant, ]}, } + + # TODO good candidate for caching + def _authorizations(self): + rawToken = cherrypy.request.headers['Authorization'][len('Bearer '):] + claims = self.zuulweb.authenticators.authenticate(rawToken) if 'zuul' in claims and 'admin' in claims.get('zuul', {}): return {'zuul': {'admin': claims['zuul']['admin']}, } job = self.rpc.submitJob('zuul:get_admin_tenants', {'claims': claims}) admin_tenants = json.loads(job.data[0]) - return {'zuul': {'admin': admin_tenants}, } + return admin_tenants @cherrypy.expose @cherrypy.tools.json_out(content_type='application/json; charset=utf-8') @@ -1222,7 +1267,10 @@ class ZuulWeb(object): # route order is important, put project actions before the more # generic tenant/{tenant}/project/{project} route route_map.connect('api', '/api/user/authorizations', - controller=api, action='user_authorizations') + controller=api, action='authorizations') + route_map.connect('api', '/api/tenant/{tenant}/authorizations', + controller=api, + action='tenant_authorizations') route_map.connect( 'api', '/api/tenant/{tenant}/project/{project:.*}/autohold', @@ -1240,7 +1288,8 @@ class ZuulWeb(object): '/api/tenant/{tenant}/promote', controller=api, action='promote') route_map.connect('api', '/api/tenant/{tenant}/autohold/{request_id}', - controller=api, action='autohold_by_request_id') + controller=api, + action='autohold_by_request_id') route_map.connect('api', '/api/tenant/{tenant}/autohold', controller=api, action='autohold_list') route_map.connect('api', '/api/tenant/{tenant}/projects',