Re: [PATCH 16/18] receive-pack: GPG-validate push certificates

2014-08-21 Thread David Turner
On Wed, 2014-08-20 at 12:38 -0700, Junio C Hamano wrote:
 David Turner dtur...@twopensource.com writes:
 
  On Wed, 2014-08-20 at 10:29 -0700, Junio C Hamano wrote:
  On Wed, Aug 20, 2014 at 9:56 AM, David Turner dtur...@twopensource.com 
  wrote:
   On Tue, 2014-08-19 at 15:06 -0700, Junio C Hamano wrote:
   Reusing the GPG signature check helpers we already have, verify
   the signature in receive-pack and give the results to the hooks
   via GIT_PUSH_CERT_{SIGNER,KEY,STATUS} environment variables.
  
   Policy decisions, such as accepting or rejecting a good signature by
   a key that is not fully trusted, is left to the hook and kept
   outside of the core.
  
   If I understand correctly, the hook does not have enough information to
   make this decision, because it is missing the date from the signature.
  
  The full certificate is available to the hook so anything we can do the 
  hook
  has enough information to do ;-)  But of course we should try to make it
  easier for the hook to validate the request.
 
  Excellent, then motivated hooks can do the right thing.
 
   This might allow an old signed push to be replayed, moving the head of a
   branch to an older state (say, one lacking the latest security updates).
  
  ... with old-sha1 recorded in the certificate?
 
  That does prevent most replays, but it does not prevent resurrection of
  a deleted branch by a replay of its initial creation (nor an undo of a
  force-push to rollback).  So I think we still need timestamps, but
  parsing them out of the cert is not terrible.
 
 As I aleady mentioned elsewhere, a more problematic thing about the
 push certificate as presented in 15/18 is that it does not say
 anything about where the push is going.  If you can capture a trial
 push to some random test repository I do with my signed push
 certificate, you could replay it to my public repository hosted at
 a more official site (say, k.org in the far distant future where it
 does not rely on ssh authentication to protect their services but
 uses the GPG signature on the push certificate to make sure it is I
 who is pushing).
 
 We can add a new pushed-to repository URL header line to the
 certificate, next to pushed-by ident time, and have the
 receiving end verify that it matches to prevent such a replay.  I
 wonder if we can further extend it to avoid replays to the same
 repository.

I think but am not certain that pushed-to repository URL, along with
the pushed-by ident time means that the nonce is not needed. The
nonce might make replays harder, but pushed-to/pushed-by makes replays
useless since the receiving server can determine that the user intended
to take this action at this time on this server. 

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 16/18] receive-pack: GPG-validate push certificates

2014-08-21 Thread Junio C Hamano
If you ignore the clock skew between the pusher and the receiver, then
you are correct,
but otherwise not quite.  Also by specifying that as nonce, not
server-timestamp,
the receiving end has a choice in how to generate and use the nonce
value. The only
requirement on the protocol is that the pusher must parrot it literally.

On Thu, Aug 21, 2014 at 4:59 PM, David Turner dtur...@twopensource.com wrote:
 On Wed, 2014-08-20 at 12:38 -0700, Junio C Hamano wrote:
 David Turner dtur...@twopensource.com writes:

  On Wed, 2014-08-20 at 10:29 -0700, Junio C Hamano wrote:
  On Wed, Aug 20, 2014 at 9:56 AM, David Turner dtur...@twopensource.com 
  wrote:
   On Tue, 2014-08-19 at 15:06 -0700, Junio C Hamano wrote:
   Reusing the GPG signature check helpers we already have, verify
   the signature in receive-pack and give the results to the hooks
   via GIT_PUSH_CERT_{SIGNER,KEY,STATUS} environment variables.
  
   Policy decisions, such as accepting or rejecting a good signature by
   a key that is not fully trusted, is left to the hook and kept
   outside of the core.
  
   If I understand correctly, the hook does not have enough information to
   make this decision, because it is missing the date from the signature.
 
  The full certificate is available to the hook so anything we can do the 
  hook
  has enough information to do ;-)  But of course we should try to make it
  easier for the hook to validate the request.
 
  Excellent, then motivated hooks can do the right thing.
 
   This might allow an old signed push to be replayed, moving the head of a
   branch to an older state (say, one lacking the latest security updates).
 
  ... with old-sha1 recorded in the certificate?
 
  That does prevent most replays, but it does not prevent resurrection of
  a deleted branch by a replay of its initial creation (nor an undo of a
  force-push to rollback).  So I think we still need timestamps, but
  parsing them out of the cert is not terrible.

 As I aleady mentioned elsewhere, a more problematic thing about the
 push certificate as presented in 15/18 is that it does not say
 anything about where the push is going.  If you can capture a trial
 push to some random test repository I do with my signed push
 certificate, you could replay it to my public repository hosted at
 a more official site (say, k.org in the far distant future where it
 does not rely on ssh authentication to protect their services but
 uses the GPG signature on the push certificate to make sure it is I
 who is pushing).

 We can add a new pushed-to repository URL header line to the
 certificate, next to pushed-by ident time, and have the
 receiving end verify that it matches to prevent such a replay.  I
 wonder if we can further extend it to avoid replays to the same
 repository.

 I think but am not certain that pushed-to repository URL, along with
 the pushed-by ident time means that the nonce is not needed. The
 nonce might make replays harder, but pushed-to/pushed-by makes replays
 useless since the receiving server can determine that the user intended
 to take this action at this time on this server.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 16/18] receive-pack: GPG-validate push certificates

2014-08-20 Thread David Turner
On Tue, 2014-08-19 at 15:06 -0700, Junio C Hamano wrote:
 Reusing the GPG signature check helpers we already have, verify
 the signature in receive-pack and give the results to the hooks
 via GIT_PUSH_CERT_{SIGNER,KEY,STATUS} environment variables.
 
 Policy decisions, such as accepting or rejecting a good signature by
 a key that is not fully trusted, is left to the hook and kept
 outside of the core.

If I understand correctly, the hook does not have enough information to
make this decision, because it is missing the date from the signature.
This might allow an old signed push to be replayed, moving the head of a
branch to an older state (say, one lacking the latest security updates).
I have not proven this, and it is entirely possible that I am wrong, but
I think it would be worth either documenting why this is not possible,
or fixing it if it is possible.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 16/18] receive-pack: GPG-validate push certificates

2014-08-20 Thread Junio C Hamano
On Wed, Aug 20, 2014 at 9:56 AM, David Turner dtur...@twopensource.com wrote:
 On Tue, 2014-08-19 at 15:06 -0700, Junio C Hamano wrote:
 Reusing the GPG signature check helpers we already have, verify
 the signature in receive-pack and give the results to the hooks
 via GIT_PUSH_CERT_{SIGNER,KEY,STATUS} environment variables.

 Policy decisions, such as accepting or rejecting a good signature by
 a key that is not fully trusted, is left to the hook and kept
 outside of the core.

 If I understand correctly, the hook does not have enough information to
 make this decision, because it is missing the date from the signature.

The full certificate is available to the hook so anything we can do the hook
has enough information to do ;-)  But of course we should try to make it
easier for the hook to validate the request.

I am not opposed to extract the timestamp from pushed-by header in the cert
and export it in another environment before calling the hook, but I am not sure
it is worth it, as that is already a single liner text information.

 This might allow an old signed push to be replayed, moving the head of a
 branch to an older state (say, one lacking the latest security updates).

... with old-sha1 recorded in the certificate?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 16/18] receive-pack: GPG-validate push certificates

2014-08-20 Thread David Turner
On Wed, 2014-08-20 at 10:29 -0700, Junio C Hamano wrote:
 On Wed, Aug 20, 2014 at 9:56 AM, David Turner dtur...@twopensource.com 
 wrote:
  On Tue, 2014-08-19 at 15:06 -0700, Junio C Hamano wrote:
  Reusing the GPG signature check helpers we already have, verify
  the signature in receive-pack and give the results to the hooks
  via GIT_PUSH_CERT_{SIGNER,KEY,STATUS} environment variables.
 
  Policy decisions, such as accepting or rejecting a good signature by
  a key that is not fully trusted, is left to the hook and kept
  outside of the core.
 
  If I understand correctly, the hook does not have enough information to
  make this decision, because it is missing the date from the signature.
 
 The full certificate is available to the hook so anything we can do the hook
 has enough information to do ;-)  But of course we should try to make it
 easier for the hook to validate the request.

