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

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

[santiago]

From: Santiago Torres <santiago@nyu.edu>

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 <gitster@pobox.com>" [...]
    ...
    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 <gitster@pobox.com>" [...]
    ...

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

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

[Junio C Hamano]

santiago@nyu.edu writes:

> 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.

Very true.

The above means that the existing package managers and other tools
need to be updated with some new code that lets them learn how to
tell if the tagname (in their refs/tags/ namespace) matches the
intended "real" tag name, and your --check-name option could be
that.

But if you are adding new code to the existing package managers and
other tools _anyway_, wouldn't it be a more direct solution to let
them learn how to tell what the intended "real" tag name is with
that new code?

It is true that "git cat-file tag v1.4.11" lets you examine all
lines of a given tag object, but the calling program needs to pick
pieces apart with something like:

	git cat-file tag v1.4.11 | sed -e '/^$/q' -e 's/^tag //p'

which may be cumbersome.  Perhaps, just like "git tag -v v1.4.11" is
a way to see if the contents of the tag is signed properly, if you
add "git tag --show-tagname v1.4.11" that does the above pipeline,
these package managers and other tools can be updated to

	tag="$1"
-	if ! git tag -v "$tag"
+	if ! git tag -v "$tag" ||
+	   test "$tag" != "$(git tag --show-tagname $tag)"
        then
		echo >&2 "Bad tag."
                exit 1
	fi
	make dest=/usr/local/$package/$tag install

Or it could even do this:

	tag="$1"
	if ! git tag -v "$tag"
	if ! git tag -v "$tag"
        then
		echo >&2 "Bad tag."
                exit 1
	fi
+	tag=$(git tag --show-tagname $tag)
	make dest=/usr/local/$package/$tag install

i.e. ignore the refname entirely and use the "real" tagname it reads
after validating the signature as the name of the resulting version
getting installed, distributed and/or used.

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

[Jeff King]

On Tue, Jun 07, 2016 at 02:05:20PM -0700, Junio C Hamano wrote:

> It is true that "git cat-file tag v1.4.11" lets you examine all
> lines of a given tag object, but the calling program needs to pick
> pieces apart with something like:
> 
> 	git cat-file tag v1.4.11 | sed -e '/^$/q' -e 's/^tag //p'
> 
> which may be cumbersome.  Perhaps, just like "git tag -v v1.4.11" is
> a way to see if the contents of the tag is signed properly, if you
> add "git tag --show-tagname v1.4.11" that does the above pipeline,
> these package managers and other tools can be updated to
> 
> 	tag="$1"
> -	if ! git tag -v "$tag"
> +	if ! git tag -v "$tag" ||
> +	   test "$tag" != "$(git tag --show-tagname $tag)"
>         then
> 		echo >&2 "Bad tag."
>                 exit 1
> 	fi
> 	make dest=/usr/local/$package/$tag install

That is much more flexible, as they could even do some more complicated
matching than a single string (though in practice, for security things,
I think simpler is better).

I think this option is going to become a blueprint for other "extended"
checks, too. E.g., you might also want to check that the tagger ident
matches the uid on the signing key.

My main worry is that we'll accrue a whole bunch of such logic. And even
though each one is relatively simple, it would be nice for callers to be
able to ask us to just do the standard safety checks.

If we do go with the "print it out and let the caller do their own
checks" strategy, I think I'd prefer rather than "--show-tagname" to
just respect the "--format" we use for tag-listing. That would let you
do:

  git tag -v --format='%(tag)%n%(tagger)'

or similar. In fact you can already do that with a separate step (modulo
%n, which we do not seem to understand here), but like your example:

> Or it could even do this:
> 
> 	tag="$1"
> 	if ! git tag -v "$tag"
> 	if ! git tag -v "$tag"
>         then
> 		echo >&2 "Bad tag."
>                 exit 1
> 	fi
> +	tag=$(git tag --show-tagname $tag)
> 	make dest=/usr/local/$package/$tag install

It is racy. That probably doesn't matter for most callers, but it would
be nice to be able to get a custom format out of the "-v" invocation.

-Peff

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

[Santiago Torres]

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

> That is much more flexible, as they could even do some more complicated
> matching than a single string (though in practice, for security things,
> I think simpler is better).
> 
> I think this option is going to become a blueprint for other "extended"
> checks, too. E.g., you might also want to check that the tagger ident
> matches the uid on the signing key.
> 
> My main worry is that we'll accrue a whole bunch of such logic. And even
> though each one is relatively simple, it would be nice for callers to be
> able to ask us to just do the standard safety checks.

I agree with this. I can't think of other checks off the top of my head,
but I wouldn't be surprised if this is the case. 

I think that having custom flags for each check can also derive in each
package manager/user picking each check based on many different
rationales, which might lead to people overcomplicating things?

> 
> If we do go with the "print it out and let the caller do their own
> checks" strategy, I think I'd prefer rather than "--show-tagname" to
> just respect the "--format" we use for tag-listing. That would let you
> do:
> 
>   git tag -v --format='%(tag)%n%(tagger)'
> 
> or similar. In fact you can already do that with a separate step (modulo
> %n, which we do not seem to understand here), but like your example:

It worries me that, in this case, the patches for upstream managers
might be harder to integrate/pitch for users.

Also, maybe we could take both strategies? add a --check-name for
verify-tag and a --format for tag -v (I think either change is easy
enough to do).

> 
> > Or it could even do this:
> > 
> > 	tag="$1"
> > 	if ! git tag -v "$tag"
> > 	if ! git tag -v "$tag"
> >         then
> > 		echo >&2 "Bad tag."
> >                 exit 1
> > 	fi
> > +	tag=$(git tag --show-tagname $tag)
> > 	make dest=/usr/local/$package/$tag install
> 
> It is racy. That probably doesn't matter for most callers, but it would
> be nice to be able to get a custom format out of the "-v" invocation.

Oh yeah, I didn't consider this either. I also don't think it's such an
issue, but it sounds like a good idea not to have these races.

> 
> -Peff

Thanks!
-Santiago.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   git tag -v --format='%(tag)%n%(tagger)'
>
> or similar. In fact you can already do that with a separate step (modulo
> %n, which we do not seem to understand here), but like your example:

Yes, "--format=%(tag)" is all that is needed to make the example work.

>> Or it could even do this:
>> 
>> 	tag="$1"
>> 	if ! git tag -v "$tag"
>> 	if ! git tag -v "$tag"
>>         then
>> 		echo >&2 "Bad tag."
>>                 exit 1
>> 	fi
>> +	tag=$(git tag --show-tagname $tag)
>> 	make dest=/usr/local/$package/$tag install
>
> It is racy. That probably doesn't matter for most callers, but it would
> be nice to be able to get a custom format out of the "-v" invocation.

Heh, you can do

-	tag="$1"
+	tag=$(git rev-parse --verify "$1")

upfront and it no longer is racy, no?

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

[Jeff King]

On Tue, Jun 07, 2016 at 02:50:23PM -0700, Junio C Hamano wrote:

> >> Or it could even do this:
> >> 
> >> 	tag="$1"
> >> 	if ! git tag -v "$tag"
> >> 	if ! git tag -v "$tag"
> >>         then
> >> 		echo >&2 "Bad tag."
> >>                 exit 1
> >> 	fi
> >> +	tag=$(git tag --show-tagname $tag)
> >> 	make dest=/usr/local/$package/$tag install
> >
> > It is racy. That probably doesn't matter for most callers, but it would
> > be nice to be able to get a custom format out of the "-v" invocation.
> 
> Heh, you can do
> 
> -	tag="$1"
> +	tag=$(git rev-parse --verify "$1")
> 
> upfront and it no longer is racy, no?

Yes, though that doesn't quite work today. The formatted output comes
from "tag -l", which wants a refname. You can almost use "git show", but
its format specifiers don't do tag parsing (they probably should).
Likewise you can't use cat-file, as it doesn't do any intra-object
parsing at all (which arguably it should).

I'm still not sure I like the direction, simply because it requires 3
invocations of git to accomplish this task (which is inefficient in
terms of processes, but also just means the interface is a bit tedious
to work with; we could be making this easier for the caller).

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Jun 07, 2016 at 02:50:23PM -0700, Junio C Hamano wrote:
>
>> >> Or it could even do this:
>> >> 
>> >> 	tag="$1"
>> >> 	if ! git tag -v "$tag"
>> >> 	if ! git tag -v "$tag"
>> >>         then
>> >> 		echo >&2 "Bad tag."
>> >>                 exit 1
>> >> 	fi
>> >> +	tag=$(git tag --show-tagname $tag)
>> >> 	make dest=/usr/local/$package/$tag install
>> >
>> > It is racy. That probably doesn't matter for most callers, but it would
>> > be nice to be able to get a custom format out of the "-v" invocation.
>> 
>> Heh, you can do
>> 
>> -	tag="$1"
>> +	tag=$(git rev-parse --verify "$1")
>> 
>> upfront and it no longer is racy, no?
>
> Yes, though that doesn't quite work today. The formatted output comes
> from "tag -l", which wants a refname.

