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

DOC Add note about permissions to readme #138

Closed

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented Jul 28, 2024

Anyone who wants to use this with their own modules will need to set the correct permissions, so we should show them how and let them know about possible security implications.

Note that we don't have this in our repos' ci.yml files right now because some if not all of our own repos have Workflow permissions set to

Read and write permissions
Workflows have read and write permissions in the repository for all scopes.

We might want to demote that to

Read repository contents and packages permissions
Workflows have read permissions in the repository for the contents and packages scopes only.

Issue

@GuySartorelli GuySartorelli force-pushed the pulls/1/update-readme branch from 276dae1 to 67f1711 Compare July 28, 2024 21:56
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Ack

In gha-ci we already set permissions: {} and individual permissions on jobs

Have we validated that setting contents: write in the module level ci.yml is quickly overwritten to the permissions: {} in the shared-workflow ci.yml?

Cos otherwise, contents: write seems like very risky thing to ask people to set? GitHub actions by default will set contents: read for pull-requests, I'm not sure if this would overwrite that, though I think it would?

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Jul 28, 2024

In gha-ci we already set permissions: {} and individual permissions on jobs

Right, and that works for everything in the silverstripe org because of the overly-permissive permissions I mentioned in the PR description. We're basically bypassing the permission requirements anyway.
"Workflows have read and write permissions in the repository for all scopes."

Have we validated that setting contents: write in the module level ci.yml is quickly overwritten to the permissions: {} in the shared-workflow ci.yml?

I don't know what you mean by that - but I did test that contents: read wasn't sufficient, and that contents: write works as expected in my own repo https://github.com/GuySartorelli/silverstripe-composable-validators/actions/runs/10135116386

@GuySartorelli
Copy link
Member Author

I guess the other option is to split out the gha-tag-release part into its own job? So CI would trigger a new workflow for tagging instead of directly using that action as part of the CI workflow?

@emteknetnz
Copy link
Member

emteknetnz commented Jul 29, 2024

We're basically bypassing the permission requirements anyway.

I don't think that's true, there was a lot of trial and error to get those permissions correct. It wasn't simply a case of "everything has write permissions by default therefore its work"

What I don't quite understand is that things work fine on the actions on the creative-commoners account, so it's possibly not a cross-org issue? Though maybe stuff isn't actually working there and I just haven't noticed

I guess the other option is to split out the gha-tag-release part into its own job?

Yeah I think that's probably a good idea, that should entirely remove the need to worry about contents: write.

We already 'trigger' other workflows in other places e.g. https://github.com/silverstripe/gha-trigger-ci/blob/1/action.yml#L81

@GuySartorelli
Copy link
Member Author

What I don't quite understand is that things work fine on the actions on the creative-commoners account, so it's possibly not a cross-org issue?

As mentioned in the issue description and clarified in person, this is because we're using "Read and write permissions" instead of "Read repository contents and packages permissions" - which means that we don't have to declare permissions directly in the ci.yml file. It seems that the initial permissions of the token are validated early on, and any permissions downstream in the shared workflow file or actions files are validated only if the initial validation passes.

I guess the other option is to split out the gha-tag-release part into its own job?

Yeah I think that's probably a good idea

I'll proceed with that.

@GuySartorelli
Copy link
Member Author

Gonna do that in a separate PR.

@GuySartorelli GuySartorelli deleted the pulls/1/update-readme branch July 29, 2024 23:50
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.

2 participants