Add support to http.sslVerify #1135

Alvenix · 2023-11-28T14:24:07Z

I just started working on 1109. So work in progress. I am very new to the project so I may take time.

Alvenix · 2023-11-29T07:39:25Z

I have tested the feature in both max and max-pure features and it works as expected. I am waiting for review.

Should I add the modified fixtures to the commit?

Byron

Thanks a lot for tackling this. I like that it adds tests for portions of gix code that already have tests.

Please don't worry about integration tests for this, it's a topic of its own that can be tackled separately.

Generally, please split commits across crate boundaries. If it's a feature, please prefix the message with feat: so the message gets into the changelog. You can take a look at other commits like it for reference.

Within each crate, also please split work into different commits if they touch different features. Commit messages could be more exhaustive as well.

If in doubt, you can create one commit per step and squash them later according to certain criteria.

While continuing to work with the codebase, I recommend to pay extra attention to the surrounding of the code you are adding, and to mimic everything down to small details like the usage of ..

gix-transport/Cargo.toml

gix-transport/src/client/blocking_io/http/mod.rs

gix-transport/src/client/blocking_io/http/reqwest/remote.rs

Byron · 2023-11-29T08:37:48Z

gix/src/repository/config/transport.rs

+                    {
+                        let key = "http.sslVerify";
+                        opts.ssl_verify = config
+                            .boolean_filter_by_key(key, &mut trusted_only)


Please take a look around and note that each of the keys above in some way references a respective static version of the key they access. This must be done here as well, and there should be a Http::SSL_VERIFY constant.

I added a debug_assert to ensure the key match the constants, is that what you meant?

That's close. The question is why it shouldn't (potentially) fail if the value can't be read. In this line there is an example for how to do this.

Okay I understand what you mean, I added it.

Byron · 2023-11-29T08:39:47Z

src/plumbing/progress.rs

@@ -408,10 +408,6 @@ static GIT_CONFIG: &[Record] = &[
        config: "http.sslCipherList",
        usage: NotPlanned { reason: "on demand" }
    },
-    Record {


It's a good rule of thumb that whenever something is removed here, it should be added as a constant.

I added the constant, but I am not sure if I should link it with the environment variable: GIT_SSL_NO_VERIFY, as I didn't add support for it and it is the negation of sslVerify.

Yes, that's a great point!

Keys can have an environment variable attached to it using this method. These then needs to be initialized here. However, as it is a negation, we'd need add our own version of that variable as gitoxide.http.sslNoVerify, link it with (with a description()) GIT_SSL_NO_VERIFY, initialize it, test it, and then let it override any value that http.sslVerify may have just like git.

Hooking up variables is quite involved, but that separation is needed to avoid global state (like environment variables) to sneak in. This way they are controlled and used exactly in one or two places in the codebase, while being introspectable via gix config and controllable (as one can turn off certain kinds of environment variables from being read).

Byron · 2023-11-29T08:41:54Z

gix/tests/repository/config/transport_options.rs

@@ -106,6 +107,9 @@ mod http {
                max: version
            })
        );
+
+        assert!(ssl_verify);


Please use the second argument to assert!() for a message that explains that the default is true here - this is the value of this assertion really, as an accidental 'false if unset' would be a terrible security issue.

I have added the message.

…disable SSL verification. Currently this option only works in the curl backend.

…sport.

Alvenix · 2023-11-29T13:25:44Z

Generally, please split commits across crate boundaries. If it's a feature, please prefix the message with feat: so the message gets into the changelog. You can take a look at other commits like it for reference.

Within each crate, also please split work into different commits if they touch different features. Commit messages could be more exhaustive as well.

I split the commits and added feat: before them.

Byron

Thanks for bearing with me!

One more issue to resolve came up in the form of GIT_NO_SSL_VERIFY, but handling it is a good thing as it leads through other parts of the boot process that you probably want to be familiar with.

Byron · 2023-11-30T06:48:09Z

gix/src/repository/config/transport.rs

+                            .map(|value| config::tree::Http::SSL_VERIFY.enrich_error(value))
+                            .transpose()
+                            .with_leniency(lenient)
+                            .map_err(config::transport::http::Error::from)?


Explicit mapping shouldn't be required here, even though it might be required where it was copied from.

This value can by overriden by GIT_SSL_NO_VERIFY env variable. We use the value to override http.sslVerify when specifying ssl_verify in transport Options.

Byron

Thanks a lot for your help with this!

