Allows configuring TLS backend #386

erickguan · 2024-10-27T22:16:24Z

Fixes #384.

dbrgn · 2024-10-27T22:51:27Z

Hm, I wonder if we shouldn't just switch to native roots and call it a day... What's your opinion @niklasmohrin?

erickguan · 2024-10-27T22:54:54Z

FYI, the default build uses the native roots.

I didn't change the build default in this PR. I can see some adjustments required and your inputs.

niklasmohrin · 2024-11-07T13:29:35Z

@dbrgn I have some thoughts written down in the issue, I think that having the functionality is nice and people can still build it without the bloat by disabling features

niklasmohrin

Thanks! I have some requests regarding the code first, once we have this ironed out I will take a closer look at the documentation. Please feel free to reach out if you have any questions or thoughts :)

niklasmohrin · 2024-11-09T16:50:46Z

Cargo.toml

@@ -25,7 +25,7 @@ app_dirs = { version = "2", package = "app_dirs2" }
 clap = { version = "4", features = ["std", "derive", "help", "usage", "cargo", "error-context", "color", "wrap_help"], default-features = false }
 env_logger = { version = "0.11", optional = true }
 log = "0.4"
-reqwest = { version = "0.12.5", features = ["blocking"], default-features = false }
+reqwest = { version = "0.12.5", features = ["blocking", "native-tls", "rustls-tls", "rustls-tls-native-roots", "rustls-tls-webpki-roots"], default-features = false }


I had something else in mind: By disabling the native-roots (etc.) features of tealdeer, we would also disable the features for reqwest here, so that the resulting tealdeer binary is as small as possible.

I see. Fixed. I saw the reqwest 0.12.9 supports additional roots. So I also extended it. What do you think about it?

src/cache.rs

src/config.rs

src/cache.rs

niklasmohrin

I finally found some time to take another look :)

src/cache.rs

niklasmohrin · 2024-12-01T22:47:51Z

src/cache.rs

+        builder = match tls_backend {
+            #[cfg(feature = "native-tls")]
+            TlsBackend::NativeTLS => builder.use_native_tls(),
+            #[cfg(feature = "native-tls-with-webpki-roots")]
+            TlsBackend::NativeTLSWithWebPKIRoots => {
+                builder.use_native_tls().tls_built_in_webpki_certs(true)
+            }
+            #[cfg(feature = "rustls")]
+            TlsBackend::Rustls => builder.use_native_tls().tls_built_in_webpki_certs(true),
+            #[cfg(feature = "rustls-with-native-roots")]
+            TlsBackend::RustlsWithNativeRoots => {
+                builder.use_native_tls().tls_built_in_native_certs(true)
+            }
+        };


General structure is correct, but the content of the match arms is not quite right. In the Rustls* cases, we shouldn't call use_native_tls, but use_rustls_tls.

Then, there are three methods related to to TLS certificates available: tls_built_in_root_certs, tls_built_in_webpki_certs, and tls_built_in_native_certs. If I understand the description correctly, the first one applies to both native-tls and rustls, and the latter two only apply to rustls. To be sure, I would call all three of the methods in each case of our match and pass true or false accordingly. Otherwise, we might use more certificates than we want (the default is true for some of them)

Good catch on Rustls arm! My mistake.

I include 3 match arms:

TlsBackend::NativeTLS

TlsBackend::Rustls: reqwest uses Rustls with WebPKI as default. So I reflect that in the names.

TlsBackend::RustlsWithNativeRoots

niklasmohrin · 2024-12-01T22:49:50Z

src/cache.rs

