* [PATCH 1/2] send-pack: typofix error message @ 2011-09-07 20:56 Junio C Hamano 2011-09-07 20:57 ` [PATCH 2/2] push -s: skeleton Junio C Hamano 0 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2011-09-07 20:56 UTC (permalink / raw) To: git; +Cc: Shawn O. Pearce The message identifies the process as receive-pack when it cannot fork the sideband demultiplexer. We are actually a send-pack. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin/send-pack.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/builtin/send-pack.c b/builtin/send-pack.c index c1f6ddd..87833f4 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -334,7 +334,7 @@ int send_pack(struct send_pack_args *args, demux.data = fd; demux.out = -1; if (start_async(&demux)) - die("receive-pack: unable to fork off sideband demultiplexer"); + die("send-pack: unable to fork off sideband demultiplexer"); in = demux.out; } -- 1.7.7.rc0.186.g50963 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 2/2] push -s: skeleton 2011-09-07 20:56 [PATCH 1/2] send-pack: typofix error message Junio C Hamano @ 2011-09-07 20:57 ` Junio C Hamano 2011-09-07 21:18 ` Shawn Pearce ` (5 more replies) 0 siblings, 6 replies; 26+ messages in thread From: Junio C Hamano @ 2011-09-07 20:57 UTC (permalink / raw) To: git; +Cc: Shawn O. Pearce If a tag is GPG-signed, and if you trust the cryptographic robustness of the SHA-1 and GPG, you can guarantee that all the history leading to the signed commit is not tampered with. However, it would be both cumbersome and cluttering to sign each and every commit. Especially if you strive to keep your history clean by tweaking, rewriting and polishing your commits before pushing the resulting history out, many commits you will create locally end up not mattering at all, and it is a waste of time to sign them. A better alternative could be to sign a "push certificate" (for the lack of better name) every time you push, asserting that what commits you are pushing to update which refs. The basic workflow goes like this: 1. You push out your work with "git push -s"; 2. "git push", as usual, learns where the remote refs are and which refs are to be updated with this push. It prepares a text file in memory that looks like this using this information: Push-Certificate-Version: 1 Pusher: Junio C Hamano <gitster@pobox.com> 1315427886 -0700 Update: e83c51633... d4e58965f... refs/heads/master Update: 5a144a288... 7931f38a2... refs/heads/next An actual push certificate records full 40-char object name, but it is ellided for brevity here. The user then is asked to sign this push certificate using GPG. The result is carried to the other side (i.e. receive-pack). In the protocol exchange, this step comes immediately after the sender tells what the result of the push should be, before it sends the pack data. 3. The receiving end will keep the signed push certificate in core, receives the pack data and unpacks (or stores and runs index-pack) as usual. 4. A new phase to record the push certificate is introduced in the codepath after the receiving end runs receive_hook(). It is envisioned that this phase: a. parses the updated-to object names, and appends the push certificate (still GPG signed) to a note attached to each of the objects that will sit at the tip of the refs; b. verifies that the push certificate is signed with a GPG key that is authorized to push into this repository; and/or c. invokes pre-push-verify-signature hook, feeds the push certificate to it and asks it to veto the ref updates. And here is a skeleton to implement it. It has all the necessary protocol extensions implemented (although I do not know if we need separate codepath for stateless RPC mode), but does not have subroutines to: - Sign the certificate with GPG key; - Parse the signed certificate to identify the updated-to objects, and add the certificate as notes to them; - Verify the certificate and find out what GPG key was used to sign it; or - Invoke and feed the certificate to pre-push-verify-hook. all of which should be fairly trivial. The places that needs to implement these are clearly marked with large comments, so I'll leave it up to other people who are interested in the topic to fill in the blanks ;-) Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin/push.c | 1 + builtin/receive-pack.c | 54 +++++++++++++++++++++++++++++++++++++++- builtin/send-pack.c | 65 +++++++++++++++++++++++++++++++++++++++++++++--- send-pack.h | 1 + transport.c | 4 +++ transport.h | 4 +++ 6 files changed, 124 insertions(+), 5 deletions(-) diff --git a/builtin/push.c b/builtin/push.c index 35cce53..2238f4e 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -261,6 +261,7 @@ int cmd_push(int argc, const char **argv, const char *prefix) OPT_BIT('u', "set-upstream", &flags, "set upstream for git pull/status", TRANSPORT_PUSH_SET_UPSTREAM), OPT_BOOLEAN(0, "progress", &progress, "force progress reporting"), + OPT_BIT('s', "signed", &flags, "GPG sign the push", TRANSPORT_PUSH_SIGNED), OPT_END() }; diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index ae164da..307fc3b 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -30,12 +30,14 @@ static int receive_unpack_limit = -1; static int transfer_unpack_limit = -1; static int unpack_limit = 100; static int report_status; +static int signed_push; static int use_sideband; static int prefer_ofs_delta = 1; static int auto_update_server_info; static int auto_gc = 1; static const char *head_name; static int sent_capabilities; +static char *push_certificate; static enum deny_action parse_deny_action(const char *var, const char *value) { @@ -114,7 +116,7 @@ static int show_ref(const char *path, const unsigned char *sha1, int flag, void else packet_write(1, "%s %s%c%s%s\n", sha1_to_hex(sha1), path, 0, - " report-status delete-refs side-band-64k", + " report-status delete-refs side-band-64k signed-push", prefer_ofs_delta ? " ofs-delta" : ""); sent_capabilities = 1; return 0; @@ -579,6 +581,31 @@ static void check_aliased_updates(struct command *commands) string_list_clear(&ref_list, 0); } +static int record_signed_push(char *cert) +{ + /* + * This is the place for you to parse the signed push + * certificate, grab the commit object names the push updates + * refs to, and append the certificate to the notes to these + * commits. + * + * You could also feed the signed push certificate to GPG, + * verify the signer identity, and all the other fun stuff, + * including feeding it to "pre-push-verify-signature" hook. + * + * Here we just throw it to stderr to demonstrate that the + * codepath is being exercised. + */ + char *cp, *ep; + for (cp = cert; *cp; cp = ep) { + ep = strchrnul(cp, '\n'); + if (*ep == '\n') + ep++; + fprintf(stderr, "RSP: %.*s", (int)(ep - cp), cp); + } + return 0; +} + static void execute_commands(struct command *commands, const char *unpacker_error) { struct command *cmd; @@ -596,6 +623,12 @@ static void execute_commands(struct command *commands, const char *unpacker_erro return; } + if (push_certificate && record_signed_push(push_certificate)) { + for (cmd = commands; cmd; cmd = cmd->next) + cmd->error_string = "n/a (push signature error)"; + return; + } + check_aliased_updates(commands); head_name = resolve_ref("HEAD", sha1, 0, NULL); @@ -636,6 +669,8 @@ static struct command *read_head_info(void) report_status = 1; if (strstr(refname + reflen + 1, "side-band-64k")) use_sideband = LARGE_PACKET_MAX; + if (strstr(refname + reflen + 1, "signed-push")) + signed_push = 1; } cmd = xcalloc(1, sizeof(struct command) + len - 80); hashcpy(cmd->old_sha1, old_sha1); @@ -731,6 +766,21 @@ static const char *unpack(void) } } +static char *receive_push_certificate(void) +{ + struct strbuf cert = STRBUF_INIT; + for (;;) { + char line[1000]; + int len; + + len = packet_read_line(0, line, sizeof(line)); + if (!len) + break; + strbuf_add(&cert, line, len); + } + return strbuf_detach(&cert, NULL); +} + static void report(struct command *commands, const char *unpack_status) { struct command *cmd; @@ -846,6 +896,8 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) if ((commands = read_head_info()) != NULL) { const char *unpack_status = NULL; + if (signed_push) + push_certificate = receive_push_certificate(); if (!delete_only(commands)) unpack_status = unpack(); execute_commands(commands, unpack_status); diff --git a/builtin/send-pack.c b/builtin/send-pack.c index 87833f4..3193f34 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -237,6 +237,27 @@ static int sideband_demux(int in, int out, void *data) return ret; } +static void sign_push_certificate(struct strbuf *cert) +{ + /* + * Here, take the contents of cert->buf, and have the user GPG + * sign it, and read it back in the strbuf. + * + * You may want to append some extra info to cert before giving + * it to GPG, possibly via a hook. + * + * Here we upcase them just to demonstrate that the codepath + * is being exercised. + */ + char *cp; + for (cp = cert->buf; *cp; cp++) { + int ch = *cp; + if ('a' <= ch && ch <= 'z') + *cp = toupper(ch); + } + return; +} + int send_pack(struct send_pack_args *args, int fd[], struct child_process *conn, struct ref *remote_refs, @@ -250,9 +271,11 @@ int send_pack(struct send_pack_args *args, int allow_deleting_refs = 0; int status_report = 0; int use_sideband = 0; + int signed_push = 0; unsigned cmds_sent = 0; int ret; struct async demux; + struct strbuf push_cert = STRBUF_INIT; /* Does the other end support the reporting? */ if (server_supports("report-status")) @@ -270,6 +293,19 @@ int send_pack(struct send_pack_args *args, return 0; } + if (args->signed_push) { + if (server_supports("signed-push")) + signed_push = !args->dry_run; + else + warning("The receiving side does not support signed-push"); + } + + if (signed_push) { + const char *committer_info = git_committer_info(0); + strbuf_addstr(&push_cert, "Push-Certificate-Version: 1\n"); + strbuf_addf(&push_cert, "Pusher: %s\n", committer_info); + } + /* * Finally, tell the other end! */ @@ -301,15 +337,19 @@ int send_pack(struct send_pack_args *args, char *old_hex = sha1_to_hex(ref->old_sha1); char *new_hex = sha1_to_hex(ref->new_sha1); - if (!cmds_sent && (status_report || use_sideband)) { - packet_buf_write(&req_buf, "%s %s %s%c%s%s", + if (!cmds_sent && + (status_report || use_sideband || signed_push)) + packet_buf_write(&req_buf, "%s %s %s%c%s%s%s", old_hex, new_hex, ref->name, 0, status_report ? " report-status" : "", - use_sideband ? " side-band-64k" : ""); - } + use_sideband ? " side-band-64k" : "", + signed_push ? " signed-push" : ""); else packet_buf_write(&req_buf, "%s %s %s", old_hex, new_hex, ref->name); + if (signed_push && hashcmp(ref->old_sha1, ref->new_sha1)) + strbuf_addf(&push_cert, "Update: %s %s %s\n", + old_hex, new_hex, ref->name); ref->status = status_report ? REF_STATUS_EXPECTING_REPORT : REF_STATUS_OK; @@ -326,6 +366,23 @@ int send_pack(struct send_pack_args *args, safe_write(out, req_buf.buf, req_buf.len); packet_flush(out); } + + if (signed_push) { + char *cp, *ep; + + sign_push_certificate(&push_cert); + strbuf_reset(&req_buf); + for (cp = push_cert.buf; *cp; cp = ep) { + ep = strchrnul(cp, '\n'); + if (*ep == '\n') + ep++; + packet_buf_write(&req_buf, "%.*s", + (int)(ep - cp), cp); + } + /* Do we need anything funky for stateless rpc? */ + safe_write(out, req_buf.buf, req_buf.len); + packet_flush(out); + } strbuf_release(&req_buf); if (use_sideband && cmds_sent) { diff --git a/send-pack.h b/send-pack.h index 05d7ab1..754943e 100644 --- a/send-pack.h +++ b/send-pack.h @@ -11,6 +11,7 @@ struct send_pack_args { use_thin_pack:1, use_ofs_delta:1, dry_run:1, + signed_push:1, stateless_rpc:1; }; diff --git a/transport.c b/transport.c index fa279d5..7a7ffe4 100644 --- a/transport.c +++ b/transport.c @@ -476,6 +476,9 @@ static int set_git_option(struct git_transport_options *opts, else opts->depth = atoi(value); return 0; + } else if (!strcmp(name, TRANS_OPT_SIGNED_PUSH)) { + opts->signed_push = !!value; + return 0; } return 1; } @@ -793,6 +796,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re args.progress = transport->progress; args.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN); args.porcelain = !!(flags & TRANSPORT_PUSH_PORCELAIN); + args.signed_push = !!(flags & TRANSPORT_PUSH_SIGNED); ret = send_pack(&args, data->fd, data->conn, remote_refs, &data->extra_have); diff --git a/transport.h b/transport.h index 059b330..d2fa478 100644 --- a/transport.h +++ b/transport.h @@ -8,6 +8,7 @@ struct git_transport_options { unsigned thin : 1; unsigned keep : 1; unsigned followtags : 1; + unsigned signed_push : 1; int depth; const char *uploadpack; const char *receivepack; @@ -102,6 +103,7 @@ struct transport { #define TRANSPORT_PUSH_PORCELAIN 16 #define TRANSPORT_PUSH_SET_UPSTREAM 32 #define TRANSPORT_RECURSE_SUBMODULES_CHECK 64 +#define TRANSPORT_PUSH_SIGNED 128 #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3) @@ -128,6 +130,8 @@ struct transport *transport_get(struct remote *, const char *); /* Aggressively fetch annotated tags if possible */ #define TRANS_OPT_FOLLOWTAGS "followtags" +#define TRANS_OPT_SIGNED_PUSH "signedpush" + /** * Returns 0 if the option was used, non-zero otherwise. Prints a * message to stderr if the option is not used. -- 1.7.7.rc0.186.g50963 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] push -s: skeleton 2011-09-07 20:57 ` [PATCH 2/2] push -s: skeleton Junio C Hamano @ 2011-09-07 21:18 ` Shawn Pearce 2011-09-07 22:21 ` Junio C Hamano 2011-09-07 22:21 ` Nguyen Thai Ngoc Duy ` (4 subsequent siblings) 5 siblings, 1 reply; 26+ messages in thread From: Shawn Pearce @ 2011-09-07 21:18 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Wed, Sep 7, 2011 at 13:57, Junio C Hamano <gitster@pobox.com> wrote: > If a tag is GPG-signed, and if you trust the cryptographic robustness of > the SHA-1 and GPG, you can guarantee that all the history leading to the > signed commit is not tampered with. However, it would be both cumbersome > and cluttering to sign each and every commit. Especially if you strive to > keep your history clean by tweaking, rewriting and polishing your commits > before pushing the resulting history out, many commits you will create > locally end up not mattering at all, and it is a waste of time to sign > them. > > A better alternative could be to sign a "push certificate" (for the lack > of better name) every time you push, asserting that what commits you are > pushing to update which refs. The basic workflow goes like this: > > 1. You push out your work with "git push -s"; Yay! > And here is a skeleton to implement it. It has all the necessary protocol > extensions implemented (although I do not know if we need separate > codepath for stateless RPC mode), but does not have subroutines to: Yea, its broken for stateless RPC. See below. > +static char *receive_push_certificate(void) > +{ > + struct strbuf cert = STRBUF_INIT; > + for (;;) { > + char line[1000]; 1000 isn't enough for some certificates. Imagine pushing a Gerrit Code Review managed repository with 2M worth of advertisement data at once. You can't sign that in 1000 bytes. > @@ -326,6 +366,23 @@ int send_pack(struct send_pack_args *args, > safe_write(out, req_buf.buf, req_buf.len); > packet_flush(out); > } > + > + if (signed_push) { > + char *cp, *ep; > + > + sign_push_certificate(&push_cert); > + strbuf_reset(&req_buf); > + for (cp = push_cert.buf; *cp; cp = ep) { > + ep = strchrnul(cp, '\n'); > + if (*ep == '\n') > + ep++; > + packet_buf_write(&req_buf, "%.*s", > + (int)(ep - cp), cp); > + } > + /* Do we need anything funky for stateless rpc? */ Yes. Above we flushed the req_buf and send that in an HTTP request. You need to hoist this block above the "if (args->stateless_rpc)" segment. -- Shawn. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] push -s: skeleton 2011-09-07 21:18 ` Shawn Pearce @ 2011-09-07 22:21 ` Junio C Hamano 2011-09-07 23:23 ` Shawn Pearce 0 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2011-09-07 22:21 UTC (permalink / raw) To: Shawn Pearce; +Cc: Junio C Hamano, git Shawn Pearce <spearce@spearce.org> writes: > Yes. Above we flushed the req_buf and send that in an HTTP request. > You need to hoist this block above the "if (args->stateless_rpc)" > segment. What do you mean by "hoist"? For the req advertisement, it seems that you are not hoisting anything but duplicating the code, turning safe_write() followed by flush into packet-buf-flush and sending the result over the sideband. Shouldn't this new data be sent over the sideband-to-http the same way? Unless you do not want signed push over http, that is... builtin/send-pack.c | 10 +++++++--- 1 files changed, 7 insertions(+), 3 deletions(-) diff --git a/builtin/send-pack.c b/builtin/send-pack.c index 3193f34..37e0313 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -379,9 +379,13 @@ int send_pack(struct send_pack_args *args, packet_buf_write(&req_buf, "%.*s", (int)(ep - cp), cp); } - /* Do we need anything funky for stateless rpc? */ - safe_write(out, req_buf.buf, req_buf.len); - packet_flush(out); + if (args->stateless_rpc) { + packet_buf_flush(&req_buf); + send_sideband(out, -1, req_buf.buf, req_buf.len, LARGE_PACKET_MAX); + } else { + safe_write(out, req_buf.buf, req_buf.len); + packet_flush(out); + } } strbuf_release(&req_buf); ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] push -s: skeleton 2011-09-07 22:21 ` Junio C Hamano @ 2011-09-07 23:23 ` Shawn Pearce 2011-09-08 16:24 ` Junio C Hamano 0 siblings, 1 reply; 26+ messages in thread From: Shawn Pearce @ 2011-09-07 23:23 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Wed, Sep 7, 2011 at 15:21, Junio C Hamano <gitster@pobox.com> wrote: > Shawn Pearce <spearce@spearce.org> writes: > >> Yes. Above we flushed the req_buf and send that in an HTTP request. >> You need to hoist this block above the "if (args->stateless_rpc)" >> segment. > > What do you mean by "hoist"? For the req advertisement, it seems that you > are not hoisting anything but duplicating the code, turning safe_write() > followed by flush into packet-buf-flush and sending the result over the > sideband. Shouldn't this new data be sent over the sideband-to-http the > same way? > > Unless you do not want signed push over http, that is... We do. > diff --git a/builtin/send-pack.c b/builtin/send-pack.c > index 3193f34..37e0313 100644 > --- a/builtin/send-pack.c > +++ b/builtin/send-pack.c > @@ -379,9 +379,13 @@ int send_pack(struct send_pack_args *args, > packet_buf_write(&req_buf, "%.*s", > (int)(ep - cp), cp); > } > - /* Do we need anything funky for stateless rpc? */ > - safe_write(out, req_buf.buf, req_buf.len); > - packet_flush(out); > + if (args->stateless_rpc) { > + packet_buf_flush(&req_buf); > + send_sideband(out, -1, req_buf.buf, req_buf.len, LARGE_PACKET_MAX); > + } else { > + safe_write(out, req_buf.buf, req_buf.len); > + packet_flush(out); > + } This sounds too late to me. I think you just caused 2 HTTP POSTs, one a partial one with the commands and no pack data, and another with the push certificate and the pack. Neither is useful. -- Shawn. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] push -s: skeleton 2011-09-07 23:23 ` Shawn Pearce @ 2011-09-08 16:24 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2011-09-08 16:24 UTC (permalink / raw) To: Shawn Pearce; +Cc: git Shawn Pearce <spearce@spearce.org> writes: > This sounds too late to me. I think you just caused 2 HTTP POSTs, one > a partial one with the commands and no pack data, and another with the > push certificate and the pack. Neither is useful. Well, then I'd stop looking at this area and let others make it useful while I hack in other areas. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] push -s: skeleton 2011-09-07 20:57 ` [PATCH 2/2] push -s: skeleton Junio C Hamano 2011-09-07 21:18 ` Shawn Pearce @ 2011-09-07 22:21 ` Nguyen Thai Ngoc Duy 2011-09-07 22:40 ` Junio C Hamano 2011-09-07 23:55 ` Robin H. Johnson ` (3 subsequent siblings) 5 siblings, 1 reply; 26+ messages in thread From: Nguyen Thai Ngoc Duy @ 2011-09-07 22:21 UTC (permalink / raw) To: Junio C Hamano, Robin H. Johnson; +Cc: git, Shawn O. Pearce On Thu, Sep 8, 2011 at 6:57 AM, Junio C Hamano <gitster@pobox.com> wrote: > A better alternative could be to sign a "push certificate" (for the lack > of better name) every time you push, asserting that what commits you are > pushing to update which refs. The basic workflow goes like this: > > 1. You push out your work with "git push -s"; > > 2. "git push", as usual, learns where the remote refs are and which refs > are to be updated with this push. It prepares a text file in memory > that looks like this using this information: > > Push-Certificate-Version: 1 > Pusher: Junio C Hamano <gitster@pobox.com> 1315427886 -0700 > Update: e83c51633... d4e58965f... refs/heads/master > Update: 5a144a288... 7931f38a2... refs/heads/next > > An actual push certificate records full 40-char object name, but it is > ellided for brevity here. > > The user then is asked to sign this push certificate using GPG. The > result is carried to the other side (i.e. receive-pack). In the > protocol exchange, this step comes immediately after the sender tells > what the result of the push should be, before it sends the pack data. > > 3. The receiving end will keep the signed push certificate in core, > receives the pack data and unpacks (or stores and runs index-pack) > as usual. > > 4. A new phase to record the push certificate is introduced in the > codepath after the receiving end runs receive_hook(). It is envisioned > that this phase: > > a. parses the updated-to object names, and appends the push > certificate (still GPG signed) to a note attached to each of the > objects that will sit at the tip of the refs; I recall Gentoo wanted something like this (recording who pushes what). Pulling Robin in if he has any comments. -- Duy ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] push -s: skeleton 2011-09-07 22:21 ` Nguyen Thai Ngoc Duy @ 2011-09-07 22:40 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2011-09-07 22:40 UTC (permalink / raw) To: Nguyen Thai Ngoc Duy Cc: Junio C Hamano, Robin H. Johnson, git, Shawn O. Pearce Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes: >> ... >> 4. A new phase to record the push certificate is introduced in the >> codepath after the receiving end runs receive_hook(). It is envisioned >> that this phase: >> >> a. parses the updated-to object names, and appends the push >> certificate (still GPG signed) to a note attached to each of the >> objects that will sit at the tip of the refs; > > I recall Gentoo wanted something like this (recording who pushes > what). Pulling Robin in if he has any comments. As the beauty of this approach is that we can update and tailor what the receiving end does using the information given from the server, it is a strange thing to do to chomp this list in the middle at a funny place here. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] push -s: skeleton 2011-09-07 20:57 ` [PATCH 2/2] push -s: skeleton Junio C Hamano 2011-09-07 21:18 ` Shawn Pearce 2011-09-07 22:21 ` Nguyen Thai Ngoc Duy @ 2011-09-07 23:55 ` Robin H. Johnson 2011-09-08 20:03 ` Jeff King 2011-09-08 4:37 ` [PATCH 3/2] Split GPG interface into its own helper library Junio C Hamano ` (2 subsequent siblings) 5 siblings, 1 reply; 26+ messages in thread From: Robin H. Johnson @ 2011-09-07 23:55 UTC (permalink / raw) To: Git Mailing List; +Cc: Shawn O. Pearce [-- Attachment #1: Type: text/plain, Size: 2111 bytes --] On Wed, Sep 07, 2011 at 01:57:27PM -0700, Junio C Hamano wrote: > If a tag is GPG-signed, and if you trust the cryptographic robustness of > the SHA-1 and GPG, you can guarantee that all the history leading to the > signed commit is not tampered with. However, it would be both cumbersome > and cluttering to sign each and every commit. Especially if you strive to > keep your history clean by tweaking, rewriting and polishing your commits > before pushing the resulting history out, many commits you will create > locally end up not mattering at all, and it is a waste of time to sign > them. Thanks to pcloud for including me on the thread. I do find the idea of these push-certificates very interesting and useful, but I think they will do best to augment signed commits, not replace them. There's a couple of related things we've been considering on the Gentoo side: - detached signatures of blobs (either the SHA1 of the blob or the blob itself) - The signature covering the message+blob details, but NOT the chain of history: this opens up the ability to cherry-pick and rebase iff there are no conflicts and the blobs are identical, all while preserving the signature. - concerns about a pre-image attack against Git. tl;dr version: 1. Attacker prepares decoy file in advance, that hashes to the same as the malicious file. 2. Attacker sends decoy in as an innocuous real commit. 3. Months later, the attacker breaks into the system and alters the packfile to include the new malicious file. 4. All new clones from that point forward get the malicious version. Re your comment on always needing to resign commits above, we'd been considering post-signing commits, not when they are initially made. After your commit is clean and ready to ship, you can fire the commit ids into the signature tool, which can generate a detached signature note for each commit. -- Robin Hugh Johnson Gentoo Linux: Developer, Trustee & Infrastructure Lead E-Mail : robbat2@gentoo.org GnuPG FP : 11AC BA4F 4778 E3F6 E4ED F38E B27B 944E 3488 4E85 [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 330 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] push -s: skeleton 2011-09-07 23:55 ` Robin H. Johnson @ 2011-09-08 20:03 ` Jeff King 2011-09-09 1:30 ` Robin H. Johnson 0 siblings, 1 reply; 26+ messages in thread From: Jeff King @ 2011-09-08 20:03 UTC (permalink / raw) To: Robin H. Johnson; +Cc: Git Mailing List, Shawn O. Pearce On Wed, Sep 07, 2011 at 11:55:44PM +0000, Robin H. Johnson wrote: > There's a couple of related things we've been considering on the Gentoo > side: > - detached signatures of blobs (either the SHA1 of the blob or the blob > itself) There's not much point in signing the blob itself and not the sha1; the first thing any signature algorithm will do is make a fixed-size digest of the content anyway. So it is only useful if you don't trust sha1 as your digest algorithm (which maybe is a reasonable concern these days...). > - The signature covering the message+blob details, but NOT the chain of > history: this opens up the ability to cherry-pick and rebase iff there > are no conflicts and the blobs are identical, all while preserving the > signature. The problem is that many of the blobs won't be identical, because they'll have new content from the new commits you rebased on top of. So _some_ blobs will be the same, but you'll end up with a half-signed commit. I think you're better to just re-sign the new history. But I'd have to see a longer description of your scheme to really critique it. I'm not 100% sure what your security goals are here. > - concerns about a pre-image attack against Git. tl;dr version: > 1. Attacker prepares decoy file in advance, that hashes to the same as > the malicious file. > 2. Attacker sends decoy in as an innocuous real commit. > 3. Months later, the attacker breaks into the system and alters the > packfile to include the new malicious file. > 4. All new clones from that point forward get the malicious version. Nit: I think you mean "collision attack" here. Pre-image attacks are matching a malicious file to what is already in the tree, but are much harder to execute. But yeah, it is a potential problem. I don't keep up very well with that sort of news anymore, but AFAIK, we still don't have any actual collisions in sha1. Wikipedia seems to seem to think the best attacks are in the 2^50-ish range, but nobody has successfully found one. So we may still be a few years away from a realistic attack. If the attacks are anything like the MD5 attacks, the decoy and malicious files will need to have a bunch of random garbage in them. Which may be hard to disguise, depending on your repo contents. I think, though, that the sane fix at that point is not to start trying to make per-blob signatures or anything like that, but to consider "git version 2" with SHA-256, or whatever ends up becoming SHA-3 next year. It would involve rewriting all of your history and dropping support for older git clients, of course, but it may be worth it at the point that sha1 is completely broken. > Re your comment on always needing to resign commits above, we'd been > considering post-signing commits, not when they are initially made. > After your commit is clean and ready to ship, you can fire the commit > ids into the signature tool, which can generate a detached signature > note for each commit. Agreed. This is just an interface problem, not a cryptographic or technical one. However, I do think there's a subtle difference between the two ideas. Signing each commit individually just indicates some approval of particular commits. But signing a push certificate is about associating particular commits with particular refs (e.g., saying "move 'master' from commit X to commit Y). I think there may be uses for both forms. -Peff ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] push -s: skeleton 2011-09-08 20:03 ` Jeff King @ 2011-09-09 1:30 ` Robin H. Johnson 2011-09-09 16:03 ` Joey Hess 0 siblings, 1 reply; 26+ messages in thread From: Robin H. Johnson @ 2011-09-09 1:30 UTC (permalink / raw) To: Jeff King, Git Mailing List; +Cc: Robin H. Johnson, Shawn O. Pearce On Thu, Sep 08, 2011 at 04:03:43PM -0400, Jeff King wrote: > On Wed, Sep 07, 2011 at 11:55:44PM +0000, Robin H. Johnson wrote: > > There's a couple of related things we've been considering on the Gentoo > > side: > > - detached signatures of blobs (either the SHA1 of the blob or the blob > > itself) > There's not much point in signing the blob itself and not the sha1; the > first thing any signature algorithm will do is make a fixed-size digest > of the content anyway. So it is only useful if you don't trust sha1 as > your digest algorithm (which maybe is a reasonable concern these > days...). To clarify here, there's two things gained by this over signing the SHA1: 1. Ability to use a different digest algorithm right now, without having to trust SHA1. 2. Ability to use HMAC. I agree that both of these are bandaids to the security of SHA*. > > - The signature covering the message+blob details, but NOT the chain of > > history: this opens up the ability to cherry-pick and rebase iff there > > are no conflicts and the blobs are identical, all while preserving the > > signature. > The problem is that many of the blobs won't be identical, because > they'll have new content from the new commits you rebased on top of. So > _some_ blobs will be the same, but you'll end up with a half-signed > commit. I think you're better to just re-sign the new history. Possibly, the rebase/cherry-pick isn't critical. > But I'd have to see a longer description of your scheme to really > critique it. I'm not 100% sure what your security goals are here. The primary goal is that a developer can certify their commits when they are ready to push them, regardless of the means of how they are going to push them (I've had requests for some git-bundle usage help in pushes). > > - concerns about a pre-image attack against Git. tl;dr version: > > 1. Attacker prepares decoy file in advance, that hashes to the same as > > the malicious file. > > 2. Attacker sends decoy in as an innocuous real commit. > > 3. Months later, the attacker breaks into the system and alters the > > packfile to include the new malicious file. > > 4. All new clones from that point forward get the malicious version. > Nit: I think you mean "collision attack" here. Pre-image attacks are > matching a malicious file to what is already in the tree, but are much > harder to execute. My bad, it was really late when I was writing the above. > But yeah, it is a potential problem. I don't keep up very well with that > sort of news anymore, but AFAIK, we still don't have any actual > collisions in sha1. Wikipedia seems to seem to think the best attacks > are in the 2^50-ish range, but nobody has successfully found one. So we > may still be a few years away from a realistic attack. If the attacks > are anything like the MD5 attacks, the decoy and malicious files will > need to have a bunch of random garbage in them. Which may be hard to > disguise, depending on your repo contents. Joey Hess discussed this two years ago, and again last week: http://kitenet.net/~joey/blog/entry/size_of_the_git_sha1_collision_attack_surface/ http://kitenet.net/~joey/blog/entry/sha-1/ This is easy in the kernel tree, it's got lots of eyeballs and only few binary files. This isn't true for lots of other Git trees, a tree with a JPEG image or a gzip file would be a great target. It's 2^53 last I saw, which is only ~250 days of cputime on my i7 desktop. Also notably that is under the complexity of the 2^56 EFF DES cracker was built do to (and the more recent COPACOBANA unit). > I think, though, that the sane fix at that point is not to start trying > to make per-blob signatures or anything like that, but to consider "git > version 2" with SHA-256, or whatever ends up becoming SHA-3 next year. > It would involve rewriting all of your history and dropping support for > older git clients, of course, but it may be worth it at the point that > sha1 is completely broken. I think this is needed sooner rather than later. > > Re your comment on always needing to resign commits above, we'd been > > considering post-signing commits, not when they are initially made. > > After your commit is clean and ready to ship, you can fire the commit > > ids into the signature tool, which can generate a detached signature > > note for each commit. > Agreed. This is just an interface problem, not a cryptographic or > technical one. However, I do think there's a subtle difference between > the two ideas. Signing each commit individually just indicates some > approval of particular commits. But signing a push certificate is about > associating particular commits with particular refs (e.g., saying "move > 'master' from commit X to commit Y). I think there may be uses for both > forms. Yes, there is use for both forms. The additional one is that you can separate who pushes from who commits. -- Robin Hugh Johnson Gentoo Linux: Developer, Trustee & Infrastructure Lead E-Mail : robbat2@gentoo.org GnuPG FP : 11AC BA4F 4778 E3F6 E4ED F38E B27B 944E 3488 4E85 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] push -s: skeleton 2011-09-09 1:30 ` Robin H. Johnson @ 2011-09-09 16:03 ` Joey Hess 2011-09-09 16:14 ` Drew Northup 2011-09-09 19:12 ` Jeff King 0 siblings, 2 replies; 26+ messages in thread From: Joey Hess @ 2011-09-09 16:03 UTC (permalink / raw) To: Git Mailing List [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="//TRANSLIT", Size: 2109 bytes --] Robin H. Johnson wrote: > Joey Hess discussed this two years ago, and again last week: > http://kitenet.net/~joey/blog/entry/size_of_the_git_sha1_collision_attack_surface/ > > This is easy in the kernel tree, it's got lots of eyeballs and only few > binary files. This isn't true for lots of other Git trees, a tree with a > JPEG image or a gzip file would be a great target. The most credible attack I have so far does not involve binary files in tree. Someone pointed out that git log, git show, etc stop printing commit messages at NULL. So colliding binary garbage can be put in a commit message and be unlikely to be noticed, and the commit can later be altered to point to a different tree. https://github.com/joeyh/supercollider joey@gnu:~/tmp/supercollider>git log commit 24f30db5790b209fa412ce81c5ef2bf8af5fd4d7 Author: Joey Hess <joey@kitenet.net> Date: Fri Sep 9 11:49:21 2011 -0400 an innocent commit If this were a sha1 colliding attack, there would be some sort of binary garbage below. Which there isn't. So this can be safely merged. joey@gnu:~/tmp/supercollider>git cat-file commit 24f30db5790b209fa412ce81c5ef2bf8af5fd4d7 tree 735a7633237c07b398856005de3bc9ea00446747 author Joey Hess <joey@kitenet.net> 1315583361 -0400 committer Joey Hess <joey@kitenet.net> 1315583361 -0400 an innocent commit If this were a sha1 colliding attack, there would be some sort of binary garbage below. Which there isn't. So this can be safely merged. \0 ??b???\x1f[?i??ͯ?t?\f2??\x02????os?\x14<????h?+,M?mY?e?EW?i\x13v$???\x14J??U}n~???L??????f??\x02?ě??3>?Q??H?\x16*zl\x1a?RA˂q?E\f?\x06\x16E\x7f7??^[?\x03\?m???U?\x1e>MU\v GY?d)?ȼ??'g?~D??ɯhQ?\x13???/"E\x04??X?m???^??S?D\x13??;w6(?`??>?\x10縘?\aAѲ?*!??@v????>?8??2\b?\x14!??=*?J ^[\r\r???\x01ynH\x10???c?w?\??K7??\x1c?N?6??\x1c???A5?FM?wZ?~?pK\x02Y?R???s7\x7f??(?\aƶ?_"??m\x11%????\x7f1\x7fa??ʀ??K[\rt??\x11??\x0e!A0?ΈfT.?T?w\a?ƌ\v?р???aco?V/2\x14??nَ? ?}?6?\x19_?z?{ It might be worth ameloriating that attack by making git log always show the full buffer. Or it would be easy to write a tool that finds any commits that have a NULL in their message. -- see shy jo [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 828 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] push -s: skeleton 2011-09-09 16:03 ` Joey Hess @ 2011-09-09 16:14 ` Drew Northup 2011-09-09 19:12 ` Jeff King 1 sibling, 0 replies; 26+ messages in thread From: Drew Northup @ 2011-09-09 16:14 UTC (permalink / raw) To: Joey Hess; +Cc: Git Mailing List, Robin H. Johnson, Shawn O. Pearce, Jeff King On Fri, 2011-09-09 at 12:03 -0400, Joey Hess wrote: > It might be worth ameloriating that attack by making git log always > show the full buffer. Or it would be easy to write a tool that finds > any commits that have a NULL in their message. Just be aware that fixing the problem by diallowing NULLs in the commit message makes it impossible to include UTF-16 text in the commit message (which isn't currently handled very well anyway). Granted, I'm not sure what the most decent way of dealing with that otherwise might be as I've not taken the time to think about it... -- -Drew Northup ________________________________________________ "As opposed to vegetable or mineral error?" -John Pescatore, SANS NewsBites Vol. 12 Num. 59 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] push -s: skeleton 2011-09-09 16:03 ` Joey Hess 2011-09-09 16:14 ` Drew Northup @ 2011-09-09 19:12 ` Jeff King 1 sibling, 0 replies; 26+ messages in thread From: Jeff King @ 2011-09-09 19:12 UTC (permalink / raw) To: Joey Hess; +Cc: Git Mailing List On Fri, Sep 09, 2011 at 12:03:01PM -0400, Joey Hess wrote: > The most credible attack I have so far does not involve binary files in > tree. Someone pointed out that git log, git show, etc stop printing > commit messages at NULL. It was me. > It might be worth ameloriating that attack by making git log always > show the full buffer. Or it would be easy to write a tool that finds > any commits that have a NULL in their message. Unfortunately, that is going to involve a pretty huge code audit of git, as the "tack a \0 to the end of an object just in case" code dates back quite a while (e871b64, unpack_sha1_file: zero-pad the unpacked object, 2005-05-25). So I suspect there is a lot of code built on top of the assumption that commit messages are NUL-terminated strings. Besides which, that is only one form of hiding. If collision attacks against sha1 become a possibility, I think we are better to talk about moving to a new hash. Even sha-256 truncated to 160 bits would be better than sha-1 (AFAIK, that family of SHA does not suffer from the same attacks, so we would still be in the 2^80 range for collision attacks). -Peff ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 3/2] Split GPG interface into its own helper library 2011-09-07 20:57 ` [PATCH 2/2] push -s: skeleton Junio C Hamano ` (2 preceding siblings ...) 2011-09-07 23:55 ` Robin H. Johnson @ 2011-09-08 4:37 ` Junio C Hamano 2011-09-08 4:38 ` [PATCH 4/2] push -s: send signed push certificate Junio C Hamano 2011-09-08 19:35 ` [PATCH 2/2] push -s: skeleton Jeff King 5 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2011-09-08 4:37 UTC (permalink / raw) To: git; +Cc: Shawn O. Pearce This moves existing code from builtin/tag.c (for signing) and builtin/verify-tag.c (for verifying) to a new gpg-interface.c file to provide a more generic library interface. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Makefile | 2 + builtin/tag.c | 60 ++++---------------------------- builtin/verify-tag.c | 35 ++----------------- gpg-interface.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++ gpg-interface.h | 11 ++++++ 5 files changed, 117 insertions(+), 85 deletions(-) create mode 100644 gpg-interface.c create mode 100644 gpg-interface.h diff --git a/Makefile b/Makefile index 8d6d451..2183223 100644 --- a/Makefile +++ b/Makefile @@ -530,6 +530,7 @@ LIB_H += exec_cmd.h LIB_H += fsck.h LIB_H += gettext.h LIB_H += git-compat-util.h +LIB_H += gpg-interface.h LIB_H += graph.h LIB_H += grep.h LIB_H += hash.h @@ -620,6 +621,7 @@ LIB_OBJS += entry.o LIB_OBJS += environment.o LIB_OBJS += exec_cmd.o LIB_OBJS += fsck.o +LIB_OBJS += gpg-interface.o LIB_OBJS += graph.o LIB_OBJS += grep.o LIB_OBJS += hash.o diff --git a/builtin/tag.c b/builtin/tag.c index 667515e..e9d36fa 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -14,6 +14,7 @@ #include "parse-options.h" #include "diff.h" #include "revision.h" +#include "gpg-interface.h" static const char * const git_tag_usage[] = { "git tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]", @@ -208,60 +209,13 @@ static int verify_tag(const char *name, const char *ref, static int do_sign(struct strbuf *buffer) { - struct child_process gpg; - const char *args[4]; - char *bracket; - int len; - int i, j; + const char *key; - if (!*signingkey) { - if (strlcpy(signingkey, git_committer_info(IDENT_ERROR_ON_NO_NAME), - sizeof(signingkey)) > sizeof(signingkey) - 1) - return error(_("committer info too long.")); - bracket = strchr(signingkey, '>'); - if (bracket) - bracket[1] = '\0'; - } - - /* When the username signingkey is bad, program could be terminated - * because gpg exits without reading and then write gets SIGPIPE. */ - signal(SIGPIPE, SIG_IGN); - - memset(&gpg, 0, sizeof(gpg)); - gpg.argv = args; - gpg.in = -1; - gpg.out = -1; - args[0] = "gpg"; - args[1] = "-bsau"; - args[2] = signingkey; - args[3] = NULL; - - if (start_command(&gpg)) - return error(_("could not run gpg.")); - - if (write_in_full(gpg.in, buffer->buf, buffer->len) != buffer->len) { - close(gpg.in); - close(gpg.out); - finish_command(&gpg); - return error(_("gpg did not accept the tag data")); - } - close(gpg.in); - len = strbuf_read(buffer, gpg.out, 1024); - close(gpg.out); - - if (finish_command(&gpg) || !len || len < 0) - return error(_("gpg failed to sign the tag")); - - /* Strip CR from the line endings, in case we are on Windows. */ - for (i = j = 0; i < buffer->len; i++) - if (buffer->buf[i] != '\r') { - if (i != j) - buffer->buf[j] = buffer->buf[i]; - j++; - } - strbuf_setlen(buffer, j); - - return 0; + if (*signingkey) + key = signingkey; + else + key = git_committer_info(IDENT_ERROR_ON_NO_NAME|IDENT_NO_DATE); + return sign_buffer(buffer, key); } static const char tag_template[] = diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c index 3134766..8b4f742 100644 --- a/builtin/verify-tag.c +++ b/builtin/verify-tag.c @@ -11,6 +11,7 @@ #include "run-command.h" #include <signal.h> #include "parse-options.h" +#include "gpg-interface.h" static const char * const verify_tag_usage[] = { "git verify-tag [-v|--verbose] <tag>...", @@ -19,42 +20,12 @@ static const char * const verify_tag_usage[] = { static int run_gpg_verify(const char *buf, unsigned long size, int verbose) { - struct child_process gpg; - const char *args_gpg[] = {"gpg", "--verify", "FILE", "-", NULL}; - char path[PATH_MAX]; - size_t len; - int fd, ret; + int len; - fd = git_mkstemp(path, PATH_MAX, ".git_vtag_tmpXXXXXX"); - if (fd < 0) - return error("could not create temporary file '%s': %s", - path, strerror(errno)); - if (write_in_full(fd, buf, size) < 0) - return error("failed writing temporary file '%s': %s", - path, strerror(errno)); - close(fd); - - /* find the length without signature */ len = parse_signature(buf, size); if (verbose) write_in_full(1, buf, len); - - memset(&gpg, 0, sizeof(gpg)); - gpg.argv = args_gpg; - gpg.in = -1; - args_gpg[2] = path; - if (start_command(&gpg)) { - unlink(path); - return error("could not run gpg."); - } - - write_in_full(gpg.in, buf, len); - close(gpg.in); - ret = finish_command(&gpg); - - unlink_or_warn(path); - - return ret; + return verify_signed_buffer(buf, size, len); } static int verify_tag(const char *name, int verbose) diff --git a/gpg-interface.c b/gpg-interface.c new file mode 100644 index 0000000..b83cca1 --- /dev/null +++ b/gpg-interface.c @@ -0,0 +1,94 @@ +/* + * Copyright (c) 2011, Google Inc. + */ +#include "cache.h" +#include "run-command.h" +#include "strbuf.h" +#include "gpg-interface.h" +#include "sigchain.h" + +int sign_buffer(struct strbuf *buffer, const char *signing_key) +{ + struct child_process gpg; + const char *args[4]; + ssize_t len; + int i, j; + + memset(&gpg, 0, sizeof(gpg)); + gpg.argv = args; + gpg.in = -1; + gpg.out = -1; + args[0] = "gpg"; + args[1] = "-bsau"; + args[2] = signing_key; + args[3] = NULL; + + if (start_command(&gpg)) + return error(_("could not run gpg.")); + + /* + * When the username signingkey is bad, program could be terminated + * because gpg exits without reading and then write gets SIGPIPE. + */ + sigchain_push(SIGPIPE, SIG_IGN); + + if (write_in_full(gpg.in, buffer->buf, buffer->len) != buffer->len) { + close(gpg.in); + close(gpg.out); + finish_command(&gpg); + return error(_("gpg did not accept the data")); + } + close(gpg.in); + len = strbuf_read(buffer, gpg.out, 1024); + close(gpg.out); + + sigchain_pop(SIGPIPE); + + if (finish_command(&gpg) || !len || len < 0) + return error(_("gpg failed to sign the data")); + + /* Strip CR from the line endings, in case we are on Windows. */ + for (i = j = 0; i < buffer->len; i++) + if (buffer->buf[i] != '\r') { + if (i != j) + buffer->buf[j] = buffer->buf[i]; + j++; + } + strbuf_setlen(buffer, j); + + return 0; +} + +int verify_signed_buffer(const char *buf, size_t total, size_t payload) +{ + struct child_process gpg; + const char *args_gpg[] = {"gpg", "--verify", "FILE", "-", NULL}; + char path[PATH_MAX]; + int fd, ret; + + fd = git_mkstemp(path, PATH_MAX, ".git_vtag_tmpXXXXXX"); + if (fd < 0) + return error("could not create temporary file '%s': %s", + path, strerror(errno)); + if (write_in_full(fd, buf, total) < 0) + return error("failed writing temporary file '%s': %s", + path, strerror(errno)); + close(fd); + + memset(&gpg, 0, sizeof(gpg)); + gpg.argv = args_gpg; + gpg.in = -1; + args_gpg[2] = path; + if (start_command(&gpg)) { + unlink(path); + return error("could not run gpg."); + } + + write_in_full(gpg.in, buf, payload); + close(gpg.in); + ret = finish_command(&gpg); + + unlink_or_warn(path); + + return ret; +} diff --git a/gpg-interface.h b/gpg-interface.h new file mode 100644 index 0000000..7689357 --- /dev/null +++ b/gpg-interface.h @@ -0,0 +1,11 @@ +#ifndef GPG_INTERFACE_H +#define GPG_INTERFACE_H + +/* + * Copyright (c) 2011, Google Inc. + */ + +extern int sign_buffer(struct strbuf *buffer, const char *signing_key); +extern int verify_signed_buffer(const char *buffer, size_t total, size_t payload); + +#endif -- 1.7.7.rc0.188.g3793ac ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 4/2] push -s: send signed push certificate 2011-09-07 20:57 ` [PATCH 2/2] push -s: skeleton Junio C Hamano ` (3 preceding siblings ...) 2011-09-08 4:37 ` [PATCH 3/2] Split GPG interface into its own helper library Junio C Hamano @ 2011-09-08 4:38 ` Junio C Hamano 2011-09-08 5:38 ` [PATCH 5/2] push -s: receiving end Junio C Hamano 2011-09-08 19:35 ` [PATCH 2/2] push -s: skeleton Jeff King 5 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2011-09-08 4:38 UTC (permalink / raw) To: git; +Cc: Shawn O. Pearce And this uses the GPG interface to sign the push certificate. The format of the signed certificate is very similar to a signed tag, in that the result is a concatenation of the payload, immediately followed by a detached signature. This places the same constraint as an annotated tag on the push certificate payload; it has to be a text file and the final line must not be an incomplete line. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin/send-pack.c | 29 ++++++++++++----------------- 1 files changed, 12 insertions(+), 17 deletions(-) diff --git a/builtin/send-pack.c b/builtin/send-pack.c index 3193f34..298e181 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -8,6 +8,7 @@ #include "send-pack.h" #include "quote.h" #include "transport.h" +#include "gpg-interface.h" static const char send_pack_usage[] = "git send-pack [--all | --mirror] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [<host>:]<directory> [<ref>...]\n" @@ -237,25 +238,18 @@ static int sideband_demux(int in, int out, void *data) return ret; } -static void sign_push_certificate(struct strbuf *cert) +/* + * Take the contents of cert->buf, and have the user GPG sign it, and + * read it back in the strbuf. + */ +static int sign_push_certificate(struct strbuf *cert) { /* - * Here, take the contents of cert->buf, and have the user GPG - * sign it, and read it back in the strbuf. - * - * You may want to append some extra info to cert before giving - * it to GPG, possibly via a hook. - * - * Here we upcase them just to demonstrate that the codepath - * is being exercised. + * You may want to append some extra info to cert before + * giving it to GPG, possibly via a hook, here. */ - char *cp; - for (cp = cert->buf; *cp; cp++) { - int ch = *cp; - if ('a' <= ch && ch <= 'z') - *cp = toupper(ch); - } - return; + + return sign_buffer(cert, git_committer_info(IDENT_NO_DATE)); } int send_pack(struct send_pack_args *args, @@ -370,7 +364,8 @@ int send_pack(struct send_pack_args *args, if (signed_push) { char *cp, *ep; - sign_push_certificate(&push_cert); + if (sign_push_certificate(&push_cert)) + return error(_("failed to sign push certificate")); strbuf_reset(&req_buf); for (cp = push_cert.buf; *cp; cp = ep) { ep = strchrnul(cp, '\n'); -- 1.7.7.rc0.188.g3793ac ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 5/2] push -s: receiving end 2011-09-08 4:38 ` [PATCH 4/2] push -s: send signed push certificate Junio C Hamano @ 2011-09-08 5:38 ` Junio C Hamano 2011-09-08 9:31 ` Johan Herland 0 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2011-09-08 5:38 UTC (permalink / raw) To: git; +Cc: Shawn O. Pearce, Johan Herland This stores the GPG signed push certificate in the receiving repository using the notes mechanism. The certificate is appended to a note in the refs/notes/signed-push tree for each object that appears on the right hand side of the push certificate, i.e. the object that was pushed to update the tip of a ref. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * This is largely untested, so take it with a large grain of salt. This concludes tonight's hacking session for me. builtin/receive-pack.c | 74 +++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 67 insertions(+), 7 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 307fc3b..257f2a5 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -11,6 +11,11 @@ #include "transport.h" #include "string-list.h" #include "sha1-array.h" +#include "gpg-interface.h" +#include "notes.h" +#include "notes-merge.h" +#include "blob.h" +#include "tag.h" static const char receive_pack_usage[] = "git receive-pack <git-dir>"; @@ -156,6 +161,7 @@ struct command { }; static const char pre_receive_hook[] = "hooks/pre-receive"; +static const char pre_receive_signature_hook[] = "hooks/pre-receive-signature"; static const char post_receive_hook[] = "hooks/post-receive"; static void rp_error(const char *err, ...) __attribute__((format (printf, 1, 2))); @@ -581,6 +587,22 @@ static void check_aliased_updates(struct command *commands) string_list_clear(&ref_list, 0); } +static void get_note_text(struct strbuf *buf, struct notes_tree *t, + const unsigned char *object) +{ + const unsigned char *sha1 = get_note(t, object); + char *text; + unsigned long len; + enum object_type type; + + if (!sha1) + return; + text = read_sha1_file(sha1, &type, &len); + if (text && len && type == OBJ_BLOB) + strbuf_add(buf, text, len); + free(text); +} + static int record_signed_push(char *cert) { /* @@ -591,19 +613,57 @@ static int record_signed_push(char *cert) * * You could also feed the signed push certificate to GPG, * verify the signer identity, and all the other fun stuff, - * including feeding it to "pre-push-verify-signature" hook. - * - * Here we just throw it to stderr to demonstrate that the - * codepath is being exercised. + * including feeding it to "pre-receive-signature" hook. */ + size_t total, payload; char *cp, *ep; - for (cp = cert; *cp; cp = ep) { + int ret = 0; + struct notes_tree *t; + struct strbuf nbuf = STRBUF_INIT; + + init_notes(NULL, "refs/notes/signed-push", NULL, 0); + t = &default_notes_tree; + + total = strlen(cert); + payload = parse_signature(cert, total); + for (cp = cert; cp < cert + payload; cp = ep) { + unsigned char sha1[20], nsha1[20]; + ep = strchrnul(cp, '\n'); if (*ep == '\n') ep++; - fprintf(stderr, "RSP: %.*s", (int)(ep - cp), cp); + if (prefixcmp(cp, "Update: ")) + continue; + cp += strlen("Update: "); + if (get_sha1_hex(cp, sha1) || cp[40] != ' ') + continue; + cp += 41; + if (get_sha1_hex(cp, sha1) || cp[40] != ' ') + continue; + + get_note_text(&nbuf, t, sha1); + if (nbuf.len) + strbuf_addch(&nbuf, '\n'); + strbuf_add(&nbuf, cert, total); + if (write_sha1_file(nbuf.buf, nbuf.len, blob_type, nsha1) || + add_note(t, sha1, nsha1, NULL)) + ret = error(_("unable to write note object")); + strbuf_reset(&nbuf); } - return 0; + + if (!ret) { + unsigned char commit[20]; + unsigned char parent[20]; + struct ref_lock *lock; + + resolve_ref(t->ref, parent, 0, NULL); + lock = lock_any_ref_for_update(t->ref, parent, 0); + create_notes_commit(t, NULL, "push", commit); + ret = write_ref_sha1(lock, commit, "push"); + } + free_notes(t); + + return ret; } static void execute_commands(struct command *commands, const char *unpacker_error) -- 1.7.7.rc0.188.g3793ac ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 5/2] push -s: receiving end 2011-09-08 5:38 ` [PATCH 5/2] push -s: receiving end Junio C Hamano @ 2011-09-08 9:31 ` Johan Herland 2011-09-08 16:43 ` Junio C Hamano 0 siblings, 1 reply; 26+ messages in thread From: Johan Herland @ 2011-09-08 9:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Shawn O. Pearce On Thursday 8. September 2011, Junio C Hamano wrote: > This stores the GPG signed push certificate in the receiving > repository using the notes mechanism. The certificate is appended to > a note in the refs/notes/signed-push tree for each object that > appears on the right hand side of the push certificate, i.e. the > object that was pushed to update the tip of a ref. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- (...) > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c > index 307fc3b..257f2a5 100644 > --- a/builtin/receive-pack.c > +++ b/builtin/receive-pack.c > @@ -156,6 +161,7 @@ struct command { > }; > > static const char pre_receive_hook[] = "hooks/pre-receive"; > +static const char pre_receive_signature_hook[] = > "hooks/pre-receive-signature"; static const char post_receive_hook[] > = "hooks/post-receive"; > > static void rp_error(const char *err, ...) __attribute__((format > (printf, 1, 2))); @@ -581,6 +587,22 @@ static void > check_aliased_updates(struct command *commands) > string_list_clear(&ref_list, 0); > } > > +static void get_note_text(struct strbuf *buf, struct notes_tree *t, > + const unsigned char *object) > +{ > + const unsigned char *sha1 = get_note(t, object); > + char *text; > + unsigned long len; > + enum object_type type; > + > + if (!sha1) > + return; > + text = read_sha1_file(sha1, &type, &len); > + if (text && len && type == OBJ_BLOB) > + strbuf_add(buf, text, len); > + free(text); > +} > + What about adding this function to notes.h as a convenience to other users of the notes API? Otherwise the code looks good to me. Have fun! :) ...Johan -- Johan Herland, <johan@herland.net> www.herland.net ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/2] push -s: receiving end 2011-09-08 9:31 ` Johan Herland @ 2011-09-08 16:43 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2011-09-08 16:43 UTC (permalink / raw) To: Johan Herland; +Cc: git, Shawn O. Pearce Johan Herland <johan@herland.net> writes: >> +static void get_note_text(struct strbuf *buf, struct notes_tree *t, >> + const unsigned char *object) >> +{ >> + const unsigned char *sha1 = get_note(t, object); >> + char *text; >> + unsigned long len; >> + enum object_type type; >> + >> + if (!sha1) >> + return; >> + text = read_sha1_file(sha1, &type, &len); >> + if (text && len && type == OBJ_BLOB) >> + strbuf_add(buf, text, len); >> + free(text); >> +} >> + > > What about adding this function to notes.h as a convenience to other > users of the notes API? I actually was hoping to hear that I do not have to do this "check existing and concatenate", and should let the add_note() function run its default combine_notes method to do the concatenation. I found a few things I wasn't quite sure in the notes/notes-merge API, by the way. - The combine_notes callback is run when a note is inserted into the in-core notes tree. I felt that this is way too early if you want to avoid racing with another process (and the patch tries to wrap create-notes-commit with lock-ref/write-ref-sha1 pair), but perhaps this is to deal with a case where the calling program calls add_notes() on the same object multiple times. - create_notes_commit() dies under a few conditions, but some callers that are recording advisory/optional notes might want to get an error and continue. I think ideally this patch should handle notes like the following, which is not quite how I coded it: - initialize in-core notes tree; - add bunch of notes, without regard to the existing ones, to in-core notes tree by calling add_notes(); - lock the notes ref and read the "parent"; we may want to add "wait and retry for a few times until we get the lock" support at lockfile API level, but doing it at the application level would be fine. - call create-notes-commit, which in turn merges the in-core notes with what collides with those already in "parent" by calling the combine-notes callback, merges and re-balances the notes tree, and makes a notes commit object; - update the notes ref with that notes commit, releasing the lock on the ref. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] push -s: skeleton 2011-09-07 20:57 ` [PATCH 2/2] push -s: skeleton Junio C Hamano ` (4 preceding siblings ...) 2011-09-08 4:38 ` [PATCH 4/2] push -s: send signed push certificate Junio C Hamano @ 2011-09-08 19:35 ` Jeff King 2011-09-08 20:48 ` Junio C Hamano 5 siblings, 1 reply; 26+ messages in thread From: Jeff King @ 2011-09-08 19:35 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Shawn O. Pearce On Wed, Sep 07, 2011 at 01:57:27PM -0700, Junio C Hamano wrote: > If a tag is GPG-signed, and if you trust the cryptographic robustness of > the SHA-1 and GPG, you can guarantee that all the history leading to the > signed commit is not tampered with. However, it would be both cumbersome > and cluttering to sign each and every commit. Especially if you strive to > keep your history clean by tweaking, rewriting and polishing your commits > before pushing the resulting history out, many commits you will create > locally end up not mattering at all, and it is a waste of time to sign > them. > > A better alternative could be to sign a "push certificate" (for the lack > of better name) every time you push, asserting that what commits you are > pushing to update which refs. The basic workflow goes like this: I think this is the right direction, but I was a little turned off by the idea that it needs a protocol extension. As I see it, there are two ways to care about the contents of a push certificate: 1. The server might care, because it only wants to accept pushes that are accompanied by a certificate matching a certain key. 2. A client fetching from the server might care, because they want the integrity and authenticity of the data to be ensured by the gpg key of the pusher, not by trusting the server. I think (1) is actually not all that interesting. The server already has credentials for each user via ssh or http. So it knows who each pusher is already. It can't relay that information cryptographically to a client who fetches later, of course, but we are just talking about whether or not to accept the push at this moment. But if you really did want to do that, it seems like a pre-receive hook would be sufficient. For (2), you don't want to trust the server, so the user's authentication to the server isn't enough. You want a cryptographic chain leading back to the original pusher. But the server doesn't actually need to see or understand that cryptographic chain for this purpose. If it were stored in a notes-tree or other format pointed to by a ref, then a client could pull down those notes and do the verification themselves. Which means you can start using this immediately, without having to care about whether your hosting provider supports it or not (or even whether your provider supports the git protocol. Such a system would Just Work across dumb http, local clones, sneakernet bundles, etc). The only issue I foresee is one of atomicity. IIRC, we never have a whole-repo lock during push, so it's possible that a client might succeed in pushing the ref with the certificate, but fail at one or more refs that the certificate mentions. And maybe a protocol extension is required for that. You've looked much more closely at this than I have, so maybe you already considered something simpler. -Peff ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] push -s: skeleton 2011-09-08 19:35 ` [PATCH 2/2] push -s: skeleton Jeff King @ 2011-09-08 20:48 ` Junio C Hamano 2011-09-08 21:02 ` Jeff King 0 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2011-09-08 20:48 UTC (permalink / raw) To: Jeff King; +Cc: git, Shawn O. Pearce Jeff King <peff@peff.net> writes: > I think (1) is actually not all that interesting. The server already has > credentials for each user via ssh or http. So it knows who each pusher > is already. It can't relay that information cryptographically to a > client who fetches later, of course, but we are just talking about > whether or not to accept the push at this moment. > > But if you really did want to do that, it seems like a pre-receive hook > would be sufficient. I see two flaws in that reasoning. The server's authentication may be found not trustworthy for some reason long after commits hit the tree, and GPG signature made by the _pusher_ would assert the integrity. Also this will open the door to accept push over an unauthenticated connection and allowing only signed pushes. > For (2), you don't want to trust the server, so the user's > authentication to the server isn't enough. You want a cryptographic > chain leading back to the original pusher. But the server doesn't > actually need to see or understand that cryptographic chain for this > purpose. Exactly. That is why the signed push certificate is stored without the server doing anything funky, only to annotate the pushed commits in the notes tree---the fetchers can peek the notes and verify the GPG signature. But not _forcing_ that the push certificate be placed in a notes tree on the client side allows different server hosting sites to additionally do different things using that data. > The only issue I foresee is one of atomicity. The very initial thinking was to create a notes tree commit on the client side and push that along with what is pushed, but that approach has an inherent flaw of causing unnecessary collisions between two people who are pushing to unrelated branches, and that is why I decided to let the server side handle it. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] push -s: skeleton 2011-09-08 20:48 ` Junio C Hamano @ 2011-09-08 21:02 ` Jeff King 2011-09-08 22:19 ` Junio C Hamano [not found] ` <CAJo=hJsQvRN3Z0xJg9q37Km1g_1qUdJKNQ6n8=a9mv3YjugyVw@mail.gmail.com> 0 siblings, 2 replies; 26+ messages in thread From: Jeff King @ 2011-09-08 21:02 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Shawn O. Pearce On Thu, Sep 08, 2011 at 01:48:07PM -0700, Junio C Hamano wrote: > > I think (1) is actually not all that interesting. The server already has > > credentials for each user via ssh or http. So it knows who each pusher > > is already. It can't relay that information cryptographically to a > > client who fetches later, of course, but we are just talking about > > whether or not to accept the push at this moment. > > > > But if you really did want to do that, it seems like a pre-receive hook > > would be sufficient. > > I see two flaws in that reasoning. The server's authentication may be > found not trustworthy for some reason long after commits hit the tree, and > GPG signature made by the _pusher_ would assert the integrity. Right, but that is not about (1), but rather about (2). IOW, there are two times to authenticate: at the moment you take the push, and every moment thereafter, no matter who you are. But I think we are just splitting hairs. We both agree that signed pushes are a good thing. When I said "a pre-receive hook is sufficient", I emphatically _didn't_ mean that the server should sign in the hook, or create a cryptographic trail starting there. I meant that git doesn't need to carry the code internally, and that a pre-receive hook can check the push certificate itself, just as a client would. BTW, is the name "push certificate" right? It seems like they are not necessarily about pushing, but about signing "I would like to move ref X from Y to Z". Is there value to making such signatures locally (i.e., not over the git protocol, but such that you could later check the integrity of what's on the disk cryptographically)? Would it not be possible to generate this information in one step, and then have a remote fetch from you, with no pushes at all? > Also this will open the door to accept push over an unauthenticated > connection and allowing only signed pushes. Yes, though a pre-receive hook could do that, too. > Exactly. That is why the signed push certificate is stored without the > server doing anything funky, only to annotate the pushed commits in the > notes tree---the fetchers can peek the notes and verify the GPG signature. > But not _forcing_ that the push certificate be placed in a notes tree on > the client side allows different server hosting sites to additionally do > different things using that data. Hmm. So you seem to take the approach of: 1. Client needs only know "I am pushing with a signed cert". 2. Server can convert that signed cert into other formats as they see fit, including breaking the signature out into a notes ref. 3. Other clients fetch from the server, seeing the notes ref (or not). But that seems backwards to me. In a decentralized system, the endpoints are what is important. So the pushing client in 1 should be the one deciding what other clients see, no? Otherwise, if I care about what's in the notes tree, I have to care whether I am pulling from kernel.org or github.com, or whatever. The server stops being dumb storage. > > The only issue I foresee is one of atomicity. > > The very initial thinking was to create a notes tree commit on the client > side and push that along with what is pushed, but that approach has an > inherent flaw of causing unnecessary collisions between two people who are > pushing to unrelated branches, and that is why I decided to let the server > side handle it. Yeah, it is a potential problem, but it just seems wrong to put too much policy work onto the server. Perhaps it would make more sense to keep one notes tree per ref, which would also resolve that locking issue? -Peff ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] push -s: skeleton 2011-09-08 21:02 ` Jeff King @ 2011-09-08 22:19 ` Junio C Hamano 2011-09-09 15:34 ` Jeff King [not found] ` <CAJo=hJsQvRN3Z0xJg9q37Km1g_1qUdJKNQ6n8=a9mv3YjugyVw@mail.gmail.com> 1 sibling, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2011-09-08 22:19 UTC (permalink / raw) To: Jeff King; +Cc: git, Shawn O. Pearce Jeff King <peff@peff.net> writes: > Yeah, it is a potential problem, but it just seems wrong to put too much > policy work onto the server. My take on it is somewhat different. The only thing in the end result we want to see is that the pushed commits are annotated with GPG signatures in the notes tree, and there is no reason for us to cast in stone that there has to be any significance in the commit history of the notes tree. In a busy hosting site that has many branches being pushed simultaneously, it is entirely plausible that the server side may just want to store each received push certificate in a new flat file in a filesystem, and have asynchronous process sweep the new certificates to update the notes tree, possibly creating a single notes tree commit that records updates by multiple pushes, for performance purposes, in its implementation of record_signed_push() in receive-pack. If you forced the clients to also prepare notes and push the notes tree to the server, you are forcing the ordering in the history of the notes, and closing the door for such a server implementation. I would consider it an unnecessary and/or premature policy decision. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] push -s: skeleton 2011-09-08 22:19 ` Junio C Hamano @ 2011-09-09 15:34 ` Jeff King 2011-09-09 17:32 ` Junio C Hamano 0 siblings, 1 reply; 26+ messages in thread From: Jeff King @ 2011-09-09 15:34 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Shawn O. Pearce On Thu, Sep 08, 2011 at 03:19:54PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > Yeah, it is a potential problem, but it just seems wrong to put too much > > policy work onto the server. > > My take on it is somewhat different. The only thing in the end result we > want to see is that the pushed commits are annotated with GPG signatures > in the notes tree, and there is no reason for us to cast in stone that > there has to be any significance in the commit history of the notes tree. Hmm. Is order really irrelevant? If you push a commit to master, moving it from X to Y, then push-rewind it back to X, then push a new commit Z, how do I cryptographically determine the correct final state of master? I'll see two push-certs, one going X..Y and one X..Z. They'll have timestamps, which can be used for ordering. But what if the first and third actions above are done by different people. Now you're trusting that their clocks are synced to order them properly. Note that simply keeping an unsigned but ordered notes history doesn't solve this problem, either; you'd probably want a parent pointer in your push cert saying "and this is the previous push cert I am based on". And maybe this is a use case we don't care about. Maybe it's enough for the push-cert to say "at some point in time, I thought it was a good idea to push these commits into master; signed, me". But I'm not really clear on exactly what the security goals are. The series you sent looks interesting, but I haven't seen the verification side of these signatures. What are they going to be used for? What guarantees are we attempting to provide? For that matter, what is our threat model? What are attackers capable of? > In a busy hosting site that has many branches being pushed simultaneously, > it is entirely plausible that the server side may just want to store each > received push certificate in a new flat file in a filesystem, and have > asynchronous process sweep the new certificates to update the notes tree, > possibly creating a single notes tree commit that records updates by > multiple pushes, for performance purposes, in its implementation of > record_signed_push() in receive-pack. OK, I see. It is not "the server can do whatever it likes with the information" as much as "the server can do whatever it likes, but at the very least should eventually create a notes tree of a given form". -Peff ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] push -s: skeleton 2011-09-09 15:34 ` Jeff King @ 2011-09-09 17:32 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2011-09-09 17:32 UTC (permalink / raw) To: Jeff King; +Cc: git, Shawn O. Pearce Jeff King <peff@peff.net> writes: > On Thu, Sep 08, 2011 at 03:19:54PM -0700, Junio C Hamano wrote: > >> My take on it is somewhat different. The only thing in the end result we >> want to see is that the pushed commits are annotated with GPG signatures >> in the notes tree, and there is no reason for us to cast in stone that >> there has to be any significance in the commit history of the notes tree. > > Hmm. Is order really irrelevant? If you push a commit to master, moving > it from X to Y, then push-rewind it back to X, then push a new commit Z, > how do I cryptographically determine the correct final state of master? You don't, as the certs are more about "up to this point, the pusher trusts the history". I should have made it clearer in the cover letter to the rerolled series, but the push certificate does not record the old value of the ref in the reroll, because the point of signed-push is not about signing the information that is equivalent to the server side reflog. You would have a signed push record that pushed Y, X and Z, and commit Z sitting at the tip of 'master'. A few days may pass and then you run $ git log --show-notes=refs/notes/push-signature master to find that the first commit with a push signature by somebody whose judgement you trust is Z. Then you would need to inspect only commits that are not ancestors of Z even if you suspect that some commits near the tip of 'master' at the server side were tampered with. You may at the same time find commits signed by the trusted people that are meant for the same branch but are not contained in the history of 'master' (e.g. Y), which might indicate that the branch was rewound, possibly by an intruder. Another possible scenario. Later you and the pusher of Z may find that when the pusher created Z, he merged something questionable and Z may now have to be in "untrustable" set. You can dig further to find X at that point. > OK, I see. It is not "the server can do whatever it likes with the > information" as much as "the server can do whatever it likes, but at the > very least should eventually create a notes tree of a given form". Yes, examples of things the server side might want to additionally do in pre-receive-signature hook are to read the push certificate to implement authorization (and it can be per-branch if you wanted to) and to forward it immediately to offsite storage for safekeeping (the storage does not have to use git notes to implement it). ^ permalink raw reply [flat|nested] 26+ messages in thread
[parent not found: <CAJo=hJsQvRN3Z0xJg9q37Km1g_1qUdJKNQ6n8=a9mv3YjugyVw@mail.gmail.com>]
* Re: [PATCH 2/2] push -s: skeleton [not found] ` <CAJo=hJsQvRN3Z0xJg9q37Km1g_1qUdJKNQ6n8=a9mv3YjugyVw@mail.gmail.com> @ 2011-09-09 15:22 ` Jeff King 0 siblings, 0 replies; 26+ messages in thread From: Jeff King @ 2011-09-09 15:22 UTC (permalink / raw) To: Shawn Pearce; +Cc: Junio C Hamano, git On Thu, Sep 08, 2011 at 03:07:25PM -0700, Shawn O. Pearce wrote: > A notes tree per ref is ugly when you have a lot of branches. Its a > problem when you merge commits from say "maint" over to "master" 2 > days after they were initially pushed. Ideally the notes tree for > master also includes what you brought over. I agree you can end up with a lot of refs if you have a lot of branches, and that may get a bit unwieldy. I don't see how the merge thing is a problem, though. If you do the merge at a client, then those commits will hit master by push, and you'll get entries in the cert tree for master. If you do the merge locally, then there's not going to be a signature on those commits hitting the master ref, no matter what the storage scheme. > However, maybe it is reasonable for a protocol extension to support > "auto-notes-merge" on a refs/notes/ ref if the client asks for it in > the send-pack/receive-pack command stream? Clients could push notes > and have the server automatically merge the client's pushed commit > into the notes tree, either by fast-forward or by performing an > automatic notes merge, with concat being applied to non-identical > notes on the same SHA-1. This would allow the client to prepare his > local certificate, and push that notes tree, while still working > around the race on the server side. > > If the server doesn't support the protocol extension, the client can > still push his signed notes, he just may run into a race with another > concurrent user pushing into the same repository. Which then means > that an upgrade of the server is really only important/necessary if > you have "central repository" model that a lot of users push into. If > you use the traditional workflow that GitHub encourages of per-user > repositories, you would never have a race on the server, and wouldn't > need the upgraded server binary with "auto-notes-merge". I like this approach, as the protocol extension provides the minimal building block that can be used to implement this, or any other notes-related scheme. -Peff ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2011-09-09 19:12 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-09-07 20:56 [PATCH 1/2] send-pack: typofix error message Junio C Hamano 2011-09-07 20:57 ` [PATCH 2/2] push -s: skeleton Junio C Hamano 2011-09-07 21:18 ` Shawn Pearce 2011-09-07 22:21 ` Junio C Hamano 2011-09-07 23:23 ` Shawn Pearce 2011-09-08 16:24 ` Junio C Hamano 2011-09-07 22:21 ` Nguyen Thai Ngoc Duy 2011-09-07 22:40 ` Junio C Hamano 2011-09-07 23:55 ` Robin H. Johnson 2011-09-08 20:03 ` Jeff King 2011-09-09 1:30 ` Robin H. Johnson 2011-09-09 16:03 ` Joey Hess 2011-09-09 16:14 ` Drew Northup 2011-09-09 19:12 ` Jeff King 2011-09-08 4:37 ` [PATCH 3/2] Split GPG interface into its own helper library Junio C Hamano 2011-09-08 4:38 ` [PATCH 4/2] push -s: send signed push certificate Junio C Hamano 2011-09-08 5:38 ` [PATCH 5/2] push -s: receiving end Junio C Hamano 2011-09-08 9:31 ` Johan Herland 2011-09-08 16:43 ` Junio C Hamano 2011-09-08 19:35 ` [PATCH 2/2] push -s: skeleton Jeff King 2011-09-08 20:48 ` Junio C Hamano 2011-09-08 21:02 ` Jeff King 2011-09-08 22:19 ` Junio C Hamano 2011-09-09 15:34 ` Jeff King 2011-09-09 17:32 ` Junio C Hamano [not found] ` <CAJo=hJsQvRN3Z0xJg9q37Km1g_1qUdJKNQ6n8=a9mv3YjugyVw@mail.gmail.com> 2011-09-09 15:22 ` Jeff King
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.