cowprotocol / cowprotocol/services

bug: Two `rustls` crypto providers are linked, so the first TLS handshake panics (in certain cases)

Open
#4,736 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug track:maintenance
Dominant language
Rust
Stars
318
Forks
189
Avg merge
2d 2h
Merged PRs (30d)
121

Description

Summary

Every binary in the workspace compiles rustls with both the ring and aws-lc-rs providers enabled. rustls 0.23 refuses to choose between them, so the first TLS configuration built anywhere in a process panics — unless some earlier code path happened to call CryptoProvider::install_default().

Nothing in the workspace installs a provider at startup. Today we get away with it because the code paths that reach TLS first are ones whose libraries install a provider on our behalf. That is an ordering accident, not a guarantee, and it is already load-bearing for the pAMM state-override stream.

Reproducer

const AMBIGUOUS: &str = "Could not automatically determine the process-level CryptoProvider";

#[test]
fn tls_panics_until_a_crypto_provider_is_installed() {
    // Both providers are linked. Naming each one compiles only because the
    // matching rustls feature is enabled somewhere in the graph, so this is a
    // compile-time assertion as much as a runtime one.
    let ring = rustls::crypto::ring::default_provider();
    let aws_lc_rs = rustls::crypto::aws_lc_rs::default_provider();
    assert!(!ring.cipher_suites.is_empty());
    assert!(!aws_lc_rs.cipher_suites.is_empty());

    // Nothing has chosen between them: no dependency installs a provider at
    // load time, and this binary has opened no connection of its own.
    assert!(
        rustls::crypto::CryptoProvider::get_default().is_none(),
        "a provider was already installed, so this test proves nothing — it must run in a process \
         that has not opened a TLS connection"
    );

    // So the first TLS config built in the process panics. This is the call
    // every rustls client bottoms out in: `tokio_tungstenite::connect_async`
    // reaches it through `tokio-tungstenite/src/tls.rs`, as does any reqwest
    // client, hyper-rustls connector or alloy websocket transport that has not
    // installed a provider first.
    let hook = std::panic::take_hook();
    std::panic::set_hook(Box::new(|_| {}));
    let built = std::panic::catch_unwind(rustls::ClientConfig::builder);
    std::panic::set_hook(hook);

    let Err(panic) = built else {
        panic!("ClientConfig::builder() succeeded, so the providers are no longer ambiguous")
    };
    let message = panic
        .downcast_ref::<String>()
        .map(String::as_str)
        .or_else(|| panic.downcast_ref::<&str>().copied())
        .unwrap_or("<non-string panic payload>");
    assert!(
        message.contains(AMBIGUOUS),
        "panicked for an unrelated reason: {message}"
    );

    // Installing one explicitly is the entire fix, and the same call every
    // affected crate has to make before its first handshake.
    aws_lc_rs
        .install_default()
        .expect("no provider should have been installed yet");
    let _ = rustls::ClientConfig::builder();
}

It asserts, in order, that both providers are linked, that no provider is installed, that rustls::ClientConfig::builder() panics, and that installing one explicitly fixes it.

Where the two providers come from

Provider Enabled via
ring observeasync-nats 0.48 → tokio-rustls/ring, tokio-websockets/ring
aws-lc-rs reqwest__rustls-aws-lc-rs, hyper-rustls/aws-lc-rs, aws-smithy-http-client/rustls-aws-lc

Both reach every binary through feature unification. Confirmed identical for simulator, driver, price-estimation, orderbook and autopilot:

$ cargo tree -p driver -e features | grep -oE 'rustls feature "(ring|aws-lc-rs)"' | sort -u
rustls feature "aws-lc-rs"
rustls feature "ring"

Relevant rustls source — rustls-0.23.41/src/crypto/mod.rs:243-256, from_crate_features() returns None when the features are ambiguous, and the .expect() above panics.

Why it has not bitten us yet

alloy-transport-ws-1.8.3/src/native.rs:138-141 installs aws-lc-rs as it opens a websocket connection:

// Install the default rustls crypto provider if not already set.
// Required since rustls 0.23+ no longer auto-installs a provider.
let _ = rustls::crypto::CryptoProvider::install_default(
    rustls::crypto::aws_lc_rs::default_provider(),
);

The binaries reach that through ethrpc::block_stream::current_block_ws_stream early in startup, which covers everything opened afterwards.

However, I managed to hit it in the process of testing a PR.

The gap

That only happens when the block stream is a websocket. crates/shared/src/current_block.rs:47-58 falls back to HTTP polling when NODE_WS_URL is unset:

match &self.node_ws_url {
    Some(ws_url) => current_block_ws_stream(alloy_provider, ws_url.clone()).await,
    None => current_block_stream(http_url, poll_interval).await,   // no ws, no provider
}

On that path nothing installs a provider. The pAMM state-override stream then becomes the process's first TLS user: it calls tokio_tungstenite::connect_async on a wss:// URL, which reaches ClientConfig::builder() at tokio-tungstenite-0.28.0/src/tls.rs:126 and panics.

The panic surfaces inside a spawned background task, so it kills that task and nothing else. The stream never delivers overrides, overrides_for reports Empty forever, and the driver keeps running with no indication that a feature it was configured for is dead.

Any future crate that opens a TLS connection without going through alloy first inherits the same trap.

Suggested fix

Install a provider once, explicitly, during startup — in observe::init or alongside it, so every binary is covered before anything opens a connection:

let _ = rustls::crypto::aws_lc_rs::default_provider().install_default();

aws-lc-rs matches what alloy already installs, so this changes no behaviour on the paths that work today; it only removes the dependency on ordering. The result is ignored deliberately: install_default returns Err when a provider is already installed, which is the normal case as soon as anything else has opened a connection, and is not a failure.

Worth considering separately: whether ring needs to be in the graph at all. async-nats enables it through default features, so default-features = false plus an explicit feature list on that dependency would remove the ambiguity at the source rather than papering over it.

Final note

Of course I do not think this is a major issue, however I can not pass it by :)

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reading observe::init and crates/shared/src/current_block.rs:47-58, then trace the startup paths used by the binaries. Add the explicit provider installation before any TLS connection can be opened, and verify the HTTP-polling path no longer lets the pAMM state-override stream panic; use the supplied reproducer and cargo tree feature check to confirm both behavior and provider features.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend, networking
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.