Merge ~eslerm/ubuntu-cve-tracker:website-fix-tag into ubuntu-cve-tracker:master

[Mark Esler]

closes https://github.com/canonical/ubuntu.com/issues/13498

[Seth Arnold (seth-arnold)]

I think this will do the job. It's probably worth trying by-hand on both an entry with a tag and an entry without a tag before checking it in, but I've got good feelings about this.

Once it works we should do a big run over all affected CVEs.

Thanks

review: Approve

[Mark Esler (eslerm)]

Thanks Seth

tags are not standardized, and there are some tags with spaces in the

so, I need to make a new patch :)

[Diogo Sousa (0xdsousa)]

> Thanks Seth
>
> tags are not standardized, and there are some tags with spaces in the
>
> so, I need to make a new patch :)

If we know that the first separator will always be a space, `maxsplit=1` should ensure two components.

[Eduardo Barretto (ebarretto)]

is this still under development?

review: Needs Information

[Mark Esler (eslerm)]

This is ready now. I tested against all active/ ignored/ and retired/ cves.

[Mark Esler (eslerm)]

retested with two lines of deadcode removed

[Alex Murray (alexmurray)]

Would it be better to update cve_lib so that it can and does parse the tags and returns these as a tuple along with the patch URL that way we can encapsulate that parsing in a more logical location?

[Mark Esler (eslerm)]

I can move this to cve_lib if preferred. I've been hesitant to move the kludge there.

To remove all kludges, our deb822 UCT file format should change to properly track tags. i.e., tags shouldn't exist as part of the url string. (To do this, tags would first need to be sanitized to remove whitespace. And what to do in the case of no tags would need to be worked out.)

e.g.,

```
diff --git a/active/CVE-2023-2953 b/active/CVE-2023-2953
index 453ef36a951..3f5664ce223 100644
--- a/active/CVE-2023-2953
+++ b/active/CVE-2023-2953
@@ -20,12 +20,16 @@ CVSS:
  nvd: CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H [7.5 HIGH]

 Patches_openldap:
- upstream: https://git.openldap.org/openldap/openldap/-/commit/ea8dd2d279c5aeaf9d4672a4e95bebd99babcce1 (master)
- upstream: https://git.openldap.org/openldap/openldap/-/commit/3f2abd0b2eeec8522e50d5c4ea4992e70e8f9915 (master)
- upstream: https://git.openldap.org/openldap/openldap/-/commit/c5c8c06a8bd52ea7b843e7d8ca961a7d1800ce5f (2_6_4)
- upstream: https://git.openldap.org/openldap/openldap/-/commit/840944e26f734bb03d925f26c4ef11a6cedcbb9c (2_6_4)
- upstream: https://git.openldap.org/openldap/openldap/-/commit/752d320cf96e46f24c0900f1a8f6af0a3fc3c4ce (2_5_14)
- upstream: https://git.openldap.org/openldap/openldap/-/commit/6563fab9e2feccb0a684d0398e78571d09fb808b (2_5_14)
+ upstream:
+ master:
+ https://git.openldap.org/openldap/openldap/-/commit/ea8dd2d279c5aeaf9d4672a4e95bebd99babcce1
+ https://git.openldap.org/openldap/openldap/-/commit/3f2abd0b2eeec8522e50d5c4ea4992e70e8f9915
+ 2_6_4:
+ https://git.openldap.org/openldap/openldap/-/commit/c5c8c06a8bd52ea7b843e7d8ca961a7d1800ce5f
+ https://git.openldap.org/openldap/openldap/-/commit/840944e26f734bb03d925f26c4ef11a6cedcbb9c
+ 2_5_14:
+ https://git.openldap.org/openldap/openldap/-/commit/752d320cf96e46f24c0900f1a8f6af0a3fc3c4ce
+ https://git.openldap.org/openldap/openldap/-/commit/6563fab9e2feccb0a684d0398e78571d09fb808b
 upstream_openldap: released (2.6.4)
 trusty_openldap: ignored (end of standard support)
 trusty/esm_openldap: released (2.4.31-1+nmu2ubuntu8.5+esm6)
```

edit: Launchpad does not show whitespace of the above example well. Please check in an editor.

[Alex Murray (alexmurray)]

I don't think we would have to change the CVE file format necessarily - although I do like the idea of making this more structured - so let's not bikeshed on that or let it get in the way of this MR - we can always do that work as a separate MR in the future, so +1 from me on this current MR as is.

review: Approve

[Mark Esler (eslerm)]

Alex, would you still like this moved to cve_lib? I don't mind doing so. I can merge this now and remove it after.