Excellent, then motivated hooks can do the right thing.

  This might allow an old signed push to be replayed, moving the head of a
  branch to an older state (say, one lacking the latest security updates).
 
 ... with old-sha1 recorded in the certificate?

That does prevent most replays, but it does not prevent resurrection of
a deleted branch by a replay of its initial creation (nor an undo of a
force-push to rollback).  So I think we still need timestamps, but
parsing them out of the cert is not terrible.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 16/18] receive-pack: GPG-validate push certificates

2014-08-20 Thread Junio C Hamano
David Turner dtur...@twopensource.com writes:

 On Wed, 2014-08-20 at 10:29 -0700, Junio C Hamano wrote:
 On Wed, Aug 20, 2014 at 9:56 AM, David Turner dtur...@twopensource.com 
 wrote:
  On Tue, 2014-08-19 at 15:06 -0700, Junio C Hamano wrote:
  Reusing the GPG signature check helpers we already have, verify
  the signature in receive-pack and give the results to the hooks
  via GIT_PUSH_CERT_{SIGNER,KEY,STATUS} environment variables.
 
  Policy decisions, such as accepting or rejecting a good signature by
  a key that is not fully trusted, is left to the hook and kept
  outside of the core.
 
  If I understand correctly, the hook does not have enough information to
  make this decision, because it is missing the date from the signature.
 
 The full certificate is available to the hook so anything we can do the hook
 has enough information to do ;-)  But of course we should try to make it
 easier for the hook to validate the request.

 Excellent, then motivated hooks can do the right thing.

  This might allow an old signed push to be replayed, moving the head of a
  branch to an older state (say, one lacking the latest security updates).
 
 ... with old-sha1 recorded in the certificate?

 That does prevent most replays, but it does not prevent resurrection of
 a deleted branch by a replay of its initial creation (nor an undo of a
 force-push to rollback).  So I think we still need timestamps, but
 parsing them out of the cert is not terrible.

As I aleady mentioned elsewhere, a more problematic thing about the
push certificate as presented in 15/18 is that it does not say
anything about where the push is going.  If you can capture a trial
push to some random test repository I do with my signed push
certificate, you could replay it to my public repository hosted at
a more official site (say, k.org in the far distant future where it
does not rely on ssh authentication to protect their services but
uses the GPG signature on the push certificate to make sure it is I
who is pushing).

We can add a new pushed-to repository URL header line to the
certificate, next to pushed-by ident time, and have the
receiving end verify that it matches to prevent such a replay.  I
wonder if we can further extend it to avoid replays to the same
repository.

Instead of pushed-to, we can tweak the capability advertisement
sent from the server upon initial contact to advertise not just
push-cert, but push-cert=nonce, add a new push-nonce nonce
header to the certificate and then have the receiving end make sure
they are the same.  That way, the receiver can make sure it is not
being fed a certificate used when a different push was done to it or
somebody else and by doing so we do not even need pushed-to
repository URL header, perhaps?

I am still fuzzy how robust such a scheme be against MITM, though.
One way I can think of to attack the nonce-only scheme would be to
create a you can push anything here service, convince me to push
garbage there, and when I try to push to it, it can turn around and
act as a client to some high-value site the attacker does not even
control, grab the nonce, relay it back to me and advertise the
same nonce, have me sign the certificate to push garbage, and
relay that push session to the high-value target.

I am not sure if that is a valid threat model we would care about,
but with pushed-to repository URL the high-value target site can
notice that I am pushing garbage to the joker site and reject the
certificate.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 16/18] receive-pack: GPG-validate push certificates

2014-08-19 Thread Junio C Hamano
Reusing the GPG signature check helpers we already have, verify
the signature in receive-pack and give the results to the hooks
via GIT_PUSH_CERT_{SIGNER,KEY,STATUS} environment variables.

Policy decisions, such as accepting or rejecting a good signature by
a key that is not fully trusted, is left to the hook and kept
outside of the core.

Signed-off-by: Junio C Hamano gits...@pobox.com
---
 Documentation/git-receive-pack.txt | 22 ++
 builtin/receive-pack.c | 29 +
 t/t5534-push-signed.sh | 18 --
 3 files changed, 63 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-receive-pack.txt 
