feat: Use cryptography based JWT parser for increased speed #854

jrconlin · 2017-03-28T22:37:32Z

Switches to using python cryptography library instead of jose/jwt
(which relied on python ecdsa library). Also discovered lots of fun
discrepencies between how ecdsa and libssl sign things.

closes #785

codecov-io · 2017-03-28T22:48:24Z

Codecov Report

Merging #854 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #854   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          48     49    +1     
  Lines        9529   9537    +8     
=====================================
+ Hits         9529   9537    +8

Impacted Files	Coverage Δ
autopush/tests/test_endpoint.py	`100% <100%> (ø)`	⬆️
autopush/jwt.py	`100% <100%> (ø)`
autopush/tests/test_web_validation.py	`100% <100%> (ø)`	⬆️
autopush/web/webpush.py	`100% <100%> (ø)`	⬆️
autopush/utils.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76f772e...fe9b776. Read the comment docs.

bbangert

I don't really understand the crypto enough to comment on that, added some Python style comments.

bbangert · 2017-03-29T18:34:39Z

autopush/jwt.py

+from pyasn1.codec.der.encoder import encode
+
+
+"""Why hand roll?


nit: Module docs go above the imports

bbangert · 2017-03-29T18:35:19Z

autopush/jwt.py

+"""
+
+
+def repad(string):


Wasn't there another padding altering function somewhere?

It's in utils, which includes jwt.py, leading to a dependency loop.

Work around that by moving the function here and changing that file to import it?

bbangert · 2017-03-29T18:35:50Z

autopush/jwt.py

+    return string
+
+
+class VerifyJWT:


Should inherit from (object) still (in Py3 that's implicit).

bbangert · 2017-03-29T18:36:06Z

autopush/jwt.py

+
+class VerifyJWT:
+
+    def __init__(self):  # pragma: no cover


Why is an empty init needed?

bbangert · 2017-03-29T18:36:51Z

autopush/jwt.py

+    def __init__(self):  # pragma: no cover
+        pass
+
+    @classmethod


Why not @staticmethod? Since these don't seem to use cls.

bbangert · 2017-03-29T18:37:10Z

autopush/jwt.py

+        new_sig = base64.urlsafe_b64encode(encoded)
+        return "{}.{}".format(payload, new_sig)
+
+    @classmethod


staticmethod?

alex

👋 new mozillian here, also one of the pyca/cryptography devs. @bbangert asked me to take a look.

alex · 2017-03-29T18:33:57Z

autopush/jwt.py

+        ).setComponents(
+            univ.Integer(int(binascii.hexlify(sig[:32]), 16)),
+            univ.Integer(int(binascii.hexlify(sig[32:]), 16)),
+        )


cryptography has a function for taking r and s and generating the DER encoded body: cryptography.hazmat.primitives.asymmetric.utils.encode_dss_signature (path from memory, hopefully correct!)

(In addition to being less code for you, it uses a different python asn.1 lib which is faster, so that'll be helpful if perf is a concern)

absolutely! Thanks!

alex · 2017-03-29T18:35:59Z

autopush/jwt.py

+        # comparing content values. If the signatures start failing for
+        # some unknown reason in the future, decode the signature and
+        # make sure it matches how we're reconstructing it.
+        verifier = pkey.verifier(


Recent cryptography's have a helper pkey.verify() method, which will save you this comment and a few lines of code :-) https://cryptography.io/en/latest/hazmat/primitives/asymmetric/ec/#cryptography.hazmat.primitives.asymmetric.ec.EllipticCurvePublicKey.verify

alex · 2017-03-29T18:36:55Z

autopush/jwt.py

+        # It will be captured externally.
+        verifier.verify()
+        return json.loads(
+            base64.urlsafe_b64decode(repad(sig_material.split('.')[1])))


You can probably factor out the base64.urlsafe_b64decode(repad(signature.encode())) above into a message var, to safe teh re-decoding.

alex · 2017-03-29T18:40:39Z

autopush/jwt.py

+        )
+        encoded = encode(ds)
+        new_sig = base64.urlsafe_b64encode(encoded)
+        return "{}.{}".format(payload, new_sig)


Since the caller immediately calls split() on this, is there a reason not to return a tuple?

This saves you the splitting, and removes the need to to base64-encode (and then decode) the signature.

alex · 2017-03-29T20:53:20Z

Usage of the cryptography API here LGTM. I don't know anything else about the autopush codebase to say further :-)

bbangert · 2017-03-29T21:52:46Z

autopush/jwt.py

@@ -0,0 +1,84 @@
+"""Why hand roll?


This will show up as the module's title in the API docs.... oh, maybe we should add a .rst so that there'll be docs generated for this module?

bbangert · 2017-03-29T21:53:45Z

autopush/jwt.py

+
+import base64
+import binascii
+import json


nit: line-break between module imports from std lib to 3rd party.

pjenvey

w/ JOSEError gone you can also move the jose and ecdsa dep from requirements to test-requirements

also there's now at least a couple related bogus mocks in the tests: test_post_bad_jwt in test_endpoint, test_invalid_encryption_jwt in test_web_validation

pjenvey · 2017-03-29T20:35:59Z

autopush/jwt.py

+    return string
+
+
+class VerifyJWT:


I assume you're keeping this as a class to flesh out more later (like the map of algs to signing algorithms, similar to jose's classes)? otherwise these are already nice and isolated and could live as module level functions

Partly. Mostly I wanted to keep the basic concept the same so I didn't have to go do a massive code rewrite.

pjenvey · 2017-03-29T21:19:02Z

autopush/jwt.py

+            ec.ECDSA(hashes.SHA256()))
+        verifier.update(sig_material.encode())
+        # This will raise an InvalidSignature exception if failure.
+        # It will be captured externally.


we're not catching it yet --

I think we can overhaul the JOSEError and AssertionError except clause in webpush now for InvalidSignature (IIRC ecdsa had all those asserts we had to catch?)

This func can definitely trigger ValueErrors (from_encoded_point, maybe the rsplits?) which is ok I guess, we're already catching them in that clause.

jose attempts to catch some ValueErrors and convert them to JOSEErrors -- but maybe it wasn't perfect at it because we still had to catch them.

Looking at jose.jws._load, it's also careful about TypeErrors/binascii.Errors, might we trigger any of those too?

pjenvey · 2017-03-29T21:31:32Z

autopush/jwt.py

+            univ.Integer(int(binascii.hexlify(sig[:32]), 16)),
+            univ.Integer(int(binascii.hexlify(sig[32:]), 16)),
+        )
+        encoded = encode(ds)


Could this ever raise a PyAsn1Error? (OUTDATED)

pjenvey · 2017-03-29T21:42:06Z

autopush/jwt.py

+        # some unknown reason in the future, decode the signature and
+        # make sure it matches how we're reconstructing it.
+        verifier = pkey.verifier(
+            base64.urlsafe_b64decode(repad(signature.encode())),


do you really want encode() w/ default encoding here and line 97? Should explicitly specify ascii then (OUDATED)

pjenvey

The test_endpoint test_post_bad_jwt still bogusly monkeypatches jose.jws.verify

pjenvey · 2017-03-30T17:15:20Z

autopush/web/webpush.py

@@ -321,7 +321,7 @@ def validate_auth(self, d):

        try:
            jwt = extract_jwt(token, public_key)
-        except (AssertionError, ValueError, JOSEError):
+        except (ValueError, InvalidSignature, Exception):


Exception here definitely not needed

pjenvey · 2017-03-30T17:26:14Z

autopush/jwt.py

+        return payload, encoded
+
+    @staticmethod
+    def decode(token, key=None, *args, **kwargs):


key should be required and don't need *args/**kwargs

yeah, I had those in because I was trying to make the function a drop in replacement and was being paranoid.

pjenvey · 2017-03-30T17:35:18Z

autopush/jwt.py

+            return json.loads(
+                base64.urlsafe_b64decode(
+                    repad(sig_material.split('.')[1]).encode('utf8')))
+        except Exception as ex:


It'd be nice to restrict this somewhat but it's a damn chore tracking all the potential exceptions. Maybe we should log totally unexpected ones to sentry?

I'll list the ones I know about, do with it what you want =]

splits: ValueError
base64: (TypeError, binascii.Error)
encode_dss_signature (underneath is pyasn stuff): not sure put probably that PyAsn1Error or whatever it was
verify: InvalidSignature
json.loads: ValueError

encode_dss_signature shouldn't ever throw

(if it is, let us know, we need to either fix a bug or update teh docs: https://cryptography.io/en/latest/hazmat/primitives/asymmetric/utils/#cryptography.hazmat.primitives.asymmetric.utils.encode_dss_signature)

@alex, encode_dss_signature shouldn't throw, but hexlify absolutely will. Granted, it's caught by the parent function, but it's also still in the critical path.

@pjenvey I like your idea about having a end Exception that reports to sentry. That will let us spot errors, but still have the code behave properly.

jrconlin · 2017-03-30T17:58:51Z

Crap, missed pulling that test. Good catch, thanks!

pjenvey · 2017-03-30T18:25:49Z

autopush/jwt.py

+                InvalidSignature):
+            raise InvalidSignature()
+        except Exception as ex:  # pragma: no cover
+            Logger().error("Unexpected error processing JWT: ", repr(ex))


you want failure("Unexpected error processing JWT"), that'll grab exc_info and send to sentry

pjenvey · 2017-03-30T18:26:28Z

autopush/jwt.py

+                base64.urlsafe_b64decode(
+                    repad(sig_material.split('.')[1]).encode('utf8')))
+        except (ValueError, TypeError, binascii.Error, PyAsn1Error,
+                InvalidSignature):


nitpick don't need InvalidSignature here

Ah, probably don't need PyAsn1Error either according to alex

pjenvey · 2017-03-30T21:26:32Z

One more related thing (I notice as I'm looking at sentry):

https://sentry.prod.mozaws.net/operations/autopush-prod/issues/383686/

let's start catching TypeError/binascii.Error in extract_jwt's base64 call

pjenvey · 2017-03-30T21:35:58Z

autopush/jwt.py

+        :return tuple containing the signature material and signature
+
+        """
+        payload, asig = auth.rsplit(".", 1)


Another thing: I think we can just auth.encode('utf-8') here to begin with: then the other 2 later encode calls aren't needed. jose.jws _load does similar. (then you can even utilize our utils.base64_urldecode)

yes on calling .encode('utf8') no on using utils.base64_urldecode. the latter would set up a circular dependency.

Switches to using python cryptography library instead of jose/jwt (which relied on python ecdsa library). Also discovered lots of fun discrepencies between how ecdsa and libssl sign things. closes #785

jrconlin requested review from pjenvey and bbangert March 28, 2017 22:37

jrconlin added the in progress label Mar 28, 2017

jrconlin force-pushed the feat/785 branch from 333df85 to 1277af2 Compare March 28, 2017 23:03

bbangert reviewed Mar 29, 2017

View reviewed changes

alex reviewed Mar 29, 2017

View reviewed changes

jrconlin force-pushed the feat/785 branch 2 times, most recently from 2b9da7b to b9340d1 Compare March 29, 2017 20:34

jrconlin force-pushed the feat/785 branch 2 times, most recently from 3da65a4 to 033e4e8 Compare March 29, 2017 21:47

bbangert reviewed Mar 29, 2017

View reviewed changes

pjenvey suggested changes Mar 29, 2017

View reviewed changes

jrconlin force-pushed the feat/785 branch 4 times, most recently from 72f8966 to 3e0771b Compare March 29, 2017 23:31

pjenvey suggested changes Mar 30, 2017

View reviewed changes

jrconlin force-pushed the feat/785 branch 2 times, most recently from 9729164 to 5f4b3a9 Compare March 30, 2017 18:20

pjenvey reviewed Mar 30, 2017

View reviewed changes

jrconlin force-pushed the feat/785 branch from 5f4b3a9 to 9b2fa0e Compare March 30, 2017 20:48

pjenvey approved these changes Mar 30, 2017

View reviewed changes

pjenvey reviewed Mar 30, 2017

View reviewed changes

bbangert approved these changes Apr 4, 2017

View reviewed changes

feat: Use cryptography based JWT parser for increased speed

fe9b776

Switches to using python cryptography library instead of jose/jwt (which relied on python ecdsa library). Also discovered lots of fun discrepencies between how ecdsa and libssl sign things. closes #785

jrconlin force-pushed the feat/785 branch from 9b2fa0e to fe9b776 Compare April 4, 2017 20:06

bbangert merged commit a03910d into master Apr 4, 2017

bbangert deleted the feat/785 branch April 4, 2017 20:32

bbangert removed the in progress label Apr 4, 2017

pjenvey mentioned this pull request Jun 20, 2017

Track down memory change issue in autoendpoint in 1.28.0 #917

Closed

		from pyasn1.codec.der.encoder import encode


		"""Why hand roll?

feat: Use cryptography based JWT parser for increased speed #854

feat: Use cryptography based JWT parser for increased speed #854

Conversation

jrconlin commented Mar 28, 2017

codecov-io commented Mar 28, 2017 • edited Loading

Codecov Report

bbangert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex commented Mar 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjenvey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjenvey Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

pjenvey Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

pjenvey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrconlin commented Mar 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjenvey commented Mar 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Mar 28, 2017 •

edited

Loading

pjenvey Mar 29, 2017 •

edited

Loading

pjenvey Mar 29, 2017 •

edited

Loading