From 28a80ef6a9f41113556d0de0bea226b8ad93b4a4 Mon Sep 17 00:00:00 2001
From: Joseph Schorr <josephschorr@users.noreply.github.com>
Date: Thu, 14 Apr 2016 16:56:15 -0400
Subject: [PATCH] Make sure to verify service names on key creation

---
 data/model/__init__.py     |  4 ++++
 data/model/service_keys.py | 14 +++++++++++++-
 test/test_endpoints.py     |  7 +++++++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/data/model/__init__.py b/data/model/__init__.py
index d8f772f45..b138bcf35 100644
--- a/data/model/__init__.py
+++ b/data/model/__init__.py
@@ -84,6 +84,10 @@ class ServiceKeyAlreadyApproved(DataModelException):
   pass
 
 
+class ServiceNameInvalid(DataModelException):
+  pass
+
+
 class TooManyLoginAttemptsException(Exception):
   def __init__(self, message, retry_after):
     super(TooManyLoginAttemptsException, self).__init__(message)
diff --git a/data/model/service_keys.py b/data/model/service_keys.py
index 8face6aa6..deae1b95c 100644
--- a/data/model/service_keys.py
+++ b/data/model/service_keys.py
@@ -1,3 +1,5 @@
+import re
+
 from calendar import timegm
 from datetime import datetime, timedelta
 from peewee import JOIN_LEFT_OUTER
@@ -6,11 +8,14 @@ from Crypto.PublicKey import RSA
 from jwkest.jwk import RSAKey
 
 from data.database import db_for_update, User, ServiceKey, ServiceKeyApproval
-from data.model import ServiceKeyDoesNotExist, ServiceKeyAlreadyApproved, db_transaction, config
+from data.model import (ServiceKeyDoesNotExist, ServiceKeyAlreadyApproved, ServiceNameInvalid,
+                        db_transaction, config)
 from data.model.notification import create_notification, delete_all_notifications_by_path_prefix
 from util.security.fingerprint import canonical_kid
 
 
+_SERVICE_NAME_REGEX = re.compile(r'^[a-z0-9_]+$')
+
 def _expired_keys_clause(service):
   return ((ServiceKey.service == service) &
           (ServiceKey.expiration_date <= datetime.utcnow()))
@@ -33,6 +38,11 @@ def _gc_expired(service):
                             _stale_unapproved_keys_clause(service)).execute()
 
 
+def _verify_service_name(service_name):
+  if not _SERVICE_NAME_REGEX.match(service_name):
+    raise ServiceNameInvalid
+
+
 def _notify_superusers(key):
   notification_metadata = {
     'name': key.name,
@@ -53,6 +63,8 @@ def _notify_superusers(key):
 
 
 def create_service_key(name, kid, service, jwk, metadata, expiration_date, rotation_duration=None):
+  _verify_service_name(service)
+
   key = ServiceKey.create(name=name, kid=kid, service=service, jwk=jwk, metadata=metadata,
                           expiration_date=expiration_date, rotation_duration=rotation_duration)
 
diff --git a/test/test_endpoints.py b/test/test_endpoints.py
index 615dc5ec7..e1486ce3d 100644
--- a/test/test_endpoints.py
+++ b/test/test_endpoints.py
@@ -233,6 +233,13 @@ class KeyServerTestCase(EndpointTestCase):
     payload = self._get_test_jwt_payload()
     token = jwt.encode(payload, private_key.exportKey('PEM'), 'RS256')
 
+    # Invalid service name should yield a 400.
+    self.putResponse('key_server.put_service_key', service='sample service', kid='kid420',
+                     headers={
+                      'Authorization': 'Bearer %s' % token,
+                      'Content-Type': 'application/json',
+                     }, data=jwk, expected_code=400)
+
     # Publish a new key
     with assert_action_logged('service_key_create'):
       self.putResponse('key_server.put_service_key', service='sample_service', kid='kid420',