Skip to content

Commit

Permalink
🌱 Add dependency remediation in raw results instead of at log time (o…
Browse files Browse the repository at this point in the history 
…ssf#3632)

* 🌱 Add dependency remediation in raw results instead of at log time

Signed-off-by: AdamKorcz <[email protected]>

* add unit test

Signed-off-by: AdamKorcz <[email protected]>

* add unit test

Signed-off-by: AdamKorcz <[email protected]>

* return error

Signed-off-by: AdamKorcz <[email protected]>

* use pointer to dependency

Signed-off-by: AdamKorcz <[email protected]>

* check for errors in test

Signed-off-by: AdamKorcz <[email protected]>

* Return nil if repo client returns an error from unsupported feature

Signed-off-by: AdamKorcz <[email protected]>

* revert error checking

Signed-off-by: AdamKorcz <[email protected]>

* revert returning nil is unsupported feature

Signed-off-by: AdamKorcz <[email protected]>

* Fix wrong test name

Signed-off-by: AdamKorcz <[email protected]>

* only create remediation when required

Signed-off-by: AdamKorcz <[email protected]>

* remove remediation helper function

Signed-off-by: AdamKorcz <[email protected]>

---------

Signed-off-by: AdamKorcz <[email protected]>
Signed-off-by: Spencer Schrock <[email protected]>
  • Loading branch information
@AdamKorcz
AdamKorcz authored Nov 9, 2023
1 parent 2c959b7 commit b3d1a5a
Show file tree
Hide file tree
Showing 4 changed files with 254 additions and 24 deletions.
14 changes: 8 additions & 6 deletions checker/raw_result.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
17 changes: 1 addition & 16 deletions checks/evaluation/pinned_dependencies.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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,
) {
Expand Down
31 changes: 29 additions & 2 deletions checks/raw/pinned_dependencies.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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
Expand Down
216 changes: 216 additions & 0 deletions checks/raw/pinned_dependencies_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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)
}
}
})
}
}

0 comments on commit b3d1a5a

Please sign in to comment.