launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/no-auto-key-retrieve into lp:launchpad.

Commit message:
Use our own GPG key retrieval implementation when verifying signatures rather than relying on auto-key-retrieve.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/no-auto-key-retrieve/+merge/364100

It's hard to verify this in the test suite, but this should avoid a current problem where we sometimes seem to tickle SKS into returning old keys with old expiration dates as a result of auto-key-retrieve using slightly different URLs.

This doesn't quite fix bug 3052 (!), but it should make it easier to fix.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/no-auto-key-retrieve into lp:launchpad.

=== modified file 'lib/lp/services/gpg/doc/gpg-signatures.txt'
--- lib/lp/services/gpg/doc/gpg-signatures.txt	2012-06-29 08:40:05 +0000
+++ lib/lp/services/gpg/doc/gpg-signatures.txt	2019-03-07 15:39:32 +0000
@@ -7,6 +7,10 @@
 
     >>> transaction.commit()
 
+    >>> from lp.testing.keyserver import KeyServerTac
+    >>> keyserver = KeyServerTac()
+    >>> keyserver.setUp()
+
     >>> from lp.testing import login
     >>> from lp.services.webapp.interfaces import ILaunchBag
 
@@ -164,7 +168,8 @@
     >>> gpghandler.getVerifiedSignature(content)
     Traceback (most recent call last):
     ...
-    GPGVerificationError: (7, 9, u'No public key')
+    GPGKeyDoesNotExistOnServer: GPG key E192C0543B1BB2EB does not exist on the
+    keyserver.
 
 Due to unpredictable behaviour between the production system and
 the external keyserver, we have a resilient signature verifier,
@@ -178,9 +183,9 @@
     Traceback (most recent call last):
     ...
     GPGVerificationError: Verification failed 3 times:
-    ["(7, 9, u'No public key')",
-     "(7, 9, u'No public key')",
-     "(7, 9, u'No public key')"]
+    ['GPG key E192C0543B1BB2EB does not exist on the keyserver.',
+     'GPG key E192C0543B1BB2EB does not exist on the keyserver.',
+     'GPG key E192C0543B1BB2EB does not exist on the keyserver.']
 
 
 Debugging exceptions
@@ -227,3 +232,4 @@
     [<gpgme.Signature object at ...>]
     7
 
+    >>> keyserver.tearDown()

=== modified file 'lib/lp/services/gpg/handler.py'
--- lib/lp/services/gpg/handler.py	2018-03-26 14:29:32 +0000
+++ lib/lp/services/gpg/handler.py	2019-03-07 15:39:32 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -99,13 +99,8 @@
         """
         self.home = tempfile.mkdtemp(prefix='gpg-')
         confpath = os.path.join(self.home, 'gpg.conf')
-        # set needed GPG options, 'auto-key-retrieve' is necessary for
-        # automatically retrieve from the keyserver unknown key when
-        # verifying signatures and 'no-auto-check-trustdb' avoid wasting
-        # time verifying the local keyring consistence.
         with open(confpath, 'w') as conf:
-            conf.write('keyserver hkp://%s\n' % config.gpghandler.host)
-            conf.write('keyserver-options auto-key-retrieve\n')
+            # Avoid wasting time verifying the local keyring's consistency.
             conf.write('no-auto-check-trustdb\n')
             # Prefer a SHA-2 hash where possible, otherwise GPG will fall
             # back to a hash it can use.
@@ -157,7 +152,7 @@
         for i in range(3):
             try:
                 signature = self.getVerifiedSignature(content, signature)
-            except GPGVerificationError as info:
+            except GPGKeyNotFoundError as info:
                 errors.append(info)
             else:
                 return signature
@@ -167,14 +162,13 @@
         raise GPGVerificationError(
             "Verification failed 3 times: %s " % stored_errors)
 
-    def getVerifiedSignature(self, content, signature=None):
-        """See IGPGHandler."""
-
-        assert not isinstance(content, unicode)
-        assert not isinstance(signature, unicode)
-
-        ctx = self._getContext()
-
+    def _rawVerifySignature(self, ctx, content, signature=None):
+        """Internals of `getVerifiedSignature`.
+
+        This is called twice during a typical verification: once to work out
+        the correct fingerprint, and once after retrieving the corresponding
+        key from the keyserver.
+        """
         # from `info gpgme` about gpgme_op_verify(SIG, SIGNED_TEXT, PLAIN):
         #
         # If SIG is a detached signature, then the signed text should be
@@ -228,22 +222,34 @@
             raise GPGVerificationError('Single signature expected, '
                                        'found multiple signatures')
 
-        signature = signatures[0]
+        return plain, signatures[0]
+
+    def getVerifiedSignature(self, content, signature=None):
+        """See IGPGHandler."""
+
+        assert not isinstance(content, unicode)
+        assert not isinstance(signature, unicode)
+
+        ctx = self._getContext()
+
+        # We may not yet have the public key, so find out the fingerprint we
+        # need to fetch.
+        _, sig = self._rawVerifySignature(ctx, content, signature=signature)
+
+        # Fetch the full key from the keyserver now that we know its
+        # fingerprint, and then verify the signature again.  (This also lets
+        # us support subkeys by using the master key fingerprint.)
+        key = self.retrieveKey(sig.fpr)
+        plain, sig = self._rawVerifySignature(
+            ctx, content, signature=signature)
+
         expired = False
-        # signature.status == 0 means "Ok"
-        if signature.status is not None:
-            if signature.status.code == gpgme.ERR_KEY_EXPIRED:
+        # sig.status == 0 means "Ok"
+        if sig.status is not None:
+            if sig.status.code == gpgme.ERR_KEY_EXPIRED:
                 expired = True
             else:
-                raise GPGVerificationError(signature.status.args)
-
-        # Support subkeys by retrieving the full key from the keyserver and
-        # using the master key fingerprint.
-        try:
-            key = self.retrieveKey(signature.fpr)
-        except GPGKeyNotFoundError:
-            raise GPGVerificationError(
-                "Unable to map subkey: %s" % signature.fpr)
+                raise GPGVerificationError(sig.status.args)
 
         if expired:
             # This should already be set, but let's make sure.
@@ -254,7 +260,7 @@
         return PymeSignature(
             fingerprint=key.fingerprint,
             plain_data=plain.getvalue(),
-            timestamp=signature.timestamp)
+            timestamp=sig.timestamp)
 
     def importPublicKey(self, content):
         """See IGPGHandler."""