support ring and aws-lc-rs (default = ring)

[GlenDC]

Closes #389

Tested using:

cargo hack check --each-feature --no-dev-deps
cargo test
cargo test --features aws_lc_rs
cargo test --examples
cargo test --examples --features aws_lc_rs

Not ran it with wasm / windows enabled though.
But I would hope the flags manage that?

Current approach does mean you always pull in ring, but it shouldn't compile if you are not using it.
Do not know of an elegant way to do it differently. We can make ring a feature as well that is enabled by default,
but it would make it akward for when you have both ring and aws-lc-rs enabled.

rustls fixes that by then just making a runtime error. Bit odd, I would prefer just it to fail then. So that's an option to still add that here, not sure if it's desired/required.

[GlenDC]

Ring does have key_pair.public_modulus_len() but it's deprecated.
So this can be removed if we upstream a patch to aws-lc-rs to have public() also available.

[GlenDC]

All other functions used by jsonwebtoken have the rng param which is ignored. Sadly this one seems to be forgotten. We could also try to upstream this to aws-lc-rs, but would require a major release, not sure how eager they are for that :)

[justsmth]

Hello! -- I stumbled across this thread, and thought I would provide a little context around this difference.

In ring v0.16.x, EcdsaKeyPair::from_pkcs8 only takes two parameters. But in ring 0.17, this function takes three parameters. Because aws-lc-rs v1.x maintains compatibility with ring 0.16.x, our function (likewise) only takes two parameters.

Unfortunately, I don't see a way we could support a 3rd parameter w/o breaking backwards compatibility. But feel free to drop us an issue if you have questions or would like to suggest an improvement to our API. Thanks!

[GlenDC]

All in all this PR is pretty innocent I think.
Only caveat is that you do expose ErrorKind in your API.

This is AFAIK the only place where you do leak the crypto library used...
If it wasn't for that it would be an invisible change... I think

[Keats]

ah I thought you meant to do the PR having the 2 rust-crypt/aws-lc-rs backends and not ring/aws-lc

[GlenDC]

That’s fine for me as well. But then we might need that to be merged first?

[Keats]

I think I can create another branch and merge it on that branch if that's ok

[Keats]

I've made a mess in #404 commit wise i'll try to clean it when I have time but should be ok to start

[Keats]

History on the other PR is fixed

[sidrubs]

Hey @GlenDC,

There seems to be constant refrain of people needing different crypto backends for their use-case.

I recently opened an issue (#409), that suggests the use of traits to define a signer and verifier. If a user ever wanted a non-implemented crypto backend they would then be able to implement the traits for it and make use of it with the jsonwebtoken crate. There could be "standard" implementations for ring, aws-lc-rs, and RustCrypto (all behind appropriate feature flags).

I realize you have a working PR here, but would you mind if I implemented it with traits instead? I feel it would make the crate more flexible going forward. I am also open to other suggestions that you would prefer.

[abalmos]

@sidrubs @GlenDC I'll add my vote in this general direction. I could make use of an external signer and verifier implementation for building out external authentication in NATS. NATS expects a variant of ed25519-nkey even though it is essentially EdDSA, meaning it is not possible to use the rest of the functionality in this crate.

[sidrubs]

Hey @GlenDC,

There seems to be constant refrain of people needing different crypto backends for their use-case.

I recently opened an issue (#409), that suggests the use of traits to define a signer and verifier. If a user ever wanted a non-implemented crypto backend they would then be able to implement the traits for it and make use of it with the jsonwebtoken crate. There could be "standard" implementations for ring, aws-lc-rs, and RustCrypto (all behind appropriate feature flags).

I realize you have a working PR here, but would you mind if I implemented it with traits instead? I feel it would make the crate more flexible going forward. I am also open to other suggestions that you would prefer.

I have gone ahead and made a draft PR with a POC on a possible method of implementing this.