Use native-tls on platforms that need it, and remove "tls" feature #391

jsha · 2021-05-28T04:19:46Z

Per #319, we want to add back native-tls support. However, as discussed in #319 (comment), using Cargo features to select a TLS backend (as we did in 1.x) is a bad fit, due to diamond dependencies.

Instead, ureq takes an opinionated stance and allows runtime configuration to change it. On all platforms that support rustls, we use rustls. On other platforms, we use native-tls. In either case, you can override the default: supply your own TLS implementation by calling AgentBuilder::tls_connector. Since this is configured per-Agent, there's no risk that crate A can accidentally change the backend used by crate B.

We now have a couple of new traits, TlsConnector and HttpsStream, that abstract over TLS configuration and encrypted streams. There are impls of TlsConnector for both rustls::ClientConfig and native_tls::TlsConnector. To enable the native_tls impl on platforms that default to rustls, build with the native-tls-adapter feature (and call AgentBuilder::tls_connector).

To mange the complexity introduced by having platform-based configs, this removes the "tls" feature. TLS is now always enabled, on all platforms. For applications that want to omit TLS support for binary-size reasons, we may in the future offer a linker-friendly AgentBuilder method that disables TLS. That should allow the linker to remove the unused TLS-related symbols and build time.

dontlaugh

Two teaspoons of bikeshed paint.

src/agent.rs

src/stream.rs

jsha · 2021-10-02T17:35:14Z

Alright, this mostly reflects my latest thinking in #319:

Rustls stays the default.
Dependencies can't change that default.
If you want to use native-tls (or another backend, like mbedtls), you need to configure that on an Agent.
If a library crate wants to let you configure alternate backends, it needs to let you pass in an Agent (we should document that).
You can remove the rustls dependency entirely (in favor of native-tls or some other backend) with --no-default-features.

jsha · 2021-10-10T03:23:13Z

This is now complete and ready for a review. I think the result is pretty good! I've rewritten the PR description to reflect the latest state.

algesten

Hey this looks excellent. Very nice improvement on the inner workings of connections!

Are we sure about this removal of the tls feature? In some respects the (re-)introduction of native-tls is a pragmatic decision to help users in outdated/odd setups. Compiling without any TLS support is in similar territory – an odd thing to want, but is it right that we say it shouldn't be possible?

Saving compile time is another reason to keep deps slim. If you absolutely don't need TLS, it might be a bit irritating to see those crates compile…

I leave this as "approved", but would like to settle the discussion :)

src/response.rs

src/stream.rs

algesten · 2021-10-10T12:21:00Z

.github/workflows/test.yml

@@ -39,21 +39,19 @@ jobs:
        with:
          command: doc
          # Keep in sync with Cargo.toml's [package.metadata.docs.rs]
-          args: --no-default-features --no-deps --features "tls json charset cookies socks-proxy"
+          args: --no-default-features --no-deps --features "native-tls-adapter json charset cookies socks-proxy"


For brevity and discoverability, I think I prefer this to be native-tls

I chose a different name because I thought people coming from 1.x might see a native-tls feature and assume it does the same thing it did in 1.x: turn on native-tls by default globally.

Cargo.toml

README.md

src/agent.rs

jsha · 2021-10-10T16:52:08Z

Compiling without any TLS support is in similar territory – an odd thing to want, but is it right that we say it shouldn't be possible?
Saving compile time is another reason to keep deps slim. If you absolutely don't need TLS, it might be a bit irritating to see those crates compile…

I agree it's good to keep compile times fast, but maintaining the tls feature has a significant maintenance burden, especially now that it will be interacting with per-platform conditional compilation and the native-tls-adapter feature (which I'm happy to rename native-tls). When we dropping native-tls support in the 1.x -> 2.x migration, it was mainly because of maintenance burden - but now I'm thinking that a lot of that burden was due to the tls feature itself.

As a way to quantify the maintenance burden, check out the diffs to test.sh and test.yml. Previously we had to test [tls, no-tls] x [every feature]. Now we just have to test [every feature]. Of course the tests are automated, but each combination in that matrix is a potential breakage when modifying code that the authors (us) have to fix.

And that maintenance burden is laid out against a fairly small benefit: keeping compile times fast for the small number of users that might explicitly disable TLS. I would be interested in hearing from such users if this turns out to cause significant burden for them, though.

ThomasLamprecht · 2021-10-11T08:01:43Z

From reading the changes to Cargo.toml it seems to me that I cannot build ureq without rustls on some platforms like x86/am64?

