Merge lp:~jr/bzr/bzr-gpgme into lp:bzr

[Jonathan Riddell]

A new command ``bzr verify-signatures`` has been added to check that commits
are correctly signed with trusted keys by GPG. This requires python-gpgme to
be installed. ``bzr log`` has gained a ``--signatures`` option to list the
validity of signatures for each commit. New config options ``acceptable_keys``
and ``validate_signatures_in_log`` can be set to control options to these
commands.

Implement bzr verify command

verifies that commits on a branch are signed by trusted keys

uses gpgme, a new (non-required) dependency for bzr

sign-my-commits.py should probably be renamed

still some tidying up to do but I'd like feedback on it so far

[Jelmer Vernooij (jelmer)]

Thanks for taking this on.. it's one of the oldest unfinished features in bzr I think :) Not a full review, but some quick notes:

VerifyFailed seems a bit too generic a name for something that applies only to verifying signatures. Perhaps something like SignatureVerificationFailed ?

GpgmeNotInstalled should probably derive from DependencyNotPresent (see also ParamikoNotPresent in bzrlib.errors).

Tests that use this should probably call self.requireFeature(GPGMeFeature) or something along those lines to make sure the tests are skipped for people that do not have gpgme installed.

[Jonathan Riddell (jr)]

Jelmer's suggestions added

[Jelmer Vernooij (jelmer)]

I think you're on the right track with this; here are some more comments. It would be great if somebody else (with more GPG experience) could also comment.

113 + if from_revno is None or to_revno is None:
114 + raise errors.BzrCommandError('Cannot verify a range of \
115 + non-revision-history revisions')
As far as I know this isn't possible - perhaps this should be an AssertionError rather than a BzrCommandError?

For consistency we've been adding --directory/-d option to most commands; it would be nice to support that to "bzr verify" as well.

I think the command name is perhaps a bit generic, perhaps it should be "bzr verify-signatures", with "verify" as one of its current aliases. We currently already have "bzr check" and to me, as a non-native speaker, these two words mean roughly the same thing in this context.

I'm unsure about supporting a --acceptable-keys argument to verify. I think it would be sufficient for verify to just check that the signatures match what is actually in the repository, leaving the issue of who to trust up to the user for the time being. I don't see myself using --acceptable-keys because I usually only care about the signature on a particular revision (not the history leading up to that point) or there are too many revisions and thus too many keys to specify (bzr has > 200 contributors, it's not practical to specify all their key ids on the command line).

We should watch out with displaying the signature status for each revision in "bzr log"; it has the potential to significantly slow down log if more people start signing their revisions.

[Jonathan Riddell (jr)]

> 113 + if from_revno is None or to_revno is None:
> 114 + raise errors.BzrCommandError('Cannot verify a range of \
> 115 + non-revision-history revisions')
> As far as I know this isn't possible - perhaps this should be an AssertionError rather than a BzrCommandError?

This happens if you specify versions from a merged revision, bzr verify -r 3.1.1..3.1.2 which isn't supported by this code
The code comes from builtins.cmd_re_sign()

> For consistency we've been adding --directory/-d option to most commands; it would be nice to support that to "bzr verify" as well.

Added.

> I think the command name is perhaps a bit generic, perhaps it should be "bzr verify-signatures", with "verify" as one of its current aliases.

Done.

> I'm unsure about supporting a --acceptable-keys argument to verify.

I trust lots of people to have digital keys that match their ID, but
it doesn't mean I trust them to commit to my code branch, so there
does need to be a way to specify which keys are acceptable.

> We should watch out with displaying the signature status for each
  revision in "bzr log"; it has the potential to significantly slow
  down log if more people start signing their revisions.

That's why it's an option which isn't on by default.

Jonathan

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 6/21/2011 2:06 PM, Jonathan Riddell wrote:
>> 113 + if from_revno is None or to_revno is None:
>> 114 + raise errors.BzrCommandError('Cannot verify a range of \
>> 115 + non-revision-history revisions')
>> As far as I know this isn't possible - perhaps this should be an AssertionError rather than a BzrCommandError?
>
> This happens if you specify versions from a merged revision, bzr verify -r 3.1.1..3.1.2 which isn't supported by this code
> The code comes from builtins.cmd_re_sign()
>

Some thoughts:

[1 needs fixing] You are adding a call to "get_ancestry()" which Jelmer
just recently marked as deprecated. I imagine this is pretty easy to fix
following his examples.

[2 comment] You import 'gpgme' inside of builtins.py. I would have
expected that sort of thing to be hidden inside 'gpg.py'. Something like
"gpg.support_verifying_signatures()" or "check_can_verify_signatures", etc.

[3 tweak] You have too much code in "cmd_verify_signatures". Most of
that should probably be moved into gpg.py and the cmd_* code should be a
reasonably small shim. That usually makes it easier to re-use and test.
Especially if a GUI ever wants to borrow some of the verify code.

[4 needsinfo] GpgStrategy.verify(self, content, testament)

- From what I can tell, you create a new gpgme Context for every revision.
Which I *think* means spawning a new gpg process. It seems like a
scalable design would want to be able to hold the context for an
extended period of time. Could it be a lazy attribute of GPGStrategy?

[5 needsinfo] Would we want a way to distinguish from "signature is
properly signed by an un-acceptable user" from "signature not properly
signed"? The latter hints more of corruption. And certainly from a UI
level, it might prompt me to say "oh, I need to add this person" rather
than "why isn't there signature valid?".

Something like SIGNATURE_UNTRUSTED, or
SIGNATURE_VALID_BUT_NOT_AUTHORIZED, etc.

[6 needsinfo] You mention that it slows down "bzr log" quite a bit. Have
you tried --lsprof to see what that cause is? (Are we re-computing the
testament from scratch for each rev?) I think it would be reasonable to
consider being able to separate out "check the signatures are gpg-valid"
from "the revision matches its testament". Just because at present, the
latter is *very* expensive, but the former should be pretty cheap.

I believe that we want to make the latter cheaper so that it can scale
to more active use. However, I'm curious what the specific performance
overhead is.

[7 needsfixing] bzrlib/tests/blackbox/test_sign_my_commits.py
You're adding one blank line per statement, which isn't preferred form.
You also have a continuation line which is a bit unnatural. Something
more like:

def test_verify_commits(self):
    wt = self.setup_tree()
    self.monkey_patch_gpg()
    self.run_bzr('sign-my-commits')
    out = self.run_bzr('verify', retcode=1)
    self.assertEquals(('',
 '4 commits with valid signatures\n'
 '0 commits with unknown keys\n'
 '0 commits not valid'
 '1 commit not signed\n'), out)

I'm also wondering why we are...

[Jonathan Riddell (jr)]

On Wed, Jun 22, 2011 at 11:43:37AM -0000, John A Meinel wrote:
> [1 needs fixing] You are adding a call to "get_ancestry()" which Jelmer
> just recently marked as deprecated. I imagine this is pretty easy to fix
> following his examples.

This was a bad merge, now fixed.

> [2 comment] You import 'gpgme' inside of builtins.py. I would have
> expected that sort of thing to be hidden inside 'gpg.py'. Something like
> "gpg.support_verifying_signatures()" or "check_can_verify_signatures", etc.

Done.

> [3 tweak] You have too much code in "cmd_verify_signatures". Most of
> that should probably be moved into gpg.py and the cmd_* code should be a
> reasonably small shim. That usually makes it easier to re-use and test.
> Especially if a GUI ever wants to borrow some of the verify code.

Done.

> [4 needsinfo] GpgStrategy.verify(self, content, testament)
>
> - From what I can tell, you create a new gpgme Context for every revision.
> Which I *think* means spawning a new gpg process. It seems like a
> scalable design would want to be able to hold the context for an
> extended period of time. Could it be a lazy attribute of GPGStrategy?

I've made the context global to the GpgStrategy class (although I
don't think it starts a new gpg process for each one).

> [5 needsinfo] Would we want a way to distinguish from "signature is
> properly signed by an un-acceptable user" from "signature not properly
> signed"?

There's no such concept as an unacceptable user, only acceptable ones.
Anything else is an unknown key and listed as such.

> [6 needsinfo] You mention that it slows down "bzr log" quite a bit. Have
> you tried --lsprof to see what that cause is?

The main cause is calls to verify() and get_key() in gpgme according to --lsprof.

> [7 needsfixing] bzrlib/tests/blackbox/test_sign_my_commits.py
> You're adding one blank line per statement, which isn't preferred form.

Done

> [8 needsfixing] I don't think we need the short-form of "verify". I
> agree with Jelmer that it sounds more like repository checking, and I
> don't think it is something people will run enough to need a short form for.
>
> The docs also need to be updated for this fact.

Done

> [9 needsfixing] We'll also want an entry in
> doc/en/release-notes/bzr-2.4.txt and probably
> doc/whatsnew/whats-new-in-2.4.txt.

Done

Jonathan

[Jonathan Riddell (jr)]

From discussion with JAM, needs to handle expired keys more gracefully, should give different error for unknown and unacceptable key, gpg verify should be commented so it can be easier understood and checked over by someone knowledgeable in gpgme.

[John A Meinel (jameinel)]

So I should clarify that while I agree with what needs to be done, we can
probably land this and do those bits as follow up cleanup. We don't want a
lot of drift, but I think this is incrementally better in all aspects, and
we shouldn't require it to be perfect. I'm sorry if I've been a bit
blockish, I have to remind myself sometimes. The rules for landing have
never been that it has to be perfect.

Merge: approve
John
=:->
On Jun 29, 2011 3:08 PM, "Jonathan Riddell" <email address hidden> wrote:
>>From discussion with JAM, needs to handle expired keys more gracefully,
should give different error for unknown and unacceptable key, gpg verify
should be commented so it can be easier understood and checked over by
someone knowledgeable in gpgme.
>
> --
> https://code.launchpad.net/~jr/bzr/bzr-gpgme/+merge/64859
> Your team bzr-core is requested to review the proposed merge of
lp:~jr/bzr/bzr-gpgme into lp:bzr.

[Jonathan Riddell (jr)]

sent to pqm by email

[Jonathan Riddell (jr)]

sent to pqm by email

[Jonathan Riddell (jr)]

sent to pqm by email

=== modified file 'bzrlib/gpg.py'
--- bzrlib/gpg.py	2011-06-30 14:36:05 +0000
+++ bzrlib/gpg.py	2011-06-30 16:17:30 +0000
@@ -161,6 +161,9 @@
 
     @staticmethod
     def verify_signatures_available():
+        """
+        :return: boolean if this strategy can verify signatures
+        """
         try:
             import gpgme
             return True
@@ -274,6 +277,7 @@
             if isinstance(acceptable_keys_config, unicode):
                 acceptable_keys_config = str(acceptable_keys_config)
         except UnicodeEncodeError:
+            #gpg Context.keylist(pattern) does not like unicode
             raise errors.BzrCommandError('Only ASCII permitted in option names')
 
         if acceptable_keys_config is not None:
@@ -296,11 +300,14 @@
                             "No GnuPG key results for pattern: {}"
                                 ).format(pattern))
 