[Alex Murray (alexmurray)]

Assuming I understand you correctly, I think if you move it to cve_lib you'll end up having to fix up all existing users of Patches within cve_lib to then expect a list of tuples, not a list of strings which feels like too much churn - particularly if you want to restructure the CVE files eventually anyway to make this more hierarchical as per your example earlier - so I think we merge this as is then can work on improving things later.

[Marc Deslauriers (mdeslaur)]

It's not intended to be a tag, it's a comment, and there could be whitespace in it. Trying to use comments as tags only would change the commit sort order. I think that would be a big change to how we use them now, and we'd still need to figure out how to add comments.

[Marc Deslauriers (mdeslaur)]

Here's an example from CVE-2023-48795:

Patches_paramiko:
 upstream: https://github.com/paramiko/paramiko/commit/be3ffc18cc466e0b0a877d716721353c12561bcc
 upstream: https://github.com/paramiko/paramiko/commit/4c7f0410c533cdf0df2890512237961f934f5ab9 (changelog)
 upstream: https://github.com/paramiko/paramiko/commit/773a174fb1e40e1d18dbe2625e16337ea401119e
 upstream: https://github.com/paramiko/paramiko/commit/c32be441a5ff0dc4914b22d6d1efa392aebe862f (not required)
 upstream: https://github.com/paramiko/paramiko/commit/f4dedacb9040d27d9844f51c81c28e0247d3e4a3
 upstream: https://github.com/paramiko/paramiko/commit/73f079f5a4bbba7f3048dadbe05b24242206745e (changelog)
 upstream: https://github.com/paramiko/paramiko/commit/75e311d3c0845a316b6e7b3fae2488d86ad5a270
 upstream: https://github.com/paramiko/paramiko/commit/fa46de7feeeb8a01dc471581a0258252ce4f2db6
 upstream: https://github.com/paramiko/paramiko/commit/8dcb237f0ee095b1d8b26765c3d41d0ab6963be9 (test suite fix)
 upstream: https://github.com/paramiko/paramiko/commit/58785d29c47570fa700e096d16b9a0d3a6069048 (changelog)
 upstream: https://github.com/paramiko/paramiko/commit/96db1e2be856eac66631761bae41167a1ebd2b4e
 upstream: https://github.com/paramiko/paramiko/commit/33508c920309860c4a775be70f209c2a400e18ec
 upstream: https://github.com/paramiko/paramiko/commit/30b447b911c39460bbef5e7834e339c43a251316 (lint)
 upstream: https://github.com/paramiko/paramiko/commit/3e4bdf998f5b1508322234a527e8fa432220368b (changelog)

[Mark Esler (eslerm)]

Ack, thanks for clarification Marc. This should not be merged.

My conclusion now is that the issue is on the webpage tooling end: https://github.com/canonical/ubuntu.com/issues/13498

Does that sound reasonable to others? If so, I'll update that issue with a comment for the web team.

They should be able to add an anchor link to our output `<source>: <url>` or `<source>: <url> (<note>)`.

Also, what is happening here: https://ubuntu.com/security/CVE-2023-48795 o.o

review: Disapprove

[Seth Arnold (seth-arnold)]

Getting the web team to adapt to what we send them has been a challenge; I think we'd be better suited to just send them the moral equivalent of foo.split(" ")[0] and leave it at that. Changing the submission format to support both the patch URL and patch description would be nice but that's even more work. We could unbreak all the links with a simple thing that'll throw away a bit of data in the display.

[Mark Esler (eslerm)]

Following Seth's advice, the new patch drops notes.

I added comments to clarify why/how things are done.

review: Approve

[Seth Arnold (seth-arnold)]

I think the regex is too tight, it'll skip entries with spaces in the note.

[Mark Esler (eslerm)]

Right you are. Thanks Seth!

Now:
```
Patches_moodle:
 upstream: http://foo.bar
 upstream: http://foo.bar
 upstream: http://foo.bar (note)
 upstream: http://foo.bar (note note)
```

Results in:
```
{'moodle': ['upstream: http://foo.bar', 'upstream: http://foo.bar', 'upstream: http://foo.bar', 'upstream: http://foo.bar']}
```

[Seth Arnold (seth-arnold)]

Cool, thanks!

