From fcdfdb728087e9b2079678a7294aee8d1e22ca6e Mon Sep 17 00:00:00 2001 From: Arnout Engelen Date: Wed, 29 May 2024 14:32:26 +0200 Subject: [PATCH 1/3] :bug: fix Docker remediations for unpinned GHA dependencies Previously, as both the check for unpinned dependencies in GitHub Actions and the check for unpinned Docker dependencies contribute to d.Dependencies, the loop that created remediations for Docker dependencies would also create try to create Docker remediations for the unpinned GitHub Actions dependencies. This could get really slow, especially when scanning a repo with many GitHub Actions such as https://github.com/apache/beam. Signed-off-by: Arnout Engelen --- checks/raw/pinned_dependencies.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/checks/raw/pinned_dependencies.go b/checks/raw/pinned_dependencies.go index 1471c19fa89..26436f09ca4 100644 --- a/checks/raw/pinned_dependencies.go +++ b/checks/raw/pinned_dependencies.go @@ -230,7 +230,7 @@ func collectDockerfilePinning(c *checker.CheckRequest, r *checker.PinningDepende for i := range r.Dependencies { rr := &r.Dependencies[i] - if !*rr.Pinned { + if rr.Type == checker.DependencyUseTypeDockerfileContainerImage && !*rr.Pinned { remediate := remediation.CreateDockerfilePinningRemediation(rr, remediation.CraneDigester{}) rr.Remediation = remediate } @@ -485,7 +485,7 @@ func collectGitHubActionsWorkflowPinning(c *checker.CheckRequest, r *checker.Pin for i := range r.Dependencies { rr := &r.Dependencies[i] - if !*rr.Pinned { + if rr.Type == checker.DependencyUseTypeGHAction && !*rr.Pinned { remediate := remediationMetadata.CreateWorkflowPinningRemediation(rr.Location.Path) rr.Remediation = remediate } From 0a2dcf012ba8adc76ed9006a03d5adc79c337192 Mon Sep 17 00:00:00 2001 From: Arnout Engelen Date: Wed, 29 May 2024 16:32:41 +0200 Subject: [PATCH 2/3] :seedling: Small refactor and test for remediations Signed-off-by: Arnout Engelen --- checks/raw/pinned_dependencies.go | 22 +++++--- checks/raw/pinned_dependencies_test.go | 70 ++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 7 deletions(-) diff --git a/checks/raw/pinned_dependencies.go b/checks/raw/pinned_dependencies.go index 26436f09ca4..f3d2d077be7 100644 --- a/checks/raw/pinned_dependencies.go +++ b/checks/raw/pinned_dependencies.go @@ -228,14 +228,18 @@ func collectDockerfilePinning(c *checker.CheckRequest, r *checker.PinningDepende return err } - for i := range r.Dependencies { - rr := &r.Dependencies[i] + applyDockerfilePinningRemediations(r.Dependencies) + return nil +} + +func applyDockerfilePinningRemediations(d []checker.Dependency) { + for i := range d { + rr := &d[i] if rr.Type == checker.DependencyUseTypeDockerfileContainerImage && !*rr.Pinned { remediate := remediation.CreateDockerfilePinningRemediation(rr, remediation.CraneDigester{}) rr.Remediation = remediate } } - return nil } var validateDockerfilesPinning fileparser.DoWhileTrueOnFileContent = func( @@ -483,14 +487,18 @@ func collectGitHubActionsWorkflowPinning(c *checker.CheckRequest, r *checker.Pin //nolint:errcheck remediationMetadata, _ := remediation.New(c) - for i := range r.Dependencies { - rr := &r.Dependencies[i] + applyWorkflowPinningRemediations(remediationMetadata, r.Dependencies) + return nil +} + +func applyWorkflowPinningRemediations(rm *remediation.RemediationMetadata, d []checker.Dependency) { + for i := range d { + rr := &d[i] if rr.Type == checker.DependencyUseTypeGHAction && !*rr.Pinned { - remediate := remediationMetadata.CreateWorkflowPinningRemediation(rr.Location.Path) + remediate := rm.CreateWorkflowPinningRemediation(rr.Location.Path) rr.Remediation = remediate } } - 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 dedd64bb04a..d3a7586ed06 100644 --- a/checks/raw/pinned_dependencies_test.go +++ b/checks/raw/pinned_dependencies_test.go @@ -28,6 +28,7 @@ import ( "github.com/ossf/scorecard/v5/checker" mockrepo "github.com/ossf/scorecard/v5/clients/mockclients" "github.com/ossf/scorecard/v5/finding" + "github.com/ossf/scorecard/v5/remediation" scut "github.com/ossf/scorecard/v5/utests" ) @@ -415,6 +416,75 @@ func TestDockerfilePinning(t *testing.T) { } } +func TestMixedPinning(t *testing.T) { + t.Parallel() + + workflowDependency := 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(false), + Type: "GitHubAction", + Remediation: nil, + } + dockerDependency := 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, + Type: 0, + }, + Pinned: boolAsPointer(false), + Type: "containerImage", + } + dependencies := []checker.Dependency{ + workflowDependency, + dockerDependency, + } + + //nolint:errcheck + remediationMetadata, _ := remediation.New(nil) + remediationMetadata.Branch = "mybranch" + remediationMetadata.Repo = "myrepo" + + applyWorkflowPinningRemediations(remediationMetadata, dependencies) + if dependencies[0].Remediation == nil { + t.Errorf("No remediation added to workflow dependency") + return + } + if !strings.Contains(dependencies[0].Remediation.Text, "update your workflow") { + t.Errorf("Unexpected workflow remediation text") + } + if dependencies[1].Remediation != nil { + t.Errorf("Unexpected docker remediation") + return + } + applyDockerfilePinningRemediations(dependencies) + if dependencies[0].Remediation == nil { + t.Errorf("Remediation disappeared from workflow dependency") + } + if !strings.Contains(dependencies[0].Remediation.Text, "update your workflow") { + t.Errorf("Unexpected workflow remediation text") + } + if dependencies[1].Remediation == nil { + t.Errorf("No remediation added to docker dependency") + return + } + if !strings.Contains(dependencies[1].Remediation.Text, "Docker") { + t.Errorf("Unexpected Docker remediation text") + } +} + func TestFileIsInVendorDir(t *testing.T) { t.Parallel() tests := []struct { From 1d01c0121294c26db8f66884720dab9dfe719d07 Mon Sep 17 00:00:00 2001 From: Arnout Engelen Date: Thu, 30 May 2024 09:27:46 +0200 Subject: [PATCH 3/3] :seedling: make test data more realistic Signed-off-by: Arnout Engelen --- checks/raw/pinned_dependencies_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/checks/raw/pinned_dependencies_test.go b/checks/raw/pinned_dependencies_test.go index d3a7586ed06..c86ed60a6db 100644 --- a/checks/raw/pinned_dependencies_test.go +++ b/checks/raw/pinned_dependencies_test.go @@ -420,13 +420,13 @@ func TestMixedPinning(t *testing.T) { t.Parallel() workflowDependency := checker.Dependency{ - Name: stringAsPointer("actions/checkout"), - PinnedAt: stringAsPointer("daadedc81d5f9d3c06d2c92f49202a3cc2b919ba"), + Name: stringAsPointer("github/codeql-action/analyze"), + PinnedAt: nil, Location: &checker.File{ Path: ".github/workflows/workflow-not-pinned.yaml", - Snippet: "actions/checkout@daadedc81d5f9d3c06d2c92f49202a3cc2b919ba", - Offset: 31, - EndOffset: 31, + Snippet: "github/codeql-action/analyze@v1", + Offset: 79, + EndOffset: 79, FileSize: 0, Type: 1, },