Puzzled.  I didn't even use --format=%(tagname) in the above.

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

[Jeff King]

On Tue, Jun 07, 2016 at 03:05:50PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Tue, Jun 07, 2016 at 02:50:23PM -0700, Junio C Hamano wrote:
> >
> >> >> Or it could even do this:
> >> >> 
> >> >> 	tag="$1"
> >> >> 	if ! git tag -v "$tag"
> >> >> 	if ! git tag -v "$tag"
> >> >>         then
> >> >> 		echo >&2 "Bad tag."
> >> >>                 exit 1
> >> >> 	fi
> >> >> +	tag=$(git tag --show-tagname $tag)
> >> >> 	make dest=/usr/local/$package/$tag install
> >> >
> >> > It is racy. That probably doesn't matter for most callers, but it would
> >> > be nice to be able to get a custom format out of the "-v" invocation.
> >> 
> >> Heh, you can do
> >> 
> >> -	tag="$1"
> >> +	tag=$(git rev-parse --verify "$1")
> >> 
> >> upfront and it no longer is racy, no?
> >
> > Yes, though that doesn't quite work today. The formatted output comes
> > from "tag -l", which wants a refname.
> 
> Puzzled.  I didn't even use --format=%(tagname) in the above.

No, but you used --show-tagname, which does not exist today (and which
IMHO should be implemented as --format). Would --show-tagname take
either a tagname _or_ a sha1? I assume it would not be calling
get_sha1(), as having it find refs/heads/$tag would be silly.

-Peff

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

[Junio C Hamano]

On Tue, Jun 7, 2016 at 3:07 PM, Jeff King <peff@peff.net> wrote:
>>
>> Puzzled.  I didn't even use --format=%(tagname) in the above.
>
> No, but you used --show-tagname, which does not exist today (and which
> IMHO should be implemented as --format). Would --show-tagname take
> either a tagname _or_ a sha1? I assume it would not be calling
> get_sha1(), as having it find refs/heads/$tag would be silly.

