Fix managed http/https transport failures #858

2021-11-07T14:50:09-06:00

darkowlzz commented

2021-11-07 14:50:09 -06:00

(Migrated from github.com)

NOTE: Reimplemented in #870

For http transport, perviously, an initial request without any
credentials was being sent and upon unauthorized response,
credentials were fetched and the request was retried with the
obtained credentials. This appeared to have resulted in the remote
server closing the connection, resulting in clone failure error:

unable to clone: Post "http://test-user:***@127.0.0.1:40463/bar/test-reponame/git-upload-pack": io: read/write on closed pipe

Querying the credentials at the very beginning and not failing fixes the
closed pipe issue. It also works when there are no credentials.

NOTE: There may be good reason for the previous structure, but in all
of my tests, which covers cloning without any credentials and cloning
with basic auth, it works well without the previous method of requesting
without credentials and failing first. This change continues to work with
following changes for https certificate.

For https transport, since the go transport doesn't have access to the
certificate, it results in the following failure:

unable to clone: Get "https://127.0.0.1:44185/bar/test-reponame/info/refs?service=git-upload-pack": x509: certificate signed by unknown authority

Since the go smart transport is private, there seems to be no way to
pass the certificates to it. Unlike the credentials, there seems to be no
method to fetch the certificate from libgit2.

Some past discussions in libgit2 talks about keeping the data outside of
libgit2 when using an external transport.
With the current structure of the code, it's hard to pass the
certificate to the go transport.

This change introduces a global CA certs pool that can be populated by
the users of git2go and the smart transport can lookup for the presence
of any certificate associated with the target URL in the global pool before
making any https requests. This solves the cloning issue due to cert signing.

A user would populate the global cert pool with:

if err := git2go.RegisterCACerts(url, caBundle); err != nil {
	return err
}

The transport will automatically pick the certificate if available for a URL
and use it.

NOTE: This implementation came out of various attempts to debug and
fix the issue. I would like to have some guidance in finding proper ways to
implement this or fix this issue.
With these changes, all the aforementioned fluxcd tests work with libgit2
v1.3 and the smart transport. We've had trouble with the libgit2 library transport
and this could help us not depend on those and resolve the issues.

__NOTE__: Reimplemented in #870 For http transport, perviously, an initial request without any credentials was being sent and upon unauthorized response, credentials were fetched and the request was retried with the obtained credentials. This appeared to have resulted in the remote server closing the connection, resulting in clone failure error: ``` unable to clone: Post "http://test-user:***@127.0.0.1:40463/bar/test-reponame/git-upload-pack": io: read/write on closed pipe ``` Querying the credentials at the very beginning and not failing fixes the closed pipe issue. It also works when there are no credentials. **NOTE**: There may be good reason for the previous structure, but in all of [my tests](https://github.com/fluxcd/source-controller/blob/main/pkg/git/strategy/strategy_test.go), which covers cloning without any credentials and cloning with basic auth, it works well without the previous method of requesting without credentials and failing first. This change continues to work with following changes for https certificate. For https transport, since the go transport doesn't have access to the certificate, it results in the following failure: ``` unable to clone: Get "https://127.0.0.1:44185/bar/test-reponame/info/refs?service=git-upload-pack": x509: certificate signed by unknown authority ``` Since the go smart transport is private, there seems to be no way to pass the certificates to it. Unlike the credentials, there seems to be no method to fetch the certificate from libgit2. Some [past discussions](https://github.com/libgit2/libgit2/issues/2790#issuecomment-68631568) in libgit2 talks about keeping the data outside of libgit2 when using an external transport. With the current structure of the code, it's hard to pass the certificate to the go transport. This change introduces a global CA certs pool that can be populated by the users of git2go and the smart transport can lookup for the presence of any certificate associated with the target URL in the global pool before making any https requests. This solves the cloning issue due to cert signing. A user would populate the global cert pool with: ```go if err := git2go.RegisterCACerts(url, caBundle); err != nil { return err } ``` The transport will automatically pick the certificate if available for a URL and use it. **NOTE**: This implementation came out of various attempts to debug and fix the issue. I would like to have some guidance in finding proper ways to implement this or fix this issue. With these changes, all the aforementioned fluxcd tests work with libgit2 v1.3 and the smart transport. We've had trouble with the [libgit2 library transport](https://github.com/libgit2/git2go/issues/851) and this could help us not depend on those and resolve the issues.

