BLOCKED: Switch to pyelliptic for faster EC2 handling #39
Conversation
jrconlin
force-pushed
the
feat/ec-clib
branch
5 times, most recently
from
7d16605 to
56eeb49
Compare
|
I'm not quite sure why the 2.6 test is failing. It may be that the call out to OpenSSL is causing 'key' to be deallocated. It's clearly being defined. I know that 2.6 has reached EOL, so not sure how critical that test is. |
jrconlin
changed the title
|
Sigh, there's a bug in the signature handler. Will update with fix. |
jrconlin
force-pushed
the
feat/ec-clib
branch
from
4c6273e to
17987dd
Compare
|
So, turns out that the backing library (pyelliptic) automatically runs whatever you pass it through SHA256. Obviously, that's bad. I've submitted a PR for that yann2192/pyelliptic#53. That PR blocks this one. |
jrconlin
changed the title
jrconlin
force-pushed
the
feat/ec-clib
branch
from
9adb761 to
8e7f35f
Compare
This patch is dependent on two other library patches landing. * mpdavis/python-jose#39 * yann2192/pyelliptic#53 Temporarily pointing to personal forks with patches applied. closes #753
This patch is dependent on two other library patches landing. * mpdavis/python-jose#39 * yann2192/pyelliptic#53 Temporarily pointing to personal forks with patches applied. closes #753
This patch is dependent on two other library patches landing. * mpdavis/python-jose#39 * yann2192/pyelliptic#53 Temporarily pointing to personal forks with patches applied. closes #753
| asn_set = decoder.decode(decoded)[0] | ||
| except: | ||
| raise JWKError("Invalid EC Key") | ||
| pri_key = self.bitstring_to_str(asn_set[1]) |
There was a problem hiding this comment.
This is only applicable for the first case; in the univ.Sequence case you end up having to shuffle this around.
I'd recommend initializing this to None, too, and moving this set into the first case block.
| if pcurve: | ||
| curve = pcurve | ||
| # Shuffle the key to the right return. | ||
| pub_key = pri_key |
There was a problem hiding this comment.
On the face of it, this is super alarming: setting the public value to the private value. Per the earlier comment, don't shuffle these. Basically, don't set pri_key to something that isn't a private key in the first place. If we aren't sure yet, let it remain asn_set[1]
| pub_key = pri_key | ||
| pri_key = None | ||
| if not curve: | ||
| raise JWKError("Invalid EC curve type key specified.") |
There was a problem hiding this comment.
Nit: It's not really that it was specified wrong, it's that the supplied data isn't one of our known formats.
| @@ -161,7 +159,8 @@ def _sign_header_and_claims(encoded_header, encoded_claims, algorithm, key_data) | |||
| signing_input = b'.'.join([encoded_header, encoded_claims]) | |||
| try: | |||
| key = jwk.construct(key_data, algorithm) | |||
| signature = key.sign(signing_input) | |||
| dat = signing_input.decode('utf8').encode('utf8') | |||
There was a problem hiding this comment.
Nit: This could use a comment explaining why it's being round-tripped through the decoder/encoder
| def repad(self, st): | ||
| """Add base64 padding back to the end of a stripped character | ||
| sequence | ||
|
|
| raise JWKError("Invalid EC curve type key specified.") | ||
| return curve, pri_key, pub_key | ||
|
|
||
| def _process_jwk(self, jwk_dict, algorythm="sha256"): |
ecdsa is very flexible as a EC library, but CPU intensive. Switching to a C lib based EC handler should make things faster and more CPU friendly under loads.
|
@jcjones Thanks again for the r?s. Addressed (and did some flake8 fixes, plus a pointer to the newer pyelliptic library patch) |
| # And finally the public key | ||
| pub_key = self.bitstring_to_str(asn_set[3]) | ||
| # A public key starts with a sequence | ||
| if isinstance(asn_set[0], univ.Sequence): |
There was a problem hiding this comment.
an elif instead of if would be a bit clearer here
|
Cryptography will be included as a dependency if I succeed, and already has elliptic curve cryptography based on openssl. I'm not going to work on this, but the point is that if you use cryptography, then this won't be blocked. |
|
So, turns out that OpenSSL (and by default, python cryptography) use a DER formatted signature value, where The fix is "reasonably simple", (check the signature length, and if 64 octets, recompile the signature into a DER and hand that over to cryptography's EC verify. Needless to say, it's going to impact this patch, so I'm going to close this for now until the new patch is ready. |
ecdsa is very flexible as a EC library, but CPU intensive. Switching to
a C lib based EC handler should make things faster and more CPU friendly
under loads.