converging on proper rotation

data/database.py

@@ -872,6 +872,7 @@ class TorrentInfo(BaseModel):
 
 class ServiceKeyApprovalType(Enum):
   SUPERUSER = 'Super User API'
+  KEY_ROTATION = 'Key Rotation'
 
 _ServiceKeyApproverProxy = Proxy()
 class ServiceKeyApproval(BaseModel):

data/model/service_keys.py

@@ -6,26 +6,27 @@ from data.model import ServiceKeyDoesNotExist, ServiceKeyAlreadyApproved, db_tra
 from data.model.notification import create_notification
 
 
+# TODO ACTION_LOGS for keys
 UNAPPROVED_TTL = timedelta(seconds=app.config['UNAPPROVED_SERVICE_KEY_TTL_SEC'])
 
 
+def _expired_keys_clause(service):
+  return ((ServiceKey.service == service) &
+          (ServiceKey.expiration_date <= datetime.utcnow()))
+
+
+def _stale_unapproved_keys_clause(service):
+  return ((ServiceKey.service >> service) &
+          (ServiceKey.approval >> None) &
+          (ServiceKey.created_date + UNAPPROVED_TTL) >= datetime.utcnow())
+
+
 def _gc_expired(service):
-  expired_keys = ((ServiceKey.service == service) &
+  ServiceKey.delete().where(_expired_keys_clause(service) |
-                  (ServiceKey.expiration_date <= datetime.utcnow()))
+                            _stale_unapproved_keys_clause(service)).execute()
-
-  stale_unapproved_keys = ((ServiceKey.service >> service) &
-                           (ServiceKey.approval >> None) &
-                           (ServiceKey.created_date + UNAPPROVED_TTL) >= datetime.utcnow())
-
-  ServiceKey.delete().where(expired_keys |  stale_unapproved_keys).execute()
-
-
-# TODO ACTION_LOGS for keys
 
 
 def create_service_key(name, kid, service, jwk, metadata, expiration_date):
-  _gc_expired(service)
-
   sk = ServiceKey.create(name=name, kid=kid, service=service, jwk=jwk, metadata=metadata,
                          expiration_date=expiration_date)
 
@@ -43,10 +44,24 @@ def create_service_key(name, kid, service, jwk, metadata, expiration_date):
       'expiration_date': expiration_date,
     })
 
-
-def update_service_key(name, kid, metadata, expiration_date):
   _gc_expired(service)
 
+
+def replace_service_key(kid, jwk, metadata, expiration_date):
+  try:
+    with db_transaction():
+      key = db_for_update(ServiceKey.select().where(ServiceKey.kid == kid)).get()
+      metadata = key.metadata.update(metadata)
+      ServiceKey.create(name=key.name, kid=kid, service=key.service, jwk=jwk,
+                        metadata=metadata, expiration_date=expiration_date, approval=key.approval)
+      key.delete_instance()
+  except ServiceKey.DoesNotExist:
+    raise ServiceKeyDoesNotExist
+
+  _gc_expired(key.service)
+
+
+def update_service_key(name, kid, metadata, expiration_date):
   try:
     with db_transaction():
       key = db_for_update(ServiceKey.select().where(ServiceKey.kid == kid)).get()
@@ -57,37 +72,18 @@ def update_service_key(name, kid, metadata, expiration_date):
   except ServiceKey.DoesNotExist:
     raise ServiceKeyDoesNotExist
 
-
+  _gc_expired(key.service)
-def get_service_keys(approved_only, kid=None, service=None):
-  _gc_expired(service)
-
-  query = ServiceKey.select()
-
-  if approved_only:
-    query = query.where(~(ServiceKey.approval >> None))
-
-  if service is not None:
-    query = query.where(ServiceKey.service == service)
-
-  if kid is not None:
-    query.where(ServiceKey.kid == kid)
-
-  return query
-
-
-def get_service_key(kid):
-  return get_service_keys(False, kid=kid).get()
 
 
 def delete_service_key(service, kid):
-  _gc_expired(service)
-
   try:
     ServiceKey.delete().where(ServiceKey.service == service,
                               ServiceKey.kid == kid).execute()
   except ServiceKey.DoesNotExist:
     raise ServiceKeyDoesNotExist()
 
+  _gc_expired(service)
+
 
 def approve_service_key(kid, approver, approval_type):
   try:
@@ -101,3 +97,32 @@ def approve_service_key(kid, approver, approval_type):
       key.save()
   except ServiceKey.DoesNotExist:
     raise ServiceKeyDoesNotExist
+
+
+def _get_service_keys_query(kid=None, service=None, approved_only=False):
+  query = ServiceKey.select()
+
+  if approved_only:
+    query = query.where(~(ServiceKey.approval >> None))
+
+  if service is not None:
+    query = query.where(ServiceKey.service == service)
+    query = query.where(~(_expired_keys_clause(service)) &
+                        ~(_stale_unapproved_keys_clause(service)))
+
+  if kid is not None:
+    query.where(ServiceKey.kid == kid)
+
+  return query
+
+
+def get_keys():
+  return list(_get_service_keys_query())
+
+
+def get_service_keys(service):
+  return list(_get_service_keys_query(service=service, approved_only=True))
+
+
+def get_service_key(kid):
+  return _get_service_keys_query(kid=kid).get()

