From e7872b5745eb7c871f5e584546f739f58974967d Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 17 May 2024 06:06:52 +0200 Subject: [PATCH] Revert "git: Restrict spawned commands to a specific dir" In bd50ad9ed (git: Restrict spawned commands to a specific dir, 2024-05-14), we have started to set `GIT_CEILING_DIRECTORIES` such that spawned Git commands will not escape their repository. In the context of commands with a repository we set the variable to the repository's path. And when we have a command without a repository, we create a temporary directory, chdir the command into it and then set up the environment variable to point to that directory. The latter case has an unintended side effect: when socket paths have been specified with a relative path, then this relative path now cannot be resolved anymore due to our use of chdir. This wasn't ever a problem with any of the other Git commands that use a repository because we use `git -C` there, which does _not_ cause us to chdir. We could likely fix this by using `git -C` here, as well, instead of setting the working directory of the new process. But for now, let's rather revert this whole change and reintroduce it in a later step. --- internal/git/command_factory.go | 25 ++----------------- internal/gitaly/config/config.go | 6 ----- .../gitaly/service/repository/fsck_test.go | 2 +- 3 files changed, 3 insertions(+), 30 deletions(-) diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go index 68286b3125..24b284673d 100644 --- a/internal/git/command_factory.go +++ b/internal/git/command_factory.go @@ -19,7 +19,6 @@ 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" @@ -434,17 +433,6 @@ 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 @@ -471,20 +459,12 @@ 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 { - // Ensure Git doesn't traverse upwards to find a Git repository. - env = append([]string{fmt.Sprintf("GIT_CEILING_DIRECTORIES=%s", repoPath)}, env...) + } else if repoPath != "" { + args = append([]string{"--git-dir", repoPath}, args...) } execEnv := cf.GetExecutionEnvironment(ctx) @@ -521,7 +501,6 @@ 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 14613a2fd2..c9e228a61a 100644 --- a/internal/gitaly/config/config.go +++ b/internal/gitaly/config/config.go @@ -944,12 +944,6 @@ 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 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