Repository: packit-service/packit
Pull request #699 (2020-02-07 10:21:42)

keys.fedoraproject.org -> keys.openpgp.org + packit/security.py rewrite
https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/COEYWJBQDAWRSYNQW7Y7TD2EKEGBWOAY/

softwarefactory-project-zuul[bot]: Build succeeded.

- [packit-pre-commit ](https://softwarefactory-project.io/zuul/t/local/build/8f4c59a3ae5e4dd9b848ea0687a6aec1) : SUCCESS in 4m 24s
- [packit-tests-rpm ](https://softwarefactory-project.io/zuul/t/local/build/ee0f459adfc547b8a831891c06569b3e) : SUCCESS in 9m 33s
- [packit-tests-pip-deps ](https://softwarefactory-project.io/zuul/t/local/build/ee0d8e42206b41c1a84b9d15d9dd666e) : SUCCESS in 10m 07s
- [packit-tests-git-master ](https://softwarefactory-project.io/zuul/t/local/build/8be421bec4114f5696247de7b2fe5147) : SUCCESS in 8m 56s
- [packit-tests-rpm-sess-rec ](https://softwarefactory-project.io/zuul/t/local/build/d6bc9b2b58ec4a68847e08d5e63a2b57) : SUCCESS in 3m 12s
- [packit-tests-pip-deps-sess-rec ](https://softwarefactory-project.io/zuul/t/local/build/e43ba76588ce4d26b4217a43985f1c46) : SUCCESS in 4m 00s
- [packit-tests-git-master-sess-rec ](https://softwarefactory-project.io/zuul/t/local/build/fee7159e837b4f3bafe925655edf4588) : SUCCESS in 4m 10s

packit-as-a-service[bot]: Congratulations! One of the builds has completed. :champagne:

You can install the built RPMs by following these steps:

* `sudo yum install -y dnf-plugins-core` on RHEL 8
* `sudo dnf install -y dnf-plugins-core` on Fedora
* `dnf copr enable packit/packit-service-packit-699`
* And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.
lgtm-com[bot]: This pull request **introduces 1 alert** when merging 79731c621e92f632ed844965d98781f9c22bb35f into 1fa033d478a3f65aece5ca5261b0f66bd83bc00d - [view on LGTM.com](https://lgtm.com/projects/g/packit-service/packit/rev/pr-3982cd9a14e25a6ef32a33449702ed243f18758f)

**new alerts:**

* 1 for Unnecessary &#39;else&#39; clause in loop
softwarefactory-project-zuul[bot]: Build succeeded.

- [packit-pre-commit ](https://softwarefactory-project.io/zuul/t/local/build/ae354e129e584b50921fd2b2f639c01e) : SUCCESS in 3m 12s
- [packit-tests-rpm ](https://softwarefactory-project.io/zuul/t/local/build/72cf6411e45d4415aded4f273667ac33) : SUCCESS in 7m 01s
- [packit-tests-pip-deps ](https://softwarefactory-project.io/zuul/t/local/build/1224e79bc73f49ac9486a8f149d63cc9) : SUCCESS in 7m 02s
- [packit-tests-git-master ](https://softwarefactory-project.io/zuul/t/local/build/16bda95dd241441aa57f88d7a84af694) : SUCCESS in 9m 04s
- [packit-tests-rpm-sess-rec ](https://softwarefactory-project.io/zuul/t/local/build/c77e2d4807dd4536a99b665f04472729) : SUCCESS in 2m 44s
- [packit-tests-pip-deps-sess-rec ](https://softwarefactory-project.io/zuul/t/local/build/086c5db9e6ed49d9862b9607453aaf08) : SUCCESS in 4m 01s
- [packit-tests-git-master-sess-rec ](https://softwarefactory-project.io/zuul/t/local/build/e1e5bdd1c677457db2d98f4d6a6dd865) : SUCCESS in 3m 14s

softwarefactory-project-zuul[bot]: Build failed.

- [packit-pre-commit ](https://softwarefactory-project.io/zuul/t/local/build/d266865bdb1541c0b27a89143d3ae153) : SUCCESS in 3m 46s
- [packit-tests-rpm ](https://softwarefactory-project.io/zuul/t/local/build/e5ef4555380b49329bae018ce96bb546) : SUCCESS in 8m 05s
- [packit-tests-pip-deps ](https://softwarefactory-project.io/zuul/t/local/build/d99459c965164f8997bcd4173e017fd2) : SUCCESS in 7m 13s
- [packit-tests-git-master ](https://softwarefactory-project.io/zuul/t/local/build/703e563c501141b29f3856a183635df5) : FAILURE in 6m 55s
- [packit-tests-rpm-sess-rec ](https://softwarefactory-project.io/zuul/t/local/build/f5875cf298214743a6fe7a6efbc2b3ea) : SUCCESS in 3m 10s
- [packit-tests-pip-deps-sess-rec ](https://softwarefactory-project.io/zuul/t/local/build/dd6ccd87bf5b450097ceb9ce004ad93c) : SUCCESS in 3m 44s
- [packit-tests-git-master-sess-rec ](https://softwarefactory-project.io/zuul/t/local/build/f46d7660f87d4a49bf4ce1b53fefd6c3) : SUCCESS in 3m 24s

jpopelka: recheck
softwarefactory-project-zuul[bot]: Build succeeded.

- [packit-pre-commit ](https://softwarefactory-project.io/zuul/t/local/build/ef96749103954a6ab082f285248aaed2) : SUCCESS in 3m 47s
- [packit-tests-rpm ](https://softwarefactory-project.io/zuul/t/local/build/19aecb06e4df4c31816de5aa8e092361) : SUCCESS in 7m 27s
- [packit-tests-pip-deps ](https://softwarefactory-project.io/zuul/t/local/build/14dfa6bb5f2e4105b3eb4a8128e7561a) : SUCCESS in 7m 50s
- [packit-tests-git-master ](https://softwarefactory-project.io/zuul/t/local/build/a9ccd590596b40cd8955b1f9dc47bb00) : SUCCESS in 7m 47s
- [packit-tests-rpm-sess-rec ](https://softwarefactory-project.io/zuul/t/local/build/a67683636c244076ac303467a4c65576) : SUCCESS in 2m 29s
- [packit-tests-pip-deps-sess-rec ](https://softwarefactory-project.io/zuul/t/local/build/4bd77f017f9d42cdb12b09d9bd1502ab) : SUCCESS in 3m 48s
- [packit-tests-git-master-sess-rec ](https://softwarefactory-project.io/zuul/t/local/build/2dfde86189af475fac43ad761e98d733) : SUCCESS in 3m 50s


Patch:
diff --git a/packit/security.py b/packit/security.py
index 18f1f18b..dcc423cf 100644
--- a/packit/security.py
+++ b/packit/security.py
@@ -38,33 +38,29 @@ logger = logging.getLogger(__name__)
 class CommitVerifier:
     """
     Class used for verifying git commits. Uses python-gnupg for accessing the GPG binary.
-
-    Uses `gpg2` instead of the `gpg` if it exists.
     """
 
     def __init__(self, key_server: str = None) -> None:
         """
-        :param key_server: GPG key server to be used, defaults to keys.fedoraproject.org
+        :param key_server: GPG key server to be used
         """
         self._gpg: Optional[GPG] = None
-        self.key_server = key_server or "keys.fedoraproject.org"
+        if key_server:
+            self.key_servers = [key_server]
+        else:
+            self.key_servers = [
+                "keys.openpgp.org",
+                "pgp.mit.edu",
+                "keyserver.ubuntu.com",
+            ]
 
     @property
     def gpg(self) -> GPG:
         """
         gnupg.GPG instance from python-gnupg
-
-        Uses `gpg2` instead of the `gpg` if it exists.
         """
         if not self._gpg:
-            for gpg_location in ["gpg2", "gpg"]:
-                try:
-                    self._gpg = GPG(gpgbinary=gpg_location)
-                    break
-                except FileNotFoundError:
-                    continue
-            else:
-                raise PackitException("GPG binary not found.")
+            self._gpg = GPG()
         return self._gpg
 
     @property
@@ -77,34 +73,30 @@ class CommitVerifier:
         return self.gpg.list_keys()
 
     @property
-    def _gpg_fingerprints(self) -> str:
+    def _gpg_fingerprints(self) -> List[str]:
         """List of fingerprints of the saved keys"""
         return self._gpg_keys.fingerprints
 
-    def download_gpg_key_if_needed(self, key_fingerprint: str) -> None:
+    def download_gpg_key_if_needed(self, key_fingerprint: str) -> str:
         """
-        Download the gpg key from the self.key_server
-        if it is not present.
+        Download the gpg key from the self.key_servers if it is not present.
 
-        :param key_fingerprint: str (fingerprint of the gpg key)
+        :param key_fingerprint: fingerprint of the gpg key
         """
         if key_fingerprint in self._gpg_fingerprints:
-            return
+            return key_fingerprint
 
         try:
-            result = self.gpg.recv_keys(self.key_server, key_fingerprint)
-            if not result.fingerprints:
-                raise PackitException(f"Cannot receive a gpg key: {key_fingerprint}")
-
-        except ValueError as error:
-            # python-gnupg do not recognise KEY_CONSIDERED response from gpg2
-            if "KEY_CONSIDERED" not in str(error):
-                raise PackitException(
-                    f"Cannot receive a gpg key: {key_fingerprint}", error
-                )
+            for keyserver in self.key_servers:
+                logger.debug(f"Downloading {key_fingerprint} from {keyserver}")
+                result = self.gpg.recv_keys(keyserver, key_fingerprint)
+                if result.fingerprints:
+                    return result.fingerprints[0]
         except Exception as ex:
             raise PackitException(f"Cannot receive a gpg key: {key_fingerprint}", ex)
 
+        raise PackitException(f"Cannot receive a gpg key: {key_fingerprint}")
+
     def check_signature_of_commit(
         self, commit: git.Commit, possible_key_fingerprints: List[str]
     ) -> bool:


Test patch:
diff --git a/tests/unit/test_security.py b/tests/unit/test_security.py
index b7261c6d..11333085 100644
--- a/tests/unit/test_security.py
+++ b/tests/unit/test_security.py
@@ -101,7 +101,7 @@ def test_check_signature_of_commit(
         flexmock(fingerprints=local_keys)
     )
     gpg_flexmock.should_receive("recv_keys").and_return(
-        flexmock(fingerprints=flexmock())
+        flexmock(fingerprints=["fingerprint"])
     ).times(download_times)
 
     repo_mock = flexmock(
@@ -154,10 +154,10 @@ def test_check_signature_of_commit_not_present_key(
     gpg_flexmock = flexmock(GPG)
     gpg_flexmock.should_receive("list_keys").and_return(
         flexmock(fingerprints=local_keys)
-    ).and_return(flexmock(fingerprints=local_keys))
+    )
 
     gpg_flexmock.should_receive("recv_keys").and_return(
-        flexmock(fingerprints=flexmock())
+        flexmock(fingerprints=["fingerprint"])
     ).times(download_times)
 
     repo_mock = flexmock(
@@ -184,9 +184,7 @@ def test_check_signature_of_commit_key_not_found():
     gpg_flexmock.should_receive("list_keys").and_return(flexmock(fingerprints=[]))
 
     # No key received
-    gpg_flexmock.should_receive("recv_keys").and_return(
-        flexmock(fingerprints=[])
-    ).once()
+    gpg_flexmock.should_receive("recv_keys").and_return(flexmock(fingerprints=[]))
 
     # Signature cannot be checked
     repo_mock = flexmock(git=flexmock().should_receive("show").and_return("E").mock())
@@ -198,3 +196,17 @@ def test_check_signature_of_commit_key_not_found():
             possible_key_fingerprints=["a"],
         )
     assert "Cannot receive" in str(ex)
+
+
+# This could possibly but unlikely fail if all the default key servers are down.
+@pytest.mark.parametrize(
+    "keyid, ok",
+    [("A3E9A812AAB73DA7", True,), ("NOTEXISTING", False,)],  # Jirka's key id
+)
+def test_download_gpg_key_if_needed(keyid, ok):
+    cf = CommitVerifier()
+    if ok:
+        assert cf.download_gpg_key_if_needed(keyid)
+    else:
+        with pytest.raises(PackitException):
+            cf.download_gpg_key_if_needed(keyid)