Re: [PATCH] tag,verify-tag: do not trip over rfc1991 signatures

Michael J Gruber [off-list ref] writes:

Currently, git expects "-----BEGIN PGP SIGNATURE-----" at the beginning of a
signature. But gpg uses "MESSAGE" instead of "SIGNATURE" when used with
the "rfc1991" option. This leads to git's faling to verify it's own
signed tags.

Be more lenient and take "-----BEGIN PGP " as the indicator.

Thanks, but it bothers me that the patch is a bit inconsistently lenient.

How many variants of PGP implementations are there?  For example, I'd ask
these without doing my own research because I am lazy:

 1. Does everybody place five dashes at the beginning (IOW, is there an
    odd variant that puts four or six)?

 2. Does everybody follow the dashes immediately with "BEGIN" (IOW, is
    there an odd variant that puts a SP between them)?

 3. Does everybody spell "BEGIN PGP " the same way, in all uppercase?

 4. Does everybody place five dashes at the end (IOW, is there an odd
    variant that puts four or six)?

 5. Does everybody follow the "BEGIN PGP SOMETHING" immediately with
    dashes without SP?

Your patch seem to answer <yes, yes, yes, no, no> to the above question.
I'd find it saner if the patched code at least checked that the line ends
with 5 dashes.

Junio C Hamano venit, vidit, dixit 05.10.2010 22:28:

Michael J Gruber [off-list ref] writes:

quoted

Currently, git expects "-----BEGIN PGP SIGNATURE-----" at the beginning of a
signature. But gpg uses "MESSAGE" instead of "SIGNATURE" when used with
the "rfc1991" option. This leads to git's faling to verify it's own
signed tags.

Be more lenient and take "-----BEGIN PGP " as the indicator.

Thanks, but it bothers me that the patch is a bit inconsistently lenient.

How many variants of PGP implementations are there?  For example, I'd ask
these without doing my own research because I am lazy:

 1. Does everybody place five dashes at the beginning (IOW, is there an
    odd variant that puts four or six)?

 2. Does everybody follow the dashes immediately with "BEGIN" (IOW, is
    there an odd variant that puts a SP between them)?

 3. Does everybody spell "BEGIN PGP " the same way, in all uppercase?

 4. Does everybody place five dashes at the end (IOW, is there an odd
    variant that puts four or six)?

 5. Does everybody follow the "BEGIN PGP SOMETHING" immediately with
    dashes without SP?

Your patch seem to answer <yes, yes, yes, no, no> to the above question.

On 4,5, my patch only implies that I (suggest we) don't care.

I'd find it saner if the patched code at least checked that the line ends
with 5 dashes.

Alternatively, we can just say we support gnupg/openpg but not pgp 2.0,
and running gpg with pgp 2.0 options is discouraged even by gpg's man page.

The main issue here is that we create a detached signature (rather than
a clear text signature) but then lump it together with the content (the
tag object sans sig). The boundary mark between the two is not
controlled by us but by gpg (and its options).

In order to verify the sig, *we* have to split the lump again but we
don't really know the boundary mark. It's insane by design. We should
have used a non-volatile boundary mark.

I'll check whether we can somehow feed the whole lump to gpg and make it
recognize the attached-detached signature. That way we'd be as
compatible as gpg.

Michael

On 5 October 2010 21:42, Michael J Gruber [off-list ref] wrote:

Junio C Hamano venit, vidit, dixit 05.10.2010 22:28:

quoted

Michael J Gruber [off-list ref] writes:

quoted

Currently, git expects "-----BEGIN PGP SIGNATURE-----" at the beginning of a
signature. But gpg uses "MESSAGE" instead of "SIGNATURE" when used with
the "rfc1991" option. This leads to git's faling to verify it's own
signed tags.

Be more lenient and take "-----BEGIN PGP " as the indicator.

Thanks, but it bothers me that the patch is a bit inconsistently lenient.

How many variants of PGP implementations are there?  For example, I'd ask
these without doing my own research because I am lazy:

 1. Does everybody place five dashes at the beginning (IOW, is there an
    odd variant that puts four or six)?

 2. Does everybody follow the dashes immediately with "BEGIN" (IOW, is
    there an odd variant that puts a SP between them)?

 3. Does everybody spell "BEGIN PGP " the same way, in all uppercase?

 4. Does everybody place five dashes at the end (IOW, is there an odd
    variant that puts four or six)?

 5. Does everybody follow the "BEGIN PGP SOMETHING" immediately with
    dashes without SP?

Your patch seem to answer <yes, yes, yes, no, no> to the above question.

On 4,5, my patch only implies that I (suggest we) don't care.

quoted

I'd find it saner if the patched code at least checked that the line ends
with 5 dashes.

Alternatively, we can just say we support gnupg/openpg but not pgp 2.0,
and running gpg with pgp 2.0 options is discouraged even by gpg's man page.

The main issue here is that we create a detached signature (rather than
a clear text signature) but then lump it together with the content (the
tag object sans sig). The boundary mark between the two is not
controlled by us but by gpg (and its options).

In order to verify the sig, *we* have to split the lump again but we
don't really know the boundary mark. It's insane by design. We should
have used a non-volatile boundary mark.

I'll check whether we can somehow feed the whole lump to gpg and make it
recognize the attached-detached signature. That way we'd be as
compatible as gpg.

Michael

5 dashes + BEGIN [other stuff] and
5 dashes + END
was part of RFC1991:
http://tools.ietf.org/html/rfc1991#section-2.4.1

Which was obsoleted by RFC4880 :
http://tools.ietf.org/html/rfc4880#section-6.2
5 dashes + BEGIN [some different stuff]
5 dashes + END

Aside from the above considerations, 5 dashes + BEGIN
would appear to conform to both the old and the current spec. Since
the current implementation of GnuPG only offers the rfc1991
compatibility options, complying with both covers all (i.e. both)
possibilties, no?
-- 

steph

Stephan Hugel venit, vidit, dixit 05.10.2010 22:51:

On 5 October 2010 21:42, Michael J Gruber [off-list ref] wrote:

quoted

Junio C Hamano venit, vidit, dixit 05.10.2010 22:28:

quoted

Michael J Gruber [off-list ref] writes:

quoted

Currently, git expects "-----BEGIN PGP SIGNATURE-----" at the beginning of a
signature. But gpg uses "MESSAGE" instead of "SIGNATURE" when used with
the "rfc1991" option. This leads to git's faling to verify it's own
signed tags.

Be more lenient and take "-----BEGIN PGP " as the indicator.

Thanks, but it bothers me that the patch is a bit inconsistently lenient.

How many variants of PGP implementations are there?  For example, I'd ask
these without doing my own research because I am lazy:

 1. Does everybody place five dashes at the beginning (IOW, is there an
    odd variant that puts four or six)?

 2. Does everybody follow the dashes immediately with "BEGIN" (IOW, is
    there an odd variant that puts a SP between them)?

 3. Does everybody spell "BEGIN PGP " the same way, in all uppercase?

 4. Does everybody place five dashes at the end (IOW, is there an odd
    variant that puts four or six)?

 5. Does everybody follow the "BEGIN PGP SOMETHING" immediately with
    dashes without SP?

Your patch seem to answer <yes, yes, yes, no, no> to the above question.

On 4,5, my patch only implies that I (suggest we) don't care.

quoted

I'd find it saner if the patched code at least checked that the line ends
with 5 dashes.

Alternatively, we can just say we support gnupg/openpg but not pgp 2.0,
and running gpg with pgp 2.0 options is discouraged even by gpg's man page.

The main issue here is that we create a detached signature (rather than
a clear text signature) but then lump it together with the content (the
tag object sans sig). The boundary mark between the two is not
controlled by us but by gpg (and its options).

In order to verify the sig, *we* have to split the lump again but we
don't really know the boundary mark. It's insane by design. We should
have used a non-volatile boundary mark.

I'll check whether we can somehow feed the whole lump to gpg and make it
recognize the attached-detached signature. That way we'd be as
compatible as gpg.

Michael

5 dashes + BEGIN [other stuff] and
5 dashes + END
was part of RFC1991:
http://tools.ietf.org/html/rfc1991#section-2.4.1

Which was obsoleted by RFC4880 :
http://tools.ietf.org/html/rfc4880#section-6.2
5 dashes + BEGIN [some different stuff]
5 dashes + END

Aside from the above considerations, 5 dashes + BEGIN
would appear to conform to both the old and the current spec. Since
the current implementation of GnuPG only offers the rfc1991
compatibility options, complying with both covers all (i.e. both)
possibilties, no?

So, we (c|sh)ould really check for the two variants rather than being
lenient, right? I'll bite the v2 apple.

Michael

Use the factored out code for sig detection when displaying tags.

Signed-off-by: Michael J Gruber <redacted>
---
 builtin/tag.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 66feeb0..617a58f 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c

@@ -68,9 +68,9 @@ static int show_reference(const char *refname, const unsigned char *sha1,
 			return 0;
 		}
 		/* only take up to "lines" lines, and strip the signature */
+		size = parse_signature(buf, size);
 		for (i = 0, sp += 2;
-				i < filter->lines && sp < buf + size &&
-				prefixcmp(sp, PGP_SIGNATURE "\n");
+				i < filter->lines && sp < buf + size;
 				i++) {
 			if (i)
 				printf("\n    ");

-- 
1.7.3.2.193.g78bbb

This mini-series fixes the handling of signed tags for users
with "rfc1991" in their gpg config. In fact, the refactoring
done in the middle three patches would be worthwhile even
without the side effect of having to fix the handling in
one place only rather than three...

Michael J Gruber (5):
  t/t7004-tag: test handling of rfc1991 signatures
  verify-tag: factor out signature detection
  tag: factor out sig detection for body edits
  tag: factor out sig detection for tag display
  tag: recognize rfc1991 signatures

 builtin/tag.c        |   16 ++++------------
 builtin/verify-tag.c |   10 ++--------
 t/t7004-tag.sh       |   43 +++++++++++++++++++++++++++++++++++++++++++
 tag.c                |   12 ++++++++++++
 tag.h                |    4 ++++
 5 files changed, 65 insertions(+), 20 deletions(-)

-- 
1.7.3.2.193.g78bbb

into tag.h/c for later reuse and modification.

Signed-off-by: Michael J Gruber <redacted>
---
 builtin/verify-tag.c |   10 ++--------
 tag.c                |   11 +++++++++++
 tag.h                |    3 +++
 3 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 9f482c2..86cac6d 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c

@@ -17,13 +17,11 @@ static const char * const verify_tag_usage[] = {
 		NULL
 };
 
-#define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
-
 static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
 {
 	struct child_process gpg;
 	const char *args_gpg[] = {"gpg", "--verify", "FILE", "-", NULL};
-	char path[PATH_MAX], *eol;
+	char path[PATH_MAX];
 	size_t len;
 	int fd, ret;
 

@@ -37,11 +35,7 @@ static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
 	close(fd);
 
 	/* find the length without signature */
-	len = 0;
-	while (len < size && prefixcmp(buf + len, PGP_SIGNATURE)) {
-		eol = memchr(buf + len, '\n', size - len);
-		len += eol ? eol - (buf + len) + 1 : size - len;
-	}
+	len = parse_signature(buf, size);
 	if (verbose)
 		write_in_full(1, buf, len);
 

diff --git a/tag.c b/tag.c
index 28641cf..5f9626c 100644
--- a/tag.c
+++ b/tag.c

@@ -133,3 +133,14 @@ int parse_tag(struct tag *item)
 	free(data);
 	return ret;
 }
+
+size_t parse_signature(const char *buf, unsigned long size)
+{
+	char *eol;
+	size_t len = 0;
+	while (len < size && prefixcmp(buf + len, PGP_SIGNATURE)) {
+		eol = memchr(buf + len, '\n', size - len);
+		len += eol ? eol - (buf + len) + 1 : size - len;
+	}
+	return len;
+}

diff --git a/tag.h b/tag.h
index 4766272..4ba2a42 100644
--- a/tag.h
+++ b/tag.h

@@ -3,6 +3,8 @@
 
 #include "object.h"
 
+#define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
+
 extern const char *tag_type;
 
 struct tag {

@@ -16,5 +18,6 @@ extern struct tag *lookup_tag(const unsigned char *sha1);
 extern int parse_tag_buffer(struct tag *item, void *data, unsigned long size);
 extern int parse_tag(struct tag *item);
 extern struct object *deref_tag(struct object *, const char *, int);
+extern size_t parse_signature(const char *buf, unsigned long size);
 
 #endif /* TAG_H */

-- 
1.7.3.2.193.g78bbb

Currently, git expects "-----BEGIN PGP SIGNATURE-----" at the beginning of a
signature. But gpg uses "MESSAGE" instead of "SIGNATURE" when used with
the "rfc1991" option. This leads to git's failing to verify it's own
signed tags, among other problems.

Add tests for all code paths (tag -v, tag -l -n largenumber, tag -f
without -m) where signature detection matters.

Reported-by: Stephan Hugel <redacted>
Signed-off-by: Michael J Gruber <redacted>
---
 t/t7004-tag.sh |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index ac943f5..22dcc45 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh

@@ -1030,6 +1030,49 @@ test_expect_success GPG \
 	test_cmp expect actual
 '
 
+# usage with rfc1991 signatures
+echo "rfc1991" > gpghome/gpg.conf
+get_tag_header rfc1991-signed-tag $commit commit $time >expect
+echo "RFC1991 signed tag" >>expect
+echo '-----BEGIN PGP MESSAGE-----' >>expect
+test_expect_success GPG \
+	'creating a signed tag with rfc1991' '
+	git tag -s -m "RFC1991 signed tag" rfc1991-signed-tag $commit &&
+	get_tag_msg rfc1991-signed-tag >actual &&
+	test_cmp expect actual
+'
+
+cat >fakeeditor <<'EOF'
+#!/bin/sh
+cp "$1" actual
+EOF
+chmod +x fakeeditor
+
+test_expect_failure GPG \
+	'reediting a signed tag body omits signature' '
+	echo "RFC1991 signed tag" >expect &&
+	GIT_EDITOR=./fakeeditor git tag -f -s rfc1991-signed-tag $commit &&
+	test_cmp expect actual
+'
+
+test_expect_failure GPG \
+	'verifying rfc1991 signature' '
+	git tag -v rfc1991-signed-tag
+'
+
+test_expect_failure GPG \
+	'list tag with rfc1991 signature' '
+	echo "rfc1991-signed-tag RFC1991 signed tag" >expect &&
+	git tag -l -n1 rfc1991-signed-tag >actual &&
+	test_cmp expect actual &&
+	git tag -l -n2 rfc1991-signed-tag >actual &&
+	test_cmp expect actual &&
+	git tag -l -n999 rfc1991-signed-tag >actual &&
+	test_cmp expect actual
+'
+
+rm -f gpghome/gpg.conf
+
 # try to sign with bad user.signingkey
 git config user.signingkey BobTheMouse
 test_expect_success GPG \

-- 
1.7.3.2.193.g78bbb

Use the factored out code for sig detection when editing existing
tag bodies (tag -a -f without -m).

Signed-off-by: Michael J Gruber <redacted>
---
 builtin/tag.c |   12 ++----------
 1 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index d311491..66feeb0 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c

@@ -29,8 +29,6 @@ struct tag_filter {
 	struct commit_list *with_commit;
 };
 
-#define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
-
 static int show_reference(const char *refname, const unsigned char *sha1,
 			  int flag, void *cb_data)
 {

@@ -242,8 +240,7 @@ static void write_tag_body(int fd, const unsigned char *sha1)
 {
 	unsigned long size;
 	enum object_type type;
-	char *buf, *sp, *eob;
-	size_t len;
+	char *buf, *sp;
 
 	buf = read_sha1_file(sha1, &type, &size);
 	if (!buf)

@@ -256,12 +253,7 @@ static void write_tag_body(int fd, const unsigned char *sha1)
 		return;
 	}
 	sp += 2; /* skip the 2 LFs */
-	eob = strstr(sp, "\n" PGP_SIGNATURE "\n");
-	if (eob)
-		len = eob - sp;
-	else
-		len = buf + size - sp;
-	write_or_die(fd, sp, len);
+	write_or_die(fd, sp, parse_signature(sp, buf + size - sp));
 
 	free(buf);
 }

-- 
1.7.3.2.193.g78bbb

We have always been creating rfc1991 signatures for users with "rfc1991"
in their gpg config but failed to recognize them (tag -l -n largenumber)
and verify them (tag -v, verify-tag).

Make good use of the refactored signature detection and let us recognize
and verify those signatures also.

Signed-off-by: Michael J Gruber <redacted>
---
 t/t7004-tag.sh |    6 +++---
 tag.c          |    3 ++-
 tag.h          |    1 +
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 22dcc45..4fe45a4 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh

@@ -1048,19 +1048,19 @@ cp "$1" actual
 EOF
 chmod +x fakeeditor
 
-test_expect_failure GPG \
+test_expect_success GPG \
 	'reediting a signed tag body omits signature' '
 	echo "RFC1991 signed tag" >expect &&
 	GIT_EDITOR=./fakeeditor git tag -f -s rfc1991-signed-tag $commit &&
 	test_cmp expect actual
 '
 
-test_expect_failure GPG \
+test_expect_success GPG \
 	'verifying rfc1991 signature' '
 	git tag -v rfc1991-signed-tag
 '
 
-test_expect_failure GPG \
+test_expect_success GPG \
 	'list tag with rfc1991 signature' '
 	echo "rfc1991-signed-tag RFC1991 signed tag" >expect &&
 	git tag -l -n1 rfc1991-signed-tag >actual &&

diff --git a/tag.c b/tag.c
index 5f9626c..18a5142 100644
--- a/tag.c
+++ b/tag.c

@@ -138,7 +138,8 @@ size_t parse_signature(const char *buf, unsigned long size)
 {
 	char *eol;
 	size_t len = 0;
-	while (len < size && prefixcmp(buf + len, PGP_SIGNATURE)) {
+	while (len < size && prefixcmp(buf + len, PGP_SIGNATURE)
+			&& prefixcmp(buf + len, PGP_MESSAGE)) {
 		eol = memchr(buf + len, '\n', size - len);
 		len += eol ? eol - (buf + len) + 1 : size - len;
 	}

diff --git a/tag.h b/tag.h
index 4ba2a42..134d572 100644
--- a/tag.h
+++ b/tag.h

@@ -4,6 +4,7 @@
 #include "object.h"
 
 #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
+#define PGP_MESSAGE "-----BEGIN PGP MESSAGE-----"
 
 extern const char *tag_type;
 

-- 
1.7.3.2.193.g78bbb

On Sat, Nov 6, 2010 at 9:04 AM, Michael J Gruber
[off-list ref] wrote:

nit: I'd move this into the tag.c file. It's only used there now.

On Sat, Nov 6, 2010 at 9:04 AM, Michael J Gruber
[off-list ref] wrote:

nit: I think this && should be in the end of the previous line. (Not
sure what is the preferred style though, comments?).

This mini-series fixes the handling of signed tags for users
with "rfc1991" in their gpg config. In fact, the refactoring
done in the middle three patches would be worthwhile even
without the side effect of having to fix the handling in
one place only rather than three...

v2 has these changes:
* additional tests for dealing with rfc1991 sigs without the rfc1991 option set (1/5)
* macros in tag.c rather than tag.h since they are used only in tag.c (2/5)
* logical && at eol for continued logical expression in C (5/5)

Michael J Gruber (5):
  t/t7004-tag: test handling of rfc1991 signatures
  verify-tag: factor out signature detection
  tag: factor out sig detection for body edits
  tag: factor out sig detection for tag display
  tag: recognize rfc1991 signatures

 builtin/tag.c        |   16 +++---------
 builtin/verify-tag.c |   10 +------
 t/t7004-tag.sh       |   66 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tag.c                |   15 +++++++++++
 tag.h                |    1 +
 5 files changed, 88 insertions(+), 20 deletions(-)

-- 
1.7.3.2.193.g78bbb

Currently, git expects "-----BEGIN PGP SIGNATURE-----" at the beginning of a
signature. But gpg uses "MESSAGE" instead of "SIGNATURE" when used with
the "rfc1991" option. This leads to git's failing to verify it's own
signed tags, among other problems.

Add tests for all code paths (tag -v, tag -l -n largenumber, tag -f
without -m) where signature detection matters.

Reported-by: Stephan Hugel <redacted>
Signed-off-by: Michael J Gruber <redacted>
---
 t/t7004-tag.sh |   66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 66 insertions(+), 0 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index ac943f5..c7d49e1 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh

@@ -1030,6 +1030,72 @@ test_expect_success GPG \
 	test_cmp expect actual
 '
 
+# usage with rfc1991 signatures
+echo "rfc1991" > gpghome/gpg.conf
+get_tag_header rfc1991-signed-tag $commit commit $time >expect
+echo "RFC1991 signed tag" >>expect
+echo '-----BEGIN PGP MESSAGE-----' >>expect
+test_expect_success GPG \
+	'creating a signed tag with rfc1991' '
+	git tag -s -m "RFC1991 signed tag" rfc1991-signed-tag $commit &&
+	get_tag_msg rfc1991-signed-tag >actual &&
+	test_cmp expect actual
+'
+
+cat >fakeeditor <<'EOF'
+#!/bin/sh
+cp "$1" actual
+EOF
+chmod +x fakeeditor
+
+test_expect_failure GPG \
+	'reediting a signed tag body omits signature' '
+	echo "RFC1991 signed tag" >expect &&
+	GIT_EDITOR=./fakeeditor git tag -f -s rfc1991-signed-tag $commit &&
+	test_cmp expect actual
+'
+
+test_expect_failure GPG \
+	'verifying rfc1991 signature' '
+	git tag -v rfc1991-signed-tag
+'
+
+test_expect_failure GPG \
+	'list tag with rfc1991 signature' '
+	echo "rfc1991-signed-tag RFC1991 signed tag" >expect &&
+	git tag -l -n1 rfc1991-signed-tag >actual &&
+	test_cmp expect actual &&
+	git tag -l -n2 rfc1991-signed-tag >actual &&
+	test_cmp expect actual &&
+	git tag -l -n999 rfc1991-signed-tag >actual &&
+	test_cmp expect actual
+'
+
+rm -f gpghome/gpg.conf
+
+test_expect_failure GPG \
+	'verifying rfc1991 signature without --rfc1991' '
+	git tag -v rfc1991-signed-tag
+'
+
+test_expect_failure GPG \
+	'list tag with rfc1991 signature without --rfc1991' '
+	echo "rfc1991-signed-tag RFC1991 signed tag" >expect &&
+	git tag -l -n1 rfc1991-signed-tag >actual &&
+	test_cmp expect actual &&
+	git tag -l -n2 rfc1991-signed-tag >actual &&
+	test_cmp expect actual &&
+	git tag -l -n999 rfc1991-signed-tag >actual &&
+	test_cmp expect actual
+'
+
+test_expect_failure GPG \
+	'reediting a signed tag body omits signature' '
+	echo "RFC1991 signed tag" >expect &&
+	GIT_EDITOR=./fakeeditor git tag -f -s rfc1991-signed-tag $commit &&
+	test_cmp expect actual
+'
+
 # try to sign with bad user.signingkey
 git config user.signingkey BobTheMouse
 test_expect_success GPG \

-- 
1.7.3.2.193.g78bbb

into tag.h/c for later reuse and modification.

Signed-off-by: Michael J Gruber <redacted>
---
 builtin/verify-tag.c |   10 ++--------
 tag.c                |   13 +++++++++++++
 tag.h                |    1 +
 3 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 9f482c2..86cac6d 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c

@@ -17,13 +17,11 @@ static const char * const verify_tag_usage[] = {
 		NULL
 };
 
-#define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
-
 static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
 {
 	struct child_process gpg;
 	const char *args_gpg[] = {"gpg", "--verify", "FILE", "-", NULL};
-	char path[PATH_MAX], *eol;
+	char path[PATH_MAX];
 	size_t len;
 	int fd, ret;
 

@@ -37,11 +35,7 @@ static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
 	close(fd);
 
 	/* find the length without signature */
-	len = 0;
-	while (len < size && prefixcmp(buf + len, PGP_SIGNATURE)) {
-		eol = memchr(buf + len, '\n', size - len);
-		len += eol ? eol - (buf + len) + 1 : size - len;
-	}
+	len = parse_signature(buf, size);
 	if (verbose)
 		write_in_full(1, buf, len);
 

diff --git a/tag.c b/tag.c
index 28641cf..d4f3080 100644
--- a/tag.c
+++ b/tag.c

@@ -4,6 +4,8 @@
 #include "tree.h"
 #include "blob.h"
 
+#define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
+
 const char *tag_type = "tag";
 
 struct object *deref_tag(struct object *o, const char *warn, int warnlen)

@@ -133,3 +135,14 @@ int parse_tag(struct tag *item)
 	free(data);
 	return ret;
 }
+
+size_t parse_signature(const char *buf, unsigned long size)
+{
+	char *eol;
+	size_t len = 0;
+	while (len < size && prefixcmp(buf + len, PGP_SIGNATURE)) {
+		eol = memchr(buf + len, '\n', size - len);
+		len += eol ? eol - (buf + len) + 1 : size - len;
+	}
+	return len;
+}

diff --git a/tag.h b/tag.h
index 4766272..8522370 100644
--- a/tag.h
+++ b/tag.h

@@ -16,5 +16,6 @@ extern struct tag *lookup_tag(const unsigned char *sha1);
 extern int parse_tag_buffer(struct tag *item, void *data, unsigned long size);
 extern int parse_tag(struct tag *item);
 extern struct object *deref_tag(struct object *, const char *, int);
+extern size_t parse_signature(const char *buf, unsigned long size);
 
 #endif /* TAG_H */

-- 
1.7.3.2.193.g78bbb

Use the factored out code for sig detection when editing existing
tag bodies (tag -a -f without -m).

Signed-off-by: Michael J Gruber <redacted>
---
 builtin/tag.c |   12 ++----------
 1 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index d311491..66feeb0 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c

@@ -29,8 +29,6 @@ struct tag_filter {
 	struct commit_list *with_commit;
 };
 
-#define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
-
 static int show_reference(const char *refname, const unsigned char *sha1,
 			  int flag, void *cb_data)
 {

@@ -242,8 +240,7 @@ static void write_tag_body(int fd, const unsigned char *sha1)
 {
 	unsigned long size;
 	enum object_type type;
-	char *buf, *sp, *eob;
-	size_t len;
+	char *buf, *sp;
 
 	buf = read_sha1_file(sha1, &type, &size);
 	if (!buf)

@@ -256,12 +253,7 @@ static void write_tag_body(int fd, const unsigned char *sha1)
 		return;
 	}
 	sp += 2; /* skip the 2 LFs */
-	eob = strstr(sp, "\n" PGP_SIGNATURE "\n");
-	if (eob)
-		len = eob - sp;
-	else
-		len = buf + size - sp;
-	write_or_die(fd, sp, len);
+	write_or_die(fd, sp, parse_signature(sp, buf + size - sp));
 
 	free(buf);
 }

-- 
1.7.3.2.193.g78bbb

Use the factored out code for sig detection when displaying tags.

Signed-off-by: Michael J Gruber <redacted>
---
 builtin/tag.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 66feeb0..617a58f 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c

@@ -68,9 +68,9 @@ static int show_reference(const char *refname, const unsigned char *sha1,
 			return 0;
 		}
 		/* only take up to "lines" lines, and strip the signature */
+		size = parse_signature(buf, size);
 		for (i = 0, sp += 2;
-				i < filter->lines && sp < buf + size &&
-				prefixcmp(sp, PGP_SIGNATURE "\n");
+				i < filter->lines && sp < buf + size;
 				i++) {
 			if (i)
 				printf("\n    ");

-- 
1.7.3.2.193.g78bbb

We have always been creating rfc1991 signatures for users with "rfc1991"
in their gpg config but failed to recognize them (tag -l -n largenumber)
and verify them (tag -v, verify-tag).

Make good use of the refactored signature detection and let us recognize
and verify those signatures also.

Signed-off-by: Michael J Gruber <redacted>
---
 t/t7004-tag.sh |   12 ++++++------
 tag.c          |    4 +++-
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index c7d49e1..6841c23 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh

@@ -1048,19 +1048,19 @@ cp "$1" actual
 EOF
 chmod +x fakeeditor
 
-test_expect_failure GPG \
+test_expect_success GPG \
 	'reediting a signed tag body omits signature' '
 	echo "RFC1991 signed tag" >expect &&
 	GIT_EDITOR=./fakeeditor git tag -f -s rfc1991-signed-tag $commit &&
 	test_cmp expect actual
 '
 
-test_expect_failure GPG \
+test_expect_success GPG \
 	'verifying rfc1991 signature' '
 	git tag -v rfc1991-signed-tag
 '
 
-test_expect_failure GPG \
+test_expect_success GPG \
 	'list tag with rfc1991 signature' '
 	echo "rfc1991-signed-tag RFC1991 signed tag" >expect &&
 	git tag -l -n1 rfc1991-signed-tag >actual &&

@@ -1073,12 +1073,12 @@ test_expect_failure GPG \
 
 rm -f gpghome/gpg.conf
 
-test_expect_failure GPG \
+test_expect_success GPG \
 	'verifying rfc1991 signature without --rfc1991' '
 	git tag -v rfc1991-signed-tag
 '
 
-test_expect_failure GPG \
+test_expect_success GPG \
 	'list tag with rfc1991 signature without --rfc1991' '
 	echo "rfc1991-signed-tag RFC1991 signed tag" >expect &&
 	git tag -l -n1 rfc1991-signed-tag >actual &&

@@ -1089,7 +1089,7 @@ test_expect_failure GPG \
 	test_cmp expect actual
 '
 
-test_expect_failure GPG \
+test_expect_success GPG \
 	'reediting a signed tag body omits signature' '
 	echo "RFC1991 signed tag" >expect &&
 	GIT_EDITOR=./fakeeditor git tag -f -s rfc1991-signed-tag $commit &&

diff --git a/tag.c b/tag.c
index d4f3080..f789744 100644
--- a/tag.c
+++ b/tag.c

@@ -5,6 +5,7 @@
 #include "blob.h"
 
 #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
+#define PGP_MESSAGE "-----BEGIN PGP MESSAGE-----"
 
 const char *tag_type = "tag";
 

@@ -140,7 +141,8 @@ size_t parse_signature(const char *buf, unsigned long size)
 {
 	char *eol;
 	size_t len = 0;
-	while (len < size && prefixcmp(buf + len, PGP_SIGNATURE)) {
+	while (len < size && prefixcmp(buf + len, PGP_SIGNATURE) &&
+			prefixcmp(buf + len, PGP_MESSAGE)) {
 		eol = memchr(buf + len, '\n', size - len);
 		len += eol ? eol - (buf + len) + 1 : size - len;
 	}

-- 
1.7.3.2.193.g78bbb