Add Zip Archive Support for Buildkite Plugins#3713
Add Zip Archive Support for Buildkite Plugins#3713adnowakodbx1 wants to merge 11 commits intobuildkite:mainfrom
Conversation
|
Hey @adnowakodbx1, thanks for the PR! It's fair to say we've been wanting a non-Git-based plugin distribution mechanism ever since implementing binary hooks, and I like the |
|
@DrJosh9000 any chance for ETA or at least ETA for ETA? :) We filed this as feature request with Buildkite as |
|
hiya @adnowakodbx1! i'm about to start reviewing this. as @DrJosh9000 says, we grander ambitions for plugins. Long term, we'd love for them to get to a state where everything is distributed as a compressed archive, and there are version constraints and lockfiles and such. A real packaging system, essentially. That said, those plans and schemes are a pretty long way off, and a solution like this — especially for binary plugins — is a great step in the right direction. |
moskyb
left a comment
There was a problem hiding this comment.
This looks really good in concept, and will be a really cool addition to the agent. Thanks so much for contributing it!
I have a couple of style nits and questions, and one more important security question, but once all that's addressed, i'm happy to merge this.
| pluginMock.Expect("testing").Once().AndCallFunc(func(c *bintest.Call) { | ||
| if err := bintest.ExpectEnv(t, c.Env, "ZIP_PLUGIN_LOADED=yes"); err != nil { | ||
| fmt.Fprintf(c.Stderr, "%v\n", err) | ||
| c.Exit(1) | ||
| } else { | ||
| c.Exit(0) | ||
| } | ||
| }) | ||
|
|
||
| tester.ExpectGlobalHook("command").Once().AndExitWith(0).AndCallFunc(func(c *bintest.Call) { | ||
| if err := bintest.ExpectEnv(t, c.Env, "ZIP_PLUGIN_LOADED=yes"); err != nil { | ||
| fmt.Fprintf(c.Stderr, "%v\n", err) | ||
| c.Exit(1) | ||
| } else { | ||
| c.Exit(0) | ||
| } | ||
| }) | ||
|
|
||
| tester.RunAndCheck(t, env...) | ||
| } |
There was a problem hiding this comment.
just so i understand what this test is testing: the point of the call to pluginMock is to ensure that the envar being set in the plugin hook (ZIP_PLUGIN_LOADED) is propagated to the environment in the hook script for the plugin. isn't that behaviour implied by the tester.ExpectGlobalHook call below? if it's set in the agent execution environment (ie the command hook), it'll have been set in the hook script
There was a problem hiding this comment.
You are right, I copied TestRunningPlugins and with that some redundant things. I cleaned up so ExpectGlobalHook asserts the hook from plugin was executed, pluginMock with assertions was copied over but not really needed.
| ) | ||
|
|
||
| // checkoutZipPlugin downloads and extracts a zip plugin to the plugins directory | ||
| func (e *Executor) checkoutZipPlugin(ctx context.Context, p *plugin.Plugin, checkout *pluginCheckout, pluginDirectory string) error { |
There was a problem hiding this comment.
calling this method checkoutZipPlugin is potentially a little misleading — nothing's getting checked out and git (thankfully!) isn't involved. potentially installZipPlugin might be clearer to future readers?
There was a problem hiding this comment.
🤔 though that said, plugins end up with a thing called a checkout, regardless of the transport used. i suppose it's probably fine.
internal/job/plugin_zip.go
Outdated
| // For http/https URLs, split host and path | ||
| parts := strings.SplitN(p.Location, "/", 2) | ||
| if len(parts) == 0 { | ||
| return "", fmt.Errorf("invalid plugin location: %s", p.Location) | ||
| } | ||
|
|
||
| u.Host = parts[0] | ||
| if len(parts) > 1 { | ||
| u.Path = "/" + parts[1] | ||
| } | ||
|
|
||
| // Add authentication if present | ||
| if p.Authentication != "" { | ||
| userInfo := strings.Split(p.Authentication, ":") | ||
| if len(userInfo) == 2 { | ||
| u.User = url.UserPassword(userInfo[0], userInfo[1]) | ||
| } else { | ||
| u.User = url.User(userInfo[0]) | ||
| } | ||
| } |
There was a problem hiding this comment.
there's a bunch of futzing around with URLs as strings in this function that strikes me as a little fragile. might it be better to parse p.Location as a URL and manipulate it that way?
internal/job/plugin_zip.go
Outdated
| if u.Scheme == "file" { | ||
| // Handle file:// URLs - copy local file | ||
| if err := copyFile(u.Path, destPath); err != nil { | ||
| return "", fmt.Errorf("failed to copy local file: %w", err) | ||
| } | ||
| } else if u.Scheme == "http" || u.Scheme == "https" { | ||
| // For HTTP/HTTPS, download with retries | ||
| err = roko.NewRetrier( | ||
| roko.WithMaxAttempts(3), | ||
| roko.WithStrategy(roko.Constant(2*time.Second)), | ||
| ).DoWithContext(ctx, func(r *roko.Retrier) error { | ||
| return e.downloadZipPluginHTTP(ctx, downloadURL, destPath) | ||
| }) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| } else { | ||
| return "", fmt.Errorf("scheme %s is not supported", u.Scheme) | ||
| } |
There was a problem hiding this comment.
style nit: this would be easier to read as a switch:
switch u.Scheme:
case "file":
// ...
case "http", "https":
// ...
default:
return "", fmt.Errorf("scheme %s is not supported", u.Scheme)
}
internal/job/plugin_zip.go
Outdated
| client := &http.Client{ | ||
| Timeout: 5 * time.Minute, | ||
| } |
There was a problem hiding this comment.
the agent has an http utility that has the default settings we like to use:
| client := &http.Client{ | |
| Timeout: 5 * time.Minute, | |
| } | |
| client := agenthttp.NewClient(agenthttp.WithTimeout(5*time.Minute)) |
| // Check total size to prevent zip bombs | ||
| var totalSize uint64 | ||
| for _, f := range r.File { | ||
| totalSize += f.UncompressedSize64 |
There was a problem hiding this comment.
it's worth noting that zip bombs can and do lie about their uncompressed size in the file header. This check will catch honest-but-genuinuely-large archives, but will fail for something actually malicious.
it might be worth adding something that can enforce the actual number of bytes written to disk to ensure that we're not trusting zip bombs on their word of how big they are — something like:
type limitWriter struct {
w io.Writer
remaining *uint64
}
func (lw *limitWriter) Write(p []byte) (int, error) {
if uint64(len(p)) > *lw.remaining {
return 0, fmt.Errorf("zip extraction exceeded %d MB limit", maxZipExtractedSize/(1024*1024))
}
n, err := lw.w.Write(p)
*lw.remaining -= uint64(n)
return n, err
}
func extractZipPlugin(zipPath, destPath string) error {
// ...all the checks we currently have, and then:
// archive-wide budget for how much we'll allow to be written to disk
remaining := uint64(maxZipExtractedSize)
for _, f := range r.File {
if err := extractZipFile(f, destPath, &remaining); err != nil {
return err
}
}
return nil
}
// extractZipFile extracts a single file from a zip archive
func extractZipFile(f *zip.File, destPath string, remaining *uint64) error {
// ... path traversal check, directory handling, symlink rejection unchanged ...
// Open zip file entry
rc, err := f.Open()
if err != nil {
return fmt.Errorf("failed to open zip entry: %w", err)
}
defer rc.Close()
// Create destination file
outFile, err := os.OpenFile(cleanPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode())
if err != nil {
return fmt.Errorf("failed to create file: %w", err)
}
defer outFile.Close()
// Copy content through a budget-enforcing writer
if _, err := io.Copy(&limitedWriter{w: outFile, remaining: remaining}, rc); err != nil {
return fmt.Errorf("failed to extract file: %w", err)
}
return nil
}| // Reject symlinks for security | ||
| if f.Mode()&os.ModeSymlink != 0 { | ||
| return fmt.Errorf("symlinks are not supported in zip plugins (found: %s)", f.Name) | ||
| } |
internal/job/plugin_zip.go
Outdated
| wantSHA256 := "" | ||
| if strings.HasPrefix(p.Version, "sha256:") { | ||
| wantSHA256 = strings.TrimPrefix(p.Version, "sha256:") | ||
| } |
There was a problem hiding this comment.
this can be replaced with:
| wantSHA256 := "" | |
| if strings.HasPrefix(p.Version, "sha256:") { | |
| wantSHA256 = strings.TrimPrefix(p.Version, "sha256:") | |
| } | |
| wantSHA256, _ := strings.CutPrefix(p.Version, "sha256:") |
also, does this imply that a bare sha256 sum without an annotation (eg a plugin like zip+http://example.com/plugin#906e6aa...) is valid? totally fine if so, but just wanted to check.
There was a problem hiding this comment.
Good point. At first I thought that bare sum should be ok but it's probably better to be explicit in case sum needs to change in the future. Changed to CutPrefix and added check for format.
Use strings.CutPrefix and reject version fragments that don't have the expected "sha256:" prefix, rather than silently ignoring them.
Zip bombs can lie about their uncompressed size in the file header. Instead of only checking the declared sizes, track actual bytes written to disk with a budget-enforcing io.Writer wrapper.
Remove redundant pluginMock env check — the command hook already verifies ZIP_PLUGIN_LOADED=yes. Remove redundant strings.ReplaceAll wrapping filepath.ToSlash which already converts all backslashes.
|
@moskyb apologies for the delay and thank you for your patience, I addressed all the comments |
Description
Adds support for distributing and using Buildkite plugins as zip archives as an alternative to distributing plugins as git repositories.
zip+httpsas schema mimics the way pip deals with specifying backends for repos: https://pip.pypa.io/en/stable/topics/vcs-support/#vcs-support. I though it's better than using extension from the URL to specify that plugin should be pulled as zip and not repo. This also should make sure it's non breaking change for any of current usecases.Context
Currently, Buildkite plugins must be distributed as git repositories, which in our environment requires forking and maintaining the forks of the plugins due to supply chain concerns. Distributing plugins as immutable zips would be more convenient and mirrors how other tools like Terraform distribute providers. Also git repositories don't seem like the best way to distribute polyglot (binary) hooks - distributing them in zips seems more natural.
Changes
internal/job/plugin_zip.gowhich contains core of the feature and things related to it including disabling extracting symlinks, protection against zip bombs, optional SHA256 validationzip-pluginsexperimentinternal/job/plugin.goto distinguiszip+https://,zip+http://, orzip+file://schemes and run new logic if the experiment gate is enabledTesting
go test ./...).go tool gofumpt -extra -w .)Disclosures / Credits
I consulted Claude Code on potential approaches and then iterated with it on the implementation - especially on potential security risks that dealing with zip archives can bring and ways to address them. Tests were written by Claude Code. I reviewed them but happy to change them if there are gaps / issues.