Remove duplicated key handlers #318
Conversation
|
in 5fd1ad9 I noticed EdDSA is using a mixture of RSA and ECDSA error codes 😦 |
https://github.com/Thalhammer/jwt-cpp/actions/runs/7161143039/job/19496334109?pr=318#step:8:248 ``` [ RUN ] OpenSSLErrorTest.ECDSACertificate /home/runner/work/jwt-cpp/jwt-cpp/tests/OpenSSLErrorTest.cpp:487: Failure Expected equality of these values: ec Which is: ecdsa_error:19 e.expected_ec Which is: rsa_error:12 ```
prince-chrismc
changed the title
| {&fail_BIO_new, 2, jwt::error::ecdsa_error::create_mem_bio_failed}, | ||
| {&fail_PEM_read_bio_X509, 1, jwt::error::ecdsa_error::cert_load_failed}, | ||
| {&fail_X509_get_pubkey, 1, jwt::error::ecdsa_error::get_key_failed}, | ||
| {&fail_PEM_write_bio_PUBKEY, 1, jwt::error::ecdsa_error::write_key_failed}, { | ||
| &fail_BIO_ctrl, 1, jwt::error::ecdsa_error::convert_to_pem_failed |
There was a problem hiding this comment.
I am not sure how I feel about this. Its a bit of a breaking change but its more consistent...
There was a problem hiding this comment.
I like the general idea of cleaning up the error handling, given that rsa_error is kind of wrong if you load an ecdsa key.
However I am not sure if I like the implementation. You are essentially duplicating every key error into ecdsa and rsa and the user has to care about that, even though the underlying error is basically the same. With all the upcoming/planned work towards jwks, key related error cases and codes are only going to get more which makes me feel/wonder if a separate error category for key handling might be a better fit. Especially given a key class would very likely be capable of holding all kinds of keys (rsa, ecdsa, maybe even symetric) and I really don't care which one was inside when I want to convert it to pem.
I am still approving this, since the code on its own looks good and it definitly is better than the current solution. I am just not sure if its the best solution.
|
I feel the same way, I was considering fixing the eddsa keys being both RSA and EC -- the big think for 0.8.0 will likely be improving the error handling starting with #252 I am happy I did this because it gave me a better idea of what we have and need to possibly change 👍 I was thinking of a proper |
LoadPublicKeyFromStringReferenceWithEcCert
Trying to test #186 to see if it's easy