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

Use rustls instead of system openssl to make compilation easier #43

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

DaniD3v
Copy link
Contributor

@DaniD3v DaniD3v commented Aug 20, 2024

On nixOS installing pkg-config and the system openssl is a bit of a pain so why don't we use rustls instead?
I imagine compiling for Windows isn't fun either but not too sure how their system crypto library works.

@Luk-ESC
Copy link
Contributor

Luk-ESC commented Aug 20, 2024

Note that this change also comes with performance implications (both speedups and slowdowns): https://github.com/aochagavia/rustls-bench-results
Not sure if these are very relevant though.

@Snowiiii
Copy link
Owner

On nixOS installing pkg-config and the system openssl is a bit of a pain so why don't we use rustls instead? I imagine compiling for Windows isn't fun either but not too sure how their system crypto library works.

Hey, Im not fimilar with nixOS. Why OpenSSL is a pain to install and rusttls is easier?. I also sometimes develop pumpkin on Windows and it compiles there without any problems

@DaniD3v
Copy link
Contributor Author

DaniD3v commented Aug 21, 2024

Hey, Im not fimilar with nixOS. Why OpenSSL is a pain to install and rusttls is easier?. I also sometimes develop pumpkin on Windows and it compiles there without any problems

That's not how NixOS actually works but just imagine that every installed application is a container. There aren't any globally installed libraries and you basically need to package everything that uses system dependencies. If a project uses 100% rust libraries this procedure can be avoided. Openssl is currently the only system library we're using atm (apart from libc).

@Snowiiii
Copy link
Owner

Snowiiii commented Aug 21, 2024

Ah okay i understand. So what are the disatvantages are when using rusttls?. Does it may break something?

@DaniD3v
Copy link
Contributor Author

DaniD3v commented Aug 21, 2024

Here's a discussion about making it the upstream reqwests default seanmonstar/reqwest#2025

It looks like native-tls (openssl) doesn't support TLS 1.3 which seems likely to affect performance and security and it uses OpenSSL on Linux which has a pretty spotty security history. rustls doesn't support TLS 1.1 (and earlier) which are officially deprecated anyway but which I guess could come up but should be increasingly rare. Also at this stage I think rustls is in a really good situation in terms of maintenance with two full-time maintainers so it's likely that issues (that are not by design) can fixed very quickly.

(re-worded for clarity)

It shouldn't break anything if Mojang doesn't depend on using only deprecated TLS 1.1. In my test it worked fine.

@Snowiiii
Copy link
Owner

Snowiiii commented Aug 21, 2024

Okay, I just got home and looked at rustls. I like the idea of having a modern TLS library written in rust instead of relying on System C Libaries. But like this comment https://users.rust-lang.org/t/any-reasons-to-prefer-native-tls-over-rustls/37626/2 mentioned it, there will may be security vulnerabilities that can't be patched via a dependency update. Sure we can update the library a vulnerability is found. But there may be users who rely on an older version of Pumpkin

@Erb3
Copy link
Contributor

Erb3 commented Aug 21, 2024

I don't think rustls vulnerabilities should be a concern, we aren't even transmitting anything sensitive over HTTPS (afaik)!

@lukas0008
Copy link
Collaborator

This is useful for some situations, like being able to use an empty docker base image for running the server, but this is also something that should not be enabled by default (due to the security point mentioned by @Snowiiii).

Maybe using a feature flag to enable this would be better?

@DaniD3v
Copy link
Contributor Author

DaniD3v commented Aug 22, 2024

Ok, I'll make it a feature for now but consider that there's talk about making this the upstream default anyways, so I really don't think that security should be a concern here (a minecraft server) when banking clients might use the reqwests library with rustls (as a default) in the future.

Also looking at the list of recent openssl vulnerabilities most of them would not be possible with rusts safety measures.

@DaniD3v
Copy link
Contributor Author

DaniD3v commented Aug 22, 2024

Actually making it a feature doesn't really work out.

I need to remove the native-tls feature from reqwests in order to actually make it easier to compile Pumpkin, but rust features currently can't do that. See https://users.rust-lang.org/t/is-possible-to-disable-default-features-with-features/46870/2

Note that rustls actually has more users than openssl
image
image

@Snowiiii
Copy link
Owner

Do we have to use the new http2 feature ? @DaniD3v

@DaniD3v
Copy link
Contributor Author

DaniD3v commented Aug 22, 2024

http2 was already enabled because it's a default feature of reqwests.
By disabling default features it got disabled as well so in order to keep changes to a minimum I decided to keep it.

charset and macos-system-configuration are actually default features that were disabled as well. Idk about macos-system-configuration but I'm pretty sure charset just adds some methods and doesn't actually change the behavior of reqwests otherwise.

@Snowiiii
Copy link
Owner

Looks like macos-system-configuration is for macOS proxy stuff. I think we should add it

@Snowiiii
Copy link
Owner

Snowiiii commented Aug 22, 2024

Thank you @DaniD3v 👍

@Snowiiii Snowiiii merged commit 290e0ed into Snowiiii:master Aug 22, 2024
5 checks passed
@Snowiiii Snowiiii mentioned this pull request Oct 14, 2024
1 task
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.

5 participants