Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Misleading function name DecodingKey::from_ed_der. #362

Open
tokarevart opened this issue Jan 13, 2024 · 10 comments
Open

Misleading function name DecodingKey::from_ed_der. #362

tokarevart opened this issue Jan 13, 2024 · 10 comments

Comments

@tokarevart
Copy link

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 #244 (comment)

@tokarevart tokarevart changed the title Misleading function name [DecodingKey::from_ed_der](https://docs.rs/jsonwebtoken/latest/jsonwebtoken/struct.DecodingKey.html#method.from_ed_der). Misleading function name DecodingKey::from_ed_der. Jan 13, 2024
@Houndie
Copy link

Houndie commented Mar 24, 2024

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

@Keats
Copy link
Owner

Keats commented Mar 25, 2024

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
Copy link
Author

tokarevart commented Mar 25, 2024

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
Copy link
Author

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
Copy link
Owner

Keats commented Apr 1, 2024

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

@tokarevart
Copy link
Author

tokarevart commented Apr 2, 2024

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

@Keats
Copy link
Owner

Keats commented Apr 2, 2024

I meant the branch in general.

@tokarevart
Copy link
Author

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

@tokarevart tokarevart mentioned this issue Apr 4, 2024
@ferdonline
Copy link

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
Copy link
Owner

Keats commented Aug 3, 2024

I'll happily accept a PR fixing it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants