From 6e2df3b339b0e8eefe6e0d6378791c197a0bc7cd Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Tue, 3 May 2016 14:01:33 -0400 Subject: [PATCH] Fix key server to not list expired keys Fixes the key server to not list expire keys and by default not return expired or unapproved keys unless explicitly requested. Fixes #1430 --- data/model/service_keys.py | 18 ++++-- endpoints/api/superuser.py | 7 ++- endpoints/key_server.py | 27 +++++---- test/test_endpoints.py | 111 ++++++++++++++++++++++++++++++++----- 4 files changed, 129 insertions(+), 34 deletions(-) diff --git a/data/model/service_keys.py b/data/model/service_keys.py index 382433da3..6a865c959 100644 --- a/data/model/service_keys.py +++ b/data/model/service_keys.py @@ -137,7 +137,7 @@ def delete_service_key(kid): def set_key_expiration(kid, expiration_date): try: - service_key = get_service_key(kid) + service_key = get_service_key(kid, alive_only=False, approved_only=False) except ServiceKey.DoesNotExist: raise ServiceKeyDoesNotExist @@ -163,12 +163,17 @@ def approve_service_key(kid, approver, approval_type, notes=''): return key -def _list_service_keys_query(kid=None, service=None, approved_only=False, approval_type=None): +def _list_service_keys_query(kid=None, service=None, approved_only=True, alive_only=True, + approval_type=None): query = ServiceKey.select().join(ServiceKeyApproval, JOIN_LEFT_OUTER) if approved_only: query = query.where(~(ServiceKey.approval >> None)) + if alive_only: + query = query.where((ServiceKey.expiration_date > datetime.utcnow()) | + (ServiceKey.expiration_date >> None)) + if approval_type is not None: query = query.where(ServiceKeyApproval.approval_type == approval_type) @@ -185,15 +190,16 @@ def _list_service_keys_query(kid=None, service=None, approved_only=False, approv def list_all_keys(): - return list(_list_service_keys_query()) + return list(_list_service_keys_query(approved_only=False, alive_only=False)) def list_service_keys(service): - return list(_list_service_keys_query(service=service, approved_only=True)) + return list(_list_service_keys_query(service=service)) -def get_service_key(kid, service=None): +def get_service_key(kid, service=None, alive_only=True, approved_only=True): try: - return _list_service_keys_query(kid=kid, service=service).get() + return _list_service_keys_query(kid=kid, service=service, approved_only=approved_only, + alive_only=alive_only).get() except ServiceKey.DoesNotExist: raise ServiceKeyDoesNotExist diff --git a/endpoints/api/superuser.py b/endpoints/api/superuser.py index fc7ef8c8e..10bd69f21 100644 --- a/endpoints/api/superuser.py +++ b/endpoints/api/superuser.py @@ -639,7 +639,7 @@ class SuperUserServiceKey(ApiResource): def get(self, kid): if SuperUserPermission().can(): try: - key = model.service_keys.get_service_key(kid) + key = model.service_keys.get_service_key(kid, approved_only=False, alive_only=False) return jsonify(key_view(key)) except model.service_keys.ServiceKeyDoesNotExist: abort(404) @@ -655,7 +655,7 @@ class SuperUserServiceKey(ApiResource): if SuperUserPermission().can(): body = request.get_json() try: - key = model.service_keys.get_service_key(kid) + key = model.service_keys.get_service_key(kid, approved_only=False, alive_only=False) except model.service_keys.ServiceKeyDoesNotExist: abort(404) @@ -690,7 +690,8 @@ class SuperUserServiceKey(ApiResource): model.service_keys.update_service_key(kid, body.get('name'), body.get('metadata')) log_action('service_key_modify', None, key_log_metadata) - return jsonify(key_view(model.service_keys.get_service_key(kid))) + updated_key = model.service_keys.get_service_key(kid, approved_only=False, alive_only=False) + return jsonify(key_view(updated_key)) abort(403) diff --git a/endpoints/key_server.py b/endpoints/key_server.py index 3839ffc5e..37b29e167 100644 --- a/endpoints/key_server.py +++ b/endpoints/key_server.py @@ -60,14 +60,19 @@ def _validate_jwt(encoded_jwt, jwk, service): abort(400) -def _signer_kid(encoded_jwt): +def _signer_kid(encoded_jwt, allow_none=False): headers = get_unverified_header(encoded_jwt) - return headers.get('kid', None) + kid = headers.get('kid', None) + if not kid and not allow_none: + abort(400) + + return kid -def _signer_key(service, signer_kid): +def _lookup_service_key(service, signer_kid, approved_only=True): try: - return data.model.service_keys.get_service_key(signer_kid, service=service) + return data.model.service_keys.get_service_key(signer_kid, service=service, + approved_only=approved_only) except data.model.ServiceKeyDoesNotExist: abort(403) @@ -81,7 +86,7 @@ def list_service_keys(service): @key_server.route('/services//keys/', methods=['GET']) def get_service_key(service, kid): try: - key = data.model.service_keys.get_service_key(kid) + key = data.model.service_keys.get_service_key(kid, alive_only=False, approved_only=False) except data.model.ServiceKeyDoesNotExist: abort(404) @@ -126,8 +131,7 @@ def put_service_key(service, kid): _validate_jwk(jwk) - signer_kid = _signer_kid(encoded_jwt) - + signer_kid = _signer_kid(encoded_jwt, allow_none=True) if kid == signer_kid or signer_kid is None: # The key is self-signed. Create a new instance and await approval. _validate_jwt(encoded_jwt, jwk, service) @@ -147,11 +151,10 @@ def put_service_key(service, kid): log_action('service_key_create', None, metadata=key_log_metadata, ip=request.remote_addr) return make_response('', 202) + # Key is going to be rotated. metadata.update({'created_by': 'Key Rotation'}) - signer_key = _signer_key(service, signer_kid) + signer_key = _lookup_service_key(service, signer_kid) signer_jwk = signer_key.jwk - if signer_key.service != service: - abort(403) _validate_jwt(encoded_jwt, signer_jwk, service) @@ -184,9 +187,9 @@ def delete_service_key(service, kid): encoded_jwt = match.group(1) signer_kid = _signer_kid(encoded_jwt) - signer_key = _signer_key(service, signer_kid) + signer_key = _lookup_service_key(service, signer_kid, approved_only=False) - self_signed = kid == signer_kid or signer_kid == '' + self_signed = kid == signer_kid approved_key_for_service = signer_key.approval is not None if self_signed or approved_key_for_service: diff --git a/test/test_endpoints.py b/test/test_endpoints.py index 45c381587..c740ced94 100644 --- a/test/test_endpoints.py +++ b/test/test_endpoints.py @@ -6,6 +6,7 @@ import unittest from urllib import urlencode from urlparse import urlparse, urlunparse, parse_qs +from datetime import datetime, timedelta import jwt @@ -201,22 +202,27 @@ class KeyServerTestCase(EndpointTestCase): } def test_list_service_keys(self): - unapproved_key = model.service_keys.get_service_key(kid='kid3') - expired_key = model.service_keys.get_service_key(kid='kid6') + # Retrieve all the keys. + all_keys = model.service_keys.list_all_keys() + visible_jwks = [key.jwk for key in model.service_keys.list_service_keys('sample_service')] + + invisible_jwks = [] + for key in all_keys: + is_expired = key.expiration_date and key.expiration_date <= datetime.utcnow() + if key.service != 'sample_service' or key.approval is None or is_expired: + invisible_jwks.append(key.jwk) rv = self.getResponse('key_server.list_service_keys', service='sample_service') jwkset = py_json.loads(rv) # Make sure the hidden keys are not returned and the visible ones are returned. - self.assertTrue(len(jwkset['keys']) > 0) - expired_key_found = False + self.assertTrue(len(visible_jwks) > 0) + self.assertTrue(len(invisible_jwks) > 0) + self.assertEquals(len(visible_jwks), len(jwkset['keys'])) + for jwk in jwkset['keys']: - self.assertNotEquals(jwk, unapproved_key.jwk) - - if expired_key.jwk == jwk: - expired_key_found = True - - self.assertTrue(expired_key_found) + self.assertIn(jwk, visible_jwks) + self.assertNotIn(jwk, invisible_jwks) def test_get_service_key(self): @@ -269,6 +275,17 @@ class KeyServerTestCase(EndpointTestCase): self.getResponse('key_server.get_service_key', service='sample_service', kid='kid420', expected_code=409) + # Attempt to rotate the key. Since not approved, it will fail. + token = jwt.encode(payload, private_key.exportKey('PEM'), 'RS256', headers={'kid': 'kid420'}) + self.putResponse('key_server.put_service_key', service='sample_service', kid='kid6969', + headers={ + 'Authorization': 'Bearer %s' % token, + 'Content-Type': 'application/json', + }, data=jwk, expected_code=403) + + # Approve the key. + model.service_keys.approve_service_key('kid420', 1, ServiceKeyApprovalType.SUPERUSER) + # Rotate that new key with assert_action_logged('service_key_rotate'): token = jwt.encode(payload, private_key.exportKey('PEM'), 'RS256', headers={'kid': 'kid420'}) @@ -289,20 +306,88 @@ class KeyServerTestCase(EndpointTestCase): }, data=jwk, expected_code=403) - def test_delete_service_key(self): + def test_attempt_delete_service_key_with_no_kid_signer(self): + # Generate two keys, approving the first. + private_key, _ = model.service_keys.generate_service_key('sample_service', None, kid='first') + + # Mint a JWT with our test payload but *no kid*. + token = jwt.encode(self._get_test_jwt_payload(), private_key.exportKey('PEM'), 'RS256', + headers={}) + + # Using the credentials of our key, attempt to delete our unapproved key + self.deleteResponse('key_server.delete_service_key', + headers={'Authorization': 'Bearer %s' % token}, + expected_code=400, service='sample_service', kid='first') + + + def test_attempt_delete_service_key_with_expired_key(self): + # Generate two keys, approving the first. + private_key, _ = model.service_keys.generate_service_key('sample_service', None, kid='first') + model.service_keys.approve_service_key('first', 1, ServiceKeyApprovalType.SUPERUSER) + model.service_keys.generate_service_key('sample_service', None, kid='second') + + # Mint a JWT with our test payload + token = jwt.encode(self._get_test_jwt_payload(), private_key.exportKey('PEM'), 'RS256', + headers={'kid': 'first'}) + + # Set the expiration of the first to now - some time. + model.service_keys.set_key_expiration('first', datetime.utcnow() - timedelta(seconds=100)) + + # Using the credentials of our second key, attempt to delete our unapproved key + self.deleteResponse('key_server.delete_service_key', + headers={'Authorization': 'Bearer %s' % token}, + expected_code=403, service='sample_service', kid='second') + + # Set the expiration to the future and delete the key. + model.service_keys.set_key_expiration('first', datetime.utcnow() + timedelta(seconds=100)) + + with assert_action_logged('service_key_delete'): + self.deleteResponse('key_server.delete_service_key', + headers={'Authorization': 'Bearer %s' % token}, + expected_code=204, service='sample_service', kid='second') + + + def test_delete_unapproved_service_key(self): # No Authorization header should yield a 400 self.deleteResponse('key_server.delete_service_key', expected_code=400, service='sample_service', kid='kid1') - # Generate two keys and approve one + # Generate an unapproved key. + private_key, _ = model.service_keys.generate_service_key('sample_service', None, + kid='unapprovedkeyhere') + + # Mint a JWT with our test payload + token = jwt.encode(self._get_test_jwt_payload(), private_key.exportKey('PEM'), 'RS256', + headers={'kid': 'unapprovedkeyhere'}) + + # Delete our unapproved key with itself. + with assert_action_logged('service_key_delete'): + self.deleteResponse('key_server.delete_service_key', + headers={'Authorization': 'Bearer %s' % token}, + expected_code=204, service='sample_service', kid='unapprovedkeyhere') + + + def test_delete_chained_service_key(self): + # No Authorization header should yield a 400 + self.deleteResponse('key_server.delete_service_key', expected_code=400, + service='sample_service', kid='kid1') + + # Generate two keys. private_key, _ = model.service_keys.generate_service_key('sample_service', None, kid='kid123') model.service_keys.generate_service_key('sample_service', None, kid='kid321') - model.service_keys.approve_service_key('kid123', 1, ServiceKeyApprovalType.SUPERUSER) # Mint a JWT with our test payload token = jwt.encode(self._get_test_jwt_payload(), private_key.exportKey('PEM'), 'RS256', headers={'kid': 'kid123'}) + # Using the credentials of our second key, attempt tp delete our unapproved key + self.deleteResponse('key_server.delete_service_key', + headers={'Authorization': 'Bearer %s' % token}, + expected_code=403, service='sample_service', kid='kid321') + + # Approve the second key. + model.service_keys.approve_service_key('kid123', 1, ServiceKeyApprovalType.SUPERUSER) + # Using the credentials of our approved key, delete our unapproved key with assert_action_logged('service_key_delete'): self.deleteResponse('key_server.delete_service_key',