-    def do_verifications(self, revisions, repository):
+    def do_verifications(self, revisions, repository,
+                            process_events_callback=None):
         """do verifications on a set of revisions
         
         :param revisions: list of revision ids to verify
         :param repository: repository object
+        :param process_events_callback: method to call for GUI frontends that
+                                                want to keep their UI refreshed
         
         :return: count dictionary of results of each type,
                  result list for each revision,
@@ -319,6 +326,8 @@
             count[verification_result] += 1
             if verification_result != SIGNATURE_VALID:
                 all_verifiable = False
+            if process_events_callback is not None:
+                process_events_callback()
         return (count, result, all_verifiable)
 
     def verbose_valid_message(self, result):
@@ -330,8 +339,8 @@
                 signers[uid] += 1
         result = []
         for uid, number in signers.items():
-             result.append( i18n.ngettext("{0} signed {1} commit", 
-                             "{0} signed {1} commits",
+             result.append( i18n.ngettext(u"{0} signed {1} commit", 
+                             u"{0} signed {1} commits",
                              number).format(uid, number) )
         return result
 
@@ -347,8 +356,8 @@
                 signers[authors] += 1
         result = []
         for authors, number in signers.items():
-            result.append( i18n.ngettext("{0} commit by author {1}", 
-                                 "{0} commits by author {1}",
+            result.append( i18n.ngettext(u"{0} commit by author {1}", 
+                                 u"{0} commits by author {1}",
                                  number).format(number, authors) )
         return result
 
@@ -363,8 +372,8 @@
                 signers[authors] += 1
         result = []
         for authors, number in signers.items():
-            result.append( i18n.ngettext("{0} commit by author {1}", 
-                                 "{0} commits by author {1}",
+            result.append( i18n.ngettext(u"{0} commit by author {1}", 
+                                 u"{0} commits by author {1}",
                                  number).format(number, authors) )
         return result
 
@@ -377,33 +386,33 @@
                 signers[fingerprint] += 1
         result = []
         for fingerprint, number in signers.items():
-            result.append( i18n.ngettext("Unknown key {0} signed {1} commit", 
-                                 "Unknown key {0} signed {1} commits",
+            result.append( i18n.ngettext(u"Unknown key {0} signed {1} commit", 
+                                 u"Unknown key {0} signed {1} commits",
                                  number).format(fingerprint, number) )
         return result
 
     def valid_commits_message(self, count):
         """returns message for number of commits"""
-        return i18n.gettext("{0} commits with valid signatures").format(
+        return i18n.gettext(u"{0} commits with valid signatures").format(
                                         count[SIGNATURE_VALID])
 
     def unknown_key_message(self, count):
         """returns message for number of commits"""
-        return i18n.ngettext("{0} commit with unknown key",
-                             "{0} commits with unknown keys",
+        return i18n.ngettext(u"{0} commit with unknown key",
+                             u"{0} commits with unknown keys",
                              count[SIGNATURE_KEY_MISSING]).format(
                                         count[SIGNATURE_KEY_MISSING])
 
     def commit_not_valid_message(self, count):
         """returns message for number of commits"""
-        return i18n.ngettext("{0} commit not valid",
-                             "{0} commits not valid",
+        return i18n.ngettext(u"{0} commit not valid",
+                             u"{0} commits not valid",
                              count[SIGNATURE_NOT_VALID]).format(
                                             count[SIGNATURE_NOT_VALID])
 
     def commit_not_signed_message(self, count):
         """returns message for number of commits"""
-        return i18n.ngettext("{0} commit not signed",
-                             "{0} commits not signed",
+        return i18n.ngettext(u"{0} commit not signed",
+                             u"{0} commits not signed",
                              count[SIGNATURE_NOT_SIGNED]).format(
                                         count[SIGNATURE_NOT_SIGNED])