HTTP client configuration is ignored when cloning with InsecureSkipTLS

[mbirbeck]

I have found that if the insecureSkipTLS field in git.CloneOptions is set to true then all HTTP client configuration is ignored. I discovered this after setting the Proxy field of a custom HTTP client. I noticed that if the insecureSkipTLS was false then the go-git HTTP traffic was correctly sent via the proxy, but if it was set to true no traffic could be seen at the proxy.

The code below can be used to demonstrate this issue, although for simplicity I have used a different piece of http client configuration. I have set the ResponseHeaderTimeout of a custom HTTP client to be one nanosecond. This should always timeout, but if the InsecureSkipTLS is set to true, it does not timeout because the configuration is not actually being used.

package main

import (
	"log"
	"net/http"
	"time"

	git "github.com/go-git/go-git/v5"
	"github.com/go-git/go-git/v5/plumbing/transport/client"
	githttp "github.com/go-git/go-git/v5/plumbing/transport/http"
	"github.com/go-git/go-git/v5/storage/memory"
)

func main() {

	// Create a custom http client that should timeout.
	customClient := &http.Client{
		Transport: &http.Transport{
			ResponseHeaderTimeout: time.Nanosecond,
		},
	}
	client.InstallProtocol("https", githttp.NewClient(customClient))

	// Set InsecureSkipTLS to true in the CloneOptions.
	cloneOptions := &git.CloneOptions{
		URL:             "https://github.com/go-git/go-git.git",
		InsecureSkipTLS: false,
	}

	// Try to clone a repository. This call should get a timeout error
	// becauuse of our http client configuration, but it doesn't. The
	// http client configuration is being ignored. If we change the
	// InsecureSkipTLS field above to false, the custom http configuration
	// is used and we will see a timeout.
	_, err := git.Clone(memory.NewStorage(), nil, cloneOptions)
	if err != nil {
		log.Fatal(err)
	}
}

I can see the http configuration is lost in the getTransport function.

go-git/plumbing/transport/client/client.go

Lines 51 to 57 in 08cffa1

	func getTransport(endpoint *transport.Endpoint) (transport.Transport, error) {
	if endpoint.Protocol == "https" {
	if endpoint.InsecureSkipTLS {
	return insecureClient, nil
	}

If InsecureSkipTLS is true within git.CloneOptions then the configured http client is not used, instead the insecureClient is used, which does not have any of the user configuration.

go-git/plumbing/transport/client/client.go

Lines 26 to 34 in 08cffa1

	var insecureClient = http.NewClient(&gohttp.Client{
	Transport: &gohttp.Transport{
	TLSClientConfig: &tls.Config{
	InsecureSkipVerify: true,
	},
	},
	})

This code arrived in commit e398f84 in PR #228 which was for issue #140.

I think a fix could be to copy the client configuration and override just the InsecureSkipVerify field if InsecureSkipTLS has been set in git.CloneOptions. It also looks like the CABundle that was added to git.CloneOptions in the same commit might not be needed as it can already be set in the http client configuration and we don't need to override it.

[mbirbeck]

Looking more closely at the Protocols map in plumbing/transport/http/client.go I can see the http clients are not concrete http client types but transport.Transport interfaces, so I don't think the fix I mentioned before will work after all, as we can't just copy/overwrite struct fields.

[mbirbeck]

So to fix this I think that all the changes in commit e398f84 need to be reversed, since using either of the two configuration options that got added to git.CloneOptions with that change (InsecureSkipTLS or CABundle) will cause any HTTP custom client configuration to be wiped out. Before that change there was already the ability to set InsecureSkipTLS or specify certificates via the InsecureSkipVerify and RootCAs fields with the TLSClientConfig struct of a custom HTTP client. For example:

	customClient := &http.Client{
		Transport: &http.Transport{
			TLSClientConfig: &tls.Config{
				InsecureSkipVerify: true},
				RootCAs: CABundle,
		},
	}
	client.InstallProtocol("https", githttp.NewClient(customClient))

So commit e398f84 did not really add any extra functionality, but did mean there are now two ways of configuring the same HTTP configuration items: the existing method of using a custom HTTP client, and the new method of doing the same thing via git.CloneOptions, which interfere with each other. It also introduced a bug which means that if git.CloneOptions is used to configure either of the two new items (InsecureSkipTLS or CABundle), then the custom HTTP client configuration code no longer works as all the custom fields that may have been configured by a user get ignored.