From 8fa174efb4bb1d58c0f72c38c0571cefc829d539 Mon Sep 17 00:00:00 2001 From: Sunny Date: Thu, 30 Sep 2021 03:23:32 +0530 Subject: [PATCH 1/2] Add libgit2 checkout test with ED25519 key This adds a test to detect any regression in libgit2's ED25519 key support. go-git supports ED25519 but not the current version of libgit2 used in flux. The updates to libgit2 in v1.2.0 adds support for ED25519. This test would help ensure the right version of libgit2 is used. Signed-off-by: Sunny --- go.mod | 2 +- go.sum | 5 +- pkg/git/libgit2/checkout.go | 2 +- pkg/git/libgit2/checkout_test.go | 77 +++++++++++++++++++++++ pkg/git/libgit2/testdata/git/repo/foo.txt | 1 + 5 files changed, 82 insertions(+), 5 deletions(-) create mode 100644 pkg/git/libgit2/testdata/git/repo/foo.txt diff --git a/go.mod b/go.mod index f9debcdf5..8fe726fc4 100644 --- a/go.mod +++ b/go.mod @@ -8,7 +8,7 @@ require ( github.com/Masterminds/semver/v3 v3.1.1 github.com/cyphar/filepath-securejoin v0.2.2 github.com/fluxcd/pkg/apis/meta v0.11.0-rc.1 - github.com/fluxcd/pkg/gittestserver v0.3.2 + github.com/fluxcd/pkg/gittestserver v0.4.0 github.com/fluxcd/pkg/gitutil v0.1.0 github.com/fluxcd/pkg/helmtestserver v0.2.0 github.com/fluxcd/pkg/lockedfile v0.1.0 diff --git a/go.sum b/go.sum index 8397a9ecc..f65c3d32f 100644 --- a/go.sum +++ b/go.sum @@ -230,8 +230,8 @@ github.com/fluxcd/pkg/apis/acl v0.0.1 h1:biCgZMjpDSv3Q4mZPikUJILx3t2MuNXR4Oa5jRQ github.com/fluxcd/pkg/apis/acl v0.0.1/go.mod h1:y3qOXUFObVWk7jzOjubMnr/u18j1kCeSi6olycnxr/E= github.com/fluxcd/pkg/apis/meta v0.11.0-rc.1 h1:RHHrztAFv9wmjM+Pk7Svt1UdD+1SdnQSp76MWFiM7Hg= github.com/fluxcd/pkg/apis/meta v0.11.0-rc.1/go.mod h1:yUblM2vg+X8TE3A2VvJfdhkGmg+uqBlSPkLk7dxi0UM= -github.com/fluxcd/pkg/gittestserver v0.3.2 h1:oc1OoZ4b+kAu0vu/RT9wUwuQZxSqEjBOlQWYYA+YeLM= -github.com/fluxcd/pkg/gittestserver v0.3.2/go.mod h1:8j36Z6B0BuKNZZ6exAWoyDEpyQoFcjz1IX3WBT7PZNg= +github.com/fluxcd/pkg/gittestserver v0.4.0 h1:VQzQ5TcHzohxbYGWpnQ/79w7/rnS2SQGC7FSDtbIsCA= +github.com/fluxcd/pkg/gittestserver v0.4.0/go.mod h1:hUPx21fe/6oox336Wih/XF1fnmzLmptNMOvATbTZXNY= github.com/fluxcd/pkg/gitutil v0.1.0 h1:VO3kJY/CKOCO4ysDNqfdpTg04icAKBOSb3lbR5uE/IE= github.com/fluxcd/pkg/gitutil v0.1.0/go.mod h1:Ybz50Ck5gkcnvF0TagaMwtlRy3X3wXuiri1HVsK5id4= github.com/fluxcd/pkg/helmtestserver v0.2.0 h1:cE7YHDmrWI0hr9QpaaeQ0vQ16Z0IiqZKiINDpqdY610= @@ -899,7 +899,6 @@ golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8U golang.org/x/crypto v0.0.0-20200414173820-0848c9571904/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= golang.org/x/crypto v0.0.0-20200709230013-948cd5f35899/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= -golang.org/x/crypto v0.0.0-20200728195943-123391ffb6de/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= golang.org/x/crypto v0.0.0-20201002170205-7f63de1d35b0/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= golang.org/x/crypto v0.0.0-20201203163018-be400aefbc4c/go.mod h1:jdWPYTVW3xRLrWPugEBEK3UY2ZEsg3UU495nc5E+M+I= golang.org/x/crypto v0.0.0-20201221181555-eec23a3978ad/go.mod h1:jdWPYTVW3xRLrWPugEBEK3UY2ZEsg3UU495nc5E+M+I= diff --git a/pkg/git/libgit2/checkout.go b/pkg/git/libgit2/checkout.go index 01363f8fa..03a074638 100644 --- a/pkg/git/libgit2/checkout.go +++ b/pkg/git/libgit2/checkout.go @@ -23,10 +23,10 @@ import ( "time" "github.com/Masterminds/semver/v3" - "github.com/fluxcd/pkg/version" git2go "github.com/libgit2/git2go/v31" "github.com/fluxcd/pkg/gitutil" + "github.com/fluxcd/pkg/version" sourcev1 "github.com/fluxcd/source-controller/api/v1beta1" "github.com/fluxcd/source-controller/pkg/git" diff --git a/pkg/git/libgit2/checkout_test.go b/pkg/git/libgit2/checkout_test.go index 6de5484d8..a7428c964 100644 --- a/pkg/git/libgit2/checkout_test.go +++ b/pkg/git/libgit2/checkout_test.go @@ -21,11 +21,18 @@ import ( "crypto/sha256" "encoding/hex" "io" + "net/url" "os" "path" + "path/filepath" "testing" + "time" + "github.com/fluxcd/pkg/gittestserver" + "github.com/fluxcd/pkg/ssh" git2go "github.com/libgit2/git2go/v31" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" "github.com/fluxcd/source-controller/pkg/git" ) @@ -77,3 +84,73 @@ func TestCheckoutTagSemVer_Checkout(t *testing.T) { t.Errorf("expected semver hash %s, got %s", cTag.Hash(), cSemVer.Hash()) } } + +// This test is specifically to detect regression in libgit2's ED25519 key +// support. +// Refer: https://github.com/fluxcd/source-controller/issues/399 +func TestCheckout_ED25519(t *testing.T) { + g := NewWithT(t) + timeout := 5 * time.Second + + // Create a git test server. + server, err := gittestserver.NewTempGitServer() + g.Expect(err).ToNot(HaveOccurred()) + defer os.RemoveAll(server.Root()) + server.Auth("test-user", "test-pswd") + server.AutoCreate() + + server.KeyDir(filepath.Join(server.Root(), "keys")) + g.Expect(server.ListenSSH()).To(Succeed()) + + go func() { + server.StartSSH() + }() + defer server.StopSSH() + + repoPath := "test.git" + + err = server.InitRepo("testdata/git/repo", git.DefaultBranch, repoPath) + g.Expect(err).NotTo(HaveOccurred()) + + sshURL := server.SSHAddress() + repoURL := sshURL + "/" + repoPath + + // Fetch host key. + u, err := url.Parse(sshURL) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(u.Host).ToNot(BeEmpty()) + knownHosts, err := ssh.ScanHostKey(u.Host, timeout) + g.Expect(err).ToNot(HaveOccurred()) + + kp, err := ssh.NewEd25519Generator().Generate() + g.Expect(err).ToNot(HaveOccurred()) + + secret := corev1.Secret{ + Data: map[string][]byte{ + "identity": kp.PrivateKey, + "known_hosts": knownHosts, + }, + } + + authStrategy, err := AuthSecretStrategyForURL(repoURL) + g.Expect(err).ToNot(HaveOccurred()) + gitAuth, err := authStrategy.Method(secret) + g.Expect(err).ToNot(HaveOccurred()) + + // Prepare for checkout. + branchCheckoutStrat := &CheckoutBranch{branch: git.DefaultBranch} + tmpDir, _ := os.MkdirTemp("", "test") + defer os.RemoveAll(tmpDir) + + ctx, cancel := context.WithTimeout(context.TODO(), timeout) + defer cancel() + + // Checkout the repo. + // This should always fail because the generated key above isn't present in + // the git server. + _, _, err = branchCheckoutStrat.Checkout(ctx, tmpDir, repoURL, gitAuth) + g.Expect(err).To(HaveOccurred()) + // NOTE: libgit2 v1.2+ supports ED25519. Flip this condition after updating + // to libgit2 v1.2+. + g.Expect(err.Error()).To(ContainSubstring("Unable to extract public key from private key")) +} diff --git a/pkg/git/libgit2/testdata/git/repo/foo.txt b/pkg/git/libgit2/testdata/git/repo/foo.txt new file mode 100644 index 000000000..16b14f5da --- /dev/null +++ b/pkg/git/libgit2/testdata/git/repo/foo.txt @@ -0,0 +1 @@ +test file From 513075209699c7b349d93bda4645c45fcc984a78 Mon Sep 17 00:00:00 2001 From: Sunny Date: Thu, 30 Sep 2021 03:40:24 +0530 Subject: [PATCH 2/2] libgit2: Update checkout to respect context Update libgit2-git2go checkout with a clone wrapper to respect the context passed to it. go-git accepts context passed to it during cloning but the git2go does not accept the context passed to it. This makes the GitRepository.Spec.Timeout ineffective when using libgit2. This change updates all the different libgit2 checkouts to use the new clone function. This is also needed to shorten the wait time for tests that depend on failure behavior and should fail early. TestCheckout_ED25519 test would take 30s without a context with timeout. This helps shorten the time for such tests. Signed-off-by: Sunny --- controllers/gitrepository_controller_test.go | 2 +- pkg/git/libgit2/checkout.go | 50 +++++++++++++++----- 2 files changed, 39 insertions(+), 13 deletions(-) diff --git a/controllers/gitrepository_controller_test.go b/controllers/gitrepository_controller_test.go index 975ad028e..3ad146608 100644 --- a/controllers/gitrepository_controller_test.go +++ b/controllers/gitrepository_controller_test.go @@ -245,7 +245,7 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { }, wantErr: true, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.GitOperationFailedReason, "Failed to checkout and determine revision: unable to clone '', error: Certificate"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.GitOperationFailedReason, "Failed to checkout and determine revision: unable to clone '': Certificate"), }, }, { diff --git a/pkg/git/libgit2/checkout.go b/pkg/git/libgit2/checkout.go index 03a074638..8733458b4 100644 --- a/pkg/git/libgit2/checkout.go +++ b/pkg/git/libgit2/checkout.go @@ -53,12 +53,34 @@ func CheckoutStrategyForRef(ref *sourcev1.GitRepositoryRef, opt git.CheckoutOpti } } +// clone is a wrapper around git2go.Clone that respects the provided context. +func clone(ctx context.Context, path, url string, auth *git.Auth, opts *git2go.CloneOptions) (*git2go.Repository, error) { + var repo *git2go.Repository + errCh := make(chan error, 1) + go func() { + var err error + repo, err = git2go.Clone(url, path, opts) + errCh <- err + }() + + select { + case err := <-errCh: + if err != nil { + return nil, fmt.Errorf("unable to clone '%s': %w", url, gitutil.LibGit2Error(err)) + } + case <-ctx.Done(): + return nil, fmt.Errorf("clone context cancelled: %w", ctx.Err()) + } + + return repo, nil +} + type CheckoutBranch struct { branch string } func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, auth *git.Auth) (git.Commit, string, error) { - repo, err := git2go.Clone(url, path, &git2go.CloneOptions{ + cloneOpts := &git2go.CloneOptions{ FetchOptions: &git2go.FetchOptions{ DownloadTags: git2go.DownloadTagsNone, RemoteCallbacks: git2go.RemoteCallbacks{ @@ -67,9 +89,10 @@ func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, auth *g }, }, CheckoutBranch: c.branch, - }) + } + repo, err := clone(ctx, path, url, auth, cloneOpts) if err != nil { - return nil, "", fmt.Errorf("unable to clone '%s', error: %w", url, gitutil.LibGit2Error(err)) + return nil, "", err } head, err := repo.Head() if err != nil { @@ -87,7 +110,7 @@ type CheckoutTag struct { } func (c *CheckoutTag) Checkout(ctx context.Context, path, url string, auth *git.Auth) (git.Commit, string, error) { - repo, err := git2go.Clone(url, path, &git2go.CloneOptions{ + cloneOpts := &git2go.CloneOptions{ FetchOptions: &git2go.FetchOptions{ DownloadTags: git2go.DownloadTagsAll, RemoteCallbacks: git2go.RemoteCallbacks{ @@ -95,9 +118,10 @@ func (c *CheckoutTag) Checkout(ctx context.Context, path, url string, auth *git. CertificateCheckCallback: auth.CertCallback, }, }, - }) + } + repo, err := clone(ctx, path, url, auth, cloneOpts) if err != nil { - return nil, "", fmt.Errorf("unable to clone '%s', error: %w", url, err) + return nil, "", err } ref, err := repo.References.Dwim(c.tag) if err != nil { @@ -131,7 +155,7 @@ type CheckoutCommit struct { } func (c *CheckoutCommit) Checkout(ctx context.Context, path, url string, auth *git.Auth) (git.Commit, string, error) { - repo, err := git2go.Clone(url, path, &git2go.CloneOptions{ + cloneOpts := &git2go.CloneOptions{ FetchOptions: &git2go.FetchOptions{ DownloadTags: git2go.DownloadTagsNone, RemoteCallbacks: git2go.RemoteCallbacks{ @@ -140,9 +164,10 @@ func (c *CheckoutCommit) Checkout(ctx context.Context, path, url string, auth *g }, }, CheckoutBranch: c.branch, - }) + } + repo, err := clone(ctx, path, url, auth, cloneOpts) if err != nil { - return nil, "", fmt.Errorf("unable to clone '%s', error: %w", url, err) + return nil, "", err } oid, err := git2go.NewOid(c.commit) if err != nil { @@ -176,7 +201,7 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, auth *g return nil, "", fmt.Errorf("semver parse range error: %w", err) } - repo, err := git2go.Clone(url, path, &git2go.CloneOptions{ + cloneOpts := &git2go.CloneOptions{ FetchOptions: &git2go.FetchOptions{ DownloadTags: git2go.DownloadTagsAll, RemoteCallbacks: git2go.RemoteCallbacks{ @@ -184,9 +209,10 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, auth *g CertificateCheckCallback: auth.CertCallback, }, }, - }) + } + repo, err := clone(ctx, path, url, auth, cloneOpts) if err != nil { - return nil, "", fmt.Errorf("unable to clone '%s', error: %w", url, err) + return nil, "", err } tags := make(map[string]string)