Merge lp:~jelmer/brz/gpg-detached-sign into lp:brz

[Jelmer Vernooij]

Add support for passing mode argument to GPGSignature.sign.

Add support for passing mode argument to GPGSignature.sign.

This is necessary for brz-git, which needs to create detached signatures.

[Martin Packman (gz)]

Thanks, so questions answered, constants are the same as the mapping, but the breezy interface doesn't depend on gpg module being installed. Also, argument not defaulted as bzr wants clear but git needs detached, so better to just pass.

review: Approve

[The Breezy Bot (the-breezy-bot)]

Running landing tests failed
https://ci.breezy-vcs.org/job/brz-dev/41/

=== modified file 'breezy/bzr/remote.py'
--- breezy/bzr/remote.py	2017-11-23 20:02:55 +0000
+++ breezy/bzr/remote.py	2018-03-24 01:56:28 +0000
@@ -2692,7 +2692,7 @@
 
     def store_revision_signature(self, gpg_strategy, plaintext, revision_id):
         with self.lock_write():
-            signature = gpg_strategy.sign(plaintext)
+            signature = gpg_strategy.sign(plaintext, gpg.MODE_CLEAR)
             self.add_signature_text(revision_id, signature)
 
     def add_signature_text(self, revision_id, signature):
@@ -2737,9 +2737,11 @@
             signature = self.get_signature_text(revision_id)
 
             testament = _mod_testament.Testament.from_revision(self, revision_id)
-            plaintext = testament.as_short_text()
 
-            return gpg_strategy.verify(signature, plaintext)
+            (status, key, signed_plaintext) = gpg_strategy.verify(signature)
+            if testament.as_short_text() != signed_plaintext:
+                return gpg.SIGNATURE_NOT_VALID, None
+            return (status, key)
 
     def item_keys_introduced_by(self, revision_ids, _files_pb=None):
         self._ensure_real()
 
=== modified file 'breezy/gpg.py'
--- breezy/gpg.py	2018-02-24 15:50:23 +0000
+++ breezy/gpg.py	2018-03-24 01:56:28 +0000
@@ -52,6 +52,10 @@
 SIGNATURE_NOT_SIGNED = 3
 SIGNATURE_EXPIRED = 4
 
+MODE_NORMAL = 0
+MODE_DETACH = 1
+MODE_CLEAR = 2
+
 
 class GpgNotInstalled(errors.DependencyNotPresent):
 
@@ -123,10 +127,10 @@
     def __init__(self, ignored):
         """Real strategies take a configuration."""
 
-    def sign(self, content):
+    def sign(self, content, mode):
         raise SigningFailed('Signing is disabled.')
 
-    def verify(self, content, testament):
+    def verify(self, signed_data, signature=None):
         raise SignatureVerificationFailed('Signature verification is \
 disabled.')
 
@@ -146,12 +150,14 @@
     def __init__(self, ignored):
         """Real strategies take a configuration."""
 
-    def sign(self, content):
+    def sign(self, content, mode):
         return ("-----BEGIN PSEUDO-SIGNED CONTENT-----\n" + content +
                 "-----END PSEUDO-SIGNED CONTENT-----\n")
 
-    def verify(self, content, testament):
-        return SIGNATURE_VALID, None
+    def verify(self, signed_data, signature=None):
+        plain_text = signed_data.replace("-----BEGIN PSEUDO-SIGNED CONTENT-----\n", "")
+        plain_text = plain_text.replace("-----END PSEUDO-SIGNED CONTENT-----\n", "")
+        return SIGNATURE_VALID, None, plain_text
 
     def set_acceptable_keys(self, command_line_input):
         if command_line_input is not None:
@@ -187,11 +193,11 @@
         try:
             import gpg
             self.context = gpg.Context()
+            self.context.armor = True
+            self.context.signers = self._get_signing_keys()
         except ImportError as error:
             pass # can't use verify()
 
-        self.context.signers = self._get_signing_keys()
-
     def _get_signing_keys(self):
         import gpg
         keyname = self._config_stack.get('gpg_signing_key')
@@ -224,7 +230,7 @@
         except ImportError as error:
             return False
 
-    def sign(self, content):
+    def sign(self, content, mode):
         import gpg
         if isinstance(content, unicode):
             raise errors.BzrBadParameterUnicode('content')
