Fix the ECDSA signature serialization format

[esneider]

According to the JSON Web Algorithms Draft, section 3.4 the ECDSA signature should be serialized by concatenating the R and S values.

The code was using ecdsa.util.sigencode_der, that encodes the signature using the DER serialization format, instead of using ecdsa.util.sigencode_string, that does exactly what the draft asks.

[coveralls]

Coverage remained the same at 100.0% when pulling b8771db on esneider:master into 6c9cada on jpadilla:master.

[mark-adams]

Thanks for submitting!

Hmm... so I did some surface level checking, and if what you say is true (I haven't done a deep dive yet), we may have a similar issue with the cryptography based primary implementation as well. (The py_ecdsa module that you updated isn't recommended and is just included for legacy purposes).

I ran the following and the signature checked out:

# Load our test keys
ec_key = open('tests/keys/testkey_ec','r').read()
ec_pub = open('tests/keys/testkey_ec.pub', 'r').read()

# Import the primary algorithm and the legacy algorithm
from jwt.algorithms import ECAlgorithm
from jwt.contrib.algorithms.py_ecdsa import ECAlgorithm as LegacyAlgorithm

# Sign a message with the legacy algorithm
legacy = LegacyAlgorithm(LegacyAlgorithm.SHA256)
legacy_key = legacy.prepare_key(ec_key)
msg = b'hello world!'
sig = legacy.sign(msg, legacy_key)

# Verify it with the primary algorithm
ec = ECAlgorithm(ECAlgorithm.SHA256)
pub_key = ec.prepare_key(ec_pub)

>> ec.verify(msg, pub_key, sig)
True

Basically, I created a digital signature with the legacy ECDSA and verified it with the cryptography-backed ECDSA, and it passed. That leads me to believe that either nothing is wrong with the current implementation or our cryptography-based implementation is also wrong. I'll have to look into this a bit more to be certain.

[mark-adams]

As you said, the JWA spec says:

1. ...
2. Turn R and S into octet sequences in big endian order, with each array being be 32 octets long. The octet sequence representations MUST NOT be shortened to omit any leading zero octets contained in the values.
3. Concatenate the two octet sequences in the order R and then S.

Upon further checking, it looks like cryptography outputs its signatures as follows: "The signature is a bytes object, whose contents is DER encoded as described in RFC 6979. This can be decoded using decode_rfc6979_signature()." reference

It looks like we'll need to change our primary algorithm in algorithms.py to use cryptography.hazmat.primivies.asmmetric.utils.decode_rfc6979_signature() as well to properly implement ECDSA.

Can you add that to this PR as well so we'll have both algorithms fixed?

cc: @jpadilla

[esneider]

Sure! I'll work on it tomorrow.

[mark-adams]

Thanks!! 👍

[coveralls]

Coverage decreased (-0.99%) to 99.01% when pulling 01b7fb0 on esneider:master into 6c9cada on jpadilla:master.

[coveralls]

Coverage decreased (-0.59%) to 99.41% when pulling 5283187 on esneider:master into 6c9cada on jpadilla:master.

[coveralls]

Coverage decreased (-0.59%) to 99.41% when pulling 5283187 on esneider:master into 6c9cada on jpadilla:master.

[coveralls]

Coverage decreased (-0.59%) to 99.41% when pulling 5283187 on esneider:master into 6c9cada on jpadilla:master.

[coveralls]

Coverage decreased (-0.4%) to 99.6% when pulling 3c479e2 on esneider:master into 6c9cada on jpadilla:master.

[coveralls]

Coverage decreased (-21.34%) to 78.66% when pulling 3c479e2 on esneider:master into 6c9cada on jpadilla:master.

[esneider]

I don't know why the test for python 3.2 isn't passing. Maybe it's because pyjwt is using the just-released cryptography v0.9, that dropped support for python 3.2.

[jpadilla]

From their changelog:

Removed support for Python 3.2. This version of Python is rarely used and caused support headaches. Users affected by this should upgrade to 3.3+.

Deprecated support for Python 2.6. At the time there is no time table for actually dropping support, however we strongly encourage all users to upgrade their Python, as Python 2.6 no longer receives support from the Python core team.

I'm fine doing exactly that as well. @mark-adams thoughts?

[mark-adams]

Agreed. Added #159 to remove Python 3.2 support.

[jpadilla]

@esneider just merged #159 try rebasing from master and see if tests pass.

[mark-adams]

Thanks @esneider for the great fix! I merged in the changes from master that remove Python 3.2 support and also added some good, known test vectors for ECDSA and it looks like your branch passed both your tests and the new test vectors so I went ahead and merged it in.

Thanks so much for contributing! 👍

[jpadilla]

I'd consider we break a new release for this soon.

[esneider]

Thanks to you guys for the awesome work you're doing with pyjwt!

Btw, I too think it might be a good idea to bump the version number, since this will break any preexisting ECDSA signature serialization.