From 3476ecb9508574a10e9260255fdbb6a83224a737 Mon Sep 17 00:00:00 2001 From: Michael Bridgen Date: Tue, 13 Jul 2021 16:06:17 +0100 Subject: [PATCH] Fail push if a ref update is rejected libgit2's Push method will succeed even when ref updates are rejected, meaning it can silently fail if you e.g., use branch protection in GitHub. To make these errors visible, a callback is supplied to Push, which checks for a non-empty status (on the advice of https://libgit2.org/libgit2/#HEAD/group/callback/git_push_update_reference_cb). For whatever reason, gogit seems overly sensitive to hook errors (in a way that `git` and libgit2 aren't), and reports "invalid pkg-len found" when it sees a rejected ref message. This doesn't affect the runtime code, since that uses libgit2 -- but it does affect the test code, which initialises the git repo used in many tests, so more care is needed to push only the main branch, so as not to trigger a rejection. Signed-off-by: Michael Bridgen --- controllers/git_test.go | 84 +++++++++++++++++-- .../imageupdateautomation_controller.go | 19 ++++- controllers/update_test.go | 4 +- go.mod | 2 +- go.sum | 3 +- 5 files changed, 102 insertions(+), 10 deletions(-) diff --git a/controllers/git_test.go b/controllers/git_test.go index e0e0a606..ca38d56a 100644 --- a/controllers/git_test.go +++ b/controllers/git_test.go @@ -1,6 +1,7 @@ package controllers import ( + "context" "io/ioutil" "os" "path/filepath" @@ -8,13 +9,17 @@ import ( "time" "github.com/go-git/go-billy/v5/memfs" - "github.com/go-git/go-git/v5" + gogit "github.com/go-git/go-git/v5" + "github.com/go-git/go-git/v5/plumbing" "github.com/go-git/go-git/v5/plumbing/object" "github.com/go-git/go-git/v5/storage/memory" "github.com/go-logr/logr" + + "github.com/fluxcd/pkg/gittestserver" + "github.com/fluxcd/source-controller/pkg/git" ) -func populateRepoFromFixture(repo *git.Repository, fixture string) error { +func populateRepoFromFixture(repo *gogit.Repository, fixture string) error { working, err := repo.Worktree() if err != nil { return err @@ -59,7 +64,7 @@ func populateRepoFromFixture(repo *git.Repository, fixture string) error { return err } - if _, err = working.Commit("Initial revision from "+fixture, &git.CommitOptions{ + if _, err = working.Commit("Initial revision from "+fixture, &gogit.CommitOptions{ Author: &object.Signature{ Name: "Testbot", Email: "test@example.com", @@ -73,7 +78,7 @@ func populateRepoFromFixture(repo *git.Repository, fixture string) error { } func TestRepoForFixture(t *testing.T) { - repo, err := git.Init(memory.NewStorage(), memfs.New()) + repo, err := gogit.Init(memory.NewStorage(), memfs.New()) if err != nil { t.Fatal(err) } @@ -92,7 +97,7 @@ func TestIgnoreBrokenSymlink(t *testing.T) { } defer os.RemoveAll(tmp) - repo, err := git.PlainInit(tmp, false) + repo, err := gogit.PlainInit(tmp, false) if err != nil { t.Fatal(err) } @@ -106,3 +111,72 @@ func TestIgnoreBrokenSymlink(t *testing.T) { t.Fatalf("expected no changes but got: %v", err) } } + +// this is a hook script that will reject a ref update for a branch +// that's not `main` +const rejectBranch = ` +if [ "$1" != "refs/heads/main" ]; then + echo "*** Rejecting push to non-main branch $1" >&2 + exit 1 +fi +` + +func TestPushRejected(t *testing.T) { + // Check that pushing to a repository which rejects a ref update + // results in an error. Why would a repo reject an update? If yu + // use e.g., branch protection in GitHub, this is what happens -- + // see + // https://github.com/fluxcd/image-automation-controller/issues/194. + + branch := "push-branch" + + gitServer, err := gittestserver.NewTempGitServer() + if err != nil { + t.Fatal(err) + } + gitServer.AutoCreate() + gitServer.InstallUpdateHook(rejectBranch) + + if err = gitServer.StartHTTP(); err != nil { + t.Fatal(err) + } + + // this is currently defined in update_test.go, but handy right here .. + if err = initGitRepo(gitServer, "testdata/appconfig", "main", "/appconfig.git"); err != nil { + t.Fatal(err) + } + + tmp, err := ioutil.TempDir("", "gotest-imageauto-git") + if err != nil { + t.Fatal(err) + } + repoURL := gitServer.HTTPAddress() + "/appconfig.git" + repo, err := gogit.PlainClone(tmp, false, &gogit.CloneOptions{ + URL: repoURL, + ReferenceName: plumbing.NewBranchReferenceName("main"), + }) + + // This is here to guard against push in general being broken + err = push(context.TODO(), tmp, "main", repoAccess{ + url: repoURL, + auth: &git.Auth{}, + }) + if err != nil { + t.Fatal(err) + } + + // This is not under test, but needed for the next bit + if err = switchBranch(repo, branch); err != nil { + t.Fatal(err) + } + + // This is supposed to fail, because the hook rejects the branch + // pushed to. + err = push(context.TODO(), tmp, branch, repoAccess{ + url: repoURL, + auth: &git.Auth{}, + }) + if err == nil { + t.Error("push to a forbidden branch is expected to fail, but succeeded") + } +} diff --git a/controllers/imageupdateautomation_controller.go b/controllers/imageupdateautomation_controller.go index fef01c5a..2aee00bb 100644 --- a/controllers/imageupdateautomation_controller.go +++ b/controllers/imageupdateautomation_controller.go @@ -674,10 +674,25 @@ func push(ctx context.Context, path, branch string, access repoAccess) error { if err != nil { return err } + + callbacks := access.remoteCallbacks() + + // calling repo.Push will succeed even if a reference update is + // rejected; to detect this case, this callback is supplied. + var callbackErr error + callbacks.PushUpdateReferenceCallback = func(refname, status string) libgit2.ErrorCode { + if status != "" { + callbackErr = fmt.Errorf("ref %s rejected: %s", refname, status) + } + return libgit2.ErrOk + } err = origin.Push([]string{fmt.Sprintf("refs/heads/%s:refs/heads/%s", branch, branch)}, &libgit2.PushOptions{ - RemoteCallbacks: access.remoteCallbacks(), + RemoteCallbacks: callbacks, }) - return libgit2PushError(err) + if err != nil { + return libgit2PushError(err) + } + return callbackErr } func libgit2PushError(err error) error { diff --git a/controllers/update_test.go b/controllers/update_test.go index 31be00a1..99bbc614 100644 --- a/controllers/update_test.go +++ b/controllers/update_test.go @@ -1149,7 +1149,9 @@ func initGitRepo(gitServer *gittestserver.GitServer, fixture, branch, repository } return remote.Push(&git.PushOptions{ - RefSpecs: []config.RefSpec{"refs/heads/*:refs/heads/*"}, + RefSpecs: []config.RefSpec{ + config.RefSpec(fmt.Sprintf("refs/heads/%s:refs/heads/%s", branch, branch)), + }, }) } diff --git a/go.mod b/go.mod index dd3279d0..a020c037 100644 --- a/go.mod +++ b/go.mod @@ -11,7 +11,7 @@ require ( // If you bump this, change REFLECTOR_VER in the Makefile to match github.com/fluxcd/image-reflector-controller/api v0.11.0 github.com/fluxcd/pkg/apis/meta v0.10.0 - github.com/fluxcd/pkg/gittestserver v0.3.0 + github.com/fluxcd/pkg/gittestserver v0.3.1 github.com/fluxcd/pkg/runtime v0.12.0 github.com/fluxcd/pkg/ssh v0.1.0 // If you bump this, change SOURCE_VER in the Makefile to match diff --git a/go.sum b/go.sum index 11f30a66..81e50404 100644 --- a/go.sum +++ b/go.sum @@ -303,8 +303,9 @@ github.com/fluxcd/image-reflector-controller/api v0.11.0 h1:Pz9GuUQvmJO5nJPEtGBR github.com/fluxcd/image-reflector-controller/api v0.11.0/go.mod h1:X4qZ11pfA5w1ajbkYbWmQ3hBW3gzCyIjhU87AvV6o2A= github.com/fluxcd/pkg/apis/meta v0.10.0 h1:N7wVGHC1cyPdT87hrDC7UwCwRwnZdQM46PBSLjG2rlE= github.com/fluxcd/pkg/apis/meta v0.10.0/go.mod h1:CW9X9ijMTpNe7BwnokiUOrLl/h13miwVr/3abEQLbKE= -github.com/fluxcd/pkg/gittestserver v0.3.0 h1:6aa30mybecBwBWaJ2IEk7pQzefWnjWjxkTSrHMHawvg= github.com/fluxcd/pkg/gittestserver v0.3.0/go.mod h1:8j36Z6B0BuKNZZ6exAWoyDEpyQoFcjz1IX3WBT7PZNg= +github.com/fluxcd/pkg/gittestserver v0.3.1 h1:TlzLjm5T30iGybOgkVz4TNDeIWLjDmsqlREnFuCSQ0A= +github.com/fluxcd/pkg/gittestserver v0.3.1/go.mod h1:8j36Z6B0BuKNZZ6exAWoyDEpyQoFcjz1IX3WBT7PZNg= 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/go.mod h1:Yie8n7xuu5Nvf1Q7302LKsubJhWpwzCaK0rLJvmF7aI=