Easier extending/replacing of key algorithms - #42
Conversation
Codecov Report@@ Coverage Diff @@## master #42 +/- ##
==========================================
+ Coverage 95.53% 95.57% +0.03%
==========================================
Files 7 7 Lines 538 542 +4 ==========================================
+ Hits 514 518 +4
Misses 24 24
Continue to review full report at Codecov.
|
mpdavis
commented
Jan 11, 2017
I'm curious what the use case for this is? Are you looking to define and use custom algorithms at run time, or is there another reason? |
friedcell
commented
Jan 11, 2017
Correct, using a custom ES256 implementation on a RaspberryPI. Also kind-of resolves #41 as it gets much easier to switch implementations. |
friedcell
commented
Jan 12, 2017
Code to switch an implementation (eg. customjose.py): fromjoseimportjwkclassMyKeyImpl(jwk.Key):
# custom implementation heredefregister():
jwk.ALGORITHMS.register_key("ES256", MyKeyImpl)Where you use jose: fromjoseimportjwtjwk.encode({}, "private key", algorithm="ES256")After: fromcustomjoseimportregisterfromjoseimportjwtregister()
jwk.encode({}, "private key", algorithm="ES256")The runtime switch is dead simple and you don't have to change any code in your application. But the change also means that you can have multiple implementations of the Key classes and use |
friedcell
commented
Jan 17, 2017
@mpdavis any feedback? |
zejn
commented
Jan 24, 2017
Hi, To check the utility of this patch, I wrote a cryptography backend for ES256/ES384/ES512 in a separate repo at zejn@f4a840a. There's still an unsettled way how to handle different backend crypto implementations. Currently used ecdsa is pure python and slow. There's a pull request for pyelliptic at #39, but it doesn't work with other than SHA256 - yann2192/pyelliptic#53 - and pyelliptic also does not support OpenSSL 1.1 - yann2192/pyelliptic#50. There's also an issue about supporting cryptography here - #41. I find this PR working, it touches very limited amount of code. Further rework would be welcome as it would allow to specify which key class to use if one does not want to use globally registered one. |
| KEYS = {} | ||
| def get_key(self, algorithm): | ||
| from jose.jwk import HMACKey, RSAKey, ECKey |
There was a problem hiding this comment.
I'm not a fan of this circular dependency. I would move this key registration/lookup logic to jwk.py.
| self.SUPPORTED.add(algorithm) | ||
| return True | ||
| else: | ||
| return False |
There was a problem hiding this comment.
Attempting to register an invalid key should raise a TypeError
mpdavis
commented
Jan 29, 2017
@friedcell My apologies for taking so long to get back to you on this. I really appreciate the work. I only have a couple of comments on the PR, and I think overall it will be a good addition. |
friedcell
commented
Jan 29, 2017
Updated the code per request |
| KEYS = {} | ||
| def register_key(self, algorithm, key_class): | ||
| from jose.jwk import Key |
There was a problem hiding this comment.
This still has a wonky dependency. Can you move it over as well?
friedcell
commented
Jan 30, 2017
Moved that too |
friedcell
commented
Feb 14, 2017
@mpdavis ping? |
mpdavis
commented
Mar 5, 2017
LGTM |
mpdavis
commented
Sep 1, 2017
Released in 1.4.0 |
blag
commented
Jul 7, 2018
Hey @friedcell, thanks for this PR! It made it really really easy for me to write PR #100. |
Changed some code to make jwk algorithm implementations easily extendable.
If you want to replace a certain key implementation you only do
jwk.ALGORITHMS.register_key("[algorithm name]", [key class])and from that moment on the algorithm will use a different class to do everything.While doing it, made some stuff a bit more pythonic.