feat(fortifyExecuteScan): implement a system to limit the number of API calls upon request failures (#3818)

* feat(fortifyExecuteScan): add a max number of retries for API calls in SARIF conversion

* feat(checkmarxExecuteScan): implement max number of retries on API call for descriptions in SARIF processing

* feat(checkmarx/fortify): extra logging line when failing an API request in SARIF conversion

* fix(fortifyExecuteScan): panic if undefined projectversion in sarif

* fix(fortifyExecuteScan): logging improvement

* fix(fortifyExecuteScan): wrong if condition caused crash

* fix(fortifyExecuteScan): do not log if retries hit -1, adjust logging

* fix(SARIF): commenting API calls for Checkmarx until a solution can be found for the API issues

* feat(SARIF): add omitempty to extensions

pkg/checkmarx/cxxml_to_sarif.go

@@ -153,32 +153,40 @@ func Parse(sys System, data []byte, scanID int) (format.SARIF, error) {
 	baseURL := "https://" + strings.Split(cxxml.DeepLink, "/")[2] + "/CxWebClient/ScanQueryDescription.aspx?"
 	cweIdsForTaxonomies := make(map[string]int) //use a map to avoid duplicates
 	cweCounter := 0
-	var apiDescription string
+	//maxretries := 5
 
 	//CxXML files contain a CxXMLResults > Query object, which represents a broken rule or type of vuln
 	//This Query object contains a list of Result objects, each representing an occurence
 	//Each Result object contains a ResultPath, which represents the exact location of the occurence (the "Snippet")
 	log.Entry().Debug("[SARIF] Now handling results.")
 	for i := 0; i < len(cxxml.Query); i++ {
-		descriptionFetched := false
 		//add cweid to array
 		cweIdsForTaxonomies[cxxml.Query[i].CweID] = cweCounter
 		cweCounter = cweCounter + 1
 		for j := 0; j < len(cxxml.Query[i].Result); j++ {
+			var apiDescription string
 			result := *new(format.Results)
 
+			// COMMENTED UNTIL CHECKMARX API WORK AS INTENDED
 			// For rules later, fetch description
-			if !descriptionFetched {
+			/*if maxretries == 0 { // Don't spam logfile: only enter the loop if maxretries is positive, and only display the error if it hits 0
+				log.Entry().Error("request failed: maximum number of retries reached, descriptions will no longer be fetched")
+				maxretries = maxretries - 1
+			} else if maxretries > 0 {
 				if sys != nil {
 					apiShortDescription, err := sys.GetShortDescription(scanID, cxxml.Query[i].Result[j].Path.PathID)
 					if err != nil {
+						maxretries = maxretries - 1
+						log.Entry().Debug("request failed: remaining retries ", maxretries)
 						log.Entry().Error(err)
 					} else {
-						descriptionFetched = true
 						apiDescription = apiShortDescription.Text
 					}
+				} else {
+					maxretries = maxretries - 1
+					log.Entry().Debug("request failed: no system instance, remaining retries ", maxretries)
 				}
-			}
+			}*/
 
 			//General
 			result.RuleID = "checkmarx-" + cxxml.Query[i].ID
@@ -186,13 +194,13 @@ func Parse(sys System, data []byte, scanID int) (format.SARIF, error) {
 			result.Level = "none"
 			msg := new(format.Message)
 			//msg.Text = cxxml.Query[i].Name + ": " + cxxml.Query[i].Categories
-			msg.Text = cxxml.Query[i].Name
+			if apiDescription != "" {
+				msg.Text = apiDescription
+			} else {
+				msg.Text = cxxml.Query[i].Name
+			}
 			result.Message = msg
-			//analysisTarget := new(format.ArtifactLocation)
-			//analysisTarget.URI = cxxml.Query[i].Result[j].FileName
-			//analysisTarget.Index = index
 
-			//result.AnalysisTarget = analysisTarget
 			if cxxml.Query[i].Name != "" {
 				msg := new(format.Message)
 				msg.Text = cxxml.Query[i].Name
@@ -200,22 +208,6 @@ func Parse(sys System, data []byte, scanID int) (format.SARIF, error) {
 			//Locations
 			for k := 0; k < len(cxxml.Query[i].Result[j].Path.PathNode); k++ {
 				loc := *new(format.Location)
-				/*index := 0
-				//Check if that artifact has been added
-				added := false
-				for file := 0; file < len(sarif.Runs[0].Artifacts); file++ {
-					if sarif.Runs[0].Artifacts[file].Location.Uri == cxxml.Query[i].Result[j].FileName {
-						added = true
-						index = file
-						break
-					}
-				}
-				if !added {
-					artifact := format.Artifact{Location: format.SarifLocation{Uri: cxxml.Query[i].Result[j].FileName}}
-					sarif.Runs[0].Artifacts = append(sarif.Runs[0].Artifacts, artifact)
-					index = len(sarif.Runs[0].Artifacts) - 1
-				}
-				loc.PhysicalLocation.ArtifactLocation.Index = index */
 				loc.PhysicalLocation.ArtifactLocation.URI = cxxml.Query[i].Result[j].FileName
 				loc.PhysicalLocation.Region.StartLine = cxxml.Query[i].Result[j].Path.PathNode[k].Line
 				loc.PhysicalLocation.Region.EndLine = cxxml.Query[i].Result[j].Path.PathNode[k].Line
@@ -223,9 +215,6 @@ func Parse(sys System, data []byte, scanID int) (format.SARIF, error) {
 				snip := new(format.SnippetSarif)
 				snip.Text = cxxml.Query[i].Result[j].Path.PathNode[k].Snippet.Line.Code
 				loc.PhysicalLocation.Region.Snippet = snip
-				//loc.PhysicalLocation.ContextRegion.StartLine = cxxml.Query[i].Result[j].Path.PathNode[k].Line
-				//loc.PhysicalLocation.ContextRegion.EndLine = cxxml.Query[i].Result[j].Path.PathNode[k].Line
-				//loc.PhysicalLocation.ContextRegion.Snippet = snip
 				result.Locations = append(result.Locations, loc)
 
 				//Related Locations
@@ -309,14 +298,21 @@ func Parse(sys System, data []byte, scanID int) (format.SARIF, error) {
 		rule.Help.Text = rule.HelpURI
 		rule.ShortDescription = new(format.Message)
 		rule.ShortDescription.Text = cxxml.Query[i].Name
-		if apiDescription != "" {
+		rule.Properties = new(format.SarifRuleProperties)
+		/*if apiDescription != "" {
 			rule.FullDescription = new(format.Message)
 			rule.FullDescription.Text = apiDescription
-		} else if cxxml.Query[i].Categories != "" {
+		} else */if cxxml.Query[i].Categories != "" {
 			rule.FullDescription = new(format.Message)
 			rule.FullDescription.Text = cxxml.Query[i].Categories
+
+			//split categories on ;
+			cats := strings.Split(cxxml.Query[i].Categories, ";")
+			for cat := 0; cat < len(cats); cat++ {
+				rule.Properties.Tags = append(rule.Properties.Tags, cats[cat])
+			}
 		}
-		rule.Properties = new(format.SarifRuleProperties)
+
 		if cxxml.Query[i].CweID != "" {
 			rule.Properties.Tags = append(rule.Properties.Tags, "external/cwe/cwe-"+cxxml.Query[i].CweID)
 		}

pkg/checkmarx/cxxml_to_sarif_test.go

@@ -121,7 +121,7 @@ func TestParse(t *testing.T) {
 		assert.Equal(t, len(sarif.Runs[0].Tool.Driver.Rules), 2)
 		assert.Equal(t, sarif.Runs[0].Results[2].Properties.ToolState, "Confirmed")
 		assert.Equal(t, sarif.Runs[0].Results[2].Properties.ToolAuditMessage, "Changed status to Confirmed \n Dummy comment")
-		assert.Equal(t, "This is a dummy short description.", sarif.Runs[0].Tool.Driver.Rules[0].FullDescription.Text)
+		//assert.Equal(t, "This is a dummy short description.", sarif.Runs[0].Tool.Driver.Rules[0].FullDescription.Text)
 	})
 
 	t.Run("Missing sys", func(t *testing.T) {

pkg/format/sarif.go

@@ -103,7 +103,7 @@ type SarifProperties struct {
 // Tool these structs are relevant to the Tool object
 type Tool struct {
 	Driver     Driver   `json:"driver"`
-	Extensions []Driver `json:"extensions"`
+	Extensions []Driver `json:"extensions,omitempty"`
 }
 
 // Driver meta information for the scan and tool context

pkg/fortify/fpr_to_sarif.go

@@ -542,17 +542,21 @@ func Parse(sys System, project *models.Project, projectVersion *models.ProjectVe
 	log.Entry().Debug("Querying Fortify SSC for batch audit data")
 	oneRequestPerIssueMode := false
 	var auditData []*models.ProjectVersionIssue
-	if sys != nil {
+	maxretries := 5 // Maximum number of requests allowed to fail before stopping them
+	if sys != nil && projectVersion != nil {
 		auditData, err = sys.GetAllIssueDetails(projectVersion.ID)
-		if err != nil {
+		if err != nil || len(auditData) == 0 { // It's reasonable to admit that with a length of 0, something went wrong
 			log.Entry().WithError(err).Error("failed to get all audit data, defaulting to one-request-per-issue basis")
 			oneRequestPerIssueMode = true
+			// We do not lower maxretries here in case a "real" bug happened
 		} else {
 			log.Entry().Debug("Request successful, data frame size: ", len(auditData), " audits")
 		}
 	} else {
-		log.Entry().Error("no system instance found, lookup impossible")
+		log.Entry().Error("no system instance or project version found, lookup impossible")
 		oneRequestPerIssueMode = true
+		maxretries = 1 // Set to 1 if the sys instance isn't defined: chances are it couldn't be created, we'll live a chance if there was an unknown bug
+		log.Entry().Debug("request failed: remaining retries ", maxretries)
 	}
 
 	//Now, we handle the sarif
@@ -785,17 +789,12 @@ func Parse(sys System, project *models.Project, projectVersion *models.ProjectVe
 		prop.Confidence = fvdl.Vulnerabilities.Vulnerability[i].InstanceInfo.Confidence
 		prop.InstanceID = fvdl.Vulnerabilities.Vulnerability[i].InstanceInfo.InstanceID
 		//Get the audit data
-		if sys != nil {
-			if err := integrateAuditData(prop, fvdl.Vulnerabilities.Vulnerability[i].InstanceInfo.InstanceID, sys, project, projectVersion, auditData, filterSet, oneRequestPerIssueMode); err != nil {
-				log.Entry().Debug(err)
-				prop.Audited = false
-				prop.ToolState = "Unknown"
-				prop.ToolAuditMessage = "Error fetching audit state"
+		if err := integrateAuditData(prop, fvdl.Vulnerabilities.Vulnerability[i].InstanceInfo.InstanceID, sys, project, projectVersion, auditData, filterSet, oneRequestPerIssueMode, maxretries); err != nil {
+			log.Entry().Debug(err)
+			maxretries = maxretries - 1
+			if maxretries >= 0 {
+				log.Entry().Debug("request failed: remaining retries ", maxretries)
 			}
-		} else {
-			prop.Audited = false
-			prop.ToolState = "Unknown"
-			prop.ToolAuditMessage = "Cannot fetch audit state"
 		}
 		result.Properties = prop
 
@@ -1157,16 +1156,25 @@ func Parse(sys System, project *models.Project, projectVersion *models.ProjectVe
 	return sarif, nil
 }
 
-func integrateAuditData(ruleProp *format.SarifProperties, issueInstanceID string, sys System, project *models.Project, projectVersion *models.ProjectVersion, auditData []*models.ProjectVersionIssue, filterSet *models.FilterSet, oneRequestPerIssue bool) error {
+func integrateAuditData(ruleProp *format.SarifProperties, issueInstanceID string, sys System, project *models.Project, projectVersion *models.ProjectVersion, auditData []*models.ProjectVersionIssue, filterSet *models.FilterSet, oneRequestPerIssue bool, maxretries int) error {
 	// Set default values
 	ruleProp.Audited = false
 	ruleProp.FortifyCategory = "Unknown"
 	ruleProp.ToolSeverity = "Unknown"
-	ruleProp.ToolState = "Unreviewed"
+	ruleProp.ToolState = "Unknown"
+	ruleProp.ToolAuditMessage = "Error fetching audit state" // We set this as default for the error phase, then reset it to nothing
 	ruleProp.ToolSeverityIndex = 0
 	ruleProp.ToolStateIndex = 0
 	// These default values allow for the property bag to be filled even if an error happens later. They all should be overwritten by a normal course of the progrma.
+	if maxretries == 0 {
+		// Max retries reached, we stop there to avoid a longer execution time
+		err := errors.New("request failed: maximum number of retries reached, placeholder values will be set from now on for audit data")
+		return err
+	} else if maxretries < 0 {
+		return nil // Avoid spamming logfile
+	}
 	if sys == nil {
+		ruleProp.ToolAuditMessage = "Cannot fetch audit state: no sys instance"
 		err := errors.New("no system instance, lookup impossible for " + issueInstanceID)
 		return err
 	}
@@ -1174,6 +1182,8 @@ func integrateAuditData(ruleProp *format.SarifProperties, issueInstanceID string
 		err := errors.New("project or projectVersion is undefined: lookup aborted for " + issueInstanceID)
 		return err
 	}
+	// Reset the audit message
+	ruleProp.ToolAuditMessage = ""
 	var data []*models.ProjectVersionIssue
 	var err error
 	if oneRequestPerIssue {

pkg/fortify/fpr_to_sarif_test.go

@@ -385,7 +385,7 @@ If you are concerned about leaking system data via NFC on an Android device, you
 		assert.Equal(t, len(sarif.Runs[0].Results), 2)
 		assert.Equal(t, len(sarif.Runs[0].Tool.Driver.Rules), 1)
 		assert.Equal(t, sarif.Runs[0].Results[0].Properties.ToolState, "Unknown")
-		assert.Equal(t, sarif.Runs[0].Results[0].Properties.ToolAuditMessage, "Cannot fetch audit state")
+		assert.Equal(t, sarif.Runs[0].Results[0].Properties.ToolAuditMessage, "Cannot fetch audit state: no sys instance")
 	})
 }
 
@@ -441,7 +441,7 @@ func TestIntegrateAuditData(t *testing.T) {
 		project := models.Project{}
 		projectVersion := models.ProjectVersion{ID: 11037}
 		auditData, _ := sys.GetAllIssueDetails(projectVersion.ID)
-		err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, &project, &projectVersion, auditData, filterSet, false)
+		err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, &project, &projectVersion, auditData, filterSet, false, 5)
 		assert.NoError(t, err, "error")
 		assert.Equal(t, ruleProp.Audited, true)
 		assert.Equal(t, ruleProp.ToolState, "Exploitable")
@@ -456,7 +456,7 @@ func TestIntegrateAuditData(t *testing.T) {
 		ruleProp := *new(format.SarifProperties)
 		projectVersion := models.ProjectVersion{ID: 11037}
 		auditData, _ := sys.GetAllIssueDetails(projectVersion.ID)
-		err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, nil, &projectVersion, auditData, filterSet, false)
+		err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, nil, &projectVersion, auditData, filterSet, false, 5)
 		assert.Error(t, err, "project or projectVersion is undefined: lookup aborted for 11037")
 	})
 
@@ -464,7 +464,7 @@ func TestIntegrateAuditData(t *testing.T) {
 		ruleProp := *new(format.SarifProperties)
 		project := models.Project{}
 		auditData, _ := sys.GetAllIssueDetails(11037)
-		err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, &project, nil, auditData, filterSet, false)
+		err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, &project, nil, auditData, filterSet, false, 5)
 		assert.Error(t, err, "project or projectVersion is undefined: lookup aborted for 11037")
 	})
 
@@ -473,7 +473,7 @@ func TestIntegrateAuditData(t *testing.T) {
 		project := models.Project{}
 		projectVersion := models.ProjectVersion{ID: 11037}
 		auditData, _ := sys.GetAllIssueDetails(projectVersion.ID)
-		err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", nil, &project, &projectVersion, auditData, filterSet, false)
+		err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", nil, &project, &projectVersion, auditData, filterSet, false, 5)
 		assert.Error(t, err, "no system instance, lookup impossible for DUMMYDUMMYDUMMY")
 	})
 
@@ -482,7 +482,7 @@ func TestIntegrateAuditData(t *testing.T) {
 		project := models.Project{}
 		projectVersion := models.ProjectVersion{ID: 11037}
 		auditData, _ := sys.GetAllIssueDetails(projectVersion.ID)
-		err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, &project, &projectVersion, auditData, nil, false)
+		err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, &project, &projectVersion, auditData, nil, false, 5)
 		assert.Error(t, err, "no filter set defined, category will be missing from 11037")
 	})
 
@@ -490,7 +490,7 @@ func TestIntegrateAuditData(t *testing.T) {
 		ruleProp := *new(format.SarifProperties)
 		project := models.Project{}
 		projectVersion := models.ProjectVersion{ID: 11037}
-		err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, &project, &projectVersion, nil, filterSet, false)
+		err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, &project, &projectVersion, nil, filterSet, false, 5)
 		assert.Error(t, err, "not exactly 1 issue found for instance ID 11037, found 0")
 	})
 
@@ -498,7 +498,7 @@ func TestIntegrateAuditData(t *testing.T) {
 		ruleProp := *new(format.SarifProperties)
 		project := models.Project{}
 		projectVersion := models.ProjectVersion{ID: 11037}
-		err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, &project, &projectVersion, nil, filterSet, true)
+		err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, &project, &projectVersion, nil, filterSet, true, 5)
 		assert.NoError(t, err, "error")
 		assert.Equal(t, ruleProp.Audited, true)
 		assert.Equal(t, ruleProp.ToolState, "Exploitable")
@@ -509,4 +509,21 @@ func TestIntegrateAuditData(t *testing.T) {
 		assert.Equal(t, ruleProp.FortifyCategory, "Audit All")
 	})
 
+	t.Run("Max retries set to 0: error raised", func(t *testing.T) {
+		ruleProp := *new(format.SarifProperties)
+		project := models.Project{}
+		projectVersion := models.ProjectVersion{ID: 11037}
+		auditData, _ := sys.GetAllIssueDetails(11037)
+		err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, &project, &projectVersion, auditData, filterSet, false, 0)
+		assert.Error(t, err, "request failed: maximum number of retries reached, placeholder values will be set from now on for audit data")
+	})
+
+	t.Run("Max retries set to -1: fail silently", func(t *testing.T) {
+		ruleProp := *new(format.SarifProperties)
+		project := models.Project{}
+		projectVersion := models.ProjectVersion{ID: 11037}
+		auditData, _ := sys.GetAllIssueDetails(11037)
+		err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, &project, &projectVersion, auditData, filterSet, false, -1)
+		assert.NoError(t, err)
+	})
 }