From 787b25b80952e8d6de183283bb4ebf4a6be358a9 Mon Sep 17 00:00:00 2001 From: James Liu Date: Tue, 14 May 2024 13:25:55 +1000 Subject: [PATCH 1/4] 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 provided with a -C flag. 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 | 27 ++++++++- internal/git/command_factory_test.go | 55 +++++++++++++++++++ internal/gitaly/config/config.go | 6 ++ .../gitaly/service/repository/fsck_test.go | 2 +- 4 files changed, 86 insertions(+), 4 deletions(-) diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go index 24b284673d..9d3208d274 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,14 +471,23 @@ 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 set repoPath to a fixed scratch + // directory here which is later used to restrict the working directory of the process. + 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...) + repoPath = config.worktreePath } + // Ensure Git doesn't traverse upwards to find a Git repository. + env = append([]string{fmt.Sprintf("GIT_CEILING_DIRECTORIES=%s", repoPath)}, env...) + args = append([]string{"-C", repoPath}, args...) + execEnv := cf.GetExecutionEnvironment(ctx) env = append(env, execEnv.EnvironmentVariables...) diff --git a/internal/git/command_factory_test.go b/internal/git/command_factory_test.go index 71a7cf4009..fd453c724d 100644 --- a/internal/git/command_factory_test.go +++ b/internal/git/command_factory_test.go @@ -632,6 +632,61 @@ func TestExecCommandFactory_config(t *testing.T) { require.Equal(t, expectedEnv, strings.Split(strings.TrimSpace(stdout.String()), "\n")) } +func TestExecCommandFactory_WorkingDirectoryNew(t *testing.T) { + ctx := testhelper.Context(t) + cfg := testcfg.Build(t) + gitCmdFactory := gittest.NewCommandFactory(t, cfg) + + t.Run("without a repository", func(t *testing.T) { + expectedWorkingDirectory := cfg.GitScratchDir() + + cmd, err := gitCmdFactory.NewWithoutRepo(ctx, git.Command{Name: "version"}) + require.NoError(t, err) + require.Contains(t, cmd.Env(), "GIT_CEILING_DIRECTORIES="+expectedWorkingDirectory) + require.Contains(t, cmd.Args(), "-C") + require.Contains(t, cmd.Args(), expectedWorkingDirectory) + }) + + t.Run("with a repository", func(t *testing.T) { + repo, repoDir := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{ + SkipCreationViaService: true, + }) + + expectedWorkingDirectory := repoDir + + cmd, err := gitCmdFactory.New(ctx, repo, git.Command{ + Name: "rev-list", + Flags: []git.Option{ + git.Flag{Name: "--all"}, + }, + }) + require.NoError(t, err) + require.Contains(t, cmd.Env(), "GIT_CEILING_DIRECTORIES="+expectedWorkingDirectory) + require.Contains(t, cmd.Args(), "-C") + require.Contains(t, cmd.Args(), expectedWorkingDirectory) + }) + + t.Run("with a repository and worktree", func(t *testing.T) { + repo, repoDir := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{ + SkipCreationViaService: true, + }) + + expectedWorkingDirectory := filepath.Join(repoDir, "my-worktree") + + cmd, err := gitCmdFactory.New(ctx, repo, git.Command{ + Name: "rev-list", + Flags: []git.Option{ + git.Flag{Name: "--all"}, + }, + }, git.WithWorktree(expectedWorkingDirectory)) + + require.NoError(t, err) + require.Contains(t, cmd.Env(), "GIT_CEILING_DIRECTORIES="+expectedWorkingDirectory) + require.Contains(t, cmd.Args(), "-C") + require.Contains(t, cmd.Args(), expectedWorkingDirectory) + }) +} + // TestFsckConfiguration tests the hardcoded configuration of the // git fsck subcommand generated through the command factory. func TestFsckConfiguration(t *testing.T) { 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 From 39f33815c384a546df90518cd803c603120b8eab Mon Sep 17 00:00:00 2001 From: James Liu Date: Mon, 27 May 2024 16:00:32 +1000 Subject: [PATCH 2/4] config: Use an absolute path for SocketPath Gitaly and Praefect can be configured to listen on a Unix socket or a TCP port. This configuration is read from the respective `config.toml` file. Since the socket path is simply a file path, it's possible for Gitaly and Praefect to be configured with a relative socket path. This happens frequently in Rails specs, where the path `tmp/tests/gitaly/*.socket` is used. This has not been problematic in the past because whenever the socket path is referenced, the current working directory of the Gitaly process has always been correct relative to that path. With https://gitlab.com/gitlab-org/gitaly/-/issues/6065 however, we want to restrict Git invocations to a specific working directory. Setting the working directory of the command or using Git's `-C` flag both have the side effect of causing test failures due to the relative socket path. Fix this by computing the absolute path of the socket when the config struct is created. --- internal/gitaly/config/config.go | 9 +++++++++ internal/gitaly/config/config_test.go | 6 +++--- internal/praefect/config/config.go | 10 ++++++++++ internal/praefect/config/config_test.go | 6 +++--- internal/testhelper/testhelper.go | 8 ++++++++ 5 files changed, 33 insertions(+), 6 deletions(-) diff --git a/internal/gitaly/config/config.go b/internal/gitaly/config/config.go index 14613a2fd2..5080ed9b13 100644 --- a/internal/gitaly/config/config.go +++ b/internal/gitaly/config/config.go @@ -716,6 +716,15 @@ func Load(file io.Reader) (Cfg, error) { cfg.Storages[i].Path = filepath.Clean(cfg.Storages[i].Path) } + // Ensure the socket path is absolute so issues don't arise if the working directory changes at runtime. + if cfg.SocketPath != "" { + absSocketPath, err := filepath.Abs(cfg.SocketPath) + if err != nil { + return Cfg{}, fmt.Errorf("get absolute socket path: %w", err) + } + cfg.SocketPath = absSocketPath + } + return cfg, nil } diff --git a/internal/gitaly/config/config_test.go b/internal/gitaly/config/config_test.go index 6b901cc4c4..5ffcbdd138 100644 --- a/internal/gitaly/config/config_test.go +++ b/internal/gitaly/config/config_test.go @@ -398,7 +398,7 @@ func TestLoadConfigCommand(t *testing.T) { }, expectedCfg: modifyDefaultConfig(func(cfg *Cfg) { cfg.ConfigCommand = cmd - cfg.SocketPath = "value" + cfg.SocketPath = filepath.Join(testhelper.WorkingDirectory(t), "value") }), } }, @@ -415,7 +415,7 @@ func TestLoadConfigCommand(t *testing.T) { }, expectedCfg: modifyDefaultConfig(func(cfg *Cfg) { cfg.ConfigCommand = cmd - cfg.SocketPath = "overridden_value" + cfg.SocketPath = filepath.Join(testhelper.WorkingDirectory(t), "overridden_value") }), } }, @@ -432,7 +432,7 @@ func TestLoadConfigCommand(t *testing.T) { }, expectedCfg: modifyDefaultConfig(func(cfg *Cfg) { cfg.ConfigCommand = cmd - cfg.SocketPath = "socket_path" + cfg.SocketPath = filepath.Join(testhelper.WorkingDirectory(t), "socket_path") cfg.ListenAddr = "listen_addr" }), } diff --git a/internal/praefect/config/config.go b/internal/praefect/config/config.go index 62cbea2f57..c336d18b32 100644 --- a/internal/praefect/config/config.go +++ b/internal/praefect/config/config.go @@ -8,6 +8,7 @@ import ( "io" "os" "os/exec" + "path/filepath" "sort" "time" @@ -338,6 +339,15 @@ func FromReader(reader io.Reader) (Config, error) { conf.setDefaults() + // Ensure the socket path is absolute so issues don't arise if the working directory changes at runtime. + if conf.SocketPath != "" { + absSocketPath, err := filepath.Abs(conf.SocketPath) + if err != nil { + return Config{}, fmt.Errorf("get absolute socket path: %w", err) + } + conf.SocketPath = absSocketPath + } + return *conf, nil } diff --git a/internal/praefect/config/config_test.go b/internal/praefect/config/config_test.go index 4818959d14..5ccc3b920e 100644 --- a/internal/praefect/config/config_test.go +++ b/internal/praefect/config/config_test.go @@ -1118,7 +1118,7 @@ func TestConfig_ConfigCommand(t *testing.T) { }, expectedCfg: modifyDefaultConfig(func(cfg *Config) { cfg.ConfigCommand = cmd - cfg.SocketPath = "value" + cfg.SocketPath = filepath.Join(testhelper.WorkingDirectory(t), "value") }), } }, @@ -1135,7 +1135,7 @@ func TestConfig_ConfigCommand(t *testing.T) { }, expectedCfg: modifyDefaultConfig(func(cfg *Config) { cfg.ConfigCommand = cmd - cfg.SocketPath = "overridden_value" + cfg.SocketPath = filepath.Join(testhelper.WorkingDirectory(t), "overridden_value") }), } }, @@ -1152,7 +1152,7 @@ func TestConfig_ConfigCommand(t *testing.T) { }, expectedCfg: modifyDefaultConfig(func(cfg *Config) { cfg.ConfigCommand = cmd - cfg.SocketPath = "socket_path" + cfg.SocketPath = filepath.Join(testhelper.WorkingDirectory(t), "socket_path") cfg.ListenAddr = "listen_addr" }), } diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go index 0d0bb09d25..10d4bd8316 100644 --- a/internal/testhelper/testhelper.go +++ b/internal/testhelper/testhelper.go @@ -448,3 +448,11 @@ func TestdataAbsolutePath(t *testing.T) string { return filepath.Join(filepath.Dir(currentFile), "testdata") } + +// WorkingDirectory returns the current working directory. +func WorkingDirectory(t *testing.T) string { + wd, err := os.Getwd() + require.NoError(t, err) + + return wd +} -- GitLab From 3beb8d188c9a1ee03ef53d8cd5562eb772cb029a Mon Sep 17 00:00:00 2001 From: James Liu Date: Fri, 31 May 2024 14:22:51 +1000 Subject: [PATCH 3/4] Revert "config: Use an absolute path for SocketPath" This reverts commit 39f33815c384a546df90518cd803c603120b8eab. --- internal/gitaly/config/config.go | 9 --------- internal/gitaly/config/config_test.go | 6 +++--- internal/praefect/config/config.go | 10 ---------- internal/praefect/config/config_test.go | 6 +++--- internal/testhelper/testhelper.go | 8 -------- 5 files changed, 6 insertions(+), 33 deletions(-) diff --git a/internal/gitaly/config/config.go b/internal/gitaly/config/config.go index 5080ed9b13..14613a2fd2 100644 --- a/internal/gitaly/config/config.go +++ b/internal/gitaly/config/config.go @@ -716,15 +716,6 @@ func Load(file io.Reader) (Cfg, error) { cfg.Storages[i].Path = filepath.Clean(cfg.Storages[i].Path) } - // Ensure the socket path is absolute so issues don't arise if the working directory changes at runtime. - if cfg.SocketPath != "" { - absSocketPath, err := filepath.Abs(cfg.SocketPath) - if err != nil { - return Cfg{}, fmt.Errorf("get absolute socket path: %w", err) - } - cfg.SocketPath = absSocketPath - } - return cfg, nil } diff --git a/internal/gitaly/config/config_test.go b/internal/gitaly/config/config_test.go index 5ffcbdd138..6b901cc4c4 100644 --- a/internal/gitaly/config/config_test.go +++ b/internal/gitaly/config/config_test.go @@ -398,7 +398,7 @@ func TestLoadConfigCommand(t *testing.T) { }, expectedCfg: modifyDefaultConfig(func(cfg *Cfg) { cfg.ConfigCommand = cmd - cfg.SocketPath = filepath.Join(testhelper.WorkingDirectory(t), "value") + cfg.SocketPath = "value" }), } }, @@ -415,7 +415,7 @@ func TestLoadConfigCommand(t *testing.T) { }, expectedCfg: modifyDefaultConfig(func(cfg *Cfg) { cfg.ConfigCommand = cmd - cfg.SocketPath = filepath.Join(testhelper.WorkingDirectory(t), "overridden_value") + cfg.SocketPath = "overridden_value" }), } }, @@ -432,7 +432,7 @@ func TestLoadConfigCommand(t *testing.T) { }, expectedCfg: modifyDefaultConfig(func(cfg *Cfg) { cfg.ConfigCommand = cmd - cfg.SocketPath = filepath.Join(testhelper.WorkingDirectory(t), "socket_path") + cfg.SocketPath = "socket_path" cfg.ListenAddr = "listen_addr" }), } diff --git a/internal/praefect/config/config.go b/internal/praefect/config/config.go index c336d18b32..62cbea2f57 100644 --- a/internal/praefect/config/config.go +++ b/internal/praefect/config/config.go @@ -8,7 +8,6 @@ import ( "io" "os" "os/exec" - "path/filepath" "sort" "time" @@ -339,15 +338,6 @@ func FromReader(reader io.Reader) (Config, error) { conf.setDefaults() - // Ensure the socket path is absolute so issues don't arise if the working directory changes at runtime. - if conf.SocketPath != "" { - absSocketPath, err := filepath.Abs(conf.SocketPath) - if err != nil { - return Config{}, fmt.Errorf("get absolute socket path: %w", err) - } - conf.SocketPath = absSocketPath - } - return *conf, nil } diff --git a/internal/praefect/config/config_test.go b/internal/praefect/config/config_test.go index 5ccc3b920e..4818959d14 100644 --- a/internal/praefect/config/config_test.go +++ b/internal/praefect/config/config_test.go @@ -1118,7 +1118,7 @@ func TestConfig_ConfigCommand(t *testing.T) { }, expectedCfg: modifyDefaultConfig(func(cfg *Config) { cfg.ConfigCommand = cmd - cfg.SocketPath = filepath.Join(testhelper.WorkingDirectory(t), "value") + cfg.SocketPath = "value" }), } }, @@ -1135,7 +1135,7 @@ func TestConfig_ConfigCommand(t *testing.T) { }, expectedCfg: modifyDefaultConfig(func(cfg *Config) { cfg.ConfigCommand = cmd - cfg.SocketPath = filepath.Join(testhelper.WorkingDirectory(t), "overridden_value") + cfg.SocketPath = "overridden_value" }), } }, @@ -1152,7 +1152,7 @@ func TestConfig_ConfigCommand(t *testing.T) { }, expectedCfg: modifyDefaultConfig(func(cfg *Config) { cfg.ConfigCommand = cmd - cfg.SocketPath = filepath.Join(testhelper.WorkingDirectory(t), "socket_path") + cfg.SocketPath = "socket_path" cfg.ListenAddr = "listen_addr" }), } diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go index 10d4bd8316..0d0bb09d25 100644 --- a/internal/testhelper/testhelper.go +++ b/internal/testhelper/testhelper.go @@ -448,11 +448,3 @@ func TestdataAbsolutePath(t *testing.T) string { return filepath.Join(filepath.Dir(currentFile), "testdata") } - -// WorkingDirectory returns the current working directory. -func WorkingDirectory(t *testing.T) string { - wd, err := os.Getwd() - require.NoError(t, err) - - return wd -} -- GitLab From a5182da7c90191bad6c0b7f638e238837baed3c4 Mon Sep 17 00:00:00 2001 From: James Liu Date: Fri, 31 May 2024 14:22:34 +1000 Subject: [PATCH 4/4] wip --- internal/git/command_factory.go | 19 +++++++++++-------- internal/git/command_factory_test.go | 8 +++++--- .../gitaly/service/repository/fsck_test.go | 2 +- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go index 9d3208d274..c677cdd751 100644 --- a/internal/git/command_factory.go +++ b/internal/git/command_factory.go @@ -472,22 +472,25 @@ 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 set repoPath to a fixed scratch - // directory here which is later used to restrict the working directory of the process. - repoPath, err = cf.scratchDir() + // If the Git command is running outside the context of a repo, we restrict the working directory of the + // process to a fixed scratch directory. + scratchDir, err := cf.scratchDir() if err != nil { return nil, err } + + // Ensure Git doesn't traverse up the directory tree. + env = append([]string{fmt.Sprintf("GIT_CEILING_DIRECTORIES=%s", scratchDir)}, env...) + + args = append([]string{"-C", scratchDir}, args...) } if config.worktreePath != "" { - repoPath = config.worktreePath + args = append([]string{"-C", config.worktreePath}, args...) + } else if repoPath != "" { + args = append([]string{"--git-dir", repoPath}, args...) } - // Ensure Git doesn't traverse upwards to find a Git repository. - env = append([]string{fmt.Sprintf("GIT_CEILING_DIRECTORIES=%s", repoPath)}, env...) - args = append([]string{"-C", repoPath}, args...) - execEnv := cf.GetExecutionEnvironment(ctx) env = append(env, execEnv.EnvironmentVariables...) diff --git a/internal/git/command_factory_test.go b/internal/git/command_factory_test.go index fd453c724d..5fc82af15c 100644 --- a/internal/git/command_factory_test.go +++ b/internal/git/command_factory_test.go @@ -661,9 +661,10 @@ func TestExecCommandFactory_WorkingDirectoryNew(t *testing.T) { }, }) require.NoError(t, err) - require.Contains(t, cmd.Env(), "GIT_CEILING_DIRECTORIES="+expectedWorkingDirectory) - require.Contains(t, cmd.Args(), "-C") + require.Contains(t, cmd.Args(), "--git-dir") require.Contains(t, cmd.Args(), expectedWorkingDirectory) + + require.NotContains(t, cmd.Args(), "-C") }) t.Run("with a repository and worktree", func(t *testing.T) { @@ -681,9 +682,10 @@ func TestExecCommandFactory_WorkingDirectoryNew(t *testing.T) { }, git.WithWorktree(expectedWorkingDirectory)) require.NoError(t, err) - require.Contains(t, cmd.Env(), "GIT_CEILING_DIRECTORIES="+expectedWorkingDirectory) require.Contains(t, cmd.Args(), "-C") require.Contains(t, cmd.Args(), expectedWorkingDirectory) + + require.NotContains(t, cmd.Args(), "--git-dir") }) } diff --git a/internal/gitaly/service/repository/fsck_test.go b/internal/gitaly/service/repository/fsck_test.go index 749c113a60..283be0d5e2 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, "(?s)^fatal: not a git repository .+\n$", string(actual.Error)) + require.Regexp(t, "^fatal: not a git repository: '.+'\n$", string(actual.Error)) }, } -- GitLab