Decoupled crypto backends (the rest) #428
Conversation
# 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.
|
:o how did i miss this sorry, i'll go through it asap |
| @@ -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"] | |||
There was a problem hiding this comment.
should we have a default crypto backend or let users pick the one they want?
There was a problem hiding this comment.
I mean, that's up to you. I personally think it's nice to have a reasonable default.
src/decoding.rs
Outdated
| return Err(new_error(ErrorKind::MissingAlgorithm)); | ||
| } | ||
|
|
||
| // Todo: This behaviour is currently not captured anywhere. |
There was a problem hiding this comment.
If you have an idea on how to improve that, i'll take it
There was a problem hiding this comment.
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...?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I've re-enabled this with adaptation to the new code structure, and at least it doesn't seem to break any tests.
|
I'll have a go at reducing the duplication through some macros and addressing the other comments. |
|
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. |
src/decoding.rs
Outdated
| return Err(new_error(ErrorKind::MissingAlgorithm)); | ||
| } | ||
|
|
||
| // Todo: This behaviour is currently not captured anywhere. |
There was a problem hiding this comment.
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
tests/rsa/mod.rs
Outdated
| @@ -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. | |||
There was a problem hiding this comment.
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
force-pushed
the
decoupled-crypto-backends
branch
from
4664843 to
76b2fe3
Compare
|
Addressed the latest comments, and also inlined |
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.