Add istio importer and tests #336

tushar912 · 2021-02-05T11:13:27Z

Description

Added importer and tests for istio
Also added test files in test_data/istio

Fixes #302

vulnerabilities/importers/istio.py

pombredanne

Thank you for this! I have added a few comments for your consideration.

vulnerabilities/importer_yielder.py

vulnerabilities/importers/istio.py

pombredanne · 2021-02-09T13:13:49Z

vulnerabilities/importers/istio.py

+
+        safe_pkg_versions = []
+        vuln_pkg_versions = []
+        all_version_list = self.version_api.get("istio/istio")


what do you gain from suffixing your variable names with a type? Would not versions or all_version be better than all_version_list?

Sure, I would change them.

pombredanne · 2021-02-09T13:24:14Z

vulnerabilities/importers/istio.py

+                cve_id = cve_id
+
+                safe_purls = {
+                    PackageURL(name="istio", type="golang", version=version)


In the logical order of fields, type would go first

pombredanne · 2021-02-09T13:25:14Z

vulnerabilities/importers/istio.py

+                }
+
+                vuln_purls = {
+                    PackageURL(name="istio", type="golang", version=version)


What about also creating github type purls?

@pombredanne I am still unable to understand how to create two package types for a single purl. Can you explain it a bit.

@tushar912 sorry if I was not clear: I meant to have multiple purls: one pkg:/golang and one pkg:github since the project has both personalities

vulnerabilities/tests/test_data/istio/test_file.md

vulnerabilities/importers/istio.py

vulnerabilities/tests/test_istio.py

tushar912 · 2021-02-10T13:32:51Z

@pombredanne @sbs2001 I have made some changes as per the suggestions made by you. Please review again.

vulnerabilities/importer_yielder.py

sbs2001 · 2021-02-12T15:17:17Z

vulnerabilities/importers/istio.py

+        safe_pkg_versions = []
+        vuln_pkg_versions = []
+        all_version_list = self.version_api.get("istio/istio")
+        if not version_range_list:


the condition and all_version should be at top. We don't want to create objects just to have them destroyed.

sbs2001 · 2021-02-12T15:19:17Z

vulnerabilities/importers/istio.py

+        all_version_list = self.version_api.get("istio/istio")
+        if not version_range_list:
+            return all_version_list, []
+        version_ranges = {RangeSpecifier(r) for r in version_range_list}


@pombredanne The codebase has (over) use of sets all over the place mostly because we were paranoid of duplicates and order didn't matter.

@tushar912 use a list, if that's ok ?

vulnerabilities/importers/istio.py

sbs2001 · 2021-02-12T15:28:28Z

vulnerabilities/importers/istio.py

+                    ubound = "<=" + release[2]
+                    releases.append(lbound + "," + ubound)
+
+        data["releases"] = releases


This a list of string version ranges now, change the key name here to something that reflects that

sbs2001 · 2021-02-12T15:30:12Z

vulnerabilities/importers/istio.py

+            safe_purls = []
+            vuln_purls = []
+
+            cve_id = cve_id


Oh, I should remove this

vulnerabilities/importers/istio.py

tushar912 · 2021-02-17T17:28:04Z

@sbs2001 I have made the changes.

sbs2001 · 2021-02-18T05:05:47Z

vulnerabilities/importers/istio.py

+            safe_pkg_versions, vuln_pkg_versions = self.get_versions_for_pkg_from_range_list(
+                                                   data["release_ranges"])
+
+            safe_purls = []


Is there a need to declare this here, since you are assigning them the union below ?

I should remove it.

sbs2001 · 2021-02-18T05:10:27Z

vulnerabilities/importers/istio.py

+                advisories.extend(processed_data)
+        return self.batch_advisories(advisories)
+
+    def get_versions_for_pkg_from_range_list(self, version_range_list):


The function name is too verbose. How about get_pkg_versions_from_ranges ?

vulnerabilities/importers/istio.py

tushar912 · 2021-02-19T12:36:59Z

@sbs2001 I have made the changes, also can you answer #336 (comment).

sbs2001 · 2021-02-23T12:34:59Z

vulnerabilities/importers/istio.py

+                    ubound = "<=" + release[2]
+                    releases.append(lbound + "," + ubound)
+                # If it is a single release
+                elif isinstance(release, int):


Could you point me to an example of this ?

https://github.com/istio/istio.io/blob/master/content/en/news/security/istio-security-2020-004/index.md

Could this be done without using isinstance ? This is going to fail in case the release is of form 1.x.y . Maybe some string check ?

Should I use a regex check?

I would try to avoid regex unless absolutely needed, because those are very cryptic when reading.

If there's no other way could you do something like https://github.com/nexB/vulnerablecode/blob/f81816fcd95d00736dc17c9aaad20e91cb6ce201/vulnerabilities/helpers.py#L81 ?

That way atleast, it would be easy to understand what the regex is trying to do without going into the details.

sbs2001

@tushar912 thanks this almost at a stage to be merged, please see my comments inline.

vulnerabilities/importers/istio.py

sbs2001 · 2021-02-23T12:46:15Z

@tushar912 Due to recent model changes, you would need to rename Advisory.cve_id ->Advisory.vulnerability_id

sbs2001 · 2021-02-24T13:47:28Z

vulnerabilities/importers/istio.py

+        as parameter and returns a tuple of safe package versions and
+        vulnerable package versions"""
+        all_version = self.version_api.get("istio/istio")
+        # if not version_range_list:


I guess this can be removed now ?

Oh I missed this. I will remove it.

sbs2001

See my comment of usage of isinstance and remove the commented code and we're good to merge :)

Signed-off-by: Tushar912 <[email protected]>

Also add github as package type Signed-off-by: Tushar912 <[email protected]>

tushar912 · 2021-03-11T07:02:38Z

@sbs2001 Sorry for the delay. I have added a better check for single release using regex.

assign cve to empty string if its N/A Signed-off-by: Tushar912 <[email protected]>

sbs2001 · 2021-03-13T16:26:06Z

vulnerabilities/importers/istio.py

+                    ubound = "<=" + release[2]
+                    releases.append(lbound + "," + ubound)
+                # If it is a single release
+                elif is_release(release):


sbs2001

Thanks @tushar912 for your patience and I'm merging this.

tushar912 mentioned this pull request Feb 5, 2021

Collect Istio #302

Closed

tushar912 force-pushed the istio branch from 16d7378 to 772c150 Compare February 5, 2021 11:54

sbs2001 self-requested a review February 7, 2021 05:58

sbs2001 requested changes Feb 7, 2021

View reviewed changes

vulnerabilities/importers/istio.py Outdated Show resolved Hide resolved

vulnerabilities/importers/istio.py Outdated Show resolved Hide resolved

vulnerabilities/importers/istio.py Outdated Show resolved Hide resolved

tushar912 force-pushed the istio branch from 772c150 to f32abcc Compare February 9, 2021 14:23

pombredanne requested changes Feb 9, 2021

View reviewed changes

tushar912 requested review from pombredanne and sbs2001 February 10, 2021 13:33

tushar912 changed the title ~~[WIP] Add istio importer and tests~~ Add istio importer and tests Feb 10, 2021

sbs2001 requested changes Feb 12, 2021

View reviewed changes

tushar912 force-pushed the istio branch 7 times, most recently from 73b81f6 to e2ad923 Compare February 17, 2021 11:43

tushar912 requested a review from sbs2001 February 17, 2021 11:43

sbs2001 requested changes Feb 18, 2021

View reviewed changes

tushar912 force-pushed the istio branch from e2ad923 to cbafcca Compare February 19, 2021 11:41

tushar912 requested a review from sbs2001 February 19, 2021 12:35

sbs2001 reviewed Feb 23, 2021

View reviewed changes

sbs2001 requested changes Feb 23, 2021

View reviewed changes

vulnerabilities/importers/istio.py Outdated Show resolved Hide resolved

tushar912 force-pushed the istio branch 3 times, most recently from d90ab42 to b1f563a Compare February 24, 2021 12:45

tushar912 requested a review from sbs2001 February 24, 2021 13:07

sbs2001 reviewed Feb 24, 2021

View reviewed changes

sbs2001 requested changes Feb 24, 2021

View reviewed changes

tushar912 force-pushed the istio branch from b1f563a to 180a579 Compare March 11, 2021 05:16

tushar912 added 3 commits March 11, 2021 10:50

Add istio importer and tests

e8c5bf1

Signed-off-by: Tushar912 <[email protected]>

Use yaml_lines of file to extract data.

05b7ba6

Signed-off-by: Tushar912 <[email protected]>

Add docstrings and add test for get_data_from_md.

43e4add

Also add github as package type Signed-off-by: Tushar912 <[email protected]>

tushar912 force-pushed the istio branch 3 times, most recently from d8e4ec3 to 5b6c5dd Compare March 11, 2021 07:00

tushar912 requested a review from sbs2001 March 11, 2021 07:02

Remove added_advisories method and

60f6c01

assign cve to empty string if its N/A Signed-off-by: Tushar912 <[email protected]>

tushar912 force-pushed the istio branch from 5b6c5dd to 60f6c01 Compare March 13, 2021 10:07

sbs2001 reviewed Mar 13, 2021

View reviewed changes

sbs2001 approved these changes Mar 13, 2021

View reviewed changes

Merge branch 'main' into istio

72e461c

sbs2001 merged commit a3e98c2 into aboutcode-org:main Mar 14, 2021

Add istio importer and tests #336

Add istio importer and tests #336

Conversation

tushar912 commented Feb 5, 2021 • edited Loading

Description

pombredanne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tushar912 commented Feb 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tushar912 commented Feb 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tushar912 commented Feb 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbs2001 left a comment

Choose a reason for hiding this comment

sbs2001 commented Feb 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbs2001 left a comment

Choose a reason for hiding this comment

tushar912 commented Mar 11, 2021

Choose a reason for hiding this comment

sbs2001 left a comment

Choose a reason for hiding this comment

tushar912 commented Feb 5, 2021 •

edited

Loading