BiagioFesta / wtransport

Async-friendly WebTransport implementation in Rust
Apache License 2.0
467 stars 31 forks source link

Upgrade to `rustls@0.23` and `quinn@0.11` #200

Closed finnbear closed 3 months ago

finnbear commented 3 months ago

Fixes #121 Fixes #175 Supersedes #194

I'm still new to wtransport, quinn, and rustls; this PR should be scrutinized for errors.

Testing

Changelog entries

- deps: update to quinn 0.11 and disable some default features by @finnbear in #200
- deps: update to rustls 0.23 and disable some default features by @finnbear in #200
- deps: update to rustls-pki-types 1.8 by @finnbear in #200
- Make `SendStream::{reset, stopped}` take a reference instead of ownership by @finnbear in #200
- docs: calling `priority` or `reset` after `reset` results in panic by @finnbear in #200
- Remove `Endpoint::reject_new_connections` by @finnbear in #200
- Add `Endpoint::open_connections` by @finnbear in #200
- Add multiple methods for `IncomingSession` for server, usable prior to awaiting it, to replace `quinn`'s `max_concurrent_connections`, `use_retry`, and `reject_new_connections` by @finnbear
- Add `ConnectionError::CidsExhausted` by @finnbear in #200
- Rename `ConnectingError::TooManyConnections` to `ConnectingError::CidsExhausted` by @finnbear in #200
- Rename `ConnectingError::InvalidDnsName` to `ConnectingError::InvalidServerName` by @finnbear in #200
- Add the number of bytes read to `StreamReadExactError::FinishedEarly` by @finnbear in #200
- docs: `with_custom_tls{_and_transport}` require TLS 1.3 support by @finnbear in #200
BiagioFesta commented 3 months ago

That's an amazing contribution! Thank you. I am going to review this ASAP

finnbear commented 3 months ago

In my first production deployment, I spotted a critical issue where the client faithfully echoes the retry token and connection id generated by quinn-proto but quinn-proto fails to decrypt them:

#1
empty token
address_validated=false
retrying with loc=3a87c8db93b2b725 orig=977888e66e7d3d2a tok=[56, 198, 134, 198, 173, 200, 227, 111, 84, 238, 219, 123, 149, 134, 76, 250, 157, 123, 92, 199, 225, 90, 114, 230, 71, 71, 61, 44, 113, 133, 110, 166, 94, 236, 3, 200, 219, 62, 127, 253, 81, 213, 50, 77, 223, 131, 216, 249, 130, 76, 209, 97]

#2
aead_key.open(...) loc=3a87c8db93b2b725 tok=[56, 198, 134, 198, 173, 200, 227, 111, 84, 238, 219, 123, 149, 134, 76, 250, 157, 123, 92, 199, 225, 90, 114, 230, 71, 71, 61, 44, 113, 133, 110, 166, 94, 236, 3, 200, 219, 62, 127, 253, 81, 213, 50, 77, 223, 131, 216, 249, 130, 76, 209, 97]
unknown token
address_validated=false

I'm going to investigate further, but I advise against deploying from this branch if you use .retry().

BiagioFesta commented 3 months ago

In my first production deployment, I spotted a critical issue where the client faithfully echoes the retry token and connection id generated by quinn-proto but quinn-proto fails to decrypt them:

#1
empty token
address_validated=false
retrying with loc=3a87c8db93b2b725 orig=977888e66e7d3d2a tok=[56, 198, 134, 198, 173, 200, 227, 111, 84, 238, 219, 123, 149, 134, 76, 250, 157, 123, 92, 199, 225, 90, 114, 230, 71, 71, 61, 44, 113, 133, 110, 166, 94, 236, 3, 200, 219, 62, 127, 253, 81, 213, 50, 77, 223, 131, 216, 249, 130, 76, 209, 97]

#2
aead_key.open(...) loc=3a87c8db93b2b725 tok=[56, 198, 134, 198, 173, 200, 227, 111, 84, 238, 219, 123, 149, 134, 76, 250, 157, 123, 92, 199, 225, 90, 114, 230, 71, 71, 61, 44, 113, 133, 110, 166, 94, 236, 3, 200, 219, 62, 127, 253, 81, 213, 50, 77, 223, 131, 216, 249, 130, 76, 209, 97]
unknown token
address_validated=false

I'm going to investigate further, but I advise against deploying from this branch if you use .retry().

Lemme know the easiest way to reproduce; if I find some spare time, maybe I can help investigate on this

finnbear commented 3 months ago

Lemme know the easiest way to reproduce; if I find some spare time, maybe I can help investigate on this

Thanks! My strong suspicion as of 3m ago confirmed root cause is that it's because I reload my wtransport::ServerConfig every minute to pick up new TLS certificates*. Internally, it calls quinn_proto::ServerConfig::with_crypto which generates a new handshake token.

I'm going to change my code to overwrite that new token with the token from the old config!

I don't see a way to make the API to accomplish this easier, but I think endpoint.reload_config could have a warning in its documentation.

Edit: an API change is needed because the old key is a private field and cannot be retrieved. wtransport will need to use quinn_proto::ServerConfig::new instead of quinn_proto::ServerConfig::with_crypto. I will add this to the PR, fix my production deployment*, and then address your other feedback :ok_hand:

*luckily I have a robust websocket fallback when wtransport fails

Here is code users can use to make a handshake token:

fn generate_token_key() -> Arc<dyn HandshakeTokenKey> {
    use rand::RngCore;
    let rng = &mut rand::thread_rng();
    let mut master_key = [0u8; 64];
    rng.fill_bytes(&mut master_key);
    let master_key = ring::hkdf::Salt::new(ring::hkdf::HKDF_SHA256, &[]).extract(&master_key);
    Arc::new(master_key)
}

Edit: I've now updated my production deployment with this fix. I suspect this same issue has existed for months, but I just wasn't looking carefully enough spot it. I've also addressed your feedback :rocket:

BiagioFesta commented 3 months ago

I am wondering if we can add token_key into a dedicated PR. I don't think it is need for this specific PR

finnbear commented 3 months ago

I am wondering if we can add token_key into a dedicated PR. I don't think it is need for this specific PR

Sure, if/when you merge this, I'll make a new PR for that!

finnbear commented 3 months ago

Awesome! I'll submit a token_key PR shortly. Consider waiting before publishing a version if you think you might merge that too.

Edit: https://github.com/BiagioFesta/wtransport/pull/201