Pr add break build thresholds #440

AvivCx · 2020-10-05T14:09:18Z

By submitting a PR to this repository, you agree to the terms within the Checkmarx Code of Conduct. Please see the contributing guidelines for how to create and submit a high-quality PR for this repo.

Description

Purpose of this PR is to add threshold logic for cxflow cli flows
Now user can configure thresholds to control break-build operations

References

Documentation guide: https://github.com/checkmarx-ltd/cx-flow/wiki/Thresholds-and-policies

Testing

Add automation voerage to cover cli flows in the following feature files:

thresholds.feature
sca-thresholds.feature

Checklist

I have added documentation for new/changed functionality in this PR (if applicable). If documentaiton is a Wiki Update, please indicate desired changes within PR MD Comment
All active GitHub checks for tests, formatting, and security are passing
The correct base branch is being used

ofersk · 2020-10-06T08:21:54Z

src/main/java/com/checkmarx/flow/CxFlowRunner.java

+    private boolean checkIfBreakBuild(ScanRequest request, ScanResults results){
+        boolean breakBuildResult = false;
+
+        if(thresholdValidator.isThresholdsConfigurationExist(request)){


merge the 2 if statements to 1 (like you do in the else if

or make thresholdValidator.thresholdsExceeded test for isThresholdsConfigurationExist

Actually this is different case.
If thresholds not exist I want to go check the break build, but if exist and exceeded, shouldn't go to the 'else' flow
so 'exceeded' vs 'not exist' is a different result

ofersk · 2020-10-06T08:29:57Z

src/main/java/com/checkmarx/flow/service/ResultsService.java

@@ -335,6 +335,11 @@ private boolean requiresCxCustomFields(List<Field> fields) {
    }

    public boolean filteredIssuesPresent(ScanResults results) {
+        if(results.getAdditionalDetails()==null) {


try to a void 2 return statements.
ex.
return Optional.ofNullable(results.getAdditionalDetails())
.map(details -> details.get(Constants.SUMMARY_KEY))
.map(flow -> !flow.isEmpty())
.orElse(false);

ofersk · 2020-10-06T08:32:16Z

src/main/java/com/checkmarx/flow/service/ThresholdValidatorImpl.java

+            if (!isMergeAllowed) {
+                log.info("Merge is not allowed, because some thresholds were exceeded.");
+                requestResult = new OperationResult(OperationStatus.FAILURE, MERGE_FAILURE_DESCRIPTION);
+            }


you are not logging the flow of is merged is allowed here

ofersk · 2020-10-06T08:37:22Z

src/main/java/com/checkmarx/flow/service/ThresholdValidatorImpl.java

+        boolean sastThresholds;
+        boolean scaThresholds = false;
+
+        sastThresholds = scanRequest.getThresholds() != null || flowProperties.getThresholds() != null;


please inline this statement

ofersk · 2020-10-06T09:12:29Z

src/main/java/com/checkmarx/flow/service/ThresholdValidatorImpl.java

        }

        if (isAllowed && scaScanner.isEnabled()) {
-            isAllowed = isAllowedSca(scanResults, pullRequestReport);
+            isAllowed = isAllowedSca(scanResults, request);
        }

        return isAllowed;


this looks more readable to me:

boolean isSastAllowed = isSast() ? isAllowedSast(scanResults, request) : true; boolean isScaAllowed = scaScanner.isEnabled() ? isAllowedSca(scanResults, request) : true; return isSastAllowed && isScaAllowed

The result would be different. we want that if one of the scanner is enabled and its thresholds were exceeded - to return false (not allowed)
in your case, if SAST not enabled (only SCA), and SCA thresholds not exceeded - still we will return notAllowed

if SAST not enabled -> isSast() == false -> boolean isSastAllowed = true
SCA thresholds not exceeded -> scaScanner.isEnabled() == true -> isAllowedSca(scanResults, request) -> true
return isSastAllowed && isScaAllowed -> true && true -> true
still we will return notAllowed <- no as you can see we will return true

You are right I changed that (and simplified per intelij suggestion)

alex-ko-dev · 2020-10-06T08:22:07Z

src/main/java/com/checkmarx/flow/CxFlowRunner.java

@@ -507,6 +508,25 @@ private void processResults(ScanRequest request, ScanResults results) throws Exi
        }
    }

+    private boolean checkIfBreakBuild(ScanRequest request, ScanResults results){


I'd move this method into ThresholdValidator. E.g. ThresholdValidator.shouldInterruptBuild(...)
This way we won't need to expose the isThresholdsConfigurationExist method.

I don't think the ThresholdValidator shouldn't be aware of 'build', 'merge' or 'pull request' terminology
This is why I added 'isThresholdsConfigurationExist' as a generic check

ThresholdValidator is already aware of 'merge' and 'pull request' - see ThresholdValidatorImpl#isMergeAllowed. isMergeAllowed is reused in several services. I don't see any problem with this.
Adding shouldInterruptBuild to ThresholdValidator will put 2 similar checks next to each other. This will make the code more consistent.

The real problem with ThresholdValidator is that it does 2 things at once: validates both SAST and SCA results. There are "parallel" methods for each of them, e.g.

failSastPrIfResultHasAnyFindings failScaPrIfResultHasAnyFindings getSastFindingCountPerSeverity getScaFindingsCountsPerSeverity isAllowedSast isAllowedSca isAnySastThresholdExceeded isAnyScaThresholdsExceeded

This is not good code design. The SAST and SCA functionalities should be separated.

NimrodGolan · 2020-10-06T06:21:23Z

src/main/java/com/checkmarx/flow/CxFlowRunner.java

+            if(thresholdValidator.thresholdsExceeded(request, results)){
+                log.info("Fail build on Thresholds exceeded.");


Is it possible to log what were the actual results and what were the thresholds so the user will understand why we failed the build? (like we do on the --web flow).
Moreover, you can set both 'if' statements inline..

it is logged inside the ThresholdValidator.
regarding 'if' statement see 1st Ofer comment

NimrodGolan · 2020-10-06T06:25:02Z

src/main/java/com/checkmarx/flow/CxFlowRunner.java

+                breakBuildResult = true;
+            }
+        } else if(flowProperties.isBreakBuild() && resultsService.filteredIssuesPresent(results)){
+            log.info("Fail build on issues presented after executing filters");


I'm not sure I understand this message...

If thresholds not exist, the logic is to fail the build if there were left vulnerabilities after the processing
https://github.com/checkmarx-ltd/cx-flow/wiki/Thresholds-and-policies#thresholds-vs-basic-filters

Maybe something like: "Build failed, because some issues were found"?

NimrodGolan · 2020-10-06T06:33:26Z

src/main/java/com/checkmarx/flow/service/ThresholdValidatorImpl.java

-        log.info(isAllowedScanner ? "Merge is allowed, because no thresholds were exceeded." :
-                "Merge is not allowed, because some of the thresholds were exceeded.");
+        log.info(isAllowedScanner ? "No thresholds were exceeded." :
+                "Thresholds were exceeded.");


What's wrong with the original detailed message?

ThresholdValidator check the thresholds logic, and shouldn't be aware of 'build', 'merge' or 'pull request'.
Moreover in CLI flow, the original message is wrong

There are still places in ThresholdValidator with such references, but the refactoring was too dramatic for this stage

alex-ko-dev · 2020-10-07T07:21:49Z

src/main/java/com/checkmarx/flow/CxFlowRunner.java

@@ -507,6 +508,25 @@ private void processResults(ScanRequest request, ScanResults results) throws Exi
        }
    }

+    private boolean checkIfBreakBuild(ScanRequest request, ScanResults results){


ThresholdValidator is already aware of 'merge' and 'pull request' - see ThresholdValidatorImpl#isMergeAllowed. isMergeAllowed is reused in several services. I don't see any problem with this.
Adding shouldInterruptBuild to ThresholdValidator will put 2 similar checks next to each other. This will make the code more consistent.

The real problem with ThresholdValidator is that it does 2 things at once: validates both SAST and SCA results. There are "parallel" methods for each of them, e.g.

failSastPrIfResultHasAnyFindings failScaPrIfResultHasAnyFindings getSastFindingCountPerSeverity getScaFindingsCountsPerSeverity isAllowedSast isAllowedSca isAnySastThresholdExceeded isAnyScaThresholdsExceeded

This is not good code design. The SAST and SCA functionalities should be separated.

alex-ko-dev · 2020-10-07T07:25:22Z

src/main/java/com/checkmarx/flow/CxFlowRunner.java

+                breakBuildResult = true;
+            }
+        } else if(flowProperties.isBreakBuild() && resultsService.filteredIssuesPresent(results)){
+            log.info("Fail build on issues presented after executing filters");


Maybe something like: "Build failed, because some issues were found"?

alex-ko-dev · 2020-10-07T08:20:17Z

src/main/java/com/checkmarx/flow/service/ResultsService.java

+            // assuming SCA results only
+            return false;
+        }
+
        Map<String, Integer> flow = (Map<String, Integer>) results.getAdditionalDetails().get(Constants.SUMMARY_KEY);


