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.

Sign up for GitHub

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

Apply allow-partial-results to JAS scans #223

Merged
merged 8 commits into from
Nov 6, 2024
Merged

Apply allow-partial-results to JAS scans #223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6ff9826
applied allow-partial-results to JAS and fixed 2 bugs related to erro…
eranturgeman Nov 5, 2024
65bc60f
removed TODO comments on bug fixes
eranturgeman Nov 5, 2024
f5fb950
added new field to JasRunnerParams + fixed an issue (commented) + pas…
eranturgeman Nov 5, 2024
011bf46
removed comment since the issue got already fixed
eranturgeman Nov 5, 2024
ddffb14
added allow partial error handling to AddGeneralError and usage notes…
eranturgeman Nov 5, 2024
32597c7
completed function calls after signature changes
eranturgeman Nov 5, 2024
c5f03d3
added tests
eranturgeman Nov 5, 2024
175c603
fix static analysis error
eranturgeman Nov 5, 2024
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
23 changes: 13 additions & 10 deletions commands/audit/audit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,30 +181,30 @@ func RunAudit(auditParams *AuditParams) (cmdResults *results.SecurityCommandResu
}
jfrogAppsConfig, err := jas.CreateJFrogAppsConfig(cmdResults.GetTargetsPaths())
if err != nil {
return cmdResults.AddGeneralError(fmt.Errorf("failed to create JFrogAppsConfig: %s", err.Error()))
return cmdResults.AddGeneralError(fmt.Errorf("failed to create JFrogAppsConfig: %s", err.Error()), false)
}
// Initialize the parallel runner
auditParallelRunner := utils.CreateSecurityParallelRunner(auditParams.threads)
// Add the JAS scans to the parallel runner
var jasScanner *jas.JasScanner
var generalJasScanErr error
if jasScanner, generalJasScanErr = RunJasScans(auditParallelRunner, auditParams, cmdResults, jfrogAppsConfig); generalJasScanErr != nil {
cmdResults.AddGeneralError(fmt.Errorf("An error has occurred during JAS scan process. JAS scan is skipped for the following directories: %s\n%s", strings.Join(cmdResults.GetTargetsPaths(), ","), generalJasScanErr.Error()))
cmdResults.AddGeneralError(fmt.Errorf("An error has occurred during JAS scan process. JAS scan is skipped for the following directories: %s\n%s", strings.Join(cmdResults.GetTargetsPaths(), ","), generalJasScanErr.Error()), auditParams.AllowPartialResults())
}
if auditParams.Progress() != nil {
auditParams.Progress().SetHeadlineMsg("Scanning for issues")
}
// The sca scan doesn't require the analyzer manager, so it can run separately from the analyzer manager download routine.
if generalScaScanError := buildDepTreeAndRunScaScan(auditParallelRunner, auditParams, cmdResults); generalScaScanError != nil {
cmdResults.AddGeneralError(fmt.Errorf("An error has occurred during SCA scan process. SCA scan is skipped for the following directories: %s\n%s", strings.Join(cmdResults.GetTargetsPaths(), ","), generalScaScanError.Error()))
cmdResults.AddGeneralError(fmt.Errorf("An error has occurred during SCA scan process. SCA scan is skipped for the following directories: %s\n%s", strings.Join(cmdResults.GetTargetsPaths(), ","), generalScaScanError.Error()), auditParams.AllowPartialResults())
}
go func() {
auditParallelRunner.ScaScansWg.Wait()
auditParallelRunner.JasWg.Wait()
// Wait for all jas scanners to complete before cleaning up scanners temp dir
auditParallelRunner.JasScannersWg.Wait()
if jasScanner != nil && jasScanner.ScannerDirCleanupFunc != nil {
cmdResults.AddGeneralError(jasScanner.ScannerDirCleanupFunc())
cmdResults.AddGeneralError(jasScanner.ScannerDirCleanupFunc(), false)
}
auditParallelRunner.Runner.Done()
}()
Expand Down Expand Up @@ -242,7 +242,7 @@ func RunJasScans(auditParallelRunner *utils.SecurityParallelRunner, auditParams
}
auditParallelRunner.JasWg.Add(1)
if _, jasErr := auditParallelRunner.Runner.AddTaskWithError(createJasScansTasks(auditParallelRunner, scanResults, serverDetails, auditParams, jasScanner, jfrogAppsConfig), func(taskErr error) {
generalError = errors.Join(generalError, fmt.Errorf("failed while adding JAS scan tasks: %s", taskErr.Error()))
scanResults.AddGeneralError(fmt.Errorf("failed while adding JAS scan tasks: %s", taskErr.Error()), auditParams.AllowPartialResults())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is a bug fix.
We missed capturing errors coming from the threads and tried to propagate them up the stack, where they "got lost" in the process or got override. This fix ensure we add the to the GeneralError so it will be examined later on in the process

}); jasErr != nil {
generalError = fmt.Errorf("failed to create JAS task: %s", jasErr.Error())
}
Expand Down Expand Up @@ -281,9 +281,12 @@ func createJasScansTasks(auditParallelRunner *utils.SecurityParallelRunner, scan
SignedDescriptions: auditParams.OutputFormat() == format.Sarif,
ScanResults: targetResult,
TargetOutputDir: auditParams.scanResultsOutputDir,
AllowPartialResults: auditParams.AllowPartialResults(),
}
if generalError := runner.AddJasScannersTasks(params); generalError != nil {
if generalError = runner.AddJasScannersTasks(params); generalError != nil {
_ = targetResult.AddTargetError(fmt.Errorf("%s failed to add JAS scan tasks: %s", logPrefix, generalError.Error()), auditParams.AllowPartialResults())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bug fix.
After adding the error to TargetError (or logging it if partial results are allowed) we left generalError as it was and it propagated up the stack and caught again later in the process. this caused the error to be logged twice.
After handling it, due to the new way we handle errors, there is no need to propagate the error by returning it after we handle it once

// We assign nil to 'generalError' after handling it to prevent it to propagate further, so it will not be captured twice - once here, and once in the error handling function of createJasScansTasks
generalError = nil
}
}
return
Expand All @@ -295,20 +298,20 @@ func initAuditCmdResults(params *AuditParams) (cmdResults *results.SecurityComma
// Initialize general information
serverDetails, err := params.ServerDetails()
if err != nil {
return cmdResults.AddGeneralError(err)
return cmdResults.AddGeneralError(err, false)
}
var xrayManager *xray.XrayServicesManager
if xrayManager, params.xrayVersion, err = xrayutils.CreateXrayServiceManagerAndGetVersion(serverDetails); err != nil {
return cmdResults.AddGeneralError(err)
return cmdResults.AddGeneralError(err, false)
} else {
cmdResults.SetXrayVersion(params.xrayVersion)
}
if err = clientutils.ValidateMinimumVersion(clientutils.Xray, params.xrayVersion, scangraph.GraphScanMinXrayVersion); err != nil {
return cmdResults.AddGeneralError(err)
return cmdResults.AddGeneralError(err, false)
}
entitledForJas, err := isEntitledForJas(xrayManager, params)
if err != nil {
return cmdResults.AddGeneralError(err)
return cmdResults.AddGeneralError(err, false)
} else {
cmdResults.SetEntitledForJas(entitledForJas)
}
Expand Down
80 changes: 64 additions & 16 deletions commands/audit/audit_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@ package audit

import (
"fmt"
commonCommands "github.com/jfrog/jfrog-cli-core/v2/common/commands"
"github.com/jfrog/jfrog-cli-core/v2/utils/coreutils"
configTests "github.com/jfrog/jfrog-cli-security/tests"
securityTestUtils "github.com/jfrog/jfrog-cli-security/tests/utils"
clientTests "github.com/jfrog/jfrog-client-go/utils/tests"
"net/http"
"path/filepath"
"sort"
Expand Down Expand Up @@ -364,6 +369,7 @@ func TestAuditWithPartialResults(t *testing.T) {
name string
allowPartialResults bool
useJas bool
pipRequirementsFile string
testDirPath string
}{
{
Expand All @@ -378,6 +384,13 @@ func TestAuditWithPartialResults(t *testing.T) {
useJas: false,
testDirPath: filepath.Join("..", "..", "tests", "testdata", "projects", "package-managers", "npm", "npm-project"),
},
{
name: "Failure in JAS scans",
allowPartialResults: false,
useJas: true,
pipRequirementsFile: "requirements.txt",
testDirPath: filepath.Join("..", "..", "tests", "testdata", "projects", "jas", "jas", "npm-project"),
},
{
name: "Skip failure in SCA during dependency tree construction",
allowPartialResults: true,
Expand All @@ -390,35 +403,70 @@ func TestAuditWithPartialResults(t *testing.T) {
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
{
name: "Skip failure in JAS scans",
allowPartialResults: true,
useJas: true,
pipRequirementsFile: "requirements.txt",
testDirPath: filepath.Join("..", "..", "tests", "testdata", "projects", "jas", "jas", "npm-project"),
},
}

serverMock, serverDetails := validations.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) {
serverMock, serverDetails := validations.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"}`, utils.EntitlementsMinVersion)))
if !assert.NoError(t, err) {
return
}
}
// All endpoints required to test failures in SCA scan
if !testcase.useJas {
if strings.Contains(r.RequestURI, "/xray/api/v1/scan/graph") && r.Method == http.MethodPost {
// We set SCA scan graph API to fail
w.WriteHeader(http.StatusBadRequest)
}
}

// All endpoints required to test failures in JAS
if testcase.useJas {
if strings.Contains(r.RequestURI, "/xsc-gen-exe-analyzer-manager-local/v1") {
w.WriteHeader(http.StatusBadRequest)
}
if strings.Contains(r.RequestURI, "api/v1/entitlements/feature/contextual_analysis") && r.Method == http.MethodGet {
_, err := w.Write([]byte(`{"entitled":true,"feature_id":"contextual_analysis"}`))
if !assert.NoError(t, err) {
return
}
}
}

})
defer serverMock.Close()

tempDirPath, createTempDirCallback := coreTests.CreateTempDirWithCallbackAndAssert(t)
defer createTempDirCallback()

if testcase.useJas {
// In order to simulate failure in Jas process we fail the AM download by using the mock server for the download and failing the endpoint call there
clientTests.SetEnvAndAssert(t, coreutils.HomeDir, filepath.Join(tempDirPath, configTests.Out, "jfroghome"))
err := commonCommands.NewConfigCommand(commonCommands.AddOrEdit, "testServer").SetDetails(serverDetails).SetInteractive(false).SetEncPassword(false).Run()
assert.NoError(t, err)
defer securityTestUtils.CleanTestsHomeEnv()

callbackEnv := clientTests.SetEnvWithCallbackAndAssert(t, coreutils.ReleasesRemoteEnv, "testServer/testRemoteRepo")
defer callbackEnv()
}

assert.NoError(t, biutils.CopyDir(testcase.testDirPath, tempDirPath, false, nil))

auditBasicParams := (&utils.AuditBasicParams{}).
SetServerDetails(serverDetails).
SetOutputFormat(format.Table).
SetUseJas(testcase.useJas).
SetAllowPartialResults(testcase.allowPartialResults)
SetAllowPartialResults(testcase.allowPartialResults).
SetPipRequirementsFile(testcase.pipRequirementsFile)

auditParams := NewAuditParams().
SetWorkingDirs([]string{tempDirPath}).
Expand Down
20 changes: 10 additions & 10 deletions commands/scan/scan.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ func (scanCmd *ScanCommand) RunAndRecordResults(cmdType utils.CommandType, recor
}

if err = recordResFunc(cmdResults); err != nil {
cmdResults.AddGeneralError(fmt.Errorf("failed to record results: %s", err.Error()))
cmdResults.AddGeneralError(fmt.Errorf("failed to record results: %s", err.Error()), false)
}
if err = cmdResults.GetErrors(); err != nil {
return
Expand Down Expand Up @@ -283,7 +283,7 @@ func (scanCmd *ScanCommand) RunScan(cmdType utils.CommandType) (cmdResults *resu
}
// Initialize the Xray Indexer
if indexerPath, indexerTempDir, cleanUp, err := initIndexer(xrayManager, cmdResults.XrayVersion); err != nil {
return cmdResults.AddGeneralError(err)
return cmdResults.AddGeneralError(err, false)
} else {
scanCmd.indexerPath = indexerPath
scanCmd.indexerTempDir = indexerTempDir
Expand All @@ -295,7 +295,7 @@ func (scanCmd *ScanCommand) RunScan(cmdType utils.CommandType) (cmdResults *resu
}
// Wait for the Download of the AnalyzerManager to complete.
if err := errGroup.Wait(); err != nil {
cmdResults.AddGeneralError(errors.New("failed while trying to get Analyzer Manager: " + err.Error()))
cmdResults.AddGeneralError(errors.New("failed while trying to get Analyzer Manager: "+err.Error()), false)
}
fileProducerConsumer := parallel.NewRunner(threads, 20000, false)
indexedFileProducerConsumer := parallel.NewRunner(threads, 20000, false)
Expand All @@ -313,22 +313,22 @@ func initScanCmdResults(cmdType utils.CommandType, serverDetails *config.ServerD
cmdResults = results.NewCommandResults(cmdType)
xrayManager, xrayVersion, err := xray.CreateXrayServiceManagerAndGetVersion(serverDetails)
if err != nil {
return xrayManager, cmdResults.AddGeneralError(err)
return xrayManager, cmdResults.AddGeneralError(err, false)
} else {
cmdResults.SetXrayVersion(xrayVersion)
}
// Validate Xray minimum version for graph scan command
if err := clientutils.ValidateMinimumVersion(clientutils.Xray, xrayVersion, scangraph.GraphScanMinXrayVersion); err != nil {
return xrayManager, cmdResults.AddGeneralError(err)
if err = clientutils.ValidateMinimumVersion(clientutils.Xray, xrayVersion, scangraph.GraphScanMinXrayVersion); err != nil {
return xrayManager, cmdResults.AddGeneralError(err, false)
}
if bypassArchiveLimits {
// Validate Xray minimum version for BypassArchiveLimits flag for indexer
if err := clientutils.ValidateMinimumVersion(clientutils.Xray, xrayVersion, BypassArchiveLimitsMinXrayVersion); err != nil {
return xrayManager, cmdResults.AddGeneralError(err)
if err = clientutils.ValidateMinimumVersion(clientutils.Xray, xrayVersion, BypassArchiveLimitsMinXrayVersion); err != nil {
return xrayManager, cmdResults.AddGeneralError(err, false)
}
}
if entitledForJas, err := isEntitledForJas(xrayManager, xrayVersion, useJas); err != nil {
return xrayManager, cmdResults.AddGeneralError(err)
return xrayManager, cmdResults.AddGeneralError(err, false)
} else {
cmdResults.SetEntitledForJas(entitledForJas)
if entitledForJas {
Expand Down Expand Up @@ -386,7 +386,7 @@ func (scanCmd *ScanCommand) prepareScanTasks(fileProducer, indexedFileProducer p
taskHandler := getAddTaskToProducerFunc(fileProducer, artifactHandlerFunc)
if generalError := collectFilesForIndexing(specFiles[i], taskHandler); generalError != nil {
log.Error(generalError)
cmdResults.AddGeneralError(generalError)
cmdResults.AddGeneralError(generalError, false)
}
}
}()
Expand Down
15 changes: 8 additions & 7 deletions jas/runner/jasrunner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,9 @@ type JasRunnerParams struct {
ServerDetails *config.ServerDetails
Scanner *jas.JasScanner

Module jfrogappsconfig.Module
ConfigProfile *services.ConfigProfile
Module jfrogappsconfig.Module
ConfigProfile *services.ConfigProfile
AllowPartialResults bool

ScansToPreform []utils.SubScanType

Expand Down Expand Up @@ -101,7 +102,7 @@ func addJasScanTaskForModuleIfNeeded(params JasRunnerParams, subScan utils.SubSc
return
}
if enabled {
generalError = addModuleJasScanTask(jasType, params.Runner, task, params.ScanResults)
generalError = addModuleJasScanTask(jasType, params.Runner, task, params.ScanResults, params.AllowPartialResults)
} else {
log.Debug(fmt.Sprintf("Skipping %s scan as requested by '%s' config profile...", jasType, params.ConfigProfile.ProfileName))
}
Expand All @@ -111,15 +112,15 @@ func addJasScanTaskForModuleIfNeeded(params JasRunnerParams, subScan utils.SubSc
log.Debug(fmt.Sprintf("Skipping %s scan as requested by local module config...", subScan))
return
}
return addModuleJasScanTask(jasType, params.Runner, task, params.ScanResults)
return addModuleJasScanTask(jasType, params.Runner, task, params.ScanResults, params.AllowPartialResults)
}

func addModuleJasScanTask(scanType jasutils.JasScanType, securityParallelRunner *utils.SecurityParallelRunner, task parallel.TaskFunc, scanResults *results.TargetResults) (generalError error) {
func addModuleJasScanTask(scanType jasutils.JasScanType, securityParallelRunner *utils.SecurityParallelRunner, task parallel.TaskFunc, scanResults *results.TargetResults, allowSkippingErrors bool) (generalError error) {
securityParallelRunner.JasScannersWg.Add(1)
if _, addTaskErr := securityParallelRunner.Runner.AddTaskWithError(task, func(err error) {
_ = scanResults.AddTargetError(fmt.Errorf("failed to run %s scan: %s", scanType, err.Error()), false)
_ = scanResults.AddTargetError(fmt.Errorf("failed to run %s scan: %s", scanType, err.Error()), allowSkippingErrors)
}); addTaskErr != nil {
generalError = fmt.Errorf("failed to create %s scan task: %s", scanType, addTaskErr.Error())
generalError = scanResults.AddTargetError(fmt.Errorf("error occurred while adding '%s' scan to parallel runner: %s", scanType, generalError.Error()), allowSkippingErrors)
eranturgeman marked this conversation as resolved.
Show resolved Hide resolved
}
return
}
Expand Down
13 changes: 9 additions & 4 deletions utils/results/results.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,13 @@ func (r *SecurityCommandResults) SetMultiScanId(multiScanId string) *SecurityCom
}

// --- Aggregated results for all targets ---

func (r *SecurityCommandResults) AddGeneralError(err error) *SecurityCommandResults {
// Adds a general error to the command results in different phases of its execution.
// Notice that in some usages we pass constant 'false' to the 'allowSkippingError' parameter in some places, where we wish to force propagation of the error when it occurs.
func (r *SecurityCommandResults) AddGeneralError(err error, allowSkippingError bool) *SecurityCommandResults {
if allowSkippingError && err != nil {
log.Warn(fmt.Sprintf("Partial results are allowed, the error is skipped: %s", err.Error()))
return r
}
r.GeneralError = errors.Join(r.GeneralError, err)
return r
}
Expand Down Expand Up @@ -292,8 +297,8 @@ func (sr *TargetResults) HasFindings() bool {
return false
}

func (sr *TargetResults) AddTargetError(err error, allowPartialResults bool) error {
if allowPartialResults {
func (sr *TargetResults) AddTargetError(err error, allowSkippingError bool) error {
if allowSkippingError && err != nil {
log.Warn(fmt.Sprintf("Partial results are allowed, the error is skipped in target '%s': %s", sr.String(), err.Error()))
return nil
}
Expand Down
Loading