I'm asking because we'd like to use ureq for some parts but need native TLS support (as we depend on OpenSSL for quite some other crypto stuff anyway) and while we do not have anything against rustls (great stuff) we'd like to avoid developers to mix usage of TLS implementations. And, FWIW we also have quite some concern about build time and a major refactoring/restructuring effort going on to improve on it, so being able to pull in as few crates as possible would be really nice for us.

So, if I read the change correctly: do you plan to make ureq also use either native/rustls exclusively or would that be too big of a maintenance burden?

algesten · 2021-10-11T11:19:30Z

Hi @ThomasLamprecht thanks for weighing in!

This is exactly the kind of viewpoint we want to hear. I say the decision here is still up in the air.

algesten · 2021-10-11T12:01:10Z

@jsha I pushed a commit reintroducing tls and native-tls as independent options.

I think the rustls arch-selection is elegant, but I think Cargo is lacking what we need to take it all the way (rust-lang/cargo#1197).

I wanted to see this maintenance burden for myself, so I decided to give it a go and I think you have already solved 99% of the problems we used to have in ureq 1.0 by your refactoring. Specifically:

The recognition of https is not feature specific. I.e. internally we always have a concept of "https stream" regardless of feature set.
The crate::default_tls_config() you made on the top, isolates the default impl very neatly.

jsha · 2021-10-11T17:23:43Z

Thanks for the input @ThomasLamprecht. That's useful to know, and a good reason to preserve the ability to build without one or the other. I might phrase it as "enforce at build time that only one TLS implementation is available to the program." Note that you are somewhat vulnerable to your dependencies here: if a dependency wants rustls, you'll still get rustls compiled in.

@algesten said:

I think the rustls arch-selection is elegant, but I think Cargo is lacking what we need to take it all the way (rust-lang/cargo#1197).

To clarify: you think we should not do the automatic selection of backend based on architecture? That's a bummer, but makes sense - it was fairly awkward to do, listing out the whole list of architectures multiple times. You left some vestige of this in Cargo.toml:

[target.'cfg(not(any(target_arch="x86", target_arch="x86_64", target_arch="armv7", target_arch="aarch64")))'.dependencies]

Maybe you meant to remove it?

I see you went ahead and renamed native-tls-adapter to native-tls. What did you think of my comment about people confusing it with the 1.0 native-tls feature?

In the latest version of the branch with your changes, you've restored some of the diamond dependency problem. If crate A has:

[dependencies]
ureq = { version = "2.x", no-default-features = true, features = [ "native-tls" ] }

It is expecting that the "basic" methods like ureq::get will always use native-tls. For instance, imagine it is connecting to a host that only supports TLS 1.0.

If crate B has:

[dependencies]
ureq = { version = "2.x", features = ["tls"] }

It is expecting that the "basic" methods will always use rustls.

If crate C depends on crate A and everything is working fine, then adding a dependency on crate B could mysteriously break crate A's activities, by changing the default from rustls to native-tls. That's why I think it's important to emphasize Agent-based configuration for picking a non-default TLS backend. If we change behavior based on features we'll always have spooky action at a distance.

algesten · 2021-10-11T20:09:08Z

To clarify: you think we should not do the automatic selection of backend based on architecture? That's a bummer, but makes sense - it was fairly awkward to do, listing out the whole list of architectures multiple times.

I don't want to shut anyone down here. So I'm open for challenge. It's like we want Cargo to do something it doesn't have.

You left some vestige of this in Cargo.toml:

I was sort of thinking that could stay, but maybe it's just messy.

I see you went ahead and renamed native-tls-adapter to native-tls. What did you think of my comment about people confusing it with the 1.0 native-tls feature?

Sorry I missed that. To have a guessable/shorterI think it's worth the potential confusion.

In the latest version of the branch with your changes, you've restored some of the diamond dependency problem.

DOH!!! You're absolutely right, I fell right back into that trap.

Ok, so what if we say native-tls ALWAYS requires configuring the Agent? Like there will never be a default-to-native-tls option. Not even on platforms that can't do rustls. Would that work?

jsha · 2021-10-11T20:25:12Z

Ok, so what if we say native-tls ALWAYS requires configuring the Agent? Like there will never be a default-to-native-tls option. Not even on platforms that can't do rustls. Would that work?

Yes, I think this would work and has the advantage of being very simple to reason about. It's a bit of a hassle for folks on platforms that can't do rustls, since they can't take advantage of the ureq::get and other convenience methods, but I think that's okay: we'd really like to emphasize using Agents more, because that's the best way to offer configuration options.

It's like we want Cargo to do something it doesn't have.

Yes, agreed. It's almost there, and I felt like we could hack our way towards it, but it felt like a dimly-lit path full of stumbling blocks.

algesten · 2021-10-12T19:16:26Z

@jsha I pushed a follow-up commit to fix the diamond dep problem.

Reading the code, I think we have a diamond dependency issue also with native-certs. It's quite reasonable to not trust the root store on a machine (corporate IT installing a root cert to be able to MITM hijack TLS traffic in the firewall). A dependency could switch on native certs against my wishes. Not quite as serious as the TLS backend, but still there… What do you think?

mcr

I've looked through the latest changes with the view that I'm integrating mbedtls.
I was 99% done ten days ago when I had to change priorities. The changes seem like progress to me.

I think that its a bug if any application has both rusttls and native-tls compiled in.
I also think that if there is some application which just uses the convenience methods, and if given an https: URL it fails, that might be a bug. I don't think we can solve these until there are new cargo features to deal with conflicts.

mcr · 2021-10-02T00:40:33Z

src/agent.rs

+    /// The parameter can be a
+    /// [`rustls::ClientConfig`](https://docs.rs/rustls/0.19.1/rustls/struct.ClientConfig.html),
+    /// a [`native_tls::TlsConnector`](https://docs.rs/native-tls/0.2.7/native_tls/struct.TlsConnector.html),
+    /// or any type for which you implement the [HttpsConnector] trait.


If I built without tls and without native-tls feature (because I don't want them in my dependency tree), then wouldn't I also lack the hooks for the HttpsConnector trait?

mcr · 2021-10-05T14:26:40Z

src/agent.rs

+                #[cfg(all(not(feature = "tls"), not(feature = "native-tls")))]
+                tls_config: Arc::new(crate::stream::NoopTlsConnector),
+                #[cfg(feature = "tls")]
+                tls_config: crate::rtls::default_tls_config(),
+                #[cfg(feature = "native-tls")]
+                tls_config: Arc::new(native_tls::TlsConnector::new().unwrap()),


As the check failure points out, this won't work if "tls" is enabled and you add native-tls.
Alas, features can't be mutually exclusive.
I think it needs to have a Box(Trait) inside of it to get the polymorphism we want.

Now we have just one implementation of connect_https. All choice of TLS backends is controlled by a feature gate in AgentBuilder::new.

This adds config gates in Cargo.toml and in the source code that make native-tls the default on platforms that don't support rustls. To keep our matrix of features and platforms simple, I've eliminated the `tls` feature. TLS is now always built-in, and which library gets used depends on your platform.

jsha · 2021-10-11T17:12:02Z

src/lib.rs

+            _dns_name: &str,
+            _tcp_stream: TcpStream,
+        ) -> Result<Box<dyn HttpsStream>, crate::error::Error> {
+            panic!("No TLS backend. Use either feature 'tls' or 'native-tls'");


Let's never panic. Instead, we can return an Error.

jsha · 2021-10-22T05:14:46Z

.github/workflows/test.yml

        feature:
          - ""
          - json
          - charset
          - cookies
          - socks-proxy
+          - native-tls-adapter


Suggested change

- native-tls-adapter

jsha · 2021-10-22T05:15:40Z

Cargo.toml

+webpki = { version = "0.21", optional = true }
+webpki-roots = { version = "0.21", optional = true }


Suggested change

webpki = { version = "0.21", optional = true }

webpki-roots = { version = "0.21", optional = true }

webpki = { version = "0.22", optional = true }

webpki-roots = { version = "0.22", optional = true }

jsha · 2021-10-22T05:16:11Z

README.md

+* `native-tls` enables an adapter so you can pass a `native_tls::TlsConnector` instance
+  to `AgentBuilder::tls_connector`. Due to the risk of diamond dependencies accidentally switching on an unwanted
+  TLS implementation, `native-tls` is never picked up as a default or used by the crate level
+  convencience calls (`ureq::get` etc) – it must be configured.


Suggested change

convencience calls (`ureq::get` etc) – it must be configured.

convenience calls (`ureq::get` etc) – it must be configured on an Agent.

jsha · 2021-10-22T05:18:09Z

src/rtls.rs

+    }
+}
+
+// TODO: After upgrading to rustls 0.20 or higher, we can remove these Read


…more-3

jsha · 2021-11-05T06:37:45Z

I pushed some small changes with my review fixes. I think this is pretty much ready to go - what do you think @algesten ?

algesten · 2021-12-17T07:22:24Z

This continues in #449

jsha mentioned this pull request Jul 10, 2021

Restore support for native-tls #389

Closed

dontlaugh reviewed Jul 25, 2021

View reviewed changes

src/agent.rs Outdated Show resolved Hide resolved

src/stream.rs Outdated Show resolved Hide resolved

jsha mentioned this pull request Oct 2, 2021

Restore support for native-tls #319

Closed

mcr mentioned this pull request Oct 2, 2021

Add support for arbitrary TLS backends (rebased, some fixes) #426

Closed

jsha force-pushed the multi-tls-more branch from 306e6e8 to 4dec81e Compare October 10, 2021 01:28

jsha changed the title ~~Add support for arbitrary TLS backends~~ Use native-tls on platforms that need it, and remove feature-based TLS backend selection Oct 10, 2021

jsha changed the title ~~Use native-tls on platforms that need it, and remove feature-based TLS backend selection~~ Use native-tls on platforms that need it, and remove "tls" feature Oct 10, 2021

algesten approved these changes Oct 10, 2021

View reviewed changes

jsha force-pushed the multi-tls-more branch from 67b8aa6 to ec6afc3 Compare October 10, 2021 16:57

algesten force-pushed the multi-tls-more branch 2 times, most recently from d83e01f to 6c822e9 Compare October 11, 2021 12:48

mcr approved these changes Oct 13, 2021

View reviewed changes

jsha and others added 8 commits October 21, 2021 08:00

Add support for alternate TLS implementations.

87055a0

Remove feature-gated definition of connect_https

00057af

Now we have just one implementation of connect_https. All choice of TLS backends is controlled by a feature gate in AgentBuilder::new.

Add native-tls-adapter

01ca3a0

Review feedback

acb6daf

Remove stray diff

794d9e0

Allow compiling without TLS backend

c8c5efb

Fix tls/native-tls diamond dep problem

14cd64c

Fixes for rustls 0.2

b672c16

algesten force-pushed the multi-tls-more branch from 13bd193 to b672c16 Compare October 21, 2021 06:39

Update CHANGELOG and Cargo.toml for 2.3.0 (algesten#431)

9587a6c

jsha commented Oct 22, 2021

View reviewed changes

jsha added 3 commits November 4, 2021 22:20

Merge branch 'multi-tls-more' of github.com:jsha/ureq into multi-tls-…

215f6df

…more-3

Review comments

f49521e

Update README

3ff455c

Fix build

5359e5d

algesten closed this Dec 17, 2021

algesten mentioned this pull request Dec 17, 2021

Add support for alternate TLS implementations. #449

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use native-tls on platforms that need it, and remove "tls" feature #391

Use native-tls on platforms that need it, and remove "tls" feature #391

jsha commented May 28, 2021 •

edited

Loading

dontlaugh left a comment

jsha commented Oct 2, 2021

jsha commented Oct 10, 2021

algesten left a comment

algesten Oct 10, 2021

jsha Oct 10, 2021

jsha commented Oct 10, 2021

ThomasLamprecht commented Oct 11, 2021

algesten commented Oct 11, 2021

algesten commented Oct 11, 2021

jsha commented Oct 11, 2021 •

edited

Loading

algesten commented Oct 11, 2021

jsha commented Oct 11, 2021

algesten commented Oct 12, 2021 •

edited

Loading

mcr left a comment

mcr Oct 2, 2021

mcr Oct 5, 2021

jsha Oct 11, 2021

jsha Oct 22, 2021

jsha Oct 22, 2021

jsha Oct 22, 2021

jsha Oct 22, 2021

jsha commented Nov 5, 2021

algesten commented Dec 17, 2021

		webpki = { version = "0.21", optional = true }
		webpki-roots = { version = "0.21", optional = true }

	convencience calls (`ureq::get` etc) – it must be configured.
	convenience calls (`ureq::get` etc) – it must be configured on an Agent.

Use native-tls on platforms that need it, and remove "tls" feature #391

Use native-tls on platforms that need it, and remove "tls" feature #391

Conversation

jsha commented May 28, 2021 • edited Loading

dontlaugh left a comment

Choose a reason for hiding this comment

jsha commented Oct 2, 2021

jsha commented Oct 10, 2021

algesten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsha commented Oct 10, 2021

ThomasLamprecht commented Oct 11, 2021

algesten commented Oct 11, 2021

algesten commented Oct 11, 2021

jsha commented Oct 11, 2021 • edited Loading

algesten commented Oct 11, 2021

jsha commented Oct 11, 2021

algesten commented Oct 12, 2021 • edited Loading

mcr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsha commented Nov 5, 2021

algesten commented Dec 17, 2021

jsha commented May 28, 2021 •

edited

Loading

jsha commented Oct 11, 2021 •

edited

Loading

algesten commented Oct 12, 2021 •

edited

Loading