[RFC/PATCH] verify-tag: add --check-name flag

From: Santiago Torres <redacted>

Hello everyone,

In a previous thread [1] we discussed about the possibility of having a
--check-name flag, for the tag-verify command (and possibly git tag -v).
Although many points were in the table, I don't think that it was
conclusive as to whether having it or not. Also, I noticed that the,
refactored interface requires really minmal changes to do this
(see attached patch). 

This is the behavior I had in mind:


    santiago at ~/.../.../git ✗ ./git-verify-tag --check-name v2.7.0-rc20000
    gpg: Signature made Tue 22 Dec 2015 05:46:04 PM EST using RSA ... 
    gpg: Good signature from "Junio C Hamano [off-list ref]" [...]
    ...
    error: tag name doesn't match tag header!(v2.7.0-rc2)

    santiago at ~/.../.../git ✗ ./git-verify-tag --check-name v2.7.0-rc2
    gpg: Signature made Tue 22 Dec 2015 05:46:04 PM EST using RSA ...
    gpg: Good signature from "Junio C Hamano [off-list ref]" [...]
    ...

The rationale behind this is the following:

1.- Using a tag ref as a check-out mechanism is pretty common by package
    managers and other tools. Verifying the tag signature provides
    authentication guarantees, but there is no feedback that the
    signature being verified belongs to the intended tag.

2.- The tuple tagname + signature can uniquely identify a tag. There
    are many tags that can have the same name, but this is mostly due
    to the naming policy. Having a tag-ref pointing to the right tag
    name with an appropriate signature provides tigther guarantees about
    the tag that's being checked-out.

3.- This follows concerns about other people who wish to provide a
    tighter binding between the refs and the tag objects. The git-evtag
    project is an example of this[2].

What I want to prevent is the following: 

    santiago at ~/.../ ✔ pip install -e git+https://.../django/@1.8.3#egg=django
    Obtaining django from git+https://.../django/@1.8.3#egg=django
    [...] 
    Successfully installed django
    santiago at ~/.../ ✔ django-admin.py --version
    1.4.11

In this example, the tag retrieved is an annotated tag, signed by the
right developer. In this case signature verification would pass, and
there are no hints that something *might* have be wrong. Needless to
say, Django 1.4.11 is deprecated and vulnerable to really nasty XSS and
SQLi vectors...

I acknowledge that it is possible to provide this by using the SHA1 of
the tag object instead of the tag-ref, but this provides comparable
guarantees while keeping a readable interface. Of course that the sha1
is the "tightest" binding, so this also allows for developers to
remove tags during quick-fixes (as Junio pointed out in [1]) and other
edge cases in which the SHA1 would break.

Of course that using this flag needs to be integrated by package
managers and other tools; I wouldn't mind reaching out and even
proposing patches for the popular ones.

A stub of the intended patch follows. I'll can make a cleaner patch
(e.g., to include this in git tag -v) based on any feedback provided.

[1] http://thread.gmane.org/gmane.comp.version-control.git/284757
[2] http://thread.gmane.org/gmane.comp.version-control.git/288439

---
 builtin/verify-tag.c | 1 +
 tag.c                | 8 ++++++++
 tag.h                | 1 +
 3 files changed, 10 insertions(+)

diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 99f8148..947babe 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, "check-name", &flags, N_("Verify the tag name"), TAG_VERIFY_NAME),
 		OPT_END()
 	};
 

diff --git a/tag.c b/tag.c
index d1dcd18..591b31e 100644
--- a/tag.c
+++ b/tag.c

@@ -55,6 +55,14 @@ int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report,
 
 	ret = run_gpg_verify(buf, size, flags);
 
+	if (flags & TAG_VERIFY_NAME) {
+		struct tag tag_info;
+		ret += parse_tag_buffer(&tag_info, buf, size);
+		if strncmp(tag_info.tag, name_to_report, size)
+			ret += error("tag name doesn't match tag header!(%s)",
+					tag_info.tag);
+	}
+
 	free(buf);
 	return ret;
 }

diff --git a/tag.h b/tag.h
index a5721b6..30c922e 100644
--- a/tag.h
+++ b/tag.h

@@ -2,6 +2,7 @@
 #define TAG_H
 
 #include "object.h"
+#define TAG_VERIFY_NAME 0x10
 
 extern const char *tag_type;
 

-- 
2.8.3

On Tue, Jun 07, 2016 at 03:56:08PM -0400, santiago@nyu.edu wrote:

Er, is this C? :)

I think the general idea of an option to check the tag-name is a good
one. But there are some corner cases to think about:

  1. What name are we comparing against? Presumably it comes from the
     name the user gave us that resolved to the tag object. We would
     want to shorten "refs/tags/v1.4" to just "v1.4", I would think.

     Would a user ever want to pass a different tagname?

  2. What do we do for non-annotated tags? Is it always a failure?

-Peff

On Tue, Jun 07, 2016 at 05:13:14PM -0400, Santiago Torres wrote:

quoted

  2. What do we do for non-annotated tags? Is it always a failure?

Right now, verify-tag fails with non-annotated tags like this: 

    santiago at ~/.../git ✔ ./git-verify-tag master
    error: master: cannot verify a non-tag object of type commit.

Although we could change this behavior. I would have to check how git
tag -v works as this behavior might change.

Oh, right. That makes sense. I think it's not worth worrying about that
case, then.

-Peff

On Tue, Jun 07, 2016 at 05:08:56PM -0400, Jeff King wrote:

On Tue, Jun 07, 2016 at 03:56:08PM -0400, santiago@nyu.edu wrote:

quoted

diff --git a/tag.c b/tag.c
index d1dcd18..591b31e 100644
--- a/tag.c
+++ b/tag.c

@@ -55,6 +55,14 @@ int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report,
 
 	ret = run_gpg_verify(buf, size, flags);
 
+	if (flags & TAG_VERIFY_NAME) {
+		struct tag tag_info;
+		ret += parse_tag_buffer(&tag_info, buf, size);
+		if strncmp(tag_info.tag, name_to_report, size)
+			ret += error("tag name doesn't match tag header!(%s)",
+					tag_info.tag);
+	}

Er, is this C? :)

Yeah, I promise this would be a "cleaner" patch in the future :P

I think the general idea of an option to check the tag-name is a good
one. But there are some corner cases to think about:

  1. What name are we comparing against? Presumably it comes from the
     name the user gave us that resolved to the tag object. We would
     want to shorten "refs/tags/v1.4" to just "v1.4", I would think.

     Would a user ever want to pass a different tagname?

Yeah, I was wondering this. It might be convenient for them to call

git verify-tag --check-name [name] [ref] 

In case the ref doesn't match the tag. I can do it either way, although
the second case would be cumbersome.

  2. What do we do for non-annotated tags? Is it always a failure?

Right now, verify-tag fails with non-annotated tags like this: 

    santiago at ~/.../git ✔ ./git-verify-tag master
    error: master: cannot verify a non-tag object of type commit.

Although we could change this behavior. I would have to check how git
tag -v works as this behavior might change.

-Peff