From aa3288112e3b849e0f58aa67b584f6734f5fcb96 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Fri, 11 Mar 2022 14:20:46 +0000 Subject: [PATCH 1/8] Implement Managed Transport for libgit2 libgit2 network operations are blocking and do not provide timeout nor context capabilities, leading for several reports by users of the controllers hanging indefinitely. By using managed transport, golang primitives such as http.Transport and net.Dial can be used to ensure timeouts are enforced. Co-Authored-by: Sunny Signed-off-by: Paulo Gomes --- controllers/gitrepository_controller.go | 30 ++- main.go | 5 + pkg/git/libgit2/managed/flag.go | 18 ++ pkg/git/libgit2/managed/http.go | 331 ++++++++++++++++++++++++ pkg/git/libgit2/managed/init.go | 61 +++++ pkg/git/libgit2/managed/options.go | 40 +++ pkg/git/libgit2/managed/ssh.go | 256 ++++++++++++++++++ pkg/git/libgit2/transport.go | 13 + 8 files changed, 753 insertions(+), 1 deletion(-) create mode 100644 pkg/git/libgit2/managed/flag.go create mode 100644 pkg/git/libgit2/managed/http.go create mode 100644 pkg/git/libgit2/managed/init.go create mode 100644 pkg/git/libgit2/managed/options.go create mode 100644 pkg/git/libgit2/managed/ssh.go diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index 6161d0412..d76690e1a 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -50,6 +50,7 @@ import ( "github.com/fluxcd/source-controller/internal/reconcile/summarize" "github.com/fluxcd/source-controller/internal/util" "github.com/fluxcd/source-controller/pkg/git" + "github.com/fluxcd/source-controller/pkg/git/libgit2/managed" "github.com/fluxcd/source-controller/pkg/git/strategy" "github.com/fluxcd/source-controller/pkg/sourceignore" ) @@ -369,10 +370,37 @@ 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 { + // 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. + 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) + } + } + // Checkout HEAD of reference in object gitCtx, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration) defer cancel() - c, err := checkoutStrategy.Checkout(gitCtx, dir, obj.Spec.URL, authOpts) + c, err := checkoutStrategy.Checkout(gitCtx, dir, repositoryURL, authOpts) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to checkout and determine revision: %w", err), diff --git a/main.go b/main.go index 19e6c35e1..120c83d5d 100644 --- a/main.go +++ b/main.go @@ -45,6 +45,7 @@ import ( sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" "github.com/fluxcd/source-controller/controllers" "github.com/fluxcd/source-controller/internal/helm" + "github.com/fluxcd/source-controller/pkg/git/libgit2/managed" // +kubebuilder:scaffold:imports ) @@ -226,6 +227,10 @@ func main() { startFileServer(storage.BasePath, storageAddr, setupLog) }() + if managed.Enabled() { + managed.InitManagedTransport() + } + setupLog.Info("starting manager") if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil { setupLog.Error(err, "problem running manager") diff --git a/pkg/git/libgit2/managed/flag.go b/pkg/git/libgit2/managed/flag.go new file mode 100644 index 000000000..7901a711f --- /dev/null +++ b/pkg/git/libgit2/managed/flag.go @@ -0,0 +1,18 @@ +package managed + +import ( + "os" + "strings" +) + +// Enabled defines whether the use of Managed Transport should be enabled. +// This is only affects git operations that uses libgit2 implementation. +// +// True is returned when the environment variable `EXPERIMENTAL_GIT_TRANSPORT` +// is detected with the value of `true` or `1`. +func Enabled() bool { + if v, ok := os.LookupEnv("EXPERIMENTAL_GIT_TRANSPORT"); ok { + return strings.ToLower(v) == "true" || v == "1" + } + return false +} diff --git a/pkg/git/libgit2/managed/http.go b/pkg/git/libgit2/managed/http.go new file mode 100644 index 000000000..cd2f65f67 --- /dev/null +++ b/pkg/git/libgit2/managed/http.go @@ -0,0 +1,331 @@ +/* +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. +*/ + +/* +This was inspired and contains part of: +https://github.com/libgit2/git2go/blob/eae00773cce87d5282a8ac7c10b5c1961ee6f9cb/http.go + +The MIT License + +Copyright (c) 2013 The git2go contributors + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in +all copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +THE SOFTWARE. +*/ + +package managed + +import ( + "crypto/tls" + "crypto/x509" + "errors" + "fmt" + "io" + "io/ioutil" + "net" + "net/http" + "net/url" + "sync" + "time" + + git2go "github.com/libgit2/git2go/v33" +) + +// registerManagedHTTP registers a Go-native implementation of an +// HTTP(S) transport that doesn't rely on any lower-level libraries +// such as OpenSSL. +func registerManagedHTTP() error { + for _, protocol := range []string{"http", "https"} { + _, err := git2go.NewRegisteredSmartTransport(protocol, true, httpSmartSubtransportFactory) + if err != nil { + return fmt.Errorf("failed to register transport for %q: %v", protocol, err) + } + } + return nil +} + +func httpSmartSubtransportFactory(remote *git2go.Remote, transport *git2go.Transport) (git2go.SmartSubtransport, error) { + sst := &httpSmartSubtransport{ + transport: transport, + } + + return sst, nil +} + +type httpSmartSubtransport struct { + transport *git2go.Transport +} + +func (t *httpSmartSubtransport) Action(targetUrl string, action git2go.SmartServiceAction) (git2go.SmartSubtransportStream, error) { + var req *http.Request + var err error + + var proxyFn func(*http.Request) (*url.URL, error) + proxyOpts, err := t.transport.SmartProxyOptions() + if err != nil { + return nil, err + } + 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) + } + + httpTransport := &http.Transport{ + // Add the proxy to the http transport. + Proxy: proxyFn, + + // Set reasonable timeouts to ensure connections are not + // left open in an idle state, nor they hang indefinitely. + // + // These are based on the official go http.DefaultTransport: + DialContext: (&net.Dialer{ + Timeout: 30 * time.Second, + KeepAlive: 30 * time.Second, + }).DialContext, + MaxIdleConns: 100, + IdleConnTimeout: 90 * time.Second, + TLSHandshakeTimeout: 10 * time.Second, + ExpectContinueTimeout: 1 * time.Second, + } + + finalUrl := targetUrl + opts, found := transportOptions(targetUrl) + if found && opts.TargetUrl != "" { + // override target URL only if options are found and a new targetURL + // is provided. + finalUrl = opts.TargetUrl + } + + // 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, fmt.Errorf("failed to use certificate from PEM") + } + httpTransport.TLSClientConfig = &tls.Config{ + RootCAs: cap, + } + } + + client := &http.Client{Transport: httpTransport, Timeout: fullHttpClientTimeOut} + + switch action { + case git2go.SmartServiceActionUploadpackLs: + req, err = http.NewRequest("GET", finalUrl+"/info/refs?service=git-upload-pack", nil) + + case git2go.SmartServiceActionUploadpack: + req, err = http.NewRequest("POST", finalUrl+"/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) + + case git2go.SmartServiceActionReceivepack: + req, err = http.NewRequest("POST", finalUrl+"/git-receive-pack", nil) + if err != nil { + break + } + req.Header.Set("Content-Type", "application/x-git-receive-pack-request") + + default: + err = errors.New("unknown action") + } + + if err != nil { + return nil, err + } + + req.Header.Set("User-Agent", "git/2.0 (git2go)") + + stream := newManagedHttpStream(t, req, client) + if req.Method == "POST" { + stream.recvReply.Add(1) + stream.sendRequestBackground() + } + + return stream, nil +} + +func (t *httpSmartSubtransport) Close() error { + return nil +} + +func (t *httpSmartSubtransport) Free() { +} + +type httpSmartSubtransportStream struct { + owner *httpSmartSubtransport + client *http.Client + req *http.Request + resp *http.Response + reader *io.PipeReader + writer *io.PipeWriter + sentRequest bool + recvReply sync.WaitGroup + httpError error +} + +func newManagedHttpStream(owner *httpSmartSubtransport, req *http.Request, client *http.Client) *httpSmartSubtransportStream { + r, w := io.Pipe() + return &httpSmartSubtransportStream{ + owner: owner, + client: client, + req: req, + reader: r, + writer: w, + } +} + +func (self *httpSmartSubtransportStream) Read(buf []byte) (int, error) { + if !self.sentRequest { + self.recvReply.Add(1) + if err := self.sendRequest(); err != nil { + return 0, err + } + } + + if err := self.writer.Close(); err != nil { + return 0, err + } + + self.recvReply.Wait() + + if self.httpError != nil { + return 0, self.httpError + } + + return self.resp.Body.Read(buf) +} + +func (self *httpSmartSubtransportStream) Write(buf []byte) (int, error) { + if self.httpError != nil { + return 0, self.httpError + } + return self.writer.Write(buf) +} + +func (self *httpSmartSubtransportStream) Free() { + if self.resp != nil { + self.resp.Body.Close() + } +} + +func (self *httpSmartSubtransportStream) sendRequestBackground() { + go func() { + self.httpError = self.sendRequest() + }() + self.sentRequest = true +} + +func (self *httpSmartSubtransportStream) sendRequest() error { + defer self.recvReply.Done() + self.resp = nil + + 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 + } + } else { + userName, password, err = cred.GetUserpassPlaintext() + if err != nil { + return err + } + defer cred.Free() + } + + for { + req := &http.Request{ + Method: self.req.Method, + URL: self.req.URL, + Header: self.req.Header, + } + if req.Method == "POST" { + req.Body = self.reader + req.ContentLength = -1 + } + + req.SetBasicAuth(userName, password) + resp, err = self.client.Do(req) + if err != nil { + return err + } + + if resp.StatusCode == http.StatusOK { + break + } + + if resp.StatusCode == http.StatusUnauthorized { + resp.Body.Close() + + cred, err := self.owner.transport.SmartCredentials("", git2go.CredentialTypeUserpassPlaintext) + if err != nil { + return err + } + defer cred.Free() + + userName, password, err = cred.GetUserpassPlaintext() + if err != nil { + return err + } + + continue + } + + io.Copy(ioutil.Discard, resp.Body) + resp.Body.Close() + return fmt.Errorf("Unhandled HTTP error %s", resp.Status) + } + + self.sentRequest = true + self.resp = resp + return nil +} diff --git a/pkg/git/libgit2/managed/init.go b/pkg/git/libgit2/managed/init.go new file mode 100644 index 000000000..8df4a9ae9 --- /dev/null +++ b/pkg/git/libgit2/managed/init.go @@ -0,0 +1,61 @@ +/* +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 ( + "sync" + "time" +) + +var ( + once sync.Once + + // sshConnectionTimeOut defines the timeout used for when + // creating ssh.ClientConfig, which translates in the timeout + // for stablishing the SSH TCP connections. + sshConnectionTimeOut time.Duration = 30 * time.Second + + // fullHttpClientTimeOut defines the maximum amount of + // time a http client may take before timing out, + // regardless of the current operation (i.e. connection, + // handshake, put/get). + fullHttpClientTimeOut time.Duration = 10 * time.Minute +) + +// InitManagedTransport initialises HTTP(S) and SSH managed transport +// for git2go, and therefore only impact git operations using the +// libgit2 implementation. +// +// This must run after git2go.init takes place, hence this is not executed +// within a init(). +// Regardless of the state in libgit2/git2go, this will replace the +// built-in transports. +// +// This function will only register managed transports once, subsequent calls +// leads to no-op. +func InitManagedTransport() error { + var err error + once.Do(func() { + if err = registerManagedHTTP(); err != nil { + return + } + + err = registerManagedSSH() + }) + + return err +} diff --git a/pkg/git/libgit2/managed/options.go b/pkg/git/libgit2/managed/options.go new file mode 100644 index 000000000..4fb211fe5 --- /dev/null +++ b/pkg/git/libgit2/managed/options.go @@ -0,0 +1,40 @@ +package managed + +import ( + "sync" +) + +// TransportOptions represents options to be applied at transport-level +// at request time. +type TransportOptions struct { + TargetUrl string + CABundle []byte +} + +var ( + transportOpts = make(map[string]TransportOptions, 0) + m sync.RWMutex +) + +func AddTransportOptions(targetUrl string, opts TransportOptions) { + m.Lock() + transportOpts[targetUrl] = opts + m.Unlock() +} + +func RemoveTransportOptions(targetUrl string) { + m.Lock() + delete(transportOpts, targetUrl) + m.Unlock() +} + +func transportOptions(targetUrl string) (*TransportOptions, bool) { + m.RLock() + opts, found := transportOpts[targetUrl] + m.RUnlock() + + if found { + return &opts, true + } + return nil, false +} diff --git a/pkg/git/libgit2/managed/ssh.go b/pkg/git/libgit2/managed/ssh.go new file mode 100644 index 000000000..76833ac67 --- /dev/null +++ b/pkg/git/libgit2/managed/ssh.go @@ -0,0 +1,256 @@ +/* +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. +*/ + +/* +This was inspired and contains part of: +https://github.com/libgit2/git2go/blob/eae00773cce87d5282a8ac7c10b5c1961ee6f9cb/ssh.go + +The MIT License + +Copyright (c) 2013 The git2go contributors + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in +all copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +THE SOFTWARE. +*/ + +package managed + +import ( + "crypto/md5" + "crypto/sha1" + "crypto/sha256" + "fmt" + "io" + "net" + "net/url" + "runtime" + "strings" + + "golang.org/x/crypto/ssh" + + git2go "github.com/libgit2/git2go/v33" +) + +// registerManagedSSH registers a Go-native implementation of +// SSH transport that doesn't rely on any lower-level libraries +// such as libssh2. +func registerManagedSSH() error { + for _, protocol := range []string{"ssh", "ssh+git", "git+ssh"} { + _, err := git2go.NewRegisteredSmartTransport(protocol, false, sshSmartSubtransportFactory) + if err != nil { + return fmt.Errorf("failed to register transport for %q: %v", protocol, err) + } + } + return nil +} + +func sshSmartSubtransportFactory(remote *git2go.Remote, transport *git2go.Transport) (git2go.SmartSubtransport, error) { + return &sshSmartSubtransport{ + transport: transport, + }, nil +} + +type sshSmartSubtransport struct { + transport *git2go.Transport + + lastAction git2go.SmartServiceAction + client *ssh.Client + session *ssh.Session + stdin io.WriteCloser + stdout io.Reader + currentStream *sshSmartSubtransportStream +} + +func (t *sshSmartSubtransport) Action(urlString string, action git2go.SmartServiceAction) (git2go.SmartSubtransportStream, error) { + runtime.LockOSThread() + defer runtime.UnlockOSThread() + + u, err := url.Parse(urlString) + if err != nil { + return nil, err + } + + // Escape \ and '. + uPath := strings.Replace(u.Path, `\`, `\\`, -1) + uPath = strings.Replace(uPath, `'`, `\'`, -1) + + // TODO: Add percentage decode similar to libgit2. + // Refer: https://github.com/libgit2/libgit2/blob/358a60e1b46000ea99ef10b4dd709e92f75ff74b/src/str.c#L455-L481 + + var cmd string + switch action { + case git2go.SmartServiceActionUploadpackLs, git2go.SmartServiceActionUploadpack: + if t.currentStream != nil { + if t.lastAction == git2go.SmartServiceActionUploadpackLs { + return t.currentStream, nil + } + t.Close() + } + cmd = fmt.Sprintf("git-upload-pack '%s'", uPath) + + case git2go.SmartServiceActionReceivepackLs, git2go.SmartServiceActionReceivepack: + if t.currentStream != nil { + if t.lastAction == git2go.SmartServiceActionReceivepackLs { + return t.currentStream, nil + } + t.Close() + } + cmd = fmt.Sprintf("git-receive-pack '%s'", uPath) + + default: + return nil, fmt.Errorf("unexpected action: %v", action) + } + + cred, err := t.transport.SmartCredentials("", git2go.CredentialTypeSSHKey|git2go.CredentialTypeSSHMemory) + if err != nil { + return nil, err + } + defer cred.Free() + + sshConfig, err := getSSHConfigFromCredential(cred) + if err != nil { + return nil, err + } + sshConfig.HostKeyCallback = func(hostname string, remote net.Addr, key ssh.PublicKey) error { + marshaledKey := key.Marshal() + cert := &git2go.Certificate{ + Kind: git2go.CertificateHostkey, + Hostkey: git2go.HostkeyCertificate{ + Kind: git2go.HostkeySHA1 | git2go.HostkeyMD5 | git2go.HostkeySHA256 | git2go.HostkeyRaw, + HashMD5: md5.Sum(marshaledKey), + HashSHA1: sha1.Sum(marshaledKey), + HashSHA256: sha256.Sum256(marshaledKey), + Hostkey: marshaledKey, + SSHPublicKey: key, + }, + } + + return t.transport.SmartCertificateCheck(cert, true, hostname) + } + + var addr string + if u.Port() != "" { + addr = fmt.Sprintf("%s:%s", u.Hostname(), u.Port()) + } else { + addr = fmt.Sprintf("%s:22", u.Hostname()) + } + + t.client, err = ssh.Dial("tcp", addr, sshConfig) + if err != nil { + return nil, err + } + + t.session, err = t.client.NewSession() + if err != nil { + return nil, err + } + + t.stdin, err = t.session.StdinPipe() + if err != nil { + return nil, err + } + + t.stdout, err = t.session.StdoutPipe() + if err != nil { + return nil, err + } + + if err := t.session.Start(cmd); err != nil { + return nil, err + } + + t.lastAction = action + t.currentStream = &sshSmartSubtransportStream{ + owner: t, + } + + return t.currentStream, nil +} + +func (t *sshSmartSubtransport) Close() error { + t.currentStream = nil + if t.client != nil { + t.stdin.Close() + t.session.Wait() + t.session.Close() + t.client = nil + } + return nil +} + +func (t *sshSmartSubtransport) Free() { +} + +type sshSmartSubtransportStream struct { + owner *sshSmartSubtransport +} + +func (stream *sshSmartSubtransportStream) Read(buf []byte) (int, error) { + return stream.owner.stdout.Read(buf) +} + +func (stream *sshSmartSubtransportStream) Write(buf []byte) (int, error) { + return stream.owner.stdin.Write(buf) +} + +func (stream *sshSmartSubtransportStream) Free() { +} + +func getSSHConfigFromCredential(cred *git2go.Credential) (*ssh.ClientConfig, error) { + username, _, privatekey, passphrase, err := cred.GetSSHKey() + if err != nil { + return nil, err + } + + var pemBytes []byte + if cred.Type() == git2go.CredentialTypeSSHMemory { + pemBytes = []byte(privatekey) + } else { + return nil, fmt.Errorf("file based SSH credential is not supported") + } + + 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 + } + + return &ssh.ClientConfig{ + User: username, + Auth: []ssh.AuthMethod{ssh.PublicKeys(key)}, + Timeout: sshConnectionTimeOut, + }, nil +} diff --git a/pkg/git/libgit2/transport.go b/pkg/git/libgit2/transport.go index 22efa054a..f62ade87b 100644 --- a/pkg/git/libgit2/transport.go +++ b/pkg/git/libgit2/transport.go @@ -36,6 +36,7 @@ import ( "golang.org/x/crypto/ssh/knownhosts" "github.com/fluxcd/source-controller/pkg/git" + "github.com/fluxcd/source-controller/pkg/git/libgit2/managed" ) var ( @@ -112,6 +113,18 @@ 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 From a000d8b859e14aefd890f429c9f14706ca020bf7 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Mon, 14 Mar 2022 15:48:55 +0000 Subject: [PATCH 2/8] Add tests for experimental libgit2 transport Signed-off-by: Paulo Gomes --- hack/ci/e2e.sh | 16 ++ pkg/git/libgit2/managed/http.go | 68 +++--- pkg/git/libgit2/managed/managed_test.go | 290 ++++++++++++++++++++++++ 3 files changed, 347 insertions(+), 27 deletions(-) create mode 100644 pkg/git/libgit2/managed/managed_test.go diff --git a/hack/ci/e2e.sh b/hack/ci/e2e.sh index d8df62abc..4afb28fde 100755 --- a/hack/ci/e2e.sh +++ b/hack/ci/e2e.sh @@ -139,3 +139,19 @@ echo "Run large Git repo tests" kubectl -n source-system apply -f "${ROOT_DIR}/config/testdata/git/large-repo.yaml" kubectl -n source-system wait gitrepository/large-repo-go-git --for=condition=ready --timeout=2m15s kubectl -n source-system wait gitrepository/large-repo-libgit2 --for=condition=ready --timeout=2m15s + + +# Test experimental libgit2 transport. Any tests against the default transport must +# either run before this, or patch the deployment again to disable this, as once enabled +# only the managed transport will be used. +kubectl -n source-system patch deployment source-controller \ + --patch '{"spec": {"template": {"spec": {"containers": [{"name": "manager","env": [{"name": "EXPERIMENTAL_GIT_TRANSPORT", "value": "true"}]}]}}}}' + +# wait until the patch took effect and the new source-controller is running +sleep 20s + +kubectl -n source-system wait --for=condition=ready --timeout=1m -l app=source-controller pod + +echo "Re-run large libgit2 repo test with managed transport" +kubectl -n source-system wait gitrepository/large-repo-libgit2 --for=condition=ready --timeout=2m15s +kubectl -n source-system exec deploy/source-controller -- printenv | grep EXPERIMENTAL_GIT_TRANSPORT=true diff --git a/pkg/git/libgit2/managed/http.go b/pkg/git/libgit2/managed/http.go index cd2f65f67..965974df7 100644 --- a/pkg/git/libgit2/managed/http.go +++ b/pkg/git/libgit2/managed/http.go @@ -85,9 +85,6 @@ type httpSmartSubtransport struct { } func (t *httpSmartSubtransport) Action(targetUrl string, action git2go.SmartServiceAction) (git2go.SmartSubtransportStream, error) { - var req *http.Request - var err error - var proxyFn func(*http.Request) (*url.URL, error) proxyOpts, err := t.transport.SmartProxyOptions() if err != nil { @@ -125,26 +122,50 @@ func (t *httpSmartSubtransport) Action(targetUrl string, action git2go.SmartServ ExpectContinueTimeout: 1 * time.Second, } - finalUrl := targetUrl - opts, found := transportOptions(targetUrl) - if found && opts.TargetUrl != "" { - // override target URL only if options are found and a new targetURL - // is provided. - finalUrl = opts.TargetUrl + client, req, err := createClientRequest(targetUrl, action, httpTransport) + if err != nil { + return nil, err + } + + stream := newManagedHttpStream(t, req, client) + if req.Method == "POST" { + stream.recvReply.Add(1) + stream.sendRequestBackground() } - // 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, fmt.Errorf("failed to use certificate from PEM") + return stream, nil +} + +func createClientRequest(targetUrl string, action git2go.SmartServiceAction, t *http.Transport) (*http.Client, *http.Request, error) { + var req *http.Request + var err error + + if t == nil { + 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 } - httpTransport.TLSClientConfig = &tls.Config{ - RootCAs: cap, + + // 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, + } } } - client := &http.Client{Transport: httpTransport, Timeout: fullHttpClientTimeOut} + client := &http.Client{Transport: t, Timeout: fullHttpClientTimeOut} switch action { case git2go.SmartServiceActionUploadpackLs: @@ -172,18 +193,11 @@ func (t *httpSmartSubtransport) Action(targetUrl string, action git2go.SmartServ } if err != nil { - return nil, err + return nil, nil, err } - req.Header.Set("User-Agent", "git/2.0 (git2go)") - - stream := newManagedHttpStream(t, req, client) - if req.Method == "POST" { - stream.recvReply.Add(1) - stream.sendRequestBackground() - } - - return stream, nil + req.Header.Set("User-Agent", "git/2.0 (flux-libgit2)") + return client, req, nil } func (t *httpSmartSubtransport) Close() error { diff --git a/pkg/git/libgit2/managed/managed_test.go b/pkg/git/libgit2/managed/managed_test.go new file mode 100644 index 000000000..aa163e872 --- /dev/null +++ b/pkg/git/libgit2/managed/managed_test.go @@ -0,0 +1,290 @@ +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" + + git2go "github.com/libgit2/git2go/v33" + . "github.com/onsi/gomega" + "gotest.tools/assert" +) + +func TestHttpAction_CreateClientRequest(t *testing.T) { + tests := []struct { + description string + url string + expectedUrl string + expectedMethod string + action git2go.SmartServiceAction + opts *TransportOptions + transport *http.Transport + wantedErr error + }{ + { + description: "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, + }, + { + description: "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, + }, + { + description: "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, + }, + { + description: "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, + }, + { + description: "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, + }, + { + description: "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"), + }, + } + + for _, tt := range tests { + 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("%s: wanted: %v got: %v", tt.description, 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) + } + } +} + +func TestOptions(t *testing.T) { + tests := []struct { + description string + registerOpts bool + url string + opts TransportOptions + expectOpts bool + expectedOpts *TransportOptions + }{ + { + description: "return registered option", + registerOpts: true, + url: "https://target/?123", + opts: TransportOptions{}, + expectOpts: true, + expectedOpts: &TransportOptions{}, + }, + { + description: "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}, + }, + }, + { + description: "ignore when options not registered", + registerOpts: false, + url: "", + opts: TransportOptions{}, + expectOpts: false, + expectedOpts: nil, + }, + } + + for _, tt := range tests { + 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.description, tt.expectOpts, found) + } + + if tt.expectOpts { + if reflect.DeepEqual(opts, *tt.expectedOpts) { + t.Errorf("%s: wanted %v got %v", tt.description, *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.description, tt.url) + } + } +} + +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") + } +} + +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() + + repoPath := "test.git" + err = server.InitRepo("../testdata/git/repo", git.DefaultBranch, repoPath) + g.Expect(err).ToNot(HaveOccurred()) + + tmpDir, _ := os.MkdirTemp("", "test") + defer os.RemoveAll(tmpDir) + + // 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, _ := os.MkdirTemp("", "test") + defer os.RemoveAll(tmpDir2) + + 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() +} From 24bc95e1d8e0edf8463341e8f8efeb8f8ea38a7d Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Mon, 14 Mar 2022 16:02:44 +0000 Subject: [PATCH 3/8] Add license headers Signed-off-by: Paulo Gomes --- pkg/git/libgit2/managed/flag.go | 16 ++++++++++++++++ pkg/git/libgit2/managed/managed_test.go | 16 ++++++++++++++++ pkg/git/libgit2/managed/options.go | 16 ++++++++++++++++ 3 files changed, 48 insertions(+) diff --git a/pkg/git/libgit2/managed/flag.go b/pkg/git/libgit2/managed/flag.go index 7901a711f..2905c7719 100644 --- a/pkg/git/libgit2/managed/flag.go +++ b/pkg/git/libgit2/managed/flag.go @@ -1,3 +1,19 @@ +/* +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 ( diff --git a/pkg/git/libgit2/managed/managed_test.go b/pkg/git/libgit2/managed/managed_test.go index aa163e872..52004f704 100644 --- a/pkg/git/libgit2/managed/managed_test.go +++ b/pkg/git/libgit2/managed/managed_test.go @@ -1,3 +1,19 @@ +/* +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 ( diff --git a/pkg/git/libgit2/managed/options.go b/pkg/git/libgit2/managed/options.go index 4fb211fe5..2ab1d1556 100644 --- a/pkg/git/libgit2/managed/options.go +++ b/pkg/git/libgit2/managed/options.go @@ -1,3 +1,19 @@ +/* +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 ( From 4ed54bc35951c78c2d157500337f840b48a11f2a Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Mon, 14 Mar 2022 17:46:55 +0000 Subject: [PATCH 4/8] Optimise basic auth for libgit2 managed transport The initial implementation was based off upstream, which cause an initial request to fail, and only then the credentials would be added into the request. Signed-off-by: Paulo Gomes --- pkg/git/libgit2/managed/http.go | 24 +++++------------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/pkg/git/libgit2/managed/http.go b/pkg/git/libgit2/managed/http.go index 965974df7..3b561e242 100644 --- a/pkg/git/libgit2/managed/http.go +++ b/pkg/git/libgit2/managed/http.go @@ -288,12 +288,15 @@ func (self *httpSmartSubtransportStream) sendRequest() error { if err.Error() != git2go.ErrorCodePassthrough.String() { return err } - } else { + } + + if cred != nil { + defer cred.Free() + userName, password, err = cred.GetUserpassPlaintext() if err != nil { return err } - defer cred.Free() } for { @@ -317,23 +320,6 @@ func (self *httpSmartSubtransportStream) sendRequest() error { break } - if resp.StatusCode == http.StatusUnauthorized { - resp.Body.Close() - - cred, err := self.owner.transport.SmartCredentials("", git2go.CredentialTypeUserpassPlaintext) - if err != nil { - return err - } - defer cred.Free() - - userName, password, err = cred.GetUserpassPlaintext() - if err != nil { - return err - } - - continue - } - io.Copy(ioutil.Discard, resp.Body) resp.Body.Close() return fmt.Errorf("Unhandled HTTP error %s", resp.Status) From 822788b79e1b6f093ba8a0630b6ed87fb93f7b6d Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Tue, 15 Mar 2022 09:38:42 +0000 Subject: [PATCH 5/8] Fix word casing Signed-off-by: Paulo Gomes --- controllers/gitrepository_controller.go | 2 +- pkg/git/libgit2/managed/http.go | 4 ++-- pkg/git/libgit2/managed/managed_test.go | 8 ++++---- pkg/git/libgit2/managed/options.go | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index d76690e1a..514653f67 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -388,7 +388,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, repositoryURL = fmt.Sprintf("http://%s/%s/%d", obj.Name, obj.UID, obj.Generation) managed.AddTransportOptions(repositoryURL, managed.TransportOptions{ - TargetUrl: obj.Spec.URL, + TargetURL: obj.Spec.URL, CABundle: authOpts.CAFile, }) diff --git a/pkg/git/libgit2/managed/http.go b/pkg/git/libgit2/managed/http.go index 3b561e242..5c71f9a34 100644 --- a/pkg/git/libgit2/managed/http.go +++ b/pkg/git/libgit2/managed/http.go @@ -147,10 +147,10 @@ func createClientRequest(targetUrl string, action git2go.SmartServiceAction, t * finalUrl := targetUrl opts, found := transportOptions(targetUrl) if found { - if opts.TargetUrl != "" { + if opts.TargetURL != "" { // override target URL only if options are found and a new targetURL // is provided. - finalUrl = opts.TargetUrl + finalUrl = opts.TargetURL } // Add any provided certificate to the http transport. diff --git a/pkg/git/libgit2/managed/managed_test.go b/pkg/git/libgit2/managed/managed_test.go index 52004f704..3aa3088ca 100644 --- a/pkg/git/libgit2/managed/managed_test.go +++ b/pkg/git/libgit2/managed/managed_test.go @@ -92,7 +92,7 @@ func TestHttpAction_CreateClientRequest(t *testing.T) { action: git2go.SmartServiceActionUploadpack, transport: &http.Transport{}, opts: &TransportOptions{ - TargetUrl: "https://final-target", + TargetURL: "https://final-target", }, wantedErr: nil, }, @@ -151,12 +151,12 @@ func TestOptions(t *testing.T) { registerOpts: true, url: "https://target/?876", opts: TransportOptions{ - TargetUrl: "https://new-target/321", + TargetURL: "https://new-target/321", CABundle: []byte{123, 213, 132}, }, expectOpts: true, expectedOpts: &TransportOptions{ - TargetUrl: "https://new-target/321", + TargetURL: "https://new-target/321", CABundle: []byte{123, 213, 132}, }, }, @@ -262,7 +262,7 @@ func TestManagedTransport_E2E(t *testing.T) { // 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, + TargetURL: server.HTTPAddress() + "/" + repoPath, }) repo, err := git2go.Clone(httpAddress, tmpDir, &git2go.CloneOptions{ diff --git a/pkg/git/libgit2/managed/options.go b/pkg/git/libgit2/managed/options.go index 2ab1d1556..13ef08128 100644 --- a/pkg/git/libgit2/managed/options.go +++ b/pkg/git/libgit2/managed/options.go @@ -23,7 +23,7 @@ import ( // TransportOptions represents options to be applied at transport-level // at request time. type TransportOptions struct { - TargetUrl string + TargetURL string CABundle []byte } From d1a7e5d6091156874cf56ad84c90655662014816 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Tue, 15 Mar 2022 14:42:54 +0000 Subject: [PATCH 6/8] Fix race condition on httpSmartSubTransport Signed-off-by: Paulo Gomes --- pkg/git/libgit2/managed/http.go | 57 ++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 26 deletions(-) diff --git a/pkg/git/libgit2/managed/http.go b/pkg/git/libgit2/managed/http.go index 5c71f9a34..b9607280f 100644 --- a/pkg/git/libgit2/managed/http.go +++ b/pkg/git/libgit2/managed/http.go @@ -217,6 +217,7 @@ type httpSmartSubtransportStream struct { sentRequest bool recvReply sync.WaitGroup httpError error + m sync.RWMutex } func newManagedHttpStream(owner *httpSmartSubtransport, req *http.Request, client *http.Client) *httpSmartSubtransportStream { @@ -244,6 +245,8 @@ func (self *httpSmartSubtransportStream) Read(buf []byte) (int, error) { self.recvReply.Wait() + self.m.RLock() + defer self.m.RUnlock() if self.httpError != nil { return 0, self.httpError } @@ -252,6 +255,8 @@ func (self *httpSmartSubtransportStream) Read(buf []byte) (int, error) { } func (self *httpSmartSubtransportStream) Write(buf []byte) (int, error) { + self.m.RLock() + defer self.m.RUnlock() if self.httpError != nil { return 0, self.httpError } @@ -266,7 +271,11 @@ func (self *httpSmartSubtransportStream) Free() { func (self *httpSmartSubtransportStream) sendRequestBackground() { go func() { - self.httpError = self.sendRequest() + err := self.sendRequest() + + self.m.Lock() + self.httpError = err + self.m.Unlock() }() self.sentRequest = true } @@ -299,33 +308,29 @@ func (self *httpSmartSubtransportStream) sendRequest() error { } } - for { - req := &http.Request{ - Method: self.req.Method, - URL: self.req.URL, - Header: self.req.Header, - } - if req.Method == "POST" { - req.Body = self.reader - req.ContentLength = -1 - } - - req.SetBasicAuth(userName, password) - resp, err = self.client.Do(req) - if err != nil { - return err - } + req := &http.Request{ + Method: self.req.Method, + URL: self.req.URL, + Header: self.req.Header, + } + if req.Method == "POST" { + req.Body = self.reader + req.ContentLength = -1 + } - if resp.StatusCode == http.StatusOK { - break - } + req.SetBasicAuth(userName, password) + resp, err = self.client.Do(req) + if err != nil { + return err + } - io.Copy(ioutil.Discard, resp.Body) - resp.Body.Close() - return fmt.Errorf("Unhandled HTTP error %s", resp.Status) + if resp.StatusCode == http.StatusOK { + self.resp = resp + self.sentRequest = true + return nil } - self.sentRequest = true - self.resp = resp - return nil + io.Copy(ioutil.Discard, resp.Body) + defer resp.Body.Close() + return fmt.Errorf("Unhandled HTTP error %s", resp.Status) } From 43661dd15eefd1b6c989e22fcf610506c3774d65 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Tue, 15 Mar 2022 16:02:35 +0000 Subject: [PATCH 7/8] Enforce effective URL on error messages Signed-off-by: Paulo Gomes --- pkg/git/libgit2/checkout.go | 9 +++++---- pkg/git/libgit2/managed/options.go | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/pkg/git/libgit2/checkout.go b/pkg/git/libgit2/checkout.go index 6732aeb12..8e1e5cad9 100644 --- a/pkg/git/libgit2/checkout.go +++ b/pkg/git/libgit2/checkout.go @@ -31,6 +31,7 @@ import ( "github.com/fluxcd/pkg/version" "github.com/fluxcd/source-controller/pkg/git" + "github.com/fluxcd/source-controller/pkg/git/libgit2/managed" ) // CheckoutStrategyForOptions returns the git.CheckoutStrategy for the given @@ -72,7 +73,7 @@ func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, opts *g CheckoutBranch: c.Branch, }) if err != nil { - return nil, fmt.Errorf("unable to clone '%s': %w", url, gitutil.LibGit2Error(err)) + return nil, fmt.Errorf("unable to clone '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) } defer repo.Free() head, err := repo.Head() @@ -101,7 +102,7 @@ func (c *CheckoutTag) Checkout(ctx context.Context, path, url string, opts *git. }, }) if err != nil { - return nil, fmt.Errorf("unable to clone '%s': %w", url, gitutil.LibGit2Error(err)) + return nil, fmt.Errorf("unable to clone '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) } defer repo.Free() cc, err := checkoutDetachedDwim(repo, c.Tag) @@ -125,7 +126,7 @@ func (c *CheckoutCommit) Checkout(ctx context.Context, path, url string, opts *g }, }) if err != nil { - return nil, fmt.Errorf("unable to clone '%s': %w", url, gitutil.LibGit2Error(err)) + return nil, fmt.Errorf("unable to clone '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) } defer repo.Free() oid, err := git2go.NewOid(c.Commit) @@ -157,7 +158,7 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, opts *g }, }) if err != nil { - return nil, fmt.Errorf("unable to clone '%s': %w", url, gitutil.LibGit2Error(err)) + return nil, fmt.Errorf("unable to clone '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) } defer repo.Free() diff --git a/pkg/git/libgit2/managed/options.go b/pkg/git/libgit2/managed/options.go index 13ef08128..d4d346ad0 100644 --- a/pkg/git/libgit2/managed/options.go +++ b/pkg/git/libgit2/managed/options.go @@ -54,3 +54,22 @@ func transportOptions(targetUrl string) (*TransportOptions, bool) { } return nil, false } + +// EffectiveURL returns the effective URL for requests. +// +// 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 { + if !Enabled() { + return targetUrl + } + + if opts, found := transportOptions(targetUrl); found { + if opts.TargetURL != "" { + return opts.TargetURL + } + } + + return targetUrl +} From 115040e9ea471572bfbe2a2ec8b0e8e881042889 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Tue, 15 Mar 2022 23:13:15 +0000 Subject: [PATCH 8/8] Support redirects for libgit2 managed transport For backwards compatibility, support for HTTP redirection is enabled when targeting the same host, and no TLS downgrade took place. Signed-off-by: Paulo Gomes --- pkg/git/libgit2/managed/http.go | 106 +++++++++++++++++------- pkg/git/libgit2/managed/managed_test.go | 21 +++++ 2 files changed, 99 insertions(+), 28 deletions(-) diff --git a/pkg/git/libgit2/managed/http.go b/pkg/git/libgit2/managed/http.go index b9607280f..24adfd665 100644 --- a/pkg/git/libgit2/managed/http.go +++ b/pkg/git/libgit2/managed/http.go @@ -44,12 +44,12 @@ THE SOFTWARE. package managed import ( + "bytes" "crypto/tls" "crypto/x509" "errors" "fmt" "io" - "io/ioutil" "net" "net/http" "net/url" @@ -133,6 +133,25 @@ func (t *httpSmartSubtransport) Action(targetUrl string, action git2go.SmartServ stream.sendRequestBackground() } + client.CheckRedirect = func(req *http.Request, via []*http.Request) error { + if len(via) >= 3 { + return fmt.Errorf("too many redirects") + } + + // golang will change POST to GET in case of redirects. + if len(via) >= 0 && req.Method != via[0].Method { + if via[0].URL.Scheme == "https" && req.URL.Scheme == "http" { + return fmt.Errorf("downgrade from https to http is not allowed: from %q to %q", via[0].URL.String(), req.URL.String()) + } + if via[0].URL.Host != req.URL.Host { + return fmt.Errorf("cross hosts redirects are not allowed: from %s to %s", via[0].URL.Host, req.URL.Host) + } + + return http.ErrUseLastResponse + } + return nil + } + return stream, nil } @@ -165,7 +184,10 @@ func createClientRequest(targetUrl string, action git2go.SmartServiceAction, t * } } - client := &http.Client{Transport: t, Timeout: fullHttpClientTimeOut} + client := &http.Client{ + Transport: t, + Timeout: fullHttpClientTimeOut, + } switch action { case git2go.SmartServiceActionUploadpackLs: @@ -218,6 +240,7 @@ type httpSmartSubtransportStream struct { recvReply sync.WaitGroup httpError error m sync.RWMutex + targetURL string } func newManagedHttpStream(owner *httpSmartSubtransport, req *http.Request, client *http.Client) *httpSmartSubtransportStream { @@ -246,18 +269,21 @@ func (self *httpSmartSubtransportStream) Read(buf []byte) (int, error) { self.recvReply.Wait() self.m.RLock() - defer self.m.RUnlock() - if self.httpError != nil { + err := self.httpError + self.m.RUnlock() + + if err != nil { return 0, self.httpError } - return self.resp.Body.Read(buf) } func (self *httpSmartSubtransportStream) Write(buf []byte) (int, error) { self.m.RLock() - defer self.m.RUnlock() - if self.httpError != nil { + err := self.httpError + self.m.RUnlock() + + if err != nil { return 0, self.httpError } return self.writer.Write(buf) @@ -308,29 +334,53 @@ func (self *httpSmartSubtransportStream) sendRequest() error { } } - req := &http.Request{ - Method: self.req.Method, - URL: self.req.URL, - Header: self.req.Header, - } - if req.Method == "POST" { - req.Body = self.reader - req.ContentLength = -1 - } + var content []byte + for { + req := &http.Request{ + Method: self.req.Method, + URL: self.req.URL, + Header: self.req.Header, + } + if req.Method == "POST" { + if len(content) == 0 { + // a copy of the request body needs to be saved so + // it can be reused in case of redirects. + if content, err = io.ReadAll(self.reader); err != nil { + return err + } + } + req.Body = io.NopCloser(bytes.NewReader(content)) + req.ContentLength = -1 + } - req.SetBasicAuth(userName, password) - resp, err = self.client.Do(req) - if err != nil { - return err - } + req.SetBasicAuth(userName, password) + resp, err = self.client.Do(req) + if err != nil { + return err + } - if resp.StatusCode == http.StatusOK { - self.resp = resp - self.sentRequest = true - return nil + // GET requests will be automatically redirected. + // POST require the new destination, and also the body content. + if req.Method == "POST" && resp.StatusCode >= 301 && resp.StatusCode <= 308 { + // The next try will go against the new destination + self.req.URL, err = resp.Location() + if err != nil { + return err + } + + continue + } + + if resp.StatusCode == http.StatusOK { + break + } + + io.Copy(io.Discard, resp.Body) + defer resp.Body.Close() + return fmt.Errorf("Unhandled HTTP error %s", resp.Status) } - io.Copy(ioutil.Discard, resp.Body) - defer resp.Body.Close() - return fmt.Errorf("Unhandled HTTP error %s", resp.Status) + self.resp = resp + self.sentRequest = true + return nil } diff --git a/pkg/git/libgit2/managed/managed_test.go b/pkg/git/libgit2/managed/managed_test.go index 3aa3088ca..1d8582778 100644 --- a/pkg/git/libgit2/managed/managed_test.go +++ b/pkg/git/libgit2/managed/managed_test.go @@ -304,3 +304,24 @@ func TestManagedTransport_E2E(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) repo.Free() } + +func TestManagedTransport_HandleRedirect(t *testing.T) { + g := NewWithT(t) + + tmpDir, _ := os.MkdirTemp("", "test") + defer os.RemoveAll(tmpDir) + + // Force managed transport to be enabled + InitManagedTransport() + + // 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() +}