launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/check-keys-from-keyserver into lp:launchpad.

Commit message:
Check fingerprints of keys received from the keyserver rather than trusting it implicitly.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/check-keys-from-keyserver/+merge/315677
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/check-keys-from-keyserver into lp:launchpad.

=== modified file 'lib/lp/services/gpg/handler.py'
--- lib/lp/services/gpg/handler.py	2017-01-12 15:03:09 +0000
+++ lib/lp/services/gpg/handler.py	2017-01-26 13:47:32 +0000
@@ -33,6 +33,7 @@
     GPGKeyAlgorithm,
     GPGKeyDoesNotExistOnServer,
     GPGKeyExpired,
+    GPGKeyMismatchOnServer,
     GPGKeyNotFoundError,
     GPGKeyRevoked,
     GPGKeyTemporarilyNotFoundError,
@@ -411,6 +412,10 @@
         if not key.exists_in_local_keyring:
             pubkey = self._getPubKey(fingerprint)
             key = self.importPublicKey(pubkey)
+            if fingerprint != key.fingerprint:
+                ctx = self._getContext()
+                ctx.delete(key.key)
+                raise GPGKeyMismatchOnServer(fingerprint, key.fingerprint)
         return key
 
     def retrieveActiveKey(self, fingerprint):

=== modified file 'lib/lp/services/gpg/interfaces.py'
--- lib/lp/services/gpg/interfaces.py	2016-11-03 15:19:01 +0000
+++ lib/lp/services/gpg/interfaces.py	2017-01-26 13:47:32 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __all__ = [
@@ -7,6 +7,7 @@
     'GPGKeyExpired',
     'GPGKeyNotFoundError',
     'GPGKeyRevoked',
+    'GPGKeyMismatchOnServer',
     'GPGKeyTemporarilyNotFoundError',
     'GPGUploadFailure',
     'GPGVerificationError',
@@ -154,6 +155,20 @@
         super(GPGKeyExpired, self).__init__("%s has expired" % (key.keyid, ))
 
 
+class GPGKeyMismatchOnServer(Exception):
+    """The keyserver returned the wrong key for a given fingerprint.
+
+    This may indicate a keyserver compromise.
+    """
+    def __init__(self, expected_fingerprint, keyserver_fingerprint):
+        self.expected_fingerprint = expected_fingerprint
+        self.keyserver_fingerprint = keyserver_fingerprint
+        message = (
+            "The keyserver returned the wrong key: expected %s, got %s." %
+            (expected_fingerprint, keyserver_fingerprint))
+        super(GPGKeyMismatchOnServer, self).__init__(message)
+
+
 class SecretGPGKeyImportDetected(Exception):
     """An attempt to import a secret GPG key."""
 

=== modified file 'lib/lp/services/gpg/tests/test_gpghandler.py'
--- lib/lp/services/gpg/tests/test_gpghandler.py	2016-11-03 15:07:36 +0000
+++ lib/lp/services/gpg/tests/test_gpghandler.py	2017-01-26 13:47:32 +0000
@@ -1,8 +1,9 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 import base64
 import os
+import shutil
 import subprocess
 
 import gpgme
@@ -11,6 +12,7 @@
 
 from lp.services.gpg.interfaces import (
     GPGKeyDoesNotExistOnServer,
+    GPGKeyMismatchOnServer,
     GPGKeyTemporarilyNotFoundError,
     IGPGHandler,
     )
@@ -29,6 +31,7 @@
     import_secret_test_key,
     iter_test_key_emails,
     test_keyrings,
+    test_pubkey_file_from_email,
     test_pubkey_from_email,
     )
 from lp.testing.keyserver import KeyServerTac
@@ -176,6 +179,20 @@
         finally:
             set_default_timeout_function(old_timeout_function)
 
+    def test_retrieveKey_checks_fingerprint(self):
+        # retrieveKey ensures that the key fetched from the keyserver has
+        # the correct fingerprint.
+        keyserver = self.useFixture(KeyServerTac())
+        fingerprint = "340CA3BB270E2716C9EE0B768E7EB7086C64A8C5"
+        # Associate a different key with this fingerprint.
+        shutil.copy2(
+            test_pubkey_file_from_email("test@xxxxxxxxxxxxx"),
+            os.path.join(keyserver.root, "0x%s.get" % fingerprint))
+        gpghandler = getUtility(IGPGHandler)
+        self.assertRaises(
+            GPGKeyMismatchOnServer, gpghandler.retrieveKey, fingerprint)
+        self.assertEqual([], list(gpghandler.localKeys()))
+
     def test_uploadPublicKey_suppress_in_config(self):
         self.useFixture(KeyServerTac())
         logger = BufferLogger()