mozilla / addons

☂ Umbrella repository for Mozilla Addons ✨
Other
125 stars 41 forks source link

[Bug]: Forks cannot push images #14841

Open KevinMind opened 2 months ago

KevinMind commented 2 months ago

What happened?

In github actions, workflows triggered by forks (and dependabot which is treated as a fork) do not have access to secrets and have a read only github token. This limits what workflows can do when triggered by events from forks.

Workflow runs triggered by Dependabot pull requests run as if they are from a forked repository, and therefore use a read-only GITHUB_TOKEN. These workflow runs cannot access any secrets. For information about strategies to keep these workflows secure, see "[Security hardening for GitHub Actions](https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions)."

https://docs.github.com/en/actions/security-guides/automatic-token-authentication https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions

This forces us to rebuild the image in any jobs that use the run-docker action. It's not the end of the world, but it is not very good DX and does cause forks to run in a slightly different way than origin.

What did you expect to happen?

There should be a way to authenticate forked users to push images to their own package registry or to configure the workflow to push to the forks github pacagke registry namespace. Or we should consider not pushing images in CI and instead building, but this is less attractive as it is a performance hit and reduces the validity of CI.

Is there an existing issue for this?

┆Issue is synchronized with this Jira Task

diox commented 2 months ago

Dependabot PRs doesn't have access to secrets either, despite technically not being a fork.

In either case (forks/dependabot) access to secrets could be a security issue (especially if the change in the PR modifies the build script, but it's also problematic if the build ends up calling a package that is modified by the PR).

We worked around the original issue of not being able to use the build+push job by adding a condition around it:

if: ${{ github.event.pull_request.head.repo.fork == false && github.actor != 'dependabot[bot]' }}
diox commented 2 months ago

We worked around the original issue of not being able to use the build+push job by adding a condition around it:

I think the dependabot part of this condition is problematic though, as we run that workflow on master commits as well, and we have commits where the github.actor is going to be dependabot[bot] when we merge dependabot PRs. We should push the image in that case...

KevinMind commented 2 months ago

I wonder if the condition that dependabot PRs not having access to secrets also applies to commits on master where the actor is dependabot?

I would bet not, so we can probably update the github.actor check to check for dependabot PR branches instead.

diox commented 2 months ago

Yes, that's my thinking as well. We should modify that check to only do it on PRs or dependabot branches. Or even figure out if the secrets are available and make that the condition if we can.