The 'flow' variable name is unclear. It should be called something like 'findingCountPerSeverity'.

alex-ko-dev · 2020-10-07T08:55:58Z

src/main/java/com/checkmarx/flow/service/ResultsService.java

@@ -335,6 +335,11 @@ private boolean requiresCxCustomFields(List<Field> fields) {
    }

    public boolean filteredIssuesPresent(ScanResults results) {


If we only count SAST issues here, then the method should be called filteredSastIssuesPresent.

alex-ko-dev · 2020-10-07T08:57:43Z

src/main/java/com/checkmarx/flow/service/ResultsService.java

+            // assuming SCA results only
+            return false;
+        }
+
        Map<String, Integer> flow = (Map<String, Integer>) results.getAdditionalDetails().get(Constants.SUMMARY_KEY);
        return flow == null || !flow.isEmpty();


What about the case when the summary has 0 as a value? e.g. {high:0, medium:0}
In this case filteredIssuesPresent should return false.

in this case !flow.isEmpty() will return false (not present)

ofersk · 2020-10-08T06:14:16Z

src/test/java/com/checkmarx/flow/cucumber/integration/cli/sast/SastCliSteps.java

+        testContext.setCxFlowExecutionException(exception);
+    }
+
+    @And("cxflow should exit with {}")


