Merge ~litios/ubuntu-cve-tracker:customer_ppa_metadata into ubuntu-cve-tracker:master

[David Fernandez Gonzalez]

Currently, only UCT CVEs could contain metadata fields. This commit
allows other project CVEs to have metadata, that will be loaded
into the main structure under the External-Metadata tag

The following fields have been considered for the external CVEs:

> 'Assigned-to', 'Notes', 'Priority', 'References'

The main motivation behind this change is so we can assign a different person to a subproject CVE as well as priority, as that may differ from the one designed in the Ubuntu CVE.

If metadata is present on the subproject CVE, it will be picked up when loading the main CVE file. If not, it will simply omit it.

Example: https://pastebin.canonical.com/p/dXnxqm3Yyg/
Example with error: https://pastebin.canonical.com/p/66S2Mp7JYJ/

[Eduardo Barretto (ebarretto)]

hey, thanks for this changes, before checking it, could you please run and paste the output:
$ python3-coverage run -a -m pytest scripts/

I also wonder if we should move those tests hidden inside $UCT/scripts to $UCT/test. But this could be done in a separate PR.

[Alex Murray (alexmurray)]

I am a bit concerned that introducing a new field 'External-Metadata' may be a bit unmaintainable as we have to remember to always check this and process it separately.

I wonder if instead it would be better to introduce the concept of making these element first class, and so making the top-level fields of Assigned-to, Notes, Priority and References always be dictionaries, keyed on the package name - this would mean a lot more refactoring but I wonder if it may be better in the long-run?

Note this is just an idea, so feel free to ignore but I would be interested to know others thoughts on it.

[David Fernandez Gonzalez (litios)]

To give a little bit of reasoning: my idea was to provide a new field to make this whole extra metadata completely optional and also because of potential new fields that could appear for special projects.

To treat that information as an appendix that may or may not exist and that we would use in customer-specific tooling instead of UCT.

But I like the idea of moving those fields to dictionaries. That would make the structure look cleaner. I was worried about the repercussions of those changes, as some of those fields are published and we would need to do some filtering over those fields.

One thing that I didn't understand is the keyed by package name. As of today, we are "bundling" those fields by the project (i.e. Ubuntu CVE and subproject CVE). Are you proposing to have those split by package? I was thinking about it as https://pastebin.canonical.com/p/M9QBWSwQpx/

I would be happy to come up with a proposal for the dictionary idea though.

[Alex Murray (alexmurray)]

Ah that seems like a better idea to have them keyed by projects rather than packages - since you are all in the room together in Prague, perhaps see what others think and if you can get some consensus, then go ahead with a more formal proposal?

[David Fernandez Gonzalez (litios)]

Sure, will do! We will try to talk about it in the "All the tech debt" meeting.
Thanks for taking the time to review it

[David Fernandez Gonzalez (litios)]

As discussed during the meeting, AlexM's idea with subprojects keys seems like the best approach.
We may want to also extend this for the pkgs, as right now everything is on the same level.

review: Disapprove

Unmerged commits

5ba83c0... by David Fernandez Gonzalez on 2023-05-03

cve_lib: add tests for external project metadata CVEs

	unit-tests:0 (build)
	check-cves:0 (build)

1 → 2 of 2 results

First • Previous • Next • Last

476f5b9... by David Fernandez Gonzalez on 2023-05-03

cve_lib: Allow metadata fields on external project CVEs

Currently, only UCT CVEs could contain metadata fields. This commit
allows other project CVEs to have metadata, that will be loaded
into the main structure under the External-Metadata tag

	unit-tests:0 (build)
	check-cves:0 (build)

1 → 2 of 2 results

First • Previous • Next • Last

diff --git a/scripts/cve_lib.py b/scripts/cve_lib.py
index d116f48..afd7749 100755
--- a/scripts/cve_lib.py
+++ b/scripts/cve_lib.py
@@ -1993,47 +1993,35 @@ class NotesParser(object):
 
 
 
