-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
workflows: Add a job for auditing release assets #92829
Conversation
This checks to ensure that uploads are only made by 'approved' uploaders, which is just everyone who has uploaded a release asset in the past. We could do more, but this is just a simple implementation so we can put something in place and see how it works. For more discussion see: https://discourse.llvm.org/t/rfc-improve-binary-security/78121
@llvm/pr-subscribers-github-workflow Author: Tom Stellard (tstellar) ChangesThis checks to ensure that uploads are only made by 'approved' uploaders, which is just everyone who has uploaded a release asset in the past. We could do more, but this is just a simple implementation so we can put something in place and see how it works. For more discussion see: Full diff: https://github.com/llvm/llvm-project/pull/92829.diff 2 Files Affected:
diff --git a/.github/workflows/release-asset-audit.py b/.github/workflows/release-asset-audit.py
new file mode 100644
index 0000000000000..02ba426293af6
--- /dev/null
+++ b/.github/workflows/release-asset-audit.py
@@ -0,0 +1,40 @@
+import github
+import sys
+
+token = sys.argv[1]
+
+gh = github.Github(login_or_token=token)
+repo = gh.get_repo('llvm/llvm-project')
+
+uploaders = set([
+ 'DimitryAndric',
+ 'stefanp-ibm',
+ 'lei137',
+ 'omjavaid',
+ 'nicolerabjohn',
+ 'amy-kwan',
+ 'mandlebug',
+ 'zmodem',
+ 'androm3da',
+ 'tru',
+ 'rovka',
+ 'rorth',
+ 'quinnlp',
+ 'kamaub',
+ 'abrisco',
+ 'jakeegan',
+ 'maryammo',
+ 'tstellar',
+ 'github-actions[bot]'
+])
+
+for release in repo.get_releases():
+ print("Release:", release.title)
+ for asset in release.get_assets():
+ created_at = asset.created_at
+ updated_at = "" if asset.created_at == asset.updated_at else asset.updated_at
+ print(f'{asset.name} : {asset.uploader.login} [{created_at} {updated_at}] ( {asset.download_count} )')
+ if asset.uploader.login not in uploaders:
+ print("Invalid uploader")
+ sys.exit(1)
+
diff --git a/.github/workflows/release-asset-audit.yml b/.github/workflows/release-asset-audit.yml
new file mode 100644
index 0000000000000..9d140527218ff
--- /dev/null
+++ b/.github/workflows/release-asset-audit.yml
@@ -0,0 +1,45 @@
+name: Release Asset Audit
+
+on:
+ workflow_dispatch:
+ schedule:
+ # * is a special character in YAML so you have to quote this string
+ # Run once an hour
+ - cron: '5 * * * *'
+
+ pull_request:
+ paths:
+ - ".github/workflows/release-asset-audit.py"
+ - ".github/workflows/release-asset-audit.yml"
+
+permissions:
+ contents: read # Default everything to read-only
+
+
+jobs:
+ audit:
+ name: "Release Asset Audit"
+ runs-on: ubuntu-22.04
+ if: github.repository == 'llvm/llvm-project'
+ steps:
+ - uses: actions/checkout@a5ac7e51b41094c92402da3b24376905380afc29 #v4.1.6
+ - name: "Run Audit Script"
+ env:
+ GITHUB_TOKEN: ${{ github.token }}
+ run: |
+ pip install --require-hashes -r ./llvm/utils/git/requirements.txt
+ python3 ./.github/workflows/release-asset-audit.py $GITHUB_TOKEN
+ - name: "File Issue"
+ if: failure()
+ uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea #v7.0.1
+ with:
+ github-token: ${{ secrets.ISSUE_SUBSCRIBER_TOKEN }}
+ script: |
+ const issue = await github.rest.issues.create({
+ owner: context.repo.owner,
+ repo: context.repo.repo,
+ title: "Release Asset Audit Failed",
+ body: `https://github.com/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`,
+ labels: ['infrastructure']
+ });
+ console.log(issue);
|
You can test this locally with the following command:darker --check --diff -r 2a2b27d99e3faf34a593c1ed8029ed4744f1cd5d...4744a1042b3399fc5a1440ce12daf81b24c450ae .github/workflows/release-asset-audit.py View the diff from darker here.--- release-asset-audit.py 2024-05-21 15:59:30.000000 +0000
+++ release-asset-audit.py 2024-05-21 16:18:56.886126 +0000
@@ -1,7 +1,8 @@
import github
import sys
+
def main():
token = sys.argv[1]
gh = github.Github(login_or_token=token)
@@ -40,12 +41,12 @@
)
print(
f"{asset.name} : {asset.uploader.login} [{created_at} {updated_at}] ( {asset.download_count} )"
)
if asset.uploader.login not in uploaders:
- with open('comment', 'w') as file:
- file.write(f'@{asset.uploader.login} is not a valid uploader.')
+ with open("comment", "w") as file:
+ file.write(f"@{asset.uploader.login} is not a valid uploader.")
sys.exit(1)
if __name__ == "__main__":
main()
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit, LGTM.
While this is a pragmatic change to allow anyone that have uploaded before, I am pretty sure what we have reached a consensus for is that only the release managers (and hans) would be allowed to upload. If we want to expand that to anyone that have been a release tester for a while I think we need to go back to the RFC thread and reach a consensus for that as well, since there was quite a bit of discussion about the specifics here. I would suggest we make the list the RM's and Hans to start with. |
schedule: | ||
# * is a special character in YAML so you have to quote this string | ||
# Run once an hour | ||
- cron: '5 * * * *' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no event to trigger the workflow on upload? I am a bit worried about leaving potential vectors up for in the worst case an hour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. Looks like release.edited
might be what we want here. The documentation seems to be a bit sparse though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a release trigger now, but it doesn't seem to work.
with: | ||
github-token: ${{ secrets.ISSUE_SUBSCRIBER_TOKEN }} | ||
script: | | ||
const issue = await github.rest.issues.create({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah this is great, I was wondering how we could highlight when it happened.
I wonder if we should tag the uploader and just say "Only RM's are allowed to upload" so they don't try to upload again if the thing was removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put a mention for the uploader in the issue comment, let me know what you think.
That is true, but we have to use the current list of uploaders in order to be able to audit historical releases. I was planning to do a follow up change with extra rules for LLVM 19+, which would include more strict checks, like requiring release attestations, but I'm still working on that. |
Ping. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge this. Then tighten the number of users that can upload for 19.x
This checks to ensure that uploads are only made by 'approved' uploaders, which is just everyone who has uploaded a release asset in the past.
We could do more, but this is just a simple implementation so we can put something in place and see how it works.
For more discussion see:
https://discourse.llvm.org/t/rfc-improve-binary-security/78121