From 4bf248eae659f0b984b543e199e28ba6349d5268 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20H=C3=B6rl?= Date: Fri, 7 Feb 2025 17:08:53 +0100 Subject: [PATCH 1/8] Ensure the cred helper does not persist config injections To not allow persisting git configs via the cred helper configuration, we delete the file and create if from scratch. This is important for executors which cache the repo (ie. the docker executor), so that no configs changes are cached across runs. --- shells/abstract.go | 1 + shells/abstract_test.go | 21 ++++++++++++++++----- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/shells/abstract.go b/shells/abstract.go index dfe5193bf8..26afb9f651 100644 --- a/shells/abstract.go +++ b/shells/abstract.go @@ -663,6 +663,7 @@ func (b *AbstractShell) writeRefspecFetchCmd(w ShellWriter, info common.ShellScr } func (b *AbstractShell) configureGitCredHelper(w ShellWriter, info common.ShellScriptInfo, credConfigFile string) error { + w.RmFile(credConfigFile) build := info.Build shellName := build.Runner.Shell if shellName == "" { diff --git a/shells/abstract_test.go b/shells/abstract_test.go index 74456e96e9..8e8621e92b 100644 --- a/shells/abstract_test.go +++ b/shells/abstract_test.go @@ -2638,8 +2638,9 @@ func TestAbstractShell_writeGetSourcesScript_scriptHooks(t *testing.T) { m.EXPECT().MkTmpDir("git-template").Return("git-template-dir").Once() m.EXPECT().Join("git-template-dir", "config").Return("git-template-dir-config").Once() m.EXPECT().Command("git", "config", "-f", "git-template-dir-config", mock.Anything, mock.Anything) - m.EXPECT().RmFile(mock.Anything) - m.EXPECT().RmFilesRecursive(path.Join("build-dir", ".git", "refs"), "*.lock").Once() + + expectFileCleanup(m) + m.EXPECT().Command("git", "init", "build-dir", "--template", "git-template-dir").Once() m.EXPECT().Cd("build-dir").Once() @@ -2681,16 +2682,26 @@ func TestAbstractShell_writeGetSourcesScript_scriptHooks(t *testing.T) { } } +func expectFileCleanup(shellWriter *MockShellWriter) { + for _, f := range []string{"index.lock", "shallow.lock", "HEAD.lock", "hooks/post-checkout", "config.lock"} { + shellWriter.EXPECT().RmFile("build-dir/.git/" + f).Once() + } + shellWriter.EXPECT().RmFilesRecursive("build-dir/.git/refs", "*.lock").Once() +} + func expectGitCredHelperSetup(shellWriter *MockShellWriter, remoteURL string) { + expectedCredHelperPath := "/some/path/cred-helper.conf" expectedCredSection := "credential." + remoteURL + shellWriter.EXPECT().RmFile(expectedCredHelperPath).Once() + shellWriter.EXPECT().TmpFile(mock.Anything).Return("/some/path/cred-helper.conf").Once() shellWriter.EXPECT().CommandWithStdin("url="+remoteURL, "git", "credential", "reject").Once() - shellWriter.EXPECT().Command("git", "config", "-f", "/some/path/cred-helper.conf", expectedCredSection+".username", mock.AnythingOfType("string")).Once() - shellWriter.EXPECT().Command("git", "config", "-f", "/some/path/cred-helper.conf", expectedCredSection+".helper", mock.MatchedBy(startWithBang)).Once() - shellWriter.EXPECT().Command("git", "config", "include.path", "/some/path/cred-helper.conf") + shellWriter.EXPECT().Command("git", "config", "-f", expectedCredHelperPath, expectedCredSection+".username", mock.AnythingOfType("string")).Once() + shellWriter.EXPECT().Command("git", "config", "-f", expectedCredHelperPath, expectedCredSection+".helper", mock.MatchedBy(startWithBang)).Once() + shellWriter.EXPECT().Command("git", "config", "include.path", expectedCredHelperPath).Once() } func startWithBang(val string) bool { -- GitLab From 7c6ccbdc1183601c43e7e883c256cecb552a079d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20H=C3=B6rl?= Date: Mon, 10 Feb 2025 13:03:56 +0100 Subject: [PATCH 2/8] Add new config to clean up git configuration We clean up some git related configuration, so it's not cached across runs: - `.tmp/git-template/config` - `.tmp/git-template/hooks` - `.git/config` - `.git/hooks` By default this is enabled, however it can explicitly disabled by users. --- common/config.go | 1 + executors/custom/integration_test.go | 103 ++++++++----- executors/shell/shell_integration_test.go | 175 ++++++++++++++-------- shells/abstract.go | 46 ++++-- shells/abstract_test.go | 92 ++++++++---- shells/powershell_test.go | 138 ++++++++++++----- 6 files changed, 380 insertions(+), 175 deletions(-) diff --git a/common/config.go b/common/config.go index e71c03c08f..3d105c219f 100644 --- a/common/config.go +++ b/common/config.go @@ -1190,6 +1190,7 @@ type RunnerSettings struct { DebugTraceDisabled bool `toml:"debug_trace_disabled,omitempty" json:"debug_trace_disabled" long:"debug-trace-disabled" env:"RUNNER_DEBUG_TRACE_DISABLED" description:"When set to true Runner will disable the possibility of using the CI_DEBUG_TRACE feature"` SafeDirectoryCheckout *bool `toml:"safe_directory_checkout,omitempty" json:"safe_directory_checkout,omitempty" long:"safe-directory-checkout" env:"RUNNER_SAFE_DIRECTORY_CHECKOUT" description:"When set to true, Git global configuration will get a safe.directory directive pointing the job's working directory'"` + CleanGitConfig *bool `toml:"clean_git_config,omitempty" json:"clean_git_config,omitempty" long:"clean-git-config" env:"RUNNER_CLEAN_GIT_CONFIG" description:"Clean git configuration. Defaults to true"` Shell string `toml:"shell,omitempty" json:"shell" long:"shell" env:"RUNNER_SHELL" description:"Select bash, sh, cmd, pwsh or powershell" jsonschema:"enum=bash,enum=sh,enum=cmd,enum=pwsh,enum=powershell,enum="` CustomBuildDir CustomBuildDir `toml:"custom_build_dir,omitempty" json:"custom_build_dir,omitempty" group:"custom build dir configuration" namespace:"custom_build_dir"` diff --git a/executors/custom/integration_test.go b/executors/custom/integration_test.go index 1f65bc09fe..7d8853b9ec 100644 --- a/executors/custom/integration_test.go +++ b/executors/custom/integration_test.go @@ -468,46 +468,73 @@ func TestBuildChangesBranchesWhenFetchingRepo(t *testing.T) { } func TestBuildPowerShellCatchesExceptions(t *testing.T) { + tests := map[string]struct { + cleanGitConfig *bool + expectFreshRepoMessage bool + }{ + "no git cleanup": { + expectFreshRepoMessage: true, + }, + "git cleanup explicitly enabled": { + cleanGitConfig: &[]bool{true}[0], + expectFreshRepoMessage: true, + }, + "git cleanup explicitly disabled": { + cleanGitConfig: &[]bool{false}[0], + expectFreshRepoMessage: false, + }, + } + for _, shell := range []string{"powershell", "pwsh"} { t.Run(shell, func(t *testing.T) { - helpers.SkipIntegrationTests(t, shell) - - successfulBuild, err := common.GetRemoteSuccessfulBuild() - require.NoError(t, err) - - build := newBuild(t, successfulBuild, shell) - build.Variables = append( - build.Variables, - common.JobVariable{Key: "ErrorActionPreference", Value: "Stop"}, - common.JobVariable{Key: "GIT_STRATEGY", Value: "fetch"}, - ) - - out, err := buildtest.RunBuildReturningOutput(t, build) - assert.NoError(t, err) - assert.Contains(t, out, "Created fresh repository") - - out, err = buildtest.RunBuildReturningOutput(t, build) - assert.NoError(t, err) - assert.NotContains(t, out, "Created fresh repository") - assert.Regexp(t, "Checking out [a-f0-9]+ as", out) - - build.Variables = append( - build.Variables, - common.JobVariable{Key: "ErrorActionPreference", Value: "Continue"}, - ) - out, err = buildtest.RunBuildReturningOutput(t, build) - assert.NoError(t, err) - assert.NotContains(t, out, "Created fresh repository") - assert.Regexp(t, "Checking out [a-f0-9]+ as", out) - - build.Variables = append( - build.Variables, - common.JobVariable{Key: "ErrorActionPreference", Value: "SilentlyContinue"}, - ) - out, err = buildtest.RunBuildReturningOutput(t, build) - assert.NoError(t, err) - assert.NotContains(t, out, "Created fresh repository") - assert.Regexp(t, "Checking out [a-f0-9]+ as", out) + for name, test := range tests { + t.Run(name, func(t *testing.T) { + helpers.SkipIntegrationTests(t, shell) + + successfulBuild, err := common.GetRemoteSuccessfulBuild() + require.NoError(t, err) + + build := newBuild(t, successfulBuild, shell) + build.Variables = append( + build.Variables, + common.JobVariable{Key: "ErrorActionPreference", Value: "Stop"}, + common.JobVariable{Key: "GIT_STRATEGY", Value: "fetch"}, + ) + build.Runner.RunnerSettings.CleanGitConfig = test.cleanGitConfig + + checkFreshRepoMessage := assert.NotContains + if test.expectFreshRepoMessage { + checkFreshRepoMessage = assert.Contains + } + + out, err := buildtest.RunBuildReturningOutput(t, build) + assert.NoError(t, err) + assert.Contains(t, out, "Created fresh repository") + + out, err = buildtest.RunBuildReturningOutput(t, build) + assert.NoError(t, err) + checkFreshRepoMessage(t, out, "Created fresh repository") + assert.Regexp(t, "Checking out [a-f0-9]+ as", out) + + build.Variables = append( + build.Variables, + common.JobVariable{Key: "ErrorActionPreference", Value: "Continue"}, + ) + out, err = buildtest.RunBuildReturningOutput(t, build) + assert.NoError(t, err) + checkFreshRepoMessage(t, out, "Created fresh repository") + assert.Regexp(t, "Checking out [a-f0-9]+ as", out) + + build.Variables = append( + build.Variables, + common.JobVariable{Key: "ErrorActionPreference", Value: "SilentlyContinue"}, + ) + out, err = buildtest.RunBuildReturningOutput(t, build) + assert.NoError(t, err) + checkFreshRepoMessage(t, out, "Created fresh repository") + assert.Regexp(t, "Checking out [a-f0-9]+ as", out) + }) + } }) } } diff --git a/executors/shell/shell_integration_test.go b/executors/shell/shell_integration_test.go index fe2effe243..5aafd17e30 100644 --- a/executors/shell/shell_integration_test.go +++ b/executors/shell/shell_integration_test.go @@ -1186,39 +1186,67 @@ func TestBuildWithGitSubmoduleStrategyRecursiveAndGitSubmoduleDepth(t *testing.T } func TestBuildWithGitFetchSubmoduleStrategyRecursive(t *testing.T) { + tests := map[string]struct { + cleanGitConfig *bool + expectFreshRepoMessage bool + }{ + "no git cleanup": { + expectFreshRepoMessage: true, + }, + "git cleanup explicitly enabled": { + cleanGitConfig: &[]bool{true}[0], + expectFreshRepoMessage: true, + }, + "git cleanup explicitly disabled": { + cleanGitConfig: &[]bool{false}[0], + expectFreshRepoMessage: false, + }, + } + shellstest.OnEachShell(t, func(t *testing.T, shell string) { - successfulBuild, err := common.GetSuccessfulBuild() - assert.NoError(t, err) - build := newBuild(t, successfulBuild, shell) + for name, test := range tests { + t.Run(name, func(t *testing.T) { + successfulBuild, err := common.GetSuccessfulBuild() + assert.NoError(t, err) + build := newBuild(t, successfulBuild, shell) - build.Variables = append( - build.Variables, - common.JobVariable{Key: "GIT_STRATEGY", Value: "fetch"}, - common.JobVariable{Key: "GIT_SUBMODULE_STRATEGY", Value: "recursive"}, - ) + build.Variables = append( + build.Variables, + common.JobVariable{Key: "GIT_STRATEGY", Value: "fetch"}, + common.JobVariable{Key: "GIT_SUBMODULE_STRATEGY", Value: "recursive"}, + ) + build.Runner.RunnerSettings.CleanGitConfig = test.cleanGitConfig - out, err := buildtest.RunBuildReturningOutput(t, build) - assert.NoError(t, err) - assert.NotContains(t, out, "Skipping Git submodules setup") - assert.NotContains(t, out, "Updating/initializing submodules...") - assert.Contains(t, out, "Updating/initializing submodules recursively...") + out, err := buildtest.RunBuildReturningOutput(t, build) + assert.NoError(t, err) + assert.NotContains(t, out, "Skipping Git submodules setup") + assert.NotContains(t, out, "Updating/initializing submodules...") + assert.Contains(t, out, "Updating/initializing submodules recursively...") - _, err = os.Stat(filepath.Join(build.BuildDir, "gitlab-grack", ".git")) - assert.NoError(t, err, "Submodule should have been initialized") + _, err = os.Stat(filepath.Join(build.BuildDir, "gitlab-grack", ".git")) + assert.NoError(t, err, "Submodule should have been initialized") - _, err = os.Stat(filepath.Join(build.BuildDir, "gitlab-grack", "tests", "example", ".git")) - assert.NoError(t, err, "The submodule's submodule should have been initialized") + _, err = os.Stat(filepath.Join(build.BuildDir, "gitlab-grack", "tests", "example", ".git")) + assert.NoError(t, err, "The submodule's submodule should have been initialized") - // Create a file not tracked that should be cleaned in submodule. - excludedFilePath := filepath.Join(build.BuildDir, "gitlab-grack", "excluded_file") - err = os.WriteFile(excludedFilePath, []byte{}, os.ModePerm) - require.NoError(t, err) + // Create a file not tracked that should be cleaned in submodule. + excludedFilePath := filepath.Join(build.BuildDir, "gitlab-grack", "excluded_file") + err = os.WriteFile(excludedFilePath, []byte{}, os.ModePerm) + require.NoError(t, err) - // Run second build, to run fetch. - out, err = buildtest.RunBuildReturningOutput(t, build) - assert.NoError(t, err) - assert.NotContains(t, out, "Created fresh repository") - assert.Contains(t, out, "Removing excluded_file") + // Run second build, to run fetch. + out, err = buildtest.RunBuildReturningOutput(t, build) + assert.NoError(t, err) + + checkFreshRepoMessage := assert.NotContains + if test.expectFreshRepoMessage { + checkFreshRepoMessage = assert.Contains + } + checkFreshRepoMessage(t, out, "Created fresh repository") + + assert.Contains(t, out, "Removing excluded_file") + }) + } }) } @@ -1605,45 +1633,72 @@ func TestBuildChangesBranchesWhenFetchingRepo(t *testing.T) { } func TestBuildPowerShellCatchesExceptions(t *testing.T) { + tests := map[string]struct { + cleanGitConfig *bool + expectFreshRepoMessage bool + }{ + "no git cleanup": { + expectFreshRepoMessage: true, + }, + "git cleanup explicitly enabled": { + cleanGitConfig: &[]bool{true}[0], + expectFreshRepoMessage: true, + }, + "git cleanup explicitly disabled": { + cleanGitConfig: &[]bool{false}[0], + expectFreshRepoMessage: false, + }, + } + for _, shell := range []string{"powershell", "pwsh"} { t.Run(shell, func(t *testing.T) { - helpers.SkipIntegrationTests(t, shell) + for name, test := range tests { + t.Run(name, func(t *testing.T) { + helpers.SkipIntegrationTests(t, shell) - successfulBuild, err := common.GetRemoteSuccessfulBuild() - assert.NoError(t, err) - build := newBuild(t, successfulBuild, shell) - build.Variables = append( - build.Variables, - common.JobVariable{Key: "ErrorActionPreference", Value: "Stop"}, - common.JobVariable{Key: "GIT_STRATEGY", Value: "fetch"}, - ) + successfulBuild, err := common.GetRemoteSuccessfulBuild() + assert.NoError(t, err) + build := newBuild(t, successfulBuild, shell) + build.Variables = append( + build.Variables, + common.JobVariable{Key: "ErrorActionPreference", Value: "Stop"}, + common.JobVariable{Key: "GIT_STRATEGY", Value: "fetch"}, + ) + build.Runner.RunnerSettings.CleanGitConfig = test.cleanGitConfig - out, err := buildtest.RunBuildReturningOutput(t, build) - assert.NoError(t, err) - assert.Contains(t, out, "Created fresh repository") + checkFreshRepoMessage := assert.NotContains + if test.expectFreshRepoMessage { + checkFreshRepoMessage = assert.Contains + } - out, err = buildtest.RunBuildReturningOutput(t, build) - assert.NoError(t, err) - assert.NotContains(t, out, "Created fresh repository") - assert.Regexp(t, "Checking out [a-f0-9]+ as", out) - - build.Variables = append( - build.Variables, - common.JobVariable{Key: "ErrorActionPreference", Value: "Continue"}, - ) - out, err = buildtest.RunBuildReturningOutput(t, build) - assert.NoError(t, err) - assert.NotContains(t, out, "Created fresh repository") - assert.Regexp(t, "Checking out [a-f0-9]+ as", out) - - build.Variables = append( - build.Variables, - common.JobVariable{Key: "ErrorActionPreference", Value: "SilentlyContinue"}, - ) - out, err = buildtest.RunBuildReturningOutput(t, build) - assert.NoError(t, err) - assert.NotContains(t, out, "Created fresh repository") - assert.Regexp(t, "Checking out [a-f0-9]+ as", out) + out, err := buildtest.RunBuildReturningOutput(t, build) + assert.NoError(t, err) + assert.Contains(t, out, "Created fresh repository") + + out, err = buildtest.RunBuildReturningOutput(t, build) + assert.NoError(t, err) + checkFreshRepoMessage(t, out, "Created fresh repository") + assert.Regexp(t, "Checking out [a-f0-9]+ as", out) + + build.Variables = append( + build.Variables, + common.JobVariable{Key: "ErrorActionPreference", Value: "Continue"}, + ) + out, err = buildtest.RunBuildReturningOutput(t, build) + assert.NoError(t, err) + checkFreshRepoMessage(t, out, "Created fresh repository") + assert.Regexp(t, "Checking out [a-f0-9]+ as", out) + + build.Variables = append( + build.Variables, + common.JobVariable{Key: "ErrorActionPreference", Value: "SilentlyContinue"}, + ) + out, err = buildtest.RunBuildReturningOutput(t, build) + assert.NoError(t, err) + checkFreshRepoMessage(t, out, "Created fresh repository") + assert.Regexp(t, "Checking out [a-f0-9]+ as", out) + }) + } }) } } diff --git a/shells/abstract.go b/shells/abstract.go index 26afb9f651..c077b75f8b 100644 --- a/shells/abstract.go +++ b/shells/abstract.go @@ -19,22 +19,22 @@ import ( url_helpers "gitlab.com/gitlab-org/gitlab-runner/helpers/url" ) -// When umask is disabled for the Kubernetes executor, -// a hidden file, .giltab-build-uid-gid, is created in the `builds_dir` directory to assist the helper container -// in retrieving the build image's configured `uid:gid`. -// This information is then applied to the working directories to prevent them from being writable by anyone. -const BuildUiGidFile = ".giltab-build-uid-gid" - -const StartupProbeFile = ".gitlab-startup-marker" - -const credHelperConfFile = "cred-helper.conf" +const ( + // When umask is disabled for the Kubernetes executor, + // a hidden file, .giltab-build-uid-gid, is created in the `builds_dir` directory to assist the helper container + // in retrieving the build image's configured `uid:gid`. + // This information is then applied to the working directories to prevent them from being writable by anyone. + BuildUiGidFile = ".giltab-build-uid-gid" + StartupProbeFile = ".gitlab-startup-marker" + + gitlabEnvFileName = "gitlab_runner_env" + credHelperConfFile = "cred-helper.conf" + gitlabCacheEnvFileName = "gitlab_runner_cache_env" + gitTemplateDir = "git-template" +) var errUnknownGitStrategy = errors.New("unknown GIT_STRATEGY") -const gitlabEnvFileName = "gitlab_runner_env" - -const gitlabCacheEnvFileName = "gitlab_runner_cache_env" - type stringQuoter func(string) string func singleQuote(s string) string { @@ -583,7 +583,7 @@ func (b *AbstractShell) writeRefspecFetchCmd(w ShellWriter, info common.ShellScr } // initializing - templateDir := w.MkTmpDir("git-template") + templateDir := w.MkTmpDir(gitTemplateDir) templateFile := w.Join(templateDir, "config") objectFormat := build.GetRepositoryObjectFormat() @@ -1353,13 +1353,27 @@ func (b *AbstractShell) writeCleanupScript(_ context.Context, w ShellWriter, inf if err := b.writeCleanupBuildDirectoryScript(w, info); err != nil { return err } - - w.RmFile(filepath.Join(info.Build.FullProjectDir(), ".git", "config")) } + b.cleanGitConfig(w, info) + return nil } +func (b *AbstractShell) cleanGitConfig(sw ShellWriter, info common.ShellScriptInfo) { + build := info.Build + + if c := build.Runner.CleanGitConfig; c != nil && !*c { + // skip, if explicitly set to false + return + } + + sw.RmFile(sw.TmpFile(sw.Join(gitTemplateDir, "config"))) + sw.RmDir(sw.TmpFile(sw.Join(gitTemplateDir, "hooks"))) + sw.RmFile(sw.Join(build.FullProjectDir(), ".git", "config")) + sw.RmDir(sw.Join(build.FullProjectDir(), ".git", "hooks")) +} + func (b *AbstractShell) writeCleanupBuildDirectoryScript(w ShellWriter, info common.ShellScriptInfo) error { switch info.Build.GetGitStrategy() { case common.GitClone, common.GitEmpty: diff --git a/shells/abstract_test.go b/shells/abstract_test.go index 8e8621e92b..dd103915af 100644 --- a/shells/abstract_test.go +++ b/shells/abstract_test.go @@ -2273,7 +2273,7 @@ func TestSkipBuildStage(t *testing.T) { } } -func TestAbstractShell_writeCleanupFileVariablesScript(t *testing.T) { +func TestAbstractShell_writeCleanupScript(t *testing.T) { testVar1 := "VAR_1" testVar2 := "VAR_2" testVar3 := "VAR_3" @@ -2282,38 +2282,80 @@ func TestAbstractShell_writeCleanupFileVariablesScript(t *testing.T) { testPath1 := "path/VAR_1_file" testPath3 := "path/VAR_3_file" - info := common.ShellScriptInfo{ - Build: &common.Build{ - JobResponse: common.JobResponse{ - Variables: common.JobVariables{ - {Key: testVar1, Value: "test", File: true}, - {Key: testVar2, Value: "test", File: false}, - {Key: testVar3, Value: "test", File: true}, - {Key: testVar4, Value: "test", File: false}, - }, - }, - Runner: &common.RunnerConfig{}, + someTrue, someFalse := true, false + + tests := map[string]struct { + cleanGitConfig *bool + shouldCleanGitConfig bool + }{ + "no clean-git-config set": { + shouldCleanGitConfig: true, + }, + "clean-git-config explicitly enabled": { + cleanGitConfig: &someTrue, + shouldCleanGitConfig: true, + }, + "clean-git-config explicitly disabled": { + cleanGitConfig: &someFalse, + shouldCleanGitConfig: false, }, } - mockShellWriter := &MockShellWriter{} - defer mockShellWriter.AssertExpectations(t) + for name, test := range tests { + t.Run(name, func(t *testing.T) { + info := common.ShellScriptInfo{ + Build: &common.Build{ + JobResponse: common.JobResponse{ + Variables: common.JobVariables{ + {Key: testVar1, Value: "test", File: true}, + {Key: testVar2, Value: "test", File: false}, + {Key: testVar3, Value: "test", File: true}, + {Key: testVar4, Value: "test", File: false}, + }, + }, + Runner: &common.RunnerConfig{ + RunnerSettings: common.RunnerSettings{ + CleanGitConfig: test.cleanGitConfig, + }, + }, + }, + } + + mockShellWriter := NewMockShellWriter(t) - mockShellWriter.On("TmpFile", "masking.db").Return("masking.db").Once() - mockShellWriter.On("RmFile", "masking.db").Once() + mockShellWriter.On("TmpFile", "masking.db").Return("masking.db").Once() + mockShellWriter.On("RmFile", "masking.db").Once() - mockShellWriter.On("TmpFile", testVar1).Return(testPath1).Once() - mockShellWriter.On("RmFile", testPath1).Once() - mockShellWriter.On("TmpFile", testVar3).Return(testPath3).Once() - mockShellWriter.On("RmFile", testPath3).Once() + mockShellWriter.On("TmpFile", testVar1).Return(testPath1).Once() + mockShellWriter.On("RmFile", testPath1).Once() + mockShellWriter.On("TmpFile", testVar3).Return(testPath3).Once() + mockShellWriter.On("RmFile", testPath3).Once() - mockShellWriter.On("TmpFile", "gitlab_runner_env").Return("temp_env").Once() - mockShellWriter.On("RmFile", "temp_env").Once() + mockShellWriter.On("TmpFile", "gitlab_runner_env").Return("temp_env").Once() + mockShellWriter.On("RmFile", "temp_env").Once() - shell := new(AbstractShell) + if test.shouldCleanGitConfig { + mockShellWriter.On("Join", "git-template", "config").Return("someTemplateBasePath").Once() + mockShellWriter.On("TmpFile", "someTemplateBasePath").Return("someTemplateTmpPath").Once() + mockShellWriter.On("RmFile", "someTemplateTmpPath").Once() - err := shell.writeCleanupScript(context.Background(), mockShellWriter, info) - assert.NoError(t, err) + mockShellWriter.On("Join", "git-template", "hooks").Return("someTemplateHooksBasePath").Once() + mockShellWriter.On("TmpFile", "someTemplateHooksBasePath").Return("someTemplateHooksTmpPath").Once() + mockShellWriter.On("RmDir", "someTemplateHooksTmpPath").Once() + + mockShellWriter.On("Join", "", ".git", "config").Return("someGitConfigPath").Once() + mockShellWriter.On("RmFile", "someGitConfigPath").Once() + + mockShellWriter.On("Join", "", ".git", "hooks").Return("someGitHooksPath").Once() + mockShellWriter.On("RmDir", "someGitHooksPath").Once() + } + + shell := new(AbstractShell) + + err := shell.writeCleanupScript(context.Background(), mockShellWriter, info) + assert.NoError(t, err) + }) + } } func testGenerateArtifactsMetadataData() (common.ShellScriptInfo, []interface{}) { diff --git a/shells/powershell_test.go b/shells/powershell_test.go index e9e53bd346..c0034f97f4 100644 --- a/shells/powershell_test.go +++ b/shells/powershell_test.go @@ -464,27 +464,17 @@ func TestPowershellPathResolveOperations(t *testing.T) { } func TestPowershell_GenerateScript(t *testing.T) { - shellInfo := common.ShellScriptInfo{ - Shell: "pwsh", - Type: common.NormalShell, - RunnerCommand: "/usr/bin/gitlab-runner-helper", - Build: &common.Build{ - Runner: &common.RunnerConfig{RunnerSettings: common.RunnerSettings{ProxyExec: func() *bool { v := false; return &v }()}}, - }, - } - shellInfo.Build.Runner.Executor = "kubernetes" - shellInfo.Build.Hostname = "Test Hostname" - + pwshShell := common.GetShell("pwsh") eol := "\n" - switch v := common.GetShell("pwsh").(type) { + switch v := pwshShell.(type) { case *PowerShell: eol = v.EOL case *ProxyExecShell: eol = v.Shell.(*PowerShell).EOL } - shebang := "" - rmGitLabEnvScript := `$CurrentDirectory = (Resolve-Path ./).Path` + eol + + rmGitLabEnvScript := `` + + `$CurrentDirectory = (Resolve-Path ./).Path` + eol + `if( (Get-Command -Name Remove-Item2 -Module NTFSSecurity -ErrorAction SilentlyContinue) -and (Test-Path "$CurrentDirectory/.tmp/gitlab_runner_env" -PathType Leaf) ) {` + eol + ` Remove-Item2 -Force "$CurrentDirectory/.tmp/gitlab_runner_env"` + eol + `} elseif(Test-Path "$CurrentDirectory/.tmp/gitlab_runner_env") {` + eol + @@ -495,53 +485,129 @@ func TestPowershell_GenerateScript(t *testing.T) { ` Remove-Item2 -Force "$CurrentDirectory/.tmp/masking.db"` + eol + `} elseif(Test-Path "$CurrentDirectory/.tmp/masking.db") {` + eol + ` Remove-Item -Force "$CurrentDirectory/.tmp/masking.db"` + eol + - `}` + eol + eol + eol + "}" + eol + eol + `}` + cleanGitConfigScript := `` + + `$CurrentDirectory = (Resolve-Path ./).Path` + eol + + `if( (Get-Command -Name Remove-Item2 -Module NTFSSecurity -ErrorAction SilentlyContinue) -and (Test-Path "$CurrentDirectory/.tmp/git-template/config" -PathType Leaf) ) {` + eol + + ` Remove-Item2 -Force "$CurrentDirectory/.tmp/git-template/config"` + eol + + `} elseif(Test-Path "$CurrentDirectory/.tmp/git-template/config") {` + eol + + ` Remove-Item -Force "$CurrentDirectory/.tmp/git-template/config"` + eol + + `}` + eol + + `` + eol + + `$CurrentDirectory = (Resolve-Path ./).Path` + eol + + `if( (Get-Command -Name Remove-Item2 -Module NTFSSecurity -ErrorAction SilentlyContinue) -and (Test-Path "$CurrentDirectory/.tmp/git-template/hooks" -PathType Container) ) {` + eol + + ` Remove-Item2 -Force -Recurse "$CurrentDirectory/.tmp/git-template/hooks"` + eol + + `} elseif(Test-Path "$CurrentDirectory/.tmp/git-template/hooks") {` + eol + + ` Remove-Item -Force -Recurse "$CurrentDirectory/.tmp/git-template/hooks"` + eol + + `}` + eol + + `` + eol + + `if( (Get-Command -Name Remove-Item2 -Module NTFSSecurity -ErrorAction SilentlyContinue) -and (Test-Path ".git/config" -PathType Leaf) ) {` + eol + + ` Remove-Item2 -Force ".git/config"` + eol + + `} elseif(Test-Path ".git/config") {` + eol + + ` Remove-Item -Force ".git/config"` + eol + + `}` + eol + + `` + eol + + `if( (Get-Command -Name Remove-Item2 -Module NTFSSecurity -ErrorAction SilentlyContinue) -and (Test-Path ".git/hooks" -PathType Container) ) {` + eol + + ` Remove-Item2 -Force -Recurse ".git/hooks"` + eol + + `} elseif(Test-Path ".git/hooks") {` + eol + + ` Remove-Item -Force -Recurse ".git/hooks"` + eol + + `}` if eol == "\n" { shebang = "#!/usr/bin/env pwsh\n" } else { rmGitLabEnvScript = strings.ReplaceAll(rmGitLabEnvScript, `/`, `\`) + cleanGitConfigScript = strings.ReplaceAll(cleanGitConfigScript, `/`, `\`) } testCases := map[string]struct { stage common.BuildStage - info common.ShellScriptInfo + updateShellInfo func(*common.ShellScriptInfo) expectedFailure bool - expectedScript string + expectedScript func(common.ShellScriptInfo) string }{ "prepare script": { stage: common.BuildStagePrepare, - info: shellInfo, expectedFailure: false, - expectedScript: shebang + "& {" + - eol + eol + - fmt.Sprintf(pwshJSONInitializationScript, shellInfo.Shell) + - eol + eol + - `$ErrorActionPreference = "Stop"` + eol + - `echo "Running on $([Environment]::MachineName) via "Test Hostname"..."` + eol + rmGitLabEnvScript, - }, - "cleanup variables": { + expectedScript: func(shellInfo common.ShellScriptInfo) string { + return shebang + "& {" + + eol + eol + + fmt.Sprintf(pwshJSONInitializationScript, shellInfo.Shell) + + eol + eol + + `$ErrorActionPreference = "Stop"` + eol + + `echo "Running on $([Environment]::MachineName) via "Test Hostname"..."` + + eol + + rmGitLabEnvScript + + eol + eol + eol + + "}" + eol + eol + }, + }, + "cleanup variables but not git config": { + stage: common.BuildStageCleanup, + updateShellInfo: func(shellInfo *common.ShellScriptInfo) { + shellInfo.Build.Runner.RunnerSettings.CleanGitConfig = &[]bool{false}[0] + }, + expectedFailure: false, + expectedScript: func(shellInfo common.ShellScriptInfo) string { + return shebang + "& {" + + eol + eol + + fmt.Sprintf(pwshJSONInitializationScript, shellInfo.Shell) + + eol + eol + + `$ErrorActionPreference = "Stop"` + eol + + rmGitLabEnvScript + + eol + eol + eol + + "}" + eol + eol + }, + }, + "cleanup variables and git config": { stage: common.BuildStageCleanup, - info: shellInfo, expectedFailure: false, - expectedScript: shebang + "& {" + - eol + eol + - fmt.Sprintf(pwshJSONInitializationScript, shellInfo.Shell) + - eol + eol + - `$ErrorActionPreference = "Stop"` + eol + rmGitLabEnvScript, + expectedScript: func(shellInfo common.ShellScriptInfo) string { + return shebang + "& {" + + eol + eol + + fmt.Sprintf(pwshJSONInitializationScript, shellInfo.Shell) + + eol + eol + + `$ErrorActionPreference = "Stop"` + eol + + rmGitLabEnvScript + + eol + eol + + cleanGitConfigScript + + eol + eol + eol + + "}" + eol + eol + }, }, "no script": { stage: "no_script", - info: shellInfo, expectedFailure: true, - expectedScript: "", }, } for tn, tc := range testCases { t.Run(tn, func(t *testing.T) { - script, err := common.GetShell("pwsh").GenerateScript(context.Background(), tc.stage, tc.info) - assert.Equal(t, tc.expectedScript, script) + shellInfo := common.ShellScriptInfo{ + Shell: "pwsh", + Type: common.NormalShell, + RunnerCommand: "/usr/bin/gitlab-runner-helper", + Build: &common.Build{ + Runner: &common.RunnerConfig{ + RunnerSettings: common.RunnerSettings{ + Executor: "kubernetes", + }, + }, + Hostname: "Test Hostname", + }, + } + if u := tc.updateShellInfo; u != nil { + u(&shellInfo) + } + + var expectedScript string + if s := tc.expectedScript; s != nil { + expectedScript = s(shellInfo) + } + + script, err := pwshShell.GenerateScript(context.Background(), tc.stage, shellInfo) + assert.Equal(t, expectedScript, script) + if tc.expectedFailure { assert.Error(t, err) } -- GitLab From d29e9e98abb3546aeaeaf8ebded223283140326c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20H=C3=B6rl?= Date: Fri, 14 Feb 2025 15:37:49 +0100 Subject: [PATCH 3/8] Do more cleanups We keep cleaning up some files (e.g. lock files) in the git repos (main repo & submodules) on build start, but in addition we do this also on build end. In a similar fashion, if git config cleanup is enabled, we also clean out git configs at the beginning and end of a build. --- shells/abstract.go | 55 +++++++++++++++++++++++++-------------- shells/abstract_test.go | 44 ++++++++++++++++++------------- shells/powershell_test.go | 47 +++++++++++++++++++++++++++++---- 3 files changed, 103 insertions(+), 43 deletions(-) diff --git a/shells/abstract.go b/shells/abstract.go index c077b75f8b..853ee82da3 100644 --- a/shells/abstract.go +++ b/shells/abstract.go @@ -28,8 +28,9 @@ const ( StartupProbeFile = ".gitlab-startup-marker" gitlabEnvFileName = "gitlab_runner_env" - credHelperConfFile = "cred-helper.conf" gitlabCacheEnvFileName = "gitlab_runner_cache_env" + credHelperConfFile = "cred-helper.conf" + gitDir = ".git" gitTemplateDir = "git-template" ) @@ -607,7 +608,7 @@ func (b *AbstractShell) writeRefspecFetchCmd(w ShellWriter, info common.ShellScr b.writeGitSSLConfig(w, build, []string{"-f", templateFile}) } - b.writeGitCleanup(w, build.GetSubmoduleStrategy(), projectDir) + b.writeGitCleanup(w, build) if objectFormat != common.DefaultObjectFormat { w.Command("git", "init", projectDir, "--template", templateDir, "--object-format", objectFormat) @@ -707,8 +708,10 @@ func (b *AbstractShell) configureGitCredHelper(w ShellWriter, info common.ShellS return nil } -func (b *AbstractShell) writeGitCleanup(w ShellWriter, submoduleStrategy common.SubmoduleStrategy, projectDir string) { - const gitDir = ".git" +func (b *AbstractShell) writeGitCleanup(w ShellWriter, build *common.Build) { + projectDir := build.FullProjectDir() + submoduleStrategy := build.GetSubmoduleStrategy() + cleanForSubmodules := submoduleStrategy == common.SubmoduleNormal || submoduleStrategy == common.SubmoduleRecursive // Remove .git/{index,shallow,HEAD,config}.lock files from .git, which can fail the fetch command // The file can be left if previous build was terminated during git operation. @@ -724,12 +727,38 @@ func (b *AbstractShell) writeGitCleanup(w ShellWriter, submoduleStrategy common. for _, f := range files { w.RmFile(path.Join(projectDir, gitDir, f)) - if submoduleStrategy == common.SubmoduleNormal || submoduleStrategy == common.SubmoduleRecursive { + if cleanForSubmodules { w.RmFilesRecursive(path.Join(projectDir, gitDir, "modules"), path.Base(f)) } } w.RmFilesRecursive(path.Join(projectDir, gitDir, "refs"), "*.lock") + + b.writeGitCleanupAllConfigs(w, build, cleanForSubmodules) +} + +// writeGitCleanupAllConfigs removes all git configs which are potentially open to malicious code injection: +// - the main git config & hooks +// - the template git config & hooks +// - any submodule's git config & hooks +func (b *AbstractShell) writeGitCleanupAllConfigs(sw ShellWriter, build *common.Build, cleanForSubmodules bool) { + if c := build.Runner.CleanGitConfig; c != nil && !*c { + // skip, if explicitly set to false + return + } + + projectDir := build.FullProjectDir() + + // clean out configs in the main git dir and in the template dir + for _, dir := range []string{sw.TmpFile(gitTemplateDir), sw.Join(projectDir, gitDir)} { + sw.RmFile(sw.Join(dir, "config")) + sw.RmDir(sw.Join(dir, "hooks")) + } + + // clean out configs in the modules' git dirs + if cleanForSubmodules { + // TODO submodules + } } func (b *AbstractShell) writeCheckoutCmd(w ShellWriter, build *common.Build) { @@ -1355,25 +1384,11 @@ func (b *AbstractShell) writeCleanupScript(_ context.Context, w ShellWriter, inf } } - b.cleanGitConfig(w, info) + b.writeGitCleanup(w, info.Build) return nil } -func (b *AbstractShell) cleanGitConfig(sw ShellWriter, info common.ShellScriptInfo) { - build := info.Build - - if c := build.Runner.CleanGitConfig; c != nil && !*c { - // skip, if explicitly set to false - return - } - - sw.RmFile(sw.TmpFile(sw.Join(gitTemplateDir, "config"))) - sw.RmDir(sw.TmpFile(sw.Join(gitTemplateDir, "hooks"))) - sw.RmFile(sw.Join(build.FullProjectDir(), ".git", "config")) - sw.RmDir(sw.Join(build.FullProjectDir(), ".git", "hooks")) -} - func (b *AbstractShell) writeCleanupBuildDirectoryScript(w ShellWriter, info common.ShellScriptInfo) error { switch info.Build.GetGitStrategy() { case common.GitClone, common.GitEmpty: diff --git a/shells/abstract_test.go b/shells/abstract_test.go index dd103915af..1c1a395d98 100644 --- a/shells/abstract_test.go +++ b/shells/abstract_test.go @@ -845,7 +845,8 @@ func TestGitFetchFlags(t *testing.T) { mockWriter.EXPECT().Command("git", "config", "-f", mock.Anything, "credential.interactive", "never").Once() mockWriter.EXPECT().Command("git", "config", "-f", mock.Anything, "transfer.bundleURI", "true").Once() - mockWriter.EXPECT().RmFilesRecursive(path.Join(dummyProjectDir, ".git", "refs"), "*.lock").Once() + expectFileCleanup(mockWriter, path.Join(dummyProjectDir, ".git")) + expectGitConfigCleanup(mockWriter, dummyProjectDir) if expectedObjectFormat != "sha1" { mockWriter.EXPECT().Command("git", "init", dummyProjectDir, "--template", mock.Anything, "--object-format", expectedObjectFormat).Once() @@ -890,6 +891,22 @@ func TestGitFetchFlags(t *testing.T) { } } +func expectGitConfigCleanup(sw *MockShellWriter, buildDir string) { + sw.EXPECT().TmpFile("git-template").Return("someGitTemplateDir").Once() + sw.EXPECT().Join(buildDir, ".git").Return("someGitDir").Once() + + sw.EXPECT().Join("someGitTemplateDir", "config").Return("someGitTemplateDir/config").Once() + sw.EXPECT().RmFile("someGitTemplateDir/config") + sw.EXPECT().Join("someGitTemplateDir", "hooks").Return("someGitTemplateDir/hooks").Once() + sw.EXPECT().RmDir("someGitTemplateDir/hooks") + sw.EXPECT().Join("someGitDir", "config").Return("someGitDir/config").Once() + sw.EXPECT().RmFile("someGitDir/config") + sw.EXPECT().Join("someGitDir", "hooks").Return("someGitDir/hooks").Once() + sw.EXPECT().RmDir("someGitDir/hooks") + + // TODO submodules +} + func TestAbstractShell_writeSubmoduleUpdateCmd(t *testing.T) { const ( exampleBaseURL = "http://test.remote" @@ -2334,20 +2351,10 @@ func TestAbstractShell_writeCleanupScript(t *testing.T) { mockShellWriter.On("TmpFile", "gitlab_runner_env").Return("temp_env").Once() mockShellWriter.On("RmFile", "temp_env").Once() - if test.shouldCleanGitConfig { - mockShellWriter.On("Join", "git-template", "config").Return("someTemplateBasePath").Once() - mockShellWriter.On("TmpFile", "someTemplateBasePath").Return("someTemplateTmpPath").Once() - mockShellWriter.On("RmFile", "someTemplateTmpPath").Once() - - mockShellWriter.On("Join", "git-template", "hooks").Return("someTemplateHooksBasePath").Once() - mockShellWriter.On("TmpFile", "someTemplateHooksBasePath").Return("someTemplateHooksTmpPath").Once() - mockShellWriter.On("RmDir", "someTemplateHooksTmpPath").Once() + expectFileCleanup(mockShellWriter, ".git") - mockShellWriter.On("Join", "", ".git", "config").Return("someGitConfigPath").Once() - mockShellWriter.On("RmFile", "someGitConfigPath").Once() - - mockShellWriter.On("Join", "", ".git", "hooks").Return("someGitHooksPath").Once() - mockShellWriter.On("RmDir", "someGitHooksPath").Once() + if test.shouldCleanGitConfig { + expectGitConfigCleanup(mockShellWriter, "") } shell := new(AbstractShell) @@ -2681,7 +2688,8 @@ func TestAbstractShell_writeGetSourcesScript_scriptHooks(t *testing.T) { m.EXPECT().Join("git-template-dir", "config").Return("git-template-dir-config").Once() m.EXPECT().Command("git", "config", "-f", "git-template-dir-config", mock.Anything, mock.Anything) - expectFileCleanup(m) + expectFileCleanup(m, "build-dir/.git") + expectGitConfigCleanup(m, "build-dir") m.EXPECT().Command("git", "init", "build-dir", "--template", "git-template-dir").Once() m.EXPECT().Cd("build-dir").Once() @@ -2724,11 +2732,11 @@ func TestAbstractShell_writeGetSourcesScript_scriptHooks(t *testing.T) { } } -func expectFileCleanup(shellWriter *MockShellWriter) { +func expectFileCleanup(shellWriter *MockShellWriter, dir string) { for _, f := range []string{"index.lock", "shallow.lock", "HEAD.lock", "hooks/post-checkout", "config.lock"} { - shellWriter.EXPECT().RmFile("build-dir/.git/" + f).Once() + shellWriter.EXPECT().RmFile(dir + "/" + f).Once() } - shellWriter.EXPECT().RmFilesRecursive("build-dir/.git/refs", "*.lock").Once() + shellWriter.EXPECT().RmFilesRecursive(dir+"/refs", "*.lock").Once() } func expectGitCredHelperSetup(shellWriter *MockShellWriter, remoteURL string) { diff --git a/shells/powershell_test.go b/shells/powershell_test.go index c0034f97f4..912221efa5 100644 --- a/shells/powershell_test.go +++ b/shells/powershell_test.go @@ -486,7 +486,41 @@ func TestPowershell_GenerateScript(t *testing.T) { `} elseif(Test-Path "$CurrentDirectory/.tmp/masking.db") {` + eol + ` Remove-Item -Force "$CurrentDirectory/.tmp/masking.db"` + eol + `}` - cleanGitConfigScript := `` + + cleanGitFiles := `` + + `if( (Get-Command -Name Remove-Item2 -Module NTFSSecurity -ErrorAction SilentlyContinue) -and (Test-Path ".git/index.lock" -PathType Leaf) ) {` + eol + + ` Remove-Item2 -Force ".git/index.lock"` + eol + + `} elseif(Test-Path ".git/index.lock") {` + eol + + ` Remove-Item -Force ".git/index.lock"` + eol + + `}` + eol + + `` + eol + + `if( (Get-Command -Name Remove-Item2 -Module NTFSSecurity -ErrorAction SilentlyContinue) -and (Test-Path ".git/shallow.lock" -PathType Leaf) ) {` + eol + + ` Remove-Item2 -Force ".git/shallow.lock"` + eol + + `} elseif(Test-Path ".git/shallow.lock") {` + eol + + ` Remove-Item -Force ".git/shallow.lock"` + eol + + `}` + eol + + `` + eol + + `if( (Get-Command -Name Remove-Item2 -Module NTFSSecurity -ErrorAction SilentlyContinue) -and (Test-Path ".git/HEAD.lock" -PathType Leaf) ) {` + eol + + ` Remove-Item2 -Force ".git/HEAD.lock"` + eol + + `} elseif(Test-Path ".git/HEAD.lock") {` + eol + + ` Remove-Item -Force ".git/HEAD.lock"` + eol + + `}` + eol + + `` + eol + + `if( (Get-Command -Name Remove-Item2 -Module NTFSSecurity -ErrorAction SilentlyContinue) -and (Test-Path ".git/hooks/post-checkout" -PathType Leaf) ) {` + eol + + ` Remove-Item2 -Force ".git/hooks/post-checkout"` + eol + + `} elseif(Test-Path ".git/hooks/post-checkout") {` + eol + + ` Remove-Item -Force ".git/hooks/post-checkout"` + eol + + `}` + eol + + `` + eol + + `if( (Get-Command -Name Remove-Item2 -Module NTFSSecurity -ErrorAction SilentlyContinue) -and (Test-Path ".git/config.lock" -PathType Leaf) ) {` + eol + + ` Remove-Item2 -Force ".git/config.lock"` + eol + + `} elseif(Test-Path ".git/config.lock") {` + eol + + ` Remove-Item -Force ".git/config.lock"` + eol + + `}` + eol + + `` + eol + + `if(Test-Path ".git/refs" -PathType Container) {` + eol + + ` Get-ChildItem -Path ".git/refs" -Filter "*.lock" -Recurse | ForEach-Object { Remove-Item -Force $_.FullName }` + eol + + `}` + eol + cleanGitConfigs := `` + `$CurrentDirectory = (Resolve-Path ./).Path` + eol + `if( (Get-Command -Name Remove-Item2 -Module NTFSSecurity -ErrorAction SilentlyContinue) -and (Test-Path "$CurrentDirectory/.tmp/git-template/config" -PathType Leaf) ) {` + eol + ` Remove-Item2 -Force "$CurrentDirectory/.tmp/git-template/config"` + eol + @@ -494,7 +528,6 @@ func TestPowershell_GenerateScript(t *testing.T) { ` Remove-Item -Force "$CurrentDirectory/.tmp/git-template/config"` + eol + `}` + eol + `` + eol + - `$CurrentDirectory = (Resolve-Path ./).Path` + eol + `if( (Get-Command -Name Remove-Item2 -Module NTFSSecurity -ErrorAction SilentlyContinue) -and (Test-Path "$CurrentDirectory/.tmp/git-template/hooks" -PathType Container) ) {` + eol + ` Remove-Item2 -Force -Recurse "$CurrentDirectory/.tmp/git-template/hooks"` + eol + `} elseif(Test-Path "$CurrentDirectory/.tmp/git-template/hooks") {` + eol + @@ -517,7 +550,8 @@ func TestPowershell_GenerateScript(t *testing.T) { shebang = "#!/usr/bin/env pwsh\n" } else { rmGitLabEnvScript = strings.ReplaceAll(rmGitLabEnvScript, `/`, `\`) - cleanGitConfigScript = strings.ReplaceAll(cleanGitConfigScript, `/`, `\`) + cleanGitFiles = strings.ReplaceAll(cleanGitFiles, `/`, `\`) + cleanGitConfigs = strings.ReplaceAll(cleanGitConfigs, `/`, `\`) } testCases := map[string]struct { @@ -555,7 +589,9 @@ func TestPowershell_GenerateScript(t *testing.T) { eol + eol + `$ErrorActionPreference = "Stop"` + eol + rmGitLabEnvScript + - eol + eol + eol + + eol + eol + + cleanGitFiles + + eol + "}" + eol + eol }, }, @@ -570,7 +606,8 @@ func TestPowershell_GenerateScript(t *testing.T) { `$ErrorActionPreference = "Stop"` + eol + rmGitLabEnvScript + eol + eol + - cleanGitConfigScript + + cleanGitFiles + + cleanGitConfigs + eol + eol + eol + "}" + eol + eol }, -- GitLab From 4a8a998ef1601a706c3407ef591478017ade5112 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20H=C3=B6rl?= Date: Fri, 14 Feb 2025 17:08:18 +0100 Subject: [PATCH 4/8] Clear git config and hooks in submodules --- executors/shell/shell_integration_test.go | 51 ++++++++ shells/abstract.go | 4 +- shells/abstract_test.go | 146 ++++++++++++++++++++-- shells/bash.go | 7 ++ shells/mock_ShellWriter.go | 34 +++++ shells/powershell.go | 12 ++ shells/shell_writer.go | 5 + shells/shell_writer_integration_test.go | 43 +++++++ 8 files changed, 291 insertions(+), 11 deletions(-) diff --git a/executors/shell/shell_integration_test.go b/executors/shell/shell_integration_test.go index 5aafd17e30..b4a766bb3e 100644 --- a/executors/shell/shell_integration_test.go +++ b/executors/shell/shell_integration_test.go @@ -2398,6 +2398,57 @@ func TestSubmoduleAutoBump(t *testing.T) { } } +func TestBuildWithCleanGitConfig(t *testing.T) { + // only update a couple of submodules, to make the test a bit faster + submodules := []string{"private-repo-ssh", "public-repo-relative"} + require.GreaterOrEqual(t, len(submodules), 1, "must manage/update at least one submodule") + + assertFilesAreCleaned := func(t *testing.T, buildDir string) { + dirs := []string{ + filepath.Join(buildDir, ".git"), + filepath.Join(buildDir, "..", "mixed-submodules-test.tmp", "git-template"), + } + for _, m := range submodules { + dirs = append(dirs, filepath.Join(buildDir, ".git", "modules", m)) + } + for _, d := range dirs { + assert.DirExists(t, d) + assert.NoFileExists(t, filepath.Join(d, "config")) + assert.NoDirExists(t, filepath.Join(d, "hooks")) + } + } + + shellstest.OnEachShell(t, func(t *testing.T, shell string) { + t.Parallel() + + jobResponse, err := common.GetSuccessfulBuild() + assert.NoError(t, err) + + jobResponse.Variables = append(jobResponse.Variables, + common.JobVariable{Key: "GIT_SUBMODULE_PATHS", Value: strings.Join(submodules, " ")}, + common.JobVariable{Key: "GIT_SUBMODULE_STRATEGY", Value: string(common.SubmoduleRecursive)}, + common.JobVariable{Key: "GIT_SUBMODULE_FORCE_HTTPS", Value: "1"}, + common.JobVariable{Key: "CI_SERVER_HOST", Value: "gitlab.com"}, + ) + jobResponse.GitInfo.RepoURL = repoURLWithSubmodules + jobResponse.GitInfo.Sha = repoShaWithSubmodules + injectJobToken(t, &jobResponse) + + build := newBuild(t, jobResponse, shell) + build.Runner.RunnerCredentials.URL = "https://gitlab.com/" + build.Runner.RunnerSettings.CleanGitConfig = &[]bool{true}[0] + + _, err = buildtest.RunBuildReturningOutput(t, build) + assert.NoError(t, err) + assertFilesAreCleaned(t, build.BuildDir) + + // run a second build to ensure submodules still work, even though we blew away their git config. + _, err = buildtest.RunBuildReturningOutput(t, build) + assert.NoError(t, err) + assertFilesAreCleaned(t, build.BuildDir) + }) +} + // injectJobToken injects a job token into an existing jobResponse by // - setting the jobResponse's token // - updating the jobResponse's gitInfo with an URL with the token diff --git a/shells/abstract.go b/shells/abstract.go index 853ee82da3..c6702a4635 100644 --- a/shells/abstract.go +++ b/shells/abstract.go @@ -757,7 +757,9 @@ func (b *AbstractShell) writeGitCleanupAllConfigs(sw ShellWriter, build *common. // clean out configs in the modules' git dirs if cleanForSubmodules { - // TODO submodules + modulesDir := sw.Join(projectDir, gitDir, "modules") + sw.RmFilesRecursive(modulesDir, "config") + sw.RmDirsRecursive(modulesDir, "hooks") } } diff --git a/shells/abstract_test.go b/shells/abstract_test.go index 1c1a395d98..24eeccb414 100644 --- a/shells/abstract_test.go +++ b/shells/abstract_test.go @@ -7,6 +7,7 @@ import ( "fmt" "path" "path/filepath" + "regexp" "slices" "strings" "testing" @@ -845,8 +846,8 @@ func TestGitFetchFlags(t *testing.T) { mockWriter.EXPECT().Command("git", "config", "-f", mock.Anything, "credential.interactive", "never").Once() mockWriter.EXPECT().Command("git", "config", "-f", mock.Anything, "transfer.bundleURI", "true").Once() - expectFileCleanup(mockWriter, path.Join(dummyProjectDir, ".git")) - expectGitConfigCleanup(mockWriter, dummyProjectDir) + expectFileCleanup(mockWriter, path.Join(dummyProjectDir, ".git"), false) + expectGitConfigCleanup(mockWriter, dummyProjectDir, false) if expectedObjectFormat != "sha1" { mockWriter.EXPECT().Command("git", "init", dummyProjectDir, "--template", mock.Anything, "--object-format", expectedObjectFormat).Once() @@ -891,7 +892,7 @@ func TestGitFetchFlags(t *testing.T) { } } -func expectGitConfigCleanup(sw *MockShellWriter, buildDir string) { +func expectGitConfigCleanup(sw *MockShellWriter, buildDir string, withSubmodules bool) { sw.EXPECT().TmpFile("git-template").Return("someGitTemplateDir").Once() sw.EXPECT().Join(buildDir, ".git").Return("someGitDir").Once() @@ -904,7 +905,11 @@ func expectGitConfigCleanup(sw *MockShellWriter, buildDir string) { sw.EXPECT().Join("someGitDir", "hooks").Return("someGitDir/hooks").Once() sw.EXPECT().RmDir("someGitDir/hooks") - // TODO submodules + if withSubmodules { + sw.EXPECT().Join(buildDir, ".git", "modules").Return("someModulesDir").Once() + sw.EXPECT().RmFilesRecursive("someModulesDir", "config") + sw.EXPECT().RmDirsRecursive("someModulesDir", "hooks") + } } func TestAbstractShell_writeSubmoduleUpdateCmd(t *testing.T) { @@ -2351,10 +2356,10 @@ func TestAbstractShell_writeCleanupScript(t *testing.T) { mockShellWriter.On("TmpFile", "gitlab_runner_env").Return("temp_env").Once() mockShellWriter.On("RmFile", "temp_env").Once() - expectFileCleanup(mockShellWriter, ".git") + expectFileCleanup(mockShellWriter, ".git", false) if test.shouldCleanGitConfig { - expectGitConfigCleanup(mockShellWriter, "") + expectGitConfigCleanup(mockShellWriter, "", false) } shell := new(AbstractShell) @@ -2688,8 +2693,8 @@ func TestAbstractShell_writeGetSourcesScript_scriptHooks(t *testing.T) { m.EXPECT().Join("git-template-dir", "config").Return("git-template-dir-config").Once() m.EXPECT().Command("git", "config", "-f", "git-template-dir-config", mock.Anything, mock.Anything) - expectFileCleanup(m, "build-dir/.git") - expectGitConfigCleanup(m, "build-dir") + expectFileCleanup(m, "build-dir/.git", false) + expectGitConfigCleanup(m, "build-dir", false) m.EXPECT().Command("git", "init", "build-dir", "--template", "git-template-dir").Once() m.EXPECT().Cd("build-dir").Once() @@ -2732,10 +2737,19 @@ func TestAbstractShell_writeGetSourcesScript_scriptHooks(t *testing.T) { } } -func expectFileCleanup(shellWriter *MockShellWriter, dir string) { - for _, f := range []string{"index.lock", "shallow.lock", "HEAD.lock", "hooks/post-checkout", "config.lock"} { +func expectFileCleanup(shellWriter *MockShellWriter, dir string, withSubmodules bool) { + files := []string{"index.lock", "shallow.lock", "HEAD.lock", "hooks/post-checkout", "config.lock"} + + for _, f := range files { shellWriter.EXPECT().RmFile(dir + "/" + f).Once() } + + if withSubmodules { + for _, f := range files { + shellWriter.EXPECT().RmFilesRecursive(dir+"/modules", filepath.Base(f)).Once() + } + } + shellWriter.EXPECT().RmFilesRecursive(dir+"/refs", "*.lock").Once() } @@ -2799,3 +2813,115 @@ func TestSanitizeCacheFallbackKey(t *testing.T) { }) } } + +func TestAbstractShell_writeGitCleanup(t *testing.T) { + submoduleStrategies := map[common.SubmoduleStrategy]bool{ + common.SubmoduleNone: false, + common.SubmoduleNormal: true, + common.SubmoduleRecursive: true, + common.SubmoduleInvalid: false, + } + cleanGitConfigs := map[string]struct { + configValue *bool + expectGitConfigsToBeCleaned bool + }{ + "": { + expectGitConfigsToBeCleaned: true, + }, + "enabled": { + configValue: &[]bool{true}[0], + expectGitConfigsToBeCleaned: true, + }, + "disabled": { + configValue: &[]bool{false}[0], + expectGitConfigsToBeCleaned: false, + }, + } + + for name, cleanGitConfig := range cleanGitConfigs { + t.Run("cleanGitConfig:"+name, func(t *testing.T) { + for submoduleStrategy, expectSubmoduleCleanupCalls := range submoduleStrategies { + t.Run("submoduleStrategy:"+string(submoduleStrategy), func(t *testing.T) { + shell := new(AbstractShell) + + info := common.ShellScriptInfo{ + Build: &common.Build{ + JobResponse: common.JobResponse{ + Variables: common.JobVariables{ + {Key: "GIT_SUBMODULE_STRATEGY", Value: string(submoduleStrategy)}, + }, + GitInfo: common.GitInfo{ + RepoURL: "https://repo-url/some/repo", + }, + }, + Runner: &common.RunnerConfig{ + RunnerSettings: common.RunnerSettings{ + CleanGitConfig: cleanGitConfig.configValue, + }, + }, + }, + } + + // ensure the cleanup is called at the beginning, from writeRefspecFetchCmd + t.Run("writeRefspecFetchCmd", func(t *testing.T) { + sw := NewMockShellWriter(t) + + sw.EXPECT().Noticef("Fetching changes...").Once() + sw.EXPECT().MkTmpDir("git-template").Return("someTmpDir").Once() + sw.EXPECT().Join("someTmpDir", "config").Return("someGitTemplateConfig").Once() + + sw.EXPECT().Command("git", "config", "-f", "someGitTemplateConfig", "init.defaultBranch", "none").Once() + sw.EXPECT().Command("git", "config", "-f", "someGitTemplateConfig", "fetch.recurseSubmodules", "false").Once() + sw.EXPECT().Command("git", "config", "-f", "someGitTemplateConfig", "credential.interactive", "never").Once() + sw.EXPECT().Command("git", "config", "-f", "someGitTemplateConfig", "transfer.bundleURI", "true").Once() + + expectFileCleanup(sw, ".git", expectSubmoduleCleanupCalls) + if cleanGitConfig.expectGitConfigsToBeCleaned { + expectGitConfigCleanup(sw, "", expectSubmoduleCleanupCalls) + } + + sw.EXPECT().Command("git", "init", "", "--template", "someTmpDir").Once() + sw.EXPECT().Cd("").Once() + + sw.EXPECT().IfCmd("git", "remote", "add", "origin", "https://repo-url/some/repo") + sw.EXPECT().Noticef("Created fresh repository.").Once() + sw.EXPECT().Else().Once() + sw.EXPECT().Command("git", "remote", "set-url", "origin", "https://repo-url/some/repo").Once() + sw.EXPECT().EndIf().Once() + + uaConfRE := regexp.MustCompile(`^http.userAgent=gitlab-runner development version [a-zA-Z0-9]+\/[a-zA-Z0-9]+$`) + uaConfMatcher := mock.MatchedBy(uaConfRE.MatchString) + + sw.EXPECT().IfFile(".git/shallow").Once() + sw.EXPECT().Command("git", "-c", uaConfMatcher, "fetch", "origin", "--no-recurse-submodules", "--prune", "--quiet", "--unshallow").Once() + sw.EXPECT().Else().Once() + sw.EXPECT().Command("git", "-c", uaConfMatcher, "fetch", "origin", "--no-recurse-submodules", "--prune", "--quiet").Once() + sw.EXPECT().EndIf().Once() + + err := shell.writeRefspecFetchCmd(sw, info) + assert.NoError(t, err) + }) + + // ensure the cleanup is also called at the end, from writeCleanupScript + t.Run("writeCleanupScript", func(t *testing.T) { + sw := NewMockShellWriter(t) + + sw.EXPECT().TmpFile("masking.db").Return("masking.db").Once() + sw.EXPECT().RmFile("masking.db").Once() + + sw.EXPECT().TmpFile("gitlab_runner_env").Return("someRunnerEnv").Once() + sw.EXPECT().RmFile("someRunnerEnv").Once() + + expectFileCleanup(sw, ".git", expectSubmoduleCleanupCalls) + if cleanGitConfig.expectGitConfigsToBeCleaned { + expectGitConfigCleanup(sw, "", expectSubmoduleCleanupCalls) + } + + err := shell.writeCleanupScript(context.TODO(), sw, info) + assert.NoError(t, err) + }) + }) + } + }) + } +} diff --git a/shells/bash.go b/shells/bash.go index cacedb5e78..afaa2af551 100644 --- a/shells/bash.go +++ b/shells/bash.go @@ -292,6 +292,13 @@ func (b *BashWriter) RmFilesRecursive(path string, name string) { b.EndIf() } +func (b *BashWriter) RmDirsRecursive(path string, name string) { + b.IfDirectory(path) + // `find -delete` is not portable; https://unix.stackexchange.com/a/194348 + b.Linef("find %q -name %q -type d -exec rm -rf -- {} +", path, name) + b.EndIf() +} + func (b *BashWriter) Absolute(dir string) string { if path.IsAbs(dir) || strings.HasPrefix(dir, "$PWD") { return dir diff --git a/shells/mock_ShellWriter.go b/shells/mock_ShellWriter.go index 4d88c9586f..9bffe4acba 100644 --- a/shells/mock_ShellWriter.go +++ b/shells/mock_ShellWriter.go @@ -1009,6 +1009,40 @@ func (_c *MockShellWriter_RmDir_Call) RunAndReturn(run func(string)) *MockShellW return _c } +// RmDirsRecursive provides a mock function with given fields: path, name +func (_m *MockShellWriter) RmDirsRecursive(path string, name string) { + _m.Called(path, name) +} + +// MockShellWriter_RmDirsRecursive_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'RmDirsRecursive' +type MockShellWriter_RmDirsRecursive_Call struct { + *mock.Call +} + +// RmDirsRecursive is a helper method to define mock.On call +// - path string +// - name string +func (_e *MockShellWriter_Expecter) RmDirsRecursive(path interface{}, name interface{}) *MockShellWriter_RmDirsRecursive_Call { + return &MockShellWriter_RmDirsRecursive_Call{Call: _e.mock.On("RmDirsRecursive", path, name)} +} + +func (_c *MockShellWriter_RmDirsRecursive_Call) Run(run func(path string, name string)) *MockShellWriter_RmDirsRecursive_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(string), args[1].(string)) + }) + return _c +} + +func (_c *MockShellWriter_RmDirsRecursive_Call) Return() *MockShellWriter_RmDirsRecursive_Call { + _c.Call.Return() + return _c +} + +func (_c *MockShellWriter_RmDirsRecursive_Call) RunAndReturn(run func(string, string)) *MockShellWriter_RmDirsRecursive_Call { + _c.Call.Return(run) + return _c +} + // RmFile provides a mock function with given fields: path func (_m *MockShellWriter) RmFile(path string) { _m.Called(path) diff --git a/shells/powershell.go b/shells/powershell.go index e45dd87003..b26ed718ee 100644 --- a/shells/powershell.go +++ b/shells/powershell.go @@ -520,6 +520,18 @@ func (p *PsWriter) RmFilesRecursive(path string, name string) { p.EndIf() } +func (p *PsWriter) RmDirsRecursive(path string, name string) { + resolvedPath := p.resolvePath(path) + p.IfDirectory(path) + p.Linef( + // `Remove-Item -Recurse` has a known issue (see Example 4 in + // https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.management/remove-item) + "Get-ChildItem -Path %s -Filter %s -Recurse | ?{ $_.PSIsContainer } | ForEach-Object { Remove-Item -Recurse -Force $_.FullName }", + resolvedPath, psQuoteVariable(name), + ) + p.EndIf() +} + func (p *PsWriter) Printf(format string, arguments ...interface{}) { coloredText := helpers.ANSI_RESET + fmt.Sprintf(format, arguments...) p.Line("echo " + psQuoteVariable(coloredText)) diff --git a/shells/shell_writer.go b/shells/shell_writer.go index 39510ebd15..b81c1f4234 100644 --- a/shells/shell_writer.go +++ b/shells/shell_writer.go @@ -28,7 +28,12 @@ type ShellWriter interface { MkDir(path string) RmDir(path string) RmFile(path string) + + // RmFilesRecursive deletes all files in path with a basename of name RmFilesRecursive(path string, name string) + // RmDirsRecursive deletes all directories and their content in path with a basename of name + RmDirsRecursive(path string, name string) + Absolute(path string) string Join(elem ...string) string TmpFile(name string) string diff --git a/shells/shell_writer_integration_test.go b/shells/shell_writer_integration_test.go index 061fb4caea..960921668f 100644 --- a/shells/shell_writer_integration_test.go +++ b/shells/shell_writer_integration_test.go @@ -121,6 +121,31 @@ func TestRmFilesRecursive(t *testing.T) { }) } +func TestRmDirsRecursive(t *testing.T) { + testFiles := testFileTree{ + "some/dir2rm/even/nested/dir2rm/file": "should be deleted incl. ancestor dirs", + "dir2rm": "this is a file and should not be deleted", + "not/really_dir2rm": "", + "random/dir2rm": "", + } + + shellstest.OnEachShellWithWriter(t, func(t *testing.T, shell string, writer shells.ShellWriter) { + tmpDir := t.TempDir() + testFiles.Create(t, tmpDir) + + writer.RmDirsRecursive(tmpDir, "dir2rm") + + runShell(t, shell, tmpDir, writer) + + assert.DirExists(t, filepath.Join(tmpDir, "some")) + assert.NoDirExists(t, filepath.Join(tmpDir, "some/dir2rm")) + assert.FileExists(t, filepath.Join(tmpDir, "dir2rm")) + assert.DirExists(t, filepath.Join(tmpDir, "not/really_dir2rm")) + assert.DirExists(t, filepath.Join(tmpDir, "random")) + assert.NoDirExists(t, filepath.Join(tmpDir, "random/dir2rm")) + }) +} + func TestCommandArgumentExpansion(t *testing.T) { tmpDir, err := ioutil.TempDir("", "runner-test") require.NoError(t, err) @@ -226,3 +251,21 @@ password=some-pass assert.Equal(t, expectedOutput, output) }) } + +type testFileTree map[string]string + +func (tft testFileTree) Create(t *testing.T, baseDir string) { + for path, content := range tft { + if content == "" { + // on empty content, we don't create a file but a leaf directory + err := os.MkdirAll(filepath.Join(baseDir, path), 0750) + require.NoError(t, err) + continue + } + + err := os.MkdirAll(filepath.Join(baseDir, filepath.Dir(path)), 0750) + require.NoError(t, err) + err = os.WriteFile(filepath.Join(baseDir, path), []byte(content), 0644) + require.NoError(t, err) + } +} -- GitLab From 067d25ed5f1ee406652b1eebee5e11037d319496 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20H=C3=B6rl?= Date: Wed, 19 Feb 2025 12:46:03 +0100 Subject: [PATCH 5/8] Run cleanup before setting configs This runs the cleanup, e.g. the deletion of the template git config before template configs are set. Before that, we set some configs and then deleted them again, which does not make much sense. --- shells/abstract.go | 4 ++-- shells/abstract_test.go | 11 ++++++----- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/shells/abstract.go b/shells/abstract.go index c6702a4635..55e7710476 100644 --- a/shells/abstract.go +++ b/shells/abstract.go @@ -588,6 +588,8 @@ func (b *AbstractShell) writeRefspecFetchCmd(w ShellWriter, info common.ShellScr templateFile := w.Join(templateDir, "config") objectFormat := build.GetRepositoryObjectFormat() + b.writeGitCleanup(w, build) + if build.SafeDirectoryCheckout { // Solves problem with newer Git versions when files existing in the working directory // are owned by different system owners. This may happen for example with Docker executor, @@ -608,8 +610,6 @@ func (b *AbstractShell) writeRefspecFetchCmd(w ShellWriter, info common.ShellScr b.writeGitSSLConfig(w, build, []string{"-f", templateFile}) } - b.writeGitCleanup(w, build) - if objectFormat != common.DefaultObjectFormat { w.Command("git", "init", projectDir, "--template", templateDir, "--object-format", objectFormat) } else { diff --git a/shells/abstract_test.go b/shells/abstract_test.go index 24eeccb414..1e35bed4b1 100644 --- a/shells/abstract_test.go +++ b/shells/abstract_test.go @@ -2867,6 +2867,12 @@ func TestAbstractShell_writeGitCleanup(t *testing.T) { sw := NewMockShellWriter(t) sw.EXPECT().Noticef("Fetching changes...").Once() + + expectFileCleanup(sw, ".git", expectSubmoduleCleanupCalls) + if cleanGitConfig.expectGitConfigsToBeCleaned { + expectGitConfigCleanup(sw, "", expectSubmoduleCleanupCalls) + } + sw.EXPECT().MkTmpDir("git-template").Return("someTmpDir").Once() sw.EXPECT().Join("someTmpDir", "config").Return("someGitTemplateConfig").Once() @@ -2875,11 +2881,6 @@ func TestAbstractShell_writeGitCleanup(t *testing.T) { sw.EXPECT().Command("git", "config", "-f", "someGitTemplateConfig", "credential.interactive", "never").Once() sw.EXPECT().Command("git", "config", "-f", "someGitTemplateConfig", "transfer.bundleURI", "true").Once() - expectFileCleanup(sw, ".git", expectSubmoduleCleanupCalls) - if cleanGitConfig.expectGitConfigsToBeCleaned { - expectGitConfigCleanup(sw, "", expectSubmoduleCleanupCalls) - } - sw.EXPECT().Command("git", "init", "", "--template", "someTmpDir").Once() sw.EXPECT().Cd("").Once() -- GitLab From 9bf84f80b27c28fa783f0e306297ac0c6bbdc125 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20H=C3=B6rl?= Date: Thu, 6 Mar 2025 12:07:12 +0100 Subject: [PATCH 6/8] Clean up TestPowershellPathResolveOperations Pull out eol for better readability. --- shells/powershell_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/shells/powershell_test.go b/shells/powershell_test.go index 912221efa5..07cc60bc38 100644 --- a/shells/powershell_test.go +++ b/shells/powershell_test.go @@ -473,6 +473,7 @@ func TestPowershell_GenerateScript(t *testing.T) { eol = v.Shell.(*PowerShell).EOL } shebang := "" + rmGitLabEnvScript := `` + `$CurrentDirectory = (Resolve-Path ./).Path` + eol + `if( (Get-Command -Name Remove-Item2 -Module NTFSSecurity -ErrorAction SilentlyContinue) -and (Test-Path "$CurrentDirectory/.tmp/gitlab_runner_env" -PathType Leaf) ) {` + eol + @@ -520,6 +521,7 @@ func TestPowershell_GenerateScript(t *testing.T) { `if(Test-Path ".git/refs" -PathType Container) {` + eol + ` Get-ChildItem -Path ".git/refs" -Filter "*.lock" -Recurse | ForEach-Object { Remove-Item -Force $_.FullName }` + eol + `}` + eol + cleanGitConfigs := `` + `$CurrentDirectory = (Resolve-Path ./).Path` + eol + `if( (Get-Command -Name Remove-Item2 -Module NTFSSecurity -ErrorAction SilentlyContinue) -and (Test-Path "$CurrentDirectory/.tmp/git-template/config" -PathType Leaf) ) {` + eol + -- GitLab From 4aab594b535c4a0d9a075c17990590f6ddc9e967 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20H=C3=B6rl?= Date: Mon, 10 Mar 2025 14:24:26 +0100 Subject: [PATCH 7/8] Set default for git config cleanup based on executor and git strategy By default the git config cleanup is enabeld, except the shell executor is used or the git strategy is "none". However, explicit configuration always has precedence. --- common/config.go | 2 +- executors/shell/shell_integration_test.go | 9 +- shells/abstract.go | 10 +- shells/abstract_test.go | 116 ++++++++++++++-------- 4 files changed, 88 insertions(+), 49 deletions(-) diff --git a/common/config.go b/common/config.go index 3d105c219f..d2905178f1 100644 --- a/common/config.go +++ b/common/config.go @@ -1190,7 +1190,7 @@ type RunnerSettings struct { DebugTraceDisabled bool `toml:"debug_trace_disabled,omitempty" json:"debug_trace_disabled" long:"debug-trace-disabled" env:"RUNNER_DEBUG_TRACE_DISABLED" description:"When set to true Runner will disable the possibility of using the CI_DEBUG_TRACE feature"` SafeDirectoryCheckout *bool `toml:"safe_directory_checkout,omitempty" json:"safe_directory_checkout,omitempty" long:"safe-directory-checkout" env:"RUNNER_SAFE_DIRECTORY_CHECKOUT" description:"When set to true, Git global configuration will get a safe.directory directive pointing the job's working directory'"` - CleanGitConfig *bool `toml:"clean_git_config,omitempty" json:"clean_git_config,omitempty" long:"clean-git-config" env:"RUNNER_CLEAN_GIT_CONFIG" description:"Clean git configuration. Defaults to true"` + CleanGitConfig *bool `toml:"clean_git_config,omitempty" json:"clean_git_config,omitempty" long:"clean-git-config" env:"RUNNER_CLEAN_GIT_CONFIG" description:"Clean git configuration before and after the build. Defaults to true, except the shell executor is used or the git strategy is \"none\""` Shell string `toml:"shell,omitempty" json:"shell" long:"shell" env:"RUNNER_SHELL" description:"Select bash, sh, cmd, pwsh or powershell" jsonschema:"enum=bash,enum=sh,enum=cmd,enum=pwsh,enum=powershell,enum="` CustomBuildDir CustomBuildDir `toml:"custom_build_dir,omitempty" json:"custom_build_dir,omitempty" group:"custom build dir configuration" namespace:"custom_build_dir"` diff --git a/executors/shell/shell_integration_test.go b/executors/shell/shell_integration_test.go index b4a766bb3e..4950fca7ee 100644 --- a/executors/shell/shell_integration_test.go +++ b/executors/shell/shell_integration_test.go @@ -1191,7 +1191,8 @@ func TestBuildWithGitFetchSubmoduleStrategyRecursive(t *testing.T) { expectFreshRepoMessage bool }{ "no git cleanup": { - expectFreshRepoMessage: true, + // shell executor defaults to not clean up git configs + expectFreshRepoMessage: false, }, "git cleanup explicitly enabled": { cleanGitConfig: &[]bool{true}[0], @@ -1638,7 +1639,8 @@ func TestBuildPowerShellCatchesExceptions(t *testing.T) { expectFreshRepoMessage bool }{ "no git cleanup": { - expectFreshRepoMessage: true, + // shell executor defaults to not clean up git configs + expectFreshRepoMessage: false, }, "git cleanup explicitly enabled": { cleanGitConfig: &[]bool{true}[0], @@ -1836,6 +1838,7 @@ func TestSanitizeGitDirectory(t *testing.T) { jobResponse, err := common.GetLocalBuildResponse( "git remote set-url origin /tmp/some/invalid/directory", ) + require.NoError(t, err, "getting job response") build := newBuild(t, jobResponse, shell) @@ -1845,6 +1848,8 @@ func TestSanitizeGitDirectory(t *testing.T) { common.JobVariable{Key: featureflags.EnableJobCleanup, Value: "true"}, ) + build.Runner.RunnerSettings.CleanGitConfig = &[]bool{true}[0] + err = buildtest.RunBuild(t, build) require.NoError(t, err) diff --git a/shells/abstract.go b/shells/abstract.go index 55e7710476..9a9cc9cb28 100644 --- a/shells/abstract.go +++ b/shells/abstract.go @@ -741,9 +741,15 @@ func (b *AbstractShell) writeGitCleanup(w ShellWriter, build *common.Build) { // - the main git config & hooks // - the template git config & hooks // - any submodule's git config & hooks +// It's by default disabled for the shell executor or when the git strategy is "none", and enabled otherwise; explicit +// configuration however always has precedence. func (b *AbstractShell) writeGitCleanupAllConfigs(sw ShellWriter, build *common.Build, cleanForSubmodules bool) { - if c := build.Runner.CleanGitConfig; c != nil && !*c { - // skip, if explicitly set to false + executor := build.Runner.Executor + shouldCleanUp := (executor != "shell" && executor != "shell-integration-test" && build.GetGitStrategy() != common.GitNone) + if config := build.Runner.CleanGitConfig; config != nil { + shouldCleanUp = *config + } + if !shouldCleanUp { return } diff --git a/shells/abstract_test.go b/shells/abstract_test.go index 1e35bed4b1..d04d004829 100644 --- a/shells/abstract_test.go +++ b/shells/abstract_test.go @@ -2305,67 +2305,95 @@ func TestAbstractShell_writeCleanupScript(t *testing.T) { testPath3 := "path/VAR_3_file" someTrue, someFalse := true, false + type executorName = string - tests := map[string]struct { + tests := map[executorName]map[string]struct { cleanGitConfig *bool + gitStrategy string shouldCleanGitConfig bool }{ - "no clean-git-config set": { - shouldCleanGitConfig: true, - }, - "clean-git-config explicitly enabled": { - cleanGitConfig: &someTrue, - shouldCleanGitConfig: true, + "shell": { + "no clean-git-config set": { + shouldCleanGitConfig: false, + }, + "clean-git-config explicitly enabled": { + cleanGitConfig: &someTrue, + shouldCleanGitConfig: true, + }, + "clean-git-config explicitly disabled": { + cleanGitConfig: &someFalse, + shouldCleanGitConfig: false, + }, }, - "clean-git-config explicitly disabled": { - cleanGitConfig: &someFalse, - shouldCleanGitConfig: false, + "not-shell": { + "no clean-git-config set": { + shouldCleanGitConfig: true, + }, + "no clean-git-config set, but git strategy is none": { + shouldCleanGitConfig: false, + gitStrategy: "none", + }, + "clean-git-config explicitly enabled": { + cleanGitConfig: &someTrue, + gitStrategy: "none", + shouldCleanGitConfig: true, + }, + "clean-git-config explicitly disabled": { + cleanGitConfig: &someFalse, + shouldCleanGitConfig: false, + }, }, } - for name, test := range tests { - t.Run(name, func(t *testing.T) { - info := common.ShellScriptInfo{ - Build: &common.Build{ - JobResponse: common.JobResponse{ - Variables: common.JobVariables{ - {Key: testVar1, Value: "test", File: true}, - {Key: testVar2, Value: "test", File: false}, - {Key: testVar3, Value: "test", File: true}, - {Key: testVar4, Value: "test", File: false}, - }, - }, - Runner: &common.RunnerConfig{ - RunnerSettings: common.RunnerSettings{ - CleanGitConfig: test.cleanGitConfig, + for executorName, testCases := range tests { + t.Run("executor:"+executorName, func(t *testing.T) { + for name, test := range testCases { + t.Run(name, func(t *testing.T) { + info := common.ShellScriptInfo{ + Build: &common.Build{ + JobResponse: common.JobResponse{ + Variables: common.JobVariables{ + {Key: testVar1, Value: "test", File: true}, + {Key: testVar2, Value: "test", File: false}, + {Key: testVar3, Value: "test", File: true}, + {Key: testVar4, Value: "test", File: false}, + }, + }, + Runner: &common.RunnerConfig{ + RunnerSettings: common.RunnerSettings{ + CleanGitConfig: test.cleanGitConfig, + Executor: executorName, + Environment: []string{"GIT_STRATEGY=" + test.gitStrategy}, + }, + }, }, - }, - }, - } + } - mockShellWriter := NewMockShellWriter(t) + mockShellWriter := NewMockShellWriter(t) - mockShellWriter.On("TmpFile", "masking.db").Return("masking.db").Once() - mockShellWriter.On("RmFile", "masking.db").Once() + mockShellWriter.On("TmpFile", "masking.db").Return("masking.db").Once() + mockShellWriter.On("RmFile", "masking.db").Once() - mockShellWriter.On("TmpFile", testVar1).Return(testPath1).Once() - mockShellWriter.On("RmFile", testPath1).Once() - mockShellWriter.On("TmpFile", testVar3).Return(testPath3).Once() - mockShellWriter.On("RmFile", testPath3).Once() + mockShellWriter.On("TmpFile", testVar1).Return(testPath1).Once() + mockShellWriter.On("RmFile", testPath1).Once() + mockShellWriter.On("TmpFile", testVar3).Return(testPath3).Once() + mockShellWriter.On("RmFile", testPath3).Once() - mockShellWriter.On("TmpFile", "gitlab_runner_env").Return("temp_env").Once() - mockShellWriter.On("RmFile", "temp_env").Once() + mockShellWriter.On("TmpFile", "gitlab_runner_env").Return("temp_env").Once() + mockShellWriter.On("RmFile", "temp_env").Once() - expectFileCleanup(mockShellWriter, ".git", false) + expectFileCleanup(mockShellWriter, ".git", false) - if test.shouldCleanGitConfig { - expectGitConfigCleanup(mockShellWriter, "", false) - } + if test.shouldCleanGitConfig { + expectGitConfigCleanup(mockShellWriter, "", false) + } - shell := new(AbstractShell) + shell := new(AbstractShell) - err := shell.writeCleanupScript(context.Background(), mockShellWriter, info) - assert.NoError(t, err) + err := shell.writeCleanupScript(context.Background(), mockShellWriter, info) + assert.NoError(t, err) + }) + } }) } } -- GitLab From b8c0e8ce15d6dba7c2493d8b60458eb11d9384c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20H=C3=B6rl?= Date: Tue, 11 Mar 2025 13:21:21 +0100 Subject: [PATCH 8/8] Document new git cleanup behaviour --- docs/configuration/advanced-configuration.md | 41 ++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/docs/configuration/advanced-configuration.md b/docs/configuration/advanced-configuration.md index 60defe7aeb..75c586d22d 100644 --- a/docs/configuration/advanced-configuration.md +++ b/docs/configuration/advanced-configuration.md @@ -257,6 +257,7 @@ Each `[[runners]]` section defines one runner. | `post_build_script` | Commands to be executed on the runner just after executing the job, but before executing `after_script`. To insert multiple commands, use a (triple-quoted) multi-line string or `\n` character. | | `clone_url` | Overwrite the URL for the GitLab instance. Used only if the runner can't connect to the GitLab URL. | | `debug_trace_disabled` | Disables [debug tracing](https://docs.gitlab.com/ci/variables/#enable-debug-logging). When set to `true`, the debug log (trace) remains disabled even if `CI_DEBUG_TRACE` is set to `true`. | +| `clean_git_config` | Cleans the Git configuration. For more information, see [Cleaning Git configuration](#cleaning-git-configuration). | | `referees` | Extra job monitoring workers that pass their results as job artifacts to GitLab. | | `unhealthy_requests_limit` | The number of `unhealthy` responses to new job requests after which a runner worker is disabled. | | `unhealthy_interval` | Duration that a runner worker is disabled for after it exceeds the unhealthy requests limit. Supports syntax like '3600 s', '1 h 30 min' etc. | @@ -1770,6 +1771,46 @@ provide stability in such cases. If you have dependencies that are required for your CI, you must install them in some other place. +## Cleaning Git configuration + +{{< history >}} + +- [Introduced](https://gitlab.com/gitlab-org/gitlab-runner/-/merge_requests/5438) in GitLab Runner 17.10. + +{{< /history >}} + +At the beginning and end of every build, GitLab Runner removes the following +files from the repository and its submodules: + +- Git lock files (`{index,shallow,HEAD,config}.lock`) +- Post-checkout hooks (`hooks/post-checkout`) + +If you enable `clean_git_config`, the following additional files or directories +are removed from the repository, its submodules, and the Git template directory: + +- `.git/config` file +- `.git/hooks` directory + +This cleanup prevents custom, ephemeral, or potentially malicious Git configuration +from caching between jobs. + +Before GitLab Runner 17.10, cleanups behaved differently: + +- Git lock files and Post-checkout hooks cleanup only occurred at the + beginning of a job and not at the end. +- Other Git configurations (now controlled by `clean_git_config`) were not removed unless + `FF_ENABLE_JOB_CLEANUP` was set. When you set this flag, only the main repository's + `.git/config` was deleted but not submodule configurations. + +The `clean_git_config` setting defaults to `true`. But, it defaults to `false` when: + +- [Shell executor](../executors/shell.md) is used. +- [Git strategy](https://docs.gitlab.com/ci/runners/configure_runners/#git-strategy) + is set to `none`. + +Explicit `clean_git_config` configuration takes precedence over the default +setting. + ## The `[runners.referees]` section Use GitLab Runner referees to pass extra job monitoring data to GitLab. Referees are workers in the runner manager that query and collect additional data related to a job. The results -- GitLab