From e4f3ede843eda9b7d62b5b81f806ecacfdd1d4a4 Mon Sep 17 00:00:00 2001 From: laurentsimon <64505099+laurentsimon@users.noreply.github.com> Date: Tue, 3 Aug 2021 16:32:34 -0700 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20=20fix/enhance=20pinned-dependencie?= =?UTF-8?q?s=20(#806)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * commit * e2e tests * typo --- checks/pinned_dependencies.go | 131 +++++++++++++++++++++----------- e2e/pinned_dependencies_test.go | 10 +-- 2 files changed, 91 insertions(+), 50 deletions(-) diff --git a/checks/pinned_dependencies.go b/checks/pinned_dependencies.go index 9df5511b..2c611094 100644 --- a/checks/pinned_dependencies.go +++ b/checks/pinned_dependencies.go @@ -54,11 +54,11 @@ type gitHubActionWorkflowConfig struct { //nolint:gochecknoinits func init() { - registerCheck(CheckPinnedDependencies, FrozenDeps) + registerCheck(CheckPinnedDependencies, PinnedDependencies) } -// FrozenDeps will check the repository if it contains frozen dependecies. -func FrozenDeps(c *checker.CheckRequest) checker.CheckResult { +// PinnedDependencies will check the repository if it contains frozen dependecies. +func PinnedDependencies(c *checker.CheckRequest) checker.CheckResult { // Lock file. lockScore, lockErr := isPackageManagerLockFilePresent(c) if lockErr != nil { @@ -121,25 +121,63 @@ func maxScore(s1, s2 int) int { return s2 } -func createReturnValues(r bool, infoMsg string, dl checker.DetailLogger, err error) (int, error) { +type pinnedResult int + +const ( + pinnedUndefined pinnedResult = iota + pinned + notPinned +) + +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 dataAsResultPointer(data FileCbData) *pinnedResult { + pdata, ok := data.(*pinnedResult) + if !ok { + // This never happens. + panic("invalid type") + } + return pdata +} + +func createReturnValues(r pinnedResult, infoMsg string, dl checker.DetailLogger, err error) (int, error) { if err != nil { return checker.InconclusiveResultScore, err } - if !r { + + switch r { + default: + panic("invalid value") + case pinned, pinnedUndefined: + dl.Info(infoMsg) + return checker.MaxResultScore, nil + case notPinned: + // No logging needed as it's done by the checks. return checker.MinResultScore, nil } - - dl.Info(infoMsg) - return checker.MaxResultScore, nil } func isShellScriptFreeOfInsecureDownloads(c *checker.CheckRequest) (int, error) { - var r bool + var r pinnedResult err := CheckFilesContent("*", false, c, validateShellScriptIsFreeOfInsecureDownloads, &r) return createReturnForIsShellScriptFreeOfInsecureDownloads(r, c.Dlogger, err) } -func createReturnForIsShellScriptFreeOfInsecureDownloads(r bool, dl checker.DetailLogger, err error) (int, error) { +func createReturnForIsShellScriptFreeOfInsecureDownloads(r pinnedResult, + dl checker.DetailLogger, err error) (int, error) { return createReturnValues(r, "no insecure (unpinned) dependency downloads found in shell scripts", dl, err) @@ -147,18 +185,18 @@ func createReturnForIsShellScriptFreeOfInsecureDownloads(r bool, dl checker.Deta func testValidateShellScriptIsFreeOfInsecureDownloads(pathfn string, content []byte, dl checker.DetailLogger) (int, error) { - var r bool + var r pinnedResult _, err := validateShellScriptIsFreeOfInsecureDownloads(pathfn, content, dl, &r) return createReturnForIsShellScriptFreeOfInsecureDownloads(r, dl, err) } func validateShellScriptIsFreeOfInsecureDownloads(pathfn string, content []byte, dl checker.DetailLogger, data FileCbData) (bool, error) { - pdata := FileGetCbDataAsBoolPointer(data) + pdata := dataAsResultPointer(data) // Validate the file type. if !isShellScriptFile(pathfn, content) { - *pdata = true + addPinnedResult(pdata, true) return true, nil } @@ -166,18 +204,20 @@ func validateShellScriptIsFreeOfInsecureDownloads(pathfn string, content []byte, if err != nil { return false, err } - *pdata = r + + addPinnedResult(pdata, r) return true, nil } func isDockerfileFreeOfInsecureDownloads(c *checker.CheckRequest) (int, error) { - var r bool + var r pinnedResult err := CheckFilesContent("*Dockerfile*", false, c, validateDockerfileIsFreeOfInsecureDownloads, &r) return createReturnForIsDockerfileFreeOfInsecureDownloads(r, c.Dlogger, err) } // Create the result. -func createReturnForIsDockerfileFreeOfInsecureDownloads(r bool, dl checker.DetailLogger, err error) (int, error) { +func createReturnForIsDockerfileFreeOfInsecureDownloads(r pinnedResult, + dl checker.DetailLogger, err error) (int, error) { return createReturnValues(r, "no insecure (unpinned) dependency downloads found in Dockerfiles", dl, err) @@ -185,23 +225,23 @@ func createReturnForIsDockerfileFreeOfInsecureDownloads(r bool, dl checker.Detai func testValidateDockerfileIsFreeOfInsecureDownloads(pathfn string, content []byte, dl checker.DetailLogger) (int, error) { - var r bool + var r pinnedResult _, err := validateDockerfileIsFreeOfInsecureDownloads(pathfn, content, dl, &r) return createReturnForIsDockerfileFreeOfInsecureDownloads(r, dl, err) } func validateDockerfileIsFreeOfInsecureDownloads(pathfn string, content []byte, dl checker.DetailLogger, data FileCbData) (bool, error) { - pdata := FileGetCbDataAsBoolPointer(data) + pdata := dataAsResultPointer(data) // Return early if this is a script, e.g. script_dockerfile_something.sh if isShellScriptFile(pathfn, content) { - *pdata = true + addPinnedResult(pdata, true) return true, nil } if !CheckFileContainsCommands(content, "#") { - *pdata = true + addPinnedResult(pdata, true) return true, nil } @@ -244,25 +284,25 @@ func validateDockerfileIsFreeOfInsecureDownloads(pathfn string, content []byte, return false, err } - *pdata = r + addPinnedResult(pdata, r) return true, nil } func isDockerfilePinned(c *checker.CheckRequest) (int, error) { - var r bool + var r pinnedResult err := CheckFilesContent("*Dockerfile*", false, c, validateDockerfileIsPinned, &r) return createReturnForIsDockerfilePinned(r, c.Dlogger, err) } // Create the result. -func createReturnForIsDockerfilePinned(r bool, dl checker.DetailLogger, err error) (int, error) { +func createReturnForIsDockerfilePinned(r pinnedResult, dl checker.DetailLogger, err error) (int, error) { return createReturnValues(r, "Dockerfile dependencies are pinned", dl, err) } func testValidateDockerfileIsPinned(pathfn string, content []byte, dl checker.DetailLogger) (int, error) { - var r bool + var r pinnedResult _, err := validateDockerfileIsPinned(pathfn, content, dl, &r) return createReturnForIsDockerfilePinned(r, dl, err) } @@ -273,15 +313,15 @@ func validateDockerfileIsPinned(pathfn string, content []byte, // Dockerfile.aarch64, Dockerfile.template, Dockerfile_template, dockerfile, Dockerfile-name.template // Templates may trigger false positives, e.g. FROM { NAME }. - pdata := FileGetCbDataAsBoolPointer(data) + pdata := dataAsResultPointer(data) // Return early if this is a script, e.g. script_dockerfile_something.sh if isShellScriptFile(pathfn, content) { - *pdata = true + addPinnedResult(pdata, true) return true, nil } if !CheckFileContainsCommands(content, "#") { - *pdata = true + addPinnedResult(pdata, true) return true, nil } @@ -351,18 +391,18 @@ 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. - *pdata = ret + addPinnedResult(pdata, ret) return true, nil } func isGitHubWorkflowScriptFreeOfInsecureDownloads(c *checker.CheckRequest) (int, error) { - var r bool + var r pinnedResult err := CheckFilesContent(".github/workflows/*", false, c, validateGitHubWorkflowIsFreeOfInsecureDownloads, &r) return createReturnForIsGitHubWorkflowScriptFreeOfInsecureDownloads(r, c.Dlogger, err) } // Create the result. -func createReturnForIsGitHubWorkflowScriptFreeOfInsecureDownloads(r bool, +func createReturnForIsGitHubWorkflowScriptFreeOfInsecureDownloads(r pinnedResult, dl checker.DetailLogger, err error) (int, error) { return createReturnValues(r, "no insecure (unpinned) dependency downloads found in GitHub workflows", @@ -371,17 +411,17 @@ func createReturnForIsGitHubWorkflowScriptFreeOfInsecureDownloads(r bool, func testValidateGitHubWorkflowScriptFreeOfInsecureDownloads(pathfn string, content []byte, dl checker.DetailLogger) (int, error) { - var r bool + var r pinnedResult _, err := validateGitHubWorkflowIsFreeOfInsecureDownloads(pathfn, content, dl, &r) return createReturnForIsGitHubWorkflowScriptFreeOfInsecureDownloads(r, dl, err) } func validateGitHubWorkflowIsFreeOfInsecureDownloads(pathfn string, content []byte, dl checker.DetailLogger, data FileCbData) (bool, error) { - pdata := FileGetCbDataAsBoolPointer(data) + pdata := dataAsResultPointer(data) if !CheckFileContainsCommands(content, "#") { - *pdata = true + addPinnedResult(pdata, true) return true, nil } @@ -433,26 +473,26 @@ func validateGitHubWorkflowIsFreeOfInsecureDownloads(pathfn string, content []by } } - *pdata = validated + addPinnedResult(pdata, validated) return true, nil } // Check pinning of github actions in workflows. func isGitHubActionsWorkflowPinned(c *checker.CheckRequest) (int, error) { - var r bool + var r pinnedResult err := CheckFilesContent(".github/workflows/*", true, c, validateGitHubActionWorkflow, &r) return createReturnForIsGitHubActionsWorkflowPinned(r, c.Dlogger, err) } // Create the result. -func createReturnForIsGitHubActionsWorkflowPinned(r bool, dl checker.DetailLogger, err error) (int, error) { +func createReturnForIsGitHubActionsWorkflowPinned(r pinnedResult, dl checker.DetailLogger, err error) (int, error) { return createReturnValues(r, "GitHub actions are pinned", dl, err) } func testIsGitHubActionsWorkflowPinned(pathfn string, content []byte, dl checker.DetailLogger) (int, error) { - var r bool + var r pinnedResult _, err := validateGitHubActionWorkflow(pathfn, content, dl, &r) return createReturnForIsGitHubActionsWorkflowPinned(r, dl, err) } @@ -460,10 +500,10 @@ func testIsGitHubActionsWorkflowPinned(pathfn string, content []byte, dl checker // Check file content. func validateGitHubActionWorkflow(pathfn string, content []byte, dl checker.DetailLogger, data FileCbData) (bool, error) { - pdata := FileGetCbDataAsBoolPointer(data) + pdata := dataAsResultPointer(data) if !CheckFileContainsCommands(content, "#") { - *pdata = true + addPinnedResult(pdata, true) return true, nil } @@ -494,18 +534,18 @@ func validateGitHubActionWorkflow(pathfn string, content []byte, } } - *pdata = ret + addPinnedResult(pdata, ret) return true, nil } // Check presence of lock files thru validatePackageManagerFile(). func isPackageManagerLockFilePresent(c *checker.CheckRequest) (int, error) { - var r bool + var r pinnedResult err := CheckIfFileExists(CheckPinnedDependencies, c, validatePackageManagerFile, &r) if err != nil { return checker.InconclusiveResultScore, err } - if !r { + if r != pinned { c.Dlogger.Warn("no lock files detected for a package manager") return checker.InconclusiveResultScore, nil } @@ -541,7 +581,8 @@ func validatePackageManagerFile(name string, dl checker.DetailLogger, data FileC case "composer.lock": dl.Info("composer lock file detected: %s", name) } - pdata := FileGetCbDataAsBoolPointer(data) - *pdata = true - return true, nil + + pdata := dataAsResultPointer(data) + addPinnedResult(pdata, true) + return false, nil } diff --git a/e2e/pinned_dependencies_test.go b/e2e/pinned_dependencies_test.go index 0ad2b1fa..829d0c8a 100644 --- a/e2e/pinned_dependencies_test.go +++ b/e2e/pinned_dependencies_test.go @@ -11,7 +11,7 @@ // 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. - +//nolint:dupl package e2e import ( @@ -48,12 +48,12 @@ var _ = Describe("E2E TEST:"+checks.CheckPinnedDependencies, func() { } expected := scut.TestReturn{ Errors: nil, - Score: 3, - NumberOfWarn: 149, - NumberOfInfo: 2, + Score: checker.MinResultScore, + NumberOfWarn: 154, + NumberOfInfo: 0, NumberOfDebug: 0, } - result := checks.FrozenDeps(&req) + result := checks.PinnedDependencies(&req) // UPGRADEv2: to remove. // Old version. Expect(result.Error).Should(BeNil())