Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the ECDSA signature serialization format #158

Merged
merged 4 commits into from
May 19, 2015
Merged

Fix the ECDSA signature serialization format #158

merged 4 commits into from
May 19, 2015

Conversation

esneider
Copy link
Contributor

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
Copy link

Coverage Status

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

@mark-adams
Copy link
Contributor

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
Copy link
Contributor

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

@mark-adams mark-adams added the bug label May 12, 2015
@esneider
Copy link
Contributor Author

Sure! I'll work on it tomorrow.

@mark-adams
Copy link
Contributor

Thanks!! 👍

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

2 similar comments
@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@esneider
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor

Agreed. Added #159 to remove Python 3.2 support.

@jpadilla
Copy link
Owner

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

@mark-adams
Copy link
Contributor

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
Copy link
Owner

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

@esneider
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants