Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Dependency-Pinning: only score detected ecosystems #3436

Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
2b2dd82
feat: Define if dependency is pinned or unpinned
gabibguti Feb 14, 2023
9638a92
refactor: Convert diff var types to pointer
gabibguti Feb 16, 2023
e497a20
fix: Pinned Dependency field type
gabibguti Feb 16, 2023
3f3dd80
feat: Count pinned and unpinned deps
gabibguti Aug 28, 2023
309158a
feat: Flag not applicable ecossystems
gabibguti Aug 28, 2023
14f69b1
feat: Score only applicable ecossystems
gabibguti Aug 28, 2023
6ff571b
feat: If no dependencies then create inconclusive score
gabibguti Aug 28, 2023
7b15634
test: GitHub Actions score and logs
gabibguti Aug 29, 2023
104f969
test: Pinned dependencies score
gabibguti Aug 29, 2023
5f67732
test: Ecossystems score and logs
gabibguti Aug 29, 2023
5691e0d
test: Remove deleted maxScore function test
gabibguti Aug 29, 2023
84757ea
test: Adding GitHub Actions dependencies to result
gabibguti Aug 29, 2023
5509c47
test: Update GitHub Actions result
gabibguti Aug 29, 2023
9a787c4
test: Update pip installs result
gabibguti Aug 29, 2023
423cac8
fix: Handle if nuget dependency is pinned or unpinned
gabibguti Apr 18, 2023
9056875
tests: Fix check warnings for unpinned dependencies
gabibguti Aug 29, 2023
c1a81fe
fix: Linter errors
gabibguti Aug 29, 2023
c8ee14a
fix: GitHub Actions pinned log
gabibguti Aug 29, 2023
7f7b89d
test: Fix "ossf-tests/scorecard-check-pinned-dependencies-e2e"
gabibguti Aug 29, 2023
a1b1781
Revert rename `asPointer` to `asStringPointer`
gabibguti Aug 31, 2023
5adf329
fix: Handle deps with parsing error and undefined pinning
gabibguti Aug 31, 2023
1ad766b
test: Delete unecessary test
gabibguti Aug 31, 2023
7a2a3a4
test: Add missing dep Location cases
gabibguti Aug 31, 2023
4412f0e
fix: Simplify Dockerfile pinned as name logic
gabibguti Aug 31, 2023
512659c
fix: If ecossystem is not found show debug log
gabibguti Aug 31, 2023
9731d8a
test: Fix e2e tests and more unit tests
gabibguti Aug 31, 2023
08bd85e
feat: Iterate all dependency types for final score
gabibguti Aug 31, 2023
e52be0b
feat: Proportional score
gabibguti Sep 4, 2023
29be827
fix: GHA weights in proportional score
gabibguti Sep 4, 2023
5dc6464
test: Fix scores and logs checking
gabibguti Sep 4, 2023
e8667cc
test: Fix e2e test
gabibguti Sep 4, 2023
407773c
refactor: Rename to ProportionalScoreWeighted
gabibguti Sep 6, 2023
a68194b
refactor: Var declarations to create proportional score
gabibguti Sep 6, 2023
aa23b15
fix: Remove unnecessary pointer
gabibguti Sep 6, 2023
e478307
fix: Dependencies priority declaration
gabibguti Sep 6, 2023
99f4dc6
fix: Ecosystem spelling
gabibguti Sep 13, 2023
7000eb5
fix: Handle 0 weight and 0 total when creating proportional weighted …
gabibguti Sep 13, 2023
6390493
fix: Revert -d flag identification change
gabibguti Sep 13, 2023
3187362
fix: npm ci command is npm download and is pinned
gabibguti Sep 13, 2023
2484ddc
fix: Linter errors
gabibguti Sep 13, 2023
24f3a8f
fix: Unexport error variable to other packages
gabibguti Sep 15, 2023
f839462
refactor: Simplify no score groups condition
gabibguti Sep 15, 2023
ca725e0
feat: Log proportion of dependencies pinned
gabibguti Sep 19, 2023
f6ccf91
test: Fix unit tests to include info logs
gabibguti Sep 19, 2023
9baf547
test: Fix e2e tests to include info logs
gabibguti Sep 19, 2023
b986c65
fix: Linter error
gabibguti Sep 19, 2023
2e3e9a5
Merge branch 'main' into fix/count-existing-ecossystems-for-pinned-deps
spencerschrock Sep 25, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions attestor/policy/attestation_policy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ func TestCheckCodeReviewed(t *testing.T) {
}
}

