Add support for git commit verification#3905
Conversation
zhming0
left a comment
zhming0
left a comment
There was a problem hiding this comment.
I see the value in this change, but I left some questions re overall directions and the cost / gain asymmetry. Also I recommend relocating some codes to another file.
| if e.Tag != "" { | ||
| return nil | ||
| } | ||
|
|
||
| // Skip if this is a PR build — the commit may be on a merge ref, not the target branch | ||
| if e.PullRequest != "" { | ||
| return nil | ||
| } | ||
|
|
||
| // Skip if a custom refspec is set — the fetch may not populate standard branch refs, | ||
| // making ancestry verification unreliable | ||
| if e.RefSpec != "" { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
While I totally see the value of this change adding a level of protection, I worry that it's a bit narrow given all these bypass conditions.
Also on the direction, it's ultimately saying: "under X case, do not trust Buildkite backend". It's a questionable direction for us to invest in though we have prior arts (I think). But I do wonder if we could/should strengthen the backend to not send false information.
It's not strictly a blocker, but something worth a number of words to explain about.
There was a problem hiding this comment.
Given this change is largely isolated from the happy path of checkout.go and checkout.go is getting really big. Would it be possible move them to maybe e.g. checkout_verify_commit.go?
This comment applies to the test code in checkout_test.go too
| // Still 128 - full unshallow as last resort | ||
| e.shell.Commentf("Deepening insufficient, performing a full unshallow...") | ||
| _ = e.shell.Command("git", "fetch", "--unshallow").Run(ctx) |
There was a problem hiding this comment.
I wonder if the cost outweigh the gain here? Now the equation become, if someone put a random commit SHA in the build api, our agent will basically do a repo clone -> which depending on situation can take a long time.
An realistically, building for a old commit in CI is pretty rare 🤔
2 participants
Description
Adds commit-on-branch verification to the checkout phase.
As part of our agent checkout improvement work, we are adding support for git commit verification. This addresses a potential security issue: if an attacker adds a malicious commit to the repo, and is then able to trigger a build which specifies
commit: 3v1l5ha123branch: mainwe would build that commit as if it were on themainbranch, without verifying that this is the case, which could potentially lead to a production deployment of malicious code.This change introduces a
BUILDKITE_GIT_COMMIT_VERIFICATIONenvironment variable, which defaults tofalse. If set totrue, when we are given a commit and branch, before checkout and build, we do the following:git merge-base --is-ancestorto check if the commit really belongs to the specified branch.unshallowand check again.In terms of alternatives, the only change I considered was just going straight to
unshallowbefore verifying any commit. But I didn't want users to have to completely give up shallow commits to get verification, so I went with the "deepen first, then unshallow if necessary" approach.Context
See SUP-6535.
Changes
Adds a
BUILDKITE_GIT_COMMIT_VERIFICATIONenv var to enable to functionality. When set, we perform various git operations in between fetch and checkout, to make sure the provided commit is valid on the provided branch.Testing
go test ./...). Buildkite employees may check this if the pipeline has run automatically.go tool gofumpt -extra -w .)Disclosures / Credits
I used Claude to help me plan the feature and teach me some Go fundamentals. I coded the core functionality myself, and let Claude write the tests.