-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
consider alternative design ideas #157
Comments
I appreciate the desire here to reduce potential attack vectors, but alternative designs that I've considered have a number of drawbacks in terms of developer experience. For example, the app listens for release events so that it create a PR to publish the entry right away. If there's a manual step to comment on an issue in the BCR, that will result in some releases being delayed because people will forget. The BCR does not give write access to non-Googlers, and because the only way to submit an entry is to create a PR, for the vast majority of ruleset owners this has to be from a branch in a fork, and the app needs write permission to that fork to push an entry. I could make a centralized BCR fork perhaps under bazel-contrib that all entries are pushed to, but then we'd need to give write access to the public which could allow malicious code to be inserted. If we only allow the app to push to that fork and deny write access to ruleset maintainers, then they wouldn't be able to push edits to the PR in response to review feedback. Perhaps the publish workflow could be reworked as a series of GitHub Actions where authorization is done via a user PAT, but that's a very different design and a project that would require funding. I'm open to more ideas on improving security though, so if you have any concrete proposals please send them. |
Thanks for the response! Point taken about needing to add the app to my account's fork of the BCR repository. Is it at all possible for the release event to trigger the PR creation without having added the app to the project's repository? I'm not too fussed about the specifics so long as explicit actuation as part of a workflow is straightforward. Regarding funding, I could contribute changes if that would help? :) |
Is your concern that the app requires write access to the repository? It's unfortunate that it requires write access just for the BCR fork and not for the actual project repositories. One solution is to use a separate app, one that only listens for release events which gets installed to the project, and the other with write access that gets installed to a BCR fork. However, there are already two apps: one that can create PRs on the BCR, and the one that users use. It exists for the same reason---the canonical BCR didn't want to grant write access. Adding a second app for users to install could be confusing. A second approach is for the app to poll repositories it knows about for new releases (perhaps you commit to a config file in this repo to add your project), then users only need to install the write-access app to their BCR fork. A third approach is just to offer a GitHub actions style workflow that uses a PAT for authenticating all of the operations. I think we'd need to see more demand for this to dedicate some SIG funding to it. You're the first person to raise any concern over the app permissions. Perhaps you could start a discussion on the bazel slack #bzlmod channel?
For sure that would be helpful if we move forward with something :). |
Yes, that's the showstopper for me. If nobody else feels particularly worried about it, 🤷, but I can't unsee it. :(
Indeed, if it isn't feasible to signal from a workflow running in the project's repository to the app running in my account's fork of the BCR repository, then I'm going to have to hack together a workflow based on the third approach. |
I think that's a very reasonable concern.
I could see a GitHub action approach being offered as an alternative to the app. I've tried to keep the core domain logic separate from the application logic (trying to follow Domain Driven Design), so the GitHub action could be a different orchestration in the application layer but use the same domain code. Would you be interested in helping to build this out? It would be best to hold hold off until I've done some planned refactoring. I'm planning to add NestJS to the app to handle all the dependencies in a cleaner way. |
I would be keen to help do the needful to make |
We had a discussion about this in the SIG meeting. The approach everyone seemed to prefer was to just make a CLI. You could use it in a GitHub workflow to perform your release and it address #9. |
Thanks for the update! That's great to hear. Looking forward to seeing how the CLI can be packaged/distributed and subsequently used. Also, please feel free to close this issue as a duplicate of that issue. :) |
Any updates on sorting out the security concerns with this app? I would like to automate BCR publishing for BoringSSL, but giving a third-party app, even one officially recommended by Bazel, write access to a core, security-critical library is definitely a concern. |
We (Aspect) have a contract with Google to implement provenance for the BCR. A likely outcome from that work is that Publish to BCR will become a GitHub Action action giving you more control over what it does via workflows. However, we likely won't start on it until the next quarter. |
@kormide GitHub provides a JWT claim to verify a request originates from a certain repo and branch. Every module could set additional filters like only publishing from certain protected branches or tags. This should be enough to implement provenance. |
I had hoped to use Publish to BCR, but there are a few issues with it. First, a security issue: Publish to BCR requires granting a third-party app write access to the GitHub repository, even though it only reads from the repository, which requires no special privileges to read a repository: bazel-contrib/publish-to-bcr#157 Second, merely cutting a release is not sufficient to satisfy https://blog.bazel.build/2023/02/15/github-archive-checksum.html One needs to manually upload a release tarball that GitHub then stores explicitly. (Perhaps someone should define a deterministic tarball creation process for git revisions and end this silliness.) Since that tarball is added by an individual developer, it seems poor that nothing checks it against the git repository. The BCR repository itself has some tooling for making a release. It works by interactively asking questions (not automatable), but then saves an undocumented JSON file with the answers. I've written a script that generates the JSON file we need from a git tag. These JSON files need to reference file paths, so they cannot be made standalone. (See bazelbuild/bazel-central-registry#2781) Instead, the script drops everything into a temporary directory. Since BCR's limitations force us to do a lot of custom processing anyway, I made the script check that: 1. The release tarball matches the archive tarball, which are stable enough in practice. This allows anyone to perform an easy (still GitHub-dependent) check that they match, unless GitHub changes the hash. 2. The tarball's contents match the git tag in the local repository, so we verify GitHub against the developer's workstation. The script then prints a command to run in a local fork of the bazel-central-registry repository to make a PR. Alas, even downloading the tarball from GitHub takes a few seconds, so I had a bit of fun with the script output. Change-Id: I2a748309f63848ff097ee3c3e93e11751ef65cd7 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/71307 Reviewed-by: Adam Langley <[email protected]> Auto-Submit: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]>
In light of CVE-2024-3094, would it make sense to consider alternative design ideas at this time? It's not that I distrust this app. It's that I don't really want to add any app to each project's repository... and to my account's fork of the BCR repository.
For example, would it be possible to trigger off "commands" specified in issues filed to BCR? It would be useful to provide an action to facilitate issue filing, I suppose, but it would be crucial to confine everything else to the BCR side.
Thanks in advance for your consideration.
The text was updated successfully, but these errors were encountered: