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

Switch to cryptography for RSA and ECDSA algorithms #51

Merged
merged 5 commits into from
Dec 22, 2014

Conversation

mark-adams
Copy link
Contributor

Currently, pyjwt uses PyCrypto (last release 2013-10) and ecdsa in order to support RSA and ECDSA algorithms. Instead of dealing with two dependencies, I propose switching to using the cryptography library for the following reasons:

  • Better performance (RSA performance is 30% faster than in PyCrypto
  • More trusted codebase. The cryptography library uses OpenSSL for most of its cryptographic functions which is trusted, very familiar, and well maintained.
  • Single library for both RSA and ecdsa support
  • cryptography is more actively maintained. (PyCrypto hasn't seen any updates in over a year)

The attached commits swap out PyCrypto and ecdsa for cryptography. There are no public API changes to pyjwt. All existing test cases pass without issue.

@jpadilla
Copy link
Owner

@mark-adams this is huge and an awesome improvement. Also thanks for the speed comparison, I'l definitely include that in the release notes.

Wondering if this should be treated as a new major version, since we are changing the main requirement.

I was wondering if installing cryptography on Heroku worked. It seems that as of a couple of months ago it does. The Python buildpack has a cryptography build step to deal with libffi.

@jpadilla
Copy link
Owner

Also, @progrium since this might possibly be considered a "large" change. What are your thoughts?

@progrium
Copy link
Contributor

If it changes or breaks the existing API or behavior, it's a new major
version. :)

On Sat, Dec 20, 2014 at 6:21 AM, José Padilla notifications@github.com
wrote:

Also, @progrium https://github.com/progrium since this might possibly
be considered a "large" change. What are your thoughts?


Reply to this email directly or view it on GitHub
#51 (comment).

Jeff Lindsay
http://progrium.com

@jpadilla
Copy link
Owner

@progrium API doesn't change at all, just its internal requirement

@progrium
Copy link
Contributor

Right. I think it's a minor version.

On Sat, Dec 20, 2014 at 11:01 AM, José Padilla notifications@github.com
wrote:

@progrium https://github.com/progrium API doesn't change at all, just
its internal requirement


Reply to this email directly or view it on GitHub
#51 (comment).

Jeff Lindsay
http://progrium.com

@mark-adams
Copy link
Contributor Author

So does that mean 0.4 or 0.3.3?

@progrium
Copy link
Contributor

0.4

On Sat, Dec 20, 2014 at 11:19 AM, Mark Adams notifications@github.com
wrote:

So does that mean 0.4 or 0.3.3?


Reply to this email directly or view it on GitHub
#51 (comment).

Jeff Lindsay
http://progrium.com

@mark-adams
Copy link
Contributor Author

Cool. I was just making sure. Since the project hadn't had a 1.0 yet, I wasn't sure. Added the commit incrementing the version.

jpadilla added a commit that referenced this pull request Dec 22, 2014
Switch to cryptography for RSA and ECDSA algorithms
@jpadilla jpadilla merged commit 75bbe71 into jpadilla:master Dec 22, 2014
@mark-adams mark-adams deleted the change-to-cryptography branch December 22, 2014 14:26
@jpadilla
Copy link
Owner

@mark-adams thanks again, just released this as v0.4.0 on PyPI.

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