Skip to content

Add checkout-override-mode option#4010

Open
petetomasik wants to merge 69 commits into
mainfrom
feat/no-checkout-override
Open

Add checkout-override-mode option#4010
petetomasik wants to merge 69 commits into
mainfrom
feat/no-checkout-override

Conversation

@petetomasik

@petetomasik petetomasik commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Description

The agent exposes a number of checkout-related environment variables (git clone/fetch/checkout/clean flags, sparse-checkout paths, submodules, skip-fetch-existing-commits, skip-checkout). Historically the protections around these were implicit and inconsistent: some were silently overwritten by agent config in createEnvironment, others were overridable from the backend job env, hooks, plugins, the Job API, or secrets, with the rules spread across a single protectedEnv map and scattered logic. Because git flags are a shell-injection vector (for example --upload-pack, -c core.sshCommand, ext:: transports), letting a job set them can be equivalent to arbitrary command execution and a way to bypass no-command-eval.

This PR replaces that implicit handling with an explicit, tri-state --checkout-override-mode flag (env BUILDKITE_CHECKOUT_OVERRIDE_MODE), shared by agent start and bootstrap, controlling which sources may override the agent's checkout-scoped configuration:

  • strict — the agent is authoritative against every source: pipeline/step env, secrets, hooks, plugins, and the Job API.
  • from-job (default) — hooks, plugins, and the Job API may set checkout-scoped vars (so a job can still tailor its own checkout), but the backend job env (pipeline/step env) and secrets may not. This matches the agent's historical behavior.
  • none — any source, including the backend job env and secrets, may set them.

Disabling command evaluation via no-command-eval=true forces the mode to strict regardless of the configured value, so git flags can never be used to undermine no-command-eval.

The protection model lives in env/protected.go and is split into two disjoint tiers (disjointness enforced by a test):

  • protectedEnv — always agent-authoritative in every mode: agent infrastructure, all mirror configuration (BUILDKITE_GIT_MIRRORS_PATH, BUILDKITE_GIT_MIRRORS_LOCK_TIMEOUT, BUILDKITE_GIT_MIRRORS_SKIP_UPDATE, BUILDKITE_GIT_MIRROR_CHECKOUT_MODE, BUILDKITE_GIT_CLONE_MIRROR_FLAGS), and BUILDKITE_GIT_SUBMODULE_CLONE_CONFIG.
  • checkoutOverrideScope — the checkout-scoped vars whose lock depends on the mode: BUILDKITE_GIT_CHECKOUT_FLAGS, BUILDKITE_GIT_CHECKOUT_TIMEOUT, BUILDKITE_GIT_CLEAN_FLAGS, BUILDKITE_GIT_CLONE_FLAGS, BUILDKITE_GIT_FETCH_FLAGS, BUILDKITE_GIT_SKIP_FETCH_EXISTING_COMMITS, BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS, BUILDKITE_GIT_SUBMODULES, BUILDKITE_SKIP_CHECKOUT.

Context

Linear

Changes

New CLI flag, shared by agent start and bootstrap:

