Don't store the placeholder provider in the process OnceLock

[SebTardif]

Reduced after review. The earlier version of this PR converted the missing-provider panic into ErrorKind::MissingCryptoProvider. @arckoor pushed back: a missing provider is a configuration error, not something a caller can act on. I agree. That change is gone. The panic stays.

What's left is a bug that is independent of the panic-vs-error question.

The bug

get_default was:

PROCESS_DEFAULT_PROVIDER.get_or_init(CryptoProvider::from_crate_features)

from_crate_features returns a dummy CryptoProvider whose factory fns panic. It does not panic itself. So the dummy is stored in the OnceLock first, and the panic only fires later, when a factory fn is called. install_default is OnceLock::set, so from that point it returns Err forever.

This bites wherever unwinds are caught and the process keeps running: tokio::spawn returns Err(JoinError), tower-http's CatchPanicLayer turns it into a 500, actix-web restarts the worker thread, libtest catches it per test. In all of those the panic message's own instruction ("Call CryptoProvider::install_default() before this point") has become impossible to follow.

tests/missing_provider.rs catches the panic and asserts install_default() still succeeds. It fails on master and passes here.

The fix

rustls, which this module is modelled on, has the same two functions and never stores a sentinel (0.23.35, crypto/mod.rs:238-257): from_crate_features returns Option<Self>, and get_default_or_install_from_crate_features calls .expect() on it, so the panic fires before anything reaches the lock. We inherited rustls's panic message almost verbatim but replaced the Option with a stored dummy. This restores the upstream shape.

• Drop the static INSTANCE dummy; from_crate_features returns Option.
• The internal accessor panics without writing to the OnceLock. The panic, its message, and its signature are unchanged. No call sites change, and anyone with exactly one backend feature enabled is unaffected, since that path never touched the dummy.
• The panic text stays the original message for both "both backends enabled" and "neither enabled".
• Add pub fn CryptoProvider::try_get_default() -> Option<&'static CryptoProvider> for a startup assertion. There is currently no way to ask "is a provider installed?" without calling install_default and consuming the slot to find out. rustls spells this one get_default; I used try_get_default to avoid renaming the existing internal accessor and churning every call site.

Rebased onto current master (includes 11.1.0).

Validation

Config	Result
cargo test --features aws_lc_rs	pass
cargo test --features rust_crypto	pass
cargo test --no-default-features --features use_pem --test missing_provider	pass (fails on master)
cargo clippy --all-targets -- -D warnings, all three configs	clean
cargo fmt --check	clean

Split out of this PR

• KeyUtils::new_unimplemented returning Err instead of panicking is Return an error from unimplemented JWK key utils #541. With a custom provider that skips JWKs, whether you reach that path depends on the key the caller passes at runtime, not on Cargo.toml, which reads like the same line drawn in Don't panic when computing thumbprint #524.
• The README/RUSTSEC-2023-0071 note is unrelated, and rsa crate dependency has security vulnerability #463 already covers it.

Separately: cargo test --features aws_lc_rs,rust_crypto fails on master today (4 jwk::tests) because the dummy's KeyUtils panics. Identical before and after this change, so not addressed here.

Ref #456

[arckoor]

I don't think we should make cryptographic choices for people. This also breaks down when someone installs jsonwebtoken with all features, and later on adds something like jsonwebtoken-openssl, intending to use that. main would have long since panicked, with this it will always (silently!) use aws-lc-rs.

[SebTardif]

@arckoor

I don't think we should make cryptographic choices for people. This also breaks down when someone installs jsonwebtoken with all features, and later on adds something like jsonwebtoken-openssl, intending to use that.

Agreed. The latest commit drops the both-features auto-pick. Both backends, or neither, now return MissingCryptoProvider until install_default runs. --all-features plus a later openssl install is no longer locked onto aws-lc.

The neither-feature panic-to-error change is unchanged.

[arckoor]

I don't quite see the point of this though, in general using Err() provides a way for an application to deal with an error. But this error here is just not recoverable, no matter how many Err()s you wrap around it. Is there a good reason I fail to see why a panic is not the correct choice here? This can only be fixed by changing your Cargo.toml or writing another line of code after all

[SebTardif]

@arckoor

But this error here is just not recoverable, no matter how many Err()s you wrap around it. Is there a good reason I fail to see why a panic is not the correct choice here?

No. encode/decode already return Result, and a miss should not consume the install slot.

The previous dummy OnceLock made the first Err unrecoverable. This tip leaves the lock empty, so install_default still works after a failed encode. rustls panics because TLS is process startup; a JWT call is mid-request.

A test without either feature now checks that install_default succeeds after MissingCryptoProvider.

[arckoor]

But does someone really write code like this?

let token = match jsonwebtoken::encode(&header, &claims, &encoding_key) {
    Ok(t) => t,
    Err(e) => match e.kind() {
        jsonwebtoken::errors::ErrorKind::MissingCryptoProvider => {
            some_provider::install_default().unwrap();
            jsonwebtoken::encode(&header, &claims, &encoding_key)?
        }
        _ => todo!(), // something else failed
    },
};

