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

workflows: Add a job for auditing release assets #92829

Merged
merged 6 commits into from
Jul 26, 2024

Conversation

tstellar
Copy link
Collaborator

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

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
@llvmbot
Copy link
Collaborator

llvmbot commented May 20, 2024

@llvm/pr-subscribers-github-workflow

Author: Tom Stellard (tstellar)

Changes

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


Full diff: https://github.com/llvm/llvm-project/pull/92829.diff

2 Files Affected:

  • (added) .github/workflows/release-asset-audit.py (+40)
  • (added) .github/workflows/release-asset-audit.yml (+45)
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);

Copy link

github-actions bot commented May 20, 2024

⚠️ Python code formatter, darker found issues in your code. ⚠️

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()

Copy link
Contributor

@boomanaiden154 boomanaiden154 left a comment

Choose a reason for hiding this comment

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

One nit, LGTM.

.github/workflows/release-asset-audit.py Outdated Show resolved Hide resolved
@tru
Copy link
Collaborator

tru commented May 21, 2024

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 * * * *'
Copy link
Collaborator

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.

Copy link
Contributor

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.

Copy link
Collaborator Author

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({
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

@tstellar
Copy link
Collaborator Author

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.

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.

@tstellar tstellar added this to the LLVM 19.X Release milestone May 22, 2024
@tstellar
Copy link
Collaborator Author

Ping.

Copy link
Collaborator

@tru tru left a 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

@tstellar tstellar merged commit 9d2461e into llvm:main Jul 26, 2024
3 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants