Skip to content

Commit

Permalink
Filter out non-applicable violations if requested by policy (#275)
Browse files Browse the repository at this point in the history
  • Loading branch information
eranturgeman authored Jan 6, 2025
1 parent 333ac33 commit c1d19ba
Show file tree
Hide file tree
Showing 11 changed files with 204 additions and 22 deletions.
11 changes: 4 additions & 7 deletions audit_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -808,7 +808,7 @@ type auditCommandTestParams struct {
// --project flag value if provided.
ProjectKey string
// --fail flag value if provided, must be provided with 'createWatchesFuncs' to create watches for the test
FailOnFailedBuildFlag bool
DisableFailOnFailedBuildFlag bool
// -- vuln flag 'True' value must be provided with 'createWatchesFuncs' to create watches for the test
WithVuln bool
// --licenses flag value if provided
Expand Down Expand Up @@ -836,12 +836,9 @@ func testAuditCommand(t *testing.T, testCli *coreTests.JfrogCli, params auditCom
if len(params.Watches) > 0 {
args = append(args, "--watches="+strings.Join(params.Watches, ","))
}
if params.FailOnFailedBuildFlag {
if len(params.Watches) == 0 {
// Verify params consistency no fail flag
assert.False(t, params.FailOnFailedBuildFlag, "Fail flag provided without watches")
}
args = append(args, "--fail")
// Default value for --fail flag is 'true'. Unless we directly pass DisableFailOnFailedBuildFlag=true, the flow will fail when security issues are found
if params.DisableFailOnFailedBuildFlag {
args = append(args, "--fail=false")
}
if params.WithVuln {
args = append(args, "--vuln")
Expand Down
2 changes: 1 addition & 1 deletion commands/audit/scarunner.go
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ func runScaWithTech(tech techutils.Technology, params *AuditParams, serverDetail
if err != nil {
return
}
log.Debug(fmt.Sprintf("Finished '%s' dependency tree scan. %s", tech.ToFormal(), utils.GetScanFindingsLog(utils.ScaScan, len(techResults[0].Vulnerabilities), len(techResults[0].Violations), -1)))
log.Info(fmt.Sprintf("Finished '%s' dependency tree scan. %s", tech.ToFormal(), utils.GetScanFindingsLog(utils.ScaScan, len(techResults[0].Vulnerabilities), len(techResults[0].Violations), -1)))
techResults = sca.BuildImpactPathsForScanResponse(techResults, fullDependencyTrees)
return
}
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ require (
gopkg.in/warnings.v0 v0.1.2 // indirect
)

replace github.com/jfrog/jfrog-client-go => github.com/jfrog/jfrog-client-go v1.28.1-0.20241230154616-e342ed5065f1
replace github.com/jfrog/jfrog-client-go => github.com/jfrog/jfrog-client-go v1.28.1-0.20250106143359-de902d8b8495

replace github.com/jfrog/jfrog-cli-core/v2 => github.com/jfrog/jfrog-cli-core/v2 v2.31.1-0.20250101110857-b26e9a6644c6

Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,8 @@ github.com/jfrog/jfrog-apps-config v1.0.1 h1:mtv6k7g8A8BVhlHGlSveapqf4mJfonwvXYL
github.com/jfrog/jfrog-apps-config v1.0.1/go.mod h1:8AIIr1oY9JuH5dylz2S6f8Ym2MaadPLR6noCBO4C22w=
github.com/jfrog/jfrog-cli-core/v2 v2.31.1-0.20250101110857-b26e9a6644c6 h1:/i1sIQS0q0gRN531ChVToQWcjaVZOKZ4KuGk7j7vDTc=
github.com/jfrog/jfrog-cli-core/v2 v2.31.1-0.20250101110857-b26e9a6644c6/go.mod h1:LfKvCRXbvwgE0V6aX3/GabkzCedghXq0Y6lmsEuxr44=
github.com/jfrog/jfrog-client-go v1.28.1-0.20241230154616-e342ed5065f1 h1:JQvbTSPDkPNpts1NLHGTKvtG4cMFY1ptBHTNMYFyMhs=
github.com/jfrog/jfrog-client-go v1.28.1-0.20241230154616-e342ed5065f1/go.mod h1:2ySOMva54L3EYYIlCBYBTcTgqfrrQ19gtpA/MWfA/ec=
github.com/jfrog/jfrog-client-go v1.28.1-0.20250106143359-de902d8b8495 h1:cPugIRHCJxE+QWW9TlvOlTWPcVI1wTRgAujQZk4I4VI=
github.com/jfrog/jfrog-client-go v1.28.1-0.20250106143359-de902d8b8495/go.mod h1:2ySOMva54L3EYYIlCBYBTcTgqfrrQ19gtpA/MWfA/ec=
github.com/k0kubun/colorstring v0.0.0-20150214042306-9440f1994b88/go.mod h1:3w7q1U84EfirKl04SVQ/s7nPm1ZPhiXd34z40TNz36k=
github.com/k0kubun/pp v3.0.1+incompatible/go.mod h1:GWse8YhT0p8pT4ir3ZgBbfZild3tgzSScAn6HmfYukg=
github.com/kevinburke/ssh_config v1.2.0 h1:x584FjTGwHzMwvHx18PXxbBVzfnxogHaAReU4gf13a4=
Expand Down
2 changes: 1 addition & 1 deletion jas/runner/jasrunner.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ func runContextualScan(securityParallelRunner *utils.SecurityParallelRunner, sca
securityParallelRunner.ResultsMu.Lock()
defer securityParallelRunner.ResultsMu.Unlock()
// We first add the scan results and only then check for errors, so we can store the exit code in order to report it in the end
scanResults.JasResults.NewApplicabilityScanResults(jas.GetAnalyzerManagerExitCode(err), caScanResults...)
scanResults.JasResults.AddApplicabilityScanResults(jas.GetAnalyzerManagerExitCode(err), caScanResults...)
if err = jas.ParseAnalyzerManagerError(jasutils.Applicability, err); err != nil {
return fmt.Errorf("%s%s", clientutils.GetLogMsgPrefix(threadId, false), err.Error())
}
Expand Down
6 changes: 3 additions & 3 deletions tests/utils/test_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -297,11 +297,11 @@ func CreateSecurityPolicy(t *testing.T, policyName string, rules ...xrayUtils.Po
}
}

func CreateTestSecurityPolicy(t *testing.T, policyName string, severity xrayUtils.Severity, failBuild bool) (string, func()) {
func CreateTestSecurityPolicy(t *testing.T, policyName string, severity xrayUtils.Severity, failBuild bool, skipNotApplicable bool) (string, func()) {
return CreateSecurityPolicy(t, policyName,
xrayUtils.PolicyRule{
Name: "sca_rule",
Criteria: *xrayUtils.CreateSeverityPolicyCriteria(severity),
Criteria: *xrayUtils.CreateSeverityPolicyCriteria(severity, skipNotApplicable),
Actions: getBuildFailAction(failBuild),
Priority: 1,
},
Expand Down Expand Up @@ -382,7 +382,7 @@ func CreateTestPolicyAndWatch(t *testing.T, policyName, watchName string, severi
Type: xrayUtils.Security,
Rules: []xrayUtils.PolicyRule{{
Name: "sec_rule",
Criteria: *xrayUtils.CreateSeverityPolicyCriteria(severity),
Criteria: *xrayUtils.CreateSeverityPolicyCriteria(severity, false),
Priority: 1,
Actions: &xrayUtils.PolicyAction{
FailBuild: clientUtils.Pointer(true),
Expand Down
27 changes: 26 additions & 1 deletion utils/results/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ func ApplyHandlerToScaVulnerabilities(target ScanTarget, vulnerabilities []servi
return nil
}

// ApplyHandlerToScaViolations allows to iterate over the provided SCA violations and call the provided handler for each impacted component/package with a violation to process it.
// Allows to iterate over the provided SCA violations and call the provided handler for each impacted component/package with a violation to process it.
func ApplyHandlerToScaViolations(target ScanTarget, violations []services.Violation, entitledForJas bool, applicabilityRuns []*sarif.Run, securityHandler ParseScaViolationFunc, licenseHandler ParseScaViolationFunc, operationalRiskHandler ParseScaViolationFunc) (watches []string, failBuild bool, err error) {
if securityHandler == nil && licenseHandler == nil && operationalRiskHandler == nil {
return
Expand Down Expand Up @@ -145,6 +145,13 @@ func ApplyHandlerToScaViolations(target ScanTarget, violations []services.Violat
// No handler was provided for security violations
continue
}

var skipNotApplicable bool
if skipNotApplicable, err = shouldSkipNotApplicable(violation, applicabilityStatus); skipNotApplicable {
log.Debug("A non-applicable violation was found and will be removed from final results as requested by its policies")
continue
}

for compIndex := 0; compIndex < len(impactedPackagesNames); compIndex++ {
if e := securityHandler(
violation, cves, applicabilityStatus, severity,
Expand Down Expand Up @@ -655,3 +662,21 @@ func GetIssueTechnology(responseTechnology string, targetTech techutils.Technolo
// if no technology is provided, use the target technology
return targetTech
}

// Checks if the violation's applicability status is NotApplicable and if all of its policies states that non-applicable CVEs should be skipped
func shouldSkipNotApplicable(violation services.Violation, applicabilityStatus jasutils.ApplicabilityStatus) (bool, error) {
if applicabilityStatus != jasutils.NotApplicable {
return false, nil
}

if len(violation.Policies) == 0 {
return false, errors.New("A violation with no policies was provided")
}

for _, policy := range violation.Policies {
if !policy.SkipNotApplicable {
return false, nil
}
}
return true, nil
}
106 changes: 106 additions & 0 deletions utils/results/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -701,3 +701,109 @@ func TestGetFinalApplicabilityStatus(t *testing.T) {
})
}
}

func TestShouldSkipNotApplicable(t *testing.T) {
testCases := []struct {
name string
violation services.Violation
applicabilityStatus jasutils.ApplicabilityStatus
shouldSkip bool
errorExpected bool
}{
{
name: "Applicable CVE - should NOT skip",
violation: services.Violation{},
applicabilityStatus: jasutils.Applicable,
shouldSkip: false,
errorExpected: false,
},
{
name: "Undetermined CVE - should NOT skip",
violation: services.Violation{},
applicabilityStatus: jasutils.ApplicabilityUndetermined,
shouldSkip: false,
errorExpected: false,
},
{
name: "Not covered CVE - should NOT skip",
violation: services.Violation{},
applicabilityStatus: jasutils.NotCovered,
shouldSkip: false,
errorExpected: false,
},
{
name: "Missing Context CVE - should NOT skip",
violation: services.Violation{},
applicabilityStatus: jasutils.MissingContext,
shouldSkip: false,
errorExpected: false,
},
{
name: "Not scanned CVE - should NOT skip",
violation: services.Violation{},
applicabilityStatus: jasutils.NotScanned,
shouldSkip: false,
errorExpected: false,
},
{
name: "Non applicable CVE with skip-non-applicable in ALL policies - SHOULD skip",
violation: services.Violation{
Policies: []services.Policy{
{
Policy: "policy-1",
SkipNotApplicable: true,
},
{
Policy: "policy-2",
SkipNotApplicable: true,
},
},
},
applicabilityStatus: jasutils.NotApplicable,
shouldSkip: true,
errorExpected: false,
},
{
name: "Non applicable CVE with skip-non-applicable in SOME policies - should NOT skip",
violation: services.Violation{
Policies: []services.Policy{
{
Policy: "policy-1",
SkipNotApplicable: true,
},
{
Policy: "policy-2",
SkipNotApplicable: false,
},
},
},
applicabilityStatus: jasutils.NotApplicable,
shouldSkip: false,
errorExpected: false,
},
{
name: "Violation without policy - error expected",
violation: services.Violation{},
applicabilityStatus: jasutils.NotApplicable,
shouldSkip: false,
errorExpected: true,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
shouldSkip, err := shouldSkipNotApplicable(tc.violation, tc.applicabilityStatus)
if tc.errorExpected {
assert.Error(t, err)
} else {
assert.NoError(t, err)
}

if tc.shouldSkip {
assert.True(t, shouldSkip)
} else {
assert.False(t, shouldSkip)
}
})
}
}
4 changes: 2 additions & 2 deletions utils/results/conversion/convertor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ func getAuditTestResults(unique bool) (*results.SecurityCommandResults, validati
ScannedStatus: "completed",
})
// Contextual analysis scan results
npmTargetResults.JasResults.NewApplicabilityScanResults(0,
npmTargetResults.JasResults.AddApplicabilityScanResults(0,
&sarif.Run{
Tool: sarif.Tool{
Driver: sarifutils.CreateDummyDriver(validations.ContextualAnalysisToolName,
Expand Down Expand Up @@ -537,7 +537,7 @@ func getDockerScanTestResults(unique bool) (*results.SecurityCommandResults, val
ScannedStatus: "completed",
})
// Contextual analysis scan results
dockerImageTarget.JasResults.NewApplicabilityScanResults(0,
dockerImageTarget.JasResults.AddApplicabilityScanResults(0,
&sarif.Run{
Tool: sarif.Tool{
Driver: sarifutils.CreateDummyDriver(validations.ContextualAnalysisToolName,
Expand Down
2 changes: 1 addition & 1 deletion utils/results/results.go
Original file line number Diff line number Diff line change
Expand Up @@ -442,7 +442,7 @@ func (ssr *ScaScanResults) HasFindings() bool {
return false
}

func (jsr *JasScansResults) NewApplicabilityScanResults(exitCode int, runs ...*sarif.Run) {
func (jsr *JasScansResults) AddApplicabilityScanResults(exitCode int, runs ...*sarif.Run) {
jsr.ApplicabilityScanResults = append(jsr.ApplicabilityScanResults, ScanResult[[]*sarif.Run]{Scan: runs, StatusCode: exitCode})
}

Expand Down
60 changes: 57 additions & 3 deletions xsc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package main
import (
"encoding/json"
"errors"
"os"
"path/filepath"
"testing"

Expand Down Expand Up @@ -66,7 +67,7 @@ func TestXscAuditViolationsWithIgnoreRule(t *testing.T) {
_, cleanUpProject := securityTestUtils.CreateTestProjectEnvAndChdir(t, filepath.Join(filepath.FromSlash(tests.GetTestResourcesPath()), "projects", "jas", "jas"))
defer cleanUpProject()
// Create policy and watch for the git repo so we will also get violations (unknown = all vulnerabilities will be reported as violations)
policyName, cleanUpPolicy := securityTestUtils.CreateTestSecurityPolicy(t, "git-repo-ignore-rule-policy", utils.Unknown, true)
policyName, cleanUpPolicy := securityTestUtils.CreateTestSecurityPolicy(t, "git-repo-ignore-rule-policy", utils.Unknown, true, false)
defer cleanUpPolicy()
_, cleanUpWatch := securityTestUtils.CreateWatchForTests(t, policyName, "git-repo-ignore-rule-watch", xscutils.GetGitRepoUrlKey(validations.TestMockGitInfo.GitRepoHttpsCloneUrl))
defer cleanUpWatch()
Expand Down Expand Up @@ -100,10 +101,63 @@ func TestXscAuditViolationsWithIgnoreRule(t *testing.T) {
validations.VerifySimpleJsonResults(t, output, validations.ValidationParams{ExactResultsMatch: true, Total: &validations.TotalCount{}, Violations: &validations.ViolationCount{ValidateScan: &validations.ScanCount{}}})
}

func TestXrayAuditJasSkipNotApplicableCvesViolations(t *testing.T) {
// Init XSC tests also enabled analytics reporting.
_, _, cleanUpXsc := integration.InitXscTest(t, func() { securityTestUtils.ValidateXrayVersion(t, services.MinXrayVersionGitRepoKey) })
defer cleanUpXsc()
// Create the audit command with git repo context injected.
cliToRun, cleanUpHome := integration.InitTestWithMockCommandOrParams(t, false, getAuditCommandWithXscGitContext(validations.TestMockGitInfo))
defer cleanUpHome()
// Create the project to scan
_, cleanUpProject := securityTestUtils.CreateTestProjectEnvAndChdir(t, filepath.Join(filepath.FromSlash(tests.GetTestResourcesPath()), "projects", "jas", "jas"))
defer cleanUpProject()
// Create policy and watch for the git repo so we will also get violations - This watch DO NOT skip not-applicable results
var firstPolicyCleaned, firstWatchCleaned bool
policyName, cleanUpPolicy := securityTestUtils.CreateTestSecurityPolicy(t, "without-skip-non-applicable-policy", utils.Low, false, false)
defer func() {
if !firstPolicyCleaned {
cleanUpPolicy()
}
}()
watchName, cleanUpWatch := securityTestUtils.CreateWatchForTests(t, policyName, "without-skip-not-applicable-watch", xscutils.GetGitRepoUrlKey(validations.TestMockGitInfo.GitRepoHttpsCloneUrl))
defer func() {
if !firstWatchCleaned {
cleanUpWatch()
}
}()
// Run the audit command with git repo and verify violations are reported to the platform.
output, err := testAuditCommand(t, cliToRun, auditCommandTestParams{Format: string(format.SimpleJson), Watches: []string{watchName}, DisableFailOnFailedBuildFlag: true})
assert.NoError(t, err)
validations.VerifySimpleJsonResults(t, output, validations.ValidationParams{
Violations: &validations.ViolationCount{ValidateScan: &validations.ScanCount{Sca: 17, Sast: 1, Secrets: 15}},
ExactResultsMatch: true,
})

// We clean the initially created Policy and Watch that are related to the Git Repo resource, because we must have all related policies with skipNotApplicable=true
cleanUpWatch()
firstWatchCleaned = true
cleanUpPolicy()
firstPolicyCleaned = true

// Create policy and watch for the git repo so we will also get violations - This watch DO NOT skip not-applicable results
skipPolicyName, skipCleanUpPolicy := securityTestUtils.CreateTestSecurityPolicy(t, "skip-non-applicable-policy", utils.Low, false, true)
defer skipCleanUpPolicy()
skipWatchName, skipCleanUpWatch := securityTestUtils.CreateWatchForTests(t, skipPolicyName, "skip-not-applicable-watch", xscutils.GetGitRepoUrlKey(validations.TestMockGitInfo.GitRepoHttpsCloneUrl))
defer skipCleanUpWatch()
output, err = testAuditCommand(t, cliToRun, auditCommandTestParams{Format: string(format.SimpleJson), Watches: []string{skipWatchName}, DisableFailOnFailedBuildFlag: true})
assert.NoError(t, err)
validations.VerifySimpleJsonResults(t, output, validations.ValidationParams{
Violations: &validations.ViolationCount{ValidateScan: &validations.ScanCount{Sca: 12, Sast: 1, Secrets: 15}},
ExactResultsMatch: true,
})

}

func TestAuditJasViolationsProjectKeySimpleJson(t *testing.T) {
_, _, cleanUpXsc := integration.InitXscTest(t, func() { securityTestUtils.ValidateXrayVersion(t, services.MinXrayVersionGitRepoKey) })
defer cleanUpXsc()
if tests.TestJfrogPlatformProjectKeyEnvVar == "" {
testsJfrogPlatformProjectKey := os.Getenv(tests.TestJfrogPlatformProjectKeyEnvVar)
if testsJfrogPlatformProjectKey == "" {
t.Skipf("skipping test. %s is not set.", tests.TestJfrogPlatformProjectKeyEnvVar)
}
// Create the audit command with git repo context injected.
Expand All @@ -114,7 +168,7 @@ func TestAuditJasViolationsProjectKeySimpleJson(t *testing.T) {
_, cleanUpProject := securityTestUtils.CreateTestProjectEnvAndChdir(t, filepath.Join(filepath.FromSlash(tests.GetTestResourcesPath()), "projects", "jas", "jas"))
defer cleanUpProject()
// Create policy and watch for the project so we will get violations (unknown = all vulnerabilities will be reported as violations)
policyName, cleanUpPolicy := securityTestUtils.CreateTestSecurityPolicy(t, "project-key-jas-violations-policy", utils.Unknown, true)
policyName, cleanUpPolicy := securityTestUtils.CreateTestSecurityPolicy(t, "project-key-jas-violations-policy", utils.Unknown, true, false)
defer cleanUpPolicy()
_, cleanUpWatch := securityTestUtils.CreateTestProjectKeyWatch(t, policyName, "project-key-jas-violations-watch", *tests.JfrogTestProjectKey)
defer cleanUpWatch()
Expand Down

0 comments on commit c1d19ba

Please sign in to comment.