[PATCH 1/2] send-pack: typofix error message

[PATCH 1/2] send-pack: typofix error message

[Junio C Hamano]

The message identifies the process as receive-pack when it cannot fork the
sideband demultiplexer. We are actually a send-pack.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/send-pack.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index c1f6ddd..87833f4 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -334,7 +334,7 @@ int send_pack(struct send_pack_args *args,
 		demux.data = fd;
 		demux.out = -1;
 		if (start_async(&demux))
-			die("receive-pack: unable to fork off sideband demultiplexer");
+			die("send-pack: unable to fork off sideband demultiplexer");
 		in = demux.out;
 	}
 
-- 
1.7.7.rc0.186.g50963

[PATCH 2/2] push -s: skeleton

[Junio C Hamano]

If a tag is GPG-signed, and if you trust the cryptographic robustness of
the SHA-1 and GPG, you can guarantee that all the history leading to the
signed commit is not tampered with. However, it would be both cumbersome
and cluttering to sign each and every commit. Especially if you strive to
keep your history clean by tweaking, rewriting and polishing your commits
before pushing the resulting history out, many commits you will create
locally end up not mattering at all, and it is a waste of time to sign
them.

A better alternative could be to sign a "push certificate" (for the lack
of better name) every time you push, asserting that what commits you are
pushing to update which refs. The basic workflow goes like this:

 1. You push out your work with "git push -s";

 2. "git push", as usual, learns where the remote refs are and which refs
    are to be updated with this push. It prepares a text file in memory
    that looks like this using this information:

	Push-Certificate-Version: 1
	Pusher: Junio C Hamano <gitster@pobox.com> 1315427886 -0700
	Update: e83c51633... d4e58965f... refs/heads/master
	Update: 5a144a288... 7931f38a2... refs/heads/next

    An actual push certificate records full 40-char object name, but it is
    ellided for brevity here.

    The user then is asked to sign this push certificate using GPG. The
    result is carried to the other side (i.e. receive-pack). In the
    protocol exchange, this step comes immediately after the sender tells
    what the result of the push should be, before it sends the pack data.

 3. The receiving end will keep the signed push certificate in core,
    receives the pack data and unpacks (or stores and runs index-pack)
    as usual.

 4. A new phase to record the push certificate is introduced in the
    codepath after the receiving end runs receive_hook(). It is envisioned
    that this phase:

    a. parses the updated-to object names, and appends the push
       certificate (still GPG signed) to a note attached to each of the
       objects that will sit at the tip of the refs;

    b. verifies that the push certificate is signed with a GPG key that is
       authorized to push into this repository; and/or

    c. invokes pre-push-verify-signature hook, feeds the push
       certificate to it and asks it to veto the ref updates.

And here is a skeleton to implement it. It has all the necessary protocol
extensions implemented (although I do not know if we need separate
codepath for stateless RPC mode), but does not have subroutines to:

 - Sign the certificate with GPG key;

 - Parse the signed certificate to identify the updated-to objects, and
   add the certificate as notes to them;

 - Verify the certificate and find out what GPG key was used to sign it;
   or

 - Invoke and feed the certificate to pre-push-verify-hook.

all of which should be fairly trivial. The places that needs to implement
these are clearly marked with large comments, so I'll leave it up to other
people who are interested in the topic to fill in the blanks ;-)

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/push.c         |    1 +
 builtin/receive-pack.c |   54 +++++++++++++++++++++++++++++++++++++++-
 builtin/send-pack.c    |   65 +++++++++++++++++++++++++++++++++++++++++++++---
 send-pack.h            |    1 +
 transport.c            |    4 +++
 transport.h            |    4 +++
 6 files changed, 124 insertions(+), 5 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 35cce53..2238f4e 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -261,6 +261,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_BIT('u', "set-upstream", &flags, "set upstream for git pull/status",
 			TRANSPORT_PUSH_SET_UPSTREAM),
 		OPT_BOOLEAN(0, "progress", &progress, "force progress reporting"),
+		OPT_BIT('s', "signed", &flags, "GPG sign the push", TRANSPORT_PUSH_SIGNED),
 		OPT_END()
 	};
 
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index ae164da..307fc3b 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -30,12 +30,14 @@ static int receive_unpack_limit = -1;
 static int transfer_unpack_limit = -1;
 static int unpack_limit = 100;
 static int report_status;
+static int signed_push;
 static int use_sideband;
 static int prefer_ofs_delta = 1;
 static int auto_update_server_info;
 static int auto_gc = 1;
 static const char *head_name;
 static int sent_capabilities;
+static char *push_certificate;
 
 static enum deny_action parse_deny_action(const char *var, const char *value)
 {
@@ -114,7 +116,7 @@ static int show_ref(const char *path, const unsigned char *sha1, int flag, void
 	else
 		packet_write(1, "%s %s%c%s%s\n",
 			     sha1_to_hex(sha1), path, 0,
-			     " report-status delete-refs side-band-64k",
+			     " report-status delete-refs side-band-64k signed-push",
 			     prefer_ofs_delta ? " ofs-delta" : "");
 	sent_capabilities = 1;
 	return 0;
@@ -579,6 +581,31 @@ static void check_aliased_updates(struct command *commands)
 	string_list_clear(&ref_list, 0);
 }
 
+static int record_signed_push(char *cert)
+{
+	/*
+	 * This is the place for you to parse the signed push
+	 * certificate, grab the commit object names the push updates
+	 * refs to, and append the certificate to the notes to these
+	 * commits.
+	 *
+	 * You could also feed the signed push certificate to GPG,
+	 * verify the signer identity, and all the other fun stuff,
+	 * including feeding it to "pre-push-verify-signature" hook.
+	 *
+	 * Here we just throw it to stderr to demonstrate that the
+	 * codepath is being exercised.
+	 */
+	char *cp, *ep;
+	for (cp = cert; *cp; cp = ep) {
+		ep = strchrnul(cp, '\n');
+		if (*ep == '\n')
+			ep++;
+		fprintf(stderr, "RSP: %.*s", (int)(ep - cp), cp);
+	}
+	return 0;
+}
+
 static void execute_commands(struct command *commands, const char *unpacker_error)
 {
 	struct command *cmd;
@@ -596,6 +623,12 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
 		return;
 	}
 
+	if (push_certificate && record_signed_push(push_certificate)) {
+		for (cmd = commands; cmd; cmd = cmd->next)
+			cmd->error_string = "n/a (push signature error)";
+		return;
+	}
+
 	check_aliased_updates(commands);
 
 	head_name = resolve_ref("HEAD", sha1, 0, NULL);
@@ -636,6 +669,8 @@ static struct command *read_head_info(void)
 				report_status = 1;
 			if (strstr(refname + reflen + 1, "side-band-64k"))
 				use_sideband = LARGE_PACKET_MAX;
+			if (strstr(refname + reflen + 1, "signed-push"))
+				signed_push = 1;
 		}
 		cmd = xcalloc(1, sizeof(struct command) + len - 80);
 		hashcpy(cmd->old_sha1, old_sha1);
@@ -731,6 +766,21 @@ static const char *unpack(void)
 	}
 }
 
+static char *receive_push_certificate(void)
+{
+	struct strbuf cert = STRBUF_INIT;
+	for (;;) {
+		char line[1000];
+		int len;
+
+		len = packet_read_line(0, line, sizeof(line));
+		if (!len)
+			break;
+		strbuf_add(&cert, line, len);
+	}
+	return strbuf_detach(&cert, NULL);
+}
+
 static void report(struct command *commands, const char *unpack_status)
 {
 	struct command *cmd;
@@ -846,6 +896,8 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 	if ((commands = read_head_info()) != NULL) {
 		const char *unpack_status = NULL;
 
+		if (signed_push)
+			push_certificate = receive_push_certificate();
 		if (!delete_only(commands))
 			unpack_status = unpack();
 		execute_commands(commands, unpack_status);
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 87833f4..3193f34 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -237,6 +237,27 @@ static int sideband_demux(int in, int out, void *data)
 	return ret;
 }
 
+static void sign_push_certificate(struct strbuf *cert)
+{
+	/*
+	 * Here, take the contents of cert->buf, and have the user GPG
+	 * sign it, and read it back in the strbuf.
+	 *
+	 * You may want to append some extra info to cert before giving
+	 * it to GPG, possibly via a hook.
+	 *
+	 * Here we upcase them just to demonstrate that the codepath
+	 * is being exercised.
+	 */
+	char *cp;
+	for (cp = cert->buf; *cp; cp++) {
+		int ch = *cp;
+		if ('a' <= ch && ch <= 'z')
+			*cp = toupper(ch);
+	}
+	return;
+}
+
 int send_pack(struct send_pack_args *args,
 	      int fd[], struct child_process *conn,
 	      struct ref *remote_refs,
@@ -250,9 +271,11 @@ int send_pack(struct send_pack_args *args,
 	int allow_deleting_refs = 0;
 	int status_report = 0;
 	int use_sideband = 0;
+	int signed_push = 0;
 	unsigned cmds_sent = 0;
 	int ret;
 	struct async demux;
+	struct strbuf push_cert = STRBUF_INIT;
 
 	/* Does the other end support the reporting? */
 	if (server_supports("report-status"))
@@ -270,6 +293,19 @@ int send_pack(struct send_pack_args *args,
 		return 0;
 	}
 
+	if (args->signed_push) {
+		if (server_supports("signed-push"))
+			signed_push = !args->dry_run;
+		else
+			warning("The receiving side does not support signed-push");
+	}
+
+	if (signed_push) {
+		const char *committer_info = git_committer_info(0);
+		strbuf_addstr(&push_cert, "Push-Certificate-Version: 1\n");
+		strbuf_addf(&push_cert, "Pusher: %s\n", committer_info);
+	}
+
 	/*
 	 * Finally, tell the other end!
 	 */
@@ -301,15 +337,19 @@ int send_pack(struct send_pack_args *args,
 			char *old_hex = sha1_to_hex(ref->old_sha1);
 			char *new_hex = sha1_to_hex(ref->new_sha1);
 
-			if (!cmds_sent && (status_report || use_sideband)) {
-				packet_buf_write(&req_buf, "%s %s %s%c%s%s",
+			if (!cmds_sent &&
+			    (status_report || use_sideband || signed_push))
+				packet_buf_write(&req_buf, "%s %s %s%c%s%s%s",
 					old_hex, new_hex, ref->name, 0,
 					status_report ? " report-status" : "",
-					use_sideband ? " side-band-64k" : "");
-			}
+					use_sideband ? " side-band-64k" : "",
+					signed_push ? " signed-push" : "");
 			else
 				packet_buf_write(&req_buf, "%s %s %s",
 					old_hex, new_hex, ref->name);
+			if (signed_push && hashcmp(ref->old_sha1, ref->new_sha1))
+				strbuf_addf(&push_cert, "Update: %s %s %s\n",
+					    old_hex, new_hex, ref->name);
 			ref->status = status_report ?
 				REF_STATUS_EXPECTING_REPORT :
 				REF_STATUS_OK;
@@ -326,6 +366,23 @@ int send_pack(struct send_pack_args *args,
 		safe_write(out, req_buf.buf, req_buf.len);
 		packet_flush(out);
 	}
+
+	if (signed_push) {
+		char *cp, *ep;
+
+		sign_push_certificate(&push_cert);
+		strbuf_reset(&req_buf);
+		for (cp = push_cert.buf; *cp; cp = ep) {
+			ep = strchrnul(cp, '\n');
+			if (*ep == '\n')
+				ep++;
+			packet_buf_write(&req_buf, "%.*s",
+					 (int)(ep - cp), cp);
+		}
+		/* Do we need anything funky for stateless rpc? */
+		safe_write(out, req_buf.buf, req_buf.len);
+		packet_flush(out);
+	}
 	strbuf_release(&req_buf);
 
 	if (use_sideband && cmds_sent) {
diff --git a/send-pack.h b/send-pack.h
index 05d7ab1..754943e 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -11,6 +11,7 @@ struct send_pack_args {
 		use_thin_pack:1,
 		use_ofs_delta:1,
 		dry_run:1,
+		signed_push:1,
 		stateless_rpc:1;
 };
 
diff --git a/transport.c b/transport.c
index fa279d5..7a7ffe4 100644
--- a/transport.c
+++ b/transport.c
@@ -476,6 +476,9 @@ static int set_git_option(struct git_transport_options *opts,
 		else
 			opts->depth = atoi(value);
 		return 0;
+	} else if (!strcmp(name, TRANS_OPT_SIGNED_PUSH)) {
+		opts->signed_push = !!value;
+		return 0;
 	}
 	return 1;
 }
@@ -793,6 +796,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 	args.progress = transport->progress;
 	args.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);
 	args.porcelain = !!(flags & TRANSPORT_PUSH_PORCELAIN);
+	args.signed_push = !!(flags & TRANSPORT_PUSH_SIGNED);
 
 	ret = send_pack(&args, data->fd, data->conn, remote_refs,
 			&data->extra_have);
diff --git a/transport.h b/transport.h
index 059b330..d2fa478 100644
--- a/transport.h
+++ b/transport.h
@@ -8,6 +8,7 @@ struct git_transport_options {
 	unsigned thin : 1;
 	unsigned keep : 1;
 	unsigned followtags : 1;
+	unsigned signed_push : 1;
 	int depth;
 	const char *uploadpack;
 	const char *receivepack;
@@ -102,6 +103,7 @@ struct transport {
 #define TRANSPORT_PUSH_PORCELAIN 16
 #define TRANSPORT_PUSH_SET_UPSTREAM 32
 #define TRANSPORT_RECURSE_SUBMODULES_CHECK 64
+#define TRANSPORT_PUSH_SIGNED 128
 
 #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
 
@@ -128,6 +130,8 @@ struct transport *transport_get(struct remote *, const char *);
 /* Aggressively fetch annotated tags if possible */
 #define TRANS_OPT_FOLLOWTAGS "followtags"
 
+#define TRANS_OPT_SIGNED_PUSH "signedpush"
+
 /**
  * Returns 0 if the option was used, non-zero otherwise. Prints a
  * message to stderr if the option is not used.
-- 
1.7.7.rc0.186.g50963

Re: [PATCH 2/2] push -s: skeleton

[Shawn Pearce]

On Wed, Sep 7, 2011 at 13:57, Junio C Hamano <gitster@pobox.com> wrote:
> If a tag is GPG-signed, and if you trust the cryptographic robustness of
> the SHA-1 and GPG, you can guarantee that all the history leading to the
> signed commit is not tampered with. However, it would be both cumbersome
> and cluttering to sign each and every commit. Especially if you strive to
> keep your history clean by tweaking, rewriting and polishing your commits
> before pushing the resulting history out, many commits you will create
> locally end up not mattering at all, and it is a waste of time to sign
> them.
>
> A better alternative could be to sign a "push certificate" (for the lack
> of better name) every time you push, asserting that what commits you are
> pushing to update which refs. The basic workflow goes like this:
>
>  1. You push out your work with "git push -s";

Yay!

> And here is a skeleton to implement it. It has all the necessary protocol
> extensions implemented (although I do not know if we need separate
> codepath for stateless RPC mode), but does not have subroutines to:

Yea, its broken for stateless RPC. See below.

> +static char *receive_push_certificate(void)
> +{
> +       struct strbuf cert = STRBUF_INIT;
> +       for (;;) {
> +               char line[1000];

1000 isn't enough for some certificates. Imagine pushing a Gerrit Code
Review managed repository with 2M worth of advertisement data at once.
You can't sign that in 1000 bytes.

> @@ -326,6 +366,23 @@ int send_pack(struct send_pack_args *args,
>                safe_write(out, req_buf.buf, req_buf.len);
>                packet_flush(out);
>        }
> +
> +       if (signed_push) {
> +               char *cp, *ep;
> +
> +               sign_push_certificate(&push_cert);
> +               strbuf_reset(&req_buf);
> +               for (cp = push_cert.buf; *cp; cp = ep) {
> +                       ep = strchrnul(cp, '\n');
> +                       if (*ep == '\n')
> +                               ep++;
> +                       packet_buf_write(&req_buf, "%.*s",
> +                                        (int)(ep - cp), cp);
> +               }
> +               /* Do we need anything funky for stateless rpc? */

Yes. Above we flushed the req_buf and send that in an HTTP request.
You need to hoist this block above the "if (args->stateless_rpc)"
segment.

-- 
Shawn.

Re: [PATCH 2/2] push -s: skeleton

[Nguyen Thai Ngoc Duy]

On Thu, Sep 8, 2011 at 6:57 AM, Junio C Hamano <gitster@pobox.com> wrote:
> A better alternative could be to sign a "push certificate" (for the lack
> of better name) every time you push, asserting that what commits you are
> pushing to update which refs. The basic workflow goes like this:
>
>  1. You push out your work with "git push -s";
>
>  2. "git push", as usual, learns where the remote refs are and which refs
>    are to be updated with this push. It prepares a text file in memory
>    that looks like this using this information:
>
>        Push-Certificate-Version: 1
>        Pusher: Junio C Hamano <gitster@pobox.com> 1315427886 -0700
>        Update: e83c51633... d4e58965f... refs/heads/master
>        Update: 5a144a288... 7931f38a2... refs/heads/next
>
>    An actual push certificate records full 40-char object name, but it is
>    ellided for brevity here.
>
>    The user then is asked to sign this push certificate using GPG. The
>    result is carried to the other side (i.e. receive-pack). In the
>    protocol exchange, this step comes immediately after the sender tells
>    what the result of the push should be, before it sends the pack data.
>
>  3. The receiving end will keep the signed push certificate in core,
>    receives the pack data and unpacks (or stores and runs index-pack)
>    as usual.
>
>  4. A new phase to record the push certificate is introduced in the
>    codepath after the receiving end runs receive_hook(). It is envisioned
>    that this phase:
>
>    a. parses the updated-to object names, and appends the push
>       certificate (still GPG signed) to a note attached to each of the
>       objects that will sit at the tip of the refs;

I recall Gentoo wanted something like this (recording who pushes
what). Pulling Robin in if he has any comments.
-- 
Duy

Re: [PATCH 2/2] push -s: skeleton

[Junio C Hamano]

Shawn Pearce <spearce@spearce.org> writes:

> Yes. Above we flushed the req_buf and send that in an HTTP request.
> You need to hoist this block above the "if (args->stateless_rpc)"
> segment.

What do you mean by "hoist"? For the req advertisement, it seems that you
are not hoisting anything but duplicating the code, turning safe_write()
followed by flush into packet-buf-flush and sending the result over the
sideband. Shouldn't this new data be sent over the sideband-to-http the
same way?

Unless you do not want signed push over http, that is...

 builtin/send-pack.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 3193f34..37e0313 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -379,9 +379,13 @@ int send_pack(struct send_pack_args *args,
 			packet_buf_write(&req_buf, "%.*s",
 					 (int)(ep - cp), cp);
 		}
-		/* Do we need anything funky for stateless rpc? */
-		safe_write(out, req_buf.buf, req_buf.len);
-		packet_flush(out);
+		if (args->stateless_rpc) {
+			packet_buf_flush(&req_buf);
+			send_sideband(out, -1, req_buf.buf, req_buf.len, LARGE_PACKET_MAX);
+		} else {
+			safe_write(out, req_buf.buf, req_buf.len);
+			packet_flush(out);
+		}
 	}
 	strbuf_release(&req_buf);
 

Re: [PATCH 2/2] push -s: skeleton

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

>> ...
>>  4. A new phase to record the push certificate is introduced in the
>>    codepath after the receiving end runs receive_hook(). It is envisioned
>>    that this phase:
>>
>>    a. parses the updated-to object names, and appends the push
>>       certificate (still GPG signed) to a note attached to each of the
>>       objects that will sit at the tip of the refs;
>
> I recall Gentoo wanted something like this (recording who pushes
> what). Pulling Robin in if he has any comments.

As the beauty of this approach is that we can update and tailor what the
receiving end does using the information given from the server, it is a
strange thing to do to chomp this list in the middle at a funny place
here.

Re: [PATCH 2/2] push -s: skeleton

[Shawn Pearce]

On Wed, Sep 7, 2011 at 15:21, Junio C Hamano <gitster@pobox.com> wrote:
> Shawn Pearce <spearce@spearce.org> writes:
>
>> Yes. Above we flushed the req_buf and send that in an HTTP request.
>> You need to hoist this block above the "if (args->stateless_rpc)"
>> segment.
>
> What do you mean by "hoist"? For the req advertisement, it seems that you
> are not hoisting anything but duplicating the code, turning safe_write()
> followed by flush into packet-buf-flush and sending the result over the
> sideband. Shouldn't this new data be sent over the sideband-to-http the
> same way?
>
> Unless you do not want signed push over http, that is...

We do.

> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index 3193f34..37e0313 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -379,9 +379,13 @@ int send_pack(struct send_pack_args *args,
>                        packet_buf_write(&req_buf, "%.*s",
>                                         (int)(ep - cp), cp);
>                }
> -               /* Do we need anything funky for stateless rpc? */
> -               safe_write(out, req_buf.buf, req_buf.len);
> -               packet_flush(out);
> +               if (args->stateless_rpc) {
> +                       packet_buf_flush(&req_buf);
> +                       send_sideband(out, -1, req_buf.buf, req_buf.len, LARGE_PACKET_MAX);
> +               } else {
> +                       safe_write(out, req_buf.buf, req_buf.len);
> +                       packet_flush(out);
> +               }

This sounds too late to me.  I think you just caused 2 HTTP POSTs, one
a partial one with the commands and no pack data, and another with the
push certificate and the pack. Neither is useful.

-- 
Shawn.

Re: [PATCH 2/2] push -s: skeleton

[Robin H. Johnson]

[-- Attachment #1: Type: text/plain, Size: 2111 bytes --]

On Wed, Sep 07, 2011 at 01:57:27PM -0700,  Junio C Hamano wrote:
> If a tag is GPG-signed, and if you trust the cryptographic robustness of
> the SHA-1 and GPG, you can guarantee that all the history leading to the
> signed commit is not tampered with. However, it would be both cumbersome
> and cluttering to sign each and every commit. Especially if you strive to
> keep your history clean by tweaking, rewriting and polishing your commits
> before pushing the resulting history out, many commits you will create
> locally end up not mattering at all, and it is a waste of time to sign
> them.
Thanks to pcloud for including me on the thread. I do find the idea of
these push-certificates very interesting and useful, but I think they
will do best to augment signed commits, not replace them.

There's a couple of related things we've been considering on the Gentoo
side:
- detached signatures of blobs (either the SHA1 of the blob or the blob
  itself)
- The signature covering the message+blob details, but NOT the chain of
  history: this opens up the ability to cherry-pick and rebase iff there
  are no conflicts and the blobs are identical, all while preserving the
  signature.
- concerns about a pre-image attack against Git. tl;dr version:
  1. Attacker prepares decoy file in advance, that hashes to the same as
     the malicious file.
  2. Attacker sends decoy in as an innocuous real commit.
  3. Months later, the attacker breaks into the system and alters the
     packfile to include the new malicious file.
  4. All new clones from that point forward get the malicious version.

Re your comment on always needing to resign commits above, we'd been
considering post-signing commits, not when they are initially made.
After your commit is clean and ready to ship, you can fire the commit
ids into the signature tool, which can generate a detached signature
note for each commit.

-- 
Robin Hugh Johnson
Gentoo Linux: Developer, Trustee & Infrastructure Lead
E-Mail     : robbat2@gentoo.org
GnuPG FP   : 11AC BA4F 4778 E3F6 E4ED  F38E B27B 944E 3488 4E85

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 330 bytes --]

[PATCH 3/2] Split GPG interface into its own helper library

[Junio C Hamano]

This moves existing code from builtin/tag.c (for signing) and
builtin/verify-tag.c (for verifying) to a new gpg-interface.c file to
provide a more generic library interface.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile             |    2 +
 builtin/tag.c        |   60 ++++----------------------------
 builtin/verify-tag.c |   35 ++-----------------
 gpg-interface.c      |   94 ++++++++++++++++++++++++++++++++++++++++++++++++++
 gpg-interface.h      |   11 ++++++
 5 files changed, 117 insertions(+), 85 deletions(-)
 create mode 100644 gpg-interface.c
 create mode 100644 gpg-interface.h

diff --git a/Makefile b/Makefile
index 8d6d451..2183223 100644
--- a/Makefile
+++ b/Makefile
@@ -530,6 +530,7 @@ LIB_H += exec_cmd.h
 LIB_H += fsck.h
 LIB_H += gettext.h
 LIB_H += git-compat-util.h
+LIB_H += gpg-interface.h
 LIB_H += graph.h
 LIB_H += grep.h
 LIB_H += hash.h
@@ -620,6 +621,7 @@ LIB_OBJS += entry.o
 LIB_OBJS += environment.o
 LIB_OBJS += exec_cmd.o
 LIB_OBJS += fsck.o
+LIB_OBJS += gpg-interface.o
 LIB_OBJS += graph.o
 LIB_OBJS += grep.o
 LIB_OBJS += hash.o
diff --git a/builtin/tag.c b/builtin/tag.c
index 667515e..e9d36fa 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -14,6 +14,7 @@
 #include "parse-options.h"
 #include "diff.h"
 #include "revision.h"
+#include "gpg-interface.h"
 
 static const char * const git_tag_usage[] = {
 	"git tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]",
@@ -208,60 +209,13 @@ static int verify_tag(const char *name, const char *ref,
 
 static int do_sign(struct strbuf *buffer)
 {
-	struct child_process gpg;
-	const char *args[4];
-	char *bracket;
-	int len;
-	int i, j;
+	const char *key;
 
-	if (!*signingkey) {
-		if (strlcpy(signingkey, git_committer_info(IDENT_ERROR_ON_NO_NAME),
-				sizeof(signingkey)) > sizeof(signingkey) - 1)
-			return error(_("committer info too long."));
-		bracket = strchr(signingkey, '>');
-		if (bracket)
-			bracket[1] = '\0';
-	}
-
-	/* When the username signingkey is bad, program could be terminated
-	 * because gpg exits without reading and then write gets SIGPIPE. */
-	signal(SIGPIPE, SIG_IGN);
-
-	memset(&gpg, 0, sizeof(gpg));
-	gpg.argv = args;
-	gpg.in = -1;
-	gpg.out = -1;
-	args[0] = "gpg";
-	args[1] = "-bsau";
-	args[2] = signingkey;
-	args[3] = NULL;
-
-	if (start_command(&gpg))
-		return error(_("could not run gpg."));
-
-	if (write_in_full(gpg.in, buffer->buf, buffer->len) != buffer->len) {
-		close(gpg.in);
-		close(gpg.out);
-		finish_command(&gpg);
-		return error(_("gpg did not accept the tag data"));
-	}
-	close(gpg.in);
-	len = strbuf_read(buffer, gpg.out, 1024);
-	close(gpg.out);
-
-	if (finish_command(&gpg) || !len || len < 0)
-		return error(_("gpg failed to sign the tag"));
-
-	/* Strip CR from the line endings, in case we are on Windows. */
-	for (i = j = 0; i < buffer->len; i++)
-		if (buffer->buf[i] != '\r') {
-			if (i != j)
-				buffer->buf[j] = buffer->buf[i];
-			j++;
-		}
-	strbuf_setlen(buffer, j);
-
-	return 0;
+	if (*signingkey)
+		key = signingkey;
+	else
+		key = git_committer_info(IDENT_ERROR_ON_NO_NAME|IDENT_NO_DATE);
+	return sign_buffer(buffer, key);
 }
 
 static const char tag_template[] =
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 3134766..8b4f742 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -11,6 +11,7 @@
 #include "run-command.h"
 #include <signal.h>
 #include "parse-options.h"
+#include "gpg-interface.h"
 
 static const char * const verify_tag_usage[] = {
 		"git verify-tag [-v|--verbose] <tag>...",
@@ -19,42 +20,12 @@ static const char * const verify_tag_usage[] = {
 
 static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
 {
-	struct child_process gpg;
-	const char *args_gpg[] = {"gpg", "--verify", "FILE", "-", NULL};
-	char path[PATH_MAX];
-	size_t len;
-	int fd, ret;
+	int len;
 
-	fd = git_mkstemp(path, PATH_MAX, ".git_vtag_tmpXXXXXX");
-	if (fd < 0)
-		return error("could not create temporary file '%s': %s",
-						path, strerror(errno));
-	if (write_in_full(fd, buf, size) < 0)
-		return error("failed writing temporary file '%s': %s",
-						path, strerror(errno));
-	close(fd);
-
-	/* find the length without signature */
 	len = parse_signature(buf, size);
 	if (verbose)
 		write_in_full(1, buf, len);
-
-	memset(&gpg, 0, sizeof(gpg));
-	gpg.argv = args_gpg;
-	gpg.in = -1;
-	args_gpg[2] = path;
-	if (start_command(&gpg)) {
-		unlink(path);
-		return error("could not run gpg.");
-	}
-
-	write_in_full(gpg.in, buf, len);
-	close(gpg.in);
-	ret = finish_command(&gpg);
-
-	unlink_or_warn(path);
-
-	return ret;
+	return verify_signed_buffer(buf, size, len);
 }
 
 static int verify_tag(const char *name, int verbose)
diff --git a/gpg-interface.c b/gpg-interface.c
new file mode 100644
index 0000000..b83cca1
--- /dev/null
+++ b/gpg-interface.c
@@ -0,0 +1,94 @@
+/*
+ * Copyright (c) 2011, Google Inc.
+ */
+#include "cache.h"
+#include "run-command.h"
+#include "strbuf.h"
+#include "gpg-interface.h"
+#include "sigchain.h"
+
+int sign_buffer(struct strbuf *buffer, const char *signing_key)
+{
+	struct child_process gpg;
+	const char *args[4];
+	ssize_t len;
+	int i, j;
+
+	memset(&gpg, 0, sizeof(gpg));
+	gpg.argv = args;
+	gpg.in = -1;
+	gpg.out = -1;
+	args[0] = "gpg";
+	args[1] = "-bsau";
+	args[2] = signing_key;
+	args[3] = NULL;
+
+	if (start_command(&gpg))
+		return error(_("could not run gpg."));
+
+	/*
+	 * When the username signingkey is bad, program could be terminated
+	 * because gpg exits without reading and then write gets SIGPIPE.
+	 */
+	sigchain_push(SIGPIPE, SIG_IGN);
+
+	if (write_in_full(gpg.in, buffer->buf, buffer->len) != buffer->len) {
+		close(gpg.in);
+		close(gpg.out);
+		finish_command(&gpg);
+		return error(_("gpg did not accept the data"));
+	}
+	close(gpg.in);
+	len = strbuf_read(buffer, gpg.out, 1024);
+	close(gpg.out);
+
+	sigchain_pop(SIGPIPE);
+
+	if (finish_command(&gpg) || !len || len < 0)
+		return error(_("gpg failed to sign the data"));
+
+	/* Strip CR from the line endings, in case we are on Windows. */
+	for (i = j = 0; i < buffer->len; i++)
+		if (buffer->buf[i] != '\r') {
+			if (i != j)
+				buffer->buf[j] = buffer->buf[i];
+			j++;
+		}
+	strbuf_setlen(buffer, j);
+
+	return 0;
+}
+
+int verify_signed_buffer(const char *buf, size_t total, size_t payload)
+{
+	struct child_process gpg;
+	const char *args_gpg[] = {"gpg", "--verify", "FILE", "-", NULL};
+	char path[PATH_MAX];
+	int fd, ret;
+
+	fd = git_mkstemp(path, PATH_MAX, ".git_vtag_tmpXXXXXX");
+	if (fd < 0)
+		return error("could not create temporary file '%s': %s",
+			     path, strerror(errno));
+	if (write_in_full(fd, buf, total) < 0)
+		return error("failed writing temporary file '%s': %s",
+			     path, strerror(errno));
+	close(fd);
+
+	memset(&gpg, 0, sizeof(gpg));
+	gpg.argv = args_gpg;
+	gpg.in = -1;
+	args_gpg[2] = path;
+	if (start_command(&gpg)) {
+		unlink(path);
+		return error("could not run gpg.");
+	}
+
+	write_in_full(gpg.in, buf, payload);
+	close(gpg.in);
+	ret = finish_command(&gpg);
+
+	unlink_or_warn(path);
+
+	return ret;
+}
diff --git a/gpg-interface.h b/gpg-interface.h
new file mode 100644
index 0000000..7689357
--- /dev/null
+++ b/gpg-interface.h
@@ -0,0 +1,11 @@
+#ifndef GPG_INTERFACE_H
+#define GPG_INTERFACE_H
+
+/*
+ * Copyright (c) 2011, Google Inc.
+ */
+
+extern int sign_buffer(struct strbuf *buffer, const char *signing_key);
+extern int verify_signed_buffer(const char *buffer, size_t total, size_t payload);
+
+#endif
-- 
1.7.7.rc0.188.g3793ac

[PATCH 4/2] push -s: send signed push certificate

[Junio C Hamano]

And this uses the GPG interface to sign the push certificate. The format
of the signed certificate is very similar to a signed tag, in that the
result is a concatenation of the payload, immediately followed by a
detached signature.

This places the same constraint as an annotated tag on the push
certificate payload; it has to be a text file and the final line
must not be an incomplete line.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/send-pack.c |   29 ++++++++++++-----------------
 1 files changed, 12 insertions(+), 17 deletions(-)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 3193f34..298e181 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -8,6 +8,7 @@
 #include "send-pack.h"
 #include "quote.h"
 #include "transport.h"
+#include "gpg-interface.h"
 
 static const char send_pack_usage[] =
 "git send-pack [--all | --mirror] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [<host>:]<directory> [<ref>...]\n"
@@ -237,25 +238,18 @@ static int sideband_demux(int in, int out, void *data)
 	return ret;
 }
 
-static void sign_push_certificate(struct strbuf *cert)
+/*
+ * Take the contents of cert->buf, and have the user GPG sign it, and
+ * read it back in the strbuf.
+ */
+static int sign_push_certificate(struct strbuf *cert)
 {
 	/*
-	 * Here, take the contents of cert->buf, and have the user GPG
-	 * sign it, and read it back in the strbuf.
-	 *
-	 * You may want to append some extra info to cert before giving
-	 * it to GPG, possibly via a hook.
-	 *
-	 * Here we upcase them just to demonstrate that the codepath
-	 * is being exercised.
+	 * You may want to append some extra info to cert before
+	 * giving it to GPG, possibly via a hook, here.
 	 */
-	char *cp;
-	for (cp = cert->buf; *cp; cp++) {
-		int ch = *cp;
-		if ('a' <= ch && ch <= 'z')
-			*cp = toupper(ch);
-	}
-	return;
+
+	return sign_buffer(cert, git_committer_info(IDENT_NO_DATE));
 }
 
 int send_pack(struct send_pack_args *args,
@@ -370,7 +364,8 @@ int send_pack(struct send_pack_args *args,
 	if (signed_push) {
 		char *cp, *ep;
 
-		sign_push_certificate(&push_cert);
+		if (sign_push_certificate(&push_cert))
+			return error(_("failed to sign push certificate"));
 		strbuf_reset(&req_buf);
 		for (cp = push_cert.buf; *cp; cp = ep) {
 			ep = strchrnul(cp, '\n');
-- 
1.7.7.rc0.188.g3793ac

[PATCH 5/2] push -s: receiving end

[Junio C Hamano]

This stores the GPG signed push certificate in the receiving repository
using the notes mechanism. The certificate is appended to a note in the
refs/notes/signed-push tree for each object that appears on the right
hand side of the push certificate, i.e. the object that was pushed to
update the tip of a ref.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * This is largely untested, so take it with a large grain of salt.
   This concludes tonight's hacking session for me.

 builtin/receive-pack.c |   74 +++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 67 insertions(+), 7 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 307fc3b..257f2a5 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -11,6 +11,11 @@
 #include "transport.h"
 #include "string-list.h"
 #include "sha1-array.h"
+#include "gpg-interface.h"
+#include "notes.h"
+#include "notes-merge.h"
+#include "blob.h"
+#include "tag.h"
 
 static const char receive_pack_usage[] = "git receive-pack <git-dir>";
 
@@ -156,6 +161,7 @@ struct command {
 };
 
 static const char pre_receive_hook[] = "hooks/pre-receive";
+static const char pre_receive_signature_hook[] = "hooks/pre-receive-signature";
 static const char post_receive_hook[] = "hooks/post-receive";
 
 static void rp_error(const char *err, ...) __attribute__((format (printf, 1, 2)));
@@ -581,6 +587,22 @@ static void check_aliased_updates(struct command *commands)
 	string_list_clear(&ref_list, 0);
 }
 
+static void get_note_text(struct strbuf *buf, struct notes_tree *t,
+			  const unsigned char *object)
+{
+	const unsigned char *sha1 = get_note(t, object);
+	char *text;
+	unsigned long len;
+	enum object_type type;
+
+	if (!sha1)
+		return;
+	text = read_sha1_file(sha1, &type, &len);
+	if (text && len && type == OBJ_BLOB)
+		strbuf_add(buf, text, len);
+	free(text);
+}
+
 static int record_signed_push(char *cert)
 {
 	/*
@@ -591,19 +613,57 @@ static int record_signed_push(char *cert)
 	 *
 	 * You could also feed the signed push certificate to GPG,
 	 * verify the signer identity, and all the other fun stuff,
-	 * including feeding it to "pre-push-verify-signature" hook.
-	 *
-	 * Here we just throw it to stderr to demonstrate that the
-	 * codepath is being exercised.
+	 * including feeding it to "pre-receive-signature" hook.
 	 */
+	size_t total, payload;
 	char *cp, *ep;
-	for (cp = cert; *cp; cp = ep) {
+	int ret = 0;
+	struct notes_tree *t;
+	struct strbuf nbuf = STRBUF_INIT;
+
+	init_notes(NULL, "refs/notes/signed-push", NULL, 0);
+	t = &default_notes_tree;
+
+	total = strlen(cert);
+	payload = parse_signature(cert, total);
+	for (cp = cert; cp < cert + payload; cp = ep) {
+		unsigned char sha1[20], nsha1[20];
+
 		ep = strchrnul(cp, '\n');
 		if (*ep == '\n')
 			ep++;
-		fprintf(stderr, "RSP: %.*s", (int)(ep - cp), cp);
+		if (prefixcmp(cp, "Update: "))
+			continue;
+		cp += strlen("Update: ");
+		if (get_sha1_hex(cp, sha1) || cp[40] != ' ')
+			continue;
+		cp += 41;
+		if (get_sha1_hex(cp, sha1) || cp[40] != ' ')
+			continue;
+
+		get_note_text(&nbuf, t, sha1);
+		if (nbuf.len)
+			strbuf_addch(&nbuf, '\n');
+		strbuf_add(&nbuf, cert, total);
+		if (write_sha1_file(nbuf.buf, nbuf.len, blob_type, nsha1) ||
+		    add_note(t, sha1, nsha1, NULL))
+			ret = error(_("unable to write note object"));
+		strbuf_reset(&nbuf);
 	}
-	return 0;
+
+	if (!ret) {
+		unsigned char commit[20];
+		unsigned char parent[20];
+		struct ref_lock *lock;
+
+		resolve_ref(t->ref, parent, 0, NULL);
+		lock = lock_any_ref_for_update(t->ref, parent, 0);
+		create_notes_commit(t, NULL, "push", commit);
+		ret = write_ref_sha1(lock, commit, "push");
+	}
+	free_notes(t);
+
+	return ret;
 }
 
 static void execute_commands(struct command *commands, const char *unpacker_error)
-- 
1.7.7.rc0.188.g3793ac

Re: [PATCH 5/2] push -s: receiving end

[Johan Herland]

On Thursday 8. September 2011, Junio C Hamano wrote:
> This stores the GPG signed push certificate in the receiving
> repository using the notes mechanism. The certificate is appended to
> a note in the refs/notes/signed-push tree for each object that
> appears on the right hand side of the push certificate, i.e. the
> object that was pushed to update the tip of a ref.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---

(...)

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 307fc3b..257f2a5 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -156,6 +161,7 @@ struct command {
>  };
> 
>  static const char pre_receive_hook[] = "hooks/pre-receive";
> +static const char pre_receive_signature_hook[] =
> "hooks/pre-receive-signature"; static const char post_receive_hook[]
> = "hooks/post-receive";
> 
>  static void rp_error(const char *err, ...) __attribute__((format
> (printf, 1, 2))); @@ -581,6 +587,22 @@ static void
> check_aliased_updates(struct command *commands)
> string_list_clear(&ref_list, 0);
>  }
> 
> +static void get_note_text(struct strbuf *buf, struct notes_tree *t,
> +			  const unsigned char *object)
> +{
> +	const unsigned char *sha1 = get_note(t, object);
> +	char *text;
> +	unsigned long len;
> +	enum object_type type;
> +
> +	if (!sha1)
> +		return;
> +	text = read_sha1_file(sha1, &type, &len);
> +	if (text && len && type == OBJ_BLOB)
> +		strbuf_add(buf, text, len);
> +	free(text);
> +}
> +

What about adding this function to notes.h as a convenience to other 
users of the notes API?

Otherwise the code looks good to me.


Have fun! :)

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 2/2] push -s: skeleton

[Junio C Hamano]

Shawn Pearce <spearce@spearce.org> writes:

> This sounds too late to me.  I think you just caused 2 HTTP POSTs, one
> a partial one with the commands and no pack data, and another with the
> push certificate and the pack. Neither is useful.

Well, then I'd stop looking at this area and let others make it useful
while I hack in other areas.

Re: [PATCH 5/2] push -s: receiving end

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

>> +static void get_note_text(struct strbuf *buf, struct notes_tree *t,
>> +			  const unsigned char *object)
>> +{
>> +	const unsigned char *sha1 = get_note(t, object);
>> +	char *text;
>> +	unsigned long len;
>> +	enum object_type type;
>> +
>> +	if (!sha1)
>> +		return;
>> +	text = read_sha1_file(sha1, &type, &len);
>> +	if (text && len && type == OBJ_BLOB)
>> +		strbuf_add(buf, text, len);
>> +	free(text);
>> +}
>> +
>
> What about adding this function to notes.h as a convenience to other 
> users of the notes API?

I actually was hoping to hear that I do not have to do this "check
existing and concatenate", and should let the add_note() function
run its default combine_notes method to do the concatenation.

I found a few things I wasn't quite sure in the notes/notes-merge API, by
the way.

 - The combine_notes callback is run when a note is inserted into the
   in-core notes tree. I felt that this is way too early if you want to
   avoid racing with another process (and the patch tries to wrap
   create-notes-commit with lock-ref/write-ref-sha1 pair), but perhaps
   this is to deal with a case where the calling program calls add_notes()
   on the same object multiple times.

 - create_notes_commit() dies under a few conditions, but some callers
   that are recording advisory/optional notes might want to get an error
   and continue.

I think ideally this patch should handle notes like the following, which
is not quite how I coded it:

 - initialize in-core notes tree;

 - add bunch of notes, without regard to the existing ones, to in-core
   notes tree by calling add_notes();

 - lock the notes ref and read the "parent"; we may want to add "wait and
   retry for a few times until we get the lock" support at lockfile API
   level, but doing it at the application level would be fine.

 - call create-notes-commit, which in turn merges the in-core
   notes with what collides with those already in "parent" by
   calling the combine-notes callback, merges and re-balances
   the notes tree, and makes a notes commit object;

 - update the notes ref with that notes commit, releasing the lock on
   the ref.

Re: [PATCH 2/2] push -s: skeleton

[Jeff King]

On Wed, Sep 07, 2011 at 01:57:27PM -0700, Junio C Hamano wrote:

> If a tag is GPG-signed, and if you trust the cryptographic robustness of
> the SHA-1 and GPG, you can guarantee that all the history leading to the
> signed commit is not tampered with. However, it would be both cumbersome
> and cluttering to sign each and every commit. Especially if you strive to
> keep your history clean by tweaking, rewriting and polishing your commits
> before pushing the resulting history out, many commits you will create
> locally end up not mattering at all, and it is a waste of time to sign
> them.
> 
> A better alternative could be to sign a "push certificate" (for the lack
> of better name) every time you push, asserting that what commits you are
> pushing to update which refs. The basic workflow goes like this:

I think this is the right direction, but I was a little turned off by
the idea that it needs a protocol extension. As I see it, there are two
ways to care about the contents of a push certificate:

  1. The server might care, because it only wants to accept pushes that
     are accompanied by a certificate matching a certain key.

  2. A client fetching from the server might care, because they want the
     integrity and authenticity of the data to be ensured by the gpg key
     of the pusher, not by trusting the server.

I think (1) is actually not all that interesting. The server already has
credentials for each user via ssh or http. So it knows who each pusher
is already. It can't relay that information cryptographically to a
client who fetches later, of course, but we are just talking about
whether or not to accept the push at this moment.

But if you really did want to do that, it seems like a pre-receive hook
would be sufficient.

For (2), you don't want to trust the server, so the user's
authentication to the server isn't enough. You want a cryptographic
chain leading back to the original pusher. But the server doesn't
actually need to see or understand that cryptographic chain for this
purpose. If it were stored in a notes-tree or other format pointed to by
a ref, then a client could pull down those notes and do the verification
themselves.

Which means you can start using this immediately, without having to care
about whether your hosting provider supports it or not (or even whether
your provider supports the git protocol. Such a system would Just Work
across dumb http, local clones, sneakernet bundles, etc).

The only issue I foresee is one of atomicity. IIRC, we never have a
whole-repo lock during push, so it's possible that a client might
succeed in pushing the ref with the certificate, but fail at one or more
refs that the certificate mentions. And maybe a protocol extension is
required for that. You've looked much more closely at this than I have,
so maybe you already considered something simpler.

-Peff

Re: [PATCH 2/2] push -s: skeleton

[Jeff King]

On Wed, Sep 07, 2011 at 11:55:44PM +0000, Robin H. Johnson wrote:

> There's a couple of related things we've been considering on the Gentoo
> side:
> - detached signatures of blobs (either the SHA1 of the blob or the blob
>   itself)

There's not much point in signing the blob itself and not the sha1; the
first thing any signature algorithm will do is make a fixed-size digest
of the content anyway. So it is only useful if you don't trust sha1 as
your digest algorithm (which maybe is a reasonable concern these
days...).

> - The signature covering the message+blob details, but NOT the chain of
>   history: this opens up the ability to cherry-pick and rebase iff there
>   are no conflicts and the blobs are identical, all while preserving the
>   signature.

The problem is that many of the blobs won't be identical, because
they'll have new content from the new commits you rebased on top of. So
_some_ blobs will be the same, but you'll end up with a half-signed
commit. I think you're better to just re-sign the new history.

But I'd have to see a longer description of your scheme to really
critique it. I'm not 100% sure what your security goals are here.

> - concerns about a pre-image attack against Git. tl;dr version:
>   1. Attacker prepares decoy file in advance, that hashes to the same as
>      the malicious file.
>   2. Attacker sends decoy in as an innocuous real commit.
>   3. Months later, the attacker breaks into the system and alters the
>      packfile to include the new malicious file.
>   4. All new clones from that point forward get the malicious version.

Nit: I think you mean "collision attack" here. Pre-image attacks are
matching a malicious file to what is already in the tree, but are much
harder to execute.

But yeah, it is a potential problem. I don't keep up very well with that
sort of news anymore, but AFAIK, we still don't have any actual
collisions in sha1. Wikipedia seems to seem to think the best attacks
are in the 2^50-ish range, but nobody has successfully found one. So we
may still be a few years away from a realistic attack. If the attacks
are anything like the MD5 attacks, the decoy and malicious files will
need to have a bunch of random garbage in them. Which may be hard to
disguise, depending on your repo contents.

I think, though, that the sane fix at that point is not to start trying
to make per-blob signatures or anything like that, but to consider "git
version 2" with SHA-256, or whatever ends up becoming SHA-3 next year.
It would involve rewriting all of your history and dropping support for
older git clients, of course, but it may be worth it at the point that
sha1 is completely broken.

> Re your comment on always needing to resign commits above, we'd been
> considering post-signing commits, not when they are initially made.
> After your commit is clean and ready to ship, you can fire the commit
> ids into the signature tool, which can generate a detached signature
> note for each commit.

Agreed. This is just an interface problem, not a cryptographic or
technical one. However, I do think there's a subtle difference between
the two ideas. Signing each commit individually just indicates some
approval of particular commits. But signing a push certificate is about
associating particular commits with particular refs (e.g., saying "move
'master' from commit X to commit Y). I think there may be uses for both
forms.

-Peff

Re: [PATCH 2/2] push -s: skeleton

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think (1) is actually not all that interesting. The server already has
> credentials for each user via ssh or http. So it knows who each pusher
> is already. It can't relay that information cryptographically to a
> client who fetches later, of course, but we are just talking about
> whether or not to accept the push at this moment.
>
> But if you really did want to do that, it seems like a pre-receive hook
> would be sufficient.

I see two flaws in that reasoning. The server's authentication may be
found not trustworthy for some reason long after commits hit the tree, and
GPG signature made by the _pusher_ would assert the integrity. Also this
will open the door to accept push over an unauthenticated connection and
allowing only signed pushes.

> For (2), you don't want to trust the server, so the user's
> authentication to the server isn't enough. You want a cryptographic
> chain leading back to the original pusher. But the server doesn't
> actually need to see or understand that cryptographic chain for this
> purpose.

Exactly. That is why the signed push certificate is stored without the
server doing anything funky, only to annotate the pushed commits in the
notes tree---the fetchers can peek the notes and verify the GPG signature.
But not _forcing_ that the push certificate be placed in a notes tree on
the client side allows different server hosting sites to additionally do
different things using that data.

> The only issue I foresee is one of atomicity.

The very initial thinking was to create a notes tree commit on the client
side and push that along with what is pushed, but that approach has an
inherent flaw of causing unnecessary collisions between two people who are
pushing to unrelated branches, and that is why I decided to let the server
side handle it.

Re: [PATCH 2/2] push -s: skeleton

[Jeff King]

On Thu, Sep 08, 2011 at 01:48:07PM -0700, Junio C Hamano wrote:

> > I think (1) is actually not all that interesting. The server already has
> > credentials for each user via ssh or http. So it knows who each pusher
> > is already. It can't relay that information cryptographically to a
> > client who fetches later, of course, but we are just talking about
> > whether or not to accept the push at this moment.
> >
> > But if you really did want to do that, it seems like a pre-receive hook
> > would be sufficient.
> 
> I see two flaws in that reasoning. The server's authentication may be
> found not trustworthy for some reason long after commits hit the tree, and
> GPG signature made by the _pusher_ would assert the integrity.

Right, but that is not about (1), but rather about (2). IOW, there are
two times to authenticate: at the moment you take the push, and every
moment thereafter, no matter who you are. But I think we are just
splitting hairs. We both agree that signed pushes are a good thing.

When I said "a pre-receive hook is sufficient", I emphatically _didn't_
mean that the server should sign in the hook, or create a cryptographic
trail starting there. I meant that git doesn't need to carry the code
internally, and that a pre-receive hook can check the push certificate
itself, just as a client would.

BTW, is the name "push certificate" right? It seems like they are not
necessarily about pushing, but about signing "I would like to move ref X
from Y to Z". Is there value to making such signatures locally (i.e.,
not over the git protocol, but such that you could later check the
integrity of what's on the disk cryptographically)? Would it not be
possible to generate this information in one step, and then have a
remote fetch from you, with no pushes at all?

> Also this will open the door to accept push over an unauthenticated
> connection and allowing only signed pushes.

Yes, though a pre-receive hook could do that, too.

> Exactly. That is why the signed push certificate is stored without the
> server doing anything funky, only to annotate the pushed commits in the
> notes tree---the fetchers can peek the notes and verify the GPG signature.
> But not _forcing_ that the push certificate be placed in a notes tree on
> the client side allows different server hosting sites to additionally do
> different things using that data.

Hmm. So you seem to take the approach of:

  1. Client needs only know "I am pushing with a signed cert".

  2. Server can convert that signed cert into other formats as they see
     fit, including breaking the signature out into a notes ref.

  3. Other clients fetch from the server, seeing the notes ref (or not).

But that seems backwards to me. In a decentralized system, the endpoints
are what is important. So the pushing client in 1 should be the one
deciding what other clients see, no? Otherwise, if I care about what's
in the notes tree, I have to care whether I am pulling from kernel.org
or github.com, or whatever. The server stops being dumb storage.

> > The only issue I foresee is one of atomicity.
> 
> The very initial thinking was to create a notes tree commit on the client
> side and push that along with what is pushed, but that approach has an
> inherent flaw of causing unnecessary collisions between two people who are
> pushing to unrelated branches, and that is why I decided to let the server
> side handle it.

Yeah, it is a potential problem, but it just seems wrong to put too much
policy work onto the server. Perhaps it would make more sense to keep
one notes tree per ref, which would also resolve that locking issue?

-Peff

Re: [PATCH 2/2] push -s: skeleton

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yeah, it is a potential problem, but it just seems wrong to put too much
> policy work onto the server.

My take on it is somewhat different. The only thing in the end result we
want to see is that the pushed commits are annotated with GPG signatures
in the notes tree, and there is no reason for us to cast in stone that
there has to be any significance in the commit history of the notes tree.

In a busy hosting site that has many branches being pushed simultaneously,
it is entirely plausible that the server side may just want to store each
received push certificate in a new flat file in a filesystem, and have
asynchronous process sweep the new certificates to update the notes tree,
possibly creating a single notes tree commit that records updates by
multiple pushes, for performance purposes, in its implementation of
record_signed_push() in receive-pack.

If you forced the clients to also prepare notes and push the notes tree to
the server, you are forcing the ordering in the history of the notes, and
closing the door for such a server implementation. I would consider it an
unnecessary and/or premature policy decision.

Re: [PATCH 2/2] push -s: skeleton

[Robin H. Johnson]

On Thu, Sep 08, 2011 at 04:03:43PM -0400,  Jeff King wrote:
> On Wed, Sep 07, 2011 at 11:55:44PM +0000, Robin H. Johnson wrote:
> > There's a couple of related things we've been considering on the Gentoo
> > side:
> > - detached signatures of blobs (either the SHA1 of the blob or the blob
> >   itself)
> There's not much point in signing the blob itself and not the sha1; the
> first thing any signature algorithm will do is make a fixed-size digest
> of the content anyway. So it is only useful if you don't trust sha1 as
> your digest algorithm (which maybe is a reasonable concern these
> days...).
To clarify here, there's two things gained by this over signing the
SHA1:
1. Ability to use a different digest algorithm right now, without
   having to trust SHA1.
2. Ability to use HMAC.

I agree that both of these are bandaids to the security of SHA*.

> > - The signature covering the message+blob details, but NOT the chain of
> >   history: this opens up the ability to cherry-pick and rebase iff there
> >   are no conflicts and the blobs are identical, all while preserving the
> >   signature.
> The problem is that many of the blobs won't be identical, because
> they'll have new content from the new commits you rebased on top of. So
> _some_ blobs will be the same, but you'll end up with a half-signed
> commit. I think you're better to just re-sign the new history.
Possibly, the rebase/cherry-pick isn't critical.

> But I'd have to see a longer description of your scheme to really
> critique it. I'm not 100% sure what your security goals are here.
The primary goal is that a developer can certify their commits when they
are ready to push them, regardless of the means of how they are going to
push them (I've had requests for some git-bundle usage help in pushes).

> > - concerns about a pre-image attack against Git. tl;dr version:
> >   1. Attacker prepares decoy file in advance, that hashes to the same as
> >      the malicious file.
> >   2. Attacker sends decoy in as an innocuous real commit.
> >   3. Months later, the attacker breaks into the system and alters the
> >      packfile to include the new malicious file.
> >   4. All new clones from that point forward get the malicious version.
> Nit: I think you mean "collision attack" here. Pre-image attacks are
> matching a malicious file to what is already in the tree, but are much
> harder to execute.
My bad, it was really late when I was writing the above.

> But yeah, it is a potential problem. I don't keep up very well with that
> sort of news anymore, but AFAIK, we still don't have any actual
> collisions in sha1. Wikipedia seems to seem to think the best attacks
> are in the 2^50-ish range, but nobody has successfully found one. So we
> may still be a few years away from a realistic attack. If the attacks
> are anything like the MD5 attacks, the decoy and malicious files will
> need to have a bunch of random garbage in them. Which may be hard to
> disguise, depending on your repo contents.
Joey Hess discussed this two years ago, and again last week:
http://kitenet.net/~joey/blog/entry/size_of_the_git_sha1_collision_attack_surface/
http://kitenet.net/~joey/blog/entry/sha-1/

This is easy in the kernel tree, it's got lots of eyeballs and only few
binary files. This isn't true for lots of other Git trees, a tree with a
JPEG image or a gzip file would be a great target.

It's 2^53 last I saw, which is only ~250 days of cputime on my i7
desktop. Also notably that is under the complexity of the 2^56 EFF DES
cracker was built do to (and the more recent COPACOBANA unit).

> I think, though, that the sane fix at that point is not to start trying
> to make per-blob signatures or anything like that, but to consider "git
> version 2" with SHA-256, or whatever ends up becoming SHA-3 next year.
> It would involve rewriting all of your history and dropping support for
> older git clients, of course, but it may be worth it at the point that
> sha1 is completely broken.
I think this is needed sooner rather than later.

> > Re your comment on always needing to resign commits above, we'd been
> > considering post-signing commits, not when they are initially made.
> > After your commit is clean and ready to ship, you can fire the commit
> > ids into the signature tool, which can generate a detached signature
> > note for each commit.
> Agreed. This is just an interface problem, not a cryptographic or
> technical one. However, I do think there's a subtle difference between
> the two ideas. Signing each commit individually just indicates some
> approval of particular commits. But signing a push certificate is about
> associating particular commits with particular refs (e.g., saying "move
> 'master' from commit X to commit Y). I think there may be uses for both
> forms.
Yes, there is use for both forms. The additional one is that you can
separate who pushes from who commits.

-- 
Robin Hugh Johnson
Gentoo Linux: Developer, Trustee & Infrastructure Lead
E-Mail     : robbat2@gentoo.org
GnuPG FP   : 11AC BA4F 4778 E3F6 E4ED  F38E B27B 944E 3488 4E85

Re: [PATCH 2/2] push -s: skeleton

[Jeff King]

On Thu, Sep 08, 2011 at 03:07:25PM -0700, Shawn O. Pearce wrote:

> A notes tree per ref is ugly when you have a lot of branches. Its a
> problem when you merge commits from say "maint" over to "master" 2
> days after they were initially pushed. Ideally the notes tree for
> master also includes what you brought over.

I agree you can end up with a lot of refs if you have a lot of branches,
and that may get a bit unwieldy.  I don't see how the merge thing is a
problem, though. If you do the merge at a client, then those commits
will hit master by push, and you'll get entries in the cert tree for
master. If you do the merge locally, then there's not going to be a
signature on those commits hitting the master ref, no matter what the
storage scheme.

> However, maybe it is reasonable for a protocol extension to support
> "auto-notes-merge" on a refs/notes/ ref if the client asks for it in
> the send-pack/receive-pack command stream? Clients could push notes
> and have the server automatically merge the client's pushed commit
> into the notes tree, either by fast-forward or by performing an
> automatic notes merge, with concat being applied to non-identical
> notes on the same SHA-1. This would allow the client to prepare his
> local certificate, and push that notes tree, while still working
> around the race on the server side.
> 
> If the server doesn't support the protocol extension, the client can
> still push his signed notes, he just may run into a race with another
> concurrent user pushing into the same repository. Which then means
> that an upgrade of the server is really only important/necessary if
> you have "central repository" model that a lot of users push into. If
> you use the traditional workflow that GitHub encourages of per-user
> repositories, you would never have a race on the server, and wouldn't
> need the upgraded server binary with "auto-notes-merge".

I like this approach, as the protocol extension provides the minimal
building block that can be used to implement this, or any other
notes-related scheme.

-Peff

Re: [PATCH 2/2] push -s: skeleton

[Jeff King]

On Thu, Sep 08, 2011 at 03:19:54PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Yeah, it is a potential problem, but it just seems wrong to put too much
> > policy work onto the server.
> 
> My take on it is somewhat different. The only thing in the end result we
> want to see is that the pushed commits are annotated with GPG signatures
> in the notes tree, and there is no reason for us to cast in stone that
> there has to be any significance in the commit history of the notes tree.

Hmm. Is order really irrelevant? If you push a commit to master, moving
it from X to Y, then push-rewind it back to X, then push a new commit Z,
how do I cryptographically determine the correct final state of master?

I'll see two push-certs, one going X..Y and one X..Z. They'll have
timestamps, which can be used for ordering. But what if the first and
third actions above are done by different people. Now you're trusting
that their clocks are synced to order them properly.

Note that simply keeping an unsigned but ordered notes history doesn't
solve this problem, either; you'd probably want a parent pointer in your
push cert saying "and this is the previous push cert I am based on".

And maybe this is a use case we don't care about. Maybe it's enough for
the push-cert to say "at some point in time, I thought it was a good
idea to push these commits into master; signed, me".

But I'm not really clear on exactly what the security goals are. The
series you sent looks interesting, but I haven't seen the verification
side of these signatures. What are they going to be used for? What
guarantees are we attempting to provide? For that matter, what is our
threat model? What are attackers capable of?

> In a busy hosting site that has many branches being pushed simultaneously,
> it is entirely plausible that the server side may just want to store each
> received push certificate in a new flat file in a filesystem, and have
> asynchronous process sweep the new certificates to update the notes tree,
> possibly creating a single notes tree commit that records updates by
> multiple pushes, for performance purposes, in its implementation of
> record_signed_push() in receive-pack.

OK, I see. It is not "the server can do whatever it likes with the
information" as much as "the server can do whatever it likes, but at the
very least should eventually create a notes tree of a given form".

-Peff

Re: [PATCH 2/2] push -s: skeleton

[Joey Hess]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="//TRANSLIT", Size: 2109 bytes --]

Robin H. Johnson wrote:
> Joey Hess discussed this two years ago, and again last week:
> http://kitenet.net/~joey/blog/entry/size_of_the_git_sha1_collision_attack_surface/
> 
> This is easy in the kernel tree, it's got lots of eyeballs and only few
> binary files. This isn't true for lots of other Git trees, a tree with a
> JPEG image or a gzip file would be a great target.

The most credible attack I have so far does not involve binary files in
tree. Someone pointed out that git log, git show, etc stop printing
commit messages at NULL. So colliding binary garbage can be put in a
commit message and be unlikely to be noticed, and the commit can
later be altered to point to a different tree.

https://github.com/joeyh/supercollider

joey@gnu:~/tmp/supercollider>git log
commit 24f30db5790b209fa412ce81c5ef2bf8af5fd4d7
Author: Joey Hess <joey@kitenet.net>
Date:   Fri Sep 9 11:49:21 2011 -0400

    an innocent commit
    
    If this were a sha1 colliding attack, there would be some sort of binary
    garbage below. Which there isn't. So this can be safely merged.
joey@gnu:~/tmp/supercollider>git cat-file commit 24f30db5790b209fa412ce81c5ef2bf8af5fd4d7
tree 735a7633237c07b398856005de3bc9ea00446747
author Joey Hess <joey@kitenet.net> 1315583361 -0400
committer Joey Hess <joey@kitenet.net> 1315583361 -0400

an innocent commit

If this were a sha1 colliding attack, there would be some sort of binary
garbage below. Which there isn't. So this can be safely merged.
\0


??b???\x1f[?i??ͯ?t?\f2??\x02????os?\x14<????h?+,M?mY?e?EW?i\x13v$???\x14J??U}n~???L??????f??\x02?ě??3>?Q??H?޸\x16*zl\x1a?RA˂q?E\f?\x06\x16E\x7f7??^[?\x03\?m???U?\x1e>MU\v	GY?d)?ȼ??'g?~D??ɯhQ?\x13???/"E\x04??X?m???^͸??S?D\x13??;w6(?`??>?\x10縘?\aAѲ?*!??@v????>?8??2\b?\x14!??=*?J	^[\r\r???\x01ynH\x10???c?w?\??K7??\x1c?N?6??\x1c???A5?FM?wZ?~?pK\x02Y?R???s7\x7f??(?\aƶ?_"??m\x11%????\x7f1\x7fa??ʀ??K[\rt??\x11??\x0e!A0?ΈfT.?T?w\a?򁛵ƌ\v?р???aco?V/2\x14??nَ?
?}?6?\x19_?z?{

It might be worth ameloriating that attack by making git log always
show the full buffer. Or it would be easy to write a tool that finds
any commits that have a NULL in their message.

-- 
see shy jo

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/2] push -s: skeleton

[Drew Northup]

On Fri, 2011-09-09 at 12:03 -0400, Joey Hess wrote:

> It might be worth ameloriating that attack by making git log always
> show the full buffer. Or it would be easy to write a tool that finds
> any commits that have a NULL in their message.

Just be aware that fixing the problem by diallowing NULLs in the commit
message makes it impossible to include UTF-16 text in the commit message
(which isn't currently handled very well anyway). Granted, I'm not sure
what the most decent way of dealing with that otherwise might be as I've
not taken the time to think about it...

-- 
-Drew Northup
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

Re: [PATCH 2/2] push -s: skeleton

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Sep 08, 2011 at 03:19:54PM -0700, Junio C Hamano wrote:
>
>> My take on it is somewhat different. The only thing in the end result we
>> want to see is that the pushed commits are annotated with GPG signatures
>> in the notes tree, and there is no reason for us to cast in stone that
>> there has to be any significance in the commit history of the notes tree.
>
> Hmm. Is order really irrelevant? If you push a commit to master, moving
> it from X to Y, then push-rewind it back to X, then push a new commit Z,
> how do I cryptographically determine the correct final state of master?

You don't, as the certs are more about "up to this point, the pusher
trusts the history". I should have made it clearer in the cover letter to
the rerolled series, but the push certificate does not record the old
value of the ref in the reroll, because the point of signed-push is not
about signing the information that is equivalent to the server side
reflog.

You would have a signed push record that pushed Y, X and Z, and commit Z
sitting at the tip of 'master'. A few days may pass and then you run

    $ git log --show-notes=refs/notes/push-signature master

to find that the first commit with a push signature by somebody whose
judgement you trust is Z. Then you would need to inspect only commits that
are not ancestors of Z even if you suspect that some commits near the tip
of 'master' at the server side were tampered with.

You may at the same time find commits signed by the trusted people that
are meant for the same branch but are not contained in the history of
'master' (e.g. Y), which might indicate that the branch was rewound,
possibly by an intruder.

Another possible scenario. Later you and the pusher of Z may find that
when the pusher created Z, he merged something questionable and Z may now
have to be in "untrustable" set. You can dig further to find X at that
point.

> OK, I see. It is not "the server can do whatever it likes with the
> information" as much as "the server can do whatever it likes, but at the
> very least should eventually create a notes tree of a given form".

Yes, examples of things the server side might want to additionally do in
pre-receive-signature hook are to read the push certificate to implement
authorization (and it can be per-branch if you wanted to) and to forward
it immediately to offsite storage for safekeeping (the storage does not
have to use git notes to implement it).

Re: [PATCH 2/2] push -s: skeleton

[Jeff King]

On Fri, Sep 09, 2011 at 12:03:01PM -0400, Joey Hess wrote:

> The most credible attack I have so far does not involve binary files in
> tree. Someone pointed out that git log, git show, etc stop printing
> commit messages at NULL.

It was me.

> It might be worth ameloriating that attack by making git log always
> show the full buffer. Or it would be easy to write a tool that finds
> any commits that have a NULL in their message.

Unfortunately, that is going to involve a pretty huge code audit of git,
as the "tack a \0 to the end of an object just in case" code dates back
quite a while (e871b64, unpack_sha1_file: zero-pad the unpacked
object, 2005-05-25). So I suspect there is a lot of code built on
top of the assumption that commit messages are NUL-terminated strings.

Besides which, that is only one form of hiding. If collision attacks
against sha1 become a possibility, I think we are better to talk about
moving to a new hash. Even sha-256 truncated to 160 bits would be better
than sha-1 (AFAIK, that family of SHA does not suffer from the same
attacks, so we would still be in the 2^80 range for collision attacks).

-Peff