--checkout-override-mode value   Controls which sources may override the agent's checkout settings;
                                 one of [strict from-job none]. 'strict' makes the agent authoritative
                                 against pipeline/step env, secrets, hooks, plugins, and the Job API.
                                 'from-job' (default) lets hooks, plugins, and the Job API set checkout
                                 vars, but blocks pipeline/step env and secrets. 'none' additionally
                                 lets pipeline/step env and secrets set them. All mirror configuration
                                 and submodule clone config stay agent-authoritative in every mode. The
                                 checkout SSH key and Git LFS toggle are not governed by this flag and
                                 stay job-settable in every mode. Disabling command-eval forces this to
                                 'strict'.
                                 (default: "from-job") [$BUILDKITE_CHECKOUT_OVERRIDE_MODE]
  • Adds a CheckoutOverrideMode type in env (strict/from-job/none) with ParseCheckoutOverrideMode, String, and RestrictedForCommandEval (forces strict when command-eval is off).
  • Wires the mode through AgentConfiguration, AgentStartConfig, BootstrapConfig, ExecutorConfig, and the Job API server (WithCheckoutOverrideMode), via a shared resolveCheckoutOverrideMode helper that applies the command-eval floor. ExecutorConfig.CheckoutOverrideMode has no env tag, so a hook cannot relax the mode mid-job.
  • Splits env/protected.go into protectedEnv (always protected) and checkoutOverrideScope (mode-dependent), and adds predicates IsCheckoutOverrideScoped, IsCheckoutLocked (within-job sources: hooks, plugins, and the Job API; strict only), and IsCheckoutLockedForSecrets (secrets; every mode except none). The backend job env follows the same rule as secrets (locked in every mode except none), enforced in createEnvironment. The two maps are asserted disjoint.
  • Enforces the mode at every point a job can reach checkout config: backend job-env precedence in createEnvironment (setCheckoutEnv), runtime config refresh in ReadFromEnvironment, hook env changes in applyEnvironmentChanges, secret-to-env mappings in fetchAndSetSecrets, and Job API patch/delete (checkProtected).
  • Makes all mirror configuration and BUILDKITE_GIT_SUBMODULE_CLONE_CONFIG always agent-authoritative, including moving BUILDKITE_GIT_MIRRORS_SKIP_UPDATE and BUILDKITE_GIT_CLONE_MIRROR_FLAGS out of the mode-dependent scope (the mirror is shared infrastructure and clone-mirror flags are a cross-job injection vector).
  • The Job API rejection message names the checkout-override lock when that is the reason for rejection.
  • Adds unit and integration coverage across env, internal/job config, the agent job runner, and the hooks/plugins/secrets/Job API paths, including: the job-env override matrix across all three modes, infra vars staying agent-authoritative even under none, secrets rejected for the protected checkout-infra vars, and an end-to-end test that disabling command-eval auto-forces strict.

Agent behavior changes

Under from-job (the default), checkout-scoped vars remain overridable from within-job sources (hooks, plugins, and the Job API) but not from the backend job env or secrets, matching the agent's historical behavior. strict offers a more protected behavior: hooks and plugins can no longer set the checkout-scoped or mirror vars (on main several were mutable from within the job). none is the only mode that opens checkout-scoped vars to the backend job env and secrets.

Testing

  • Tests have run locally (with go test ./...).
  • Code is formatted (with go tool gofumpt -extra -w .)

go test ./... passes locally, including the full checkout-override-mode unit and integration coverage across env, internal/job, agent, jobapi, and clicommand.

Disclosures / Credits

Code generated by Claude Code (Opus 4.8) under my guidance. Multiple reviews were conducted to establish the correctness and completeness of the implementation of this new agent config option and protection model. The core feature and its conversion from the earlier binary design to the tri-state mode were implemented in earlier revisions.