I understand that the encoding failure immediately kills the program, as that was the point. I don't understand how / why someone would write code to deal with a failure of this kind, when the simple solution is just to initialise the provider beforehand.

some_provider::install_default().unwrap();
let token = jsonwebtoken::encode(&header, &claims, &encoding_key)?; // if an error, a different much more helpful one

[SebTardif]

You're right. A missing provider is a configuration error, there's no sensible code to write at the call site, and wrapping it in Err just moves the crash. I've dropped MissingCryptoProvider and kept the panic.

There's a separate bug underneath that I buried in the API argument, and it stands whether or not we keep the panic.

get_default was PROCESS_DEFAULT_PROVIDER.get_or_init(CryptoProvider::from_crate_features). from_crate_features returns &INSTANCE — the dummy — without panicking; the panic lives in the factory fns and only fires later, when one is called. So the dummy lands in the OnceLock first, and since install_default is OnceLock::set, it returns Err from then on.

rustls, which this module is modelled on, has the same two functions and never stores a sentinel:

// rustls 0.23.35, crypto/mod.rs:238-257
pub(crate) fn get_default_or_install_from_crate_features() -> &'static Arc<Self> {
    if let Some(provider) = Self::get_default() {
        return provider;
    }
    let provider = Self::from_crate_features()
        .expect(r###"
Could not automatically determine the process-level CryptoProvider from Rustls crate features.
..."###);
    let _ = provider.install_default();
    Self::get_default().unwrap()
}

from_crate_features() returns Option<Self> there, and .expect() panics before anything reaches the OnceLock. We inherited that panic message almost verbatim but replaced the Option with a stored dummy — that's the one place this diverged from upstream, and it's what eats the install slot.

It matters anywhere unwinds get caught: tokio::spawn returns Err(JoinError) and keeps the runtime alive, tower-http's CatchPanicLayer turns it into a 500, actix-web restarts the worker thread, libtest catches it per test. In all of those the process is still running with the slot consumed — and the panic message's own instruction, "Call CryptoProvider::install_default() before this point", is no longer possible to follow.

I've pushed the reduced version, so the diff is the real thing rather than a description. It's now one source file:

1. Drop the static INSTANCE, have from_crate_features return Option, panic in the accessor without writing to the OnceLock — the rustls shape above. Same panic, same message, same signature, no new error kind, no call sites touched, and no behaviour change at all for anyone with exactly one backend feature enabled, since that path never reached the dummy.

   Plus a cfg on the message so it names which case you're in — "both aws_lc_rs and rust_crypto are enabled" vs "neither is enabled". Today both produce the same text, and the both-enabled case usually arrives via feature unification, where it's least obvious.

2. Optional, and I won't push on it: pub fn CryptoProvider::try_get_default() -> Option<&'static CryptoProvider>. rustls has this public and non-panicking alongside its pub(crate) panicking accessor; we have only the panicking one. It's for a startup assertion — right now there's no way to ask "is a provider installed?" without calling install_default and consuming the slot to find out. I used try_get_default rather than rustls's get_default purely to avoid renaming the internal accessor and churning every call site. Happy to drop it or split it out if you'd rather keep the surface small, and it's @Keats's call on public API either way.

tests/missing_provider.rs catches the panic and asserts install_default() still works afterwards — fails on master, passes here. It needs the --no-default-features config, which isn't built anywhere today, hence the one CI step. Drop both if you'd rather not add a CI job for it.

I'll move KeyUtils::new_unimplemented returning Err to its own PR. I'd like to make the case for it separately: with a custom provider that skips JWKs, whether you reach that path depends on the key the caller passes at runtime, not on Cargo.toml. I read that as the same line you drew in #524 — tell me if I'm reading it wrong. The README/RUSTSEC change is unrelated and I've dropped it; #463 already covers it.

One thing I noticed while testing and am not touching here: cargo test --features aws_lc_rs,rust_crypto fails on master today (4 jwk::tests), same cause — the dummy's KeyUtils panics. Identical before and after this change.

[SebTardif]

Split out as #541 (draft, stacked on this one — only the second commit belongs to it). Keeping it in draft until this PR is resolved, since on master it would change the no-provider path back to an error, which is exactly what we agreed against here.

[arckoor]

Sorry for the delay, also needs a rebase to latest master please

[arckoor]

As a side note, I don't greatly care for corporate advertising in the contributor list, so I personally think the Co-Authored-By should be dropped

[SebTardif]

Dropped the Co-Authored-By trailer from the commit, and the session URL that was next to it.

[arckoor]

@SebTardif Are you going to address the rest of the review and the conflict too? :)

[SebTardif]

@arckoor

Are you going to address the rest of the review and the conflict too?

Yes. Rebased onto current master, and the three review notes are in the latest commit.

[arckoor]

Thanks!