PR#402: Find out which pkgs in the advisory fix the CVE - freshmaker

freshmaker

#402 Find out which pkgs in the advisory fix the CVE

Merged 4 years ago by jkaluza. Opened 4 years ago by gnaponie.

gnaponie/freshmaker pkgs-fix-cve into master

Find out which pkgs in the advisory fix the CVE

gnaponie • 4 years ago

54b34cc

freshmaker/bugzilla.py

file modified

+19 -7

		`@@ -83,17 +83,21 @@`
		`# Convert the whiteboard to a dict, and return`
		`return dict(entry.split('=', 1) for entry in whiteboard.split(','))`

		`- def get_highest_impact(self, cve_list):`
		`+ def fetch_cve_metadata(self, cve_list):`
		`"""`
		- Fetches metadata about each CVE in `cve_list` and returns the name of
		- highest severity rate. See `BugzillaAPI.THREAT_SEVERITIES` for
		`- list of possible severity rates.`
		+ Fetches metadata about each CVE in `cve_list` and returns a tuple with
		`+ the name of highest severity rate and the affected packages (a dictionary`
		`+ with product and pkg_name).`
		+ See `BugzillaAPI.THREAT_SEVERITIES` for list of possible severity rates.

		`:param list cve_list: List of strings with CVE names.`
		`:rtype: str`
		- :return: Name of highest severity rate occuring in CVEs from `cve_list`.
		`+ :return: Tuple, the first element is the name of highest severity rate occuring`
		+ in CVEs from `cve_list`. The second element is a list of dicts, with "product"
		`+ and "pkg_name" of the affected packages.`
		`"""`
		`max_rating = -1`
		`+ affected_pkgs = []`
		`for cve in cve_list:`
		`try:`
		`data = self._get_cve_whiteboard(cve)`
		`@@ -119,6 +123,14 @@`
		`continue`

		`try:`
		`+ affected_pkgs.extend([`
		`+ {'product': pkg.split('/')[0], 'pkg_name': pkg.split('/')[-1]}`
		`+ for pkg, isaffected in data.items() if isaffected == 'affected'])`
		`+ except IndexError:`
		`+ log.warning("CVE %s has no affected packages in bugzilla whiteboard", cve)`
		`+ continue`
		`+`
		`+ try:`
		`rating = BugzillaAPI.THREAT_SEVERITIES.index(severity)`
		`except ValueError:`
		`log.error("Unknown threat_severity '%s' for CVE %s",`
		`@@ -128,5 +140,5 @@`
		`max_rating = max(max_rating, rating)`

		`if max_rating == -1:`
		`- return None`
		`- return BugzillaAPI.THREAT_SEVERITIES[max_rating]`
		`+ return (None, affected_pkgs)`
		`+ return (BugzillaAPI.THREAT_SEVERITIES[max_rating], affected_pkgs)`

freshmaker/errata.py

file modified

+2 -1

		`@@ -57,7 +57,8 @@`
		`self.has_hightouch_bug = has_hightouch_bug`

		`bugzilla = BugzillaAPI()`
		`- self.highest_cve_severity = bugzilla.get_highest_impact(self.cve_list)`
		`+`
		`+ self.highest_cve_severity, self.affected_pkgs = bugzilla.fetch_cve_metadata(self.cve_list)`
lucarval commented 4 years ago I'm missing where `affected_pkgs` is actually used.
gnaponie commented 4 years ago It will be used in the next PR related to FACTORY-3527. The card is supposed to have 2 PRs. edit: ups. Sorry I saw Jan's comment after posting this one. Edited 4 years ago by gnaponie

		`@classmethod`
		`def from_advisory_id(cls, errata, errata_id):`

tests/test_bugzilla.py

file modified

