implement jwa algorithms #650

valkrypton · 2025-07-31T09:59:25Z

working on #621.

@GlenDC I've written the conversions in rama-crypto/src/jose/jwa.rs file. Let me know if they are correct. However, I'd like some guidance on what changes needs to be made in the rama-crypto/src/jose/jwk.rs file.

GlenDC · 2025-07-31T10:02:46Z

I've asked @soundofspace to do the initial reviews and guidance here. He'll come back to you.

soundofspace · 2025-07-31T15:11:03Z

For changes in rama-crypto/src/jose/jwk.rs I would start with RSA as it behaves quite similar to EC curves.

Places that need updating:

Add JWK::new_from_rsa_keypair using RsaKeyPair from awc_lc_rs. Here you create key_type JWKType::RSA with base64 encoded values for n, e (using RsaKeyPair.public_key().modules/exponent)
In JWK::unparsed_public_key implement the reverse so we can get an UnparsedPublicKey
Add RsaKey very similar to EcdsaKey but modify where needed to use RSA stuff
Rename EcdsaKeySigningHeaders to SigningHeaders so you can also use it to sign for RSA
Implement Signer for RsaKey, this logic can probably be copied from the implementation for EcdsaKey but might need small tweaks
Add some tests to verify that everything works like expected. These can be hardcoded to some expected output for a given input, there are lots of other projects online that have already a very nice test suite for this if you need inspiration
If you see opportunities to extend the current (very minimal) test suite please also do, it would appreciated a lot

Also don't be afraid to look at the jwk rfc it's a decently sized one but it's split up in a lot of chapters and it also contains some examples

And thanks for looking into this!

soundofspace · 2025-07-31T14:41:08Z

rama-crypto/src/jose/jwa.rs

+    }
+}
+
+impl TryFrom<JWA> for Box<&dyn RsaEncoding> {


Any specific reason to box this and not use &'static RsaSignatureEncoding instead?

RsaSignatureEncoding is not exposed outside of the crate.

Missed that it was not exposed, in that case this is indeed a good approach

Btw my i've been busy with my day job so this issue slipped my mind. I'll make a complete PR in a few days

No worries, no rush here

Missed that it was not exposed, in that case this is indeed a good approach

@soundofspace do you think that's a mistake of that crate? If so can you perhaps check with those crate authors to get that improved so we can do perhaps better here in future?

@GlenDC already did that a few hours ago

valkrypton · 2025-08-31T15:36:11Z

rama-crypto/src/jose/jwk.rs

+                result.extend_from_slice(&final_len);
+                result.extend_from_slice(&final_sequence);
+
+                Ok(signature::UnparsedPublicKey::new(self.alg.try_into()?, result))


@soundofspace
The UnparsedPublicKey::new takes trait objects of type VerificationAlgorithm as argument. And JWA.alg is of type dyn RsaEncoding. So I couldn't figure out a way to convert our alg field to that type. Any guidance would be appreciated.

valkrypton · 2025-08-31T15:39:09Z

rama-crypto/src/jose/jwk.rs

+            .sign(*alg, self.rng(), data.as_bytes(), &mut sig)
+            .context("sign protected data")?;
+        // problem here
+        Ok(sig.into())


The sign method for RsaKeyPair does not return the Signature and the Signature type does not expose it's new method. So there is no way to construct that type. What should we do here?

soundofspace

Nice progress, and thanks for looking into this!

soundofspace · 2025-09-02T17:38:31Z

rama-crypto/src/jose/jwa.rs

+    }
+}
+
+impl TryFrom<JWA> for Box<&dyn RsaEncoding> {


Can you change this to &'static dyn RsaEncoding so no allocations are needed

soundofspace · 2025-09-02T17:42:13Z

rama-crypto/src/jose/jwa.rs

+            )),
+        }
+    }
+}


If you also add these it should solve the issue you have with converting

impl TryFrom<JWA> for &'static dyn VerificationAlgorithm { type Error = OpaqueError; fn try_from(value: JWA) -> Result<Self, Self::Error> { match value { JWA::RS256 => Ok(&RSA_PKCS1_2048_8192_SHA256),

And once that is is added, the logic from this one, should also be placed in there
impl TryFrom<JWA> for &'static EcdsaVerificationAlgorithm {

Something along these lines:

JWA::PS512 => Ok(&RSA_PSS_2048_8192_SHA512), JWA::ES256 | JWA::ES384 ... => { let signing_algo: &'static EcdsaSigningAlgorithm = value.try_into()?; Ok(signing_algo.deref()) }

Otherwise there might be issues with try_into overlapping for the use case of verification

soundofspace · 2025-09-02T17:50:10Z

rama-crypto/src/jose/jwk.rs

+}
+
+impl Signer for RsaKey {
+    type Signature = Signature;


For Rsa signing this is a different signature

Suggested change

type Signature = Signature;

type Signature = Vec<u8>;

So no need to do extra try_into in the sign method

rama-crypto/src/jose/jwk.rs

soundofspace

Nice progress, some minor remarks but looks like it's almost ready

soundofspace · 2025-09-15T19:27:57Z

rama-crypto/src/jose/jwk.rs

+                let n_der_encoded = utils::encode_integer(n_bytes);
+                let e_der_encoded = utils::encode_integer(e_bytes);
+
+                let mut rsa_public_key_sequence = Vec::new();


It should be possible to create all the vec's in this block with Vec::with_capacity to prevent extra allocations and moves

soundofspace · 2025-09-15T19:44:59Z

rama-crypto/src/jose/jwk.rs

+                let e_der_encoded = utils::encode_integer(e_bytes);
+
+                let mut rsa_public_key_sequence = Vec::new();
+                rsa_public_key_sequence.extend_from_slice(&n_der_encoded);


Since you don't need these "original" vectors later you can you extend instead which takes ownership and is more optimized

soundofspace · 2025-09-15T20:04:17Z

rama-crypto/src/jose/jwk.rs

+                n: String::try_from(n.big_endian_without_leading_zero().to_vec())
+                    .map_err(|e| OpaqueError::from_display(format!("{:?}", e)))?,


n and e should be base64 encoded bytes which is not the same as String::try_from, that one does utf8 encoding, instead use

BASE64_URL_SAFE_NO_PAD.encode(n)

Can you also try to add some unit tests which should detect mistakes like this

soundofspace · 2025-09-15T20:07:32Z

rama-crypto/src/jose/jwk.rs

    }
 }