+    macro_rules! https_client_tests {
+        // Define each test with an optional cfg attribute for conditional compilation
+        ($(
+            $(#[$cfg:meta])? $name:ident: $backend:expr
+        ),* $(,)?) => {
+            $(
+                $( #[$cfg] )?
+                #[test]
+                fn $name() {
+                    let dir = tempfile::tempdir().unwrap();
+
+                    let _ = Cache::build_client(&Cache {
+                        cache_dir: dir.into_path(),
+                        enable_styles: false,
+                        tls_backend: $backend,
+                    }, $backend).context("Expect built the client.");
+
+                    // intentionally empty, assumes we have built the client.
+                }
+            )*
+        };


I would just remove the macro and write three tests. They should also indicate that we are just testing that the client can be created, something like test_create_https_client_with_native_tls or so.

Finally, just calling .context on a Result will not check it - we need to use .expect or the ? operator

Make sense.

Yeah, .context is my mistake. It should have been .expect

src/config.rs

src/cache.rs

niklasmohrin · 2024-12-01T22:58:58Z

Cargo.toml

 native-tls = ["reqwest/native-tls"]
+native-tls-with-webpki-roots = ["reqwest/native-tls", "reqwest/rustls-tls-webpki-roots-no-provider"]


I don't think this works as intended. If I understand correctly, this still just uses the native tls with whatever certificates this has configured. Let's not add a new option in this PR, if this really works, we can still do it in a separate discussion :)

Hmm, my understanding is reqwest will be able to use WebPKI roots with this feature.

Removed.

Cargo.toml

erickguan

Thanks for the review!

I implemented tls_backend as a somewhat special config to bubble up the config conversion error as your comment.

Please have a look. If everything works well, I will update the documentation.

Cargo.toml

erickguan · 2024-12-03T10:14:00Z

Cargo.toml

 native-tls = ["reqwest/native-tls"]
+native-tls-with-webpki-roots = ["reqwest/native-tls", "reqwest/rustls-tls-webpki-roots-no-provider"]


Hmm, my understanding is reqwest will be able to use WebPKI roots with this feature.

Removed.

src/cache.rs

erickguan · 2024-12-03T10:22:26Z

src/cache.rs

+        builder = match tls_backend {
+            #[cfg(feature = "native-tls")]
+            TlsBackend::NativeTLS => builder.use_native_tls(),
+            #[cfg(feature = "native-tls-with-webpki-roots")]
+            TlsBackend::NativeTLSWithWebPKIRoots => {
+                builder.use_native_tls().tls_built_in_webpki_certs(true)
+            }
+            #[cfg(feature = "rustls")]
+            TlsBackend::Rustls => builder.use_native_tls().tls_built_in_webpki_certs(true),
+            #[cfg(feature = "rustls-with-native-roots")]
+            TlsBackend::RustlsWithNativeRoots => {
+                builder.use_native_tls().tls_built_in_native_certs(true)
+            }
+        };


Good catch on Rustls arm! My mistake.

I include 3 match arms:

TlsBackend::NativeTLS

TlsBackend::Rustls: reqwest uses Rustls with WebPKI as default. So I reflect that in the names.

TlsBackend::RustlsWithNativeRoots

src/cache.rs

erickguan · 2024-12-03T10:27:49Z

src/cache.rs

+    macro_rules! https_client_tests {
+        // Define each test with an optional cfg attribute for conditional compilation
+        ($(
+            $(#[$cfg:meta])? $name:ident: $backend:expr
+        ),* $(,)?) => {
+            $(
+                $( #[$cfg] )?
+                #[test]
+                fn $name() {
+                    let dir = tempfile::tempdir().unwrap();
+
+                    let _ = Cache::build_client(&Cache {
+                        cache_dir: dir.into_path(),
+                        enable_styles: false,
+                        tls_backend: $backend,
+                    }, $backend).context("Expect built the client.");
+
+                    // intentionally empty, assumes we have built the client.
+                }
+            )*
+        };


Make sense.

Yeah, .context is my mistake. It should have been .expect

src/config.rs

niklasmohrin · 2024-12-19T22:24:20Z

src/config.rs

+    } else {
+        0
+    };
+const SUPPORTED_TLS_BACKENDS: [RawTlsBackend; SUPPORTED_NUM_TLS_BACKENDS] = [


We can avoid the calculation of SUPPORTED_NUM_TLS_BACKENDS if we use the following:

Suggested change

const SUPPORTED_TLS_BACKENDS: [RawTlsBackend; SUPPORTED_NUM_TLS_BACKENDS] = [

const SUPPORTED_TLS_BACKENDS: &[RawTlsBackend] = &[

niklasmohrin · 2024-12-30T23:29:44Z

src/cache.rs

+    /// Builds HTTPS client based on configuration.
+    ///
+    /// Note that `Cargo.toml` also defines default feature .


I think this is not needed, the function signature speaks for itself :)

Suggested change

/// Builds HTTPS client based on configuration.

///

/// Note that `Cargo.toml` also defines default feature .

niklasmohrin · 2024-12-30T23:31:47Z

src/cache.rs

+        assert!(
+            Cache::build_client(TlsBackend::NativeTls).is_ok(),
+            "fails to build a client."
+        );


Nit: All three tests here can just use Cache::build_client(...).expect("Failed to build client") and not use assert! at all. Both will have the behavior regarding pass or fail, but the expect version will also print where the error in build_client came from

niklasmohrin · 2024-12-30T23:36:23Z

src/cache.rs

+    #[test]
+    fn test_create_https_client_with_rustls() {


These tests don't compile unless the respective features are activated, for example running cargo test --no-default-features -F rustls-with-native-roots will give a compile error on this test here. The tests should thus also have a cfg(...) attribute

niklasmohrin · 2024-12-30T23:54:29Z

src/config.rs

@@ -282,11 +300,52 @@ pub struct DirectoriesConfig {
    pub custom_pages_dir: Option<PathWithSource>,
 }

+#[derive(Copy, Clone, Debug, PartialEq, Eq, Deserialize, Serialize)]
+pub enum RawTlsBackend {


This still needs the following to allow the correct strings in the config file:

Suggested change

pub enum RawTlsBackend {

#[serde(rename_all = "kebab-case")]

pub enum RawTlsBackend {

niklasmohrin · 2024-12-30T23:55:33Z

src/config.rs

+        match self {
+            RawTlsBackend::NativeTls => write!(f, "native-tls"),
+            RawTlsBackend::Rustls => write!(f, "rustls"),
+            RawTlsBackend::RustlsWithNativeRoots => write!(f, "rustls-with-native-roots"),
+        }


If we use serde::Serialize as _; at the top of the file, we can actually reuse the renaming scheme from serde here:

Suggested change

match self {

RawTlsBackend::NativeTls => write!(f, "native-tls"),

RawTlsBackend::Rustls => write!(f, "rustls"),

RawTlsBackend::RustlsWithNativeRoots => write!(f, "rustls-with-native-roots"),

}

self.serialize(f)

niklasmohrin · 2024-12-30T23:56:54Z

src/config.rs

+    /// Rustls with WebPKI roots.
+    Rustls,
+    /// Rustls with native roots.
+    RustlsWithNativeRoots,


I feel that both options should be called rustls-with-something for consistency. The first option would then be RustlsWithWebpkiRoots

niklasmohrin · 2024-12-31T00:01:31Z

src/config.rs

+                    "Unsupported TLS backend: {}. This tealdeer build has support for the following options: {}",
+                 raw_config.updates.tls_backend,
+                 SUPPORTED_TLS_BACKENDS.map(|b| b.to_string()).join(", ")
+                ))


Very nice error message! Let's add a test that checks the output if this case occurs. To do so, you can use for example let testenv = TestEnv::new().no_default_features().with_feature("rustls"); in our integration tests at tests/lib.rs.

I discovered that our implementation of TestEnv::command is not correct at the moment, sorry about that. ~~You should change line 123 of tests/lib.rs~~
edit: I am fixing this in #399

Allows configuring tls_backend

c4ba0dd

niklasmohrin requested changes Nov 9, 2024

View reviewed changes

erickguan added 2 commits November 10, 2024 20:09

fixup! Allows configuring tls_backend

429187f

fixup! Allows configuring tls_backend

6abfcdc

erickguan requested a review from niklasmohrin November 10, 2024 23:18

niklasmohrin requested changes Dec 1, 2024

View reviewed changes

fixup! Allows configuring tls_backend

d1f1f4f

erickguan commented Dec 3, 2024

View reviewed changes

erickguan requested a review from niklasmohrin December 3, 2024 14:09

niklasmohrin requested changes Dec 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allows configuring TLS backend #386

Allows configuring TLS backend #386

erickguan commented Oct 27, 2024

dbrgn commented Oct 27, 2024

erickguan commented Oct 27, 2024

niklasmohrin commented Nov 7, 2024

niklasmohrin left a comment

niklasmohrin Nov 9, 2024

erickguan Nov 10, 2024

niklasmohrin left a comment

niklasmohrin Dec 1, 2024

erickguan Dec 3, 2024

niklasmohrin Dec 1, 2024

erickguan Dec 3, 2024

niklasmohrin Dec 1, 2024

erickguan Dec 3, 2024

erickguan left a comment •

edited

Loading

erickguan Dec 3, 2024

erickguan Dec 3, 2024

erickguan Dec 3, 2024

niklasmohrin Dec 19, 2024

niklasmohrin Dec 30, 2024

niklasmohrin Dec 30, 2024

niklasmohrin Dec 30, 2024

niklasmohrin Dec 30, 2024

niklasmohrin Dec 30, 2024

niklasmohrin Dec 30, 2024

niklasmohrin Dec 31, 2024 •

edited

Loading

		native-tls = ["reqwest/native-tls"]
		native-tls-with-webpki-roots = ["reqwest/native-tls", "reqwest/rustls-tls-webpki-roots-no-provider"]

	const SUPPORTED_TLS_BACKENDS: [RawTlsBackend; SUPPORTED_NUM_TLS_BACKENDS] = [
	const SUPPORTED_TLS_BACKENDS: &[RawTlsBackend] = &[

	/// Builds HTTPS client based on configuration.
	///
	/// Note that `Cargo.toml` also defines default feature .

	pub enum RawTlsBackend {
	#[serde(rename_all = "kebab-case")]
	pub enum RawTlsBackend {

Allows configuring TLS backend #386

Are you sure you want to change the base?

Allows configuring TLS backend #386

Conversation

erickguan commented Oct 27, 2024

dbrgn commented Oct 27, 2024

erickguan commented Oct 27, 2024

niklasmohrin commented Nov 7, 2024

niklasmohrin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasmohrin 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

erickguan left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasmohrin Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

erickguan left a comment •

edited

Loading

niklasmohrin Dec 31, 2024 •

edited

Loading