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

Jump to bottom

Pr add break build thresholds #440

Merged
merged 11 commits into from
Oct 8, 2020
Merged

Pr add break build thresholds #440

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
62a14f0
add break build thresholds
AvivCx Sep 24, 2020
171b03c
add break build logic
AvivCx Sep 25, 2020
6e1ad09
fix sast cli integration tests
AvivCx Sep 26, 2020
82146f3
fix component tests
AvivCx Sep 26, 2020
3608fec
add component tests to sca flow
AvivCx Oct 1, 2020
89df6b3
add additionalDetails validation for only SCA results
AvivCx Oct 1, 2020
6923b84
fix skipped sca cli steps
AvivCx Oct 1, 2020
2ecf730
code review comment
AvivCx Oct 7, 2020
cbf5478
pull latest changes to fix deploy step
AvivCx Oct 7, 2020
5c47936
fix integration tests
AvivCx Oct 7, 2020
723b2da
fix code review comment
AvivCx Oct 8, 2020
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
22 changes: 21 additions & 1 deletion src/main/java/com/checkmarx/flow/CxFlowRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ public class CxFlowRunner implements ApplicationRunner {
private final FilterFactory filterFactory;
private final ConfigurationOverrider configOverrider;
private final List<VulnerabilityScanner> scanners;
private final ThresholdValidator thresholdValidator;
private static final String ERROR_BREAK_MSG = "Exiting with Error code 10 due to issues present";

@Override
Expand Down Expand Up @@ -497,7 +498,7 @@ private void publishLatestScanResults(ScanRequest request) throws ExitThrowable
private void processResults(ScanRequest request, ScanResults results) throws ExitThrowable {
try {
resultsService.processResults(request, results, null);
if (flowProperties.isBreakBuild() && resultsService.filteredIssuesPresent(results)) {
if (checkIfBreakBuild(request, results)) {
log.error(ERROR_BREAK_MSG);
exit(ExitCode.BUILD_INTERRUPTED);
}
Expand All @@ -507,6 +508,25 @@ private void processResults(ScanRequest request, ScanResults results) throws Exi
}
}

private boolean checkIfBreakBuild(ScanRequest request, ScanResults results){
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@AvivCx AvivCx Oct 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

boolean breakBuildResult = false;

if(thresholdValidator.isThresholdsConfigurationExist(request)){
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

or make thresholdValidator.thresholdsExceeded test for isThresholdsConfigurationExist

Copy link
Contributor Author

@AvivCx AvivCx Oct 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

if(thresholdValidator.thresholdsExceeded(request, results)){
log.info("Fail build on Thresholds exceeded.");
Comment on lines +515 to +516
Copy link
Contributor

Choose a reason for hiding this comment

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

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..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

breakBuildResult = true;
}
else {
log.info("Build succeeded. all checks passed");
}

return breakBuildResult;
}

private ScanResults runOnActiveScanners(Function<? super VulnerabilityScanner, ScanResults> action) throws ExitThrowable {
try {
ScanResults[] scanResultslist = scanners.stream()
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/com/checkmarx/flow/service/ResultsService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,11 @@ private boolean requiresCxCustomFields(List<Field> fields) {
}

public boolean filteredIssuesPresent(ScanResults results) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

if(results.getAdditionalDetails()==null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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);

// assuming SCA results only
return false;
}

Map<String, Integer> flow = (Map<String, Integer>) results.getAdditionalDetails().get(Constants.SUMMARY_KEY);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

return flow == null || !flow.isEmpty();
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/com/checkmarx/flow/service/ThresholdValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
package com.checkmarx.flow.service;

import com.checkmarx.flow.config.RepoProperties;
import com.checkmarx.flow.dto.ScanRequest;
import com.checkmarx.flow.dto.report.PullRequestReport;
import com.checkmarx.sdk.dto.ScanResults;

public interface ThresholdValidator {
boolean isMergeAllowed(ScanResults results, RepoProperties repoProperties, PullRequestReport pullRequestReport);
boolean thresholdsExceeded(ScanRequest request, ScanResults results);
boolean isThresholdsConfigurationExist(ScanRequest scanRequest);
}
75 changes: 50 additions & 25 deletions src/main/java/com/checkmarx/flow/service/ThresholdValidatorImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import com.checkmarx.sdk.config.Constants;
import com.checkmarx.sdk.config.ScaConfig;
import com.checkmarx.sdk.config.ScaProperties;
import com.checkmarx.sdk.dto.Sca;
import com.checkmarx.sdk.dto.ScanResults;
import com.cx.restclient.dto.scansummary.Severity;
import com.cx.restclient.ast.dto.sca.report.Finding;
Expand Down Expand Up @@ -53,30 +54,58 @@ public ThresholdValidatorImpl(@Lazy SastScanner sastScanner, @Lazy SCAScanner sc
@Override
public boolean isMergeAllowed(ScanResults scanResults, RepoProperties repoProperties, PullRequestReport pullRequestReport) {
OperationResult requestResult = new OperationResult(OperationStatus.SUCCESS, MERGE_SUCCESS_DESCRIPTION);
boolean isMergeAllowed = isAllowed(scanResults, repoProperties, pullRequestReport);
boolean isMergeAllowed = true;

if (!isMergeAllowed) {
requestResult = new OperationResult(OperationStatus.FAILURE, MERGE_FAILURE_DESCRIPTION);
if (!repoProperties.isErrorMerge()) {
log.info("Merge is allowed, because error-merge is set to false.");
pullRequestReport.setPullRequestResult(requestResult);
}
else{
isMergeAllowed = isAllowed(scanResults, pullRequestReport.getScanRequest());

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added log

}

pullRequestReport.setPullRequestResult(requestResult);

return isMergeAllowed;
}

private boolean isAllowed(ScanResults scanResults, RepoProperties repoProperties, PullRequestReport pullRequestReport) {
if (!repoProperties.isErrorMerge()) {
log.info("Merge is allowed, because error-merge is set to false.");
return true;
@Override
public boolean thresholdsExceeded(ScanRequest request, ScanResults results){
return !isAllowed(results, request);
}

@Override
public boolean isThresholdsConfigurationExist(ScanRequest scanRequest){
boolean sastThresholds;
boolean scaThresholds = false;

sastThresholds = scanRequest.getThresholds() != null || flowProperties.getThresholds() != null;
Copy link
Contributor

Choose a reason for hiding this comment

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

please inline this statement

if(scaProperties != null){
scaThresholds = scaProperties.getThresholdsSeverity() != null || scaProperties.getThresholdsScore() != null;
}

if (!scaThresholds && scanRequest.getScaConfig() != null) {
scaThresholds = scanRequest.getScaConfig().getThresholdsSeverity() != null || scanRequest.getScaConfig().getThresholdsScore() != null;
}

log.info("Checking Thresholds exists. sast thresholds: {}. sca thresholds: {}", sastThresholds, scaThresholds);
return sastThresholds || scaThresholds;
}

private boolean isAllowed(ScanResults scanResults, ScanRequest request) {

boolean isAllowed = true;
if (isSast()) {
isAllowed = isAllowedSast(scanResults, pullRequestReport);
isAllowed = isAllowedSast(scanResults, request);
}

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

return isAllowed;
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks more readable to me:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Expand All @@ -86,37 +115,34 @@ private boolean isSast() {
return sastScanner.isEnabled();
}

private boolean isAllowedSca(ScanResults scanResults, PullRequestReport pullRequestReport) {
private boolean isAllowedSca(ScanResults scanResults, ScanRequest request) {
log.debug("Checking if CxSCA pull request merge is allowed.");
Map<Severity, Integer> scaThresholdsSeverity = getScaEffectiveThresholdsSeverity(pullRequestReport.getScanRequest());
Double scaThresholdsScore = getScaEffectiveThresholdsScore(pullRequestReport.getScanRequest());
Map<Severity, Integer> scaThresholdsSeverity = getScaEffectiveThresholdsSeverity(request);
Double scaThresholdsScore = getScaEffectiveThresholdsScore(request);

writeMapToLog(scaThresholdsSeverity, "Using CxSCA thresholds severity");
writeMapToLog(scaThresholdsScore, "Using CxSCA thresholds score");
pullRequestReport.setScaThresholdsSeverity(scaThresholdsSeverity);
pullRequestReport.setScaThresholdsScore(scaThresholdsScore);

boolean isAllowedSca = !isAnyScaThresholdsExceeded(scanResults, scaThresholdsSeverity, scaThresholdsScore, pullRequestReport);
boolean isAllowedSca = !isAnyScaThresholdsExceeded(scanResults, scaThresholdsSeverity, scaThresholdsScore);
isAllowedScannerToLog(isAllowedSca);

return isAllowedSca;
}

private boolean isAllowedSast(ScanResults scanResults, PullRequestReport pullRequestReport) {
private boolean isAllowedSast(ScanResults scanResults, ScanRequest request) {
log.debug("Checking if CxSAST pull request merge is allowed.");
Map<FindingSeverity, Integer> thresholds = getSastEffectiveThresholds(pullRequestReport.getScanRequest());
Map<FindingSeverity, Integer> thresholds = getSastEffectiveThresholds(request);
writeMapToLog(thresholds, "Using CxSAST thresholds");
pullRequestReport.setThresholds(thresholds);

boolean isAllowed = !isAnySastThresholdExceeded(scanResults, thresholds, pullRequestReport);
boolean isAllowed = !isAnySastThresholdExceeded(scanResults, thresholds);
isAllowedScannerToLog(isAllowed);

return isAllowed;
}

private void isAllowedScannerToLog(boolean isAllowedScanner) {
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.");
Comment on lines -118 to +143
Copy link
Contributor

Choose a reason for hiding this comment

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

What's wrong with the original detailed message?

Copy link
Contributor Author

@AvivCx AvivCx Oct 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

}

private Map<FindingSeverity, Integer> getSastEffectiveThresholds(ScanRequest scanRequest) {
Expand Down Expand Up @@ -216,11 +242,10 @@ private boolean areScaThresholdsSeverityDefinedFromProperties() {
}

private static boolean isAnyScaThresholdsExceeded(ScanResults scanResults, Map<Severity, Integer> scaThresholds,
Double scaThresholdsScore, PullRequestReport pullRequestReport) {
Double scaThresholdsScore) {
boolean isExceeded = isExceedsScaThresholdsScore(scanResults, scaThresholdsScore);

Map<Severity, Integer> scaFindingsCountsPerSeverity = getScaFindingsCountsPerSeverity(scanResults);
pullRequestReport.setScaFindingsSeverityCount(scaFindingsCountsPerSeverity);

for (Map.Entry<Severity, Integer> entry : scaFindingsCountsPerSeverity.entrySet()) {
Severity severity = entry.getKey();
Expand Down Expand Up @@ -250,10 +275,10 @@ private static boolean isExceedsScaThresholdsScore(ScanResults scanResults, Doub
return isExceeded;
}

private static boolean isAnySastThresholdExceeded(ScanResults scanResults, Map<FindingSeverity, Integer> sastThresholds, PullRequestReport pullRequestReport) {
private static boolean isAnySastThresholdExceeded(ScanResults scanResults, Map<FindingSeverity, Integer> sastThresholds) {
boolean result = false;
Map<FindingSeverity, Integer> findingsPerSeverity = getSastFindingCountPerSeverity(scanResults);
pullRequestReport.setFindingsPerSeverity(findingsPerSeverity);

for (Map.Entry<FindingSeverity, Integer> entry : findingsPerSeverity.entrySet()) {
if (isExceedsSastThreshold(entry, sastThresholds)) {
result = true;
Expand Down
4 changes: 3 additions & 1 deletion src/test/java/com/checkmarx/flow/cucumber/component/batch/BatchComponentSteps.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ public class BatchComponentSteps {
private final OsaScannerService osaScannerService;
private final FilterFactory filterFactory;
private final ConfigurationOverrider configOverrider;
private final ThresholdValidator thresholdValidator;

private CxFlowRunner cxFlowRunner;
private String projectName;
Expand All @@ -69,7 +70,8 @@ public void sastClientIsMocked() throws CheckmarxException {
osaScannerService,
filterFactory,
configOverrider,
scanners);
scanners,
thresholdValidator);
}

@Given("project is provided: {string} and team: {string}")
Expand Down
Loading