threads / rfc / 42558

RFC patchverify-tag: add --check-name flag

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

## tl;dr

21 messages between Jun 7, 2016 and Jun 16, 2016. Diffs are folded; open one to read it.

replies: 20people: 4as markdown or json

santiago@nyu.edu· Jun 7, 2016, 19:56 UTC · lore
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(+)
Show changes to 3 files +10 −0

builtin/verify-tag.c, tag.c, tag.h

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
Junio C Hamano· Jun 7, 2016, 21:05 UTC · re: santiago@nyu.edu · lore

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

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.

Jeff King· Jun 7, 2016, 21:17 UTC · re: Junio C Hamano · lore

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

On Tue, Jun 07, 2016 at 02:05:20PM -0700, Junio C Hamano wrote:
Show 20 quoted lines
> 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:

Show 11 quoted lines
> 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
Santiago Torres· Jun 7, 2016, 21:30 UTC · re: Jeff King · lore

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

On Tue, Jun 07, 2016 at 05:17:07PM -0400, Jeff King wrote:
Show 11 quoted lines
> 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?

Show 10 quoted lines
> 
> 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).

Show 15 quoted lines
> 
> > 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.

Junio C Hamano· Jun 7, 2016, 21:50 UTC · re: Jeff King · lore

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

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.
Show 14 quoted lines
>> 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?
Jeff King· Jun 7, 2016, 21:55 UTC · re: Junio C Hamano · lore

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

On Tue, Jun 07, 2016 at 02:50:23PM -0700, Junio C Hamano wrote:
Show 21 quoted lines
> >> 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
Junio C Hamano· Jun 7, 2016, 22:05 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 26 quoted lines
> 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.
Jeff King· Jun 7, 2016, 22:07 UTC · re: Junio C Hamano · lore

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

On Tue, Jun 07, 2016 at 03:05:50PM -0700, Junio C Hamano wrote:
Show 30 quoted lines
> 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
Junio C Hamano· Jun 7, 2016, 22:11 UTC · re: Jeff King · lore

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

On Tue, Jun 7, 2016 at 3:07 PM, Jeff King <peff@peff.net> wrote:
Show 7 quoted lines
>>
>> 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.
Jeff King· Jun 7, 2016, 22:13 UTC · re: Junio C Hamano · lore

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

On Tue, Jun 07, 2016 at 03:11:47PM -0700, Junio C Hamano wrote:
Show 16 quoted lines
> 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
Santiago Torres· Jun 7, 2016, 22:16 UTC · re: Jeff King · lore

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

On Tue, Jun 07, 2016 at 06:13:25PM -0400, Jeff King wrote:
Show 22 quoted lines
> 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.
Junio C Hamano· Jun 7, 2016, 22:21 UTC · re: Jeff King · lore

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

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?

Jeff King· Jun 7, 2016, 22:29 UTC · re: Junio C Hamano · lore

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

On Tue, Jun 07, 2016 at 03:21:48PM -0700, Junio C Hamano wrote:
Show 9 quoted lines
> 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
Junio C Hamano· Jun 7, 2016, 22:35 UTC · re: Jeff King · lore

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

On Tue, Jun 7, 2016 at 3:29 PM, Jeff King <peff@peff.net> wrote:
Show 7 quoted lines
> 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.
Santiago Torres· Jun 8, 2016, 14:21 UTC · re: Junio C Hamano · lore

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

On Tue, Jun 07, 2016 at 03:35:07PM -0700, Junio C Hamano wrote:
Show 16 quoted lines
> 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.

Junio C Hamano· Jun 8, 2016, 18:43 UTC · re: Santiago Torres · lore

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

Santiago Torres <santiago@nyu.edu> writes:
Show 9 quoted lines
> 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.
Michael J Gruber· Jun 9, 2016, 11:48 UTC · re: Junio C Hamano · lore

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

Junio C Hamano venit, vidit, dixit 08.06.2016 20:43:
Show 34 quoted lines
> 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
Santiago Torres· Jun 7, 2016, 21:20 UTC · re: Junio C Hamano · lore

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

On Tue, Jun 07, 2016 at 02:05:20PM -0700, Junio C Hamano wrote:
Show 19 quoted lines
> 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).

Show 11 quoted lines
> 
> 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.

Jeff King· Jun 7, 2016, 21:08 UTC · re: santiago@nyu.edu · lore

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

On Tue, Jun 07, 2016 at 03:56:08PM -0400, santiago@nyu.edu wrote:
Show 15 quoted lines
> 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
Santiago Torres· Jun 7, 2016, 21:13 UTC · re: Jeff King · lore

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

On Tue, Jun 07, 2016 at 05:08:56PM -0400, Jeff King wrote:
Show 19 quoted lines
> 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
Show 9 quoted lines
> 
> 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
Jeff King· Jun 7, 2016, 21:18 UTC · re: Santiago Torres · lore

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

On Tue, Jun 07, 2016 at 05:13:14PM -0400, Santiago Torres wrote:
Show 9 quoted lines
> >   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

← back to recent threads