From bfdb8dac7ec248b67be02a14bf31be5f8adf3fc5 Mon Sep 17 00:00:00 2001 From: Samuel Erb Date: Sun, 5 Jan 2025 04:56:30 +0000 Subject: [PATCH] Add local repo support to allstar. Also update workflow policy to test all branches. Signed-off-by: Samuel Erb --- README.md | 2 + pkg/policies/binary/binary.go | 2 +- pkg/policies/scorecard/scorecard.go | 4 +- pkg/policies/workflow/workflow.go | 163 ++++++++++++++++------------ pkg/scorecard/scorecard.go | 135 ++++++++++++++++++++--- pkg/scorecard/scorecard_test.go | 39 ++++++- whats-new.md | 2 +- 7 files changed, 248 insertions(+), 99 deletions(-) diff --git a/README.md b/README.md index 638a7da7..8292c92f 100644 --- a/README.md +++ b/README.md @@ -331,6 +331,8 @@ This policy's config file is named `dangerous_workflow.yaml`, and the [config definitions are here](https://pkg.go.dev/github.com/ossf/allstar/pkg/policies/workflow#OrgConfig). +This policy will run against **all** branches, see rationale [here](https://github.com/ossf/allstar/issues/569). + This policy checks the GitHub Actions workflow configuration files (`.github/workflows`), for any patterns that match known dangerous behavior. See the [OpenSSF Scorecard diff --git a/pkg/policies/binary/binary.go b/pkg/policies/binary/binary.go index a9ba1fe3..67fb1297 100644 --- a/pkg/policies/binary/binary.go +++ b/pkg/policies/binary/binary.go @@ -120,7 +120,7 @@ func (b Binary) Check(ctx context.Context, c *github.Client, owner, fullName := fmt.Sprintf("%s/%s", owner, repo) tr := c.Client().Transport - scc, err := scorecard.Get(ctx, fullName, tr) + scc, err := scorecard.Get(ctx, fullName, false, tr) if err != nil { return nil, err } diff --git a/pkg/policies/scorecard/scorecard.go b/pkg/policies/scorecard/scorecard.go index 1df0b136..1e18e82c 100644 --- a/pkg/policies/scorecard/scorecard.go +++ b/pkg/policies/scorecard/scorecard.go @@ -87,7 +87,7 @@ type details struct { var configFetchConfig func(context.Context, *github.Client, string, string, string, config.ConfigLevel, interface{}) error var configIsEnabled func(context.Context, config.OrgOptConfig, config.RepoOptConfig, config.RepoOptConfig, *github.Client, string, string) (bool, error) -var scorecardGet func(context.Context, string, http.RoundTripper) (*scorecard.ScClient, error) +var scorecardGet func(context.Context, string, bool, http.RoundTripper) (*scorecard.ScClient, error) var checksAllChecks checker.CheckNameToFnMap var scRun func(context.Context, clients.Repo, ...sc.Option) (sc.Result, error) @@ -141,7 +141,7 @@ func (b Scorecard) Check(ctx context.Context, c *github.Client, owner, fullName := fmt.Sprintf("%s/%s", owner, repo) tr := c.Client().Transport - scc, err := scorecardGet(ctx, fullName, tr) + scc, err := scorecardGet(ctx, fullName, false, tr) if err != nil { return nil, err } diff --git a/pkg/policies/workflow/workflow.go b/pkg/policies/workflow/workflow.go index 827abf75..d4f5fff7 100644 --- a/pkg/policies/workflow/workflow.go +++ b/pkg/policies/workflow/workflow.go @@ -106,86 +106,105 @@ func (b Workflow) Check(ctx context.Context, c *github.Client, owner, fullName := fmt.Sprintf("%s/%s", owner, repo) tr := c.Client().Transport - scc, err := scorecard.Get(ctx, fullName, tr) + scc, err := scorecard.Get(ctx, fullName, true, tr) if err != nil { return nil, err } - - allRes, err := sc.Run(ctx, scc.ScRepo, - sc.WithRepoClient(scc.ScRepoClient), - sc.WithChecks([]string{checks.CheckDangerousWorkflow}), - ) + // Fetch branches and run sc.Run against every branch. + branches, err := scc.FetchBranches() if err != nil { - msg := "Error while running checks.DangerousWorkflow" - log.Warn(). - Str("org", owner). - Str("repo", repo). - Str("area", polName). - Err(err). - Msg(msg) - return &policydef.Result{ - Enabled: enabled, - Pass: true, - NotifyText: fmt.Sprintf("%s: %v", msg, err), - Details: details{}, - }, nil - } - if len(allRes.Checks) != 1 { - msg := "Error while running checks.DangerousWorkflow : did not get expected checks" - log.Warn(). - Str("org", owner). - Str("repo", repo). - Str("area", polName). - Int("chk_len", len(allRes.Checks)). - Msg(msg) - return &policydef.Result{ - Enabled: enabled, - Pass: true, - NotifyText: msg, - Details: details{}, - }, nil - } - res := allRes.Checks[0] - - if res.Error != nil { - msg := "Error while running checks.DangerousWorkflow" - log.Warn(). - Str("org", owner). - Str("repo", repo). - Str("area", polName). - Err(res.Error). - Msg(msg) - return &policydef.Result{ - Enabled: enabled, - Pass: true, - NotifyText: fmt.Sprintf("%s: %v", msg, res.Error), - Details: details{}, - }, nil + return nil, err } - logs := convertLogs(res.Details) - pass := res.Score >= checker.MaxResultScore || res.Score == checker.InconclusiveResultScore var notify string - if !pass { - notify = fmt.Sprintf(`Project is out of compliance with Dangerous Workflow policy: %v - -**Rule Description** -Dangerous 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. - -**Remediation Steps** -Avoid 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. -`, - res.Reason) - if len(logs) > 10 { - notify += fmt.Sprintf( - "**First 10 Dangerous Patterns Found**\n\n%v"+ - "- Run a Scorecard scan to see full list.\n\n", - listJoin(logs[:10])) - } else { - notify += fmt.Sprintf("**Dangerous Patterns Found**\n\n%v\n", listJoin(logs)) + var logs []string + pass := true + for _, branch := range branches { + err = scc.SwitchLocalBranch(branch) + if err != nil { + return nil, err + } + + allRes, err := sc.Run(ctx, scc.ScRepo, + sc.WithRepoClient(scc.ScRepoClient), + sc.WithChecks([]string{checks.CheckDangerousWorkflow}), + ) + if err != nil { + msg := "Error while running checks.DangerousWorkflow" + log.Warn(). + Str("org", owner). + Str("repo", repo). + Str("area", polName). + Err(err). + Msg(msg) + return &policydef.Result{ + Enabled: enabled, + Pass: true, + NotifyText: fmt.Sprintf("%s: %v", msg, err), + Details: details{}, + }, nil + } + if len(allRes.Checks) != 1 { + msg := "Error while running checks.DangerousWorkflow : did not get expected checks" + log.Warn(). + Str("org", owner). + Str("repo", repo). + Str("area", polName). + Int("chk_len", len(allRes.Checks)). + Msg(msg) + return &policydef.Result{ + Enabled: enabled, + Pass: true, + NotifyText: msg, + Details: details{}, + }, nil + } + res := allRes.Checks[0] + + if res.Error != nil { + msg := "Error while running checks.DangerousWorkflow" + log.Warn(). + Str("org", owner). + Str("repo", repo). + Str("area", polName). + Err(res.Error). + Msg(msg) + return &policydef.Result{ + Enabled: enabled, + Pass: true, + NotifyText: fmt.Sprintf("%s: %v", msg, res.Error), + Details: details{}, + }, nil + } + + logs = convertLogs(res.Details) + branchPass := res.Score >= checker.MaxResultScore || res.Score == checker.InconclusiveResultScore + if !branchPass { + pass = false + if notify == "" { + notify = fmt.Sprintf(`Project is out of compliance with Dangerous Workflow policy: %v + + **Rule Description** + Dangerous 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). + + **Remediation Steps** + Avoid 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. + `, + res.Reason) + if len(logs) > 10 { + notify += fmt.Sprintf( + "**First 10 Dangerous Patterns Found**\n\n%v"+ + "- Run a Scorecard scan to see full list.\n\n", + listJoin(logs[:10])) + } else { + notify += fmt.Sprintf("**Dangerous Patterns Found**\n\n%v\n", listJoin(logs)) + } + notify += `**Additional Information** + This policy uses [OpenSSF Scorecard](https://github.com/ossf/scorecard/). You may wish to run a Scorecard scan directly on this repository for more details. +` + } + notify += fmt.Sprintf("\nVulnerable Branch: %s", branch) } - notify += `**Additional Information** -This policy uses [OpenSSF Scorecard](https://github.com/ossf/scorecard/). You may wish to run a Scorecard scan directly on this repository for more details.` } return &policydef.Result{ diff --git a/pkg/scorecard/scorecard.go b/pkg/scorecard/scorecard.go index c8a97ec7..01348755 100644 --- a/pkg/scorecard/scorecard.go +++ b/pkg/scorecard/scorecard.go @@ -23,10 +23,15 @@ package scorecard import ( "context" "net/http" + "os" "sync" + "github.com/go-git/go-git/v5" + "github.com/go-git/go-git/v5/plumbing" "github.com/ossf/scorecard/v5/clients" "github.com/ossf/scorecard/v5/clients/githubrepo" + "github.com/ossf/scorecard/v5/clients/localdir" + "github.com/rs/zerolog/log" ) // Type ScClient is returned from Get. It contains the clients needed to call @@ -34,9 +39,12 @@ import ( type ScClient struct { ScRepo clients.Repo ScRepoClient clients.RepoClient + localPath string + gitRepo *git.Repository } -var scClients map[string]*ScClient = make(map[string]*ScClient) +var scClientsRemote map[string]*ScClient = make(map[string]*ScClient) +var scClientsLocal map[string]*ScClient = make(map[string]*ScClient) var mMutex sync.RWMutex const defaultGitRef = "HEAD" @@ -52,37 +60,130 @@ func init() { // Function Get will get the scorecard clients and create them if they don't // exist. The github repo is initialized, which means the tarball is // downloaded. -func Get(ctx context.Context, fullRepo string, tr http.RoundTripper) (*ScClient, error) { +// If local is set, a local copy of the repo will be used instead, this allows for branch +// changes without another scorecard repo download. +func Get(ctx context.Context, fullRepo string, local bool, tr http.RoundTripper) (*ScClient, error) { + defer mMutex.Unlock() mMutex.Lock() - if scc, ok := scClients[fullRepo]; ok { - mMutex.Unlock() + if local { + if scc, ok := scClientsLocal[fullRepo]; ok { + return scc, nil + } + scc, err := createLocal(ctx, fullRepo) + if err != nil { + return nil, err + } + scClientsLocal[fullRepo] = scc return scc, nil + + } else { + // remote + if scc, ok := scClientsRemote[fullRepo]; ok { + return scc, nil + } + scc, err := createRemote(ctx, fullRepo, tr) + if err != nil { + return nil, err + } + scClientsRemote[fullRepo] = scc + return scc, nil + } +} + +// Switch the local repo between branches +func (scc ScClient) SwitchLocalBranch(branchName string) error { + log.Info(). + Str("branch", branchName). + Msg("Branch checkout") + + w, err := scc.gitRepo.Worktree() + if err != nil { + return err + } + err = w.Checkout(&git.CheckoutOptions{Branch: plumbing.ReferenceName(branchName)}) + if err != nil { + return err } - scc, err := create(ctx, fullRepo, tr) + return nil +} + +// Fetch branches from the local repo (used for workflow rule checking) +func (scc ScClient) FetchBranches() ([]string, error) { + refs, err := scc.gitRepo.References() if err != nil { - mMutex.Unlock() return nil, err } - scClients[fullRepo] = scc - mMutex.Unlock() - return scc, nil + + var ret []string + refs.ForEach(func(ref *plumbing.Reference) error { + if ref.Name().IsRemote() { + ret = append(ret, ref.Name().String()) + } + return nil + }) + return ret, nil +} + +// Checkout a repo into a local directory +// returns the path to the local repo and a git repo reference +func checkoutRepo(fullRepo string) (string, *git.Repository, error) { + log.Info(). + Str("repo", fullRepo). + Msg("Repo checkout") + // create a temp dir to store the repo + dir, err := os.MkdirTemp("", "allstar") + if err != nil { + return "", nil, err + } + + // checkout to the temp dir + repo, err := git.PlainClone(dir, false, &git.CloneOptions{ + URL: "https://github.com/" + fullRepo, + }) + 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) { + defer mMutex.Unlock() mMutex.Lock() - scc, ok := scClients[fullRepo] - if !ok { - mMutex.Unlock() - return + // local + scc, ok := scClientsLocal[fullRepo] + if ok { + // delete local temp directory + os.RemoveAll(scc.localPath) + delete(scClientsLocal, fullRepo) + } + // remote + scc, ok = scClientsRemote[fullRepo] + if ok { + scc.ScRepoClient.Close() + delete(scClientsRemote, fullRepo) } - scc.ScRepoClient.Close() - delete(scClients, fullRepo) - mMutex.Unlock() } -func create(ctx context.Context, fullRepo string, tr http.RoundTripper) (*ScClient, error) { +func createLocal(ctx context.Context, fullRepo string) (*ScClient, error) { + localPath, gitRepo, err := checkoutRepo(fullRepo) + if err != nil { + return nil, err + } + + scr, err := localdir.MakeLocalDirRepo(localPath) + if err != nil { + return nil, err + } + return &ScClient{ + ScRepo: scr, + localPath: localPath, + gitRepo: gitRepo, + }, nil +} + +func createRemote(ctx context.Context, fullRepo string, tr http.RoundTripper) (*ScClient, error) { scr, err := githubrepoMakeGitHubRepo(fullRepo) if err != nil { return nil, err diff --git a/pkg/scorecard/scorecard_test.go b/pkg/scorecard/scorecard_test.go index 443d0a15..97d94d6b 100644 --- a/pkg/scorecard/scorecard_test.go +++ b/pkg/scorecard/scorecard_test.go @@ -150,7 +150,7 @@ func TestGetNew(t *testing.T) { close = func() error { return nil } - _, _ = Get(context.Background(), "org/repo", nil) + _, _ = Get(context.Background(), "org/repo", false, nil) if !makeCalled { t.Error("githubrepo.MakeGithubRepo not called for new repo") } @@ -180,9 +180,9 @@ func TestGetExisting(t *testing.T) { close = func() error { return nil } - _, _ = Get(context.Background(), "org/repo", nil) + _, _ = Get(context.Background(), "org/repo", false, nil) makeCalled, createCalled, initCalled = false, false, false - _, _ = Get(context.Background(), "org/repo", nil) + _, _ = Get(context.Background(), "org/repo", false, nil) if makeCalled { t.Error("githubrepo.MakeGithubRepo called for existing repo") } @@ -210,7 +210,7 @@ func TestClose(t *testing.T) { closeCalled = true return nil } - _, _ = Get(context.Background(), "org/repo", nil) + _, _ = Get(context.Background(), "org/repo", false, nil) Close("org/repo") if !closeCalled { t.Error("repoclient.Close not called for Close") @@ -234,10 +234,10 @@ func TestRecreate(t *testing.T) { close = func() error { return nil } - _, _ = Get(context.Background(), "org/repo", nil) + _, _ = Get(context.Background(), "org/repo", false, nil) Close("org/repo") makeCalled, createCalled, initCalled = false, false, false - _, _ = Get(context.Background(), "org/repo", nil) + _, _ = Get(context.Background(), "org/repo", false, nil) if !makeCalled { t.Error("githubrepo.MakeGithubRepo not called for new repo") } @@ -249,3 +249,30 @@ func TestRecreate(t *testing.T) { } Close("org/repo") } + +func TestLocal(t *testing.T) { + repo := "go-git/go-git" + scc, err := Get(context.Background(), repo, true, nil) + if err != nil { + t.Fatalf("Get error: %s", err) + } + branches, err := scc.FetchBranches() + if err != nil { + t.Fatalf("FetchBranches error: %s", err) + } + err = scc.SwitchLocalBranch(branches[0]) + if err != nil { + t.Fatalf("SwitchLocalBranch error: %s", err) + } + + // fetch again and make sure the reference is the same + sccB, err := Get(context.Background(), repo, true, nil) + if err != nil { + t.Fatalf("Get error: %s", err) + } + if scc != sccB { + t.Fatalf("Get error: unexpected different reference returned") + } + + Close(repo) +} diff --git a/whats-new.md b/whats-new.md index b65a1fca..f4b6f248 100644 --- a/whats-new.md +++ b/whats-new.md @@ -4,7 +4,7 @@ Major features and changes added to Allstar. ## Added since last release -- +- Dangerous Workflow policy will now be run for all branches. [Link](https://github.com/ossf/allstar/issues/569) ## Release v3.0