-def amend_external_subproject_pkg(cve, data, srcmap, amendments, code, msg):
-    linenum = 0
-    for line in amendments.splitlines():
-        linenum += 1
-        if len(line) == 0 or line.startswith('#') or line.startswith(' '):
-            continue
-        try:
-            field, value = line.split(':', 1)
-            field = field.strip()
-            value = value.strip()
-        except ValueError as e:
-            msg += "%s: bad line '%s' (%s)\n" % (cve, line, e)
-            code = EXIT_FAIL
-            return code, msg
-
-        if '_' in field:
-            success, pkg, release, state, details, code, msg = parse_cve_release_package_field(cve, field, data, value, code, msg, linenum)
-            if not success:
-                return code, msg
-
-            data.setdefault("pkgs", dict())
-            data["pkgs"].setdefault(pkg, dict())
-            srcmap["pkgs"].setdefault(pkg, dict())
-            # override existing release info if it exists
-            data["pkgs"][pkg][release] = [state, details]
-            srcmap["pkgs"][pkg][release] = (cve, linenum)
-
-    return code, msg
-
-
 def load_external_subproject_cve_data(cve, data, srcmap, code, msg):
     cve_id = os.path.basename(cve)
+    extra_fields = ['Assigned-to', 'Notes', 'Priority', 'References']
     for f in find_external_subproject_cves(cve_id):
-        with codecs.open(f, 'r', encoding="utf-8") as fp:
-            amendments = fp.read()
-            fp.close()
-        code, msg = amend_external_subproject_pkg(f, data, srcmap, amendments, code, msg)
+        subproject = f.rsplit('/', 2)[1]
+        try:
+            external_data = load_cve(f, srcmap=srcmap, is_external=True)
+            for pkg in external_data['pkgs']:
+                for rel in external_data['pkgs'][pkg]:
+                    state, details = external_data['pkgs'][pkg][rel]
+                    data.setdefault("pkgs", dict())
+                    data["pkgs"].setdefault(pkg, dict())
+                    # override existing release info if it exists
+                    data["pkgs"][pkg][rel] = [state, details]
+
+            for field in extra_fields:
+                if field in external_data and \
+                    external_data[field] != [] and external_data[field] != '' and \
+                    external_data[field] != 'untriaged':
+                    data.setdefault("External-Metadata", dict())
+                    data["External-Metadata"].setdefault(subproject, dict())
+                    data["External-Metadata"][subproject][field] = external_data[field]
+
+        except ValueError as e:
+            return EXIT_FAIL, msg + str(e)
 
     return code, msg
 