@@ -232,29 +238,34 @@
         plain_text = gpg.Data(content)
         try:
             output, result = self.context.sign(
-                plain_text, mode=gpg.constants.sig.mode.CLEAR)
+                plain_text, mode={
+                    MODE_DETACH: gpg.constants.sig.mode.DETACH,
+                    MODE_CLEAR: gpg.constants.sig.mode.CLEAR,
+                    MODE_NORMAL: gpg.constants.sig.mode.NORMAL,
+                    }[mode])
         except gpg.errors.GPGMEError as error:
             raise SigningFailed(str(error))
 
         return output
 
-    def verify(self, content, testament):
+    def verify(self, signed_data, signature=None):
         """Check content has a valid signature.
 
-        :param content: the commit signature
-        :param testament: the valid testament string for the commit
+        :param signed_data; Signed data
+        :param signature: optional signature (if detached)
 
-        :return: SIGNATURE_VALID or a failed SIGNATURE_ value, key uid if valid
+        :return: SIGNATURE_VALID or a failed SIGNATURE_ value, key uid if valid, plain text
         """
         try:
             import gpg
         except ImportError as error:
             raise errors.GpgNotInstalled(error)
 
-        signature = gpg.Data(content)
-        sink = gpg.Data()
+        signed_data = gpg.Data(signed_data)
+        if signature:
+            signature = gpg.Data(signature)
         try:
-            plain_output, result = self.context.verify(signature)
+            plain_output, result = self.context.verify(signed_data, signature)
         except gpg.errors.BadSignatures as error:
             fingerprint = error.result.signatures[0].fpr
             if error.result.signatures[0].summary & gpg.constants.SIGSUM_KEY_EXPIRED:
@@ -262,35 +273,32 @@
                 if expires > error.result.signatures[0].timestamp:
                     # The expired key was not expired at time of signing.
                     # test_verify_expired_but_valid()
-                    return SIGNATURE_EXPIRED, fingerprint[-8:]
+                    return SIGNATURE_EXPIRED, fingerprint[-8:], None
                 else:
                     # I can't work out how to create a test where the signature
                     # was expired at the time of signing.
-                    return SIGNATURE_NOT_VALID, None
+                    return SIGNATURE_NOT_VALID, None, None
 
             # GPG does not know this key.
             # test_verify_unknown_key()
             if error.result.signatures[0].summary & gpg.constants.SIGSUM_KEY_MISSING:
-                return SIGNATURE_KEY_MISSING, fingerprint[-8:]
+                return SIGNATURE_KEY_MISSING, fingerprint[-8:], None
 
-            return SIGNATURE_NOT_VALID, None
+            return SIGNATURE_NOT_VALID, None, None
         except gpg.errors.GPGMEError as error:
-            raise SignatureVerificationFailed(error[2])
+            raise SignatureVerificationFailed(error)
 
         # No result if input is invalid.
         # test_verify_invalid()
         if len(result.signatures) == 0:
-            return SIGNATURE_NOT_VALID, None
+            return SIGNATURE_NOT_VALID, None, plain_output
+
         # User has specified a list of acceptable keys, check our result is in
         # it.  test_verify_unacceptable_key()
         fingerprint = result.signatures[0].fpr
         if self.acceptable_keys is not None:
             if not fingerprint in self.acceptable_keys:
-                return SIGNATURE_KEY_MISSING, fingerprint[-8:]
-        # Check the signature actually matches the testament.
-        # test_verify_bad_testament()
-        if testament != plain_output:
-            return SIGNATURE_NOT_VALID, None
+                return SIGNATURE_KEY_MISSING, fingerprint[-8:], plain_output
         # Yay gpg set the valid bit.
         # Can't write a test for this one as you can't set a key to be
         # trusted using gpg.
@@ -298,20 +306,20 @@
             key = self.context.get_key(fingerprint)
             name = key.uids[0].name
             email = key.uids[0].email
-            return SIGNATURE_VALID, name + " <" + email + ">"
+            return SIGNATURE_VALID, name.decode('utf-8') + u" <" + email.decode('utf-8') + u">", plain_output
         # Sigsum_red indicates a problem, unfortunatly I have not been able
         # to write any tests which actually set this.
         if result.signatures[0].summary & gpg.constants.SIGSUM_RED:
-            return SIGNATURE_NOT_VALID, None
+            return SIGNATURE_NOT_VALID, None, plain_output
         # Summary isn't set if sig is valid but key is untrusted but if user
         # has explicity set the key as acceptable we can validate it.
         if result.signatures[0].summary == 0 and self.acceptable_keys is not None:
             if fingerprint in self.acceptable_keys:
                 # test_verify_untrusted_but_accepted()
-                return SIGNATURE_VALID, None
+                return SIGNATURE_VALID, None, plain_output
         # test_verify_valid_but_untrusted()
         if result.signatures[0].summary == 0 and self.acceptable_keys is None:
-            return SIGNATURE_NOT_VALID, None
+            return SIGNATURE_NOT_VALID, None, plain_output
         # Other error types such as revoked keys should (I think) be caught by
         # SIGSUM_RED so anything else means something is buggy.
         raise SignatureVerificationFailed(
 
=== modified file 'breezy/merge_directive.py'
--- breezy/merge_directive.py	2017-10-27 00:18:42 +0000
+++ breezy/merge_directive.py	2018-03-24 01:56:28 +0000
@@ -251,7 +251,7 @@
         :return: a string
         """
         my_gpg = gpg.GPGStrategy(branch.get_config_stack())
-        return my_gpg.sign(''.join(self.to_lines()))
+        return my_gpg.sign(''.join(self.to_lines()), gpg.MODE_CLEAR)
 
     def to_email(self, mail_to, branch, sign=False):
         """Serialize as an email message.
 
=== modified file 'breezy/repository.py'
--- breezy/repository.py	2018-02-24 15:50:23 +0000
+++ breezy/repository.py	2018-03-24 01:56:28 +0000
@@ -887,7 +887,7 @@
 
     def store_revision_signature(self, gpg_strategy, plaintext, revision_id):
         with self.lock_write():
-            signature = gpg_strategy.sign(plaintext)
+            signature = gpg_strategy.sign(plaintext, gpg.MODE_CLEAR)
             self.add_signature_text(revision_id, signature)
 
     def add_signature_text(self, revision_id, signature):
@@ -1104,11 +1104,12 @@
                 return gpg.SIGNATURE_NOT_SIGNED, None
             signature = self.get_signature_text(revision_id)
 
-            testament = _mod_testament.Testament.from_revision(
-                    self, revision_id)
-            plaintext = testament.as_short_text()
+            testament = _mod_testament.Testament.from_revision(self, revision_id)
 
-            return gpg_strategy.verify(signature, plaintext)
+            (status, key, signed_plaintext) = gpg_strategy.verify(signature)
+            if testament.as_short_text() != signed_plaintext:
+                return gpg.SIGNATURE_NOT_VALID, None
+            return (status, key)
 
     def verify_revision_signatures(self, revision_ids, gpg_strategy):
         """Verify revision signatures for a number of revisions.
 
=== modified file 'breezy/tests/per_repository/test_signatures.py'
--- breezy/tests/per_repository/test_signatures.py	2017-08-07 19:09:47 +0000
+++ breezy/tests/per_repository/test_signatures.py	2018-03-24 01:56:28 +0000
@@ -123,7 +123,7 @@
                          '-----END PSEUDO-SIGNED CONTENT-----\n',
                          repo.get_signature_text(a))
         self.assertEqual(
-            (gpg.SIGNATURE_VALID, None, ),
+            (gpg.SIGNATURE_VALID, None),
             repo.verify_revision_signature(a, strategy))
 
     def test_verify_revision_signatures(self):
 
=== modified file 'breezy/tests/test_commit.py'
--- breezy/tests/test_commit.py	2018-02-15 19:38:33 +0000
+++ breezy/tests/test_commit.py	2018-03-24 01:56:28 +0000
@@ -435,7 +435,8 @@
                 message="base", allow_pointless=True, rev_id='B',
                 working_tree=wt)
             def sign(text):
-                return breezy.gpg.LoopbackGPGStrategy(None).sign(text)
+                return breezy.gpg.LoopbackGPGStrategy(None).sign(
+                        text, breezy.gpg.MODE_CLEAR)
             self.assertEqual(sign(Testament.from_revision(branch.repository,
                                                           'B').as_short_text()),
                              branch.repository.get_signature_text('B'))
 
=== modified file 'breezy/tests/test_gpg.py'
--- breezy/tests/test_gpg.py	2017-07-04 20:03:11 +0000
+++ breezy/tests/test_gpg.py	2018-03-24 01:56:28 +0000
@@ -224,8 +224,7 @@
 """
         my_gpg = gpg.GPGStrategy(FakeConfig())
         my_gpg.set_acceptable_keys("[e-mail]")
-        self.assertEqual((gpg.SIGNATURE_VALID, None), my_gpg.verify(content,
-                            plain))
+        self.assertEqual((gpg.SIGNATURE_VALID, None, plain), my_gpg.verify(content))
 
     def test_verify_unacceptable_key(self):
         self.requireFeature(features.gpg)
@@ -255,8 +254,8 @@
 """
         my_gpg = gpg.GPGStrategy(FakeConfig())
         my_gpg.set_acceptable_keys("[e-mail]")
-        self.assertEqual((gpg.SIGNATURE_KEY_MISSING, u'E3080E45'),
-                         my_gpg.verify(content, plain))
+        self.assertEqual((gpg.SIGNATURE_KEY_MISSING, u'E3080E45', plain),
+                         my_gpg.verify(content))
 
     def test_verify_valid_but_untrusted(self):
         self.requireFeature(features.gpg)
@@ -285,40 +284,7 @@
 sha1: 6411f9bdf6571200357140c9ce7c0f50106ac9a4
 """
         my_gpg = gpg.GPGStrategy(FakeConfig())
-        self.assertEqual((gpg.SIGNATURE_NOT_VALID, None), my_gpg.verify(content,
-                            plain))
-
-    def test_verify_bad_testament(self):
-        self.requireFeature(features.gpg)
-        self.import_keys()
-
-        content = """-----BEGIN PGP SIGNED MESSAGE-----
-Hash: SHA1
-
-bazaar-ng testament short form 1
-revision-id: [e-mail]-20110527185938-hluafawphszb8dl1
-sha1: 6411f9bdf6571200357140c9ce7c0f50106ac9a4
------BEGIN PGP SIGNATURE-----
-Version: GnuPG v1.4.11 (GNU/Linux)
-
-iQEcBAEBAgAGBQJN+ekFAAoJEIdoGx7jCA5FGtEH/i+XxJRvqU6wdBtLVrGBMAGk
-FZ5VP+KyXYtymSbgSstj/vM12NeMIeFs3xGnNnYuX1MIcY6We5TKtCH0epY6ym5+
-6g2Q2QpQ5/sT2d0mWzR0K4uVngmxVQaXTdk5PdZ40O7ULeDLW6CxzxMHyUL1rsIx
-7UBUTBh1O/1n3ZfD99hUkm3hVcnsN90uTKH59zV9NWwArU0cug60+5eDKJhSJDbG
-rIwlqbFAjDZ7L/48e+IaYIJwBZFzMBpJKdCxzALLtauMf+KK8hGiL2hrRbWm7ty6
-NgxfkMYOB4rDPdSstT35N+5uBG3n/UzjxHssi0svMfVETYYX40y57dm2eZQXFp8=
-=iwsn
------END PGP SIGNATURE-----
-"""
-        plain = """bazaar-ng testament short form 1
-revision-id: [e-mail]-20110527185938-hluafawphszb8dl1
-sha1: 6411f9bdf6571200357140c9ce7c0f50106ac9a4
-"""
-        my_gpg = gpg.GPGStrategy(FakeConfig())
-        my_gpg.set_acceptable_keys("[e-mail]")
-        self.assertEqual((gpg.SIGNATURE_NOT_VALID, None), my_gpg.verify(content,
-                            plain))
-
+        self.assertEqual((gpg.SIGNATURE_NOT_VALID, None, plain), my_gpg.verify(content))
 
     def test_verify_revoked_signature(self):
         self.requireFeature(features.gpg)
@@ -341,8 +307,7 @@
         plain = """asdf\n"""
         my_gpg = gpg.GPGStrategy(FakeConfig())
         my_gpg.set_acceptable_keys("[e-mail]")
-        self.assertEqual((gpg.SIGNATURE_NOT_VALID, None), my_gpg.verify(content,
-                            plain))
+        self.assertEqual((gpg.SIGNATURE_NOT_VALID, None, None), my_gpg.verify(content))
 
     def test_verify_invalid(self):
         self.requireFeature(features.gpg)
@@ -366,8 +331,8 @@
 sha1: 6411f9bdf6571200357140c9ce7c0f50106ac9a4
 """
         my_gpg = gpg.GPGStrategy(FakeConfig())
-        self.assertEqual((gpg.SIGNATURE_NOT_VALID, None),
-                            my_gpg.verify(content, plain))
+        self.assertEqual((gpg.SIGNATURE_NOT_VALID, None, plain),
+                            my_gpg.verify(content))
 
     def test_verify_expired_but_valid(self):
         self.requireFeature(features.gpg)
@@ -388,13 +353,9 @@
 =uHen
 -----END PGP SIGNATURE-----
 """
-        plain = """bazaar-ng testament short form 1
-revision-id: [e-mail]-20110801100657-f1dr1nompeex723z
-sha1: 59ab434be4c2d5d646dee84f514aa09e1b72feeb
-"""
         my_gpg = gpg.GPGStrategy(FakeConfig())
-        self.assertEqual((gpg.SIGNATURE_EXPIRED, u'4F8D1513'),
-                            my_gpg.verify(content, plain))
+        self.assertEqual((gpg.SIGNATURE_EXPIRED, u'4F8D1513', None),
+                            my_gpg.verify(content))
 
     def test_verify_unknown_key(self):
         self.requireFeature(features.gpg)
@@ -415,10 +376,9 @@
 =RNR5
 -----END PGP SIGNATURE-----
 """
-        plain = "asdf\n"
         my_gpg = gpg.GPGStrategy(FakeConfig())
-        self.assertEqual((gpg.SIGNATURE_KEY_MISSING, u'5D51E56F'),
-                            my_gpg.verify(content, plain))
+        self.assertEqual((gpg.SIGNATURE_KEY_MISSING, u'5D51E56F', None),
+                            my_gpg.verify(content))
 
     def test_set_acceptable_keys(self):
         self.requireFeature(features.gpg)
@@ -454,9 +414,8 @@
 
     def test_sign(self):
         self.assertRaises(gpg.SigningFailed,
-                          gpg.DisabledGPGStrategy(None).sign, 'content')
+                          gpg.DisabledGPGStrategy(None).sign, 'content', gpg.MODE_CLEAR)
 
     def test_verify(self):
         self.assertRaises(gpg.SignatureVerificationFailed,
-                          gpg.DisabledGPGStrategy(None).verify, 'content',
-                          'testament')
+                          gpg.DisabledGPGStrategy(None).verify, 'content')
 
=== modified file 'breezy/tests/test_log.py'
--- breezy/tests/test_log.py	2017-12-04 00:25:02 +0000
+++ breezy/tests/test_log.py	2018-03-24 01:56:28 +0000
@@ -317,31 +317,22 @@
 
 
 class TestFormatSignatureValidity(tests.TestCaseWithTransport):
-    class UTFLoopbackGPGStrategy(gpg.LoopbackGPGStrategy):
-        def verify(self, content, testament):
-            return (gpg.SIGNATURE_VALID,
-                u'UTF8 Test \xa1\xb1\xc1\xd1\xe1\xf1 <[e-mail]>')
-
-    def has_signature_for_revision_id(self, revision_id):
-        return True
-
-    def get_signature_text(self, revision_id):
-        return ''
+
+    def verify_revision_signature(self, revid, gpg_strategy):
+        return (gpg.SIGNATURE_VALID,
+            u'UTF8 Test \xa1\xb1\xc1\xd1\xe1\xf1 <[e-mail]>')
 
     def test_format_signature_validity_utf(self):
         """Check that GPG signatures containing UTF-8 names are formatted
         correctly."""
-        # Monkey patch to use our UTF-8 generating GPGStrategy
-        self.overrideAttr(gpg, 'GPGStrategy', self.UTFLoopbackGPGStrategy)
         wt = self.make_branch_and_tree('.')
         revid = wt.commit('empty commit')
         repo = wt.branch.repository
         # Monkey patch out checking if this rev is actually signed, since we
         # can't sign it without a heavier TestCase and LoopbackGPGStrategy
         # doesn't care anyways.
-        self.overrideAttr(repo, 'has_signature_for_revision_id',
-                self.has_signature_for_revision_id)
-        self.overrideAttr(repo, 'get_signature_text', self.get_signature_text)
+        self.overrideAttr(repo, 'verify_revision_signature',
+                self.verify_revision_signature)
         out = log.format_signature_validity(revid, wt.branch)
         self.assertEqual(
 u'valid signature from UTF8 Test \xa1\xb1\xc1\xd1\xe1\xf1 <[e-mail]>',