Decoupled crypto backends (the rest) #428

sulami · 2025-05-11T23:17:57Z

This is following up on #410 with the rest of the decoupled implementations, and addressing most of the review comments (I think?).

There are still some todos left for documentation, which I haven't filled in yet.

I've verified that tests & clippy pass with all combinations of features, but a lot of this I only know enough about to be dangerous, so I'd appreciate close looks at the actual implementations to make sure e.g. I've picked the correct library functions in each case.

# Conflicts: # Cargo.toml # examples/custom_header.rs # examples/validation.rs # src/crypto/ecdsa.rs # src/crypto/rsa.rs # src/jwk.rs

This is following the existing scheme established in hmac & rsa.

Keats · 2025-06-24T07:23:11Z

:o how did i miss this sorry, i'll go through it asap

Keats

Looks great overall. I think some macros would help reduce the duplication

Keats · 2025-06-24T08:11:32Z

Cargo.toml

@@ -48,8 +60,10 @@ time = { version = "0.3", features = ["wasm-bindgen"] }
 criterion = { version = "0.4", default-features = false }

 [features]
-default = ["use_pem"]
+default = ["use_pem", "aws_lc_rs"]


should we have a default crypto backend or let users pick the one they want?

I mean, that's up to you. I personally think it's nice to have a reasonable default.

aws_lc_rs would be a good default

src/crypto/aws_lc/ecdsa.rs

src/crypto/aws_lc/hmac.rs

src/crypto/aws_lc/rsa.rs

src/crypto/rust_crypto/hmac.rs

src/crypto/utils.rs

Keats · 2025-06-24T08:34:52Z

src/decoding.rs

+        return Err(new_error(ErrorKind::MissingAlgorithm));
+    }
+
+    // Todo: This behaviour is currently not captured anywhere.


If you have an idea on how to improve that, i'll take it

This comment is actually @sidrubs', I'm not entirely sure what they mean by that, whether it's not covered by tests, or docs, or...?

We probably want to uncomment it for now, but ideally we find a better way to handle it before releasing the next version since it trips up people

I've re-enabled this with adaptation to the new code structure, and at least it doesn't seem to break any tests.

tests/hmac.rs

sulami · 2025-06-25T02:43:33Z

I'll have a go at reducing the duplication through some macros and addressing the other comments.

sulami · 2025-06-25T04:20:44Z

Alright, done some refactoring to reduce the duplication and re-enabled the WASM HMAC test cases. I checked that tests pass with both AWS-LC and Rust-crypto.

Keats

That looks great. I'll have another look before merging but it's a great improvement!

Keats · 2025-07-02T15:34:17Z

src/decoding.rs

+        return Err(new_error(ErrorKind::MissingAlgorithm));
+    }
+
+    // Todo: This behaviour is currently not captured anywhere.


We probably want to uncomment it for now, but ideally we find a better way to handle it before releasing the next version since it trips up people

Keats · 2025-07-02T15:36:38Z

tests/rsa/mod.rs

@@ -26,6 +26,8 @@ pub struct Claims {
    exp: i64,
 }

+// Todo: These no longer apply because `verify` does not exist, would probably need to convert it to test the factory for getting signers and verifiers. But I would rather this not be part of the public facing API.


is it still a todo?

Ah true, missed that one. verify does indeed exist again.

This was commented out with the note that it wasn't "captured anywhere," presumably in tests, but is present in similar form in the pre-decoupling version of the code.

They had todos for docstrings, and I realized they're single-use and should probably not be exposed publicly anyway.

sulami · 2025-07-03T03:54:58Z

Addressed the latest comments, and also inlined _encode and _decode, which were single use, for some reason pub, and had TODO as docstrings.

sulami · 2025-08-05T00:28:17Z

@Keats Just in case you missed it again, I think this should be good for another review now.

Keats

That looks good. One small copy/paste issue but fine otherwise. I need to create a 0.10 branch to group everything

src/crypto/rust_crypto/rsa.rs

Keats · 2025-08-14T18:42:21Z

The wasm failure seems legit?

sulami · 2025-08-14T23:15:38Z

Looking at the AWS-LC docs, I don't think it builds/is supported on WASM. I've also recently read this with regards to default features, and I'm thinking maybe we shouldn't have a default backend after all? That way we could also pick a different backend for the WASM build.

Keats · 2025-08-15T10:01:38Z

Yeah maybe let's not have a default if the default doesn't work with wasm.

In most situations AWS-LC is the better backend, but it's not available on all platforms, for example WASM. Explicitly asking the user to pick a backend is also more future-proof.

Keats · 2025-08-18T12:20:57Z

Looks like legit failures as well

sulami · 2025-08-19T06:29:05Z

I don't know if this'll work or not, at least it builds on my machine, but it doesn't pass the WASM tests. Neither does master though, so I'm not sure that counts for much.

