From 54e69d7307265584a5baae05adcd3e99629b1e4c Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Mon, 23 May 2022 20:50:35 +0530 Subject: [PATCH 1/9] 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 f3c4e5713..7270ff9cc 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 116aca703b6f8ba2f6b24c7cea8de61928b7eac5 Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Tue, 24 May 2022 22:05:29 +0530 Subject: [PATCH 2/9] separate managed and unmanaged code Signed-off-by: Sanskar Jaiswal --- controllers/gitrepository_controller.go | 42 ++-- go.mod | 2 +- go.sum | 4 +- pkg/git/libgit2/checkout.go | 30 ++- pkg/git/libgit2/managed/http.go | 8 +- pkg/git/libgit2/managed/managed_test.go | 48 ----- pkg/git/libgit2/managed/options.go | 7 + pkg/git/libgit2/managed/ssh.go | 7 +- pkg/git/libgit2/managed/ssh_test.go | 4 +- pkg/git/libgit2/managed/transport.go | 100 +++++++++ pkg/git/libgit2/managed/transport_test.go | 108 ++++++++++ pkg/git/libgit2/managed_test.go | 31 --- pkg/git/libgit2/transport.go | 169 +-------------- pkg/git/libgit2/transport_test.go | 242 ---------------------- pkg/git/options.go | 2 + 15 files changed, 285 insertions(+), 519 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 7270ff9cc..4884e204a 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -455,26 +455,8 @@ 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 { - return sreconcile.ResultEmpty, err - } // Observe if the artifacts still match the previous included ones if artifacts.Diff(obj.Status.IncludedArtifacts) { @@ -491,7 +473,27 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, optimizedClone = true } - c, err := r.gitCheckout(ctx, obj, repositoryURL, authOpts, dir, optimizedClone) + // 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(obj.Spec.URL, "http") { + authOpts.TransportAuthID = fmt.Sprintf("http://%s/%s/%d", obj.Name, obj.UID, obj.Generation) + } else if strings.HasPrefix(obj.Spec.URL, "ssh") { + authOpts.TransportAuthID = 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, + } + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e + } + } + + c, err := r.gitCheckout(ctx, obj, obj.Spec.URL, authOpts, dir, optimizedClone) if err != nil { e := serror.NewGeneric( fmt.Errorf("failed to checkout and determine revision: %w", err), @@ -530,7 +532,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, obj.Spec.URL, authOpts, dir, false) if err != nil { e := serror.NewGeneric( fmt.Errorf("failed to checkout and determine revision: %w", err), diff --git a/go.mod b/go.mod index 8661f23fc..947a5a969 100644 --- a/go.mod +++ b/go.mod @@ -28,7 +28,7 @@ require ( github.com/fluxcd/pkg/helmtestserver v0.5.0 github.com/fluxcd/pkg/lockedfile v0.1.0 github.com/fluxcd/pkg/runtime v0.16.0 - 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 fde3f65f2..0d356e28b 100644 --- a/go.sum +++ b/go.sum @@ -350,8 +350,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.0 h1:ynzvkOedFFZHlsa47EE7XtxZe8qs8edhtmjVZBEWi1Y= github.com/fluxcd/pkg/runtime v0.16.0/go.mod h1:Iklg+r/Jnqc9cNf2NK+iaosvw49CxX07Pyn0r3zSg/o= -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/pkg/git/libgit2/checkout.go b/pkg/git/libgit2/checkout.go index 56752951d..222348bd5 100644 --- a/pkg/git/libgit2/checkout.go +++ b/pkg/git/libgit2/checkout.go @@ -69,6 +69,8 @@ type CheckoutBranch struct { func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, opts *git.AuthOptions) (_ *git.Commit, err error) { defer recoverPanic(&err) + remoteCallBacks := RemoteCallbacks(ctx, opts) + 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 @@ -77,15 +79,18 @@ func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, opts *g // 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. + if opts.TransportAuthID == "" { + return nil, fmt.Errorf("can't use managed transport without a valid transport auth id.") + } managed.AddTransportOptions(opts.TransportAuthID, managed.TransportOptions{ TargetURL: url, AuthOpts: opts, }) url = opts.TransportAuthID + remoteCallBacks = managed.RemoteCallbacks() defer managed.RemoveTransportOptions(opts.TransportAuthID) } - remoteCallBacks := RemoteCallbacks(ctx, opts) proxyOpts := &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto} repo, remote, err := initializeRepoWithRemote(ctx, path, url, opts) @@ -186,16 +191,21 @@ type CheckoutTag struct { func (c *CheckoutTag) Checkout(ctx context.Context, path, url string, opts *git.AuthOptions) (_ *git.Commit, err error) { defer recoverPanic(&err) + remoteCallBacks := RemoteCallbacks(ctx, opts) + if managed.Enabled() { + if opts.TransportAuthID == "" { + return nil, fmt.Errorf("can't use managed transport without a valid transport auth id.") + } managed.AddTransportOptions(opts.TransportAuthID, managed.TransportOptions{ TargetURL: url, AuthOpts: opts, }) url = opts.TransportAuthID + remoteCallBacks = managed.RemoteCallbacks() defer managed.RemoveTransportOptions(opts.TransportAuthID) } - remoteCallBacks := RemoteCallbacks(ctx, opts) proxyOpts := &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto} repo, remote, err := initializeRepoWithRemote(ctx, path, url, opts) @@ -274,19 +284,25 @@ 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() { + if opts.TransportAuthID == "" { + return nil, fmt.Errorf("can't use managed transport without a valid transport auth id.") + } managed.AddTransportOptions(opts.TransportAuthID, managed.TransportOptions{ TargetURL: url, AuthOpts: opts, }) url = opts.TransportAuthID + remoteCallBacks = managed.RemoteCallbacks() defer managed.RemoveTransportOptions(opts.TransportAuthID) } repo, err := git2go.Clone(url, path, &git2go.CloneOptions{ FetchOptions: git2go.FetchOptions{ DownloadTags: git2go.DownloadTagsNone, - RemoteCallbacks: RemoteCallbacks(ctx, opts), + RemoteCallbacks: remoteCallBacks, ProxyOptions: git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, }, }) @@ -312,12 +328,18 @@ 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() { + if opts.TransportAuthID == "" { + return nil, fmt.Errorf("can't use managed transport without a valid transport auth id.") + } managed.AddTransportOptions(opts.TransportAuthID, managed.TransportOptions{ TargetURL: url, AuthOpts: opts, }) url = opts.TransportAuthID + remoteCallBacks = managed.RemoteCallbacks() defer managed.RemoveTransportOptions(opts.TransportAuthID) } @@ -329,7 +351,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), + RemoteCallbacks: remoteCallBacks, ProxyOptions: git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, }, }) diff --git a/pkg/git/libgit2/managed/http.go b/pkg/git/libgit2/managed/http.go index ffccb1f69..48afc39a9 100644 --- a/pkg/git/libgit2/managed/http.go +++ b/pkg/git/libgit2/managed/http.go @@ -157,6 +157,10 @@ func createClientRequest(transportAuthID string, action git2go.SmartServiceActio } targetURL := opts.TargetURL + if targetURL == "" { + return nil, nil, fmt.Errorf("repository URL cannot be empty") + } + if len(targetURL) > URLMaxLength { return nil, nil, fmt.Errorf("URL exceeds the max length (%d)", URLMaxLength) } @@ -197,7 +201,9 @@ func createClientRequest(transportAuthID string, action git2go.SmartServiceActio // Add any provided certificate to the http transport. if opts.AuthOpts != nil { - req.SetBasicAuth(opts.AuthOpts.Username, opts.AuthOpts.Password) + if len(opts.AuthOpts.Username) > 0 { + 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 { 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..122954f34 100644 --- a/pkg/git/libgit2/managed/options.go +++ b/pkg/git/libgit2/managed/options.go @@ -35,12 +35,19 @@ var ( m sync.RWMutex ) +// AddTransportOptions registers a TransportOptions object mapped to the +// provided id. The id must be a valid url, i.e. prefixed with http or ssh, +// as the id 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(id string, opts TransportOptions) { m.Lock() transportOpts[id] = opts m.Unlock() } +// RemoveTransportOptions removes the registerd TransportOptions object +// mapped to the provided id. func RemoveTransportOptions(id string) { m.Lock() delete(transportOpts, id) diff --git a/pkg/git/libgit2/managed/ssh.go b/pkg/git/libgit2/managed/ssh.go index 3895fbe4e..be242e467 100644 --- a/pkg/git/libgit2/managed/ssh.go +++ b/pkg/git/libgit2/managed/ssh.go @@ -167,7 +167,7 @@ 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, + Kind: git2go.HostkeySHA256, HashMD5: md5.Sum(marshaledKey), HashSHA1: sha1.Sum(marshaledKey), HashSHA256: sha256.Sum256(marshaledKey), @@ -176,7 +176,10 @@ func (t *sshSmartSubtransport) Action(credentialsID string, action git2go.SmartS }, } - 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..e6ccb8548 100644 --- a/pkg/git/libgit2/managed/ssh_test.go +++ b/pkg/git/libgit2/managed/ssh_test.go @@ -113,7 +113,9 @@ func TestSSHManagedTransport_E2E(t *testing.T) { // 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{}, + 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..8b2abc39a --- /dev/null +++ b/pkg/git/libgit2/managed/transport.go @@ -0,0 +1,100 @@ +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) + } + + // 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) + var fingerprint []byte + var hasher hash.Hash + switch { + case cert.Hostkey.Kind&git2go.HostkeySHA256 > 0: + fingerprint = cert.Hostkey.HashSHA256[:] + hasher = sha256.New() + 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") + } + 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..3c289abae 100644 --- a/pkg/git/libgit2/managed_test.go +++ b/pkg/git/libgit2/managed_test.go @@ -123,7 +123,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,16 +138,6 @@ 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, @@ -233,7 +222,6 @@ func Test_ManagedSSH_KeyExchangeAlgos(t *testing.T) { }, } - os.Setenv("EXPERIMENTAL_GIT_TRANSPORT", "true") managed.InitManagedTransport(logr.Discard()) for _, tt := range tests { @@ -280,15 +268,6 @@ 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, @@ -402,7 +381,6 @@ func Test_ManagedSSH_HostKeyAlgos(t *testing.T) { }, } - os.Setenv("EXPERIMENTAL_GIT_TRANSPORT", "true") managed.InitManagedTransport(logr.Discard()) for _, tt := range tests { @@ -459,15 +437,6 @@ 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, 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..cb634b23a 100644 --- a/pkg/git/options.go +++ b/pkg/git/options.go @@ -76,6 +76,8 @@ type AuthOptions struct { // 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. + // It must be prefixed with a valid transport protocol (ssh or http) because + // of the way managed transports are registered and invoked. TransportAuthID string } From 8926edf8be0cad5cecf30448743fe33a18580ead Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Wed, 25 May 2022 02:04:34 +0530 Subject: [PATCH 3/9] address comments Signed-off-by: Sanskar Jaiswal --- controllers/gitrepository_controller.go | 62 +++++++++++++++---------- pkg/git/libgit2/checkout.go | 40 ++++++++-------- pkg/git/libgit2/managed/http.go | 12 ++--- pkg/git/libgit2/managed/http_test.go | 6 +-- pkg/git/libgit2/managed/options.go | 26 +++++------ pkg/git/libgit2/managed/ssh.go | 10 ++-- pkg/git/libgit2/managed/ssh_test.go | 8 ++-- pkg/git/libgit2/managed/transport.go | 13 ++++-- pkg/git/libgit2/managed_test.go | 10 ++-- pkg/git/options.go | 13 ++++-- 10 files changed, 107 insertions(+), 93 deletions(-) diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index 4884e204a..71b047b11 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -473,28 +473,15 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, optimizedClone = true } - // 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(obj.Spec.URL, "http") { - authOpts.TransportAuthID = fmt.Sprintf("http://%s/%s/%d", obj.Name, obj.UID, obj.Generation) - } else if strings.HasPrefix(obj.Spec.URL, "ssh") { - authOpts.TransportAuthID = 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, - } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) - return sreconcile.ResultEmpty, e - } - } - - c, err := r.gitCheckout(ctx, obj, obj.Spec.URL, authOpts, dir, optimizedClone) + c, err := r.gitCheckout(ctx, obj, authOpts, dir, optimizedClone) if err != nil { + // If the error is a Stalling error, make sure to record and return that, because returning a Generic + // error, would force us to lose context. + var stalling *serror.Stalling + if errors.As(err, &stalling) { + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, stalling.Reason, stalling.Err.Error()) + return sreconcile.ResultEmpty, stalling + } e := serror.NewGeneric( fmt.Errorf("failed to checkout and determine revision: %w", err), sourcev1.GitOperationFailedReason, @@ -532,8 +519,15 @@ 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, obj.Spec.URL, authOpts, dir, false) + c, err := r.gitCheckout(ctx, obj, authOpts, dir, false) if err != nil { + // If the error is a Stalling error, make sure to record and return that, because returning a Generic + // error, would force us to lose context. + var stalling *serror.Stalling + if errors.As(err, &stalling) { + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, stalling.Reason, stalling.Err.Error()) + return sreconcile.ResultEmpty, stalling + } e := serror.NewGeneric( fmt.Errorf("failed to checkout and determine revision: %w", err), sourcev1.GitOperationFailedReason, @@ -729,7 +723,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 { @@ -755,15 +749,33 @@ 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() - return checkoutStrategy.Checkout(gitCtx, dir, repoURL, authOpts) + return checkoutStrategy.Checkout(gitCtx, dir, obj.Spec.URL, authOpts) } // fetchIncludes fetches artifact metadata of all the included repos. diff --git a/pkg/git/libgit2/checkout.go b/pkg/git/libgit2/checkout.go index 222348bd5..60c9ea3b1 100644 --- a/pkg/git/libgit2/checkout.go +++ b/pkg/git/libgit2/checkout.go @@ -72,23 +72,23 @@ func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, opts *g remoteCallBacks := RemoteCallbacks(ctx, opts) 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. - if opts.TransportAuthID == "" { + // _actual_ target URL and the correct credentials to use. + if opts.TransportOptionsURL == "" { return nil, fmt.Errorf("can't use managed transport without a valid transport auth id.") } - managed.AddTransportOptions(opts.TransportAuthID, managed.TransportOptions{ + managed.AddTransportOptions(opts.TransportOptionsURL, managed.TransportOptions{ TargetURL: url, AuthOpts: opts, }) - url = opts.TransportAuthID + url = opts.TransportOptionsURL remoteCallBacks = managed.RemoteCallbacks() - defer managed.RemoveTransportOptions(opts.TransportAuthID) + defer managed.RemoveTransportOptions(opts.TransportOptionsURL) } proxyOpts := &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto} @@ -194,16 +194,16 @@ func (c *CheckoutTag) Checkout(ctx context.Context, path, url string, opts *git. remoteCallBacks := RemoteCallbacks(ctx, opts) if managed.Enabled() { - if opts.TransportAuthID == "" { + if opts.TransportOptionsURL == "" { return nil, fmt.Errorf("can't use managed transport without a valid transport auth id.") } - managed.AddTransportOptions(opts.TransportAuthID, managed.TransportOptions{ + managed.AddTransportOptions(opts.TransportOptionsURL, managed.TransportOptions{ TargetURL: url, AuthOpts: opts, }) - url = opts.TransportAuthID + url = opts.TransportOptionsURL remoteCallBacks = managed.RemoteCallbacks() - defer managed.RemoveTransportOptions(opts.TransportAuthID) + defer managed.RemoveTransportOptions(opts.TransportOptionsURL) } proxyOpts := &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto} @@ -287,16 +287,16 @@ func (c *CheckoutCommit) Checkout(ctx context.Context, path, url string, opts *g remoteCallBacks := RemoteCallbacks(ctx, opts) if managed.Enabled() { - if opts.TransportAuthID == "" { + if opts.TransportOptionsURL == "" { return nil, fmt.Errorf("can't use managed transport without a valid transport auth id.") } - managed.AddTransportOptions(opts.TransportAuthID, managed.TransportOptions{ + managed.AddTransportOptions(opts.TransportOptionsURL, managed.TransportOptions{ TargetURL: url, AuthOpts: opts, }) - url = opts.TransportAuthID + url = opts.TransportOptionsURL remoteCallBacks = managed.RemoteCallbacks() - defer managed.RemoveTransportOptions(opts.TransportAuthID) + defer managed.RemoveTransportOptions(opts.TransportOptionsURL) } repo, err := git2go.Clone(url, path, &git2go.CloneOptions{ @@ -331,16 +331,16 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, opts *g remoteCallBacks := RemoteCallbacks(ctx, opts) if managed.Enabled() { - if opts.TransportAuthID == "" { + if opts.TransportOptionsURL == "" { return nil, fmt.Errorf("can't use managed transport without a valid transport auth id.") } - managed.AddTransportOptions(opts.TransportAuthID, managed.TransportOptions{ + managed.AddTransportOptions(opts.TransportOptionsURL, managed.TransportOptions{ TargetURL: url, AuthOpts: opts, }) - url = opts.TransportAuthID + url = opts.TransportOptionsURL remoteCallBacks = managed.RemoteCallbacks() - defer managed.RemoveTransportOptions(opts.TransportAuthID) + defer managed.RemoveTransportOptions(opts.TransportOptionsURL) } verConstraint, err := semver.NewConstraint(c.SemVer) diff --git a/pkg/git/libgit2/managed/http.go b/pkg/git/libgit2/managed/http.go index 48afc39a9..e288fd9dc 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(transportAuthID string, action git2go.SmartServiceAction) (git2go.SmartSubtransportStream, error) { +func (t *httpSmartSubtransport) Action(transportOptionsURL 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(transportAuthID string, action git2go.Sma t.httpTransport.Proxy = proxyFn t.httpTransport.DisableCompression = false - client, req, err := createClientRequest(transportAuthID, action, t.httpTransport) + client, req, err := createClientRequest(transportOptionsURL, action, t.httpTransport) if err != nil { return nil, err } @@ -142,7 +142,7 @@ 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(transportOptionsURL string, action git2go.SmartServiceAction, t *http.Transport) (*http.Client, *http.Request, error) { var req *http.Request var err error @@ -150,10 +150,10 @@ func createClientRequest(transportAuthID string, action git2go.SmartServiceActio return nil, nil, fmt.Errorf("failed to create client: transport cannot be nil") } - opts, found := getTransportOptions(transportAuthID) + opts, found := getTransportOptions(transportOptionsURL) if !found { - return nil, nil, fmt.Errorf("failed to create client: could not find transport options for the object: %s", transportAuthID) + return nil, nil, fmt.Errorf("failed to create client: could not find transport options for the object: %s", transportOptionsURL) } targetURL := opts.TargetURL @@ -199,7 +199,7 @@ func createClientRequest(transportAuthID string, action git2go.SmartServiceActio return nil, nil, err } - // Add any provided certificate to the http transport. + // Apply authentication and TLS settings to the HTTP transport. if opts.AuthOpts != nil { if len(opts.AuthOpts.Username) > 0 { req.SetBasicAuth(opts.AuthOpts.Username, opts.AuthOpts.Password) diff --git a/pkg/git/libgit2/managed/http_test.go b/pkg/git/libgit2/managed/http_test.go index bf54de597..3a9fd8cf0 100644 --- a/pkg/git/libgit2/managed/http_test.go +++ b/pkg/git/libgit2/managed/http_test.go @@ -198,9 +198,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/options.go b/pkg/git/libgit2/managed/options.go index 122954f34..c27b8c880 100644 --- a/pkg/git/libgit2/managed/options.go +++ b/pkg/git/libgit2/managed/options.go @@ -30,33 +30,33 @@ type TransportOptions struct { } 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 ) // AddTransportOptions registers a TransportOptions object mapped to the -// provided id. The id must be a valid url, i.e. prefixed with http or ssh, -// as the id is used as a dummy url for all git operations and the managed +// 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(id string, opts TransportOptions) { +func AddTransportOptions(transportOptsURL string, opts TransportOptions) { m.Lock() - transportOpts[id] = opts + transportOpts[transportOptsURL] = opts m.Unlock() } // RemoveTransportOptions removes the registerd TransportOptions object // mapped to the provided id. -func RemoveTransportOptions(id string) { +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 { @@ -70,16 +70,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 be242e467..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) @@ -168,8 +166,6 @@ func (t *sshSmartSubtransport) Action(credentialsID string, action git2go.SmartS Kind: git2go.CertificateHostkey, Hostkey: git2go.HostkeyCertificate{ Kind: git2go.HostkeySHA256, - HashMD5: md5.Sum(marshaledKey), - HashSHA1: sha1.Sum(marshaledKey), HashSHA256: sha256.Sum256(marshaledKey), Hostkey: marshaledKey, SSHPublicKey: key, diff --git a/pkg/git/libgit2/managed/ssh_test.go b/pkg/git/libgit2/managed/ssh_test.go index e6ccb8548..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,9 +110,9 @@ 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{ + repo, err := git2go.Clone(transportOptsURL, tmpDir, &git2go.CloneOptions{ FetchOptions: git2go.FetchOptions{ RemoteCallbacks: RemoteCallbacks(), }, diff --git a/pkg/git/libgit2/managed/transport.go b/pkg/git/libgit2/managed/transport.go index 8b2abc39a..763e6f8b2 100644 --- a/pkg/git/libgit2/managed/transport.go +++ b/pkg/git/libgit2/managed/transport.go @@ -43,17 +43,14 @@ func KnownHostsCallback(host string, knownHosts []byte) git2go.CertificateCheckC 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) 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() @@ -63,6 +60,12 @@ func KnownHostsCallback(host string, knownHosts []byte) git2go.CertificateCheckC 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 diff --git a/pkg/git/libgit2/managed_test.go b/pkg/git/libgit2/managed_test.go index 3c289abae..263ddcab9 100644 --- a/pkg/git/libgit2/managed_test.go +++ b/pkg/git/libgit2/managed_test.go @@ -142,7 +142,7 @@ func Test_ManagedSSH_KeyTypes(t *testing.T) { Identity: kp.PrivateKey, KnownHosts: knownHosts, } - authOpts.TransportAuthID = "ssh://" + getTransportAuthID() + authOpts.TransportOptionsURL = getTransportOptionsURL() // Prepare for checkout. branchCheckoutStrat := &CheckoutBranch{Branch: git.DefaultBranch} @@ -272,7 +272,7 @@ func Test_ManagedSSH_KeyExchangeAlgos(t *testing.T) { Identity: kp.PrivateKey, KnownHosts: knownHosts, } - authOpts.TransportAuthID = "ssh://" + getTransportAuthID() + authOpts.TransportOptionsURL = getTransportOptionsURL() // Prepare for checkout. branchCheckoutStrat := &CheckoutBranch{Branch: git.DefaultBranch} @@ -441,7 +441,7 @@ func Test_ManagedSSH_HostKeyAlgos(t *testing.T) { Identity: kp.PrivateKey, KnownHosts: knownHosts, } - authOpts.TransportAuthID = "ssh://" + getTransportAuthID() + authOpts.TransportOptionsURL = getTransportOptionsURL() // Prepare for checkout. branchCheckoutStrat := &CheckoutBranch{Branch: git.DefaultBranch} @@ -457,11 +457,11 @@ func Test_ManagedSSH_HostKeyAlgos(t *testing.T) { } } -func getTransportAuthID() string { +func getTransportOptionsURL() string { letterRunes := []rune("abcdefghijklmnopqrstuvwxyz1234567890") b := make([]rune, 10) for i := range b { b[i] = letterRunes[rand.Intn(len(letterRunes))] } - return string(b) + return "ssh://" + string(b) } diff --git a/pkg/git/options.go b/pkg/git/options.go index cb634b23a..a9169a590 100644 --- a/pkg/git/options.go +++ b/pkg/git/options.go @@ -72,13 +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. - // It must be prefixed with a valid transport protocol (ssh or http) because + // 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. - TransportAuthID string + // 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 3d4d84cebb18d2340eef85b4fe24711ffc07d706 Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Wed, 25 May 2022 20:49:56 +0530 Subject: [PATCH 4/9] further separate managed and unmanaged code; fix proxy on managed Signed-off-by: Sanskar Jaiswal --- main.go | 8 + pkg/git/libgit2/checkout.go | 360 +++++++++++++++------------ pkg/git/libgit2/checkout_test.go | 116 +++------ pkg/git/libgit2/managed/http.go | 100 +++++--- pkg/git/libgit2/managed/http_test.go | 94 ++++--- pkg/git/libgit2/managed/options.go | 6 +- pkg/git/libgit2/managed_test.go | 161 +++++++++++- 7 files changed, 511 insertions(+), 334 deletions(-) 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 60c9ea3b1..900de159b 100644 --- a/pkg/git/libgit2/checkout.go +++ b/pkg/git/libgit2/checkout.go @@ -69,8 +69,10 @@ type CheckoutBranch struct { func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, opts *git.AuthOptions) (_ *git.Commit, err error) { defer recoverPanic(&err) - remoteCallBacks := RemoteCallbacks(ctx, opts) - + // This branching is temproary, 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 TransportOptionsURL, because managed transports don't provide a way for any kind of @@ -79,108 +81,136 @@ func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, opts *g // 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. + 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, + TargetURL: url, + AuthOpts: opts, + ProxyOptions: &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, }) url = opts.TransportOptionsURL - remoteCallBacks = managed.RemoteCallbacks() + remoteCallBacks := managed.RemoteCallbacks() defer managed.RemoveTransportOptions(opts.TransportOptionsURL) - } - - 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, err + } + // Open remote connection. + err = remote.ConnectFetch(&remoteCallBacks, nil, nil) 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, + 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 { @@ -191,90 +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) - remoteCallBacks := RemoteCallbacks(ctx, opts) - + // This branching is temproary, 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() { 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, + TargetURL: url, + AuthOpts: opts, + ProxyOptions: &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, }) url = opts.TransportOptionsURL - remoteCallBacks = managed.RemoteCallbacks() + remoteCallBacks := managed.RemoteCallbacks() defer managed.RemoveTransportOptions(opts.TransportOptionsURL) - } - - 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, err + } + // Open remote connection. + err = remote.ConnectFetch(&remoteCallBacks, nil, nil) 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 { + 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 { @@ -291,8 +339,9 @@ func (c *CheckoutCommit) Checkout(ctx context.Context, path, url string, opts *g 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, + TargetURL: url, + AuthOpts: opts, + ProxyOptions: &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, }) url = opts.TransportOptionsURL remoteCallBacks = managed.RemoteCallbacks() @@ -303,7 +352,6 @@ func (c *CheckoutCommit) Checkout(ctx context.Context, path, url string, opts *g FetchOptions: git2go.FetchOptions{ DownloadTags: git2go.DownloadTagsNone, RemoteCallbacks: remoteCallBacks, - ProxyOptions: git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, }, }) if err != nil { @@ -335,8 +383,9 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, opts *g 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, + TargetURL: url, + AuthOpts: opts, + ProxyOptions: &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, }) url = opts.TransportOptionsURL remoteCallBacks = managed.RemoteCallbacks() @@ -352,7 +401,6 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, opts *g FetchOptions: git2go.FetchOptions{ DownloadTags: git2go.DownloadTagsAll, RemoteCallbacks: remoteCallBacks, - ProxyOptions: git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, }, }) 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 e288fd9dc..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" ) @@ -87,29 +89,44 @@ type httpSmartSubtransport struct { } func (t *httpSmartSubtransport) Action(transportOptionsURL 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 + 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(transportOptionsURL, 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(transportOptionsURL string, action git2go return stream, nil } -func createClientRequest(transportOptionsURL 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,21 +168,6 @@ func createClientRequest(transportOptionsURL string, action git2go.SmartServiceA return nil, nil, fmt.Errorf("failed to create client: transport cannot be nil") } - opts, found := getTransportOptions(transportOptionsURL) - - if !found { - return nil, nil, fmt.Errorf("failed to create client: could not find transport options for the object: %s", transportOptionsURL) - } - targetURL := opts.TargetURL - - if targetURL == "" { - return nil, nil, fmt.Errorf("repository URL cannot be empty") - } - - if len(targetURL) > URLMaxLength { - return nil, nil, fmt.Errorf("URL exceeds the max length (%d)", URLMaxLength) - } - client := &http.Client{ Transport: t, Timeout: fullHttpClientTimeOut, @@ -180,6 +183,9 @@ func createClientRequest(transportOptionsURL string, action git2go.SmartServiceA 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) @@ -190,6 +196,9 @@ func createClientRequest(transportOptionsURL string, action git2go.SmartServiceA 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") @@ -200,13 +209,19 @@ func createClientRequest(transportOptionsURL string, action git2go.SmartServiceA } // Apply authentication and TLS settings to the HTTP transport. - if opts.AuthOpts != nil { - if len(opts.AuthOpts.Username) > 0 { - req.SetBasicAuth(opts.AuthOpts.Username, opts.AuthOpts.Password) + 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(opts.AuthOpts.CAFile) > 0 { + 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{ @@ -216,6 +231,9 @@ func createClientRequest(transportOptionsURL string, action git2go.SmartServiceA } 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 } @@ -395,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 3a9fd8cf0..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, diff --git a/pkg/git/libgit2/managed/options.go b/pkg/git/libgit2/managed/options.go index c27b8c880..900d593cc 100644 --- a/pkg/git/libgit2/managed/options.go +++ b/pkg/git/libgit2/managed/options.go @@ -20,13 +20,15 @@ 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 ( diff --git a/pkg/git/libgit2/managed_test.go b/pkg/git/libgit2/managed_test.go index 263ddcab9..9f6f48f36 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" ) @@ -142,7 +143,7 @@ func Test_ManagedSSH_KeyTypes(t *testing.T) { Identity: kp.PrivateKey, KnownHosts: knownHosts, } - authOpts.TransportOptionsURL = getTransportOptionsURL() + authOpts.TransportOptionsURL = getTransportOptionsURL("ssh") // Prepare for checkout. branchCheckoutStrat := &CheckoutBranch{Branch: git.DefaultBranch} @@ -272,7 +273,7 @@ func Test_ManagedSSH_KeyExchangeAlgos(t *testing.T) { Identity: kp.PrivateKey, KnownHosts: knownHosts, } - authOpts.TransportOptionsURL = getTransportOptionsURL() + authOpts.TransportOptionsURL = getTransportOptionsURL("ssh") // Prepare for checkout. branchCheckoutStrat := &CheckoutBranch{Branch: git.DefaultBranch} @@ -441,7 +442,7 @@ func Test_ManagedSSH_HostKeyAlgos(t *testing.T) { Identity: kp.PrivateKey, KnownHosts: knownHosts, } - authOpts.TransportOptionsURL = getTransportOptionsURL() + authOpts.TransportOptionsURL = getTransportOptionsURL("ssh") // Prepare for checkout. branchCheckoutStrat := &CheckoutBranch{Branch: git.DefaultBranch} @@ -457,11 +458,161 @@ func Test_ManagedSSH_HostKeyAlgos(t *testing.T) { } } -func getTransportOptionsURL() 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("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("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("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 string) string { letterRunes := []rune("abcdefghijklmnopqrstuvwxyz1234567890") b := make([]rune, 10) for i := range b { b[i] = letterRunes[rand.Intn(len(letterRunes))] } - return "ssh://" + string(b) + return transport + "://" + string(b) } From bc8dedb080a3e544448cb54015fbafe956fa09a6 Mon Sep 17 00:00:00 2001 From: Sunny Date: Wed, 25 May 2022 02:48:36 +0530 Subject: [PATCH 5/9] helmrepo: Fix test flake in type update test In TestHelmRepositoryReconciler_ReconcileTypeUpdatePredicateFilter, when the type of HelmRepo is updated and immediately checked for the object to be ready, if the check happens before the client cache is updated, it results in observing the object to be ready in the previous generation. This results in status check failure: ``` [Check-FAIL]: [Ready condition must be False when the ObservedGeneration is less than the object Generation, Ready condition must be False when any of the status condition's ObservedGeneration is less than the object Generation: [Ready ArtifactInStorage]] ``` Explicitly look for the object with the next generation to prevent such failure. Signed-off-by: Sunny --- controllers/helmrepository_controller_test.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/controllers/helmrepository_controller_test.go b/controllers/helmrepository_controller_test.go index 2230a72e3..0acf0c41e 100644 --- a/controllers/helmrepository_controller_test.go +++ b/controllers/helmrepository_controller_test.go @@ -1166,9 +1166,11 @@ func TestHelmRepositoryReconciler_ReconcileTypeUpdatePredicateFilter(t *testing. Name: secret.Name, } + oldGen := obj.GetGeneration() g.Expect(testEnv.Update(ctx, obj)).To(Succeed()) + newGen := oldGen + 1 - // Wait for HelmRepository to be Ready + // Wait for HelmRepository to be Ready with new generation. g.Eventually(func() bool { if err := testEnv.Get(ctx, key, obj); err != nil { return false @@ -1178,8 +1180,8 @@ func TestHelmRepositoryReconciler_ReconcileTypeUpdatePredicateFilter(t *testing. } readyCondition := conditions.Get(obj, meta.ReadyCondition) return readyCondition.Status == metav1.ConditionTrue && - obj.Generation == readyCondition.ObservedGeneration && - obj.Generation == obj.Status.ObservedGeneration + newGen == readyCondition.ObservedGeneration && + newGen == obj.Status.ObservedGeneration }, timeout).Should(BeTrue()) // Check if the object status is valid. From 28663743fb2cc401746be46bf77104484dc1832f Mon Sep 17 00:00:00 2001 From: Sunny Date: Tue, 24 May 2022 20:36:47 +0530 Subject: [PATCH 6/9] reconcile: Set observed gen when conditions exist The observed generation must be set only when actual observation is made. When an actual observation is made, some conditions are set on the object. Introduce a helper function addPatchOptionWithStatusObservedGeneration() to set the patcher option WithStatusObservedGeneration only when there's any condition in the status. Updates the existing tests that depended on this behavior. This fixes the issue where the observed generation is set by the patcher when a reconciler does an early return for setting the finalizers only. With this, the observed generation will be updated only when some observations are made on the object based on the usual rules of success result, no error, ignore error and stalled condition. Signed-off-by: Sunny --- internal/reconcile/reconcile.go | 22 +++++++-- internal/reconcile/reconcile_test.go | 69 +++++++++++++++++++++++++++- 2 files changed, 85 insertions(+), 6 deletions(-) diff --git a/internal/reconcile/reconcile.go b/internal/reconcile/reconcile.go index b1e11409a..5e3b21e4c 100644 --- a/internal/reconcile/reconcile.go +++ b/internal/reconcile/reconcile.go @@ -128,11 +128,11 @@ func ComputeReconcileResult(obj conditions.Setter, res Result, recErr error, rb switch t := recErr.(type) { case *serror.Stalling: if res == ResultEmpty { + conditions.MarkStalled(obj, t.Reason, t.Error()) // The current generation has been reconciled successfully and it // has resulted in a stalled state. Return no error to stop further // requeuing. - pOpts = append(pOpts, patch.WithStatusObservedGeneration{}) - conditions.MarkStalled(obj, t.Reason, t.Error()) + pOpts = addPatchOptionWithStatusObservedGeneration(obj, pOpts) return pOpts, result, nil } // NOTE: Non-empty result with stalling error indicates that the @@ -150,7 +150,7 @@ func ComputeReconcileResult(obj conditions.Setter, res Result, recErr error, rb if t.Ignore { // The current generation has been reconciled successfully with // no-op result. Update status observed generation. - pOpts = append(pOpts, patch.WithStatusObservedGeneration{}) + pOpts = addPatchOptionWithStatusObservedGeneration(obj, pOpts) conditions.Delete(obj, meta.ReconcilingCondition) return pOpts, result, nil } @@ -159,7 +159,7 @@ func ComputeReconcileResult(obj conditions.Setter, res Result, recErr error, rb // state. If a requeue is requested, the current generation has not been // reconciled successfully. if res != ResultRequeue { - pOpts = append(pOpts, patch.WithStatusObservedGeneration{}) + pOpts = addPatchOptionWithStatusObservedGeneration(obj, pOpts) } conditions.Delete(obj, meta.StalledCondition) default: @@ -207,3 +207,17 @@ func FailureRecovery(oldObj, newObj conditions.Getter, failConditions []string) } return failuresBefore > 0 } + +// addPatchOptionWithStatusObservedGeneration adds patch option +// WithStatusObservedGeneration to the provided patch option slice only if there +// is any condition present on the object, and returns it. This is necessary to +// prevent setting status observed generation without any effectual observation. +// An object must have some condition in the status if it has been observed. +// TODO: Move this to fluxcd/pkg/runtime/patch package after it has proven its +// need. +func addPatchOptionWithStatusObservedGeneration(obj conditions.Setter, opts []patch.Option) []patch.Option { + if len(obj.GetConditions()) > 0 { + opts = append(opts, patch.WithStatusObservedGeneration{}) + } + return opts +} diff --git a/internal/reconcile/reconcile_test.go b/internal/reconcile/reconcile_test.go index 3d3f4fc0a..b9b2ccfea 100644 --- a/internal/reconcile/reconcile_test.go +++ b/internal/reconcile/reconcile_test.go @@ -71,11 +71,17 @@ func TestComputeReconcileResult(t *testing.T) { afterFunc func(t *WithT, obj conditions.Setter, patchOpts *patch.HelperOptions) }{ { - name: "successful result", - result: ResultSuccess, + name: "successful result", + result: ResultSuccess, + beforeFunc: func(obj conditions.Setter) { + conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "foo") + }, recErr: nil, wantResult: ctrl.Result{RequeueAfter: testSuccessInterval}, wantErr: false, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "foo"), + }, afterFunc: func(t *WithT, obj conditions.Setter, patchOpts *patch.HelperOptions) { t.Expect(patchOpts.IncludeStatusObservedGeneration).To(BeTrue()) }, @@ -85,10 +91,14 @@ func TestComputeReconcileResult(t *testing.T) { result: ResultSuccess, beforeFunc: func(obj conditions.Setter) { conditions.MarkReconciling(obj, "NewRevision", "new revision") + conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "foo") }, recErr: nil, wantResult: ctrl.Result{RequeueAfter: testSuccessInterval}, wantErr: false, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "foo"), + }, afterFunc: func(t *WithT, obj conditions.Setter, patchOpts *patch.HelperOptions) { t.Expect(patchOpts.IncludeStatusObservedGeneration).To(BeTrue()) t.Expect(conditions.IsUnknown(obj, meta.ReconcilingCondition)).To(BeTrue()) @@ -367,3 +377,58 @@ func TestFailureRecovery(t *testing.T) { }) } } + +func TestAddOptionWithStatusObservedGeneration(t *testing.T) { + tests := []struct { + name string + beforeFunc func(obj conditions.Setter) + patchOpts []patch.Option + want bool + }{ + { + name: "no conditions", + want: false, + }, + { + name: "some condition", + beforeFunc: func(obj conditions.Setter) { + conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "foo") + }, + want: true, + }, + { + name: "existing option with conditions", + beforeFunc: func(obj conditions.Setter) { + conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "foo") + }, + patchOpts: []patch.Option{patch.WithForceOverwriteConditions{}, patch.WithStatusObservedGeneration{}}, + want: true, + }, + { + name: "existing option, no conditions, can't remove", + patchOpts: []patch.Option{patch.WithForceOverwriteConditions{}, patch.WithStatusObservedGeneration{}}, + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + obj := &sourcev1.GitRepository{} + + if tt.beforeFunc != nil { + tt.beforeFunc(obj) + } + + tt.patchOpts = addPatchOptionWithStatusObservedGeneration(obj, tt.patchOpts) + + // Apply the options and evaluate the result. + options := &patch.HelperOptions{} + for _, opt := range tt.patchOpts { + opt.ApplyToHelper(options) + } + g.Expect(options.IncludeStatusObservedGeneration).To(Equal(tt.want)) + }) + } +} From 38b56c898f6702830225983aacd1fdae180afc30 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Tue, 24 May 2022 16:43:27 +0100 Subject: [PATCH 7/9] Fix tests failing in Ubuntu Some test cases rely on checksum to match in order to pass. Those checksums were calculated based on file headers which contain their file modes. In Ubuntu, the umask is set to 002 by default, resulting in the tests files having different permissions then when the same files are cloned on another Linux machine with umask set to 022. This change ensures that the files are always set (to 0644 and the directories to 0755) before running the aforementioned tests. Signed-off-by: Paulo Gomes --- controllers/gitrepository_controller_test.go | 23 ++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/controllers/gitrepository_controller_test.go b/controllers/gitrepository_controller_test.go index fd78abcde..50a9463fe 100644 --- a/controllers/gitrepository_controller_test.go +++ b/controllers/gitrepository_controller_test.go @@ -919,6 +919,8 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) + resetChmod(tt.dir, 0o755, 0o644) + r := &GitRepositoryReconciler{ EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, @@ -2142,3 +2144,24 @@ func TestGitRepositoryReconciler_calculateContentConfigChecksum(t *testing.T) { artifactCsumModChecksum := r.calculateContentConfigChecksum(obj, artifacts) g.Expect(artifactModChecksum).ToNot(Equal(artifactCsumModChecksum)) } + +func resetChmod(path string, dirMode os.FileMode, fileMode os.FileMode) error { + err := filepath.Walk(path, + func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + + if info.IsDir() && info.Mode() != dirMode { + os.Chmod(path, dirMode) + } else if !info.IsDir() && info.Mode() != fileMode { + os.Chmod(path, fileMode) + } + return nil + }) + if err != nil { + return fmt.Errorf("cannot reset file permissions: %v", err) + } + + return nil +} From 98cdfbb5c388441910dae2316d41c272af8236c4 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Wed, 25 May 2022 13:57:54 +0100 Subject: [PATCH 8/9] tests: ignore proxy settings when running tests Users environmental proxy settings should not impact the execution of the tests. The changes override both HTTP_PROXY and HTTPS_PROXY to ensure that is the case. Signed-off-by: Paulo Gomes --- Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile b/Makefile index 17d8b28cf..15a81b8f2 100644 --- a/Makefile +++ b/Makefile @@ -98,6 +98,7 @@ build: check-deps $(LIBGIT2) ## Build manager binary KUBEBUILDER_ASSETS?="$(shell $(ENVTEST) --arch=$(ENVTEST_ARCH) use -i $(ENVTEST_KUBERNETES_VERSION) --bin-dir=$(ENVTEST_ASSETS_DIR) -p path)" test: $(LIBGIT2) install-envtest test-api check-deps ## Run tests + HTTPS_PROXY="" HTTP_PROXY="" \ KUBEBUILDER_ASSETS=$(KUBEBUILDER_ASSETS) \ GIT_CONFIG_GLOBAL=/dev/null \ go test $(GO_STATIC_FLAGS) \ From eb5200bbf0397d23c63940b7b2793e75a1504756 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Wed, 25 May 2022 12:35:15 +0100 Subject: [PATCH 9/9] libgit2: add test coverage for HTTP proxy The tests cover the key support for HTTP, HTTPS and NO_PROXY. The tests only focuses on managed transport, as unmanaged transport is sunsetting. Note that unmanaged transport does not support HTTP_PROXY. Signed-off-by: Paulo Gomes --- pkg/git/strategy/proxy/strategy_proxy_test.go | 175 ++++++++++++------ 1 file changed, 123 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..7f8862c71 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,9 +202,10 @@ 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") { + // userAgent := ctx.Req.Header.Get("User-Agent") + if strings.Contains(host, "example.com") { *proxyGotRequest = true return goproxy.OkConnect, u.Host } @@ -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,21 +239,19 @@ 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") { @@ -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 {