From 90dc2d9a4e3441fdbbedb6f61f506cbe660c8ee3 Mon Sep 17 00:00:00 2001 From: James Liu Date: Tue, 14 May 2024 15:52:55 +1000 Subject: [PATCH 1/2] test: Use absolute path for signing keys A subsequent commit will begin to spawn Git commands from either the repository it operates on, or a fixed scratch directory. This will cause test failures due to the use of relative paths for various signing keys. Add a test helper to compute the absolute path to the current test's `testdata/` directory, and pass this to Git and Gitaly configuration during test setup. --- internal/git/localrepo/commit_test.go | 3 ++- internal/gitaly/service/commit/commit_signatures_test.go | 5 +++-- internal/gitaly/service/operations/cherry_pick_test.go | 2 +- internal/gitaly/service/operations/commit_files_test.go | 2 +- internal/gitaly/service/operations/merge_branch_test.go | 2 +- internal/gitaly/service/operations/revert_test.go | 2 +- internal/gitaly/service/operations/squash_test.go | 3 ++- internal/testhelper/testhelper.go | 8 ++++++++ 8 files changed, 19 insertions(+), 8 deletions(-) diff --git a/internal/git/localrepo/commit_test.go b/internal/git/localrepo/commit_test.go index 59f502c83c..09bf4ffe6e 100644 --- a/internal/git/localrepo/commit_test.go +++ b/internal/git/localrepo/commit_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "fmt" + "path/filepath" "strings" "testing" "time" @@ -433,7 +434,7 @@ func testWriteCommit(t *testing.T, ctx context.Context) { CommitterDate: commitDate, Message: "my custom message", GitConfig: config.Git{ - SigningKey: "testdata/signing_gpg_key", + SigningKey: filepath.Join(testhelper.TestdataAbsolutePath(t), "signing_gpg_key"), }, Sign: tc.sign, } diff --git a/internal/gitaly/service/commit/commit_signatures_test.go b/internal/gitaly/service/commit/commit_signatures_test.go index 4cb1dc804f..2ab2816fd5 100644 --- a/internal/gitaly/service/commit/commit_signatures_test.go +++ b/internal/gitaly/service/commit/commit_signatures_test.go @@ -3,6 +3,7 @@ package commit import ( "context" "fmt" + "path/filepath" "strings" "testing" @@ -58,8 +59,8 @@ func testGetCommitSignatures(t *testing.T, ctx context.Context) { cfg := testcfg.Build(t) testcfg.BuildGitalyGPG(t, cfg) - cfg.Git.SigningKey = "testdata/signing_ssh_key_ed25519" - cfg.Git.RotatedSigningKeys = []string{"testdata/signing_ssh_key_rsa"} + cfg.Git.SigningKey = filepath.Join(testhelper.TestdataAbsolutePath(t), "signing_ssh_key_ed25519") + cfg.Git.RotatedSigningKeys = []string{filepath.Join(testhelper.TestdataAbsolutePath(t), "signing_ssh_key_rsa")} cfg.SocketPath = startTestServices(t, cfg) client := newCommitServiceClient(t, cfg.SocketPath) diff --git a/internal/gitaly/service/operations/cherry_pick_test.go b/internal/gitaly/service/operations/cherry_pick_test.go index 0acfb1729d..3437500dfa 100644 --- a/internal/gitaly/service/operations/cherry_pick_test.go +++ b/internal/gitaly/service/operations/cherry_pick_test.go @@ -425,7 +425,7 @@ func testServerUserCherryPickMergeCommit(t *testing.T, ctx context.Context) { var opts []testserver.GitalyServerOpt if featureflag.GPGSigning.IsEnabled(ctx) { - opts = append(opts, testserver.WithSigningKey("testdata/signing_ssh_key_ecdsa")) + opts = append(opts, testserver.WithSigningKey(filepath.Join(testhelper.TestdataAbsolutePath(t), "signing_ssh_key_ecdsa"))) } ctx, cfg, client := setupOperationsService(t, ctx, opts...) diff --git a/internal/gitaly/service/operations/commit_files_test.go b/internal/gitaly/service/operations/commit_files_test.go index 03487fe0be..82355d7819 100644 --- a/internal/gitaly/service/operations/commit_files_test.go +++ b/internal/gitaly/service/operations/commit_files_test.go @@ -42,7 +42,7 @@ func TestUserCommitFiles(t *testing.T) { func testUserCommitFiles(t *testing.T, ctx context.Context) { opts := []testserver.GitalyServerOpt{ - testserver.WithSigningKey("testdata/signing_gpg_key"), + testserver.WithSigningKey(filepath.Join(testhelper.TestdataAbsolutePath(t), "signing_gpg_key")), } ctx, cfg, client := setupOperationsService(t, ctx, opts...) diff --git a/internal/gitaly/service/operations/merge_branch_test.go b/internal/gitaly/service/operations/merge_branch_test.go index d2f8802aa0..7119532a9d 100644 --- a/internal/gitaly/service/operations/merge_branch_test.go +++ b/internal/gitaly/service/operations/merge_branch_test.go @@ -49,7 +49,7 @@ func TestUserMergeBranch(t *testing.T) { func testUserMergeBranch(t *testing.T, ctx context.Context) { var opts []testserver.GitalyServerOpt if featureflag.GPGSigning.IsEnabled(ctx) { - opts = append(opts, testserver.WithSigningKey("testdata/signing_gpg_key")) + opts = append(opts, testserver.WithSigningKey(filepath.Join(testhelper.TestdataAbsolutePath(t), "signing_gpg_key"))) } ctx, cfg, client := setupOperationsService(t, ctx, opts...) diff --git a/internal/gitaly/service/operations/revert_test.go b/internal/gitaly/service/operations/revert_test.go index 6924f9caa2..3bf10fd878 100644 --- a/internal/gitaly/service/operations/revert_test.go +++ b/internal/gitaly/service/operations/revert_test.go @@ -511,7 +511,7 @@ func testServerUserRevertMergeCommit(t *testing.T, ctx context.Context) { var opts []testserver.GitalyServerOpt if featureflag.GPGSigning.IsEnabled(ctx) { - opts = append(opts, testserver.WithSigningKey("testdata/signing_ssh_key_rsa")) + opts = append(opts, testserver.WithSigningKey(filepath.Join(testhelper.TestdataAbsolutePath(t), "signing_ssh_key_rsa"))) } ctx, cfg, client := setupOperationsService(t, ctx, opts...) diff --git a/internal/gitaly/service/operations/squash_test.go b/internal/gitaly/service/operations/squash_test.go index 80b613e9d2..580f73ddcc 100644 --- a/internal/gitaly/service/operations/squash_test.go +++ b/internal/gitaly/service/operations/squash_test.go @@ -3,6 +3,7 @@ package operations import ( "context" "errors" + "path/filepath" "strings" "testing" @@ -50,7 +51,7 @@ func testUserSquashSuccessful(t *testing.T, ctx context.Context) { var opts []testserver.GitalyServerOpt if featureflag.GPGSigning.IsEnabled(ctx) { - opts = append(opts, testserver.WithSigningKey("testdata/signing_ssh_key_ed25519")) + opts = append(opts, testserver.WithSigningKey(filepath.Join(testhelper.TestdataAbsolutePath(t), "signing_ssh_key_ed25519"))) } ctx, cfg, client := setupOperationsService(t, ctx, opts...) diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go index 85888e9d28..fad1bfddd0 100644 --- a/internal/testhelper/testhelper.go +++ b/internal/testhelper/testhelper.go @@ -442,3 +442,11 @@ func PkgPath(paths ...string) string { rootPkgPath := path.Dir(internalPkgPath) return path.Join(append([]string{rootPkgPath}, paths...)...) } + +// TestdataAbsolutePath returns the absolute path to the current test's `testdata/` directory. +func TestdataAbsolutePath(t *testing.T) string { + _, currentFile, _, ok := runtime.Caller(1) + require.True(t, ok) + + return filepath.Join(filepath.Dir(currentFile), "testdata") +} -- GitLab From bd50ad9ed466f9e3cbacbf60d329c0ec03bfd0b1 Mon Sep 17 00:00:00 2001 From: James Liu Date: Tue, 14 May 2024 13:25:55 +1000 Subject: [PATCH 2/2] git: Restrict spawned commands to a specific dir Git commands can be created in Gitaly using the New() or NewWithoutRepo() command factory functions. The former executes the Git command in the context of a repository (by setting the --git-dir option) while the latter simply omits this option. In both cases, the command itself will be spawned in whatever the current directory is. This has recently caused some test failures in CI which could not be reproduced locally because the Git command in question _was not_ running in the context of a repository in CI, but _was_ locally (in the local Gitaly source tree). Modify the command options so Git commands are always spawned within a known working directory. This is either a fixed scratch directory within the Gitaly runtime dir for commands that don't need a repository, or the repository's relative path otherwise. Specify GIT_CEILING_DIRECTORIES to be the same directory so Git does not venture outside of its working directory. --- internal/git/command_factory.go | 25 +++++++++++++++++-- internal/gitaly/config/config.go | 6 +++++ .../gitaly/service/repository/fsck_test.go | 2 +- 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go index 6bca65ff3c..570385294d 100644 --- a/internal/git/command_factory.go +++ b/internal/git/command_factory.go @@ -19,6 +19,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/git/trace2hooks" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" + "gitlab.com/gitlab-org/gitaly/v16/internal/helper/perm" "gitlab.com/gitlab-org/gitaly/v16/internal/log" "gitlab.com/gitlab-org/gitaly/v16/internal/tracing" "gitlab.com/gitlab-org/labkit/correlation" @@ -433,6 +434,17 @@ func (cf *ExecCommandFactory) GitVersion(ctx context.Context) (Version, error) { return gitVersion, nil } +// scratchDir returns a location within Gitaly's runtime directory to spawn Git processes that do +// not operate in the context of a repository. The directory is created once if it does not exist. +func (cf *ExecCommandFactory) scratchDir() (string, error) { + dir := cf.cfg.GitScratchDir() + if err := os.Mkdir(dir, perm.PrivateDir); err != nil && !errors.Is(err, os.ErrExist) { + return "", fmt.Errorf("create scratch dir: %w", err) + } + + return dir, nil +} + // newCommand creates a new command.Command for the given git command. If a repo is given, then the // command will be run in the context of that repository. Note that this sets up arguments and // environment variables for git, but doesn't run in the directory itself. If a directory @@ -459,12 +471,20 @@ func (cf *ExecCommandFactory) newCommand(ctx context.Context, repo storage.Repos } env = append(alternates.Env(repoPath, repo.GetGitObjectDirectory(), repo.GetGitAlternateObjectDirectories()), env...) + } else { + // If the Git command is running outside the context of a repo, we restrict it to a fixed scratch + // directory here. + repoPath, err = cf.scratchDir() + if err != nil { + return nil, err + } } if config.worktreePath != "" { args = append([]string{"-C", config.worktreePath}, args...) - } else if repoPath != "" { - args = append([]string{"--git-dir", repoPath}, args...) + } else { + // Ensure Git doesn't traverse upwards to find a Git repository. + env = append([]string{fmt.Sprintf("GIT_CEILING_DIRECTORIES=%s", repoPath)}, env...) } execEnv := cf.GetExecutionEnvironment(ctx) @@ -501,6 +521,7 @@ func (cf *ExecCommandFactory) newCommand(ctx context.Context, repo storage.Repos commandOpts = append( commandOpts, + command.WithDir(repoPath), command.WithEnvironment(env), command.WithCommandName("git", sc.Name), command.WithCgroup(cf.cgroupsManager, cgroupsAddCommandOpts...), diff --git a/internal/gitaly/config/config.go b/internal/gitaly/config/config.go index c9e228a61a..14613a2fd2 100644 --- a/internal/gitaly/config/config.go +++ b/internal/gitaly/config/config.go @@ -944,6 +944,12 @@ func (cfg *Cfg) InternalSocketPath() string { return filepath.Join(cfg.InternalSocketDir(), "intern") } +// GitScratchDir is a fixed directory for running Git commands that don't operate +// against a repository. +func (cfg *Cfg) GitScratchDir() string { + return filepath.Join(cfg.RuntimeDir, "git-scratch") +} + func (cfg *Cfg) validateBinDir() error { if len(cfg.BinDir) == 0 { return fmt.Errorf("bin_dir: is not set") diff --git a/internal/gitaly/service/repository/fsck_test.go b/internal/gitaly/service/repository/fsck_test.go index 283be0d5e2..749c113a60 100644 --- a/internal/gitaly/service/repository/fsck_test.go +++ b/internal/gitaly/service/repository/fsck_test.go @@ -82,7 +82,7 @@ func TestFsck(t *testing.T) { setupData := setupData{ repo: repo, requireResponse: func(actual *gitalypb.FsckResponse) { - require.Regexp(t, "^fatal: not a git repository: '.+'\n$", string(actual.Error)) + require.Regexp(t, "(?s)^fatal: not a git repository .+\n$", string(actual.Error)) }, } -- GitLab