I'm not terribly familiar with WASM overall, so I wouldn't mind additional pointers & opinions.

Cargo.toml

src/validation.rs

This is fixing a Clippy complaint.

sulami · 2025-08-23T02:28:49Z

I've addressed @jplatte's feedback, and I've also run a wasm CI run in my fork to shorten the feedback loop a bit here.

jplatte

Looking forward to this landing :)

itanxiao and others added 30 commits June 19, 2023 13:35

support wasm

33d2c21

upgrade dependency

b116e44

remove ring

e2a203f

Merge branch 'master' into wasm_support

5bb1d75

# Conflicts: # Cargo.toml # examples/custom_header.rs # examples/validation.rs # src/crypto/ecdsa.rs # src/crypto/rsa.rs # src/jwk.rs

fix wasm test failed

d80832c

fix cargo format

69cbfa3

change ci branch

a66864f

fix ci

485fb9f

fix ci

490564c

fix ci

a672130

remove ci branch

73ea869

fix examples ed25519 test failed

a41c817

Next version

6f9b553

Remove downgrade steps

e061882

Merge branch 'master' into new-backends

e5ba5a1

feat(encoder): Add encoder builder

502faa4

feat(encoder): Convert to dynamic dispatch

a701547

feat(decoder): Create decoder

6a41ba7

test: Get HMAC tests passing

66d54be

docs: Neaten up docstrings

0886064

feat(crypto): Implement JwtSigner and JwtVerifier for aws-lc-rs

337f9ed

feat: Remove builder style implementation

a0431d8

feat: Use original encoding and decoding key structs

4225e1f

feat(crypto): Add RSA family

78e84c1

Add ECDSA via AWS-LC

2a50e0d

This is following the existing scheme established in hmac & rsa.

Implement EdDSA through AWS-LC

19ee8f3

Verify ES and ED keys are of the right type

765c04c

Implement RSA-PSS via AWS-LC

e1305ef

Implement EdDSA via RustCrypto/Dalek

e37ebee

Implement EcDSA through RustCrypto

25f84c3

Keats requested changes Jun 24, 2025

View reviewed changes

sulami added 2 commits June 25, 2025 13:01

Reduce code duplication in crypto impls through macros

01eca4c

Move try_get_hmac_secret directly into en-/decoding key impls

903ecff

Keats reviewed Jul 2, 2025

View reviewed changes

sulami added 3 commits July 3, 2025 12:39

Re-enable the HMAC sign & verify test cases

b8d2906

Uncomment the algorithm matching in verify_signature again

d6151bd

This was commented out with the note that it wasn't "captured anywhere," presumably in tests, but is present in similar form in the pre-decoupling version of the code.

Inline _encode and _decode

76b2fe3

They had todos for docstrings, and I realized they're single-use and should probably not be exposed publicly anyway.

sulami force-pushed the decoupled-crypto-backends branch from 4664843 to 76b2fe3 Compare July 3, 2025 03:53

sulami requested a review from Keats August 5, 2025 00:28

Keats approved these changes Aug 12, 2025

View reviewed changes

src/crypto/rust_crypto/rsa.rs Outdated Show resolved Hide resolved

Fix some docstring typos

09e8d42

Don't default to a crypto backend

f35c3e8

In most situations AWS-LC is the better backend, but it's not available on all platforms, for example WASM. Explicitly asking the user to pick a backend is also more future-proof.

jplatte reviewed Aug 19, 2025

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

jplatte reviewed Aug 19, 2025

View reviewed changes

src/validation.rs Outdated Show resolved Hide resolved

Explicitly annotate an implicit lifetime

755d868

This is fixing a Clippy complaint.

sulami force-pushed the decoupled-crypto-backends branch from 6619f3b to 570d732 Compare August 23, 2025 02:15

Enable getrandom/js for WASM tests in CI

eca3603

sulami force-pushed the decoupled-crypto-backends branch from 570d732 to eca3603 Compare August 23, 2025 02:27

jplatte approved these changes Aug 23, 2025

View reviewed changes

Decoupled crypto backends (the rest) #428

Are you sure you want to change the base?

Decoupled crypto backends (the rest) #428

Uh oh!

Conversation

sulami commented May 11, 2025

Uh oh!

Keats commented Jun 24, 2025

Uh oh!

Keats 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

sulami commented Jun 25, 2025

Uh oh!

sulami commented Jun 25, 2025

Uh oh!

Keats 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!

sulami commented Jul 3, 2025

Uh oh!

sulami commented Aug 5, 2025

Uh oh!

Keats left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Keats commented Aug 14, 2025

Uh oh!

sulami commented Aug 14, 2025

Uh oh!

Keats commented Aug 15, 2025

Uh oh!

Keats commented Aug 18, 2025

Uh oh!

sulami commented Aug 19, 2025

Uh oh!

Uh oh!

Uh oh!

sulami commented Aug 23, 2025

Uh oh!

jplatte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!