[PATCH] hooks/update: Add a hooks.denyunsignedtags option

[PATCH] hooks/update: Add a hooks.denyunsignedtags option

[Julian Andres Klode]

Introduce an option to deny unsigned tags from entering
a repository. This is useful in teams where members forget
to sign their release tags.

It does not actually check whether the signature is actually
complete or valid, it just checks for the beginning of a
signature, as further checks would be too involved.

This effectively also denies un-annotated tags, as those
are unsigned by definition.

Signed-off-by: Julian Andres Klode <jak@debian.org>
---

Note: Submitted for review on Sep 12, re-asked on Sep 22, but
no feedback, so I assume it's good to go, see
http://thread.gmane.org/gmane.comp.version-control.git/277722
for details

 templates/hooks--update.sample | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/templates/hooks--update.sample b/templates/hooks--update.sample
index d847583..e261d30 100755
--- a/templates/hooks--update.sample
+++ b/templates/hooks--update.sample
@@ -16,6 +16,9 @@
 # hooks.allowmodifytag
 #   This boolean sets whether a tag may be modified after creation. By default
 #   it won't be.
+# hooks.denyunsignedtag
+#   This boolean sets whether creating unsigned tags will be denied. By
+#   default this is allowed.
 # hooks.allowdeletebranch
 #   This boolean sets whether deleting branches will be allowed in the
 #   repository.  By default they won't be.
@@ -48,6 +51,7 @@ allowdeletebranch=$(git config --bool hooks.allowdeletebranch)
 denycreatebranch=$(git config --bool hooks.denycreatebranch)
 allowdeletetag=$(git config --bool hooks.allowdeletetag)
 allowmodifytag=$(git config --bool hooks.allowmodifytag)
+denyunsignedtag=$(git config --bool hooks.denyunsignedtag)
 
 # check for no description
 projectdesc=$(sed -e '1q' "$GIT_DIR/description")
@@ -71,7 +75,7 @@ case "$refname","$newrev_type" in
 	refs/tags/*,commit)
 		# un-annotated tag
 		short_refname=${refname##refs/tags/}
-		if [ "$allowunannotated" != "true" ]; then
+		if [ "$allowunannotated" != "true" ] || [ "$denyunsignedtag" = "true" ]; then
 			echo "*** The un-annotated tag, $short_refname, is not allowed in this repository" >&2
 			echo "*** Use 'git tag [ -a | -s ]' for tags you want to propagate." >&2
 			exit 1
@@ -86,6 +90,14 @@ case "$refname","$newrev_type" in
 		;;
 	refs/tags/*,tag)
 		# annotated tag
+		if [ "$denyunsignedtag" != "true" ] || git cat-file -p $newrev | grep -q 'BEGIN PGP SIGNATURE'; then
+			:
+		else
+			echo "*** Tag '$refname' is unsigned"
+			echo "*** Unsigned tags are not allowed in this repository." >&2
+			exit 1
+		fi
+
 		if [ "$allowmodifytag" != "true" ] && git rev-parse $refname > /dev/null 2>&1
 		then
 			echo "*** Tag '$refname' already exists." >&2
-- 
2.6.4

Re: [PATCH] hooks/update: Add a hooks.denyunsignedtags option

[Junio C Hamano]

Julian Andres Klode <jak@debian.org> writes:

> Introduce an option to deny unsigned tags from entering
> a repository. This is useful in teams where members forget
> to sign their release tags.
>
> It does not actually check whether the signature is actually
> complete or valid, it just checks for the beginning of a
> signature, as further checks would be too involved.
>
> This effectively also denies un-annotated tags, as those
> are unsigned by definition.
>
> Signed-off-by: Julian Andres Klode <jak@debian.org>
> ---
>
> Note: Submitted for review on Sep 12, re-asked on Sep 22, but
> no feedback, so I assume it's good to go,

You assumed wrong.  No response merely means that the patch did not
find anybody who is interested enough to even comment on it.

> see http://thread.gmane.org/gmane.comp.version-control.git/277722
> for details

There aren't much details there, but thanks for the pointer.  It
gave me a good guess as to why nobody commented on it.  You said
"locally fixed" without showing the fixed result and then asked "any
comments?"  People didn't have anything to comment on.

Anyway, let's see what we have here.

> Subject: Re: [PATCH] hooks/update: Add a hooks.denyunsignedtags option

See "git log --oneline --no-merges" and notice the local
convention.  s/Add/add/; 

> @@ -71,7 +75,7 @@ case "$refname","$newrev_type" in
>  	refs/tags/*,commit)
>  		# un-annotated tag
>  		short_refname=${refname##refs/tags/}
> -		if [ "$allowunannotated" != "true" ]; then
> +		if [ "$allowunannotated" != "true" ] || [ "$denyunsignedtag" = "true" ]; then

Somehow this combination of "allow-unannotated" and "deny-unsigned"
bothers me.  Would it make the resulting code and logic more
consistent and easier to follow if this new setting is renamed to
"$allowunsigned", I wonder...

Can we do something with the overly long line, by the way?

>  			echo "*** The un-annotated tag, $short_refname, is not allowed in this repository" >&2
>  			echo "*** Use 'git tag [ -a | -s ]' for tags you want to propagate." >&2

When $denyunsignedtag is in effect, isn't the user saying that _all_
tags _must_ be signed?  In other words, isn't it a configuration error
if allowunannotated and denyunsignedtag are both set to true?

And if you reject that configuration error early, then perhaps you
can even only check allowunannotated here without touching anything
in this hunk, perhaps doing something like this upfront:

    allowunannotated=$(git config --bool hooks.allowunannotated || echo false)
    allowunsigned=$(git config --bool hooks.allowunsigned || echo true)

    case "$allowunannotated,$allowunsigned" in
    true,false)
            echo >&2 "*** inconsistent setting"
            exit 1
    esac

Then at this point of the code, if your new condition holds true,
i.e. $allowunsigned is set to "false", $allowunannotated must be
something other than "true", so you do not have to touch the "if".

> @@ -86,6 +90,14 @@ case "$refname","$newrev_type" in
>  		;;
>  	refs/tags/*,tag)
>  		# annotated tag
> +		if [ "$denyunsignedtag" != "true" ] || git cat-file -p $newrev | grep -q 'BEGIN PGP SIGNATURE'; then
> +			:

Again, can we do something with the overly long line?

Use of "cat-file -p" is a bad manner in scripts, as we reserve the
right to change what "-p" output looks like purely on human
usability.  "cat-file tag", perhaps?

Also,

	$ git grep ' PGP '

in our source tells me that we use a bit tighter pattern even when
we are casually trying to see if the thing looks like a PGP signed
payload.

	if test "$allowunsigned" = "true" ||
           git cat-file "$newrev" |
           grep -q '^-----BEGIN PGP SIGNATURE-----$'
	then
		...

or something?


> +		else
> +			echo "*** Tag '$refname' is unsigned"
> +			echo "*** Unsigned tags are not allowed in this repository." >&2
> +			exit 1
> +		fi
> +
>  		if [ "$allowmodifytag" != "true" ] && git rev-parse $refname > /dev/null 2>&1
>  		then
>  			echo "*** Tag '$refname' already exists." >&2

Re: [PATCH] hooks/update: Add a hooks.denyunsignedtags option

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Use of "cat-file -p" is a bad manner in scripts, as we reserve the
> right to change what "-p" output looks like purely on human
> usability.  "cat-file tag", perhaps?
>
> Also,
>
> 	$ git grep ' PGP '
>
> in our source tells me that we use a bit tighter pattern even when
> we are casually trying to see if the thing looks like a PGP signed
> payload.
>
> 	if test "$allowunsigned" = "true" ||
>            git cat-file "$newrev" |
>            grep -q '^-----BEGIN PGP SIGNATURE-----$'
> 	then
> 		...
>
> or something?

I think an intelligent reader would have understood what I meant,
but the 'cat-file' in the above needs to say what type of thing
it is asking to dump, i.e.

 	if test "$allowunsigned" = "true" ||
            git cat-file tag "$newrev" |
            grep -q '^-----BEGIN PGP SIGNATURE-----$'
 	then

Sorry for the noise.

Re: [PATCH] hooks/update: Add a hooks.denyunsignedtags option

[Eric Sunshine]

On Monday, December 21, 2015, Julian Andres Klode <jak@debian.org> wrote:
> Introduce an option to deny unsigned tags from entering
> a repository. This is useful in teams where members forget
> to sign their release tags.
>
> It does not actually check whether the signature is actually
> complete or valid, it just checks for the beginning of a
> signature, as further checks would be too involved.
>
> This effectively also denies un-annotated tags, as those
> are unsigned by definition.
>
> Signed-off-by: Julian Andres Klode <jak@debian.org>
> ---
> diff --git a/templates/hooks--update.sample b/templates/hooks--update.sample
> @@ -71,7 +75,7 @@ case "$refname","$newrev_type" in
>         refs/tags/*,commit)
>                 # un-annotated tag
>                 short_refname=${refname##refs/tags/}
> -               if [ "$allowunannotated" != "true" ]; then
> +               if [ "$allowunannotated" != "true" ] || [ "$denyunsignedtag" = "true" ]; then
>                         echo "*** The un-annotated tag, $short_refname, is not allowed in this repository" >&2
>                         echo "*** Use 'git tag [ -a | -s ]' for tags you want to propagate." >&2

Hmm. Is this diagnostic sufficient to help the person resolve the
issue? Isn't it actively misleading to advise using '-a'? Perhaps a
distinct message is warranted?

(Alternately, if you follow Junio's advice and disallow this
combination of options, then this issue becomes moot.)

>                         exit 1
> @@ -86,6 +90,14 @@ case "$refname","$newrev_type" in
>                 ;;
>         refs/tags/*,tag)
>                 # annotated tag
> +               if [ "$denyunsignedtag" != "true" ] || git cat-file -p $newrev | grep -q 'BEGIN PGP SIGNATURE'; then
> +                       :
> +               else
> +                       echo "*** Tag '$refname' is unsigned"
> +                       echo "*** Unsigned tags are not allowed in this repository." >&2

The diagnostic for $allowunannotated gives helpful advice about how to
resolve the problem. Should this one follow suit?

Also consistency might suggest patterning this message after the one
for $allowunannotated. Perhaps something like this:

    The unsigned tag $short_refname is not allowed in this repository.
    Use 'git tag -s' for tags you want to propagate.

or something.

> +                       exit 1
> +               fi
> +
>                 if [ "$allowmodifytag" != "true" ] && git rev-parse $refname > /dev/null 2>&1
>                 then
>                         echo "*** Tag '$refname' already exists." >&2
> --
> 2.6.4

[PATCH] hooks/update: Add a hooks.denyunsignedtags option

[Julian Andres Klode]

Introduce an option to deny unsigned tags from entering
a repository. This is useful in teams where members forget
to sign their release tags.

It does not actually check whether the signature is actually
complete or valid, it just checks for the beginning of a
signature, as further checks would be too involved.

This effectively also denies un-annotated tags, as those
are unsigned by definition.

Signed-off-by: Julian Andres Klode <jak@debian.org>
---
 templates/hooks--update.sample | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/templates/hooks--update.sample b/templates/hooks--update.sample
index d847583..5a0460f 100755
--- a/templates/hooks--update.sample
+++ b/templates/hooks--update.sample
@@ -16,6 +16,9 @@
 # hooks.allowmodifytag
 #   This boolean sets whether a tag may be modified after creation. By default
 #   it won't be.
+# hooks.denyunsignedtag
+#   This boolean sets whether creating unsigned tags will be denied. By
+#   default this is allowed.
 # hooks.allowdeletebranch
 #   This boolean sets whether deleting branches will be allowed in the
 #   repository.  By default they won't be.
@@ -48,6 +51,7 @@ allowdeletebranch=$(git config --bool hooks.allowdeletebranch)
 denycreatebranch=$(git config --bool hooks.denycreatebranch)
 allowdeletetag=$(git config --bool hooks.allowdeletetag)
 allowmodifytag=$(git config --bool hooks.allowmodifytag)
+denyunsignedtag=$(git config --bool hooks.denyunsignedtag)
 
 # check for no description
 projectdesc=$(sed -e '1q' "$GIT_DIR/description")
@@ -71,7 +75,7 @@ case "$refname","$newrev_type" in
 	refs/tags/*,commit)
 		# un-annotated tag
 		short_refname=${refname##refs/tags/}
-		if [ "$allowunannotated" != "true" ]; then
+		if [ "$allowunannotated" != "true" ] || [ "$denyunsignedtag" = "true" ]; then
 			echo "*** The un-annotated tag, $short_refname, is not allowed in this repository" >&2
 			echo "*** Use 'git tag [ -a | -s ]' for tags you want to propagate." >&2
 			exit 1
@@ -86,6 +90,14 @@ case "$refname","$newrev_type" in
 		;;
 	refs/tags/*,tag)
 		# annotated tag
+		if [ "$denyunsignedtag" != "true" ] || git cat-file -p $newrev | grep -q 'BEGIN PGP SIGNATURE'; then
+			:
+		else
+			echo "*** Tag '$refname' is unsigned"
+			echo "*** Unsigned tags are not allowed in this repository." >&2
+	                exit 1
+		fi
+
 		if [ "$allowmodifytag" != "true" ] && git rev-parse $refname > /dev/null 2>&1
 		then
 			echo "*** Tag '$refname' already exists." >&2
-- 
2.5.1

Re: [PATCH] hooks/update: Add a hooks.denyunsignedtags option

[Julian Andres Klode]

On Sat, Sep 12, 2015 at 12:37:33PM +0200, Julian Andres Klode wrote:
> Introduce an option to deny unsigned tags from entering
> a repository. This is useful in teams where members forget
> to sign their release tags.
> 
> It does not actually check whether the signature is actually
> complete or valid, it just checks for the beginning of a
> signature, as further checks would be too involved.
> 
> This effectively also denies un-annotated tags, as those
> are unsigned by definition.
> 
> Signed-off-by: Julian Andres Klode <jak@debian.org>
> ---
>  templates/hooks--update.sample | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
[...]
> @@ -86,6 +90,14 @@ case "$refname","$newrev_type" in
>  		;;
>  	refs/tags/*,tag)
>  		# annotated tag
> +		if [ "$denyunsignedtag" != "true" ] || git cat-file -p $newrev | grep -q 'BEGIN PGP SIGNATURE'; then
> +			:
> +		else
> +			echo "*** Tag '$refname' is unsigned"
> +			echo "*** Unsigned tags are not allowed in this repository." >&2
> +	                exit 1

There are some accidental space characters in front of that, this is fixed locally
already. Sorry.

-- 
Julian Andres Klode  - Debian Developer, Ubuntu Member

See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.

Be friendly, do not top-post, and follow RFC 1855 "Netiquette".
    - If you don't I might ignore you.

Re: [PATCH] hooks/update: Add a hooks.denyunsignedtags option

[Julian Andres Klode]

On Sat, Sep 12, 2015 at 12:37:33PM +0200, Julian Andres Klode wrote:
> Introduce an option to deny unsigned tags from entering
> a repository. This is useful in teams where members forget
> to sign their release tags.
> 
> It does not actually check whether the signature is actually
> complete or valid, it just checks for the beginning of a
> signature, as further checks would be too involved.
> 
> This effectively also denies un-annotated tags, as those
> are unsigned by definition.

No comments?

-- 
Julian Andres Klode  - Debian Developer, Ubuntu Member

See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.

Be friendly, do not top-post, and follow RFC 1855 "Netiquette".
    - If you don't I might ignore you.