-
Notifications
You must be signed in to change notification settings - Fork 31
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
Change update-proto workflow auth #116
Conversation
Instead of using the Commander Data PAT to commit, use the workflow's own secret. Add the permission to commit to this branch in the job definition.
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.
Makes perfect sense. Will need a server team member to approve as well.
@cretz do you have an opinion on how important it is or is not to allow the person running this workflow to choose a different identity and message for this commit to run as? It would be better security-wise to just have it identify itself as a bot and have a commit message like "Committed by Update Proto run $GITHUB_RUN_ID, attempt $GITHUB_RUN_ATTEMPT" |
Probably not important but would like others' opinion here (e.g. @alexshtin). The majority of the time this is triggered from https://github.com/temporalio/api/blob/master/.github/workflows/trigger-api-go-update.yml (so also please make sure that continues to work). Manual dispatch is rare, though I did need it recently. I guess it does help track back to who caused this to run though and looks nice in commit log, though obviously a spoofed unsigned commit I know is rough. |
I triggered this GHA manually only when I was testing it. |
Instead of using the Commander Data PAT to commit, use the workflow's own secret. Add the permission to commit to this branch in the job definition.
Since this commits but doesn't open a PR, this doesn't need permissions to open or edit PRs. You can unselect that check box and do read-only for the other default options as well.
Why?
We are trying to move away from PATs and instead have each workflow have its own minimal permissions.
How did you test it?
I ran this in my own fork here, with the settings I described at the top for default workflow permissions in the repo. It even allowed me to spoof as myself when it was the action, which...might be concerning, actually, since we take this from user inputs, but we've had it that way the whole time.
Potential risks
Permissions being wrong and it not updating proto in the desired branch. This can be seen by looking at the output of the workflow run.