From 96fabb4072edd758fc4bc23e02d54d9b637e0294 Mon Sep 17 00:00:00 2001 From: JoaoCxMartins Date: Thu, 4 Jan 2024 15:54:04 +0000 Subject: [PATCH 1/7] add a timeout to decode results --- pkg/engine/inspector.go | 89 +++++++++++++++++++++++------------------ pkg/remediation/scan.go | 4 +- 2 files changed, 52 insertions(+), 41 deletions(-) diff --git a/pkg/engine/inspector.go b/pkg/engine/inspector.go index f5cb39c3171..7b4ec331684 100644 --- a/pkg/engine/inspector.go +++ b/pkg/engine/inspector.go @@ -212,6 +212,7 @@ func (c *Inspector) Inspect( } queries := c.getQueriesByPlat(platforms) + for i, queryMeta := range queries { currentQuery <- 1 @@ -259,8 +260,8 @@ func (c *Inspector) Inspect( vulnerabilities = append(vulnerabilities, vuls...) c.tracker.TrackQueryExecution(query.Metadata.Aggregation) - } + } return vulnerabilities, nil } @@ -343,11 +344,13 @@ func (c *Inspector) doRun(ctx *QueryContext) (vulns []model.Vulnerability, err e Str("scanID", ctx.scanID). Msgf("Inspector executed with result %+v, query=%s", results, ctx.Query.Metadata.Query) - return c.DecodeQueryResults(ctx, results) + timeoutCtxToDecode, cancelDecode := context.WithTimeout(ctx.Ctx, c.queryExecTimeout) + defer cancelDecode() + return c.DecodeQueryResults(ctx, timeoutCtxToDecode, results) } // DecodeQueryResults decodes the results into []model.Vulnerability -func (c *Inspector) DecodeQueryResults(ctx *QueryContext, results rego.ResultSet) ([]model.Vulnerability, error) { +func (c *Inspector) DecodeQueryResults(ctx *QueryContext, ctxTimeout context.Context, results rego.ResultSet) ([]model.Vulnerability, error) { if len(results) == 0 { return nil, ErrNoResult } @@ -367,48 +370,54 @@ func (c *Inspector) DecodeQueryResults(ctx *QueryContext, results rego.ResultSet vulnerabilities := make([]model.Vulnerability, 0, len(queryResultItems)) failedDetectLine := false for _, queryResultItem := range queryResultItems { - vulnerability, err := c.vb(ctx, c.tracker, queryResultItem, c.detector) - if err != nil && err.Error() == ErrNoResult.Error() { - // Ignoring bad results - continue - } - if err != nil { - sentryReport.ReportSentry(&sentryReport.Report{ - Message: fmt.Sprintf("Inspector can't save vulnerability, query=%s", ctx.Query.Metadata.Query), - Err: err, - Location: "func decodeQueryResults()", - Platform: ctx.Query.Metadata.Platform, - Metadata: ctx.Query.Metadata.Metadata, - Query: ctx.Query.Metadata.Query, - }, true) - - if _, ok := c.failedQueries[ctx.Query.Metadata.Query]; !ok { - c.failedQueries[ctx.Query.Metadata.Query] = err + select { + case <-ctxTimeout.Done(): + log.Err(ctxTimeout.Err()).Msgf("Timeout processing the results of the query: %s %s", ctx.Query.Metadata.Platform, ctx.Query.Metadata.Query) + break + default: + vulnerability, err := c.vb(ctx, c.tracker, queryResultItem, c.detector) + if err != nil && err.Error() == ErrNoResult.Error() { + // Ignoring bad results + continue + } + if err != nil { + sentryReport.ReportSentry(&sentryReport.Report{ + Message: fmt.Sprintf("Inspector can't save vulnerability, query=%s", ctx.Query.Metadata.Query), + Err: err, + Location: "func decodeQueryResults()", + Platform: ctx.Query.Metadata.Platform, + Metadata: ctx.Query.Metadata.Metadata, + Query: ctx.Query.Metadata.Query, + }, true) + + if _, ok := c.failedQueries[ctx.Query.Metadata.Query]; !ok { + c.failedQueries[ctx.Query.Metadata.Query] = err + } + + continue + } + file := ctx.Files[vulnerability.FileID] + if ShouldSkipVulnerability(file.Commands, vulnerability.QueryID) { + log.Debug().Msgf("Skipping vulnerability in file %s for query '%s':%s", file.FilePath, vulnerability.QueryName, vulnerability.QueryID) + continue } - continue - } - file := ctx.Files[vulnerability.FileID] - if ShouldSkipVulnerability(file.Commands, vulnerability.QueryID) { - log.Debug().Msgf("Skipping vulnerability in file %s for query '%s':%s", file.FilePath, vulnerability.QueryName, vulnerability.QueryID) - continue - } + if vulnerability.Line == UndetectedVulnerabilityLine { + failedDetectLine = true + } - if vulnerability.Line == UndetectedVulnerabilityLine { - failedDetectLine = true - } + if _, ok := c.excludeResults[vulnerability.SimilarityID]; ok { + log.Debug(). + Msgf("Excluding result SimilarityID: %s", vulnerability.SimilarityID) + continue + } else if checkComment(vulnerability.Line, file.LinesIgnore) { + log.Debug(). + Msgf("Excluding result Comment: %s", vulnerability.SimilarityID) + continue + } - if _, ok := c.excludeResults[vulnerability.SimilarityID]; ok { - log.Debug(). - Msgf("Excluding result SimilarityID: %s", vulnerability.SimilarityID) - continue - } else if checkComment(vulnerability.Line, file.LinesIgnore) { - log.Debug(). - Msgf("Excluding result Comment: %s", vulnerability.SimilarityID) - continue + vulnerabilities = append(vulnerabilities, *vulnerability) } - - vulnerabilities = append(vulnerabilities, *vulnerability) } if failedDetectLine { diff --git a/pkg/remediation/scan.go b/pkg/remediation/scan.go index afa81532c7d..21506a30cdc 100644 --- a/pkg/remediation/scan.go +++ b/pkg/remediation/scan.go @@ -174,7 +174,9 @@ func runQuery(r *runQueryInfo) []model.Vulnerability { Files: r.files.ToMap(), } - decoded, err := r.inspector.DecodeQueryResults(queryCtx, results) + timeoutCtxToDecode, cancelDecode := context.WithTimeout(context.Background(), queryExecTimeout) + defer cancelDecode() + decoded, err := r.inspector.DecodeQueryResults(queryCtx, timeoutCtxToDecode, results) if err != nil { log.Err(err) From de34b693d3b072fa742e30ad733349c43748a127 Mon Sep 17 00:00:00 2001 From: JoaoCxMartins Date: Thu, 4 Jan 2024 16:13:38 +0000 Subject: [PATCH 2/7] wip --- pkg/engine/inspector.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/pkg/engine/inspector.go b/pkg/engine/inspector.go index 7b4ec331684..ee48460107c 100644 --- a/pkg/engine/inspector.go +++ b/pkg/engine/inspector.go @@ -260,7 +260,6 @@ func (c *Inspector) Inspect( vulnerabilities = append(vulnerabilities, vuls...) c.tracker.TrackQueryExecution(query.Metadata.Aggregation) - } return vulnerabilities, nil } @@ -350,7 +349,10 @@ func (c *Inspector) doRun(ctx *QueryContext) (vulns []model.Vulnerability, err e } // DecodeQueryResults decodes the results into []model.Vulnerability -func (c *Inspector) DecodeQueryResults(ctx *QueryContext, ctxTimeout context.Context, results rego.ResultSet) ([]model.Vulnerability, error) { +func (c *Inspector) DecodeQueryResults( + ctx *QueryContext, + ctxTimeout context.Context, + results rego.ResultSet) ([]model.Vulnerability, error) { if len(results) == 0 { return nil, ErrNoResult } @@ -372,7 +374,10 @@ func (c *Inspector) DecodeQueryResults(ctx *QueryContext, ctxTimeout context.Con for _, queryResultItem := range queryResultItems { select { case <-ctxTimeout.Done(): - log.Err(ctxTimeout.Err()).Msgf("Timeout processing the results of the query: %s %s", ctx.Query.Metadata.Platform, ctx.Query.Metadata.Query) + log.Err(ctxTimeout.Err()).Msgf( + "Timeout processing the results of the query: %s %s", + ctx.Query.Metadata.Platform, + ctx.Query.Metadata.Query) break default: vulnerability, err := c.vb(ctx, c.tracker, queryResultItem, c.detector) From 2f87df5343965848947d53bd4e9c1eac4851c256 Mon Sep 17 00:00:00 2001 From: JoaoCxMartins Date: Tue, 16 Jan 2024 12:08:50 +0000 Subject: [PATCH 3/7] improve logs --- pkg/engine/inspector.go | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/pkg/engine/inspector.go b/pkg/engine/inspector.go index ee48460107c..3be3c66057f 100644 --- a/pkg/engine/inspector.go +++ b/pkg/engine/inspector.go @@ -241,6 +241,7 @@ func (c *Inspector) Inspect( vuls, err := c.doRun(queryContext) if err != nil { + fmt.Println() sentryReport.ReportSentry(&sentryReport.Report{ Message: fmt.Sprintf("Inspector. query executed with error, query=%s", query.Metadata.Query), Err: err, @@ -371,13 +372,11 @@ func (c *Inspector) DecodeQueryResults( vulnerabilities := make([]model.Vulnerability, 0, len(queryResultItems)) failedDetectLine := false + timeOut := false for _, queryResultItem := range queryResultItems { select { case <-ctxTimeout.Done(): - log.Err(ctxTimeout.Err()).Msgf( - "Timeout processing the results of the query: %s %s", - ctx.Query.Metadata.Platform, - ctx.Query.Metadata.Query) + timeOut = true break default: vulnerability, err := c.vb(ctx, c.tracker, queryResultItem, c.detector) @@ -425,6 +424,14 @@ func (c *Inspector) DecodeQueryResults( } } + if timeOut { + fmt.Println() + log.Err(ctxTimeout.Err()).Msgf( + "Timeout processing the results of the query: %s %s", + ctx.Query.Metadata.Platform, + ctx.Query.Metadata.Query) + } + if failedDetectLine { c.tracker.FailedDetectLine() } From 62f9d03161b1960f1cda41a7539378c314b59424 Mon Sep 17 00:00:00 2001 From: JoaoCxMartins Date: Tue, 16 Jan 2024 13:58:02 +0000 Subject: [PATCH 4/7] reduce cyclomatic --- pkg/engine/inspector.go | 90 +++++++++++++++++++++++------------------ 1 file changed, 50 insertions(+), 40 deletions(-) diff --git a/pkg/engine/inspector.go b/pkg/engine/inspector.go index 3be3c66057f..2954f9a7c5d 100644 --- a/pkg/engine/inspector.go +++ b/pkg/engine/inspector.go @@ -379,48 +379,13 @@ func (c *Inspector) DecodeQueryResults( timeOut = true break default: - vulnerability, err := c.vb(ctx, c.tracker, queryResultItem, c.detector) - if err != nil && err.Error() == ErrNoResult.Error() { - // Ignoring bad results - continue + vulnerability, aux := getVulnerabilitiesFromQuery(ctx, c, queryResultItem) + if aux { + failedDetectLine = aux } - if err != nil { - sentryReport.ReportSentry(&sentryReport.Report{ - Message: fmt.Sprintf("Inspector can't save vulnerability, query=%s", ctx.Query.Metadata.Query), - Err: err, - Location: "func decodeQueryResults()", - Platform: ctx.Query.Metadata.Platform, - Metadata: ctx.Query.Metadata.Metadata, - Query: ctx.Query.Metadata.Query, - }, true) - - if _, ok := c.failedQueries[ctx.Query.Metadata.Query]; !ok { - c.failedQueries[ctx.Query.Metadata.Query] = err - } - - continue + if vulnerability != nil && aux == false { + vulnerabilities = append(vulnerabilities, *vulnerability) } - file := ctx.Files[vulnerability.FileID] - if ShouldSkipVulnerability(file.Commands, vulnerability.QueryID) { - log.Debug().Msgf("Skipping vulnerability in file %s for query '%s':%s", file.FilePath, vulnerability.QueryName, vulnerability.QueryID) - continue - } - - if vulnerability.Line == UndetectedVulnerabilityLine { - failedDetectLine = true - } - - if _, ok := c.excludeResults[vulnerability.SimilarityID]; ok { - log.Debug(). - Msgf("Excluding result SimilarityID: %s", vulnerability.SimilarityID) - continue - } else if checkComment(vulnerability.Line, file.LinesIgnore) { - log.Debug(). - Msgf("Excluding result Comment: %s", vulnerability.SimilarityID) - continue - } - - vulnerabilities = append(vulnerabilities, *vulnerability) } } @@ -439,6 +404,51 @@ func (c *Inspector) DecodeQueryResults( return vulnerabilities, nil } +func getVulnerabilitiesFromQuery(ctx *QueryContext, c *Inspector, queryResultItem interface{}) (*model.Vulnerability, bool) { + vulnerability, err := c.vb(ctx, c.tracker, queryResultItem, c.detector) + if err != nil && err.Error() == ErrNoResult.Error() { + // Ignoring bad results + return nil, false + } + if err != nil { + sentryReport.ReportSentry(&sentryReport.Report{ + Message: fmt.Sprintf("Inspector can't save vulnerability, query=%s", ctx.Query.Metadata.Query), + Err: err, + Location: "func decodeQueryResults()", + Platform: ctx.Query.Metadata.Platform, + Metadata: ctx.Query.Metadata.Metadata, + Query: ctx.Query.Metadata.Query, + }, true) + + if _, ok := c.failedQueries[ctx.Query.Metadata.Query]; !ok { + c.failedQueries[ctx.Query.Metadata.Query] = err + } + + return nil, false + } + file := ctx.Files[vulnerability.FileID] + if ShouldSkipVulnerability(file.Commands, vulnerability.QueryID) { + log.Debug().Msgf("Skipping vulnerability in file %s for query '%s':%s", file.FilePath, vulnerability.QueryName, vulnerability.QueryID) + return nil, false + } + + if vulnerability.Line == UndetectedVulnerabilityLine { + return nil, true + } + + if _, ok := c.excludeResults[vulnerability.SimilarityID]; ok { + log.Debug(). + Msgf("Excluding result SimilarityID: %s", vulnerability.SimilarityID) + return nil, false + } else if checkComment(vulnerability.Line, file.LinesIgnore) { + log.Debug(). + Msgf("Excluding result Comment: %s", vulnerability.SimilarityID) + return nil, false + } + + return vulnerability, false +} + // checkComment checks if the vulnerability should be skipped from comment func checkComment(line int, ignoreLines []int) bool { for _, ignoreLine := range ignoreLines { From d6fd490858693a2e8e2a08be04f917fbdde03d04 Mon Sep 17 00:00:00 2001 From: JoaoCxMartins Date: Tue, 16 Jan 2024 15:41:22 +0000 Subject: [PATCH 5/7] simpler --- pkg/engine/inspector.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/engine/inspector.go b/pkg/engine/inspector.go index 2954f9a7c5d..bf4ecb179a3 100644 --- a/pkg/engine/inspector.go +++ b/pkg/engine/inspector.go @@ -383,7 +383,7 @@ func (c *Inspector) DecodeQueryResults( if aux { failedDetectLine = aux } - if vulnerability != nil && aux == false { + if vulnerability != nil && !aux { vulnerabilities = append(vulnerabilities, *vulnerability) } } From 0836018ff9900433c2a7b490a9ac1fb9cc74a3f4 Mon Sep 17 00:00:00 2001 From: JoaoCxMartins Date: Wed, 17 Jan 2024 16:49:07 +0000 Subject: [PATCH 6/7] unit test to test the behaviour on time out --- pkg/engine/inspector_test.go | 61 ++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/pkg/engine/inspector_test.go b/pkg/engine/inspector_test.go index 87e9099700b..4382e108748 100644 --- a/pkg/engine/inspector_test.go +++ b/pkg/engine/inspector_test.go @@ -3,6 +3,8 @@ package engine import ( "context" "fmt" + "github.com/open-policy-agent/opa/rego" + "github.com/stretchr/testify/assert" "io" "os" "path/filepath" @@ -677,6 +679,65 @@ func TestShouldSkipFile(t *testing.T) { } } +func TestInspector_DecodeQueryResults_ShouldNotFail_WhenTimeout(t *testing.T) { + //build inspector + c := newInspectorInstance(t, []string{ + filepath.FromSlash("../../assets/queries/terraform/aws/alb_deletion_protection_disabled"), + }) + + //context + myContext := context.Background() + + //build result set + myResultSet := newResultset() + + //query context + myQueryContext := newQueryContext(myContext) + + //create a context with 0 second to timeout + timeoutDuration, _ := time.ParseDuration("0s") + myCtxTimeOut, _ := context.WithTimeout(myContext, timeoutDuration) + + //call method + result, erro := c.DecodeQueryResults(&myQueryContext, myCtxTimeOut, myResultSet) + assert.Nil(t, erro, "Error not as expected") + assert.Equal(t, 0, len(result), "Array size is not as expected") +} + +func newResultset() rego.ResultSet { + myValue := make(map[string]interface{}) + myValue["documentId"] = "3a3be8f7-896e-4ef8-9db3-d6c19e60510b" + myValue["issueType"] = "IncorrectValue" + myValue["keyActualValue"] = "COPY --from referencesthe current FROM alias" + myValue["keyExpectedValue"] = "COPY --from should not references the current FROM alias" + myValue["searchKey"] = "{{ADD ${JAR_FILE} app.jar}}" + + myBinding := make([]interface{}, 1) + myBinding[0] = myValue + + myresult := rego.Result{ + Bindings: map[string]interface{}{ + "result": myBinding, + }, + } + myResultSet := rego.ResultSet{myresult} + return myResultSet +} + +func newQueryContext(ctx context.Context) QueryContext { + queryMetadata := model.QueryMetadata{ + Platform: "myPlatform", + Query: "myQuery"} + myQuery := PreparedQuery{ + Metadata: queryMetadata, + } + queryContext := QueryContext{ + Ctx: ctx, + Query: &myQuery, + } + return queryContext +} + func newInspectorInstance(t *testing.T, queryPath []string) *Inspector { querySource := source.NewFilesystemSource(queryPath, []string{""}, []string{""}, filepath.FromSlash("./assets/libraries"), true) var vb = func(ctx *QueryContext, tracker Tracker, v interface{}, From 9711f88bf3c5ebb505417b7d7a83d8e3a6610d80 Mon Sep 17 00:00:00 2001 From: JoaoCxMartins Date: Wed, 17 Jan 2024 21:47:57 +0000 Subject: [PATCH 7/7] improve test --- pkg/engine/inspector_test.go | 52 +++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 18 deletions(-) diff --git a/pkg/engine/inspector_test.go b/pkg/engine/inspector_test.go index 4382e108748..fe41bc20626 100644 --- a/pkg/engine/inspector_test.go +++ b/pkg/engine/inspector_test.go @@ -679,29 +679,45 @@ func TestShouldSkipFile(t *testing.T) { } } -func TestInspector_DecodeQueryResults_ShouldNotFail_WhenTimeout(t *testing.T) { - //build inspector - c := newInspectorInstance(t, []string{ - filepath.FromSlash("../../assets/queries/terraform/aws/alb_deletion_protection_disabled"), - }) +func TestInspector_DecodeQueryResults(t *testing.T) { //context - myContext := context.Background() - - //build result set - myResultSet := newResultset() + contextToUSe := context.Background() - //query context - myQueryContext := newQueryContext(myContext) + //build inspector + c := newInspectorInstance(t, []string{}) - //create a context with 0 second to timeout - timeoutDuration, _ := time.ParseDuration("0s") - myCtxTimeOut, _ := context.WithTimeout(myContext, timeoutDuration) + type args struct { + queryContext QueryContext + regoResult rego.ResultSet + timeDuration string + } + tests := []struct { + name string + args args + expected int + }{ + { + name: "should_not_fail_when_timeout", + args: args{ + queryContext: newQueryContext(contextToUSe), + regoResult: newResultset(), + timeDuration: "0s", + }, + expected: 0, + }, + } - //call method - result, erro := c.DecodeQueryResults(&myQueryContext, myCtxTimeOut, myResultSet) - assert.Nil(t, erro, "Error not as expected") - assert.Equal(t, 0, len(result), "Array size is not as expected") + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + //create a context with 0 second to timeout + timeoutDuration, _ := time.ParseDuration(tt.args.timeDuration) + myCtxTimeOut, _ := context.WithTimeout(contextToUSe, timeoutDuration) + result, err := c.DecodeQueryResults(&tt.args.queryContext, myCtxTimeOut, tt.args.regoResult) + assert.Nil(t, err, "Error not as expected") + assert.Equal(t, 0, len(result), "Array size is not as expected") + }) + } } func newResultset() rego.ResultSet {