search

ramosbugs / openidconnect-rs

OpenID Connect Library for Rust
MIT License
427 stars 103 forks source link

Added Send to CoreRsaPrivateSigningKey.rng #93

Closed Lzok closed 2 years ago

Lzok commented 2 years ago

Hello! How are you?

It looks like the type that holds RSA keys from the openidconnectcrate (CoreRsaPrivateSigningKey) is not Send because it holds a reference to a ring::rand:SecureRandom + 'static. This seems a bit odd to me in real use cases (e.g. backends with multiple threads), since it doesn’t look like a key should be something !Send.

One question I ask to myself is why can’t I share a single key among several worker threads signing tokens? Do you have any concern with this that I am not being aware of?

Important Note This PR does not break any public API, that's why I am pull requesting it directly.

I would like to hear your thoughts on this and how we can improve the solution together.

Thank you!

ramosbugs commented 2 years ago

Thanks for the PR!

Do you have any concern with this that I am not being aware of?

Nope, just an oversight. Will merge once CI passes. I agree that this doesn't appear to be a breaking change since it only affects internal APIs.

codecov[bot] commented 2 years ago

Codecov Report

Merging #93 (ac5f2dd) into main (c25eb06) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #93   +/-   ##
=======================================
  Coverage   72.11%   72.11%           
=======================================
  Files          16       16           
  Lines        3794     3794           
=======================================
  Hits         2736     2736           
  Misses       1058     1058
Impacted Files Coverage Δ
src/core/jwk.rs 90.80% <ø> (ø)

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

Lzok commented 2 years ago

@ramosbugs Thanks a lot!

ramosbugs commented 2 years ago

This has now been released in 2.4.0!