You've spent a fair amount of time with this function, and this file; would it be easy to add some tests here? those four examples are a really decent start on test cases. I *think* it wouldn't be too much more to get some tests, but hooking them up to run periodically would be the bigger challenge. (And it might be super-easy once you know the trick, I just don't know it off-hand.)

Thanks

[Seth Arnold (seth-arnold)]

Useless comment to see if I can reproduce an oops.

[Mark Esler (eslerm)]

Updates for tests added.

Tests across UCT are a bit scattered and loosely defined, so I've filed https://bugs.launchpad.net/ubuntu-cve-tracker/+bug/2052830

[Tobias Heider (tobhe)]

Just stumbled over that bug, patch looks good!

review: Approve

diff --git a/scripts/publish-cves-to-website-api.py b/scripts/publish-cves-to-website-api.py
index 5ee1737..aafe81e 100755
--- a/scripts/publish-cves-to-website-api.py
+++ b/scripts/publish-cves-to-website-api.py
@@ -1,7 +1,6 @@
 #! /usr/bin/env python3
 # Standard library
 import os
-import pprint
 import sys
 import cve_lib
 import re
@@ -54,7 +53,20 @@ def get_tags(cve_data):
 def get_patches(cve_data):
     patches = {}
     for pkg in cve_data['patches']:
-        patches[pkg] = [patch_type + ": " + entry for patch_type, entry in cve_data['patches'].get(pkg)]
+        temp = []
+        for source, entry in cve_data['patches'].get(pkg):
+            # NOTE: KLUDGE1: cve_lib is not aware of patch notes in entry
+            entry_match = re.match(r"(\S+) \((.+)\)", entry)
+            if entry_match:
+                url = entry_match.groups()[0]
+                # NOTE: KLUDGE2: external web tooling cannot handle notes
+                #                drop them.
+                #       https://github.com/canonical/ubuntu.com/issues/13498
+                #note = entry_match.groups()[1]
+            else:
+                url = entry
+            temp.append(f"{source}: {url}")
+        patches[pkg] = temp
     return patches
 
 def get_devel_codename(cve_releases):
diff --git a/scripts/test_publish-cves-to-website-api.py b/scripts/test_publish-cves-to-website-api.py
index 7e4ed88..d2b37a5 100755
--- a/scripts/test_publish-cves-to-website-api.py
+++ b/scripts/test_publish-cves-to-website-api.py
@@ -2,27 +2,101 @@
 
 # Dependencies: python3-macaroonbakery
 
-import pytest
+import cve_lib
 import json
 import os
 import importlib
+import pytest
+
 publish_cves = importlib.import_module("publish-cves-to-website-api")
 
 TEST_DATA_DIR = "test"
-PARSE_OKAY_TESTS = [f for f in os.listdir("test/website_api") \
-        if f.startswith("use_") and not f.endswith(".json")]
+PARSE_OKAY_TESTS = [
+    file
+    for file in os.listdir("test/website_api")
+    if file.startswith("use_") and not file.endswith(".json")
+]
+
 
 class TestWebSiteAPI:
+    """pytest website api dry run"""
+
     def __check_simple_okay(self, cve_test_file):
+        """test website api dry run"""
         _test_file = os.path.join(TEST_DATA_DIR, "website_api", cve_test_file)
         assert os.path.exists(_test_file)
-        payload = publish_cves.main(['--dry-run', '--ignore-filename-check', _test_file])
+        payload = publish_cves.main(
+            ["--dry-run", "--ignore-filename-check", _test_file]
+        )
         assert payload is not None
-        with open("%s.json" % _test_file, "rt") as f:
-            payload_json = json.load(f)
+        with open(f"{_test_file}.json", "rt") as file:
+            payload_json = json.load(file)
         assert payload_json is not None
         assert payload == payload_json
 
     @pytest.mark.parametrize("cve_test_file", PARSE_OKAY_TESTS)
     def test_simple_okay(self, cve_test_file):
+        """parameterize website api dry run"""
         self.__check_simple_okay(cve_test_file)
+
+    def test_get_patches(self):
+        """test publish_cves.get_patches()"""
+        test_file = "active/CVE-2023-48795"
+        assert os.path.exists(test_file)
+        cve_data = cve_lib.load_cve(test_file)
+        patches = publish_cves.get_patches(cve_data)
+        assert patches == {
+            "openssh": [
+                "upstream: https://github.com/openssh/openssh-portable/commit/1edb00c58f8a6875fad6a497aa2bacf37f9e6cd5"
+            ],
+            "dropbear": [
+                "upstream: https://github.com/mkj/dropbear/commit/6e43be5c7b99dbee49dc72b6f989f29fdd7e9356"
+            ],
+            "golang-go.crypto": [
+                "upstream: https://github.com/golang/crypto/commit/9d2ee975ef9fe627bf0a6f01c1f69e8ef1d4f05d"
+            ],
+            "snapd": [],
+            "lxd": [],
+            "libssh": [
+                "upstream: https://gitlab.com/libssh/libssh-mirror/-/commit/4cef5e965a46e9271aed62631b152e4bd23c1e3c",
+                "upstream: https://gitlab.com/libssh/libssh-mirror/-/commit/0870c8db28be9eb457ee3d4f9a168959d9507efd",
+                "upstream: https://gitlab.com/libssh/libssh-mirror/-/commit/5846e57538c750c5ce67df887d09fa99861c79c6",
+                "upstream: https://gitlab.com/libssh/libssh-mirror/-/commit/89df759200d31fc79fbbe213d8eda0d329eebf6d",
+            ],
+            "libssh2": [
+                "upstream: https://github.com/libssh2/libssh2/commit/d34d9258b8420b19ec3f97b4cc5bf7aa7d98e35a",
+                "upstream: https://github.com/libssh2/libssh2/pull/1291",
+            ],
+            "openssh-ssh1": [],
+            "paramiko": [
+                "upstream: https://github.com/paramiko/paramiko/commit/be3ffc18cc466e0b0a877d716721353c12561bcc",
+                "upstream: https://github.com/paramiko/paramiko/commit/4c7f0410c533cdf0df2890512237961f934f5ab9",
+                "upstream: https://github.com/paramiko/paramiko/commit/773a174fb1e40e1d18dbe2625e16337ea401119e",
+                "upstream: https://github.com/paramiko/paramiko/commit/c32be441a5ff0dc4914b22d6d1efa392aebe862f",
+                "upstream: https://github.com/paramiko/paramiko/commit/f4dedacb9040d27d9844f51c81c28e0247d3e4a3",
+                "upstream: https://github.com/paramiko/paramiko/commit/73f079f5a4bbba7f3048dadbe05b24242206745e",
+                "upstream: https://github.com/paramiko/paramiko/commit/75e311d3c0845a316b6e7b3fae2488d86ad5a270",
+                "upstream: https://github.com/paramiko/paramiko/commit/fa46de7feeeb8a01dc471581a0258252ce4f2db6",
+                "upstream: https://github.com/paramiko/paramiko/commit/8dcb237f0ee095b1d8b26765c3d41d0ab6963be9",
+                "upstream: https://github.com/paramiko/paramiko/commit/58785d29c47570fa700e096d16b9a0d3a6069048",
+                "upstream: https://github.com/paramiko/paramiko/commit/96db1e2be856eac66631761bae41167a1ebd2b4e",
+                "upstream: https://github.com/paramiko/paramiko/commit/33508c920309860c4a775be70f209c2a400e18ec",
+                "upstream: https://github.com/paramiko/paramiko/commit/30b447b911c39460bbef5e7834e339c43a251316",
+                "upstream: https://github.com/paramiko/paramiko/commit/3e4bdf998f5b1508322234a527e8fa432220368b",
+            ],
+            "putty": [
+                "upstream: https://git.tartarus.org/?p=simon/putty.git;a=commit;h=9e099151574885f3c717ac10a633a9218db8e7bb",
+                "upstream: https://git.tartarus.org/?p=simon/putty.git;a=commit;h=f2e7086902b3605c96e54ef9c956ca7ab000010e",
+                "upstream: https://git.tartarus.org/?p=simon/putty.git;a=commit;h=9fcbb86f715bc03e58921482efe663aa0c662d62",
+                "upstream: https://git.tartarus.org/?p=simon/putty.git;a=commit;h=244be5412728a7334a2d457fbac4e0a2597165e5",
+                "upstream: https://git.tartarus.org/?p=simon/putty.git;a=commit;h=58fc33a155ad496bdcf380fa6193302240a15ae9",
+                "upstream: https://git.tartarus.org/?p=simon/putty.git;a=commit;h=0b00e4ce26d89cd010e31e66fd02ac77cb982367",
+                "upstream: https://git.tartarus.org/?p=simon/putty.git;a=commit;h=fdc891d17063ab26cf68c74245ab1fd9771556cb",
+                "upstream: https://git.tartarus.org/?p=simon/putty.git;a=commit;h=b80a41d386dbfa1b095c17bd2ed001477f302d46",
+            ],
+            "proftpd-dfsg": [],
+            "python-asyncssh": [
+                "upstream: https://github.com/ronf/asyncssh/commit/0bc73254f41acb140187e0c89606311f88de5b7b"
+            ],
+            "filezilla": [],
+        }