Expose cryptography backends via CryptoProvider

[arckoor]

Adds a CryptoProvider struct that allows replacing the built-in providers with something custom.
All the details from this implementation that could be considered "interesting" are stolen straight from rustls's CryptoProvider.

I've marked the new_signer, new_verifier and JWK functions from the two built in backends as pub, so you can do stuff like this:

fn new_signer(algorithm: &Algorithm, key: &EncodingKey) -> Result<Box<dyn JwtSigner>, Error> {
    let jwt_signer = match algorithm {
        Algorithm::EdDSA => Box::new(CustomEdDSASigner::new(key)?) as Box<dyn JwtSigner>,
        _ => jsonwebtoken::crypto::aws_lc::new_signer(algorithm, key)?,
    };

    Ok(jwt_signer)
}

i.e. overwrite just specific algorithms.

One area I'm a little unsure about is JwkUtils, 1) about the name and 2) about the Default implementation. The CryptoProvider::signer_ and CryptoProvider::verifier_factory functions are obviously mandatory for a custom provider, but not everyone uses JWK, so the default just uses dummy functions with unimplemented!().

[arckoor]

@Keats any chance of a review on this?

[drusellers]

Would this allow applications to use rustls rather than openssl?

[arckoor]

@drusellers As far as I know rustls doesn't implement the cryptography directly, rather it relies on other crates like ring, aws-lc-rs, rustcrypto, ...
So you can't really use rustls with this, but you can implement the cryptography with whatever backend you choose, be it openssl, botan, or something else entirely (this crate has aws-lc-rs and rustcrypto built in, using these doesn't require this PR)

[Keats]

I think this can be simplified

[arckoor]

@Keats I made the JWK functions private after all. For the PROCESS_DEFAULT_PROVIDER I still think this is the best way to do it, so I left it as is. I also removed the custom-provider feature, because it wasn't really doing anything anymore after making all the things pub / adding getters, so now you users just select neither of the built-in providers.

In regards to the macro, I also left it as is, if #461 goes through and does gate the implementation itself behind a feature, I guess the best would be to remove the macro and write the functions by hand for each built-in provider.

As for the example, I used the botan-rs crate, though I'm not confident you'll like it, it does force devs (and notably CI) to compile botan from source everytime.
That said, I chose it because 1) rust_crypto and aws_ls_rs are already here, and I didn't just want to duplicate the code 2) ring still seems to be unmaintained 3) openssl you have to compile as well and 4) I'm not aware of other solid options.
I feel it might be best to just duplicate the rust_crypto EdDSA implementation for the example, but then I don't know how useful the example is.
A different way to do it would be to introduce an internal __custom_provider_example = [dep:botan] feature, but that doesn't sound too great either :/

[arckoor]

Seems I forgot to bring this up before, but it would really be great if we could do a breaking change to turn this into Result<Vec<u8>>. Everything else lets you return an error should it happen in your custom provider (e.g. botan can technically error here, because it does go through an FFI, and while it shouldn't error, I still need to call unwrap here)

[Keats]

Can you create an issue for it? We can change that for the next major version?

[DoumanAsh]

I left some comments how to implement it better and simplify provider logic
I think it would be also great to have NOOP provider implementation to be used by default instead of relying on Option to reduce indirection

[arckoor]

@DoumanAsh I can sympathize with most of the comments, and will try them out soon-ish. What I don't sympathize with is returning a default provider when both features are enabled. If there is a default provider, it should be a default feature. Making cryptographic choices for the user that they might not know about is imo a very bad idea.

[DoumanAsh]

@arckoor This is user responsibility to install provider he wants use, if he doesn't want to install any, then user is fine with any. I suggest AWS-LC because it has FIPS mode by default, which makes it most reasonable choice
If user care, he would manually install provider always so I think it is reasonable tradeoff to ensure jsonwebtoken as library is always working and only panic if you do not install any provider or enable no crypto features

P.s. let me know if you need any help to get this PR ready, it would be nice to get jsonwebtoken into working state when both crypto features are enabled

P.s.s Please note that it is my personal opinion and has nothing to with author of the library so defer to Keats's approval to get this PR merged

[Keats]

I agree with @arckoor if several features are enabled in the tree but the user didn't pick. That's a one time choice for the user but for example aws-ls-rc doesn't work with WASM afaik so I would rather not pick the backend library without a clear input in case of conflict.

[DoumanAsh]

@Keats The problem is that feature can be enabled by your dependency, it can be alleviated by having NOOP backend so that library could encourage others not to enable features I guess, but if for some reason you accidentally enable both features, you have no way to diagnose it simply, so that's my concern

[arckoor]

@Keats oh joy, CI fails because of the botan use in the example. I don't know what other crypto lib to use, if you have any preference, please let me know. I can also copy paste code from one of the built in providers, or just remove the example entirely and e.g. point to my jsonwebtoken-botan repo, or something else entirely, whatever you think is best.

[Keats]

Sorry for the delay. Let's point to other backends for examples, no need to maintain one ourselves.

[Keats]

LGTM. Are you ok with that @DoumanAsh ?

[Keats]

Can you create an issue for it? We can change that for the next major version?

[Keats]

why non exhaustive?

[arckoor]

So that when e.g. #461 is merged, providers still compile and throw an unimplemented!() instead of not compiling at all

[DoumanAsh]

Functionally wise it solves issue, but generally I think Arc and Option are adding unnecessary indirection to access provider (I pointed it out in my review but as I'm not really owner I didn't insist on it)
If you think it is ok, then I'm fine with it too (alternative approach is to use 'static reference to provider and store pointer to it, similarly how log crate does with logger)

[Keats]

If you think it is ok, then I'm fine with it too

I really have no opinion on that tbh. @arckoor what do you think?

[arckoor]

Well, I originally wrote it the way I did because rustls did it this way, and my thinking was "It's rustls, they ought to know what they're doing". The get_default() -> Option<... CryptoProvider> is a symptom of me not wanting to add a NOOP_PROVIDER, and I think it should stay that way, either the cryptography works or the lib yells at you.
The Arc is purely a rustls copy, doing it with get_default() -> Option<&'static CryptoProvider> should be totally doable.
So that is completely a case of "you decide @Keats".

[Keats]

So that is completely a case of "you decide @Keats".

😅 I really do not care personally but if Arc is not needed in any way we can remove it

[DoumanAsh]

@arckoor @Keats Here is quick approach to remove Arc and Option
DoumanAsh@5edada7

It moves everything into static variables and relies on OnceLock for initialisation to benefit from its optimisations instead of manually checking Option/install default on first access

P.s. Dynamic creation (i.e. without static) would require leaking Box using https://doc.rust-lang.org/std/boxed/struct.Box.html#method.leak but I'm pretty much doubt anyone would require such case

[arckoor]

Yeah, I see the point with the static, but imo for the cryptography it's important to panic as early as possible when nothing is installed

[DoumanAsh]

I'm not sure why this case would make difference? This optimises default initialisation path, but panic will happen as provider will always initialise on the first usage of any method inside provider. There should be practically no difference aside from overhead of individual interface's method invocation.

[arckoor]

@Keats can be merged from my side