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

Update to native-tls 0.2 #308

Closed
est31 opened this issue Jun 26, 2018 · 14 comments
Closed

Update to native-tls 0.2 #308

est31 opened this issue Jun 26, 2018 · 14 comments
Labels
E-easy Effort: Easy! Start here :D
Milestone

Comments

@est31
Copy link
Contributor

est31 commented Jun 26, 2018

native-tls 0.2 brings in the openssl update from 0.9 to 0.10.

@FauxFaux
Copy link

This seems to depend on upgrading hyper-tls to 0.3, which Sean released yesterday.

(To save anyone else doing some of the work..)

@seanmonstar
Copy link
Owner

To get this into the 0.8.x branch, the code from tokio-tls and hyper-tls would need to be inlined into reqwest directly, since hyper-tls already expects hyper 0.12. I asked about the changes in native-tls, and it's mostly new features, so it seems best to just make this upgrade along with the hyper 0.12 upgrade, being part of reqwest 0.9.

@seanmonstar seanmonstar added this to the 0.9 milestone Jul 4, 2018
@seanmonstar
Copy link
Owner

The upgrade to hyper 0.12 is done, so the next step here would be to just upgrade to hyper-tls v0.3.0, and then we'd be "done"!

@seanmonstar seanmonstar added the E-easy Effort: Easy! Start here :D label Jul 5, 2018
@FauxFaux
Copy link

FauxFaux commented Jul 6, 2018

This still depends on: tokio-rs/tokio-tls#43 , without it,

tls.connect_async(&host, tunneled)
's use of connect_async doesn't make sense.

Also, I suspect Connector#danger_disable_hostname_verification needs to go.

@seanmonstar
Copy link
Owner

hyper-tls 0.3 dropped the tokio-tls dependency, so we're not blocked. The danger method can customize the TlsConnector directly.

@FauxFaux
Copy link

FauxFaux commented Jul 6, 2018

I have no idea how to proceed. The code in hyper_tls seems to solve this issue by using hyper_tls::TlsStream(new: native_tls::TlsStream), which is pub(crate), so not accessible to us. I'm going to leave this one to the experts.

@seanmonstar
Copy link
Owner

@FauxFaux what issue exactly are you running into? Would you be able to push a diff somewhere?

@sbditto85
Copy link

I have a branch that isn't ready to be merged if someone wants to check it. https://github.com/sbditto85/reqwest/tree/update_native_tls

the reason it isn't ready to be merged is i had to update tokio-tls and I'm not sure I updated it properly as I'm new to the ecosystem.

my updates to tokio-tls are found at https://github.com/sbditto85/tokio-tls/tree/update_native_tls and I basically commented out one of the functions as TlsConnector doesn't have it anymore.

Not sure the best way to proceed.

@seanmonstar
Copy link
Owner

Is there a reason tokio-tls is still needed? I don't think it should be required anymore...

@sbditto85
Copy link

sbditto85 commented Jul 9, 2018

used in src/connect.rs for tls.connect_async(&host, tunneled) on line 68 (the TlsConnectorExt trait)

not sure where else though

@seanmonstar
Copy link
Owner

Oh shoot, you're right, it's need to tunneled proxies. Gah, I'd rather not have a dependency on tokio-tls, since it's lagging behind (still depends on tokio-core) and probably won't get much attention...

Perhaps the connect_async that's used in hyper-tls can be exported publicly?

@sbditto85
Copy link

I hope your not asking me haha ... i have no idea how most of this works. Just figuring it out now :)

@mehcode
Copy link

mehcode commented Jul 9, 2018

Added my take of this at #313 (already did this last week and forgot to send a PR).

Pending tokio-rs/tokio-tls#43

@seanmonstar
Copy link
Owner

This was done in #325!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Effort: Easy! Start here :D
Projects
None yet
Development

No branches or pull requests

5 participants