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

Add --native-tls flag #154

Merged
merged 15 commits into from
Aug 5, 2021
Merged

Add --native-tls flag #154

merged 15 commits into from
Aug 5, 2021

Conversation

blyxxyz
Copy link
Collaborator

@blyxxyz blyxxyz commented Jun 9, 2021

  • Enable feature by default on macOS and Windows (?)
    • Test to make sure releases still work
  • Support DER certificates
  • Add tests
    • Run native-tls tests in CI on platforms where that doesn't give issues
  • Add --debug flag
  • Automatically switch the backend in certain cases?
    • Do we offer a way to still force rustls? A --rustls flag?
  • Support for compiling without rustls, if the --cert flag is already partially conditional?
    • Support for compiling without TLS support?
  • Add warning for (or fix) Client seems to ignore additional root certificates with default_tls/native_tls seanmonstar/reqwest#1260
    • Check for similar issues

See #136

@blyxxyz blyxxyz marked this pull request as draft June 9, 2021 10:43
@ducaale
Copy link
Owner

ducaale commented Jun 11, 2021

Enable feature by default on macOS and Windows (?)

Is this because native-tls doesn't use OpenSSL in these platforms? my only worry is that it could become harder to document when native-tls is used vs rustls. Or maybe this is all an implementation detail and users shouldn't worry about which tls backend is being used behind the scenes?

By the way, is there a way to mark these tls flags as, for the lack of a better word, "unstable"? because reqwest could hypothetically decide to deprecate one of the backends.

@blyxxyz
Copy link
Collaborator Author

blyxxyz commented Jun 11, 2021

I meant the cargo feature, not the runtime feature. It'd still use rustls by default, but it'd also compile with native-tls support since (AFAIK) it's stable enough to not hurt portability.

reqwest could hypothetically decide to deprecate one of the backends

In principle they can do anything, but is there a reason to expect that?

@ducaale
Copy link
Owner

ducaale commented Jun 11, 2021

I meant the cargo feature, not the runtime feature. It'd still use rustls by default, but it'd also compile with native-tls support since (AFAIK) it's stable enough to not hurt portability.

Ah, so you mean something like this?

[target.'cfg(unix)'.dependencies.reqwest]
version = "0.11.1"
default-features = false
features = ["rustls-tls"]

[target.'cfg(any(windows, macos))'.dependencies.reqwest]
version = "0.11.1"
default-features = false
features = ["native-tls", "rustls-tls"]

# How can we later determine if native-tls is enabled 🤔?
# Edit: or maybe we can just check the current target

In principle they can do anything, but is there a reason to expect that?

It was a hypothetical scenario. I don't have any reason to expect it.

@blyxxyz
Copy link
Collaborator Author

blyxxyz commented Jun 13, 2021

I hoped to enable xh's own native-tls feature by default on those platforms, but it seems there's no way to do that. I think the best option is to enable it explicitly for our releases.

@blyxxyz
Copy link
Collaborator Author

blyxxyz commented Jun 16, 2021

I can't find a good way to get the library versions for --debug. The only solution I've seen is to parse Cargo.lock, but that's by default ignored when running cargo install so it would be inaccurate.

I'll have to think a bit about what to include.

Copy link
Owner

@ducaale ducaale left a comment

Choose a reason for hiding this comment

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

Should we update this line in the README.md?

+ * rustls is used instead of the system's TLS library by default.
- * rustls is used instead of the system's TLS library.

Cargo.toml Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/request_items.rs Outdated Show resolved Hide resolved
src/utils.rs Outdated Show resolved Hide resolved
src/cli.rs Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
@blyxxyz blyxxyz marked this pull request as ready for review August 5, 2021 19:22
@blyxxyz
Copy link
Collaborator Author

blyxxyz commented Aug 5, 2021

I can't find anyone asking for DER support in HTTPie, so let's not bother. (I also don't have a good understanding of it.)

I think it would make sense for the --debug flag to enable logging, and at that point it deserves its own PR, so I'll leave that out.

Releasing still works, with the usual caveat that I can't run the Windows and macOS binaries.

@blyxxyz blyxxyz requested a review from ducaale August 5, 2021 20:06
Copy link
Owner

@ducaale ducaale left a comment

Choose a reason for hiding this comment

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

Approved 🎉

@ducaale ducaale merged commit 1f43a5b into ducaale:develop Aug 5, 2021
@blyxxyz blyxxyz deleted the native-tls branch August 5, 2021 22:27
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.

None yet

2 participants