bug: decode and process crypto-key header correctly

The Crypto-Key header has a different format than what was first
understood. A valid Crypto-Key header may look like `keyid="foo";
key1="data",key2="data"`. This patch specialized Crypto-Key parsing
to a class.

Closes #410

autopush/crypto_key.py

@@ -0,0 +1,81 @@
+"""Crypto-Key header parser and manager"""
+
+class CryptoKeyException(Exception):
+    """Invalid CryptoKey"""
+
+
+class CryptoKey(object):
+    """Parse the Crypto-Key header per
+    http://tools.ietf.org/html/draft-ietf-httpbis-encryption-encoding-00#section-4
+
+    """
+    _values_ = []
+
+    def __init__(self, header):
+        """Parse the Crypto-Key header
+
+        :param header: Header content string.
+
+        """
+        chunks = header.split(",")
+        if self._values_:
+            self._values_ = []
+        for chunk in chunks:
+            bits = chunk.split(";")
+            hash = {}
+            for bit in bits:
+                try:
+                    (key, value) = bit.split("=", 1)
+                except ValueError:
+                    raise CryptoKeyException("Invalid Crypto Key value")
+                hash[key.strip()] = value.strip(' "')
+            self._values_.append(hash)
+
+    def get_keyid(self, keyid):
+        """Return the Crypto-Key hash referred to by a given keyid.
+
+        For example, for a CryptoKey specified as:
+        Crypto-Key: keyid="apple";foo="fruit",keyid="gorp";bar="snake"
+
+        get_keyid("apple") would return a hash of
+        {"keyid": "apple", "foo":"fruit"}
+
+        :param keyid: The keyid to reference
+        :returns: hash of the matching key data or None
+
+        """
+        for val in self._values_:
+            if keyid == val.get('keyid'):
+                return val
+        return None
+
+    def get_label(self, label):
+        """Return the Crypto-Key value referred to by a given label.
+
+        For example, for a CryptoKey specified as:
+        Crypto-Key: keyid="apple";foo="fruit",keyid="gorp";bar="snake"
+
+        get_label("foo")
+        would return a value of "apple"
+
+        **NOTE** This presumes that "label" is unique. Otherwise it will
+        only return the FIRST instance of "label". Use get_keyid() if
+        you know of multiple sections containing similar labels.
+        :param label: The label to reference.
+        :returns: Value associated with the key data or None
+
+        """
+        for val in self._values_:
+            if label in val:
+                return val.get(label)
+        return None
+
+    def to_string(self):
+        """Return a reformulated Crypto-Key header string"""
+        chunks = []
+        for val in self._values_:
+            bits = []
+            for key in val:
+                bits.append("{}=\"{}\"".format(key, val[key]))
+            chunks.append(';'.join(bits))
+        return ','.join(chunks)

autopush/endpoint.py

@@ -51,7 +51,6 @@
     generate_hash,
     validate_uaid,
     ErrorLogger,
-    parse_header,
     extract_jwt,
 )
 
@@ -311,7 +310,7 @@ def _invalid_auth(self, fail):
         log.msg("Invalid bearer token: " + message, **self._client_info)
         raise VapidAuthException("Invalid bearer token: " + message)
 
