🐛 fix dangerous workflow test and workflow parsing

[asraa]

Signed-off-by: Asra Ali [email protected]

• Please check if the PR fulfills these requirements

• Tests for the changes have been added (for bug fixes / features)
• PR title follows the guidelines defined in https://github.com/ossf/scorecard/blob/main/CONTRIBUTING.md#pr-process

• What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

• Fixes unit test for dangerous workflow

• Fixes small issue with workflow parsing the trigger.

• What is the current behavior? (You can also link to an open issue here)

• What is the new behavior (if this is a feature change)?

• dangerous workflow check succeeds for system/systemd where before it wouldn't recognize a list of event triggers (seems to parse as []interface{} rather than []string)

• https://github.com/systemd/systemd/blob/2ec0c4f94de7091c2f4fdf76742bb793d64de781/.github/workflows/labeler.yml#L7-L8

• Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

• Other information:

[asraa]

Fixes #1257 (comment) dangerous workfow check on systemd btw @evverx

[asraa]

Still planning on moving to the other parser, just figured i'd fix this bug while it's here

[laurentsimon]

Thanks!

[evverx]

I can confirm that the issue is gone. Thanks!

[asraa]

@azeemshaikh38 is there a known issue with the e2e token?

[naveensrinivasan]

@azeemshaikh38 is there a known issue with the e2e token?

Looks like an issue. I will turn off the check required we will get this merged and I will send out a PR for that.

[azeemshaikh38]

@azeemshaikh38 is there a known issue with the e2e token?

Looks like an issue. I will turn off the check required we will get this merged and I will send out a PR for that.

Let's not turn off e2e tests to merge PRs please. What is the issue with the token exactly? Can we fix that first before we merge this in?

[naveensrinivasan]

@azeemshaikh38 is there a known issue with the e2e token?

Looks like an issue. I will turn off the check required we will get this merged and I will send out a PR for that.

Let's not turn off e2e tests to merge PRs please. What is the issue with the token exactly? Can we fix that first before we merge this in?

I don't know. I would have to investigate. I can turn it back on.

[azeemshaikh38]

@azeemshaikh38 is there a known issue with the e2e token?

Looks like an issue. I will turn off the check required we will get this merged and I will send out a PR for that.

Let's not turn off e2e tests to merge PRs please. What is the issue with the token exactly? Can we fix that first before we merge this in?

I don't know. I would have to investigate. I can turn it back on.

Looks like a common issue:

• "Resource not accessible by integration" actions/first-interaction#10
• https://github.community/t/github-actions-are-severely-limited-on-prs/18179/5

The workaround is very complicated AFAICT. How about we remove the update comment part of the e2e workflows? That should unblock us. Wdyt @naveensrinivasan?

[azeemshaikh38]

@azeemshaikh38 is there a known issue with the e2e token?

Looks like an issue. I will turn off the check required we will get this merged and I will send out a PR for that.

Let's not turn off e2e tests to merge PRs please. What is the issue with the token exactly? Can we fix that first before we merge this in?

I don't know. I would have to investigate. I can turn it back on.

Yes, do turn it back on. Thanks Naveen!

[evverx]

Looks like a common issue:

If it's similar to the "labeler" action it could probably be fixed by simply switching to "pull_request_target" instead of "pull_request". As far as I can see, judging by https://github.com/peter-evans/create-or-update-comment#action-inputs

In public repositories this action does not work in pull_request workflows when triggered by forks. Any attempt will be met with the error, Resource not accessible by integration. This is due to token restrictions put in place by GitHub Actions ... Alternatively, use the pull_request_target event to comment on pull requests.

That would certainly be a dangerous workflow though given that there is a checkout somewhere :-) Another option would be to switch to workflow_run mentioned at https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

Together with the pull_request_target, a new trigger workflow_run was introduced to enable scenarios that require building the untrusted code and also need write permissions to update the PR with e.g. code coverage results or other test results. To do this in a secure manner, the untrusted code must be handled via the pull_request trigger so that it is isolated in an unprivileged environment. The workflow processing the PR should then store any results like code coverage or failed/passed tests in artifacts and exit.

It's an interesting coincidence that the PR related to dangerous workflows triggered an issue related to dangerous workflows :-)

[azeemshaikh38]

Looks like a common issue:

If it's similar to the "labeler" action it could probably be fixed by simply switching to "pull_request_target" instead of "pull_request". As far as I can see, judging by https://github.com/peter-evans/create-or-update-comment#action-inputs

In public repositories this action does not work in pull_request workflows when triggered by forks. Any attempt will be met with the error, Resource not accessible by integration. This is due to token restrictions put in place by GitHub Actions ... Alternatively, use the pull_request_target event to comment on pull requests.

