Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to rustls@0.23 and quinn@0.11 #200

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

finnbear
Copy link
Contributor

@finnbear finnbear commented Aug 17, 2024

Fixes #121
Fixes #175
Supersedes #194

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

Testing

  • CI
  • ran client & server examples
  • testing in my application (localhost)
  • testing in my application (prod)
production deployments
  • turnfight.com (Aug 18)
  • mazean.com

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

@finnbear finnbear marked this pull request as ready for review August 17, 2024 04:56
@finnbear finnbear marked this pull request as draft August 17, 2024 05:37
@finnbear finnbear marked this pull request as ready for review August 17, 2024 05:50
@finnbear finnbear marked this pull request as draft August 17, 2024 19:28
wtransport/src/stream.rs Outdated Show resolved Hide resolved
@finnbear finnbear marked this pull request as ready for review August 17, 2024 20:14
@finnbear finnbear marked this pull request as draft August 17, 2024 20:21
@finnbear finnbear marked this pull request as ready for review August 17, 2024 21:03
@BiagioFesta
Copy link
Owner

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

wtransport/Cargo.toml Show resolved Hide resolved
wtransport/src/driver/streams/mod.rs Outdated Show resolved Hide resolved
wtransport/src/driver/streams/mod.rs Outdated Show resolved Hide resolved
wtransport/src/driver/streams/mod.rs Outdated Show resolved Hide resolved
wtransport/src/driver/streams/mod.rs Outdated Show resolved Hide resolved
wtransport/src/endpoint.rs Show resolved Hide resolved
wtransport/src/tls.rs Outdated Show resolved Hide resolved
wtransport/src/tls.rs Outdated Show resolved Hide resolved
@finnbear

This comment was marked as resolved.

@BiagioFesta
Copy link
Owner

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

This comment was marked as resolved.

@BiagioFesta
Copy link
Owner

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

Copy link
Owner

@BiagioFesta BiagioFesta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before merge, we need to cleanup the commits history :)

@finnbear finnbear force-pushed the rustls_0_23_quinn_0_11 branch from d88906e to 41add1e Compare August 19, 2024 23:18
@finnbear
Copy link
Contributor Author

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 finnbear requested a review from BiagioFesta August 20, 2024 18:54
wtransport/src/config.rs Outdated Show resolved Hide resolved
wtransport/src/config.rs Outdated Show resolved Hide resolved
@finnbear finnbear force-pushed the rustls_0_23_quinn_0_11 branch from 8427d50 to ef04929 Compare August 20, 2024 20:55
@finnbear finnbear force-pushed the rustls_0_23_quinn_0_11 branch from ef04929 to 80eaf09 Compare August 20, 2024 20:58
@finnbear finnbear requested a review from BiagioFesta August 20, 2024 21:02
Copy link
Owner

@BiagioFesta BiagioFesta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an amazing work. Thank you

@BiagioFesta BiagioFesta merged commit 1026ea3 into BiagioFesta:master Aug 20, 2024
8 checks passed
@finnbear
Copy link
Contributor Author

finnbear commented Aug 20, 2024

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

Edit: #201

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SendStream::stopped requires move Supporting rustls 0.22
2 participants