From b3d1a5ac457d134c035d9af98081221295e60072 Mon Sep 17 00:00:00 2001 From: AdamKorcz <44787359+AdamKorcz@users.noreply.github.com> Date: Thu, 9 Nov 2023 18:32:06 +0000 Subject: [PATCH] :seedling: Add dependency remediation in raw results instead of at log time (#3632) * :seedling: Add dependency remediation in raw results instead of at log time Signed-off-by: AdamKorcz * add unit test Signed-off-by: AdamKorcz * add unit test Signed-off-by: AdamKorcz * return error Signed-off-by: AdamKorcz * use pointer to dependency Signed-off-by: AdamKorcz * check for errors in test Signed-off-by: AdamKorcz * Return nil if repo client returns an error from unsupported feature Signed-off-by: AdamKorcz * revert error checking Signed-off-by: AdamKorcz * revert returning nil is unsupported feature Signed-off-by: AdamKorcz * Fix wrong test name Signed-off-by: AdamKorcz * only create remediation when required Signed-off-by: AdamKorcz * remove remediation helper function Signed-off-by: AdamKorcz --------- Signed-off-by: AdamKorcz Signed-off-by: Spencer Schrock --- checker/raw_result.go | 14 +- checks/evaluation/pinned_dependencies.go | 17 +- checks/raw/pinned_dependencies.go | 31 +++- checks/raw/pinned_dependencies_test.go | 216 +++++++++++++++++++++++ 4 files changed, 254 insertions(+), 24 deletions(-) diff --git a/checker/raw_result.go b/checker/raw_result.go index 697fa07b9bc..abac71eecf2 100644 --- a/checker/raw_result.go +++ b/checker/raw_result.go @@ -20,6 +20,7 @@ import ( "github.com/ossf/scorecard/v4/clients" "github.com/ossf/scorecard/v4/finding" + "github.com/ossf/scorecard/v4/rule" ) // RawResults contains results before a policy @@ -120,12 +121,13 @@ type PinningDependenciesData struct { type Dependency struct { // TODO: unique dependency name. // TODO: Job *WorkflowJob - Name *string - PinnedAt *string - Location *File - Msg *string // Only for debug messages. - Pinned *bool - Type DependencyUseType + Name *string + PinnedAt *string + Location *File + Msg *string // Only for debug messages. + Pinned *bool + Remediation *rule.Remediation + Type DependencyUseType } // MaintainedData contains the raw results diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index bba2a16be9d..b5672bc10b8 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -21,8 +21,6 @@ import ( "github.com/ossf/scorecard/v4/checks/fileparser" sce "github.com/ossf/scorecard/v4/errors" "github.com/ossf/scorecard/v4/finding" - "github.com/ossf/scorecard/v4/remediation" - "github.com/ossf/scorecard/v4/rule" ) type pinnedResult struct { @@ -63,8 +61,6 @@ func PinningDependencies(name string, c *checker.CheckRequest, var wp worklowPinningResult pr := make(map[checker.DependencyUseType]pinnedResult) dl := c.Dlogger - //nolint:errcheck - remediationMetadata, _ := remediation.New(c) for _, e := range r.ProcessingErrors { e := e @@ -118,7 +114,7 @@ func PinningDependencies(name string, c *checker.CheckRequest, EndOffset: rr.Location.EndOffset, Text: generateTextUnpinned(&rr), Snippet: rr.Location.Snippet, - Remediation: generateRemediation(remediationMetadata, &rr), + Remediation: rr.Remediation, }) } // Update the pinning status. @@ -159,17 +155,6 @@ func PinningDependencies(name string, c *checker.CheckRequest, "dependency not pinned by hash detected", score, checker.MaxResultScore) } -func generateRemediation(remediationMd *remediation.RemediationMetadata, rr *checker.Dependency) *rule.Remediation { - switch rr.Type { - case checker.DependencyUseTypeGHAction: - return remediationMd.CreateWorkflowPinningRemediation(rr.Location.Path) - case checker.DependencyUseTypeDockerfileContainerImage: - return remediation.CreateDockerfilePinningRemediation(rr, remediation.CraneDigester{}) - default: - return nil - } -} - func updatePinningResults(rr *checker.Dependency, wp *worklowPinningResult, pr map[checker.DependencyUseType]pinnedResult, ) { diff --git a/checks/raw/pinned_dependencies.go b/checks/raw/pinned_dependencies.go index deae08f08b8..0d5fe791cb7 100644 --- a/checks/raw/pinned_dependencies.go +++ b/checks/raw/pinned_dependencies.go @@ -28,6 +28,7 @@ import ( "github.com/ossf/scorecard/v4/checks/fileparser" sce "github.com/ossf/scorecard/v4/errors" "github.com/ossf/scorecard/v4/finding" + "github.com/ossf/scorecard/v4/remediation" ) // PinningDependencies checks for (un)pinned dependencies. @@ -188,10 +189,22 @@ func isDockerfile(pathfn string, content []byte) bool { } func collectDockerfilePinning(c *checker.CheckRequest, r *checker.PinningDependenciesData) error { - return fileparser.OnMatchingFileContentDo(c.RepoClient, fileparser.PathMatcher{ + err := fileparser.OnMatchingFileContentDo(c.RepoClient, fileparser.PathMatcher{ Pattern: "*Dockerfile*", CaseSensitive: false, }, validateDockerfilesPinning, r) + if err != nil { + return err + } + + for i := range r.Dependencies { + rr := &r.Dependencies[i] + if !*rr.Pinned { + remdtion := remediation.CreateDockerfilePinningRemediation(rr, remediation.CraneDigester{}) + rr.Remediation = remdtion + } + } + return nil } var validateDockerfilesPinning fileparser.DoWhileTrueOnFileContent = func( @@ -424,10 +437,24 @@ var validateGitHubWorkflowIsFreeOfInsecureDownloads fileparser.DoWhileTrueOnFile // Check pinning of github actions in workflows. func collectGitHubActionsWorkflowPinning(c *checker.CheckRequest, r *checker.PinningDependenciesData) error { - return fileparser.OnMatchingFileContentDo(c.RepoClient, fileparser.PathMatcher{ + err := fileparser.OnMatchingFileContentDo(c.RepoClient, fileparser.PathMatcher{ Pattern: ".github/workflows/*", CaseSensitive: true, }, validateGitHubActionWorkflow, r) + if err != nil { + return err + } + //nolint:errcheck + remediationMetadata, _ := remediation.New(c) + + for i := range r.Dependencies { + rr := &r.Dependencies[i] + if !*rr.Pinned { + remdtion := remediationMetadata.CreateWorkflowPinningRemediation(rr.Location.Path) + rr.Remediation = remdtion + } + } + return nil } // validateGitHubActionWorkflow checks if the workflow file contains unpinned actions. Returns true if the check diff --git a/checks/raw/pinned_dependencies_test.go b/checks/raw/pinned_dependencies_test.go index 70b7381ba7f..4650b5b5661 100644 --- a/checks/raw/pinned_dependencies_test.go +++ b/checks/raw/pinned_dependencies_test.go @@ -15,14 +15,19 @@ package raw import ( + "fmt" "os" + "path/filepath" "strings" "testing" + "github.com/golang/mock/gomock" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/ossf/scorecard/v4/checker" + mockrepo "github.com/ossf/scorecard/v4/clients/mockclients" + "github.com/ossf/scorecard/v4/rule" scut "github.com/ossf/scorecard/v4/utests" ) @@ -1546,3 +1551,214 @@ func countUnpinned(r []checker.Dependency) int { return unpinned } + +func stringAsPointer(s string) *string { + return &s +} + +func boolAsPointer(b bool) *bool { + return &b +} + +// TestCollectDockerfilePinning tests the collectDockerfilePinning function. +func TestCollectDockerfilePinning(t *testing.T) { + t.Parallel() + tests := []struct { + name string + filename string + outcomeDependencies []checker.Dependency + expectError bool + }{ + { + name: "Workflow with error", + filename: "./testdata/.github/workflows/github-workflow-download-lines.yaml", + expectError: true, + }, + { + name: "Pinned dockerfile", + filename: "./testdata/Dockerfile-pinned", + expectError: false, + outcomeDependencies: []checker.Dependency{ + { + Name: stringAsPointer("python"), + PinnedAt: stringAsPointer("3.7@sha256:45b23dee08af5e43a7fea6c4cf9c25ccf269ee113168c19722f87876677c5cb2"), + Location: &checker.File{ + Path: "./testdata/Dockerfile-pinned", + Snippet: "FROM python:3.7@sha256:45b23dee08af5e43a7fea6c4cf9c25ccf269ee113168c19722f87876677c5cb2", + Offset: 16, + EndOffset: 16, + Type: 1, + }, + Pinned: boolAsPointer(true), + Type: "containerImage", + }, + }, + }, + { + name: "Non-pinned dockerfile", + filename: "./testdata/Dockerfile-not-pinned", + expectError: false, + outcomeDependencies: []checker.Dependency{ + { + Name: stringAsPointer("python"), + PinnedAt: stringAsPointer("3.7"), + Location: &checker.File{ + Path: "./testdata/Dockerfile-not-pinned", + Snippet: "FROM python:3.7", + Offset: 17, + EndOffset: 17, + FileSize: 0, + Type: 1, + }, + Pinned: boolAsPointer(false), + Type: "containerImage", + Remediation: &rule.Remediation{ + Text: "pin your Docker image by updating python:3.7 to python:3.7" + + "@sha256:eedf63967cdb57d8214db38ce21f105003ed4e4d0358f02bedc057341bcf92a0", + Markdown: "pin your Docker image by updating python:3.7 to python:3.7" + + "@sha256:eedf63967cdb57d8214db38ce21f105003ed4e4d0358f02bedc057341bcf92a0", + }, + }, + }, + }, + } + + for _, tt := range tests { + tt := tt // Re-initializing variable so it is not changed while executing the closure below + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mockRepoClient := mockrepo.NewMockRepoClient(ctrl) + mockRepoClient.EXPECT().ListFiles(gomock.Any()).Return([]string{tt.filename}, nil).AnyTimes() + mockRepoClient.EXPECT().GetDefaultBranchName().Return("main", nil).AnyTimes() + mockRepoClient.EXPECT().URI().Return("github.com/ossf/scorecard").AnyTimes() + mockRepoClient.EXPECT().GetFileContent(gomock.Any()).DoAndReturn(func(file string) ([]byte, error) { + // This will read the file and return the content + content, err := os.ReadFile(file) + if err != nil { + return content, fmt.Errorf("%w", err) + } + return content, nil + }) + + req := checker.CheckRequest{ + RepoClient: mockRepoClient, + } + var r checker.PinningDependenciesData + err := collectDockerfilePinning(&req, &r) + if err != nil { + if !tt.expectError { + t.Error(err.Error()) + } + } + for i := range tt.outcomeDependencies { + outcomeDependency := &tt.outcomeDependencies[i] + depend := &r.Dependencies[i] + if diff := cmp.Diff(outcomeDependency, depend); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + } + }) + } +} + +// TestCollectGitHubActionsWorkflowPinning tests the collectGitHubActionsWorkflowPinning function. +func TestCollectGitHubActionsWorkflowPinning(t *testing.T) { + t.Parallel() + tests := []struct { + name string + filename string + outcomeDependencies []checker.Dependency + expectError bool + }{ + { + name: "Pinned dockerfile", + filename: "Dockerfile-empty", + expectError: true, + }, + { + name: "Pinned workflow", + filename: ".github/workflows/workflow-pinned.yaml", + expectError: false, + outcomeDependencies: []checker.Dependency{ + { + Name: stringAsPointer("actions/checkout"), + PinnedAt: stringAsPointer("daadedc81d5f9d3c06d2c92f49202a3cc2b919ba"), + Location: &checker.File{ + Path: ".github/workflows/workflow-pinned.yaml", + Snippet: "actions/checkout@daadedc81d5f9d3c06d2c92f49202a3cc2b919ba", + Offset: 31, + EndOffset: 31, + Type: 1, + }, + Pinned: boolAsPointer(true), + Type: "GitHubAction", + Remediation: nil, + }, + }, + }, + { + name: "Non-pinned workflow", + filename: ".github/workflows/workflow-not-pinned.yaml", + expectError: false, + outcomeDependencies: []checker.Dependency{ + { + Name: stringAsPointer("actions/checkout"), + PinnedAt: stringAsPointer("daadedc81d5f9d3c06d2c92f49202a3cc2b919ba"), + Location: &checker.File{ + Path: ".github/workflows/workflow-not-pinned.yaml", + Snippet: "actions/checkout@daadedc81d5f9d3c06d2c92f49202a3cc2b919ba", + Offset: 31, + EndOffset: 31, + FileSize: 0, + Type: 1, + }, + Pinned: boolAsPointer(true), + Type: "GitHubAction", + Remediation: nil, + }, + }, + }, + } + + for _, tt := range tests { + tt := tt // Re-initializing variable so it is not changed while executing the closure below + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mockRepoClient := mockrepo.NewMockRepoClient(ctrl) + mockRepoClient.EXPECT().ListFiles(gomock.Any()).Return([]string{tt.filename}, nil).AnyTimes() + mockRepoClient.EXPECT().GetDefaultBranchName().Return("main", nil).AnyTimes() + mockRepoClient.EXPECT().URI().Return("github.com/ossf/scorecard").AnyTimes() + mockRepoClient.EXPECT().GetFileContent(gomock.Any()).DoAndReturn(func(file string) ([]byte, error) { + // This will read the file and return the content + content, err := os.ReadFile(filepath.Join("testdata", file)) + if err != nil { + return content, fmt.Errorf("%w", err) + } + return content, nil + }) + + req := checker.CheckRequest{ + RepoClient: mockRepoClient, + } + var r checker.PinningDependenciesData + err := collectGitHubActionsWorkflowPinning(&req, &r) + if err != nil { + if !tt.expectError { + t.Error(err.Error()) + } + } + t.Log(r.Dependencies) + for i := range tt.outcomeDependencies { + outcomeDependency := &tt.outcomeDependencies[i] + depend := &r.Dependencies[i] + if diff := cmp.Diff(outcomeDependency, depend); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + } + }) + } +}