From fac5e614292786d4eb899e3673221894218eaa8a Mon Sep 17 00:00:00 2001 From: Eran Turgeman Date: Sun, 6 Oct 2024 17:26:26 +0300 Subject: [PATCH 01/10] added partial results error handling after dep tree construction + after SCA scan execution --- commands/audit/scarunner.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/commands/audit/scarunner.go b/commands/audit/scarunner.go index 38054d23..90a6adfe 100644 --- a/commands/audit/scarunner.go +++ b/commands/audit/scarunner.go @@ -76,13 +76,13 @@ func buildDepTreeAndRunScaScan(auditParallelRunner *utils.SecurityParallelRunner // Get the dependency tree for the technology in the working directory. treeResult, bdtErr := buildDependencyTree(scan, auditParams) if bdtErr != nil { - 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())) + _ = 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())) }) if taskErr != nil { return fmt.Errorf("failed to create sca scan task for '%s': %s", scan.Target, taskErr.Error()) From 4ca3cd469eb4b2c76f8ed64e81aa28ae1d6d86b0 Mon Sep 17 00:00:00 2001 From: Eran Turgeman Date: Sun, 6 Oct 2024 17:28:53 +0300 Subject: [PATCH 02/10] added a fix to ScaScansWg bug where we closed the channel before inserting the error to it in case we have an error --- commands/audit/scarunner.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/commands/audit/scarunner.go b/commands/audit/scarunner.go index 90a6adfe..baa441a5 100644 --- a/commands/audit/scarunner.go +++ b/commands/audit/scarunner.go @@ -83,6 +83,7 @@ func buildDepTreeAndRunScaScan(auditParallelRunner *utils.SecurityParallelRunner auditParallelRunner.ScaScansWg.Add(1) _, taskErr := auditParallelRunner.Runner.AddTaskWithError(executeScaScanTask(auditParallelRunner, serverDetails, auditParams, scan, treeResult), func(err error) { _ = 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()) @@ -141,8 +142,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) From 9ba39bc45d9282a5a530203c53cbcb1b4e19a103 Mon Sep 17 00:00:00 2001 From: Eran Turgeman Date: Sun, 6 Oct 2024 17:33:01 +0300 Subject: [PATCH 03/10] added allowPartialResults field to audit params + setter and getter --- utils/auditbasicparams.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/utils/auditbasicparams.go b/utils/auditbasicparams.go index df3a23fd..d884ed43 100644 --- a/utils/auditbasicparams.go +++ b/utils/auditbasicparams.go @@ -63,6 +63,7 @@ type AuditBasicParams struct { dependenciesForApplicabilityScan []string exclusions []string isRecursiveScan bool + allowPartialResults bool } func (abp *AuditBasicParams) DirectDependencies() *[]string { @@ -98,6 +99,11 @@ func (abp *AuditBasicParams) SetUseJas(useJas bool) *AuditBasicParams { return abp } +func (abp *AuditBasicParams) SetAllowPartialResults(allowPartialResults bool) *AuditBasicParams { + abp.allowPartialResults = allowPartialResults + return abp +} + func (abp *AuditBasicParams) UseJas() bool { return abp.useJas } @@ -253,3 +259,7 @@ func (abp *AuditBasicParams) SetIsRecursiveScan(isRecursiveScan bool) *AuditBasi func (abp *AuditBasicParams) IsRecursiveScan() bool { return abp.isRecursiveScan } + +func (abp *AuditBasicParams) AllowPartialResults() bool { + return abp.allowPartialResults +} From b29cc47b5acac8ed32411f5c0a399af3fcaa17b6 Mon Sep 17 00:00:00 2001 From: Eran Turgeman Date: Sun, 6 Oct 2024 17:52:38 +0300 Subject: [PATCH 04/10] added partial results error handling after buildDepTreeAndRunScaScan + the handler function --- commands/audit/audit.go | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/commands/audit/audit.go b/commands/audit/audit.go index 85ec44f2..79c72337 100644 --- a/commands/audit/audit.go +++ b/commands/audit/audit.go @@ -202,7 +202,7 @@ 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) + _ = 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 +277,26 @@ func downloadAnalyzerManagerAndRunScanners(auditParallelRunner *utils.SecurityPa } return } + +// This function checks if partial results is allowed. If so we log the error and continue. +// If partial results is not allowed we add the error to the SecurityParallelRunner error's channel if one is provided, or simply return the error if not. +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 +} From e799602da2c52c5e162275d7ed7c42912c3d5fab Mon Sep 17 00:00:00 2001 From: Eran Turgeman Date: Sun, 6 Oct 2024 17:53:16 +0300 Subject: [PATCH 05/10] added tests for allow partial results for SCA scan ONLY + for createErrorIfPartialResultsDisabled --- commands/audit/audit_test.go | 144 +++++++++++++++++++++++++++++++++++ 1 file changed, 144 insertions(+) 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 + } +} From fe5c5c00b2530f58e9e5ea65060035e774938415 Mon Sep 17 00:00:00 2001 From: Eran Turgeman Date: Mon, 7 Oct 2024 09:34:19 +0300 Subject: [PATCH 06/10] Added a new test project --- .../npm/npm-un-installable/package.json | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 tests/testdata/projects/package-managers/npm/npm-un-installable/package.json 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" + } +} From 472d89537fb10d4edd95d5716fcbe49332e65ee7 Mon Sep 17 00:00:00 2001 From: Eran Turgeman Date: Tue, 8 Oct 2024 12:10:46 +0300 Subject: [PATCH 07/10] fixed a bug related to scanned tech collection in a multi-project (multi working dirs) repo --- utils/results.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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) } From a2eccc5a968d2a8cf6e6101eccd7c3dc5e2a239f Mon Sep 17 00:00:00 2001 From: Eran Turgeman Date: Tue, 8 Oct 2024 16:24:53 +0300 Subject: [PATCH 08/10] added --allow-partial-results flag to enable feature usage from CLI --- cli/docs/flags.go | 8 +++++--- cli/scancommands.go | 3 ++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/cli/docs/flags.go b/cli/docs/flags.go index bf9643e7..b4101177 100644 --- a/cli/docs/flags.go +++ b/cli/docs/flags.go @@ -118,6 +118,7 @@ const ( RequirementsFile = "requirements-file" WorkingDirs = "working-dirs" OutputDir = "output-dir" + AllowPartialResults = "allow-partial-results" // Unique curation flags CurationOutput = "curation-format" @@ -154,7 +155,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, + Sca, Iac, Sast, Secrets, WithoutCA, ScanVuln, SecretValidation, OutputDir, AllowPartialResults, }, CurationAudit: { CurationOutput, WorkingDirs, Threads, RequirementsFile, @@ -229,8 +230,9 @@ 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()), + 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()), + AllowPartialResults: components.NewBoolFlag(AllowPartialResults, "Set to true to allow partial results and continuance of the scan in case of certain errors."), 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 fcd620e3..2255775d 100644 --- a/cli/scancommands.go +++ b/cli/scancommands.go @@ -477,7 +477,8 @@ func CreateAuditCmd(c *components.Context) (*audit.AuditCommand, error) { SetMinSeverityFilter(minSeverity). SetFixableOnly(c.GetBoolFlagValue(flags.FixableOnly)). SetThirdPartyApplicabilityScan(c.GetBoolFlagValue(flags.ThirdPartyContextualAnalysis)). - SetScansResultsOutputDir(scansOutputDir) + SetScansResultsOutputDir(scansOutputDir). + SetAllowPartialResults(c.GetBoolFlagValue(flags.AllowPartialResults)) if c.GetStringFlagValue(flags.Watches) != "" { auditCmd.SetWatches(splitByCommaAndTrim(c.GetStringFlagValue(flags.Watches))) From caff52f28ff78c69eb70ae7f6024736ccaaa44a8 Mon Sep 17 00:00:00 2001 From: Eran Turgeman Date: Thu, 10 Oct 2024 16:31:52 +0300 Subject: [PATCH 09/10] added comments and docs --- commands/audit/audit.go | 6 ++++-- commands/audit/scarunner.go | 1 + 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/commands/audit/audit.go b/commands/audit/audit.go index 79c72337..13b3d2af 100644 --- a/commands/audit/audit.go +++ b/commands/audit/audit.go @@ -202,6 +202,7 @@ 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 { + // 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() { @@ -278,8 +279,9 @@ func downloadAnalyzerManagerAndRunScanners(auditParallelRunner *utils.SecurityPa return } -// This function checks if partial results is allowed. If so we log the error and continue. -// If partial results is not allowed we add the error to the SecurityParallelRunner error's channel if one is provided, or simply return the error if not. +// 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 diff --git a/commands/audit/scarunner.go b/commands/audit/scarunner.go index baa441a5..93ab5795 100644 --- a/commands/audit/scarunner.go +++ b/commands/audit/scarunner.go @@ -82,6 +82,7 @@ func buildDepTreeAndRunScaScan(auditParallelRunner *utils.SecurityParallelRunner // Create sca scan task auditParallelRunner.ScaScansWg.Add(1) _, taskErr := auditParallelRunner.Runner.AddTaskWithError(executeScaScanTask(auditParallelRunner, serverDetails, auditParams, scan, treeResult), func(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() }) From 1fa3ddd0d0f350509928fc00a3dacf69babfc3c0 Mon Sep 17 00:00:00 2001 From: Eran Turgeman Date: Thu, 10 Oct 2024 17:46:27 +0300 Subject: [PATCH 10/10] marked new flag as hidden --- cli/docs/flags.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cli/docs/flags.go b/cli/docs/flags.go index 6ec45bbc..1f2c0ce4 100644 --- a/cli/docs/flags.go +++ b/cli/docs/flags.go @@ -231,10 +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()), - AllowPartialResults: components.NewBoolFlag(AllowPartialResults, "Set to true to allow partial results and continuance of the scan in case of certain errors."), + 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.",