Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions agent/integration/job_environment_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -759,19 +759,19 @@ func TestCheckoutScopedJobEnvOverrideHonorsCheckoutOverrideMode(t *testing.T) {
wantIgnoredEnvVars: []string{"BUILDKITE_GIT_CHECKOUT_TIMEOUT"},
},
// Commit verification is an enum (not a flag) but is checkout-override
// scoped, so it follows the same mode rules. The first case is the reported
// scenario: agent leaves it unset, the pipeline requests strict, and none
// lets the job env win so verification actually runs.
// scoped, so it follows the same mode rules. With v4's strict default, none
// lets the backend job env turn verification off instead.
{
name: "none_allows_job_env_to_enable_commit_verification_when_agent_unset",
name: "none_allows_job_env_to_override_default_commit_verification",
varName: "BUILDKITE_GIT_COMMIT_VERIFICATION",
jobEnv: map[string]string{
"BUILDKITE_GIT_COMMIT_VERIFICATION": "strict",
"BUILDKITE_GIT_COMMIT_VERIFICATION": "off",
},
agentCfg: agent.AgentConfiguration{
CheckoutOverrideMode: env.CheckoutOverrideNone,
GitCommitVerification: "strict",
CheckoutOverrideMode: env.CheckoutOverrideNone,
},
wantEnvValue: "strict",
wantEnvValue: "off",
},
{
name: "none_allows_job_env_to_override_commit_verification",
Expand All @@ -780,7 +780,7 @@ func TestCheckoutScopedJobEnvOverrideHonorsCheckoutOverrideMode(t *testing.T) {
"BUILDKITE_GIT_COMMIT_VERIFICATION": "strict",
},
agentCfg: agent.AgentConfiguration{
GitCommitVerification: "warn",
GitCommitVerification: "off",
CheckoutOverrideMode: env.CheckoutOverrideNone,
},
wantEnvValue: "strict",
Expand All @@ -792,10 +792,10 @@ func TestCheckoutScopedJobEnvOverrideHonorsCheckoutOverrideMode(t *testing.T) {
"BUILDKITE_GIT_COMMIT_VERIFICATION": "strict",
},
agentCfg: agent.AgentConfiguration{
GitCommitVerification: "warn",
GitCommitVerification: "off",
CheckoutOverrideMode: env.CheckoutOverrideStrict,
},
wantEnvValue: "warn",
wantEnvValue: "off",
wantIgnoredEnvVars: []string{"BUILDKITE_GIT_COMMIT_VERIFICATION"},
},
{
Expand All @@ -807,10 +807,10 @@ func TestCheckoutScopedJobEnvOverrideHonorsCheckoutOverrideMode(t *testing.T) {
"BUILDKITE_GIT_COMMIT_VERIFICATION": "strict",
},
agentCfg: agent.AgentConfiguration{
GitCommitVerification: "warn",
GitCommitVerification: "off",
CheckoutOverrideMode: env.CheckoutOverrideFromJob,
},
wantEnvValue: "warn",
wantEnvValue: "off",
wantIgnoredEnvVars: []string{"BUILDKITE_GIT_COMMIT_VERIFICATION"},
},
}
Expand Down
7 changes: 4 additions & 3 deletions clicommand/agent_start.go
Original file line number Diff line number Diff line change
Expand Up @@ -820,9 +820,10 @@ var AgentStartCommand = &cli.Command{
}
}

// Validate the commit verification option input
if v := cfg.GitCommitVerification; v != "" && v != "strict" && v != "warn" {
return fmt.Errorf("invalid value for --git-commit-verification: %q (must be \"strict\" or \"warn\")", v)
// The config file is loaded after CLI flag validation, so validate its
// commit verification value here as well.
if err := validateGitCommitVerification(cfg.GitCommitVerification); err != nil {
return err
}

// Force some settings if on Windows (these aren't supported yet)
Expand Down
21 changes: 18 additions & 3 deletions clicommand/global.go
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,18 @@ var (
}
)

func validateGitCommitVerification(value string) error {
if value != job.GitCommitVerificationStrict && value != job.GitCommitVerificationOff {
return fmt.Errorf(
"invalid value for --git-commit-verification: %q (must be %q or %q)",
value,
job.GitCommitVerificationStrict,
job.GitCommitVerificationOff,
)
}
return nil
}