That would certainly be a dangerous workflow though given that there is a checkout somewhere :-) Another option would be to switch to workflow_run mentioned at https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

Together with the pull_request_target, a new trigger workflow_run was introduced to enable scenarios that require building the untrusted code and also need write permissions to update the PR with e.g. code coverage results or other test results. To do this in a secure manner, the untrusted code must be handled via the pull_request trigger so that it is isolated in an unprivileged environment. The workflow processing the PR should then store any results like code coverage or failed/passed tests in artifacts and exit.

It's an interesting coincidence that the PR related to dangerous workflows triggered an issue related to dangerous workflows :-)

Thanks @evverx very helpful as always :) I'm thinking of removing the update comment step from the workflow as a quick fix to merge this PR in. We should file an issue to look into this further.

@asraa could you comment out this step in your PR and see if that unblocks you from submitting - https://github.com/ossf/scorecard/blob/main/.github/workflows/integration.yml#L100

[asraa]

Looks its still failing on missing the auth token this when it tries to check-env, despite being set here, I think.

scorecard/.github/workflows/integration.yml

Line 86 in 9878c4e

GITHUB_AUTH_TOKEN: ${{ secrets.GH_AUTH_TOKEN }}

[azeemshaikh38]

Looks its still failing on missing the auth token this when it tries to check-env, despite being set here, I think.

scorecard/.github/workflows/integration.yml

Line 86 in 9878c4e

GITHUB_AUTH_TOKEN: ${{ secrets.GH_AUTH_TOKEN }}

Argh! This is frustrating - https://docs.github.com/en/actions/security-guides/encrypted-secrets. Will look into it. If I can't resolve this by tomo will force submit this PR myself.

[azeemshaikh38]

Looks its still failing on missing the auth token this when it tries to check-env, despite being set here, I think.

scorecard/.github/workflows/integration.yml

Line 86 in 9878c4e

GITHUB_AUTH_TOKEN: ${{ secrets.GH_AUTH_TOKEN }}

Argh! This is frustrating - https://docs.github.com/en/actions/security-guides/encrypted-secrets. Will look into it. If I can't resolve this by tomo will force submit this PR myself.

Ok, so this is my understanding so far based on https://securitylab.github.com/research/github-actions-preventing-pwn-requests/:

• You can use pull_request_target to allow forks to access secrets. But, since this a security concern, the recommended way is to gate this (say using labels).
• There is also something called Protected Environments that is available through GitHub. What this does is gate a workflow from running until certain conditions are met (e.g required approvers have approved it).
• So, if we were to use pull_request_target and gate it with a protected environment, that should solve our problem while not adding any security concern. - https://dev.to/petrsvihlik/using-environment-protection-rules-to-secure-secrets-when-building-external-forks-with-pullrequesttarget-hci

@asraa @laurentsimon @naveensrinivasan for any feedback to ensure I'm not missing anything here. Will be sending out a PR to fix this, can also discuss over there.

[laurentsimon]

Looks its still failing on missing the auth token this when it tries to check-env, despite being set here, I think.

scorecard/.github/workflows/integration.yml

Line 86 in 9878c4e

GITHUB_AUTH_TOKEN: ${{ secrets.GH_AUTH_TOKEN }}

Argh! This is frustrating - https://docs.github.com/en/actions/security-guides/encrypted-secrets. Will look into it. If I can't resolve this by tomo will force submit this PR myself.

Ok, so this is my understanding so far based on https://securitylab.github.com/research/github-actions-preventing-pwn-requests/:

• You can use pull_request_target to allow forks to access secrets. But, since this a security concern, the recommended way is to gate this (say using labels).
• There is also something called Protected Environments that is available through GitHub. What this does is gate a workflow from running until certain conditions are met (e.g required approvers have approved it).
• So, if we were to use pull_request_target and gate it with a protected environment, that should solve our problem while not adding any security concern. - https://dev.to/petrsvihlik/using-environment-protection-rules-to-secure-secrets-when-building-external-forks-with-pullrequesttarget-hci

@asraa @laurentsimon @naveensrinivasan for any feedback to ensure I'm not missing anything here. Will be sending out a PR to fix this, can also discuss over there.

related note: @asraa looks like to remove false positive for dangerous workflows check in this case, we need to check the permissions set in the repo (pull_request_target + ref + permissions set seems fine). Let's create a tracking issue. Once we merge Token-Permissions into Dangerous workflows, it will make it easier to implement.
Trickier to remove false positive for secrets used in protected env.

[asraa]

created issue #1311 to track.

[github-actions]

Integration tests success for
[bcb35f5]
(https://github.com/ossf/scorecard/actions/runs/1483177334)

[azeemshaikh38]

Ok, looks like the protected environment is working. Phew!

[naveensrinivasan]

Ok, looks like the protected environment is working. Phew!

Yes!! Thanks