endpoints/api/superuser.py

@@ -514,7 +514,7 @@ class SuperUserServiceKeyManagement(ApiResource):
   @require_scope(scopes.SUPERUSER)
   def get(self):
     if SuperUserPermission().can():
-      return jsonify(list(model.service_keys.get_service_keys(False)))
+      return jsonify(model.service_keys.get_keys())
     abort(403)
 
   @verify_not_prod
@@ -538,7 +538,7 @@ class SuperUserServiceKeyManagement(ApiResource):
       metadata.update({
         'created_by': 'Quay SuperUser Panel',
         'creator': user.username,
-        'superuser ip': request.remote_addr,
+        'ip': request.remote_addr,
       })
 
       private_key = RSA.generate(2048)
@@ -551,7 +551,8 @@ class SuperUserServiceKeyManagement(ApiResource):
                                             metadata, expiration_date)
       model.service_keys.approve_service_key(kid, user, ServiceKeyApprovalType.SUPERUSER)
 
-      return jsonify({'private_key': private_key.exportKey('PEM')})
+      return jsonify({'public_key': private_key.publickey().exportKey('PEM'),
+                      'private_key': private_key.exportKey('PEM')})
 
     abort(403)
 
@@ -559,7 +560,7 @@ class SuperUserServiceKeyManagement(ApiResource):
 @resource('/v1/superuser/keys/<kid>')
 @path_param('kid', 'The unique identifier for a service key')
 @show_if(features.SUPER_USERS)
-class SuperUserServiceKeyManagement(ApiResource):
+class SuperUserServiceKeyUpdater(ApiResource):
   """ Resource for managing service keys. """
   schemas = {
     'PutServiceKey': {

endpoints/key_server.py

@@ -1,3 +1,5 @@
+import json
+
 from datetime import datetime
 
 import jwt
@@ -46,20 +48,16 @@ def _validate_jwt(encoded_jwt, jwk, service):
     abort(400)
 
 
-def _signer_jwk(encoded_jwt, jwk, kid):
+def _signer_kid(encoded_jwt):
   decoded_jwt = jwt.decode(encoded_jwt, verify=False)
+  return decoded_jwt.get('signer_kid', None)
 
-  signer_kid = decoded_jwt.get('signer_kid', '')
-  # If we already have our own JWK and it's the signer, short-circuit.
-  if (signer_kid == kid or signer_kid == '') and jwk is not None:
-    return jwk
-  else:
-    try:
-      service_key = data.model.service_keys.get_service_key(signer_kid)
-    except data.model.ServiceKeyDoesNotExist:
-      abort(404)
 
-    return service_key.jwk
+def _signer_key(signer_kid):
+  try:
+    return data.model.service_keys.get_service_key(signer_kid)
+  except data.model.ServiceKeyDoesNotExist:
+    abort(403)
 
 
 @key_server.route('/services/<service>/keys', methods=['GET'])
@@ -88,25 +86,31 @@ def put_service_keys(service, kid):
   except ValueError:
     abort(400)
 
-  _validate_jwk(jwk, kid)
-
   encoded_jwt = request.headers.get(JWT_HEADER_NAME, None)
   if not encoded_jwt:
     abort(400)
 
-  signer_jwk = _signer_jwk(encoded_jwt, jwk, kid)
+  _validate_jwk(jwk, kid)
-  _validate_jwt(encoded_jwt, signer_jwk, service)
+
+
+  signer_kid = _signer_kid(encoded_jwt)
+  signer_key = _signer_key(signer_kid)
+
+  if signer_key.service != service:
+    abort(403)
+
+  _validate_jwt(encoded_jwt, signer_key.jwk, service)
 
   metadata = {
     'ip': request.remote_addr,
-    'signer_jwk': signer_jwk,
+    'created_by': 'Key Rotation',
+    'rotated_by': json.dumps(signer_key),
   }
 
-
   try:
-    data.model.service_keys.update_service_key('', kid, metadata, expiration_date)
+    data.model.service_keys.replace_service_key(kid, jwk, metadata, expiration_date)
   except data.model.ServiceKeyDoesNotExist:
-    data.model.service_keys.create_service_key('', kid, service, jwk, metadata, expiration_date)
+    abort(404)
 
 
 @key_server.route('/services/<service>/keys/<kid>', methods=['DELETE'])
@@ -115,12 +119,15 @@ def delete_service_key(service, kid):
   if not encoded_jwt:
     abort(400)
 
-  signer_jwk = _signer_jwk(encoded_jwt, None, kid)
+  signer_kid = _signer_kid(encoded_jwt)
-  _validate_jwt(encoded_jwt, signer_jwk, service)
+  signer_key = _signer_key(signer_kid)
 
-  try:
+  if (kid == signer_kid) or (signer_key.approval is not None):
-    data.model.service_keys.delete_service_key(service, kid)
+    _validate_jwt(encoded_jwt, signer_key.jwk, service)
-  except data.model.ServiceKeyDoesNotExist:
+    try:
-    abort(404)
+      data.model.service_keys.delete_service_key(service, kid)
+    except data.model.ServiceKeyDoesNotExist:
+      abort(404)
+    return make_response('', 200)
 
-  return make_response('', 200)
+  abort(403)