scadu and others added 30 commits April 27, 2026 12:06
Add setCheckoutEnv helper to export checkout-related vars when
no-checkout-override=false
# Conflicts:
#	clicommand/env_set.go
#	clicommand/env_unset.go
Add skip checkout tests for `no-checkout-override`
Mirror checkout mode affects the checkout but stayed mutable from hooks, plugins, the Job API, and secrets while the lock was on. Also add a disjointness test for the two protection maps and correct the scope test's protection assertions.
AgentStartConfig and BootstrapConfig use inverted command-eval conventions, so their zero values mean opposite things; per-type tables make that legible and drop the kind/switch discriminator.
The none mode help and comment claimed any source could override the
agent's checkout config. Mirror-infra vars and SUBMODULE_CLONE_CONFIG
stay agent-authoritative in every mode, so scope the wording to the
checkout-override-scoped vars and call out the always-locked exceptions.
The within-job tightening of SUBMODULE_CLONE_CONFIG and
MIRROR_CHECKOUT_MODE was only unit-tested via IsProtected. Add an
integration test where an environment hook is blocked from setting them
under the most permissive mode (none), proving they stay agent-
authoritative regardless of the checkout-override mode.
@petetomasik petetomasik marked this pull request as draft July 15, 2026 14:21
The mode enum iota is FromJob=0, Strict=1, None=2, so "floor at strict"
read as a numeric lower-bound clamp, but the helper unconditionally forces
strict when command-eval is disabled (its own comment said "raised").
Rename to RestrictedForCommandEval and align the "floors"->"forces"
wording in the resolver doc, config-helper comments, scope comment, and
flag usage.
The strict help text and CheckoutOverrideStrict doc claimed the agent is
authoritative against every source, but GIT_SSH_KEY and GIT_LFS_ENABLED are
governed by neither map (job-settable in every mode) and REPO/REFSPEC stay
mutableFromWithinJob. Scope the wording, document the intentional exclusions
next to checkoutOverrideScope, and add a test pinning them so a future change
that scopes or protects them has to update the expectation on purpose.
The alias was referenced only by the flag usage string and, unlike its
sibling mirrorCheckoutModes, never used for validation (that goes through
ParseCheckoutOverrideMode). Use env.CheckoutOverrideModeNames directly.
GIT_MIRROR_CHECKOUT_MODE and GIT_SUBMODULE_CLONE_CONFIG became fully
protectedEnv in this branch, but secret rejection was only covered
transitively via IsProtected. Add an integration test mapping a secret to
each (as the sole secret) under none, asserting rejection so a future move
into checkoutOverrideScope has to update the expectation.
GIT_CLONE_MIRROR_FLAGS and GIT_MIRRORS_SKIP_UPDATE were in
checkoutOverrideScope, so under none a pipeline or secret could set them.
CLONE_MIRROR_FLAGS is applied to the shared git clone --mirror, so a job
setting it is a cross-job injection vector. Mirrors are agent-local infra
the backend has no concept of, so move both to protectedEnv{} (agent-only in
every mode, matching the other mirror vars) and emit them via setEnv. Move
their integration cases to TestCheckoutInfraVarsAreAgentAuthoritative.
Follow-up to making clone-mirror-flags and mirrors-skip-update agent-only:
the flag help enumerated only path/lock-timeout/checkout-mode as the always
agent-authoritative mirror vars, which is now incomplete. Generalize to all
mirror configuration.
@petetomasik petetomasik changed the title Add no-checkout-override option Add checkout-override-mode option Jul 15, 2026

@buildsworth-bk-app buildsworth-bk-app Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn’t find any new issues in this pass. This is security-sensitive agent checkout/env protection logic, so it’s above my approval ceiling and I’m leaving the review as a comment.

Want to dig deeper?

The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:

Download the buildsworth logs from build 5323, then answer my questions about the findings.

@buildsworth-bk-app buildsworth-bk-app Bot removed the request for review from buildsworth-bk July 15, 2026 17:10
from-job was intended to match the agent's historical behaviour, which
blocked the backend job env (pipeline/step env) from setting checkout-scoped
vars. It was instead letting backend job env override them, reopening the
git-flag injection vector the mode exists to close. Lock the backend job env
in every mode except none (the same rule secrets follow); only within-job
sources (hooks, plugins, Job API) keep the strict-only lock.
@petetomasik petetomasik marked this pull request as ready for review July 15, 2026 17:38

@buildsworth-bk-app buildsworth-bk-app Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one blocking issue around allowed secret mappings not updating the checkout config, and left a question on the default timeout behavior.

Want to dig deeper?

The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:

Download the buildsworth logs from build 5326, then answer my questions about the findings.

Comment thread internal/job/executor.go
Comment thread agent/job_runner.go
Secrets are written to e.shell.Env in setUp before any hook runs, but the
checkout phase reads ExecutorConfig struct fields (GitCloneFlags,
SkipCheckout, GitSparseCheckoutPaths) that are only refreshed from env when
a hook triggers applyEnvironmentChanges. With no environment/plugin hook, a
checkout-scoped var a secret is allowed to set under none mode never reached
checkout. Refresh the config right after fetchAndSetSecrets so it takes
effect, calling ReadFromEnvironment directly to avoid logging the value.
The checkout-override lock force-emitted the submodules, skip-checkout,
skip-fetch, and checkout-timeout vars for both strict and from-job, so under
the default from-job mode a pipeline/step env value on the agent's silent side
was overwritten with the agent default. For the checkout timeout that meant an
unset agent config forced 0 (no timeout), a backward-incompatible change to the
default for a value that isn't a git-flag injection vector.

