threads / patch / 37381

patch, 18 partsSigned push

Subject: [PATCH 00/18] Signed push

## tl;dr

59 messages between Aug 19, 2014 and Sep 5, 2014. Diffs are folded; open one to read it.

replies: 58people: 10as markdown or json

Junio C Hamano· Aug 19, 2014, 22:06 UTC · lore

While signed tags and commits assert that the objects thusly signed came from you, who signed these objects, there is not a good way to assert that you wanted to have a particular object at the tip of a particular branch. My signing v2.0.1 tag only means I want to call the version v2.0.1, and it does not mean I want to push it out to my 'master' branch---it is likely that I only want it in 'maint', so the signature on the object alone is insufficient.

The only assurance to you that 'maint' points at what I wanted to place there comes from your trust on the hosting site and my authentication with it, which cannot easily audited later.

This series introduces a cryptographic assurance for ref updates done by "git push" by introducing a mechanism that allows you to sign a "push certificate" (for the lack of better name) every time you push. Think of it as working on an axis orthogonal to the traditional "signed tags".

The most interesting part starts at 15/18; everything that precedes that patch are preparatory clean-ups.

    [PATCH 15/18] the beginning of the signed push
	This step presents the basic idea.  If you remember the
	underlying "git push" protocol exchange, it goes like this:
	- The server advertises the existing refs and where they
	  point at, and the capabilities the server supports;
	- The "git push" client sends update commands (one or more
	  "old-sha1 new-sha1 refname") followed by the pack data;
	- The server unpacks and updates the refname to point at
	  new-sha1.
	We introduce "push-cert" capability, and allow the client to
	sign the "update commands" it will send to the server and
	send this signature using the new "push-cert" command.
	This certificate is exported to the pre/post-receive hooks
	of the server, so that the pre-receive hook can GPG validate
	(and possibly reject a bad push); post-receive hook can log
	the certificate.
    [PATCH 16/18] receive-pack: GPG-validate push certificates
	This step builds a native GPG validation into the server to
	help the pre-receive hook.  The signature is verified
	against the GPG keychain the server uses (it is likely that
	you would want to set and export GNUPGHOME when starting
	your server), and verification result is given to the
	pre/post-receive hook.
    [PATCH 17/18] send-pack: send feature request on push-cert packet
    [PATCH 18/18] signed push: final protocol update
	With the protocol introduced in 15/18, the update commands
	and the push certificate record the same information twice;
	the protocol was kept inefficient to make it easier to
	review the changes.
	These two steps updates the protocol to the final version,
	which does not to send the update commands when a push
	certificate is in use.

If the server's GPG keychain and pre-receive hook are properly set up, a "git push --signed" over an unauthenticated and unencrypted communication channel (aka "git daemon") can be made as secure as, and even more secure than, the authenticated "git push ssh://".

With the signed push certificate, together with the connectivity check done when the server accepts the packed data, we are assured that the trusted user vouches for the history leading to the proposed tips of refs (aka "new-sha1"s), and a man-in-the-middle would not be able to make the server accept an update altered in transit.

Junio C Hamano (18):
  receive-pack: do not overallocate command structure
  receive-pack: parse feature request a bit earlier
  receive-pack: do not reuse old_sha1[] to other things
  receive-pack: factor out queueing of command
  send-pack: move REF_STATUS_REJECT_NODELETE logic a bit higher
  send-pack: refactor decision to send update per ref
  send-pack: always send capabilities
  send-pack: factor out capability string generation
  send-pack: rename "new_refs" to "need_pack_data"
  send-pack: refactor inspecting and resetting status and sending commands
  send-pack: clarify that cmds_sent is a boolean
  gpg-interface: move parse_gpg_output() to where it should be
  gpg-interface: move parse_signature() to where it should be
  pack-protocol doc: typofix for PKT-LINE
  the beginning of the signed push
  receive-pack: GPG-validate push certificates
  send-pack: send feature request on push-cert packet
  signed push: final protocol update
 Documentation/git-push.txt                        |   9 +-
 Documentation/git-receive-pack.txt                |  30 +++-
 Documentation/technical/pack-protocol.txt         |  24 ++-
 Documentation/technical/protocol-capabilities.txt |  12 +-
 builtin/push.c                                    |   1 +
 builtin/receive-pack.c                            | 161 +++++++++++++++---
 commit.c                                          |  36 -----
 gpg-interface.c                                   |  57 +++++++
 gpg-interface.h                                   |  18 ++-
 send-pack.c                                       | 188 ++++++++++++++++------
 send-pack.h                                       |   1 +
 t/t5534-push-signed.sh                            |  77 +++++++++
 tag.c                                             |  20 ---
 tag.h                                             |   1 -
 transport.c                                       |   4 +
 transport.h                                       |   5 +
 16 files changed, 502 insertions(+), 142 deletions(-)
 create mode 100755 t/t5534-push-signed.sh
-- 
2.1.0-301-g54593e2
Junio C Hamano· Aug 19, 2014, 22:06 UTC · re: Junio C Hamano · lore

[PATCH 01/18] receive-pack: do not overallocate command structure

An "update" command in the protocol exchange consists of 40-hex old object name, SP, 40-hex new object name, SP, and a refname, but the first instance is further followed by a NUL with feature requests.

The command structure, which has a flex-array member that stores the refname at the end, was allocated based on the whole length of the update command, without excluding the trailing feature requests.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/receive-pack.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Show changes to builtin/receive-pack.c +3 −2
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index f93ac45..1663beb 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -872,10 +872,11 @@ static struct command *read_head_info(struct sha1_array *shallow)
 			if (parse_feature_request(feature_list, "quiet"))
 				quiet = 1;
 		}
-		cmd = xcalloc(1, sizeof(struct command) + len - 80);
+		cmd = xcalloc(1, sizeof(struct command) + reflen + 1);
 		hashcpy(cmd->old_sha1, old_sha1);
 		hashcpy(cmd->new_sha1, new_sha1);
-		memcpy(cmd->ref_name, line + 82, len - 81);
+		memcpy(cmd->ref_name, refname, reflen);
+		cmd->ref_name[reflen] = '\0';
 		*p = cmd;
 		p = &cmd->next;
 	}
-- 
2.1.0-301-g54593e2
Junio C Hamano· Aug 19, 2014, 22:06 UTC · re: Junio C Hamano · lore

[PATCH 02/18] receive-pack: parse feature request a bit earlier

Ideally, we should have also allowed the first "shallow" to carry the feature request trailer, but that is water under the bridge now. This makes the next step to factor out the queuing of commands easier to review.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/receive-pack.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)
Show changes to builtin/receive-pack.c +13 −11
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 1663beb..43f35c4 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -840,7 +840,7 @@ static struct command *read_head_info(struct sha1_array *shallow)
 		unsigned char old_sha1[20], new_sha1[20];
 		struct command *cmd;
 		char *refname;
-		int len, reflen;
+		int len, reflen, linelen;
 
 		line = packet_read_line(0, &len);
 		if (!line)
@@ -853,7 +853,18 @@ static struct command *read_head_info(struct sha1_array *shallow)
 			continue;
 		}
 
-		if (len < 83 ||
+		linelen = strlen(line);
+		if (linelen < len) {
+			const char *feature_list = line + linelen + 1;
+			if (parse_feature_request(feature_list, "report-status"))
+				report_status = 1;
+			if (parse_feature_request(feature_list, "side-band-64k"))
+				use_sideband = LARGE_PACKET_MAX;
+			if (parse_feature_request(feature_list, "quiet"))
+				quiet = 1;
+		}
+
+		if (linelen < 83 ||
 		    line[40] != ' ' ||
 		    line[81] != ' ' ||
 		    get_sha1_hex(line, old_sha1) ||
@@ -863,15 +874,6 @@ static struct command *read_head_info(struct sha1_array *shallow)
 
 		refname = line + 82;
 		reflen = strlen(refname);
-		if (reflen + 82 < len) {
-			const char *feature_list = refname + reflen + 1;
-			if (parse_feature_request(feature_list, "report-status"))
-				report_status = 1;
-			if (parse_feature_request(feature_list, "side-band-64k"))
-				use_sideband = LARGE_PACKET_MAX;
-			if (parse_feature_request(feature_list, "quiet"))
-				quiet = 1;
-		}
 		cmd = xcalloc(1, sizeof(struct command) + reflen + 1);
 		hashcpy(cmd->old_sha1, old_sha1);
 		hashcpy(cmd->new_sha1, new_sha1);
-- 
2.1.0-301-g54593e2
Junio C Hamano· Aug 19, 2014, 22:31 UTC · re: Junio C Hamano · lore

Re: [PATCH 02/18] receive-pack: parse feature request a bit earlier

Junio C Hamano <gitster@pobox.com> writes:
Show 32 quoted lines
> Ideally, we should have also allowed the first "shallow" to carry
> the feature request trailer, but that is water under the bridge
> now.  This makes the next step to factor out the queuing of commands
> easier to review.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> ...
> @@ -853,7 +853,18 @@ static struct command *read_head_info(struct sha1_array *shallow)
>  			continue;
>  		}
>  
> -		if (len < 83 ||
> +		linelen = strlen(line);
> +		if (linelen < len) {
> +			const char *feature_list = line + linelen + 1;
> +			if (parse_feature_request(feature_list, "report-status"))
> +				report_status = 1;
> +			if (parse_feature_request(feature_list, "side-band-64k"))
> +				use_sideband = LARGE_PACKET_MAX;
> +			if (parse_feature_request(feature_list, "quiet"))
> +				quiet = 1;
> +		}
> +
> +		if (linelen < 83 ||
>  		    line[40] != ' ' ||
>  		    line[81] != ' ' ||
>  		    get_sha1_hex(line, old_sha1) ||
> @@ -863,15 +874,6 @@ static struct command *read_head_info(struct sha1_array *shallow)
>  
>  		refname = line + 82;
>  		reflen = strlen(refname);

A later patch updates this to "reflen = linelen - 82" while moving this code to a helper function, but it may be better to do that in this patch.

Junio C Hamano· Aug 19, 2014, 22:06 UTC · re: Junio C Hamano · lore

[PATCH 03/18] receive-pack: do not reuse old_sha1[] to other things

This piece of code reads object names of shallow boundaries, not old_sha1[].

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/receive-pack.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
Show changes to builtin/receive-pack.c +5 −3
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 43f35c4..ee855b4 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -847,9 +847,11 @@ static struct command *read_head_info(struct sha1_array *shallow)
 			break;
 
 		if (len == 48 && starts_with(line, "shallow ")) {
-			if (get_sha1_hex(line + 8, old_sha1))
-				die("protocol error: expected shallow sha, got '%s'", line + 8);
-			sha1_array_append(shallow, old_sha1);
+			unsigned char sha1[20];
+			if (get_sha1_hex(line + 8, sha1))
+				die("protocol error: expected shallow sha, got '%s'",
+				    line + 8);
+			sha1_array_append(shallow, sha1);
 			continue;
 		}
 
-- 
2.1.0-301-g54593e2
Junio C Hamano· Aug 19, 2014, 22:32 UTC · re: Junio C Hamano · lore

Re: [PATCH 03/18] receive-pack: do not reuse old_sha1[] to other things

Junio C Hamano <gitster@pobox.com> writes:
Show 5 quoted lines
> This piece of code reads object names of shallow boundaries,
> not old_sha1[].
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---

I should double the subject line and say "do not reuse ... for other things".

Junio C Hamano· Aug 19, 2014, 22:06 UTC · re: Junio C Hamano · lore

[PATCH 04/18] receive-pack: factor out queueing of command

Make a helper function to accept a line of a protocol message and queue an update command out of the code from read_head_info().

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/receive-pack.c | 50 +++++++++++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 21 deletions(-)
Show changes to builtin/receive-pack.c +29 −21
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index ee855b4..341bb46 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -831,16 +831,40 @@ static void execute_commands(struct command *commands,
 		      "the reported refs above");
 }
 
+static struct command **queue_command(struct command **p,
+				      const char *line,
+				      int linelen)
+{
+	unsigned char old_sha1[20], new_sha1[20];
+	struct command *cmd;
+	const char *refname;
+	int reflen;
+
+	if (linelen < 83 ||
+	    line[40] != ' ' ||
+	    line[81] != ' ' ||
+	    get_sha1_hex(line, old_sha1) ||
+	    get_sha1_hex(line + 41, new_sha1))
+		die("protocol error: expected old/new/ref, got '%s'", line);
+
+	refname = line + 82;
+	reflen = linelen - 82;
+	cmd = xcalloc(1, sizeof(struct command) + reflen + 1);
+	hashcpy(cmd->old_sha1, old_sha1);
+	hashcpy(cmd->new_sha1, new_sha1);
+	memcpy(cmd->ref_name, refname, reflen);
+	cmd->ref_name[reflen] = '\0';
+	*p = cmd;
+	return &cmd->next;
+}
+
 static struct command *read_head_info(struct sha1_array *shallow)
 {
 	struct command *commands = NULL;
 	struct command **p = &commands;
 	for (;;) {
 		char *line;
-		unsigned char old_sha1[20], new_sha1[20];
-		struct command *cmd;
-		char *refname;
-		int len, reflen, linelen;
+		int len, linelen;
 
 		line = packet_read_line(0, &len);
 		if (!line)
@@ -866,23 +890,7 @@ static struct command *read_head_info(struct sha1_array *shallow)
 				quiet = 1;
 		}
 
-		if (linelen < 83 ||
-		    line[40] != ' ' ||
-		    line[81] != ' ' ||
-		    get_sha1_hex(line, old_sha1) ||
-		    get_sha1_hex(line + 41, new_sha1))
-			die("protocol error: expected old/new/ref, got '%s'",
-			    line);
-
-		refname = line + 82;
-		reflen = strlen(refname);
-		cmd = xcalloc(1, sizeof(struct command) + reflen + 1);
-		hashcpy(cmd->old_sha1, old_sha1);
-		hashcpy(cmd->new_sha1, new_sha1);
-		memcpy(cmd->ref_name, refname, reflen);
-		cmd->ref_name[reflen] = '\0';
-		*p = cmd;
-		p = &cmd->next;
+		p = queue_command(p, line, linelen);
 	}
 	return commands;
 }
-- 
2.1.0-301-g54593e2
Junio C Hamano· Aug 19, 2014, 22:06 UTC · re: Junio C Hamano · lore

[PATCH 05/18] send-pack: move REF_STATUS_REJECT_NODELETE logic a bit higher

20e8b465 (refactor ref status logic for pushing, 2010-01-08) restructured the code to set status for each ref to be pushed, but did not quite go far enough. We inspect the status set earlier by set_refs_status_for_push() and then perform yet another update to the status of a ref with an otherwise OK status to be deleted to mark it with REF_STATUS_REJECT_NODELETE when the protocol tells us never to delete.

Split the latter into a separate loop that comes before we enter the per-ref loop. This way we would have one less condition to check in the main loop.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 send-pack.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)
Show changes to send-pack.c +10 −5
diff --git a/send-pack.c b/send-pack.c
index 6129b0f..7428ae6 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -231,6 +231,15 @@ int send_pack(struct send_pack_args *args,
 		return 0;
 	}
 
+	/*
+	 * NEEDSWORK: why is delete-refs so specific to send-pack
+	 * machinery that set_ref_status_for_push() cannot set this
+	 * bit for us???
+	 */
+	for (ref = remote_refs; ref; ref = ref->next)
+		if (ref->deletion && !allow_deleting_refs)
+			ref->status = REF_STATUS_REJECT_NODELETE;
+
 	if (!args->dry_run)
 		advertise_shallow_grafts_buf(&req_buf);
 
@@ -249,17 +258,13 @@ int send_pack(struct send_pack_args *args,
 		case REF_STATUS_REJECT_FETCH_FIRST:
 		case REF_STATUS_REJECT_NEEDS_FORCE:
 		case REF_STATUS_REJECT_STALE:
+		case REF_STATUS_REJECT_NODELETE:
 		case REF_STATUS_UPTODATE:
 			continue;
 		default:
 			; /* do nothing */
 		}
 
-		if (ref->deletion && !allow_deleting_refs) {
-			ref->status = REF_STATUS_REJECT_NODELETE;
-			continue;
-		}
-
 		if (!ref->deletion)
 			new_refs++;
 
-- 
2.1.0-301-g54593e2
Junio C Hamano· Aug 19, 2014, 22:06 UTC · re: Junio C Hamano · lore

[PATCH 06/18] send-pack: refactor decision to send update per ref

A new helper function ref_update_to_be_sent() decides for each ref if the update is to be sent based on the status previously set by set_ref_status_for_push() and also if this is a mirrored push.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 send-pack.c | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)
Show changes to send-pack.c +21 −15
diff --git a/send-pack.c b/send-pack.c
index 7428ae6..f3c5ebe 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -190,6 +190,26 @@ static void advertise_shallow_grafts_buf(struct strbuf *sb)
 	for_each_commit_graft(advertise_shallow_grafts_cb, sb);
 }
 
