From 82246648f49cb8374514f6c21126bc6f9037016b Mon Sep 17 00:00:00 2001 From: Eran Turgeman <81029514+eranturgeman@users.noreply.github.com> Date: Sun, 13 Oct 2024 10:38:00 +0300 Subject: [PATCH] Enable allow-partial-results for Audit SCA scan and dep tree construction (#200) --- cli/docs/flags.go | 10 +- cli/scancommands.go | 3 +- commands/audit/audit.go | 27 +++- commands/audit/audit_test.go | 144 ++++++++++++++++++ commands/audit/scarunner.go | 12 +- .../npm/npm-un-installable/package.json | 14 ++ utils/auditbasicparams.go | 10 ++ utils/results.go | 4 +- 8 files changed, 213 insertions(+), 11 deletions(-) create mode 100644 tests/testdata/projects/package-managers/npm/npm-un-installable/package.json diff --git a/cli/docs/flags.go b/cli/docs/flags.go index c9173fb0..1f2c0ce4 100644 --- a/cli/docs/flags.go +++ b/cli/docs/flags.go @@ -119,6 +119,7 @@ const ( WorkingDirs = "working-dirs" OutputDir = "output-dir" SkipAutoInstall = "skip-auto-install" + AllowPartialResults = "allow-partial-results" // Unique curation flags CurationOutput = "curation-format" @@ -155,7 +156,7 @@ var commandFlags = map[string][]string{ url, user, password, accessToken, ServerId, InsecureTls, Project, Watches, RepoPath, Licenses, OutputFormat, ExcludeTestDeps, useWrapperAudit, DepType, RequirementsFile, Fail, ExtendedTable, WorkingDirs, ExclusionsAudit, Mvn, Gradle, Npm, Pnpm, Yarn, Go, Nuget, Pip, Pipenv, Poetry, MinSeverity, FixableOnly, ThirdPartyContextualAnalysis, Threads, - Sca, Iac, Sast, Secrets, WithoutCA, ScanVuln, SecretValidation, OutputDir, SkipAutoInstall, + Sca, Iac, Sast, Secrets, WithoutCA, ScanVuln, SecretValidation, OutputDir, SkipAutoInstall, AllowPartialResults, }, CurationAudit: { CurationOutput, WorkingDirs, Threads, RequirementsFile, @@ -230,9 +231,10 @@ var flagsMap = map[string]components.Flag{ "Set to false if you wish to not use the gradle or maven wrapper.", components.WithBoolDefaultValue(true), ), - WorkingDirs: components.NewStringFlag(WorkingDirs, "A comma-separated list of relative working directories, to determine audit targets locations."), - OutputDir: components.NewStringFlag(OutputDir, "Target directory to save partial results to.", components.SetHiddenStrFlag()), - SkipAutoInstall: components.NewBoolFlag(SkipAutoInstall, "Set to true to skip auto-install of dependencies in un-built modules. Currently supported for Yarn and NPM only.", components.SetHiddenBoolFlag()), + WorkingDirs: components.NewStringFlag(WorkingDirs, "A comma-separated list of relative working directories, to determine audit targets locations."), + OutputDir: components.NewStringFlag(OutputDir, "Target directory to save partial results to.", components.SetHiddenStrFlag()), + SkipAutoInstall: components.NewBoolFlag(SkipAutoInstall, "Set to true to skip auto-install of dependencies in un-built modules. Currently supported for Yarn and NPM only.", components.SetHiddenBoolFlag()), + AllowPartialResults: components.NewBoolFlag(AllowPartialResults, "Set to true to allow partial results and continuance of the scan in case of certain errors.", components.SetHiddenBoolFlag()), ExclusionsAudit: components.NewStringFlag( Exclusions, "List of exclusions separated by semicolons, utilized to skip sub-projects from undergoing an audit. These exclusions may incorporate the * and ? wildcards.", diff --git a/cli/scancommands.go b/cli/scancommands.go index a84747cd..fe553370 100644 --- a/cli/scancommands.go +++ b/cli/scancommands.go @@ -478,7 +478,8 @@ func CreateAuditCmd(c *components.Context) (*audit.AuditCommand, error) { SetFixableOnly(c.GetBoolFlagValue(flags.FixableOnly)). SetThirdPartyApplicabilityScan(c.GetBoolFlagValue(flags.ThirdPartyContextualAnalysis)). SetScansResultsOutputDir(scansOutputDir). - SetSkipAutoInstall(c.GetBoolFlagValue(flags.SkipAutoInstall)) + SetSkipAutoInstall(c.GetBoolFlagValue(flags.SkipAutoInstall)). + SetAllowPartialResults(c.GetBoolFlagValue(flags.AllowPartialResults)) if c.GetStringFlagValue(flags.Watches) != "" { auditCmd.SetWatches(splitByCommaAndTrim(c.GetStringFlagValue(flags.Watches))) diff --git a/commands/audit/audit.go b/commands/audit/audit.go index 85ec44f2..13b3d2af 100644 --- a/commands/audit/audit.go +++ b/commands/audit/audit.go @@ -202,7 +202,8 @@ func RunAudit(auditParams *AuditParams) (results *utils.Results, err error) { } // The sca scan doesn't require the analyzer manager, so it can run separately from the analyzer manager download routine. if scaScanErr := buildDepTreeAndRunScaScan(auditParallelRunner, auditParams, results); scaScanErr != nil { - auditParallelRunner.AddErrorToChan(scaScanErr) + // If error to be caught, we add it to the auditParallelRunner error queue and continue. The error need not be returned + _ = createErrorIfPartialResultsDisabled(auditParams, auditParallelRunner, fmt.Sprintf("An error has occurred during SCA scan process. SCA scan is skipped for the following directories: %s.", auditParams.workingDirs), scaScanErr) } go func() { auditParallelRunner.ScaScansWg.Wait() @@ -277,3 +278,27 @@ func downloadAnalyzerManagerAndRunScanners(auditParallelRunner *utils.SecurityPa } return } + +// This function checks if partial results are allowed. If so we log the error and continue. +// If partial results are not allowed and a SecurityParallelRunner is provided we add the error to its error queue and return without an error, since the errors will be later collected from the queue. +// If partial results are not allowed and a SecurityParallelRunner is not provided we return the error. +func createErrorIfPartialResultsDisabled(auditParams *AuditParams, auditParallelRunner *utils.SecurityParallelRunner, extraMassageForLog string, err error) error { + if err == nil { + return nil + } + + if auditParams.AllowPartialResults() { + if extraMassageForLog == "" { + extraMassageForLog = "An error has occurred during the audit scans" + } + log.Warn(fmt.Sprintf("%s\nSince partial results are allowed, the error is skipped: %s", extraMassageForLog, err.Error())) + return nil + } + + // When SecurityParallelRunner is provided we add the error to the queue, otherwise we return the error + if auditParallelRunner != nil { + auditParallelRunner.AddErrorToChan(err) + return nil + } + return err +} diff --git a/commands/audit/audit_test.go b/commands/audit/audit_test.go index 2f86776c..45c99863 100644 --- a/commands/audit/audit_test.go +++ b/commands/audit/audit_test.go @@ -1,6 +1,9 @@ package audit import ( + "errors" + "fmt" + "net/http" "path/filepath" "strings" "testing" @@ -197,3 +200,144 @@ func searchForStrWithSubString(t *testing.T, filesList []string, subString strin } assert.Fail(t, "File %s not found in the list", subString) } + +func TestAuditWithPartialResults(t *testing.T) { + testcases := []struct { + name string + allowPartialResults bool + useJas bool + testDirPath string + }{ + { + name: "Failure in SCA during dependency tree construction", + allowPartialResults: false, + useJas: false, + testDirPath: filepath.Join("..", "..", "tests", "testdata", "projects", "package-managers", "npm", "npm-un-installable"), + }, + { + name: "Failure in SCA during scan itself", + allowPartialResults: false, + useJas: false, + testDirPath: filepath.Join("..", "..", "tests", "testdata", "projects", "package-managers", "npm", "npm-project"), + }, + { + name: "Skip failure in SCA during dependency tree construction", + allowPartialResults: true, + useJas: false, + testDirPath: filepath.Join("..", "..", "tests", "testdata", "projects", "package-managers", "npm", "npm-un-installable"), + }, + { + name: "Skip failure in SCA during scan itself", + allowPartialResults: true, + useJas: false, + testDirPath: filepath.Join("..", "..", "tests", "testdata", "projects", "package-managers", "npm", "npm-project"), + }, + // TODO when applying allow-partial-results to JAS make sure to add a test case that checks failures in JAS scans + add some JAS api call to the mock server + } + + serverMock, serverDetails := utils.CreateXrayRestsMockServer(func(w http.ResponseWriter, r *http.Request) { + if r.RequestURI == "/xray/api/v1/system/version" { + _, err := w.Write([]byte(fmt.Sprintf(`{"xray_version": "%s", "xray_revision": "xxx"}`, scangraph.GraphScanMinXrayVersion))) + if !assert.NoError(t, err) { + return + } + } + if strings.HasPrefix(r.RequestURI, "/xray/api/v1/scan/graph") && r.Method == http.MethodPost { + // We set SCA scan graph API to fail + w.WriteHeader(http.StatusBadRequest) + } + }) + defer serverMock.Close() + + for _, testcase := range testcases { + t.Run(testcase.name, func(t *testing.T) { + tempDirPath, createTempDirCallback := coreTests.CreateTempDirWithCallbackAndAssert(t) + defer createTempDirCallback() + + assert.NoError(t, biutils.CopyDir(testcase.testDirPath, tempDirPath, false, nil)) + + auditBasicParams := (&utils.AuditBasicParams{}). + SetServerDetails(serverDetails). + SetOutputFormat(format.Table). + SetUseJas(testcase.useJas). + SetAllowPartialResults(testcase.allowPartialResults) + + auditParams := NewAuditParams(). + SetWorkingDirs([]string{tempDirPath}). + SetGraphBasicParams(auditBasicParams). + SetCommonGraphScanParams(&scangraph.CommonGraphScanParams{ + ScanType: scanservices.Dependency, + IncludeVulnerabilities: true, + MultiScanId: utils.TestScaScanId, + }) + auditParams.SetIsRecursiveScan(true) + + scanResults, err := RunAudit(auditParams) + if testcase.allowPartialResults { + assert.NoError(t, scanResults.ScansErr) + assert.NoError(t, err) + } else { + assert.Error(t, scanResults.ScansErr) + assert.NoError(t, err) + } + }) + } +} + +func TestCreateErrorIfPartialResultsDisabled(t *testing.T) { + testcases := []struct { + name string + allowPartialResults bool + auditParallelRunner bool + }{ + { + name: "Allow partial results - no error expected", + allowPartialResults: true, + auditParallelRunner: true, + }, + { + name: "Partial results disabled with SecurityParallelRunner", + allowPartialResults: false, + auditParallelRunner: true, + }, + { + name: "Partial results disabled without SecurityParallelRunner", + allowPartialResults: false, + auditParallelRunner: false, + }, + } + + for _, testcase := range testcases { + t.Run(testcase.name, func(t *testing.T) { + auditBasicParams := (&utils.AuditBasicParams{}).SetAllowPartialResults(testcase.allowPartialResults) + auditParams := NewAuditParams().SetGraphBasicParams(auditBasicParams) + + var auditParallelRunner *utils.SecurityParallelRunner + if testcase.auditParallelRunner { + auditParallelRunner = utils.CreateSecurityParallelRunner(1) + } + + err := createErrorIfPartialResultsDisabled(auditParams, auditParallelRunner, "", errors.New("error")) + if testcase.allowPartialResults { + assert.NoError(t, err) + } else { + if testcase.auditParallelRunner { + assert.False(t, isErrorsQueueEmpty(auditParallelRunner)) + } else { + assert.Error(t, err) + } + } + }) + } +} + +func isErrorsQueueEmpty(spr *utils.SecurityParallelRunner) bool { + select { + case <-spr.ErrorsQueue: + // Channel is not empty + return false + default: + // Channel is empty + return true + } +} diff --git a/commands/audit/scarunner.go b/commands/audit/scarunner.go index 847b7732..633b9de9 100644 --- a/commands/audit/scarunner.go +++ b/commands/audit/scarunner.go @@ -82,13 +82,15 @@ func buildDepTreeAndRunScaScan(auditParallelRunner *utils.SecurityParallelRunner log.Warn(bdtErr.Error()) continue } - err = errors.Join(err, fmt.Errorf("audit command in '%s' failed:\n%s", scan.Target, bdtErr.Error())) + err = errors.Join(err, createErrorIfPartialResultsDisabled(auditParams, nil, fmt.Sprintf("Dependencies tree construction ha failed for the following target: %s", scan.Target), fmt.Errorf("audit command in '%s' failed:\n%s", scan.Target, bdtErr.Error()))) continue } // Create sca scan task auditParallelRunner.ScaScansWg.Add(1) _, taskErr := auditParallelRunner.Runner.AddTaskWithError(executeScaScanTask(auditParallelRunner, serverDetails, auditParams, scan, treeResult), func(err error) { - auditParallelRunner.AddErrorToChan(fmt.Errorf("audit command in '%s' failed:\n%s", scan.Target, err.Error())) + // If error to be caught, we add it to the auditParallelRunner error queue and continue. The error need not be returned + _ = createErrorIfPartialResultsDisabled(auditParams, auditParallelRunner, fmt.Sprintf("Failed to execute SCA scan for the following target: %s", scan.Target), fmt.Errorf("audit command in '%s' failed:\n%s", scan.Target, err.Error())) + auditParallelRunner.ScaScansWg.Done() }) if taskErr != nil { return fmt.Errorf("failed to create sca scan task for '%s': %s", scan.Target, taskErr.Error()) @@ -147,8 +149,12 @@ func executeScaScanTask(auditParallelRunner *utils.SecurityParallelRunner, serve scan *xrayutils.ScaScanResult, treeResult *DependencyTreeResult) parallel.TaskFunc { return func(threadId int) (err error) { log.Info(clientutils.GetLogMsgPrefix(threadId, false)+"Running SCA scan for", scan.Target, "vulnerable dependencies in", scan.Target, "directory...") + var xrayErr error defer func() { - auditParallelRunner.ScaScansWg.Done() + if xrayErr == nil { + // We Sca waitGroup as done only when we have no errors. If we have errors we mark it done in the error's handler function + auditParallelRunner.ScaScansWg.Done() + } }() // Scan the dependency tree. scanResults, xrayErr := runScaWithTech(scan.Technology, auditParams, serverDetails, *treeResult.FlatTree, treeResult.FullDepTrees) diff --git a/tests/testdata/projects/package-managers/npm/npm-un-installable/package.json b/tests/testdata/projects/package-managers/npm/npm-un-installable/package.json new file mode 100644 index 00000000..7e4cf93e --- /dev/null +++ b/tests/testdata/projects/package-managers/npm/npm-un-installable/package.json @@ -0,0 +1,14 @@ +{ + "name": "jfrog-cli-tests", + "version": "v1.0.0", + "description": "test package", + "main": "index.js", + "scripts": { + "test": "echo \"Error: no test specified\" && exit 1" + }, + "author": "", + "license": "ISC", + "dependencies": { + "non-existing-dependency": "1.0.1" + } +} diff --git a/utils/auditbasicparams.go b/utils/auditbasicparams.go index 0c5660fc..a30887e3 100644 --- a/utils/auditbasicparams.go +++ b/utils/auditbasicparams.go @@ -65,6 +65,7 @@ type AuditBasicParams struct { exclusions []string isRecursiveScan bool skipAutoInstall bool + allowPartialResults bool } func (abp *AuditBasicParams) DirectDependencies() *[]string { @@ -105,6 +106,11 @@ func (abp *AuditBasicParams) SetSkipAutoInstall(skipAutoInstall bool) *AuditBasi return abp } +func (abp *AuditBasicParams) SetAllowPartialResults(allowPartialResults bool) *AuditBasicParams { + abp.allowPartialResults = allowPartialResults + return abp +} + func (abp *AuditBasicParams) UseJas() bool { return abp.useJas } @@ -264,3 +270,7 @@ func (abp *AuditBasicParams) IsRecursiveScan() bool { func (abp *AuditBasicParams) SkipAutoInstall() bool { return abp.skipAutoInstall } + +func (abp *AuditBasicParams) AllowPartialResults() bool { + return abp.allowPartialResults +} diff --git a/utils/results.go b/utils/results.go index e78e2cf7..2233d32e 100644 --- a/utils/results.go +++ b/utils/results.go @@ -31,8 +31,8 @@ func (r *Results) GetScaScansXrayResults() (results []services.ScanResponse) { return } -func (r *Results) GetScaScannedTechnologies() []techutils.Technology { - technologies := datastructures.MakeSet[techutils.Technology]() +func (r *Results) GetScaScannedTechnologies(otherTech ...techutils.Technology) []techutils.Technology { + technologies := datastructures.MakeSetFromElements(otherTech...) for _, scaResult := range r.ScaResults { technologies.Add(scaResult.Technology) }