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 rustls features #100

Closed
wants to merge 2 commits into from
Closed

Add rustls features #100

wants to merge 2 commits into from

Conversation

enfipy
Copy link

@enfipy enfipy commented Jun 15, 2020

Better to add features to turn on rustls in reqwest.

Library native-tls is dynamically linked and it's hard sometimes to set up the environment for it. I added features that enable rustls in reqwest but by default reqwest uses native-tls in feature default-tls and needs to be disabled to make work rustls.

So to use rustls with oauth2 all you need to specify:

oauth2 = { version = "3.0", features = ["reqwest-010", "reqwest-010-rustls"], default-features = false }

@mettke
Copy link
Contributor

mettke commented Jun 16, 2020

Thanks for the PR. However a similar one #92 was rejected recently to not further complicate the feature flags in this crate.

To get rid of the openssl dependency it is possible to specify your own HttpClient using HttpRequest -> Result<HttpResponse, RE> and disabling default-features = false.

@enfipy
Copy link
Author

enfipy commented Jun 16, 2020

Thanks for the PR. However a similar one #92 was rejected recently to not further complicate the feature flags in this crate.

To get rid of the openssl dependency it is possible to specify your own HttpClient using HttpRequest -> Result<HttpResponse, RE> and disabling default-features = false.

Well, it's very unclear and more code to write. Because you need to specify another 2 libraries like http and reqwest. I think better to provide features like it done in your PR.

@ramosbugs
Copy link
Owner

my main objection to #92 was the feature flag complexity that it introduced (since it had to work with both reqwest 0.9 and 0.10). however, I think we have an opportunity to revisit this now that we've moved on to a new major release (4.x), which is a good time to simplify things by removing legacy support (as @mettke alluded to in #99).

for 4.x, it seems reasonable to remove support for both futures 0.1 and reqwest 0.9 and leave only (optional) futures 0.3 and reqwest 0.10 support. as part of this change, how about we change the reqwest 0.10 import to unconditionally use rustls-tls instead of default-tls? I think it makes sense to drop support for default-tls given that:

  1. native_tls causes frequent build issues (I've seen many coworkers waste time dealing with OpenSSL linking/versioning issues)
  2. many Linux LTS releases (e.g., Ubuntu 14.04 and 16.04) don't have OpenSSL package versions newer than 1.0.2 available, which runs into both security issues and known certificate verification issues [1] [2]

so, at a minimum, we probably want to change the default to rustls-tls, but using default-tls on purpose at this point seems so ill-advised that we can just ask people to supply their own HttpRequest -> HttpResponse implementation if they truly want it (in effect, inverting the current burden this crate imposes on rusttls-tls users). most users probably won't care and won't even notice the change in the underlying TLS implementation.

@enfipy
Copy link
Author

enfipy commented Jun 16, 2020

my main objection to #92 was the feature flag complexity that it introduced (since it had to work with both reqwest 0.9 and 0.10). however, I think we have an opportunity to revisit this now that we've moved on to a new major release (4.x), which is a good time to simplify things by removing legacy support (as @mettke alluded to in #99).

for 4.x, it seems reasonable to remove support for both futures 0.1 and reqwest 0.9 and leave only (optional) futures 0.3 and reqwest 0.10 support. as part of this change, how about we change the reqwest 0.10 import to unconditionally use rustls-tls instead of default-tls? I think it makes sense to drop support for default-tls given that:

  1. native_tls causes frequent build issues (I've seen many coworkers waste time dealing with OpenSSL linking/versioning issues)
  2. many Linux LTS releases (e.g., Ubuntu 14.04 and 16.04) don't have OpenSSL package versions newer than 1.0.2 available, which runs into both security issues and known certificate verification issues [1] [2]

so, at a minimum, we probably want to change the default to rustls-tls, but using default-tls on purpose at this point seems so ill-advised that we can just ask people to supply their own HttpRequest -> HttpResponse implementation if they truly want it (in effect, inverting the current burden this crate imposes on rusttls-tls users). most users probably won't care and won't even notice the change in the underlying TLS implementation.

So I can close this PR and you will add it in new v4, right?

@ramosbugs ramosbugs mentioned this pull request Jun 16, 2020
8 tasks
@ramosbugs
Copy link
Owner

So I can close this PR and you will add it in new v4, right?

I'm open to PRs that implement the changes I suggested, but I can't guarantee when I'll have the time to make these changes myself.

@mettke
Copy link
Contributor

mettke commented Jun 17, 2020

@enfipy Yes feel free to close it. I will add this to our roadmap in #99

@enfipy enfipy closed this Jun 17, 2020
@enfipy enfipy deleted the patch-1 branch June 17, 2020 13:11
@enfipy enfipy restored the patch-1 branch June 17, 2020 13:11
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

3 participants