Skip to content

ci: drop rocksbot for collaborators list#309

Open
cjdcordeiro wants to merge 3 commits into
canonical:mainfrom
cjdcordeiro:enhance-release-security
Open

ci: drop rocksbot for collaborators list#309
cjdcordeiro wants to merge 3 commits into
canonical:mainfrom
cjdcordeiro:enhance-release-security

Conversation

@cjdcordeiro

Copy link
Copy Markdown
Collaborator
  • Have you signed the CLA?

This is a CI security enhancement.

In #284, we improved the security of the build CI workflow, ensuring the whole CI job wouldn't have unnecessary write permissions to the repository, instead shifting towards a fine grained personal token that would only be used within a single, conditional step that only ran on GitHub release events.

NEW: this PR goes one step further by removing the need to have a bot account (which owns the aforementioned fine grained personal token) with write permissions in the repository's collaborators list. Instead, this PR splits the privileged step into a separate privileged job. This means the new job can use the ephemeral GITHUB_TOKEN to perform the privileged step, without giving unnecessary privileges to other jobs&steps of the workflow.

closes #308

@cjdcordeiro cjdcordeiro requested a review from upils July 8, 2026 09:29

@upils upils left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @cjdcordeiro! I have a couple of suggestions and questions but overall it seems to be safer, easier to maintain to more robust than the previous solution.

If we are happy with it we may want to do the same with the Security workflow.

Comment thread .github/workflows/build.yml Outdated
Comment thread .github/workflows/build.yml Outdated
Comment thread .github/workflows/build.yml Outdated
Comment on lines -134 to -137
# - How could this token be compromised?
# - By unintentionally merging a change that allows this step to run unvetted scripts
# (either via a "pull_request_target", use of an insecure 3rd party action, or malicious changes to the
# current script)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This maybe superfluous now but this bit was informative and would maybe prevent us from falling into this trap again. WDYT of keeping it (probably by rewording it a bit)?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. however, as you say, this is applicable to any other job in the CI that uses any token. It is less relevant now cause if this token gets compromised, the risk only exists during the execution of the job (unlike the previous FGT).

for dir in chisel_*/; do
for file in "$dir"*; do
echo "Uploading $file to release $CHISEL_VERSION"
gh release upload "$CHISEL_VERSION" "$file" --repo "$GITHUB_REPOSITORY"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the --repo $GITHUB_REPOSITORY argument needed now?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously it was running in a checkout, but now it's not (its now a separate job)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⬆️

@lczyk lczyk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afaict looks good.

for dir in chisel_*/; do
for file in "$dir"*; do
echo "Uploading $file to release $CHISEL_VERSION"
gh release upload "$CHISEL_VERSION" "$file" --repo "$GITHUB_REPOSITORY"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously it was running in a checkout, but now it's not (its now a separate job)

Comment thread .github/workflows/build.yml Outdated
Comment thread .github/workflows/build.yml
cjdcordeiro and others added 2 commits July 14, 2026 11:00
Co-authored-by: Upils <5464641+upils@users.noreply.github.com>
Re-word the new CI job comments
@cjdcordeiro cjdcordeiro requested a review from upils July 14, 2026 10:09

@upils upils left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @cjdcordeiro.

@upils upils added the Simple Nice for a quick look on a minute or two label Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Simple Nice for a quick look on a minute or two

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: improve github release process

3 participants