[PATCH] verify-tag: allow to verify signed blob objects

[PATCH] verify-tag: allow to verify signed blob objects

[Michael J Gruber]

Currently, there is no easy way to verify push certificates. They have
the same structure as signed tags: "attached detached signatures", that
is: the concatenation of the signed material and its detached signature.

Introduce a `--blob` option to verify-tag so that it allows to verify
tags and blobs.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
The first outcome of my long announced project to describe our signature
formats in Documentation/technical.... (progress underway)

In fact, that whole area is in need of refactoring: gpg related bits are
all over the place, including tag.c. The proposed patch neither improves
nor worsens the situation in that respect. But, since we make it
unnecessarily hard to verify signatures (git cat-file | gpg --verify fails)
it's only fair to provide a tool for pre-receive hook writers.

 Documentation/git-verify-tag.txt | 4 ++++
 builtin/verify-tag.c             | 1 +
 gpg-interface.h                  | 1 +
 t/t5534-push-signed.sh           | 3 ++-
 tag.c                            | 2 +-
 5 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-verify-tag.txt b/Documentation/git-verify-tag.txt
index d590edc..2e5cf4d 100644
--- a/Documentation/git-verify-tag.txt
+++ b/Documentation/git-verify-tag.txt
@@ -20,6 +20,10 @@ OPTIONS
 	Print the raw gpg status output to standard error instead of the normal
 	human-readable output.
 
+--blob::
+	Allow to verify signed blob objects (in addition to tag objects), such as the
+	objects containing a push certificate.
+
 -v::
 --verbose::
 	Print the contents of the tag object before validating it.
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 99f8148..19d26b0 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -33,6 +33,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 	const struct option verify_tag_options[] = {
 		OPT__VERBOSE(&verbose, N_("print tag contents")),
 		OPT_BIT(0, "raw", &flags, N_("print raw gpg status output"), GPG_VERIFY_RAW),
+		OPT_BIT(0, "blob", &flags, N_("allow to verify blob objects"), GPG_VERIFY_BLOB),
 		OPT_END()
 	};
 
diff --git a/gpg-interface.h b/gpg-interface.h
index ea68885..a3cbfc3 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -3,6 +3,7 @@
 
 #define GPG_VERIFY_VERBOSE	1
 #define GPG_VERIFY_RAW		2
