ci: split perf workflow

[upils]

• Have you signed the CLA?

---

Do not expose the token used to write the comment to PRs from forks.
In the workflow posting the comment, use a fine-grained token with the
minimum permissions.

[lczyk]

reminds me of https://github.com/canonical/chisel-releases/blob/main/.github/actions/upload-pr-comment/action.yaml which we then trigger, just like the above, and collect all the artifacts like this: https://github.com/canonical/chisel-releases/blob/main/.github/workflows/pr-comments.yaml

might be worth just doing the same? this way there is parity + the approach from chisel-releases is extendible if you ever need to post more comments.

[upils]

I looked at that too and almost copied it. However using the matrix in our case is not needed I believe and it felt artificial to add this complexity if it served no purpose. I also thought we could set this workflow in a common repository and "import" it here but it felt too big of a change for the fix I wanted to do. But it can be the next step in the future.

[letFunny]

A few comments, thank for looking into this Paul. It would be nice if we can get some guidance for security as well.

[letFunny]

Do we need another workflow instead of another job in the performance file? I think the token permissions are scoped per workflow and then per job so that should work and it will be clearer as this is not a general comment, but only posting the comment for performance benchmarks.

These second job will depend on the first one completing.

[upils]

It is about execution context. The bottom of the issue is that we needed to use pull_request_target before so the workflow was executed in our repository context so the token could be used to post the comment. The point of splitting in 2 workflows is that the one under the control of the contributor will execute in the context of the fork, while the one actually using the token to post the comment will be executed in the context of our repo.

We are applying the approach recommended by the GH security team to deal with this exact issue, see https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/.

This approach is also in line with other repository solving the same problem.

Also, maybe only relying on token permissions could solve the issue (or help mitigate an exploitation) but I would rather have multiple layers here and completely remove the vulnerability.

[letFunny]

I just find it weird to have a general workflow called comment-pr that looks for a specific path "perf-report.txt" from another workflow. Maybe change the name to make it more specific.

[letFunny]

We are using:

    if: ${{ github.event_name != 'workflow_run' || github.event.workflow_run.conclusion == 'success' }}

in another job, what is the difference? If either is better let's make it uniform.

[upils]

Your snippet is to exclude the workflow_run case. The if condition in the change is to make sure the event that triggered the workflow is coming from a PR, and not from other sources (regular automatic workflow runs, etc.)

[letFunny]

Okay, the difference is that for pro-tests we want it to run periodically as well as in the PRs while this workflow will only run in the context of a PR, I get that. However, there are also similarities in that the pro workflow has a token we should not leak and it will also run for pull requests. I just want to check whether the logic is also sound in the other one or whether we should change it as well to be closer to your changes.

[letFunny]

Reading the description I don't think this is needed.

[upils]

I put that here on purpose to make it explicit what token is used for now. The next step (that should be coordinated with @cjdcordeiro) is to create a fine-grained token that will be used here instead.

[cjdcordeiro]

I've put ROCKSBOT_CHISEL_PR_COMMENTER in place. Please replace the secret

[upils]

Thanks. See 3f2c033. I also removed the permissions added in this workflow since it should not use the default GITHUB_TOKEN anymore (see 5a4b590).