// Git related flags shared between agent start and bootstrap
var (
SkipCheckoutFlag = &cli.BoolFlag{
Expand Down Expand Up @@ -246,9 +258,12 @@ var (
}

GitCommitVerificationFlag = &cli.StringFlag{
Name: "git-commit-verification",
Usage: "Enable git commit verification",
Sources: cli.EnvVars("BUILDKITE_GIT_COMMIT_VERIFICATION"),
Name: "git-commit-verification",
Value: job.GitCommitVerificationStrict,
Usage: "Verify that the commit being built exists on the specified branch; one of strict or off",
Comment thread
jamiemonserrate marked this conversation as resolved.
Sources: cli.EnvVars("BUILDKITE_GIT_COMMIT_VERIFICATION"),
ValidateDefaults: true,
Validator: validateGitCommitVerification,
}

GitFetchFlagsFlag = &cli.StringFlag{
Expand Down
48 changes: 48 additions & 0 deletions clicommand/global_test.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
package clicommand

import (
"context"
"slices"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/urfave/cli/v3"
)

func TestAllFlagEnvs(t *testing.T) {
Expand All @@ -23,3 +25,49 @@ func TestAllFlagEnvs(t *testing.T) {
t.Errorf("allFlagEnvs(EnvDumpCommand) diff (-got +want):\n%s", diff)
}
}

func TestGitCommitVerificationFlag(t *testing.T) {
tests := []struct {
name string
args []string
want string
}{
{
name: "defaults to strict",
want: "strict",
},
{
name: "can be overridden with off",
args: []string{"--git-commit-verification", "off"},
want: "off",
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
flag := &cli.StringFlag{
Name: GitCommitVerificationFlag.Name,
Value: GitCommitVerificationFlag.Value,
ValidateDefaults: GitCommitVerificationFlag.ValidateDefaults,
Validator: GitCommitVerificationFlag.Validator,
}

var got string
command := &cli.Command{
Name: "test",
Flags: []cli.Flag{flag},
Action: func(_ context.Context, command *cli.Command) error {
got = command.String("git-commit-verification")
return nil
},
}

if err := command.Run(t.Context(), append([]string{"test"}, test.args...)); err != nil {
t.Fatalf("command.Run() error = %v", err)
}
if got != test.want {
t.Errorf("git-commit-verification = %q, want %q", got, test.want)
}
})
}
}
10 changes: 5 additions & 5 deletions env/protected.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,12 +103,12 @@ var protectedEnv = map[string]protection{
// Locking matters because 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 (which is why disabling command-eval forces the mode to
// strict). BUILDKITE_GIT_COMMIT_VERIFICATION is an enum ("", "warn", "strict"),
// strict). BUILDKITE_GIT_COMMIT_VERIFICATION is an enum ("strict", "off"),
// not an injection vector, but the backend exposes it under `checkout:` alongside
// the flag vars, so it's governed by the mode too: only none lets a job's own
// checkout config (pipeline/step env, secrets) turn verification on, matching the
// other checkout settings. Vars here must not also appear in protectedEnv; the
// two maps are disjoint.
// the flag vars, so it's governed by the mode too: only none lets the backend job
// env and secrets select the verification mode, matching the other checkout
// settings. Vars here must not also appear in protectedEnv; the two maps are
// disjoint.
var checkoutOverrideScope = map[string]struct{}{
"BUILDKITE_GIT_CHECKOUT_FLAGS": {},
"BUILDKITE_GIT_CHECKOUT_TIMEOUT": {},
Expand Down
39 changes: 26 additions & 13 deletions internal/job/commit_verification.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@ var ErrCommitVerificationFailed = errors.New("commit verification failed")
// This is NOT evidence of an attack — it's an infrastructure problem.
var ErrCommitVerificationUnavailable = errors.New("commit verification unavailable")

const (
// GitCommitVerificationStrict checks branch ancestry and blocks a definitive mismatch.
GitCommitVerificationStrict = "strict"
// GitCommitVerificationOff skips branch ancestry verification entirely.
GitCommitVerificationOff = "off"
)

// checkCommitOnBranch performs the actual git ancestry check, handling shallow
// clones by deepening or unshallowing as needed. It returns:
// - nil if the commit is verified on the branch
Expand Down Expand Up @@ -237,12 +244,24 @@ func stripRefSuppressingFetchFlags(flags []string) []string {
return out
}

// verifyCommit is called if the user has commit verification enabled. It ensures that the commit we are
// asked to build exists and is reachable on the branch we are given.
// verifyCommit ensures that the commit we are asked to build exists and is
// reachable on the branch we are given.
func (e *Executor) verifyCommit(ctx context.Context) error {
// Skip if not enabled
if e.GitCommitVerification == "" {
switch e.GitCommitVerification {
case GitCommitVerificationOff:
e.shell.Commentf("Skipping commit verification: mode is off")
return nil
case GitCommitVerificationStrict, "":
// The zero value is treated as strict for programmatic ExecutorConfig
// consumers. The CLI rejects empty values, and only explicit off skips.
// Continue below.
default:
return fmt.Errorf(
"invalid git commit verification mode %q (must be %q or %q)",
e.GitCommitVerification,
GitCommitVerificationStrict,
GitCommitVerificationOff,
)
}

// Skip if commit is HEAD (nothing to verify)
Expand Down Expand Up @@ -287,17 +306,11 @@ func (e *Executor) verifyCommit(ctx context.Context) error {

// Definitive failure — commit is provably not on the branch
if errors.Is(err, ErrCommitVerificationFailed) {
if e.GitCommitVerification == "strict" {
return err
}
// err already begins with "commit verification failed", so log it as-is.
e.shell.Warningf("%s", err)
return nil
return err
}

// Verification unavailable — infrastructure issue, not a security concern.
// We always warn but never block, even in strict mode, to avoid users
// disabling verification entirely due to infrastructure false positives.
// Verification unavailable — infrastructure issue, not a definitive mismatch.
// We always warn but never block, even in strict mode.
// err already begins with "commit verification unavailable", so log it as-is.
e.shell.Warningf("%s", err)
return nil
Expand Down
Loading