+#define GPG_VERIFY_BLOB		4
 
 struct signature_check {
 	char *payload;
diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index ecb8d44..de4d38b 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -94,7 +94,8 @@ test_expect_success GPG 'signed push sends push certificate' '
 	# record the push certificate
 	if test -n "${GIT_PUSH_CERT-}"
 	then
-		git cat-file blob $GIT_PUSH_CERT >../push-cert
+		git cat-file blob $GIT_PUSH_CERT >../push-cert &&
+		git verify-tag --blob $GIT_PUSH_CERT
 	fi &&
 
 	cat >../push-cert-status <<E_O_F
diff --git a/tag.c b/tag.c
index d1dcd18..d5f090b 100644
--- a/tag.c
+++ b/tag.c
@@ -39,7 +39,7 @@ int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report,
 	int ret;
 
 	type = sha1_object_info(sha1, NULL);
-	if (type != OBJ_TAG)
+	if ((type != OBJ_TAG) && ((type != OBJ_BLOB) || !(flags & GPG_VERIFY_BLOB)))
 		return error("%s: cannot verify a non-tag object of type %s.",
 				name_to_report ?
 				name_to_report :
-- 
2.9.0.382.g87fd384

Re: [PATCH] verify-tag: allow to verify signed blob objects

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> diff --git a/tag.c b/tag.c
> index d1dcd18..d5f090b 100644
> --- a/tag.c
> +++ b/tag.c
> @@ -39,7 +39,7 @@ int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report,
>  	int ret;
>  
>  	type = sha1_object_info(sha1, NULL);
> -	if (type != OBJ_TAG)
> +	if ((type != OBJ_TAG) && ((type != OBJ_BLOB) || !(flags & GPG_VERIFY_BLOB)))
>  		return error("%s: cannot verify a non-tag object of type %s.",
>  				name_to_report ?
>  				name_to_report :

The double negation is very hard to read.  I wonder

	if ((type != OBJ_TAG) &&
            !((type == OBJ_BLOB) && (flags & GPG_VERIFY_BLOB)))

is easier to follow?  "It is not a tag object, and it's not like we
were asked to verify blob and the user gave us a blob, either, so
let's complain" is easier to follow, at least for me.

Or even

	if ((flags & GPG_VERIFY_BLOB) && (type != OBJ_BLOB))
               	"you told me to check blob but didn't give me one";
	} else if (type != OBJ_TAG)
		"you didn't give me a tag";

Re: [PATCH] verify-tag: allow to verify signed blob objects

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 15.06.2016 20:39:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> diff --git a/tag.c b/tag.c
>> index d1dcd18..d5f090b 100644
>> --- a/tag.c
>> +++ b/tag.c
>> @@ -39,7 +39,7 @@ int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report,
>>  	int ret;
>>  
>>  	type = sha1_object_info(sha1, NULL);
>> -	if (type != OBJ_TAG)
>> +	if ((type != OBJ_TAG) && ((type != OBJ_BLOB) || !(flags & GPG_VERIFY_BLOB)))
>>  		return error("%s: cannot verify a non-tag object of type %s.",
>>  				name_to_report ?
>>  				name_to_report :
> 
> The double negation is very hard to read.  I wonder
> 
> 	if ((type != OBJ_TAG) &&
>             !((type == OBJ_BLOB) && (flags & GPG_VERIFY_BLOB)))
> 
> is easier to follow?  "It is not a tag object, and it's not like we
> were asked to verify blob and the user gave us a blob, either, so
> let's complain" is easier to follow, at least for me.

As a further exercise in boolean algebra, you can pull out the negation
completely:

if (!( (type == OBJ_TAG) || ((type == OBJ_BLOB) && (flags &
GPG_VERIFY_BLOB)) ))

> Or even
> 
> 	if ((flags & GPG_VERIFY_BLOB) && (type != OBJ_BLOB))
>                	"you told me to check blob but didn't give me one";
> 	} else if (type != OBJ_TAG)
> 		"you didn't give me a tag";
> 

I just tried to stay as close to the original as possible, but I don't
care either way. Your latter version is more strict and would require a
slight documentation change, but would be fine, too.

Michael

Re: [PATCH] verify-tag: allow to verify signed blob objects

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

>> Or even
>> 
>> 	if ((flags & GPG_VERIFY_BLOB) && (type != OBJ_BLOB))
>>                	"you told me to check blob but didn't give me one";
>> 	} else if (type != OBJ_TAG)
>> 		"you didn't give me a tag";
>> 
>
> I just tried to stay as close to the original as possible, but I don't
> care either way. Your latter version is more strict and would require a
> slight documentation change, but would be fine, too.

Actually, the message you reused is not reusable for this new mode.
I guess starting from more strict (which makes sense, as you do not
want to silently say "Yeah, the blob verifies OK" when the user
tells you "I want you to verify this blob, and here it is" and hands
you a tag.  If that were an acceptable behaviour, you do not even
need VERIFY_BLOB as an option, do you?

So I do not care too strongly about this feature, if it were to be
added, I think you would need to separate error messages and type
verification should not be lax, I would think.

Re: [PATCH] verify-tag: allow to verify signed blob objects

[Jacob Keller]

On Wed, Jun 15, 2016 at 12:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>>> Or even
>>>
>>>      if ((flags & GPG_VERIFY_BLOB) && (type != OBJ_BLOB))
>>>                      "you told me to check blob but didn't give me one";
>>>      } else if (type != OBJ_TAG)
>>>              "you didn't give me a tag";
>>>
>>
>> I just tried to stay as close to the original as possible, but I don't
>> care either way. Your latter version is more strict and would require a
>> slight documentation change, but would be fine, too.
>
> Actually, the message you reused is not reusable for this new mode.
> I guess starting from more strict (which makes sense, as you do not
> want to silently say "Yeah, the blob verifies OK" when the user
> tells you "I want you to verify this blob, and here it is" and hands
> you a tag.  If that were an acceptable behaviour, you do not even
> need VERIFY_BLOB as an option, do you?
>
> So I do not care too strongly about this feature, if it were to be
> added, I think you would need to separate error messages and type
> verification should not be lax, I would think.
>

I agree that Junio's suggestion is (a) both easier to read and (b)
more clear to the end user, and thus preferable.

Thanks,
Jake

Re: [PATCH] verify-tag: allow to verify signed blob objects

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Currently, there is no easy way to verify push certificates. They have
> the same structure as signed tags: "attached detached signatures", that
> is: the concatenation of the signed material and its detached signature.
>
> Introduce a `--blob` option to verify-tag so that it allows to verify
> tags and blobs.
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
> The first outcome of my long announced project to describe our signature
> formats in Documentation/technical.... (progress underway)
>
> In fact, that whole area is in need of refactoring: gpg related bits are
> all over the place, including tag.c. The proposed patch neither improves
> nor worsens the situation in that respect. But, since we make it
> unnecessarily hard to verify signatures (git cat-file | gpg --verify fails)
> it's only fair to provide a tool for pre-receive hook writers.


Another (orthogonal) thing to think about is "is it sensible to add
a new feature to verify blob objects that has signature?"

That is, if we are adding one new feature, isn't it more sensible
for it to accept a stream of bytes from the standard input and run
verify-signed-bufter on it?  That way, if you already happen to have
a blob, you can feed it "cat-file blob" output to get the new
feature you added in this patch.

But you cannot go in the other direction without incurring downside.

If you start from "verify signature in blob" interface, and if all
you have is a log of push certificates in a flat text file [1], you
would need to do "hash-objects -w" first only to be able to use that
interface (which in turn means that you would need write access to
the object store of the repository.


[Footnote]

*1* push certificate is first written as a blob in the object store
    only so that we can safely run multiple receive-pack instances
    without them stepping on each others' toes; it is expected that
    they are collected by the server operator in chronological order
    and published in a human readable log form, so that outside
    people can verify honesty of the server operator.