Use rustls instead of system openssl to make compilation easier

[DaniD3v]

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]

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]

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]

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]

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

[DaniD3v]

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]

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]

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

[lukas0008]

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]

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]

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

[Snowiiii]

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

[DaniD3v]

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]

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

[Snowiiii]

Thank you @DaniD3v 👍