func asPointer(s string) *string {
func asStringPointer(s string) *string {
return &s
}

Expand All @@ -399,7 +399,7 @@ func TestNoUnpinnedDependencies(t *testing.T) {
raw: &checker.RawResults{
PinningDependenciesResults: checker.PinningDependenciesData{
Dependencies: []checker.Dependency{
{Name: asPointer("foo"), PinnedAt: asPointer("abcdef")},
{Name: asStringPointer("foo"), PinnedAt: asStringPointer("abcdef")},
},
},
},
Expand All @@ -413,7 +413,7 @@ func TestNoUnpinnedDependencies(t *testing.T) {
raw: &checker.RawResults{
PinningDependenciesResults: checker.PinningDependenciesData{
Dependencies: []checker.Dependency{
{Name: asPointer("foo"), PinnedAt: nil},
{Name: asStringPointer("foo"), PinnedAt: nil},
},
},
},
Expand All @@ -427,7 +427,7 @@ func TestNoUnpinnedDependencies(t *testing.T) {
raw: &checker.RawResults{
PinningDependenciesResults: checker.PinningDependenciesData{
Dependencies: []checker.Dependency{
{Name: asPointer("foo"), PinnedAt: nil},
{Name: asStringPointer("foo"), PinnedAt: nil},
},
},
},
Expand All @@ -439,7 +439,7 @@ func TestNoUnpinnedDependencies(t *testing.T) {
raw: &checker.RawResults{
PinningDependenciesResults: checker.PinningDependenciesData{
Dependencies: []checker.Dependency{
{Name: asPointer("second-pkg"), Location: &checker.File{Path: "bar"}, PinnedAt: nil},
{Name: asStringPointer("second-pkg"), Location: &checker.File{Path: "bar"}, PinnedAt: nil},
},
},
},
Expand Down Expand Up @@ -651,7 +651,7 @@ func TestAttestationPolicy_EvaluateResults(t *testing.T) {
raw: &checker.RawResults{
PinningDependenciesResults: checker.PinningDependenciesData{
Dependencies: []checker.Dependency{
{Name: asPointer("foo"), PinnedAt: asPointer("abcdef")},
{Name: asStringPointer("foo"), PinnedAt: asStringPointer("abcdef")},
},
},
},
Expand Down
1 change: 1 addition & 0 deletions checker/raw_result.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ type Dependency struct {
PinnedAt *string
Location *File
Msg *string // Only for debug messages.
Pinned *bool
spencerschrock marked this conversation as resolved.
Show resolved Hide resolved
Type DependencyUseType
}

Expand Down
169 changes: 91 additions & 78 deletions checks/evaluation/pinned_dependencies.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
package evaluation

import (
"errors"
"fmt"

"github.com/ossf/scorecard/v4/checker"
Expand All @@ -26,15 +25,10 @@ import (
"github.com/ossf/scorecard/v4/rule"
)

var errInvalidValue = errors.New("invalid value")

type pinnedResult int

const (
pinnedUndefined pinnedResult = iota
pinned
notPinned
)
type pinnedResult struct {
pinned int
total int
}

// Structure to host information about pinned github
// or third party dependencies.
Expand Down Expand Up @@ -80,7 +74,7 @@ func PinningDependencies(name string, c *checker.CheckRequest,
Text: *rr.Msg,
Snippet: rr.Location.Snippet,
})
} else {
} else if !*rr.Pinned {
dl.Warn(&checker.LogMessage{
Path: rr.Location.Path,
Type: rr.Location.Type,
Expand All @@ -90,10 +84,10 @@ func PinningDependencies(name string, c *checker.CheckRequest,
Snippet: rr.Location.Snippet,
Remediation: generateRemediation(remediationMetadata, &rr),
})

// Update the pinning status.
updatePinningResults(&rr, &wp, pr)
}

// Update the pinning status.
updatePinningResults(&rr, &wp, pr)
spencerschrock marked this conversation as resolved.
Show resolved Hide resolved
}

// Generate scores and Info results.
Expand Down Expand Up @@ -139,17 +133,22 @@ func PinningDependencies(name string, c *checker.CheckRequest,
return checker.CreateRuntimeErrorResult(name, err)
}

// Scores may be inconclusive.
actionScore = maxScore(0, actionScore)
dockerFromScore = maxScore(0, dockerFromScore)
dockerDownloadScore = maxScore(0, dockerDownloadScore)
scriptScore = maxScore(0, scriptScore)
pipScore = maxScore(0, pipScore)
npmScore = maxScore(0, npmScore)
goScore = maxScore(0, goScore)
// If no dependencies of an ecossystem are found, it results in an inconclusive score.
// We filter out inconclusive scores so only applicable ecossystems are considered in
// the final score.
scores := []int{actionScore, dockerFromScore, dockerDownloadScore, scriptScore, pipScore, npmScore, goScore}
spencerschrock marked this conversation as resolved.
Show resolved Hide resolved
conclusiveScores := []int{}
for i := range scores {
if scores[i] != checker.InconclusiveResultScore {
conclusiveScores = append(conclusiveScores, scores[i])
}
}

if len(conclusiveScores) == 0 {
return checker.CreateInconclusiveResult(name, "no dependencies found")
}

score := checker.AggregateScores(actionScore, dockerFromScore,
dockerDownloadScore, scriptScore, pipScore, npmScore, goScore)
score := checker.AggregateScores(conclusiveScores...)
spencerschrock marked this conversation as resolved.
Show resolved Hide resolved

if score == checker.MaxResultScore {
return checker.CreateMaxScoreResult(name, "all dependencies are pinned")
Expand Down Expand Up @@ -177,13 +176,13 @@ func updatePinningResults(rr *checker.Dependency,
// Note: `Snippet` contains `action/name@xxx`, so we cna use it to infer
// if it's a GitHub-owned action or not.
gitHubOwned := fileparser.IsGitHubOwnedAction(rr.Location.Snippet)
addWorkflowPinnedResult(wp, false, gitHubOwned)
addWorkflowPinnedResult(rr, wp, gitHubOwned)
return
}

// Update other result types.
var p pinnedResult
addPinnedResult(&p, false)
p := pr[rr.Type]
addPinnedResult(rr, &p)
pr[rr.Type] = p
}

Expand All @@ -205,36 +204,18 @@ func generateOwnerToDisplay(gitHubOwned bool) string {
return "third-party"
}

// TODO(laurent): need to support GCB pinning.
func maxScore(s1, s2 int) int {
if s1 > s2 {
return s1
func addPinnedResult(rr *checker.Dependency, r *pinnedResult) {
if *rr.Pinned {
r.pinned += 1
}
return s2
r.total += 1
}

// For the 'to' param, true means the file is pinning dependencies (or there are no dependencies),
// false means there are unpinned dependencies.
func addPinnedResult(r *pinnedResult, to bool) {
// If the result is `notPinned`, we keep it.
// In other cases, we always update the result.
if *r == notPinned {
return
}

switch to {
case true:
*r = pinned
case false:
*r = notPinned
}
}

func addWorkflowPinnedResult(w *worklowPinningResult, to, isGitHub bool) {
func addWorkflowPinnedResult(rr *checker.Dependency, w *worklowPinningResult, isGitHub bool) {
if isGitHub {
addPinnedResult(&w.gitHubOwned, to)
addPinnedResult(rr, &w.gitHubOwned)
} else {
addPinnedResult(&w.thirdParties, to)
addPinnedResult(rr, &w.thirdParties)
}
}

Expand All @@ -244,6 +225,7 @@ func createReturnForIsShellScriptFreeOfInsecureDownloads(pr map[checker.Dependen
) (int, error) {
return createReturnValues(pr, checker.DependencyUseTypeDownloadThenRun,
"no insecure (not pinned by hash) dependency downloads found in shell scripts",
"no dependency downloads found in shell scripts",
dl)
}

Expand All @@ -253,6 +235,7 @@ func createReturnForIsDockerfilePinned(pr map[checker.DependencyUseType]pinnedRe
) (int, error) {
return createReturnValues(pr, checker.DependencyUseTypeDockerfileContainerImage,
"Dockerfile dependencies are pinned",
"no Dockerfile dependencies found",
dl)
}

Expand All @@ -262,6 +245,7 @@ func createReturnForIsDockerfileFreeOfInsecureDownloads(pr map[checker.Dependenc
) (int, error) {
return createReturnValues(pr, checker.DependencyUseTypeDownloadThenRun,
"no insecure (not pinned by hash) dependency downloads found in Dockerfiles",
"no dependency downloads found in Dockerfiles",
dl)
}

Expand All @@ -270,7 +254,8 @@ func createReturnForIsPipInstallPinned(pr map[checker.DependencyUseType]pinnedRe
dl checker.DetailLogger,
) (int, error) {
return createReturnValues(pr, checker.DependencyUseTypePipCommand,
"Pip installs are pinned",
"pip installs are pinned",
"no pip installs found",
dl)
}

Expand All @@ -280,6 +265,7 @@ func createReturnForIsNpmInstallPinned(pr map[checker.DependencyUseType]pinnedRe
) (int, error) {
return createReturnValues(pr, checker.DependencyUseTypeNpmCommand,
"npm installs are pinned",
"no npm installs found",
dl)
}

Expand All @@ -289,27 +275,33 @@ func createReturnForIsGoInstallPinned(pr map[checker.DependencyUseType]pinnedRes
) (int, error) {
return createReturnValues(pr, checker.DependencyUseTypeGoCommand,
"go installs are pinned",
"no go installs found",
dl)
}

func createReturnValues(pr map[checker.DependencyUseType]pinnedResult,
t checker.DependencyUseType, infoMsg string,
t checker.DependencyUseType, maxResultMsg string, inconclusiveResultMsg string,
dl checker.DetailLogger,
) (int, error) {
// Note: we don't check if the entry exists,
// as it will have the default value which is handled in the switch statement.
// If there are zero dependencies of this type, it will generate an inconclusive score,
// so we can disconsider it in the aggregated score.
// If all dependencies of this type are pinned, it will get a maximum score.
// If 1 or more dependencies of this type are unpinned, it will get a minimum score.
//nolint
r, _ := pr[t]
switch r {
default:
return checker.InconclusiveResultScore, fmt.Errorf("%w: %v", errInvalidValue, r)
case pinned, pinnedUndefined:
r := pr[t]

switch r.total {
case 0:
dl.Info(&checker.LogMessage{
spencerschrock marked this conversation as resolved.
Show resolved Hide resolved
Text: inconclusiveResultMsg,
})
return checker.InconclusiveResultScore, nil
case r.pinned:
dl.Info(&checker.LogMessage{
Text: infoMsg,
Text: maxResultMsg,
})
return checker.MaxResultScore, nil
case notPinned:
// No logging needed as it's done by the checks.
default:
return checker.MinResultScore, nil
}
}
Expand All @@ -318,31 +310,52 @@ func createReturnValues(pr map[checker.DependencyUseType]pinnedResult,
func createReturnForIsGitHubActionsWorkflowPinned(wp worklowPinningResult, dl checker.DetailLogger) (int, error) {
return createReturnValuesForGitHubActionsWorkflowPinned(wp,
fmt.Sprintf("%ss are pinned", checker.DependencyUseTypeGHAction),
fmt.Sprintf("%ss found", checker.DependencyUseTypeGHAction),
dl)
}

func createReturnValuesForGitHubActionsWorkflowPinned(r worklowPinningResult, infoMsg string,
dl checker.DetailLogger,
func createReturnValuesForGitHubActionsWorkflowPinned(r worklowPinningResult, maxResultMsg string,
inconclusiveResultMsg string, dl checker.DetailLogger,
) (int, error) {
score := checker.MinResultScore

if r.gitHubOwned != notPinned {
score += 2
if r.gitHubOwned.total == 0 {
dl.Info(&checker.LogMessage{
spencerschrock marked this conversation as resolved.
Show resolved Hide resolved
Type: finding.FileTypeSource,
Offset: checker.OffsetDefault,
Text: fmt.Sprintf("%s %s", "GitHub-owned", infoMsg),
Text: fmt.Sprintf("%s %s", "no GitHub-owned", inconclusiveResultMsg),
})
}

if r.thirdParties != notPinned {
score += 8
if r.thirdParties.total == 0 {
dl.Info(&checker.LogMessage{
spencerschrock marked this conversation as resolved.
Show resolved Hide resolved
Type: finding.FileTypeSource,
Offset: checker.OffsetDefault,
Text: fmt.Sprintf("%s %s", "Third-party", infoMsg),
Text: fmt.Sprintf("%s %s", "no Third-party", inconclusiveResultMsg),
})
}

if r.gitHubOwned.total == 0 && r.thirdParties.total == 0 {
return checker.InconclusiveResultScore, nil
}

score := checker.MinResultScore

if r.gitHubOwned.total == r.gitHubOwned.pinned {
score += 2
if r.gitHubOwned.total != 0 {
dl.Info(&checker.LogMessage{
Type: finding.FileTypeSource,
Offset: checker.OffsetDefault,
Text: fmt.Sprintf("%s %s", "GitHub-owned", maxResultMsg),
})
}
}

if r.thirdParties.total == r.thirdParties.pinned {
score += 8
if r.thirdParties.total != 0 {
dl.Info(&checker.LogMessage{
Type: finding.FileTypeSource,
Offset: checker.OffsetDefault,
Text: fmt.Sprintf("%s %s", "Third-party", maxResultMsg),
})
}
}

return score, nil
}
Loading
Loading