Merge lp:~vexo/bzr/fix-1123460 into lp:bzr

[Reagan Sanders]

Fix bug LP: #1123460, verify-signature crashes on non ascii characters (and on -v)

Fixes bug LP: #1123460. This bug really wraps up two completely independent issues.

First, the issue with verify-signatures --verbose. We were attempting to pass an array of strings to write() directly. Changed to loop through the array and write them all.

Second, issue with UTF characters in GPG names. We were trying to stick the UTF strings from the GPG subsystem into plain ASCII format strings. Changed the format strings to be UTF as well.

After these fixes, bzr verify-signatures -v and bzr qlog both succeed on a test repository signed with a key containing a UTF-8 name.

vexo@anput:~/sign$ bzr verify-signatures -v
1 commits with valid signatures
  ÀÇÐőbˆ Doe <email address hidden> signed 1 commit
0 commits with key now expired
1 commit with unknown key
  Unknown key B2C03A8B signed 1 commit
0 commits not valid
0 commits not signed
vexo@anput:~/sign$ bzr qlog
vexo@anput:~/sign$

[Reagan Sanders (vexo)]

Additional test example for the all-valid code path:

vexo@anput:~/sign$ bzr verify-signatures -v
All commits signed with verifiable keys
  ÀÇÐőbˆ Doe <email address hidden> signed 1 commit
vexo@anput:~/sign$

[Richard Wilbur (richard-wilbur)]

On Sun, Jul 28, 2013 at 12:06 PM, Reagan Sanders <email address hidden> wrote:
> Additional test example for the all-valid code path:
>
> vexo@anput:~/sign$ bzr verify-signatures -v
> All commits signed with verifiable keys
> ÀÇÐőbˆ Doe <email address hidden> signed 1 commit
> vexo@anput:~/sign$
> --
> https://code.launchpad.net/~vexofp/bzr/fix-1123460/+merge/177290
> Your team bzr-core is requested to review the proposed merge of lp:~vexofp/bzr/fix-1123460 into lp:bzr.

Reagan,

I tried to review this merge request and I'm having trouble bringing
it up. It looks like some great fixes that would be nice to get in
the trunk.

I don't think the change to write out the array of strings in a loop
requires a test case--you fixed a flaw in the implementation! Nice
work.

How hard would it be to code up your 'verify signatures' test case?

Sincerely,

Richard

[Reagan Sanders (vexo)]

I went ahead and added a couple basic test cases for verify-signatures --verbose, as it was completely uncovered previously and that didn't seem good. The minimal existing tests for verify-signatures were also oddly lumped in with sign-my-commits, so I split everything off into a new file.

After looking at it a bit more, the UTF-8 issue never actually had anything to do with verify-signatures, and the issue was really with generating the log, so I added the test case for it there. It seems like there might be a cleaner way of writing the test, but actually signing the test commit didn't seem to play nice with the way TestCaseWithTransport sets up its repository.

[Richard Wilbur (richard-wilbur)]

Thanks for the great work! I sincerely appreciate the improved test coverage, and thank you for splitting out the verify-signatures test cases.
+1

review: Approve

[Reagan Sanders (vexo)]

Don't know how I overlooked the merge request for lp:~krause/bzr/1037108, but it appears to be a subset of what I've done here. I'm not sure if there's merge-proposal equivalent for marking it as a duplicate or invalid, but noting it here so it can be handled appropriately if this gets landed.

https://code.launchpad.net/~krause/bzr/1037108/+merge/119912

[Richard Wilbur (richard-wilbur)]

Thanks for noting the previous merge request. I will look into
marking it "duplicate" or "invalid", etc. Did you notice John
Meinel's suggestion to decode the utf-8 to unicode? Does that still
apply? I'm still learning about unicode vs. utf-8, etc.

Do we need to worry about unicode format string for
bzrlib/log.py:format_signature_validity:339 ? How about translations
of these format strings and the two format strings in
bzrlib/log.py:format_signature_validity:341,343?

[Reagan Sanders (vexo)]

> Thanks for noting the previous merge request. I will look into
> marking it "duplicate" or "invalid", etc. Did you notice John
> Meinel's suggestion to decode the utf-8 to unicode? Does that still
> apply? I'm still learning about unicode vs. utf-8, etc.

I don't *think* so, as pygpgme was already nice enough to do what John was suggesting might be required.

result[1] is created at gpg.py:295 as the concatenation of key.uids[0].name and key.uids[0].email, where key is self.context.get_key(fingerprint) as returned from pygpgme. Digging into the code for pygpgme, these equate to the c functions pygpgme_user_id_get_name/email, both of which return the output of a call to PyUnicode_DecodeUTF8. Thus, we already have a decoded python unicode object (vice a raw UTF-8 encoded bytestream), and don't need to decode it any further.

http://bazaar.launchpad.net/~jamesh/pygpgme/trunk/view/head:/src/pygpgme-key.c#L338

> Do we need to worry about unicode format string for
> bzrlib/log.py:format_signature_validity:339 ?

No, if the key is missing, that string will always be the hexadecimal ID for the string (eg. 1F4AAD09), since that would be the only thing we know at that point. Similar to the above analysis, the value here is derived from a call to PyUnicode_DecodeASCII in pygpgme, so this will always be a plain str object.

http://bazaar.launchpad.net/~jamesh/pygpgme/trunk/view/head:/src/pygpgme-key.c#L109

> How about translations
> of these format strings and the two format strings in
> bzrlib/log.py:format_signature_validity:341,343?

As far as I can tell, nothing else that shows up in the *output* of the log seems to be translated, though the error messages are. Either way, I think that'd go beyond the scope of this bug fix and into adding new functionality.

Now, a valid question might be why format_signature_validity is public at the module scope at all, rather than being a member of _DefaultLogGenerator like _format_diff? I don't see it being called anywhere else and can't think of much reason that it ever would be. Then again, there's a lot of public functions like that in log.py and I can't think of any specific downsides to exposing it.

[Richard Wilbur (richard-wilbur)]

I reviewed https://code.launchpad.net/~krause/bzr/1037108/+merge/119912 as "Disapprove (branch superceded)" with a pointer to this branch and a short explanation of the situation, then I marked the merge request "Rejected".

I think this ready for merging. Don't you?

[Reagan Sanders (vexo)]

Agree. (Good thing too, with how my free time has been lately... :/ )

review: Approve

[Richard Wilbur (richard-wilbur)]

sent to pqm by email

[Richard Wilbur (richard-wilbur)]

sent to pqm by email

[Richard Wilbur (richard-wilbur)]

Reagan, I finally got my bzr and gpg configuration properly setup and the folks on launchpad setup the authentication configuration for pqm so I am able to merge to trunk. Many thanks to John Meinel and Vincent Ladeuil for their advice and assistance.

Thus, this branch finally landed on trunk!

=== modified file 'bzrlib/commit_signature_commands.py'
--- bzrlib/commit_signature_commands.py	2012-03-11 16:51:49 +0000
+++ bzrlib/commit_signature_commands.py	2013-08-11 20:38:13 +0000
@@ -165,7 +165,8 @@
         if all_verifiable:
                write(gettext("All commits signed with verifiable keys"))
                if verbose:
-                   write(gpg.verbose_valid_message(result))
+                   for message in gpg.verbose_valid_message(result):
+                       write_verbose(message)
                return 0
         else:
             write(gpg.valid_commits_message(count))

=== modified file 'bzrlib/log.py'
--- bzrlib/log.py	2012-03-14 11:30:42 +0000
+++ bzrlib/log.py	2013-08-11 20:38:13 +0000
@@ -334,7 +334,7 @@
     gpg_strategy = gpg.GPGStrategy(None)
     result = repo.verify_revision_signature(rev_id, gpg_strategy)
     if result[0] == gpg.SIGNATURE_VALID:
-        return "valid signature from {0}".format(result[1])
+        return u"valid signature from {0}".format(result[1])
     if result[0] == gpg.SIGNATURE_KEY_MISSING:
         return "unknown key {0}".format(result[1])
     if result[0] == gpg.SIGNATURE_NOT_VALID:

=== modified file 'bzrlib/tests/blackbox/__init__.py'
--- bzrlib/tests/blackbox/__init__.py	2012-09-08 13:26:18 +0000
+++ bzrlib/tests/blackbox/__init__.py	2013-08-11 20:38:13 +0000
@@ -118,6 +118,7 @@
                      'test_shell_complete',
                      'test_shelve',
                      'test_sign_my_commits',
+                     'test_verify_signatures',
                      'test_split',
                      'test_status',
                      'test_switch',

=== modified file 'bzrlib/tests/blackbox/test_sign_my_commits.py'
--- bzrlib/tests/blackbox/test_sign_my_commits.py	2012-03-11 16:51:49 +0000
+++ bzrlib/tests/blackbox/test_sign_my_commits.py	2013-08-11 20:38:13 +0000
@@ -116,29 +116,6 @@
         self.assertUnsigned(repo, 'D')
         self.assertUnsigned(repo, 'E')
 
-    def test_verify_commits(self):
-        wt = self.setup_tree()
-        self.monkey_patch_gpg()
-        self.run_bzr('sign-my-commits')
-        out = self.run_bzr('verify-signatures', retcode=1)
-        self.assertEquals(('4 commits with valid signatures\n'
-                           '0 commits with key now expired\n'
-                           '0 commits with unknown keys\n'
-                           '0 commits not valid\n'
-                           '1 commit not signed\n', ''), out)
-
-    def test_verify_commits_acceptable_key(self):
-        wt = self.setup_tree()
-        self.monkey_patch_gpg()
-        self.run_bzr('sign-my-commits')
-        out = self.run_bzr(['verify-signatures', '--acceptable-keys=foo,bar'],
-                            retcode=1)
-        self.assertEquals(('4 commits with valid signatures\n'
-                           '0 commits with key now expired\n'
-                           '0 commits with unknown keys\n'
-                           '0 commits not valid\n'
-                           '1 commit not signed\n', ''), out)
-
 
 class TestSmartServerSignMyCommits(tests.TestCaseWithTransport):
 
@@ -168,23 +145,3 @@
         self.assertLength(15, self.hpss_calls)
         self.assertLength(1, self.hpss_connections)
         self.assertThat(self.hpss_calls, ContainsNoVfsCalls)
-
-    def test_verify_commits(self):
-        self.setup_smart_server_with_call_log()
-        t = self.make_branch_and_tree('branch')
-        self.build_tree_contents([('branch/foo', 'thecontents')])
-        t.add("foo")
-        t.commit("message")
-        self.monkey_patch_gpg()
-        out, err = self.run_bzr(['sign-my-commits', self.get_url('branch')])
-        self.reset_smart_call_log()
-        self.run_bzr('sign-my-commits')
-        out = self.run_bzr(['verify-signatures', self.get_url('branch')])
-        # This figure represent the amount of work to perform this use case. It
-        # is entirely ok to reduce this number if a test fails due to rpc_count
-        # being too low. If rpc_count increases, more network roundtrips have
-        # become necessary for this use case. Please do not adjust this number
-        # upwards without agreement from bzr's network support maintainers.
-        self.assertLength(10, self.hpss_calls)
-        self.assertLength(1, self.hpss_connections)
-        self.assertThat(self.hpss_calls, ContainsNoVfsCalls)

=== added file 'bzrlib/tests/blackbox/test_verify_signatures.py'
--- bzrlib/tests/blackbox/test_verify_signatures.py	1970-01-01 00:00:00 +0000
+++ bzrlib/tests/blackbox/test_verify_signatures.py	2013-08-11 20:38:13 +0000
@@ -0,0 +1,124 @@
+# Copyright (C) 2005 Canonical Ltd
+# -*- coding: utf-8 -*-
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+"""Black-box tests for bzr verify-signatures."""
+
+from bzrlib import (
+    gpg,
+    tests,
+    )
+from bzrlib.tests.matchers import ContainsNoVfsCalls
+
+
+class TestVerifySignatures(tests.TestCaseWithTransport):
+
+    def monkey_patch_gpg(self):
+        """Monkey patch the gpg signing strategy to be a loopback.
+
+        This also registers the cleanup, so that we will revert to
+        the original gpg strategy when done.
+        """
+        # monkey patch gpg signing mechanism
+        self.overrideAttr(gpg, 'GPGStrategy', gpg.LoopbackGPGStrategy)
+
+    def setup_tree(self, location='.'):
+        wt = self.make_branch_and_tree(location)
+        wt.commit("base A", allow_pointless=True, rev_id='A')
+        wt.commit("base B", allow_pointless=True, rev_id='B')
+        wt.commit("base C", allow_pointless=True, rev_id='C')
+        wt.commit("base D", allow_pointless=True, rev_id='D',
+                committer='Alternate <[e-mail]>')
+        wt.add_parent_tree_id("aghost")
+        wt.commit("base E", allow_pointless=True, rev_id='E')
+        return wt
+
+    def test_verify_signatures(self):
+        wt = self.setup_tree()
+        self.monkey_patch_gpg()
+        self.run_bzr('sign-my-commits')
+        out = self.run_bzr('verify-signatures', retcode=1)
+        self.assertEquals(('4 commits with valid signatures\n'
+                           '0 commits with key now expired\n'
+                           '0 commits with unknown keys\n'
+                           '0 commits not valid\n'
+                           '1 commit not signed\n', ''), out)
+
+    def test_verify_signatures_acceptable_key(self):
+        wt = self.setup_tree()
+        self.monkey_patch_gpg()
+        self.run_bzr('sign-my-commits')
+        out = self.run_bzr(['verify-signatures', '--acceptable-keys=foo,bar'],
+                            retcode=1)
+        self.assertEquals(('4 commits with valid signatures\n'
+                           '0 commits with key now expired\n'
+                           '0 commits with unknown keys\n'
+                           '0 commits not valid\n'
+                           '1 commit not signed\n', ''), out)
+
+    def test_verify_signatures_verbose(self):
+        wt = self.setup_tree()
+        self.monkey_patch_gpg()
+        self.run_bzr('sign-my-commits')
+        out = self.run_bzr('verify-signatures --verbose', retcode=1)
+        self.assertEquals(('4 commits with valid signatures\n'
+                           '  None signed 4 commits\n'
+                           '0 commits with key now expired\n'
+                           '0 commits with unknown keys\n'
+                           '0 commits not valid\n'
+                           '1 commit not signed\n'
+                           '  1 commit by author Alternate <[e-mail]>\n', ''), out)
+
+    def test_verify_signatures_verbose_all_valid(self):
+        wt = self.setup_tree()
+        self.monkey_patch_gpg()
+        self.run_bzr('sign-my-commits')
+        self.run_bzr(['sign-my-commits', '.', 'Alternate <[e-mail]>'])
+        out = self.run_bzr('verify-signatures --verbose')
+        self.assertEquals(('All commits signed with verifiable keys\n'
+                           '  None signed 5 commits\n', ''), out)
+
+
+class TestSmartServerVerifySignatures(tests.TestCaseWithTransport):
+
+    def monkey_patch_gpg(self):
+        """Monkey patch the gpg signing strategy to be a loopback.
+
+        This also registers the cleanup, so that we will revert to
+        the original gpg strategy when done.
+        """
+        # monkey patch gpg signing mechanism
+        self.overrideAttr(gpg, 'GPGStrategy', gpg.LoopbackGPGStrategy)
+
+    def test_verify_signatures(self):
+        self.setup_smart_server_with_call_log()
+        t = self.make_branch_and_tree('branch')
+        self.build_tree_contents([('branch/foo', 'thecontents')])
+        t.add("foo")
+        t.commit("message")
+        self.monkey_patch_gpg()
+        out, err = self.run_bzr(['sign-my-commits', self.get_url('branch')])
+        self.reset_smart_call_log()
+        self.run_bzr('sign-my-commits')
+        out = self.run_bzr(['verify-signatures', self.get_url('branch')])
+        # This figure represent the amount of work to perform this use case. It
+        # is entirely ok to reduce this number if a test fails due to rpc_count
+        # being too low. If rpc_count increases, more network roundtrips have
+        # become necessary for this use case. Please do not adjust this number
+        # upwards without agreement from bzr's network support maintainers.
+        self.assertLength(10, self.hpss_calls)
+        self.assertLength(1, self.hpss_connections)
+        self.assertThat(self.hpss_calls, ContainsNoVfsCalls)

=== modified file 'bzrlib/tests/test_log.py'
--- bzrlib/tests/test_log.py	2012-08-04 14:27:47 +0000
+++ bzrlib/tests/test_log.py	2013-08-11 20:38:13 +0000
@@ -25,6 +25,8 @@
     revision,
     revisionspec,
     tests,
+    gpg,
+    trace,
     )
 
 
@@ -281,6 +283,38 @@
         self.checkDelta(logentry.delta, added=['file1', 'file2'])
 
 
+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 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)
+        out = log.format_signature_validity(revid, repo)
+        self.assertEqual(
+u'valid signature from UTF8 Test \xa1\xb1\xc1\xd1\xe1\xf1 <[e-mail]>',
+                out)
+
+
 class TestShortLogFormatter(TestCaseForLogFormatter):
 
     def test_trailing_newlines(self):