Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix managed http/https transport failures #858

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

darkowlzz
Copy link
Contributor

@darkowlzz darkowlzz commented Nov 7, 2021

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.

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.

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's 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 in the global pool before making any https requests.
This solves the cloning issue due to cert signing.
@darkowlzz
Copy link
Contributor Author

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

@hiddeco
Copy link

hiddeco commented Nov 8, 2021

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
Copy link

hiddeco commented Nov 8, 2021

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: 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
Copy link

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants