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

Change update-proto workflow auth #116

Merged
merged 2 commits into from
May 13, 2024
Merged

Change update-proto workflow auth #116

merged 2 commits into from
May 13, 2024

Conversation

jackdawm
Copy link
Contributor

@jackdawm jackdawm commented Aug 25, 2023

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.

workflow options in repo

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.

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.
@jackdawm jackdawm requested review from a team as code owners August 25, 2023 17:41
Copy link
Member

@cretz cretz left a 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.

@jackdawm
Copy link
Contributor Author

@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"

@cretz
Copy link
Member

cretz commented Aug 28, 2023

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?

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.

@alexshtin
Copy link
Member

I triggered this GHA manually only when I was testing it.

@jackdawm jackdawm merged commit f055b7f into master May 13, 2024
3 checks passed
@jackdawm jackdawm deleted the workflow-auth branch May 13, 2024 16:00
gow added a commit that referenced this pull request May 20, 2024
gow added a commit that referenced this pull request May 20, 2024
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

Successfully merging this pull request may close these issues.

3 participants