From 765031d962eb1308d0b183849fbe5b32212408f8 Mon Sep 17 00:00:00 2001 From: Juan Fernandez Date: Tue, 13 Sep 2022 11:41:52 -0300 Subject: [PATCH] Bug 6717: Duplicate calls on vulnerability third party integrations. (#7698) If the same vulnerability was present in more than one software, then duplicated third party integration calls were being issued. --- ...7-duplicate-calls-third-party-integrations | 2 ++ cmd/fleet/cron_test.go | 2 ++ ee/server/webhooks/mapper.go | 4 +-- ee/server/webhooks/mapper_test.go | 2 +- server/webhooks/mapper.go | 8 +++--- server/webhooks/mapper_test.go | 2 +- server/webhooks/vulnerabilities.go | 14 +++++----- server/webhooks/vulnerabilities_test.go | 7 +++++ server/worker/jira.go | 12 ++++++--- server/worker/jira_test.go | 26 +++++++++++++++++++ server/worker/zendesk.go | 12 ++++++--- server/worker/zendesk_test.go | 26 +++++++++++++++++++ 12 files changed, 95 insertions(+), 22 deletions(-) create mode 100644 changes/bug-6717-duplicate-calls-third-party-integrations diff --git a/changes/bug-6717-duplicate-calls-third-party-integrations b/changes/bug-6717-duplicate-calls-third-party-integrations new file mode 100644 index 0000000000..76b2168851 --- /dev/null +++ b/changes/bug-6717-duplicate-calls-third-party-integrations @@ -0,0 +1,2 @@ +* If the same vulnerability was detected in more than one Software, duplicate calls were being +issued to third party integrations. \ No newline at end of file diff --git a/cmd/fleet/cron_test.go b/cmd/fleet/cron_test.go index 6a956246a6..16b4ab2fa6 100644 --- a/cmd/fleet/cron_test.go +++ b/cmd/fleet/cron_test.go @@ -40,6 +40,7 @@ func TestFilterRecentVulns(t *testing.T) { ovalVulns := []fleet.SoftwareVulnerability{ {CVE: "cve-recent-1"}, {CVE: "cve-recent-2"}, + {CVE: "cve-recent-2"}, {CVE: "cve-outdated-1"}, } @@ -70,6 +71,7 @@ func TestFilterRecentVulns(t *testing.T) { "cve-recent-3": {CVE: "cve-recent-3"}, } + require.Equal(t, len(expected), len(actual)) require.ElementsMatch(t, expected, actual) require.Equal(t, expectedMeta, meta) }) diff --git a/ee/server/webhooks/mapper.go b/ee/server/webhooks/mapper.go index acec402766..d3d79b705e 100644 --- a/ee/server/webhooks/mapper.go +++ b/ee/server/webhooks/mapper.go @@ -18,12 +18,12 @@ func NewMapper() fleetwebhooks.VulnMapper { func (m *Mapper) GetPayload( hostBaseURL *url.URL, hosts []*fleet.HostShort, - vuln fleet.SoftwareVulnerability, + cve string, meta fleet.CVEMeta, ) fleetwebhooks.WebhookPayload { r := m.Mapper.GetPayload(hostBaseURL, hosts, - vuln, + cve, meta, ) r.EPSSProbability = meta.EPSSProbability diff --git a/ee/server/webhooks/mapper_test.go b/ee/server/webhooks/mapper_test.go index 3a5eed5d53..ebd5c0eaa7 100644 --- a/ee/server/webhooks/mapper_test.go +++ b/ee/server/webhooks/mapper_test.go @@ -26,7 +26,7 @@ func TestGetPayload(t *testing.T) { sut := Mapper{} - result := sut.GetPayload(serverURL, nil, vuln, meta) + result := sut.GetPayload(serverURL, nil, vuln.CVE, meta) require.Equal(t, *meta.CISAKnownExploit, *result.CISAKnownExploit) require.Equal(t, *meta.EPSSProbability, *result.EPSSProbability) require.Equal(t, *meta.CVSSScore, *result.CVSSScore) diff --git a/server/webhooks/mapper.go b/server/webhooks/mapper.go index 4af3739ed2..63dee49d1e 100644 --- a/server/webhooks/mapper.go +++ b/server/webhooks/mapper.go @@ -12,7 +12,7 @@ import ( // VulnMapper used for mapping vulnerabilities and their associated data into the payload that // will be sent via thrid party webhooks. type VulnMapper interface { - GetPayload(*url.URL, []*fleet.HostShort, fleet.SoftwareVulnerability, fleet.CVEMeta) WebhookPayload + GetPayload(*url.URL, []*fleet.HostShort, string, fleet.CVEMeta) WebhookPayload } type hostPayloadPart struct { @@ -56,12 +56,12 @@ func (m *Mapper) getHostPayloadPart( func (m *Mapper) GetPayload( hostBaseURL *url.URL, hosts []*fleet.HostShort, - vuln fleet.SoftwareVulnerability, + cve string, meta fleet.CVEMeta, ) WebhookPayload { return WebhookPayload{ - CVE: vuln.CVE, - Link: fmt.Sprintf("https://nvd.nist.gov/vuln/detail/%s", vuln.CVE), + CVE: cve, + Link: fmt.Sprintf("https://nvd.nist.gov/vuln/detail/%s", cve), Hosts: m.getHostPayloadPart(hostBaseURL, hosts), } } diff --git a/server/webhooks/mapper_test.go b/server/webhooks/mapper_test.go index 759680ecd8..325180d088 100644 --- a/server/webhooks/mapper_test.go +++ b/server/webhooks/mapper_test.go @@ -26,7 +26,7 @@ func TestGetPaylaod(t *testing.T) { sut := Mapper{} - result := sut.GetPayload(serverURL, nil, vuln, meta) + result := sut.GetPayload(serverURL, nil, vuln.CVE, meta) require.Empty(t, result.CISAKnownExploit) require.Empty(t, result.EPSSProbability) require.Empty(t, result.CVSSScore) diff --git a/server/webhooks/vulnerabilities.go b/server/webhooks/vulnerabilities.go index 25e829cbc3..8a7f59ad70 100644 --- a/server/webhooks/vulnerabilities.go +++ b/server/webhooks/vulnerabilities.go @@ -36,17 +36,15 @@ func TriggerVulnerabilitiesWebhook( targetURL := vulnConfig.DestinationURL batchSize := vulnConfig.HostBatchSize - softwareIDsGroupedByCVE := make(map[string][]uint) + groups := make(map[string][]uint) for _, v := range args.Vulnerablities { - softwareIDsGroupedByCVE[v.CVE] = append(softwareIDsGroupedByCVE[v.CVE], v.SoftwareID) + groups[v.CVE] = append(groups[v.CVE], v.SoftwareID) } - for _, v := range args.Vulnerablities { - softwareIDs := softwareIDsGroupedByCVE[v.CVE] - - hosts, err := ds.HostsBySoftwareIDs(ctx, softwareIDs) + for cve, sIDs := range groups { + hosts, err := ds.HostsBySoftwareIDs(ctx, sIDs) if err != nil { - return ctxerr.Wrap(ctx, err, "get hosts by CPE") + return ctxerr.Wrap(ctx, err, "get hosts by software ids") } for len(hosts) > 0 { @@ -54,7 +52,7 @@ func TriggerVulnerabilitiesWebhook( if batchSize > 0 && len(hosts) > batchSize { limit = batchSize } - payload := mapper.GetPayload(serverURL, hosts[:limit], v, args.Meta[v.CVE]) + payload := mapper.GetPayload(serverURL, hosts[:limit], cve, args.Meta[cve]) if err := sendVulnerabilityHostBatch(ctx, targetURL, payload, args.Time); err != nil { return ctxerr.Wrap(ctx, err, "send vulnerability host batch") } diff --git a/server/webhooks/vulnerabilities_test.go b/server/webhooks/vulnerabilities_test.go index 4166e75910..b79dc98601 100644 --- a/server/webhooks/vulnerabilities_test.go +++ b/server/webhooks/vulnerabilities_test.go @@ -115,6 +115,13 @@ func TestTriggerVulnerabilitiesWebhook(t *testing.T) { hosts[:1], fmt.Sprintf("%s[%s]}}", jsonCVE1, jsonH1), }, + { + "1 vuln in multiple software, 1 host", + []fleet.SoftwareVulnerability{{CVE: cves[0], SoftwareID: 1}, {CVE: cves[0], SoftwareID: 1}, {CVE: cves[0], SoftwareID: 2}}, + nil, + hosts[:1], + fmt.Sprintf("%s[%s]}}", jsonCVE1, jsonH1), + }, { "1 vuln, 2 hosts", []fleet.SoftwareVulnerability{{CVE: cves[0], SoftwareID: 1}}, diff --git a/server/worker/jira.go b/server/worker/jira.go index d99918df69..456e3f967b 100644 --- a/server/worker/jira.go +++ b/server/worker/jira.go @@ -337,8 +337,13 @@ func QueueJiraVulnJobs(ctx context.Context, ds fleet.Datastore, logger kitlog.Lo sort.Strings(cves) level.Debug(logger).Log("recent_cves", fmt.Sprintf("%v", cves)) - for _, vuln := range recentVulns { - job, err := QueueJob(ctx, ds, jiraName, jiraArgs{CVE: vuln.CVE}) + uniqCVEs := make(map[string]bool) + for _, v := range recentVulns { + uniqCVEs[v.CVE] = true + } + + for cve := range uniqCVEs { + job, err := QueueJob(ctx, ds, jiraName, jiraArgs{CVE: cve}) if err != nil { return ctxerr.Wrap(ctx, err, "queueing job") } @@ -350,7 +355,8 @@ func QueueJiraVulnJobs(ctx context.Context, ds fleet.Datastore, logger kitlog.Lo // QueueJiraFailingPolicyJob queues a Jira job for a failing policy to process // asynchronously via the worker. func QueueJiraFailingPolicyJob(ctx context.Context, ds fleet.Datastore, logger kitlog.Logger, - policy *fleet.Policy, hosts []fleet.PolicySetHost) error { + policy *fleet.Policy, hosts []fleet.PolicySetHost, +) error { attrs := []interface{}{ "enabled", "true", "failing_policy", policy.ID, diff --git a/server/worker/jira_test.go b/server/worker/jira_test.go index f73e817b81..7269e6ef82 100644 --- a/server/worker/jira_test.go +++ b/server/worker/jira_test.go @@ -134,6 +134,32 @@ func TestJiraQueueVulnJobs(t *testing.T) { ctx := context.Background() logger := kitlog.NewNopLogger() + t.Run("same vulnerability on multiple software only queue one job", func(t *testing.T) { + var count int + ds.NewJobFunc = func(ctx context.Context, job *fleet.Job) (*fleet.Job, error) { + count++ + return job, nil + } + vulns := []fleet.SoftwareVulnerability{{ + CVE: "CVE-1234-5678", + SoftwareID: 1, + }, { + CVE: "CVE-1234-5678", + SoftwareID: 2, + }, { + CVE: "CVE-1234-5678", + SoftwareID: 2, + }, { + CVE: "CVE-1234-5678", + SoftwareID: 3, + }} + + err := QueueJiraVulnJobs(ctx, ds, logger, vulns) + require.NoError(t, err) + require.True(t, ds.NewJobFuncInvoked) + require.Equal(t, 1, count) + }) + t.Run("success", func(t *testing.T) { ds.NewJobFunc = func(ctx context.Context, job *fleet.Job) (*fleet.Job, error) { return job, nil diff --git a/server/worker/zendesk.go b/server/worker/zendesk.go index 50c2a12670..b8c752c521 100644 --- a/server/worker/zendesk.go +++ b/server/worker/zendesk.go @@ -330,8 +330,13 @@ func QueueZendeskVulnJobs(ctx context.Context, ds fleet.Datastore, logger kitlog sort.Strings(cves) level.Debug(logger).Log("recent_cves", fmt.Sprintf("%v", cves)) - for _, vuln := range recentVulns { - job, err := QueueJob(ctx, ds, zendeskName, zendeskArgs{CVE: vuln.CVE}) + uniqCVEs := make(map[string]bool) + for _, v := range recentVulns { + uniqCVEs[v.CVE] = true + } + + for cve := range uniqCVEs { + job, err := QueueJob(ctx, ds, zendeskName, zendeskArgs{CVE: cve}) if err != nil { return ctxerr.Wrap(ctx, err, "queueing job") } @@ -343,7 +348,8 @@ func QueueZendeskVulnJobs(ctx context.Context, ds fleet.Datastore, logger kitlog // QueueZendeskFailingPolicyJob queues a Zendesk job for a failing policy to // process asynchronously via the worker. func QueueZendeskFailingPolicyJob(ctx context.Context, ds fleet.Datastore, logger kitlog.Logger, - policy *fleet.Policy, hosts []fleet.PolicySetHost) error { + policy *fleet.Policy, hosts []fleet.PolicySetHost, +) error { attrs := []interface{}{ "enabled", "true", "failing_policy", policy.ID, diff --git a/server/worker/zendesk_test.go b/server/worker/zendesk_test.go index 20e7de33f9..849750bc34 100644 --- a/server/worker/zendesk_test.go +++ b/server/worker/zendesk_test.go @@ -122,6 +122,32 @@ func TestZendeskQueueVulnJobs(t *testing.T) { ctx := context.Background() logger := kitlog.NewNopLogger() + t.Run("same vulnerability on multiple software only queue one job", func(t *testing.T) { + var count int + ds.NewJobFunc = func(ctx context.Context, job *fleet.Job) (*fleet.Job, error) { + count++ + return job, nil + } + vulns := []fleet.SoftwareVulnerability{{ + CVE: "CVE-1234-5678", + SoftwareID: 1, + }, { + CVE: "CVE-1234-5678", + SoftwareID: 2, + }, { + CVE: "CVE-1234-5678", + SoftwareID: 2, + }, { + CVE: "CVE-1234-5678", + SoftwareID: 3, + }} + + err := QueueZendeskVulnJobs(ctx, ds, logger, vulns) + require.NoError(t, err) + require.True(t, ds.NewJobFuncInvoked) + require.Equal(t, 1, count) + }) + t.Run("success", func(t *testing.T) { ds.NewJobFunc = func(ctx context.Context, job *fleet.Job) (*fleet.Job, error) { return job, nil