Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add alternate RustCrypto backend #58

Closed
wants to merge 1 commit into from
Closed

Conversation

sbihel
Copy link
Contributor

@sbihel sbihel commented Jan 5, 2022

Hiya,

I have been working on deploying an IdP on Cloudflare Workers (wasm32-unknown-unknown target) and it is not well supported by ring. So I have started working on adding support for another "crypto backend" with the RustCrypto crates. And so far, it seems to work just fine. And only a few PSS padding tests are failing.

So I am opening this draft PR to see if you would be interested in the first place, and if you have any opinion or piece of advice on how to proceed. Apologies for the current state of the code, the APIs are fairly different and I wanted to limit the amount of refactoring for this first pass.

Thanks for making this crate, it's been tremendously helpful!

@ramosbugs
Copy link
Owner

hi @sbihel, thanks for the PR! Given some other challenges around ring (e.g., mixed versions within a single binary crate), I'd definitely be in favor of removing the dependency, or at least enabling alternatives via a feature flag as this PR does.

the biggest constraint right now is time: I'm focused on shipping a new SaaS project right now, and I probably won't have the bandwidth to properly review and support this substantial of a change for a few months. I'd suggest using a vendored version of this crate with your changes for now, and I'll plan to circle back in the spring to help get this merged upstream. thanks again!

@ctron
Copy link
Contributor

ctron commented Apr 12, 2022

I am currently trying to get this working. However, there is one issue with that PR. When any other crate pulls in ring too, this will automatically enable all sections in this crate (which use ring) too. So you get duplicate functions.

I would suggest to use #[cfg(not(feature="rustcrypto"))] instead. So it wouldn't matter if ring would be present. If the user selects rustcrypto, then that is what is being used.

@ramosbugs
Copy link
Owner

I am currently trying to get this working. However, there is one issue with that PR. When any other crate pulls in ring too, this will automatically enable all sections in this crate (which use ring) too. So you get duplicate functions.

I would suggest to use #[cfg(not(feature="rustcrypto"))] instead. So it wouldn't matter if ring would be present. If the user selects rustcrypto, then that is what is being used.

Interesting, are you saying that if you include this crate with default-features = false, features = ["rustcrypto"] that #[cfg(feature = "ring")]-gated code in this crate gets compiled if ring is otherwise included by the parent crate? That's pretty surprising 🤯

@ctron
Copy link
Contributor

ctron commented Apr 13, 2022

Interesting, are you saying that if you include this crate with default-features = false, features = ["rustcrypto"] that #[cfg(feature = "ring")]-gated code in this crate gets compiled if ring is otherwise included by the parent crate? That's pretty surprising 🤯

To my understanding that is just how features work. An optional dependency automatically has a feature with the same name. So when I include ring in my build, the that feature (in the whole tree) would no longer be optional, and thus becomes active. So it would be active here too.

Also see: https://doc.rust-lang.org/cargo/reference/features.html

@sbihel
Copy link
Contributor Author

sbihel commented May 6, 2022

That makes sense, if it's decided to keep ring as an option we can rely on the rustcrypto feature as said above, or put ring behind its own feature, or maybe the new namespaced feature would make it work without any change to the code.

Not sure what's the best way to make them mutually exclusive though, it would probably be preferable to fail the compilation if both are enabled.

@ctron ctron mentioned this pull request Jul 6, 2022
@Erik1000 Erik1000 mentioned this pull request Aug 29, 2022
@ctron
Copy link
Contributor

ctron commented Oct 7, 2022

@ramosbugs is it possible to make any progress on this issue?

@ramosbugs
Copy link
Owner

I'll try to find some time to review PRs related to this effort. I'm fine with removing ring entirely rather than adding RustCrypto behind a feature flag and maintaining both versions of the code. Hopefully this won't involve any breaking changes to the public API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants