diff --git a/agent/agent_configuration.go b/agent/agent_configuration.go index 64a87e8d86..e327b69240 100644 --- a/agent/agent_configuration.go +++ b/agent/agent_configuration.go @@ -31,6 +31,7 @@ type AgentConfiguration struct { GitSubmoduleCloneConfig []string SkipCheckout bool GitSkipFetchExistingCommits bool + NoCheckoutOverride bool CheckoutAttempts int AllowedRepositories []*regexp.Regexp AllowedPlugins []*regexp.Regexp diff --git a/agent/integration/job_environment_integration_test.go b/agent/integration/job_environment_integration_test.go index c71d2441a4..4616b3036f 100644 --- a/agent/integration/job_environment_integration_test.go +++ b/agent/integration/job_environment_integration_test.go @@ -1,6 +1,7 @@ package integration import ( + "slices" "strings" "testing" @@ -229,6 +230,497 @@ func TestBuildkiteRequestHeaders(t *testing.T) { } } +func TestCheckoutScopedJobEnvOverrideHonorsNoCheckoutOverride(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + varName string + jobEnv map[string]string + agentCfg agent.AgentConfiguration + wantEnvValue string + wantIgnoredEnvVars []string + }{ + { + name: "disabled_allows_job_env_to_override_clone_flags", + varName: "BUILDKITE_GIT_CLONE_FLAGS", + jobEnv: map[string]string{ + "BUILDKITE_GIT_CLONE_FLAGS": "--no-tags", + }, + agentCfg: agent.AgentConfiguration{ + GitCloneFlags: "--mirror", + }, + wantEnvValue: "--no-tags", + }, + { + name: "enabled_locks_clone_flags_to_agent_config", + varName: "BUILDKITE_GIT_CLONE_FLAGS", + jobEnv: map[string]string{ + "BUILDKITE_GIT_CLONE_FLAGS": "--no-tags", + }, + agentCfg: agent.AgentConfiguration{ + GitCloneFlags: "--mirror", + NoCheckoutOverride: true, + }, + wantEnvValue: "--mirror", + wantIgnoredEnvVars: []string{"BUILDKITE_GIT_CLONE_FLAGS"}, + }, + { + name: "disabled_allows_job_env_to_enable_submodules", + varName: "BUILDKITE_GIT_SUBMODULES", + jobEnv: map[string]string{ + "BUILDKITE_GIT_SUBMODULES": "true", + }, + agentCfg: agent.AgentConfiguration{ + GitSubmodules: false, + }, + wantEnvValue: "true", + }, + { + name: "enabled_locks_submodules_to_agent_config", + varName: "BUILDKITE_GIT_SUBMODULES", + jobEnv: map[string]string{ + "BUILDKITE_GIT_SUBMODULES": "true", + }, + agentCfg: agent.AgentConfiguration{ + GitSubmodules: false, + NoCheckoutOverride: true, + }, + wantEnvValue: "false", + wantIgnoredEnvVars: []string{"BUILDKITE_GIT_SUBMODULES"}, + }, + { + name: "disabled_allows_job_env_to_override_skip_checkout", + varName: "BUILDKITE_SKIP_CHECKOUT", + jobEnv: map[string]string{ + "BUILDKITE_SKIP_CHECKOUT": "false", + }, + agentCfg: agent.AgentConfiguration{ + SkipCheckout: true, + }, + wantEnvValue: "false", + }, + { + name: "enabled_locks_skip_checkout_to_agent_config", + varName: "BUILDKITE_SKIP_CHECKOUT", + jobEnv: map[string]string{ + "BUILDKITE_SKIP_CHECKOUT": "false", + }, + agentCfg: agent.AgentConfiguration{ + SkipCheckout: true, + NoCheckoutOverride: true, + }, + wantEnvValue: "true", + wantIgnoredEnvVars: []string{"BUILDKITE_SKIP_CHECKOUT"}, + }, + { + name: "disabled_allows_job_env_to_override_sparse_checkout_paths", + varName: "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS", + jobEnv: map[string]string{ + "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS": "job/path", + }, + agentCfg: agent.AgentConfiguration{ + GitSparseCheckoutPaths: []string{"agent/path"}, + }, + wantEnvValue: "job/path", + }, + { + name: "enabled_locks_sparse_checkout_paths_to_agent_config", + varName: "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS", + jobEnv: map[string]string{ + "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS": "job/path", + }, + agentCfg: agent.AgentConfiguration{ + GitSparseCheckoutPaths: []string{"agent/path"}, + NoCheckoutOverride: true, + }, + wantEnvValue: "agent/path", + wantIgnoredEnvVars: []string{"BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS"}, + }, + // Inverse cases: when the agent config sits on the side that emits no var + // by default, the lock must still force the agent value (regression for the + // leak where backend job env survived under no-checkout-override). + { + name: "enabled_locks_submodules_on_to_agent_config", + varName: "BUILDKITE_GIT_SUBMODULES", + jobEnv: map[string]string{ + "BUILDKITE_GIT_SUBMODULES": "false", + }, + agentCfg: agent.AgentConfiguration{ + GitSubmodules: true, + NoCheckoutOverride: true, + }, + wantEnvValue: "true", + wantIgnoredEnvVars: []string{"BUILDKITE_GIT_SUBMODULES"}, + }, + { + name: "disabled_allows_job_env_to_disable_submodules", + varName: "BUILDKITE_GIT_SUBMODULES", + jobEnv: map[string]string{ + "BUILDKITE_GIT_SUBMODULES": "false", + }, + agentCfg: agent.AgentConfiguration{ + GitSubmodules: true, + }, + wantEnvValue: "false", + }, + { + name: "enabled_locks_skip_checkout_off_to_agent_config", + varName: "BUILDKITE_SKIP_CHECKOUT", + jobEnv: map[string]string{ + "BUILDKITE_SKIP_CHECKOUT": "true", + }, + agentCfg: agent.AgentConfiguration{ + SkipCheckout: false, + NoCheckoutOverride: true, + }, + wantEnvValue: "false", + wantIgnoredEnvVars: []string{"BUILDKITE_SKIP_CHECKOUT"}, + }, + { + name: "enabled_locks_skip_fetch_existing_commits_to_agent_config", + varName: "BUILDKITE_GIT_SKIP_FETCH_EXISTING_COMMITS", + jobEnv: map[string]string{ + "BUILDKITE_GIT_SKIP_FETCH_EXISTING_COMMITS": "true", + }, + agentCfg: agent.AgentConfiguration{ + GitSkipFetchExistingCommits: false, + NoCheckoutOverride: true, + }, + wantEnvValue: "false", + wantIgnoredEnvVars: []string{"BUILDKITE_GIT_SKIP_FETCH_EXISTING_COMMITS"}, + }, + // The remaining checkout-scoped vars all flow through setCheckoutEnv; cover + // each in both directions so a regression in any one is caught. The git flag + // vars are the injection vectors the lock exists to contain. + { + name: "disabled_allows_job_env_to_override_checkout_flags", + varName: "BUILDKITE_GIT_CHECKOUT_FLAGS", + jobEnv: map[string]string{ + "BUILDKITE_GIT_CHECKOUT_FLAGS": "--quiet", + }, + agentCfg: agent.AgentConfiguration{ + GitCheckoutFlags: "-f", + }, + wantEnvValue: "--quiet", + }, + { + name: "enabled_locks_checkout_flags_to_agent_config", + varName: "BUILDKITE_GIT_CHECKOUT_FLAGS", + jobEnv: map[string]string{ + "BUILDKITE_GIT_CHECKOUT_FLAGS": "--quiet", + }, + agentCfg: agent.AgentConfiguration{ + GitCheckoutFlags: "-f", + NoCheckoutOverride: true, + }, + wantEnvValue: "-f", + wantIgnoredEnvVars: []string{"BUILDKITE_GIT_CHECKOUT_FLAGS"}, + }, + { + name: "disabled_allows_job_env_to_override_fetch_flags", + varName: "BUILDKITE_GIT_FETCH_FLAGS", + jobEnv: map[string]string{ + "BUILDKITE_GIT_FETCH_FLAGS": "--prune", + }, + agentCfg: agent.AgentConfiguration{ + GitFetchFlags: "-v", + }, + wantEnvValue: "--prune", + }, + { + name: "enabled_locks_fetch_flags_to_agent_config", + varName: "BUILDKITE_GIT_FETCH_FLAGS", + jobEnv: map[string]string{ + "BUILDKITE_GIT_FETCH_FLAGS": "--prune", + }, + agentCfg: agent.AgentConfiguration{ + GitFetchFlags: "-v", + NoCheckoutOverride: true, + }, + wantEnvValue: "-v", + wantIgnoredEnvVars: []string{"BUILDKITE_GIT_FETCH_FLAGS"}, + }, + { + name: "disabled_allows_job_env_to_override_clean_flags", + varName: "BUILDKITE_GIT_CLEAN_FLAGS", + jobEnv: map[string]string{ + "BUILDKITE_GIT_CLEAN_FLAGS": "-fdq", + }, + agentCfg: agent.AgentConfiguration{ + GitCleanFlags: "-ffxdq", + }, + wantEnvValue: "-fdq", + }, + { + name: "enabled_locks_clean_flags_to_agent_config", + varName: "BUILDKITE_GIT_CLEAN_FLAGS", + jobEnv: map[string]string{ + "BUILDKITE_GIT_CLEAN_FLAGS": "-fdq", + }, + agentCfg: agent.AgentConfiguration{ + GitCleanFlags: "-ffxdq", + NoCheckoutOverride: true, + }, + wantEnvValue: "-ffxdq", + wantIgnoredEnvVars: []string{"BUILDKITE_GIT_CLEAN_FLAGS"}, + }, + { + name: "disabled_allows_job_env_to_override_clone_mirror_flags", + varName: "BUILDKITE_GIT_CLONE_MIRROR_FLAGS", + jobEnv: map[string]string{ + "BUILDKITE_GIT_CLONE_MIRROR_FLAGS": "--mirror", + }, + agentCfg: agent.AgentConfiguration{ + GitCloneMirrorFlags: "--bare", + }, + wantEnvValue: "--mirror", + }, + { + name: "enabled_locks_clone_mirror_flags_to_agent_config", + varName: "BUILDKITE_GIT_CLONE_MIRROR_FLAGS", + jobEnv: map[string]string{ + "BUILDKITE_GIT_CLONE_MIRROR_FLAGS": "--mirror", + }, + agentCfg: agent.AgentConfiguration{ + GitCloneMirrorFlags: "--bare", + NoCheckoutOverride: true, + }, + wantEnvValue: "--bare", + wantIgnoredEnvVars: []string{"BUILDKITE_GIT_CLONE_MIRROR_FLAGS"}, + }, + { + name: "disabled_allows_job_env_to_override_mirrors_skip_update", + varName: "BUILDKITE_GIT_MIRRORS_SKIP_UPDATE", + jobEnv: map[string]string{ + "BUILDKITE_GIT_MIRRORS_SKIP_UPDATE": "true", + }, + agentCfg: agent.AgentConfiguration{ + GitMirrorsSkipUpdate: false, + }, + wantEnvValue: "true", + }, + { + name: "enabled_locks_mirrors_skip_update_to_agent_config", + varName: "BUILDKITE_GIT_MIRRORS_SKIP_UPDATE", + jobEnv: map[string]string{ + "BUILDKITE_GIT_MIRRORS_SKIP_UPDATE": "true", + }, + agentCfg: agent.AgentConfiguration{ + GitMirrorsSkipUpdate: false, + NoCheckoutOverride: true, + }, + wantEnvValue: "false", + wantIgnoredEnvVars: []string{"BUILDKITE_GIT_MIRRORS_SKIP_UPDATE"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + ctx := t.Context() + jobEnv := map[string]string{ + "BUILDKITE_COMMAND": "echo hello world", + } + for k, v := range tc.jobEnv { + jobEnv[k] = v + } + + job := &api.Job{ + ID: "my-job-id", + ChunksMaxSizeBytes: 1024, + Env: jobEnv, + Token: "bkaj_job-token", + } + + mb := mockBootstrap(t) + defer mb.CheckAndClose(t) //nolint:errcheck // bintest logs to t + + mb.Expect().Once().AndExitWith(0).AndCallFunc(func(c *bintest.Call) { + if got, want := c.GetEnv(tc.varName), tc.wantEnvValue; got != want { + t.Errorf("c.GetEnv(%s) = %q, want %q", tc.varName, got, want) + c.Exit(1) + return + } + + ignored := strings.Split(strings.TrimSpace(c.GetEnv("BUILDKITE_IGNORED_ENV")), ",") + for _, wantIgnored := range tc.wantIgnoredEnvVars { + if !slices.Contains(ignored, wantIgnored) { + t.Errorf("BUILDKITE_IGNORED_ENV = %q, want it to contain %q", c.GetEnv("BUILDKITE_IGNORED_ENV"), wantIgnored) + c.Exit(1) + return + } + } + if len(tc.wantIgnoredEnvVars) == 0 && c.GetEnv("BUILDKITE_IGNORED_ENV") != "" { + t.Errorf("BUILDKITE_IGNORED_ENV = %q, want empty", c.GetEnv("BUILDKITE_IGNORED_ENV")) + c.Exit(1) + return + } + + c.Exit(0) + }) + + e := createTestAgentEndpoint() + server := e.server() + defer server.Close() + + if err := runJob(t, ctx, testRunJobConfig{ + job: job, + server: server, + agentCfg: tc.agentCfg, + mockBootstrap: mb, + }); err != nil { + t.Fatalf("runJob() error = %v", err) + } + }) + } +} + +func TestCheckoutInfraVarsAreAgentAuthoritative(t *testing.T) { + t.Parallel() + + // SSH_KEYSCAN, GIT_MIRRORS_PATH, GIT_MIRRORS_LOCK_TIMEOUT and + // GIT_MIRROR_CHECKOUT_MODE are agent-only: job env cannot override them even + // with no-checkout-override disabled. + tests := []struct { + name string + varName string + jobEnvValue string + agentCfg agent.AgentConfiguration + wantEnvValue string + }{ + { + name: "ssh_keyscan", + varName: "BUILDKITE_SSH_KEYSCAN", + jobEnvValue: "false", + agentCfg: agent.AgentConfiguration{SSHKeyscan: true}, + wantEnvValue: "true", + }, + { + name: "git_mirrors_path", + varName: "BUILDKITE_GIT_MIRRORS_PATH", + jobEnvValue: "/tmp/attacker-mirrors", + agentCfg: agent.AgentConfiguration{GitMirrorsPath: "/agent/mirrors"}, + wantEnvValue: "/agent/mirrors", + }, + { + name: "git_mirrors_lock_timeout", + varName: "BUILDKITE_GIT_MIRRORS_LOCK_TIMEOUT", + jobEnvValue: "1", + agentCfg: agent.AgentConfiguration{GitMirrorsLockTimeout: 300}, + wantEnvValue: "300", + }, + { + name: "git_mirror_checkout_mode", + varName: "BUILDKITE_GIT_MIRROR_CHECKOUT_MODE", + jobEnvValue: "id", + agentCfg: agent.AgentConfiguration{GitMirrorCheckoutMode: "raw"}, + wantEnvValue: "raw", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + ctx := t.Context() + job := &api.Job{ + ID: "my-job-id", + ChunksMaxSizeBytes: 1024, + Env: map[string]string{ + "BUILDKITE_COMMAND": "echo hello world", + tc.varName: tc.jobEnvValue, + }, + Token: "bkaj_job-token", + } + + mb := mockBootstrap(t) + defer mb.CheckAndClose(t) //nolint:errcheck // bintest logs to t + + mb.Expect().Once().AndExitWith(0).AndCallFunc(func(c *bintest.Call) { + if got, want := c.GetEnv(tc.varName), tc.wantEnvValue; got != want { + t.Errorf("c.GetEnv(%s) = %q, want %q", tc.varName, got, want) + c.Exit(1) + return + } + + ignored := strings.Split(strings.TrimSpace(c.GetEnv("BUILDKITE_IGNORED_ENV")), ",") + if !slices.Contains(ignored, tc.varName) { + t.Errorf("BUILDKITE_IGNORED_ENV = %q, want it to contain %q", c.GetEnv("BUILDKITE_IGNORED_ENV"), tc.varName) + c.Exit(1) + return + } + + c.Exit(0) + }) + + e := createTestAgentEndpoint() + server := e.server() + defer server.Close() + + if err := runJob(t, ctx, testRunJobConfig{ + job: job, + server: server, + agentCfg: tc.agentCfg, + mockBootstrap: mb, + }); err != nil { + t.Fatalf("runJob() error = %v", err) + } + }) + } +} + +func TestNoCheckoutOverrideFlagIgnoresJobEnvOverride(t *testing.T) { + t.Parallel() + + // The agent's no-checkout-override setting is authoritative: a job that + // supplies BUILDKITE_NO_CHECKOUT_OVERRIDE cannot turn the lock off. + ctx := t.Context() + job := &api.Job{ + ID: "my-job-id", + ChunksMaxSizeBytes: 1024, + Env: map[string]string{ + "BUILDKITE_COMMAND": "echo hello world", + "BUILDKITE_NO_CHECKOUT_OVERRIDE": "false", + }, + Token: "bkaj_job-token", + } + + mb := mockBootstrap(t) + defer mb.CheckAndClose(t) //nolint:errcheck // bintest logs to t + + mb.Expect().Once().AndExitWith(0).AndCallFunc(func(c *bintest.Call) { + if got, want := c.GetEnv("BUILDKITE_NO_CHECKOUT_OVERRIDE"), "true"; got != want { + t.Errorf("c.GetEnv(BUILDKITE_NO_CHECKOUT_OVERRIDE) = %q, want %q", got, want) + c.Exit(1) + return + } + + ignored := strings.Split(strings.TrimSpace(c.GetEnv("BUILDKITE_IGNORED_ENV")), ",") + if !slices.Contains(ignored, "BUILDKITE_NO_CHECKOUT_OVERRIDE") { + t.Errorf("BUILDKITE_IGNORED_ENV = %q, want it to contain BUILDKITE_NO_CHECKOUT_OVERRIDE", c.GetEnv("BUILDKITE_IGNORED_ENV")) + c.Exit(1) + return + } + + c.Exit(0) + }) + + e := createTestAgentEndpoint() + server := e.server() + defer server.Close() + + if err := runJob(t, ctx, testRunJobConfig{ + job: job, + server: server, + agentCfg: agent.AgentConfiguration{NoCheckoutOverride: true}, + mockBootstrap: mb, + }); err != nil { + t.Fatalf("runJob() error = %v", err) + } +} + func TestArtifactUploadConcurrencyFromAgentConfigIsSet(t *testing.T) { t.Parallel() diff --git a/agent/integration/job_runner_integration_test.go b/agent/integration/job_runner_integration_test.go index b9cdaa1771..5913f5850f 100644 --- a/agent/integration/job_runner_integration_test.go +++ b/agent/integration/job_runner_integration_test.go @@ -16,6 +16,7 @@ import ( "github.com/buildkite/agent/v3/agent" "github.com/buildkite/agent/v3/api" + "github.com/buildkite/agent/v3/internal/experiments" "github.com/buildkite/bintest/v3" ) @@ -280,6 +281,66 @@ func TestJobRunner_WhenJobHasToken_ItOverridesAccessToken(t *testing.T) { } } +func TestJobRunnerPassesNoCheckoutOverrideToBootstrapAndEnvFile(t *testing.T) { + t.Parallel() + + ctx, _ := experiments.Enable(t.Context(), experiments.PropagateAgentConfigVars) + + j := &api.Job{ + ID: "my-job-id", + ChunksMaxSizeBytes: 1024, + Env: map[string]string{ + "BUILDKITE_COMMAND": "echo hello world", + }, + Token: "bkaj_job-token", + } + + mb := mockBootstrap(t) + t.Cleanup(func() { + if err := mb.CheckAndClose(t); err != nil { + t.Errorf("mb.CheckAndClose(t) error = %v", err) + } + }) + + mb.Expect().Once().AndExitWith(0).AndCallFunc(func(c *bintest.Call) { + if got, want := c.GetEnv("BUILDKITE_NO_CHECKOUT_OVERRIDE"), "true"; got != want { + t.Errorf("c.GetEnv(BUILDKITE_NO_CHECKOUT_OVERRIDE) = %q, want %q", got, want) + c.Exit(1) + return + } + + envFile := c.GetEnv("BUILDKITE_ENV_FILE") + contents, err := os.ReadFile(envFile) + if err != nil { + t.Errorf("os.ReadFile(%q) error = %v", envFile, err) + c.Exit(1) + return + } + + if !strings.Contains(string(contents), "BUILDKITE_NO_CHECKOUT_OVERRIDE") { + t.Errorf("env file %q did not contain BUILDKITE_NO_CHECKOUT_OVERRIDE", envFile) + c.Exit(1) + return + } + + c.Exit(0) + }) + + e := createTestAgentEndpoint() + server := e.server() + defer server.Close() + + err := runJob(t, ctx, testRunJobConfig{ + job: j, + server: server, + agentCfg: agent.AgentConfiguration{NoCheckoutOverride: true}, + mockBootstrap: mb, + }) + if err != nil { + t.Fatalf("runJob() error = %v", err) + } +} + // TODO 2023-07-17: What is this testing? How is it testing it? // Maybe that the job runner pulls the access token from the API client? but that's all handled in the `runJob` helper... func TestJobRunnerPassesAccessTokenToBootstrap(t *testing.T) { diff --git a/agent/job_runner.go b/agent/job_runner.go index c95d55c8c8..5afac23199 100644 --- a/agent/job_runner.go +++ b/agent/job_runner.go @@ -407,8 +407,9 @@ func (r *JobRunner) normalizeVerificationBehavior(behavior string) (string, erro func (r *JobRunner) createEnvironment(ctx context.Context) ([]string, error) { // Create a clone of our jobs environment. We'll then set the // environment variables provided by the agent, which will override any - // sent by Buildkite. The variables below should always take - // precedence. + // sent by Buildkite. The variables below should always take precedence, + // except the checkout-scoped vars set via setCheckoutEnv, which defer to + // the Buildkite-sent value unless no-checkout-override is enabled. env := make(map[string]string) maps.Copy(env, r.conf.Job.Env) @@ -440,6 +441,17 @@ func (r *JobRunner) createEnvironment(ctx context.Context) ([]string, error) { } env[name] = value } + // For some checkout env vars, we allow the Job env (if present) to + // have higher precedence than the agent configuration, unless + // NoCheckoutOverride is set. + setCheckoutEnv := func(name, value string) { + if !r.conf.AgentConfiguration.NoCheckoutOverride { + if _, exists := env[name]; exists { + return + } + } + setEnv(name, value) + } // Write out the job environment to file: // - envShellFile: in k="v" format, with newlines escaped. If the @@ -467,6 +479,7 @@ BUILDKITE_GIT_MIRRORS_PATH BUILDKITE_GIT_MIRRORS_SKIP_UPDATE BUILDKITE_GIT_SUBMODULES BUILDKITE_GIT_SUBMODULE_CLONE_CONFIG +BUILDKITE_NO_CHECKOUT_OVERRIDE BUILDKITE_CANCEL_GRACE_PERIOD BUILDKITE_COMMAND_EVAL BUILDKITE_LOCAL_HOOKS_ENABLED @@ -591,25 +604,37 @@ BUILDKITE_AGENT_JWKS_KEY_ID` setEnv("BUILDKITE_BUILD_PATH", r.conf.AgentConfiguration.BuildPath) setEnv("BUILDKITE_SOCKETS_PATH", r.conf.AgentConfiguration.SocketsPath) setEnv("BUILDKITE_GIT_MIRRORS_PATH", r.conf.AgentConfiguration.GitMirrorsPath) - setEnv("BUILDKITE_GIT_MIRRORS_SKIP_UPDATE", fmt.Sprint(r.conf.AgentConfiguration.GitMirrorsSkipUpdate)) + setCheckoutEnv("BUILDKITE_GIT_MIRRORS_SKIP_UPDATE", fmt.Sprint(r.conf.AgentConfiguration.GitMirrorsSkipUpdate)) setEnv("BUILDKITE_HOOKS_PATH", r.conf.AgentConfiguration.HooksPath) setEnv("BUILDKITE_ADDITIONAL_HOOKS_PATHS", strings.Join(r.conf.AgentConfiguration.AdditionalHooksPaths, ",")) setEnv("BUILDKITE_PLUGINS_PATH", r.conf.AgentConfiguration.PluginsPath) setEnv("BUILDKITE_SSH_KEYSCAN", fmt.Sprint(r.conf.AgentConfiguration.SSHKeyscan)) - // Disable cloning submodules if specified in Agent config as precedence - // else allow pipeline/step env to control it via BUILDKITE_GIT_SUBMODULES - if !r.conf.AgentConfiguration.GitSubmodules { - setEnv("BUILDKITE_GIT_SUBMODULES", "false") - } - // Allow BUILDKITE_SKIP_CHECKOUT to be enabled either by agent config - // or by pipeline/step env - // This is here now to make it ready for if/when we add skip_checkout to the core app - if r.conf.AgentConfiguration.SkipCheckout { - setEnv("BUILDKITE_SKIP_CHECKOUT", "true") - } - if r.conf.AgentConfiguration.GitSkipFetchExistingCommits { - setEnv("BUILDKITE_GIT_SKIP_FETCH_EXISTING_COMMITS", "true") + // These three vars are only emitted by the agent on one side of their + // default (submodules off, skip-checkout on, skip-fetch on); on the other + // side the agent stays silent and lets pipeline/step env decide. That silent + // side would leak past no-checkout-override, so when the lock is on we emit + // the agent value unconditionally to keep agent config authoritative. + if r.conf.AgentConfiguration.NoCheckoutOverride { + setEnv("BUILDKITE_GIT_SUBMODULES", fmt.Sprint(r.conf.AgentConfiguration.GitSubmodules)) + setEnv("BUILDKITE_SKIP_CHECKOUT", fmt.Sprint(r.conf.AgentConfiguration.SkipCheckout)) + setEnv("BUILDKITE_GIT_SKIP_FETCH_EXISTING_COMMITS", fmt.Sprint(r.conf.AgentConfiguration.GitSkipFetchExistingCommits)) + } else { + // Default submodules off when disabled in agent config, but let pipeline/ + // step env override via BUILDKITE_GIT_SUBMODULES. + if !r.conf.AgentConfiguration.GitSubmodules { + setCheckoutEnv("BUILDKITE_GIT_SUBMODULES", "false") + } + // Allow BUILDKITE_SKIP_CHECKOUT to be enabled either by agent config + // or by pipeline/step env + // This is here now to make it ready for if/when we add skip_checkout to the core app + if r.conf.AgentConfiguration.SkipCheckout { + setCheckoutEnv("BUILDKITE_SKIP_CHECKOUT", "true") + } + if r.conf.AgentConfiguration.GitSkipFetchExistingCommits { + setCheckoutEnv("BUILDKITE_GIT_SKIP_FETCH_EXISTING_COMMITS", "true") + } } + setEnv("BUILDKITE_NO_CHECKOUT_OVERRIDE", fmt.Sprint(r.conf.AgentConfiguration.NoCheckoutOverride)) setEnv("BUILDKITE_CHECKOUT_ATTEMPTS", strconv.Itoa(r.conf.AgentConfiguration.CheckoutAttempts)) setEnv("BUILDKITE_COMMAND_EVAL", fmt.Sprint(r.conf.AgentConfiguration.CommandEval)) setEnv("BUILDKITE_PLUGINS_ENABLED", fmt.Sprint(r.conf.AgentConfiguration.PluginsEnabled)) @@ -620,16 +645,13 @@ BUILDKITE_AGENT_JWKS_KEY_ID` } setEnv("BUILDKITE_LOCAL_HOOKS_ENABLED", fmt.Sprint(r.conf.AgentConfiguration.LocalHooksEnabled)) - setEnv("BUILDKITE_GIT_CHECKOUT_FLAGS", r.conf.AgentConfiguration.GitCheckoutFlags) - setEnv("BUILDKITE_GIT_CLONE_FLAGS", r.conf.AgentConfiguration.GitCloneFlags) - setEnv("BUILDKITE_GIT_FETCH_FLAGS", r.conf.AgentConfiguration.GitFetchFlags) - - if len(r.conf.AgentConfiguration.GitSparseCheckoutPaths) > 0 { - setEnv("BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS", strings.Join(r.conf.AgentConfiguration.GitSparseCheckoutPaths, ",")) - } - setEnv("BUILDKITE_GIT_CLONE_MIRROR_FLAGS", r.conf.AgentConfiguration.GitCloneMirrorFlags) + setCheckoutEnv("BUILDKITE_GIT_CHECKOUT_FLAGS", r.conf.AgentConfiguration.GitCheckoutFlags) + setCheckoutEnv("BUILDKITE_GIT_CLONE_FLAGS", r.conf.AgentConfiguration.GitCloneFlags) + setCheckoutEnv("BUILDKITE_GIT_FETCH_FLAGS", r.conf.AgentConfiguration.GitFetchFlags) + setCheckoutEnv("BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS", strings.Join(r.conf.AgentConfiguration.GitSparseCheckoutPaths, ",")) + setCheckoutEnv("BUILDKITE_GIT_CLONE_MIRROR_FLAGS", r.conf.AgentConfiguration.GitCloneMirrorFlags) setEnv("BUILDKITE_GIT_MIRROR_CHECKOUT_MODE", r.conf.AgentConfiguration.GitMirrorCheckoutMode) - setEnv("BUILDKITE_GIT_CLEAN_FLAGS", r.conf.AgentConfiguration.GitCleanFlags) + setCheckoutEnv("BUILDKITE_GIT_CLEAN_FLAGS", r.conf.AgentConfiguration.GitCleanFlags) setEnv("BUILDKITE_GIT_MIRRORS_LOCK_TIMEOUT", strconv.Itoa(r.conf.AgentConfiguration.GitMirrorsLockTimeout)) if r.conf.AgentConfiguration.GitCheckoutTimeout > 0 { setEnv("BUILDKITE_GIT_CHECKOUT_TIMEOUT", strconv.Itoa(r.conf.AgentConfiguration.GitCheckoutTimeout)) diff --git a/clicommand/agent_start.go b/clicommand/agent_start.go index 7a2fad21c8..c408e440be 100644 --- a/clicommand/agent_start.go +++ b/clicommand/agent_start.go @@ -174,6 +174,7 @@ type AgentStartConfig struct { GitSubmoduleCloneConfig []string `cli:"git-submodule-clone-config"` SkipCheckout bool `cli:"skip-checkout"` GitSkipFetchExistingCommits bool `cli:"git-skip-fetch-existing-commits"` + NoCheckoutOverride bool `cli:"no-checkout-override"` CheckoutAttempts int `cli:"checkout-attempts"` NoSSHKeyscan bool `cli:"no-ssh-keyscan"` @@ -230,6 +231,16 @@ type AgentStartConfig struct { DisconnectAfterJobTimeout int `cli:"disconnect-after-job-timeout" deprecated:"Use disconnect-after-idle-timeout instead"` } +// lockCheckoutWhenCommandEvalDisabled forces no-checkout-override on when +// command-eval is disabled, so git flags can't be used to bypass it. +// AgentStartConfig stores this as NoCommandEval; the BootstrapConfig sibling +// uses CommandEval, so its check is inverted. Keep the two in sync. +func (cfg *AgentStartConfig) lockCheckoutWhenCommandEvalDisabled() { + if cfg.NoCommandEval { + cfg.NoCheckoutOverride = true + } +} + func (asc AgentStartConfig) Features(ctx context.Context) []string { if asc.NoFeatureReporting { return []string{} @@ -535,6 +546,7 @@ var AgentStartCommand = cli.Command{ // Various git related flags shared with bootstrap SkipCheckoutFlag, + NoCheckoutOverrideFlag, GitCheckoutFlagsFlag, GitCloneFlagsFlag, GitCleanFlagsFlag, @@ -916,6 +928,8 @@ var AgentStartCommand = cli.Command{ cfg.NoPlugins = true } + cfg.lockCheckoutWhenCommandEvalDisabled() + // Guess the shell if none is provided if cfg.Shell == "" { cfg.Shell = DefaultShell() @@ -1088,6 +1102,7 @@ var AgentStartCommand = cli.Command{ GitSubmoduleCloneConfig: cfg.GitSubmoduleCloneConfig, SkipCheckout: cfg.SkipCheckout, GitSkipFetchExistingCommits: cfg.GitSkipFetchExistingCommits, + NoCheckoutOverride: cfg.NoCheckoutOverride, CheckoutAttempts: cfg.CheckoutAttempts, SSHKeyscan: !cfg.NoSSHKeyscan, CommandEval: !cfg.NoCommandEval, diff --git a/clicommand/agent_start_test.go b/clicommand/agent_start_test.go index 42738d292f..e180880392 100644 --- a/clicommand/agent_start_test.go +++ b/clicommand/agent_start_test.go @@ -246,3 +246,55 @@ func TestAgentStartJobAcquisitionRejected_ExitCode27(t *testing.T) { t.Errorf("Expected exit code 27 for job acquisition rejected, got: %d", exitErr.ExitCode()) } } + +func TestAgentStartLockCheckoutWhenCommandEvalDisabled(t *testing.T) { + t.Parallel() + + // AgentStartConfig uses NoCommandEval, so the zero value leaves command-eval + // enabled and the lock off. + tests := []struct { + name string + cfg AgentStartConfig + want bool + }{ + {name: "explicit_flag", cfg: AgentStartConfig{NoCheckoutOverride: true}, want: true}, + {name: "no_command_eval_forces_lock", cfg: AgentStartConfig{NoCommandEval: true}, want: true}, + {name: "defaults_unlocked", cfg: AgentStartConfig{}, want: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + tc.cfg.lockCheckoutWhenCommandEvalDisabled() + if got := tc.cfg.NoCheckoutOverride; got != tc.want { + t.Errorf("NoCheckoutOverride = %v, want %v", got, tc.want) + } + }) + } +} + +func TestBootstrapLockCheckoutWhenCommandEvalDisabled(t *testing.T) { + t.Parallel() + + // BootstrapConfig uses CommandEval, so the zero value disables command-eval + // and forces the lock on; "unlocked" cases must set CommandEval explicitly. + tests := []struct { + name string + cfg BootstrapConfig + want bool + }{ + {name: "explicit_flag", cfg: BootstrapConfig{NoCheckoutOverride: true, CommandEval: true}, want: true}, + {name: "command_eval_disabled_forces_lock", cfg: BootstrapConfig{CommandEval: false}, want: true}, + {name: "defaults_unlocked", cfg: BootstrapConfig{CommandEval: true}, want: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + tc.cfg.lockCheckoutWhenCommandEvalDisabled() + if got := tc.cfg.NoCheckoutOverride; got != tc.want { + t.Errorf("NoCheckoutOverride = %v, want %v", got, tc.want) + } + }) + } +} diff --git a/clicommand/bootstrap.go b/clicommand/bootstrap.go index e1c32ff30a..0821a726bb 100644 --- a/clicommand/bootstrap.go +++ b/clicommand/bootstrap.go @@ -86,6 +86,7 @@ type BootstrapConfig struct { GitMirrorsLockTimeout int `cli:"git-mirrors-lock-timeout"` GitMirrorsSkipUpdate bool `cli:"git-mirrors-skip-update"` GitSubmoduleCloneConfig []string `cli:"git-submodule-clone-config" normalize:"list"` + NoCheckoutOverride bool `cli:"no-checkout-override"` BinPath string `cli:"bin-path" normalize:"filepath"` BuildPath string `cli:"build-path" normalize:"filepath"` HooksPath string `cli:"hooks-path" normalize:"filepath"` @@ -121,6 +122,16 @@ type BootstrapConfig struct { CheckoutAttempts int `cli:"checkout-attempts"` } +// lockCheckoutWhenCommandEvalDisabled forces no-checkout-override on when +// command-eval is disabled, so git flags can't be used to bypass it. +// BootstrapConfig stores this as CommandEval; the AgentStartConfig sibling uses +// NoCommandEval, so its check is inverted. Keep the two in sync. +func (cfg *BootstrapConfig) lockCheckoutWhenCommandEvalDisabled() { + if !cfg.CommandEval { + cfg.NoCheckoutOverride = true + } +} + var BootstrapCommand = cli.Command{ Name: "bootstrap", Usage: "Harness used internally by the agent to run jobs as subprocesses", @@ -242,6 +253,7 @@ var BootstrapCommand = cli.Command{ // Various git related flags shared with agent start SkipCheckoutFlag, + NoCheckoutOverrideFlag, GitCheckoutFlagsFlag, GitCloneFlagsFlag, GitCloneMirrorFlagsFlag, @@ -442,6 +454,8 @@ var BootstrapCommand = cli.Command{ return fmt.Errorf("while parsing trace context encoding: %v", err) } + cfg.lockCheckoutWhenCommandEvalDisabled() + // Configure the bootstraper bootstrap := job.New(job.ExecutorConfig{ AgentName: cfg.AgentName, @@ -457,6 +471,7 @@ var BootstrapCommand = cli.Command{ SkipCheckout: cfg.SkipCheckout, GitCheckoutTimeout: cfg.GitCheckoutTimeout, GitSkipFetchExistingCommits: cfg.GitSkipFetchExistingCommits, + NoCheckoutOverride: cfg.NoCheckoutOverride, Command: cfg.Command, CommandEval: cfg.CommandEval, Commit: cfg.Commit, diff --git a/clicommand/global.go b/clicommand/global.go index 3fedb94f9f..a13f8b706e 100644 --- a/clicommand/global.go +++ b/clicommand/global.go @@ -205,6 +205,12 @@ var ( EnvVar: "BUILDKITE_SKIP_CHECKOUT", } + NoCheckoutOverrideFlag = cli.BoolFlag{ + Name: "no-checkout-override", + Usage: "Don't allow pipeline/step env, hooks, plugins, the Job API, or secrets to override the agent's checkout settings (default: false)", + EnvVar: "BUILDKITE_NO_CHECKOUT_OVERRIDE", + } + GitCheckoutFlagsFlag = cli.StringFlag{ Name: "git-checkout-flags", Value: "-f", diff --git a/env/protected.go b/env/protected.go index d3b6a1bbfb..f32e935787 100644 --- a/env/protected.go +++ b/env/protected.go @@ -22,15 +22,9 @@ type protection struct { // disables plugins, but even if it is changed by a hook, the agent doesn't // reconfigure no-command-eval based on any changes.) // -// Git flags are an example of vars that are primarily driven by agent config, -// but we want to allow plugins to set git flags to alter the default checkout -// process (see for example the git-clean plugin). Doing this deliberately -// reconfigures the executor (see ReadFromEnvironment and config struct tags in -// internal/job/config.go). -// But we don't want the job env from the backend to be able to set them, -// because git is riddled with shell injections, and someone with privileges to -// start a build and supply env vars could then bypass protections like -// no-command-eval. +// BUILDKITE_GIT_SUBMODULE_CLONE_CONFIG is applied as `git -c ` before +// submodule clones (an injection vector) and has no backend customization, so +// it stays agent-authoritative rather than in checkoutOverrideScope. // // The actual enforcement of protected env within the agent level (overriding // job-level env vars based on agent configuration) happens implicitly rather @@ -45,44 +39,59 @@ type protection struct { // filter backend-supplied vars, and such vars are still necessary for a job to // function. // -// When updating ExecutorConfig in internal/job/config.go, ensure -// mutableFromWithinJob is enabled here for reconfigurable vars. +// When updating ExecutorConfig in internal/job/config.go, ensure always- +// protected reconfigurable vars set mutableFromWithinJob here, and checkout- +// scoped vars are added to checkoutOverrideScope below. var protectedEnv = map[string]protection{ - "BUILDKITE_AGENT_ACCESS_TOKEN": {}, - "BUILDKITE_AGENT_DEBUG": {}, - "BUILDKITE_AGENT_ENDPOINT": {}, - "BUILDKITE_AGENT_JOB_TIMEOUT_FILE": {}, - "BUILDKITE_AGENT_PID": {}, - "BUILDKITE_ARTIFACT_PATHS": {mutableFromWithinJob: true}, - "BUILDKITE_ARTIFACT_UPLOAD_DESTINATION": {mutableFromWithinJob: true}, - "BUILDKITE_BIN_PATH": {}, - "BUILDKITE_BUILD_PATH": {}, - "BUILDKITE_COMMAND_EVAL": {}, - "BUILDKITE_CONFIG_PATH": {}, - "BUILDKITE_CONTAINER_COUNT": {}, - "BUILDKITE_GIT_CHECKOUT_FLAGS": {mutableFromWithinJob: true}, - "BUILDKITE_GIT_CLEAN_FLAGS": {mutableFromWithinJob: true}, - "BUILDKITE_GIT_CLONE_FLAGS": {mutableFromWithinJob: true}, - "BUILDKITE_GIT_CLONE_MIRROR_FLAGS": {mutableFromWithinJob: true}, - "BUILDKITE_GIT_COMMIT_VERIFICATION": {}, - "BUILDKITE_GIT_FETCH_FLAGS": {mutableFromWithinJob: true}, - "BUILDKITE_GIT_MIRRORS_LOCK_TIMEOUT": {}, - "BUILDKITE_GIT_MIRRORS_PATH": {}, - "BUILDKITE_GIT_MIRRORS_SKIP_UPDATE": {mutableFromWithinJob: true}, - "BUILDKITE_GIT_SKIP_FETCH_EXISTING_COMMITS": {mutableFromWithinJob: true}, - "BUILDKITE_GIT_SUBMODULES": {mutableFromWithinJob: true}, - "BUILDKITE_GIT_SUBMODULE_CLONE_CONFIG": {mutableFromWithinJob: true}, - "BUILDKITE_HOOKS_PATH": {}, - "BUILDKITE_HOOKS_SHELL": {}, - "BUILDKITE_KUBERNETES_EXEC": {}, - "BUILDKITE_LOCAL_HOOKS_ENABLED": {}, - "BUILDKITE_PLUGINS_ALWAYS_CLONE_FRESH": {mutableFromWithinJob: true}, - "BUILDKITE_PLUGINS_ENABLED": {}, - "BUILDKITE_PLUGINS_PATH": {}, - "BUILDKITE_REFSPEC": {mutableFromWithinJob: true}, - "BUILDKITE_REPO": {mutableFromWithinJob: true}, - "BUILDKITE_SHELL": {}, - "BUILDKITE_SSH_KEYSCAN": {}, + "BUILDKITE_AGENT_ACCESS_TOKEN": {}, + "BUILDKITE_AGENT_DEBUG": {}, + "BUILDKITE_AGENT_ENDPOINT": {}, + "BUILDKITE_AGENT_JOB_TIMEOUT_FILE": {}, + "BUILDKITE_AGENT_PID": {}, + "BUILDKITE_ARTIFACT_PATHS": {mutableFromWithinJob: true}, + "BUILDKITE_ARTIFACT_UPLOAD_DESTINATION": {mutableFromWithinJob: true}, + "BUILDKITE_BIN_PATH": {}, + "BUILDKITE_BUILD_PATH": {}, + "BUILDKITE_COMMAND_EVAL": {}, + "BUILDKITE_CONFIG_PATH": {}, + "BUILDKITE_CONTAINER_COUNT": {}, + "BUILDKITE_GIT_COMMIT_VERIFICATION": {}, + "BUILDKITE_GIT_MIRRORS_LOCK_TIMEOUT": {}, + "BUILDKITE_GIT_MIRRORS_PATH": {}, + "BUILDKITE_GIT_MIRROR_CHECKOUT_MODE": {}, + "BUILDKITE_GIT_SUBMODULE_CLONE_CONFIG": {}, + "BUILDKITE_HOOKS_PATH": {}, + "BUILDKITE_HOOKS_SHELL": {}, + "BUILDKITE_KUBERNETES_EXEC": {}, + "BUILDKITE_LOCAL_HOOKS_ENABLED": {}, + "BUILDKITE_NO_CHECKOUT_OVERRIDE": {}, + "BUILDKITE_PLUGINS_ALWAYS_CLONE_FRESH": {mutableFromWithinJob: true}, + "BUILDKITE_PLUGINS_ENABLED": {}, + "BUILDKITE_PLUGINS_PATH": {}, + "BUILDKITE_REFSPEC": {mutableFromWithinJob: true}, + "BUILDKITE_REPO": {mutableFromWithinJob: true}, + "BUILDKITE_SHELL": {}, + "BUILDKITE_SSH_KEYSCAN": {}, +} + +// checkoutOverrideScope contains checkout-related vars that remain mutable in +// hooks, plugins, Job API, and secrets by default so jobs can tailor checkout +// behavior. When checkout override is enabled, those same vars become locked so +// agent checkout config wins: git is riddled with shell injections, so letting a +// job set git flags would otherwise be a way to bypass protections like +// no-command-eval. Vars here must not also appear in protectedEnv; the two maps +// are disjoint. +var checkoutOverrideScope = map[string]struct{}{ + "BUILDKITE_GIT_CHECKOUT_FLAGS": {}, + "BUILDKITE_GIT_CLEAN_FLAGS": {}, + "BUILDKITE_GIT_CLONE_FLAGS": {}, + "BUILDKITE_GIT_CLONE_MIRROR_FLAGS": {}, + "BUILDKITE_GIT_FETCH_FLAGS": {}, + "BUILDKITE_GIT_MIRRORS_SKIP_UPDATE": {}, + "BUILDKITE_GIT_SKIP_FETCH_EXISTING_COMMITS": {}, + "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS": {}, + "BUILDKITE_GIT_SUBMODULES": {}, + "BUILDKITE_SKIP_CHECKOUT": {}, } // IsProtected reports whether the environment variable is write-protected when @@ -102,3 +111,11 @@ func IsProtectedFromWithinJob(name string) bool { } return !prot.mutableFromWithinJob } + +// IsCheckoutOverrideScoped reports whether the environment variable is a +// checkout-related var that is write-protected (against job env, secrets, hooks, +// plugins, and the Job API) when no-checkout-override is enabled. +func IsCheckoutOverrideScoped(name string) bool { + _, exists := checkoutOverrideScope[normalizeKeyName(name)] + return exists +} diff --git a/env/protected_test.go b/env/protected_test.go index 01e5819c41..a99b86588c 100644 --- a/env/protected_test.go +++ b/env/protected_test.go @@ -17,17 +17,14 @@ func TestProtectedEnv(t *testing.T) { "BUILDKITE_COMMAND_EVAL", "BUILDKITE_CONFIG_PATH", "BUILDKITE_CONTAINER_COUNT", - "BUILDKITE_GIT_CLEAN_FLAGS", - "BUILDKITE_GIT_CLONE_FLAGS", - "BUILDKITE_GIT_CLONE_MIRROR_FLAGS", - "BUILDKITE_GIT_FETCH_FLAGS", "BUILDKITE_GIT_MIRRORS_LOCK_TIMEOUT", "BUILDKITE_GIT_MIRRORS_PATH", - "BUILDKITE_GIT_MIRRORS_SKIP_UPDATE", - "BUILDKITE_GIT_SUBMODULES", + "BUILDKITE_GIT_MIRROR_CHECKOUT_MODE", + "BUILDKITE_GIT_SUBMODULE_CLONE_CONFIG", "BUILDKITE_HOOKS_PATH", "BUILDKITE_KUBERNETES_EXEC", "BUILDKITE_LOCAL_HOOKS_ENABLED", + "BUILDKITE_NO_CHECKOUT_OVERRIDE", "BUILDKITE_PLUGINS_ENABLED", "BUILDKITE_PLUGINS_PATH", "BUILDKITE_SHELL", @@ -48,6 +45,9 @@ func TestProtectedEnv(t *testing.T) { "SECRET_KEY", "DATABASE_URL", "API_TOKEN", + "BUILDKITE_GIT_CLONE_FLAGS", + "BUILDKITE_GIT_SUBMODULES", + "BUILDKITE_SKIP_CHECKOUT", "BUILDKITE_BRANCH", // This is a standard build env var, not protected "BUILDKITE_COMMIT", // This is a standard build env var, not protected "BUILDKITE_MESSAGE", // This is a standard build env var, not protected @@ -69,3 +69,70 @@ func TestIsProtected_Normalised(t *testing.T) { t.Errorf("IsProtected(%q) = %t, want %t", name, got, want) } } + +func TestCheckoutOverrideScope(t *testing.T) { + t.Parallel() + + scoped := []string{ + "BUILDKITE_GIT_CHECKOUT_FLAGS", + "BUILDKITE_GIT_CLONE_FLAGS", + "BUILDKITE_GIT_CLONE_MIRROR_FLAGS", + "BUILDKITE_GIT_CLEAN_FLAGS", + "BUILDKITE_GIT_FETCH_FLAGS", + "BUILDKITE_GIT_MIRRORS_SKIP_UPDATE", + "BUILDKITE_GIT_SUBMODULES", + "BUILDKITE_GIT_SKIP_FETCH_EXISTING_COMMITS", + "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS", + "BUILDKITE_SKIP_CHECKOUT", + } + + // These vars are conditionally locked: scoped, write-protected only when + // no-checkout-override is enabled. (Disjointness from protectedEnv is + // covered by TestProtectedAndCheckoutScopeDisjoint.) + for _, envVar := range scoped { + if got := IsCheckoutOverrideScoped(envVar); !got { + t.Errorf("IsCheckoutOverrideScoped(%q) = false, want true", envVar) + } + } + + // Mirror infra is agent-only: always protected and never in the override + // scope, so a job can't relocate the mirror, stretch its lock timeout, or + // change the mirror checkout mode. Submodule clone config is likewise + // always protected (a `git -c` injection vector with no backend knob). + for _, envVar := range []string{ + "BUILDKITE_GIT_MIRRORS_PATH", + "BUILDKITE_GIT_MIRRORS_LOCK_TIMEOUT", + "BUILDKITE_GIT_MIRROR_CHECKOUT_MODE", + "BUILDKITE_GIT_SUBMODULE_CLONE_CONFIG", + "BUILDKITE_SSH_KEYSCAN", + "BUILDKITE_COMMAND_EVAL", + } { + if got := IsProtected(envVar); !got { + t.Errorf("IsProtected(%q) = false, want true", envVar) + } + if got := IsCheckoutOverrideScoped(envVar); got { + t.Errorf("IsCheckoutOverrideScoped(%q) = true, want false", envVar) + } + } + + // An unrecognised var is neither protected nor checkout-scoped. + if IsProtected("MY_CUSTOM_VAR") { + t.Errorf("IsProtected(MY_CUSTOM_VAR) = true, want false") + } + if IsCheckoutOverrideScoped("MY_CUSTOM_VAR") { + t.Errorf("IsCheckoutOverrideScoped(MY_CUSTOM_VAR) = true, want false") + } +} + +func TestProtectedAndCheckoutScopeDisjoint(t *testing.T) { + t.Parallel() + + // A var must sit in exactly one tier. The enforcement sites read the two + // maps through different predicates, so a var in both would be locked + // inconsistently across hooks, secrets, the Job API, and config refresh. + for name := range checkoutOverrideScope { + if _, ok := protectedEnv[name]; ok { + t.Errorf("%q is in both protectedEnv and checkoutOverrideScope; it must sit in exactly one tier", name) + } + } +} diff --git a/internal/job/api.go b/internal/job/api.go index 1de01ef969..a0f1872774 100644 --- a/internal/job/api.go +++ b/internal/job/api.go @@ -36,6 +36,9 @@ We'll continue to run your job, but you won't be able to use the Job API`) if e.Debug { jobAPIOpts = append(jobAPIOpts, jobapi.WithDebug()) } + if e.NoCheckoutOverride { + jobAPIOpts = append(jobAPIOpts, jobapi.WithNoCheckoutOverride()) + } srv, token, err := jobapi.NewServer(e.shell.Logger, socketPath, e.shell.Env, e.redactors, jobAPIOpts...) if err != nil { return cleanup, fmt.Errorf("creating job API server: %w", err) diff --git a/internal/job/config.go b/internal/job/config.go index 3e88c572d0..9311e8184a 100644 --- a/internal/job/config.go +++ b/internal/job/config.go @@ -21,7 +21,7 @@ import ( // To add a new config option that is mapped from an environment variable, add a // struct tag, then don't forget to add a corresponding CLI flag over in the // clicommand/bootstrap.go(BootstrapConfig) struct, otherwise it won't work. -// Also check the protectedEnv map in env/protected.go. +// Also check protectedEnv and checkoutOverrideScope in env/protected.go. type ExecutorConfig struct { // The command to run @@ -90,6 +90,10 @@ type ExecutorConfig struct { // Skip git fetch if the commit already exists locally GitSkipFetchExistingCommits bool `env:"BUILDKITE_GIT_SKIP_FETCH_EXISTING_COMMITS"` + // Lock the agent's checkout settings so the job cannot override them. + // Intentionally has no env tag so hooks cannot disable it at runtime. + NoCheckoutOverride bool + // Timeout in seconds for the git checkout phase (0 means no timeout) GitCheckoutTimeout int `env:"BUILDKITE_GIT_CHECKOUT_TIMEOUT"` @@ -247,6 +251,10 @@ func (c *ExecutorConfig) ReadFromEnvironment(environ *env.Environment) map[strin // Find struct fields with env tag if tag := f.Tag.Get("env"); tag != "" && environ.Exists(tag) { + if c.NoCheckoutOverride && env.IsCheckoutOverrideScoped(tag) { + continue + } + newStr, _ := environ.Get(tag) switch v.Kind() { diff --git a/internal/job/config_test.go b/internal/job/config_test.go index de50fb52c1..fd4c217048 100644 --- a/internal/job/config_test.go +++ b/internal/job/config_test.go @@ -112,6 +112,46 @@ func TestReadFromEnvironmentIgnoresMalformedBooleans(t *testing.T) { } } +func TestReadFromEnvironmentDoesNotRefreshNoCheckoutOverride(t *testing.T) { + t.Parallel() + + config := &ExecutorConfig{NoCheckoutOverride: true} + environ := env.FromSlice([]string{"BUILDKITE_NO_CHECKOUT_OVERRIDE=false"}) + + changes := config.ReadFromEnvironment(environ) + if len(changes) != 0 { + t.Errorf("changes = %v, want none", changes) + } + if got, want := config.NoCheckoutOverride, true; got != want { + t.Errorf("config.NoCheckoutOverride = %t, want %t", got, want) + } +} + +func TestReadFromEnvironmentSkipsCheckoutScopedVarsWhenNoCheckoutOverrideEnabled(t *testing.T) { + t.Parallel() + + config := &ExecutorConfig{ + NoCheckoutOverride: true, + SkipCheckout: false, + GitCloneFlags: "-v", + } + environ := env.FromSlice([]string{ + "BUILDKITE_SKIP_CHECKOUT=true", + "BUILDKITE_GIT_CLONE_FLAGS=--mirror", + }) + + changes := config.ReadFromEnvironment(environ) + if len(changes) != 0 { + t.Errorf("changes = %v, want none", changes) + } + if got, want := config.SkipCheckout, false; got != want { + t.Errorf("config.SkipCheckout = %t, want %t", got, want) + } + if got, want := config.GitCloneFlags, "-v"; got != want { + t.Errorf("config.GitCloneFlags = %q, want %q", got, want) + } +} + func TestGitSubmodulesBidirectionalControl(t *testing.T) { t.Parallel() diff --git a/internal/job/executor.go b/internal/job/executor.go index 629b8239a9..7a98cc4c2a 100644 --- a/internal/job/executor.go +++ b/internal/job/executor.go @@ -648,7 +648,8 @@ func (e *Executor) applyEnvironmentChanges(changes hook.EnvChanges) { // breaks the rest of the job. var protected []string for k := range changes.Diff.Keys { - if env.IsProtectedFromWithinJob(k) { + if env.IsProtectedFromWithinJob(k) || + (e.NoCheckoutOverride && env.IsCheckoutOverrideScoped(k)) { protected = append(protected, k) changes.Diff.Remove(k) } @@ -991,6 +992,9 @@ func (e *Executor) fetchAndSetSecrets(ctx context.Context) error { if env.IsProtected(pipelineSecret.EnvironmentVariable) { return fmt.Errorf("secret %q cannot set protected environment variable %q", pipelineSecret.Key, pipelineSecret.EnvironmentVariable) } + if e.NoCheckoutOverride && env.IsCheckoutOverrideScoped(pipelineSecret.EnvironmentVariable) { + return fmt.Errorf("secret %q cannot set checkout-locked environment variable %q while BUILDKITE_NO_CHECKOUT_OVERRIDE is enabled", pipelineSecret.Key, pipelineSecret.EnvironmentVariable) + } var alreadySet bool _, alreadySet = e.shell.Env.Get(pipelineSecret.EnvironmentVariable) diff --git a/internal/job/integration/hooks_integration_test.go b/internal/job/integration/hooks_integration_test.go index 7b05f91cb5..a8339651ff 100644 --- a/internal/job/integration/hooks_integration_test.go +++ b/internal/job/integration/hooks_integration_test.go @@ -120,6 +120,190 @@ func TestHooksCanUnsetEnvironmentVariables(t *testing.T) { tester.RunAndCheck(t, "MY_CUSTOM_ENV=1") } +func TestEnvironmentHookNoCheckoutOverride(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + envVar string + envValue string + noCheckoutOverride bool + wantEnv string + wantBlockedWarning bool + }{ + { + name: "disabled_allows_skip_checkout", + envVar: "BUILDKITE_SKIP_CHECKOUT", + envValue: "true", + wantEnv: "true", + }, + { + name: "enabled_blocks_skip_checkout", + envVar: "BUILDKITE_SKIP_CHECKOUT", + envValue: "true", + noCheckoutOverride: true, + wantBlockedWarning: true, + }, + { + // Sparse paths is only exercised in the blocked direction: the lock + // strips it before checkout, so the real checkout is unaffected. + name: "enabled_blocks_sparse_checkout_paths", + envVar: "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS", + envValue: "a/b", + noCheckoutOverride: true, + wantBlockedWarning: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + tester, err := NewExecutorTester(mainCtx) + if err != nil { + t.Fatalf("NewExecutorTester() error = %v", err) + } + defer tester.Close() + + filename := "environment" + script := []string{ + "#!/usr/bin/env bash", + fmt.Sprintf("export %s=%s", tc.envVar, tc.envValue), + } + if runtime.GOOS == "windows" { + filename = "environment.bat" + script = []string{ + "@echo off", + fmt.Sprintf("set %s=%s", tc.envVar, tc.envValue), + } + } + + if err := os.WriteFile(filepath.Join(tester.HooksDir, filename), []byte(strings.Join(script, "\n")), 0o700); err != nil { + t.Fatalf("os.WriteFile(%q, script, 0o700) = %v", filename, err) + } + + tester.ExpectGlobalHook("command").Once().AndExitWith(0).AndCallFunc(func(c *bintest.Call) { + if got, want := c.GetEnv(tc.envVar), tc.wantEnv; got != want { + _, _ = fmt.Fprintf(c.Stderr, "Expected %s=%q, got %q\n", tc.envVar, want, got) + c.Exit(1) + return + } + c.Exit(0) + }) + + env := []string{} + if tc.noCheckoutOverride { + env = append(env, "BUILDKITE_NO_CHECKOUT_OVERRIDE=true") + } + + tester.RunAndCheck(t, env...) + + containsWarning := strings.Contains(tester.Output, "env vars were blocked") && + strings.Contains(tester.Output, tc.envVar) + if containsWarning != tc.wantBlockedWarning { + t.Fatalf("blocked warning presence = %t, want %t\noutput: %s", containsWarning, tc.wantBlockedWarning, tester.Output) + } + }) + } +} + +func TestEnvironmentHookCannotDisableNoCheckoutOverride(t *testing.T) { + t.Parallel() + + // A hook must not be able to turn the lock off mid-job: exporting + // BUILDKITE_NO_CHECKOUT_OVERRIDE=false should be ignored, so a checkout- + // scoped var the same hook tries to set stays blocked. + tester, err := NewExecutorTester(mainCtx) + if err != nil { + t.Fatalf("NewExecutorTester() error = %v", err) + } + defer tester.Close() + + filename := "environment" + script := []string{ + "#!/usr/bin/env bash", + "export BUILDKITE_NO_CHECKOUT_OVERRIDE=false", + "export BUILDKITE_SKIP_CHECKOUT=true", + } + if runtime.GOOS == "windows" { + filename = "environment.bat" + script = []string{ + "@echo off", + "set BUILDKITE_NO_CHECKOUT_OVERRIDE=false", + "set BUILDKITE_SKIP_CHECKOUT=true", + } + } + + if err := os.WriteFile(filepath.Join(tester.HooksDir, filename), []byte(strings.Join(script, "\n")), 0o700); err != nil { + t.Fatalf("os.WriteFile(%q, script, 0o700) = %v", filename, err) + } + + tester.ExpectGlobalHook("command").Once().AndExitWith(0).AndCallFunc(func(c *bintest.Call) { + if got := c.GetEnv("BUILDKITE_SKIP_CHECKOUT"); got == "true" { + _, _ = fmt.Fprintf(c.Stderr, "BUILDKITE_SKIP_CHECKOUT=%q, want the lock to stay on and block it\n", got) + c.Exit(1) + return + } + c.Exit(0) + }) + + tester.RunAndCheck(t, "BUILDKITE_NO_CHECKOUT_OVERRIDE=true") + + // Both the lock var itself and the scoped var should be reported as blocked. + for _, want := range []string{"BUILDKITE_NO_CHECKOUT_OVERRIDE", "BUILDKITE_SKIP_CHECKOUT"} { + if !strings.Contains(tester.Output, "env vars were blocked") || !strings.Contains(tester.Output, want) { + t.Fatalf("output did not report %q as blocked\noutput: %s", want, tester.Output) + } + } +} + +func TestNoCommandEvalAutoLocksCheckoutVars(t *testing.T) { + t.Parallel() + + // no-command-eval implies no-checkout-override: with command eval disabled + // the agent must lock checkout vars so git flags cannot be used to bypass + // the no-command-eval protection. The job never sets + // BUILDKITE_NO_CHECKOUT_OVERRIDE; the lock is derived purely from + // command-eval being off. + tester, err := NewExecutorTester(mainCtx) + if err != nil { + t.Fatalf("NewExecutorTester() error = %v", err) + } + defer tester.Close() + + filename := "environment" + script := []string{ + "#!/usr/bin/env bash", + "export BUILDKITE_SKIP_CHECKOUT=true", + } + if runtime.GOOS == "windows" { + filename = "environment.bat" + script = []string{ + "@echo off", + "set BUILDKITE_SKIP_CHECKOUT=true", + } + } + + if err := os.WriteFile(filepath.Join(tester.HooksDir, filename), []byte(strings.Join(script, "\n")), 0o700); err != nil { + t.Fatalf("os.WriteFile(%q, script, 0o700) = %v", filename, err) + } + + tester.ExpectGlobalHook("command").Once().AndExitWith(0).AndCallFunc(func(c *bintest.Call) { + if got := c.GetEnv("BUILDKITE_SKIP_CHECKOUT"); got == "true" { + _, _ = fmt.Fprintf(c.Stderr, "BUILDKITE_SKIP_CHECKOUT=%q, want the no-command-eval lock to block it\n", got) + c.Exit(1) + return + } + c.Exit(0) + }) + + tester.RunAndCheck(t, "BUILDKITE_COMMAND_EVAL=false") + + if !strings.Contains(tester.Output, "env vars were blocked") || !strings.Contains(tester.Output, "BUILDKITE_SKIP_CHECKOUT") { + t.Fatalf("expected BUILDKITE_SKIP_CHECKOUT to be reported as blocked\noutput: %s", tester.Output) + } +} + func TestDirectoryPassesBetweenHooks(t *testing.T) { t.Parallel() diff --git a/internal/job/integration/plugin_integration_test.go b/internal/job/integration/plugin_integration_test.go index 344a48c563..c587422973 100644 --- a/internal/job/integration/plugin_integration_test.go +++ b/internal/job/integration/plugin_integration_test.go @@ -78,6 +78,89 @@ func TestRunningPlugins(t *testing.T) { tester.RunAndCheck(t, env...) } +func TestPluginEnvironmentHookNoCheckoutOverride(t *testing.T) { + t.Parallel() + + // A plugin's environment hook (e.g. git-clean) may set checkout flags by + // default, but no-checkout-override must block that mutation. + tests := []struct { + name string + envVar string + envValue string + noCheckoutOverride bool + wantBlocked bool + }{ + {name: "disabled_allows_plugin_to_override_skip_checkout", envVar: "BUILDKITE_SKIP_CHECKOUT", envValue: "true"}, + {name: "enabled_blocks_plugin_skip_checkout_override", envVar: "BUILDKITE_SKIP_CHECKOUT", envValue: "true", noCheckoutOverride: true, wantBlocked: true}, + {name: "enabled_blocks_plugin_sparse_checkout_paths_override", envVar: "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS", envValue: "a/b", noCheckoutOverride: true, wantBlocked: true}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + tester, err := NewExecutorTester(mainCtx) + if err != nil { + t.Fatalf("NewExecutorTester() error = %v", err) + } + defer tester.Close() + + // These scoped vars keep the checkout safe: SKIP_CHECKOUT just skips + // cloning, and sparse paths is only exercised in the blocked direction + // where the lock strips it before checkout. Setting a real clone flag + // in the allowed direction would break the checkout. + hooks := map[string][]string{ + "environment": { + "#!/usr/bin/env bash", + fmt.Sprintf("export %s=%s", tc.envVar, tc.envValue), + }, + } + if runtime.GOOS == "windows" { + hooks = map[string][]string{ + "environment.bat": { + "@echo off", + fmt.Sprintf("set %s=%s", tc.envVar, tc.envValue), + }, + } + } + + p := createTestPlugin(t, hooks) + pluginJSON, err := p.ToJSON() + if err != nil { + t.Fatalf("testPlugin.ToJSON() error = %v", err) + } + + tester.ExpectGlobalHook("command").Once().AndExitWith(0).AndCallFunc(func(c *bintest.Call) { + got := c.GetEnv(tc.envVar) + if tc.wantBlocked && got == tc.envValue { + _, _ = fmt.Fprintf(c.Stderr, "%s=%q, want the plugin override to be blocked\n", tc.envVar, got) + c.Exit(1) + return + } + if !tc.wantBlocked && got != tc.envValue { + _, _ = fmt.Fprintf(c.Stderr, "%s=%q, want %q\n", tc.envVar, got, tc.envValue) + c.Exit(1) + return + } + c.Exit(0) + }) + + env := []string{"BUILDKITE_PLUGINS=" + pluginJSON} + if tc.noCheckoutOverride { + env = append(env, "BUILDKITE_NO_CHECKOUT_OVERRIDE=true") + } + + tester.RunAndCheck(t, env...) + + containsWarning := strings.Contains(tester.Output, "env vars were blocked") && + strings.Contains(tester.Output, tc.envVar) + if containsWarning != tc.wantBlocked { + t.Fatalf("blocked warning presence = %t, want %t\noutput: %s", containsWarning, tc.wantBlocked, tester.Output) + } + }) + } +} + func TestExitCodesPropagateOutFromPlugins(t *testing.T) { t.Parallel() diff --git a/internal/job/integration/secrets_integration_test.go b/internal/job/integration/secrets_integration_test.go index 192d893b37..6d8f54b5b0 100644 --- a/internal/job/integration/secrets_integration_test.go +++ b/internal/job/integration/secrets_integration_test.go @@ -563,3 +563,79 @@ func TestSecretsIntegration_ProtectedEnvironmentVariableRejection(t *testing.T) } } } + +func TestSecretsIntegration_NoCheckoutOverride(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + envVar string + secretValue string + noCheckoutOverride bool + wantErr bool + }{ + {name: "disabled_allows_checkout_scoped_secret", envVar: "BUILDKITE_GIT_CLONE_FLAGS", secretValue: "--mirror"}, + {name: "enabled_rejects_checkout_locked_secret", envVar: "BUILDKITE_GIT_CLONE_FLAGS", secretValue: "--mirror", noCheckoutOverride: true, wantErr: true}, + {name: "enabled_rejects_sparse_checkout_paths_secret", envVar: "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS", secretValue: "a/b", noCheckoutOverride: true, wantErr: true}, + // The lock must not over-block: a non-checkout secret is still allowed. + {name: "enabled_allows_unscoped_secret", envVar: "MY_CUSTOM_VAR", secretValue: "hello", noCheckoutOverride: true}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + tester, err := NewExecutorTester(mainCtx) + if err != nil { + t.Fatalf("setting up executor tester: %v", err) + } + defer tester.Close() + + apiServer := setupSecretsAPIServer(t, map[string]string{"CHECKOUT_SECRET": tc.secretValue}) + defer apiServer.Close() + + secretsJSON, err := json.Marshal([]pipeline.Secret{{ + Key: "CHECKOUT_SECRET", + EnvironmentVariable: tc.envVar, + }}) + if err != nil { + t.Fatalf("marshaling secrets: %v", err) + } + + if !tc.wantErr { + tester.ExpectGlobalHook("command").AndCallFunc(func(c *bintest.Call) { + if got, want := c.GetEnv(tc.envVar), tc.secretValue; got != want { + _, _ = fmt.Fprintf(c.Stderr, "Expected %s=%q, got %q\n", tc.envVar, want, got) + c.Exit(1) + return + } + c.Exit(0) + }) + } + + env := []string{ + fmt.Sprintf("BUILDKITE_SECRETS_CONFIG=%s", string(secretsJSON)), + fmt.Sprintf("BUILDKITE_AGENT_ENDPOINT=%s", apiServer.URL), + } + if tc.noCheckoutOverride { + env = append(env, "BUILDKITE_NO_CHECKOUT_OVERRIDE=true") + } + + err = tester.Run(t, env...) + if tc.wantErr { + if err == nil { + t.Fatalf("expected job to fail due to checkout-locked secret mapping, but it succeeded. Full output: %s", tester.Output) + } + if !strings.Contains(tester.Output, "checkout-locked environment variable") || !strings.Contains(tester.Output, tc.envVar) { + t.Fatalf("expected output to mention checkout-locked env rejection, got: %s", tester.Output) + } + return + } + + if err != nil { + t.Fatalf("running executor tester: %v", err) + } + tester.CheckMocks(t) + }) + } +} diff --git a/jobapi/env.go b/jobapi/env.go index ee96aeb4fb..b6158c3044 100644 --- a/jobapi/env.go +++ b/jobapi/env.go @@ -36,12 +36,12 @@ func (s *Server) patchEnv(w http.ResponseWriter, r *http.Request) { added := make([]string, 0, len(req.Env)) updated := make([]string, 0, len(req.Env)) - protected := checkProtected(slices.Collect(maps.Keys(req.Env))) + protected := s.checkProtected(slices.Collect(maps.Keys(req.Env))) if len(protected) > 0 { err := socket.WriteError( w, - fmt.Sprintf("the following environment variables are protected, and cannot be modified: % v", protected), + s.protectedEnvError(protected), http.StatusUnprocessableEntity, ) if err != nil { @@ -106,11 +106,11 @@ func (s *Server) deleteEnv(w http.ResponseWriter, r *http.Request) { return } - protected := checkProtected(req.Keys) + protected := s.checkProtected(req.Keys) if len(protected) > 0 { err := socket.WriteError( w, - fmt.Sprintf("the following environment variables are protected, and cannot be modified: % v", protected), + s.protectedEnvError(protected), http.StatusUnprocessableEntity, ) if err != nil { @@ -138,14 +138,31 @@ func (s *Server) deleteEnv(w http.ResponseWriter, r *http.Request) { } } -func checkProtected(candidates []string) []string { +func (s *Server) checkProtected(candidates []string) []string { protected := make([]string, 0, len(candidates)) for _, c := range candidates { // The Job API is only accessible from within the job, so allow writes // to vars that allow write from within job. - if env.IsProtectedFromWithinJob(c) { + if env.IsProtectedFromWithinJob(c) || + (s.noCheckoutOverride && env.IsCheckoutOverrideScoped(c)) { protected = append(protected, c) } } return protected } + +// protectedEnvError builds the rejection message for protected candidates, +// noting when the rejection is due to the no-checkout-override lock rather than +// an always-protected var. +func (s *Server) protectedEnvError(protected []string) string { + msg := fmt.Sprintf("the following environment variables are protected, and cannot be modified: % v", protected) + if s.noCheckoutOverride { + for _, p := range protected { + if env.IsCheckoutOverrideScoped(p) { + msg += ". Checkout-related variables are locked because BUILDKITE_NO_CHECKOUT_OVERRIDE is enabled" + break + } + } + } + return msg +} diff --git a/jobapi/server.go b/jobapi/server.go index a67c8589b1..d6c34f6153 100644 --- a/jobapi/server.go +++ b/jobapi/server.go @@ -22,6 +22,12 @@ func WithDebug() ServerOpts { } } +func WithNoCheckoutOverride() ServerOpts { + return func(s *Server) { + s.noCheckoutOverride = true + } +} + // WithPromiseFailureDeclarer sets the function the /promise-failure endpoint // uses to declare promised failures to the Buildkite API. Debouncing means it's // called at most once per successfully-declared exit status. If unset (e.g. in @@ -42,9 +48,10 @@ type PromiseFailureDeclarer func(ctx context.Context, exitStatus int, reason str // and allows jobs to introspect and mutate their own state type Server struct { // SocketPath is the path to the socket that the server is (or will be) listening on - SocketPath string - Logger shell.Logger - debug bool + SocketPath string + Logger shell.Logger + debug bool + noCheckoutOverride bool mtx sync.RWMutex environ *env.Environment diff --git a/jobapi/server_test.go b/jobapi/server_test.go index 77e49f8a37..b16f559b73 100644 --- a/jobapi/server_test.go +++ b/jobapi/server_test.go @@ -37,6 +37,12 @@ func testEnviron() *env.Environment { return e } +func testEnvironWith(key, value string) *env.Environment { + e := testEnviron() + e.Set(key, value) + return e +} + func testServer(t *testing.T, e *env.Environment, mux *replacer.Mux, opts ...jobapi.ServerOpts) (*jobapi.Server, string, error) { sockName, err := jobapi.NewSocketPath(os.TempDir()) if err != nil { @@ -356,6 +362,200 @@ func TestPatchEnv(t *testing.T) { } } +func TestPatchEnvAllowsCheckoutScopedVarsWhenNoCheckoutOverrideDisabled(t *testing.T) { + t.Parallel() + + environ := testEnvironWith("BUILDKITE_GIT_CLONE_FLAGS", "-v") + srv, token, err := testServer(t, environ, replacer.NewMux()) + if err != nil { + t.Fatalf("creating server: %v", err) + } + + if err := srv.Start(); err != nil { + t.Fatalf("starting server: %v", err) + } + defer func() { + if err := srv.Stop(); err != nil { + t.Fatalf("stopping server: %v", err) + } + }() + + buf := bytes.NewBuffer(nil) + if err := json.NewEncoder(buf).Encode(&jobapi.EnvUpdateRequestPayload{ + Env: map[string]*string{"BUILDKITE_GIT_CLONE_FLAGS": pt("--mirror")}, + }); err != nil { + t.Fatalf("encoding request body: %v", err) + } + + req, err := http.NewRequest(http.MethodPatch, "http://job/api/current-job/v0/env", buf) + if err != nil { + t.Fatalf("creating request: %v", err) + } + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) + + testAPI(t, environ, req, testSocketClient(srv.SocketPath), apiTestCase[jobapi.EnvUpdateRequestPayload, jobapi.EnvUpdateResponse]{ + expectedStatus: http.StatusOK, + expectedEnv: testEnvironWith("BUILDKITE_GIT_CLONE_FLAGS", "--mirror").Dump(), + }) +} + +func TestPatchEnvRejectsCheckoutScopedVarsWhenNoCheckoutOverrideEnabled(t *testing.T) { + t.Parallel() + + environ := testEnvironWith("BUILDKITE_SKIP_CHECKOUT", "false") + srv, token, err := testServer(t, environ, replacer.NewMux(), jobapi.WithNoCheckoutOverride()) + if err != nil { + t.Fatalf("creating server: %v", err) + } + + if err := srv.Start(); err != nil { + t.Fatalf("starting server: %v", err) + } + defer func() { + if err := srv.Stop(); err != nil { + t.Fatalf("stopping server: %v", err) + } + }() + + buf := bytes.NewBuffer(nil) + if err := json.NewEncoder(buf).Encode(&jobapi.EnvUpdateRequestPayload{ + Env: map[string]*string{"BUILDKITE_SKIP_CHECKOUT": pt("true")}, + }); err != nil { + t.Fatalf("encoding request body: %v", err) + } + + req, err := http.NewRequest(http.MethodPatch, "http://job/api/current-job/v0/env", buf) + if err != nil { + t.Fatalf("creating request: %v", err) + } + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) + + testAPI(t, environ, req, testSocketClient(srv.SocketPath), apiTestCase[jobapi.EnvUpdateRequestPayload, jobapi.EnvUpdateResponse]{ + expectedStatus: http.StatusUnprocessableEntity, + expectedError: &jobapi.ErrorResponse{ + Error: "the following environment variables are protected, and cannot be modified: [BUILDKITE_SKIP_CHECKOUT]. Checkout-related variables are locked because BUILDKITE_NO_CHECKOUT_OVERRIDE is enabled", + }, + expectedEnv: testEnvironWith("BUILDKITE_SKIP_CHECKOUT", "false").Dump(), + }) +} + +func TestDeleteEnvRejectsCheckoutScopedVarsWhenNoCheckoutOverrideEnabled(t *testing.T) { + t.Parallel() + + environ := testEnvironWith("BUILDKITE_SKIP_CHECKOUT", "false") + srv, token, err := testServer(t, environ, replacer.NewMux(), jobapi.WithNoCheckoutOverride()) + if err != nil { + t.Fatalf("creating server: %v", err) + } + + if err := srv.Start(); err != nil { + t.Fatalf("starting server: %v", err) + } + defer func() { + if err := srv.Stop(); err != nil { + t.Fatalf("stopping server: %v", err) + } + }() + + buf := bytes.NewBuffer(nil) + if err := json.NewEncoder(buf).Encode(&jobapi.EnvDeleteRequest{Keys: []string{"BUILDKITE_SKIP_CHECKOUT"}}); err != nil { + t.Fatalf("encoding request body: %v", err) + } + + req, err := http.NewRequest(http.MethodDelete, "http://job/api/current-job/v0/env", buf) + if err != nil { + t.Fatalf("creating request: %v", err) + } + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) + + testAPI(t, environ, req, testSocketClient(srv.SocketPath), apiTestCase[jobapi.EnvDeleteRequest, jobapi.EnvDeleteResponse]{ + expectedStatus: http.StatusUnprocessableEntity, + expectedError: &jobapi.ErrorResponse{ + Error: "the following environment variables are protected, and cannot be modified: [BUILDKITE_SKIP_CHECKOUT]. Checkout-related variables are locked because BUILDKITE_NO_CHECKOUT_OVERRIDE is enabled", + }, + expectedEnv: testEnvironWith("BUILDKITE_SKIP_CHECKOUT", "false").Dump(), + }) +} + +func TestPatchEnvRejectsSparseCheckoutPathsWhenNoCheckoutOverrideEnabled(t *testing.T) { + t.Parallel() + + environ := testEnvironWith("BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS", "a/b") + srv, token, err := testServer(t, environ, replacer.NewMux(), jobapi.WithNoCheckoutOverride()) + if err != nil { + t.Fatalf("creating server: %v", err) + } + + if err := srv.Start(); err != nil { + t.Fatalf("starting server: %v", err) + } + defer func() { + if err := srv.Stop(); err != nil { + t.Fatalf("stopping server: %v", err) + } + }() + + buf := bytes.NewBuffer(nil) + if err := json.NewEncoder(buf).Encode(&jobapi.EnvUpdateRequestPayload{ + Env: map[string]*string{"BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS": pt("c/d")}, + }); err != nil { + t.Fatalf("encoding request body: %v", err) + } + + req, err := http.NewRequest(http.MethodPatch, "http://job/api/current-job/v0/env", buf) + if err != nil { + t.Fatalf("creating request: %v", err) + } + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) + + testAPI(t, environ, req, testSocketClient(srv.SocketPath), apiTestCase[jobapi.EnvUpdateRequestPayload, jobapi.EnvUpdateResponse]{ + expectedStatus: http.StatusUnprocessableEntity, + expectedError: &jobapi.ErrorResponse{ + Error: "the following environment variables are protected, and cannot be modified: [BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS]. Checkout-related variables are locked because BUILDKITE_NO_CHECKOUT_OVERRIDE is enabled", + }, + expectedEnv: testEnvironWith("BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS", "a/b").Dump(), + }) +} + +func TestPatchEnvAllowsUnscopedVarsWhenNoCheckoutOverrideEnabled(t *testing.T) { + t.Parallel() + + // The lock must not over-block: a normal, non-checkout var stays writable + // while BUILDKITE_NO_CHECKOUT_OVERRIDE is enabled. + environ := testEnviron() + srv, token, err := testServer(t, environ, replacer.NewMux(), jobapi.WithNoCheckoutOverride()) + if err != nil { + t.Fatalf("creating server: %v", err) + } + + if err := srv.Start(); err != nil { + t.Fatalf("starting server: %v", err) + } + defer func() { + if err := srv.Stop(); err != nil { + t.Fatalf("stopping server: %v", err) + } + }() + + buf := bytes.NewBuffer(nil) + if err := json.NewEncoder(buf).Encode(&jobapi.EnvUpdateRequestPayload{ + Env: map[string]*string{"MY_CUSTOM_VAR": pt("hello")}, + }); err != nil { + t.Fatalf("encoding request body: %v", err) + } + + req, err := http.NewRequest(http.MethodPatch, "http://job/api/current-job/v0/env", buf) + if err != nil { + t.Fatalf("creating request: %v", err) + } + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) + + testAPI(t, environ, req, testSocketClient(srv.SocketPath), apiTestCase[jobapi.EnvUpdateRequestPayload, jobapi.EnvUpdateResponse]{ + expectedStatus: http.StatusOK, + expectedEnv: testEnvironWith("MY_CUSTOM_VAR", "hello").Dump(), + }) +} + func TestGetEnv(t *testing.T) { t.Parallel()