Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include PR info into release version #384

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

artem-zinnatullin
Copy link
Contributor

Apparently, any PR to this repo whether made by untrusted 3rd-party or by a member of Buildkite team which successfully ran CI publishes:

They get versioned via git describe like so:

image image

Where g59acf83 in ghcr.io/buildkite/agent-stack-k8s/controller:0.15.0-5-g59acf83 indicates a short commit sha.

The security issue here is that for a typical end user who wants to grab what they think is latest build they just go to the packages linked right on GitHub and grab a latest build like the controller:0.15.0-5-g59acf83

But in reality controller:0.15.0-5-g59acf83 is build from an unmerged PR #381!

In worst case scenario such PRs can be made by a malicious 3rd-party doing god knows what like leaking user credentials or doing Remote Code Execution in their CI setup, PRs are not yet merged onto main branch of original repo but already appear in the available packages list: here is controller build from my old PR as I found out now https://github.com/buildkite/agent-stack-k8s/pkgs/container/agent-stack-k8s%2Fcontroller/218244275?tag=0.11.0-1-g43e4777

As of right now if I understand correctly a member of Buildkite team needs to manually approve CI to run on a 3rd-party PR, which should prevent malicious code from being published, but for the end user nothing indicates the difference between the releases on the package page.

This PR adds PR information into the release version name so it's clear that you're not taking some latest main branch commit release: controller:0.15.0-5-g59acf83controller:0.15.0-5-g59acf83-PR-381


I do want to emphasize that having 3rd-party PRs release as packages is actually very useful for testing the changes by the community and I'd love Buildkite to keep that feature, but to do so in a secure manner the release names need to clearly indicate that it is an untrusted 3rd-party PR release.

@DrJosh9000
Copy link
Contributor

This is a problem I'd definitely like to solve, and this is a reasonable approach.

Another approach we came up in discussion was: publish the "edge" and "PR" builds to a subfolder (e.g. agent-stack-k8s/pkgs/controller/edge), with only the numbered releases at the main path.

@DrJosh9000
Copy link
Contributor

👋 Hey @artem-zinnatullin, I've just pushed the first release which splits the paths of the builds (#450). The repo split addresses your main concern: only tagged releases should be pushed to the main repos, and unmerged or unreleased PRs are pushed to paths containing -dev.

That said, I'd still be happy to merge this, as it may be handy to find the PR which led to a particular dev build. Would you mind rebasing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants