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

Proposed by Reagan Sanders on 2013-07-28
Status: Merged
Approved by: Richard Wilbur on 2013-10-14
Approved revision: 6588
Merged at revision: 6594
Proposed branch: lp:~vexo/bzr/fix-1123460
Merge into: lp:bzr
Diff against target: 277 lines (+162/-45)
6 files modified
bzrlib/commit_signature_commands.py (+2/-1)
bzrlib/log.py (+1/-1)
bzrlib/tests/blackbox/__init__.py (+1/-0)
bzrlib/tests/blackbox/test_sign_my_commits.py (+0/-43)
bzrlib/tests/blackbox/test_verify_signatures.py (+124/-0)
bzrlib/tests/test_log.py (+34/-0)
To merge this branch: bzr merge lp:~vexo/bzr/fix-1123460
Reviewer Review Type Date Requested Status
Reagan Sanders (community) Approve on 2013-10-10
Richard Wilbur 2013-07-28 Approve on 2013-08-20
Review via email: mp+177290@code.launchpad.net

Commit message

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

Description of the change

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$

To post a comment you must log in.
Reagan Sanders (vexo) 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$

Richard Wilbur (richard-wilbur) wrote :

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

lp:~vexo/bzr/fix-1123460 updated on 2013-08-11
6586. By Reagan Sanders on 2013-08-11

Reverted the UTF change to the SIGNATURE_KEY_MISSING case, as the values here will always be a key ID, and that will never be non-ASCII.

6587. By Reagan Sanders on 2013-08-11

Split the existing test cases for verify-signatures away from those for sign-my-commits and added test cases covering the --verbose option.

6588. By Reagan Sanders on 2013-08-11

Added a test case covering the possibility of UTF-8 characters within the name portion of a GPG key. This can happen during a 'bzr log --signatures' command.

Reagan Sanders (vexo) wrote :

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

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

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

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

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

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

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

review: Approve
Richard Wilbur (richard-wilbur) wrote :

sent to pqm by email

Richard Wilbur (richard-wilbur) wrote :

sent to pqm by email

Richard Wilbur (richard-wilbur) wrote :

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!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/commit_signature_commands.py'
2--- bzrlib/commit_signature_commands.py 2012-03-11 16:51:49 +0000
3+++ bzrlib/commit_signature_commands.py 2013-08-11 20:38:13 +0000
4@@ -165,7 +165,8 @@
5 if all_verifiable:
6 write(gettext("All commits signed with verifiable keys"))
7 if verbose:
8- write(gpg.verbose_valid_message(result))
9+ for message in gpg.verbose_valid_message(result):
10+ write_verbose(message)
11 return 0
12 else:
13 write(gpg.valid_commits_message(count))
14
15=== modified file 'bzrlib/log.py'
16--- bzrlib/log.py 2012-03-14 11:30:42 +0000
17+++ bzrlib/log.py 2013-08-11 20:38:13 +0000
18@@ -334,7 +334,7 @@
19 gpg_strategy = gpg.GPGStrategy(None)
20 result = repo.verify_revision_signature(rev_id, gpg_strategy)
21 if result[0] == gpg.SIGNATURE_VALID:
22- return "valid signature from {0}".format(result[1])
23+ return u"valid signature from {0}".format(result[1])
24 if result[0] == gpg.SIGNATURE_KEY_MISSING:
25 return "unknown key {0}".format(result[1])
26 if result[0] == gpg.SIGNATURE_NOT_VALID:
27
28=== modified file 'bzrlib/tests/blackbox/__init__.py'
29--- bzrlib/tests/blackbox/__init__.py 2012-09-08 13:26:18 +0000
30+++ bzrlib/tests/blackbox/__init__.py 2013-08-11 20:38:13 +0000
31@@ -118,6 +118,7 @@
32 'test_shell_complete',
33 'test_shelve',
34 'test_sign_my_commits',
35+ 'test_verify_signatures',
36 'test_split',
37 'test_status',
38 'test_switch',
39
40=== modified file 'bzrlib/tests/blackbox/test_sign_my_commits.py'
41--- bzrlib/tests/blackbox/test_sign_my_commits.py 2012-03-11 16:51:49 +0000
42+++ bzrlib/tests/blackbox/test_sign_my_commits.py 2013-08-11 20:38:13 +0000
43@@ -116,29 +116,6 @@
44 self.assertUnsigned(repo, 'D')
45 self.assertUnsigned(repo, 'E')
46
47- def test_verify_commits(self):
48- wt = self.setup_tree()
49- self.monkey_patch_gpg()
50- self.run_bzr('sign-my-commits')
51- out = self.run_bzr('verify-signatures', retcode=1)
52- self.assertEquals(('4 commits with valid signatures\n'
53- '0 commits with key now expired\n'
54- '0 commits with unknown keys\n'
55- '0 commits not valid\n'
56- '1 commit not signed\n', ''), out)
57-
58- def test_verify_commits_acceptable_key(self):
59- wt = self.setup_tree()
60- self.monkey_patch_gpg()
61- self.run_bzr('sign-my-commits')
62- out = self.run_bzr(['verify-signatures', '--acceptable-keys=foo,bar'],
63- retcode=1)
64- self.assertEquals(('4 commits with valid signatures\n'
65- '0 commits with key now expired\n'
66- '0 commits with unknown keys\n'
67- '0 commits not valid\n'
68- '1 commit not signed\n', ''), out)
69-
70
71 class TestSmartServerSignMyCommits(tests.TestCaseWithTransport):
72
73@@ -168,23 +145,3 @@
74 self.assertLength(15, self.hpss_calls)
75 self.assertLength(1, self.hpss_connections)
76 self.assertThat(self.hpss_calls, ContainsNoVfsCalls)
77-
78- def test_verify_commits(self):
79- self.setup_smart_server_with_call_log()
80- t = self.make_branch_and_tree('branch')
81- self.build_tree_contents([('branch/foo', 'thecontents')])
82- t.add("foo")
83- t.commit("message")
84- self.monkey_patch_gpg()
85- out, err = self.run_bzr(['sign-my-commits', self.get_url('branch')])
86- self.reset_smart_call_log()
87- self.run_bzr('sign-my-commits')
88- out = self.run_bzr(['verify-signatures', self.get_url('branch')])
89- # This figure represent the amount of work to perform this use case. It
90- # is entirely ok to reduce this number if a test fails due to rpc_count
91- # being too low. If rpc_count increases, more network roundtrips have
92- # become necessary for this use case. Please do not adjust this number
93- # upwards without agreement from bzr's network support maintainers.
94- self.assertLength(10, self.hpss_calls)
95- self.assertLength(1, self.hpss_connections)
96- self.assertThat(self.hpss_calls, ContainsNoVfsCalls)
97
98=== added file 'bzrlib/tests/blackbox/test_verify_signatures.py'
99--- bzrlib/tests/blackbox/test_verify_signatures.py 1970-01-01 00:00:00 +0000
100+++ bzrlib/tests/blackbox/test_verify_signatures.py 2013-08-11 20:38:13 +0000
101@@ -0,0 +1,124 @@
102+# Copyright (C) 2005 Canonical Ltd
103+# -*- coding: utf-8 -*-
104+#
105+# This program is free software; you can redistribute it and/or modify
106+# it under the terms of the GNU General Public License as published by
107+# the Free Software Foundation; either version 2 of the License, or
108+# (at your option) any later version.
109+#
110+# This program is distributed in the hope that it will be useful,
111+# but WITHOUT ANY WARRANTY; without even the implied warranty of
112+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
113+# GNU General Public License for more details.
114+#
115+# You should have received a copy of the GNU General Public License
116+# along with this program; if not, write to the Free Software
117+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
118+
119+"""Black-box tests for bzr verify-signatures."""
120+
121+from bzrlib import (
122+ gpg,
123+ tests,
124+ )
125+from bzrlib.tests.matchers import ContainsNoVfsCalls
126+
127+
128+class TestVerifySignatures(tests.TestCaseWithTransport):
129+
130+ def monkey_patch_gpg(self):
131+ """Monkey patch the gpg signing strategy to be a loopback.
132+
133+ This also registers the cleanup, so that we will revert to
134+ the original gpg strategy when done.
135+ """
136+ # monkey patch gpg signing mechanism
137+ self.overrideAttr(gpg, 'GPGStrategy', gpg.LoopbackGPGStrategy)
138+
139+ def setup_tree(self, location='.'):
140+ wt = self.make_branch_and_tree(location)
141+ wt.commit("base A", allow_pointless=True, rev_id='A')
142+ wt.commit("base B", allow_pointless=True, rev_id='B')
143+ wt.commit("base C", allow_pointless=True, rev_id='C')
144+ wt.commit("base D", allow_pointless=True, rev_id='D',
145+ committer='Alternate <alt@foo.com>')
146+ wt.add_parent_tree_id("aghost")
147+ wt.commit("base E", allow_pointless=True, rev_id='E')
148+ return wt
149+
150+ def test_verify_signatures(self):
151+ wt = self.setup_tree()
152+ self.monkey_patch_gpg()
153+ self.run_bzr('sign-my-commits')
154+ out = self.run_bzr('verify-signatures', retcode=1)
155+ self.assertEquals(('4 commits with valid signatures\n'
156+ '0 commits with key now expired\n'
157+ '0 commits with unknown keys\n'
158+ '0 commits not valid\n'
159+ '1 commit not signed\n', ''), out)
160+
161+ def test_verify_signatures_acceptable_key(self):
162+ wt = self.setup_tree()
163+ self.monkey_patch_gpg()
164+ self.run_bzr('sign-my-commits')
165+ out = self.run_bzr(['verify-signatures', '--acceptable-keys=foo,bar'],
166+ retcode=1)
167+ self.assertEquals(('4 commits with valid signatures\n'
168+ '0 commits with key now expired\n'
169+ '0 commits with unknown keys\n'
170+ '0 commits not valid\n'
171+ '1 commit not signed\n', ''), out)
172+
173+ def test_verify_signatures_verbose(self):
174+ wt = self.setup_tree()
175+ self.monkey_patch_gpg()
176+ self.run_bzr('sign-my-commits')
177+ out = self.run_bzr('verify-signatures --verbose', retcode=1)
178+ self.assertEquals(('4 commits with valid signatures\n'
179+ ' None signed 4 commits\n'
180+ '0 commits with key now expired\n'
181+ '0 commits with unknown keys\n'
182+ '0 commits not valid\n'
183+ '1 commit not signed\n'
184+ ' 1 commit by author Alternate <alt@foo.com>\n', ''), out)
185+
186+ def test_verify_signatures_verbose_all_valid(self):
187+ wt = self.setup_tree()
188+ self.monkey_patch_gpg()
189+ self.run_bzr('sign-my-commits')
190+ self.run_bzr(['sign-my-commits', '.', 'Alternate <alt@foo.com>'])
191+ out = self.run_bzr('verify-signatures --verbose')
192+ self.assertEquals(('All commits signed with verifiable keys\n'
193+ ' None signed 5 commits\n', ''), out)
194+
195+
196+class TestSmartServerVerifySignatures(tests.TestCaseWithTransport):
197+
198+ def monkey_patch_gpg(self):
199+ """Monkey patch the gpg signing strategy to be a loopback.
200+
201+ This also registers the cleanup, so that we will revert to
202+ the original gpg strategy when done.
203+ """
204+ # monkey patch gpg signing mechanism
205+ self.overrideAttr(gpg, 'GPGStrategy', gpg.LoopbackGPGStrategy)
206+
207+ def test_verify_signatures(self):
208+ self.setup_smart_server_with_call_log()
209+ t = self.make_branch_and_tree('branch')
210+ self.build_tree_contents([('branch/foo', 'thecontents')])
211+ t.add("foo")
212+ t.commit("message")
213+ self.monkey_patch_gpg()
214+ out, err = self.run_bzr(['sign-my-commits', self.get_url('branch')])
215+ self.reset_smart_call_log()
216+ self.run_bzr('sign-my-commits')
217+ out = self.run_bzr(['verify-signatures', self.get_url('branch')])
218+ # This figure represent the amount of work to perform this use case. It
219+ # is entirely ok to reduce this number if a test fails due to rpc_count
220+ # being too low. If rpc_count increases, more network roundtrips have
221+ # become necessary for this use case. Please do not adjust this number
222+ # upwards without agreement from bzr's network support maintainers.
223+ self.assertLength(10, self.hpss_calls)
224+ self.assertLength(1, self.hpss_connections)
225+ self.assertThat(self.hpss_calls, ContainsNoVfsCalls)
226
227=== modified file 'bzrlib/tests/test_log.py'
228--- bzrlib/tests/test_log.py 2012-08-04 14:27:47 +0000
229+++ bzrlib/tests/test_log.py 2013-08-11 20:38:13 +0000
230@@ -25,6 +25,8 @@
231 revision,
232 revisionspec,
233 tests,
234+ gpg,
235+ trace,
236 )
237
238
239@@ -281,6 +283,38 @@
240 self.checkDelta(logentry.delta, added=['file1', 'file2'])
241
242
243+class TestFormatSignatureValidity(tests.TestCaseWithTransport):
244+ class UTFLoopbackGPGStrategy(gpg.LoopbackGPGStrategy):
245+ def verify(self, content, testament):
246+ return (gpg.SIGNATURE_VALID,
247+ u'UTF8 Test \xa1\xb1\xc1\xd1\xe1\xf1 <jrandom@example.com>')
248+
249+ def has_signature_for_revision_id(self, revision_id):
250+ return True
251+
252+ def get_signature_text(self, revision_id):
253+ return ''
254+
255+ def test_format_signature_validity_utf(self):
256+ """Check that GPG signatures containing UTF-8 names are formatted
257+ correctly."""
258+ # Monkey patch to use our UTF-8 generating GPGStrategy
259+ self.overrideAttr(gpg, 'GPGStrategy', self.UTFLoopbackGPGStrategy)
260+ wt = self.make_branch_and_tree('.')
261+ revid = wt.commit('empty commit')
262+ repo = wt.branch.repository
263+ # Monkey patch out checking if this rev is actually signed, since we
264+ # can't sign it without a heavier TestCase and LoopbackGPGStrategy
265+ # doesn't care anyways.
266+ self.overrideAttr(repo, 'has_signature_for_revision_id',
267+ self.has_signature_for_revision_id)
268+ self.overrideAttr(repo, 'get_signature_text', self.get_signature_text)
269+ out = log.format_signature_validity(revid, repo)
270+ self.assertEqual(
271+u'valid signature from UTF8 Test \xa1\xb1\xc1\xd1\xe1\xf1 <jrandom@example.com>',
272+ out)
273+
274+
275 class TestShortLogFormatter(TestCaseForLogFormatter):
276
277 def test_trailing_newlines(self):