-    def _process_auth(self, result, keys):
+    def _process_auth(self, result):
         """Process the optional VAPID auth token.
 
         VAPID requires two headers to be present;
@@ -324,22 +323,15 @@ def _process_auth(self, result, keys):
         # No auth present, so it's not a VAPID call.
         if not authorization:
             return result
-        if not keys:
-            raise VapidAuthException("Missing Crypto-Key")
-        if not isinstance(keys, dict):
-            raise VapidAuthException("Invalid Crypto-Key")
-        crypto_key = keys.get('p256ecdsa')
-        if not crypto_key:
-            raise VapidAuthException("Invalid bearer token: "
-                                     "improperly specified crypto-key")
+        public_key = result.get("public_key")
         try:
             (auth_type, token) = authorization.split(' ', 1)
         except ValueError:
             raise VapidAuthException("Invalid Authorization header")
         # if it's a bearer token containing what may be a JWT
         if auth_type.lower() == AUTH_SCHEME and '.' in token:
-            d = deferToThread(extract_jwt, token, crypto_key)
-            d.addCallback(self._store_auth, crypto_key, token, result)
+            d = deferToThread(extract_jwt, token, public_key)
+            d.addCallback(self._store_auth, public_key, token, result)
             d.addErrback(self._invalid_auth)
             return d
         # otherwise, it's not, so ignore the VAPID data.
@@ -412,20 +404,13 @@ def put(self, api_ver="v0", token=None):
         """
         api_ver = api_ver or "v0"
         self.start_time = time.time()
-        public_key = None
-        keys = {}
-        crypto_key = self.request.headers.get('crypto-key')
-        if crypto_key:
-            header_info = parse_header(crypto_key)
-            keys = header_info[-1]
-            if isinstance(keys, dict):
-                public_key = keys.get('p256ecdsa')
+        crypto_key_header = self.request.headers.get('crypto-key')
 
         d = deferToThread(self.ap_settings.parse_endpoint,
                           token,
                           api_ver,
-                          public_key)
-        d.addCallback(self._process_auth, keys)
+                          crypto_key_header)
+        d.addCallback(self._process_auth)
         d.addCallback(self._token_valid)
         d.addErrback(self._auth_err)
         d.addErrback(self._token_err)
@@ -437,10 +422,8 @@ def put(self, api_ver="v0", token=None):
     #############################################################
     def _token_valid(self, result):
         """Called after the token is decrypted successfully"""
-        if len(result) != 2:
-            raise InvalidTokenException("Wrong subscription token components")
-
-        self.uaid, self.chid = result
+        self.uaid = result.get("uaid")
+        self.chid = result.get("chid")
         d = deferToThread(self.ap_settings.router.get_uaid, self.uaid)
         d.addCallback(self._uaid_lookup_results)
         d.addErrback(self._uaid_not_found_err)
@@ -476,12 +459,6 @@ def _uaid_lookup_results(self, result):
                 log.msg("Client error", status_code=400, errno=101,
                         **self._client_info)
                 return self._write_response(400, 101)
-            if ("encryption-key" in self.request.headers and
-                    "crypto-key" in self.request.headers):
-                log.msg("Client error", status_code=400, errno=110,
-                        **self._client_info)
-                return self._write_response(
-                    400, 110, message="Invalid crypto headers")
             self._client_info["message_size"] = len(data) if data else 0
         else:
             # GCM/APNS pull data out of body

autopush/settings.py

@@ -39,6 +39,7 @@
 )
 from autopush.utils import canonical_url, resolve_ip
 from autopush.senderids import SENDERID_EXPRY, DEFAULT_BUCKET
+from autopush.crypto_key import (CryptoKey, CryptoKeyException)
 
 
 VALID_V0_TOKEN = re.compile(r'[0-9A-Za-z-]{32,36}:[0-9A-Za-z-]{32,36}')
@@ -280,26 +281,34 @@ def make_endpoint(self, uaid, chid, key=None):
 
         return root + 'v2/' + self.fernet.encrypt(base + sha256(key).digest())
 
-    def parse_endpoint(self, token, version="v0", public_key=None):
+    def parse_endpoint(self, token, version="v0", ckey_header=None):
         """Parse an endpoint into component elements of UAID, CHID and optional
         key hash if v2
 
         :param token: The obscured subscription data.
         :param version: This is the API version of the token.
-        :param public_key: the public key (from Encryption-Key: p256ecdsa=)
+        :param ckey_header: the Crypto-Key header bearing the public key
+        (from Crypto-Key: p256ecdsa=)
 
         :raises ValueError: In the case of a malformed endpoint.
 
-        :returns: a tuple containing the (UAID, CHID)
+        :returns: a dict containing (uaid=UAID, chid=CHID, public_key=KEY)
 
         """
 
         token = self.fernet.decrypt(token.encode('utf8'))
