Update workflow policy to scan all branches for dangerous workflows #569 #622

serb-google · 2025-01-06T01:59:06Z

This PR addresses #569 and introduces 2 changes:

Adds support for local git repos in scorecard.go, with additional functionality to switch between all branches. I opted to not replace the existing Github client as policies may want to interact directly with Github.
Updates workflow.go to scan all remote branches in the local git repo.

Tested locally using https://github.com/serb-google/allstar, with a branch insecure_test which contains a fake unsafe workflow rule. Verified this triggered the alert which included output Vulnerable Branch: refs/remotes/origin/insecure_test.

This is my first contribution to this repo, please review carefully :)

all branches. Signed-off-by: Samuel Erb <[email protected]>

Signed-off-by: Samuel Erb <[email protected]>

jeffmendoza

LGTM

jeffmendoza · 2025-01-17T22:38:12Z

@raghavkaul can you review as well? Thanks.

raghavkaul

Thanks for this push, looks good with minor feedback.

raghavkaul · 2025-02-11T19:17:02Z

pkg/scorecard/scorecard.go

+	if err != nil {
+		return "", nil, err
+	}
+	return dir, repo, nil
 }

 // Function Close will close the scorecard clients. This cleans up the
 // downloaded tarball.
 func Close(fullRepo string) {


@jeffmendoza do we need to defer this func somewhere?

Nice catch! I went through and defer scorecard.Close... all uses of the Get function.

We have the Close here:

allstar/pkg/enforce/enforce.go

Line 342 in 5ef0769

defer scorecard.Close(fmt.Sprintf("%s/%s", owner, repo))

Just one per repo run, so that each policy can get the clients, the first one will download the tarball, but the subsequent policies that use scorecard will not have to download the tarball again.

Yup, you're right. I was able to validate this running ./main -once=true with inotifywait -m -e modify,create,delete /tmp also running. Only one folder named allstar<random number> is created and then deleted per-run after removing the unnecessary closes.

raghavkaul · 2025-02-11T19:23:52Z

pkg/policies/workflow/workflow.go

+				Msg(msg)
+			return &policydef.Result{
+				Enabled:    enabled,
+				Pass:       true,


Suggested change

Pass: true,

Pass: false,

raghavkaul · 2025-02-11T19:25:02Z

pkg/policies/workflow/workflow.go

+				Msg(msg)
+			return &policydef.Result{
+				Enabled:    enabled,
+				Pass:       true,


Suggested change

Pass: true,

Pass: false,

raghavkaul · 2025-02-11T19:25:05Z

pkg/policies/workflow/workflow.go

+				Msg(msg)
+			return &policydef.Result{
+				Enabled:    enabled,
+				Pass:       true,


Suggested change

Pass: true,

Pass: false,

serb-google · 2025-02-28T21:37:16Z

Validated this still works as expected:

{"severity":"INFO","org":"serb-google","repo":"allstar","area":"Dangerous Workflow","result":false,"enabled":false,"notify":"Project is out of compliance with Dangerous Workflow policy: dangerous workflow patterns detected\n\n\t\t**Rule Description**\n\t\tDangerous workflows are GitHub Action workflows that exhibit dangerous patterns that could render them vulnerable to attack. A vulnerable workflow is susceptible to leaking repository secrets, or allowing an attacker write access using the GITHUB_TOKEN. For more information about the particular patterns that are detected, see the [OpenSSF Scorecard documentation](https://github.com/ossf/scorecard/blob/main/docs/checks.md#dangerous-workflow) on dangerous workflows. Any vulnerable branch can be exploited, so this rule will check all branches (vulnerable list below).\n\n\t\t**Remediation Steps**\n\t\tAvoid the dangerous workflow patterns. See this [post](https://securitylab.github.com/research/github-actions-preventing-pwn-requests/) for information on avoiding untrusted code checkouts. See this [document](https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#understanding-the-risk-of-script-injections) for information on avoiding and mitigating the risk of script injections.\n\t\t**Dangerous Patterns Found**\n\n- [0]:\n\n**Additional Information**\n\tThis policy uses [OpenSSF Scorecard](https://github.com/ossf/scorecard/). You may wish to run a Scorecard scan directly on this repository for more details.\n\nVulnerable Branch: refs/remotes/origin/insecure_test","details":{"Findings":null},"time":"2025-02-28T21:36:22Z","message":"Policy run result."}

Signed-off-by: Samuel Erb <[email protected]>

serb-google · 2025-02-28T21:45:21Z

Force-pushed to add Signed-off-by line which was missing in previous commit.

raghavkaul

New changes LGTM.

Signed-off-by: Samuel Erb <[email protected]>

jeffmendoza · 2025-03-04T23:17:16Z

Thanks!

coheigea · 2025-03-06T06:57:56Z

@jeffmendoza @serb-google This PR is breaking the dangerous workflows scan for private repositories. It fails with an authentication failure on:

repo, err := git.PlainClone(dir, false, &git.CloneOptions{
		URL: "https://github.com/" + fullRepo,
	})

which can only work for public repos as it's not re-using the http config. The logs I see are:

{"severity":"ERROR","error":"authentication required: Repository not found.","time":"2025-03-06T07:02:58Z","message":"Unexpected error running policies."}

serb-google · 2025-03-07T01:49:49Z

Hi @coheigea, I believe serb-google@57aa84f should address this, but I need to find a way to test this before sending it out for a PR.

coheigea · 2025-03-07T09:55:09Z

@serb-google Hi, I get a different error message with the patch.

Before:

{"severity":"ERROR","error":"authentication required: Repository not found.","time":"2025-03-07T09:54:00Z","message":"Unexpected error running policies."}

After:

{"severity":"ERROR","error":"authentication required: invalid credentials","time":"2025-03-07T09:54:40Z","message":"Unexpected error running policies."}

serb-google · 2025-03-07T21:12:59Z

Hi @coheigea , I was able to work through this issue and found a way to also test this. If you don't mind can you test #660 as well?

Thanks,
Sam

coheigea · 2025-03-20T11:14:38Z

@serb-google I've found our scans running way slower than before on big repos with many branches after this change. What do you think about adding a boolean config whether to restrict to the default branch or not?

serb-google · 2025-04-04T22:55:38Z

Sorry for the delay @coheigea. I created #677 which should give you enough control to specify which branches you would like checked.

Thanks,
Sam

Add local repo support to allstar. Also update workflow policy to test

bfdb8da

all branches. Signed-off-by: Samuel Erb <[email protected]>

serb-google requested a review from a team as a code owner January 6, 2025 01:59

serb-google added 2 commits January 6, 2025 02:04

Fix unit test.

62919c4

Signed-off-by: Samuel Erb <[email protected]>

Fix linter.

026476f

Signed-off-by: Samuel Erb <[email protected]>

jeffmendoza previously approved these changes Jan 17, 2025

View reviewed changes

jeffmendoza requested a review from raghavkaul January 17, 2025 22:37

raghavkaul approved these changes Feb 11, 2025

View reviewed changes

serb-google dismissed jeffmendoza’s stale review via ac9b28f February 28, 2025 21:29

address #622 comments

56c269f

Signed-off-by: Samuel Erb <[email protected]>

serb-google force-pushed the workflows branch from ac9b28f to 56c269f Compare February 28, 2025 21:42

serb-google requested a review from jeffmendoza February 28, 2025 21:44

raghavkaul approved these changes Mar 3, 2025

View reviewed changes

serb-google added 2 commits March 4, 2025 22:40

Remove unnecessary calls to close.

ecb1f9a

Signed-off-by: Samuel Erb <[email protected]>

Remove unnecessary close call.

e76ae9c

Signed-off-by: Samuel Erb <[email protected]>

jeffmendoza approved these changes Mar 4, 2025

View reviewed changes

jeffmendoza merged commit bb2b83b into ossf:main Mar 4, 2025
7 checks passed

jeffmendoza pushed a commit that referenced this pull request Mar 4, 2025

address #622 comments

f3a7f1d

Signed-off-by: Samuel Erb <[email protected]>

serb-google mentioned this pull request Mar 7, 2025

Add auth workflow to clone private repos #660

Merged

serb-google mentioned this pull request Apr 4, 2025

Add config option to specify branches for dangerous workflow #677

Merged

Update workflow policy to scan all branches for dangerous workflows #569 #622

Update workflow policy to scan all branches for dangerous workflows #569 #622

Uh oh!

Conversation

serb-google commented Jan 6, 2025

Uh oh!

jeffmendoza left a comment

Choose a reason for hiding this comment

Uh oh!

jeffmendoza commented Jan 17, 2025

Uh oh!

raghavkaul left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serb-google commented Feb 28, 2025

Uh oh!

serb-google commented Feb 28, 2025

Uh oh!

raghavkaul left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jeffmendoza commented Mar 4, 2025

Uh oh!

coheigea commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serb-google commented Mar 7, 2025

Uh oh!

coheigea commented Mar 7, 2025

Uh oh!

serb-google commented Mar 7, 2025

Uh oh!

coheigea commented Mar 20, 2025

Uh oh!

serb-google commented Apr 4, 2025

Uh oh!

Uh oh!

coheigea commented Mar 6, 2025 •

edited

Loading