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

Add workflow scope to github-bot token used on nodejs/docker-node #574

Closed
nschonni opened this issue Dec 1, 2020 · 21 comments
Closed

Add workflow scope to github-bot token used on nodejs/docker-node #574

nschonni opened this issue Dec 1, 2020 · 21 comments

Comments

@nschonni
Copy link
Member

nschonni commented Dec 1, 2020

Why

https://github.com/docker-library/official-images started to use GitHub Actions, so the automated PRs have started failing as the current PAT can no longer push to it's own fork that it creates PRs for.

Add workflow scope to the Personal Access Token we use on nodejs/docker-node. I'm not sure if the reverse lookup from the existing token is easy. I believe it can be found #556 (comment)

Potential risk

The risks outlined in #572 probably also apply here

@mmarchini
Copy link
Contributor

cc @nodejs/tsc @nodejs/community-committee

@mmarchini
Copy link
Contributor

docker-library/official-images started to use GitHub Actions, so the automated PRs have started failing as the current PAT can no longer push to it's own fork that it creates PRs for.

care to elaborate? I'm not sure I understand why it fails without workflow scope. A link to a failure (and to a discussion on that issue, if it exists) would be helpful.

@nschonni
Copy link
Member Author

nschonni commented Dec 1, 2020

Sure! Here is the failure we get now. EX: https://github.com/nodejs/docker-node/runs/1449886770?check_suite_focus=true#step:5:496

/usr/bin/git push --force-with-lease fork HEAD:refs/heads/node
To https://github.com/nodejs-github-bot/official-images
! [remote rejected] HEAD -> node (refusing to allow a Personal Access Token to create or update workflow .github/workflows/munge-pr.yml without workflow scope)

The munge-pr.yml file is actually something the Docker folks added in official images https://github.com/docker-library/official-images/blob/master/.github/workflows/munge-pr.yml. Since they added it, our PAT now requires the workflow scope to push to the github-bot fork when creating the automated PR to their repo.

It's one of those oddities I haven't seen in awhile, but was common when actions started to roll out. Now a day, things like GitHub Desktop fix the tokens it generates, but pushing a commit that includes a workflow file (even just a fast-forward merge) requires the scope.

@Trott
Copy link
Member

Trott commented Dec 9, 2020

I'm not sure how to proceed, but I don't want to see this stall either. @mmarchini or anyone else have anything to add at this point?

@mmarchini
Copy link
Contributor

@Trott this needs the usual four approvals (two from TSC, two from CommComm). I didn't approve yet because I want to understand why the workflow needs this permission (didn't have time to look into it yet, probably won't have time until late next week). If others feel comfortable approving it I would mind though.

@mmarchini
Copy link
Contributor

To be clear: I'm not blocking this, just withholding my approval until I understand how the permission fit in the workflow.

@nschonni
Copy link
Member Author

nschonni commented Dec 9, 2020

@mmarchini I'm not really sure what else I can add to my previous repsonse

@mmarchini
Copy link
Contributor

@nschonni I think your response is enough, I just didn't have time to explore it.

@mhdawson
Copy link
Member

mhdawson commented Dec 9, 2020

I think I'm +1 on this. It seems that it increases our risk because being able to push changes to workflow files could also somebody to export GITHUB_TOKEN which in terms could provide some additional privileges.

I'm not sure we have much choice so we need to depend on the protection from where the Personal Access Token. @nschonni is that Jenkins?

@nschonni
Copy link
Member Author

nschonni commented Dec 9, 2020

Not Jenkins, it's used by this GitHub Action https://github.com/nodejs/docker-node/blob/8ca0250e3aad08328fd8719bbe2f6a61ba004a2f/.github/workflows/official-pr.yml#L45 and is stored as a Repo level secret. Here is what it prints currently https://github.com/nodejs/docker-node/runs/1449886770?check_suite_focus=true#step:5:3
That Action only runs on pull_request_target, so a drive-by PR can't alter anything unless the PR changing it lands on the default branch.

I remember there have been issues in the past with the masking breaking on Travis, but I'm not sure if GitHub Actions have had one of those events (yet).

@mhdawson
Copy link
Member

@nschonni thanks, I figured it was either Jenkins or GitHub actions.. I'll reaffirm my +1

@targos
Copy link
Member

targos commented Dec 10, 2020

+1. This token is only used to update a fork and to open a pull request. If a workflow file was changed by a malicious actor, that would not pass the PR code review.

@mmarchini
Copy link
Contributor

(still needs two more +1s @nodejs/tsc @nodejs/community-committee)

@mmarchini
Copy link
Contributor

oh I see, since GitHub doesn't have an option to sync forks automatically, if there are changes on docker-library/official-images workflows, it will fail. I really wish GH had an auto-sync option by default :(

Ok, so this is more risky than #572 because it actually pulls code from a repository outside of the org. @nschonni do we need to keep Actions enabled on the fork (nodejs-github-bot/official-images)? If we can disable Actions I'm fine adding the scope to the PAT (it might be disabled already, I don't see any runs there).

@mmarchini
Copy link
Contributor

Since they added it, our PAT now requires the workflow scope to push to the github-bot fork when creating the automated PR to their repo.

the scope is only needed when a workflow file is changed upstream. it doesn't matter which file it is.

@ljharb
Copy link
Member

ljharb commented Dec 14, 2020

@mmarchini i use this action: https://github.com/wei/pull to keep forks up to date automatically.

@mmarchini
Copy link
Contributor

mmarchini commented Dec 14, 2020

@ljharb does it need workflow scope for when a workflow file is updated? Or do you just use github-provided token?

@mmarchini
Copy link
Contributor

It looks like it have the same issue: wei/pull#249 (comment)

@nschonni
Copy link
Member Author

in our case, we checkout the upstream repo for creating the patch anyway, so syncing forks isn't an issue in this case.

Ok, so this is more risky than #572 because it actually pulls code from a repository outside of the org. @nschonni do we need to keep Actions enabled on the fork (nodejs-github-bot/official-images)? If we can disable Actions I'm fine adding the scope to the PAT (it might be disabled already, I don't see any runs there).

Turning off the actions on the fork repo is fine, because it's the Action on nodejs/docker-node that actually does the work

@joyeecheung
Copy link
Member

+1 from me

@mmarchini
Copy link
Contributor

@nschonni disabled Actions on nodejs/official-images and created a new PAT with workflow scope for nodejs/docker-node (updated the existing secret with that token, so you shouldn't need to make any changes to the Action for it to work). I'll close this issue, please let me know if it doesn't work.

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

No branches or pull requests

7 participants