Update GHA workflows to run on external PR's from forked repo's

[fredvd]

See #4619 and https://community.plone.org/t/gsoc-students-please-focus-on-your-application-restrictions-on-developer-team-access-on-github/17041/6

Almost all workflows now only trigger on push, which works only for branches/PR on the main repo. (only the towncrier one was already triggered).

We should probably use the pull_request_target trigger, as it runs the workflow in the context of the fork, so that secrets and other sensitive information cannot be used/accessed. Also this guarantees that the incoming pull request doesn't contain a modification to the triggered workflows themselves, which increases security.

https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target

Also: first time contributors will need to have an approval from an existing maintainer on the Developers team before the worfklows are run for the first time:

[davisagli]

@fredvd I think you have misinterpreted how the pull_request_target event works. You said "it runs the workflow in the context of the fork, so that secrets and other sensitive information cannot be used/accessed" but the docs say:

This event runs in the context of the base of the pull request, rather than in the context of the merge commit

That means it does the opposite of what you said. It would run the workflow in the context of the target branch in the plone organization. Now, you're still right that that is what we want here because it means that if someone submits a PR which tampers with the GHA workflows in the .github directory, the actions will still be run based on the upstream workflow definition.

We do still need to be very careful what is done in workflows that use this trigger, because it runs with read/write access to the repository and with access to our secrets. Read this paragraph from the docs carefully:

Warning: For workflows that are triggered by the pull_request_target event, the GITHUB_TOKEN is granted read/write repository permission unless the permissions key is specified and the workflow can access secrets, even when it is triggered from a fork. Although the workflow runs in the context of the base of the pull request, you should make sure that you do not check out, build, or run untrusted code from the pull request with this event. Additionally, any caches share the same scope as the base branch. To help prevent cache poisoning, you should not save the cache if there is a possibility that the cache contents were altered. For more information, see "Keeping your GitHub Actions and workflows secure: Preventing pwn requests" on the GitHub Security Lab website.

In general the very first thing our workflows do is check out code from the pull request branch, which means that if there is some step in the workflow which runs that code (running tests, for example), then it could be modified to steal our secrets or use the workflow's elevated permissions to write to the repository.

[davisagli]

On the other hand the pull_request event will run the workflow without access to elevated permissions and our secrets, but then we need to make sure that we have workflows which degrade in helpful ways when those aren't available. For example: don't try to send the cypress test results to the cypress SaaS if the secret isn't available

[stevepiercy]

On the other hand the pull_request event will run the workflow without access to elevated permissions and our secrets, but then we need to make sure that we have workflows which degrade in helpful ways when those aren't available. For example: don't try to send the cypress test results to the cypress SaaS if the secret isn't available

This is what I suggested in #4346, but without @davisagli's astute observation for graceful degradation. I think this is the way.

[fredvd]

@davisagli Thanks for reviewing, yes I completely mixed up cause and effect here, but still guessing in the right direction :-$

One technique for limiting impact or changes that we can do is adding a CODEOWNERS file, for example in the .github repo that provides some extra safety when workflow files are modified . I added one for example (and more for testing) at https://github.com/plone/2023.ploneconf.org/blob/main/.github/CODEOWNERS

It still means that incoming external PR's need to be scrutinised on exactly what is in the change set before a first run of them is made, but that is what is now configured with the 'first contribution' check. And it's not only the first run, it is any commit or PR from any Developer team member having write access to the repo.

The risk level that we identify now has already been there for a longer time and is also impacted by a very large group that has access to the repo. Decreasing that risk area is another broader community effort for the near future.

So what are next stept to finish this change? We'll need to keep the use of secrets for deployment distribution of build artefacts to a minimum and remember they could be compromised. As far as I can see we need credentials for cypress deploy to netlify and there is a token to trigger the documentation build which pulls in the Volto documentation. But otherwise there are no goodies here on that part.

[sneridagh]

Let me just drop a line here, I'd never merge a PR from anybody (let alone a newbie) where I do not have any clue if the tests are all green.

It's true tht nowadays even dependabot is not able to get properly the secrets... AFAIK the only secret that we are saving for tests is the cypress dashboard ones. Maybe adding a check, if they are accessible, then use the dashboard, if not, do not?

I haven't time to take a look myself, but isn't there a GHA setting that allows you to ask for permission to run actions on forked PRs? Therefore maybe there's a way to allow access to secrets.

I saw it working somewhere...

[davisagli]

@sneridagh Our goal here is to provide a way for builds to run on PRs from forks without providing them with access to secrets, which is not safe.

I can work on trying some of the details but probably not until later this week. So far it sounds like we need to:

1. Make it possible to run the cypress tests even if we don't have the secret for the cypress dashboard.
2. Check how netlify handles external PRs (we probably have less control here, since the triggers are linked to Netlify's github app, not our workflow configuration)
3. Switch the trigger for GHA workflows to include pull_request rather than just push.

[fredvd]

Let me just drop a line here, I'd never merge a PR from anybody (let alone a newbie) where I do not have any clue if the tests are all green.

We are discussing different risks here. What you mention is about malicious code slipping into Plone/Volto. The secrets discussing is about running any automated action on untrusted code that is still in development.

I haven't time to take a look myself, but isn't there a GHA setting that allows you to ask for permission to run actions on forked PRs?

Yes and that setting is already in effect and now already working. When an external PR is created, the workflows are not run before they are approved by someone from the Developer Team. Maybe we should restrict that further. We can also set it to be required for every worfklow run on an external pull reques, but that will mean extra load on the active developer team.

[stevepiercy]

2. Check how netlify handles external PRs (we probably have less control here, since the triggers are linked to Netlify's github app, not our workflow configuration)

Netlify handles PRs from external forks.

• Example: fixed Responsive Error in login page #3250 #4616,
• and resulting build: https://app.netlify.com/sites/volto/deploys/642038084b43650008045de6

PRs should get built according to the per project configuration in Netlify, usually with the command make netlify. Here is Volto's:

Netlify looks to see if there is a git diff in the defined docs files per netlify.toml. When there is a diff, it clones the repo, installs requirements, and builds static docs files all on its environment, just like anyone on the planet can do on their own environment. Netlify does nothing when there is no git diff for those files defined as docs, as the above example shows. I think there is no inherent security issue here.

---

I don't know if this could be useful to other parts of this discussion, but the does the secret GITHUB_TOKEN as used in docs.yml provide anything that could help? See https://docs.github.com/en/actions/security-guides/automatic-token-authentication#about-the-github_token-secret

[davisagli]

@stevepiercy Yeah, I looked at Netlify's docs and reached pretty much the same conclusion. An external user could open a PR which changes make netlify to do anything they like, but as long as the environment where Netlify runs it does not have access to any of our secrets, that is Netlify's problem rather than ours.

GITHUB_TOKEN is a bit orthogonal to the discussion: it is the way that commands in a workflow can authenticate back to the github API to do the things that are permitted by the workflow, but we are talking about the ways we have to influence what that list of permissions is.

[davisagli]

Closed by #4629