Merge lp:~cjwatson/launchpad/digest-algo-sha512 into lp:launchpad

[Colin Watson]

Use SHA-512 digests for GPG signing where possible.

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.

[William Grant (wgrant)]

review: Approve (code)

[Jason Gerard DeRose (jderose)]

Colin - would you consider using SHA-384 instead?

Like SHA-512, SHA-384 uses the same 512-bit internal state size, but because SHA-384 only exposes 384 bits of of this internal state in the digest, it's immune to length extension attacks:

https://en.wikipedia.org/wiki/Length_extension_attack

From page 87 of *Cryptography Engineering* by Niels Ferguson, Bruce Schneier, and Tadayoshi Kohno:

"""
There is another fix to some of these weaknesses with the SHA-2 family of iterative hash functions: Truncate the output. If a hash function produces n-bit outputs, only use the first n - s bit as the hash value for some positive s. In fact, SHA-224 and SHA-384 both already do this; SHA-224 is roughly SHA-256 with 32 output bits dropped, and SHA-384 is roughly SHA-512 with 128 output bits dropped.
"""

They ultimately recommend using SHA-512 truncated to 256 bits (if you're going to use a SHA-2 hash), but that's not feasible in this case as GPG doesn't support truncated SHA-512.

In terms of standard SHA-2 family hash functions supported by GPG, SHA-384 is the best option in my opinion.

Note that I don't recommend SHA-224 as its 256-bit internal state size is too small by modern standards. SHA-384/SHA-512 are also considerably faster than SHA-224/SHA-256 when it comes to 64-bit software implementations.

[William Grant (wgrant)]

We avoided SHA-512 in Packages/Sources due to data size, and SHA-384 wasn't an option there. But it would be a possibility for OpenPGP signatures.

=== modified file 'lib/lp/services/gpg/handler.py'
--- lib/lp/services/gpg/handler.py	2016-03-16 00:34:56 +0000
+++ lib/lp/services/gpg/handler.py	2016-03-18 00:51:12 +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
@@ -13,7 +13,6 @@
 import atexit
 import base64
 import httplib
-import json
 import os
 import shutil
 import socket
@@ -93,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-17 04:05:36 +0000
+++ lib/lp/services/gpg/tests/test_gpghandler.py	2016-03-18 00:51:12 +0000
@@ -1,28 +1,22 @@
-# 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).
 
+import os
 import random
 import string
+import subprocess
 
+import gpgme
 from testtools.matchers import (
-    Contains,
     ContainsDict,
     Equals,
     HasLength,
-    Not,
     raises,
     )
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
-from lp.registry.interfaces.gpg import IGPGKeySet
-from lp.services.config.fixture import (
-    ConfigFixture,
-    ConfigUseFixture,
-    )
-from lp.services.gpg.handler import GPGClient
 from lp.services.gpg.interfaces import (
-    GPGKeyAlgorithm,
     GPGKeyDoesNotExistOnServer,
     GPGKeyTemporarilyNotFoundError,
     GPGServiceException,
@@ -30,7 +24,6 @@
     IGPGHandler,
     )
 from lp.services.log.logger import BufferLogger
-from lp.services.openid.model.openididentifier import OpenIdIdentifier
 from lp.services.timeout import (
     get_default_timeout_function,
     set_default_timeout_function,
@@ -49,7 +42,6 @@
     test_keyrings,
     test_pubkey_from_email,
     )
-from lp.testing.gpgservice import GPGKeyServiceFixture
 from lp.testing.keyserver import KeyServerTac
 from lp.testing.layers import (
     GPGServiceLayer,
@@ -58,13 +50,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()
@@ -75,7 +67,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():
@@ -213,6 +205,37 @@
             GPGKeyDoesNotExistOnServer,
             removeSecurityProxy(self.gpg_handler)._getPubKey, fingerprint)
 
+    def test_signContent_uses_sha512_digests(self):
+        secret_keys = [
+            ("[e-mail]", ""),       # 1024R
+            ("[e-mail]", ""),  # 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]))
+
 
 class GPGServiceZopelessLayer(ZopelessDatabaseLayer, GPGServiceLayer):
     """A layer specifically for running the IGPGClient utility tests."""