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

Feature request : optionnaly use aws-lc-rs instead of ring #389

Open
nicolaspernoud opened this issue May 12, 2024 · 9 comments · May be fixed by #402
Open

Feature request : optionnaly use aws-lc-rs instead of ring #389

nicolaspernoud opened this issue May 12, 2024 · 9 comments · May be fixed by #402

Comments

@nicolaspernoud
Copy link

Hello,

Thanks for this useful crate.
A lot of the ecosystem seems to be moving from ring to aws-lc-rs as default cryptographic library (like https://github.com/rustls/rustls/blob/a2c21fe0509f634431e72bffe8803fca1f892d56/rustls/Cargo.toml#L31) .
Maybe it would be nice to propose the alternative.

Best regards.

@Keats
Copy link
Owner

Keats commented May 13, 2024

See #377

It seems there are issues building it on Windows?

@nicolaspernoud
Copy link
Author

Well, maybe the best would be to offer the choice between ring and aws-lc-rs, defaulting to ring on windows ?

@Keats
Copy link
Owner

Keats commented May 14, 2024

I'd rather not have multiple backend if possible

@nicolaspernoud
Copy link
Author

Understandable, but that seems to be the tendency in all the librairies switching from ring to aws-lc-rs...
I guess that the alternative is to wait for aws-lc-rs to build properly on windows.
Closing for now...

@GlenDC
Copy link

GlenDC commented Aug 27, 2024

FWIW @Keats @nicolaspernoud
made a fork where I replace ring with aws-lc-rs

https://crates.io/crates/jsonwebtoken-aws-lc

Having done the work I realized that it is really trivial for you to support both backends.

In pseudo code it would be as simple as:

// private: src/crypto.rs

#[not wasm and not windows and aws-lc-rs]
pub(crate) use aws_lc_rs::{...};

#[wasm or windows or not aws-lc-rs]
pub(crate) use ring::{...};

The ring API surface that you touch on with your library is 100% compatible with aws-lc-rs,
so it would be as simple as using crate::crypto instead of ::ring in your codebase,
and given this ring-compatibility is by design and the fact you do not really expose ring for anything important,
it seems like a save thing to do?

Either way... didn't want to pressure you into anything. It is your project and crate @Keats so I fully respect whatever you wish to do with it. If you do like it I do not mind making a ticket and PR for it. Either way, without any pressure or demand I did want to unblock myself (and possibly others) as I needed this crate for several projects and this was still the only project using ring...

(I have nothing against ring btw... I actually like it. But the rustls ecosystem seems to have moved in favour of aws-lc-rs so like a good sheep I'm just following it in this instance... And Yes sure they allow to use ring, but it gets harder for deps of deps and deps of deps of deps, etc...)

@Keats
Copy link
Owner

Keats commented Aug 27, 2024

I don't mind that switch too much if it's easy to build on Windows. Another thing to consider is #318 which removes Ring entirely and uses rust-crypto crates instead. Easier to build and we can probably add EcDSA.
It's definitely possible to support both of those backends, it just needs some work from someone to expose a facade API that can be used with aws-ls-rc and rust-crypto stuff. I would love a PR for it.

@GlenDC
Copy link

GlenDC commented Aug 27, 2024

I can make it happen for sure, the PR that is.
Would you want me to wait until #318 is done and merged by the ongoing people, or how do you see it?

@Keats
Copy link
Owner

Keats commented Aug 28, 2024

I'm not sure what's the best way to go at it. #318 removes ring but we want to add a facade so maybe it could be done as starting a branch, merging that PR on it (squashing commits) and then adding the facade by taking back the ring code from master? Not sure, do as you see fit.

@GlenDC
Copy link

GlenDC commented Aug 28, 2024

Ok going to open a PR now. Can we re-open this issue or you want to create a new issue with a different/updated scope? Even though seems pretty much like the feature that is requested above.

@Keats Keats reopened this Aug 28, 2024
@GlenDC GlenDC linked a pull request Aug 28, 2024 that will close this issue
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 a pull request may close this issue.

3 participants