launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/digest-algo-sha512 into lp:launchpad.

Commit message:
Use SHA-512 digests for GPG signing where possible.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1556666 in Launchpad itself: "PPA (In)Release files use SHA1 digests for GPG signature"
  https://bugs.launchpad.net/launchpad/+bug/1556666

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/digest-algo-sha512/+merge/289052

Use SHA-512 digests for GPG signing where possible.

This may not work for 1024-bit DSA keys, but it will at least still continue to sign content as gpg falls back to a digest it can use.  As far as I can tell, all the key types we've ever used as PPA signing keys can use SHA-512 digests.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/digest-algo-sha512 into lp:launchpad.

=== modified file 'lib/lp/services/gpg/handler.py'
--- lib/lp/services/gpg/handler.py	2016-02-18 19:31:17 +0000
+++ lib/lp/services/gpg/handler.py	2016-03-15 14:31:55 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -92,15 +92,18 @@
         """
         self.home = tempfile.mkdtemp(prefix='gpg-')
         confpath = os.path.join(self.home, 'gpg.conf')
-        conf = open(confpath, 'w')
         # 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.
-        conf.write('keyserver hkp://%s\n'
-                   'keyserver-options auto-key-retrieve\n'
-                   'no-auto-check-trustdb\n' % config.gpghandler.host)
-        conf.close()
+        with open(confpath, 'w') as conf:
+            conf.write('keyserver hkp://%s\n' % config.gpghandler.host)
+            conf.write('keyserver-options auto-key-retrieve\n')
+            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.
+            conf.write(
+                'personal-digest-preferences SHA512 SHA384 SHA256 SHA224\n')
         # create a local atexit handler to remove the configuration directory
         # on normal termination.
 

=== modified file 'lib/lp/services/gpg/tests/test_gpghandler.py'
--- lib/lp/services/gpg/tests/test_gpghandler.py	2016-03-15 08:21:10 +0000
+++ lib/lp/services/gpg/tests/test_gpghandler.py	2016-03-15 14:31:55 +0000
@@ -1,6 +1,7 @@
-# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+<<<<<<< TREE
 import random
 import string
 
@@ -12,6 +13,12 @@
     Not,
     raises,
     )
+=======
+import os
+import subprocess
+
+import gpgme
+>>>>>>> MERGE-SOURCE
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
@@ -54,13 +61,13 @@
     )
 
 
-class TestImportKeyRing(TestCase):
-    """Tests for keyring imports"""
+class TestGPGHandler(TestCase):
+    """Unit tests for the GPG handler."""
     layer = LaunchpadFunctionalLayer
 
     def setUp(self):
         """Get a gpghandler and login"""
-        super(TestImportKeyRing, self).setUp()
+        super(TestGPGHandler, self).setUp()
         login(ANONYMOUS)
         self.gpg_handler = getUtility(IGPGHandler)
         self.gpg_handler.resetLocalState()
@@ -71,7 +78,7 @@
         # This should be a zope test cleanup thing per SteveA.
         self.gpg_handler.resetLocalState()
         logout()
-        super(TestImportKeyRing, self).tearDown()
+        super(TestGPGHandler, self).tearDown()
 
     def populateKeyring(self):
         for email in iter_test_key_emails():
@@ -208,6 +215,7 @@
         self.assertRaises(
             GPGKeyDoesNotExistOnServer,
             removeSecurityProxy(self.gpg_handler)._getPubKey, fingerprint)
+<<<<<<< TREE
 
 
 class GPGServiceZopelessLayer(ZopelessLayer, GPGServiceLayer):
@@ -354,3 +362,36 @@
             lambda: client.disableKeyForOwner(self.get_random_owner_id_string(), ''),
             raises(ValueError("Invalid fingerprint: ''."))
         )
+=======
+
+    def test_signContent_uses_sha512_digests(self):
+        secret_keys = [
+            ("ppa-sample@xxxxxxxxxxxxxxxxx", ""),       # 1024R
+            ("ppa-sample-4096@xxxxxxxxxxxxxxxxx", ""),  # 4096R
+            ]
+        for key_name, password in secret_keys:
+            self.gpg_handler.resetLocalState()
+            secret_key = import_secret_test_key(key_name)
+            content = "abc\n"
+            signed_content = self.gpg_handler.signContent(
+                content, secret_key.fingerprint, password)
+            signature = self.gpg_handler.getVerifiedSignature(signed_content)
+            self.assertEqual(content, signature.plain_data)
+            self.assertEqual(secret_key.fingerprint, signature.fingerprint)
+            # pygpgme doesn't tell us the hash algorithm used for a verified
+            # signature, so we have to do this by hand.  Sending --status-fd
+            # output to stdout is a bit dodgy, but at least with --quiet
+            # it's OK for test purposes and it simplifies subprocess
+            # plumbing.
+            with open(os.devnull, "w") as devnull:
+                gpg_proc = subprocess.Popen(
+                    ["gpg", "--quiet", "--status-fd", "1", "--verify"],
+                    stdin=subprocess.PIPE, stdout=subprocess.PIPE,
+                    stderr=devnull, universal_newlines=True)
+            status = gpg_proc.communicate(signed_content)[0].splitlines()
+            validsig_prefix = "[GNUPG:] VALIDSIG "
+            [validsig_line] = [
+                line for line in status if line.startswith(validsig_prefix)]
+            validsig_tokens = validsig_line[len(validsig_prefix):].split()
+            self.assertEqual(gpgme.MD_SHA512, int(validsig_tokens[7]))
+>>>>>>> MERGE-SOURCE