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

Remove dependency on webpki #2267

Closed
wants to merge 10 commits into from
Closed

Conversation

randomairborne
Copy link
Contributor

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.

@github-actions github-actions bot added c-cache Affects the cache crate c-gateway Affects the gateway crate c-gateway-queue Affects the gateway queue crate c-http Affects the http crate c-lavalink Affects the lavalink crate c-model Affects the model crate c-standby Affects the standby crate labels Aug 26, 2023
@github-actions github-actions bot added c-book Affects the book c-mention Affects the mention crate c-validate Affects the validate crate labels Aug 26, 2023
@randomairborne
Copy link
Contributor Author

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

@vilgotf
Copy link
Member

vilgotf commented Aug 26, 2023

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
Copy link
Contributor Author

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
Copy link
Member

vilgotf commented Aug 26, 2023

#2268 is now merged

@vilgotf
Copy link
Member

vilgotf commented Aug 26, 2023

Could you split out the twilight-validated fixes too?

@vilgotf
Copy link
Member

vilgotf commented Aug 26, 2023

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

@randomairborne
Copy link
Contributor Author

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?

@github-actions github-actions bot removed c-book Affects the book c-gateway-queue Affects the gateway queue crate labels Aug 26, 2023
Copy link
Member

@vilgotf vilgotf left a comment

Choose a reason for hiding this comment

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

Forgot to mark this as request changes

Could you split out the twilight-validated fixes too?

@vilgotf
Copy link
Member

vilgotf commented Aug 26, 2023

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

Yes that would be awesome 😊

@Gelbpunkt
Copy link
Member

Gelbpunkt commented Aug 26, 2023

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
Copy link
Member

vilgotf commented Aug 26, 2023

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
Copy link
Member

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
Copy link
Contributor Author

Oops, did not see this before creating #2270

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-cache Affects the cache crate c-gateway Affects the gateway crate c-http Affects the http crate c-lavalink Affects the lavalink crate c-mention Affects the mention crate c-model Affects the model crate c-standby Affects the standby crate c-validate Affects the validate crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants