{"thread":{"id":"41818","subject":"[PATCH/RFC] builtin/tag.c: move PGP verification inside builtin.","startedAt":"2016-03-24T21:39:20Z","lastAt":"2016-03-24T23:27:45Z","messageCount":7,"participants":["santiago@nyu.edu","Santiago Torres","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"281727","messageId":"1458855560-28519-1-git-send-email-santiago@nyu.edu","threadId":"41818","inReplyTo":null,"subject":"[PATCH/RFC] builtin/tag.c: move PGP verification inside builtin.","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-03-24T21:39:20Z","receivedAt":"2016-03-24T21:39:20Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"From: Santiago Torres <torresariass@gmail.com>\n\nThe verify tag function is just a thin wrapper around the verify-tag\ncommand. We can avoid one fork call by doing the verification instide\nthe tag builtin instead.\n\nSigned-off-by: Santiago Torres <santiago@nyu.edu>\n---\n builtin/tag.c | 44 ++++++++++++++++++++++++++++++++++++++------\n 1 file changed, 38 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 1705c94..be5d7c7 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -30,6 +30,27 @@ static const char * const git_tag_usage[] = {\n \n static unsigned int colopts;\n \n+static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)\n+{\n+\tstruct signature_check sigc;\n+\tint len;\n+\tint ret;\n+\n+\tmemset(&sigc, 0, sizeof(sigc));\n+\n+\tlen = parse_signature(buf, size);\n+\n+\tif (size == len) {\n+\t\twrite_in_full(1, buf, len);\n+\t}\n+\n+\tret = check_signature(buf, len, buf + len, size - len, &sigc);\n+\tprint_signature_buffer(&sigc, flags);\n+\n+\tsignature_check_clear(&sigc);\n+\treturn ret;\n+}\n+\n static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)\n {\n \tstruct ref_array array;\n@@ -104,13 +125,24 @@ static int delete_tag(const char *name, const char *ref,\n static int verify_tag(const char *name, const char *ref,\n \t\t\t\tconst unsigned char *sha1)\n {\n-\tconst char *argv_verify_tag[] = {\"verify-tag\",\n-\t\t\t\t\t\"-v\", \"SHA1_HEX\", NULL};\n-\targv_verify_tag[2] = sha1_to_hex(sha1);\n \n-\tif (run_command_v_opt(argv_verify_tag, RUN_GIT_CMD))\n-\t\treturn error(_(\"could not verify the tag '%s'\"), name);\n-\treturn 0;\n+\tenum object_type type;\n+\tunsigned long size;\n+\tconst char* buf;\n+\tint ret;\n+\n+\ttype = sha1_object_info(sha1, NULL);\n+\tif (type != OBJ_TAG)\n+\t\treturn error(\"%s: cannot verify a non-tag object of type %s.\",\n+\t\t\t\tname, typename(type));\n+\n+\tbuf = read_sha1_file(sha1, &type, &size);\n+\tif (!buf)\n+\t\treturn error(\"%s: unable to read file.\", name);\n+\n+\tret = run_gpg_verify(buf, size, 0);\n+\n+\treturn ret;\n }\n \n static int do_sign(struct strbuf *buffer)\n-- \n2.7.3\n"},{"id":"281731","messageId":"20160324215104.GC8830@LykOS","threadId":"41818","inReplyTo":"1458855560-28519-1-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH/RFC] builtin/tag.c: move PGP verification inside builtin.","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-03-24T21:51:05Z","receivedAt":"2016-03-24T21:51:05Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"Hi Jeff.\n\nSorry for the delay with this, I got caught up with coursework.\n\nThis is my first stab at this, in the dumbest/simplest way imaginable. I\ndon't like that there is no code reuse (the run_gpg_verify function is\nrepeated here and in the plumbing command). I would appreciate pointers\non what would be the best way to avoid this.\n\nI also spent quite some time figuring out what you meant with\n\n> Do note the trickery with SIGPIPE in verify-tag, though. We probably\n> need to do the same here (in fact, I wonder if that should be pushed\n> down into the code that calls gpg).\nI don't see any explicit SIGPIPE trickery here. Any pointers?\n\nThanks!\n-Santiago.\n\n\nOn Thu, Mar 24, 2016 at 05:39:20PM -0400, santiago@nyu.edu wrote:\n> From: Santiago Torres <torresariass@gmail.com>\n> \n> The verify tag function is just a thin wrapper around the verify-tag\n> command. We can avoid one fork call by doing the verification instide\n> the tag builtin instead.\n> \n> Signed-off-by: Santiago Torres <santiago@nyu.edu>\n> ---\n>  builtin/tag.c | 44 ++++++++++++++++++++++++++++++++++++++------\n>  1 file changed, 38 insertions(+), 6 deletions(-)\n> \n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 1705c94..be5d7c7 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -30,6 +30,27 @@ static const char * const git_tag_usage[] = {\n>  \n>  static unsigned int colopts;\n>  \n> +static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)\n> +{\n> +\tstruct signature_check sigc;\n> +\tint len;\n> +\tint ret;\n> +\n> +\tmemset(&sigc, 0, sizeof(sigc));\n> +\n> +\tlen = parse_signature(buf, size);\n> +\n> +\tif (size == len) {\n> +\t\twrite_in_full(1, buf, len);\n> +\t}\n> +\n> +\tret = check_signature(buf, len, buf + len, size - len, &sigc);\n> +\tprint_signature_buffer(&sigc, flags);\n> +\n> +\tsignature_check_clear(&sigc);\n> +\treturn ret;\n> +}\n> +\n>  static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)\n>  {\n>  \tstruct ref_array array;\n> @@ -104,13 +125,24 @@ static int delete_tag(const char *name, const char *ref,\n>  static int verify_tag(const char *name, const char *ref,\n>  \t\t\t\tconst unsigned char *sha1)\n>  {\n> -\tconst char *argv_verify_tag[] = {\"verify-tag\",\n> -\t\t\t\t\t\"-v\", \"SHA1_HEX\", NULL};\n> -\targv_verify_tag[2] = sha1_to_hex(sha1);\n>  \n> -\tif (run_command_v_opt(argv_verify_tag, RUN_GIT_CMD))\n> -\t\treturn error(_(\"could not verify the tag '%s'\"), name);\n> -\treturn 0;\n> +\tenum object_type type;\n> +\tunsigned long size;\n> +\tconst char* buf;\n> +\tint ret;\n> +\n> +\ttype = sha1_object_info(sha1, NULL);\n> +\tif (type != OBJ_TAG)\n> +\t\treturn error(\"%s: cannot verify a non-tag object of type %s.\",\n> +\t\t\t\tname, typename(type));\n> +\n> +\tbuf = read_sha1_file(sha1, &type, &size);\n> +\tif (!buf)\n> +\t\treturn error(\"%s: unable to read file.\", name);\n> +\n> +\tret = run_gpg_verify(buf, size, 0);\n> +\n> +\treturn ret;\n>  }\n>  \n>  static int do_sign(struct strbuf *buffer)\n> -- \n> 2.7.3\n> \n"},{"id":"281735","messageId":"20160324221020.GA17805@sigill.intra.peff.net","threadId":"41818","inReplyTo":"1458855560-28519-1-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH/RFC] builtin/tag.c: move PGP verification inside builtin.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-24T22:10:20Z","receivedAt":"2016-03-24T22:10:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 24, 2016 at 05:39:20PM -0400, santiago@nyu.edu wrote:\n\n> +static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)\n> +{\n> +\tstruct signature_check sigc;\n> +\tint len;\n> +\tint ret;\n> +\n> +\tmemset(&sigc, 0, sizeof(sigc));\n> +\n> +\tlen = parse_signature(buf, size);\n\nI know you are just copying this from the one in builtin/verify-tag.c,\nbut I find the use of \"size\" and \"len\" for two different purposes\nconfusing. Those words are synonyms, so how do the variables differ?\n\nPerhaps \"payload_size\", or \"signature_offset\" would be a better term for\n\"len\".\n\n> +\tif (size == len) {\n> +\t\twrite_in_full(1, buf, len);\n> +\t}\n\nIf the two are the same, we have no signature. Should we be returning\nearly, and skipping check_signature() in that case?\n\n> +\tret = check_signature(buf, len, buf + len, size - len, &sigc);\n> +\tprint_signature_buffer(&sigc, flags);\n> +\n> +\tsignature_check_clear(&sigc);\n> +\treturn ret;\n> +}\n\nThis part looks OK.\n\n> @@ -104,13 +125,24 @@ static int delete_tag(const char *name, const char *ref,\n>  static int verify_tag(const char *name, const char *ref,\n>  \t\t\t\tconst unsigned char *sha1)\n>  {\n> -\tconst char *argv_verify_tag[] = {\"verify-tag\",\n> -\t\t\t\t\t\"-v\", \"SHA1_HEX\", NULL};\n\nSo the original was passing \"-v\" to verify-tag. That should put\nGPG_VERIFY_VERBOSE into the flags field. But later:\n\n> +\tret = run_gpg_verify(buf, size, 0);\n\nWe don't pass any flags. Shouldn't this unconditionally pass\nGPG_VERIFY_VERBOSE?\n\n> +\tenum object_type type;\n> +\tunsigned long size;\n> +\tconst char* buf;\n> +\tint ret;\n> +\n> +\ttype = sha1_object_info(sha1, NULL);\n> +\tif (type != OBJ_TAG)\n> +\t\treturn error(\"%s: cannot verify a non-tag object of type %s.\",\n> +\t\t\t\tname, typename(type));\n> +\n> +\tbuf = read_sha1_file(sha1, &type, &size);\n> +\tif (!buf)\n> +\t\treturn error(\"%s: unable to read file.\", name);\n> +\n> +\tret = run_gpg_verify(buf, size, 0);\n> +\n> +\treturn ret;\n\nAll of this seems like a repetition of verify_tag() in\nbuiltin/verify-tag.c (and ditto with run_gpg_verify()). Can we move\nthose functions into tag.c and just call them from both places, or is\nthere some difference that needs to be taken into account (and if the\nlatter, can we refactor them to account for the differences?).\n\n-Peff\n"},{"id":"281736","messageId":"20160324221457.GB17805@sigill.intra.peff.net","threadId":"41818","inReplyTo":"20160324215104.GC8830@LykOS","subject":"Re: [PATCH/RFC] builtin/tag.c: move PGP verification inside builtin.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-24T22:14:57Z","receivedAt":"2016-03-24T22:14:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 24, 2016 at 05:51:05PM -0400, Santiago Torres wrote:\n\n> Sorry for the delay with this, I got caught up with coursework.\n\nNo problem. The project moves forward as contributor time permits.\n\n> This is my first stab at this, in the dumbest/simplest way imaginable. I\n> don't like that there is no code reuse (the run_gpg_verify function is\n> repeated here and in the plumbing command). I would appreciate pointers\n> on what would be the best way to avoid this.\n\nIt looks to me like you could factor the repeated code into a common\nverify_tag(), but maybe I am missing something.\n\n> I also spent quite some time figuring out what you meant with\n> \n> > Do note the trickery with SIGPIPE in verify-tag, though. We probably\n> > need to do the same here (in fact, I wonder if that should be pushed\n> > down into the code that calls gpg).\n> I don't see any explicit SIGPIPE trickery here. Any pointers?\n\nThere is a call to ignore SIGPIPE in builtin/verify-tag.c, line 100.\nDo we need to do be doing the same thing here?\n\nThere's some discussion in the thread starting at:\n\n  http://thread.gmane.org/gmane.comp.version-control.git/53878/focus=53904\n\nThe claim there is that we get SIGPIPE and die early if we feed gpg a\ntag which isn't signed. We _should_ be catching that case already via\nparse_signature(), though I wonder if it can be fooled (e.g., something\nthat looks like a signature, but when gpg parses it, it turns out to be\nbogus). So we should probably continue ignoring SIGPIPE to be on the\nsafe side.\n\nBut I notice that we already handle SIGPIPE explicitly in sign_buffer()\nfor similar reasons.  What I was wondering earlier was whether we should\nteach other functions that call gpg (like verify_signed_buffer()) to\nignore SIGPIPE, too, so that we can return a reasonable error value\nrather than just killing the whole program.\n\n-Peff\n"},{"id":"281737","messageId":"20160324222451.GD8830@LykOS","threadId":"41818","inReplyTo":"20160324221020.GA17805@sigill.intra.peff.net","subject":"Re: [PATCH/RFC] builtin/tag.c: move PGP verification inside builtin.","fromName":"Santiago Torres","fromEmail":"torresariass@gmail.com","sentAt":"2016-03-24T22:24:52Z","receivedAt":"2016-03-24T22:24:52Z","isPatch":true,"sender":{"key":"torresariass@gmail.com","avatar":null},"body":"> I know you are just copying this from the one in builtin/verify-tag.c,\n> but I find the use of \"size\" and \"len\" for two different purposes\n> confusing. Those words are synonyms, so how do the variables differ?\n> \n> Perhaps \"payload_size\", or \"signature_offset\" would be a better term for\n> \"len\".\n\nI agree, I'll give this a go.\n> \n> > +\tif (size == len) {\n> > +\t\twrite_in_full(1, buf, len);\n> > +\t}\n> \n> If the two are the same, we have no signature. Should we be returning\n> early, and skipping check_signature() in that case?\n\nThis makes sense, for both the builtin and the plumbing. Let me give\nthis a try.\n\n \n> > @@ -104,13 +125,24 @@ static int delete_tag(const char *name, const char *ref,\n> >  static int verify_tag(const char *name, const char *ref,\n> >  \t\t\t\tconst unsigned char *sha1)\n> >  {\n> > -\tconst char *argv_verify_tag[] = {\"verify-tag\",\n> > -\t\t\t\t\t\"-v\", \"SHA1_HEX\", NULL};\n> \n> So the original was passing \"-v\" to verify-tag. That should put\n> GPG_VERIFY_VERBOSE into the flags field. But later:\n> \n> > +\tret = run_gpg_verify(buf, size, 0);\n> \n> We don't pass any flags. Shouldn't this unconditionally pass\n> GPG_VERIFY_VERBOSE?\n> \n\nRight, I missed this. Sorry about this.\n\n> All of this seems like a repetition of verify_tag() in\n> builtin/verify-tag.c (and ditto with run_gpg_verify()). Can we move\n> those functions into tag.c and just call them from both places, or is\n> there some difference that needs to be taken into account (and if the\n> latter, can we refactor them to account for the differences?).\n> \n\nYep, this is what was troubling me (as I mentioned on the followup). I\ndidn't want to remove the \"static\" classifier for the function (as there\ncould be a major reason for this decision). \n\nIf this last chage is ok with you I can send the fixed-up version right\naway.\n\nThanks!\n-Santiago.\n"},{"id":"281740","messageId":"20160324223257.GE8830@LykOS","threadId":"41818","inReplyTo":"20160324221457.GB17805@sigill.intra.peff.net","subject":"Re: [PATCH/RFC] builtin/tag.c: move PGP verification inside builtin.","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-03-24T22:32:58Z","receivedAt":"2016-03-24T22:32:58Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"> > This is my first stab at this, in the dumbest/simplest way imaginable. I\n> > don't like that there is no code reuse (the run_gpg_verify function is\n> > repeated here and in the plumbing command). I would appreciate pointers\n> > on what would be the best way to avoid this.\n> \n> It looks to me like you could factor the repeated code into a common\n> verify_tag(), but maybe I am missing something.\n\nI see, yes. This looks like the way forward.\n> \n> > I also spent quite some time figuring out what you meant with\n> > \n> > > Do note the trickery with SIGPIPE in verify-tag, though. We probably\n> > > need to do the same here (in fact, I wonder if that should be pushed\n> > > down into the code that calls gpg).\n> > I don't see any explicit SIGPIPE trickery here. Any pointers?\n> \n> There is a call to ignore SIGPIPE in builtin/verify-tag.c, line 100.\n> Do we need to do be doing the same thing here?\n> \n> There's some discussion in the thread starting at:\n> \n>   http://thread.gmane.org/gmane.comp.version-control.git/53878/focus=53904\n> \n> The claim there is that we get SIGPIPE and die early if we feed gpg a\n> tag which isn't signed. We _should_ be catching that case already via\n> parse_signature(), though I wonder if it can be fooled (e.g., something\n> that looks like a signature, but when gpg parses it, it turns out to be\n> bogus). So we should probably continue ignoring SIGPIPE to be on the\n> safe side.\n> \n> But I notice that we already handle SIGPIPE explicitly in sign_buffer()\n> for similar reasons.  What I was wondering earlier was whether we should\n> teach other functions that call gpg (like verify_signed_buffer()) to\n> ignore SIGPIPE, too, so that we can return a reasonable error value\n> rather than just killing the whole program.\n> \n\nNow I get it  I think this should be easy to achieve by moving\nverify_tag() to tag.c, along with the static run_gpg_verify functions. I\ncould move the SIGPIPE call inside the verify-tag command and patch up\neverything accordingly. Does this sound ok?\n\nThanks,\n-Santiago.\n"},{"id":"281745","messageId":"20160324232745.GA18499@sigill.intra.peff.net","threadId":"41818","inReplyTo":"20160324223257.GE8830@LykOS","subject":"Re: [PATCH/RFC] builtin/tag.c: move PGP verification inside builtin.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-24T23:27:45Z","receivedAt":"2016-03-24T23:27:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 24, 2016 at 06:32:58PM -0400, Santiago Torres wrote:\n\n> > But I notice that we already handle SIGPIPE explicitly in sign_buffer()\n> > for similar reasons.  What I was wondering earlier was whether we should\n> > teach other functions that call gpg (like verify_signed_buffer()) to\n> > ignore SIGPIPE, too, so that we can return a reasonable error value\n> > rather than just killing the whole program.\n> \n> Now I get it  I think this should be easy to achieve by moving\n> verify_tag() to tag.c, along with the static run_gpg_verify functions.\n\nExactly.\n\n> I could move the SIGPIPE call inside the verify-tag command and patch up\n> everything accordingly. Does this sound ok?\n\nI think that works, but take note of two things:\n\n  - convert it to sigchain_push(), and make sure you sigchain_pop() it\n    when you are done, so that the caller retains their original SIGPIPE\n    behavior after the function returns. See the example in\n    sign_buffer().\n\n  - you should probably do it as close to the gpg call as possible, so\n    as to affect as little code as possible. So probably in\n    verify_signed_buffer(), not in verify_tag().\n\n-Peff\n"}]}