+        public_key = None
+        if ckey_header:
+            try:
+                public_key = CryptoKey(ckey_header).get_label('p256ecdsa')
+            except CryptoKeyException:
+                raise InvalidTokenException("Invalid key data")
 
         if version == 'v0':
             if not VALID_V0_TOKEN.match(token):
                 raise InvalidTokenException("Corrupted push token")
-            return tuple(token.split(':'))
+            items = token.split(':')
+            return dict(uaid=items[0], chid=items[1], public_key=public_key)
         if version == 'v1' and len(token) != 32:
             raise InvalidTokenException("Corrupted push token")
         if version == 'v2':
@@ -310,4 +319,6 @@ def parse_endpoint(self, token, version="v0", public_key=None):
             if not constant_time.bytes_eq(sha256(public_key).digest(),
                                           token[32:]):
                 raise InvalidTokenException("Key mismatch")
-        return (token[:16].encode('hex'), token[16:32].encode('hex'))
+        return dict(uaid=token[:16].encode('hex'),
+                    chid=token[16:32].encode('hex'),
+                    public_key=public_key)

autopush/tests/test_cryptokey.py

@@ -0,0 +1,42 @@
+import unittest
+
+from nose.tools import (eq_, ok_)
+
+from autopush.crypto_key import CryptoKey
+
+
+class CryptoKeyTestCase(unittest.TestCase):
+
+    valid_key = (
+        'keyid="p256dh";dh="BDw9T0eImd4ax818VcYqDK_DOhcuDswKero'
+        'YyNkdhYmygoLSDlSiWpuoWYUSSFxi25cyyNTR5k9Ny93DzZc0UI4",'
+        'p256ecdsa="BF92zdI_AKcH5Q31_Rr-04bPqOHU_Qg6lAawHbvfQrY'
+        'xV_vIsAsHSyaiuyfofvxT8ZVIXccykd4V2Z7iJVfreT8"')
+
+    def test_parse(self):
+        ckey = CryptoKey(self.valid_key)
+        eq_(ckey.get_keyid("p256dh"),
+            {"keyid": "p256dh",
+             "dh": "BDw9T0eImd4ax818VcYqDK_DOhcuDswKero"
+                   "YyNkdhYmygoLSDlSiWpuoWYUSSFxi25cyyNTR5k9Ny93DzZc0UI4"})
+        eq_(ckey.get_label("p256ecdsa"),
+            "BF92zdI_AKcH5Q31_Rr-04bPqOHU_Qg6lAawHbvfQrY"
+            "xV_vIsAsHSyaiuyfofvxT8ZVIXccykd4V2Z7iJVfreT8")
+        ok_(ckey.get_keyid("missing") is None)
+        ok_(ckey.get_label("missing") is None)
+
+    def test_parse_lenient(self):
+        ckey = CryptoKey(self.valid_key.replace('"', ''))
+        str = ckey.to_string()
+        ckey2 = CryptoKey(str)
+        ok_(ckey.get_keyid("p256dh"), ckey2.get_keyid("p256dh"))
+        ok_(ckey.get_label("p256ecdsa") is not None)
+        ok_(ckey.get_label("p256ecdsa"), ckey2.get_label("p256ecdsa"))
+
+    def test_string(self):
+        ckey = CryptoKey(self.valid_key)
+        str = ckey.to_string()
+        ckey2 = CryptoKey(str)
+        ok_(ckey.get_keyid("p256dh"), ckey2.get_keyid("p256dh"))
+        ok_(ckey.get_label("p256ecdsa") is not None)
+        ok_(ckey.get_label("p256ecdsa"), ckey2.get_label("p256ecdsa"))