Feat: Added support for JWK decoding Ed25519 keys#506
Conversation
|
Thanks for the review @arckoor! I have implemented the requested changes if I could please get a re-review. |
arckoor
left a comment
arckoor
left a comment
There was a problem hiding this comment.
Sorry this took so long to get back to.
Classifying Ed448 curves properly and later on rejecting them with UnsupportedAlgorithm is a nice idea, however doing it like this is misleading for PEM users:
let privkey_pem = include_bytes!("private_ed448_key.pem");
let encoding_key = EncodingKey::from_ed_pem(privkey_pem).unwrap(); // this succeeds
let claims = Claims {... };
let token = encode(&Header::new(Algorithm::EdDSA), &claims, &encoding_key).unwrap(); // obviously fails because it's not EdDSA, but there is also no Ed448 algorithm here :/If I can construct a (PEM) key, I'd expect to be able to sign with it.
So I'd prefer if that bit were removed. Should also mean that you don't need classify_ed_curve at all anymore, and can just match based on key length
| pkcs1::{DecodeRsaPrivateKey, DecodeRsaPublicKey}, | ||
| traits::PublicKeyParts, | ||
| }; | ||
| use ed25519_dalek::SigningKey; |
There was a problem hiding this comment.
Import as Ed25519SigningKey
| // --- EllipticCurve Constants --- | ||
| // Key Lengths | ||
| // ED25519: https://datatracker.ietf.org/doc/html/rfc8032#section-5.1.5 | ||
| pub(crate) const ED25519_PUBLIC_KEY_LENGTH: usize = 32; | ||
| // ED448: https://datatracker.ietf.org/doc/html/rfc8032#section-5.2.5 | ||
| pub(crate) const ED448_PUBLIC_KEY_LENGTH: usize = 57; |
| } | ||
|
|
||
| pub struct EdDSAVerifier(VerifyingKey); | ||
| pub struct EdDSAVerifier(pub VerifyingKey); |
|
@arckoor I'm made the changes and added a test case for extracting the PEM key which now fails as expected 👍 |
|
Sorry, I should have been clearer, I meant to remove the 448 stuff entirely
The 448 test is good to keep though |
|
@arckoor I have removed all Ed448 references now (keeping the test). Let me know if there is anything else you would like me to change. |
| let (curve_type, x) = match &key.kind() { | ||
| DecodingKeyKind::SecretOrDer(pub_bytes) => { | ||
| match pub_bytes.len() { | ||
| // ED25519: https://datatracker.ietf.org/doc/html/rfc8032#section-5.1.5 | ||
| 32 => (EllipticCurve::Ed25519, pub_bytes), | ||
| _ => return Err(ErrorKind::InvalidEddsaKey.into()), | ||
| } | ||
| } | ||
| _ => return Err(ErrorKind::InvalidKeyFormat.into()), | ||
| }; |
There was a problem hiding this comment.
In #515 should be able to use try_get_as_bytes (just note to self so I don't forget :p)
2 participants
Summary: Added decoding JWK support for Ed25519 keys
Changes:
unimplementedJwk::from_encoding_keywhenAlgorithm::EdDSAxparameter (https://datatracker.ietf.org/doc/html/rfc8037#section-2)EllipticCurvetypeEd448for Ed448 curvesSolves: #244