And you do not even want to rely on where refs/tags/* it lives.
show-tagname, as I hinted in the first response, was meant to be
a short-hand for

       git cat-file tag $tag_object_name | sed -e '/^$/q' -e 's/^tag //p'

so I am still puzzled.

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

[Jeff King]

On Tue, Jun 07, 2016 at 03:11:47PM -0700, Junio C Hamano wrote:

> On Tue, Jun 7, 2016 at 3:07 PM, Jeff King <peff@peff.net> wrote:
> >>
> >> Puzzled.  I didn't even use --format=%(tagname) in the above.
> >
> > No, but you used --show-tagname, which does not exist today (and which
> > IMHO should be implemented as --format). Would --show-tagname take
> > either a tagname _or_ a sha1? I assume it would not be calling
> > get_sha1(), as having it find refs/heads/$tag would be silly.
> 
> And you do not even want to rely on where refs/tags/* it lives.
> show-tagname, as I hinted in the first response, was meant to be
> a short-hand for
> 
>        git cat-file tag $tag_object_name | sed -e '/^$/q' -e 's/^tag //p'
> 
> so I am still puzzled.

If you are suggesting that you can do the whole thing today by parsing
the tag object yourself, then sure, I agree. I thought the point of the
exercise was to make that less painful for the callers.

-Peff

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

[Santiago Torres]

On Tue, Jun 07, 2016 at 06:13:25PM -0400, Jeff King wrote:
> On Tue, Jun 07, 2016 at 03:11:47PM -0700, Junio C Hamano wrote:
> 
> > On Tue, Jun 7, 2016 at 3:07 PM, Jeff King <peff@peff.net> wrote:
> > >>
> > >> Puzzled.  I didn't even use --format=%(tagname) in the above.
> > >
> > > No, but you used --show-tagname, which does not exist today (and which
> > > IMHO should be implemented as --format). Would --show-tagname take
> > > either a tagname _or_ a sha1? I assume it would not be calling
> > > get_sha1(), as having it find refs/heads/$tag would be silly.
> > 
> > And you do not even want to rely on where refs/tags/* it lives.
> > show-tagname, as I hinted in the first response, was meant to be
> > a short-hand for
> > 
> >        git cat-file tag $tag_object_name | sed -e '/^$/q' -e 's/^tag //p'
> > 
> > so I am still puzzled.
> 
> If you are suggesting that you can do the whole thing today by parsing
> the tag object yourself, then sure, I agree. I thought the point of the
> exercise was to make that less painful for the callers.

This is what I understand so far, it seems all of us are on the same
understanding here?

1.- we can do this right now by sed-ing out the tagname, but it might
    not be optimal.

2.- We can, instead, provide a --format flag to git tag -v for the same
    purpose. Which would only print the tag (if the appropriate format
    string is provided)

I still agree with the rest of Peff's comments about this approach. I'm
not sure about which approach to take either.

-Santiago.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> If you are suggesting that you can do the whole thing today by parsing
> the tag object yourself, then sure, I agree. I thought the point of the
> exercise was to make that less painful for the callers.

Yes, and I somehow thought everybody agreed that --show-tag-name was
striking the balance at about the right level for ease-of-use and
simplicity?

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

[Jeff King]

On Tue, Jun 07, 2016 at 03:21:48PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > If you are suggesting that you can do the whole thing today by parsing
> > the tag object yourself, then sure, I agree. I thought the point of the
> > exercise was to make that less painful for the callers.
> 
> Yes, and I somehow thought everybody agreed that --show-tag-name was
> striking the balance at about the right level for ease-of-use and
> simplicity?

No, I think "--format" would be much better, unless you want to add a
separate "--show-tagger-ident" when somebody wants to do a check between
the tagger's ident and the key uid.

But either way, I think the whole "do a rev-parse first" thing raises
the question of what object identifiers "git tag" would accept. We would
presumably expect:

  git tag --show-tag-name v1.0

to work. And I think in your world-view, so would:

  git tag --show-tag-name $(git rev-parse v1.0)

How about:

  git tag --show-tag-name refs/tags/v1.0

And what about:

  git tag --show-tag-name refs/remotes/foo/v1.0

or even:

  git tag --show-tag-name foo/v1.0

when refs/remotes/foo/v1.0 exists?

The rule right now is generally that "git tag" takes actual tag names.
Plumbing like "verify-tag" takes arbitrary get_sha1() expressions, but
you're expected to qualify or resolve your refnames before you get
there, to avoid weird situations. This "tag --show-tag-name" seems to
sit in the middle of plumbing and porcelain (for that matter, I am not
sure that it should belong to git-tag at all, as it is really about
scripting).

-Peff

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

[Junio C Hamano]

On Tue, Jun 7, 2016 at 3:29 PM, Jeff King <peff@peff.net> wrote:
> or even:
>
>   git tag --show-tag-name foo/v1.0
>
> when refs/remotes/foo/v1.0 exists?
>
> The rule right now is generally that "git tag" takes actual tag names.

Ahh, I forgot about that. Yes, indeed the command does not work
like other Git commands, which would just let generic revision parser
to accept an object name from anywhere. Probably it was a mistake,
because "git tag --verify $T" may find refs/tags/$T but that may not
necessarily mean "git checkout $T^0" would give you that exact
tree state, but it is too late to change now.

So yes, I agree with you that any validation-related thing needs to
start from names relative to refs/tags/, not from object names, to be
consistent.

Which is a bit sad, but I do not offhand think of a way to avoid it.

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

[Santiago Torres]

On Tue, Jun 07, 2016 at 03:35:07PM -0700, Junio C Hamano wrote:
> On Tue, Jun 7, 2016 at 3:29 PM, Jeff King <peff@peff.net> wrote:
> > or even:
> >
> >   git tag --show-tag-name foo/v1.0
> >
> > when refs/remotes/foo/v1.0 exists?
> >
> > The rule right now is generally that "git tag" takes actual tag names.
> 
> Ahh, I forgot about that. Yes, indeed the command does not work
> like other Git commands, which would just let generic revision parser
> to accept an object name from anywhere. Probably it was a mistake,
> because "git tag --verify $T" may find refs/tags/$T but that may not
> necessarily mean "git checkout $T^0" would give you that exact
> tree state, but it is too late to change now.
> 

Sorry I'm trying to follow this. Would it be best to then have

    verify-tag [--check-name=tagname] (tag-ref|tag-name|sha1)?

and

    tag -v [--check-name] (tag-name)

Or would --format still work better?

Thanks!
-Santiago.

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

[Junio C Hamano]

Santiago Torres <santiago@nyu.edu> writes:

> Sorry I'm trying to follow this. Would it be best to then have
>
>     verify-tag [--check-name=tagname] (tag-ref|tag-name|sha1)?
>
> and
>
>     tag -v [--check-name] (tag-name)
>
> Or would --format still work better?

No matter what you do, don't call that "--check-name".  It does not
tell the users what aspect of that thing is "checked".  Avoid being
asked "Does this check tagname to make sure it does not have
non-ASCII letters?", in other words.

As a longer-term direction, I think the best one is to make what
peff@ originally suggested, i.e.

    If we do go with the "print it out and let the caller do their own
    checks" strategy, I think I'd prefer rather than "--show-tagname" to
    just respect the "--format" we use for tag-listing. That would let you
    do:

      git tag -v --format='%(tag)%n%(tagger)'

    or similar. In fact you can already do that with a separate step (modulo
    %n, which we do not seem to understand here)...

work.

Thanks.

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 08.06.2016 20:43:
> Santiago Torres <santiago@nyu.edu> writes:
> 
>> Sorry I'm trying to follow this. Would it be best to then have
>>
>>     verify-tag [--check-name=tagname] (tag-ref|tag-name|sha1)?
>>
>> and
>>
>>     tag -v [--check-name] (tag-name)
>>
>> Or would --format still work better?
> 
> No matter what you do, don't call that "--check-name".  It does not
> tell the users what aspect of that thing is "checked".  Avoid being
> asked "Does this check tagname to make sure it does not have
> non-ASCII letters?", in other words.
> 
> As a longer-term direction, I think the best one is to make what
> peff@ originally suggested, i.e.
> 
>     If we do go with the "print it out and let the caller do their own
>     checks" strategy, I think I'd prefer rather than "--show-tagname" to
>     just respect the "--format" we use for tag-listing. That would let you
>     do:
> 
>       git tag -v --format='%(tag)%n%(tagger)'
> 
>     or similar. In fact you can already do that with a separate step (modulo
>     %n, which we do not seem to understand here)...
> 
> work.
> 
> Thanks.
> 

The extent of this thread shows (again) that assigning trust is an
individual decision, and the base for that decision will be different in
different projects. (While the gpg project keeps emphasizing that, it
doesn't keep gpg users from thinking differently.)

All that git can realistically do is:
A) provide the answer that gpg gives (which depends on the configured
trust model, available keys and the trustdb entries) - this is about
trust in the validity of the signature

B) provide easy access to all data that a project may potentially want
to use for their manual or automatic decision - this is about trust in
the meaning of the signature

We can do better for B), and we will with a for-each-ref'ed "git tag"
that knows format strings.

Nota bene: A) really requires a tightened keyring and trustdb etc.,
something that is usually not found on the user side.

If we want to do all this "in git" we would need a "plug-in"
infrastructure/trust helper that receives the tag object and decides
about the trust (taking project specifics into account) - that is not
that much different from scripting it around git, and could probably be
"monkey patched in" today by specifying a different "gpg".

Michael

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

[Santiago Torres]

On Tue, Jun 07, 2016 at 02:05:20PM -0700, Junio C Hamano wrote:
> santiago@nyu.edu writes:
> 
> > 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.
> 
> Very true.
> 
> The above means that the existing package managers and other tools
> need to be updated with some new code that lets them learn how to
> tell if the tagname (in their refs/tags/ namespace) matches the
> intended "real" tag name, and your --check-name option could be
> that.
> 
> But if you are adding new code to the existing package managers and
> other tools _anyway_, wouldn't it be a more direct solution to let
> them learn how to tell what the intended "real" tag name is with
> that new code?

Yeah, you're right, I didn't consider that. I'm thinking that this kind
of verification could simplify the lives of upstream maintainers if we
do the verification in-house though (i.e., by having them just add the
flag).

> 
> which may be cumbersome.  Perhaps, just like "git tag -v v1.4.11" is
> a way to see if the contents of the tag is signed properly, if you
> add "git tag --show-tagname v1.4.11" that does the above pipeline,
> these package managers and other tools can be updated to
> ...
> make dest=/usr/local/$package/$tag install
> 
> i.e. ignore the refname entirely and use the "real" tagname it reads
> after validating the signature as the name of the resulting version
> getting installed, distributed and/or used.

This is also an alternative, that might be cleaner. I'm wondering if
this is easier to implement than having the --check-name flag.
Intuitively, it seems like that's the case. Would you suggest taking
this path instead?

Thanks!
-Santiago.

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

[Jeff King]

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

> 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? :)

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

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

[Santiago Torres]

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:
> 
> > 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

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

[Jeff King]

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

> >   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