ramosbugs / openidconnect-rs

OpenID Connect Library for Rust
MIT License
404 stars 100 forks source link

Replace ring with RustCrypto crates #96

Closed sbihel closed 1 year ago

sbihel commented 1 year ago

Replaces #58. Removes ring completely.

The breaking changes I see are:

Closes #78. Closes #94.

codecov[bot] commented 1 year ago

Codecov Report

Merging #96 (9db2d99) into main (4ba900b) will increase coverage by 0.61%. The diff coverage is 79.87%.

@@            Coverage Diff             @@
##             main      #96      +/-   ##
==========================================
+ Coverage   72.74%   73.35%   +0.61%     
==========================================
  Files          16       16              
  Lines        3918     3989      +71     
==========================================
+ Hits         2850     2926      +76     
+ Misses       1068     1063       -5     
Impacted Files Coverage Δ
src/jwt.rs 80.86% <ø> (ø)
src/core/mod.rs 53.76% <33.33%> (-0.64%) :arrow_down:
src/core/crypto.rs 79.03% <81.48%> (-2.51%) :arrow_down:
src/core/jwk.rs 88.01% <83.05%> (-2.34%) :arrow_down:
src/macros.rs 23.84% <0.00%> (+6.60%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ramosbugs commented 1 year ago

Thanks for the PR! It looks like these new dependencies will raise the MSRV to 1.57, which will require a major version bump for this crate. I'm working on some separate changes to update the MSRV that CI checks against. Once that's pushed, we can hopefully rebase this PR on top of that to make sure the updated CI passes before merging. I'll take a closer look at the PR itself soon too.

ramosbugs commented 1 year ago
  • PSS signatures are a bit different (had to modify a few tests), which I'm not sure why. Every parameter is the same; and

I think this is ok. PSS signatures are probabilistic (i.e., the same input signed twice always produces distinct signatures, subject to the PRNG). The way that the PRNG seed is used for producing the random salt isn't specified by RFC 8017. I'm guessing ring and the new crates just use different but equally (?) valid approaches.

ramosbugs commented 1 year ago

Thanks for updating the PR! It looks like clippy is complaining on the 1.57 CI build. I'll try to do another review and hopefully merge the changes this weekend.