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

Implement cryptography as one of the backends #49

Merged
merged 12 commits into from
May 3, 2017
Merged

Conversation

zejn
Copy link
Collaborator

@zejn zejn commented Apr 14, 2017

This pull request implements (optional) cryptography backend for both EC and RSA.

In order to be able to do that I had to refactor jose.jwk to make part of the jwk module importable by the backends. The utility functions base64_to_long and int_arr_to_long were moved to jose.utils and class Key was moved to jose.backends.base.

In order to be able to support conversion and reuse between key implementations, each asymmetric key implementation (EC and RSA) now has two new methods. Method to_pem returns PEM encoded key data of six.binary_type. Method public_key returns public key from a private key or self when the instance represents public key.

Existing ECDSA signing implementation from ecdsa python library was moved into jose.backends.ecdsa_backend and existing PyCrypto was moved into jose.backends.pycrypto_backend.

Default ECKey and RSAKey are chosen at import time in jose/backends/__init__.py, but a better explicit selection might be useful. RSAKey defaults to pycrypto and fallbacks to cryptography backend and EC defaults to cryptography and fallbacks to ecdsa backend.

I've tested on Python 2.7 and Python 3.5.

Gasper Zejn added 4 commits April 13, 2017 16:25
…nds. Use pycrypto backend by default for RSA. Add to_pem and public_key to EC and RSA keys in order to abstract away backend key implementations and be able to convert between keys. Move base64_to_long and int_arr_to_long to jose.utils to be importable by backends.
…turns six.binary_type - str on PY2, bytes on PY3.
… of deducing it from desired algorithm. This matches behavior of ecdsa backend, which allows signing different digests with same key (provided they are shorter than the key).
@mpdavis
Copy link
Owner

mpdavis commented Apr 14, 2017

First of all, thank you for putting this together. At first glance this looks very good.

I don't have time to dive into this tonight, but there are a couple of issues that look like they will be easy to fix.

  1. The pypy tests are broken on Travis due to cryptography not supporting pypy < 2.6. It looks like we can force a newer version of pypy. Upgrade PyPy to 2.6 travis-ci/travis-ci#4756

  2. If I understand jose/backends/__init__.py correctly, this would require that PyCrypto or cryptography is installed, otherwise it will result in an ImportError. If a user only needs to use HMAC signing (or EC using the ecdsa library). We should handle instantiation without either library installed and throw an error at runtime if they try to sign/verify a token with a method that isn't supported.

Those are the only issues that came up from a quick pass through. Thanks again for putting this together.

@zejn
Copy link
Collaborator Author

zejn commented Apr 14, 2017

Very good observations, will try to fix. Thanks.

@codecov-io
Copy link

codecov-io commented Apr 14, 2017

Codecov Report

Merging #49 into master will increase coverage by 0.06%.
The diff coverage is 95.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #49      +/-   ##
==========================================
+ Coverage   95.57%   95.64%   +0.06%     
==========================================
  Files           7       12       +5     
  Lines         542      734     +192     
==========================================
+ Hits          518      702     +184     
- Misses         24       32       +8
Impacted Files Coverage Δ
jose/utils.py 97.43% <100%> (+1.13%) ⬆️
jose/backends/cryptography_backend.py 100% <100%> (ø)
jose/backends/__init__.py 50% <50%> (ø)
jose/jwk.py 94.93% <71.42%> (-1.48%) ⬇️
jose/backends/base.py 81.81% <81.81%> (ø)
jose/backends/pycrypto_backend.py 96.42% <96.42%> (ø)
jose/backends/ecdsa_backend.py 96.61% <96.61%> (ø)
... and 2 more

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 1a23c39...7d83918. Read the comment docs.

Gasper Zejn added 4 commits April 14, 2017 12:59
… which uses an incorrect marker to encode a RSA public key into PEM while using OID.
…rrently expected to be (for compatibility), but pass if it fails. Maybe user just needs one of them, which works, or HMACKey. Function jwk.get_key will still try to import RSAKey or ECKey if needed, and throw an ImportError if unsuccessful.
@zejn
Copy link
Collaborator Author

zejn commented Apr 15, 2017

The RSAKey and ECKey in jose.jwk module are now imported from jose.backends and if import fails, nothing happens. This ensures the library isn't unusable if user only needs one of the implementations, which is available.

The Travis configuration file was updated to use tox-travis and now runs tests on newer version of PyPy and also CPython 3.5 and 3.6.

@mpdavis
Copy link
Owner

mpdavis commented May 3, 2017

@zejn This looks great. My apologies for taking so long to take a look.

@mpdavis mpdavis merged commit 069e684 into mpdavis:master May 3, 2017
@mpdavis
Copy link
Owner

mpdavis commented Sep 1, 2017

Released in 1.4.0

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

Successfully merging this pull request may close these issues.

3 participants