Misleading function name `DecodingKey::from_ed_der`.

[tokarevart]

It seems like DecodingKey::from_ed_der function actually expects raw 32 bytes public key, which ring's Ed25519KeyPair::public_key returns. Also running openssl asn1parse -inform der -in file-with-pubkey shows again that these 32 bytes are not DER encoded. I've also additionally checked it using ed25519-dalek crate. So essentially it seems DecodingKey::from_ed_der name is just misleading and DecodingKey doesn't yet support DER encoded ed25519 public keys. Maybe it would be a good idea to add a new method like DecodingKey::from_ed behaving exactly the same as existing DecodingKey::from_ed_der, but specified in the descrition that it expects raw 32 bytes public key. What to do with existing misleading name of DecodingKey::from_ed_der I'm not sure, changing implementation would be a (potentially) quite annoying breaking change.

Originally posted by @tokarevart in https://github.com/Keats/jsonwebtoken/issues/244#issuecomment-1890308797

[Houndie]

Thank you for this, I 100% agree, and you just saved me 30 minutes of debugging.

[Keats]

It would be a breaking change but it doesn't do what it says it does it should be more of a bug fix? How hard is it to to actually have a DecodingKey::from_ed_der?

[tokarevart]

Maybe it can be called a bugfix, but I think a lot of people already depend on the current behaviour and their code would crash at runtime if implementation changed, no compiler warning, so maybe it would be a good idea to somehow check in the body of DecodingKey::from_ed_der that input is actually just a raw 32 bytes, not a DER, and panic with a helpful message. Maybe also even bump a major version or include a fix for the current problem when the next major version bump happens. Or maybe there's some other way I didn't think about.

Regarding how hard it is to implement a DER pubkey (I think we actually need SPKI?) decoding I don't know, I don't think this crate depends on anything that can do that (ring can't afaik). I've used some dependency of ed25519-dalek for it.

[tokarevart]

Oh it seems #318 adds ed25519-dalek as a dependency, so when/if it's merged adding DER/SPKI decoding wouldn't be much of a problem I think.

[Keats]

Yes #318 is an interesting one. Can you try it if you have time?

[tokarevart]

You mean implement a proper DecodingKey::from_ed_der on top of #318 branch? I'll try it this week/weekend.

[Keats]

I meant the branch in general.

[tokarevart]

Oh okay, I'll take a look later then.

[ferdonline]

I have just spent hours checking why my openssl DER key was not being accepted until I found this 😢 If the function is not expecting proper der as created with openssl -outform der please rename it

[Keats]

I'll happily accept a PR fixing it