{"thread":{"id":"42558","subject":"[RFC/PATCH] verify-tag: add --check-name flag","startedAt":"2016-06-07T19:56:08Z","lastAt":"2016-06-16T02:19:49Z","messageCount":21,"participants":["santiago@nyu.edu","Junio C Hamano","Jeff King","Santiago Torres","Michael J Gruber"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"288634","messageId":"20160607195608.16643-1-santiago@nyu.edu","threadId":"42558","inReplyTo":null,"subject":"[RFC/PATCH] verify-tag: add --check-name flag","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-06-07T19:56:08Z","receivedAt":"2016-06-07T19:56:08Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"From: Santiago Torres \n\nHello everyone,\n\nIn a previous thread [1] we discussed about the possibility of having a\n--check-name flag, for the tag-verify command (and possibly git tag -v).\nAlthough many points were in the table, I don't think that it was\nconclusive as to whether having it or not. Also, I noticed that the,\nrefactored interface requires really minmal changes to do this\n(see attached patch). \n\nThis is the behavior I had in mind:\n\n\n santiago at ~/.../.../git ✗ ./git-verify-tag --check-name v2.7.0-rc20000\n gpg: Signature made Tue 22 Dec 2015 05:46:04 PM EST using RSA ... \n gpg: Good signature from \"Junio C Hamano \" [...]\n ...\n error: tag name doesn't match tag header!(v2.7.0-rc2)\n\n santiago at ~/.../.../git ✗ ./git-verify-tag --check-name v2.7.0-rc2\n gpg: Signature made Tue 22 Dec 2015 05:46:04 PM EST using RSA ...\n gpg: Good signature from \"Junio C Hamano \" [...]\n ...\n\nThe rationale behind this is the following:\n\n1.- Using a tag ref as a check-out mechanism is pretty common by package\n managers and other tools. Verifying the tag signature provides\n authentication guarantees, but there is no feedback that the\n signature being verified belongs to the intended tag.\n\n2.- The tuple tagname + signature can uniquely identify a tag. There\n are many tags that can have the same name, but this is mostly due\n to the naming policy. Having a tag-ref pointing to the right tag\n name with an appropriate signature provides tigther guarantees about\n the tag that's being checked-out.\n\n3.- This follows concerns about other people who wish to provide a\n tighter binding between the refs and the tag objects. The git-evtag\n project is an example of this[2].\n\nWhat I want to prevent is the following: \n\n santiago at ~/.../ ✔ pip install -e git+https://.../django/@1.8.3#egg=django\n Obtaining django from git+https://.../django/@1.8.3#egg=django\n [...] \n Successfully installed django\n santiago at ~/.../ ✔ django-admin.py --version\n 1.4.11\n\nIn this example, the tag retrieved is an annotated tag, signed by the\nright developer. In this case signature verification would pass, and\nthere are no hints that something *might* have be wrong. Needless to\nsay, Django 1.4.11 is deprecated and vulnerable to really nasty XSS and\nSQLi vectors...\n\nI acknowledge that it is possible to provide this by using the SHA1 of\nthe tag object instead of the tag-ref, but this provides comparable\nguarantees while keeping a readable interface. Of course that the sha1\nis the \"tightest\" binding, so this also allows for developers to\nremove tags during quick-fixes (as Junio pointed out in [1]) and other\nedge cases in which the SHA1 would break.\n\nOf course that using this flag needs to be integrated by package\nmanagers and other tools; I wouldn't mind reaching out and even\nproposing patches for the popular ones.\n\nA stub of the intended patch follows. I'll can make a cleaner patch\n(e.g., to include this in git tag -v) based on any feedback provided.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/284757\n[2] http://thread.gmane.org/gmane.comp.version-control.git/288439\n\n---\n builtin/verify-tag.c | 1 +\n tag.c | 8 ++++++++\n tag.h | 1 +\n 3 files changed, 10 insertions(+)\n\ndiff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\nindex 99f8148..947babe 100644\n--- a/builtin/verify-tag.c\n+++ b/builtin/verify-tag.c\n@@ -33,6 +33,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n \tconst struct option verify_tag_options[] = {\n \t\tOPT__VERBOSE(&verbose, N_(\"print tag contents\")),\n \t\tOPT_BIT(0, \"raw\", &flags, N_(\"print raw gpg status output\"), GPG_VERIFY_RAW),\n+\t\tOPT_BIT(0, \"check-name\", &flags, N_(\"Verify the tag name\"), TAG_VERIFY_NAME),\n \t\tOPT_END()\n \t};\n \ndiff --git a/tag.c b/tag.c\nindex d1dcd18..591b31e 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -55,6 +55,14 @@ int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report,\n \n \tret = run_gpg_verify(buf, size, flags);\n \n+\tif (flags & TAG_VERIFY_NAME) {\n+\t\tstruct tag tag_info;\n+\t\tret += parse_tag_buffer(&tag_info, buf, size);\n+\t\tif strncmp(tag_info.tag, name_to_report, size)\n+\t\t\tret += error(\"tag name doesn't match tag header!(%s)\",\n+\t\t\t\t\ttag_info.tag);\n+\t}\n+\n \tfree(buf);\n \treturn ret;\n }\ndiff --git a/tag.h b/tag.h\nindex a5721b6..30c922e 100644\n--- a/tag.h\n+++ b/tag.h\n@@ -2,6 +2,7 @@\n #define TAG_H\n \n #include \"object.h\"\n+#define TAG_VERIFY_NAME 0x10\n \n extern const char *tag_type;\n \n-- \n2.8.3\n"},{"id":"288644","messageId":"xmqq7fe0pv5b.fsf@gitster.mtv.corp.google.com","threadId":"42558","inReplyTo":"20160607195608.16643-1-santiago@nyu.edu","subject":"Re: [RFC/PATCH] verify-tag: add --check-name flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-07T21:05:20Z","receivedAt":"2016-06-07T21:05:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"santiago@nyu.edu writes:\n\n> 1.- Using a tag ref as a check-out mechanism is pretty common by package\n> managers and other tools. Verifying the tag signature provides\n> authentication guarantees, but there is no feedback that the\n> signature being verified belongs to the intended tag.\n\nVery true.\n\nThe above means that the existing package managers and other tools\nneed to be updated with some new code that lets them learn how to\ntell if the tagname (in their refs/tags/ namespace) matches the\nintended \"real\" tag name, and your --check-name option could be\nthat.\n\nBut if you are adding new code to the existing package managers and\nother tools _anyway_, wouldn't it be a more direct solution to let\nthem learn how to tell what the intended \"real\" tag name is with\nthat new code?\n\nIt is true that \"git cat-file tag v1.4.11\" lets you examine all\nlines of a given tag object, but the calling program needs to pick\npieces apart with something like:\n\n\tgit cat-file tag v1.4.11 | sed -e '/^$/q' -e 's/^tag //p'\n\nwhich may be cumbersome. Perhaps, just like \"git tag -v v1.4.11\" is\na way to see if the contents of the tag is signed properly, if you\nadd \"git tag --show-tagname v1.4.11\" that does the above pipeline,\nthese package managers and other tools can be updated to\n\n\ttag=\"$1\"\n-\tif ! git tag -v \"$tag\"\n+\tif ! git tag -v \"$tag\" ||\n+\t test \"$tag\" != \"$(git tag --show-tagname $tag)\"\n then\n\t\techo >&2 \"Bad tag.\"\n exit 1\n\tfi\n\tmake dest=/usr/local/$package/$tag install\n\nOr it could even do this:\n\n\ttag=\"$1\"\n\tif ! git tag -v \"$tag\"\n\tif ! git tag -v \"$tag\"\n then\n\t\techo >&2 \"Bad tag.\"\n exit 1\n\tfi\n+\ttag=$(git tag --show-tagname $tag)\n\tmake dest=/usr/local/$package/$tag install\n\ni.e. ignore the refname entirely and use the \"real\" tagname it reads\nafter validating the signature as the name of the resulting version\ngetting installed, distributed and/or used.\n"},{"id":"288645","messageId":"20160607210856.GA6807@sigill.intra.peff.net","threadId":"42558","inReplyTo":"20160607195608.16643-1-santiago@nyu.edu","subject":"Re: [RFC/PATCH] verify-tag: add --check-name flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-07T21:08:56Z","receivedAt":"2016-06-07T21:08:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 07, 2016 at 03:56:08PM -0400, santiago@nyu.edu wrote:\n\n> diff --git a/tag.c b/tag.c\n> index d1dcd18..591b31e 100644\n> --- a/tag.c\n> +++ b/tag.c\n> @@ -55,6 +55,14 @@ int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report,\n> \n> \tret = run_gpg_verify(buf, size, flags);\n> \n> +\tif (flags & TAG_VERIFY_NAME) {\n> +\t\tstruct tag tag_info;\n> +\t\tret += parse_tag_buffer(&tag_info, buf, size);\n> +\t\tif strncmp(tag_info.tag, name_to_report, size)\n> +\t\t\tret += error(\"tag name doesn't match tag header!(%s)\",\n> +\t\t\t\t\ttag_info.tag);\n> +\t}\n\nEr, is this C? :)\n\nI think the general idea of an option to check the tag-name is a good\none. But there are some corner cases to think about:\n\n 1. What name are we comparing against? Presumably it comes from the\n name the user gave us that resolved to the tag object. We would\n want to shorten \"refs/tags/v1.4\" to just \"v1.4\", I would think.\n\n Would a user ever want to pass a different tagname?\n\n 2. What do we do for non-annotated tags? Is it always a failure?\n\n-Peff\n"},{"id":"288650","messageId":"20160607211313.GD24676@LykOS.localdomain","threadId":"42558","inReplyTo":"20160607210856.GA6807@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] verify-tag: add --check-name flag","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-06-07T21:13:14Z","receivedAt":"2016-06-07T21:13:14Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"On Tue, Jun 07, 2016 at 05:08:56PM -0400, Jeff King wrote:\n> On Tue, Jun 07, 2016 at 03:56:08PM -0400, santiago@nyu.edu wrote:\n> \n> > diff --git a/tag.c b/tag.c\n> > index d1dcd18..591b31e 100644\n> > --- a/tag.c\n> > +++ b/tag.c\n> > @@ -55,6 +55,14 @@ int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report,\n> > \n> > \tret = run_gpg_verify(buf, size, flags);\n> > \n> > +\tif (flags & TAG_VERIFY_NAME) {\n> > +\t\tstruct tag tag_info;\n> > +\t\tret += parse_tag_buffer(&tag_info, buf, size);\n> > +\t\tif strncmp(tag_info.tag, name_to_report, size)\n> > +\t\t\tret += error(\"tag name doesn't match tag header!(%s)\",\n> > +\t\t\t\t\ttag_info.tag);\n> > +\t}\n> \n> Er, is this C? :)\n\nYeah, I promise this would be a \"cleaner\" patch in the future :P\n\n> \n> I think the general idea of an option to check the tag-name is a good\n> one. But there are some corner cases to think about:\n> \n> 1. What name are we comparing against? Presumably it comes from the\n> name the user gave us that resolved to the tag object. We would\n> want to shorten \"refs/tags/v1.4\" to just \"v1.4\", I would think.\n> \n> Would a user ever want to pass a different tagname?\n\nYeah, I was wondering this. It might be convenient for them to call\n\ngit verify-tag --check-name [name] [ref] \n\nIn case the ref doesn't match the tag. I can do it either way, although\nthe second case would be cumbersome.\n\n> \n> 2. What do we do for non-annotated tags? Is it always a failure?\n\nRight now, verify-tag fails with non-annotated tags like this: \n\n santiago at ~/.../git ✔ ./git-verify-tag master\n error: master: cannot verify a non-tag object of type commit.\n\nAlthough we could change this behavior. I would have to check how git\ntag -v works as this behavior might change.\n\n> \n> -Peff\n"},{"id":"288646","messageId":"20160607211707.GA7981@sigill.intra.peff.net","threadId":"42558","inReplyTo":"xmqq7fe0pv5b.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH] verify-tag: add --check-name flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-07T21:17:07Z","receivedAt":"2016-06-07T21:17:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 07, 2016 at 02:05:20PM -0700, Junio C Hamano wrote:\n\n> It is true that \"git cat-file tag v1.4.11\" lets you examine all\n> lines of a given tag object, but the calling program needs to pick\n> pieces apart with something like:\n> \n> \tgit cat-file tag v1.4.11 | sed -e '/^$/q' -e 's/^tag //p'\n> \n> which may be cumbersome. Perhaps, just like \"git tag -v v1.4.11\" is\n> a way to see if the contents of the tag is signed properly, if you\n> add \"git tag --show-tagname v1.4.11\" that does the above pipeline,\n> these package managers and other tools can be updated to\n> \n> \ttag=\"$1\"\n> -\tif ! git tag -v \"$tag\"\n> +\tif ! git tag -v \"$tag\" ||\n> +\t test \"$tag\" != \"$(git tag --show-tagname $tag)\"\n> then\n> \t\techo >&2 \"Bad tag.\"\n> exit 1\n> \tfi\n> \tmake dest=/usr/local/$package/$tag install\n\nThat is much more flexible, as they could even do some more complicated\nmatching than a single string (though in practice, for security things,\nI think simpler is better).\n\nI think this option is going to become a blueprint for other \"extended\"\nchecks, too. E.g., you might also want to check that the tagger ident\nmatches the uid on the signing key.\n\nMy main worry is that we'll accrue a whole bunch of such logic. And even\nthough each one is relatively simple, it would be nice for callers to be\nable to ask us to just do the standard safety checks.\n\nIf we do go with the \"print it out and let the caller do their own\nchecks\" strategy, I think I'd prefer rather than \"--show-tagname\" to\njust respect the \"--format\" we use for tag-listing. That would let you\ndo:\n\n git tag -v --format='%(tag)%n%(tagger)'\n\nor similar. In fact you can already do that with a separate step (modulo\n%n, which we do not seem to understand here), but like your example:\n\n> Or it could even do this:\n> \n> \ttag=\"$1\"\n> \tif ! git tag -v \"$tag\"\n> \tif ! git tag -v \"$tag\"\n> then\n> \t\techo >&2 \"Bad tag.\"\n> exit 1\n> \tfi\n> +\ttag=$(git tag --show-tagname $tag)\n> \tmake dest=/usr/local/$package/$tag install\n\nIt is racy. That probably doesn't matter for most callers, but it would\nbe nice to be able to get a custom format out of the \"-v\" invocation.\n\n-Peff\n"},{"id":"288647","messageId":"20160607211845.GA7696@sigill.intra.peff.net","threadId":"42558","inReplyTo":"20160607211313.GD24676@LykOS.localdomain","subject":"Re: [RFC/PATCH] verify-tag: add --check-name flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-07T21:18:45Z","receivedAt":"2016-06-07T21:18:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 07, 2016 at 05:13:14PM -0400, Santiago Torres wrote:\n\n> > 2. What do we do for non-annotated tags? Is it always a failure?\n> \n> Right now, verify-tag fails with non-annotated tags like this: \n> \n> santiago at ~/.../git ✔ ./git-verify-tag master\n> error: master: cannot verify a non-tag object of type commit.\n> \n> Although we could change this behavior. I would have to check how git\n> tag -v works as this behavior might change.\n\nOh, right. That makes sense. I think it's not worth worrying about that\ncase, then.\n\n-Peff\n"},{"id":"288648","messageId":"20160607212023.GE24676@LykOS.localdomain","threadId":"42558","inReplyTo":"xmqq7fe0pv5b.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH] verify-tag: add --check-name flag","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-06-07T21:20:24Z","receivedAt":"2016-06-07T21:20:24Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"On Tue, Jun 07, 2016 at 02:05:20PM -0700, Junio C Hamano wrote:\n> santiago@nyu.edu writes:\n> \n> > 1.- Using a tag ref as a check-out mechanism is pretty common by package\n> > managers and other tools. Verifying the tag signature provides\n> > authentication guarantees, but there is no feedback that the\n> > signature being verified belongs to the intended tag.\n> \n> Very true.\n> \n> The above means that the existing package managers and other tools\n> need to be updated with some new code that lets them learn how to\n> tell if the tagname (in their refs/tags/ namespace) matches the\n> intended \"real\" tag name, and your --check-name option could be\n> that.\n> \n> But if you are adding new code to the existing package managers and\n> other tools _anyway_, wouldn't it be a more direct solution to let\n> them learn how to tell what the intended \"real\" tag name is with\n> that new code?\n\nYeah, you're right, I didn't consider that. I'm thinking that this kind\nof verification could simplify the lives of upstream maintainers if we\ndo the verification in-house though (i.e., by having them just add the\nflag).\n\n> \n> which may be cumbersome. Perhaps, just like \"git tag -v v1.4.11\" is\n> a way to see if the contents of the tag is signed properly, if you\n> add \"git tag --show-tagname v1.4.11\" that does the above pipeline,\n> these package managers and other tools can be updated to\n> ...\n> make dest=/usr/local/$package/$tag install\n> \n> i.e. ignore the refname entirely and use the \"real\" tagname it reads\n> after validating the signature as the name of the resulting version\n> getting installed, distributed and/or used.\n\nThis is also an alternative, that might be cleaner. I'm wondering if\nthis is easier to implement than having the --check-name flag.\nIntuitively, it seems like that's the case. Would you suggest taking\nthis path instead?\n\nThanks!\n-Santiago.\n"},{"id":"288649","messageId":"20160607213050.GF24676@LykOS.localdomain","threadId":"42558","inReplyTo":"20160607211707.GA7981@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] verify-tag: add --check-name flag","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-06-07T21:30:50Z","receivedAt":"2016-06-07T21:30:50Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"On Tue, Jun 07, 2016 at 05:17:07PM -0400, Jeff King wrote:\n\n> That is much more flexible, as they could even do some more complicated\n> matching than a single string (though in practice, for security things,\n> I think simpler is better).\n> \n> I think this option is going to become a blueprint for other \"extended\"\n> checks, too. E.g., you might also want to check that the tagger ident\n> matches the uid on the signing key.\n> \n> My main worry is that we'll accrue a whole bunch of such logic. And even\n> though each one is relatively simple, it would be nice for callers to be\n> able to ask us to just do the standard safety checks.\n\nI agree with this. I can't think of other checks off the top of my head,\nbut I wouldn't be surprised if this is the case. \n\nI think that having custom flags for each check can also derive in each\npackage manager/user picking each check based on many different\nrationales, which might lead to people overcomplicating things?\n\n> \n> If we do go with the \"print it out and let the caller do their own\n> checks\" strategy, I think I'd prefer rather than \"--show-tagname\" to\n> just respect the \"--format\" we use for tag-listing. That would let you\n> do:\n> \n> git tag -v --format='%(tag)%n%(tagger)'\n> \n> or similar. In fact you can already do that with a separate step (modulo\n> %n, which we do not seem to understand here), but like your example:\n\nIt worries me that, in this case, the patches for upstream managers\nmight be harder to integrate/pitch for users.\n\nAlso, maybe we could take both strategies? add a --check-name for\nverify-tag and a --format for tag -v (I think either change is easy\nenough to do).\n\n> \n> > Or it could even do this:\n> > \n> > \ttag=\"$1\"\n> > \tif ! git tag -v \"$tag\"\n> > \tif ! git tag -v \"$tag\"\n> > then\n> > \t\techo >&2 \"Bad tag.\"\n> > exit 1\n> > \tfi\n> > +\ttag=$(git tag --show-tagname $tag)\n> > \tmake dest=/usr/local/$package/$tag install\n> \n> It is racy. That probably doesn't matter for most callers, but it would\n> be nice to be able to get a custom format out of the \"-v\" invocation.\n\nOh yeah, I didn't consider this either. I also don't think it's such an\nissue, but it sounds like a good idea not to have these races.\n\n> \n> -Peff\n\nThanks!\n-Santiago.\n"},{"id":"288651","messageId":"xmqq37oopt28.fsf@gitster.mtv.corp.google.com","threadId":"42558","inReplyTo":"20160607211707.GA7981@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] verify-tag: add --check-name flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-07T21:50:23Z","receivedAt":"2016-06-07T21:50:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> git tag -v --format='%(tag)%n%(tagger)'\n>\n> or similar. In fact you can already do that with a separate step (modulo\n> %n, which we do not seem to understand here), but like your example:\n\nYes, \"--format=%(tag)\" is all that is needed to make the example work.\n\n>> Or it could even do this:\n>> \n>> \ttag=\"$1\"\n>> \tif ! git tag -v \"$tag\"\n>> \tif ! git tag -v \"$tag\"\n>> then\n>> \t\techo >&2 \"Bad tag.\"\n>> exit 1\n>> \tfi\n>> +\ttag=$(git tag --show-tagname $tag)\n>> \tmake dest=/usr/local/$package/$tag install\n>\n> It is racy. That probably doesn't matter for most callers, but it would\n> be nice to be able to get a custom format out of the \"-v\" invocation.\n\nHeh, you can do\n\n-\ttag=\"$1\"\n+\ttag=$(git rev-parse --verify \"$1\")\n\nupfront and it no longer is racy, no?\n"},{"id":"288652","messageId":"20160607215536.GA20768@sigill.intra.peff.net","threadId":"42558","inReplyTo":"xmqq37oopt28.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH] verify-tag: add --check-name flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-07T21:55:37Z","receivedAt":"2016-06-07T21:55:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 07, 2016 at 02:50:23PM -0700, Junio C Hamano wrote:\n\n> >> Or it could even do this:\n> >> \n> >> \ttag=\"$1\"\n> >> \tif ! git tag -v \"$tag\"\n> >> \tif ! git tag -v \"$tag\"\n> >> then\n> >> \t\techo >&2 \"Bad tag.\"\n> >> exit 1\n> >> \tfi\n> >> +\ttag=$(git tag --show-tagname $tag)\n> >> \tmake dest=/usr/local/$package/$tag install\n> >\n> > It is racy. That probably doesn't matter for most callers, but it would\n> > be nice to be able to get a custom format out of the \"-v\" invocation.\n> \n> Heh, you can do\n> \n> -\ttag=\"$1\"\n> +\ttag=$(git rev-parse --verify \"$1\")\n> \n> upfront and it no longer is racy, no?\n\nYes, though that doesn't quite work today. The formatted output comes\nfrom \"tag -l\", which wants a refname. You can almost use \"git show\", but\nits format specifiers don't do tag parsing (they probably should).\nLikewise you can't use cat-file, as it doesn't do any intra-object\nparsing at all (which arguably it should).\n\nI'm still not sure I like the direction, simply because it requires 3\ninvocations of git to accomplish this task (which is inefficient in\nterms of processes, but also just means the interface is a bit tedious\nto work with; we could be making this easier for the caller).\n\n-Peff\n"},{"id":"288653","messageId":"xmqqy46gods1.fsf@gitster.mtv.corp.google.com","threadId":"42558","inReplyTo":"20160607215536.GA20768@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] verify-tag: add --check-name flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-07T22:05:50Z","receivedAt":"2016-06-07T22:05:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Jun 07, 2016 at 02:50:23PM -0700, Junio C Hamano wrote:\n>\n>> >> Or it could even do this:\n>> >> \n>> >> \ttag=\"$1\"\n>> >> \tif ! git tag -v \"$tag\"\n>> >> \tif ! git tag -v \"$tag\"\n>> >> then\n>> >> \t\techo >&2 \"Bad tag.\"\n>> >> exit 1\n>> >> \tfi\n>> >> +\ttag=$(git tag --show-tagname $tag)\n>> >> \tmake dest=/usr/local/$package/$tag install\n>> >\n>> > It is racy. That probably doesn't matter for most callers, but it would\n>> > be nice to be able to get a custom format out of the \"-v\" invocation.\n>> \n>> Heh, you can do\n>> \n>> -\ttag=\"$1\"\n>> +\ttag=$(git rev-parse --verify \"$1\")\n>> \n>> upfront and it no longer is racy, no?\n>\n> Yes, though that doesn't quite work today. The formatted output comes\n> from \"tag -l\", which wants a refname.\n\nPuzzled. I didn't even use --format=%(tagname) in the above.\n"},{"id":"288654","messageId":"20160607220743.GA21043@sigill.intra.peff.net","threadId":"42558","inReplyTo":"xmqqy46gods1.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH] verify-tag: add --check-name flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-07T22:07:44Z","receivedAt":"2016-06-07T22:07:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 07, 2016 at 03:05:50PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Tue, Jun 07, 2016 at 02:50:23PM -0700, Junio C Hamano wrote:\n> >\n> >> >> Or it could even do this:\n> >> >> \n> >> >> \ttag=\"$1\"\n> >> >> \tif ! git tag -v \"$tag\"\n> >> >> \tif ! git tag -v \"$tag\"\n> >> >> then\n> >> >> \t\techo >&2 \"Bad tag.\"\n> >> >> exit 1\n> >> >> \tfi\n> >> >> +\ttag=$(git tag --show-tagname $tag)\n> >> >> \tmake dest=/usr/local/$package/$tag install\n> >> >\n> >> > It is racy. That probably doesn't matter for most callers, but it would\n> >> > be nice to be able to get a custom format out of the \"-v\" invocation.\n> >> \n> >> Heh, you can do\n> >> \n> >> -\ttag=\"$1\"\n> >> +\ttag=$(git rev-parse --verify \"$1\")\n> >> \n> >> upfront and it no longer is racy, no?\n> >\n> > Yes, though that doesn't quite work today. The formatted output comes\n> > from \"tag -l\", which wants a refname.\n> \n> Puzzled. I didn't even use --format=%(tagname) in the above.\n\nNo, but you used --show-tagname, which does not exist today (and which\nIMHO should be implemented as --format). Would --show-tagname take\neither a tagname _or_ a sha1? I assume it would not be calling\nget_sha1(), as having it find refs/heads/$tag would be silly.\n\n-Peff\n"},{"id":"288656","messageId":"CAPc5daV=gqDLeFLB2csJDvNo4fpSKW_FjoB10TyroapQiHFq=A@mail.gmail.com","threadId":"42558","inReplyTo":"20160607220743.GA21043@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] verify-tag: add --check-name flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-07T22:11:47Z","receivedAt":"2016-06-07T22:11:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Tue, Jun 7, 2016 at 3:07 PM, Jeff King wrote:\n>>\n>> Puzzled. I didn't even use --format=%(tagname) in the above.\n>\n> No, but you used --show-tagname, which does not exist today (and which\n> IMHO should be implemented as --format). Would --show-tagname take\n> either a tagname _or_ a sha1? I assume it would not be calling\n> get_sha1(), as having it find refs/heads/$tag would be silly.\n\nAnd you do not even want to rely on where refs/tags/* it lives.\nshow-tagname, as I hinted in the first response, was meant to be\na short-hand for\n\n git cat-file tag $tag_object_name | sed -e '/^$/q' -e 's/^tag //p'\n\nso I am still puzzled.\n"},{"id":"288657","messageId":"20160607221325.GA21166@sigill.intra.peff.net","threadId":"42558","inReplyTo":"CAPc5daV=gqDLeFLB2csJDvNo4fpSKW_FjoB10TyroapQiHFq=A@mail.gmail.com","subject":"Re: [RFC/PATCH] verify-tag: add --check-name flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-07T22:13:25Z","receivedAt":"2016-06-07T22:13:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 07, 2016 at 03:11:47PM -0700, Junio C Hamano wrote:\n\n> On Tue, Jun 7, 2016 at 3:07 PM, Jeff King wrote:\n> >>\n> >> Puzzled. I didn't even use --format=%(tagname) in the above.\n> >\n> > No, but you used --show-tagname, which does not exist today (and which\n> > IMHO should be implemented as --format). Would --show-tagname take\n> > either a tagname _or_ a sha1? I assume it would not be calling\n> > get_sha1(), as having it find refs/heads/$tag would be silly.\n> \n> And you do not even want to rely on where refs/tags/* it lives.\n> show-tagname, as I hinted in the first response, was meant to be\n> a short-hand for\n> \n> git cat-file tag $tag_object_name | sed -e '/^$/q' -e 's/^tag //p'\n> \n> so I am still puzzled.\n\nIf you are suggesting that you can do the whole thing today by parsing\nthe tag object yourself, then sure, I agree. I thought the point of the\nexercise was to make that less painful for the callers.\n\n-Peff\n"},{"id":"288658","messageId":"20160607221621.GG24676@LykOS.localdomain","threadId":"42558","inReplyTo":"20160607221325.GA21166@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] verify-tag: add --check-name flag","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-06-07T22:16:22Z","receivedAt":"2016-06-07T22:16:22Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"On Tue, Jun 07, 2016 at 06:13:25PM -0400, Jeff King wrote:\n> On Tue, Jun 07, 2016 at 03:11:47PM -0700, Junio C Hamano wrote:\n> \n> > On Tue, Jun 7, 2016 at 3:07 PM, Jeff King wrote:\n> > >>\n> > >> Puzzled. I didn't even use --format=%(tagname) in the above.\n> > >\n> > > No, but you used --show-tagname, which does not exist today (and which\n> > > IMHO should be implemented as --format). Would --show-tagname take\n> > > either a tagname _or_ a sha1? I assume it would not be calling\n> > > get_sha1(), as having it find refs/heads/$tag would be silly.\n> > \n> > And you do not even want to rely on where refs/tags/* it lives.\n> > show-tagname, as I hinted in the first response, was meant to be\n> > a short-hand for\n> > \n> > git cat-file tag $tag_object_name | sed -e '/^$/q' -e 's/^tag //p'\n> > \n> > so I am still puzzled.\n> \n> If you are suggesting that you can do the whole thing today by parsing\n> the tag object yourself, then sure, I agree. I thought the point of the\n> exercise was to make that less painful for the callers.\n\nThis is what I understand so far, it seems all of us are on the same\nunderstanding here?\n\n1.- we can do this right now by sed-ing out the tagname, but it might\n not be optimal.\n\n2.- We can, instead, provide a --format flag to git tag -v for the same\n purpose. Which would only print the tag (if the appropriate format\n string is provided)\n\nI still agree with the rest of Peff's comments about this approach. I'm\nnot sure about which approach to take either.\n\n-Santiago.\n"},{"id":"288659","messageId":"xmqqk2i0od1f.fsf@gitster.mtv.corp.google.com","threadId":"42558","inReplyTo":"20160607221325.GA21166@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] verify-tag: add --check-name flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-07T22:21:48Z","receivedAt":"2016-06-07T22:21:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If you are suggesting that you can do the whole thing today by parsing\n> the tag object yourself, then sure, I agree. I thought the point of the\n> exercise was to make that less painful for the callers.\n\nYes, and I somehow thought everybody agreed that --show-tag-name was\nstriking the balance at about the right level for ease-of-use and\nsimplicity?\n"},{"id":"288661","messageId":"20160607222908.GA25631@sigill.intra.peff.net","threadId":"42558","inReplyTo":"xmqqk2i0od1f.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH] verify-tag: add --check-name flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-07T22:29:09Z","receivedAt":"2016-06-07T22:29:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 07, 2016 at 03:21:48PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > If you are suggesting that you can do the whole thing today by parsing\n> > the tag object yourself, then sure, I agree. I thought the point of the\n> > exercise was to make that less painful for the callers.\n> \n> Yes, and I somehow thought everybody agreed that --show-tag-name was\n> striking the balance at about the right level for ease-of-use and\n> simplicity?\n\nNo, I think \"--format\" would be much better, unless you want to add a\nseparate \"--show-tagger-ident\" when somebody wants to do a check between\nthe tagger's ident and the key uid.\n\nBut either way, I think the whole \"do a rev-parse first\" thing raises\nthe question of what object identifiers \"git tag\" would accept. We would\npresumably expect:\n\n git tag --show-tag-name v1.0\n\nto work. And I think in your world-view, so would:\n\n git tag --show-tag-name $(git rev-parse v1.0)\n\nHow about:\n\n git tag --show-tag-name refs/tags/v1.0\n\nAnd what about:\n\n git tag --show-tag-name refs/remotes/foo/v1.0\n\nor even:\n\n git tag --show-tag-name foo/v1.0\n\nwhen refs/remotes/foo/v1.0 exists?\n\nThe rule right now is generally that \"git tag\" takes actual tag names.\nPlumbing like \"verify-tag\" takes arbitrary get_sha1() expressions, but\nyou're expected to qualify or resolve your refnames before you get\nthere, to avoid weird situations. This \"tag --show-tag-name\" seems to\nsit in the middle of plumbing and porcelain (for that matter, I am not\nsure that it should belong to git-tag at all, as it is really about\nscripting).\n\n-Peff\n"},{"id":"288667","messageId":"CAPc5daV9ZvHqFtdzr565vp6Mv7O66ySr-p5Vi8o6bd6=GyVELg@mail.gmail.com","threadId":"42558","inReplyTo":"20160607222908.GA25631@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] verify-tag: add --check-name flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-07T22:35:07Z","receivedAt":"2016-06-07T22:35:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Tue, Jun 7, 2016 at 3:29 PM, Jeff King wrote:\n> or even:\n>\n> git tag --show-tag-name foo/v1.0\n>\n> when refs/remotes/foo/v1.0 exists?\n>\n> The rule right now is generally that \"git tag\" takes actual tag names.\n\nAhh, I forgot about that. Yes, indeed the command does not work\nlike other Git commands, which would just let generic revision parser\nto accept an object name from anywhere. Probably it was a mistake,\nbecause \"git tag --verify $T\" may find refs/tags/$T but that may not\nnecessarily mean \"git checkout $T^0\" would give you that exact\ntree state, but it is too late to change now.\n\nSo yes, I agree with you that any validation-related thing needs to\nstart from names relative to refs/tags/, not from object names, to be\nconsistent.\n\nWhich is a bit sad, but I do not offhand think of a way to avoid it.\n"},{"id":"288720","messageId":"20160608142132.GA32299@LykOS.localdomain","threadId":"42558","inReplyTo":"CAPc5daV9ZvHqFtdzr565vp6Mv7O66ySr-p5Vi8o6bd6=GyVELg@mail.gmail.com","subject":"Re: [RFC/PATCH] verify-tag: add --check-name flag","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-06-08T14:21:33Z","receivedAt":"2016-06-08T14:21:33Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"On Tue, Jun 07, 2016 at 03:35:07PM -0700, Junio C Hamano wrote:\n> On Tue, Jun 7, 2016 at 3:29 PM, Jeff King wrote:\n> > or even:\n> >\n> > git tag --show-tag-name foo/v1.0\n> >\n> > when refs/remotes/foo/v1.0 exists?\n> >\n> > The rule right now is generally that \"git tag\" takes actual tag names.\n> \n> Ahh, I forgot about that. Yes, indeed the command does not work\n> like other Git commands, which would just let generic revision parser\n> to accept an object name from anywhere. Probably it was a mistake,\n> because \"git tag --verify $T\" may find refs/tags/$T but that may not\n> necessarily mean \"git checkout $T^0\" would give you that exact\n> tree state, but it is too late to change now.\n> \n\nSorry I'm trying to follow this. Would it be best to then have\n\n verify-tag [--check-name=tagname] (tag-ref|tag-name|sha1)?\n\nand\n\n tag -v [--check-name] (tag-name)\n\nOr would --format still work better?\n\nThanks!\n-Santiago.\n"},{"id":"288752","messageId":"xmqqk2hzldx8.fsf@gitster.mtv.corp.google.com","threadId":"42558","inReplyTo":"20160608142132.GA32299@LykOS.localdomain","subject":"Re: [RFC/PATCH] verify-tag: add --check-name flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-08T18:43:15Z","receivedAt":"2016-06-08T18:43:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Santiago Torres writes:\n\n> Sorry I'm trying to follow this. Would it be best to then have\n>\n> verify-tag [--check-name=tagname] (tag-ref|tag-name|sha1)?\n>\n> and\n>\n> tag -v [--check-name] (tag-name)\n>\n> Or would --format still work better?\n\nNo matter what you do, don't call that \"--check-name\". It does not\ntell the users what aspect of that thing is \"checked\". Avoid being\nasked \"Does this check tagname to make sure it does not have\nnon-ASCII letters?\", in other words.\n\nAs a longer-term direction, I think the best one is to make what\npeff@ originally suggested, i.e.\n\n If we do go with the \"print it out and let the caller do their own\n checks\" strategy, I think I'd prefer rather than \"--show-tagname\" to\n just respect the \"--format\" we use for tag-listing. That would let you\n do:\n\n git tag -v --format='%(tag)%n%(tagger)'\n\n or similar. In fact you can already do that with a separate step (modulo\n %n, which we do not seem to understand here)...\n\nwork.\n\nThanks.\n"},{"id":"288801","messageId":"c4b944c7-d72b-d84b-7c53-79708af250cb@drmicha.warpmail.net","threadId":"42558","inReplyTo":"xmqqk2hzldx8.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH] verify-tag: add --check-name flag","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2016-06-09T11:48:02Z","receivedAt":"2016-06-16T02:19:49Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 08.06.2016 20:43:\n> Santiago Torres writes:\n> \n>> Sorry I'm trying to follow this. Would it be best to then have\n>>\n>> verify-tag [--check-name=tagname] (tag-ref|tag-name|sha1)?\n>>\n>> and\n>>\n>> tag -v [--check-name] (tag-name)\n>>\n>> Or would --format still work better?\n> \n> No matter what you do, don't call that \"--check-name\". It does not\n> tell the users what aspect of that thing is \"checked\". Avoid being\n> asked \"Does this check tagname to make sure it does not have\n> non-ASCII letters?\", in other words.\n> \n> As a longer-term direction, I think the best one is to make what\n> peff@ originally suggested, i.e.\n> \n> If we do go with the \"print it out and let the caller do their own\n> checks\" strategy, I think I'd prefer rather than \"--show-tagname\" to\n> just respect the \"--format\" we use for tag-listing. That would let you\n> do:\n> \n> git tag -v --format='%(tag)%n%(tagger)'\n> \n> or similar. In fact you can already do that with a separate step (modulo\n> %n, which we do not seem to understand here)...\n> \n> work.\n> \n> Thanks.\n> \n\nThe extent of this thread shows (again) that assigning trust is an\nindividual decision, and the base for that decision will be different in\ndifferent projects. (While the gpg project keeps emphasizing that, it\ndoesn't keep gpg users from thinking differently.)\n\nAll that git can realistically do is:\nA) provide the answer that gpg gives (which depends on the configured\ntrust model, available keys and the trustdb entries) - this is about\ntrust in the validity of the signature\n\nB) provide easy access to all data that a project may potentially want\nto use for their manual or automatic decision - this is about trust in\nthe meaning of the signature\n\nWe can do better for B), and we will with a for-each-ref'ed \"git tag\"\nthat knows format strings.\n\nNota bene: A) really requires a tightened keyring and trustdb etc.,\nsomething that is usually not found on the user side.\n\nIf we want to do all this \"in git\" we would need a \"plug-in\"\ninfrastructure/trust helper that receives the tag object and decides\nabout the trust (taking project specifics into account) - that is not\nthat much different from scripting it around git, and could probably be\n\"monkey patched in\" today by specifying a different \"gpg\".\n\nMichael\n"}]}