Skip to content

Commit b1f4508

Browse files
eranturgemaneranturgeman
authored
Fix build failure logic to consider applied policies rules (jfrog#501)
1 parent 915c689 commit b1f4508

File tree

4 files changed

+321
-19
lines changed

4 files changed

+321
-19
lines changed

‎commands/audit/audit.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,12 @@ func ProcessResultsAndOutput(auditResults *results.SecurityCommandResults, outpu
240240
return
241241
}
242242
// Only in case Xray's context was given (!auditCmd.IncludeVulnerabilities), and the user asked to fail the build accordingly, do so.
243-
if failBuild && auditResults.HasViolationContext() && results.CheckIfFailBuild(auditResults.GetScaScansXrayResults()) {
243+
shouldFailBuildByPolicy, err := results.CheckIfFailBuild(auditResults)
244+
if err != nil {
245+
return fmt.Errorf("failed to check if the build should fail: %w", err)
246+
}
247+
248+
if failBuild && auditResults.HasViolationContext() && shouldFailBuildByPolicy {
244249
err = results.NewFailBuildError()
245250
}
246251
return

‎commands/scan/scan.go‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -248,10 +248,13 @@ func (scanCmd *ScanCommand) RunAndRecordResults(cmdType utils.CommandType, recor
248248
}
249249
// We consider failing the build only when --fail=true. If a user had provided --fail=false, we don't fail the build even when fail-build rules are applied.
250250
// If violation context was provided, we need to check all existing violations for fail-build rules.
251-
if scanCmd.fail && scanCmd.resultsContext.HasViolationContext() {
252-
if results.CheckIfFailBuild(cmdResults.GetScaScansXrayResults()) {
253-
return results.NewFailBuildError()
254-
}
251+
shouldFailBuildByPolicy, err := results.CheckIfFailBuild(cmdResults)
252+
if err != nil {
253+
return fmt.Errorf("failed to check if build should fail: %w", err)
254+
}
255+
256+
if scanCmd.fail && scanCmd.resultsContext.HasViolationContext() && shouldFailBuildByPolicy {
257+
return results.NewFailBuildError()
255258
}
256259
log.Info("Scan completed successfully.")
257260
return nil

‎utils/results/common.go‎

Lines changed: 93 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -53,18 +53,106 @@ func NewFailBuildError() error {
5353
return coreutils.CliError{ExitCode: coreutils.ExitCodeVulnerableBuild, ErrorMsg: "One or more of the detected violations are configured to fail the build that including them"}
5454
}
5555

56-
// In case one (or more) of the violations contains the field FailBuild set to true, CliError with exit code 3 will be returned.
57-
func CheckIfFailBuild(results []services.ScanResponse) bool {
58-
for _, result := range results {
59-
for _, violation := range result.Violations {
60-
if violation.FailBuild {
56+
// This func iterates every violation and checks if there is a violation that should fail the build.
57+
// The build should be failed if there exists at least one violation in any target that holds the following conditions:
58+
// 1) The violation is set to fail the build by FailBuild or FailPr
59+
// 2) The violation has applicability status other than 'NotApplicable' OR the violation has 'NotApplicable' status and is not set to skip-non-applicable
60+
func CheckIfFailBuild(auditResults *SecurityCommandResults) (bool, error) {
61+
for _, target := range auditResults.Targets {
62+
shouldFailBuild := false
63+
// We first check if JasResults exist so we can extract CA results and consider Applicability status when checking if the build should fail.
64+
if target.JasResults == nil {
65+
shouldFailBuild = checkIfFailBuildWithoutConsideringApplicability(target)
66+
} else {
67+
// If JasResults are not empty we check old and new violation while considering Applicability status and Skip-not-applicable policy rule.
68+
if err := checkIfFailBuildConsideringApplicability(target, auditResults.EntitledForJas, &shouldFailBuild); err != nil {
69+
return false, fmt.Errorf("failed to check if build should fail for target %s: %w", target.ScanTarget.Target, err)
70+
}
71+
}
72+
if shouldFailBuild {
73+
// If we found a violation that should fail the build, we return true.
74+
return true, nil
75+
}
76+
}
77+
return false, nil
78+
}
79+
80+
func checkIfFailBuildConsideringApplicability(target *TargetResults, entitledForJas bool, shouldFailBuild *bool) error {
81+
jasApplicabilityResults := target.JasResults.GetApplicabilityScanResults()
82+
83+
// Get new violations from the target
84+
newViolations := target.ScaResults.Violations
85+
86+
// Here we iterate the new violation results and check if any of them should fail the build.
87+
_, _, err := ForEachScanGraphViolation(
88+
target.ScanTarget,
89+
newViolations,
90+
entitledForJas,
91+
jasApplicabilityResults,
92+
checkIfShouldFailBuildAccordingToPolicy(shouldFailBuild),
93+
nil,
94+
nil)
95+
if err != nil {
96+
return err
97+
}
98+
99+
// Here we iterate the deprecated violation results to check if any of them should fail the build.
100+
// TODO remove this part once the DeprecatedXrayResults are completely removed and no longer in use
101+
for _, result := range target.ScaResults.DeprecatedXrayResults {
102+
deprecatedViolations := result.Scan.Violations
103+
_, _, err = ForEachScanGraphViolation(
104+
target.ScanTarget,
105+
deprecatedViolations,
106+
entitledForJas,
107+
jasApplicabilityResults,
108+
checkIfShouldFailBuildAccordingToPolicy(shouldFailBuild),
109+
nil,
110+
nil)
111+
if err != nil {
112+
return err
113+
}
114+
}
115+
return nil
116+
}
117+
118+
func checkIfFailBuildWithoutConsideringApplicability(target *TargetResults) bool {
119+
for _, newViolation := range target.ScaResults.Violations {
120+
if newViolation.FailBuild || newViolation.FailPr {
121+
return true
122+
}
123+
}
124+
125+
// TODO remove this for loop once the DeprecatedXrayResults are completely removed and no longer in use
126+
for _, scanResponse := range target.GetScaScansXrayResults() {
127+
for _, oldViolation := range scanResponse.Violations {
128+
if oldViolation.FailBuild || oldViolation.FailPr {
61129
return true
62130
}
63131
}
64132
}
65133
return false
66134
}
67135

136+
func checkIfShouldFailBuildAccordingToPolicy(shouldFailBuild *bool) func(violation services.Violation, cves []formats.CveRow, applicabilityStatus jasutils.ApplicabilityStatus, severity severityutils.Severity, impactedPackagesId string, fixedVersion []string, directComponents []formats.ComponentRow, impactPaths [][]formats.ComponentRow) (err error) {
137+
return func(violation services.Violation, cves []formats.CveRow, applicabilityStatus jasutils.ApplicabilityStatus, severity severityutils.Severity, impactedPackagesId string, fixedVersion []string, directComponents []formats.ComponentRow, impactPaths [][]formats.ComponentRow) (err error) {
138+
if !violation.FailBuild && !violation.FailPr {
139+
// If the violation is not set to fail the build we simply return
140+
return nil
141+
}
142+
// If the violation is set to fail the build, we check if the violation has NotApplicable status and is set to skip-non-applicable.
143+
// If the violation is NotApplicable and is set to skip-non-applicable, we don't fail the build.
144+
// If the violation has any other status OR has NotApplicable status but is not set to skip-not-applicable, we fail the build.
145+
var shouldSkip bool
146+
if shouldSkip, err = shouldSkipNotApplicable(violation, applicabilityStatus); err != nil {
147+
return err
148+
}
149+
if !shouldSkip {
150+
*shouldFailBuild = true
151+
}
152+
return nil
153+
}
154+
}
155+
68156
type ParseScanGraphVulnerabilityFunc func(vulnerability services.Vulnerability, cves []formats.CveRow, applicabilityStatus jasutils.ApplicabilityStatus, severity severityutils.Severity, impactedPackagesId string, fixedVersion []string, directComponents []formats.ComponentRow, impactPaths [][]formats.ComponentRow) error
69157
type ParseScanGraphViolationFunc func(violation services.Violation, cves []formats.CveRow, applicabilityStatus jasutils.ApplicabilityStatus, severity severityutils.Severity, impactedPackagesId string, fixedVersion []string, directComponents []formats.ComponentRow, impactPaths [][]formats.ComponentRow) error
70158
type ParseLicenseFunc func(license services.License, impactedPackagesId string, directComponents []formats.ComponentRow, impactPaths [][]formats.ComponentRow) error

‎utils/results/common_test.go‎

Lines changed: 215 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -24,21 +24,227 @@ import (
2424

2525
func TestViolationFailBuild(t *testing.T) {
2626
components := map[string]services.Component{"gav://antparent:ant:1.6.5": {}}
27+
2728
tests := []struct {
28-
violations []services.Violation
29-
expectedError bool
29+
name string
30+
auditResults *SecurityCommandResults
31+
expectedResult bool
3032
}{
31-
{[]services.Violation{{Components: components, FailBuild: false}, {Components: components, FailBuild: false}, {Components: components, FailBuild: false}}, false},
32-
{[]services.Violation{{Components: components, FailBuild: false}, {Components: components, FailBuild: true}, {Components: components, FailBuild: false}}, true},
33-
{[]services.Violation{{Components: components, FailBuild: true}, {Components: components, FailBuild: true}, {Components: components, FailBuild: true}}, true},
33+
{
34+
name: "non-applicable violations with FailBuild & no skip-non-applicable in ScaResults.Violations - build should fail",
35+
auditResults: createSecurityCommandResultsForFailBuildTest(true, true, utils.NewBoolPtr(false)),
36+
expectedResult: true,
37+
},
38+
{
39+
name: "non-applicable violations with FailBuild & skip-non-applicable in ScaResults.Violations - build should not fail",
40+
auditResults: createSecurityCommandResultsForFailBuildTest(true, true, utils.NewBoolPtr(true)),
41+
expectedResult: false,
42+
},
43+
{
44+
name: "non-applicable violations with FailBuild & no skip-non-applicable in DeprecatedXrayResults - build should fail",
45+
auditResults: createSecurityCommandResultsForFailBuildTest(false, true, utils.NewBoolPtr(false)),
46+
expectedResult: true,
47+
},
48+
{
49+
name: "non-applicable violations with FailBuild & skip-non-applicable in DeprecatedXrayResults - build should not fail",
50+
auditResults: createSecurityCommandResultsForFailBuildTest(false, true, utils.NewBoolPtr(true)),
51+
expectedResult: false,
52+
},
53+
{
54+
name: "no applicability results, violations with FailBuild in DeprecatedXrayResults - build should fail",
55+
auditResults: createSecurityCommandResultsForFailBuildTest(false, false, nil),
56+
expectedResult: true,
57+
},
58+
{
59+
name: "no applicability results, violations with FailBuild in ScaResults.Violations - build should fail",
60+
auditResults: createSecurityCommandResultsForFailBuildTest(true, false, nil),
61+
expectedResult: true,
62+
},
63+
{
64+
name: "multiple targets - first target should not fail, second target should fail",
65+
auditResults: &SecurityCommandResults{
66+
EntitledForJas: true,
67+
Targets: []*TargetResults{
68+
{
69+
// First target - should not fail
70+
ScanTarget: ScanTarget{Target: "test-target-1"},
71+
ScaResults: &ScaScanResults{
72+
Violations: []services.Violation{
73+
{
74+
// Violation 1: FailBuild & FailPr set to false - should not fail
75+
Components: components,
76+
ViolationType: utils.ViolationTypeSecurity.String(),
77+
FailBuild: false,
78+
FailPr: false,
79+
Cves: []services.Cve{{Id: "CVE-2024-1111"}},
80+
Severity: "High",
81+
},
82+
{
83+
// Violation 2: FailBuild=true, notApplicable, skip-not-applicable - should not fail
84+
Components: components,
85+
ViolationType: utils.ViolationTypeSecurity.String(),
86+
FailBuild: true,
87+
Policies: []services.Policy{{SkipNotApplicable: true}},
88+
Cves: []services.Cve{{Id: "CVE-2024-2222"}},
89+
Severity: "High",
90+
},
91+
},
92+
},
93+
JasResults: &JasScansResults{
94+
ApplicabilityScanResults: []ScanResult[[]*sarif.Run]{
95+
{
96+
Scan: []*sarif.Run{
97+
{
98+
Tool: &sarif.Tool{
99+
Driver: &sarif.ToolComponent{
100+
Rules: []*sarif.ReportingDescriptor{
101+
{
102+
ID: utils.NewStringPtr(jasutils.CveToApplicabilityRuleId("CVE-2024-2222")),
103+
Properties: &sarif.PropertyBag{
104+
Properties: map[string]interface{}{
105+
jasutils.ApplicabilitySarifPropertyKey: "not_applicable",
106+
},
107+
},
108+
},
109+
},
110+
},
111+
},
112+
},
113+
},
114+
},
115+
},
116+
},
117+
},
118+
{
119+
// Second target - should fail
120+
ScanTarget: ScanTarget{Target: "test-target-2"},
121+
ScaResults: &ScaScanResults{
122+
Violations: []services.Violation{
123+
{
124+
// Violation 1: FailBuild=true, notApplicable, NOT skip-not-applicable - should fail
125+
Components: components,
126+
ViolationType: utils.ViolationTypeSecurity.String(),
127+
FailBuild: true,
128+
Policies: []services.Policy{{SkipNotApplicable: false}},
129+
Cves: []services.Cve{{Id: "CVE-2024-3333"}},
130+
Severity: "High",
131+
},
132+
{
133+
// Violation 2: FailBuild & FailPr set to false - should not fail
134+
Components: components,
135+
ViolationType: utils.ViolationTypeSecurity.String(),
136+
FailBuild: false,
137+
FailPr: false,
138+
Cves: []services.Cve{{Id: "CVE-2024-4444"}},
139+
Severity: "High",
140+
},
141+
},
142+
},
143+
JasResults: &JasScansResults{
144+
ApplicabilityScanResults: []ScanResult[[]*sarif.Run]{
145+
{
146+
Scan: []*sarif.Run{
147+
{
148+
Tool: &sarif.Tool{
149+
Driver: &sarif.ToolComponent{
150+
Rules: []*sarif.ReportingDescriptor{
151+
{
152+
ID: utils.NewStringPtr(jasutils.CveToApplicabilityRuleId("CVE-2024-3333")),
153+
Properties: &sarif.PropertyBag{
154+
Properties: map[string]interface{}{
155+
jasutils.ApplicabilitySarifPropertyKey: "not_applicable",
156+
},
157+
},
158+
},
159+
},
160+
},
161+
},
162+
},
163+
},
164+
},
165+
},
166+
},
167+
},
168+
},
169+
},
170+
expectedResult: true, // Should fail because second target has a violation that should fail
171+
},
34172
}
35173

36174
for _, test := range tests {
37-
var err error
38-
if CheckIfFailBuild([]services.ScanResponse{{Violations: test.violations}}) {
39-
err = NewFailBuildError()
175+
t.Run(test.name, func(t *testing.T) {
176+
shouldFailBuild, err := CheckIfFailBuild(test.auditResults)
177+
assert.NoError(t, err)
178+
assert.Equal(t, test.expectedResult, shouldFailBuild)
179+
})
180+
}
181+
}
182+
183+
func createSecurityCommandResultsForFailBuildTest(useNewViolations bool, hasJasResults bool, skipNotApplicable *bool) *SecurityCommandResults {
184+
components := map[string]services.Component{"gav://antparent:ant:1.6.5": {}}
185+
cveId := "CVE-2024-1234"
186+
187+
target := &TargetResults{
188+
ScanTarget: ScanTarget{Target: "test-target"},
189+
ScaResults: &ScaScanResults{},
190+
}
191+
192+
violation := services.Violation{
193+
Components: components,
194+
ViolationType: utils.ViolationTypeSecurity.String(),
195+
FailBuild: true,
196+
Cves: []services.Cve{{Id: cveId}},
197+
Severity: "High",
198+
}
199+
200+
if skipNotApplicable != nil {
201+
violation.Policies = []services.Policy{{SkipNotApplicable: *skipNotApplicable}}
202+
}
203+
204+
if useNewViolations {
205+
target.ScaResults.Violations = []services.Violation{violation}
206+
} else {
207+
target.ScaResults.DeprecatedXrayResults = []ScanResult[services.ScanResponse]{
208+
{
209+
Scan: services.ScanResponse{
210+
Violations: []services.Violation{violation},
211+
},
212+
},
213+
}
214+
}
215+
216+
if hasJasResults {
217+
target.JasResults = &JasScansResults{
218+
ApplicabilityScanResults: []ScanResult[[]*sarif.Run]{
219+
{
220+
Scan: []*sarif.Run{
221+
{
222+
Tool: &sarif.Tool{
223+
Driver: &sarif.ToolComponent{
224+
Rules: []*sarif.ReportingDescriptor{
225+
{
226+
ID: utils.NewStringPtr(jasutils.CveToApplicabilityRuleId(cveId)),
227+
Properties: &sarif.PropertyBag{
228+
Properties: map[string]interface{}{
229+
jasutils.ApplicabilitySarifPropertyKey: "not_applicable",
230+
},
231+
},
232+
},
233+
},
234+
},
235+
},
236+
},
237+
},
238+
},
239+
},
40240
}
41-
assert.Equal(t, test.expectedError, err != nil)
241+
} else {
242+
target.JasResults = nil
243+
}
244+
245+
return &SecurityCommandResults{
246+
EntitledForJas: true,
247+
Targets: []*TargetResults{target},
42248
}
43249
}
44250

0 commit comments

Comments
 (0)