From 02498eced8f9d7f23cdd0c5e240e7d8045d90438 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Wed, 17 Oct 2018 17:03:17 +0200 Subject: [PATCH] Make git hooks self healing If the git hooks aren't symlinked properly, GitLab authentication and authorization is skipped completely. So on most Git operations, if these hooks are missing this commit will try to repair them when triggering an upload-pack, or receive-pack git action. This change also allows the Gitaly team to change the hooks in following releases to any new ones. And allowing for absorbing the gitlab-shell hooks into this project. Previously attempted in: https://gitlab.com/gitlab-org/gitaly/commit/87198d32837, which ran into race conditions. This approach does not Part of https://gitlab.com/gitlab-org/gitaly/issues/1226 --- .../unreleased/zj-self-healing-hooks.yml | 5 ++ internal/git/helper.go | 4 +- internal/git/hooks/hooks.go | 12 ++++ internal/git/hooks/hooks_test.go | 12 ++++ internal/rubyserver/rubyserver.go | 2 + internal/service/operations/branches_test.go | 22 +++---- .../service/operations/cherry_pick_test.go | 11 ++-- .../service/operations/commit_files_test.go | 11 ++-- internal/service/operations/merge_test.go | 18 +++--- internal/service/operations/rebase_test.go | 10 +--- internal/service/operations/revert_test.go | 11 ++-- internal/service/operations/tags_test.go | 22 +++---- .../service/operations/testhelper_test.go | 37 +++++++++--- .../operations/update_branches_test.go | 13 ++-- internal/service/smarthttp/receive_pack.go | 10 ++-- .../service/smarthttp/receive_pack_test.go | 44 +++++++++++++- internal/service/ssh/receive_pack.go | 9 ++- internal/service/ssh/receive_pack_test.go | 25 ++++++++ ruby/lib/gitlab/config.rb | 8 +-- ruby/lib/gitlab/git/gitlab_projects.rb | 2 +- ruby/lib/gitlab/git/hook.rb | 12 ++-- ruby/lib/gitlab/git/repository.rb | 4 +- ruby/spec/lib/gitlab/config_spec.rb | 1 - ruby/spec/lib/gitlab/git/hook_spec.rb | 59 +++++++++++++++++++ ruby/spec/spec_helper.rb | 1 + 25 files changed, 261 insertions(+), 104 deletions(-) create mode 100644 changelogs/unreleased/zj-self-healing-hooks.yml create mode 100644 internal/git/hooks/hooks.go create mode 100644 internal/git/hooks/hooks_test.go create mode 100644 ruby/spec/lib/gitlab/git/hook_spec.rb diff --git a/changelogs/unreleased/zj-self-healing-hooks.yml b/changelogs/unreleased/zj-self-healing-hooks.yml new file mode 100644 index 0000000000..1b64a06446 --- /dev/null +++ b/changelogs/unreleased/zj-self-healing-hooks.yml @@ -0,0 +1,5 @@ +--- +title: Make git hooks self healing +merge_request: 886 +author: +type: added diff --git a/internal/git/helper.go b/internal/git/helper.go index 85f607c2c4..87a7222be8 100644 --- a/internal/git/helper.go +++ b/internal/git/helper.go @@ -63,8 +63,8 @@ func Version() (string, error) { func BuildGitOptions(gitOpts []string, otherOpts ...string) []string { args := []string{} - if len(gitOpts) > 0 { - args = append([]string{"-c"}, gitOpts...) + for _, opt := range gitOpts { + args = append(args, "-c", opt) } return append(args, otherOpts...) diff --git a/internal/git/hooks/hooks.go b/internal/git/hooks/hooks.go new file mode 100644 index 0000000000..63e1053302 --- /dev/null +++ b/internal/git/hooks/hooks.go @@ -0,0 +1,12 @@ +package hooks + +import ( + "path" + + "gitlab.com/gitlab-org/gitaly/internal/config" +) + +// Path returns the path where the global git hooks are located +func Path() string { + return path.Join(config.Config.GitlabShell.Dir, "hooks") +} diff --git a/internal/git/hooks/hooks_test.go b/internal/git/hooks/hooks_test.go new file mode 100644 index 0000000000..9a8088de94 --- /dev/null +++ b/internal/git/hooks/hooks_test.go @@ -0,0 +1,12 @@ +package hooks + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestPath(t *testing.T) { + assert.True(t, strings.HasSuffix(Path(), "hooks")) +} diff --git a/internal/rubyserver/rubyserver.go b/internal/rubyserver/rubyserver.go index 46e935ca9c..3fdfc31f75 100644 --- a/internal/rubyserver/rubyserver.go +++ b/internal/rubyserver/rubyserver.go @@ -16,6 +16,7 @@ import ( "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/internal/command" "gitlab.com/gitlab-org/gitaly/internal/config" + "gitlab.com/gitlab-org/gitaly/internal/git/hooks" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/internal/rubyserver/balancer" "gitlab.com/gitlab-org/gitaly/internal/supervisor" @@ -107,6 +108,7 @@ func Start() (*Server, error) { "GITALY_RUBY_GITLAB_SHELL_PATH="+cfg.GitlabShell.Dir, "GITALY_RUBY_GITALY_BIN_DIR="+cfg.BinDir, "GITALY_VERSION="+version.GetVersion(), + "GITALY_GIT_HOOKS_DIR="+hooks.Path(), ) env = append(env, command.GitEnv...) diff --git a/internal/service/operations/branches_test.go b/internal/service/operations/branches_test.go index ab6d8e5d59..9c41aac213 100644 --- a/internal/service/operations/branches_test.go +++ b/internal/service/operations/branches_test.go @@ -1,10 +1,8 @@ package operations import ( - "io/ioutil" "os" "os/exec" - "path" "testing" "github.com/stretchr/testify/require" @@ -99,9 +97,7 @@ func TestSuccessfulGitHooksForUserCreateBranchRequest(t *testing.T) { t.Run(hookName, func(t *testing.T) { defer exec.Command("git", "-C", testRepoPath, "branch", "-D", branchName).Run() - hookPath, hookOutputTempPath := WriteEnvToHook(t, testRepoPath, hookName) - defer os.Remove(hookPath) - defer os.Remove(hookOutputTempPath) + hookOutputTempPath := WriteEnvToHook(t, testRepoPath, hookName) ctx, cancel := testhelper.Context() defer cancel() @@ -138,9 +134,9 @@ func TestFailedUserCreateBranchDueToHooks(t *testing.T) { hookContent := []byte("#!/bin/sh\nprintenv | paste -sd ' ' -\nexit 1") for _, hookName := range gitlabPreHooks { - hookPath := path.Join(testRepoPath, "hooks", hookName) - ioutil.WriteFile(hookPath, hookContent, 0755) - defer os.Remove(hookPath) + remove, err := OverrideHooks(hookName, hookContent) + require.NoError(t, err) + defer remove() ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -289,8 +285,7 @@ func TestSuccessfulGitHooksForUserDeleteBranchRequest(t *testing.T) { t.Run(hookName, func(t *testing.T) { testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", branchNameInput) - hookPath, hookOutputTempPath := WriteEnvToHook(t, testRepoPath, hookName) - defer os.Remove(hookPath) + hookOutputTempPath := WriteEnvToHook(t, testRepoPath, hookName) defer os.Remove(hookOutputTempPath) ctx, cancel := testhelper.Context() @@ -395,10 +390,9 @@ func TestFailedUserDeleteBranchDueToHooks(t *testing.T) { for _, hookName := range gitlabPreHooks { t.Run(hookName, func(t *testing.T) { - require.NoError(t, os.MkdirAll(path.Join(testRepoPath, "hooks"), 0755)) - hookPath := path.Join(testRepoPath, "hooks", hookName) - ioutil.WriteFile(hookPath, hookContent, 0755) - defer os.Remove(hookPath) + remove, err := OverrideHooks(hookName, hookContent) + require.NoError(t, err) + defer remove() ctx, cancel := testhelper.Context() defer cancel() diff --git a/internal/service/operations/cherry_pick_test.go b/internal/service/operations/cherry_pick_test.go index 6326419ca7..d9440d2cb0 100644 --- a/internal/service/operations/cherry_pick_test.go +++ b/internal/service/operations/cherry_pick_test.go @@ -1,10 +1,8 @@ package operations_test import ( - "io/ioutil" "net" "os" - "path" "testing" "github.com/stretchr/testify/require" @@ -160,8 +158,7 @@ func TestSuccessfulGitHooksForUserCherryPickRequest(t *testing.T) { for _, hookName := range operations.GitlabHooks { t.Run(hookName, func(t *testing.T) { - hookPath, hookOutputTempPath := operations.WriteEnvToHook(t, testRepoPath, hookName) - defer os.Remove(hookPath) + hookOutputTempPath := operations.WriteEnvToHook(t, testRepoPath, hookName) defer os.Remove(hookOutputTempPath) md := testhelper.GitalyServersMetadata(t, serverSocketPath) @@ -301,9 +298,9 @@ func TestFailedUserCherryPickRequestDueToPreReceiveError(t *testing.T) { for _, hookName := range operations.GitlabPreHooks { t.Run(hookName, func(t *testing.T) { - hookPath := path.Join(testRepoPath, "hooks", hookName) - require.NoError(t, ioutil.WriteFile(hookPath, hookContent, 0755)) - defer os.Remove(hookPath) + remove, err := operations.OverrideHooks(hookName, hookContent) + require.NoError(t, err) + defer remove() md := testhelper.GitalyServersMetadata(t, serverSocketPath) ctx := metadata.NewOutgoingContext(ctxOuter, md) diff --git a/internal/service/operations/commit_files_test.go b/internal/service/operations/commit_files_test.go index ec76bad83d..ab8d501538 100644 --- a/internal/service/operations/commit_files_test.go +++ b/internal/service/operations/commit_files_test.go @@ -2,9 +2,6 @@ package operations_test import ( "fmt" - "io/ioutil" - "os" - "path" "testing" "github.com/stretchr/testify/require" @@ -139,7 +136,7 @@ func TestFailedUserCommitFilesRequestDueToHooks(t *testing.T) { client, conn := operations.NewOperationClient(t, serverSocketPath) defer conn.Close() - testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) + testRepo, _, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() ctxOuter, cancel := testhelper.Context() @@ -154,9 +151,9 @@ func TestFailedUserCommitFilesRequestDueToHooks(t *testing.T) { for _, hookName := range operations.GitlabPreHooks { t.Run(hookName, func(t *testing.T) { - hookPath := path.Join(testRepoPath, "hooks", hookName) - ioutil.WriteFile(hookPath, hookContent, 0755) - defer os.Remove(hookPath) + remove, err := operations.OverrideHooks(hookName, hookContent) + require.NoError(t, err) + defer remove() md := testhelper.GitalyServersMetadata(t, serverSocketPath) ctx := metadata.NewOutgoingContext(ctxOuter, md) diff --git a/internal/service/operations/merge_test.go b/internal/service/operations/merge_test.go index 1fdea95dba..397633f429 100644 --- a/internal/service/operations/merge_test.go +++ b/internal/service/operations/merge_test.go @@ -6,7 +6,6 @@ import ( "io/ioutil" "os" "os/exec" - "path" "strings" "testing" "time" @@ -51,9 +50,7 @@ func TestSuccessfulMerge(t *testing.T) { hooks := GitlabHooks hookTempfiles := make([]string, len(hooks)) for i, h := range hooks { - var hookPath string - hookPath, hookTempfiles[i] = WriteEnvToHook(t, testRepoPath, h) - defer os.Remove(hookPath) + hookTempfiles[i] = WriteEnvToHook(t, testRepoPath, h) defer os.Remove(hookTempfiles[i]) } @@ -238,10 +235,9 @@ func TestFailedMergeDueToHooks(t *testing.T) { for _, hookName := range gitlabPreHooks { t.Run(hookName, func(t *testing.T) { - require.NoError(t, os.MkdirAll(path.Join(testRepoPath, "hooks"), 0755)) - hookPath := path.Join(testRepoPath, "hooks", hookName) - ioutil.WriteFile(hookPath, hookContent, 0755) - defer os.Remove(hookPath) + remove, err := OverrideHooks(hookName, hookContent) + require.NoError(t, err) + defer remove() ctx, cancel := testhelper.Context() defer cancel() @@ -443,9 +439,9 @@ func TestFailedUserFFBranchDueToHooks(t *testing.T) { for _, hookName := range gitlabPreHooks { t.Run(hookName, func(t *testing.T) { - hookPath := path.Join(testRepoPath, "hooks", hookName) - ioutil.WriteFile(hookPath, hookContent, 0755) - defer os.Remove(hookPath) + remove, err := OverrideHooks(hookName, hookContent) + require.NoError(t, err) + defer remove() ctx, cancel := testhelper.Context() defer cancel() diff --git a/internal/service/operations/rebase_test.go b/internal/service/operations/rebase_test.go index 18fca0c688..79c8c2e5ca 100644 --- a/internal/service/operations/rebase_test.go +++ b/internal/service/operations/rebase_test.go @@ -1,9 +1,6 @@ package operations_test import ( - "io/ioutil" - "os" - "path" "strings" "testing" @@ -101,12 +98,11 @@ func TestFailedUserRebaseRequestDueToPreReceiveError(t *testing.T) { } hookContent := []byte("#!/bin/sh\necho GL_ID=$GL_ID\nexit 1") - for _, hookName := range operations.GitlabPreHooks { t.Run(hookName, func(t *testing.T) { - hookPath := path.Join(testRepoPath, "hooks", hookName) - require.NoError(t, ioutil.WriteFile(hookPath, hookContent, 0755)) - defer os.Remove(hookPath) + remove, err := operations.OverrideHooks(hookName, hookContent) + require.NoError(t, err) + defer remove() md := testhelper.GitalyServersMetadata(t, serverSocketPath) ctx := metadata.NewOutgoingContext(ctxOuter, md) diff --git a/internal/service/operations/revert_test.go b/internal/service/operations/revert_test.go index 340fb6416a..621b858dec 100644 --- a/internal/service/operations/revert_test.go +++ b/internal/service/operations/revert_test.go @@ -1,9 +1,7 @@ package operations_test import ( - "io/ioutil" "os" - "path" "testing" "github.com/stretchr/testify/require" @@ -157,8 +155,7 @@ func TestSuccessfulGitHooksForUserRevertRequest(t *testing.T) { for _, hookName := range operations.GitlabHooks { t.Run(hookName, func(t *testing.T) { - hookPath, hookOutputTempPath := operations.WriteEnvToHook(t, testRepoPath, hookName) - defer os.Remove(hookPath) + hookOutputTempPath := operations.WriteEnvToHook(t, testRepoPath, hookName) defer os.Remove(hookOutputTempPath) md := testhelper.GitalyServersMetadata(t, serverSocketPath) @@ -298,9 +295,9 @@ func TestFailedUserRevertRequestDueToPreReceiveError(t *testing.T) { for _, hookName := range operations.GitlabPreHooks { t.Run(hookName, func(t *testing.T) { - hookPath := path.Join(testRepoPath, "hooks", hookName) - require.NoError(t, ioutil.WriteFile(hookPath, hookContent, 0755)) - defer os.Remove(hookPath) + remove, err := operations.OverrideHooks(hookName, hookContent) + require.NoError(t, err) + defer remove() md := testhelper.GitalyServersMetadata(t, serverSocketPath) ctx := metadata.NewOutgoingContext(ctxOuter, md) diff --git a/internal/service/operations/tags_test.go b/internal/service/operations/tags_test.go index 385711a110..268d3571da 100644 --- a/internal/service/operations/tags_test.go +++ b/internal/service/operations/tags_test.go @@ -1,10 +1,8 @@ package operations import ( - "io/ioutil" "os" "os/exec" - "path" "strings" "testing" @@ -83,8 +81,7 @@ func TestSuccessfulGitHooksForUserDeleteTagRequest(t *testing.T) { t.Run(hookName, func(t *testing.T) { testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", tagNameInput) - hookPath, hookOutputTempPath := WriteEnvToHook(t, testRepoPath, hookName) - defer os.Remove(hookPath) + hookOutputTempPath := WriteEnvToHook(t, testRepoPath, hookName) defer os.Remove(hookOutputTempPath) ctx, cancel := testhelper.Context() @@ -211,8 +208,7 @@ func TestSuccessfulGitHooksForUserCreateTagRequest(t *testing.T) { t.Run(hookName, func(t *testing.T) { defer exec.Command("git", "-C", testRepoPath, "tag", "-d", tagName).Run() - hookPath, hookOutputTempPath := WriteEnvToHook(t, testRepoPath, hookName) - defer os.Remove(hookPath) + hookOutputTempPath := WriteEnvToHook(t, testRepoPath, hookName) defer os.Remove(hookOutputTempPath) ctx, cancel := testhelper.Context() @@ -318,9 +314,9 @@ func TestFailedUserDeleteTagDueToHooks(t *testing.T) { for _, hookName := range gitlabPreHooks { t.Run(hookName, func(t *testing.T) { - hookPath := path.Join(testRepoPath, "hooks", hookName) - ioutil.WriteFile(hookPath, hookContent, 0755) - defer os.Remove(hookPath) + remove, err := OverrideHooks(hookName, hookContent) + require.NoError(t, err) + defer remove() ctx, cancel := testhelper.Context() defer cancel() @@ -342,7 +338,7 @@ func TestFailedUserCreateTagDueToHooks(t *testing.T) { client, conn := newOperationClient(t, serverSocketPath) defer conn.Close() - testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) + testRepo, _, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() user := &gitalypb.User{ @@ -361,9 +357,9 @@ func TestFailedUserCreateTagDueToHooks(t *testing.T) { hookContent := []byte("#!/bin/sh\necho GL_ID=$GL_ID\nexit 1") for _, hookName := range gitlabPreHooks { - hookPath := path.Join(testRepoPath, "hooks", hookName) - ioutil.WriteFile(hookPath, hookContent, 0755) - defer os.Remove(hookPath) + remove, err := OverrideHooks(hookName, hookContent) + require.NoError(t, err) + defer remove() ctx, cancel := testhelper.Context() defer cancel() diff --git a/internal/service/operations/testhelper_test.go b/internal/service/operations/testhelper_test.go index a000798689..dc182a93eb 100644 --- a/internal/service/operations/testhelper_test.go +++ b/internal/service/operations/testhelper_test.go @@ -12,6 +12,8 @@ import ( log "github.com/sirupsen/logrus" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" + "gitlab.com/gitlab-org/gitaly/internal/config" + "gitlab.com/gitlab-org/gitaly/internal/git/hooks" "gitlab.com/gitlab-org/gitaly/internal/rubyserver" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "google.golang.org/grpc" @@ -44,7 +46,20 @@ func TestMain(m *testing.M) { func testMain(m *testing.M) int { defer testhelper.MustHaveNoChildProcess() - var err error + hookDir, err := ioutil.TempDir("", "gitaly-tmp-hooks") + if err != nil { + log.Fatal(err) + } + + defer func(oldDir string) { + config.Config.GitlabShell.Dir = oldDir + }(config.Config.GitlabShell.Dir) + config.Config.GitlabShell.Dir = hookDir + + if err := os.MkdirAll(hooks.Path(), 0755); err != nil { + log.Fatal(err) + } + defer os.RemoveAll(hookDir) testhelper.ConfigureGitalySSH() @@ -92,17 +107,25 @@ func newOperationClient(t *testing.T, serverSocketPath string) (gitalypb.Operati var NewOperationClient = newOperationClient -func WriteEnvToHook(t *testing.T, repoPath, hookName string) (string, string) { +// The callee is responsible for clean up of the specific hook, testMain removes +// the hook dir +func WriteEnvToHook(t *testing.T, repoPath, hookName string) string { hookOutputTemp, err := ioutil.TempFile("", "") require.NoError(t, err) require.NoError(t, hookOutputTemp.Close()) - defer os.Remove(hookOutputTemp.Name()) - hookContent := fmt.Sprintf("#!/bin/sh\n/usr/bin/env > %s\n", hookOutputTemp.Name()) - hookPath := path.Join(repoPath, "hooks", hookName) - ioutil.WriteFile(hookPath, []byte(hookContent), 0755) + _, err = OverrideHooks(hookName, []byte(hookContent)) + require.NoError(t, err) + + return hookOutputTemp.Name() +} + +// When testing hooks, the content for one specific hook can be defined, to simulate +// behaviours on different hook content +func OverrideHooks(name string, content []byte) (func(), error) { + fullPath := path.Join(hooks.Path(), name) - return hookPath, hookOutputTemp.Name() + return func() { os.Remove(fullPath) }, ioutil.WriteFile(fullPath, content, 0755) } diff --git a/internal/service/operations/update_branches_test.go b/internal/service/operations/update_branches_test.go index 6cc0aac1ff..56be3faf24 100644 --- a/internal/service/operations/update_branches_test.go +++ b/internal/service/operations/update_branches_test.go @@ -1,9 +1,7 @@ package operations import ( - "io/ioutil" "os" - "path" "testing" "github.com/stretchr/testify/require" @@ -64,8 +62,7 @@ func TestSuccessfulGitHooksForUserUpdateBranchRequest(t *testing.T) { testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() - hookPath, hookOutputTempPath := WriteEnvToHook(t, testRepoPath, hookName) - defer os.Remove(hookPath) + hookOutputTempPath := WriteEnvToHook(t, testRepoPath, hookName) defer os.Remove(hookOutputTempPath) ctx, cancel := testhelper.Context() @@ -112,11 +109,11 @@ func TestFailedUserUpdateBranchDueToHooks(t *testing.T) { hookContent := []byte("#!/bin/sh\nprintenv | paste -sd ' ' -\nexit 1") for _, hookName := range gitlabPreHooks { - hookPath := path.Join(testRepoPath, "hooks", hookName) - ioutil.WriteFile(hookPath, hookContent, 0755) - defer os.Remove(hookPath) + remove, err := OverrideHooks(hookName, hookContent) + require.NoError(t, err) + defer remove() - ctx, cancel := context.WithCancel(context.Background()) + ctx, cancel := testhelper.Context() defer cancel() response, err := client.UserUpdateBranch(ctx, request) diff --git a/internal/service/smarthttp/receive_pack.go b/internal/service/smarthttp/receive_pack.go index 040193aaf9..a98988e092 100644 --- a/internal/service/smarthttp/receive_pack.go +++ b/internal/service/smarthttp/receive_pack.go @@ -8,6 +8,7 @@ import ( "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/internal/command" "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/git/hooks" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/streamio" "google.golang.org/grpc/codes" @@ -50,15 +51,16 @@ func (s *server) PostReceivePack(stream gitalypb.SmartHTTPService_PostReceivePac env = append(env, fmt.Sprintf("GL_USERNAME=%s", req.GlUsername)) } - env = git.AddGitProtocolEnv(ctx, req, env) - env = append(env, command.GitEnv...) - repoPath, err := helper.GetRepoPath(req.Repository) if err != nil { return err } - gitOptions := git.BuildGitOptions(req.GitConfigOptions, "receive-pack", "--stateless-rpc", repoPath) + env = git.AddGitProtocolEnv(ctx, req, env) + env = append(env, command.GitEnv...) + + opts := append([]string{fmt.Sprintf("core.hooksPath=%s", hooks.Path())}, req.GitConfigOptions...) + gitOptions := git.BuildGitOptions(opts, "receive-pack", "--stateless-rpc", repoPath) cmd, err := git.BareCommand(ctx, stdin, stdout, nil, env, gitOptions...) if err != nil { diff --git a/internal/service/smarthttp/receive_pack_test.go b/internal/service/smarthttp/receive_pack_test.go index dc6ffbbf5e..679270f7ac 100644 --- a/internal/service/smarthttp/receive_pack_test.go +++ b/internal/service/smarthttp/receive_pack_test.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "io/ioutil" + "os" "path" "strings" "testing" @@ -14,6 +15,7 @@ import ( "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/internal/config" "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/git/hooks" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/streamio" "golang.org/x/net/context" @@ -30,7 +32,7 @@ func TestSuccessfulReceivePackRequest(t *testing.T) { client, conn := newSmartHTTPClient(t, serverSocketPath) defer conn.Close() - ctx, cancel := context.WithCancel(context.Background()) + ctx, cancel := testhelper.Context() defer cancel() stream, err := client.PostReceivePack(ctx) @@ -57,7 +59,7 @@ func TestSuccessfulReceivePackRequestWithGitOpts(t *testing.T) { client, conn := newSmartHTTPClient(t, serverSocketPath) defer conn.Close() - ctx, cancel := context.WithCancel(context.Background()) + ctx, cancel := testhelper.Context() defer cancel() stream, err := client.PostReceivePack(ctx) @@ -137,6 +139,44 @@ func TestFailedReceivePackRequestWithGitOpts(t *testing.T) { require.Equal(t, expectedResponse, string(response), "Expected response to be %q, got %q", expectedResponse, response) } +func TestFailedReceivePackRequestDueToHooksFailure(t *testing.T) { + hookDir, err := ioutil.TempDir("", "gitaly-tmp-hooks") + require.NoError(t, err) + defer os.RemoveAll(hookDir) + + defer func(oldDir string) { + config.Config.GitlabShell.Dir = hookDir + }(config.Config.GitlabShell.Dir) + config.Config.GitlabShell.Dir = hookDir + + require.NoError(t, os.MkdirAll(hooks.Path(), 0755)) + + hookContent := []byte("#!/bin/sh\nexit 1") + ioutil.WriteFile(path.Join(hooks.Path(), "pre-receive"), hookContent, 0755) + + server, serverSocketPath := runSmartHTTPServer(t) + defer server.Stop() + + repo, _, cleanup := testhelper.NewTestRepo(t) + defer cleanup() + + client, conn := newSmartHTTPClient(t, serverSocketPath) + defer conn.Close() + + ctx, cancel := testhelper.Context() + defer cancel() + + stream, err := client.PostReceivePack(ctx) + require.NoError(t, err) + + push := newTestPush(t, nil) + firstRequest := &gitalypb.PostReceivePackRequest{Repository: repo, GlId: "user-123", GlRepository: "project-123"} + response := doPush(t, stream, firstRequest, push.body) + + expectedResponse := "004a\x01000eunpack ok\n0033ng refs/heads/master pre-receive hook declined\n00000000" + require.Equal(t, expectedResponse, string(response), "Expected response to be %q, got %q", expectedResponse, response) +} + func doPush(t *testing.T, stream gitalypb.SmartHTTPService_PostReceivePackClient, firstRequest *gitalypb.PostReceivePackRequest, body io.Reader) []byte { require.NoError(t, stream.Send(firstRequest)) diff --git a/internal/service/ssh/receive_pack.go b/internal/service/ssh/receive_pack.go index 84decfe9c8..c74e66ad26 100644 --- a/internal/service/ssh/receive_pack.go +++ b/internal/service/ssh/receive_pack.go @@ -8,6 +8,7 @@ import ( "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/internal/command" "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/git/hooks" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/streamio" "google.golang.org/grpc/codes" @@ -50,15 +51,17 @@ func (s *server) SSHReceivePack(stream gitalypb.SSHService_SSHReceivePackServer) if req.GlRepository != "" { env = append(env, fmt.Sprintf("GL_REPOSITORY=%s", req.GlRepository)) } - env = git.AddGitProtocolEnv(ctx, req, env) - env = append(env, command.GitEnv...) repoPath, err := helper.GetRepoPath(req.Repository) if err != nil { return err } - gitOptions := git.BuildGitOptions(req.GitConfigOptions, "receive-pack", repoPath) + env = git.AddGitProtocolEnv(ctx, req, env) + env = append(env, command.GitEnv...) + + opts := append([]string{fmt.Sprintf("core.hooksPath=%s", hooks.Path())}, req.GitConfigOptions...) + gitOptions := git.BuildGitOptions(opts, "receive-pack", repoPath) cmd, err := git.BareCommand(ctx, stdin, stdout, stderr, env, gitOptions...) if err != nil { diff --git a/internal/service/ssh/receive_pack_test.go b/internal/service/ssh/receive_pack_test.go index 37531c853a..55f1e47407 100644 --- a/internal/service/ssh/receive_pack_test.go +++ b/internal/service/ssh/receive_pack_test.go @@ -15,6 +15,7 @@ import ( "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/internal/config" "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/git/hooks" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "golang.org/x/net/context" "google.golang.org/grpc/codes" @@ -138,6 +139,30 @@ func TestReceivePackPushFailure(t *testing.T) { t.Errorf("local and remote head equal. push did not fail") } } + +} + +func TestReceivePackPushHookFailure(t *testing.T) { + server, serverSocketPath := runSSHServer(t) + defer server.Stop() + + hookDir, err := ioutil.TempDir("", "gitaly-tmp-hooks") + require.NoError(t, err) + defer os.RemoveAll(hookDir) + + defer func(oldDir string) { + config.Config.GitlabShell.Dir = oldDir + }(config.Config.GitlabShell.Dir) + config.Config.GitlabShell.Dir = hookDir + + require.NoError(t, os.MkdirAll(hooks.Path(), 0755)) + + hookContent := []byte("#!/bin/sh\nexit 1") + ioutil.WriteFile(path.Join(hooks.Path(), "pre-receive"), hookContent, 0755) + + _, _, err = testCloneAndPush(t, serverSocketPath, pushParams{storageName: testRepo.GetStorageName(), glID: "1"}) + require.Error(t, err) + require.Contains(t, err.Error(), "(pre-receive hook declined)") } func testCloneAndPush(t *testing.T, serverSocketPath string, params pushParams) (string, string, error) { diff --git a/ruby/lib/gitlab/config.rb b/ruby/lib/gitlab/config.rb index d6fae8db64..379d28f4ce 100644 --- a/ruby/lib/gitlab/config.rb +++ b/ruby/lib/gitlab/config.rb @@ -6,6 +6,10 @@ module Gitlab ENV['GITALY_RUBY_GIT_BIN_PATH'] end + def hooks_directory + ENV['GITALY_GIT_HOOKS_DIR'] + end + def write_buffer_size @write_buffer_size ||= ENV['GITALY_RUBY_WRITE_BUFFER_SIZE'].to_i end @@ -20,10 +24,6 @@ module Gitlab ENV['GITALY_RUBY_GITLAB_SHELL_PATH'] end - def hooks_path - File.join(path, 'hooks') - end - def git_timeout 10800 # TODO make this configurable or eliminate otherwise https://gitlab.com/gitlab-org/gitaly/issues/885 end diff --git a/ruby/lib/gitlab/git/gitlab_projects.rb b/ruby/lib/gitlab/git/gitlab_projects.rb index 5cbe85574f..6ad7e14787 100644 --- a/ruby/lib/gitlab/git/gitlab_projects.rb +++ b/ruby/lib/gitlab/git/gitlab_projects.rb @@ -23,7 +23,7 @@ module Gitlab Gitlab::Git::GitlabProjects.new( storage_path, gitaly_repository.relative_path, - global_hooks_path: Gitlab.config.gitlab_shell.hooks_path, + global_hooks_path: Gitlab::Git::Hook.directory, logger: Rails.logger ) end diff --git a/ruby/lib/gitlab/git/hook.rb b/ruby/lib/gitlab/git/hook.rb index 94ff5b4980..43b35b39ad 100644 --- a/ruby/lib/gitlab/git/hook.rb +++ b/ruby/lib/gitlab/git/hook.rb @@ -1,17 +1,19 @@ -# Gitaly note: JV: looks like this is only used by Gitlab::Git::HooksService in -# app/services. We shouldn't bother migrating this until we know how -# Gitlab::Git::HooksService will be migrated. +# frozen_string_literal: true module Gitlab module Git class Hook - GL_PROTOCOL = 'web'.freeze + def self.directory + Gitlab.config.git.hooks_directory + end + + GL_PROTOCOL = 'web' attr_reader :name, :path, :repository def initialize(name, repository) @name = name @repository = repository - @path = File.join(repo_path, 'hooks', name) + @path = File.join(self.class.directory, name) end def repo_path diff --git a/ruby/lib/gitlab/git/repository.rb b/ruby/lib/gitlab/git/repository.rb index f3e6505c96..b782fbc06e 100644 --- a/ruby/lib/gitlab/git/repository.rb +++ b/ruby/lib/gitlab/git/repository.rb @@ -64,7 +64,9 @@ module Gitlab repo = Rugged::Repository.init_at(repo_path, true) repo.close - symlink_hooks_to = Gitlab.config.gitlab_shell.hooks_path + # TODO: remove this when self healing hooks has been merged at least + # one release ago: https://gitlab.com/gitlab-org/gitaly/merge_requests/886 + symlink_hooks_to = Gitlab::Git::Hook.directory create_hooks(repo_path, symlink_hooks_to) if symlink_hooks_to.present? end diff --git a/ruby/spec/lib/gitlab/config_spec.rb b/ruby/spec/lib/gitlab/config_spec.rb index bd2aaaede2..2a503d0052 100644 --- a/ruby/spec/lib/gitlab/config_spec.rb +++ b/ruby/spec/lib/gitlab/config_spec.rb @@ -15,6 +15,5 @@ describe Gitlab::Config do end it { expect(subject.path).to eq(gitlab_shell_path) } - it { expect(subject.hooks_path).to eq(File.join(gitlab_shell_path, 'hooks')) } end end diff --git a/ruby/spec/lib/gitlab/git/hook_spec.rb b/ruby/spec/lib/gitlab/git/hook_spec.rb new file mode 100644 index 0000000000..b43e6a1532 --- /dev/null +++ b/ruby/spec/lib/gitlab/git/hook_spec.rb @@ -0,0 +1,59 @@ +require 'spec_helper' + +describe Gitlab::Git::Hook do + include TestRepo + + describe '.directory' do + it 'does not raise an KeyError' do + expect { described_class.directory }.not_to raise_error + end + end + + describe '#trigger' do + let!(:old_env) { ENV['GITALY_GIT_HOOKS_DIR'] } + let(:tmp_dir) { Dir.mktmpdir } + let(:hook_names) { %w[pre-receive post-receive update] } + let(:repo) { gitlab_git_from_gitaly(test_repo_read_only) } + + before do + hook_names.each do |f| + path = File.join(tmp_dir, f) + File.write(path, script) + FileUtils.chmod("u+x", path) + end + + ENV['GITALY_GIT_HOOKS_DIR'] = tmp_dir + end + + after do + FileUtils.remove_entry(tmp_dir) + ENV['GITALY_GIT_HOOKS_DIR'] = old_env + end + + context 'when the hooks are successful' do + let(:script) { "#!/usr/bin/env true" } + + it 'returns true' do + hook_names.each do |hook| + trigger_result = described_class.new(hook, repo) + .trigger('1', 'admin', '0' * 40, 'a' * 40, 'master') + + expect(trigger_result.first).to be(true) + end + end + end + + context 'when the hooks fail' do + let(:script) { "#!/usr/bin/env false" } + + it 'returns false' do + hook_names.each do |hook| + trigger_result = described_class.new(hook, repo) + .trigger('1', 'admin', '0' * 40, 'a' * 40, 'master') + + expect(trigger_result.first).to be(false) + end + end + end + end +end diff --git a/ruby/spec/spec_helper.rb b/ruby/spec/spec_helper.rb index 818c3c44c4..5a7a6e4830 100644 --- a/ruby/spec/spec_helper.rb +++ b/ruby/spec/spec_helper.rb @@ -8,6 +8,7 @@ require 'factory_bot' Dir[File.join(__dir__, 'support/helpers/*.rb')].each { |f| require f } ENV['GITALY_RUBY_GIT_BIN_PATH'] ||= 'git' +ENV['GITALY_GIT_HOOKS_DIR'] ||= File.join(Gitlab.config.gitlab_shell.path.to_s, "hooks") RSpec.configure do |config| config.include FactoryBot::Syntax::Methods -- GitLab