Closed calderonth closed 4 years ago
I've amended the code with a dedicated ECVerifier as it's a much more clean way of doing it so RSA/EC are not mingled and it simplifies the if/else spaghetti code too.
Fantastic stuff! Lemme give this another gander tomorrow and should be good to go!
Your work is greatly appreciated!
Thanks, no worries, good thing you didn't merge immediately there was one last regression with RS512 signature (for which there's no test so I missed it ^_^).
Ooo nice catch, mind adding a test?
Done.
Amazing work! Thanks a bunch for your contribution. From my end, these tests are the basis of my regression checking so I think I'm good to go. Anything else you can think of or shall I go ahead and merge?
Cheers, it's good on my end too :-)
Give me a couple days of a buffer and I'll get a release out on LuaRocks.
@cdbattags any ETA on getting a release cut to luarocks? Would love to see this be able to validate the es256 keys from Pomerium :-)
Hello,
This PR should fix ES256 which was not behaving like the standard expects, so ES256 signature were not compatible with other implementation, this is due to the signature format being different from plain ECDSA signature (ASN.1 encoded) vs JWS standard expecting a raw concatenated signature. So that should close #24 (even if secp384 isn't done, it'd be trivial to add it later).
I had to add more import to the OpenSSL cdef in order to convert between signature formats and in since there's only 1 verifier (RSAVerifier) I had to add a ECDSA specific method there to do the conversion. I would be better to have a dedicated ECVerifier but that's more work and break the API, hence why I put it there for now.
I've added more tests and verified this works with Python JWT and jwt.io so interoperability should be better. I've done my best to declare the GC on the new C code but it's worthwhile maybe trying to see if I haven't missed anything/generated memory leak.