+pub struct RsaKey {


This one will also have to import/exported in mod.rs

When you run linting/clippy/tests

just qa

or only linting/clippy without tests

just qq

It should also detect these things, same for unused imports

soundofspace · 2025-09-15T20:12:06Z

rama-crypto/src/jose/jwk.rs

+    }
+
+    /// Create pkcs8 der for the current [`RsaKeyPair`]
+    pub fn pkcs8_der(&self) -> Result<(JWA, Pkcs8V1Der<'_>), OpaqueError> {


Suggested change

pub fn pkcs8_der(&self) -> Result<(JWA, Pkcs8V1Der<'_>), OpaqueError> {

pub fn pkcs8_der(&self) -> Result<(JWA, Pkcs8V1Der<'static>), OpaqueError> {

'static is more explicit here and its also what happens internally

soundofspace · 2025-09-15T20:12:48Z

rama-crypto/src/jose/jwk.rs

+
+    fn sign(&self, data: &str) -> Result<Self::Signature, Self::Error> {
+        let mut sig: Vec<u8> = Vec::new();
+        let alg = <&'static dyn RsaEncoding as TryFrom<JWA>>::try_from(self.alg)?;


Suggested change

let alg = <&'static dyn RsaEncoding as TryFrom<JWA>>::try_from(self.alg)?;

let alg = self.alg.try_into()?;

Should be possible to just do this?

soundofspace · 2025-09-15T20:44:01Z

rama-crypto/src/jose/jwk.rs

+        }
+    }
+
+    pub(super) fn encode_integer(mut bytes: Vec<u8>) -> Vec<u8> {


Can you add comment that this function should only be used for parsing JWK encoded rsa values. This function does not work for general asn1 encoding which is fine for this use case, but a comment should prevent other people from later extending this module and using this function for wrong use cases.

Some things that this function does not handle:

empty bytes vec

negative numbers

assumes input is already in minimal format, which is not always the case in general asn 1

Again fine for this use case, but warrants a comment

Could you suggest me some resources where I could learn more about these specs?

GlenDC requested review from soundofspace and GlenDC July 31, 2025 10:00

soundofspace reviewed Jul 31, 2025

View reviewed changes

valkrypton force-pushed the implement-jwa-algorithms branch 2 times, most recently from ba45afd to 4377157 Compare August 31, 2025 15:35

valkrypton commented Aug 31, 2025

View reviewed changes

valkrypton requested a review from soundofspace September 2, 2025 11:33

soundofspace requested changes Sep 2, 2025

View reviewed changes

soundofspace self-assigned this Sep 2, 2025

valkrypton force-pushed the implement-jwa-algorithms branch from 4377157 to e1c1ade Compare September 15, 2025 15:28

valkrypton requested a review from soundofspace September 15, 2025 15:28

soundofspace requested changes Sep 15, 2025

View reviewed changes

valkrypton force-pushed the implement-jwa-algorithms branch 2 times, most recently from 8bfdb48 to 71b1713 Compare September 16, 2025 08:36

valkrypton requested a review from soundofspace September 16, 2025 08:44

implement jwa algorithms

a839ce0

valkrypton force-pushed the implement-jwa-algorithms branch from 71b1713 to a839ce0 Compare September 16, 2025 10:04

valkrypton marked this pull request as ready for review September 18, 2025 10:48

		n: String::try_from(n.big_endian_without_leading_zero().to_vec())
		.map_err(\|e\| OpaqueError::from_display(format!("{:?}", e)))?,

	pub fn pkcs8_der(&self) -> Result<(JWA, Pkcs8V1Der<'_>), OpaqueError> {
	pub fn pkcs8_der(&self) -> Result<(JWA, Pkcs8V1Der<'static>), OpaqueError> {

	let alg = <&'static dyn RsaEncoding as TryFrom<JWA>>::try_from(self.alg)?;
	let alg = self.alg.try_into()?;

Uh oh!

implement jwa algorithms #650

Are you sure you want to change the base?

implement jwa algorithms #650

Uh oh!

Conversation

valkrypton commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GlenDC commented Jul 31, 2025

Uh oh!

soundofspace commented Jul 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valkrypton Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valkrypton Aug 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valkrypton Aug 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soundofspace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

soundofspace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valkrypton Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

valkrypton commented Jul 31, 2025 •

edited

Loading

valkrypton Aug 18, 2025 •

edited

Loading

valkrypton Aug 31, 2025 •

edited

Loading

valkrypton Aug 31, 2025 •

edited

Loading

valkrypton Sep 16, 2025 •

edited

Loading