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

Back from the dead: PyCrypto and ECDSA #103

Merged
merged 5 commits into from
Mar 17, 2015

Conversation

mark-adams
Copy link
Contributor

This PR takes care of a chunk of the work towards resolving #99. Specifically, it accomplishes two things:

  • Support for PyCrypto-based RSA and ecdsa-based EC signatures is restored in the jwt.contrib.algorithms module
  • Each of the preferred jwt.algorithms as well as the jwt.contrib.algorithms now have static class variables for SHA256, SHA384, and SHA512 to make our default registration code a little simpler (it no longer has to know the specifics about the hash objects). This also could eventually allow us to write some shared test code between the preferred and jwt.contrib.algorithms algorithms.

This PR would allow a consumer to call register_algorithm and pass in the RSAAlgorithm or the ECAlgorithm from jwt.contrib.algorithms if they would like to use the legacy libraries. I think we could make our default algorithms code much more complex by trying to autodetect all three libraries (cryptography, pycrypto, and ecdsa) but I think it is much simpler for us to only detect for cryptography and simply make the other algorithms available for those on AppEngine or other environments where they need specific support.

- Algorithms now have SHA256, SHA384, and SHA512 static properties that
  refer to the callable that instantiates their hash class
- All algorithms now expect a class (callable) as their hash_alg now.
  This behavior was inconsistent before.
@coveralls
Copy link

Coverage Status

Coverage decreased (-14.33%) to 85.67% when pulling 507d2c9 on mark-adams:contrib-algorithms into 2d0e827 on jpadilla:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.19%) to 98.81% when pulling 3519665 on mark-adams:contrib-algorithms into 2d0e827 on jpadilla:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 975fa0f on mark-adams:contrib-algorithms into 2d0e827 on jpadilla:master.

@jpadilla
Copy link
Owner

@mark-adams dude this is pure gold right here!

@@ -86,6 +86,8 @@ class HMACAlgorithm(Algorithm):
def __init__(self, hash_alg):
self.hash_alg = hash_alg

SHA256, SHA384, SHA512 = hashlib.sha256, hashlib.sha384, hashlib.sha512
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd prefer to change the style of these everywhere to separate lines and before the __init__ method. I think it's easier to parse.

class HMACAlgorithm(Algorithm):
    """
    Performs signing and verification operations using HMAC
    and the specified hash function.
    """
    SHA256 = hashlib.sha256
    SHA384 = hashlib.sha384
    SHA512 = hashlib.sha512

    def __init__(self, hash_alg):
        self.hash_alg = hash_alg

@jpadilla jpadilla modified the milestone: v1.0.0 Mar 17, 2015
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 7c4e96b on mark-adams:contrib-algorithms into 2d0e827 on jpadilla:master.

@mark-adams
Copy link
Contributor Author

@jpadilla Thanks!! I think you're right about the hash function variables. I debated which way to lay them out for a while and was torn between my way and the way you suggested. You're probably right, it is more readable your way. I went ahead and made the change.

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

Successfully merging this pull request may close these issues.

3 participants