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

Allow for optionally using OpenSSL #136

Closed
svenstaro opened this issue May 4, 2021 · 6 comments
Closed

Allow for optionally using OpenSSL #136

svenstaro opened this issue May 4, 2021 · 6 comments

Comments

@svenstaro
Copy link

Hey, I think it'd be neat if the user could choose between using rustls or OpenSSL. For instance, I do sometimes have the requirement to use either one or the other and if xh supported that it'd be pretty good. I'm not sure whether it'd be a compile-time switch or a runtime one as linking to OpenSSL at all times would decrease the nice portability of xh which I do value.

Perhaps allow the user to add OpenSSL support during compile time? Then distro packagers could choose to ship a xh supporting both versions while you still keep building the static builds with only rustls.

I imagine building this would enable a flag --native-tls which would then internally switch TLS calls to the linked OpenSSL.

Use cases:

  • Sometimes I want it to use my system certificate store instead of the shipped certs.
  • Sometimes rustls introduces weird behavior or performes badly (for instance Download fails with "broken pipe" error #135)
  • Sometimes I want to make sure a given server I poke is compliant in more ways than one.

What do you think?

@blyxxyz
Copy link
Collaborator

blyxxyz commented May 5, 2021

That sounds useful and doable. It's possible to choose between native-tls and rustls at runtime.

  • Do we compile with native-tls by default? On Linux you'd need libssl-dev (or equivalent) installed. I think that random people running cargo install would want it disabled (for convenience) while people packaging it for distros would want it enabled. So maybe disable it by default but add a notice for packagers (that could also point out the man page and completions) or contact them directly.

  • If both backends are available, which one do we use by default? Apparently reqwest can't do ALPN using the native-tls backend, so it's not a straight upgrade even when it's available.

  • Do we ever want to use native-tls-vendored? That uses a statically linked OpenSSL on Linux (and matches native-tls on other platforms).

  • Do we support compiling without rustls? That'd have to remove the --cert flag but it would cut down the binary size by a megabyte or so compared to compiling with both.

@svenstaro
Copy link
Author

That sounds useful and doable. It's possible to choose between native-tls and rustls at runtime.

* Do we compile with native-tls by default? On Linux you'd need libssl-dev (or equivalent) installed. I think that random people running `cargo install` would want it disabled (for convenience) while people packaging it for distros would want it enabled. So maybe disable it by default but add a notice for packagers (that could also point out the man page and completions) or contact them directly.

This is exactly the way I'd recommend doing it.

* If both backends are available, which one do we use by default? Apparently reqwest can't do ALPN using the native-tls backend, so it's not a straight upgrade even when it's available.

Yes, I think rustls should stay default so as to not surprise users.

* Do we ever want to use native-tls-vendored? That uses a statically linked OpenSSL on Linux (and matches native-tls on other platforms).

I think that's probably a worse compromise and should be avoided.

* Do we support compiling without rustls? That'd have to remove the `--cert` flag but it would cut down the binary size by a megabyte or so compared to compiling with both.

I would rather prefer if that weren't an option and I don't think it's worth it. It would also complicate options for users for no large benefit (well, except for a slightly smaller binary).

@ducaale
Copy link
Owner

ducaale commented Jun 9, 2021

If both backends are available, which one do we use by default? Apparently reqwest can't do ALPN using the native-tls backend, so it's not a straight upgrade even when it's available.

It seems native-tls has added support for ALPN a couple of months ago. This PR should enable it for reqwest seanmonstar/reqwest#1283

Do we support compiling without rustls? That'd have to remove the --cert flag but it would cut down the binary size by a megabyte or so compared to compiling with both.

If we are planning to support --cert flag for native-tls users, it means we will be determining which tls backend to use at runtime, correct? In that case, how can we handle cases where a user wants to use the --cert flag with https://{IP}?

Also, do we want to add a --debug flag for determining what flavour the binary has been built with?

@blyxxyz
Copy link
Collaborator

blyxxyz commented Jun 9, 2021

we will be determining which tls backend to use at runtime, correct?

Yes.

In that case, how can we handle cases where a user wants to use the --cert flag with https://{IP}?

If we automatically detect https://{IP} then I think reqwest will give an "incompatible TLS identity type" error on its own. native-tls can't handle PEM and rustls can't handle PKCS12/DER.

But it might also be interesting to support DER files and automatically switch the TLS backend based on that.

We should take care with the error messages here, if we just let it crash they'll be confusing.

Also, do we want to add a --debug flag for determining what flavour the binary has been built with?

That's a good idea.

I already have a proof of concept, I'll make a WIP PR.

@blyxxyz blyxxyz mentioned this issue Jun 9, 2021
10 tasks
@ducaale
Copy link
Owner

ducaale commented Jun 11, 2021

Not super related, but it would be good if our tests don't depend on OpenSSL unless native-tls is enabled. This should be possible once alexcrichton/curl-rust#374 gets merged.

@ducaale
Copy link
Owner

ducaale commented Aug 5, 2021

xh v0.12.0 has been released which supports using the system's TLS library 🎉

# make sure to enable the newly added native-tls feature when compiling xh i.e cargo build --features=native-tls
$ xh --native-tls https://1.1.1.1
HTTP/2.0 200 OK
age: 270
cache-control: public, max-age=14400
cf-cache-status: HIT
cf-ray: 67a3e3c28af3fe50-HEL
content-type: text/html
date: Thu, 05 Aug 2021 23:43:47 GMT
expect-ct: max-age=604800, report-uri="https://report-uri.cloudflare.com/cdn-cgi/beacon/expect-ct"
expires: Fri, 06 Aug 2021 03:43:47 GMT
last-modified: Tue, 03 Aug 2021 14:31:58 GMT
served-in-seconds: 0.003
server: cloudflare
strict-transport-security: max-age=31536000
vary: Accept-Encoding
x-amz-request-id: tx0000000000000083a103c-00610953bb-5653bdb-default

# response body is omitted

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

No branches or pull requests

3 participants