+38 -18

		`@@ -45,6 +45,13 @@`
		`<status_whiteboard></status_whiteboard>`
		`</bug></bugzilla>`
		`"""`
		`+`
		`+ xml_with_affected_pkgs = """`
		`+ <bugzilla><bug>`
		`+ <status_whiteboard>impact={impact},{packages}</status_whiteboard>`
		`+ </bug></bugzilla>`
		`+ """`
		`+`
		`xml_without_status = """<bugzilla><bug></bug></bugzilla>"""`
		`xml_with_empty_bug = """<bugzilla></bugzilla>"""`

		`@@ -52,50 +59,63 @@`
		`class TestBugzillaAPI(helpers.FreshmakerTestCase):`

		`@patch("freshmaker.bugzilla.requests.get")`
		`- def test_get_highest_impact(self, requests_get):`
		`+ def test_fetch_cve_metadata(self, requests_get):`
		`impacts = ["Low", "Moderate", "Important", "Critical"]`
		`bugzilla = BugzillaAPI()`
		`for num_of_cves in range(1, 4):`
		`requests_get.side_effect = [`
		`MockResponse(xml_with_status.format(impact=impact))`
		`for impact in impacts]`
		`- ret = bugzilla.get_highest_impact(["CVE-1"] * num_of_cves)`
		`- self.assertEqual(ret, impacts[num_of_cves - 1].lower())`
		`+ highest_cve_severity, _ = bugzilla.fetch_cve_metadata(["CVE-1"] * num_of_cves)`
		`+ self.assertEqual(highest_cve_severity, impacts[num_of_cves - 1].lower())`

		`@patch("freshmaker.bugzilla.requests.get")`
		`- def test_get_highest_impact_empty_list(self, requests_get):`
		`+ def test_fetch_cve_metadata_empty_list(self, requests_get):`
		`bugzilla = BugzillaAPI()`
		`- ret = bugzilla.get_highest_impact([])`
		`- self.assertEqual(ret, None)`
		`+ highest_cve_severity, _ = bugzilla.fetch_cve_metadata([])`
		`+ self.assertEqual(highest_cve_severity, None)`
		`requests_get.assert_not_called()`

		`@patch("freshmaker.bugzilla.requests.get")`
		`- def test_get_highest_impact_no_status(self, requests_get):`
		`+ def test_fetch_cve_metadata_no_status(self, requests_get):`
		`bugzilla = BugzillaAPI()`
		`requests_get.return_value = MockResponse(xml_without_status)`
		`- ret = bugzilla.get_highest_impact(["CVE-1"])`
		`- self.assertEqual(ret, None)`
		`+ highest_cve_severity, _ = bugzilla.fetch_cve_metadata(["CVE-1"])`
		`+ self.assertEqual(highest_cve_severity, None)`

		`@patch("freshmaker.bugzilla.requests.get")`
		`- def test_get_highest_impact_empty_status(self, requests_get):`
		`+ def test_fetch_cve_metadata_empty_status(self, requests_get):`
		`bugzilla = BugzillaAPI()`
		`requests_get.return_value = MockResponse(xml_with_empty_status)`
		`- ret = bugzilla.get_highest_impact(["CVE-1"])`
		`- self.assertEqual(ret, None)`
		`+ highest_cve_severity, _ = bugzilla.fetch_cve_metadata(["CVE-1"])`
		`+ self.assertEqual(highest_cve_severity, None)`

		`@patch("freshmaker.bugzilla.requests.get")`
		`- def test_get_highest_impact_empty_bug(self, requests_get):`
		`+ def test_fetch_cve_metadata_empty_bug(self, requests_get):`
		`bugzilla = BugzillaAPI()`
		`requests_get.return_value = MockResponse(xml_with_empty_bug)`
		`- ret = bugzilla.get_highest_impact(["CVE-1"])`
		`- self.assertEqual(ret, None)`
		`+ highest_cve_severity, _ = bugzilla.fetch_cve_metadata(["CVE-1"])`
		`+ self.assertEqual(highest_cve_severity, None)`

		`@patch("freshmaker.bugzilla.requests.get")`
		`- def test_get_highest_impact_unknown_impact(self, requests_get):`
		`+ def test_fetch_cve_metadata_unknown_impact(self, requests_get):`
		`impacts = ["Low", "unknown"]`
		`requests_get.side_effect = [`
		`MockResponse(xml_with_status.format(impact=impact))`
		`for impact in impacts]`
		`bugzilla = BugzillaAPI()`
		`- ret = bugzilla.get_highest_impact(["CVE-1", "CVE-2"])`
		`- self.assertEqual(ret, "low")`
		`+ highest_cve_severity, _ = bugzilla.fetch_cve_metadata(["CVE-1", "CVE-2"])`
		`+ self.assertEqual(highest_cve_severity, "low")`
		`+`
		`+ @patch("freshmaker.bugzilla.requests.get")`
		`+ def test_fetch_cve_metadata_with_affected_pkgs(self, requests_get):`
		`+ impacts = ["Low"]`
		`+ packages = "openshift-enterprise-3.11/atomic-openshift=affected,openshift-enterprise-4.1/openshift=notaffected"`
		`+ requests_get.side_effect = [`
		`+ MockResponse(xml_with_affected_pkgs.format(impact=impact, packages=packages))`
		`+ for impact in impacts]`
		`+ bugzilla = BugzillaAPI()`
		`+ highest_cve_severity, affected_pkgs = bugzilla.fetch_cve_metadata(["CVE-1"])`
		`+ self.assertEqual(highest_cve_severity, "low")`
		`+ self.assertEqual(affected_pkgs[0]['product'], 'openshift-enterprise-3.11')`
		`+ self.assertEqual(affected_pkgs[0]['pkg_name'], 'atomic-openshift')`

tests/test_errata.py

file modified

+2 -2

		`@@ -163,8 +163,8 @@`

		`self.patcher = helpers.Patcher(`
		`'freshmaker.errata.BugzillaAPI.')`
		`- self.patcher.patch("get_highest_impact",`
		`- return_value="moderate")`
		`+ self.patcher.patch("fetch_cve_metadata",`
		`+ return_value=["moderate", {}])`

		`def tearDown(self):`
		`super(TestErrata, self).tearDown()`

gnaponie commented 4 years ago

An RHSA that addresses a specific advisory will contain RPM build(s)
for addressing a specific CVE. Freshmaker uses these RPM build(s) to
find out which images should be rebuilt. In some cases, additional
RPM build(s) are attached to RHSA. These are unrelated to the CVE.
With this change Freshmaker is going to find out which are the pkgs
in the advisory actually fix the CVE.

Signed-off-by: gnaponie gnaponie@redhat.com

lucarval commented on line 35 of freshmaker/bugzilla.py 4 years ago

Are we expecting anything other than IndexError here?

lucarval commented on line 7 of freshmaker/errata.py 4 years ago

I'm missing where affected_pkgs is actually used.

jkaluza commented 4 years ago

I'm missing where affected_pkgs is actually used.

That will be in next PR. I've advised to split these into two PRs to make them smaller and more focused.

jkaluza commented on line 33 of freshmaker/bugzilla.py 4 years ago

I think the pkg_name should be set to pkg.split('/')[-1], because in case of modules, the format is product/module_nsvc/pkg_name. We should still return pkg_name in this case.