👍 2

darkowlzz commented

2021-11-07 14:59:56 -06:00

(Migrated from github.com)

I'd work on fixing the test failures once I get some feedback and clarity about the solution.

hiddeco commented

2021-11-08 03:49:04 -06:00

(Migrated from github.com)

Side note that for us to be able to properly make use of the Go transport, having a non-global (custom) cert pool (or other way of configuration) is a pretty hard requirement to ensure proper isolation.

hiddeco commented

2021-11-08 04:47:53 -06:00

(Migrated from github.com)

Having thought some more about it, this issue isn't unique to git2go but also present in e.g. the Go native go-git project due to it using global protocol based configurations: 641ee1dd69/plumbing/transport/client/client.go (L16).

Which - while constructed a bit different - results in the same pattern of it being hard to overwrite defaults on a per-repository/operation basis.

Having thought some more about it, this issue isn't unique to `git2go` but also present in e.g. the Go native [`go-git` project](https://github.com/go-git/go-git) due to it using global protocol based configurations: https://github.com/go-git/go-git/blob/641ee1dd69d3b8616127623e4b9341f4f4196d12/plumbing/transport/client/client.go#L16. Which - while constructed a bit different - results in the same pattern of it being hard to overwrite defaults on a per-repository/operation basis.

jasperem commented

2021-11-18 14:46:58 -06:00

(Migrated from github.com)

Hi @darkowlzz, to fix Push you need to change req, err = http.NewRequest("POST", url+"/info/refs?service=git-upload-pack", nil) in 7f64702bd0/http.go (L94) to req, err = http.NewRequest("POST", url+"/info/refs?service="git-receive-pack", nil). After these changes, your implementation works for me.

Hi @darkowlzz, to fix Push you need to change `req, err = http.NewRequest("POST", url+"/info/refs?service=git-upload-pack", nil)` in https://github.com/darkowlzz/git2go/blob/7f64702bd0195300f75d0e5ff24706dd03776363/http.go#L94 to `req, err = http.NewRequest("POST", url+"/info/refs?service="git-receive-pack", nil)`. After these changes, your implementation works for me.

darkowlzz commented

2021-11-18 15:12:51 -06:00

(Migrated from github.com)

@jasperem Thanks for pointing that out. All of the testing I did were related to cloning. Didn't try pushing. Very helpful.
I'll update that.
I'm also about to attempt to reimplement the same without the global cert pool. I think that'll be a better approach in general.

@jasperem Thanks for pointing that out. All of the testing I did were related to cloning. Didn't try pushing. Very helpful. I'll update that. I'm also about to attempt to reimplement the same without the global cert pool. I think that'll be a better approach in general.

darkowlzz commented

2021-11-22 08:36:13 -06:00

(Migrated from github.com)

Update: A rewrite of this is in #870 . I took a different approach this time. Since we had a need to not share the secrets globally, I added the ability to setup the smart transport and configure it accordingly per operation. This way, any secrets used in the transport can be deleted and not shared.
Also, figured out the reason for the test failures above. When there's no credentials provided and no remote callbacks set, it resulted in passthrough error. Handled that accordingly and now the tests pass.

Hi @jasperem, I've added your suggestion as part of #870. It'd be nice if you could try that and see if it works for you. Any feedback would be very helpful.

__Update__: A rewrite of this is in #870 . I took a different approach this time. Since we had a need to not share the secrets globally, I added the ability to setup the smart transport and configure it accordingly per operation. This way, any secrets used in the transport can be deleted and not shared. Also, figured out the reason for the test failures above. When there's no credentials provided and no remote callbacks set, it resulted in passthrough error. Handled that accordingly and now the tests pass. Hi @jasperem, I've added your suggestion as part of #870. It'd be nice if you could try that and see if it works for you. Any feedback would be very helpful.

jasperem commented

2021-11-23 04:56:49 -06:00

(Migrated from github.com)

@darkowlzz thx for your work! I will test it and report how it works for me

👍 1

This pull request has changes conflicting with the target branch.