From 29594d4294b704c5f6514b39a5e6d5fa58712b81 Mon Sep 17 00:00:00 2001 From: laurentsimon <64505099+laurentsimon@users.noreply.github.com> Date: Fri, 30 Jul 2021 08:09:52 -0700 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20=20change=20signature=20of=20FileIf?= =?UTF-8?q?Exist=20and=20FileContent=20(#787)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * draft * add pinning * remove functions * typo * commment * name --- checks/automatic_dependency_update.go | 22 +++-- checks/binary_artifact.go | 21 +++-- checks/file_utils.go | 117 +++++++++----------------- checks/permissions.go | 2 +- checks/pinned_dependencies.go | 103 ++++++++++++++++------- checks/security_policy.go | 26 +++--- e2e/binary_artifacts_test.go | 11 +-- 7 files changed, 163 insertions(+), 139 deletions(-) diff --git a/checks/automatic_dependency_update.go b/checks/automatic_dependency_update.go index f5b74181..820e0f47 100644 --- a/checks/automatic_dependency_update.go +++ b/checks/automatic_dependency_update.go @@ -29,30 +29,38 @@ func init() { // AutomaticDependencyUpdate will check the repository if it contains Automatic dependency update. func AutomaticDependencyUpdate(c *checker.CheckRequest) checker.CheckResult { - r, err := CheckIfFileExists(CheckAutomaticDependencyUpdate, c, fileExists) + var r bool + err := CheckIfFileExists(CheckAutomaticDependencyUpdate, c, fileExists, &r) if err != nil { return checker.CreateRuntimeErrorResult(CheckAutomaticDependencyUpdate, err) } if !r { - return checker.CreateMinScoreResult(CheckAutomaticDependencyUpdate, "no tool detected [dependabot|renovabot]") + c.Dlogger.Warn("dependabot not detected") + c.Dlogger.Warn("renovatebot not detected") + return checker.CreateMinScoreResult(CheckAutomaticDependencyUpdate, "no update tool detected") } // High score result. - return checker.CreateMaxScoreResult(CheckAutomaticDependencyUpdate, "tool detected") + return checker.CreateMaxScoreResult(CheckAutomaticDependencyUpdate, "update tool detected") } // fileExists will validate the if frozen dependencies file name exists. -func fileExists(name string, dl checker.DetailLogger) (bool, error) { +func fileExists(name string, dl checker.DetailLogger, data FileCbData) (bool, error) { + pdata := FileGetCbDataAsBoolPointer(data) + switch strings.ToLower(name) { case ".github/dependabot.yml": dl.Info("dependabot detected : %s", name) - return true, nil // https://docs.renovatebot.com/configuration-options/ case ".github/renovate.json", ".github/renovate.json5", ".renovaterc.json", "renovate.json", "renovate.json5", ".renovaterc": dl.Info("renovate detected: %s", name) - return true, nil default: - return false, nil + // Continue iterating. + return true, nil } + + *pdata = true + // We found the file, no need to continue iterating. + return false, nil } diff --git a/checks/binary_artifact.go b/checks/binary_artifact.go index a764f232..1aafd1e8 100644 --- a/checks/binary_artifact.go +++ b/checks/binary_artifact.go @@ -17,6 +17,7 @@ package checks import ( "fmt" "path/filepath" + "strings" "github.com/h2non/filetype" "github.com/h2non/filetype/types" @@ -34,11 +35,12 @@ func init() { // BinaryArtifacts will check the repository if it contains binary artifacts. func BinaryArtifacts(c *checker.CheckRequest) checker.CheckResult { - r, err := CheckFilesContent("*", false, c, checkBinaryFileContent) + var binFound bool + err := CheckFilesContent("*", false, c, checkBinaryFileContent, &binFound) if err != nil { return checker.CreateRuntimeErrorResult(CheckBinaryArtifacts, err) } - if !r { + if binFound { return checker.CreateMinScoreResult(CheckBinaryArtifacts, "binaries present in source code") } @@ -46,7 +48,8 @@ func BinaryArtifacts(c *checker.CheckRequest) checker.CheckResult { } func checkBinaryFileContent(path string, content []byte, - dl checker.DetailLogger) (bool, error) { + dl checker.DetailLogger, data FileCbData) (bool, error) { + pfound := FileGetCbDataAsBoolPointer(data) binaryFileTypes := map[string]bool{ "crx": true, "deb": true, @@ -92,12 +95,14 @@ func checkBinaryFileContent(path string, content []byte, } if _, ok := binaryFileTypes[t.Extension]; ok { - dl.Warn("binary found: %s", path) - return false, nil - } else if _, ok := binaryFileTypes[filepath.Ext(path)]; ok { + dl.Warn("binary detected: %s", path) + *pfound = true + return true, nil + } else if _, ok := binaryFileTypes[strings.ReplaceAll(filepath.Ext(path), ".", "")]; ok { // Falling back to file based extension. - dl.Warn("binary found: %s", path) - return false, nil + dl.Warn("binary detected: %s", path) + *pfound = true + return true, nil } return true, nil diff --git a/checks/file_utils.go b/checks/file_utils.go index 522b40e5..25d73abb 100644 --- a/checks/file_utils.go +++ b/checks/file_utils.go @@ -24,29 +24,6 @@ import ( sce "github.com/ossf/scorecard/v2/errors" ) -// CheckIfFileExists downloads the tar of the repository and calls the onFile() to check -// for the occurrence. -func CheckIfFileExists(checkName string, c *checker.CheckRequest, onFile func(name string, - dl checker.DetailLogger) (bool, error)) (bool, error) { - matchedFiles, err := c.RepoClient.ListFiles(func(string) (bool, error) { return true, nil }) - if err != nil { - // nolint: wrapcheck - return false, err - } - for _, filename := range matchedFiles { - rr, err := onFile(filename, c.Dlogger) - if err != nil { - return false, err - } - - if rr { - return true, nil - } - } - - return false, nil -} - // isMatchingPath uses 'pattern' to shell-match the 'path' and its filename // 'caseSensitive' indicates the match should be case-sensitive. Default: no. func isMatchingPath(pattern, fullpath string, caseSensitive bool) (bool, error) { @@ -79,58 +56,6 @@ func isScorecardTestFile(owner, repo, fullpath string) bool { strings.Contains(fullpath, "/testdata/")) } -// CheckFilesContent downloads the tar of the repository and calls the onFileContent() function -// shellPathFnPattern is used for https://golang.org/pkg/path/#Match -// Warning: the pattern is used to match (1) the entire path AND (2) the filename alone. This means: -// - To scope the search to a directory, use "./dirname/*". Example, for the root directory, -// use "./*". -// - A pattern such as "*mypatern*" will match files containing mypattern in *any* directory. -//nolint -func CheckFilesContent(shellPathFnPattern string, - caseSensitive bool, - c *checker.CheckRequest, - onFileContent func(path string, content []byte, - dl checker.DetailLogger) (bool, error), -) (bool, error) { - predicate := func(filepath string) (bool, error) { - // Filter out Scorecard's own test files. - if isScorecardTestFile(c.Owner, c.Repo, filepath) { - return false, nil - } - // Filter out files based on path/names using the pattern. - b, err := isMatchingPath(shellPathFnPattern, filepath, caseSensitive) - if err != nil { - return false, err - } - return b, nil - } - res := true - matchedFiles, err := c.RepoClient.ListFiles(predicate) - if err != nil { - // nolint: wrapcheck - return false, err - } - for _, file := range matchedFiles { - content, err := c.RepoClient.GetFileContent(file) - if err != nil { - //nolint - return false, err - } - - rr, err := onFileContent(file, content, c.Dlogger) - if err != nil { - return false, err - } - // We don't return rightway to let the onFileContent() - // handler log. - if !rr { - res = false - } - } - - return res, nil -} - // FileCbData is any data the caller can act upon // to keep state. type FileCbData interface{} @@ -141,7 +66,13 @@ type FileCbData interface{} type FileContentCb func(path string, content []byte, dl checker.DetailLogger, data FileCbData) (bool, error) -func CheckFilesContent2(shellPathFnPattern string, +// CheckFilesContent downloads the tar of the repository and calls the onFileContent() function +// shellPathFnPattern is used for https://golang.org/pkg/path/#Match +// Warning: the pattern is used to match (1) the entire path AND (2) the filename alone. This means: +// - To scope the search to a directory, use "./dirname/*". Example, for the root directory, +// use "./*". +// - A pattern such as "*mypatern*" will match files containing mypattern in *any* directory. +func CheckFilesContent(shellPathFnPattern string, caseSensitive bool, c *checker.CheckRequest, onFileContent FileContentCb, @@ -186,6 +117,40 @@ func CheckFilesContent2(shellPathFnPattern string, return nil } +type FileCb func(path string, + dl checker.DetailLogger, data FileCbData) (bool, error) + +// CheckIfFileExists downloads the tar of the repository and calls the onFile() to check +// for the occurrence. +func CheckIfFileExists(checkName string, c *checker.CheckRequest, onFile FileCb, data FileCbData) error { + matchedFiles, err := c.RepoClient.ListFiles(func(string) (bool, error) { return true, nil }) + if err != nil { + // nolint: wrapcheck + return err + } + for _, filename := range matchedFiles { + continueIter, err := onFile(filename, c.Dlogger, data) + if err != nil { + return err + } + + if !continueIter { + break + } + } + + return nil +} + +func FileGetCbDataAsBoolPointer(data FileCbData) *bool { + pdata, ok := data.(*bool) + if !ok { + // This never happens. + panic("invalid type") + } + return pdata +} + // CheckFileContainsCommands checks if the file content contains commands or not. // `comment` is the string or character that indicates a comment: // for example for Dockerfiles, it would be `#`. diff --git a/checks/permissions.go b/checks/permissions.go index b65ff9ca..9ab766f9 100644 --- a/checks/permissions.go +++ b/checks/permissions.go @@ -40,7 +40,7 @@ type permissionCbData struct { func TokenPermissions(c *checker.CheckRequest) checker.CheckResult { data := permissionCbData{writePermissions: make(map[string]bool)} - err := CheckFilesContent2(".github/workflows/*", false, + err := CheckFilesContent(".github/workflows/*", false, c, validateGitHubActionTokenPermissions, &data) return createResultForLeastPrivilegeTokens(data, err) } diff --git a/checks/pinned_dependencies.go b/checks/pinned_dependencies.go index c4a631d1..9df5511b 100644 --- a/checks/pinned_dependencies.go +++ b/checks/pinned_dependencies.go @@ -134,7 +134,8 @@ func createReturnValues(r bool, infoMsg string, dl checker.DetailLogger, err err } func isShellScriptFreeOfInsecureDownloads(c *checker.CheckRequest) (int, error) { - r, err := CheckFilesContent("*", false, c, validateShellScriptIsFreeOfInsecureDownloads) + var r bool + err := CheckFilesContent("*", false, c, validateShellScriptIsFreeOfInsecureDownloads, &r) return createReturnForIsShellScriptFreeOfInsecureDownloads(r, c.Dlogger, err) } @@ -146,21 +147,32 @@ func createReturnForIsShellScriptFreeOfInsecureDownloads(r bool, dl checker.Deta func testValidateShellScriptIsFreeOfInsecureDownloads(pathfn string, content []byte, dl checker.DetailLogger) (int, error) { - r, err := validateShellScriptIsFreeOfInsecureDownloads(pathfn, content, dl) + var r bool + _, err := validateShellScriptIsFreeOfInsecureDownloads(pathfn, content, dl, &r) return createReturnForIsShellScriptFreeOfInsecureDownloads(r, dl, err) } func validateShellScriptIsFreeOfInsecureDownloads(pathfn string, content []byte, - dl checker.DetailLogger) (bool, error) { + dl checker.DetailLogger, data FileCbData) (bool, error) { + pdata := FileGetCbDataAsBoolPointer(data) + // Validate the file type. if !isShellScriptFile(pathfn, content) { + *pdata = true return true, nil } - return validateShellFile(pathfn, content, dl) + + r, err := validateShellFile(pathfn, content, dl) + if err != nil { + return false, err + } + *pdata = r + return true, nil } func isDockerfileFreeOfInsecureDownloads(c *checker.CheckRequest) (int, error) { - r, err := CheckFilesContent("*Dockerfile*", false, c, validateDockerfileIsFreeOfInsecureDownloads) + var r bool + err := CheckFilesContent("*Dockerfile*", false, c, validateDockerfileIsFreeOfInsecureDownloads, &r) return createReturnForIsDockerfileFreeOfInsecureDownloads(r, c.Dlogger, err) } @@ -173,18 +185,23 @@ func createReturnForIsDockerfileFreeOfInsecureDownloads(r bool, dl checker.Detai func testValidateDockerfileIsFreeOfInsecureDownloads(pathfn string, content []byte, dl checker.DetailLogger) (int, error) { - r, err := validateDockerfileIsFreeOfInsecureDownloads(pathfn, content, dl) + var r bool + _, err := validateDockerfileIsFreeOfInsecureDownloads(pathfn, content, dl, &r) return createReturnForIsDockerfileFreeOfInsecureDownloads(r, dl, err) } func validateDockerfileIsFreeOfInsecureDownloads(pathfn string, content []byte, - dl checker.DetailLogger) (bool, error) { + dl checker.DetailLogger, data FileCbData) (bool, error) { + pdata := FileGetCbDataAsBoolPointer(data) + // Return early if this is a script, e.g. script_dockerfile_something.sh if isShellScriptFile(pathfn, content) { + *pdata = true return true, nil } if !CheckFileContainsCommands(content, "#") { + *pdata = true return true, nil } @@ -221,11 +238,19 @@ func validateDockerfileIsFreeOfInsecureDownloads(pathfn string, content []byte, bytes = append(bytes, cmd...) bytes = append(bytes, '\n') } - return validateShellFile(pathfn, bytes, dl) + + r, err := validateShellFile(pathfn, bytes, dl) + if err != nil { + return false, err + } + + *pdata = r + return true, nil } func isDockerfilePinned(c *checker.CheckRequest) (int, error) { - r, err := CheckFilesContent("*Dockerfile*", false, c, validateDockerfileIsPinned) + var r bool + err := CheckFilesContent("*Dockerfile*", false, c, validateDockerfileIsPinned, &r) return createReturnForIsDockerfilePinned(r, c.Dlogger, err) } @@ -237,22 +262,26 @@ func createReturnForIsDockerfilePinned(r bool, dl checker.DetailLogger, err erro } func testValidateDockerfileIsPinned(pathfn string, content []byte, dl checker.DetailLogger) (int, error) { - r, err := validateDockerfileIsPinned(pathfn, content, dl) + var r bool + _, err := validateDockerfileIsPinned(pathfn, content, dl, &r) return createReturnForIsDockerfilePinned(r, dl, err) } func validateDockerfileIsPinned(pathfn string, content []byte, - dl checker.DetailLogger) (bool, error) { + dl checker.DetailLogger, data FileCbData) (bool, error) { // Users may use various names, e.g., // Dockerfile.aarch64, Dockerfile.template, Dockerfile_template, dockerfile, Dockerfile-name.template // Templates may trigger false positives, e.g. FROM { NAME }. + pdata := FileGetCbDataAsBoolPointer(data) // Return early if this is a script, e.g. script_dockerfile_something.sh if isShellScriptFile(pathfn, content) { + *pdata = true return true, nil } if !CheckFileContainsCommands(content, "#") { + *pdata = true return true, nil } @@ -322,11 +351,13 @@ func validateDockerfileIsPinned(pathfn string, content []byte, // The file need not have a FROM statement, // https://github.com/tensorflow/tensorflow/blob/master/tensorflow/tools/dockerfiles/partials/jupyter.partial.Dockerfile. - return ret, nil + *pdata = ret + return true, nil } func isGitHubWorkflowScriptFreeOfInsecureDownloads(c *checker.CheckRequest) (int, error) { - r, err := CheckFilesContent(".github/workflows/*", false, c, validateGitHubWorkflowIsFreeOfInsecureDownloads) + var r bool + err := CheckFilesContent(".github/workflows/*", false, c, validateGitHubWorkflowIsFreeOfInsecureDownloads, &r) return createReturnForIsGitHubWorkflowScriptFreeOfInsecureDownloads(r, c.Dlogger, err) } @@ -340,13 +371,17 @@ func createReturnForIsGitHubWorkflowScriptFreeOfInsecureDownloads(r bool, func testValidateGitHubWorkflowScriptFreeOfInsecureDownloads(pathfn string, content []byte, dl checker.DetailLogger) (int, error) { - r, err := validateGitHubWorkflowIsFreeOfInsecureDownloads(pathfn, content, dl) + var r bool + _, err := validateGitHubWorkflowIsFreeOfInsecureDownloads(pathfn, content, dl, &r) return createReturnForIsGitHubWorkflowScriptFreeOfInsecureDownloads(r, dl, err) } func validateGitHubWorkflowIsFreeOfInsecureDownloads(pathfn string, content []byte, - dl checker.DetailLogger) (bool, error) { + dl checker.DetailLogger, data FileCbData) (bool, error) { + pdata := FileGetCbDataAsBoolPointer(data) + if !CheckFileContainsCommands(content, "#") { + *pdata = true return true, nil } @@ -398,12 +433,14 @@ func validateGitHubWorkflowIsFreeOfInsecureDownloads(pathfn string, content []by } } - return validated, nil + *pdata = validated + return true, nil } // Check pinning of github actions in workflows. func isGitHubActionsWorkflowPinned(c *checker.CheckRequest) (int, error) { - r, err := CheckFilesContent(".github/workflows/*", true, c, validateGitHubActionWorkflow) + var r bool + err := CheckFilesContent(".github/workflows/*", true, c, validateGitHubActionWorkflow, &r) return createReturnForIsGitHubActionsWorkflowPinned(r, c.Dlogger, err) } @@ -415,13 +452,18 @@ func createReturnForIsGitHubActionsWorkflowPinned(r bool, dl checker.DetailLogge } func testIsGitHubActionsWorkflowPinned(pathfn string, content []byte, dl checker.DetailLogger) (int, error) { - r, err := validateGitHubActionWorkflow(pathfn, content, dl) + var r bool + _, err := validateGitHubActionWorkflow(pathfn, content, dl, &r) return createReturnForIsGitHubActionsWorkflowPinned(r, dl, err) } // Check file content. -func validateGitHubActionWorkflow(pathfn string, content []byte, dl checker.DetailLogger) (bool, error) { +func validateGitHubActionWorkflow(pathfn string, content []byte, + dl checker.DetailLogger, data FileCbData) (bool, error) { + pdata := FileGetCbDataAsBoolPointer(data) + if !CheckFileContainsCommands(content, "#") { + *pdata = true return true, nil } @@ -452,12 +494,14 @@ func validateGitHubActionWorkflow(pathfn string, content []byte, dl checker.Deta } } - return ret, nil + *pdata = ret + return true, nil } // Check presence of lock files thru validatePackageManagerFile(). func isPackageManagerLockFilePresent(c *checker.CheckRequest) (int, error) { - r, err := CheckIfFileExists(CheckPinnedDependencies, c, validatePackageManagerFile) + var r bool + err := CheckIfFileExists(CheckPinnedDependencies, c, validatePackageManagerFile, &r) if err != nil { return checker.InconclusiveResultScore, err } @@ -472,37 +516,32 @@ func isPackageManagerLockFilePresent(c *checker.CheckRequest) (int, error) { // validatePackageManagerFile will validate the if frozen dependecies file name exists. // TODO(laurent): need to differentiate between libraries and programs. // TODO(laurent): handle multi-language repos. -func validatePackageManagerFile(name string, dl checker.DetailLogger) (bool, error) { +func validatePackageManagerFile(name string, dl checker.DetailLogger, data FileCbData) (bool, error) { switch strings.ToLower(name) { // TODO(laurent): "go.mod" is for libraries + default: + return true, nil case "go.sum": dl.Info("go lock file detected: %s", name) - return true, nil case "vendor/", "third_party/", "third-party/": dl.Info("vendoring detected in: %s", name) - return true, nil case "package-lock.json", "npm-shrinkwrap.json": dl.Info("javascript lock file detected: %s", name) - return true, nil // TODO(laurent): add check for hashbased pinning in requirements.txt - https://davidwalsh.name/hashin // Note: because requirements.txt does not handle transitive dependencies, we consider it // not a lock file, until we have remediation steps for pip-build. case "pipfile.lock": dl.Info("python lock file detected: %s", name) - return true, nil case "gemfile.lock": dl.Info("ruby lock file detected: %s", name) - return true, nil case "cargo.lock": dl.Info("rust lock file detected: %s", name) - return true, nil case "yarn.lock": dl.Info("yarn lock file detected: %s", name) - return true, nil case "composer.lock": dl.Info("composer lock file detected: %s", name) - return true, nil - default: - return false, nil } + pdata := FileGetCbDataAsBoolPointer(data) + *pdata = true + return true, nil } diff --git a/checks/security_policy.go b/checks/security_policy.go index e647d69b..ecd8310d 100644 --- a/checks/security_policy.go +++ b/checks/security_policy.go @@ -34,18 +34,22 @@ func init() { } func SecurityPolicy(c *checker.CheckRequest) checker.CheckResult { - // check repository for repository-specific policy - onFile := func(name string, dl checker.DetailLogger) (bool, error) { + var r bool + // Check repository for repository-specific policy. + onFile := func(name string, dl checker.DetailLogger, data FileCbData) (bool, error) { + pdata := FileGetCbDataAsBoolPointer(data) if strings.EqualFold(name, "security.md") { c.Dlogger.Info("security policy detected: %s", name) - return true, nil + *pdata = true + return false, nil } else if isSecurityRstFound(name) { c.Dlogger.Info("security policy detected: %s", name) - return true, nil + *pdata = true + return false, nil } - return false, nil + return true, nil } - r, err := CheckIfFileExists(CheckSecurityPolicy, c, onFile) + err := CheckIfFileExists(CheckSecurityPolicy, c, onFile, &r) if err != nil { return checker.CreateRuntimeErrorResult(CheckSecurityPolicy, err) } @@ -64,14 +68,16 @@ func SecurityPolicy(c *checker.CheckRequest) checker.CheckResult { case err == nil: defer dotGitHubClient.Close() dotGitHub.RepoClient = dotGitHubClient - onFile = func(name string, dl checker.DetailLogger) (bool, error) { + onFile = func(name string, dl checker.DetailLogger, data FileCbData) (bool, error) { + pdata := FileGetCbDataAsBoolPointer(data) if strings.EqualFold(name, "security.md") { dl.Info("security policy detected in .github folder: %s", name) - return true, nil + *pdata = true + return false, nil } - return false, nil + return true, nil } - r, err = CheckIfFileExists(CheckSecurityPolicy, dotGitHub, onFile) + err = CheckIfFileExists(CheckSecurityPolicy, dotGitHub, onFile, &r) if err != nil { return checker.CreateRuntimeErrorResult(CheckSecurityPolicy, err) } diff --git a/e2e/binary_artifacts_test.go b/e2e/binary_artifacts_test.go index b3428c57..ab525b5a 100644 --- a/e2e/binary_artifacts_test.go +++ b/e2e/binary_artifacts_test.go @@ -29,7 +29,7 @@ import ( // TODO: use dedicated repo that don't change. // TODO: need negative results. -var _ = Describe("E2E TEST:Binary-Artifacts", func() { +var _ = Describe("E2E TEST:"+checks.CheckBinaryArtifacts, func() { Context("E2E TEST:Binary artifacts are not present in source code", func() { It("Should return not binary artifacts in source code", func() { dl := scut.TestDetailLogger{} @@ -65,22 +65,23 @@ var _ = Describe("E2E TEST:Binary-Artifacts", func() { It("Should return binary artifacts present in source code", func() { dl := scut.TestDetailLogger{} repoClient := githubrepo.CreateGithubRepoClient(context.Background(), ghClient, graphClient) - err := repoClient.InitRepo("a1ive", "grub2-filemanager") + err := repoClient.InitRepo("ossf-tests", "scorecard-check-binary-artifacts-e2e") Expect(err).Should(BeNil()) req := checker.CheckRequest{ Ctx: context.Background(), Client: ghClient, RepoClient: repoClient, - Owner: "a1ive", - Repo: "grub2-filemanager", + Owner: "ossf-tests", + Repo: "scorecard-check-binary-artifacts-e2e", GraphClient: graphClient, Dlogger: &dl, } + // TODO: upload real binaries to the repo as well. expected := scut.TestReturn{ Errors: nil, Score: checker.MinResultScore, - NumberOfWarn: 1, + NumberOfWarn: 35, NumberOfInfo: 0, NumberOfDebug: 0, }