I have a made a few modifications and had to push them to a separate PR (#1142), which will be merged once CI passes. Thus, this PR can be closed as it was superseded.

Please note that next to my changes done in separate commits, I also changed one commit message from feat: to feat!: to indicate a breaking change - I forgot to mention this previously.

There have been a few adjustments you might find interesting, and I recommend to re-test from my branch to be sure it still works. I'd expect that, as gix now/still does what git does as well in terms of curl configuration.

Byron · 2023-12-01T08:35:18Z

gix/src/config/tree/sections/gitoxide.rs

@@ -179,6 +179,15 @@ mod subsections {
            http::SslVersion::new_ssl_version("sslVersionMax", &Gitoxide::HTTP).with_note(
                "entirely new to set the upper bound for the allowed ssl version range. Overwrites the max bound of `http.sslVersion` if set. Min and Max must be set to become effective.",
            );
+        /// The `gitoxide.http.sslNoVerify` key.
+        ///
+        /// If set, disable SSL verification. Using this is discouraged as it can lead to


Adding key-specific documentation here is commendable, and I think ideally gix would document all of its keys like that, effectively duplicating the git config documentation.

Byron · 2023-12-01T08:36:30Z

gix/src/config/tree/sections/gitoxide.rs

+        /// git server uses a self-signed certificate and the user accepts the associated security risks.
+        pub const SSL_NO_VERIFY: keys::Boolean = keys::Boolean::new_boolean("sslNoVerify", &Gitoxide::HTTP)
+            .with_environment_override("GIT_SSL_NO_VERIFY")
+            .with_deviation("Only supported when using curl as https backend")


Deviations are only in relation to git, so this would be used to say how it works differently compared to git. As there is no difference, I will remove it.

Byron · 2023-12-01T09:07:51Z

Closing as this PR was superseded by #1142.

Alvenix force-pushed the support_ssl_verify branch 6 times, most recently from 3da348e to 24f514a Compare November 28, 2023 17:02

Alvenix changed the title ~~DRAFT: Add support to http.sslVerify~~ Add support to http.sslVerify Nov 28, 2023

Alvenix force-pushed the support_ssl_verify branch from 24f514a to f25b63b Compare November 29, 2023 07:16

Byron requested changes Nov 29, 2023

View reviewed changes

Alvenix force-pushed the support_ssl_verify branch 3 times, most recently from 778e597 to 31bb08e Compare November 29, 2023 13:15

Alvenix added 2 commits November 29, 2023 16:23

feat: Add verify_ssl field to gix-transport Options which is used to …

08d36f6

…disable SSL verification. Currently this option only works in the curl backend.

feat: In gix read http.sslVerify config value and pass it to gix-tran…

7655be0

…sport.

Alvenix force-pushed the support_ssl_verify branch from 31bb08e to 7655be0 Compare November 29, 2023 13:24

Byron reviewed Nov 30, 2023

View reviewed changes

Alvenix force-pushed the support_ssl_verify branch from 1a8364f to e6a782c Compare November 30, 2023 12:07

feat: Add config value gitoxide.http.sslNoVerify

8fdd311

This value can by overriden by GIT_SSL_NO_VERIFY env variable. We use the value to override http.sslVerify when specifying ssl_verify in transport Options.

Alvenix force-pushed the support_ssl_verify branch from e6a782c to 8fdd311 Compare November 30, 2023 12:10

Byron mentioned this pull request Dec 1, 2023

support ssl verify (built on atop of 1135) #1142

Merged

Byron approved these changes Dec 1, 2023

View reviewed changes

Byron closed this Dec 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to http.sslVerify #1135

Add support to http.sslVerify #1135

Alvenix commented Nov 28, 2023 •

edited

Loading

Alvenix commented Nov 29, 2023

Byron left a comment

Byron Nov 29, 2023

Alvenix Nov 29, 2023

Byron Nov 29, 2023

Alvenix Nov 29, 2023

Byron Nov 29, 2023

Alvenix Nov 29, 2023 •

edited

Loading

Byron Nov 30, 2023

Byron Nov 29, 2023

Alvenix Nov 29, 2023

Alvenix commented Nov 29, 2023

Byron left a comment

Byron Nov 30, 2023

Byron left a comment

Byron Dec 1, 2023

Byron Dec 1, 2023

Byron commented Dec 1, 2023

Add support to http.sslVerify #1135

Add support to http.sslVerify #1135

Conversation

Alvenix commented Nov 28, 2023 • edited Loading

Alvenix commented Nov 29, 2023

Byron 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

Alvenix Nov 29, 2023 • 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

Alvenix commented Nov 29, 2023

Byron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Byron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Byron commented Dec 1, 2023

Alvenix commented Nov 28, 2023 •

edited

Loading

Alvenix Nov 29, 2023 •

edited

Loading