-def load_cve(cve, strict=False, srcmap=None):
+def load_cve(cve, strict=False, srcmap=None, is_external=False):
     '''Loads a given CVE into:
        dict( fields...
              'pkgs' -> dict(  pkg -> dict(  release ->  (state, details)   ) )
@@ -2225,12 +2213,13 @@ def load_cve(cve, strict=False, srcmap=None):
         if strict:
             nonempty += ['PublicDate']
 
-        if field not in data or field not in fields_seen:
-            msg += "%s: %d: missing field '%s'\n" % (cve, linenum, field)
-            code = EXIT_FAIL
-        elif field in nonempty and data[field].strip() == "":
-            msg += "%s: %d: required field '%s' is empty\n" % (cve, linenum, field)
-            code = EXIT_FAIL
+        if not is_external:
+            if field not in data or field not in fields_seen:
+                msg += "%s: %d: missing field '%s'\n" % (cve, linenum, field)
+                code = EXIT_FAIL
+            elif field in nonempty and data[field].strip() == "":
+                msg += "%s: %d: required field '%s' is empty\n" % (cve, linenum, field)
+                code = EXIT_FAIL
 
     # Fill in defaults for missing fields
     if 'Priority' not in data:
@@ -2259,7 +2248,8 @@ def load_cve(cve, strict=False, srcmap=None):
 
     data['pkgs'] = affected
 
-    code, msg = load_external_subproject_cve_data(cve, data, srcmap, code, msg)
+    if not is_external:
+        code, msg = load_external_subproject_cve_data(cve, data, srcmap, code, msg)
 
     if code != EXIT_OKAY:
         raise ValueError(msg.strip())
diff --git a/scripts/test_cve_lib.py b/scripts/test_cve_lib.py
index f9bd1d2..0f1d78e 100755
--- a/scripts/test_cve_lib.py
+++ b/scripts/test_cve_lib.py
@@ -186,6 +186,10 @@ class TestParseCVEFiles:
                 assert cvss[0]['baseSeverity'] == js['baseMetricV3']['cvssV3']['baseSeverity']
 
 
+MOCK_SUBPROJECTS = [
+    "foo","bar"
+]
+
 class TestSubprojects:
 
     def test_load_subprojects_loads_every_config_available(self):
@@ -204,4 +208,36 @@ class TestSubprojects:
             if project and "customer" in project:
                 assert cve_lib.subprojects[rel]["customer"] == project["customer"]
 
+    @pytest.mark.parametrize("subproject", MOCK_SUBPROJECTS)
+    def test_external_subproject_metadata(self, subproject):
+        real_subproject_dir = cve_lib.subprojects_dir
+        cve_lib.subprojects_dir = os.path.join(TEST_DATA_DIR, 'subprojects')
+        cve_lib.load_external_subprojects()
+
+        cve = cve_lib.load_cve(os.path.join(TEST_DATA_DIR, 'okay/priority-critical'), strict=True)
+        assert cve['External-Metadata']
+        assert cve['External-Metadata'][subproject]
+
+        subproject_cve_data = cve_lib.load_cve(os.path.join(TEST_DATA_DIR, 'subprojects', subproject, 'priority-critical'), is_external=True)
+
+        for key in cve['External-Metadata'][subproject]:
+            assert cve['External-Metadata'][subproject][key] == subproject_cve_data[key]
+
+        cve_lib.subprojects_dir = real_subproject_dir
+        cve_lib.load_external_subprojects()
+
+    @pytest.mark.parametrize("subproject", MOCK_SUBPROJECTS)
+    def test_invalid_external_subproject_metadata(self, subproject):
+        real_subproject_dir = cve_lib.subprojects_dir
+        cve_lib.subprojects_dir = os.path.join(TEST_DATA_DIR, 'subprojects')
+        cve_lib.load_external_subprojects()
+        cve_lib.external_releases = [os.path.join(subproject, 'focal')]
+
+        error =  f"test/bad/priority-invalid-value: 13: unknown Priority 'meduim'\n"
+        subproject_err = f"test/subprojects/{subproject}/priority-invalid-value: 5: unknown Priority"
+
+        with pytest.raises(ValueError, match=error+subproject_err):
+            cve_lib.load_cve(os.path.join(TEST_DATA_DIR, 'bad/priority-invalid-value'), strict=True)
 
+        cve_lib.subprojects_dir = real_subproject_dir
+        cve_lib.load_external_subprojects()
 \ No newline at end of file
diff --git a/test/subprojects/bar/focal/supported.txt b/test/subprojects/bar/focal/supported.txt
 new file mode 100644
index 0000000..83b3780
--- /dev/null
+++ b/test/subprojects/bar/focal/supported.txt
@@ -0,0 +1 @@
+ppp
diff --git a/test/subprojects/bar/priority-critical b/test/subprojects/bar/priority-critical
 new file mode 100644
index 0000000..4b7736d
--- /dev/null
+++ b/test/subprojects/bar/priority-critical
@@ -0,0 +1,8 @@
+References: 
+ https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-0001 
+Notes: 
+ bar> Test
+Priority: critical
+Assigned-to: bar
+
+bar/focal_ppp: released (2.4.7-2+4.1ubuntu8)
diff --git a/test/subprojects/bar/priority-invalid-value b/test/subprojects/bar/priority-invalid-value
 new file mode 100644
index 0000000..c826aa7
--- /dev/null
+++ b/test/subprojects/bar/priority-invalid-value
@@ -0,0 +1,8 @@
+References: 
+ https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-0002
+Notes: 
+ foo> Test
+Priority: Critical
+Assigned-to: foo
+
+bar/focal_ppp: released (2.4.7-2+4.1ubuntu8)
diff --git a/test/subprojects/foo/focal/supported.txt b/test/subprojects/foo/focal/supported.txt
 new file mode 100644
index 0000000..83b3780
--- /dev/null
+++ b/test/subprojects/foo/focal/supported.txt
@@ -0,0 +1 @@
+ppp
diff --git a/test/subprojects/foo/priority-critical b/test/subprojects/foo/priority-critical
 new file mode 100644
index 0000000..4035ba8
--- /dev/null
+++ b/test/subprojects/foo/priority-critical
@@ -0,0 +1,8 @@
+References: 
+ https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-0002
+Notes: 
+ foo> Test
+Priority: low
+Assigned-to: foo
+
+foo/focal_ppp: released (2.4.7-2+4.1ubuntu8)
diff --git a/test/subprojects/foo/priority-invalid-value b/test/subprojects/foo/priority-invalid-value
 new file mode 100644
index 0000000..6ac754d
--- /dev/null
+++ b/test/subprojects/foo/priority-invalid-value
@@ -0,0 +1,8 @@
+References: 
+ https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-0002
+Notes: 
+ foo> Test
+Priority: Low
+Assigned-to: foo
+
+foo/focal_ppp: released (2.4.7-2+4.1ubuntu8)