[PATCH/RFC] builtin/tag.c: move PGP verification inside builtin.

[PATCH/RFC] builtin/tag.c: move PGP verification inside builtin.

[santiago]

From: Santiago Torres <torresariass@gmail.com>

The verify tag function is just a thin wrapper around the verify-tag
command. We can avoid one fork call by doing the verification instide
the tag builtin instead.

Signed-off-by: Santiago Torres <santiago@nyu.edu>
---
 builtin/tag.c | 44 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 6 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 1705c94..be5d7c7 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -30,6 +30,27 @@ static const char * const git_tag_usage[] = {
 
 static unsigned int colopts;
 
+static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
+{
+	struct signature_check sigc;
+	int len;
+	int ret;
+
+	memset(&sigc, 0, sizeof(sigc));
+
+	len = parse_signature(buf, size);
+
+	if (size == len) {
+		write_in_full(1, buf, len);
+	}
+
+	ret = check_signature(buf, len, buf + len, size - len, &sigc);
+	print_signature_buffer(&sigc, flags);
+
+	signature_check_clear(&sigc);
+	return ret;
+}
+
 static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
 {
 	struct ref_array array;
@@ -104,13 +125,24 @@ static int delete_tag(const char *name, const char *ref,
 static int verify_tag(const char *name, const char *ref,
 				const unsigned char *sha1)
 {
-	const char *argv_verify_tag[] = {"verify-tag",
-					"-v", "SHA1_HEX", NULL};
-	argv_verify_tag[2] = sha1_to_hex(sha1);
 
-	if (run_command_v_opt(argv_verify_tag, RUN_GIT_CMD))
-		return error(_("could not verify the tag '%s'"), name);
-	return 0;
+	enum object_type type;
+	unsigned long size;
+	const char* buf;
+	int ret;
+
+	type = sha1_object_info(sha1, NULL);
+	if (type != OBJ_TAG)
+		return error("%s: cannot verify a non-tag object of type %s.",
+				name, typename(type));
+
+	buf = read_sha1_file(sha1, &type, &size);
+	if (!buf)
+		return error("%s: unable to read file.", name);
+
+	ret = run_gpg_verify(buf, size, 0);
+
+	return ret;
 }
 
 static int do_sign(struct strbuf *buffer)
-- 
2.7.3

Re: [PATCH/RFC] builtin/tag.c: move PGP verification inside builtin.

[Santiago Torres]

Hi Jeff.

Sorry for the delay with this, I got caught up with coursework.

This is my first stab at this, in the dumbest/simplest way imaginable. I
don't like that there is no code reuse (the run_gpg_verify function is
repeated here and in the plumbing command). I would appreciate pointers
on what would be the best way to avoid this.

I also spent quite some time figuring out what you meant with

> Do note the trickery with SIGPIPE in verify-tag, though. We probably
> need to do the same here (in fact, I wonder if that should be pushed
> down into the code that calls gpg).
I don't see any explicit SIGPIPE trickery here. Any pointers?

Thanks!
-Santiago.


On Thu, Mar 24, 2016 at 05:39:20PM -0400, santiago@nyu.edu wrote:
> From: Santiago Torres <torresariass@gmail.com>
> 
> The verify tag function is just a thin wrapper around the verify-tag
> command. We can avoid one fork call by doing the verification instide
> the tag builtin instead.
> 
> Signed-off-by: Santiago Torres <santiago@nyu.edu>
> ---
>  builtin/tag.c | 44 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 1705c94..be5d7c7 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -30,6 +30,27 @@ static const char * const git_tag_usage[] = {
>  
>  static unsigned int colopts;
>  
> +static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
> +{
> +	struct signature_check sigc;
> +	int len;
> +	int ret;
> +
> +	memset(&sigc, 0, sizeof(sigc));
> +
> +	len = parse_signature(buf, size);
> +
> +	if (size == len) {
> +		write_in_full(1, buf, len);
> +	}
> +
> +	ret = check_signature(buf, len, buf + len, size - len, &sigc);
> +	print_signature_buffer(&sigc, flags);
> +
> +	signature_check_clear(&sigc);
> +	return ret;
> +}
> +
>  static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
>  {
>  	struct ref_array array;
> @@ -104,13 +125,24 @@ static int delete_tag(const char *name, const char *ref,
>  static int verify_tag(const char *name, const char *ref,
>  				const unsigned char *sha1)
>  {
> -	const char *argv_verify_tag[] = {"verify-tag",
> -					"-v", "SHA1_HEX", NULL};
> -	argv_verify_tag[2] = sha1_to_hex(sha1);
>  
> -	if (run_command_v_opt(argv_verify_tag, RUN_GIT_CMD))
> -		return error(_("could not verify the tag '%s'"), name);
> -	return 0;
> +	enum object_type type;
> +	unsigned long size;
> +	const char* buf;
> +	int ret;
> +
> +	type = sha1_object_info(sha1, NULL);
> +	if (type != OBJ_TAG)
> +		return error("%s: cannot verify a non-tag object of type %s.",
> +				name, typename(type));
> +
> +	buf = read_sha1_file(sha1, &type, &size);
> +	if (!buf)
> +		return error("%s: unable to read file.", name);
> +
> +	ret = run_gpg_verify(buf, size, 0);
> +
> +	return ret;
>  }
>  
>  static int do_sign(struct strbuf *buffer)
> -- 
> 2.7.3
> 

Re: [PATCH/RFC] builtin/tag.c: move PGP verification inside builtin.

[Jeff King]

On Thu, Mar 24, 2016 at 05:51:05PM -0400, Santiago Torres wrote:

> Sorry for the delay with this, I got caught up with coursework.

No problem. The project moves forward as contributor time permits.

> This is my first stab at this, in the dumbest/simplest way imaginable. I
> don't like that there is no code reuse (the run_gpg_verify function is
> repeated here and in the plumbing command). I would appreciate pointers
> on what would be the best way to avoid this.

It looks to me like you could factor the repeated code into a common
verify_tag(), but maybe I am missing something.

> I also spent quite some time figuring out what you meant with
> 
> > Do note the trickery with SIGPIPE in verify-tag, though. We probably
> > need to do the same here (in fact, I wonder if that should be pushed
> > down into the code that calls gpg).
> I don't see any explicit SIGPIPE trickery here. Any pointers?

There is a call to ignore SIGPIPE in builtin/verify-tag.c, line 100.
Do we need to do be doing the same thing here?

There's some discussion in the thread starting at:

  http://thread.gmane.org/gmane.comp.version-control.git/53878/focus=53904

The claim there is that we get SIGPIPE and die early if we feed gpg a
tag which isn't signed. We _should_ be catching that case already via
parse_signature(), though I wonder if it can be fooled (e.g., something
that looks like a signature, but when gpg parses it, it turns out to be
bogus). So we should probably continue ignoring SIGPIPE to be on the
safe side.

But I notice that we already handle SIGPIPE explicitly in sign_buffer()
for similar reasons.  What I was wondering earlier was whether we should
teach other functions that call gpg (like verify_signed_buffer()) to
ignore SIGPIPE, too, so that we can return a reasonable error value
rather than just killing the whole program.

-Peff

Re: [PATCH/RFC] builtin/tag.c: move PGP verification inside builtin.

[Santiago Torres]

> > This is my first stab at this, in the dumbest/simplest way imaginable. I
> > don't like that there is no code reuse (the run_gpg_verify function is
> > repeated here and in the plumbing command). I would appreciate pointers
> > on what would be the best way to avoid this.
> 
> It looks to me like you could factor the repeated code into a common
> verify_tag(), but maybe I am missing something.

I see, yes. This looks like the way forward.
> 
> > I also spent quite some time figuring out what you meant with
> > 
> > > Do note the trickery with SIGPIPE in verify-tag, though. We probably
> > > need to do the same here (in fact, I wonder if that should be pushed
> > > down into the code that calls gpg).
> > I don't see any explicit SIGPIPE trickery here. Any pointers?
> 
> There is a call to ignore SIGPIPE in builtin/verify-tag.c, line 100.
> Do we need to do be doing the same thing here?
> 
> There's some discussion in the thread starting at:
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/53878/focus=53904
> 
> The claim there is that we get SIGPIPE and die early if we feed gpg a
> tag which isn't signed. We _should_ be catching that case already via
> parse_signature(), though I wonder if it can be fooled (e.g., something
> that looks like a signature, but when gpg parses it, it turns out to be
> bogus). So we should probably continue ignoring SIGPIPE to be on the
> safe side.
> 
> But I notice that we already handle SIGPIPE explicitly in sign_buffer()
> for similar reasons.  What I was wondering earlier was whether we should
> teach other functions that call gpg (like verify_signed_buffer()) to
> ignore SIGPIPE, too, so that we can return a reasonable error value
> rather than just killing the whole program.
> 

Now I get it  I think this should be easy to achieve by moving
verify_tag() to tag.c, along with the static run_gpg_verify functions. I
could move the SIGPIPE call inside the verify-tag command and patch up
everything accordingly. Does this sound ok?

Thanks,
-Santiago.

Re: [PATCH/RFC] builtin/tag.c: move PGP verification inside builtin.

[Jeff King]

On Thu, Mar 24, 2016 at 06:32:58PM -0400, Santiago Torres wrote:

> > But I notice that we already handle SIGPIPE explicitly in sign_buffer()
> > for similar reasons.  What I was wondering earlier was whether we should
> > teach other functions that call gpg (like verify_signed_buffer()) to
> > ignore SIGPIPE, too, so that we can return a reasonable error value
> > rather than just killing the whole program.
> 
> Now I get it  I think this should be easy to achieve by moving
> verify_tag() to tag.c, along with the static run_gpg_verify functions.

Exactly.

> I could move the SIGPIPE call inside the verify-tag command and patch up
> everything accordingly. Does this sound ok?

I think that works, but take note of two things:

  - convert it to sigchain_push(), and make sure you sigchain_pop() it
    when you are done, so that the caller retains their original SIGPIPE
    behavior after the function returns. See the example in
    sign_buffer().

  - you should probably do it as close to the gpg call as possible, so
    as to affect as little code as possible. So probably in
    verify_signed_buffer(), not in verify_tag().

-Peff

Re: [PATCH/RFC] builtin/tag.c: move PGP verification inside builtin.

[Jeff King]

On Thu, Mar 24, 2016 at 05:39:20PM -0400, santiago@nyu.edu wrote:

> +static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
> +{
> +	struct signature_check sigc;
> +	int len;
> +	int ret;
> +
> +	memset(&sigc, 0, sizeof(sigc));
> +
> +	len = parse_signature(buf, size);

I know you are just copying this from the one in builtin/verify-tag.c,
but I find the use of "size" and "len" for two different purposes
confusing. Those words are synonyms, so how do the variables differ?

Perhaps "payload_size", or "signature_offset" would be a better term for
"len".

> +	if (size == len) {
> +		write_in_full(1, buf, len);
> +	}

If the two are the same, we have no signature. Should we be returning
early, and skipping check_signature() in that case?

> +	ret = check_signature(buf, len, buf + len, size - len, &sigc);
> +	print_signature_buffer(&sigc, flags);
> +
> +	signature_check_clear(&sigc);
> +	return ret;
> +}

This part looks OK.

> @@ -104,13 +125,24 @@ static int delete_tag(const char *name, const char *ref,
>  static int verify_tag(const char *name, const char *ref,
>  				const unsigned char *sha1)
>  {
> -	const char *argv_verify_tag[] = {"verify-tag",
> -					"-v", "SHA1_HEX", NULL};

So the original was passing "-v" to verify-tag. That should put
GPG_VERIFY_VERBOSE into the flags field. But later:

> +	ret = run_gpg_verify(buf, size, 0);

We don't pass any flags. Shouldn't this unconditionally pass
GPG_VERIFY_VERBOSE?

> +	enum object_type type;
> +	unsigned long size;
> +	const char* buf;
> +	int ret;
> +
> +	type = sha1_object_info(sha1, NULL);
> +	if (type != OBJ_TAG)
> +		return error("%s: cannot verify a non-tag object of type %s.",
> +				name, typename(type));
> +
> +	buf = read_sha1_file(sha1, &type, &size);
> +	if (!buf)
> +		return error("%s: unable to read file.", name);
> +
> +	ret = run_gpg_verify(buf, size, 0);
> +
> +	return ret;

All of this seems like a repetition of verify_tag() in
builtin/verify-tag.c (and ditto with run_gpg_verify()). Can we move
those functions into tag.c and just call them from both places, or is
there some difference that needs to be taken into account (and if the
latter, can we refactor them to account for the differences?).

-Peff

Re: [PATCH/RFC] builtin/tag.c: move PGP verification inside builtin.

[Santiago Torres]

> I know you are just copying this from the one in builtin/verify-tag.c,
> but I find the use of "size" and "len" for two different purposes
> confusing. Those words are synonyms, so how do the variables differ?
> 
> Perhaps "payload_size", or "signature_offset" would be a better term for
> "len".

I agree, I'll give this a go.
> 
> > +	if (size == len) {
> > +		write_in_full(1, buf, len);
> > +	}
> 
> If the two are the same, we have no signature. Should we be returning
> early, and skipping check_signature() in that case?

This makes sense, for both the builtin and the plumbing. Let me give
this a try.

 
> > @@ -104,13 +125,24 @@ static int delete_tag(const char *name, const char *ref,
> >  static int verify_tag(const char *name, const char *ref,
> >  				const unsigned char *sha1)
> >  {
> > -	const char *argv_verify_tag[] = {"verify-tag",
> > -					"-v", "SHA1_HEX", NULL};
> 
> So the original was passing "-v" to verify-tag. That should put
> GPG_VERIFY_VERBOSE into the flags field. But later:
> 
> > +	ret = run_gpg_verify(buf, size, 0);
> 
> We don't pass any flags. Shouldn't this unconditionally pass
> GPG_VERIFY_VERBOSE?
> 

Right, I missed this. Sorry about this.

> All of this seems like a repetition of verify_tag() in
> builtin/verify-tag.c (and ditto with run_gpg_verify()). Can we move
> those functions into tag.c and just call them from both places, or is
> there some difference that needs to be taken into account (and if the
> latter, can we refactor them to account for the differences?).
> 

Yep, this is what was troubling me (as I mentioned on the followup). I
didn't want to remove the "static" classifier for the function (as there
could be a major reason for this decision). 

If this last chage is ok with you I can send the fixed-up version right
away.

Thanks!
-Santiago.