sould be {int} and not {}

sonarcloud · 2020-10-08T06:27:42Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
1 Code Smell

0.0% Coverage
0.0% Duplication

The version of Java (1.8.0_265) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

alex-ko-dev · 2020-10-08T08:19:02Z

src/main/java/com/checkmarx/flow/service/ResultsService.java

-        Map<String, Integer> flow = (Map<String, Integer>) results.getAdditionalDetails().get(Constants.SUMMARY_KEY);
-        return flow == null || !flow.isEmpty();
+        Map<String, Integer> findingCountPerSeverity = (Map<String, Integer>) results.getAdditionalDetails().get(Constants.SUMMARY_KEY);
+        return findingCountPerSeverity == null || !findingCountPerSeverity.isEmpty();


If e.g. findingCountPerSeverity has a single 'HIGH' key with the value 0, then findingCountPerSeverity.isEmpty() will return false. The result of filteredSastIssuesPresent() will then be 'true'. However, in fact no issues are present.

AvivCx added 7 commits October 1, 2020 17:39

add break build thresholds

62a14f0

add break build logic

171b03c

fix sast cli integration tests

6e1ad09

fix component tests

82146f3

add component tests to sca flow

3608fec

add additionalDetails validation for only SCA results

89df6b3

fix skipped sca cli steps

6923b84

AvivCx requested review from alex-ko-dev, NimrodGolan, ofersk and olgakil October 6, 2020 05:40

ofersk reviewed Oct 6, 2020

View reviewed changes

alex-ko-dev reviewed Oct 6, 2020

View reviewed changes

NimrodGolan reviewed Oct 6, 2020

View reviewed changes

AvivCx added 2 commits October 7, 2020 09:49

code review comment

2ecf730

pull latest changes to fix deploy step

cbf5478

alex-ko-dev reviewed Oct 7, 2020

View reviewed changes

fix integration tests

5c47936

AvivCx requested a review from NimrodGolan October 7, 2020 13:37

NimrodGolan previously approved these changes Oct 7, 2020

View reviewed changes

ofersk reviewed Oct 8, 2020

View reviewed changes

fix code review comment

723b2da

AvivCx dismissed NimrodGolan’s stale review via 723b2da October 8, 2020 06:24

alex-ko-dev reviewed Oct 8, 2020

View reviewed changes

AvivCx merged commit 9c37df6 into develop Oct 8, 2020

AvivCx deleted the pr-Add-break-build-thresholds branch November 16, 2020 06:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pr add break build thresholds #440

Pr add break build thresholds #440

AvivCx commented Oct 5, 2020 •

edited

Loading

ofersk Oct 6, 2020

ofersk Oct 6, 2020

AvivCx Oct 7, 2020 •

edited

Loading

ofersk Oct 6, 2020

ofersk Oct 6, 2020

AvivCx Oct 7, 2020

ofersk Oct 6, 2020

ofersk Oct 6, 2020

AvivCx Oct 7, 2020

ofersk Oct 8, 2020

AvivCx Oct 8, 2020

alex-ko-dev Oct 6, 2020

AvivCx Oct 7, 2020 •

edited

Loading

alex-ko-dev Oct 7, 2020

NimrodGolan Oct 6, 2020

AvivCx Oct 7, 2020

NimrodGolan Oct 6, 2020

AvivCx Oct 7, 2020

alex-ko-dev Oct 7, 2020

AvivCx Oct 7, 2020

NimrodGolan Oct 6, 2020

AvivCx Oct 7, 2020 •

edited

Loading

alex-ko-dev Oct 7, 2020

alex-ko-dev Oct 7, 2020

alex-ko-dev Oct 7, 2020

AvivCx Oct 7, 2020

alex-ko-dev Oct 7, 2020

AvivCx Oct 7, 2020

alex-ko-dev Oct 7, 2020

AvivCx Oct 7, 2020

ofersk Oct 8, 2020

sonarcloud bot commented Oct 8, 2020

alex-ko-dev Oct 8, 2020

		if(thresholdValidator.thresholdsExceeded(request, results)){
		log.info("Fail build on Thresholds exceeded.");

		@@ -335,6 +335,11 @@ private boolean requiresCxCustomFields(List<Field> fields) {
		}

		public boolean filteredIssuesPresent(ScanResults results) {

Pr add break build thresholds #440

Pr add break build thresholds #440

Conversation

AvivCx commented Oct 5, 2020 • edited Loading

Description

References

Testing

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AvivCx Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AvivCx Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AvivCx Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Oct 8, 2020

Choose a reason for hiding this comment

AvivCx commented Oct 5, 2020 •

edited

Loading

AvivCx Oct 7, 2020 •

edited

Loading

AvivCx Oct 7, 2020 •

edited

Loading

AvivCx Oct 7, 2020 •

edited

Loading