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

Proposed by Colin Watson
Status: Merged
Merged at revision: 17952
Proposed branch: lp:~cjwatson/launchpad/digest-algo-sha512
Merge into: lp:launchpad
Diff against target: 158 lines (+48/-23)
2 files modified
lib/lp/services/gpg/handler.py (+9/-7)
lib/lp/services/gpg/tests/test_gpghandler.py (+39/-16)
To merge this branch: bzr merge lp:~cjwatson/launchpad/digest-algo-sha512
Reviewer Review Type Date Requested Status
William Grant code Approve
Review via email: mp+289052@code.launchpad.net

Commit message

Use SHA-512 digests for GPG signing where possible.

Description of the change

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.

To post a comment you must log in.
William Grant (wgrant) :
review: Approve (code)
Jason Gerard DeRose (jderose) wrote :

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) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/services/gpg/handler.py'
2--- lib/lp/services/gpg/handler.py 2016-03-16 00:34:56 +0000
3+++ lib/lp/services/gpg/handler.py 2016-03-18 00:51:12 +0000
4@@ -1,4 +1,4 @@
5-# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
6+# Copyright 2009-2016 Canonical Ltd. This software is licensed under the
7 # GNU Affero General Public License version 3 (see the file LICENSE).
8
9 __metaclass__ = type
10@@ -13,7 +13,6 @@
11 import atexit
12 import base64
13 import httplib
14-import json
15 import os
16 import shutil
17 import socket
18@@ -93,15 +92,18 @@
19 """
20 self.home = tempfile.mkdtemp(prefix='gpg-')
21 confpath = os.path.join(self.home, 'gpg.conf')
22- conf = open(confpath, 'w')
23 # set needed GPG options, 'auto-key-retrieve' is necessary for
24 # automatically retrieve from the keyserver unknown key when
25 # verifying signatures and 'no-auto-check-trustdb' avoid wasting
26 # time verifying the local keyring consistence.
27- conf.write('keyserver hkp://%s\n'
28- 'keyserver-options auto-key-retrieve\n'
29- 'no-auto-check-trustdb\n' % config.gpghandler.host)
30- conf.close()
31+ with open(confpath, 'w') as conf:
32+ conf.write('keyserver hkp://%s\n' % config.gpghandler.host)
33+ conf.write('keyserver-options auto-key-retrieve\n')
34+ conf.write('no-auto-check-trustdb\n')
35+ # Prefer a SHA-2 hash where possible, otherwise GPG will fall
36+ # back to a hash it can use.
37+ conf.write(
38+ 'personal-digest-preferences SHA512 SHA384 SHA256 SHA224\n')
39 # create a local atexit handler to remove the configuration directory
40 # on normal termination.
41
42
43=== modified file 'lib/lp/services/gpg/tests/test_gpghandler.py'
44--- lib/lp/services/gpg/tests/test_gpghandler.py 2016-03-17 04:05:36 +0000
45+++ lib/lp/services/gpg/tests/test_gpghandler.py 2016-03-18 00:51:12 +0000
46@@ -1,28 +1,22 @@
47-# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
48+# Copyright 2009-2016 Canonical Ltd. This software is licensed under the
49 # GNU Affero General Public License version 3 (see the file LICENSE).
50
51+import os
52 import random
53 import string
54+import subprocess
55
56+import gpgme
57 from testtools.matchers import (
58- Contains,
59 ContainsDict,
60 Equals,
61 HasLength,
62- Not,
63 raises,
64 )
65 from zope.component import getUtility
66 from zope.security.proxy import removeSecurityProxy
67
68-from lp.registry.interfaces.gpg import IGPGKeySet
69-from lp.services.config.fixture import (
70- ConfigFixture,
71- ConfigUseFixture,
72- )
73-from lp.services.gpg.handler import GPGClient
74 from lp.services.gpg.interfaces import (
75- GPGKeyAlgorithm,
76 GPGKeyDoesNotExistOnServer,
77 GPGKeyTemporarilyNotFoundError,
78 GPGServiceException,
79@@ -30,7 +24,6 @@
80 IGPGHandler,
81 )
82 from lp.services.log.logger import BufferLogger
83-from lp.services.openid.model.openididentifier import OpenIdIdentifier
84 from lp.services.timeout import (
85 get_default_timeout_function,
86 set_default_timeout_function,
87@@ -49,7 +42,6 @@
88 test_keyrings,
89 test_pubkey_from_email,
90 )
91-from lp.testing.gpgservice import GPGKeyServiceFixture
92 from lp.testing.keyserver import KeyServerTac
93 from lp.testing.layers import (
94 GPGServiceLayer,
95@@ -58,13 +50,13 @@
96 )
97
98
99-class TestImportKeyRing(TestCase):
100- """Tests for keyring imports"""
101+class TestGPGHandler(TestCase):
102+ """Unit tests for the GPG handler."""
103 layer = LaunchpadFunctionalLayer
104
105 def setUp(self):
106 """Get a gpghandler and login"""
107- super(TestImportKeyRing, self).setUp()
108+ super(TestGPGHandler, self).setUp()
109 login(ANONYMOUS)
110 self.gpg_handler = getUtility(IGPGHandler)
111 self.gpg_handler.resetLocalState()
112@@ -75,7 +67,7 @@
113 # This should be a zope test cleanup thing per SteveA.
114 self.gpg_handler.resetLocalState()
115 logout()
116- super(TestImportKeyRing, self).tearDown()
117+ super(TestGPGHandler, self).tearDown()
118
119 def populateKeyring(self):
120 for email in iter_test_key_emails():
121@@ -213,6 +205,37 @@
122 GPGKeyDoesNotExistOnServer,
123 removeSecurityProxy(self.gpg_handler)._getPubKey, fingerprint)
124
125+ def test_signContent_uses_sha512_digests(self):
126+ secret_keys = [
127+ ("ppa-sample@canonical.com.sec", ""), # 1024R
128+ ("ppa-sample-4096@canonical.com.sec", ""), # 4096R
129+ ]
130+ for key_name, password in secret_keys:
131+ self.gpg_handler.resetLocalState()
132+ secret_key = import_secret_test_key(key_name)
133+ content = "abc\n"
134+ signed_content = self.gpg_handler.signContent(
135+ content, secret_key.fingerprint, password)
136+ signature = self.gpg_handler.getVerifiedSignature(signed_content)
137+ self.assertEqual(content, signature.plain_data)
138+ self.assertEqual(secret_key.fingerprint, signature.fingerprint)
139+ # pygpgme doesn't tell us the hash algorithm used for a verified
140+ # signature, so we have to do this by hand. Sending --status-fd
141+ # output to stdout is a bit dodgy, but at least with --quiet
142+ # it's OK for test purposes and it simplifies subprocess
143+ # plumbing.
144+ with open(os.devnull, "w") as devnull:
145+ gpg_proc = subprocess.Popen(
146+ ["gpg", "--quiet", "--status-fd", "1", "--verify"],
147+ stdin=subprocess.PIPE, stdout=subprocess.PIPE,
148+ stderr=devnull, universal_newlines=True)
149+ status = gpg_proc.communicate(signed_content)[0].splitlines()
150+ validsig_prefix = "[GNUPG:] VALIDSIG "
151+ [validsig_line] = [
152+ line for line in status if line.startswith(validsig_prefix)]
153+ validsig_tokens = validsig_line[len(validsig_prefix):].split()
154+ self.assertEqual(gpgme.MD_SHA512, int(validsig_tokens[7]))
155+
156
157 class GPGServiceZopelessLayer(ZopelessDatabaseLayer, GPGServiceLayer):
158 """A layer specifically for running the IGPGClient utility tests."""