Extend and improve the importing and exporting of keys.#52
Conversation
As far as I can tell, the curve P-256 in RFC7518 refers to the curve secp256r1 (which is also the curve supported elsewhere in the code), not the curve secp256k1.
…entation. To export a key in JSON, i.e., according to RFC7518, we need to obtain some values from the backend and return those as a dictionary. This commit takes care of that by implementing the as_dict function on all backends.
Up until now, the private components of a JWK dictionary were ignored when constructing the key. This commit changes that by reading the private parameters and passing them to the backend to generate a private key, if those parameters are present.
tests/algorithms/test_EC.py
Outdated
| "d": "AAhRON2r9cqXX1hg-RoI6R1tX5p2rUAYdmpHZoC1XNM56KtscrX6zbKipQrCW9CGZH3T4ubpnoTKLDYJ_fF3_rJt", | ||
| } | ||
|
|
||
| ECDSAECKey(key, ALGORITHMS.ES512) |
There was a problem hiding this comment.
It would be sensible to check if the keys returned are actually public or private in this test.
As suggested by @zejn in pull request mpdavis#52.
Codecov Report
@@ Coverage Diff @@
## master #52 +/- ##
==========================================
- Coverage 95.64% 94.52% -1.12%
==========================================
Files 12 12
Lines 734 840 +106
==========================================
+ Hits 702 794 +92
- Misses 32 46 +14
Continue to review full report at Codecov.
|
|
@TobiasKappe I appreciate the contribution. I need to look over it in more depth, but at first look this appears to be good. I agree that this functionality is beneficial to the library. |
mpdavis
left a comment
mpdavis
left a comment
There was a problem hiding this comment.
Other than some nit-picky comments about splitting up unit tests, this looks great to me.
Again, I appreciate you jumping in.
tests/algorithms/test_EC.py
Outdated
| with pytest.raises(JWKError): | ||
| ECDSAECKey({'kty': 'bla'}, ALGORITHMS.ES256) | ||
|
|
||
| def test_EC_jwk(self): |
There was a problem hiding this comment.
I know that it makes for some duplicate code, but can you split this test into separate tests? I get much more value out of seeing every test that fails in a single test run instead of fixing one portion of a test only to see another portion has failed.
I see this as:
test_ECDSAECKey_public_jwktest_ECDSAECKey_private_jwktest_ECDSAECKey_invalid_jwktest_CryptographyECKey_public_jwktest_CryptographyECKey_private_jwktest_CryptographyECKey_invalid_jwk
tests/algorithms/test_EC.py
Outdated
| # Private parameters should be absent | ||
| assert 'd' not in as_dict | ||
|
|
||
| def test_to_dict(self): |
There was a problem hiding this comment.
Split this into:
test_CryptographyECKey_public_to_dicttest_CryptographyECKey_private_to_dicttest_ ECDSAECKey_public_to_dicttest_ ECDSAECKey_private_to_dict
tests/algorithms/test_RSA.py
Outdated
| @@ -118,12 +120,47 @@ def test_invalid_algorithm(self): | |||
| CryptographyRSAKey({'kty': 'bla'}, ALGORITHMS.RS256) | |||
|
|
|||
| def test_RSA_jwk(self): | |||
tests/algorithms/test_RSA.py
Outdated
| def assert_roundtrip(self, key, impl): | ||
| assert impl(key.to_dict(), ALGORITHMS.RS256).to_dict() == key.to_dict() | ||
|
|
||
| def test_to_dict(self): |
|
Sure, splitting up the tests is a good idea. I'll get on this later today. |
…rors. This is to mirror the behavior of the PyCrypto backend.
Some tests had duplicated code within them to test for backends, and for other tests there were different testing harnesses that tested parts of each backend. This commit unifies the tests to be as backend-agnostic as possible, and execute them for each backend using pytest's parametrization.
|
Any further feedback? |
|
@TobiasKappe No, this looks great! I apologize for the wait, I was skimming this too quickly and reading too much into the "failing" checks. I appreciate the work! |
|
Great, thanks! |
4 participants
I noticed that the current library does not have functionality to export a Key implementation to a JWK as specified in RFC7518. This PR adds a method called
as_dictto every Key implementation, which returns a dictionary version of the JWK, ready to be exported to JSON.I also added functionality to the
_process_jwkfunctions to recognize when a dictionary encodes a private key, and import the private key parameters if possible.