Restore support for native-tls

[jsha]

This allows any of: rustls, native-tls, both, or neither. It replaces the stream::Inner enum with a trait, which will hopefully be more pleasant to work with.

Fixes #319

[algesten]

This looks great to me. The Inner solution was what I was stuck on in my attempt.

👍

Thanks for doing this!

[algesten]

Nice!

[dralley]

Is anything blocking this from merging? @jsha @algesten

[jsha]

Mainly just indecision about whether to merge this vs the more general version at #391. Also looks like I need to fix up some tests.

[dontlaugh]

I do see how the feature gating in connect_https is a bit noisy, especially if you were to add another implementation. I have also read through #391 and it seems like a nice solution.

But I still lean towards this approach that doesn't introduce more types (yet).

There is nothing in this PR that precludes us from adding something like #391 later, right?

[algesten]

Thanks @dontlaugh, that is good to hear.

Jacob is driving these changes, and he's currently on vacation, however before he left, the last thing we discussed was that he's leaning towards this solution too.

You weighing in gives us more confidence it is the right way forward. Thanks!

[jsha]

Thinking some more about this in the context of feature unification: https://doc.rust-lang.org/cargo/reference/features.html#feature-unification

A consequence of this is that features should be *additive*. That is,

enabling a feature should not disable functionality, and it should usually be safe to enable any combination of features. This PR fails the test above: the native-tls feature isn't additive. It disables use of rustls. So if you have a big dependency tree, one dependency might be expecting to use rustls and another might be expecting to use native-tls. Due to feature unification, native-tls would be in force for both dependencies. I think the ideal would be for the top-level crate to be able to pick a TLS backend that would be in force for all its dependencies, so we would inly need to build one of rustls or native-tls for any given binary. I don't think it's possible to achieve that with the cargo feature system. A second best way to handle the situation of two deps wanting rustls and native-tls would be: they both get compiled, but only one gets used at runtime. We could do this the way the `log` crate does, with global state. For instance, we would have a global default TlsConnector, which the application could modify under a lock to use whichever backend it prefers. There's also the question of what happens when you're building on a platform that is unsupported by rustls, and one of your deps uses ureq with --no-default-features --features native-tls feature, and another dep uses the default features. I think we should use a platform config gate for the unsupported platforms, and default to native-tls in that case. So, current thinking: implement the platform gate first, since it doesn't commit us to a new customization API, and then figure out jf we want further customization and how it should work with diamond dependencies.

[algesten]

I will write more tomorrow, but I think there's a problem with intention here.

Why would one dep select native-tls instead of relying on defaults?

The only reason I believe would be that it consumes an API on TLS 1.0 or some bad cipher. Ie the function of the lib requires that backwards compat.

Making a global singleton wouldn't solve that, since we effectively could get libraries "fighting" over setting the singleton.

Maybe it would be better if ureq preferred native-tls if it is enabled, but there's an override switch on the agent builder?

[dontlaugh]

It's not just old ciphers. WebPKI, a hard dep of rustls, doesn't support IP SANs. briansmith/webpki#54

Honestly avoiding anything ring/ruslts/webpki makes everyone's life easier.

[algesten]

Honestly avoiding anything ring/ruslts/webpki makes everyone's life easier.

I don't agree, but that's beside the point. I understand there are cases where people can't use rustls, and do want to support that. My comment is more about how we achieve that if we get diamond dependencies on ureq with different sets of feature flags.

[jsha]

Closing in favor of #391