Restrict the unconditional force-emit to strict; from-job and none keep the
historical conditional emit. The checkout flags stay agent-authoritative under
from-job as before. Add from-job silent-side coverage and update the mode docs.
@buildsworth-bk-app buildsworth-bk-app Bot dismissed their stale review July 15, 2026 19:47

Previous concerns have been addressed.

@buildsworth-bk-app buildsworth-bk-app Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous secret-refresh and timeout concerns are addressed. I found one new blocker in the secret refresh path.

This is security-sensitive agent checkout/env protection logic, so it remains above my approval ceiling.

Want to dig deeper?

The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:

Download the buildsworth logs from build 5331, then answer my questions about the findings.

Comment thread internal/job/executor.go
ReadFromEnvironment runs over the full shell env, which now includes
secret-backed values (setUp refreshes config right after fetching secrets).
The bool/int/slice parse-error branches logged the raw value via the stdlib
logger, which bypasses the shell redactors, so a malformed secret-backed value
(e.g. an unparseable BUILDKITE_GIT_CHECKOUT_TIMEOUT, or BUILDKITE_GIT_LFS_ENABLED
which any source may set) could leak to the job log.

Log only the var name (and type) in those warnings.
@buildsworth-bk-app buildsworth-bk-app Bot dismissed their stale review July 15, 2026 20:14

Previous concerns have been addressed.

@buildsworth-bk-app buildsworth-bk-app Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous secret-refresh leak is fixed. I left one non-blocking edge case around secret-backed LFS enabling; this security-sensitive checkout/env protection change is still above my approval ceiling, so I’m leaving a comment.

Want to dig deeper?

The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:

Download the buildsworth logs from build 5332, then answer my questions about the findings.

Comment thread internal/job/executor.go
GIT_LFS_SKIP_SMUDGE was set in setUp before secrets were fetched and the
executor config refreshed, so a secret enabling BUILDKITE_GIT_LFS_ENABLED (it's
outside both protection maps, so any source may set it) turned on the LFS
checkout path without skip-smudge. The plain git checkout would then pull LFS
objects through the automatic smudge filter instead of the controlled LFS
fetch/checkout path, bypassing sparse scoping.

Move the skip-smudge decision to after the secret refresh so it sees the
secret-enabled LFS.

@buildsworth-bk-app buildsworth-bk-app Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one help-text mismatch in the new mode description and left a related LFS timing follow-up in the existing thread. This remains security-sensitive agent checkout/env protection logic, so it’s above my approval ceiling.

Want to dig deeper?

The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:

Download the buildsworth logs from build 5333, then answer my questions about the findings.

Comment thread clicommand/global.go Outdated
The default from-job mode blocks secrets and keeps the checkout flags
authoritative over pipeline/step env, but still lets pipeline/step env set the
checkout timeout, submodules, skip-checkout, and skip-fetch-existing-commits
toggles the agent leaves unset (matching earlier agent behaviour). Spell out
that carve-out so the default isn't read as a complete backend-env lock.
GIT_LFS_SKIP_SMUDGE is set in setUp, but a hook, plugin, or Job API call that
enables BUILDKITE_GIT_LFS_ENABLED afterward flips GitLFSEnabled via
ReadFromEnvironment without re-applying skip-smudge, so the plain checkout would
pull LFS objects through the automatic smudge filter instead of the controlled
fetch/checkout path.

Re-apply skip-smudge in applyEnvironmentChanges when LFS is enabled, covering
within-job sources; setUp still handles the agent-config and secret paths.

@buildsworth-bk-app buildsworth-bk-app Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous LFS timing and help-text concerns are addressed. I didn’t find any new issues in this pass; this is still security-sensitive agent checkout/env protection logic, so it’s above my approval ceiling.

Want to dig deeper?

The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:

Download the buildsworth logs from build 5335, then answer my questions about the findings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New user-facing feature!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants