search

twilight-rs / twilight

Powerful, flexible, and scalable ecosystem of Rust libraries for the Discord API.
https://discord.gg/twilight-rs
ISC License
670 stars 131 forks source link

Remove dependency on `webpki` #2267

Closed randomairborne closed 1 year ago

randomairborne commented 1 year ago

webpki has recently had an issue with a GitHub advisory being published. While this advisory doesn't really apply to twilight, it's still annoying, so I've updated hyper-rustls and tokio-tungstenite to versions that depend on rustls-webpki, which is an actively-maintained version of webpki. I also fixed some clippy warnings.

randomairborne commented 1 year ago

This enables Nagle's algorithm, which after a little reading might not be appropriate for Twilight

vilgotf commented 1 year ago

I did not see this prior to opening #2268. I'd like to keep each PR focused on one thing, so if you don't mind I'll go ahead and merge that PR before looking at this.

randomairborne commented 1 year ago

Yeah, I'll gladly resolve any conflicts with the base branch (or even remake the PR, it's not that complex)- I just knew that clippy pass is required for merging.

vilgotf commented 1 year ago

2268 is now merged

vilgotf commented 1 year ago

Could you split out the twilight-validated fixes too?

vilgotf commented 1 year ago

CC @Gelbpunkt as this is essentially a backport of https://github.com/twilight-rs/twilight/pull/2201

randomairborne commented 1 year ago

Sure, want me to make a separate PR for the validate stuff? Also, that PR only updates next to 0.19, which does NOT contain the vulnerability fix. In addition, we determined in the twilight discord that this is probably breaking, so should I take the main content of this to next?

vilgotf commented 1 year ago

Sure, want me to make a separate PR for the validate stuff?

Yes that would be awesome 😊

Gelbpunkt commented 1 year ago

CC @Gelbpunkt as this is essentially a backport of #2201

I'm strictly against merging this into main. We should never be updating rustls in a patch release, hence this should only ever be done in next. The reason for that is that rustls is an essential crate that shouldn't be duplicated in the dependency tree. Even if there are no public API changes and we can update it hidden away from the users, they are highly likely already pulling in rustls in their own dependencies, likely transitively through crates like hyper-rustls. We should do the same as hyper-rustls, tokio-tungstenite and many other crates that depend on rustls: Make users opt into the new version by releasing a new minor release. Having two different versions in the tree can increase binary size a fair bit and is bad practice, especially with core ecosystem crates like rustls, hyper, etc.

vilgotf commented 1 year ago

But if the ecosystem's already moved on to the new rustls version, wouldn't Twilight be the reason for downstream users rustls version being duplicated? next won't release any time soon.

Gelbpunkt commented 1 year ago

But if the ecosystem's already moved on to the new rustls version, wouldn't Twilight be the reason for downstream users rustls version being duplicated? next won't release any time soon.

I do not think that our slow(er) minor release cadence is justification for breaking with ecosystem semver standards and our own past dependency update policy.

randomairborne commented 1 year ago

Oops, did not see this before creating #2270