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

@@ -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

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)
 

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/<service>/keys/<kid>', 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:

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')
+    # Retrieve all the keys.
-    expired_key = model.service_keys.get_service_key(kid='kid6')
+    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)
+    self.assertTrue(len(visible_jwks) > 0)
-    expired_key_found = False
+    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)
+      self.assertIn(jwk, visible_jwks)
-
+      self.assertNotIn(jwk, invisible_jwks)
-      if expired_key.jwk == jwk:
-        expired_key_found = True
-
-    self.assertTrue(expired_key_found)
 
 
   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',