From d4beacb6ad89265bc6a6a216b466fd1eda409d76 Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Mon, 23 May 2022 20:50:35 +0530 Subject: [PATCH 1/8] Remove dependency on libgit2 credentials callback Injects transport and auth options at the transport level directly to bypass the inbuilt credentials callback because of it's several shortcomings. Moves some of the pre-existing logic from the reconciler to the checkout implementation. Signed-off-by: Sanskar Jaiswal --- controllers/gitrepository_controller.go | 28 +-- pkg/git/libgit2/checkout.go | 43 ++++ pkg/git/libgit2/managed/http.go | 77 +++---- pkg/git/libgit2/managed/http_test.go | 235 +++++++++++++++++++ pkg/git/libgit2/managed/managed_test.go | 287 ++---------------------- pkg/git/libgit2/managed/options.go | 25 ++- pkg/git/libgit2/managed/options_test.go | 94 ++++++++ pkg/git/libgit2/managed/ssh.go | 50 ++--- pkg/git/libgit2/managed/ssh_test.go | 124 ++++++++++ pkg/git/libgit2/managed_test.go | 124 +++++++--- pkg/git/libgit2/transport.go | 13 -- pkg/git/options.go | 5 + 12 files changed, 674 insertions(+), 431 deletions(-) create mode 100644 pkg/git/libgit2/managed/http_test.go create mode 100644 pkg/git/libgit2/managed/options_test.go create mode 100644 pkg/git/libgit2/managed/ssh_test.go diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index de03c2cf2..ae922ff36 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -458,27 +458,15 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, repositoryURL := obj.Spec.URL // managed GIT transport only affects the libgit2 implementation if managed.Enabled() && obj.Spec.GitImplementation == sourcev1.LibGit2Implementation { - // At present only HTTP connections have the ability to define remote options. - // Although this can be easily extended by ensuring that the fake URL below uses the - // target ssh scheme, and the libgit2/managed/ssh.go pulls that information accordingly. - // - // This is due to the fact the key libgit2 remote callbacks do not take place for HTTP - // whilst most still work for SSH. + // We set the TransportAuthID of this set of authentication options here by constructing + // a unique ID that won't clash in a multi tenant environment. This unique ID is used by + // libgit2 managed transports. This enables us to bypass the inbuilt credentials callback in + // libgit2, which is inflexible and unstable. if strings.HasPrefix(repositoryURL, "http") { - // Due to the lack of the callback feature, a fake target URL is created to allow - // for the smart sub transport be able to pick the options specific for this - // GitRepository object. - // The URL should use unique information that do not collide in a multi tenant - // deployment. - repositoryURL = fmt.Sprintf("http://%s/%s/%d", obj.Name, obj.UID, obj.Generation) - managed.AddTransportOptions(repositoryURL, - managed.TransportOptions{ - TargetURL: obj.Spec.URL, - CABundle: authOpts.CAFile, - }) - - // We remove the options from memory, to avoid accumulating unused options over time. - defer managed.RemoveTransportOptions(repositoryURL) + authOpts.TransportAuthID = fmt.Sprintf("http://%s/%s/%d", obj.Name, obj.UID, obj.Generation) + } + if strings.HasPrefix(repositoryURL, "ssh") { + authOpts.TransportAuthID = fmt.Sprintf("ssh://%s/%s/%d", obj.Name, obj.UID, obj.Generation) } } diff --git a/pkg/git/libgit2/checkout.go b/pkg/git/libgit2/checkout.go index cc6f8e487..56752951d 100644 --- a/pkg/git/libgit2/checkout.go +++ b/pkg/git/libgit2/checkout.go @@ -69,6 +69,22 @@ type CheckoutBranch struct { func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, opts *git.AuthOptions) (_ *git.Commit, err error) { defer recoverPanic(&err) + if managed.Enabled() { + // We store the target url and auth options mapped to a unique ID. We overwrite the target url + // with the TransportAuthID, because managed transports don't provide a way for any kind of + // dependency injection. This lets us have a way of doing interop between application level code + // and transport level code. + // Performing all fetch operations with the TransportAuthID as the url, lets the managed + // transport action use it to fetch the registered transport options which contains the + // _actual_ target url and the correct credentials to use. + managed.AddTransportOptions(opts.TransportAuthID, managed.TransportOptions{ + TargetURL: url, + AuthOpts: opts, + }) + url = opts.TransportAuthID + defer managed.RemoveTransportOptions(opts.TransportAuthID) + } + remoteCallBacks := RemoteCallbacks(ctx, opts) proxyOpts := &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto} @@ -170,6 +186,15 @@ type CheckoutTag struct { func (c *CheckoutTag) Checkout(ctx context.Context, path, url string, opts *git.AuthOptions) (_ *git.Commit, err error) { defer recoverPanic(&err) + if managed.Enabled() { + managed.AddTransportOptions(opts.TransportAuthID, managed.TransportOptions{ + TargetURL: url, + AuthOpts: opts, + }) + url = opts.TransportAuthID + defer managed.RemoveTransportOptions(opts.TransportAuthID) + } + remoteCallBacks := RemoteCallbacks(ctx, opts) proxyOpts := &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto} @@ -249,6 +274,15 @@ type CheckoutCommit struct { func (c *CheckoutCommit) Checkout(ctx context.Context, path, url string, opts *git.AuthOptions) (_ *git.Commit, err error) { defer recoverPanic(&err) + if managed.Enabled() { + managed.AddTransportOptions(opts.TransportAuthID, managed.TransportOptions{ + TargetURL: url, + AuthOpts: opts, + }) + url = opts.TransportAuthID + defer managed.RemoveTransportOptions(opts.TransportAuthID) + } + repo, err := git2go.Clone(url, path, &git2go.CloneOptions{ FetchOptions: git2go.FetchOptions{ DownloadTags: git2go.DownloadTagsNone, @@ -278,6 +312,15 @@ type CheckoutSemVer struct { func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, opts *git.AuthOptions) (_ *git.Commit, err error) { defer recoverPanic(&err) + if managed.Enabled() { + managed.AddTransportOptions(opts.TransportAuthID, managed.TransportOptions{ + TargetURL: url, + AuthOpts: opts, + }) + url = opts.TransportAuthID + defer managed.RemoveTransportOptions(opts.TransportAuthID) + } + verConstraint, err := semver.NewConstraint(c.SemVer) if err != nil { return nil, fmt.Errorf("semver parse error: %w", err) diff --git a/pkg/git/libgit2/managed/http.go b/pkg/git/libgit2/managed/http.go index 09c0ee26a..ffccb1f69 100644 --- a/pkg/git/libgit2/managed/http.go +++ b/pkg/git/libgit2/managed/http.go @@ -86,7 +86,7 @@ type httpSmartSubtransport struct { httpTransport *http.Transport } -func (t *httpSmartSubtransport) Action(targetUrl string, action git2go.SmartServiceAction) (git2go.SmartSubtransportStream, error) { +func (t *httpSmartSubtransport) Action(transportAuthID string, action git2go.SmartServiceAction) (git2go.SmartSubtransportStream, error) { var proxyFn func(*http.Request) (*url.URL, error) proxyOpts, err := t.transport.SmartProxyOptions() if err != nil { @@ -109,7 +109,7 @@ func (t *httpSmartSubtransport) Action(targetUrl string, action git2go.SmartServ t.httpTransport.Proxy = proxyFn t.httpTransport.DisableCompression = false - client, req, err := createClientRequest(targetUrl, action, t.httpTransport) + client, req, err := createClientRequest(transportAuthID, action, t.httpTransport) if err != nil { return nil, err } @@ -142,7 +142,7 @@ func (t *httpSmartSubtransport) Action(targetUrl string, action git2go.SmartServ return stream, nil } -func createClientRequest(targetUrl string, action git2go.SmartServiceAction, t *http.Transport) (*http.Client, *http.Request, error) { +func createClientRequest(transportAuthID string, action git2go.SmartServiceAction, t *http.Transport) (*http.Client, *http.Request, error) { var req *http.Request var err error @@ -150,28 +150,14 @@ func createClientRequest(targetUrl string, action git2go.SmartServiceAction, t * return nil, nil, fmt.Errorf("failed to create client: transport cannot be nil") } - finalUrl := targetUrl - opts, found := transportOptions(targetUrl) - if found { - if opts.TargetURL != "" { - // override target URL only if options are found and a new targetURL - // is provided. - finalUrl = opts.TargetURL - } + opts, found := getTransportOptions(transportAuthID) - // Add any provided certificate to the http transport. - if len(opts.CABundle) > 0 { - cap := x509.NewCertPool() - if ok := cap.AppendCertsFromPEM(opts.CABundle); !ok { - return nil, nil, fmt.Errorf("failed to use certificate from PEM") - } - t.TLSClientConfig = &tls.Config{ - RootCAs: cap, - } - } + if !found { + return nil, nil, fmt.Errorf("failed to create client: could not find transport options for the object: %s", transportAuthID) } + targetURL := opts.TargetURL - if len(finalUrl) > URLMaxLength { + if len(targetURL) > URLMaxLength { return nil, nil, fmt.Errorf("URL exceeds the max length (%d)", URLMaxLength) } @@ -182,20 +168,20 @@ func createClientRequest(targetUrl string, action git2go.SmartServiceAction, t * switch action { case git2go.SmartServiceActionUploadpackLs: - req, err = http.NewRequest("GET", finalUrl+"/info/refs?service=git-upload-pack", nil) + req, err = http.NewRequest("GET", targetURL+"/info/refs?service=git-upload-pack", nil) case git2go.SmartServiceActionUploadpack: - req, err = http.NewRequest("POST", finalUrl+"/git-upload-pack", nil) + req, err = http.NewRequest("POST", targetURL+"/git-upload-pack", nil) if err != nil { break } req.Header.Set("Content-Type", "application/x-git-upload-pack-request") case git2go.SmartServiceActionReceivepackLs: - req, err = http.NewRequest("GET", finalUrl+"/info/refs?service=git-receive-pack", nil) + req, err = http.NewRequest("GET", targetURL+"/info/refs?service=git-receive-pack", nil) case git2go.SmartServiceActionReceivepack: - req, err = http.NewRequest("POST", finalUrl+"/git-receive-pack", nil) + req, err = http.NewRequest("POST", targetURL+"/git-receive-pack", nil) if err != nil { break } @@ -209,6 +195,20 @@ func createClientRequest(targetUrl string, action git2go.SmartServiceAction, t * return nil, nil, err } + // Add any provided certificate to the http transport. + if opts.AuthOpts != nil { + req.SetBasicAuth(opts.AuthOpts.Username, opts.AuthOpts.Password) + if len(opts.AuthOpts.CAFile) > 0 { + certPool := x509.NewCertPool() + if ok := certPool.AppendCertsFromPEM(opts.AuthOpts.CAFile); !ok { + return nil, nil, fmt.Errorf("failed to use certificate from PEM") + } + t.TLSClientConfig = &tls.Config{ + RootCAs: certPool, + } + } + } + req.Header.Set("User-Agent", "git/2.0 (flux-libgit2)") return client, req, nil } @@ -239,7 +239,6 @@ type httpSmartSubtransportStream struct { recvReply sync.WaitGroup httpError error m sync.RWMutex - targetURL string } func newManagedHttpStream(owner *httpSmartSubtransport, req *http.Request, client *http.Client) *httpSmartSubtransportStream { @@ -324,29 +323,8 @@ func (self *httpSmartSubtransportStream) sendRequest() error { var resp *http.Response var err error - var userName string - var password string - - // Obtain the credentials and use them if available. - cred, err := self.owner.transport.SmartCredentials("", git2go.CredentialTypeUserpassPlaintext) - if err != nil { - // Passthrough error indicates that no credentials were provided. - // Continue without credentials. - if err.Error() != git2go.ErrorCodePassthrough.String() { - return err - } - } - - if cred != nil { - defer cred.Free() - - userName, password, err = cred.GetUserpassPlaintext() - if err != nil { - return err - } - } - var content []byte + for { req := &http.Request{ Method: self.req.Method, @@ -365,7 +343,6 @@ func (self *httpSmartSubtransportStream) sendRequest() error { req.ContentLength = -1 } - req.SetBasicAuth(userName, password) traceLog.Info("[http]: new request", "method", req.Method, "URL", req.URL) resp, err = self.client.Do(req) if err != nil { diff --git a/pkg/git/libgit2/managed/http_test.go b/pkg/git/libgit2/managed/http_test.go new file mode 100644 index 000000000..bf54de597 --- /dev/null +++ b/pkg/git/libgit2/managed/http_test.go @@ -0,0 +1,235 @@ +/* +Copyright 2022 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package managed + +import ( + "fmt" + "net/http" + "os" + "path/filepath" + "testing" + + "github.com/fluxcd/pkg/gittestserver" + "github.com/fluxcd/source-controller/pkg/git" + "github.com/go-logr/logr" + . "github.com/onsi/gomega" + + git2go "github.com/libgit2/git2go/v33" +) + +func TestHttpAction_CreateClientRequest(t *testing.T) { + opts := &TransportOptions{ + TargetURL: "https://final-target/abc", + } + + optsWithAuth := &TransportOptions{ + TargetURL: "https://final-target/abc", + AuthOpts: &git.AuthOptions{ + Username: "user", + Password: "pwd", + }, + } + id := "https://obj-id" + + tests := []struct { + name string + assertFunc func(g *WithT, req *http.Request, client *http.Client) + action git2go.SmartServiceAction + opts *TransportOptions + transport *http.Transport + wantedErr error + }{ + { + name: "Uploadpack: URL and method are correctly set", + action: git2go.SmartServiceActionUploadpack, + transport: &http.Transport{}, + assertFunc: func(g *WithT, req *http.Request, _ *http.Client) { + g.Expect(req.URL.String()).To(Equal("https://final-target/abc/git-upload-pack")) + g.Expect(req.Method).To(Equal("POST")) + }, + opts: opts, + wantedErr: nil, + }, + { + name: "UploadpackLs: URL and method are correctly set", + action: git2go.SmartServiceActionUploadpackLs, + transport: &http.Transport{}, + assertFunc: func(g *WithT, req *http.Request, _ *http.Client) { + g.Expect(req.URL.String()).To(Equal("https://final-target/abc/info/refs?service=git-upload-pack")) + g.Expect(req.Method).To(Equal("GET")) + }, + opts: opts, + wantedErr: nil, + }, + { + name: "Receivepack: URL and method are correctly set", + action: git2go.SmartServiceActionReceivepack, + transport: &http.Transport{}, + assertFunc: func(g *WithT, req *http.Request, _ *http.Client) { + g.Expect(req.URL.String()).To(Equal("https://final-target/abc/git-receive-pack")) + g.Expect(req.Method).To(Equal("POST")) + }, + opts: opts, + wantedErr: nil, + }, + { + name: "ReceivepackLs: URL and method are correctly set", + action: git2go.SmartServiceActionReceivepackLs, + transport: &http.Transport{}, + assertFunc: func(g *WithT, req *http.Request, _ *http.Client) { + g.Expect(req.URL.String()).To(Equal("https://final-target/abc/info/refs?service=git-receive-pack")) + g.Expect(req.Method).To(Equal("GET")) + }, + opts: opts, + wantedErr: nil, + }, + { + name: "credentials are correctly configured", + action: git2go.SmartServiceActionUploadpack, + transport: &http.Transport{}, + opts: optsWithAuth, + assertFunc: func(g *WithT, req *http.Request, client *http.Client) { + g.Expect(req.URL.String()).To(Equal("https://final-target/abc/git-upload-pack")) + g.Expect(req.Method).To(Equal("POST")) + + username, pwd, ok := req.BasicAuth() + if !ok { + t.Errorf("could not find Authentication header in request.") + } + g.Expect(username).To(Equal("user")) + g.Expect(pwd).To(Equal("pwd")) + }, + wantedErr: nil, + }, + { + name: "error when no http.transport provided", + action: git2go.SmartServiceActionUploadpack, + transport: nil, + opts: opts, + wantedErr: fmt.Errorf("failed to create client: transport cannot be nil"), + }, + { + name: "error when no transport options are registered", + action: git2go.SmartServiceActionUploadpack, + transport: &http.Transport{}, + opts: nil, + wantedErr: fmt.Errorf("failed to create client: could not find transport options for the object: https://obj-id"), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + if tt.opts != nil { + AddTransportOptions(id, *tt.opts) + } + + client, req, err := createClientRequest(id, tt.action, tt.transport) + if err != nil { + t.Log(err) + } + if tt.wantedErr != nil { + g.Expect(err).To(Equal(tt.wantedErr)) + } else { + tt.assertFunc(g, req, client) + } + + if tt.opts != nil { + RemoveTransportOptions(id) + } + }) + } +} + +func TestHTTPManagedTransport_E2E(t *testing.T) { + g := NewWithT(t) + + server, err := gittestserver.NewTempGitServer() + g.Expect(err).ToNot(HaveOccurred()) + defer os.RemoveAll(server.Root()) + + user := "test-user" + pwd := "test-pswd" + server.Auth(user, pwd) + server.KeyDir(filepath.Join(server.Root(), "keys")) + + err = server.ListenSSH() + g.Expect(err).ToNot(HaveOccurred()) + + err = server.StartHTTP() + g.Expect(err).ToNot(HaveOccurred()) + defer server.StopHTTP() + + go func() { + server.StartSSH() + }() + defer server.StopSSH() + + // Force managed transport to be enabled + InitManagedTransport(logr.Discard()) + + repoPath := "test.git" + err = server.InitRepo("../../testdata/git/repo", git.DefaultBranch, repoPath) + g.Expect(err).ToNot(HaveOccurred()) + + tmpDir := t.TempDir() + + // Register the auth options and target url mapped to a unique id. + id := "http://obj-id" + AddTransportOptions(id, TransportOptions{ + TargetURL: server.HTTPAddress() + "/" + repoPath, + AuthOpts: &git.AuthOptions{ + Username: user, + Password: pwd, + }, + }) + + // We call Clone with id instead of the actual url, as the transport action + // will fetch the actual url and the required credentials using the id as + // a identifier. + repo, err := git2go.Clone(id, tmpDir, &git2go.CloneOptions{ + CheckoutOptions: git2go.CheckoutOptions{ + Strategy: git2go.CheckoutForce, + }, + }) + g.Expect(err).ToNot(HaveOccurred()) + repo.Free() +} + +func TestHTTPManagedTransport_HandleRedirect(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + + // Force managed transport to be enabled + InitManagedTransport(logr.Discard()) + + id := "http://obj-id" + AddTransportOptions(id, TransportOptions{ + TargetURL: "http://github.com/stefanprodan/podinfo", + }) + + // GitHub will cause a 301 and redirect to https + repo, err := git2go.Clone(id, tmpDir, &git2go.CloneOptions{ + CheckoutOptions: git2go.CheckoutOptions{ + Strategy: git2go.CheckoutForce, + }, + }) + + g.Expect(err).ToNot(HaveOccurred()) + repo.Free() +} diff --git a/pkg/git/libgit2/managed/managed_test.go b/pkg/git/libgit2/managed/managed_test.go index 5bfd1c1ef..beda7fc2a 100644 --- a/pkg/git/libgit2/managed/managed_test.go +++ b/pkg/git/libgit2/managed/managed_test.go @@ -17,287 +17,32 @@ limitations under the License. package managed import ( - "fmt" - "net/http" "os" - "path/filepath" - "reflect" "testing" - - "github.com/fluxcd/pkg/gittestserver" - "github.com/fluxcd/pkg/ssh" - "github.com/fluxcd/source-controller/pkg/git" - "github.com/go-logr/logr" - - git2go "github.com/libgit2/git2go/v33" - . "github.com/onsi/gomega" - "gotest.tools/assert" ) -func TestHttpAction_CreateClientRequest(t *testing.T) { - tests := []struct { - name string - url string - expectedUrl string - expectedMethod string - action git2go.SmartServiceAction - opts *TransportOptions - transport *http.Transport - wantedErr error - }{ - { - name: "Uploadpack: no changes when no options found", - url: "https://sometarget/abc", - expectedUrl: "https://sometarget/abc/git-upload-pack", - expectedMethod: "POST", - action: git2go.SmartServiceActionUploadpack, - transport: &http.Transport{}, - opts: nil, - wantedErr: nil, - }, - { - name: "UploadpackLs: no changes when no options found", - url: "https://sometarget/abc", - expectedUrl: "https://sometarget/abc/info/refs?service=git-upload-pack", - expectedMethod: "GET", - action: git2go.SmartServiceActionUploadpackLs, - transport: &http.Transport{}, - opts: nil, - wantedErr: nil, - }, - { - name: "Receivepack: no changes when no options found", - url: "https://sometarget/abc", - expectedUrl: "https://sometarget/abc/git-receive-pack", - expectedMethod: "POST", - action: git2go.SmartServiceActionReceivepack, - transport: &http.Transport{}, - opts: nil, - wantedErr: nil, - }, - { - name: "ReceivepackLs: no changes when no options found", - url: "https://sometarget/abc", - expectedUrl: "https://sometarget/abc/info/refs?service=git-receive-pack", - expectedMethod: "GET", - action: git2go.SmartServiceActionReceivepackLs, - transport: &http.Transport{}, - opts: nil, - wantedErr: nil, - }, - { - name: "override URL via options", - url: "https://initial-target/abc", - expectedUrl: "https://final-target/git-upload-pack", - expectedMethod: "POST", - action: git2go.SmartServiceActionUploadpack, - transport: &http.Transport{}, - opts: &TransportOptions{ - TargetURL: "https://final-target", - }, - wantedErr: nil, - }, - { - name: "error when no http.transport provided", - url: "https://initial-target/abc", - expectedUrl: "", - expectedMethod: "", - action: git2go.SmartServiceActionUploadpack, - transport: nil, - opts: nil, - wantedErr: fmt.Errorf("failed to create client: transport cannot be nil"), - }, +func TestFlagStatus(t *testing.T) { + if Enabled() { + t.Errorf("experimental transport should not be enabled by default") } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if tt.opts != nil { - AddTransportOptions(tt.url, *tt.opts) - } - - _, req, err := createClientRequest(tt.url, tt.action, tt.transport) - if tt.wantedErr != nil { - if tt.wantedErr.Error() != err.Error() { - t.Errorf("wanted: %v got: %v", tt.wantedErr, err) - } - } else { - assert.Equal(t, req.URL.String(), tt.expectedUrl) - assert.Equal(t, req.Method, tt.expectedMethod) - } - - if tt.opts != nil { - RemoveTransportOptions(tt.url) - } - }) + os.Setenv("EXPERIMENTAL_GIT_TRANSPORT", "true") + if !Enabled() { + t.Errorf("experimental transport should be enabled when env EXPERIMENTAL_GIT_TRANSPORT=true") } -} -func TestOptions(t *testing.T) { - tests := []struct { - name string - registerOpts bool - url string - opts TransportOptions - expectOpts bool - expectedOpts *TransportOptions - }{ - { - name: "return registered option", - registerOpts: true, - url: "https://target/?123", - opts: TransportOptions{}, - expectOpts: true, - expectedOpts: &TransportOptions{}, - }, - { - name: "match registered options", - registerOpts: true, - url: "https://target/?876", - opts: TransportOptions{ - TargetURL: "https://new-target/321", - CABundle: []byte{123, 213, 132}, - }, - expectOpts: true, - expectedOpts: &TransportOptions{ - TargetURL: "https://new-target/321", - CABundle: []byte{123, 213, 132}, - }, - }, - { - name: "ignore when options not registered", - registerOpts: false, - url: "", - opts: TransportOptions{}, - expectOpts: false, - expectedOpts: nil, - }, + os.Setenv("EXPERIMENTAL_GIT_TRANSPORT", "1") + if !Enabled() { + t.Errorf("experimental transport should be enabled when env EXPERIMENTAL_GIT_TRANSPORT=1") } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if tt.registerOpts { - AddTransportOptions(tt.url, tt.opts) - } - - opts, found := transportOptions(tt.url) - if tt.expectOpts != found { - t.Errorf("%s: wanted %v got %v", tt.name, tt.expectOpts, found) - } - - if tt.expectOpts { - if reflect.DeepEqual(opts, *tt.expectedOpts) { - t.Errorf("%s: wanted %v got %v", tt.name, *tt.expectedOpts, opts) - } - } - - if tt.registerOpts { - RemoveTransportOptions(tt.url) - } - - if _, found = transportOptions(tt.url); found { - t.Errorf("%s: option for %s was not removed", tt.name, tt.url) - } - }) + os.Setenv("EXPERIMENTAL_GIT_TRANSPORT", "somethingelse") + if Enabled() { + t.Errorf("experimental transport should be enabled only when env EXPERIMENTAL_GIT_TRANSPORT is 1 or true but was enabled for 'somethingelse'") } -} - -func TestManagedTransport_E2E(t *testing.T) { - g := NewWithT(t) - - server, err := gittestserver.NewTempGitServer() - g.Expect(err).ToNot(HaveOccurred()) - defer os.RemoveAll(server.Root()) - - user := "test-user" - pasword := "test-pswd" - server.Auth(user, pasword) - server.KeyDir(filepath.Join(server.Root(), "keys")) - - err = server.ListenSSH() - g.Expect(err).ToNot(HaveOccurred()) - - err = server.StartHTTP() - g.Expect(err).ToNot(HaveOccurred()) - defer server.StopHTTP() - - go func() { - server.StartSSH() - }() - defer server.StopSSH() - // Force managed transport to be enabled - InitManagedTransport(logr.Discard()) - - repoPath := "test.git" - err = server.InitRepo("../../testdata/git/repo", git.DefaultBranch, repoPath) - g.Expect(err).ToNot(HaveOccurred()) - - tmpDir := t.TempDir() - - // Test HTTP transport - - // Use a fake-url and force it to be overriden by the smart transport. - // This was the way found to ensure that the built-in transport was not used. - httpAddress := "http://fake-url" - AddTransportOptions(httpAddress, TransportOptions{ - TargetURL: server.HTTPAddress() + "/" + repoPath, - }) - - repo, err := git2go.Clone(httpAddress, tmpDir, &git2go.CloneOptions{ - FetchOptions: git2go.FetchOptions{ - RemoteCallbacks: git2go.RemoteCallbacks{ - CredentialsCallback: func(url, username_from_url string, allowed_types git2go.CredentialType) (*git2go.Credential, error) { - return git2go.NewCredentialUserpassPlaintext(user, pasword) - }, - }, - }, - CheckoutOptions: git2go.CheckoutOptions{ - Strategy: git2go.CheckoutForce, - }, - }) - g.Expect(err).ToNot(HaveOccurred()) - repo.Free() - - tmpDir2 := t.TempDir() - - kp, err := ssh.NewEd25519Generator().Generate() - g.Expect(err).ToNot(HaveOccurred()) - - // Test SSH transport - sshAddress := server.SSHAddress() + "/" + repoPath - repo, err = git2go.Clone(sshAddress, tmpDir2, &git2go.CloneOptions{ - FetchOptions: git2go.FetchOptions{ - RemoteCallbacks: git2go.RemoteCallbacks{ - CredentialsCallback: func(url, username_from_url string, allowed_types git2go.CredentialType) (*git2go.Credential, error) { - return git2go.NewCredentialSSHKeyFromMemory("git", "", string(kp.PrivateKey), "") - }, - }, - }, - CheckoutOptions: git2go.CheckoutOptions{ - Strategy: git2go.CheckoutForce, - }, - }) - - g.Expect(err).ToNot(HaveOccurred()) - repo.Free() -} - -func TestManagedTransport_HandleRedirect(t *testing.T) { - g := NewWithT(t) - - tmpDir := t.TempDir() - - // Force managed transport to be enabled - InitManagedTransport(logr.Discard()) - - // GitHub will cause a 301 and redirect to https - repo, err := git2go.Clone("http://github.com/stefanprodan/podinfo", tmpDir, &git2go.CloneOptions{ - FetchOptions: git2go.FetchOptions{}, - CheckoutOptions: git2go.CheckoutOptions{ - Strategy: git2go.CheckoutForce, - }, - }) - - g.Expect(err).ToNot(HaveOccurred()) - repo.Free() + os.Unsetenv("EXPERIMENTAL_GIT_TRANSPORT") + if Enabled() { + t.Errorf("experimental transport should not be enabled when env EXPERIMENTAL_GIT_TRANSPORT is not present") + } } diff --git a/pkg/git/libgit2/managed/options.go b/pkg/git/libgit2/managed/options.go index d4d346ad0..58a04da75 100644 --- a/pkg/git/libgit2/managed/options.go +++ b/pkg/git/libgit2/managed/options.go @@ -18,35 +18,38 @@ package managed import ( "sync" + + "github.com/fluxcd/source-controller/pkg/git" ) // TransportOptions represents options to be applied at transport-level // at request time. type TransportOptions struct { TargetURL string - CABundle []byte + AuthOpts *git.AuthOptions } var ( + // transportOpts maps a unique id to a set of transport options. transportOpts = make(map[string]TransportOptions, 0) m sync.RWMutex ) -func AddTransportOptions(targetUrl string, opts TransportOptions) { +func AddTransportOptions(id string, opts TransportOptions) { m.Lock() - transportOpts[targetUrl] = opts + transportOpts[id] = opts m.Unlock() } -func RemoveTransportOptions(targetUrl string) { +func RemoveTransportOptions(id string) { m.Lock() - delete(transportOpts, targetUrl) + delete(transportOpts, id) m.Unlock() } -func transportOptions(targetUrl string) (*TransportOptions, bool) { +func getTransportOptions(id string) (*TransportOptions, bool) { m.RLock() - opts, found := transportOpts[targetUrl] + opts, found := transportOpts[id] m.RUnlock() if found { @@ -60,16 +63,16 @@ func transportOptions(targetUrl string) (*TransportOptions, bool) { // Given that TransportOptions can allow for the target URL to be overriden // this returns the same input if Managed Transport is disabled or if no TargetURL // is set on TransportOptions. -func EffectiveURL(targetUrl string) string { +func EffectiveURL(id string) string { if !Enabled() { - return targetUrl + return id } - if opts, found := transportOptions(targetUrl); found { + if opts, found := getTransportOptions(id); found { if opts.TargetURL != "" { return opts.TargetURL } } - return targetUrl + return id } diff --git a/pkg/git/libgit2/managed/options_test.go b/pkg/git/libgit2/managed/options_test.go new file mode 100644 index 000000000..4f35a0fcd --- /dev/null +++ b/pkg/git/libgit2/managed/options_test.go @@ -0,0 +1,94 @@ +/* +Copyright 2022 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package managed + +import ( + "testing" + + "github.com/fluxcd/source-controller/pkg/git" + . "github.com/onsi/gomega" +) + +func TestTransportOptions(t *testing.T) { + tests := []struct { + name string + registerOpts bool + url string + opts TransportOptions + expectOpts bool + expectedOpts *TransportOptions + }{ + { + name: "return registered option", + registerOpts: true, + url: "https://target/?123", + opts: TransportOptions{}, + expectOpts: true, + expectedOpts: &TransportOptions{}, + }, + { + name: "match registered options", + registerOpts: true, + url: "https://target/?876", + opts: TransportOptions{ + TargetURL: "https://new-target/321", + AuthOpts: &git.AuthOptions{ + CAFile: []byte{123, 213, 132}, + }, + }, + expectOpts: true, + expectedOpts: &TransportOptions{ + TargetURL: "https://new-target/321", + AuthOpts: &git.AuthOptions{ + CAFile: []byte{123, 213, 132}, + }, + }, + }, + { + name: "ignore when options not registered", + registerOpts: false, + url: "", + opts: TransportOptions{}, + expectOpts: false, + expectedOpts: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + if tt.registerOpts { + AddTransportOptions(tt.url, tt.opts) + } + + opts, found := getTransportOptions(tt.url) + g.Expect(found).To(Equal(found)) + + if tt.expectOpts { + g.Expect(tt.expectedOpts).To(Equal(opts)) + } + + if tt.registerOpts { + RemoveTransportOptions(tt.url) + } + + _, found = getTransportOptions(tt.url) + g.Expect(found).To(BeFalse()) + }) + } +} diff --git a/pkg/git/libgit2/managed/ssh.go b/pkg/git/libgit2/managed/ssh.go index ea7bd491b..3895fbe4e 100644 --- a/pkg/git/libgit2/managed/ssh.go +++ b/pkg/git/libgit2/managed/ssh.go @@ -96,11 +96,16 @@ type sshSmartSubtransport struct { connected bool } -func (t *sshSmartSubtransport) Action(urlString string, action git2go.SmartServiceAction) (git2go.SmartSubtransportStream, error) { +func (t *sshSmartSubtransport) Action(credentialsID string, action git2go.SmartServiceAction) (git2go.SmartSubtransportStream, error) { runtime.LockOSThread() defer runtime.UnlockOSThread() - u, err := url.Parse(urlString) + opts, found := getTransportOptions(credentialsID) + if !found { + return nil, fmt.Errorf("could not find transport options for object: %s", credentialsID) + } + + u, err := url.Parse(opts.TargetURL) if err != nil { return nil, err } @@ -146,19 +151,13 @@ func (t *sshSmartSubtransport) Action(urlString string, action git2go.SmartServi _ = t.Close() } - cred, err := t.transport.SmartCredentials("", git2go.CredentialTypeSSHMemory) - if err != nil { - return nil, err - } - defer cred.Free() - port := "22" if u.Port() != "" { port = u.Port() } t.addr = net.JoinHostPort(u.Hostname(), port) - sshConfig, err := clientConfig(t.addr, cred) + sshConfig, err := createClientConfig(opts.AuthOpts) if err != nil { return nil, err } @@ -307,39 +306,28 @@ func (stream *sshSmartSubtransportStream) Free() { traceLog.Info("[ssh]: sshSmartSubtransportStream.Free()") } -func clientConfig(remoteAddress string, cred *git2go.Credential) (*ssh.ClientConfig, error) { - if cred == nil { - return nil, fmt.Errorf("cannot create ssh client config from a nil credential") +func createClientConfig(authOpts *git.AuthOptions) (*ssh.ClientConfig, error) { + if authOpts == nil { + return nil, fmt.Errorf("cannot create ssh client config from nil ssh auth options") } - username, _, privatekey, passphrase, err := cred.GetSSHKey() - if err != nil { - return nil, err - } - - var pemBytes []byte - if cred.Type() == git2go.CredentialTypeSSHMemory { - pemBytes = []byte(privatekey) + var signer ssh.Signer + var err error + if authOpts.Password != "" { + signer, err = ssh.ParsePrivateKeyWithPassphrase(authOpts.Identity, []byte(authOpts.Password)) } else { - return nil, fmt.Errorf("file based SSH credential is not supported") + signer, err = ssh.ParsePrivateKey(authOpts.Identity) } - - var key ssh.Signer - if passphrase != "" { - key, err = ssh.ParsePrivateKeyWithPassphrase(pemBytes, []byte(passphrase)) - } else { - key, err = ssh.ParsePrivateKey(pemBytes) - } - if err != nil { return nil, err } cfg := &ssh.ClientConfig{ - User: username, - Auth: []ssh.AuthMethod{ssh.PublicKeys(key)}, + User: authOpts.Username, + Auth: []ssh.AuthMethod{ssh.PublicKeys(signer)}, Timeout: sshConnectionTimeOut, } + if len(git.KexAlgos) > 0 { cfg.Config.KeyExchanges = git.KexAlgos } diff --git a/pkg/git/libgit2/managed/ssh_test.go b/pkg/git/libgit2/managed/ssh_test.go new file mode 100644 index 000000000..4d5a7b37a --- /dev/null +++ b/pkg/git/libgit2/managed/ssh_test.go @@ -0,0 +1,124 @@ +/* +Copyright 2022 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package managed + +import ( + "os" + "path/filepath" + "testing" + + "github.com/fluxcd/pkg/ssh" + "github.com/fluxcd/source-controller/pkg/git" + "github.com/go-logr/logr" + . "github.com/onsi/gomega" + + "github.com/fluxcd/pkg/gittestserver" + git2go "github.com/libgit2/git2go/v33" +) + +func TestSSHAction_clientConfig(t *testing.T) { + kp, err := ssh.GenerateKeyPair(ssh.RSA_4096) + if err != nil { + t.Fatalf("could not generate keypair: %s", err) + } + tests := []struct { + name string + authOpts *git.AuthOptions + expectedUsername string + expectedAuthLen int + expectErr string + }{ + { + name: "nil SSHTransportOptions returns an error", + authOpts: nil, + expectErr: "cannot create ssh client config from nil ssh auth options", + }, + { + name: "valid SSHTransportOptions returns a valid SSHClientConfig", + authOpts: &git.AuthOptions{ + Identity: kp.PrivateKey, + Username: "user", + }, + expectedUsername: "user", + expectedAuthLen: 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + cfg, err := createClientConfig(tt.authOpts) + if tt.expectErr != "" { + g.Expect(tt.expectErr).To(Equal(err.Error())) + return + } + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(cfg.User).To(Equal(tt.expectedUsername)) + g.Expect(len(cfg.Auth)).To(Equal(tt.expectedAuthLen)) + }) + } +} + +func TestSSHManagedTransport_E2E(t *testing.T) { + g := NewWithT(t) + + server, err := gittestserver.NewTempGitServer() + g.Expect(err).ToNot(HaveOccurred()) + defer os.RemoveAll(server.Root()) + + server.KeyDir(filepath.Join(server.Root(), "keys")) + + err = server.ListenSSH() + g.Expect(err).ToNot(HaveOccurred()) + + go func() { + server.StartSSH() + }() + defer server.StopSSH() + InitManagedTransport(logr.Discard()) + + kp, err := ssh.NewEd25519Generator().Generate() + g.Expect(err).ToNot(HaveOccurred()) + + repoPath := "test.git" + err = server.InitRepo("../../testdata/git/repo", git.DefaultBranch, repoPath) + g.Expect(err).ToNot(HaveOccurred()) + + transportID := "ssh://git@fake-url" + sshAddress := server.SSHAddress() + "/" + repoPath + AddTransportOptions(transportID, TransportOptions{ + TargetURL: sshAddress, + AuthOpts: &git.AuthOptions{ + Username: "user", + Identity: kp.PrivateKey, + }, + }) + + tmpDir := t.TempDir() + + // We call git2go.Clone with transportID, so that the managed ssh transport can + // fetch the correct set of credentials and the actual target url as well. + repo, err := git2go.Clone(transportID, tmpDir, &git2go.CloneOptions{ + FetchOptions: git2go.FetchOptions{}, + CheckoutOptions: git2go.CheckoutOptions{ + Strategy: git2go.CheckoutForce, + }, + }) + + g.Expect(err).ToNot(HaveOccurred()) + repo.Free() +} diff --git a/pkg/git/libgit2/managed_test.go b/pkg/git/libgit2/managed_test.go index 0d812a23c..728c61fe5 100644 --- a/pkg/git/libgit2/managed_test.go +++ b/pkg/git/libgit2/managed_test.go @@ -19,6 +19,7 @@ package libgit2 import ( "context" "fmt" + "math/rand" "net/url" "os" "path/filepath" @@ -36,7 +37,6 @@ import ( . "github.com/onsi/gomega" cryptossh "golang.org/x/crypto/ssh" - corev1 "k8s.io/api/core/v1" ) const testRepositoryPath = "../testdata/git/repo" @@ -50,12 +50,36 @@ func Test_ManagedSSH_KeyTypes(t *testing.T) { authorized bool wantErr string }{ - {name: "RSA 4096", keyType: ssh.RSA_4096, authorized: true}, - {name: "ECDSA P256", keyType: ssh.ECDSA_P256, authorized: true}, - {name: "ECDSA P384", keyType: ssh.ECDSA_P384, authorized: true}, - {name: "ECDSA P521", keyType: ssh.ECDSA_P521, authorized: true}, - {name: "ED25519", keyType: ssh.ED25519, authorized: true}, - {name: "unauthorized key", keyType: ssh.RSA_4096, wantErr: "Failed to retrieve list of SSH authentication methods"}, + { + name: "RSA 4096", + keyType: ssh.RSA_4096, + authorized: true, + }, + { + name: "ECDSA P256", + keyType: ssh.ECDSA_P256, + authorized: true, + }, + { + name: "ECDSA P384", + keyType: ssh.ECDSA_P384, + authorized: true, + }, + { + name: "ECDSA P521", + keyType: ssh.ECDSA_P521, + authorized: true, + }, + { + name: "ED25519", + keyType: ssh.ED25519, + authorized: true, + }, + { + name: "unauthorized key", + keyType: ssh.RSA_4096, + wantErr: "unable to authenticate, attempted methods [none publickey], no supported methods remain", + }, } serverRootDir := t.TempDir() @@ -99,6 +123,9 @@ func Test_ManagedSSH_KeyTypes(t *testing.T) { knownHosts, err := ssh.ScanHostKey(u.Host, timeout, git.HostKeyAlgos, false) g.Expect(err).ToNot(HaveOccurred()) + os.Setenv("EXPERIMENTAL_GIT_TRANSPORT", "true") + managed.InitManagedTransport(logr.Discard()) + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) @@ -112,15 +139,21 @@ func Test_ManagedSSH_KeyTypes(t *testing.T) { authorizedPublicKey = string(kp.PublicKey) } - secret := corev1.Secret{ - Data: map[string][]byte{ - "identity": kp.PrivateKey, - "known_hosts": knownHosts, - }, + // secret := corev1.Secret{ + // Data: map[string][]byte{ + // "identity": kp.PrivateKey, + // "known_hosts": knownHosts, + // }, + // } + // + // authOpts, err := git.AuthOptionsFromSecret(repoURL, &secret) + // g.Expect(err).ToNot(HaveOccurred()) + + authOpts := &git.AuthOptions{ + Identity: kp.PrivateKey, + KnownHosts: knownHosts, } - - authOpts, err := git.AuthOptionsFromSecret(repoURL, &secret) - g.Expect(err).ToNot(HaveOccurred()) + authOpts.TransportAuthID = "ssh://" + getTransportAuthID() // Prepare for checkout. branchCheckoutStrat := &CheckoutBranch{Branch: git.DefaultBranch} @@ -200,6 +233,9 @@ func Test_ManagedSSH_KeyExchangeAlgos(t *testing.T) { }, } + os.Setenv("EXPERIMENTAL_GIT_TRANSPORT", "true") + managed.InitManagedTransport(logr.Discard()) + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) @@ -223,8 +259,6 @@ func Test_ManagedSSH_KeyExchangeAlgos(t *testing.T) { }() defer server.StopSSH() - os.Setenv("EXPERIMENTAL_GIT_TRANSPORT", "true") - managed.InitManagedTransport(logr.Discard()) repoPath := "test.git" err := server.InitRepo(testRepositoryPath, git.DefaultBranch, repoPath) @@ -246,15 +280,20 @@ func Test_ManagedSSH_KeyExchangeAlgos(t *testing.T) { kp, err := ssh.GenerateKeyPair(ssh.ED25519) g.Expect(err).ToNot(HaveOccurred()) - secret := corev1.Secret{ - Data: map[string][]byte{ - "identity": kp.PrivateKey, - "known_hosts": knownHosts, - }, + // secret := corev1.Secret{ + // Data: map[string][]byte{ + // "identity": kp.PrivateKey, + // "known_hosts": knownHosts, + // }, + // } + // + // authOpts, err := git.AuthOptionsFromSecret(repoURL, &secret) + // g.Expect(err).ToNot(HaveOccurred()) + authOpts := &git.AuthOptions{ + Identity: kp.PrivateKey, + KnownHosts: knownHosts, } - - authOpts, err := git.AuthOptionsFromSecret(repoURL, &secret) - g.Expect(err).ToNot(HaveOccurred()) + authOpts.TransportAuthID = "ssh://" + getTransportAuthID() // Prepare for checkout. branchCheckoutStrat := &CheckoutBranch{Branch: git.DefaultBranch} @@ -363,6 +402,9 @@ func Test_ManagedSSH_HostKeyAlgos(t *testing.T) { }, } + os.Setenv("EXPERIMENTAL_GIT_TRANSPORT", "true") + managed.InitManagedTransport(logr.Discard()) + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) @@ -396,8 +438,6 @@ func Test_ManagedSSH_HostKeyAlgos(t *testing.T) { }() defer server.StopSSH() - os.Setenv("EXPERIMENTAL_GIT_TRANSPORT", "true") - managed.InitManagedTransport(logr.Discard()) repoPath := "test.git" err = server.InitRepo(testRepositoryPath, git.DefaultBranch, repoPath) @@ -419,15 +459,20 @@ func Test_ManagedSSH_HostKeyAlgos(t *testing.T) { kp, err := ssh.GenerateKeyPair(ssh.ED25519) g.Expect(err).ToNot(HaveOccurred()) - secret := corev1.Secret{ - Data: map[string][]byte{ - "identity": kp.PrivateKey, - "known_hosts": knownHosts, - }, + // secret := corev1.Secret{ + // Data: map[string][]byte{ + // "identity": kp.PrivateKey, + // "known_hosts": knownHosts, + // }, + // } + // + // authOpts, err := git.AuthOptionsFromSecret(repoURL, &secret) + // g.Expect(err).ToNot(HaveOccurred()) + authOpts := &git.AuthOptions{ + Identity: kp.PrivateKey, + KnownHosts: knownHosts, } - - authOpts, err := git.AuthOptionsFromSecret(repoURL, &secret) - g.Expect(err).ToNot(HaveOccurred()) + authOpts.TransportAuthID = "ssh://" + getTransportAuthID() // Prepare for checkout. branchCheckoutStrat := &CheckoutBranch{Branch: git.DefaultBranch} @@ -442,3 +487,12 @@ func Test_ManagedSSH_HostKeyAlgos(t *testing.T) { }) } } + +func getTransportAuthID() string { + letterRunes := []rune("abcdefghijklmnopqrstuvwxyz1234567890") + b := make([]rune, 10) + for i := range b { + b[i] = letterRunes[rand.Intn(len(letterRunes))] + } + return string(b) +} diff --git a/pkg/git/libgit2/transport.go b/pkg/git/libgit2/transport.go index 592c53014..e7c9671c0 100644 --- a/pkg/git/libgit2/transport.go +++ b/pkg/git/libgit2/transport.go @@ -38,7 +38,6 @@ import ( "golang.org/x/crypto/ssh/knownhosts" "github.com/fluxcd/source-controller/pkg/git" - "github.com/fluxcd/source-controller/pkg/git/libgit2/managed" ) var ( @@ -115,18 +114,6 @@ func pushTransferProgressCallback(ctx context.Context) git2go.PushTransferProgre func credentialsCallback(opts *git.AuthOptions) git2go.CredentialsCallback { return func(url string, username string, allowedTypes git2go.CredentialType) (*git2go.Credential, error) { if allowedTypes&(git2go.CredentialTypeSSHKey|git2go.CredentialTypeSSHCustom|git2go.CredentialTypeSSHMemory) != 0 { - if managed.Enabled() { - // CredentialTypeSSHMemory requires libgit2 to be built using libssh2. - // When using managed transport (handled in go instead of libgit2), - // there may be ways to remove such requirement, thefore decreasing the - // need of libz, libssh2 and OpenSSL but further investigation is required - // once Managed Transport is no longer experimental. - // - // CredentialSSHKeyFromMemory is currently required for SSH key access - // when managed transport is enabled. - return git2go.NewCredentialSSHKeyFromMemory(opts.Username, "", string(opts.Identity), opts.Password) - } - var ( signer ssh.Signer err error diff --git a/pkg/git/options.go b/pkg/git/options.go index ff1bccac1..81bbd6ce9 100644 --- a/pkg/git/options.go +++ b/pkg/git/options.go @@ -72,6 +72,11 @@ type AuthOptions struct { Identity []byte KnownHosts []byte CAFile []byte + // TransportAuthID is a unique identifier for this set of authentication + // options. It's used by managed libgit2 transports to uniquely identify + // which credentials to use for a particular git operation, and avoid misuse + // of credentials in a multi tenant environment. + TransportAuthID string } // KexAlgos hosts the key exchange algorithms to be used for SSH connections. From 7d2bc64f47f1439bbcc1a9e2cf88294a837786dd Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Thu, 26 May 2022 13:34:19 +0530 Subject: [PATCH 2/8] fix panics on unmanaged http and proxy on managed http Signed-off-by: Sanskar Jaiswal --- controllers/gitrepository_controller.go | 44 +- controllers/gitrepository_controller_test.go | 18 +- go.mod | 2 +- go.sum | 4 +- main.go | 8 + pkg/git/libgit2/checkout.go | 410 +++++++++++-------- pkg/git/libgit2/checkout_test.go | 116 ++---- pkg/git/libgit2/managed/http.go | 100 +++-- pkg/git/libgit2/managed/http_test.go | 100 ++--- pkg/git/libgit2/managed/managed_test.go | 48 --- pkg/git/libgit2/managed/options.go | 35 +- pkg/git/libgit2/managed/ssh.go | 17 +- pkg/git/libgit2/managed/ssh_test.go | 12 +- pkg/git/libgit2/managed/transport.go | 103 +++++ pkg/git/libgit2/managed/transport_test.go | 108 +++++ pkg/git/libgit2/managed_test.go | 192 +++++++-- pkg/git/libgit2/transport.go | 169 +------- pkg/git/libgit2/transport_test.go | 242 ----------- pkg/git/options.go | 13 +- 19 files changed, 845 insertions(+), 896 deletions(-) delete mode 100644 pkg/git/libgit2/managed/managed_test.go create mode 100644 pkg/git/libgit2/managed/transport.go create mode 100644 pkg/git/libgit2/managed/transport_test.go diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index ae922ff36..04e404451 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -455,21 +455,6 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, return sreconcile.ResultEmpty, e } - repositoryURL := obj.Spec.URL - // managed GIT transport only affects the libgit2 implementation - if managed.Enabled() && obj.Spec.GitImplementation == sourcev1.LibGit2Implementation { - // We set the TransportAuthID of this set of authentication options here by constructing - // a unique ID that won't clash in a multi tenant environment. This unique ID is used by - // libgit2 managed transports. This enables us to bypass the inbuilt credentials callback in - // libgit2, which is inflexible and unstable. - if strings.HasPrefix(repositoryURL, "http") { - authOpts.TransportAuthID = fmt.Sprintf("http://%s/%s/%d", obj.Name, obj.UID, obj.Generation) - } - if strings.HasPrefix(repositoryURL, "ssh") { - authOpts.TransportAuthID = fmt.Sprintf("ssh://%s/%s/%d", obj.Name, obj.UID, obj.Generation) - } - } - // Fetch the included artifact metadata. artifacts, err := r.fetchIncludes(ctx, obj) if err != nil { @@ -491,7 +476,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, optimizedClone = true } - c, err := r.gitCheckout(ctx, obj, repositoryURL, authOpts, dir, optimizedClone) + c, err := r.gitCheckout(ctx, obj, authOpts, dir, optimizedClone) if err != nil { return sreconcile.ResultEmpty, err } @@ -525,7 +510,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, // If we can't skip the reconciliation, checkout again without any // optimization. - c, err := r.gitCheckout(ctx, obj, repositoryURL, authOpts, dir, false) + c, err := r.gitCheckout(ctx, obj, authOpts, dir, false) if err != nil { return sreconcile.ResultEmpty, err } @@ -717,7 +702,7 @@ func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, // gitCheckout builds checkout options with the given configurations and // performs a git checkout. func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context, - obj *sourcev1.GitRepository, repoURL string, authOpts *git.AuthOptions, dir string, optimized bool) (*git.Commit, error) { + obj *sourcev1.GitRepository, authOpts *git.AuthOptions, dir string, optimized bool) (*git.Commit, error) { // Configure checkout strategy. checkoutOpts := git.CheckoutOptions{RecurseSubmodules: obj.Spec.RecurseSubmodules} if ref := obj.Spec.Reference; ref != nil { @@ -743,15 +728,34 @@ func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context, Err: fmt.Errorf("failed to configure checkout strategy for Git implementation '%s': %w", obj.Spec.GitImplementation, err), Reason: sourcev1.GitOperationFailedReason, } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) // Do not return err as recovery without changes is impossible. return nil, e } + // managed GIT transport only affects the libgit2 implementation + if managed.Enabled() && obj.Spec.GitImplementation == sourcev1.LibGit2Implementation { + // We set the TransportOptionsURL of this set of authentication options here by constructing + // a unique ID that won't clash in a multi tenant environment. This unique ID is used by + // libgit2 managed transports. This enables us to bypass the inbuilt credentials callback in + // libgit2, which is inflexible and unstable. + if strings.HasPrefix(obj.Spec.URL, "http") { + authOpts.TransportOptionsURL = fmt.Sprintf("http://%s/%s/%d", obj.Name, obj.UID, obj.Generation) + } else if strings.HasPrefix(obj.Spec.URL, "ssh") { + authOpts.TransportOptionsURL = fmt.Sprintf("ssh://%s/%s/%d", obj.Name, obj.UID, obj.Generation) + } else { + e := &serror.Stalling{ + Err: fmt.Errorf("git repository URL has invalid transport type: '%s'", obj.Spec.URL), + Reason: sourcev1.GitOperationFailedReason, + } + return nil, e + } + } + // Checkout HEAD of reference in object gitCtx, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration) defer cancel() - commit, err := checkoutStrategy.Checkout(gitCtx, dir, repoURL, authOpts) + + commit, err := checkoutStrategy.Checkout(gitCtx, dir, obj.Spec.URL, authOpts) if err != nil { e := serror.NewGeneric( fmt.Errorf("failed to checkout and determine revision: %w", err), diff --git a/controllers/gitrepository_controller_test.go b/controllers/gitrepository_controller_test.go index 50a9463fe..a30f608b0 100644 --- a/controllers/gitrepository_controller_test.go +++ b/controllers/gitrepository_controller_test.go @@ -362,7 +362,7 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { }, wantErr: true, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.GitOperationFailedReason, "failed to checkout and determine revision: unable to fetch-connect to remote '': PEM CA bundle could not be appended to x509 certificate pool"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.GitOperationFailedReason, "failed to checkout and determine revision: unable to clone '': PEM CA bundle could not be appended to x509 certificate pool"), }, }, { @@ -645,10 +645,11 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) } conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "foo") }, - want: sreconcile.ResultEmpty, - wantErr: true, - wantRevision: "staging/", - wantArtifactOutdated: false, + want: sreconcile.ResultEmpty, + wantErr: true, + wantRevision: "staging/", + wantArtifactOutdated: false, + skipForImplementation: "libgit2", }, { name: "Optimized clone different ignore", @@ -669,9 +670,10 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) } conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "foo") }, - want: sreconcile.ResultSuccess, - wantRevision: "staging/", - wantArtifactOutdated: false, + want: sreconcile.ResultSuccess, + wantRevision: "staging/", + wantArtifactOutdated: false, + skipForImplementation: "libgit2", }, } diff --git a/go.mod b/go.mod index 4f00f6f4b..52dc835bd 100644 --- a/go.mod +++ b/go.mod @@ -28,7 +28,7 @@ require ( github.com/fluxcd/pkg/helmtestserver v0.7.2 github.com/fluxcd/pkg/lockedfile v0.1.0 github.com/fluxcd/pkg/runtime v0.16.1 - github.com/fluxcd/pkg/ssh v0.3.4 + github.com/fluxcd/pkg/ssh v0.4.0 github.com/fluxcd/pkg/testserver v0.2.0 github.com/fluxcd/pkg/untar v0.1.0 github.com/fluxcd/pkg/version v0.1.0 diff --git a/go.sum b/go.sum index c59b79563..4dfcd2597 100644 --- a/go.sum +++ b/go.sum @@ -282,8 +282,8 @@ github.com/fluxcd/pkg/lockedfile v0.1.0 h1:YsYFAkd6wawMCcD74ikadAKXA4s2sukdxrn7w github.com/fluxcd/pkg/lockedfile v0.1.0/go.mod h1:EJLan8t9MiOcgTs8+puDjbE6I/KAfHbdvIy9VUgIjm8= github.com/fluxcd/pkg/runtime v0.16.1 h1:WU1vNZz4TAzmATQ/tl2zB/FX6GIUTgYeBn/G5RuTA2c= github.com/fluxcd/pkg/runtime v0.16.1/go.mod h1:cgVJkOXCg9OmrIUGklf/0UtV28MNzkuoBJhaEQICT6E= -github.com/fluxcd/pkg/ssh v0.3.4 h1:Ko+MUNiiQG3evyoMO19iRk7d4X0VJ6w6+GEeVQ1jLC0= -github.com/fluxcd/pkg/ssh v0.3.4/go.mod h1:KGgOUOy1uI6RC6+qxIBLvP1AeOOs/nLB25Ca6TZMIXE= +github.com/fluxcd/pkg/ssh v0.4.0 h1:2HY88irZ5BCSMlzZExR6cnhRkjxCDsK/lTHHQqCJDJQ= +github.com/fluxcd/pkg/ssh v0.4.0/go.mod h1:KGgOUOy1uI6RC6+qxIBLvP1AeOOs/nLB25Ca6TZMIXE= github.com/fluxcd/pkg/testserver v0.2.0 h1:Mj0TapmKaywI6Fi5wvt1LAZpakUHmtzWQpJNKQ0Krt4= github.com/fluxcd/pkg/testserver v0.2.0/go.mod h1:bgjjydkXsZTeFzjz9Cr4heGANr41uTB1Aj1Q5qzuYVk= github.com/fluxcd/pkg/untar v0.1.0 h1:k97V/xV5hFrAkIkVPuv5AVhyxh1ZzzAKba/lbDfGo6o= diff --git a/main.go b/main.go index 50a6bc559..fb54cb74a 100644 --- a/main.go +++ b/main.go @@ -312,6 +312,14 @@ func main() { if enabled, _ := features.Enabled(features.GitManagedTransport); enabled { managed.InitManagedTransport(ctrl.Log.WithName("managed-transport")) + } else { + if optimize, _ := feathelper.Enabled(features.OptimizedGitClones); optimize { + setupLog.Error( + fmt.Errorf("OptimizedGitClones=true but GitManagedTransport=false"), + "git clones can only be optimized when using managed transort", + ) + os.Exit(1) + } } setupLog.Info("starting manager") diff --git a/pkg/git/libgit2/checkout.go b/pkg/git/libgit2/checkout.go index 56752951d..83c602f81 100644 --- a/pkg/git/libgit2/checkout.go +++ b/pkg/git/libgit2/checkout.go @@ -69,113 +69,148 @@ type CheckoutBranch struct { func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, opts *git.AuthOptions) (_ *git.Commit, err error) { defer recoverPanic(&err) + // This branching is temporary, to address the transient panics observed when using unmanaged transport. + // The panics probably happen because we perform multiple fetch ops (introduced as a part of optimizing git clones). + // The branching lets us establish a clear code path to help us be certain of the expected behaviour. + // When we get rid of unmanaged transports, we can get rid of this branching as well. if managed.Enabled() { - // We store the target url and auth options mapped to a unique ID. We overwrite the target url - // with the TransportAuthID, because managed transports don't provide a way for any kind of + // We store the target URL and auth options mapped to a unique ID. We overwrite the target URL + // with the TransportOptionsURL, because managed transports don't provide a way for any kind of // dependency injection. This lets us have a way of doing interop between application level code // and transport level code. - // Performing all fetch operations with the TransportAuthID as the url, lets the managed + // Performing all fetch operations with the TransportOptionsURL as the URL, lets the managed // transport action use it to fetch the registered transport options which contains the - // _actual_ target url and the correct credentials to use. - managed.AddTransportOptions(opts.TransportAuthID, managed.TransportOptions{ - TargetURL: url, - AuthOpts: opts, + // _actual_ target URL and the correct credentials to use. + if opts == nil { + return nil, fmt.Errorf("can't use managed transport with an empty set of auth options") + } + if opts.TransportOptionsURL == "" { + return nil, fmt.Errorf("can't use managed transport without a valid transport auth id.") + } + managed.AddTransportOptions(opts.TransportOptionsURL, managed.TransportOptions{ + TargetURL: url, + AuthOpts: opts, + ProxyOptions: &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, }) - url = opts.TransportAuthID - defer managed.RemoveTransportOptions(opts.TransportAuthID) - } + url = opts.TransportOptionsURL + remoteCallBacks := managed.RemoteCallbacks() + defer managed.RemoveTransportOptions(opts.TransportOptionsURL) - remoteCallBacks := RemoteCallbacks(ctx, opts) - proxyOpts := &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto} - - repo, remote, err := initializeRepoWithRemote(ctx, path, url, opts) - if err != nil { - return nil, err - } - // Open remote connection. - err = remote.ConnectFetch(&remoteCallBacks, proxyOpts, nil) - if err != nil { - remote.Free() - repo.Free() - return nil, fmt.Errorf("unable to fetch-connect to remote '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) - } - defer func() { - remote.Disconnect() - remote.Free() - repo.Free() - }() - - // When the last observed revision is set, check whether it is still the - // same at the remote branch. If so, short-circuit the clone operation here. - if c.LastRevision != "" { - heads, err := remote.Ls(c.Branch) + repo, remote, err := initializeRepoWithRemote(ctx, path, url, opts) if err != nil { - return nil, fmt.Errorf("unable to remote ls for '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) - } - if len(heads) > 0 { - hash := heads[0].Id.String() - currentRevision := fmt.Sprintf("%s/%s", c.Branch, hash) - if currentRevision == c.LastRevision { - // Construct a partial commit with the existing information. - c := &git.Commit{ - Hash: git.Hash(hash), - Reference: "refs/heads/" + c.Branch, + return nil, err + } + // Open remote connection. + err = remote.ConnectFetch(&remoteCallBacks, nil, nil) + if err != nil { + remote.Free() + repo.Free() + return nil, fmt.Errorf("unable to fetch-connect to remote '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) + } + defer func() { + remote.Disconnect() + remote.Free() + repo.Free() + }() + + // When the last observed revision is set, check whether it is still the + // same at the remote branch. If so, short-circuit the clone operation here. + if c.LastRevision != "" { + heads, err := remote.Ls(c.Branch) + if err != nil { + return nil, fmt.Errorf("unable to remote ls for '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) + } + if len(heads) > 0 { + hash := heads[0].Id.String() + currentRevision := fmt.Sprintf("%s/%s", c.Branch, hash) + if currentRevision == c.LastRevision { + // Construct a partial commit with the existing information. + c := &git.Commit{ + Hash: git.Hash(hash), + Reference: "refs/heads/" + c.Branch, + } + return c, nil } - return c, nil } } - } - // Limit the fetch operation to the specific branch, to decrease network usage. - err = remote.Fetch([]string{c.Branch}, - &git2go.FetchOptions{ - DownloadTags: git2go.DownloadTagsNone, - RemoteCallbacks: remoteCallBacks, - ProxyOptions: git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, - }, - "") - if err != nil { - return nil, fmt.Errorf("unable to fetch remote '%s': %w", - managed.EffectiveURL(url), gitutil.LibGit2Error(err)) - } + // Limit the fetch operation to the specific branch, to decrease network usage. + err = remote.Fetch([]string{c.Branch}, + &git2go.FetchOptions{ + DownloadTags: git2go.DownloadTagsNone, + RemoteCallbacks: remoteCallBacks, + }, + "") + if err != nil { + return nil, fmt.Errorf("unable to fetch remote '%s': %w", + managed.EffectiveURL(url), gitutil.LibGit2Error(err)) + } - branch, err := repo.References.Lookup(fmt.Sprintf("refs/remotes/origin/%s", c.Branch)) - if err != nil { - return nil, fmt.Errorf("unable to lookup branch '%s' for '%s': %w", - c.Branch, managed.EffectiveURL(url), gitutil.LibGit2Error(err)) - } - defer branch.Free() + branch, err := repo.References.Lookup(fmt.Sprintf("refs/remotes/origin/%s", c.Branch)) + if err != nil { + return nil, fmt.Errorf("unable to lookup branch '%s' for '%s': %w", + c.Branch, managed.EffectiveURL(url), gitutil.LibGit2Error(err)) + } + defer branch.Free() - upstreamCommit, err := repo.LookupCommit(branch.Target()) - if err != nil { - return nil, fmt.Errorf("unable to lookup commit '%s' for '%s': %w", - c.Branch, managed.EffectiveURL(url), gitutil.LibGit2Error(err)) - } - defer upstreamCommit.Free() + upstreamCommit, err := repo.LookupCommit(branch.Target()) + if err != nil { + return nil, fmt.Errorf("unable to lookup commit '%s' for '%s': %w", + c.Branch, managed.EffectiveURL(url), gitutil.LibGit2Error(err)) + } + defer upstreamCommit.Free() - // Once the index has been updated with Fetch, and we know the tip commit, - // a hard reset can be used to align the local worktree with the remote branch's. - err = repo.ResetToCommit(upstreamCommit, git2go.ResetHard, &git2go.CheckoutOptions{ - Strategy: git2go.CheckoutForce, - }) - if err != nil { - return nil, fmt.Errorf("unable to hard reset to commit for '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) - } + // Once the index has been updated with Fetch, and we know the tip commit, + // a hard reset can be used to align the local worktree with the remote branch's. + err = repo.ResetToCommit(upstreamCommit, git2go.ResetHard, &git2go.CheckoutOptions{ + Strategy: git2go.CheckoutForce, + }) + if err != nil { + return nil, fmt.Errorf("unable to hard reset to commit for '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) + } - // Use the current worktree's head as reference for the commit to be returned. - head, err := repo.Head() - if err != nil { - return nil, fmt.Errorf("git resolve HEAD error: %w", err) - } - defer head.Free() + // Use the current worktree's head as reference for the commit to be returned. + head, err := repo.Head() + if err != nil { + return nil, fmt.Errorf("git resolve HEAD error: %w", err) + } + defer head.Free() - cc, err := repo.LookupCommit(head.Target()) - if err != nil { - return nil, fmt.Errorf("failed to lookup HEAD commit '%s' for branch '%s': %w", head.Target(), c.Branch, err) + cc, err := repo.LookupCommit(head.Target()) + if err != nil { + return nil, fmt.Errorf("failed to lookup HEAD commit '%s' for branch '%s': %w", head.Target(), c.Branch, err) + } + defer cc.Free() + + return buildCommit(cc, "refs/heads/"+c.Branch), nil + } else { + repo, err := git2go.Clone(url, path, &git2go.CloneOptions{ + FetchOptions: git2go.FetchOptions{ + DownloadTags: git2go.DownloadTagsNone, + RemoteCallbacks: RemoteCallbacks(ctx, opts), + ProxyOptions: git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, + }, + CheckoutOptions: git2go.CheckoutOptions{ + Strategy: git2go.CheckoutForce, + }, + CheckoutBranch: c.Branch, + }) + if err != nil { + return nil, fmt.Errorf("unable to clone '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) + } + defer repo.Free() + head, err := repo.Head() + if err != nil { + return nil, fmt.Errorf("git resolve HEAD error: %w", err) + } + defer head.Free() + cc, err := repo.LookupCommit(head.Target()) + if err != nil { + return nil, fmt.Errorf("failed to lookup HEAD commit '%s' for branch '%s': %w", head.Target(), c.Branch, err) + } + defer cc.Free() + return buildCommit(cc, "refs/heads/"+c.Branch), nil } - defer cc.Free() - - return buildCommit(cc, "refs/heads/"+c.Branch), nil } type CheckoutTag struct { @@ -186,85 +221,108 @@ type CheckoutTag struct { func (c *CheckoutTag) Checkout(ctx context.Context, path, url string, opts *git.AuthOptions) (_ *git.Commit, err error) { defer recoverPanic(&err) + // This branching is temporary, to address the transient panics observed when using unmanaged transport. + // The panics probably happen because we perform multiple fetch ops (introduced as a part of optimizing git clones). + // The branching lets us establish a clear code path to help us be certain of the expected behaviour. + // When we get rid of unmanaged transports, we can get rid of this branching as well. if managed.Enabled() { - managed.AddTransportOptions(opts.TransportAuthID, managed.TransportOptions{ - TargetURL: url, - AuthOpts: opts, + if opts.TransportOptionsURL == "" { + return nil, fmt.Errorf("can't use managed transport without a valid transport auth id.") + } + managed.AddTransportOptions(opts.TransportOptionsURL, managed.TransportOptions{ + TargetURL: url, + AuthOpts: opts, + ProxyOptions: &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, }) - url = opts.TransportAuthID - defer managed.RemoveTransportOptions(opts.TransportAuthID) - } + url = opts.TransportOptionsURL + remoteCallBacks := managed.RemoteCallbacks() + defer managed.RemoveTransportOptions(opts.TransportOptionsURL) - remoteCallBacks := RemoteCallbacks(ctx, opts) - proxyOpts := &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto} - - repo, remote, err := initializeRepoWithRemote(ctx, path, url, opts) - if err != nil { - return nil, err - } - // Open remote connection. - err = remote.ConnectFetch(&remoteCallBacks, proxyOpts, nil) - if err != nil { - remote.Free() - repo.Free() - return nil, fmt.Errorf("unable to fetch-connect to remote '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) - } - defer func() { - remote.Disconnect() - remote.Free() - repo.Free() - }() - - // When the last observed revision is set, check whether it is still the - // same at the remote branch. If so, short-circuit the clone operation here. - if c.LastRevision != "" { - heads, err := remote.Ls(c.Tag) + repo, remote, err := initializeRepoWithRemote(ctx, path, url, opts) if err != nil { - return nil, fmt.Errorf("unable to remote ls for '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) - } - if len(heads) > 0 { - hash := heads[0].Id.String() - currentRevision := fmt.Sprintf("%s/%s", c.Tag, hash) - var same bool - if currentRevision == c.LastRevision { - same = true - } else if len(heads) > 1 { - hash = heads[1].Id.String() - currentAnnotatedRevision := fmt.Sprintf("%s/%s", c.Tag, hash) - if currentAnnotatedRevision == c.LastRevision { + return nil, err + } + // Open remote connection. + err = remote.ConnectFetch(&remoteCallBacks, nil, nil) + if err != nil { + remote.Free() + repo.Free() + return nil, fmt.Errorf("unable to fetch-connect to remote '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) + } + defer func() { + remote.Disconnect() + remote.Free() + repo.Free() + }() + + // When the last observed revision is set, check whether it is still the + // same at the remote branch. If so, short-circuit the clone operation here. + if c.LastRevision != "" { + heads, err := remote.Ls(c.Tag) + if err != nil { + return nil, fmt.Errorf("unable to remote ls for '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) + } + if len(heads) > 0 { + hash := heads[0].Id.String() + currentRevision := fmt.Sprintf("%s/%s", c.Tag, hash) + var same bool + if currentRevision == c.LastRevision { same = true + } else if len(heads) > 1 { + hash = heads[1].Id.String() + currentAnnotatedRevision := fmt.Sprintf("%s/%s", c.Tag, hash) + if currentAnnotatedRevision == c.LastRevision { + same = true + } } - } - if same { - // Construct a partial commit with the existing information. - c := &git.Commit{ - Hash: git.Hash(hash), - Reference: "refs/tags/" + c.Tag, + if same { + // Construct a partial commit with the existing information. + c := &git.Commit{ + Hash: git.Hash(hash), + Reference: "refs/tags/" + c.Tag, + } + return c, nil } - return c, nil } } - } - err = remote.Fetch([]string{c.Tag}, - &git2go.FetchOptions{ - DownloadTags: git2go.DownloadTagsAuto, - RemoteCallbacks: remoteCallBacks, - ProxyOptions: *proxyOpts, - }, - "") + err = remote.Fetch([]string{c.Tag}, + &git2go.FetchOptions{ + DownloadTags: git2go.DownloadTagsAuto, + RemoteCallbacks: remoteCallBacks, + }, + "") - if err != nil { - return nil, fmt.Errorf("unable to fetch remote '%s': %w", - managed.EffectiveURL(url), gitutil.LibGit2Error(err)) - } + if err != nil { + return nil, fmt.Errorf("unable to fetch remote '%s': %w", + managed.EffectiveURL(url), gitutil.LibGit2Error(err)) + } - cc, err := checkoutDetachedDwim(repo, c.Tag) - if err != nil { - return nil, err + cc, err := checkoutDetachedDwim(repo, c.Tag) + if err != nil { + return nil, err + } + defer cc.Free() + return buildCommit(cc, "refs/tags/"+c.Tag), nil + } else { + repo, err := git2go.Clone(url, path, &git2go.CloneOptions{ + FetchOptions: git2go.FetchOptions{ + DownloadTags: git2go.DownloadTagsAll, + RemoteCallbacks: RemoteCallbacks(ctx, opts), + ProxyOptions: git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, + }, + }) + if err != nil { + return nil, fmt.Errorf("unable to clone '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) + } + defer repo.Free() + cc, err := checkoutDetachedDwim(repo, c.Tag) + if err != nil { + return nil, err + } + defer cc.Free() + return buildCommit(cc, "refs/tags/"+c.Tag), nil } - defer cc.Free() - return buildCommit(cc, "refs/tags/"+c.Tag), nil } type CheckoutCommit struct { @@ -274,20 +332,26 @@ type CheckoutCommit struct { func (c *CheckoutCommit) Checkout(ctx context.Context, path, url string, opts *git.AuthOptions) (_ *git.Commit, err error) { defer recoverPanic(&err) + remoteCallBacks := RemoteCallbacks(ctx, opts) + if managed.Enabled() { - managed.AddTransportOptions(opts.TransportAuthID, managed.TransportOptions{ - TargetURL: url, - AuthOpts: opts, + if opts.TransportOptionsURL == "" { + return nil, fmt.Errorf("can't use managed transport without a valid transport auth id.") + } + managed.AddTransportOptions(opts.TransportOptionsURL, managed.TransportOptions{ + TargetURL: url, + AuthOpts: opts, + ProxyOptions: &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, }) - url = opts.TransportAuthID - defer managed.RemoveTransportOptions(opts.TransportAuthID) + url = opts.TransportOptionsURL + remoteCallBacks = managed.RemoteCallbacks() + defer managed.RemoveTransportOptions(opts.TransportOptionsURL) } repo, err := git2go.Clone(url, path, &git2go.CloneOptions{ FetchOptions: git2go.FetchOptions{ DownloadTags: git2go.DownloadTagsNone, - RemoteCallbacks: RemoteCallbacks(ctx, opts), - ProxyOptions: git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, + RemoteCallbacks: remoteCallBacks, }, }) if err != nil { @@ -312,13 +376,20 @@ type CheckoutSemVer struct { func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, opts *git.AuthOptions) (_ *git.Commit, err error) { defer recoverPanic(&err) + remoteCallBacks := RemoteCallbacks(ctx, opts) + if managed.Enabled() { - managed.AddTransportOptions(opts.TransportAuthID, managed.TransportOptions{ - TargetURL: url, - AuthOpts: opts, + if opts.TransportOptionsURL == "" { + return nil, fmt.Errorf("can't use managed transport without a valid transport auth id.") + } + managed.AddTransportOptions(opts.TransportOptionsURL, managed.TransportOptions{ + TargetURL: url, + AuthOpts: opts, + ProxyOptions: &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, }) - url = opts.TransportAuthID - defer managed.RemoveTransportOptions(opts.TransportAuthID) + url = opts.TransportOptionsURL + remoteCallBacks = managed.RemoteCallbacks() + defer managed.RemoveTransportOptions(opts.TransportOptionsURL) } verConstraint, err := semver.NewConstraint(c.SemVer) @@ -329,8 +400,7 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, opts *g repo, err := git2go.Clone(url, path, &git2go.CloneOptions{ FetchOptions: git2go.FetchOptions{ DownloadTags: git2go.DownloadTagsAll, - RemoteCallbacks: RemoteCallbacks(ctx, opts), - ProxyOptions: git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, + RemoteCallbacks: remoteCallBacks, }, }) if err != nil { diff --git a/pkg/git/libgit2/checkout_test.go b/pkg/git/libgit2/checkout_test.go index b4f6c11d1..c2fe7a12c 100644 --- a/pkg/git/libgit2/checkout_test.go +++ b/pkg/git/libgit2/checkout_test.go @@ -77,49 +77,29 @@ func TestCheckoutBranch_Checkout(t *testing.T) { } tests := []struct { - name string - branch string - filesCreated map[string]string - lastRevision string - expectedCommit string - expectedConcreteCommit bool - expectedErr string + name string + branch string + filesCreated map[string]string + lastRevision string + expectedCommit string + expectedErr string }{ { - name: "Default branch", - branch: defaultBranch, - filesCreated: map[string]string{"branch": "second"}, - expectedCommit: secondCommit.String(), - expectedConcreteCommit: true, + name: "Default branch", + branch: defaultBranch, + filesCreated: map[string]string{"branch": "second"}, + expectedCommit: secondCommit.String(), }, { - name: "Other branch", - branch: "test", - filesCreated: map[string]string{"branch": "init"}, - expectedCommit: firstCommit.String(), - expectedConcreteCommit: true, + name: "Other branch", + branch: "test", + filesCreated: map[string]string{"branch": "init"}, + expectedCommit: firstCommit.String(), }, { - name: "Non existing branch", - branch: "invalid", - expectedErr: "reference 'refs/remotes/origin/invalid' not found", - expectedConcreteCommit: true, - }, - { - name: "skip clone - lastRevision hasn't changed", - branch: defaultBranch, - filesCreated: map[string]string{"branch": "second"}, - lastRevision: fmt.Sprintf("%s/%s", defaultBranch, secondCommit.String()), - expectedCommit: secondCommit.String(), - expectedConcreteCommit: false, - }, - { - name: "lastRevision is different", - branch: defaultBranch, - filesCreated: map[string]string{"branch": "second"}, - lastRevision: fmt.Sprintf("%s/%s", defaultBranch, firstCommit.String()), - expectedCommit: secondCommit.String(), - expectedConcreteCommit: true, + name: "Non existing branch", + branch: "invalid", + expectedErr: "reference 'refs/remotes/origin/invalid' not found", }, } @@ -142,14 +122,6 @@ func TestCheckoutBranch_Checkout(t *testing.T) { } g.Expect(err).ToNot(HaveOccurred()) g.Expect(cc.String()).To(Equal(tt.branch + "/" + tt.expectedCommit)) - g.Expect(git.IsConcreteCommit(*cc)).To(Equal(tt.expectedConcreteCommit)) - - if tt.expectedConcreteCommit { - for k, v := range tt.filesCreated { - g.Expect(filepath.Join(tmpDir, k)).To(BeARegularFile()) - g.Expect(os.ReadFile(filepath.Join(tmpDir, k))).To(BeEquivalentTo(v)) - } - } }) } } @@ -161,24 +133,20 @@ func TestCheckoutTag_Checkout(t *testing.T) { } tests := []struct { - name string - tagsInRepo []testTag - checkoutTag string - lastRevTag string - expectErr string - expectConcreteCommit bool + name string + tagsInRepo []testTag + checkoutTag string + expectErr string }{ { - name: "Tag", - tagsInRepo: []testTag{{"tag-1", false}}, - checkoutTag: "tag-1", - expectConcreteCommit: true, + name: "Tag", + tagsInRepo: []testTag{{"tag-1", false}}, + checkoutTag: "tag-1", }, { - name: "Annotated", - tagsInRepo: []testTag{{"annotated", true}}, - checkoutTag: "annotated", - expectConcreteCommit: true, + name: "Annotated", + tagsInRepo: []testTag{{"annotated", true}}, + checkoutTag: "annotated", }, { name: "Non existing tag", @@ -186,18 +154,14 @@ func TestCheckoutTag_Checkout(t *testing.T) { expectErr: "unable to find 'invalid': no reference found for shorthand 'invalid'", }, { - name: "Skip clone - last revision unchanged", - tagsInRepo: []testTag{{"tag-1", false}}, - checkoutTag: "tag-1", - lastRevTag: "tag-1", - expectConcreteCommit: false, + name: "Skip clone - last revision unchanged", + tagsInRepo: []testTag{{"tag-1", false}}, + checkoutTag: "tag-1", }, { - name: "Last revision changed", - tagsInRepo: []testTag{{"tag-1", false}, {"tag-2", false}}, - checkoutTag: "tag-2", - lastRevTag: "tag-1", - expectConcreteCommit: true, + name: "Last revision changed", + tagsInRepo: []testTag{{"tag-1", false}, {"tag-2", false}}, + checkoutTag: "tag-2", }, } for _, tt := range tests { @@ -235,12 +199,6 @@ func TestCheckoutTag_Checkout(t *testing.T) { checkoutTag := CheckoutTag{ Tag: tt.checkoutTag, } - // If last revision is provided, configure it. - if tt.lastRevTag != "" { - lc := tagCommits[tt.lastRevTag] - checkoutTag.LastRevision = fmt.Sprintf("%s/%s", tt.lastRevTag, lc.Id().String()) - } - tmpDir := t.TempDir() cc, err := checkoutTag.Checkout(context.TODO(), tmpDir, repo.Path(), nil) @@ -252,16 +210,12 @@ func TestCheckoutTag_Checkout(t *testing.T) { } // Check successful checkout results. - g.Expect(git.IsConcreteCommit(*cc)).To(Equal(tt.expectConcreteCommit)) targetTagCommit := tagCommits[tt.checkoutTag] g.Expect(err).ToNot(HaveOccurred()) g.Expect(cc.String()).To(Equal(tt.checkoutTag + "/" + targetTagCommit.Id().String())) - // Check file content only when there's an actual checkout. - if tt.lastRevTag != tt.checkoutTag { - g.Expect(filepath.Join(tmpDir, "tag")).To(BeARegularFile()) - g.Expect(os.ReadFile(filepath.Join(tmpDir, "tag"))).To(BeEquivalentTo(tt.checkoutTag)) - } + g.Expect(filepath.Join(tmpDir, "tag")).To(BeARegularFile()) + g.Expect(os.ReadFile(filepath.Join(tmpDir, "tag"))).To(BeEquivalentTo(tt.checkoutTag)) }) } } diff --git a/pkg/git/libgit2/managed/http.go b/pkg/git/libgit2/managed/http.go index ffccb1f69..1533e6bdd 100644 --- a/pkg/git/libgit2/managed/http.go +++ b/pkg/git/libgit2/managed/http.go @@ -47,6 +47,7 @@ import ( "bytes" "crypto/tls" "crypto/x509" + "encoding/base64" "errors" "fmt" "io" @@ -55,6 +56,7 @@ import ( "sync" pool "github.com/fluxcd/source-controller/internal/transport" + "github.com/fluxcd/source-controller/pkg/git" git2go "github.com/libgit2/git2go/v33" ) @@ -86,30 +88,45 @@ type httpSmartSubtransport struct { httpTransport *http.Transport } -func (t *httpSmartSubtransport) Action(transportAuthID string, action git2go.SmartServiceAction) (git2go.SmartSubtransportStream, error) { - var proxyFn func(*http.Request) (*url.URL, error) - proxyOpts, err := t.transport.SmartProxyOptions() - if err != nil { - return nil, err +func (t *httpSmartSubtransport) Action(transportOptionsURL string, action git2go.SmartServiceAction) (git2go.SmartSubtransportStream, error) { + opts, found := getTransportOptions(transportOptionsURL) + + if !found { + return nil, fmt.Errorf("failed to create client: could not find transport options for the object: %s", transportOptionsURL) } - switch proxyOpts.Type { - case git2go.ProxyTypeNone: - proxyFn = nil - case git2go.ProxyTypeAuto: - proxyFn = http.ProxyFromEnvironment - case git2go.ProxyTypeSpecified: - parsedUrl, err := url.Parse(proxyOpts.Url) - if err != nil { - return nil, err - } + targetURL := opts.TargetURL - proxyFn = http.ProxyURL(parsedUrl) + if targetURL == "" { + return nil, fmt.Errorf("repository URL cannot be empty") } - t.httpTransport.Proxy = proxyFn + if len(targetURL) > URLMaxLength { + return nil, fmt.Errorf("URL exceeds the max length (%d)", URLMaxLength) + } + + var proxyFn func(*http.Request) (*url.URL, error) + proxyOpts := opts.ProxyOptions + if proxyOpts != nil { + switch proxyOpts.Type { + case git2go.ProxyTypeNone: + proxyFn = nil + case git2go.ProxyTypeAuto: + proxyFn = http.ProxyFromEnvironment + case git2go.ProxyTypeSpecified: + parsedUrl, err := url.Parse(proxyOpts.Url) + if err != nil { + return nil, err + } + proxyFn = http.ProxyURL(parsedUrl) + } + t.httpTransport.Proxy = proxyFn + t.httpTransport.ProxyConnectHeader = map[string][]string{} + } else { + t.httpTransport.Proxy = nil + } t.httpTransport.DisableCompression = false - client, req, err := createClientRequest(transportAuthID, action, t.httpTransport) + client, req, err := createClientRequest(targetURL, action, t.httpTransport, opts.AuthOpts) if err != nil { return nil, err } @@ -142,7 +159,8 @@ func (t *httpSmartSubtransport) Action(transportAuthID string, action git2go.Sma return stream, nil } -func createClientRequest(transportAuthID string, action git2go.SmartServiceAction, t *http.Transport) (*http.Client, *http.Request, error) { +func createClientRequest(targetURL string, action git2go.SmartServiceAction, + t *http.Transport, authOpts *git.AuthOptions) (*http.Client, *http.Request, error) { var req *http.Request var err error @@ -150,17 +168,6 @@ func createClientRequest(transportAuthID string, action git2go.SmartServiceActio return nil, nil, fmt.Errorf("failed to create client: transport cannot be nil") } - opts, found := getTransportOptions(transportAuthID) - - if !found { - return nil, nil, fmt.Errorf("failed to create client: could not find transport options for the object: %s", transportAuthID) - } - targetURL := opts.TargetURL - - if len(targetURL) > URLMaxLength { - return nil, nil, fmt.Errorf("URL exceeds the max length (%d)", URLMaxLength) - } - client := &http.Client{ Transport: t, Timeout: fullHttpClientTimeOut, @@ -176,6 +183,9 @@ func createClientRequest(transportAuthID string, action git2go.SmartServiceActio break } req.Header.Set("Content-Type", "application/x-git-upload-pack-request") + if t.Proxy != nil { + t.ProxyConnectHeader.Set("Content-Type", "application/x-git-upload-pack-request") + } case git2go.SmartServiceActionReceivepackLs: req, err = http.NewRequest("GET", targetURL+"/info/refs?service=git-receive-pack", nil) @@ -186,6 +196,9 @@ func createClientRequest(transportAuthID string, action git2go.SmartServiceActio break } req.Header.Set("Content-Type", "application/x-git-receive-pack-request") + if t.Proxy != nil { + t.ProxyConnectHeader.Set("Content-Type", "application/x-git-receive-pack-request") + } default: err = errors.New("unknown action") @@ -195,12 +208,20 @@ func createClientRequest(transportAuthID string, action git2go.SmartServiceActio return nil, nil, err } - // Add any provided certificate to the http transport. - if opts.AuthOpts != nil { - req.SetBasicAuth(opts.AuthOpts.Username, opts.AuthOpts.Password) - if len(opts.AuthOpts.CAFile) > 0 { + // Apply authentication and TLS settings to the HTTP transport. + if authOpts != nil { + if len(authOpts.Username) > 0 { + req.SetBasicAuth(authOpts.Username, authOpts.Password) + if t.Proxy != nil { + t.ProxyConnectHeader.Set( + "Authorization", + "Basic "+basicAuth(authOpts.Username, authOpts.Password), + ) + } + } + if len(authOpts.CAFile) > 0 { certPool := x509.NewCertPool() - if ok := certPool.AppendCertsFromPEM(opts.AuthOpts.CAFile); !ok { + if ok := certPool.AppendCertsFromPEM(authOpts.CAFile); !ok { return nil, nil, fmt.Errorf("failed to use certificate from PEM") } t.TLSClientConfig = &tls.Config{ @@ -210,6 +231,9 @@ func createClientRequest(transportAuthID string, action git2go.SmartServiceActio } req.Header.Set("User-Agent", "git/2.0 (flux-libgit2)") + if t.Proxy != nil { + t.ProxyConnectHeader.Set("User-Agent", "git/2.0 (flux-libgit2)") + } return client, req, nil } @@ -389,3 +413,9 @@ func (self *httpSmartSubtransportStream) sendRequest() error { self.sentRequest = true return nil } + +// From: https://github.com/golang/go/blob/go1.18/src/net/http/client.go#L418 +func basicAuth(username, password string) string { + auth := username + ":" + password + return base64.StdEncoding.EncodeToString([]byte(auth)) +} diff --git a/pkg/git/libgit2/managed/http_test.go b/pkg/git/libgit2/managed/http_test.go index bf54de597..32b2137a6 100644 --- a/pkg/git/libgit2/managed/http_test.go +++ b/pkg/git/libgit2/managed/http_test.go @@ -32,76 +32,88 @@ import ( ) func TestHttpAction_CreateClientRequest(t *testing.T) { - opts := &TransportOptions{ - TargetURL: "https://final-target/abc", + authOpts := git.AuthOptions{ + Username: "user", + Password: "pwd", } - - optsWithAuth := &TransportOptions{ - TargetURL: "https://final-target/abc", - AuthOpts: &git.AuthOptions{ - Username: "user", - Password: "pwd", - }, - } - id := "https://obj-id" + url := "https://final-target/abc" tests := []struct { name string assertFunc func(g *WithT, req *http.Request, client *http.Client) action git2go.SmartServiceAction - opts *TransportOptions + authOpts git.AuthOptions transport *http.Transport wantedErr error }{ { - name: "Uploadpack: URL and method are correctly set", - action: git2go.SmartServiceActionUploadpack, - transport: &http.Transport{}, + name: "Uploadpack: URL, method and headers are correctly set", + action: git2go.SmartServiceActionUploadpack, + transport: &http.Transport{ + Proxy: http.ProxyFromEnvironment, + ProxyConnectHeader: map[string][]string{}, + }, assertFunc: func(g *WithT, req *http.Request, _ *http.Client) { g.Expect(req.URL.String()).To(Equal("https://final-target/abc/git-upload-pack")) g.Expect(req.Method).To(Equal("POST")) + g.Expect(req.Header).To(BeEquivalentTo(map[string][]string{ + "User-Agent": []string{"git/2.0 (flux-libgit2)"}, + "Content-Type": []string{"application/x-git-upload-pack-request"}, + })) }, - opts: opts, wantedErr: nil, }, { - name: "UploadpackLs: URL and method are correctly set", + name: "UploadpackLs: URL, method and headers are correctly set", action: git2go.SmartServiceActionUploadpackLs, transport: &http.Transport{}, assertFunc: func(g *WithT, req *http.Request, _ *http.Client) { g.Expect(req.URL.String()).To(Equal("https://final-target/abc/info/refs?service=git-upload-pack")) g.Expect(req.Method).To(Equal("GET")) + g.Expect(req.Header).To(BeEquivalentTo(map[string][]string{ + "User-Agent": []string{"git/2.0 (flux-libgit2)"}, + })) }, - opts: opts, wantedErr: nil, }, { - name: "Receivepack: URL and method are correctly set", - action: git2go.SmartServiceActionReceivepack, - transport: &http.Transport{}, + name: "Receivepack: URL, method and headers are correctly set", + action: git2go.SmartServiceActionReceivepack, + transport: &http.Transport{ + Proxy: http.ProxyFromEnvironment, + ProxyConnectHeader: map[string][]string{}, + }, assertFunc: func(g *WithT, req *http.Request, _ *http.Client) { g.Expect(req.URL.String()).To(Equal("https://final-target/abc/git-receive-pack")) g.Expect(req.Method).To(Equal("POST")) + g.Expect(req.Header).To(BeEquivalentTo(map[string][]string{ + "Content-Type": []string{"application/x-git-receive-pack-request"}, + "User-Agent": []string{"git/2.0 (flux-libgit2)"}, + })) }, - opts: opts, wantedErr: nil, }, { - name: "ReceivepackLs: URL and method are correctly set", + name: "ReceivepackLs: URL, method and headars are correctly set", action: git2go.SmartServiceActionReceivepackLs, transport: &http.Transport{}, assertFunc: func(g *WithT, req *http.Request, _ *http.Client) { g.Expect(req.URL.String()).To(Equal("https://final-target/abc/info/refs?service=git-receive-pack")) g.Expect(req.Method).To(Equal("GET")) + g.Expect(req.Header).To(BeEquivalentTo(map[string][]string{ + "User-Agent": []string{"git/2.0 (flux-libgit2)"}, + })) }, - opts: opts, wantedErr: nil, }, { - name: "credentials are correctly configured", - action: git2go.SmartServiceActionUploadpack, - transport: &http.Transport{}, - opts: optsWithAuth, + name: "credentials are correctly configured", + action: git2go.SmartServiceActionUploadpack, + transport: &http.Transport{ + Proxy: http.ProxyFromEnvironment, + ProxyConnectHeader: map[string][]string{}, + }, + authOpts: authOpts, assertFunc: func(g *WithT, req *http.Request, client *http.Client) { g.Expect(req.URL.String()).To(Equal("https://final-target/abc/git-upload-pack")) g.Expect(req.Method).To(Equal("POST")) @@ -119,26 +131,15 @@ func TestHttpAction_CreateClientRequest(t *testing.T) { name: "error when no http.transport provided", action: git2go.SmartServiceActionUploadpack, transport: nil, - opts: opts, wantedErr: fmt.Errorf("failed to create client: transport cannot be nil"), }, - { - name: "error when no transport options are registered", - action: git2go.SmartServiceActionUploadpack, - transport: &http.Transport{}, - opts: nil, - wantedErr: fmt.Errorf("failed to create client: could not find transport options for the object: https://obj-id"), - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - if tt.opts != nil { - AddTransportOptions(id, *tt.opts) - } - client, req, err := createClientRequest(id, tt.action, tt.transport) + client, req, err := createClientRequest(url, tt.action, tt.transport, &tt.authOpts) if err != nil { t.Log(err) } @@ -148,9 +149,6 @@ func TestHttpAction_CreateClientRequest(t *testing.T) { tt.assertFunc(g, req, client) } - if tt.opts != nil { - RemoveTransportOptions(id) - } }) } } @@ -167,18 +165,10 @@ func TestHTTPManagedTransport_E2E(t *testing.T) { server.Auth(user, pwd) server.KeyDir(filepath.Join(server.Root(), "keys")) - err = server.ListenSSH() - g.Expect(err).ToNot(HaveOccurred()) - err = server.StartHTTP() g.Expect(err).ToNot(HaveOccurred()) defer server.StopHTTP() - go func() { - server.StartSSH() - }() - defer server.StopSSH() - // Force managed transport to be enabled InitManagedTransport(logr.Discard()) @@ -188,7 +178,7 @@ func TestHTTPManagedTransport_E2E(t *testing.T) { tmpDir := t.TempDir() - // Register the auth options and target url mapped to a unique id. + // Register the auth options and target url mapped to a unique url. id := "http://obj-id" AddTransportOptions(id, TransportOptions{ TargetURL: server.HTTPAddress() + "/" + repoPath, @@ -198,9 +188,9 @@ func TestHTTPManagedTransport_E2E(t *testing.T) { }, }) - // We call Clone with id instead of the actual url, as the transport action - // will fetch the actual url and the required credentials using the id as - // a identifier. + // We call git2go.Clone with transportOptsURL instead of the actual URL, + // as the transport action will fetch the actual URL and the required + // credentials using the it as an identifier. repo, err := git2go.Clone(id, tmpDir, &git2go.CloneOptions{ CheckoutOptions: git2go.CheckoutOptions{ Strategy: git2go.CheckoutForce, diff --git a/pkg/git/libgit2/managed/managed_test.go b/pkg/git/libgit2/managed/managed_test.go deleted file mode 100644 index beda7fc2a..000000000 --- a/pkg/git/libgit2/managed/managed_test.go +++ /dev/null @@ -1,48 +0,0 @@ -/* -Copyright 2022 The Flux authors - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package managed - -import ( - "os" - "testing" -) - -func TestFlagStatus(t *testing.T) { - if Enabled() { - t.Errorf("experimental transport should not be enabled by default") - } - - os.Setenv("EXPERIMENTAL_GIT_TRANSPORT", "true") - if !Enabled() { - t.Errorf("experimental transport should be enabled when env EXPERIMENTAL_GIT_TRANSPORT=true") - } - - os.Setenv("EXPERIMENTAL_GIT_TRANSPORT", "1") - if !Enabled() { - t.Errorf("experimental transport should be enabled when env EXPERIMENTAL_GIT_TRANSPORT=1") - } - - os.Setenv("EXPERIMENTAL_GIT_TRANSPORT", "somethingelse") - if Enabled() { - t.Errorf("experimental transport should be enabled only when env EXPERIMENTAL_GIT_TRANSPORT is 1 or true but was enabled for 'somethingelse'") - } - - os.Unsetenv("EXPERIMENTAL_GIT_TRANSPORT") - if Enabled() { - t.Errorf("experimental transport should not be enabled when env EXPERIMENTAL_GIT_TRANSPORT is not present") - } -} diff --git a/pkg/git/libgit2/managed/options.go b/pkg/git/libgit2/managed/options.go index 58a04da75..900d593cc 100644 --- a/pkg/git/libgit2/managed/options.go +++ b/pkg/git/libgit2/managed/options.go @@ -20,36 +20,45 @@ import ( "sync" "github.com/fluxcd/source-controller/pkg/git" + git2go "github.com/libgit2/git2go/v33" ) // TransportOptions represents options to be applied at transport-level // at request time. type TransportOptions struct { - TargetURL string - AuthOpts *git.AuthOptions + TargetURL string + AuthOpts *git.AuthOptions + ProxyOptions *git2go.ProxyOptions } var ( - // transportOpts maps a unique id to a set of transport options. + // transportOpts maps a unique url to a set of transport options. transportOpts = make(map[string]TransportOptions, 0) m sync.RWMutex ) -func AddTransportOptions(id string, opts TransportOptions) { +// AddTransportOptions registers a TransportOptions object mapped to the +// provided transportOptsURL, which must be a valid URL, i.e. prefixed with "http://" +// or "ssh://", as it is used as a dummy URL for all git operations and the managed +// transports will only be invoked for the protocols that they have been +// registered for. +func AddTransportOptions(transportOptsURL string, opts TransportOptions) { m.Lock() - transportOpts[id] = opts + transportOpts[transportOptsURL] = opts m.Unlock() } -func RemoveTransportOptions(id string) { +// RemoveTransportOptions removes the registerd TransportOptions object +// mapped to the provided id. +func RemoveTransportOptions(transportOptsURL string) { m.Lock() - delete(transportOpts, id) + delete(transportOpts, transportOptsURL) m.Unlock() } -func getTransportOptions(id string) (*TransportOptions, bool) { +func getTransportOptions(transportOptsURL string) (*TransportOptions, bool) { m.RLock() - opts, found := transportOpts[id] + opts, found := transportOpts[transportOptsURL] m.RUnlock() if found { @@ -63,16 +72,16 @@ func getTransportOptions(id string) (*TransportOptions, bool) { // Given that TransportOptions can allow for the target URL to be overriden // this returns the same input if Managed Transport is disabled or if no TargetURL // is set on TransportOptions. -func EffectiveURL(id string) string { +func EffectiveURL(transporOptsURL string) string { if !Enabled() { - return id + return transporOptsURL } - if opts, found := getTransportOptions(id); found { + if opts, found := getTransportOptions(transporOptsURL); found { if opts.TargetURL != "" { return opts.TargetURL } } - return id + return transporOptsURL } diff --git a/pkg/git/libgit2/managed/ssh.go b/pkg/git/libgit2/managed/ssh.go index 3895fbe4e..dddcadc09 100644 --- a/pkg/git/libgit2/managed/ssh.go +++ b/pkg/git/libgit2/managed/ssh.go @@ -45,8 +45,6 @@ package managed import ( "context" - "crypto/md5" - "crypto/sha1" "crypto/sha256" "fmt" "io" @@ -96,13 +94,13 @@ type sshSmartSubtransport struct { connected bool } -func (t *sshSmartSubtransport) Action(credentialsID string, action git2go.SmartServiceAction) (git2go.SmartSubtransportStream, error) { +func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go.SmartServiceAction) (git2go.SmartSubtransportStream, error) { runtime.LockOSThread() defer runtime.UnlockOSThread() - opts, found := getTransportOptions(credentialsID) + opts, found := getTransportOptions(transportOptionsURL) if !found { - return nil, fmt.Errorf("could not find transport options for object: %s", credentialsID) + return nil, fmt.Errorf("could not find transport options for object: %s", transportOptionsURL) } u, err := url.Parse(opts.TargetURL) @@ -167,16 +165,17 @@ func (t *sshSmartSubtransport) Action(credentialsID string, action git2go.SmartS cert := &git2go.Certificate{ Kind: git2go.CertificateHostkey, Hostkey: git2go.HostkeyCertificate{ - Kind: git2go.HostkeySHA1 | git2go.HostkeyMD5 | git2go.HostkeySHA256 | git2go.HostkeyRaw, - HashMD5: md5.Sum(marshaledKey), - HashSHA1: sha1.Sum(marshaledKey), + Kind: git2go.HostkeySHA256, HashSHA256: sha256.Sum256(marshaledKey), Hostkey: marshaledKey, SSHPublicKey: key, }, } - return t.transport.SmartCertificateCheck(cert, true, hostname) + if len(opts.AuthOpts.KnownHosts) > 0 { + return KnownHostsCallback(hostname, opts.AuthOpts.KnownHosts)(cert, true, hostname) + } + return nil } err = t.createConn(t.addr, sshConfig) diff --git a/pkg/git/libgit2/managed/ssh_test.go b/pkg/git/libgit2/managed/ssh_test.go index 4d5a7b37a..81b83f3cc 100644 --- a/pkg/git/libgit2/managed/ssh_test.go +++ b/pkg/git/libgit2/managed/ssh_test.go @@ -98,9 +98,9 @@ func TestSSHManagedTransport_E2E(t *testing.T) { err = server.InitRepo("../../testdata/git/repo", git.DefaultBranch, repoPath) g.Expect(err).ToNot(HaveOccurred()) - transportID := "ssh://git@fake-url" + transportOptsURL := "ssh://git@fake-url" sshAddress := server.SSHAddress() + "/" + repoPath - AddTransportOptions(transportID, TransportOptions{ + AddTransportOptions(transportOptsURL, TransportOptions{ TargetURL: sshAddress, AuthOpts: &git.AuthOptions{ Username: "user", @@ -110,10 +110,12 @@ func TestSSHManagedTransport_E2E(t *testing.T) { tmpDir := t.TempDir() - // We call git2go.Clone with transportID, so that the managed ssh transport can + // We call git2go.Clone with transportOptsURL, so that the managed ssh transport can // fetch the correct set of credentials and the actual target url as well. - repo, err := git2go.Clone(transportID, tmpDir, &git2go.CloneOptions{ - FetchOptions: git2go.FetchOptions{}, + repo, err := git2go.Clone(transportOptsURL, tmpDir, &git2go.CloneOptions{ + FetchOptions: git2go.FetchOptions{ + RemoteCallbacks: RemoteCallbacks(), + }, CheckoutOptions: git2go.CheckoutOptions{ Strategy: git2go.CheckoutForce, }, diff --git a/pkg/git/libgit2/managed/transport.go b/pkg/git/libgit2/managed/transport.go new file mode 100644 index 000000000..763e6f8b2 --- /dev/null +++ b/pkg/git/libgit2/managed/transport.go @@ -0,0 +1,103 @@ +package managed + +import ( + "crypto/md5" + "crypto/sha1" + "crypto/sha256" + "fmt" + "hash" + "net" + + pkgkh "github.com/fluxcd/pkg/ssh/knownhosts" + git2go "github.com/libgit2/git2go/v33" + "golang.org/x/crypto/ssh/knownhosts" +) + +// knownHostCallback returns a CertificateCheckCallback that verifies +// the key of Git server against the given host and known_hosts for +// git.SSH Transports. +func KnownHostsCallback(host string, knownHosts []byte) git2go.CertificateCheckCallback { + return func(cert *git2go.Certificate, valid bool, hostname string) error { + kh, err := pkgkh.ParseKnownHosts(string(knownHosts)) + if err != nil { + return fmt.Errorf("failed to parse known_hosts: %w", err) + } + + // First, attempt to split the configured host and port to validate + // the port-less hostname given to the callback. + hostWithoutPort, _, err := net.SplitHostPort(host) + if err != nil { + // SplitHostPort returns an error if the host is missing + // a port, assume the host has no port. + hostWithoutPort = host + } + + // Different versions of libgit handle this differently. + // This fixes the case in which ports may be sent back. + hostnameWithoutPort, _, err := net.SplitHostPort(hostname) + if err != nil { + hostnameWithoutPort = hostname + } + + if hostnameWithoutPort != hostWithoutPort { + return fmt.Errorf("host mismatch: %q %q", hostWithoutPort, hostnameWithoutPort) + } + + var fingerprint []byte + var hasher hash.Hash + switch { + case cert.Hostkey.Kind&git2go.HostkeySHA256 > 0: + fingerprint = cert.Hostkey.HashSHA256[:] + hasher = sha256.New() + // SHA1 and MD5 are present here, because they're used for unmanaged transport. + // TODO: get rid of this, when unmanaged transport is completely removed. + case cert.Hostkey.Kind&git2go.HostkeySHA1 > 0: + fingerprint = cert.Hostkey.HashSHA1[:] + hasher = sha1.New() + case cert.Hostkey.Kind&git2go.HostkeyMD5 > 0: + fingerprint = cert.Hostkey.HashMD5[:] + hasher = md5.New() + default: + return fmt.Errorf("invalid host key kind, expected to be one of SHA256, SHA1, MD5") + } + + // We are now certain that the configured host and the hostname + // given to the callback match. Use the configured host (that + // includes the port), and normalize it, so we can check if there + // is an entry for the hostname _and_ port. + h := knownhosts.Normalize(host) + for _, k := range kh { + if k.Matches(h, fingerprint, hasher) { + return nil + } + } + return fmt.Errorf("hostkey could not be verified") + } +} + +// RemoteCallbacks constructs git2go.RemoteCallbacks with dummy callbacks. +func RemoteCallbacks() git2go.RemoteCallbacks { + // This may not be fully removed as without some of the callbacks git2go + // gets anxious and panics. + return git2go.RemoteCallbacks{ + CredentialsCallback: credentialsCallback(), + CertificateCheckCallback: certificateCallback(), + } +} + +// credentialsCallback constructs a dummy CredentialsCallback. +func credentialsCallback() git2go.CredentialsCallback { + return func(url string, username string, allowedTypes git2go.CredentialType) (*git2go.Credential, error) { + // If credential is nil, panic will ensue. We fake it as managed transport does not + // require it. + return git2go.NewCredentialUserpassPlaintext("", "") + } +} + +// certificateCallback constructs a dummy CertificateCallback. +func certificateCallback() git2go.CertificateCheckCallback { + // returning a nil func can cause git2go to panic. + return func(cert *git2go.Certificate, valid bool, hostname string) error { + return nil + } +} diff --git a/pkg/git/libgit2/managed/transport_test.go b/pkg/git/libgit2/managed/transport_test.go new file mode 100644 index 000000000..2428d599e --- /dev/null +++ b/pkg/git/libgit2/managed/transport_test.go @@ -0,0 +1,108 @@ +package managed + +import ( + "crypto/x509" + "encoding/base64" + "encoding/pem" + "errors" + "fmt" + "testing" + + git2go "github.com/libgit2/git2go/v33" + . "github.com/onsi/gomega" +) + +// knownHostsFixture is known_hosts fixture in the expected +// format. +var knownHostsFixture = `github.com ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAq2A7hRGmdnm9tUDbO9IDSwBK6TbQa+PXYPCPy6rbTrTtw7PHkccKrpp0yVhp5HdEIcKr6pLlVDBfOLX9QUsyCOV0wzfjIJNlGEYsdlLJizHhbn2mUjvSAHQqZETYP81eFzLQNnPHt4EVVUh7VfDESU84KezmD5QlWpXLmvU31/yMf+Se8xhHTvKSCZIFImWwoG6mbUoWf9nzpIoaSjB+weqqUUmpaaasXVal72J+UX2B+2RPW3RcT0eOzQgqlJL3RKrTJvdsjE3JEAvGq3lGHSZXy28G3skua2SmVi/w4yCE6gbODqnTWlg7+wC604ydGXA8VJiS5ap43JXiUFFAaQ==` + +func TestKnownHostsCallback(t *testing.T) { + tests := []struct { + name string + host string + expectedHost string + knownHosts []byte + hostkey git2go.HostkeyCertificate + want error + }{ + { + name: "Match", + host: "github.com", + knownHosts: []byte(knownHostsFixture), + hostkey: git2go.HostkeyCertificate{Kind: git2go.HostkeySHA1 | git2go.HostkeyMD5, HashSHA1: sha1Fingerprint("v2toJdKXfFEaR1u++4iq1UqSrHM")}, + expectedHost: "github.com", + want: nil, + }, + { + name: "Match with port", + host: "github.com", + knownHosts: []byte(knownHostsFixture), + hostkey: git2go.HostkeyCertificate{Kind: git2go.HostkeySHA1 | git2go.HostkeyMD5, HashSHA1: sha1Fingerprint("v2toJdKXfFEaR1u++4iq1UqSrHM")}, + expectedHost: "github.com:22", + want: nil, + }, + { + name: "Hostname mismatch", + host: "github.com", + knownHosts: []byte(knownHostsFixture), + hostkey: git2go.HostkeyCertificate{Kind: git2go.HostkeySHA1 | git2go.HostkeyMD5, HashSHA1: sha1Fingerprint("v2toJdKXfFEaR1u++4iq1UqSrHM")}, + expectedHost: "example.com", + want: fmt.Errorf("host mismatch: %q %q", "example.com", "github.com"), + }, + { + name: "Hostkey mismatch", + host: "github.com", + knownHosts: []byte(knownHostsFixture), + hostkey: git2go.HostkeyCertificate{Kind: git2go.HostkeyMD5, HashMD5: md5Fingerprint("\xb6\x03\x0e\x39\x97\x9e\xd0\xe7\x24\xce\xa3\x77\x3e\x01\x42\x09")}, + expectedHost: "github.com", + want: fmt.Errorf("hostkey could not be verified"), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + cert := &git2go.Certificate{Hostkey: tt.hostkey} + callback := KnownHostsCallback(tt.expectedHost, tt.knownHosts) + result := g.Expect(callback(cert, false, tt.host)) + if tt.want == nil { + result.To(BeNil()) + } else { + result.To(Equal(tt.want)) + } + }) + } +} +func md5Fingerprint(in string) [16]byte { + var out [16]byte + copy(out[:], in) + return out +} + +func sha1Fingerprint(in string) [20]byte { + d, err := base64.RawStdEncoding.DecodeString(in) + if err != nil { + panic(err) + } + var out [20]byte + copy(out[:], d) + return out +} + +func sha256Fingerprint(in string) [32]byte { + d, err := base64.RawStdEncoding.DecodeString(in) + if err != nil { + panic(err) + } + var out [32]byte + copy(out[:], d) + return out +} + +func certificateFromPEM(pemBytes string) (*x509.Certificate, error) { + block, _ := pem.Decode([]byte(pemBytes)) + if block == nil { + return nil, errors.New("failed to decode PEM") + } + return x509.ParseCertificate(block.Bytes) +} diff --git a/pkg/git/libgit2/managed_test.go b/pkg/git/libgit2/managed_test.go index 728c61fe5..8cb07016d 100644 --- a/pkg/git/libgit2/managed_test.go +++ b/pkg/git/libgit2/managed_test.go @@ -36,6 +36,7 @@ import ( "github.com/go-logr/logr" . "github.com/onsi/gomega" + git2go "github.com/libgit2/git2go/v33" cryptossh "golang.org/x/crypto/ssh" ) @@ -123,7 +124,6 @@ func Test_ManagedSSH_KeyTypes(t *testing.T) { knownHosts, err := ssh.ScanHostKey(u.Host, timeout, git.HostKeyAlgos, false) g.Expect(err).ToNot(HaveOccurred()) - os.Setenv("EXPERIMENTAL_GIT_TRANSPORT", "true") managed.InitManagedTransport(logr.Discard()) for _, tt := range tests { @@ -139,21 +139,11 @@ func Test_ManagedSSH_KeyTypes(t *testing.T) { authorizedPublicKey = string(kp.PublicKey) } - // secret := corev1.Secret{ - // Data: map[string][]byte{ - // "identity": kp.PrivateKey, - // "known_hosts": knownHosts, - // }, - // } - // - // authOpts, err := git.AuthOptionsFromSecret(repoURL, &secret) - // g.Expect(err).ToNot(HaveOccurred()) - authOpts := &git.AuthOptions{ Identity: kp.PrivateKey, KnownHosts: knownHosts, } - authOpts.TransportAuthID = "ssh://" + getTransportAuthID() + authOpts.TransportOptionsURL = getTransportOptionsURL(git.SSH) // Prepare for checkout. branchCheckoutStrat := &CheckoutBranch{Branch: git.DefaultBranch} @@ -233,7 +223,6 @@ func Test_ManagedSSH_KeyExchangeAlgos(t *testing.T) { }, } - os.Setenv("EXPERIMENTAL_GIT_TRANSPORT", "true") managed.InitManagedTransport(logr.Discard()) for _, tt := range tests { @@ -280,20 +269,11 @@ func Test_ManagedSSH_KeyExchangeAlgos(t *testing.T) { kp, err := ssh.GenerateKeyPair(ssh.ED25519) g.Expect(err).ToNot(HaveOccurred()) - // secret := corev1.Secret{ - // Data: map[string][]byte{ - // "identity": kp.PrivateKey, - // "known_hosts": knownHosts, - // }, - // } - // - // authOpts, err := git.AuthOptionsFromSecret(repoURL, &secret) - // g.Expect(err).ToNot(HaveOccurred()) authOpts := &git.AuthOptions{ Identity: kp.PrivateKey, KnownHosts: knownHosts, } - authOpts.TransportAuthID = "ssh://" + getTransportAuthID() + authOpts.TransportOptionsURL = getTransportOptionsURL(git.SSH) // Prepare for checkout. branchCheckoutStrat := &CheckoutBranch{Branch: git.DefaultBranch} @@ -402,7 +382,6 @@ func Test_ManagedSSH_HostKeyAlgos(t *testing.T) { }, } - os.Setenv("EXPERIMENTAL_GIT_TRANSPORT", "true") managed.InitManagedTransport(logr.Discard()) for _, tt := range tests { @@ -459,20 +438,11 @@ func Test_ManagedSSH_HostKeyAlgos(t *testing.T) { kp, err := ssh.GenerateKeyPair(ssh.ED25519) g.Expect(err).ToNot(HaveOccurred()) - // secret := corev1.Secret{ - // Data: map[string][]byte{ - // "identity": kp.PrivateKey, - // "known_hosts": knownHosts, - // }, - // } - // - // authOpts, err := git.AuthOptionsFromSecret(repoURL, &secret) - // g.Expect(err).ToNot(HaveOccurred()) authOpts := &git.AuthOptions{ Identity: kp.PrivateKey, KnownHosts: knownHosts, } - authOpts.TransportAuthID = "ssh://" + getTransportAuthID() + authOpts.TransportOptionsURL = getTransportOptionsURL(git.SSH) // Prepare for checkout. branchCheckoutStrat := &CheckoutBranch{Branch: git.DefaultBranch} @@ -488,11 +458,161 @@ func Test_ManagedSSH_HostKeyAlgos(t *testing.T) { } } -func getTransportAuthID() string { +func Test_ManagedHTTPCheckout(t *testing.T) { + g := NewWithT(t) + + timeout := 5 * time.Second + server, err := gittestserver.NewTempGitServer() + g.Expect(err).ToNot(HaveOccurred()) + defer os.RemoveAll(server.Root()) + + user := "test-user" + pwd := "test-pswd" + server.Auth(user, pwd) + + err = server.StartHTTP() + g.Expect(err).ToNot(HaveOccurred()) + defer server.StopHTTP() + + // Force managed transport to be enabled + managed.InitManagedTransport(logr.Discard()) + + repoPath := "test.git" + err = server.InitRepo("../testdata/git/repo", git.DefaultBranch, repoPath) + g.Expect(err).ToNot(HaveOccurred()) + + authOpts := &git.AuthOptions{ + Username: "test-user", + Password: "test-pswd", + } + authOpts.TransportOptionsURL = getTransportOptionsURL(git.HTTP) + + // Prepare for checkout. + branchCheckoutStrat := &CheckoutBranch{Branch: git.DefaultBranch} + tmpDir := t.TempDir() + + ctx, cancel := context.WithTimeout(context.TODO(), timeout) + defer cancel() + + repoURL := server.HTTPAddress() + "/" + repoPath + // Checkout the repo. + _, err = branchCheckoutStrat.Checkout(ctx, tmpDir, repoURL, authOpts) + g.Expect(err).Error().ShouldNot(HaveOccurred()) +} + +func TestManagedCheckoutBranch_Checkout(t *testing.T) { + managed.InitManagedTransport(logr.Discard()) + g := NewWithT(t) + + timeout := 5 * time.Second + server, err := gittestserver.NewTempGitServer() + g.Expect(err).ToNot(HaveOccurred()) + defer os.RemoveAll(server.Root()) + + err = server.StartHTTP() + g.Expect(err).ToNot(HaveOccurred()) + defer server.StopHTTP() + + repoPath := "test.git" + err = server.InitRepo("../testdata/git/repo", git.DefaultBranch, repoPath) + g.Expect(err).ToNot(HaveOccurred()) + + repo, err := git2go.OpenRepository(filepath.Join(server.Root(), repoPath)) + g.Expect(err).ToNot(HaveOccurred()) + + branchRef, err := repo.References.Lookup(fmt.Sprintf("refs/heads/%s", git.DefaultBranch)) + g.Expect(err).ToNot(HaveOccurred()) + defer branchRef.Free() + + commit, err := repo.LookupCommit(branchRef.Target()) + g.Expect(err).ToNot(HaveOccurred()) + + authOpts := &git.AuthOptions{ + TransportOptionsURL: getTransportOptionsURL(git.HTTP), + } + + tmpDir := t.TempDir() + + ctx, cancel := context.WithTimeout(context.TODO(), timeout) + defer cancel() + + repoURL := server.HTTPAddress() + "/" + repoPath + branch := CheckoutBranch{ + Branch: git.DefaultBranch, + // Set last revision to HEAD commit, to force a no-op clone. + LastRevision: fmt.Sprintf("%s/%s", git.DefaultBranch, commit.Id().String()), + } + + cc, err := branch.Checkout(ctx, tmpDir, repoURL, authOpts) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(cc.String()).To(Equal(git.DefaultBranch + "/" + commit.Id().String())) + g.Expect(git.IsConcreteCommit(*cc)).To(Equal(false)) + + // Set last revision to a fake commit to force a full clone. + branch.LastRevision = fmt.Sprintf("%s/non-existent-commit", git.DefaultBranch) + cc, err = branch.Checkout(ctx, tmpDir, repoURL, authOpts) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(cc.String()).To(Equal(git.DefaultBranch + "/" + commit.Id().String())) + g.Expect(git.IsConcreteCommit(*cc)).To(Equal(true)) +} + +func TestManagedCheckoutTag_Checkout(t *testing.T) { + managed.InitManagedTransport(logr.Discard()) + g := NewWithT(t) + + timeout := 5 * time.Second + server, err := gittestserver.NewTempGitServer() + g.Expect(err).ToNot(HaveOccurred()) + defer os.RemoveAll(server.Root()) + + err = server.StartHTTP() + g.Expect(err).ToNot(HaveOccurred()) + defer server.StopHTTP() + + repoPath := "test.git" + err = server.InitRepo("../testdata/git/repo", git.DefaultBranch, repoPath) + g.Expect(err).ToNot(HaveOccurred()) + + repo, err := git2go.OpenRepository(filepath.Join(server.Root(), repoPath)) + g.Expect(err).ToNot(HaveOccurred()) + + branchRef, err := repo.References.Lookup(fmt.Sprintf("refs/heads/%s", git.DefaultBranch)) + g.Expect(err).ToNot(HaveOccurred()) + defer branchRef.Free() + + commit, err := repo.LookupCommit(branchRef.Target()) + g.Expect(err).ToNot(HaveOccurred()) + _, err = tag(repo, commit.Id(), false, "tag-1", time.Now()) + + checkoutTag := CheckoutTag{ + Tag: "tag-1", + } + authOpts := &git.AuthOptions{ + TransportOptionsURL: getTransportOptionsURL(git.HTTP), + } + repoURL := server.HTTPAddress() + "/" + repoPath + tmpDir := t.TempDir() + + ctx, cancel := context.WithTimeout(context.TODO(), timeout) + defer cancel() + + cc, err := checkoutTag.Checkout(ctx, tmpDir, repoURL, authOpts) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(cc.String()).To(Equal("tag-1" + "/" + commit.Id().String())) + g.Expect(git.IsConcreteCommit(*cc)).To(Equal(true)) + + checkoutTag.LastRevision = "tag-1" + "/" + commit.Id().String() + cc, err = checkoutTag.Checkout(ctx, tmpDir, repoURL, authOpts) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(cc.String()).To(Equal("tag-1" + "/" + commit.Id().String())) + g.Expect(git.IsConcreteCommit(*cc)).To(Equal(false)) +} + +func getTransportOptionsURL(transport git.TransportType) string { letterRunes := []rune("abcdefghijklmnopqrstuvwxyz1234567890") b := make([]rune, 10) for i := range b { b[i] = letterRunes[rand.Intn(len(letterRunes))] } - return string(b) + return string(transport) + "://" + string(b) } diff --git a/pkg/git/libgit2/transport.go b/pkg/git/libgit2/transport.go index e7c9671c0..f9aeefe21 100644 --- a/pkg/git/libgit2/transport.go +++ b/pkg/git/libgit2/transport.go @@ -17,27 +17,16 @@ limitations under the License. package libgit2 import ( - "bufio" - "bytes" "context" - "crypto/hmac" - "crypto/md5" - "crypto/sha1" - "crypto/sha256" "crypto/x509" - "encoding/base64" "fmt" - "hash" - "io" - "net" - "strings" "time" git2go "github.com/libgit2/git2go/v33" "golang.org/x/crypto/ssh" - "golang.org/x/crypto/ssh/knownhosts" "github.com/fluxcd/source-controller/pkg/git" + "github.com/fluxcd/source-controller/pkg/git/libgit2/managed" ) var ( @@ -148,7 +137,7 @@ func certificateCallback(opts *git.AuthOptions) git2go.CertificateCheckCallback } case git.SSH: if len(opts.KnownHosts) > 0 && opts.Host != "" { - return knownHostsCallback(opts.Host, opts.KnownHosts) + return managed.KnownHostsCallback(opts.Host, opts.KnownHosts) } } return nil @@ -174,157 +163,3 @@ func x509Callback(caBundle []byte) git2go.CertificateCheckCallback { return nil } } - -// knownHostCallback returns a CertificateCheckCallback that verifies -// the key of Git server against the given host and known_hosts for -// git.SSH Transports. -func knownHostsCallback(host string, knownHosts []byte) git2go.CertificateCheckCallback { - return func(cert *git2go.Certificate, valid bool, hostname string) error { - kh, err := parseKnownHosts(string(knownHosts)) - if err != nil { - return fmt.Errorf("failed to parse known_hosts: %w", err) - } - - // First, attempt to split the configured host and port to validate - // the port-less hostname given to the callback. - hostWithoutPort, _, err := net.SplitHostPort(host) - if err != nil { - // SplitHostPort returns an error if the host is missing - // a port, assume the host has no port. - hostWithoutPort = host - } - - // Different versions of libgit handle this differently. - // This fixes the case in which ports may be sent back. - hostnameWithoutPort, _, err := net.SplitHostPort(hostname) - if err != nil { - hostnameWithoutPort = hostname - } - - if hostnameWithoutPort != hostWithoutPort { - return fmt.Errorf("host mismatch: %q %q", hostWithoutPort, hostnameWithoutPort) - } - - // We are now certain that the configured host and the hostname - // given to the callback match. Use the configured host (that - // includes the port), and normalize it, so we can check if there - // is an entry for the hostname _and_ port. - h := knownhosts.Normalize(host) - for _, k := range kh { - if k.matches(h, cert.Hostkey) { - return nil - } - } - return fmt.Errorf("hostkey could not be verified") - } -} - -type knownKey struct { - hosts []string - key ssh.PublicKey -} - -func parseKnownHosts(s string) ([]knownKey, error) { - var knownHosts []knownKey - scanner := bufio.NewScanner(strings.NewReader(s)) - for scanner.Scan() { - _, hosts, pubKey, _, _, err := ssh.ParseKnownHosts(scanner.Bytes()) - if err != nil { - // Lines that aren't host public key result in EOF, like a comment - // line. Continue parsing the other lines. - if err == io.EOF { - continue - } - return []knownKey{}, err - } - - knownHost := knownKey{ - hosts: hosts, - key: pubKey, - } - knownHosts = append(knownHosts, knownHost) - } - - if err := scanner.Err(); err != nil { - return []knownKey{}, err - } - - return knownHosts, nil -} - -func (k knownKey) matches(host string, hostkey git2go.HostkeyCertificate) bool { - if !containsHost(k.hosts, host) { - return false - } - - var fingerprint []byte - var hasher hash.Hash - switch { - case hostkey.Kind&git2go.HostkeySHA256 > 0: - fingerprint = hostkey.HashSHA256[:] - hasher = sha256.New() - case hostkey.Kind&git2go.HostkeySHA1 > 0: - fingerprint = hostkey.HashSHA1[:] - hasher = sha1.New() - case hostkey.Kind&git2go.HostkeyMD5 > 0: - fingerprint = hostkey.HashMD5[:] - hasher = md5.New() - default: - return false - } - hasher.Write(k.key.Marshal()) - return bytes.Equal(hasher.Sum(nil), fingerprint) -} - -func containsHost(hosts []string, host string) bool { - for _, kh := range hosts { - // hashed host must start with a pipe - if kh[0] == '|' { - match, _ := MatchHashedHost(kh, host) - if match { - return true - } - - } else if kh == host { // unhashed host check - return true - } - } - return false -} - -// MatchHashedHost tries to match a hashed known host (kh) to -// host. -// -// Note that host is not hashed, but it is rather hashed during -// the matching process using the same salt used when hashing -// the known host. -func MatchHashedHost(kh, host string) (bool, error) { - if kh == "" || kh[0] != '|' { - return false, fmt.Errorf("hashed known host must begin with '|': '%s'", kh) - } - - components := strings.Split(kh, "|") - if len(components) != 4 { - return false, fmt.Errorf("invalid format for hashed known host: '%s'", kh) - } - - if components[1] != "1" { - return false, fmt.Errorf("unsupported hash type '%s'", components[1]) - } - - hkSalt, err := base64.StdEncoding.DecodeString(components[2]) - if err != nil { - return false, fmt.Errorf("cannot decode hashed known host: '%w'", err) - } - - hkHash, err := base64.StdEncoding.DecodeString(components[3]) - if err != nil { - return false, fmt.Errorf("cannot decode hashed known host: '%w'", err) - } - - mac := hmac.New(sha1.New, hkSalt) - mac.Write([]byte(host)) - hostHash := mac.Sum(nil) - - return bytes.Equal(hostHash, hkHash), nil -} diff --git a/pkg/git/libgit2/transport_test.go b/pkg/git/libgit2/transport_test.go index f645807fb..2e0c57d14 100644 --- a/pkg/git/libgit2/transport_test.go +++ b/pkg/git/libgit2/transport_test.go @@ -20,7 +20,6 @@ import ( "bytes" "context" "crypto/x509" - "encoding/base64" "encoding/pem" "errors" "fmt" @@ -205,159 +204,6 @@ func Test_x509Callback(t *testing.T) { } } -func Test_knownHostsCallback(t *testing.T) { - tests := []struct { - name string - host string - expectedHost string - knownHosts []byte - hostkey git2go.HostkeyCertificate - want error - }{ - { - name: "Match", - host: "github.com", - knownHosts: []byte(knownHostsFixture), - hostkey: git2go.HostkeyCertificate{Kind: git2go.HostkeySHA1 | git2go.HostkeyMD5, HashSHA1: sha1Fingerprint("v2toJdKXfFEaR1u++4iq1UqSrHM")}, - expectedHost: "github.com", - want: nil, - }, - { - name: "Match with port", - host: "github.com", - knownHosts: []byte(knownHostsFixture), - hostkey: git2go.HostkeyCertificate{Kind: git2go.HostkeySHA1 | git2go.HostkeyMD5, HashSHA1: sha1Fingerprint("v2toJdKXfFEaR1u++4iq1UqSrHM")}, - expectedHost: "github.com:22", - want: nil, - }, - { - name: "Hostname mismatch", - host: "github.com", - knownHosts: []byte(knownHostsFixture), - hostkey: git2go.HostkeyCertificate{Kind: git2go.HostkeySHA1 | git2go.HostkeyMD5, HashSHA1: sha1Fingerprint("v2toJdKXfFEaR1u++4iq1UqSrHM")}, - expectedHost: "example.com", - want: fmt.Errorf("host mismatch: %q %q", "example.com", "github.com"), - }, - { - name: "Hostkey mismatch", - host: "github.com", - knownHosts: []byte(knownHostsFixture), - hostkey: git2go.HostkeyCertificate{Kind: git2go.HostkeyMD5, HashMD5: md5Fingerprint("\xb6\x03\x0e\x39\x97\x9e\xd0\xe7\x24\xce\xa3\x77\x3e\x01\x42\x09")}, - expectedHost: "github.com", - want: fmt.Errorf("hostkey could not be verified"), - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - cert := &git2go.Certificate{Hostkey: tt.hostkey} - callback := knownHostsCallback(tt.expectedHost, tt.knownHosts) - result := g.Expect(callback(cert, false, tt.host)) - if tt.want == nil { - result.To(BeNil()) - } else { - result.To(Equal(tt.want)) - } - }) - } -} - -func Test_parseKnownHosts_matches(t *testing.T) { - tests := []struct { - name string - hostkey git2go.HostkeyCertificate - wantMatches bool - }{ - {"good sha256 hostkey", git2go.HostkeyCertificate{Kind: git2go.HostkeySHA256 | git2go.HostkeySHA1 | git2go.HostkeyMD5, HashSHA256: sha256Fingerprint("nThbg6kXUpJWGl7E1IGOCspRomTxdCARLviKw6E5SY8")}, true}, - {"bad sha256 hostkey", git2go.HostkeyCertificate{Kind: git2go.HostkeySHA256 | git2go.HostkeySHA1 | git2go.HostkeyMD5, HashSHA256: sha256Fingerprint("ROQFvPThGrW4RuWLoL9tq9I9zJ42fK4XywyRtbOz/EQ")}, false}, - {"good sha1 hostkey", git2go.HostkeyCertificate{Kind: git2go.HostkeySHA1 | git2go.HostkeyMD5, HashSHA1: sha1Fingerprint("v2toJdKXfFEaR1u++4iq1UqSrHM")}, true}, - {"bad sha1 hostkey", git2go.HostkeyCertificate{Kind: git2go.HostkeySHA1 | git2go.HostkeyMD5, HashSHA1: sha1Fingerprint("tfpLlQhDDFP3yGdewTvHNxWmAdk")}, false}, - {"good md5 hostkey", git2go.HostkeyCertificate{Kind: git2go.HostkeyMD5, HashMD5: md5Fingerprint("\x16\x27\xac\xa5\x76\x28\x2d\x36\x63\x1b\x56\x4d\xeb\xdf\xa6\x48")}, true}, - {"bad md5 hostkey", git2go.HostkeyCertificate{Kind: git2go.HostkeyMD5, HashMD5: md5Fingerprint("\xb6\x03\x0e\x39\x97\x9e\xd0\xe7\x24\xce\xa3\x77\x3e\x01\x42\x09")}, false}, - {"invalid hostkey", git2go.HostkeyCertificate{}, false}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - knownKeys, err := parseKnownHosts(knownHostsFixture) - if err != nil { - t.Error(err) - return - } - matches := knownKeys[0].matches("github.com", tt.hostkey) - g.Expect(matches).To(Equal(tt.wantMatches)) - }) - } -} - -func Test_parseKnownHosts(t *testing.T) { - tests := []struct { - name string - fixture string - wantErr bool - }{ - { - name: "empty file", - fixture: "", - wantErr: false, - }, - { - name: "single host", - fixture: `github.com ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAq2A7hRGmdnm9tUDbO9IDSwBK6TbQa+PXYPCPy6rbTrTtw7PHkccKrpp0yVhp5HdEIcKr6pLlVDBfOLX9QUsyCOV0wzfjIJNlGEYsdlLJizHhbn2mUjvSAHQqZETYP81eFzLQNnPHt4EVVUh7VfDESU84KezmD5QlWpXLmvU31/yMf+Se8xhHTvKSCZIFImWwoG6mbUoWf9nzpIoaSjB+weqqUUmpaaasXVal72J+UX2B+2RPW3RcT0eOzQgqlJL3RKrTJvdsjE3JEAvGq3lGHSZXy28G3skua2SmVi/w4yCE6gbODqnTWlg7+wC604ydGXA8VJiS5ap43JXiUFFAaQ==`, - wantErr: false, - }, - { - name: "single host with comment", - fixture: `# github.com -github.com ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAq2A7hRGmdnm9tUDbO9IDSwBK6TbQa+PXYPCPy6rbTrTtw7PHkccKrpp0yVhp5HdEIcKr6pLlVDBfOLX9QUsyCOV0wzfjIJNlGEYsdlLJizHhbn2mUjvSAHQqZETYP81eFzLQNnPHt4EVVUh7VfDESU84KezmD5QlWpXLmvU31/yMf+Se8xhHTvKSCZIFImWwoG6mbUoWf9nzpIoaSjB+weqqUUmpaaasXVal72J+UX2B+2RPW3RcT0eOzQgqlJL3RKrTJvdsjE3JEAvGq3lGHSZXy28G3skua2SmVi/w4yCE6gbODqnTWlg7+wC604ydGXA8VJiS5ap43JXiUFFAaQ==`, - wantErr: false, - }, - { - name: "multiple hosts with comments", - fixture: `# github.com -github.com ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAq2A7hRGmdnm9tUDbO9IDSwBK6TbQa+PXYPCPy6rbTrTtw7PHkccKrpp0yVhp5HdEIcKr6pLlVDBfOLX9QUsyCOV0wzfjIJNlGEYsdlLJizHhbn2mUjvSAHQqZETYP81eFzLQNnPHt4EVVUh7VfDESU84KezmD5QlWpXLmvU31/yMf+Se8xhHTvKSCZIFImWwoG6mbUoWf9nzpIoaSjB+weqqUUmpaaasXVal72J+UX2B+2RPW3RcT0eOzQgqlJL3RKrTJvdsjE3JEAvGq3lGHSZXy28G3skua2SmVi/w4yCE6gbODqnTWlg7+wC604ydGXA8VJiS5ap43JXiUFFAaQ== -# gitlab.com -gitlab.com ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIAfuCHKVTjquxvt6CM6tdG4SLp1Btn/nOeHHE5UOzRdf`, - }, - { - name: "no host key, only comments", - fixture: `# example.com -#github.com -# gitlab.com`, - wantErr: false, - }, - { - name: "invalid host entry", - fixture: `github.com ssh-rsa`, - wantErr: true, - }, - { - name: "invalid content", - fixture: `some random text`, - wantErr: true, - }, - { - name: "invalid line with valid host key", - fixture: `some random text -gitlab.com ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIAfuCHKVTjquxvt6CM6tdG4SLp1Btn/nOeHHE5UOzRdf`, - wantErr: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - _, err := parseKnownHosts(tt.fixture) - if tt.wantErr { - g.Expect(err).To(HaveOccurred()) - } else { - g.Expect(err).ToNot(HaveOccurred()) - } - }) - } -} - func Test_transferProgressCallback(t *testing.T) { tests := []struct { name string @@ -522,94 +368,6 @@ func Test_pushTransferProgressCallback(t *testing.T) { } } -func TestMatchHashedHost(t *testing.T) { - tests := []struct { - name string - knownHost string - host string - match bool - wantErr string - }{ - { - name: "match valid known host", - knownHost: "|1|vApZG0Ybr4rHfTb69+cjjFIGIv0=|M5sSXen14encOvQAy0gseRahnJw=", - host: "[127.0.0.1]:44167", - match: true, - }, - { - name: "empty known host errors", - wantErr: "hashed known host must begin with '|'", - }, - { - name: "unhashed known host errors", - knownHost: "[127.0.0.1]:44167", - wantErr: "hashed known host must begin with '|'", - }, - { - name: "invalid known host format errors", - knownHost: "|1M5sSXen14encOvQAy0gseRahnJw=", - wantErr: "invalid format for hashed known host", - }, - { - name: "invalid hash type errors", - knownHost: "|2|vApZG0Ybr4rHfTb69+cjjFIGIv0=|M5sSXen14encOvQAy0gseRahnJw=", - wantErr: "unsupported hash type", - }, - { - name: "invalid base64 component[2] errors", - knownHost: "|1|azz|M5sSXen14encOvQAy0gseRahnJw=", - wantErr: "cannot decode hashed known host", - }, - { - name: "invalid base64 component[3] errors", - knownHost: "|1|M5sSXen14encOvQAy0gseRahnJw=|azz", - wantErr: "cannot decode hashed known host", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - matched, err := MatchHashedHost(tt.knownHost, tt.host) - - if tt.wantErr == "" { - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(matched).To(Equal(tt.match)) - } else { - g.Expect(err).To(HaveOccurred()) - g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) - } - }) - } -} - -func md5Fingerprint(in string) [16]byte { - var out [16]byte - copy(out[:], in) - return out -} - -func sha1Fingerprint(in string) [20]byte { - d, err := base64.RawStdEncoding.DecodeString(in) - if err != nil { - panic(err) - } - var out [20]byte - copy(out[:], d) - return out -} - -func sha256Fingerprint(in string) [32]byte { - d, err := base64.RawStdEncoding.DecodeString(in) - if err != nil { - panic(err) - } - var out [32]byte - copy(out[:], d) - return out -} - func certificateFromPEM(pemBytes string) (*x509.Certificate, error) { block, _ := pem.Decode([]byte(pemBytes)) if block == nil { diff --git a/pkg/git/options.go b/pkg/git/options.go index 81bbd6ce9..a9169a590 100644 --- a/pkg/git/options.go +++ b/pkg/git/options.go @@ -72,11 +72,16 @@ type AuthOptions struct { Identity []byte KnownHosts []byte CAFile []byte - // TransportAuthID is a unique identifier for this set of authentication + // TransportOptionsURL is a unique identifier for this set of authentication // options. It's used by managed libgit2 transports to uniquely identify - // which credentials to use for a particular git operation, and avoid misuse - // of credentials in a multi tenant environment. - TransportAuthID string + // which credentials to use for a particular Git operation, and avoid misuse + // of credentials in a multi-tenant environment. + // It must be prefixed with a valid transport protocol ("ssh:// "or "http://") because + // of the way managed transports are registered and invoked. + // It's a field of AuthOptions despite not providing any kind of authentication + // info, as it's the only way to sneak it into git.Checkout, without polluting + // it's args and keeping it generic. + TransportOptionsURL string } // KexAlgos hosts the key exchange algorithms to be used for SSH connections. From 90ec1e230cdf1cfd0e8a61df97ce427311bd60bc Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Thu, 26 May 2022 14:49:59 +0530 Subject: [PATCH 3/8] expand proxy tests to cover managed transport Signed-off-by: Sanskar Jaiswal Co-authored-by: Paulo Gomes --- pkg/git/strategy/proxy/strategy_proxy_test.go | 174 ++++++++++++------ 1 file changed, 122 insertions(+), 52 deletions(-) diff --git a/pkg/git/strategy/proxy/strategy_proxy_test.go b/pkg/git/strategy/proxy/strategy_proxy_test.go index 8c3133598..5bb43a3a9 100644 --- a/pkg/git/strategy/proxy/strategy_proxy_test.go +++ b/pkg/git/strategy/proxy/strategy_proxy_test.go @@ -29,17 +29,23 @@ import ( "github.com/elazarl/goproxy" "github.com/fluxcd/pkg/gittestserver" + "github.com/go-logr/logr" . "github.com/onsi/gomega" "github.com/fluxcd/source-controller/pkg/git" "github.com/fluxcd/source-controller/pkg/git/gogit" "github.com/fluxcd/source-controller/pkg/git/libgit2" + "github.com/fluxcd/source-controller/pkg/git/libgit2/managed" "github.com/fluxcd/source-controller/pkg/git/strategy" ) // These tests are run in a different _test.go file because go-git uses the ProxyFromEnvironment function of the net/http package // which caches the Proxy settings, hence not including other tests in the same file ensures a clean proxy setup for the tests to run. func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) { + // for libgit2 we are only testing for managed transport, + // as unmanaged is sunsetting. + // Unmanaged transport does not support HTTP_PROXY. + managed.InitManagedTransport(logr.Discard()) type cleanupFunc func() @@ -62,8 +68,104 @@ func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) { proxyAddr := fmt.Sprintf("localhost:%d", l.Addr().(*net.TCPAddr).Port) g.Expect(l.Close()).ToNot(HaveOccurred()) - // Note there is no libgit2 HTTP_PROXY test as libgit2 doesnt support proxied HTTP requests. cases := []testCase{ + { + name: "gogit_HTTP_PROXY", + gitImpl: gogit.Implementation, + url: "http://example.com/bar/test-reponame", + branch: "main", + setupGitProxy: func(g *WithT, proxy *goproxy.ProxyHttpServer, proxyGotRequest *bool) (*git.AuthOptions, cleanupFunc) { + // Create the git server. + gitServer, err := gittestserver.NewTempGitServer() + g.Expect(err).ToNot(HaveOccurred()) + + username := "test-user" + password := "test-password" + gitServer.Auth(username, password) + gitServer.KeyDir(gitServer.Root()) + + g.Expect(gitServer.StartHTTP()).ToNot(HaveOccurred()) + + // Initialize a git repo. + err = gitServer.InitRepo("../testdata/repo1", "main", "bar/test-reponame") + g.Expect(err).ToNot(HaveOccurred()) + + u, err := url.Parse(gitServer.HTTPAddress()) + g.Expect(err).ToNot(HaveOccurred()) + + // The request is being forwarded to the local test git server in this handler. + var proxyHandler goproxy.FuncReqHandler = func(req *http.Request, ctx *goproxy.ProxyCtx) (*http.Request, *http.Response) { + userAgent := req.Header.Get("User-Agent") + if strings.Contains(req.Host, "example.com") && strings.Contains(userAgent, "git") { + *proxyGotRequest = true + req.Host = u.Host + req.URL.Host = req.Host + return req, nil + } + // Reject if it isnt our request. + return req, goproxy.NewResponse(req, goproxy.ContentTypeText, http.StatusForbidden, "") + } + proxy.OnRequest().Do(proxyHandler) + + return &git.AuthOptions{ + Transport: git.HTTP, + Username: username, + Password: password, + }, func() { + os.RemoveAll(gitServer.Root()) + gitServer.StopHTTP() + } + }, + shortTimeout: false, + wantUsedProxy: true, + wantError: false, + }, + { + name: "gogit_HTTPS_PROXY", + gitImpl: gogit.Implementation, + url: "https://github.com/git-fixtures/basic", + branch: "master", + setupGitProxy: func(g *WithT, proxy *goproxy.ProxyHttpServer, proxyGotRequest *bool) (*git.AuthOptions, cleanupFunc) { + var proxyHandler goproxy.FuncHttpsHandler = func(host string, ctx *goproxy.ProxyCtx) (*goproxy.ConnectAction, string) { + // We don't check for user agent as this handler is only going to process CONNECT requests, and because Go's net/http + // is the one making such a request on behalf of go-git, adding a check for the go net/http user agent (Go-http-client) + // would only allow false positives from any request originating from Go's net/http. + if strings.Contains(host, "github.com") { + *proxyGotRequest = true + return goproxy.OkConnect, host + } + // Reject if it isnt our request. + return goproxy.RejectConnect, host + } + proxy.OnRequest().HandleConnect(proxyHandler) + + // go-git does not allow to use an HTTPS proxy and a custom root CA at the same time. + // See https://github.com/fluxcd/source-controller/pull/524#issuecomment-1006673163. + return nil, func() {} + }, + shortTimeout: false, + wantUsedProxy: true, + wantError: false, + }, + { + name: "gogit_NO_PROXY", + gitImpl: gogit.Implementation, + url: "https://192.0.2.1/bar/test-reponame", + branch: "main", + setupGitProxy: func(g *WithT, proxy *goproxy.ProxyHttpServer, proxyGotRequest *bool) (*git.AuthOptions, cleanupFunc) { + var proxyHandler goproxy.FuncHttpsHandler = func(host string, ctx *goproxy.ProxyCtx) (*goproxy.ConnectAction, string) { + // We shouldn't hit the proxy so we just want to check for any interaction, then reject. + *proxyGotRequest = true + return goproxy.RejectConnect, host + } + proxy.OnRequest().HandleConnect(proxyHandler) + + return nil, func() {} + }, + shortTimeout: true, + wantUsedProxy: false, + wantError: true, + }, { name: "libgit2_HTTPS_PROXY", gitImpl: libgit2.Implementation, @@ -100,6 +202,7 @@ func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) { // The request is being forwarded to the local test git server in this handler. // The certificate used here is valid for both example.com and localhost. var proxyHandler goproxy.FuncHttpsHandler = func(host string, ctx *goproxy.ProxyCtx) (*goproxy.ConnectAction, string) { + defer managed.RemoveTransportOptions("https://example.com/bar/test-reponame") // Check if the host matches with the git server address and the user-agent is the expected git client. userAgent := ctx.Req.Header.Get("User-Agent") if strings.Contains(host, "example.com") && strings.Contains(userAgent, "libgit2") { @@ -112,10 +215,11 @@ func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) { proxy.OnRequest().HandleConnect(proxyHandler) return &git.AuthOptions{ - Transport: git.HTTPS, - Username: username, - Password: password, - CAFile: exampleCA, + Transport: git.HTTPS, + Username: username, + Password: password, + CAFile: exampleCA, + TransportOptionsURL: "https://proxy-test", }, func() { os.RemoveAll(gitServer.Root()) gitServer.StopHTTP() @@ -126,8 +230,8 @@ func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) { wantError: false, }, { - name: "gogit_HTTP_PROXY", - gitImpl: gogit.Implementation, + name: "libgit2_HTTP_PROXY", + gitImpl: libgit2.Implementation, url: "http://example.com/bar/test-reponame", branch: "main", setupGitProxy: func(g *WithT, proxy *goproxy.ProxyHttpServer, proxyGotRequest *bool) (*git.AuthOptions, cleanupFunc) { @@ -135,24 +239,22 @@ func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) { gitServer, err := gittestserver.NewTempGitServer() g.Expect(err).ToNot(HaveOccurred()) - username := "test-user" - password := "test-password" - gitServer.Auth(username, password) - gitServer.KeyDir(gitServer.Root()) - - g.Expect(gitServer.StartHTTP()).ToNot(HaveOccurred()) + err = gitServer.StartHTTP() + g.Expect(err).ToNot(HaveOccurred()) // Initialize a git repo. - err = gitServer.InitRepo("../testdata/repo1", "main", "bar/test-reponame") + repoPath := "bar/test-reponame" + err = gitServer.InitRepo("../testdata/repo1", "main", repoPath) g.Expect(err).ToNot(HaveOccurred()) u, err := url.Parse(gitServer.HTTPAddress()) g.Expect(err).ToNot(HaveOccurred()) // The request is being forwarded to the local test git server in this handler. + // The certificate used here is valid for both example.com and localhost. var proxyHandler goproxy.FuncReqHandler = func(req *http.Request, ctx *goproxy.ProxyCtx) (*http.Request, *http.Response) { userAgent := req.Header.Get("User-Agent") - if strings.Contains(req.Host, "example.com") && strings.Contains(userAgent, "git") { + if strings.Contains(req.Host, "example.com") && strings.Contains(userAgent, "libgit2") { *proxyGotRequest = true req.Host = u.Host req.URL.Host = req.Host @@ -164,9 +266,8 @@ func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) { proxy.OnRequest().Do(proxyHandler) return &git.AuthOptions{ - Transport: git.HTTP, - Username: username, - Password: password, + Transport: git.HTTP, + TransportOptionsURL: "http://proxy-test", }, func() { os.RemoveAll(gitServer.Root()) gitServer.StopHTTP() @@ -177,35 +278,8 @@ func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) { wantError: false, }, { - name: "gogit_HTTPS_PROXY", - gitImpl: gogit.Implementation, - url: "https://github.com/git-fixtures/basic", - branch: "master", - setupGitProxy: func(g *WithT, proxy *goproxy.ProxyHttpServer, proxyGotRequest *bool) (*git.AuthOptions, cleanupFunc) { - var proxyHandler goproxy.FuncHttpsHandler = func(host string, ctx *goproxy.ProxyCtx) (*goproxy.ConnectAction, string) { - // We don't check for user agent as this handler is only going to process CONNECT requests, and because Go's net/http - // is the one making such a request on behalf of go-git, adding a check for the go net/http user agent (Go-http-client) - // would only allow false positives from any request originating from Go's net/http. - if strings.Contains(host, "github.com") { - *proxyGotRequest = true - return goproxy.OkConnect, host - } - // Reject if it isnt our request. - return goproxy.RejectConnect, host - } - proxy.OnRequest().HandleConnect(proxyHandler) - - // go-git does not allow to use an HTTPS proxy and a custom root CA at the same time. - // See https://github.com/fluxcd/source-controller/pull/524#issuecomment-1006673163. - return nil, func() {} - }, - shortTimeout: false, - wantUsedProxy: true, - wantError: false, - }, - { - name: "gogit_NO_PROXY", - gitImpl: gogit.Implementation, + name: "libgit2_NO_PROXY", + gitImpl: libgit2.Implementation, url: "https://192.0.2.1/bar/test-reponame", branch: "main", setupGitProxy: func(g *WithT, proxy *goproxy.ProxyHttpServer, proxyGotRequest *bool) (*git.AuthOptions, cleanupFunc) { @@ -218,13 +292,10 @@ func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) { return nil, func() {} }, - shortTimeout: true, + shortTimeout: false, wantUsedProxy: false, wantError: true, }, - // TODO: Add a NO_PROXY test for libgit2 once the version of libgit2 used by the source controller is updated to a version that includes - // the NO_PROXY functionality - // This PR introduces the functionality in libgit2: https://github.com/libgit2/libgit2/pull/6026 } for _, tt := range cases { @@ -282,7 +353,6 @@ func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) { } g.Expect(proxyGotRequest).To(Equal(tt.wantUsedProxy)) - }) } } From 7501e8622c5d135e7b1a7328e1a060620922ad3e Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Thu, 26 May 2022 15:16:52 +0530 Subject: [PATCH 4/8] add method to disable features internally Signed-off-by: Sanskar Jaiswal --- internal/features/features.go | 8 ++++++++ main.go | 7 +++---- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/internal/features/features.go b/internal/features/features.go index a7b4c1c21..c46847431 100644 --- a/internal/features/features.go +++ b/internal/features/features.go @@ -65,3 +65,11 @@ func FeatureGates() map[string]bool { func Enabled(feature string) (bool, error) { return feathelper.Enabled(feature) } + +// Disable disables the specified feature. If the feature is not +// present, it's a no-op +func Disable(feature string) { + if _, ok := features[feature]; ok { + features[feature] = false + } +} diff --git a/main.go b/main.go index fb54cb74a..660a89cdc 100644 --- a/main.go +++ b/main.go @@ -314,11 +314,10 @@ func main() { managed.InitManagedTransport(ctrl.Log.WithName("managed-transport")) } else { if optimize, _ := feathelper.Enabled(features.OptimizedGitClones); optimize { - setupLog.Error( - fmt.Errorf("OptimizedGitClones=true but GitManagedTransport=false"), - "git clones can only be optimized when using managed transort", + features.Disable(features.OptimizedGitClones) + setupLog.Info( + "disabling optimzied git clones; git clones can only be optimized when using managed transort", ) - os.Exit(1) } } From 94c50fa3a8504877c5084406a511f424a1f701a7 Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Thu, 26 May 2022 15:46:19 +0530 Subject: [PATCH 5/8] remvoe support for sha1 and md5 hashing for public keys Signed-off-by: Sanskar Jaiswal --- pkg/git/libgit2/managed/ssh.go | 2 +- pkg/git/libgit2/managed/transport.go | 12 +----------- pkg/git/libgit2/managed/transport_test.go | 23 ++++------------------- 3 files changed, 6 insertions(+), 31 deletions(-) diff --git a/pkg/git/libgit2/managed/ssh.go b/pkg/git/libgit2/managed/ssh.go index dddcadc09..ca0e02e3e 100644 --- a/pkg/git/libgit2/managed/ssh.go +++ b/pkg/git/libgit2/managed/ssh.go @@ -165,7 +165,7 @@ func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go. cert := &git2go.Certificate{ Kind: git2go.CertificateHostkey, Hostkey: git2go.HostkeyCertificate{ - Kind: git2go.HostkeySHA256, + Kind: git2go.HostkeySHA256 | git2go.HostkeyRaw, HashSHA256: sha256.Sum256(marshaledKey), Hostkey: marshaledKey, SSHPublicKey: key, diff --git a/pkg/git/libgit2/managed/transport.go b/pkg/git/libgit2/managed/transport.go index 763e6f8b2..502c82f62 100644 --- a/pkg/git/libgit2/managed/transport.go +++ b/pkg/git/libgit2/managed/transport.go @@ -1,8 +1,6 @@ package managed import ( - "crypto/md5" - "crypto/sha1" "crypto/sha256" "fmt" "hash" @@ -49,16 +47,8 @@ func KnownHostsCallback(host string, knownHosts []byte) git2go.CertificateCheckC case cert.Hostkey.Kind&git2go.HostkeySHA256 > 0: fingerprint = cert.Hostkey.HashSHA256[:] hasher = sha256.New() - // SHA1 and MD5 are present here, because they're used for unmanaged transport. - // TODO: get rid of this, when unmanaged transport is completely removed. - case cert.Hostkey.Kind&git2go.HostkeySHA1 > 0: - fingerprint = cert.Hostkey.HashSHA1[:] - hasher = sha1.New() - case cert.Hostkey.Kind&git2go.HostkeyMD5 > 0: - fingerprint = cert.Hostkey.HashMD5[:] - hasher = md5.New() default: - return fmt.Errorf("invalid host key kind, expected to be one of SHA256, SHA1, MD5") + return fmt.Errorf("invalid host key kind, expected to be of kind SHA256") } // We are now certain that the configured host and the hostname diff --git a/pkg/git/libgit2/managed/transport_test.go b/pkg/git/libgit2/managed/transport_test.go index 2428d599e..fc847ba66 100644 --- a/pkg/git/libgit2/managed/transport_test.go +++ b/pkg/git/libgit2/managed/transport_test.go @@ -29,7 +29,7 @@ func TestKnownHostsCallback(t *testing.T) { name: "Match", host: "github.com", knownHosts: []byte(knownHostsFixture), - hostkey: git2go.HostkeyCertificate{Kind: git2go.HostkeySHA1 | git2go.HostkeyMD5, HashSHA1: sha1Fingerprint("v2toJdKXfFEaR1u++4iq1UqSrHM")}, + hostkey: git2go.HostkeyCertificate{Kind: git2go.HostkeySHA256, HashSHA256: sha256Fingerprint("nThbg6kXUpJWGl7E1IGOCspRomTxdCARLviKw6E5SY8")}, expectedHost: "github.com", want: nil, }, @@ -37,7 +37,7 @@ func TestKnownHostsCallback(t *testing.T) { name: "Match with port", host: "github.com", knownHosts: []byte(knownHostsFixture), - hostkey: git2go.HostkeyCertificate{Kind: git2go.HostkeySHA1 | git2go.HostkeyMD5, HashSHA1: sha1Fingerprint("v2toJdKXfFEaR1u++4iq1UqSrHM")}, + hostkey: git2go.HostkeyCertificate{Kind: git2go.HostkeySHA256, HashSHA256: sha256Fingerprint("nThbg6kXUpJWGl7E1IGOCspRomTxdCARLviKw6E5SY8")}, expectedHost: "github.com:22", want: nil, }, @@ -45,7 +45,7 @@ func TestKnownHostsCallback(t *testing.T) { name: "Hostname mismatch", host: "github.com", knownHosts: []byte(knownHostsFixture), - hostkey: git2go.HostkeyCertificate{Kind: git2go.HostkeySHA1 | git2go.HostkeyMD5, HashSHA1: sha1Fingerprint("v2toJdKXfFEaR1u++4iq1UqSrHM")}, + hostkey: git2go.HostkeyCertificate{Kind: git2go.HostkeySHA256, HashSHA256: sha256Fingerprint("nThbg6kXUpJWGl7E1IGOCspRomTxdCARLviKw6E5SY8")}, expectedHost: "example.com", want: fmt.Errorf("host mismatch: %q %q", "example.com", "github.com"), }, @@ -53,7 +53,7 @@ func TestKnownHostsCallback(t *testing.T) { name: "Hostkey mismatch", host: "github.com", knownHosts: []byte(knownHostsFixture), - hostkey: git2go.HostkeyCertificate{Kind: git2go.HostkeyMD5, HashMD5: md5Fingerprint("\xb6\x03\x0e\x39\x97\x9e\xd0\xe7\x24\xce\xa3\x77\x3e\x01\x42\x09")}, + hostkey: git2go.HostkeyCertificate{Kind: git2go.HostkeySHA256, HashSHA256: sha256Fingerprint("ROQFvPThGrW4RuWLoL9tq9I9zJ42fK4XywyRtbOz/EQ")}, expectedHost: "github.com", want: fmt.Errorf("hostkey could not be verified"), }, @@ -73,21 +73,6 @@ func TestKnownHostsCallback(t *testing.T) { }) } } -func md5Fingerprint(in string) [16]byte { - var out [16]byte - copy(out[:], in) - return out -} - -func sha1Fingerprint(in string) [20]byte { - d, err := base64.RawStdEncoding.DecodeString(in) - if err != nil { - panic(err) - } - var out [20]byte - copy(out[:], d) - return out -} func sha256Fingerprint(in string) [32]byte { d, err := base64.RawStdEncoding.DecodeString(in) From 5152721ae0f1900a64a0e16a0452d750a21aafbd Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Fri, 27 May 2022 11:32:52 +0530 Subject: [PATCH 6/8] factor out unmanaged checkout into its own functions Signed-off-by: Sanskar Jaiswal --- pkg/git/libgit2/checkout.go | 94 +++++++++++++++++--------------- pkg/git/libgit2/checkout_test.go | 4 +- 2 files changed, 53 insertions(+), 45 deletions(-) diff --git a/pkg/git/libgit2/checkout.go b/pkg/git/libgit2/checkout.go index 83c602f81..3c49633bd 100644 --- a/pkg/git/libgit2/checkout.go +++ b/pkg/git/libgit2/checkout.go @@ -184,33 +184,37 @@ func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, opts *g return buildCommit(cc, "refs/heads/"+c.Branch), nil } else { - repo, err := git2go.Clone(url, path, &git2go.CloneOptions{ - FetchOptions: git2go.FetchOptions{ - DownloadTags: git2go.DownloadTagsNone, - RemoteCallbacks: RemoteCallbacks(ctx, opts), - ProxyOptions: git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, - }, - CheckoutOptions: git2go.CheckoutOptions{ - Strategy: git2go.CheckoutForce, - }, - CheckoutBranch: c.Branch, - }) - if err != nil { - return nil, fmt.Errorf("unable to clone '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) - } - defer repo.Free() - head, err := repo.Head() - if err != nil { - return nil, fmt.Errorf("git resolve HEAD error: %w", err) - } - defer head.Free() - cc, err := repo.LookupCommit(head.Target()) - if err != nil { - return nil, fmt.Errorf("failed to lookup HEAD commit '%s' for branch '%s': %w", head.Target(), c.Branch, err) - } - defer cc.Free() - return buildCommit(cc, "refs/heads/"+c.Branch), nil + return c.checkoutUnmanaged(ctx, path, url, opts) + } +} + +func (c *CheckoutBranch) checkoutUnmanaged(ctx context.Context, path, url string, opts *git.AuthOptions) (_ *git.Commit, err error) { + repo, err := git2go.Clone(url, path, &git2go.CloneOptions{ + FetchOptions: git2go.FetchOptions{ + DownloadTags: git2go.DownloadTagsNone, + RemoteCallbacks: RemoteCallbacks(ctx, opts), + ProxyOptions: git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, + }, + CheckoutOptions: git2go.CheckoutOptions{ + Strategy: git2go.CheckoutForce, + }, + CheckoutBranch: c.Branch, + }) + if err != nil { + return nil, fmt.Errorf("unable to clone '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) + } + defer repo.Free() + head, err := repo.Head() + if err != nil { + return nil, fmt.Errorf("git resolve HEAD error: %w", err) + } + defer head.Free() + cc, err := repo.LookupCommit(head.Target()) + if err != nil { + return nil, fmt.Errorf("failed to lookup HEAD commit '%s' for branch '%s': %w", head.Target(), c.Branch, err) } + defer cc.Free() + return buildCommit(cc, "refs/heads/"+c.Branch), nil } type CheckoutTag struct { @@ -305,26 +309,30 @@ func (c *CheckoutTag) Checkout(ctx context.Context, path, url string, opts *git. defer cc.Free() return buildCommit(cc, "refs/tags/"+c.Tag), nil } else { - repo, err := git2go.Clone(url, path, &git2go.CloneOptions{ - FetchOptions: git2go.FetchOptions{ - DownloadTags: git2go.DownloadTagsAll, - RemoteCallbacks: RemoteCallbacks(ctx, opts), - ProxyOptions: git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, - }, - }) - if err != nil { - return nil, fmt.Errorf("unable to clone '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) - } - defer repo.Free() - cc, err := checkoutDetachedDwim(repo, c.Tag) - if err != nil { - return nil, err - } - defer cc.Free() - return buildCommit(cc, "refs/tags/"+c.Tag), nil + return c.checkoutUnmanaged(ctx, path, url, opts) } } +func (c *CheckoutTag) checkoutUnmanaged(ctx context.Context, path, url string, opts *git.AuthOptions) (_ *git.Commit, err error) { + repo, err := git2go.Clone(url, path, &git2go.CloneOptions{ + FetchOptions: git2go.FetchOptions{ + DownloadTags: git2go.DownloadTagsAll, + RemoteCallbacks: RemoteCallbacks(ctx, opts), + ProxyOptions: git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, + }, + }) + if err != nil { + return nil, fmt.Errorf("unable to clone '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) + } + defer repo.Free() + cc, err := checkoutDetachedDwim(repo, c.Tag) + if err != nil { + return nil, err + } + defer cc.Free() + return buildCommit(cc, "refs/tags/"+c.Tag), nil +} + type CheckoutCommit struct { Commit string } diff --git a/pkg/git/libgit2/checkout_test.go b/pkg/git/libgit2/checkout_test.go index c2fe7a12c..46f8628c3 100644 --- a/pkg/git/libgit2/checkout_test.go +++ b/pkg/git/libgit2/checkout_test.go @@ -30,7 +30,7 @@ import ( . "github.com/onsi/gomega" ) -func TestCheckoutBranch_Checkout(t *testing.T) { +func TestCheckoutBranch_checkoutUnmanaged(t *testing.T) { repo, err := initBareRepo(t) if err != nil { t.Fatal(err) @@ -126,7 +126,7 @@ func TestCheckoutBranch_Checkout(t *testing.T) { } } -func TestCheckoutTag_Checkout(t *testing.T) { +func TestCheckoutTag_checkoutUnmanaged(t *testing.T) { type testTag struct { name string annotated bool From ec45a612b1d03963bf3798ea723b25bf7422e83f Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Fri, 27 May 2022 11:34:54 +0530 Subject: [PATCH 7/8] enable managed transport for controller tests Signed-off-by: Sanskar Jaiswal --- controllers/gitrepository_controller_test.go | 18 ++++++++---------- controllers/suite_test.go | 4 ++++ pkg/git/libgit2/managed/http.go | 2 +- pkg/git/libgit2/managed_test.go | 13 ++++--------- 4 files changed, 17 insertions(+), 20 deletions(-) diff --git a/controllers/gitrepository_controller_test.go b/controllers/gitrepository_controller_test.go index a30f608b0..50a9463fe 100644 --- a/controllers/gitrepository_controller_test.go +++ b/controllers/gitrepository_controller_test.go @@ -362,7 +362,7 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { }, wantErr: true, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.GitOperationFailedReason, "failed to checkout and determine revision: unable to clone '': PEM CA bundle could not be appended to x509 certificate pool"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.GitOperationFailedReason, "failed to checkout and determine revision: unable to fetch-connect to remote '': PEM CA bundle could not be appended to x509 certificate pool"), }, }, { @@ -645,11 +645,10 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) } conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "foo") }, - want: sreconcile.ResultEmpty, - wantErr: true, - wantRevision: "staging/", - wantArtifactOutdated: false, - skipForImplementation: "libgit2", + want: sreconcile.ResultEmpty, + wantErr: true, + wantRevision: "staging/", + wantArtifactOutdated: false, }, { name: "Optimized clone different ignore", @@ -670,10 +669,9 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) } conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "foo") }, - want: sreconcile.ResultSuccess, - wantRevision: "staging/", - wantArtifactOutdated: false, - skipForImplementation: "libgit2", + want: sreconcile.ResultSuccess, + wantRevision: "staging/", + wantArtifactOutdated: false, }, } diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 6531d633f..757ac78f7 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -39,6 +39,7 @@ import ( "github.com/fluxcd/pkg/runtime/controller" "github.com/fluxcd/pkg/runtime/testenv" "github.com/fluxcd/pkg/testserver" + "github.com/go-logr/logr" "github.com/phayes/freeport" "github.com/distribution/distribution/v3/configuration" @@ -50,6 +51,7 @@ import ( "github.com/fluxcd/source-controller/internal/cache" "github.com/fluxcd/source-controller/internal/features" "github.com/fluxcd/source-controller/internal/helm/registry" + "github.com/fluxcd/source-controller/pkg/git/libgit2/managed" // +kubebuilder:scaffold:imports ) @@ -207,6 +209,8 @@ func TestMain(m *testing.M) { panic(fmt.Sprintf("Failed to create OCI registry client")) } + managed.InitManagedTransport(logr.Discard()) + if err := (&GitRepositoryReconciler{ Client: testEnv, EventRecorder: record.NewFakeRecorder(32), diff --git a/pkg/git/libgit2/managed/http.go b/pkg/git/libgit2/managed/http.go index 1533e6bdd..937d10971 100644 --- a/pkg/git/libgit2/managed/http.go +++ b/pkg/git/libgit2/managed/http.go @@ -222,7 +222,7 @@ func createClientRequest(targetURL string, action git2go.SmartServiceAction, if len(authOpts.CAFile) > 0 { certPool := x509.NewCertPool() if ok := certPool.AppendCertsFromPEM(authOpts.CAFile); !ok { - return nil, nil, fmt.Errorf("failed to use certificate from PEM") + return nil, nil, fmt.Errorf("PEM CA bundle could not be appended to x509 certificate pool") } t.TLSClientConfig = &tls.Config{ RootCAs: certPool, diff --git a/pkg/git/libgit2/managed_test.go b/pkg/git/libgit2/managed_test.go index 8cb07016d..f5e290201 100644 --- a/pkg/git/libgit2/managed_test.go +++ b/pkg/git/libgit2/managed_test.go @@ -42,6 +42,10 @@ import ( const testRepositoryPath = "../testdata/git/repo" +func TestMain(m *testing.M) { + managed.InitManagedTransport(logr.Discard()) +} + // Test_ManagedSSH_KeyTypes assures support for the different // types of keys for SSH Authentication supported by Flux. func Test_ManagedSSH_KeyTypes(t *testing.T) { @@ -124,8 +128,6 @@ func Test_ManagedSSH_KeyTypes(t *testing.T) { knownHosts, err := ssh.ScanHostKey(u.Host, timeout, git.HostKeyAlgos, false) g.Expect(err).ToNot(HaveOccurred()) - managed.InitManagedTransport(logr.Discard()) - for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) @@ -223,8 +225,6 @@ func Test_ManagedSSH_KeyExchangeAlgos(t *testing.T) { }, } - managed.InitManagedTransport(logr.Discard()) - for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) @@ -382,8 +382,6 @@ func Test_ManagedSSH_HostKeyAlgos(t *testing.T) { }, } - managed.InitManagedTransport(logr.Discard()) - for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) @@ -475,7 +473,6 @@ func Test_ManagedHTTPCheckout(t *testing.T) { defer server.StopHTTP() // Force managed transport to be enabled - managed.InitManagedTransport(logr.Discard()) repoPath := "test.git" err = server.InitRepo("../testdata/git/repo", git.DefaultBranch, repoPath) @@ -501,7 +498,6 @@ func Test_ManagedHTTPCheckout(t *testing.T) { } func TestManagedCheckoutBranch_Checkout(t *testing.T) { - managed.InitManagedTransport(logr.Discard()) g := NewWithT(t) timeout := 5 * time.Second @@ -557,7 +553,6 @@ func TestManagedCheckoutBranch_Checkout(t *testing.T) { } func TestManagedCheckoutTag_Checkout(t *testing.T) { - managed.InitManagedTransport(logr.Discard()) g := NewWithT(t) timeout := 5 * time.Second From 972d1cac2a155a966a554b890bce00319880395b Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Fri, 27 May 2022 11:35:09 +0530 Subject: [PATCH 8/8] fix docs, error handling and managed proxy auth Signed-off-by: Sanskar Jaiswal --- controllers/gitrepository_controller.go | 4 ++-- internal/features/features.go | 2 +- main.go | 2 +- pkg/git/libgit2/managed/http.go | 13 ------------- pkg/git/libgit2/managed/options.go | 2 +- pkg/git/libgit2/managed_test.go | 2 -- pkg/git/strategy/proxy/strategy_proxy_test.go | 4 ++-- 7 files changed, 7 insertions(+), 22 deletions(-) diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index 04e404451..a185d1818 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -735,7 +735,7 @@ func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context, // managed GIT transport only affects the libgit2 implementation if managed.Enabled() && obj.Spec.GitImplementation == sourcev1.LibGit2Implementation { // We set the TransportOptionsURL of this set of authentication options here by constructing - // a unique ID that won't clash in a multi tenant environment. This unique ID is used by + // a unique URL that won't clash in a multi tenant environment. This unique URL is used by // libgit2 managed transports. This enables us to bypass the inbuilt credentials callback in // libgit2, which is inflexible and unstable. if strings.HasPrefix(obj.Spec.URL, "http") { @@ -745,7 +745,7 @@ func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context, } else { e := &serror.Stalling{ Err: fmt.Errorf("git repository URL has invalid transport type: '%s'", obj.Spec.URL), - Reason: sourcev1.GitOperationFailedReason, + Reason: sourcev1.URLInvalidReason, } return nil, e } diff --git a/internal/features/features.go b/internal/features/features.go index c46847431..9cc2cfd14 100644 --- a/internal/features/features.go +++ b/internal/features/features.go @@ -67,7 +67,7 @@ func Enabled(feature string) (bool, error) { } // Disable disables the specified feature. If the feature is not -// present, it's a no-op +// present, it's a no-op. func Disable(feature string) { if _, ok := features[feature]; ok { features[feature] = false diff --git a/main.go b/main.go index 660a89cdc..83d3cd429 100644 --- a/main.go +++ b/main.go @@ -316,7 +316,7 @@ func main() { if optimize, _ := feathelper.Enabled(features.OptimizedGitClones); optimize { features.Disable(features.OptimizedGitClones) setupLog.Info( - "disabling optimzied git clones; git clones can only be optimized when using managed transort", + "disabling optimized git clones; git clones can only be optimized when using managed transport", ) } } diff --git a/pkg/git/libgit2/managed/http.go b/pkg/git/libgit2/managed/http.go index 937d10971..fcfdc3fb2 100644 --- a/pkg/git/libgit2/managed/http.go +++ b/pkg/git/libgit2/managed/http.go @@ -47,7 +47,6 @@ import ( "bytes" "crypto/tls" "crypto/x509" - "encoding/base64" "errors" "fmt" "io" @@ -212,12 +211,6 @@ func createClientRequest(targetURL string, action git2go.SmartServiceAction, if authOpts != nil { if len(authOpts.Username) > 0 { req.SetBasicAuth(authOpts.Username, authOpts.Password) - if t.Proxy != nil { - t.ProxyConnectHeader.Set( - "Authorization", - "Basic "+basicAuth(authOpts.Username, authOpts.Password), - ) - } } if len(authOpts.CAFile) > 0 { certPool := x509.NewCertPool() @@ -413,9 +406,3 @@ func (self *httpSmartSubtransportStream) sendRequest() error { self.sentRequest = true return nil } - -// From: https://github.com/golang/go/blob/go1.18/src/net/http/client.go#L418 -func basicAuth(username, password string) string { - auth := username + ":" + password - return base64.StdEncoding.EncodeToString([]byte(auth)) -} diff --git a/pkg/git/libgit2/managed/options.go b/pkg/git/libgit2/managed/options.go index 900d593cc..3af0d914b 100644 --- a/pkg/git/libgit2/managed/options.go +++ b/pkg/git/libgit2/managed/options.go @@ -32,7 +32,7 @@ type TransportOptions struct { } var ( - // transportOpts maps a unique url to a set of transport options. + // transportOpts maps a unique URL to a set of transport options. transportOpts = make(map[string]TransportOptions, 0) m sync.RWMutex ) diff --git a/pkg/git/libgit2/managed_test.go b/pkg/git/libgit2/managed_test.go index f5e290201..a0ada1e0e 100644 --- a/pkg/git/libgit2/managed_test.go +++ b/pkg/git/libgit2/managed_test.go @@ -472,8 +472,6 @@ func Test_ManagedHTTPCheckout(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) defer server.StopHTTP() - // Force managed transport to be enabled - repoPath := "test.git" err = server.InitRepo("../testdata/git/repo", git.DefaultBranch, repoPath) g.Expect(err).ToNot(HaveOccurred()) diff --git a/pkg/git/strategy/proxy/strategy_proxy_test.go b/pkg/git/strategy/proxy/strategy_proxy_test.go index 5bb43a3a9..e575cd37e 100644 --- a/pkg/git/strategy/proxy/strategy_proxy_test.go +++ b/pkg/git/strategy/proxy/strategy_proxy_test.go @@ -320,10 +320,10 @@ func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) { defer proxyServer.Close() // Set the proxy env vars for both HTTP and HTTPS because go-git caches them. - os.Setenv("HTTPS_PROXY", fmt.Sprintf("http://%s", proxyAddr)) + os.Setenv("HTTPS_PROXY", fmt.Sprintf("http://smth:else@%s", proxyAddr)) defer os.Unsetenv("HTTPS_PROXY") - os.Setenv("HTTP_PROXY", fmt.Sprintf("http://%s", proxyAddr)) + os.Setenv("HTTP_PROXY", fmt.Sprintf("http://smth:else@%s", proxyAddr)) defer os.Unsetenv("HTTP_PROXY") os.Setenv("NO_PROXY", "*.0.2.1")