b/Documentation/git-receive-pack.txt
index 6c458af..a66884c 100644
--- a/Documentation/git-receive-pack.txt
+++ b/Documentation/git-receive-pack.txt
@@ -56,7 +56,21 @@ sha1-old and sha1-new should be valid objects in the 
repository.
 When accepting a signed push (see linkgit:git-push[1]), the signed
 push certificate is stored in a blob and an environment variable
 `GIT_PUSH_CERT` can be consulted for its object name.  See the
-description of `post-receive` hook for an example.
+description of `post-receive` hook for an example.  In addition, the
+certificate is verified using GPG and the result is exported with
+the following environment variables:
+
+GIT_PUSH_CERT_SIGNER::
+   The name and the e-mail address of the owner of the key that
+   signed the push certificate.
+
+GIT_PUSH_CERT_KEY::
+   The GPG key ID of the key that signed the push certificate.
+
+GIT_PUSH_CERT_STATUS::
+   The status of GPG verification of the push certificate,
+   using the same mnemonic as used in `%G?` format of `git log`
+   family of commands (see linkgit:git-log[1]).
 
 This hook is called before any refname is updated and before any
 fast-forward checks are performed.
@@ -106,13 +120,13 @@ the update.  Refs that were created will have sha1-old 
equal to
 0\{40}, otherwise sha1-old and sha1-new should be valid objects in
 the repository.
 
-The `GIT_PUSH_CERT` environment variable can be inspected, just as
+The `GIT_PUSH_CERT*` environment variables can be inspected, just as
 in `pre-receive` hook, after accepting a signed push.
 
 Using this hook, it is easy to generate mails describing the updates
 to the repository.  This example script sends one mail message per
 ref listing the commits pushed to the repository, and logs the push
-certificates of signed pushes to a file:
+certificates of signed pushes with good signatures to a file:
 
#!/bin/sh
# mail out commit update information.
@@ -129,7 +143,7 @@ certificates of signed pushes to a file:
mail -s Changes to ref $ref commit-list@mydomain
done
# log signed push certificate, if any
-   if test -n ${GIT_PUSH_CERT-}
+   if test -n ${GIT_PUSH_CERT-}  test ${GIT_PUSH_CERT_STATUS} = G
then
git cat-file blob ${GIT_PUSH_CERT} /var/log/push-log
fi
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index f30df8a..abdc296 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -15,6 +15,8 @@
 #include connected.h
 #include argv-array.h
 #include version.h
+#include tag.h
+#include gpg-interface.h
 
 static const char receive_pack_usage[] = git receive-pack git-dir;
 
@@ -48,6 +50,7 @@ static int shallow_update;
 static const char *alt_shallow_file;
 static struct strbuf push_cert = STRBUF_INIT;
 static unsigned char push_cert_sha1[20];
+static struct signature_check sigcheck;
 
 static enum deny_action parse_deny_action(const char *var, const char *value)
 {
@@ -260,12 +263,38 @@ static void prepare_push_cert_sha1(struct child_process 
*proc)
struct argv_array env = ARGV_ARRAY_INIT;
 
if (!already_done) {
+   struct strbuf gpg_output = STRBUF_INIT;
+   struct strbuf gpg_status = STRBUF_INIT;
+   int bogs /* beginning_of_gpg_sig */;
+
already_done = 1;
if (write_sha1_file(push_cert.buf, push_cert.len, blob, 
push_cert_sha1))
hashclr(push_cert_sha1);
+
+   memset(sigcheck, '\0', sizeof(sigcheck));
+   sigcheck.result = 'N';
+
+   bogs = parse_signature(push_cert.buf, push_cert.len);
+   if (verify_signed_buffer(push_cert.buf, bogs,
+push_cert.buf + bogs, push_cert.len - 
bogs,
+gpg_output, gpg_status)  0) {
+   ; /* error running gpg */
+   } else {
+   sigcheck.payload = push_cert.buf;
+   sigcheck.gpg_output = gpg_output.buf;
+   sigcheck.gpg_status = gpg_status.buf;
+   parse_gpg_output(sigcheck);
+   }
+
+   strbuf_release(gpg_output);
+   strbuf_release(gpg_status);
}
if