From 529008d7e696b1afff243257262c5e94f85ab9f6 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Mon, 11 Jul 2022 04:56:49 +0000 Subject: [PATCH 1/7] Use crane to add hash suggestion to unpinned Docker images --- checks/evaluation/pinned_dependencies.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index 4701b7e4417..d3845041a04 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -18,6 +18,7 @@ import ( "errors" "fmt" + "github.com/google/go-containerregistry/pkg/crane" "github.com/ossf/scorecard/v4/checker" "github.com/ossf/scorecard/v4/checks/fileparser" sce "github.com/ossf/scorecard/v4/errors" @@ -163,6 +164,11 @@ func generateText(rr *checker.Dependency) string { gitHubOwned := fileparser.IsGitHubOwnedAction(rr.Location.Snippet) owner := generateOwnerToDisplay(gitHubOwned) return fmt.Sprintf("%s %s not pinned by hash", owner, rr.Type) + } else if rr.Type == checker.DependencyUseTypeDockerfileContainerImage { + hash, err := crane.Digest(*rr.Name) + if err == nil { + return fmt.Sprintf("%s not pinned by hash. Fix by updating %[2]s to %[2]s@%s", rr.Type, *rr.Name, hash) + } } return fmt.Sprintf("%s not pinned by hash", rr.Type) From 1efab913699c1a45d56a832d8dfea1679d68fc69 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Mon, 11 Jul 2022 21:55:05 +0000 Subject: [PATCH 2/7] Add nil check before dereferencing name for image digest --- checks/evaluation/pinned_dependencies.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index d3845041a04..d64f05ff6dc 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -159,18 +159,20 @@ func updatePinningResults(rr *checker.Dependency, } func generateText(rr *checker.Dependency) string { - if rr.Type == checker.DependencyUseTypeGHAction { + switch rr.Type { + case checker.DependencyUseTypeGHAction: // Check if we are dealing with a GitHub action or a third-party one. gitHubOwned := fileparser.IsGitHubOwnedAction(rr.Location.Snippet) owner := generateOwnerToDisplay(gitHubOwned) return fmt.Sprintf("%s %s not pinned by hash", owner, rr.Type) - } else if rr.Type == checker.DependencyUseTypeDockerfileContainerImage { - hash, err := crane.Digest(*rr.Name) - if err == nil { + case checker.DependencyUseTypeDockerfileContainerImage: + if rr.Name == nil { + break + } + if hash, err := crane.Digest(*rr.Name); err == nil { // if NO error return fmt.Sprintf("%s not pinned by hash. Fix by updating %[2]s to %[2]s@%s", rr.Type, *rr.Name, hash) } } - return fmt.Sprintf("%s not pinned by hash", rr.Type) } From 203809ae4d4a9d64c59f6a784c8eb802ae627884 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Wed, 13 Jul 2022 19:41:13 +0000 Subject: [PATCH 3/7] Reformat changes to comply with linter --- checks/evaluation/pinned_dependencies.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index d64f05ff6dc..5adbfa14b3b 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -19,6 +19,7 @@ import ( "fmt" "github.com/google/go-containerregistry/pkg/crane" + "github.com/ossf/scorecard/v4/checker" "github.com/ossf/scorecard/v4/checks/fileparser" sce "github.com/ossf/scorecard/v4/errors" @@ -172,6 +173,7 @@ func generateText(rr *checker.Dependency) string { if hash, err := crane.Digest(*rr.Name); err == nil { // if NO error return fmt.Sprintf("%s not pinned by hash. Fix by updating %[2]s to %[2]s@%s", rr.Type, *rr.Name, hash) } + default: } return fmt.Sprintf("%s not pinned by hash", rr.Type) } From cfafacd5df21cb13b21dd85dab56502b8d9fe52b Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Wed, 13 Jul 2022 20:28:03 +0000 Subject: [PATCH 4/7] Add basic remediation for dockerfile pinning --- checks/evaluation/pinned_dependencies.go | 11 +++++++++-- remediation/remediations.go | 19 +++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index 5adbfa14b3b..da6ae5914d4 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -136,10 +136,17 @@ func PinningDependencies(name string, dl checker.DetailLogger, } func generateRemediation(rr *checker.Dependency) *checker.Remediation { - if rr.Type == checker.DependencyUseTypeGHAction { + switch rr.Type { + case checker.DependencyUseTypeGHAction: return remediation.CreateWorkflowPinningRemediation(rr.Location.Path) + case checker.DependencyUseTypeDockerfileContainerImage: + if rr.Name == nil { + return nil + } + return remediation.CreateDockerfilePinningRemediation(*rr.Name) + default: + return nil } - return nil } func updatePinningResults(rr *checker.Dependency, diff --git a/remediation/remediations.go b/remediation/remediations.go index 06555a0caf0..ef3b4455f5b 100644 --- a/remediation/remediations.go +++ b/remediation/remediations.go @@ -20,6 +20,8 @@ import ( "strings" "sync" + "github.com/google/go-containerregistry/pkg/crane" + "github.com/ossf/scorecard/v4/checker" "github.com/ossf/scorecard/v4/clients" ) @@ -37,6 +39,7 @@ var ( workflowText = "update your workflow using https://app.stepsecurity.io/secureworkflow/%s/%s/%s?enable=%s" //nolint workflowMarkdown = "update your workflow using [https://app.stepsecurity.io](https://app.stepsecurity.io/secureworkflow/%s/%s/%s?enable=%s)" + dockerfileText = "pin your Docker image (%[1]s). For linux/amd64 update to %[1]s@%s" ) //nolint:gochecknoinits @@ -94,3 +97,19 @@ func createWorkflowRemediation(path, t string) *checker.Remediation { HelpMarkdown: markdown, } } + +// CreateDockerfilePinningRemediation create remediaiton for pinning Dockerfile images. +func CreateDockerfilePinningRemediation(image string) *checker.Remediation { + hash, err := crane.Digest(image) + if err != nil { + return nil + } + + text := fmt.Sprintf(dockerfileText, image, hash) + markdown := text + + return &checker.Remediation{ + HelpText: text, + HelpMarkdown: markdown, + } +} From c4a2dd36f6a4de2357a4f8c0a6be1b983d4a816d Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Fri, 15 Jul 2022 19:44:22 +0000 Subject: [PATCH 5/7] Deduplicate remediation code --- checks/evaluation/pinned_dependencies.go | 19 +++---------------- remediation/remediations.go | 9 ++++++--- 2 files changed, 9 insertions(+), 19 deletions(-) diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index da6ae5914d4..d68c8621239 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -18,8 +18,6 @@ import ( "errors" "fmt" - "github.com/google/go-containerregistry/pkg/crane" - "github.com/ossf/scorecard/v4/checker" "github.com/ossf/scorecard/v4/checks/fileparser" sce "github.com/ossf/scorecard/v4/errors" @@ -140,10 +138,7 @@ func generateRemediation(rr *checker.Dependency) *checker.Remediation { case checker.DependencyUseTypeGHAction: return remediation.CreateWorkflowPinningRemediation(rr.Location.Path) case checker.DependencyUseTypeDockerfileContainerImage: - if rr.Name == nil { - return nil - } - return remediation.CreateDockerfilePinningRemediation(*rr.Name) + return remediation.CreateDockerfilePinningRemediation(rr.Name) default: return nil } @@ -167,21 +162,13 @@ func updatePinningResults(rr *checker.Dependency, } func generateText(rr *checker.Dependency) string { - switch rr.Type { - case checker.DependencyUseTypeGHAction: + if rr.Type == checker.DependencyUseTypeGHAction { // Check if we are dealing with a GitHub action or a third-party one. gitHubOwned := fileparser.IsGitHubOwnedAction(rr.Location.Snippet) owner := generateOwnerToDisplay(gitHubOwned) return fmt.Sprintf("%s %s not pinned by hash", owner, rr.Type) - case checker.DependencyUseTypeDockerfileContainerImage: - if rr.Name == nil { - break - } - if hash, err := crane.Digest(*rr.Name); err == nil { // if NO error - return fmt.Sprintf("%s not pinned by hash. Fix by updating %[2]s to %[2]s@%s", rr.Type, *rr.Name, hash) - } - default: } + return fmt.Sprintf("%s not pinned by hash", rr.Type) } diff --git a/remediation/remediations.go b/remediation/remediations.go index ef3b4455f5b..34013ad8875 100644 --- a/remediation/remediations.go +++ b/remediation/remediations.go @@ -99,13 +99,16 @@ func createWorkflowRemediation(path, t string) *checker.Remediation { } // CreateDockerfilePinningRemediation create remediaiton for pinning Dockerfile images. -func CreateDockerfilePinningRemediation(image string) *checker.Remediation { - hash, err := crane.Digest(image) +func CreateDockerfilePinningRemediation(name *string) *checker.Remediation { + if name == nil { + return nil + } + hash, err := crane.Digest(*name) if err != nil { return nil } - text := fmt.Sprintf(dockerfileText, image, hash) + text := fmt.Sprintf(dockerfileText, *name, hash) markdown := text return &checker.Remediation{ From e56e743fefe122443e1fddd16f91a19ce3c7fbc9 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Fri, 15 Jul 2022 20:27:34 +0000 Subject: [PATCH 6/7] Remove reference to linux/amd64, as crane digest should be universal --- remediation/remediations.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/remediation/remediations.go b/remediation/remediations.go index 34013ad8875..ba24a6e6c1f 100644 --- a/remediation/remediations.go +++ b/remediation/remediations.go @@ -38,8 +38,8 @@ var errInvalidArg = errors.New("invalid argument") var ( workflowText = "update your workflow using https://app.stepsecurity.io/secureworkflow/%s/%s/%s?enable=%s" //nolint - workflowMarkdown = "update your workflow using [https://app.stepsecurity.io](https://app.stepsecurity.io/secureworkflow/%s/%s/%s?enable=%s)" - dockerfileText = "pin your Docker image (%[1]s). For linux/amd64 update to %[1]s@%s" + workflowMarkdown = "update your workflow using [https://app.stepsecurity.io](https://app.stepsecurity.io/secureworkflow/%s/%s/%s?enable=%s)" + dockerfilePinText = "pin your Docker image by updating %[1]s to %[1]s@%s" ) //nolint:gochecknoinits @@ -108,7 +108,7 @@ func CreateDockerfilePinningRemediation(name *string) *checker.Remediation { return nil } - text := fmt.Sprintf(dockerfileText, *name, hash) + text := fmt.Sprintf(dockerfilePinText, *name, hash) markdown := text return &checker.Remediation{ From d2df929cbf6d17c9b34aa52162b4d8ac46636972 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Fri, 15 Jul 2022 22:09:36 +0000 Subject: [PATCH 7/7] add remediation info to scorecard output. switch to using strings.Builder for more maintainable logic --- pkg/common.go | 26 +++++--- pkg/common_test.go | 144 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 161 insertions(+), 9 deletions(-) create mode 100644 pkg/common_test.go diff --git a/pkg/common.go b/pkg/common.go index 0b45fd4a081..4221c2af92d 100644 --- a/pkg/common.go +++ b/pkg/common.go @@ -32,16 +32,24 @@ func DetailToString(d *checker.CheckDetail, logLevel log.Level) string { return "" } - switch { - case d.Msg.Path != "" && d.Msg.Offset != 0 && d.Msg.EndOffset != 0 && d.Msg.Offset < d.Msg.EndOffset: - return fmt.Sprintf("%s: %s: %s:%d-%d", typeToString(d.Type), d.Msg.Text, d.Msg.Path, d.Msg.Offset, d.Msg.EndOffset) - case d.Msg.Path != "" && d.Msg.Offset != 0: - return fmt.Sprintf("%s: %s: %s:%d", typeToString(d.Type), d.Msg.Text, d.Msg.Path, d.Msg.Offset) - case d.Msg.Path != "" && d.Msg.Offset == 0: - return fmt.Sprintf("%s: %s: %s", typeToString(d.Type), d.Msg.Text, d.Msg.Path) - default: - return fmt.Sprintf("%s: %s", typeToString(d.Type), d.Msg.Text) + var sb strings.Builder + sb.WriteString(fmt.Sprintf("%s: %s", typeToString(d.Type), d.Msg.Text)) + + if d.Msg.Path != "" { + sb.WriteString(fmt.Sprintf(": %s", d.Msg.Path)) + if d.Msg.Offset != 0 { + sb.WriteString(fmt.Sprintf(":%d", d.Msg.Offset)) + } + if d.Msg.EndOffset != 0 && d.Msg.Offset < d.Msg.EndOffset { + sb.WriteString(fmt.Sprintf("-%d", d.Msg.EndOffset)) + } } + + if d.Msg.Remediation != nil { + sb.WriteString(fmt.Sprintf(": %s", d.Msg.Remediation.HelpText)) + } + + return sb.String() } func detailsToString(details []checker.CheckDetail, logLevel log.Level) (string, bool) { diff --git a/pkg/common_test.go b/pkg/common_test.go new file mode 100644 index 00000000000..758dd59e1ec --- /dev/null +++ b/pkg/common_test.go @@ -0,0 +1,144 @@ +// Copyright 2022 Security Scorecard Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package pkg + +import ( + "testing" + + "github.com/ossf/scorecard/v4/checker" + "github.com/ossf/scorecard/v4/log" +) + +func TestDetailString(t *testing.T) { + t.Parallel() + tests := []struct { + name string + detail checker.CheckDetail + log log.Level + want string + }{ + { + name: "ignoreDebug", + detail: checker.CheckDetail{ + Msg: checker.LogMessage{ + Text: "should not appear", + }, + Type: checker.DetailDebug, + }, + log: log.DefaultLevel, + want: "", + }, + { + name: "includeDebug", + detail: checker.CheckDetail{ + Msg: checker.LogMessage{ + Text: "should appear", + }, + Type: checker.DetailDebug, + }, + log: log.DebugLevel, + want: "Debug: should appear", + }, + { + name: "onlyType", + detail: checker.CheckDetail{ + Msg: checker.LogMessage{ + Text: "some meaningful text", + }, + Type: checker.DetailWarn, + }, + log: log.DefaultLevel, + want: "Warn: some meaningful text", + }, + { + name: "displayPath", + detail: checker.CheckDetail{ + Msg: checker.LogMessage{ + Text: "some meaningful text", + Path: "Dockerfile", + }, + Type: checker.DetailWarn, + }, + log: log.DefaultLevel, + want: "Warn: some meaningful text: Dockerfile", + }, + { + name: "displayStartOffset", + detail: checker.CheckDetail{ + Msg: checker.LogMessage{ + Text: "some meaningful text", + Path: "Dockerfile", + Offset: 1, + }, + Type: checker.DetailWarn, + }, + log: log.DefaultLevel, + want: "Warn: some meaningful text: Dockerfile:1", + }, + { + name: "displayEndOffset", + detail: checker.CheckDetail{ + Msg: checker.LogMessage{ + Text: "some meaningful text", + Path: "Dockerfile", + Offset: 1, + EndOffset: 7, + }, + Type: checker.DetailWarn, + }, + log: log.DefaultLevel, + want: "Warn: some meaningful text: Dockerfile:1-7", + }, + { + name: "ignoreInvalidEndOffset", + detail: checker.CheckDetail{ + Msg: checker.LogMessage{ + Text: "some meaningful text", + Path: "Dockerfile", + Offset: 3, + EndOffset: 2, + }, + Type: checker.DetailWarn, + }, + log: log.DefaultLevel, + want: "Warn: some meaningful text: Dockerfile:3", + }, + { + name: "includeRemediation", + detail: checker.CheckDetail{ + Msg: checker.LogMessage{ + Text: "some meaningful text", + Path: "Dockerfile", + Remediation: &checker.Remediation{ + HelpText: "fix x by doing y", + }, + }, + Type: checker.DetailWarn, + }, + log: log.DefaultLevel, + want: "Warn: some meaningful text: Dockerfile: fix x by doing y", + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := DetailToString(&tt.detail, tt.log) + if got != tt.want { + t.Errorf("got %v, want %v", got, tt.want) + } + }) + } +}