+static int ref_update_to_be_sent(const struct ref *ref, const struct send_pack_args *args)
+{
+	if (!ref->peer_ref && !args->send_mirror)
+		return 0;
+
+	/* Check for statuses set by set_ref_status_for_push() */
+	switch (ref->status) {
+	case REF_STATUS_REJECT_NONFASTFORWARD:
+	case REF_STATUS_REJECT_ALREADY_EXISTS:
+	case REF_STATUS_REJECT_FETCH_FIRST:
+	case REF_STATUS_REJECT_NEEDS_FORCE:
+	case REF_STATUS_REJECT_STALE:
+	case REF_STATUS_REJECT_NODELETE:
+	case REF_STATUS_UPTODATE:
+		return 0;
+	default:
+		return 1;
+	}
+}
+
 int send_pack(struct send_pack_args *args,
 	      int fd[], struct child_process *conn,
 	      struct ref *remote_refs,
@@ -248,23 +268,9 @@ int send_pack(struct send_pack_args *args,
 	 */
 	new_refs = 0;
 	for (ref = remote_refs; ref; ref = ref->next) {
-		if (!ref->peer_ref && !args->send_mirror)
+		if (!ref_update_to_be_sent(ref, args))
 			continue;
 
-		/* Check for statuses set by set_ref_status_for_push() */
-		switch (ref->status) {
-		case REF_STATUS_REJECT_NONFASTFORWARD:
-		case REF_STATUS_REJECT_ALREADY_EXISTS:
-		case REF_STATUS_REJECT_FETCH_FIRST:
-		case REF_STATUS_REJECT_NEEDS_FORCE:
-		case REF_STATUS_REJECT_STALE:
-		case REF_STATUS_REJECT_NODELETE:
-		case REF_STATUS_UPTODATE:
-			continue;
-		default:
-			; /* do nothing */
-		}
-
 		if (!ref->deletion)
 			new_refs++;
 
-- 
2.1.0-301-g54593e2
Junio C Hamano· Aug 19, 2014, 22:06 UTC · re: Junio C Hamano · lore

[PATCH 07/18] send-pack: always send capabilities

We tried to avoid sending one extra byte, NUL and nothing behind it to signal there is no protocol capabilities being sent, on the first command packet on the wire, but it just made the code look ugly.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 send-pack.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
Show changes to send-pack.c +1 −3
diff --git a/send-pack.c b/send-pack.c
index f3c5ebe..2fa6c34 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -281,8 +281,7 @@ int send_pack(struct send_pack_args *args,
 			char *new_hex = sha1_to_hex(ref->new_sha1);
 			int quiet = quiet_supported && (args->quiet || !args->progress);
 
-			if (!cmds_sent && (status_report || use_sideband ||
-					   quiet || agent_supported)) {
+			if (!cmds_sent)
 				packet_buf_write(&req_buf,
 						 "%s %s %s%c%s%s%s%s%s",
 						 old_hex, new_hex, ref->name, 0,
@@ -292,7 +291,6 @@ int send_pack(struct send_pack_args *args,
 						 agent_supported ? " agent=" : "",
 						 agent_supported ? git_user_agent_sanitized() : ""
 						);
-			}
 			else
 				packet_buf_write(&req_buf, "%s %s %s",
 						 old_hex, new_hex, ref->name);
-- 
2.1.0-301-g54593e2
Junio C Hamano· Aug 19, 2014, 22:06 UTC · re: Junio C Hamano · lore

[PATCH 08/18] send-pack: factor out capability string generation

A run of 'var ? " var" : ""' fed to a long printf string in a deeply nested block was hard to read. Move it outside the loop and format it into a strbuf.

As an added bonus, the trick to add "agent=<agent-name>" by using two conditionals is replaced by a more readable version.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 send-pack.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)
Show changes to send-pack.c +13 −8
diff --git a/send-pack.c b/send-pack.c
index 2fa6c34..c1c64ee 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -218,6 +218,7 @@ int send_pack(struct send_pack_args *args,
 	int in = fd[0];
 	int out = fd[1];
 	struct strbuf req_buf = STRBUF_INIT;
+	struct strbuf cap_buf = STRBUF_INIT;
 	struct ref *ref;
 	int new_refs;
 	int allow_deleting_refs = 0;
@@ -251,6 +252,15 @@ int send_pack(struct send_pack_args *args,
 		return 0;
 	}
 
+	if (status_report)
+		strbuf_addstr(&cap_buf, " report-status");
+	if (use_sideband)
+		strbuf_addstr(&cap_buf, " side-band-64k");
+	if (quiet_supported && (args->quiet || !args->progress))
+		strbuf_addstr(&cap_buf, " quiet");
+	if (agent_supported)
+		strbuf_addf(&cap_buf, " agent=%s", git_user_agent_sanitized());
+
 	/*
 	 * NEEDSWORK: why is delete-refs so specific to send-pack
 	 * machinery that set_ref_status_for_push() cannot set this
@@ -279,18 +289,12 @@ int send_pack(struct send_pack_args *args,
 		} else {
 			char *old_hex = sha1_to_hex(ref->old_sha1);
 			char *new_hex = sha1_to_hex(ref->new_sha1);
-			int quiet = quiet_supported && (args->quiet || !args->progress);
 
 			if (!cmds_sent)
 				packet_buf_write(&req_buf,
-						 "%s %s %s%c%s%s%s%s%s",
+						 "%s %s %s%c%s",
 						 old_hex, new_hex, ref->name, 0,
-						 status_report ? " report-status" : "",
-						 use_sideband ? " side-band-64k" : "",
-						 quiet ? " quiet" : "",
-						 agent_supported ? " agent=" : "",
-						 agent_supported ? git_user_agent_sanitized() : ""
-						);
+						 cap_buf.buf);
 			else
 				packet_buf_write(&req_buf, "%s %s %s",
 						 old_hex, new_hex, ref->name);
@@ -311,6 +315,7 @@ int send_pack(struct send_pack_args *args,
 		packet_flush(out);
 	}
 	strbuf_release(&req_buf);
+	strbuf_release(&cap_buf);
 
 	if (use_sideband && cmds_sent) {
 		memset(&demux, 0, sizeof(demux));
-- 
2.1.0-301-g54593e2
Junio C Hamano· Aug 19, 2014, 22:06 UTC · re: Junio C Hamano · lore

[PATCH 09/18] send-pack: rename "new_refs" to "need_pack_data"

The variable counts how many non-deleting command is being sent, but is only checked with 0-ness to decide if we need to send the pack data.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 send-pack.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
Show changes to send-pack.c +3 −4
diff --git a/send-pack.c b/send-pack.c
index c1c64ee..590eb0a 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -220,7 +220,7 @@ int send_pack(struct send_pack_args *args,
 	struct strbuf req_buf = STRBUF_INIT;
 	struct strbuf cap_buf = STRBUF_INIT;
 	struct ref *ref;
-	int new_refs;
+	int need_pack_data = 0;
 	int allow_deleting_refs = 0;
 	int status_report = 0;
 	int use_sideband = 0;
@@ -276,13 +276,12 @@ int send_pack(struct send_pack_args *args,
 	/*
 	 * Finally, tell the other end!
 	 */
-	new_refs = 0;
 	for (ref = remote_refs; ref; ref = ref->next) {
 		if (!ref_update_to_be_sent(ref, args))
 			continue;
 
 		if (!ref->deletion)
-			new_refs++;
+			need_pack_data = 1;
 
 		if (args->dry_run) {
 			ref->status = REF_STATUS_OK;
@@ -327,7 +326,7 @@ int send_pack(struct send_pack_args *args,
 		in = demux.out;
 	}
 
-	if (new_refs && cmds_sent) {
+	if (need_pack_data && cmds_sent) {
 		if (pack_objects(out, remote_refs, extra_have, args) < 0) {
 			for (ref = remote_refs; ref; ref = ref->next)
 				ref->status = REF_STATUS_NONE;
-- 
2.1.0-301-g54593e2
Junio C Hamano· Aug 19, 2014, 22:06 UTC · re: Junio C Hamano · lore

[PATCH 10/18] send-pack: refactor inspecting and resetting status and sending commands

The main loop over remote_refs list inspects the ref status to see if we need to generate pack data (i.e. a delete-only push does not need to send any additional data), resets it to "expecting the status report" state, and formats the actual update commands to be sent.

Split the former two out of the main loop, as it will become conditional in later steps.

Besides, we should have code that does real thing here, before the "Finally, tell the other end!" part ;-)

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 send-pack.c | 49 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 19 deletions(-)
Show changes to send-pack.c +30 −19
diff --git a/send-pack.c b/send-pack.c
index 590eb0a..f3262f2 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -274,7 +274,8 @@ int send_pack(struct send_pack_args *args,
 		advertise_shallow_grafts_buf(&req_buf);
 
 	/*
-	 * Finally, tell the other end!
+	 * Clear the status for each ref and see if we need to send
+	 * the pack data.
 	 */
 	for (ref = remote_refs; ref; ref = ref->next) {
 		if (!ref_update_to_be_sent(ref, args))
@@ -283,25 +284,35 @@ int send_pack(struct send_pack_args *args,
 		if (!ref->deletion)
 			need_pack_data = 1;
 
-		if (args->dry_run) {
+		if (args->dry_run || !status_report)
 			ref->status = REF_STATUS_OK;
-		} else {
-			char *old_hex = sha1_to_hex(ref->old_sha1);
-			char *new_hex = sha1_to_hex(ref->new_sha1);
-
-			if (!cmds_sent)
-				packet_buf_write(&req_buf,
-						 "%s %s %s%c%s",
-						 old_hex, new_hex, ref->name, 0,
-						 cap_buf.buf);
-			else
-				packet_buf_write(&req_buf, "%s %s %s",
-						 old_hex, new_hex, ref->name);
-			ref->status = status_report ?
-				REF_STATUS_EXPECTING_REPORT :
-				REF_STATUS_OK;
-			cmds_sent++;
-		}
+		else
+			ref->status = REF_STATUS_EXPECTING_REPORT;
+	}
+
+	/*
+	 * Finally, tell the other end!
+	 */
+	for (ref = remote_refs; ref; ref = ref->next) {
+		char *old_hex, *new_hex;
+
+		if (args->dry_run)
+			continue;
+
+		if (!ref_update_to_be_sent(ref, args))
+			continue;
+
+		old_hex = sha1_to_hex(ref->old_sha1);
+		new_hex = sha1_to_hex(ref->new_sha1);
+		if (!cmds_sent)
+			packet_buf_write(&req_buf,
+					 "%s %s %s%c%s",
+					 old_hex, new_hex, ref->name, 0,
+					 cap_buf.buf);
+		else
+			packet_buf_write(&req_buf, "%s %s %s",
+					 old_hex, new_hex, ref->name);
+		cmds_sent++;
 	}
 
 	if (args->stateless_rpc) {
-- 
2.1.0-301-g54593e2
Junio C Hamano· Aug 19, 2014, 22:06 UTC · re: Junio C Hamano · lore

[PATCH 11/18] send-pack: clarify that cmds_sent is a boolean

We use it to make sure that the feature request is sent only once on the very first request packet (ignoring the "shallow " line, which was an unfortunate mistake we cannot retroactively fix with existing receive-pack already deployed in the field) and we set it to "true" with cmds_sent++, not because we care about the actual number of updates sent but because it is merely an old idiomatic way.

Set it explicitly to one to clarify that the code that uses this variable only cares about its zero-ness.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 send-pack.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
Show changes to send-pack.c +4 −3
diff --git a/send-pack.c b/send-pack.c
index f3262f2..05926d2 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -304,15 +304,16 @@ int send_pack(struct send_pack_args *args,
 
 		old_hex = sha1_to_hex(ref->old_sha1);
 		new_hex = sha1_to_hex(ref->new_sha1);
-		if (!cmds_sent)
+		if (!cmds_sent) {
 			packet_buf_write(&req_buf,
 					 "%s %s %s%c%s",
 					 old_hex, new_hex, ref->name, 0,
 					 cap_buf.buf);
-		else
+			cmds_sent = 1;
+		} else {
 			packet_buf_write(&req_buf, "%s %s %s",
 					 old_hex, new_hex, ref->name);
-		cmds_sent++;
+		}
 	}
 
 	if (args->stateless_rpc) {
-- 
2.1.0-301-g54593e2
Junio C Hamano· Aug 19, 2014, 22:06 UTC · re: Junio C Hamano · lore

[PATCH 12/18] gpg-interface: move parse_gpg_output() to where it should be

Earlier, ffb6d7d5 (Move commit GPG signature verification to commit.c, 2013-03-31) moved this helper that used to be in pretty.c (i.e. the output code path) to commit.c for better reusability.

It was a good first step in the right direction, but still suffers a myopic view that commits will be the only thing we would ever want to sign---we would actually want to be able to reuse it even wider.

The function interprets what GPG said; gpg-interface is obviously a better place. Move it there.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 commit.c        | 36 ------------------------------------
 gpg-interface.c | 36 ++++++++++++++++++++++++++++++++++++
 gpg-interface.h | 17 ++++++++++++-----
 3 files changed, 48 insertions(+), 41 deletions(-)
Show changes to 3 files +48 −41

commit.c, gpg-interface.c, gpg-interface.h

diff --git a/commit.c b/commit.c
index ae7f2b1..01cdad2 100644
--- a/commit.c
+++ b/commit.c
@@ -1220,42 +1220,6 @@ free_return:
 	free(buf);
 }
 
-static struct {
-	char result;
-	const char *check;
-} sigcheck_gpg_status[] = {
-	{ 'G', "\n[GNUPG:] GOODSIG " },
-	{ 'B', "\n[GNUPG:] BADSIG " },
-	{ 'U', "\n[GNUPG:] TRUST_NEVER" },
-	{ 'U', "\n[GNUPG:] TRUST_UNDEFINED" },
-};
-
-static void parse_gpg_output(struct signature_check *sigc)
-{
-	const char *buf = sigc->gpg_status;
-	int i;
-
-	/* Iterate over all search strings */
-	for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
-		const char *found, *next;
-
-		if (!skip_prefix(buf, sigcheck_gpg_status[i].check + 1, &found)) {
-			found = strstr(buf, sigcheck_gpg_status[i].check);
-			if (!found)
-				continue;
-			found += strlen(sigcheck_gpg_status[i].check);
-		}
-		sigc->result = sigcheck_gpg_status[i].result;
-		/* The trust messages are not followed by key/signer information */
-		if (sigc->result != 'U') {
-			sigc->key = xmemdupz(found, 16);
-			found += 17;
-			next = strchrnul(found, '\n');
-			sigc->signer = xmemdupz(found, next - found);
-		}
-	}
-}
-
 void check_commit_signature(const struct commit* commit, struct signature_check *sigc)
 {
 	struct strbuf payload = STRBUF_INIT;
diff --git a/gpg-interface.c b/gpg-interface.c
index ff07012..3c9624c 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -21,6 +21,42 @@ void signature_check_clear(struct signature_check *sigc)
 	sigc->key = NULL;
 }
 
+static struct {
+	char result;
+	const char *check;
+} sigcheck_gpg_status[] = {
+	{ 'G', "\n[GNUPG:] GOODSIG " },
+	{ 'B', "\n[GNUPG:] BADSIG " },
+	{ 'U', "\n[GNUPG:] TRUST_NEVER" },
+	{ 'U', "\n[GNUPG:] TRUST_UNDEFINED" },
+};
+
+void parse_gpg_output(struct signature_check *sigc)
+{
+	const char *buf = sigc->gpg_status;
+	int i;
+
+	/* Iterate over all search strings */
+	for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
+		const char *found, *next;
+
+		if (!skip_prefix(buf, sigcheck_gpg_status[i].check + 1, &found)) {
+			found = strstr(buf, sigcheck_gpg_status[i].check);
+			if (!found)
+				continue;
+			found += strlen(sigcheck_gpg_status[i].check);
+		}
+		sigc->result = sigcheck_gpg_status[i].result;
+		/* The trust messages are not followed by key/signer information */
+		if (sigc->result != 'U') {
+			sigc->key = xmemdupz(found, 16);
+			found += 17;
+			next = strchrnul(found, '\n');
+			sigc->signer = xmemdupz(found, next - found);
+		}
+	}
+}
+
 void set_signing_key(const char *key)
 {
 	free(configured_signing_key);
diff --git a/gpg-interface.h b/gpg-interface.h
index 37c23da..8d677cc 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -5,16 +5,23 @@ struct signature_check {
 	char *payload;
 	char *gpg_output;
 	char *gpg_status;
-	char result; /* 0 (not checked),
-		      * N (checked but no further result),
-		      * U (untrusted good),
-		      * G (good)
-		      * B (bad) */
+
+	/*
+	 * possible "result":
+	 * 0 (not checked)
+	 * N (checked but no further result)
+	 * U (untrusted good)
+	 * G (good)
+	 * B (bad)
+	 */
+	char result;
 	char *signer;
 	char *key;
 };
 
 extern void signature_check_clear(struct signature_check *sigc);
+extern void parse_gpg_output(struct signature_check *);
+
 extern int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key);
 extern int verify_signed_buffer(const char *payload, size_t payload_size, const char *signature, size_t signature_size, struct strbuf *gpg_output, struct strbuf *gpg_status);
 extern int git_gpg_config(const char *, const char *, void *);
-- 
2.1.0-301-g54593e2
Junio C Hamano· Aug 19, 2014, 22:06 UTC · re: Junio C Hamano · lore

[PATCH 13/18] gpg-interface: move parse_signature() to where it should be

Our signed-tag objects set the standard format used by Git to store GPG-signed payload (i.e. the payload followed by its detached signature), and it made sense to have a helper to find the boundary between the payload and its signature in tag.c back then.

Newer code added later to parse other kinds of objects that learned to use the same format to store GPG-signed payload (e.g. signed commits), however, kept using the helper from the same location.

Move it to gpg-interface; the helper is no longer about signed tag, but it is how our code and data interact with GPG.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 gpg-interface.c | 21 +++++++++++++++++++++
 gpg-interface.h |  1 +
 tag.c           | 20 --------------------
 tag.h           |  1 -
 4 files changed, 22 insertions(+), 21 deletions(-)
Show changes to 4 files +22 −21

gpg-interface.c, gpg-interface.h, tag.c, tag.h

diff --git a/gpg-interface.c b/gpg-interface.c
index 3c9624c..0dd11ea 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -7,6 +7,9 @@
 static char *configured_signing_key;
 static const char *gpg_program = "gpg";
 
+#define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
+#define PGP_MESSAGE "-----BEGIN PGP MESSAGE-----"
+
 void signature_check_clear(struct signature_check *sigc)
 {
 	free(sigc->payload);
@@ -57,6 +60,24 @@ void parse_gpg_output(struct signature_check *sigc)
 	}
 }
 
+/*
+ * Look at GPG signed content (e.g. a signed tag object), whose
+ * payload is followed by a detached signature on it.  Return the
+ * offset where the embedded detached signature begins, or the end of
+ * the data when there is no such signature.
+ */
+size_t parse_signature(const char *buf, unsigned long size)
+{
+	char *eol;
+	size_t len = 0;
+	while (len < size && !starts_with(buf + len, PGP_SIGNATURE) &&
+			!starts_with(buf + len, PGP_MESSAGE)) {
+		eol = memchr(buf + len, '\n', size - len);
+		len += eol ? eol - (buf + len) + 1 : size - len;
+	}
+	return len;
+}
+
 void set_signing_key(const char *key)
 {
 	free(configured_signing_key);
diff --git a/gpg-interface.h b/gpg-interface.h
index 8d677cc..93c76c2 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -20,6 +20,7 @@ struct signature_check {
 };
 
 extern void signature_check_clear(struct signature_check *sigc);
+extern size_t parse_signature(const char *buf, unsigned long size);
 extern void parse_gpg_output(struct signature_check *);
 
 extern int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key);
diff --git a/tag.c b/tag.c
index 82d841b..5b0ac62 100644
--- a/tag.c
+++ b/tag.c
@@ -4,9 +4,6 @@
 #include "tree.h"
 #include "blob.h"
 
-#define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
-#define PGP_MESSAGE "-----BEGIN PGP MESSAGE-----"
-
 const char *tag_type = "tag";
 
 struct object *deref_tag(struct object *o, const char *warn, int warnlen)
@@ -143,20 +140,3 @@ int parse_tag(struct tag *item)
 	free(data);
 	return ret;
 }
-
-/*
- * Look at a signed tag object, and return the offset where
- * the embedded detached signature begins, or the end of the
- * data when there is no such signature.
- */
-size_t parse_signature(const char *buf, unsigned long size)
-{
-	char *eol;
-	size_t len = 0;
-	while (len < size && !starts_with(buf + len, PGP_SIGNATURE) &&
-			!starts_with(buf + len, PGP_MESSAGE)) {
-		eol = memchr(buf + len, '\n', size - len);
-		len += eol ? eol - (buf + len) + 1 : size - len;
-	}
-	return len;
-}
diff --git a/tag.h b/tag.h
index bc8a1e4..f4580ae 100644
--- a/tag.h
+++ b/tag.h
@@ -17,6 +17,5 @@ extern int parse_tag_buffer(struct tag *item, const void *data, unsigned long si
 extern int parse_tag(struct tag *item);
 extern struct object *deref_tag(struct object *, const char *, int);
 extern struct object *deref_tag_noverify(struct object *);
-extern size_t parse_signature(const char *buf, unsigned long size);
 
 #endif /* TAG_H */
-- 
2.1.0-301-g54593e2
Junio C Hamano· Aug 19, 2014, 22:06 UTC · re: Junio C Hamano · lore

[PATCH 14/18] pack-protocol doc: typofix for PKT-LINE

Everywhere else we use PKT-LINE to denote the pkt-line formatted data, but "shallow/deepen" messages are described with PKT_LINE().

Fix them.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/technical/pack-protocol.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to Documentation/technical/pack-protocol.txt +2 −2
diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
index 18dea8d..a845d51 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -212,9 +212,9 @@ out of what the server said it could do with the first 'want' line.
   want-list         =  first-want
 		       *additional-want
 
-  shallow-line      =  PKT_LINE("shallow" SP obj-id)
+  shallow-line      =  PKT-LINE("shallow" SP obj-id)
 
-  depth-request     =  PKT_LINE("deepen" SP depth)
+  depth-request     =  PKT-LINE("deepen" SP depth)
 
   first-want        =  PKT-LINE("want" SP obj-id SP capability-list LF)
   additional-want   =  PKT-LINE("want" SP obj-id LF)
-- 
2.1.0-301-g54593e2
Junio C Hamano· Aug 19, 2014, 22:06 UTC · re: Junio C Hamano · lore

[PATCH 15/18] the beginning of the signed push

While signed tags and commits assert that the objects thusly signed came from you, who signed these objects, there is not a good way to assert that you wanted to have a particular object at the tip of a particular branch. My signing v2.0.1 tag only means I want to call the version v2.0.1, and it does not mean I want to push it out to my 'master' branch---it is likely that I only want it in 'maint'.

Introduce a mechanism that allows you to sign a "push certificate" (for the lack of better name) every time you push, asserting that what object you are pushing to update which ref that used to point at what other object. Think of it as a cryptographic protection for ref updates, similar to signed tags/commits but working on an orthogonal axis.

The basic flow based on this mechanism goes like this:
 1. You push out your work with "git push -s".
 2. The sending side learns where the remote refs are as usual,
    together with what protocol extension the receiving end
    supports.  If the receiving end does not advertise the protocol
    extension "push-cert", the sending side falls back to the normal
    push that is not signed.
    Otherwise, a text file, that looks like the following, is
    prepared in core:
	certificate version 0.1
	pusher Junio C Hamano <gitster@pobox.com> 1315427886 -0700
	7339ca65... 21580ecb... refs/heads/master
	3793ac56... 12850bec... refs/heads/next
    Each line shows the old and the new object name at the tip of
    the ref this push tries to update, in the way identical to how
    the underlying "git push" protocol exchange tells the ref
    updates to the receiving end (by recording the "old" object
    name, the push certificate also protects against replaying).  It
    is expected that new command packet types other than the
    old-new-refname kind will be included in push certificate in the
    same way as would appear in the plain vanilla command packets in
    unsigned pushes.
    The user then is asked to sign this push certificate using GPG,
    formatted in a way similar to how signed tag objects are signed,
    and the result is sent to the other side (i.e. receive-pack).
    In the protocol exchange, this step comes immediately before the
    sender tells what the result of the push should be, which in
    turn comes before it sends the pack data.
 3. When the receiving end sees a push certificate, the certificate
    is written out as a blob.  The pre-receive hook can learn about
    the certificate by checking GIT_PUSH_CERT environment variable,
    which, if present, tells the object name of this blob, and make
    the decision to allow or reject this push.  Additionally, the
    post-receive hook can also look at the certificate, which may be
    a good place to log all the received certificates for later
    audits.

Because a push certificate carry the same information as the usual command packets in the protocol exchange, we can omit the latter when a push certificate is in use and reduce the protocol overhead. This however is not included in this patch to make it easier to review (in other words, the series at this step should never be released without the remainder of the series, as it implements an interim protocol that will be incompatible with the final one, merely for reviewing purposes). As such, the documentation update for the protocol is left out of this step.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-push.txt         |  9 +++++-
 Documentation/git-receive-pack.txt | 16 ++++++++-
 builtin/push.c                     |  1 +
 builtin/receive-pack.c             | 43 ++++++++++++++++++++++++-
 send-pack.c                        | 66 ++++++++++++++++++++++++++++++++++++++
 send-pack.h                        |  1 +
 t/t5534-push-signed.sh             | 63 ++++++++++++++++++++++++++++++++++++
 transport.c                        |  4 +++
 transport.h                        |  5 +++
 9 files changed, 205 insertions(+), 3 deletions(-)
 create mode 100755 t/t5534-push-signed.sh
Show changes to 9 files +205 −3

Documentation/git-push.txt, Documentation/git-receive-pack.txt, builtin/push.c, builtin/receive-pack.c, send-pack.c, send-pack.h, t/t5534-push-signed.sh, transport.c, transport.h

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 21cd455..21b3f29 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -10,7 +10,8 @@ SYNOPSIS
 --------
 [verse]
 'git push' [--all | --mirror | --tags] [--follow-tags] [-n | --dry-run] [--receive-pack=<git-receive-pack>]
-	   [--repo=<repository>] [-f | --force] [--prune] [-v | --verbose] [-u | --set-upstream]
+	   [--repo=<repository>] [-f | --force] [--prune] [-v | --verbose]
+	   [-u | --set-upstream] [--signed]
 	   [--force-with-lease[=<refname>[:<expect>]]]
 	   [--no-verify] [<repository> [<refspec>...]]
 
@@ -129,6 +130,12 @@ already exists on the remote side.
 	from the remote but are pointing at commit-ish that are
 	reachable from the refs being pushed.
 
+--signed::
+	GPG-sign the push request to update refs on the receiving
+	side, to allow it to be checked by the hooks and/or be
+	logged.  See linkgit:git-receive-pack[1] for the details
+	on the receiving end.
+
 --receive-pack=<git-receive-pack>::
 --exec=<git-receive-pack>::
 	Path to the 'git-receive-pack' program on the remote
diff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt
index b1f7dc6..6c458af 100644
--- a/Documentation/git-receive-pack.txt
+++ b/Documentation/git-receive-pack.txt
@@ -53,6 +53,11 @@ the update.  Refs to be created will have sha1-old equal to 0\{40},
 while refs to be deleted will have sha1-new equal to 0\{40}, otherwise
 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.
+
 This hook is called before any refname is updated and before any
 fast-forward checks are performed.
 
@@ -101,9 +106,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
+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:
+ref listing the commits pushed to the repository, and logs the push
+certificates of signed pushes to a file:
 
 	#!/bin/sh
 	# mail out commit update information.
@@ -119,6 +128,11 @@ ref listing the commits pushed to the repository:
 		fi |
 		mail -s "Changes to ref $ref" commit-list@mydomain
 	done
+	# log signed push certificate, if any
+	if test -n "${GIT_PUSH_CERT-}"
+	then
+		git cat-file blob ${GIT_PUSH_CERT} >>/var/log/push-log
+	fi
 	exit 0
 
 The exit code from this hook invocation is ignored, however a
diff --git a/builtin/push.c b/builtin/push.c
index f50e3d5..ae56f73 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -506,6 +506,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "no-verify", &flags, N_("bypass pre-push hook"), TRANSPORT_PUSH_NO_HOOK),
 		OPT_BIT(0, "follow-tags", &flags, N_("push missing but relevant tags"),
 			TRANSPORT_PUSH_FOLLOW_TAGS),
+		OPT_BIT(0, "signed", &flags, N_("GPG sign the push"), TRANSPORT_PUSH_CERT),
 		OPT_END()
 	};
 
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 341bb46..f30df8a 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -46,6 +46,8 @@ static void *head_name_to_free;
 static int sent_capabilities;
 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 enum deny_action parse_deny_action(const char *var, const char *value)
 {
@@ -142,7 +144,7 @@ static void show_ref(const char *path, const unsigned char *sha1)
 	else
 		packet_write(1, "%s %s%c%s%s agent=%s\n",
 			     sha1_to_hex(sha1), path, 0,
-			     " report-status delete-refs side-band-64k quiet",
+			     " report-status delete-refs side-band-64k quiet push-cert",
 			     prefer_ofs_delta ? " ofs-delta" : "",
 			     git_user_agent_sanitized());
 	sent_capabilities = 1;
@@ -252,6 +254,22 @@ static int copy_to_sideband(int in, int out, void *arg)
 	return 0;
 }
 
+static void prepare_push_cert_sha1(struct child_process *proc)
+{
+	static int already_done;
+	struct argv_array env = ARGV_ARRAY_INIT;
+
+	if (!already_done) {
+		already_done = 1;
+		if (write_sha1_file(push_cert.buf, push_cert.len, "blob", push_cert_sha1))
+			hashclr(push_cert_sha1);
+	}
+	if (!is_null_sha1(push_cert_sha1)) {
+		argv_array_pushf(&env, "GIT_PUSH_CERT=%s", sha1_to_hex(push_cert_sha1));
+		proc->env = env.argv;
+	}
+}
+
 typedef int (*feed_fn)(void *, const char **, size_t *);
 static int run_and_feed_hook(const char *hook_name, feed_fn feed, void *feed_state)
 {
@@ -271,6 +289,8 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed, void *feed_sta
 	proc.in = -1;
 	proc.stdout_to_stderr = 1;
 
+	prepare_push_cert_sha1(&proc);
+
 	if (use_sideband) {
 		memset(&muxer, 0, sizeof(muxer));
 		muxer.proc = copy_to_sideband;
@@ -890,6 +910,27 @@ static struct command *read_head_info(struct sha1_array *shallow)
 				quiet = 1;
 		}
 
+		if (!strcmp(line, "push-cert")) {
+			int true_flush = 0;
+			char certbuf[1024];
+
+			for (;;) {
+				len = packet_read(0, NULL, NULL,
+						  certbuf, sizeof(certbuf), 0);
+				if (!len) {
+					true_flush = 1;
+					break;
+				}
+				if (!strcmp(certbuf, "push-cert-end\n"))
+					break; /* end of cert */
+				strbuf_addstr(&push_cert, certbuf);
+			}
+
+			if (true_flush)
+				break;
+			continue;
+		}
+
 		p = queue_command(p, line, linelen);
 	}
 	return commands;
diff --git a/send-pack.c b/send-pack.c
index 05926d2..a073891 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -11,6 +11,7 @@
 #include "transport.h"
 #include "version.h"
 #include "sha1-array.h"
+#include "gpg-interface.h"
 
 static int feed_object(const unsigned char *sha1, int fd, int negative)
 {
@@ -210,6 +211,64 @@ static int ref_update_to_be_sent(const struct ref *ref, const struct send_pack_a
 	}
 }
 
+/*
+ * the beginning of the next line, or the end of buffer.
+ *
+ * NEEDSWORK: perhaps move this to git-compat-util.h or somewhere and
+ * convert many similar uses found by "git grep -A4 memchr".
+ */
+static const char *next_line(const char *line, size_t len)
+{
+	const char *nl = memchr(line, '\n', len);
+	if (!nl)
+		return line + len; /* incomplete line */
+	return nl + 1;
+}
+
+static void generate_push_cert(struct strbuf *req_buf,
+			       const struct ref *remote_refs,
+			       struct send_pack_args *args)
+{
+	const struct ref *ref;
+	char stamp[60];
+	char *signing_key = xstrdup(get_signing_key());
+	const char *cp, *np;
+	struct strbuf cert = STRBUF_INIT;
+	int update_seen = 0;
+
+	datestamp(stamp, sizeof(stamp));
+	strbuf_addf(&cert, "certificate version 0.1\n");
+	strbuf_addf(&cert, "pusher %s %s\n", signing_key, stamp);
+	strbuf_addstr(&cert, "\n");
+
+	for (ref = remote_refs; ref; ref = ref->next) {
+		if (!ref_update_to_be_sent(ref, args))
+			continue;
+		update_seen = 1;
+		strbuf_addf(&cert, "%s %s %s\n",
+			    sha1_to_hex(ref->old_sha1),
+			    sha1_to_hex(ref->new_sha1),
+			    ref->name);
+	}
+	if (!update_seen)
+		goto free_return;
+
+	if (sign_buffer(&cert, &cert, signing_key))
+		die(_("failed to sign the push certificate"));
+
+	packet_buf_write(req_buf, "push-cert\n");
+	for (cp = cert.buf; cp < cert.buf + cert.len; cp = np) {
+		np = next_line(cp, cert.buf + cert.len - cp);
+		packet_buf_write(req_buf,
+				 "%.*s", (int)(np - cp), cp);
+	}
+	packet_buf_write(req_buf, "push-cert-end\n");
+
+free_return:
+	free(signing_key);
+	strbuf_release(&cert);
+}
+
 int send_pack(struct send_pack_args *args,
 	      int fd[], struct child_process *conn,
 	      struct ref *remote_refs,
@@ -245,6 +304,10 @@ int send_pack(struct send_pack_args *args,
 		agent_supported = 1;
 	if (server_supports("no-thin"))
 		args->use_thin_pack = 0;
+	if (args->push_cert && !server_supports("push-cert")) {
+		warning(_("the receiving end does not support --signed push"));
+		args->push_cert = 0;
+	}
 
 	if (!remote_refs) {
 		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
@@ -273,6 +336,9 @@ int send_pack(struct send_pack_args *args,
 	if (!args->dry_run)
 		advertise_shallow_grafts_buf(&req_buf);
 
+	if (!args->dry_run && args->push_cert)
+		generate_push_cert(&req_buf, remote_refs, args);
+
 	/*
 	 * Clear the status for each ref and see if we need to send
 	 * the pack data.
diff --git a/send-pack.h b/send-pack.h
index 8e84392..3555d8e 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,
+		push_cert:1,
 		stateless_rpc:1;
 };
 
diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
new file mode 100755
index 0000000..0eb5004
--- /dev/null
+++ b/t/t5534-push-signed.sh
@@ -0,0 +1,63 @@
+#!/bin/sh
+
+test_description='signed push'
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-gpg.sh
+
+prepare_dst () {
+	rm -fr dst &&
+	test_create_repo dst &&
+
+	git push dst master:noop master:ff master:noff
+}
+
+test_expect_success setup '
+	# master, ff and noff branches pointing at the same commit
+	test_tick &&
+	git commit --allow-empty -m initial &&
+
+	git checkout -b noop &&
+	git checkout -b ff &&
+	git checkout -b noff &&
+
+	# noop stays the same, ff advances, noff rewrites
+	test_tick &&
+	git commit --allow-empty --amend -m rewritten &&
+	git checkout ff &&
+
+	test_tick &&
+	git commit --allow-empty -m second
+'
+
+test_expect_success 'unsigned push does not send push certificate' '
+	prepare_dst &&
+	mkdir -p dst/.git/hooks &&
+	write_script dst/.git/hooks/post-receive <<-\EOF &&
+	if test -n "${GIT_PUSH_CERT-}"
+	then
+		git cat-file blob $GIT_PUSH_CERT >../push-cert
+	fi
+	EOF
+
+	git push dst noop ff +noff &&
+	test -f dst/push-cert &&
+	! test -s dst/push-cert
+'
+
+test_expect_success GPG 'signed push sends push certificate' '
+	prepare_dst &&
+	mkdir -p dst/.git/hooks &&
+	write_script dst/.git/hooks/post-receive <<-\EOF &&
+	if test -n "${GIT_PUSH_CERT-}"
+	then
+		git cat-file blob $GIT_PUSH_CERT >../push-cert
+	fi
+	EOF
+
+	git push --signed dst noop ff +noff &&
+	grep "$(git rev-parse noop ff) refs/heads/ff" dst/push-cert &&
+	grep "$(git rev-parse noop noff) refs/heads/noff" dst/push-cert
+'
+
+test_done
diff --git a/transport.c b/transport.c
index 662421b..07fdf86 100644
--- a/transport.c
+++ b/transport.c
@@ -480,6 +480,9 @@ static int set_git_option(struct git_transport_options *opts,
 				die("transport: invalid depth option '%s'", value);
 		}
 		return 0;
+	} else if (!strcmp(name, TRANS_OPT_PUSH_CERT)) {
+		opts->push_cert = !!value;
+		return 0;
 	}
 	return 1;
 }
@@ -823,6 +826,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.push_cert = !!(flags & TRANSPORT_PUSH_CERT);
 
 	ret = send_pack(&args, data->fd, data->conn, remote_refs,
 			&data->extra_have);
diff --git a/transport.h b/transport.h
index 02ea248..3e0091e 100644
--- a/transport.h
+++ b/transport.h
@@ -12,6 +12,7 @@ struct git_transport_options {
 	unsigned check_self_contained_and_connected : 1;
 	unsigned self_contained_and_connected : 1;
 	unsigned update_shallow : 1;
+	unsigned push_cert : 1;
 	int depth;
 	const char *uploadpack;
 	const char *receivepack;
@@ -123,6 +124,7 @@ struct transport {
 #define TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND 256
 #define TRANSPORT_PUSH_NO_HOOK 512
 #define TRANSPORT_PUSH_FOLLOW_TAGS 1024
+#define TRANSPORT_PUSH_CERT 2048
 
 #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
 #define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)
@@ -156,6 +158,9 @@ struct transport *transport_get(struct remote *, const char *);
 /* Accept refs that may update .git/shallow without --depth */
 #define TRANS_OPT_UPDATE_SHALLOW "updateshallow"
 
+/* Send push certificates */
+#define TRANS_OPT_PUSH_CERT "pushcert"
+
 /**
  * Returns 0 if the option was used, non-zero otherwise. Prints a
  * message to stderr if the option is not used.
-- 
2.1.0-301-g54593e2
brian m. carlson· Aug 20, 2014, 02:48 UTC · re: Junio C Hamano · lore

Re: [PATCH 15/18] the beginning of the signed push

On Tue, Aug 19, 2014 at 03:06:24PM -0700, Junio C Hamano wrote:
Show 17 quoted lines
> While signed tags and commits assert that the objects thusly signed
> came from you, who signed these objects, there is not a good way to
> assert that you wanted to have a particular object at the tip of a
> particular branch.  My signing v2.0.1 tag only means I want to call
> the version v2.0.1, and it does not mean I want to push it out to my
> 'master' branch---it is likely that I only want it in 'maint'.
> 
> Introduce a mechanism that allows you to sign a "push certificate"
> (for the lack of better name) every time you push, asserting that
> what object you are pushing to update which ref that used to point
> at what other object.  Think of it as a cryptographic protection for
> ref updates, similar to signed tags/commits but working on an
> orthogonal axis.
> 
> The basic flow based on this mechanism goes like this:
> 
>  1. You push out your work with "git push -s".

You wrote "git push -s", but the command below only seems to understand --signed, not -s. It should probably be consistent.

Show 12 quoted lines
> diff --git a/builtin/push.c b/builtin/push.c
> index f50e3d5..ae56f73 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -506,6 +506,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
>  		OPT_BIT(0, "no-verify", &flags, N_("bypass pre-push hook"), TRANSPORT_PUSH_NO_HOOK),
>  		OPT_BIT(0, "follow-tags", &flags, N_("push missing but relevant tags"),
>  			TRANSPORT_PUSH_FOLLOW_TAGS),
> +		OPT_BIT(0, "signed", &flags, N_("GPG sign the push"), TRANSPORT_PUSH_CERT),
>  		OPT_END()
>  	};
>  
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187
Bert Wesarg· Aug 20, 2014, 06:57 UTC · re: Junio C Hamano · lore

Re: [PATCH 15/18] the beginning of the signed push

On Wed, Aug 20, 2014 at 12:06 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
> The basic flow based on this mechanism goes like this:
>
>  1. You push out your work with "git push -s".
>
>  2. The sending side learns where the remote refs are as usual,
>     together with what protocol extension the receiving end
>     supports.  If the receiving end does not advertise the protocol
>     extension "push-cert", the sending side falls back to the normal
>     push that is not signed.
>

Is this fallback silently? If so I think it would be better to abort the push, if the receiver does not support this but the user requested it.

Bert
Junio C Hamano· Aug 20, 2014, 23:41 UTC · re: Bert Wesarg · lore

Re: [PATCH 15/18] the beginning of the signed push

Bert Wesarg <bert.wesarg@googlemail.com> writes:
Show 15 quoted lines
> On Wed, Aug 20, 2014 at 12:06 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> The basic flow based on this mechanism goes like this:
>>
>>  1. You push out your work with "git push -s".
>>
>>  2. The sending side learns where the remote refs are as usual,
>>     together with what protocol extension the receiving end
>>     supports.  If the receiving end does not advertise the protocol
>>     extension "push-cert", the sending side falls back to the normal
>>     push that is not signed.
>>
>
> Is this fallback silently? If so I think it would be better to abort
> the push, if the receiver does not support this but the user requested
> it.

Let me change it in the reroll. I however am not quite sure if warning is insufficient, because there is nothing, other than rerunning the command without "--signed", that the user could do when it happens.

Junio C Hamano· Aug 19, 2014, 22:06 UTC · re: Junio C Hamano · lore

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

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 <gitster@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(-)
Show changes to 3 files +63 −6

Documentation/git-receive-pack.txt, builtin/receive-pack.c, t/t5534-push-signed.sh

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 (!is_null_sha1(push_cert_sha1)) {
 		argv_array_pushf(&env, "GIT_PUSH_CERT=%s", sha1_to_hex(push_cert_sha1));
+		argv_array_pushf(&env, "GIT_PUSH_CERT_SIGNER=%s", sigcheck.signer);
+		argv_array_pushf(&env, "GIT_PUSH_CERT_KEY=%s", sigcheck.key);
+		argv_array_pushf(&env, "GIT_PUSH_CERT_STATUS=%c", sigcheck.result);
+
 		proc->env = env.argv;
 	}
 }
diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index 0eb5004..659bca0 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -52,12 +52,26 @@ test_expect_success GPG 'signed push sends push certificate' '
 	if test -n "${GIT_PUSH_CERT-}"
 	then
 		git cat-file blob $GIT_PUSH_CERT >../push-cert
-	fi
+	fi &&
+
+	cat >../push-cert-status <<E_O_F
+	SIGNER=${GIT_PUSH_CERT_SIGNER-nobody}
+	KEY=${GIT_PUSH_CERT_KEY-nokey}
+	STATUS=${GIT_PUSH_CERT_STATUS-nostatus}
+	E_O_F
+
+	EOF
+
+	cat >expect <<-\EOF &&
+	SIGNER=C O Mitter <committer@example.com>
+	KEY=13B6F51ECDDE430D
+	STATUS=G
 	EOF
 
 	git push --signed dst noop ff +noff &&
 	grep "$(git rev-parse noop ff) refs/heads/ff" dst/push-cert &&
-	grep "$(git rev-parse noop noff) refs/heads/noff" dst/push-cert
+	grep "$(git rev-parse noop noff) refs/heads/noff" dst/push-cert &&
+	test_cmp expect dst/push-cert-status
 '
 
 test_done
-- 
2.1.0-301-g54593e2
David Turner· Aug 20, 2014, 16:56 UTC · re: Junio C Hamano · lore

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

On Tue, 2014-08-19 at 15:06 -0700, Junio C Hamano wrote:
Show 7 quoted lines
> 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.

Junio C Hamano· Aug 20, 2014, 17:29 UTC · re: David Turner · lore

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

On Wed, Aug 20, 2014 at 9:56 AM, David Turner <dturner@twopensource.com> wrote:
Show 11 quoted lines
> 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?
David Turner· Aug 20, 2014, 17:56 UTC · re: Junio C Hamano · lore

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

On Wed, 2014-08-20 at 10:29 -0700, Junio C Hamano wrote:
Show 16 quoted lines
> On Wed, Aug 20, 2014 at 9:56 AM, David Turner <dturner@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.

Junio C Hamano· Aug 20, 2014, 19:38 UTC · re: David Turner · lore

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

David Turner <dturner@twopensource.com> writes:
Show 29 quoted lines
> On Wed, 2014-08-20 at 10:29 -0700, Junio C Hamano wrote:
>> On Wed, Aug 20, 2014 at 9:56 AM, David Turner <dturner@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.

David Turner· Aug 21, 2014, 23:59 UTC · re: Junio C Hamano · lore

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

On Wed, 2014-08-20 at 12:38 -0700, Junio C Hamano wrote:
Show 47 quoted lines
> David Turner <dturner@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 <dturner@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.

Junio C Hamano· Aug 22, 2014, 00:11 UTC · re: David Turner · lore

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

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 <dturner@twopensource.com> wrote:
Show 55 quoted lines
> On Wed, 2014-08-20 at 12:38 -0700, Junio C Hamano wrote:
>> David Turner <dturner@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 <dturner@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.
>
Junio C Hamano· Aug 19, 2014, 22:06 UTC · re: Junio C Hamano · lore

[PATCH 17/18] send-pack: send feature request on push-cert packet

We would want to update the interim protocol so that we do not send the usual update commands when the push certificate feature is in use, as the same information is in the certificate. Once that happens, the push-cert packet may become the only protocol command, but then there is no packet to put the feature request behind, like we always did.

As we have prepared the receiving end that understands the push-cert feature to accept the feature request on the first protocol packet (other than "shallow ", which was an unfortunate historical mistake that has to come before everything else), we can give the feature request on the push-cert packet instead of the first update protocol packet, in preparation for the next step to actually update to the final protocol.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 send-pack.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)
Show changes to send-pack.c +8 −5
diff --git a/send-pack.c b/send-pack.c
index a073891..2de9826 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -225,9 +225,10 @@ static const char *next_line(const char *line, size_t len)
 	return nl + 1;
 }
 
-static void generate_push_cert(struct strbuf *req_buf,
-			       const struct ref *remote_refs,
-			       struct send_pack_args *args)
+static int generate_push_cert(struct strbuf *req_buf,
+			      const struct ref *remote_refs,
+			      struct send_pack_args *args,
+			      const char *cap_string)
 {
 	const struct ref *ref;
 	char stamp[60];
@@ -256,7 +257,7 @@ static void generate_push_cert(struct strbuf *req_buf,
 	if (sign_buffer(&cert, &cert, signing_key))
 		die(_("failed to sign the push certificate"));
 
-	packet_buf_write(req_buf, "push-cert\n");
+	packet_buf_write(req_buf, "push-cert%c%s", 0, cap_string);
 	for (cp = cert.buf; cp < cert.buf + cert.len; cp = np) {
 		np = next_line(cp, cert.buf + cert.len - cp);
 		packet_buf_write(req_buf,
@@ -267,6 +268,7 @@ static void generate_push_cert(struct strbuf *req_buf,
 free_return:
 	free(signing_key);
 	strbuf_release(&cert);
+	return 1;
 }
 
 int send_pack(struct send_pack_args *args,
@@ -337,7 +339,8 @@ int send_pack(struct send_pack_args *args,
 		advertise_shallow_grafts_buf(&req_buf);
 
 	if (!args->dry_run && args->push_cert)
-		generate_push_cert(&req_buf, remote_refs, args);
+		cmds_sent = generate_push_cert(&req_buf, remote_refs, args,
+					       cap_buf.buf);
 
 	/*
 	 * Clear the status for each ref and see if we need to send
-- 
2.1.0-301-g54593e2
Junio C Hamano· Aug 19, 2014, 22:06 UTC · re: Junio C Hamano · lore

[PATCH 18/18] signed push: final protocol update

With the interim protocol, we used to send the update commands even though we already send a signed copy of the same information when push certificate is in use. Update the send-pack/receive-pack pair not to do so.

The notable thing on the receive-pack side is that it makes sure that there is no command sent over the traditional protocol packet outside the push certificate. Otherwise a pusher can claim to be pushing one set of ref updates in the signed certificate while issuing commands to update unrelated refs, and such an update will evade later audits.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/technical/pack-protocol.txt         | 20 ++++++++++++++++-
 Documentation/technical/protocol-capabilities.txt | 12 +++++++++--
 builtin/receive-pack.c                            | 26 +++++++++++++++++++++++
 send-pack.c                                       |  2 +-
 4 files changed, 56 insertions(+), 4 deletions(-)
Show changes to 4 files +56 −4

Documentation/technical/pack-protocol.txt, Documentation/technical/protocol-capabilities.txt, builtin/receive-pack.c, send-pack.c

diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
index a845d51..f6c3073 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -465,7 +465,7 @@ contain all the objects that the server will need to complete the new
 references.
 
 ----
-  update-request    =  *shallow command-list [pack-file]
+  update-request    =  *shallow ( command-list | push-cert ) [pack-file]
 
   shallow           =  PKT-LINE("shallow" SP obj-id)
 
@@ -481,12 +481,30 @@ references.
   old-id            =  obj-id
   new-id            =  obj-id
 
+  push-cert         = PKT-LINE("push-cert" NUL capability-list LF)
+		      PKT-LINE("certificate version 0.1" LF)
+		      PKT-LINE("pusher" ident LF)
+		      PKT-LINE(LF)
+		      *PKT-LINE(command LF)
+		      *PKT-LINE(GPG signature lines LF)
+		      PKT-LINE("push-cert-end" LF)
+
   pack-file         = "PACK" 28*(OCTET)
 ----
 
 If the receiving end does not support delete-refs, the sending end MUST
 NOT ask for delete command.
 
+If the receiving end does not support push-cert, the sending end MUST
+NOT send a push-cert command.
+
+When a push-cert command is sent, command-list MUST NOT be sent; the
+commands recorded in the push certificate is used instead.  The GPG
+signature lines are detached signature for the contents recorded in
+the push certificate before the signature block begins and is used
+to certify that the commands were given by the pusher which must be
+the signer.
+
 The pack-file MUST NOT be sent if the only command used is 'delete'.
 
 A pack-file MUST be sent if either create or update command is used,
diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
index e174343..5b398e9 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -18,8 +18,8 @@ was sent.  Server MUST NOT ignore capabilities that client requested
 and server advertised.  As a consequence of these rules, server MUST
 NOT advertise capabilities it does not understand.
 
-The 'report-status', 'delete-refs', and 'quiet' capabilities are sent and
-recognized by the receive-pack (push to server) process.
+The 'report-status', 'delete-refs', 'quiet', and 'push-cert' capabilities
+are sent and recognized by the receive-pack (push to server) process.
 
 The 'ofs-delta' and 'side-band-64k' capabilities are sent and recognized
 by both upload-pack and receive-pack protocols.  The 'agent' capability
@@ -250,3 +250,11 @@ allow-tip-sha1-in-want
 If the upload-pack server advertises this capability, fetch-pack may
 send "want" lines with SHA-1s that exist at the server but are not
 advertised by upload-pack.
+
+push-cert
+---------
+
+The receive-pack server that advertises this capability is willing
+to accept a signed push certificate.  A send-pack client MUST NOT
+send push-cert packet unless the receive-pack server advertises this
+capability.
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index abdc296..96e4c99 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -907,6 +907,28 @@ static struct command **queue_command(struct command **p,
 	return &cmd->next;
 }
 
+static void queue_commands_from_cert(struct command **p,
+				     struct strbuf *push_cert)
+{
+	const char *boc, *eoc;
+
+	if (*p)
+		die("protocol error: got both push certificate and unsigned commands");
+
+	boc = strstr(push_cert->buf, "\n\n");
+	if (!boc)
+		die("malformed push certificate %.*s", 100, push_cert->buf);
+	else
+		boc += 2;
+	eoc = push_cert->buf + parse_signature(push_cert->buf, push_cert->len);
+
+	while (boc < eoc) {
+		const char *eol = memchr(boc, '\n', eoc - boc);
+		p = queue_command(p, boc, eol ? eol - boc : eoc - eol);
+		boc = eol ? eol + 1 : eoc;
+	}
+}
+
 static struct command *read_head_info(struct sha1_array *shallow)
 {
 	struct command *commands = NULL;
@@ -962,6 +984,10 @@ static struct command *read_head_info(struct sha1_array *shallow)
 
 		p = queue_command(p, line, linelen);
 	}
+
+	if (push_cert.len)
+		queue_commands_from_cert(p, &push_cert);
+
 	return commands;
 }
 
diff --git a/send-pack.c b/send-pack.c
index 2de9826..ca2ae2b 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -365,7 +365,7 @@ int send_pack(struct send_pack_args *args,
 	for (ref = remote_refs; ref; ref = ref->next) {
 		char *old_hex, *new_hex;
 
-		if (args->dry_run)
+		if (args->dry_run || args->push_cert)
 			continue;
 
 		if (!ref_update_to_be_sent(ref, args))
-- 
2.1.0-301-g54593e2
Shawn Pearce· Aug 21, 2014, 19:28 UTC · re: Junio C Hamano · lore

Re: [PATCH 18/18] signed push: final protocol update

On Tue, Aug 19, 2014 at 3:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> +  push-cert         = PKT-LINE("push-cert" NUL capability-list LF)
Haha. NUL.  I love our wire protocol.
Show 5 quoted lines
> +                     PKT-LINE("certificate version 0.1" LF)
> +                     PKT-LINE("pusher" ident LF)
> +                     PKT-LINE(LF)
> +                     *PKT-LINE(command LF)
> +                     *PKT-LINE(GPG signature lines LF)
Should we include the URL as part of this certificate?

Perhaps the pusher means to sign the master branch of experimental tree, but not their trunk tree?

Junio C Hamano· Aug 21, 2014, 23:40 UTC · re: Shawn Pearce · lore

Re: [PATCH 18/18] signed push: final protocol update

Shawn Pearce <spearce@spearce.org> writes:
Show 16 quoted lines
> On Tue, Aug 19, 2014 at 3:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> +  push-cert         = PKT-LINE("push-cert" NUL capability-list LF)
>
> Haha. NUL.  I love our wire protocol.
>
>> +                     PKT-LINE("certificate version 0.1" LF)
>> +                     PKT-LINE("pusher" ident LF)
>> +                     PKT-LINE(LF)
>> +                     *PKT-LINE(command LF)
>> +                     *PKT-LINE(GPG signature lines LF)
>
> Should we include the URL as part of this certificate?
>
> Perhaps the pusher means to sign the master branch of experimental
> tree, but not their trunk tree?

Yes, in $gmane/255582 I cover this and also mention that we would need some "nonce" from the receiving end to make it harder to replay.

Currently I am leaning toward to add both "pushed-to <URL>" and also "nonce <nonce>", the latter of which the receiver can ask with "push-cert=<nonce>" in its initial capability advertisement.

There are a few gotchas I can certainly use help on, especially from a smart-http expert ;-).

 * "pushed-to <URL>" will identify the site and the repository, so
   you cannot MITM my push to an experimental server and replay it
   against the authoritative server.
   However, the receiving end may not even know what name its users
   call the repository being pushed into.  Obviously gethostname()
   may not be what the pusher called us, and getcwd() may not match
   the repository name without leading "/var/repos/shard3/" path
   components stripped, for example.
   I am not sure if we even have the necessary information at
   send-pack.c::send_pack() level, where it already has an
   established connection to the server (hence it does not need to
   know to whom it is talking to).
 * The receiving end will issue "push-cert=<nonce>" in its initial
   capability advertisement, and this <nonce> will be given on the
   PUSH_CERT_NONCE environment to the pre/post-receive hooks, to
   allow the "nonce <nonce>" header in the signed certificate to be
   checked against it.  You cannot capture my an earlier push to the
   authoritative server and replay it later.
   That would all work well within a single receive-pack process,
   but with "stateless" RPC, it is unclear to me how we should
   arrange the <nonce> the initial instance of receive-pack placed
   on its capability advertisement to be securely passed to the
   instance of receive-pack that actually receives the push
   certificate.
Kyle J. McKay· Aug 22, 2014, 03:06 UTC · re: Junio C Hamano · lore

Re: [PATCH 18/18] signed push: final protocol update

On Aug 21, 2014, at 16:40, Junio C Hamano wrote:
Show 13 quoted lines
> * The receiving end will issue "push-cert=<nonce>" in its initial
>   capability advertisement, and this <nonce> will be given on the
>   PUSH_CERT_NONCE environment to the pre/post-receive hooks, to
>   allow the "nonce <nonce>" header in the signed certificate to be
>   checked against it.  You cannot capture my an earlier push to the
>   authoritative server and replay it later.
>
>   That would all work well within a single receive-pack process,
>   but with "stateless" RPC, it is unclear to me how we should
>   arrange the <nonce> the initial instance of receive-pack placed
>   on its capability advertisement to be securely passed to the
>   instance of receive-pack that actually receives the push
>   certificate.

Have you considered having the advertised nonce only be updated after receipt of a successful signed push?

It would eliminate the stateless issue. And since the next nonce to be advertised would be updated at the successful completion of a receive of a signed push no replay would be possible. (I'm assuming that receive hook activity is already pipelined in the case of simultaneous pushes via some lock file or something or this scheme falls apart.)

The obvious downside is that only one of two or more simultaneous signed pushers could succeed. But the sender could be modified to automatically retry (a limited number of times) on a nonce mismatch error.

A receive hook could also be responsible for generating the next nonce value using this technique.

Junio C Hamano· Aug 22, 2014, 17:59 UTC · re: Junio C Hamano · lore

Re: [PATCH 18/18] signed push: final protocol update

Junio C Hamano <gitster@pobox.com> writes:
Show 32 quoted lines
> There are a few gotchas I can certainly use help on, especially from
> a smart-http expert ;-).
>
>  * "pushed-to <URL>" will identify the site and the repository, so
>    you cannot MITM my push to an experimental server and replay it
>    against the authoritative server.
>
>    However, the receiving end may not even know what name its users
>    call the repository being pushed into.  Obviously gethostname()
>    may not be what the pusher called us, and getcwd() may not match
>    the repository name without leading "/var/repos/shard3/" path
>    components stripped, for example.
>
>    I am not sure if we even have the necessary information at
>    send-pack.c::send_pack() level, where it already has an
>    established connection to the server (hence it does not need to
>    know to whom it is talking to).
>
>
>  * The receiving end will issue "push-cert=<nonce>" in its initial
>    capability advertisement, and this <nonce> will be given on the
>    PUSH_CERT_NONCE environment to the pre/post-receive hooks, to
>    allow the "nonce <nonce>" header in the signed certificate to be
>    checked against it.  You cannot capture my an earlier push to the
>    authoritative server and replay it later.
>
>    That would all work well within a single receive-pack process,
>    but with "stateless" RPC, it is unclear to me how we should
>    arrange the <nonce> the initial instance of receive-pack placed
>    on its capability advertisement to be securely passed to the
>    instance of receive-pack that actually receives the push
>    certificate.

A good <nonce> may be something like taking the SHA-1 hash of the concatenation of the sitename, repo-path and the timestamp when the receive-pack generated the <nonce>. Replaying a push certificate for a push to a repository at a site that gives such a <nonce> can succeed at the same chance of finding a SHA-1 collision [*1*]. As long as you exercise good hygiene and only push to repositories that give such <nonce>, we can do without checking "pushed-to" that says where the push went.

So "nonce <nonce>" is the only thing that is necessary to make them impossible to replay. For auditing purposes, "pushed-to <URL>" that records the repository the pusher intended to push to may help but probably not necessary [*2*].

[Footnote]
*1* And the old-sha1s recorded in the certificate has to match what
    the repository being attacked currently has; otherwise the push
    will fail with "the ref moved while you were trying to push".
*2* When auditing the history for a repository at a site, the
    certificate the auditors examine would be the ones accumulated
    at that site for the repository, so we would implicitly know the
    value for <URL> already.
Shawn Pearce· Aug 22, 2014, 23:54 UTC · re: Junio C Hamano · lore

Re: [PATCH 18/18] signed push: final protocol update

On Fri, Aug 22, 2014 at 10:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 43 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> There are a few gotchas I can certainly use help on, especially from
>> a smart-http expert ;-).
>>
>>  * "pushed-to <URL>" will identify the site and the repository, so
>>    you cannot MITM my push to an experimental server and replay it
>>    against the authoritative server.
>>
>>    However, the receiving end may not even know what name its users
>>    call the repository being pushed into.  Obviously gethostname()
>>    may not be what the pusher called us, and getcwd() may not match
>>    the repository name without leading "/var/repos/shard3/" path
>>    components stripped, for example.
>>
>>    I am not sure if we even have the necessary information at
>>    send-pack.c::send_pack() level, where it already has an
>>    established connection to the server (hence it does not need to
>>    know to whom it is talking to).
>>
>>
>>  * The receiving end will issue "push-cert=<nonce>" in its initial
>>    capability advertisement, and this <nonce> will be given on the
>>    PUSH_CERT_NONCE environment to the pre/post-receive hooks, to
>>    allow the "nonce <nonce>" header in the signed certificate to be
>>    checked against it.  You cannot capture my an earlier push to the
>>    authoritative server and replay it later.
>>
>>    That would all work well within a single receive-pack process,
>>    but with "stateless" RPC, it is unclear to me how we should
>>    arrange the <nonce> the initial instance of receive-pack placed
>>    on its capability advertisement to be securely passed to the
>>    instance of receive-pack that actually receives the push
>>    certificate.
>
> A good <nonce> may be something like taking the SHA-1 hash of the
> concatenation of the sitename, repo-path and the timestamp when the
> receive-pack generated the <nonce>.  Replaying a push certificate
> for a push to a repository at a site that gives such a <nonce> can
> succeed at the same chance of finding a SHA-1 collision [*1*].  As
> long as you exercise good hygiene and only push to repositories that
> give such <nonce>, we can do without checking "pushed-to" that says
> where the push went.
Yes, this is an interesting solution.

As you know, the stateless HTTP thing doesn't allow the nonce on the server to be carried from the initial ref advertisement into the final receive-pack. We would either need to write the nonce to disk and load it back up later (ick), or use some sort of stateless nonce.

A stateless nonce could look like:
  nonce = HMAC_SHA1( SHA1(site+path) + '.' + now, site_key )

where site_key is a private key known to the server. It doesn't have to be per-repo.

receive-pack would then be willing to accept any nonce whose timestamp is within a window, e.g. 10 minutes of the current time, and whose signature verifies in the HMAC. The 10 minute window is important to allow clients time to generate the object list, perform delta compression, and begin transmitting to the server.

> So "nonce <nonce>" is the only thing that is necessary to make them
> impossible to replay.  For auditing purposes, "pushed-to <URL>" that
> records the repository the pusher intended to push to may help but
> probably not necessary [*2*].

So pushed-to is still useful as a documentation/historical artifact, but isn't important for verifying the certificate. That fixes a lot of problems with repositories having different paths under e.g. git:// vs. ssh:// vs. https:// on the same host.

Junio C Hamano· Aug 25, 2014, 17:59 UTC · re: Shawn Pearce · lore

Re: [PATCH 18/18] signed push: final protocol update

Shawn Pearce <spearce@spearce.org> writes:
Show 12 quoted lines
> A stateless nonce could look like:
>
>   nonce = HMAC_SHA1( SHA1(site+path) + '.' + now, site_key )
>
> where site_key is a private key known to the server. It doesn't have
> to be per-repo.
>
> receive-pack would then be willing to accept any nonce whose timestamp
> is within a window, e.g. 10 minutes of the current time, and whose
> signature verifies in the HMAC. The 10 minute window is important to
> allow clients time to generate the object list, perform delta
> compression, and begin transmitting to the server.

Hmph, don't you send the "finally tell the other end" the sequence of "update this ref from old to new" and the packdata separately? I think we have a FLUSH in between, and the push certificate is given before the FLUSH, which you do not have to wait for 10 minutes.

Shawn Pearce· Aug 26, 2014, 17:33 UTC · re: Junio C Hamano · lore

Re: [PATCH 18/18] signed push: final protocol update

On Mon, Aug 25, 2014 at 10:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 17 quoted lines
> Shawn Pearce <spearce@spearce.org> writes:
>
>> A stateless nonce could look like:
>>
>>   nonce = HMAC_SHA1( SHA1(site+path) + '.' + now, site_key )
>>
>> where site_key is a private key known to the server. It doesn't have
>> to be per-repo.
>>
>> receive-pack would then be willing to accept any nonce whose timestamp
>> is within a window, e.g. 10 minutes of the current time, and whose
>> signature verifies in the HMAC. The 10 minute window is important to
>> allow clients time to generate the object list, perform delta
>> compression, and begin transmitting to the server.
>
> Hmph, don't you send the "finally tell the other end" the sequence
> of "update this ref from old to new" and the packdata separately?

No. The command list (triples of old, new, ref) is sent in the same HTTP request as the pack data, ahead of the pack data. So its one request.

Push on smart HTTP is 3 HTTP requests:
  1)  get advertisement
  2)  POST empty flush packet to tickle auth (literally just "0000").
  3)  POST command list + pack
The nonce can be sent server->client in 1, and client->server in 3.
>  I
> think we have a FLUSH in between, and the push certificate is given
> before the FLUSH, which you do not have to wait for 10 minutes.

Nope I think you need to wait for the pack to generate enough to start sending the pack data stream. Nothing forces the smart HTTP client to push its pending buffer out. We wait for the pack data to either finish, or overflow the in-memory buffer, and then start transmitting. If your client needs a lot of time for counting and delta compression, we aren't likely to overflow and transmit for a while.

If you send a _lot_ of refs you can overflow, which will cause us to transmit early. But we are talking about megabytes worth of (old, new, ref) triplets to reach that overflow point.

Junio C Hamano· Aug 26, 2014, 19:38 UTC · re: Shawn Pearce · lore

Re: [PATCH 18/18] signed push: final protocol update

Shawn Pearce <spearce@spearce.org> writes:
Show 22 quoted lines
> On Mon, Aug 25, 2014 at 10:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Shawn Pearce <spearce@spearce.org> writes:
>>
>>> A stateless nonce could look like:
>>>
>>>   nonce = HMAC_SHA1( SHA1(site+path) + '.' + now, site_key )
>>>
>>> where site_key is a private key known to the server. It doesn't have
>>> to be per-repo.
>>>
>>> receive-pack would then be willing to accept any nonce whose timestamp
>>> is within a window, e.g. 10 minutes of the current time, and whose
>>> signature verifies in the HMAC. The 10 minute window is important to
>>> allow clients time to generate the object list, perform delta
>>> compression, and begin transmitting to the server.
>>
>> Hmph, don't you send the "finally tell the other end" the sequence
>> of "update this ref from old to new" and the packdata separately?
>
> No. The command list (triples of old, new, ref) is sent in the same
> HTTP request as the pack data, ahead of the pack data. So its one
> request.

That is unfortunate. Would it be a major surgery to update the protocol not to do that, perhaps by moving the command list from 3 to 2 (the latter of which is not currently doing anything useful payload-wise, other than flushing a HTTP request early)?

Show 22 quoted lines
> Push on smart HTTP is 3 HTTP requests:
>
>   1)  get advertisement
>   2)  POST empty flush packet to tickle auth (literally just "0000").
>   3)  POST command list + pack
>
> The nonce can be sent server->client in 1, and client->server in 3.
>
>>  I
>> think we have a FLUSH in between, and the push certificate is given
>> before the FLUSH, which you do not have to wait for 10 minutes.
>
> Nope I think you need to wait for the pack to generate enough to start
> sending the pack data stream. Nothing forces the smart HTTP client to
> push its pending buffer out. We wait for the pack data to either
> finish, or overflow the in-memory buffer, and then start transmitting.
> If your client needs a lot of time for counting and delta compression,
> we aren't likely to overflow and transmit for a while.
>
> If you send a _lot_ of refs you can overflow, which will cause us to
> transmit early. But we are talking about megabytes worth of (old, new,
> ref) triplets to reach that overflow point.
Junio C Hamano· Aug 26, 2014, 19:52 UTC · re: Junio C Hamano · lore

Re: [PATCH 18/18] signed push: final protocol update

Junio C Hamano <gitster@pobox.com> writes:
> That is unfortunate.  Would it be a major surgery to update the
> protocol not to do that, perhaps by moving the command list from 3
> to 2 (the latter of which is not currently doing anything useful
> payload-wise, other than flushing a HTTP request early)?

Nah, that was one of the most stupid thing I ever said here X. There is nothing that ties #2 and #3 unless the server side keeps some state, so that would not work very well X-<.

Show 6 quoted lines
>
>> Push on smart HTTP is 3 HTTP requests:
>>
>>   1)  get advertisement
>>   2)  POST empty flush packet to tickle auth (literally just "0000").
>>   3)  POST command list + pack
Junio C Hamano· Sep 4, 2014, 23:57 UTC · re: Shawn Pearce · lore

Re: [PATCH 18/18] signed push: final protocol update

Shawn Pearce <spearce@spearce.org> writes:
Show 11 quoted lines
> As you know, the stateless HTTP thing doesn't allow the nonce on the
> server to be carried from the initial ref advertisement into the final
> receive-pack. We would either need to write the nonce to disk and load
> it back up later (ick), or use some sort of stateless nonce.
>
> A stateless nonce could look like:
>
>   nonce = HMAC_SHA1( SHA1(site+path) + '.' + now, site_key )
>
> where site_key is a private key known to the server. It doesn't have
> to be per-repo.

Doing the above naively will force you to check 600 HMAC if your slack is for 10 minutes. You could just instead use

	nonce = now '-' HMAC_SHA1(path + '.' + now, site_key)

and the validation side can make sure the same site_key was used, and also "now" readable from the plaintext part is fresh enough, with a single HMAC.

I may be missing something, but with this, we can always validate that "nonce" is what the repository issued (whether "stateless" is used or not). The hook script can decide if "now" is recent enough or not without bothering receive-pack at all.

Shawn Pearce· Sep 5, 2014, 02:41 UTC · re: Junio C Hamano · lore

Re: [PATCH 18/18] signed push: final protocol update

On Thu, Sep 4, 2014 at 4:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 22 quoted lines
> Shawn Pearce <spearce@spearce.org> writes:
>
>> As you know, the stateless HTTP thing doesn't allow the nonce on the
>> server to be carried from the initial ref advertisement into the final
>> receive-pack. We would either need to write the nonce to disk and load
>> it back up later (ick), or use some sort of stateless nonce.
>>
>> A stateless nonce could look like:
>>
>>   nonce = HMAC_SHA1( SHA1(site+path) + '.' + now, site_key )
>>
>> where site_key is a private key known to the server. It doesn't have
>> to be per-repo.
>
> Doing the above naively will force you to check 600 HMAC if your
> slack is for 10 minutes.  You could just instead use
>
>         nonce = now '-' HMAC_SHA1(path + '.' + now, site_key)
>
> and the validation side can make sure the same site_key was used,
> and also "now" readable from the plaintext part is fresh enough,
> with a single HMAC.
Argh. Yes, thank you. This is what I meant but did not write. :(
> I may be missing something, but with this, we can always validate
> that "nonce" is what the repository issued (whether "stateless" is
> used or not).  The hook script can decide if "now" is recent enough
> or not without bothering receive-pack at all.
Correct.
Junio C Hamano· Aug 22, 2014, 04:20 UTC · re: Shawn Pearce · lore

Re: [PATCH 18/18] signed push: final protocol update

On Thu, Aug 21, 2014 at 12:28 PM, Shawn Pearce <spearce@spearce.org> wrote:
Show 5 quoted lines
> On Tue, Aug 19, 2014 at 3:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> +  push-cert         = PKT-LINE("push-cert" NUL capability-list LF)
>
> Haha. NUL.  I love our wire protocol.
It is a direct and natural consequence of [PATCH 02/18].

We could use SP here, if we really wanted to, but that would make the push-cert packet a special kind that is different from others, which we would want to avoid. "shallow" is already special in that it cannot even carry the feature request, and it is not worth introducing and advertising a new capability to fix it, but at least we can avoid making the same mistake here.

David Turner· Aug 22, 2014, 00:22 UTC · re: Junio C Hamano · lore

Re: [PATCH 18/18] signed push: final protocol update

On Tue, 2014-08-19 at 15:06 -0700, Junio C Hamano wrote:
Show 7 quoted lines
>  
> +If the receiving end does not support push-cert, the sending end MUST
> +NOT send a push-cert command.
> +
> +When a push-cert command is sent, command-list MUST NOT be sent; the
> +commands recorded in the push certificate is used instead.  The GPG
> +signature lines are detached signature for the contents recorded in
"are a detached signature"
> +the push certificate before the signature block begins and is used
"which is used" (or "and are used")
> +to certify that the commands were given by the pusher which must be
", who must be"
Show 6 quoted lines
> +the signer.
> +
> +
> +The receive-pack server that advertises this capability is willing
> +to accept a signed push certificate.  A send-pack client MUST NOT
> +send push-cert packet unless the receive-pack server advertises this
"packets" (or "a push-cert packet")
>  
> +static void queue_commands_from_cert(struct command **p,
Uninformative parameter name p.
Duy Nguyen· Aug 19, 2014, 23:07 UTC · re: Junio C Hamano · lore

Re: [PATCH 00/18] Signed push

On Wed, Aug 20, 2014 at 5:06 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> While signed tags and commits assert that the objects thusly signed
> came from you, who signed these objects, there is not a good way to
> assert that you wanted to have a particular object at the tip of a
> particular branch.  My signing v2.0.1 tag only means I want to call
> the version v2.0.1, and it does not mean I want to push it out to my
> 'master' branch---it is likely that I only want it in 'maint', so
> the signature on the object alone is insufficient.
>
> The only assurance to you that 'maint' points at what I wanted to
> place there comes from your trust on the hosting site and my
> authentication with it, which cannot easily audited later.

I only had a quick read of a few important patches and may miss something. But all this audit recording is left to the hook, right? I suppose git-notes could be used to store the push cert. blob, or the server could make a signed tag to record this info in the ref.. or do you intend any other way to record these blobs?

-- 
Duy
Junio C Hamano· Aug 19, 2014, 23:29 UTC · re: Duy Nguyen · lore

Re: [PATCH 00/18] Signed push

I don't think anybody can come up with a good auditing and recording scheme without sufficient experience. Because I want to avoid whatever random scheme we happen to implement first in git-core, even if it is way suboptimal, ends up as the de-facto standard, I deliberately stayed away from adding one of my own in this initial series, whose purpose is to lay a groundwork for server operators can build on. For the same reason, I tried to avoid anything that defines and forces a particular policy (e.g. does a server that accepts push certificate accept an unsigned push to some of its refs? Is it OK if the GPG key is slightly stale? etc.).

"git notes" may or may not be a good vehicle to store them. This series deliberately refrains from making that judgement. I'd rather leave these operational issues to folks who work on systems like Gitolite and Gerrit.

On Tue, Aug 19, 2014 at 4:07 PM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 20 quoted lines
> On Wed, Aug 20, 2014 at 5:06 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> While signed tags and commits assert that the objects thusly signed
>> came from you, who signed these objects, there is not a good way to
>> assert that you wanted to have a particular object at the tip of a
>> particular branch.  My signing v2.0.1 tag only means I want to call
>> the version v2.0.1, and it does not mean I want to push it out to my
>> 'master' branch---it is likely that I only want it in 'maint', so
>> the signature on the object alone is insufficient.
>>
>> The only assurance to you that 'maint' points at what I wanted to
>> place there comes from your trust on the hosting site and my
>> authentication with it, which cannot easily audited later.
>
> I only had a quick read of a few important patches and may miss
> something. But all this audit recording is left to the hook, right? I
> suppose git-notes could be used to store the push cert. blob, or the
> server could make a signed tag to record this info in the ref.. or do
> you intend any other way to record these blobs?
> --
> Duy
Nico Williams· Aug 20, 2014, 01:19 UTC · re: Junio C Hamano · lore

Re: [PATCH 00/18] Signed push

On Tue, Aug 19, 2014 at 03:06:09PM -0700, Junio C Hamano wrote:
Show 15 quoted lines
> While signed tags and commits assert that the objects thusly signed
> came from you, who signed these objects, there is not a good way to
> assert that you wanted to have a particular object at the tip of a
> particular branch.  My signing v2.0.1 tag only means I want to call
> the version v2.0.1, and it does not mean I want to push it out to my
> 'master' branch---it is likely that I only want it in 'maint', so
> the signature on the object alone is insufficient.
> 
> [...]
> 
> This series introduces a cryptographic assurance for ref updates
> done by "git push" by introducing a mechanism that allows you to
> sign a "push certificate" (for the lack of better name) every time
> you push.  Think of it as working on an axis orthogonal to the
> traditional "signed tags".

Sounds a lot like the "branch object" concept I suggest earlier, where each push would also push a commit to a branch object describing the updates to the branch, including signing of the updates to the branch (hey, it's just a signed commit), groups of commits pushed together / to be backed out together, rebase history, ... (What about pushing orphaned commits?)

Code-wise, would that be more or less generic?
Nico
Junio C Hamano· Aug 20, 2014, 02:54 UTC · re: Nico Williams · lore

Re: [PATCH 00/18] Signed push

Sorry, but I cannot answer, as the only thing that I recall when I hear "branch object" was that I heard the phrase used but without much substance.

I do not think I saw a clear explanation on what it is, how it is represented, how it is presented to the end user, how it is propagated across repositories (if it is to be propagated), how it is stored (if it is to be stored), how it is to be used, etc. and such crucial details necessary to judge the merit of thinking about introducing one. Perhaps it was explained in the old thread, but I don't recall, so I cannot judge how useful it would be to solve the problem the push certificate is trying to solve.

On Tue, Aug 19, 2014 at 6:19 PM, Nico Williams <nico@cryptonector.com> wrote:
Show 28 quoted lines
> On Tue, Aug 19, 2014 at 03:06:09PM -0700, Junio C Hamano wrote:
>> While signed tags and commits assert that the objects thusly signed
>> came from you, who signed these objects, there is not a good way to
>> assert that you wanted to have a particular object at the tip of a
>> particular branch.  My signing v2.0.1 tag only means I want to call
>> the version v2.0.1, and it does not mean I want to push it out to my
>> 'master' branch---it is likely that I only want it in 'maint', so
>> the signature on the object alone is insufficient.
>>
>> [...]
>>
>> This series introduces a cryptographic assurance for ref updates
>> done by "git push" by introducing a mechanism that allows you to
>> sign a "push certificate" (for the lack of better name) every time
>> you push.  Think of it as working on an axis orthogonal to the
>> traditional "signed tags".
>
> Sounds a lot like the "branch object" concept I suggest earlier, where
> each push would also push a commit to a branch object describing the
> updates to the branch, including signing of the updates to the branch
> (hey, it's just a signed commit), groups of commits pushed together / to
> be backed out together, rebase history, ...  (What about pushing
> orphaned commits?)
>
> Code-wise, would that be more or less generic?
>
> Nico
> --
Junio C Hamano· Aug 20, 2014, 05:57 UTC · re: Junio C Hamano · lore

Re: [PATCH 00/18] Signed push

On Tue, Aug 19, 2014 at 7:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Sorry, but I cannot answer, as the only thing that I recall when
> I hear "branch object" was that I heard the phrase used but
> without much substance.

Just to avoid unnecessary misunderstanding, by the above, especially the "without much substance" part, I do not mean that those without code have no say in the way the project improves its product. It is true that this project does not operate in such a way that visionaries throw ideas for coding minions to implement. A person with an itch and idea on how to scratch that itch is expected to lead the design and code, possibly with help from others. But in order to ask others evaluate and join force to help with the design and make it real with code, you need to present your idea to sufficient level of detail to be actionable.

By "actionable", consider the level of detail in which the proposed log message (not code or documentation update) of PATCH 15/18. Even though the message alone does not give any working code, or it does not even spell out the byte-level detail of how the protocol messages look like, people should be able to read enough details such as what kind of information a push certificate is to contain, when a certificate is created, how it is transferred, when and by whom it is received, how it will be used by the receiver, how a server operator can tweak his or her system to make use of the information, and how the newly added system component would fit into the existing system. In other words, the description should be sufficient to assess both how useful the end result would be, how much new work needs to be done to get there, and how well the resulting system as a whole would fit together well.

I went back to the old thread to re-read the mention of "branch object", but I did not get an impression that the idea was presented to the actionable level of detail; at least not yet.

Junio C Hamano· Aug 20, 2014, 02:39 UTC · re: Junio C Hamano · lore

Re: [PATCH 00/18] Signed push

On Tue, Aug 19, 2014 at 3:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
>
> If the server's GPG keychain and pre-receive hook are properly set
> up, a "git push --signed" over an unauthenticated and unencrypted
> communication channel (aka "git daemon") can be made as secure as,
> and even more secure than, the authenticated "git push ssh://".
>
> With the signed push certificate, together with the connectivity
> check done when the server accepts the packed data, we are assured
> that the trusted user vouches for the history leading to the
> proposed tips of refs (aka "new-sha1"s), and a man-in-the-middle
> would not be able to make the server accept an update altered in
> transit.

The above was written long after the series was done, but rethinking about it within that frame of mind helped me find one nit in the push certificate design.

We would need a PKT-LINE("pushed-to" SP URL LF) in the header part to prevent the certificate and the same set of refs from getting replayed. Otherwise, a different repository that happens to have the same set of refs with same set of old-sha1 values can be made to accept a push that the signer never intended to make to it by replaying an earlier push to a different site the signer did make.

Nico Williams· Aug 20, 2014, 06:28 UTC · re: Junio C Hamano · lore

Re: [PATCH 00/18] Signed push

No code == no substance might be a stretch, but definitely fair enough.

I thought the idea was clear enough, but I can flesh it out if desired. The particular advantage I saw in it is that it would reuse the existing object infrastructure, and extend to branches the first-class treatment that [signed] tags already get. I.e., generality. Other benefits include the ability to fetch and view a remote branch's object and its history (which would represent branch history in detail and with metadata that would otherwise not be available).

I'm not interested in pushing something different when you already have code that would achieve much of what I'd wanted. At this point my interest is in seeing if the architecture would be "purer" (in some sense) by reusing existing infrastructure.

Nico --

Stefan Beller· Aug 22, 2014, 19:59 UTC · re: Junio C Hamano · lore

Re: [PATCH 00/18] Signed push

On 20.08.2014 00:06, Junio C Hamano wrote:
Show 17 quoted lines
> While signed tags and commits assert that the objects thusly signed
> came from you, who signed these objects, there is not a good way to
> assert that you wanted to have a particular object at the tip of a
> particular branch.  My signing v2.0.1 tag only means I want to call
> the version v2.0.1, and it does not mean I want to push it out to my
> 'master' branch---it is likely that I only want it in 'maint', so
> the signature on the object alone is insufficient.
> 
> The only assurance to you that 'maint' points at what I wanted to
> place there comes from your trust on the hosting site and my
> authentication with it, which cannot easily audited later.
> 
> This series introduces a cryptographic assurance for ref updates
> done by "git push" by introducing a mechanism that allows you to
> sign a "push certificate" (for the lack of better name) every time
> you push.  Think of it as working on an axis orthogonal to the
> traditional "signed tags".

What kind of additional benefit do we gain? (i.e. why?) The described problem, the lacking auditability of what's pushed at which time, could be worked around like this:

Whenever you do a push, you just tag the latest commit in that branch.
So there would be tags like:
	master_2014_08_21
	master_2014_08_22
	...
	maint_2014_08_13
	maint_2014_08_21
and so on. Whenever there is no tag at the tip of the branch, we'd
know there is something wrong.
	
My guess would be usability as tagging so many branches is cumbersome
for a maintainer?

Looking at my made-up workaround again: That would produce lots of tags. So I could imagine there would also be lots of push certs. The number of push certs would roughly scale linear to time passed. May this result in slowness over time?

Does this patch series mean, we'd get another object type (additional to blobs, commits, tags, trees)?

I did not read the code yet, it's just first thoughts, so this weigh this input lightly.

Stefan
Junio C Hamano· Aug 22, 2014, 20:03 UTC · re: Stefan Beller · lore

Re: [PATCH 00/18] Signed push

Stefan Beller <stefanbeller@gmail.com> writes:
Show 8 quoted lines
> So there would be tags like:
> 	master_2014_08_21
> 	master_2014_08_22
> 	...
> 	maint_2014_08_13
> 	maint_2014_08_21
> and so on. Whenever there is no tag at the tip of the branch, we'd
> know there is something wrong.
Who creates that tag?
Stefan Beller· Aug 22, 2014, 20:22 UTC · re: Junio C Hamano · lore

Re: [PATCH 00/18] Signed push

On 22.08.2014 22:03, Junio C Hamano wrote:
Show 13 quoted lines
> Stefan Beller <stefanbeller@gmail.com> writes:
> 
>> So there would be tags like:
>> 	master_2014_08_21
>> 	master_2014_08_22
>> 	...
>> 	maint_2014_08_13
>> 	maint_2014_08_21
>> and so on. Whenever there is no tag at the tip of the branch, we'd
>> know there is something wrong.
> 
> Who creates that tag?
> 
> My guess would be usability as tagging so many branches is cumbersome
for a maintainer?
Junio C Hamano· Aug 22, 2014, 20:33 UTC · re: Stefan Beller · lore

Re: [PATCH 00/18] Signed push

Stefan Beller <stefanbeller@gmail.com> writes:
Show 17 quoted lines
> On 22.08.2014 22:03, Junio C Hamano wrote:
>> Stefan Beller <stefanbeller@gmail.com> writes:
>> 
>>> So there would be tags like:
>>> 	master_2014_08_21
>>> 	master_2014_08_22
>>> 	...
>>> 	maint_2014_08_13
>>> 	maint_2014_08_21
>>> and so on. Whenever there is no tag at the tip of the branch, we'd
>>> know there is something wrong.
>> 
>> Who creates that tag?
>> 
>
>> My guess would be usability as tagging so many branches is cumbersome
> for a maintainer?
Did you answer my question?  Who creates these tags?
Stefan Beller· Aug 22, 2014, 20:38 UTC · re: Junio C Hamano · lore

Re: [PATCH 00/18] Signed push

On 22.08.2014 22:33, Junio C Hamano wrote:
Show 22 quoted lines
> Stefan Beller <stefanbeller@gmail.com> writes:
> 
>> On 22.08.2014 22:03, Junio C Hamano wrote:
>>> Stefan Beller <stefanbeller@gmail.com> writes:
>>>
>>>> So there would be tags like:
>>>> 	master_2014_08_21
>>>> 	master_2014_08_22
>>>> 	...
>>>> 	maint_2014_08_13
>>>> 	maint_2014_08_21
>>>> and so on. Whenever there is no tag at the tip of the branch, we'd
>>>> know there is something wrong.
>>>
>>> Who creates that tag?
>>>
>>
>>> My guess would be usability as tagging so many branches is cumbersome
>> for a maintainer?
> 
> Did you answer my question?  Who creates these tags?
> 
It would be up to the one who pushes, the user, or in our case you!

This way of working would require the informal notion of 'always tag the last commit before pushing.'

As I wrote in the first email, I made up this workaround and wanted to see, what's so bad about that workaround and how to overcome the problems. And all I could find was a burden on the maintainer/user.

Sorry, Stefan

Junio C Hamano· Aug 22, 2014, 22:32 UTC · re: Stefan Beller · lore

Re: [PATCH 00/18] Signed push

Stefan Beller <stefanbeller@gmail.com> writes:
Show 29 quoted lines
> On 22.08.2014 22:33, Junio C Hamano wrote:
>> Stefan Beller <stefanbeller@gmail.com> writes:
>> 
>>> On 22.08.2014 22:03, Junio C Hamano wrote:
>>>> Stefan Beller <stefanbeller@gmail.com> writes:
>>>>
>>>>> So there would be tags like:
>>>>> 	master_2014_08_21
>>>>> 	master_2014_08_22
>>>>> 	...
>>>>> 	maint_2014_08_13
>>>>> 	maint_2014_08_21
>>>>> and so on. Whenever there is no tag at the tip of the branch, we'd
>>>>> know there is something wrong.
>>>>
>>>> Who creates that tag?
>>>>
>>>
>>>> My guess would be usability as tagging so many branches is cumbersome
>>> for a maintainer?
>> 
>> Did you answer my question?  Who creates these tags?
>> 
>
> It would be up to the one who pushes, the user, or in our case you!
> ...
> As I wrote in the first email, I made up this workaround and wanted to
> see, what's so bad about that workaround and how to overcome the
> problems. And all I could find was a burden on the maintainer/user.

"burden" is not an issue, as I'll be signing the push certificate anyway when I push. A signed tag or a signed commit and signed push certificate solves two completely separate and orthogonal issues.

What happens if you break into GitHub or k.org and did
    $ git tag maint_2014_08_22 master_2014_08_22

to create an extra tag out of the tag signed by me? If you want, you could also remove the original while at it. The goal is to let us validate without having to trust the hosting site, its management and its software, which is what creates the tag there, controls where the tag sits in refs/ hierarchy and how it is shown to the outside world.

Stefan Beller· Aug 22, 2014, 22:51 UTC · re: Junio C Hamano · lore

Re: [PATCH 00/18] Signed push

On 23.08.2014 00:32, Junio C Hamano wrote:
Show 39 quoted lines
> Stefan Beller <stefanbeller@gmail.com> writes:
> 
>> On 22.08.2014 22:33, Junio C Hamano wrote:
>>> Stefan Beller <stefanbeller@gmail.com> writes:
>>>
>>>> On 22.08.2014 22:03, Junio C Hamano wrote:
>>>>> Stefan Beller <stefanbeller@gmail.com> writes:
>>>>>
>>>>>> So there would be tags like:
>>>>>> 	master_2014_08_21
>>>>>> 	master_2014_08_22
>>>>>> 	...
>>>>>> 	maint_2014_08_13
>>>>>> 	maint_2014_08_21
>>>>>> and so on. Whenever there is no tag at the tip of the branch, we'd
>>>>>> know there is something wrong.
>>>>>
>>>>> Who creates that tag?
>>>>>
>>>>
>>>>> My guess would be usability as tagging so many branches is cumbersome
>>>> for a maintainer?
>>>
>>> Did you answer my question?  Who creates these tags?
>>>
>>
>> It would be up to the one who pushes, the user, or in our case you!
>> ...
>> As I wrote in the first email, I made up this workaround and wanted to
>> see, what's so bad about that workaround and how to overcome the
>> problems. And all I could find was a burden on the maintainer/user.
> 
> "burden" is not an issue, as I'll be signing the push certificate
> anyway when I push.  A signed tag or a signed commit and signed push
> certificate solves two completely separate and orthogonal issues.
> 
> What happens if you break into GitHub or k.org and did
> 
>     $ git tag maint_2014_08_22 master_2014_08_22
Ok, I personally haven't used tags a lot.
I just tried to
	git tag -s testbreaktag v2.1.0
	git show testbreaktag
	# However it would still read:
tag v2.1.0
Tagger: Junio C Hamano <gitster@pobox.com>
Date:   Fri Aug 15 15:09:28 2014 -0700

So as I do not posess your private key I could not create signed tags even if I were to break into github/k.org

> 
> to create an extra tag out of the tag signed by me?  If you want,
> you could also remove the original while at it. 

Considering I'm in the hosting server, could I delete the push cert as well? Now that I deleted the push certificate, I could pretend "Junio just forgot to sign the push cert today" and we're back at the tag solution?

Ah wait! the subsequent push certs would not match, I'd need to delete them as well.

Show 6 quoted lines
> The goal is to let
> us validate without having to trust the hosting site, its management
> and its software, which is what creates the tag there, controls
> where the tag sits in refs/ hierarchy and how it is shown to the
> outside world.
> 
Ok, I got the goal. :)

Thanks for your patience, Stefan

Junio C Hamano· Aug 25, 2014, 17:54 UTC · re: Stefan Beller · lore

Re: [PATCH 00/18] Signed push

Stefan Beller <stefanbeller@gmail.com> writes:
Show 19 quoted lines
>> "burden" is not an issue, as I'll be signing the push certificate
>> anyway when I push.  A signed tag or a signed commit and signed push
>> certificate solves two completely separate and orthogonal issues.
>> 
>> What happens if you break into GitHub or k.org and did
>> 
>>     $ git tag maint_2014_08_22 master_2014_08_22
>
> Ok, I personally haven't used tags a lot.
> I just tried to
> 	git tag -s testbreaktag v2.1.0
> 	git show testbreaktag
> 	# However it would still read:
> tag v2.1.0
> Tagger: Junio C Hamano <gitster@pobox.com>
> Date:   Fri Aug 15 15:09:28 2014 -0700
>
> So as I do not posess your private key I could not create signed tags
> even if I were to break into github/k.org

The point was that after I push to 'maint', you break into the site and copy or move that tag as if I pushed to 'master'.

You could argue that I could create a signed tag 'maint-2014-08-25', push it, and if you moved it to tags/master-2014-08-25 as an attacker, the refname would not match the "tag " line in the signed tag object. While that is true, nobody other thaan fsck checks the contents on the "tag " line in practice.

But more importantly.

I may deem a commit a sensible version for the 'master' branch of one repository but it would not be sensible for another repository's 'master' branch. Imagine a world just like the kernel development during 2.6 era used to be, where there was a separate tree 2.4 maintained with its own 'master' branch. What is appropriate for the tip of 'master' to one repository is not good for the other one, and your timestamped "tag " line may say for which branch the push was for but does not say for which repository. The exact problem is also shared with the desire to have a "branch" object expressed elsewhere; as there is no identity for a "branch" in a distributed world, trying to name "branch" as if it is a global entity without mentioning what repository will lead to tears.

Besides, these tags/maint-2014-08-25 tags will be interesting only for those who are auditing and not for general public, and we do not have a good way to "hide" uninteresting refs until those with narrow niche interest ask yet, which is something we may want to add soon, but I do not want "auditable push" taken hostage to that unrelated feature.

Jason Pyeron· Aug 25, 2014, 18:38 UTC · re: Junio C Hamano · lore

RE: [PATCH 00/18] Signed push

Show 30 quoted lines
> -----Original Message-----
> From: Junio C Hamano
> Sent: Monday, August 25, 2014 13:55
> 
> Stefan Beller <stefanbeller@gmail.com> writes:
> 
> >> "burden" is not an issue, as I'll be signing the push certificate
> >> anyway when I push.  A signed tag or a signed commit and 
> signed push
> >> certificate solves two completely separate and orthogonal issues.
> >> 
> >> What happens if you break into GitHub or k.org and did
> >> 
> >>     $ git tag maint_2014_08_22 master_2014_08_22
> >
> > Ok, I personally haven't used tags a lot.
> > I just tried to
> > 	git tag -s testbreaktag v2.1.0
> > 	git show testbreaktag
> > 	# However it would still read:
> > tag v2.1.0
> > Tagger: Junio C Hamano <gitster@pobox.com>
> > Date:   Fri Aug 15 15:09:28 2014 -0700
> >
> > So as I do not posess your private key I could not create 
> signed tags
> > even if I were to break into github/k.org
> 
> The point was that after I push to 'maint', you break into the site
> and copy or move that tag as if I pushed to 'master'.
What is needed is not a signed push per se, but rather a need for a set of signed HEADS ...
Show 15 quoted lines
> 
> You could argue that I could create a signed tag 'maint-2014-08-25',
> push it, and if you moved it to tags/master-2014-08-25 as an
> attacker, the refname would not match the "tag " line in the signed
> tag object.  While that is true, nobody other thaan fsck checks the
> contents on the "tag " line in practice.
> 
> But more importantly.
> 
> I may deem a commit a sensible version for the 'master' branch of
> one repository but it would not be sensible for another repository's
> 'master' branch.  Imagine a world just like the kernel development
> during 2.6 era used to be, where there was a separate tree 2.4
> maintained with its own 'master' branch.  What is appropriate for
> the tip of 'master' to one repository is not good for the other one,
... and these signed HEADS need to be tied to a particular repository instance. AFAIK git does not have any unique identifier per repository instance to leverage. If you were to make a repository instance id you could take that and the branch name as input to a signed hash for verification later. But this leads to deeper issues about new workflow, new configuration storage mechanisms, etc.
Show 19 quoted lines
> and your timestamped "tag " line may say for which branch the push
> was for but does not say for which repository.  The exact problem is
> also shared with the desire to have a "branch" object expressed
> elsewhere; as there is no identity for a "branch" in a distributed
> world, trying to name "branch" as if it is a global entity without
> mentioning what repository will lead to tears.
> 
> Besides, these tags/maint-2014-08-25 tags will be interesting only
> for those who are auditing and not for general public, and we do not
> have a good way to "hide" uninteresting refs until those with narrow
> niche interest ask yet, which is something we may want to add soon,
> but I do not want "auditable push" taken hostage to that unrelated
> feature.
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

← back to recent threads