Message ID | 7c8502c65b833e7e563a833b592f6932421b1056.1627501009.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | ssh signing: Add commit & tag signing/verification via SSH keys using ssh-keygen | expand |
I think this patch set is beyond the "is this a good idea in general" phase (in particular, I think that being able to sign Git commits by using SSH infrastructure is very useful), so I'll proceed to critiquing the commits in more detail. Firstly, in commit messages, the left side of the colon is usually the name of the subsystem - in this case, "gpg-interface". > To be able to implement new signing formats this commit: > - makes the sigc structure more generic by renaming "gpg_output" to > "output" > - introduces function pointers in the gpg_format structure to call > format specific signing and verification functions > - moves format detection from verify_signed_buffer into the check_signature > api function and calls the format specific verify > - renames and wraps sign_buffer to handle format specific signing logic > as well I think that this commit should be further split up - in particular, it is hard for reviewers to verify that there is no difference in functionality before and after this commit. I already spotted one difference - perhaps there are more. For me, splitting the above 4 points into 4 commits would be an acceptable split. > diff --git a/gpg-interface.c b/gpg-interface.c > index 127aecfc2b0..31cf4ba3938 100644 > --- a/gpg-interface.c > +++ b/gpg-interface.c > @@ -15,6 +15,12 @@ struct gpg_format { > const char *program; > const char **verify_args; > const char **sigs; > + int (*verify_signed_buffer)(struct signature_check *sigc, > + struct gpg_format *fmt, const char *payload, > + size_t payload_size, const char *signature, > + size_t signature_size); > + int (*sign_buffer)(struct strbuf *buffer, struct strbuf *signature, > + const char *signing_key); > }; [snip] > static struct gpg_format gpg_format[] = { > - { .name = "openpgp", .program = "gpg", > - .verify_args = openpgp_verify_args, > - .sigs = openpgp_sigs > + { > + .name = "openpgp", > + .program = "gpg", > + .verify_args = openpgp_verify_args, > + .sigs = openpgp_sigs, > + .verify_signed_buffer = verify_gpg_signed_buffer, > + .sign_buffer = sign_buffer_gpg, > }, > - { .name = "x509", .program = "gpgsm", > - .verify_args = x509_verify_args, > - .sigs = x509_sigs > + { > + .name = "x509", > + .program = "gpgsm", > + .verify_args = x509_verify_args, > + .sigs = x509_sigs, > + .verify_signed_buffer = verify_gpg_signed_buffer, > + .sign_buffer = sign_buffer_gpg, > }, > }; I think that verify_signed_buffer and sign_buffer should replace verify_args and sigs, not be alongside them. In particular, I see from later patches that a new entry will be introduced for SSH, and the corresponding new "verify" function does not use verify_args or sigs. > @@ -279,10 +300,6 @@ static int verify_signed_buffer(const char *payload, size_t payload_size, > return -1; > } > > - fmt = get_format_by_sig(signature); > - if (!fmt) > - BUG("bad signature '%s'", signature); Here is the difference in functionality that I spotted. Here, lack of fmt is fatal... > @@ -309,35 +330,32 @@ static int verify_signed_buffer(const char *payload, size_t payload_size, > int check_signature(const char *payload, size_t plen, const char *signature, > size_t slen, struct signature_check *sigc) > { > - struct strbuf gpg_output = STRBUF_INIT; > - struct strbuf gpg_status = STRBUF_INIT; > + struct gpg_format *fmt; > int status; > > sigc->result = 'N'; > sigc->trust_level = -1; > > - status = verify_signed_buffer(payload, plen, signature, slen, > - &gpg_output, &gpg_status); > - if (status && !gpg_output.len) > - goto out; > - sigc->payload = xmemdupz(payload, plen); > - sigc->gpg_output = strbuf_detach(&gpg_output, NULL); > - sigc->gpg_status = strbuf_detach(&gpg_status, NULL); > - parse_gpg_output(sigc); > + fmt = get_format_by_sig(signature); > + if (!fmt) > + return error(_("bad/incompatible signature '%s'"), signature); ...but here it is not.
Jonathan Tan <jonathantanmy@google.com> writes: >> - fmt = get_format_by_sig(signature); >> - if (!fmt) >> - BUG("bad signature '%s'", signature); > > Here is the difference in functionality that I spotted. Here, lack of > fmt is fatal... > >> + fmt = get_format_by_sig(signature); >> + if (!fmt) >> + return error(_("bad/incompatible signature '%s'"), signature); > > ...but here it is not. While I was reviewing this step, I was assumign that the callers would respond to this error return appropriately. If it is not the case, then we do have to fix that. The original's use of BUG() is wrong in any case, I woud think. The "signature" there is an external input, so we were reporting a data error (it should have been die()), not a program logic error. Thanks.
On 29.07.21 02:58, Junio C Hamano wrote: > Jonathan Tan <jonathantanmy@google.com> writes: > >>> - fmt = get_format_by_sig(signature); >>> - if (!fmt) >>> - BUG("bad signature '%s'", signature); >> >> Here is the difference in functionality that I spotted. Here, lack of >> fmt is fatal... >> >>> + fmt = get_format_by_sig(signature); >>> + if (!fmt) >>> + return error(_("bad/incompatible signature '%s'"), signature); >> >> ...but here it is not. > > While I was reviewing this step, I was assumign that the callers > would respond to this error return appropriately. If it is not the > case, then we do have to fix that. > > The original's use of BUG() is wrong in any case, I woud think. The > "signature" there is an external input, so we were reporting a data > error (it should have been die()), not a program logic error. > > Thanks. > My intention was to actually change this behavior (i should have made that clear in the commit message). When the current git version encounters an unknown signature format it will BUG() and leave the user with a coredump. I assume that some repos will have multiple different signature formats in their history in the future and the effect i would have liked was to only mark the commits with unknown signatures as bad/unknown when using git log --show-signature for example. I have checked all the calls to check_signature and unfortunately the result check is really inconsistent. Some ignore it completely, others check but still then dereference fields from the sigcheck struct. So for now a die() is probably the correct way to go. In the future we might want to differentiate between verifying a single commit (in which case we can die()) and a list of commits. Or fix all the calls to check_signature to check the return code. Thanks
On 29.07.21 00:32, Jonathan Tan wrote: > I think this patch set is beyond the "is this a good idea in general" > phase (in particular, I think that being able to sign Git commits by > using SSH infrastructure is very useful), so I'll proceed to critiquing > the commits in more detail. Thanks for your help. > > Firstly, in commit messages, the left side of the colon is usually the > name of the subsystem - in this case, "gpg-interface". > The docs call this "name of the component you're working on". Since this code does not actually change any gpg functionality (at least it should not) i think gpg-interface in the commits might be a bit misleading. >> To be able to implement new signing formats this commit: >> - makes the sigc structure more generic by renaming "gpg_output" to >> "output" >> - introduces function pointers in the gpg_format structure to call >> format specific signing and verification functions >> - moves format detection from verify_signed_buffer into the check_signature >> api function and calls the format specific verify >> - renames and wraps sign_buffer to handle format specific signing logic >> as well > > I think that this commit should be further split up - in particular, it > is hard for reviewers to verify that there is no difference in > functionality before and after this commit. I already spotted one > difference - perhaps there are more. For me, splitting the above 4 > points into 4 commits would be an acceptable split. > The rename can of course be easily separated. The others would probably require some code in between commits that's not present in the final patch result to make the individual commits compile / work. Otherwise those would only add unused code with the last commit then actually using everything. I don't think that would make things easier to verify, would it? > [snip] > >> static struct gpg_format gpg_format[] = { >> - { .name = "openpgp", .program = "gpg", >> - .verify_args = openpgp_verify_args, >> - .sigs = openpgp_sigs >> + { >> + .name = "openpgp", >> + .program = "gpg", >> + .verify_args = openpgp_verify_args, >> + .sigs = openpgp_sigs, >> + .verify_signed_buffer = verify_gpg_signed_buffer, >> + .sign_buffer = sign_buffer_gpg, >> }, >> - { .name = "x509", .program = "gpgsm", >> - .verify_args = x509_verify_args, >> - .sigs = x509_sigs >> + { >> + .name = "x509", >> + .program = "gpgsm", >> + .verify_args = x509_verify_args, >> + .sigs = x509_sigs, >> + .verify_signed_buffer = verify_gpg_signed_buffer, >> + .sign_buffer = sign_buffer_gpg, >> }, >> }; > > I think that verify_signed_buffer and sign_buffer should replace > verify_args and sigs, not be alongside them. In particular, I see from > later patches that a new entry will be introduced for SSH, and the > corresponding new "verify" function does not use verify_args or sigs. > I kept the verify_args since i would either have to duplicate the verify_gpg_signed_buffer for gpg & gpgsm or have an if within deciding what format to use. Also this is something that we might want to make a configuration option in the future and pass to ssh-keygen as well (there are a couple of -O options for it users might want) sigs is still needed for the parse_signed_buffer api function.
diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c index 0f66818e0f8..fb300bb4b67 100644 --- a/fmt-merge-msg.c +++ b/fmt-merge-msg.c @@ -526,11 +526,11 @@ static void fmt_merge_msg_sigs(struct strbuf *out) buf = payload.buf; len = payload.len; if (check_signature(payload.buf, payload.len, sig.buf, - sig.len, &sigc) && - !sigc.gpg_output) + sig.len, &sigc) && + !sigc.output) strbuf_addstr(&sig, "gpg verification failed.\n"); else - strbuf_addstr(&sig, sigc.gpg_output); + strbuf_addstr(&sig, sigc.output); } signature_check_clear(&sigc); diff --git a/gpg-interface.c b/gpg-interface.c index 127aecfc2b0..31cf4ba3938 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -15,6 +15,12 @@ struct gpg_format { const char *program; const char **verify_args; const char **sigs; + int (*verify_signed_buffer)(struct signature_check *sigc, + struct gpg_format *fmt, const char *payload, + size_t payload_size, const char *signature, + size_t signature_size); + int (*sign_buffer)(struct strbuf *buffer, struct strbuf *signature, + const char *signing_key); }; static const char *openpgp_verify_args[] = { @@ -35,14 +41,29 @@ static const char *x509_sigs[] = { NULL }; +static int verify_gpg_signed_buffer(struct signature_check *sigc, + struct gpg_format *fmt, const char *payload, + size_t payload_size, const char *signature, + size_t signature_size); +static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature, + const char *signing_key); + static struct gpg_format gpg_format[] = { - { .name = "openpgp", .program = "gpg", - .verify_args = openpgp_verify_args, - .sigs = openpgp_sigs + { + .name = "openpgp", + .program = "gpg", + .verify_args = openpgp_verify_args, + .sigs = openpgp_sigs, + .verify_signed_buffer = verify_gpg_signed_buffer, + .sign_buffer = sign_buffer_gpg, }, - { .name = "x509", .program = "gpgsm", - .verify_args = x509_verify_args, - .sigs = x509_sigs + { + .name = "x509", + .program = "gpgsm", + .verify_args = x509_verify_args, + .sigs = x509_sigs, + .verify_signed_buffer = verify_gpg_signed_buffer, + .sign_buffer = sign_buffer_gpg, }, }; @@ -72,7 +93,7 @@ static struct gpg_format *get_format_by_sig(const char *sig) void signature_check_clear(struct signature_check *sigc) { FREE_AND_NULL(sigc->payload); - FREE_AND_NULL(sigc->gpg_output); + FREE_AND_NULL(sigc->output); FREE_AND_NULL(sigc->gpg_status); FREE_AND_NULL(sigc->signer); FREE_AND_NULL(sigc->key); @@ -257,16 +278,16 @@ error: FREE_AND_NULL(sigc->key); } -static 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) +static int verify_gpg_signed_buffer(struct signature_check *sigc, + struct gpg_format *fmt, const char *payload, + size_t payload_size, const char *signature, + size_t signature_size) { struct child_process gpg = CHILD_PROCESS_INIT; - struct gpg_format *fmt; struct tempfile *temp; int ret; - struct strbuf buf = STRBUF_INIT; + struct strbuf gpg_stdout = STRBUF_INIT; + struct strbuf gpg_stderr = STRBUF_INIT; temp = mks_tempfile_t(".git_vtag_tmpXXXXXX"); if (!temp) @@ -279,10 +300,6 @@ static int verify_signed_buffer(const char *payload, size_t payload_size, return -1; } - fmt = get_format_by_sig(signature); - if (!fmt) - BUG("bad signature '%s'", signature); - strvec_push(&gpg.args, fmt->program); strvec_pushv(&gpg.args, fmt->verify_args); strvec_pushl(&gpg.args, @@ -290,18 +307,22 @@ static int verify_signed_buffer(const char *payload, size_t payload_size, "--verify", temp->filename.buf, "-", NULL); - if (!gpg_status) - gpg_status = &buf; - sigchain_push(SIGPIPE, SIG_IGN); - ret = pipe_command(&gpg, payload, payload_size, - gpg_status, 0, gpg_output, 0); + ret = pipe_command(&gpg, payload, payload_size, &gpg_stdout, 0, + &gpg_stderr, 0); sigchain_pop(SIGPIPE); delete_tempfile(&temp); - ret |= !strstr(gpg_status->buf, "\n[GNUPG:] GOODSIG "); - strbuf_release(&buf); /* no matter it was used or not */ + ret |= !strstr(gpg_stdout.buf, "\n[GNUPG:] GOODSIG "); + sigc->payload = xmemdupz(payload, payload_size); + sigc->output = strbuf_detach(&gpg_stderr, NULL); + sigc->gpg_status = strbuf_detach(&gpg_stdout, NULL); + + parse_gpg_output(sigc); + + strbuf_release(&gpg_stdout); + strbuf_release(&gpg_stderr); return ret; } @@ -309,35 +330,32 @@ static int verify_signed_buffer(const char *payload, size_t payload_size, int check_signature(const char *payload, size_t plen, const char *signature, size_t slen, struct signature_check *sigc) { - struct strbuf gpg_output = STRBUF_INIT; - struct strbuf gpg_status = STRBUF_INIT; + struct gpg_format *fmt; int status; sigc->result = 'N'; sigc->trust_level = -1; - status = verify_signed_buffer(payload, plen, signature, slen, - &gpg_output, &gpg_status); - if (status && !gpg_output.len) - goto out; - sigc->payload = xmemdupz(payload, plen); - sigc->gpg_output = strbuf_detach(&gpg_output, NULL); - sigc->gpg_status = strbuf_detach(&gpg_status, NULL); - parse_gpg_output(sigc); + fmt = get_format_by_sig(signature); + if (!fmt) + return error(_("bad/incompatible signature '%s'"), signature); + + status = fmt->verify_signed_buffer(sigc, fmt, payload, plen, signature, + slen); + + if (status && !sigc->output) + return !!status; + status |= sigc->result != 'G'; status |= sigc->trust_level < configured_min_trust_level; - out: - strbuf_release(&gpg_status); - strbuf_release(&gpg_output); - return !!status; } void print_signature_buffer(const struct signature_check *sigc, unsigned flags) { - const char *output = flags & GPG_VERIFY_RAW ? - sigc->gpg_status : sigc->gpg_output; + const char *output = flags & GPG_VERIFY_RAW ? sigc->gpg_status : + sigc->output; if (flags & GPG_VERIFY_VERBOSE && sigc->payload) fputs(sigc->payload, stdout); @@ -441,6 +459,12 @@ const char *get_signing_key(void) } int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key) +{ + return use_format->sign_buffer(buffer, signature, signing_key); +} + +static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature, + const char *signing_key) { struct child_process gpg = CHILD_PROCESS_INIT; int ret; diff --git a/gpg-interface.h b/gpg-interface.h index 80567e48948..feac4decf8b 100644 --- a/gpg-interface.h +++ b/gpg-interface.h @@ -17,7 +17,7 @@ enum signature_trust_level { struct signature_check { char *payload; - char *gpg_output; + char *output; char *gpg_status; /* diff --git a/log-tree.c b/log-tree.c index 7b823786c2c..20af9bd1c82 100644 --- a/log-tree.c +++ b/log-tree.c @@ -513,10 +513,10 @@ static void show_signature(struct rev_info *opt, struct commit *commit) status = check_signature(payload.buf, payload.len, signature.buf, signature.len, &sigc); - if (status && !sigc.gpg_output) + if (status && !sigc.output) show_sig_lines(opt, status, "No signature\n"); else - show_sig_lines(opt, status, sigc.gpg_output); + show_sig_lines(opt, status, sigc.output); signature_check_clear(&sigc); out: @@ -583,8 +583,8 @@ static int show_one_mergetag(struct commit *commit, /* could have a good signature */ status = check_signature(payload.buf, payload.len, signature.buf, signature.len, &sigc); - if (sigc.gpg_output) - strbuf_addstr(&verify_message, sigc.gpg_output); + if (sigc.output) + strbuf_addstr(&verify_message, sigc.output); else strbuf_addstr(&verify_message, "No signature\n"); signature_check_clear(&sigc); diff --git a/pretty.c b/pretty.c index b1ecd039cef..daa71394efd 100644 --- a/pretty.c +++ b/pretty.c @@ -1432,8 +1432,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ check_commit_signature(c->commit, &(c->signature_check)); switch (placeholder[1]) { case 'G': - if (c->signature_check.gpg_output) - strbuf_addstr(sb, c->signature_check.gpg_output); + if (c->signature_check.output) + strbuf_addstr(sb, c->signature_check.output); break; case '?': switch (c->signature_check.result) {