Skip to content

Feat: Added support for JWK decoding Ed25519 keys#506

Open
richy1623 wants to merge 9 commits into
Keats:masterfrom
richy1623:feat/jwk_decode_ed25519
Open

Feat: Added support for JWK decoding Ed25519 keys#506
richy1623 wants to merge 9 commits into
Keats:masterfrom
richy1623:feat/jwk_decode_ed25519

Conversation

@richy1623

@richy1623 richy1623 commented Apr 28, 2026

Copy link
Copy Markdown

Summary: Added decoding JWK support for Ed25519 keys

Changes:

  • Implemented the unimplemented Jwk::from_encoding_key when Algorithm::EdDSA
  • Added to the JWK utils to support fetching Ed25519
  • Updated both backends to support extracting the x parameter (https://datatracker.ietf.org/doc/html/rfc8037#section-2)
  • Added a new EllipticCurve type Ed448 for Ed448 curves
  • Updated the decoder to allow decoding PEMs of type Ed448
  • Added tests for all changes

Solves: #244

@richy1623 richy1623 marked this pull request as ready for review April 28, 2026 11:29
Comment thread src/crypto/aws_lc/eddsa.rs Outdated
Comment thread src/crypto/mod.rs Outdated
Comment thread src/jwk.rs Outdated
Comment on lines +495 to +498
let curve_type: EllipticCurve = match key.inner().len() {
48 => Ok(EllipticCurve::Ed25519),
73 => Ok(EllipticCurve::Ed448),
_ => Err(Error::from(ErrorKind::InvalidEddsaKey)),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the point of classifying Ed448 curves if the provider just says "I don't support this" anyways. I.e. why have the extra enum value and detection logic if this could be a simple

match key.inner().len() {
    48 => Ok(...),
    _ => Err(...),
}

I haven't worked with 448 before, why is this one checking for length of 73 when from_ed25519_encoding_key is checking for 57?

@richy1623 richy1623 Jun 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mismatch was actually a mistake, 57 bytes is the length of the key itself, 73 bytes is the length of the key including the ANS.1 header.

I have however, improved the checking logic to rather use OIDs instead of length checks which is more reliable. The logic was largely lifted from the pem/decoder#classify_pem method where it uses ans1 headers to determine algorithm types.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for why I would prefer to include Ed448 in the check is they are valid Ed algorithms, so to provide a clear error code (ErrorKind::UnsupportedAlgorithm) we need to first identify it as a valid Ed algorithm, then down the line we can state it is unsupported, rather than the catch all InvalidAlgorithm/InvalidEddsaKey.

Comment thread tests/eddsa/mod.rs Outdated
Comment thread src/jwk.rs
/// Type of cryptographic curve used by a key. This is defined in
/// [RFC 7518 #7.6](https://tools.ietf.org/html/rfc7518#section-7.6)
#[derive(Clone, Debug, Default, Eq, PartialEq, Serialize, Deserialize, Hash)]
#[non_exhaustive]

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following fa89018 the EllipticCurve's should also be non-exhaustive

@richy1623

Copy link
Copy Markdown
Author

Thanks for the review @arckoor! I have implemented the requested changes if I could please get a re-review.

@richy1623 richy1623 requested a review from arckoor June 14, 2026 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants