[PATCH 0/8] gpg-interface: Multiple signing tools

[PATCH 0/8] gpg-interface: Multiple signing tools

[Ben Toews]

This series extends the configuration to allow Git to better work with multiple signing tools.

Ben Toews (1):
  gpg-interface: handle alternative signature types

Jeff King (7):
  gpg-interface: handle bool user.signingkey
  gpg-interface: modernize function declarations
  gpg-interface: use size_t for signature buffer size
  gpg-interface: fix const-correctness of "eol" pointer
  gpg-interface: extract gpg line matching helper
  gpg-interface: find the last gpg signature line
  gpg-interface: prepare for parsing arbitrary PEM blocks

 Documentation/config.txt |  40 +++++++---
 builtin/fmt-merge-msg.c  |   6 +-
 builtin/receive-pack.c   |   7 +-
 builtin/tag.c            |   2 +-
 commit.c                 |   2 +-
 gpg-interface.c          | 198 +++++++++++++++++++++++++++++++++++++++--------
 gpg-interface.h          |  67 +++++++++++++---
 log-tree.c               |   7 +-
 ref-filter.c             |   2 +-
 t/lib-gpg.sh             |  26 +++++++
 t/t7004-tag.sh           |  11 +++
 t/t7510-signed-commit.sh |  32 +++++++-
 tag.c                    |   2 +-
 13 files changed, 336 insertions(+), 66 deletions(-)

-- 
2.15.1 (Apple Git-101)

[PATCH 1/8] gpg-interface: handle bool user.signingkey

[Ben Toews]

From: Jeff King <peff@peff.net>

The config handler for user.signingkey does not check for a
boolean value, and thus:

  git -c user.signingkey tag

will segfault. We could fix this and even shorten the code
by using git_config_string(). But our set_signing_key()
helper is used by other code outside of gpg-interface.c, so
we must keep it (and we may as well use it, because unlike
git_config_string() it does not leak when we overwrite an
old value).

Ironically, the handler for gpg.program just below _could_
use git_config_string() but doesn't. But since we're going
to touch that in a future patch, we'll leave it alone for
now. We will add some whitespace and returns in preparation
for adding more config keys, though.
---
 gpg-interface.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gpg-interface.c b/gpg-interface.c
index 4feacf16e5..61c0690e12 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -128,13 +128,19 @@ void set_signing_key(const char *key)
 int git_gpg_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "user.signingkey")) {
+		if (!value)
+			return config_error_nonbool(var);
 		set_signing_key(value);
+		return 0;
 	}
+
 	if (!strcmp(var, "gpg.program")) {
 		if (!value)
 			return config_error_nonbool(var);
 		gpg_program = xstrdup(value);
+		return 0;
 	}
+
 	return 0;
 }
 
-- 
2.15.1 (Apple Git-101)

Re: [PATCH 1/8] gpg-interface: handle bool user.signingkey

[Eric Sunshine]

On Mon, Apr 9, 2018 at 4:41 PM, Ben Toews <mastahyeti@gmail.com> wrote:
> From: Jeff King <peff@peff.net>
>
> The config handler for user.signingkey does not check for a
> boolean value, and thus:
>
>   git -c user.signingkey tag
>
> will segfault. We could fix this and even shorten the code
> by using git_config_string(). But our set_signing_key()
> helper is used by other code outside of gpg-interface.c, so
> we must keep it (and we may as well use it, because unlike
> git_config_string() it does not leak when we overwrite an
> old value).
>
> Ironically, the handler for gpg.program just below _could_
> use git_config_string() but doesn't. But since we're going
> to touch that in a future patch, we'll leave it alone for
> now. We will add some whitespace and returns in preparation
> for adding more config keys, though.
> ---

Peff's Signed-off-by: is missing. Also, since you're forwarding this
patch on Peff's behalf, your Signed-off-by: should follow his. Same
comment applies to all patches by Peff in this series.

The patch itself looks reasonable.

> diff --git a/gpg-interface.c b/gpg-interface.c
> index 4feacf16e5..61c0690e12 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -128,13 +128,19 @@ void set_signing_key(const char *key)
>  int git_gpg_config(const char *var, const char *value, void *cb)
>  {
>         if (!strcmp(var, "user.signingkey")) {
> +               if (!value)
> +                       return config_error_nonbool(var);
>                 set_signing_key(value);
> +               return 0;
>         }
> +
>         if (!strcmp(var, "gpg.program")) {
>                 if (!value)
>                         return config_error_nonbool(var);
>                 gpg_program = xstrdup(value);
> +               return 0;
>         }
> +
>         return 0;
>  }

Re: [PATCH 1/8] gpg-interface: handle bool user.signingkey

[Jeff King]

On Mon, Apr 09, 2018 at 04:55:26PM -0400, Eric Sunshine wrote:

> Peff's Signed-off-by: is missing. Also, since you're forwarding this
> patch on Peff's behalf, your Signed-off-by: should follow his. Same
> comment applies to all patches by Peff in this series.

I usually sign-off as I send to the list, but I passed these patches to
Ben via a repository. For the record, it's OK to add my s-o-b to the
whole series.

-Peff

[PATCH 2/8] gpg-interface: modernize function declarations

[Ben Toews]

From: Jeff King <peff@peff.net>

Let's drop "extern" from our declarations, which brings us
in line with our modern style guidelines. While we're
here, let's wrap some of the overly long lines, and move
docstrings for public functions to their declarations, since
they document the interface.
---
 gpg-interface.c | 17 -----------------
 gpg-interface.h | 49 ++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 38 insertions(+), 28 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 61c0690e12..08de0daa41 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -101,12 +101,6 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
 		fputs(output, stderr);
 }
 
-/*
- * Look at GPG signed content (e.g. a signed tag object), whose
- * payload is followed by a detached signature on it.  Return the
- * offset where the embedded detached signature begins, or the end of
- * the data when there is no such signature.
- */
 size_t parse_signature(const char *buf, unsigned long size)
 {
 	char *eol;
@@ -151,12 +145,6 @@ const char *get_signing_key(void)
 	return git_committer_info(IDENT_STRICT|IDENT_NO_DATE);
 }
 
-/*
- * Create a detached signature for the contents of "buffer" and append
- * it after "signature"; "buffer" and "signature" can be the same
- * strbuf instance, which would cause the detached signature appended
- * at the end.
- */
 int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
 {
 	struct child_process gpg = CHILD_PROCESS_INIT;
@@ -198,11 +186,6 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
 	return 0;
 }
 
-/*
- * Run "gpg" to see if the payload matches the detached signature.
- * gpg_output, when set, receives the diagnostic output from GPG.
- * gpg_status, when set, receives the status output from GPG.
- */
 int verify_signed_buffer(const char *payload, size_t payload_size,
 			 const char *signature, size_t signature_size,
 			 struct strbuf *gpg_output, struct strbuf *gpg_status)
diff --git a/gpg-interface.h b/gpg-interface.h
index d2d4fd3a65..2c40a9175f 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -23,16 +23,43 @@ struct signature_check {
 	char *key;
 };
 
-extern void signature_check_clear(struct signature_check *sigc);
-extern size_t parse_signature(const char *buf, unsigned long size);
-extern void parse_gpg_output(struct signature_check *);
-extern int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key);
-extern int verify_signed_buffer(const char *payload, size_t payload_size, const char *signature, size_t signature_size, struct strbuf *gpg_output, struct strbuf *gpg_status);
-extern int git_gpg_config(const char *, const char *, void *);
-extern void set_signing_key(const char *);
-extern const char *get_signing_key(void);
-extern int check_signature(const char *payload, size_t plen,
-	const char *signature, size_t slen, struct signature_check *sigc);
-void print_signature_buffer(const struct signature_check *sigc, unsigned flags);
+void signature_check_clear(struct signature_check *sigc);
+
+/*
+ * Look at GPG signed content (e.g. a signed tag object), whose
+ * payload is followed by a detached signature on it.  Return the
+ * offset where the embedded detached signature begins, or the end of
+ * the data when there is no such signature.
+ */
+size_t parse_signature(const char *buf, unsigned long size);
+
+void parse_gpg_output(struct signature_check *);
+
+/*
+ * Create a detached signature for the contents of "buffer" and append
+ * it after "signature"; "buffer" and "signature" can be the same
+ * strbuf instance, which would cause the detached signature appended
+ * at the end.
+ */
+int sign_buffer(struct strbuf *buffer, struct strbuf *signature,
+		const char *signing_key);
+
+/*
+ * Run "gpg" to see if the payload matches the detached signature.
+ * gpg_output, when set, receives the diagnostic output from GPG.
+ * gpg_status, when set, receives the status output from GPG.
+ */
+int verify_signed_buffer(const char *payload, size_t payload_size,
+			 const char *signature, size_t signature_size,
+			 struct strbuf *gpg_output, struct strbuf *gpg_status);
+
+int git_gpg_config(const char *, const char *, void *);
+void set_signing_key(const char *);
+const char *get_signing_key(void);
+int check_signature(const char *payload, size_t plen,
+		    const char *signature, size_t slen,
+		    struct signature_check *sigc);
+void print_signature_buffer(const struct signature_check *sigc,
+			    unsigned flags);
 
 #endif
-- 
2.15.1 (Apple Git-101)

[PATCH 3/8] gpg-interface: use size_t for signature buffer size

[Ben Toews]

From: Jeff King <peff@peff.net>

Even though our object sizes (from which these buffers would
come) are typically "unsigned long", this is something we'd
like to eventually fix (since it's only 32-bits even on
64-bit Windows). It makes more sense to use size_t when
taking an in-memory buffer.
---
 gpg-interface.c | 2 +-
 gpg-interface.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 08de0daa41..ac852ad4b9 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -101,7 +101,7 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
 		fputs(output, stderr);
 }
 
-size_t parse_signature(const char *buf, unsigned long size)
+size_t parse_signature(const char *buf, size_t size)
 {
 	char *eol;
 	size_t len = 0;
diff --git a/gpg-interface.h b/gpg-interface.h
index 2c40a9175f..a5e6517ae6 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -31,7 +31,7 @@ void signature_check_clear(struct signature_check *sigc);
  * offset where the embedded detached signature begins, or the end of
  * the data when there is no such signature.
  */
-size_t parse_signature(const char *buf, unsigned long size);
+size_t parse_signature(const char *buf, size_t size);
 
 void parse_gpg_output(struct signature_check *);
 
-- 
2.15.1 (Apple Git-101)

[PATCH 4/8] gpg-interface: fix const-correctness of "eol" pointer

[Ben Toews]

From: Jeff King <peff@peff.net>

We accidentally shed the "const" of our buffer by passing it
through memchr. Let's fix that, and while we're at it, move
our variable declaration inside the loop, which is the only
place that uses it.
---
 gpg-interface.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index ac852ad4b9..3414af38b9 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -103,11 +103,10 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
 
 size_t parse_signature(const char *buf, size_t size)
 {
-	char *eol;
 	size_t len = 0;
 	while (len < size && !starts_with(buf + len, PGP_SIGNATURE) &&
 			!starts_with(buf + len, PGP_MESSAGE)) {
-		eol = memchr(buf + len, '\n', size - len);
+		const char *eol = memchr(buf + len, '\n', size - len);
 		len += eol ? eol - (buf + len) + 1 : size - len;
 	}
 	return len;
-- 
2.15.1 (Apple Git-101)

[PATCH 5/8] gpg-interface: extract gpg line matching helper

[Ben Toews]

From: Jeff King <peff@peff.net>

Let's separate the actual line-by-line parsing of signatures
from the notion of "is this a gpg signature line". That will
make it easier to do more refactoring of this loop in future
patches.
---
 gpg-interface.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 3414af38b9..79333c1ee8 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -101,11 +101,16 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
 		fputs(output, stderr);
 }
 
+static int is_gpg_start(const char *line)
+{
+	return starts_with(line, PGP_SIGNATURE) ||
+		starts_with(line, PGP_MESSAGE);
+}
+
 size_t parse_signature(const char *buf, size_t size)
 {
 	size_t len = 0;
-	while (len < size && !starts_with(buf + len, PGP_SIGNATURE) &&
-			!starts_with(buf + len, PGP_MESSAGE)) {
+	while (len < size && !is_gpg_start(buf + len)) {
 		const char *eol = memchr(buf + len, '\n', size - len);
 		len += eol ? eol - (buf + len) + 1 : size - len;
 	}
-- 
2.15.1 (Apple Git-101)

[PATCH 6/8] gpg-interface: find the last gpg signature line

[Ben Toews]

From: Jeff King <peff@peff.net>

A signed tag has a detached signature like this:

  object ...
  [...more header...]

  This is the tag body.

  -----BEGIN PGP SIGNATURE-----
  [opaque gpg data]
  -----END PGP SIGNATURE-----

Our parser finds the _first_ line that appears to start a
PGP signature block, meaning we may be confused by a
signature (or a signature-like line) in the actual body.
Let's keep parsing and always find the final block, which
should be the detached signature over all of the preceding
content.
---
 gpg-interface.c | 12 +++++++++---
 t/t7004-tag.sh  | 11 +++++++++++
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 79333c1ee8..0647bd6348 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -110,11 +110,17 @@ static int is_gpg_start(const char *line)
 size_t parse_signature(const char *buf, size_t size)
 {
 	size_t len = 0;
-	while (len < size && !is_gpg_start(buf + len)) {
-		const char *eol = memchr(buf + len, '\n', size - len);
+	size_t match = size;
+	while (len < size) {
+		const char *eol;
+
+		if (is_gpg_start(buf + len))
+			match = len;
+
+		eol = memchr(buf + len, '\n', size - len);
 		len += eol ? eol - (buf + len) + 1 : size - len;
 	}
-	return len;
+	return match;
 }
 
 void set_signing_key(const char *key)
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index ee093b393d..e3f1e014aa 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1059,6 +1059,17 @@ test_expect_success GPG \
 	git tag -v blanknonlfile-signed-tag
 '
 
+test_expect_success GPG 'signed tag with embedded PGP message' '
+	cat >msg <<-\EOF &&
+	-----BEGIN PGP MESSAGE-----
+
+	this is not a real PGP message
+	-----END PGP MESSAGE-----
+	EOF
+	git tag -s -F msg confusing-pgp-message &&
+	git tag -v confusing-pgp-message
+'
+
 # messages with commented lines for signed tags:
 
 cat >sigcommentsfile <<EOF
-- 
2.15.1 (Apple Git-101)

Re: [PATCH 6/8] gpg-interface: find the last gpg signature line

[Eric Sunshine]

On Mon, Apr 9, 2018 at 4:41 PM, Ben Toews <mastahyeti@gmail.com> wrote:
> From: Jeff King <peff@peff.net>
>
> A signed tag has a detached signature like this:
>
>   object ...
>   [...more header...]
>
>   This is the tag body.
>
>   -----BEGIN PGP SIGNATURE-----
>   [opaque gpg data]
>   -----END PGP SIGNATURE-----
>
> Our parser finds the _first_ line that appears to start a
> PGP signature block, meaning we may be confused by a
> signature (or a signature-like line) in the actual body.
> Let's keep parsing and always find the final block, which
> should be the detached signature over all of the preceding
> content.
> ---
> diff --git a/gpg-interface.c b/gpg-interface.c
> @@ -110,11 +110,17 @@ static int is_gpg_start(const char *line)
>  size_t parse_signature(const char *buf, size_t size)
>  {
>         size_t len = 0;
> -       while (len < size && !is_gpg_start(buf + len)) {
> -               const char *eol = memchr(buf + len, '\n', size - len);
> +       size_t match = size;

If no GPG-start line is found then 'size' will be returned, which
matches the logic before this change. Okay.

> +       while (len < size) {
> +               const char *eol;
> +
> +               if (is_gpg_start(buf + len))
> +                       match = len;

Otherwise, the position of the final GPG-start line will be remembered
and returned. Makes sense.

> +               eol = memchr(buf + len, '\n', size - len);
>                 len += eol ? eol - (buf + len) + 1 : size - len;
>         }
> -       return len;
> +       return match;
>  }
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> @@ -1059,6 +1059,17 @@ test_expect_success GPG \
> +test_expect_success GPG 'signed tag with embedded PGP message' '
> +       cat >msg <<-\EOF &&
> +       -----BEGIN PGP MESSAGE-----
> +
> +       this is not a real PGP message
> +       -----END PGP MESSAGE-----
> +       EOF

This bogus PGP message is just in the body...

> +       git tag -s -F msg confusing-pgp-message &&

and "git tag -s" adds the real PGP message at the end...

> +       git tag -v confusing-pgp-message

and the new logic finds the real PGP message rather than the bogus
one, so "git tag -v" exits successfully. Looks good.

> +'

Re: [PATCH 6/8] gpg-interface: find the last gpg signature line

[Junio C Hamano]

Ben Toews <mastahyeti@gmail.com> writes:

> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index ee093b393d..e3f1e014aa 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -1059,6 +1059,17 @@ test_expect_success GPG \
>  	git tag -v blanknonlfile-signed-tag
>  '
>  
> +test_expect_success GPG 'signed tag with embedded PGP message' '
> +	cat >msg <<-\EOF &&
> +	-----BEGIN PGP MESSAGE-----
> +
> +	this is not a real PGP message
> +	-----END PGP MESSAGE-----
> +	EOF
> +	git tag -s -F msg confusing-pgp-message &&
> +	git tag -v confusing-pgp-message
> +'
> +
>  # messages with commented lines for signed tags:
>  
>  cat >sigcommentsfile <<EOF

Hmmmm, what vintage of our codebase is this patch based on?  Did I
miss a patch that removes these lines


    printf '      ' >sigblanknonlfile
    get_tag_header blanknonlfile-signed-tag $commit commit $time >expect
    echo '-----BEGIN PGP SIGNATURE-----' >>expect
    test_expect_success GPG \
            'creating a signed tag with spaces and no newline should succeed' '
            git tag -s -F sigblanknonlfile blanknonlfile-signed-tag &&
            get_tag_msg blanknonlfile-signed-tag >actual &&
            test_cmp expect actual &&
            git tag -v signed-tag
    '

which appear between the pre- and post- context of the lines you are
inserting?  They date back to 2007-2009.

Re: [PATCH 6/8] gpg-interface: find the last gpg signature line

[Ben Toews]

On Tue, Apr 10, 2018 at 3:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ben Toews <mastahyeti@gmail.com> writes:
>
>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
>> index ee093b393d..e3f1e014aa 100755
>> --- a/t/t7004-tag.sh
>> +++ b/t/t7004-tag.sh
>> @@ -1059,6 +1059,17 @@ test_expect_success GPG \
>>       git tag -v blanknonlfile-signed-tag
>>  '
>>
>> +test_expect_success GPG 'signed tag with embedded PGP message' '
>> +     cat >msg <<-\EOF &&
>> +     -----BEGIN PGP MESSAGE-----
>> +
>> +     this is not a real PGP message
>> +     -----END PGP MESSAGE-----
>> +     EOF
>> +     git tag -s -F msg confusing-pgp-message &&
>> +     git tag -v confusing-pgp-message
>> +'
>> +
>>  # messages with commented lines for signed tags:
>>
>>  cat >sigcommentsfile <<EOF
>
> Hmmmm, what vintage of our codebase is this patch based on?  Did I
> miss a patch that removes these lines
>
>
>     printf '      ' >sigblanknonlfile
>     get_tag_header blanknonlfile-signed-tag $commit commit $time >expect
>     echo '-----BEGIN PGP SIGNATURE-----' >>expect
>     test_expect_success GPG \
>             'creating a signed tag with spaces and no newline should succeed' '
>             git tag -s -F sigblanknonlfile blanknonlfile-signed-tag &&
>             get_tag_msg blanknonlfile-signed-tag >actual &&
>             test_cmp expect actual &&
>             git tag -v signed-tag
>     '
>
> which appear between the pre- and post- context of the lines you are
> inserting?  They date back to 2007-2009.
>

That test was fixed a week ago:
https://github.com/git/git/commit/a99d903f21d102a5768f19157085a9733aeb68dd



-- 
-Ben Toews

Re: [PATCH 6/8] gpg-interface: find the last gpg signature line

[Junio C Hamano]

Ben Toews <mastahyeti@gmail.com> writes:

>> Hmmmm, what vintage of our codebase is this patch based on?  Did I
>> miss a patch that removes these lines
>>
>>
>>     printf '      ' >sigblanknonlfile
>>     get_tag_header blanknonlfile-signed-tag $commit commit $time >expect
>>     echo '-----BEGIN PGP SIGNATURE-----' >>expect
>>     test_expect_success GPG \
>>             'creating a signed tag with spaces and no newline should succeed' '
>>             git tag -s -F sigblanknonlfile blanknonlfile-signed-tag &&
>>             get_tag_msg blanknonlfile-signed-tag >actual &&
>>             test_cmp expect actual &&
>>             git tag -v signed-tag
>>     '
>>
>> which appear between the pre- and post- context of the lines you are
>> inserting?  They date back to 2007-2009.
>>
>
> That test was fixed a week ago:
> https://github.com/git/git/commit/a99d903f21d102a5768f19157085a9733aeb68dd

Well, you cannot expect any reviewer to know about a change that has
never been sent to the list and has never been part of even 'pu'
branch, no matter how old such a private "fix" is.  

What other unpublished things that are not even on 'pu' do these
patches depend on?

Re: [PATCH 6/8] gpg-interface: find the last gpg signature line

[Junio C Hamano]

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

>> That test was fixed a week ago:
>> https://github.com/git/git/commit/a99d903f21d102a5768f19157085a9733aeb68dd
>
> Well, you cannot expect any reviewer to know about a change that has
> never been sent to the list and has never been part of even 'pu'
> branch, no matter how old such a private "fix" is.  
>
> What other unpublished things that are not even on 'pu' do these
> patches depend on?

Answering my own question after digging a bit more, now I know that
a99d903 comes from the 'private' branch in peff/git/ repository
hosted at GitHub [*1*].  The branch builds on 'next' by merging many
private branches, and 'jk/non-pgp-signatures' branch has that commit.

    peff.git/private$ git log --oneline --boundary 0c8726318..7a526834e^2
    c9ce7c5b7 gpg-interface: handle multiple signing tools
    914951682 gpg-interface: handle bool user.signingkey
    1f2ea84b3 gpg-interface: return signature type from parse_signature()
    6d2ce6547 gpg-interface: prepare for parsing arbitrary PEM blocks
    fb1d173db gpg-interface: find the last gpg signature line
    6bc4e7e17 gpg-interface: extract gpg line matching helper
    4f883ac49 gpg-interface: fix const-correctness of "eol" pointer
    ae6529fdb gpg-interface: use size_t for signature buffer size
    1bca4296b gpg-interface: modernize function declarations
    a99d903f2 t7004: fix mistaken tag name
    - 468165c1d Git 2.17

It seems to me that Peff did the t7004 change as the earliest
preliminary step of the series, but it caused confusion when it was
not sent as part of the series by mistake.  Judging from the shape
of the topic, I do not think this topic has any other hidden
dependencies as it builds directly on top of v2.17.0.

For those who are reading and reviewing from the sideline, I've
attached that missing 0.9/8 patch at the end of this message.

[Footnote]

*1* I do not know if it deserves to be called a bug, but it
    certainly is an anomaly GitHub exhibits that a99d903f can be
    viewed at https://github.com/git/git/commit/a99d903f..., as if
    it is part of the official git/git history, when it only exists
    in a fork of that repository.  I can understand why it happens
    but it certainly is unexpected.

-- >8 --
From: Jeff King <peff@peff.net>
Date: Tue, 3 Apr 2018 17:10:30 -0400
Subject: [PATCH 0.9/8] t7004: fix mistaken tag name

We have a series of tests which create signed tags with
various properties, but one test accidentally verifies a tag
from much earlier in the series.
---
 t/t7004-tag.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 2aac77af7..ee093b393 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1056,7 +1056,7 @@ test_expect_success GPG \
 	git tag -s -F sigblanknonlfile blanknonlfile-signed-tag &&
 	get_tag_msg blanknonlfile-signed-tag >actual &&
 	test_cmp expect actual &&
-	git tag -v signed-tag
+	git tag -v blanknonlfile-signed-tag
 '
 
 # messages with commented lines for signed tags:
-- 
2.17.0-140-g0b0cc9f867

Re: [PATCH 6/8] gpg-interface: find the last gpg signature line

[Ben Toews]

On Tue, Apr 10, 2018 at 4:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> That test was fixed a week ago:
>>> https://github.com/git/git/commit/a99d903f21d102a5768f19157085a9733aeb68dd
>>
>> Well, you cannot expect any reviewer to know about a change that has
>> never been sent to the list and has never been part of even 'pu'
>> branch, no matter how old such a private "fix" is.
>>
>> What other unpublished things that are not even on 'pu' do these
>> patches depend on?
>
> Answering my own question after digging a bit more, now I know that
> a99d903 comes from the 'private' branch in peff/git/ repository
> hosted at GitHub [*1*].  The branch builds on 'next' by merging many
> private branches, and 'jk/non-pgp-signatures' branch has that commit.
>
>     peff.git/private$ git log --oneline --boundary 0c8726318..7a526834e^2
>     c9ce7c5b7 gpg-interface: handle multiple signing tools
>     914951682 gpg-interface: handle bool user.signingkey
>     1f2ea84b3 gpg-interface: return signature type from parse_signature()
>     6d2ce6547 gpg-interface: prepare for parsing arbitrary PEM blocks
>     fb1d173db gpg-interface: find the last gpg signature line
>     6bc4e7e17 gpg-interface: extract gpg line matching helper
>     4f883ac49 gpg-interface: fix const-correctness of "eol" pointer
>     ae6529fdb gpg-interface: use size_t for signature buffer size
>     1bca4296b gpg-interface: modernize function declarations
>     a99d903f2 t7004: fix mistaken tag name
>     - 468165c1d Git 2.17
>
> It seems to me that Peff did the t7004 change as the earliest
> preliminary step of the series, but it caused confusion when it was
> not sent as part of the series by mistake.  Judging from the shape
> of the topic, I do not think this topic has any other hidden
> dependencies as it builds directly on top of v2.17.0.
>
> For those who are reading and reviewing from the sideline, I've
> attached that missing 0.9/8 patch at the end of this message.

Sorry for the confusion Junio. I'm not used to the mailing list
workflow and it seems that I missed a patch. I'll make sure to include
that patch in v2.

>
> [Footnote]
>
> *1* I do not know if it deserves to be called a bug, but it
>     certainly is an anomaly GitHub exhibits that a99d903f can be
>     viewed at https://github.com/git/git/commit/a99d903f..., as if
>     it is part of the official git/git history, when it only exists
>     in a fork of that repository.  I can understand why it happens
>     but it certainly is unexpected.
>
> -- >8 --
> From: Jeff King <peff@peff.net>
> Date: Tue, 3 Apr 2018 17:10:30 -0400
> Subject: [PATCH 0.9/8] t7004: fix mistaken tag name
>
> We have a series of tests which create signed tags with
> various properties, but one test accidentally verifies a tag
> from much earlier in the series.
> ---
>  t/t7004-tag.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 2aac77af7..ee093b393 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -1056,7 +1056,7 @@ test_expect_success GPG \
>         git tag -s -F sigblanknonlfile blanknonlfile-signed-tag &&
>         get_tag_msg blanknonlfile-signed-tag >actual &&
>         test_cmp expect actual &&
> -       git tag -v signed-tag
> +       git tag -v blanknonlfile-signed-tag
>  '
>
>  # messages with commented lines for signed tags:
> --
> 2.17.0-140-g0b0cc9f867
>



-- 
-Ben Toews

[PATCH 7/8] gpg-interface: prepare for parsing arbitrary PEM blocks

[Ben Toews]

From: Jeff King <peff@peff.net>

In preparation for handling more PEM blocks besides "PGP
SIGNATURE" and "PGP MESSAGE', let's break up the parsing to
parameterize the actual block type.
---
 gpg-interface.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 0647bd6348..0ba4a8ac3b 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -9,9 +9,6 @@
 static char *configured_signing_key;
 static const char *gpg_program = "gpg";
 
-#define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
-#define PGP_MESSAGE "-----BEGIN PGP MESSAGE-----"
-
 void signature_check_clear(struct signature_check *sigc)
 {
 	FREE_AND_NULL(sigc->payload);
@@ -101,10 +98,17 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
 		fputs(output, stderr);
 }
 
-static int is_gpg_start(const char *line)
+static int is_pem_start(const char *line)
 {
-	return starts_with(line, PGP_SIGNATURE) ||
-		starts_with(line, PGP_MESSAGE);
+	if (!skip_prefix(line, "-----BEGIN ", &line))
+		return 0;
+	if (!skip_prefix(line, "PGP SIGNATURE", &line) &&
+	    !skip_prefix(line, "PGP MESSAGE", &line))
+		return 0;
+	if (!starts_with(line, "-----"))
+		return 0;
+
+	return 1;
 }
 
 size_t parse_signature(const char *buf, size_t size)
@@ -114,7 +118,7 @@ size_t parse_signature(const char *buf, size_t size)
 	while (len < size) {
 		const char *eol;
 
-		if (is_gpg_start(buf + len))
+		if (is_pem_start(buf + len))
 			match = len;
 
 		eol = memchr(buf + len, '\n', size - len);
-- 
2.15.1 (Apple Git-101)

[PATCH 8/8] gpg-interface: handle alternative signature types

[Ben Toews]

From: Ben Toews <btoews@github.com>

Currently you can only sign commits and tags using "gpg".
You can _almost_ plug in a related tool like "gpgsm" (which
uses S/MIME-style signatures instead of PGP) using
gpg.program, as it has command-line compatibility. But there
are a few rough edges:

  1. gpgsm generates a slightly different PEM format than
     gpg. It says:

       -----BEGIN SIGNED MESSAGE-----

     instead of:

       -----BEGIN PGP SIGNATURE-----

     This actually works OK for signed commits, where we
     just dump the gpgsig header to gpg.program regardless.

     But for tags, we actually have to parse out the
     detached signature, and we fail to recognize the gpgsm
     version.

  2. You can't mix the two types of signatures easily, as
     we'd send the output to whatever tool you've
     configured. For verification, we'd ideally dispatch gpg
     signatures to gpg, gpgsm ones to gpgsm, and so on. For
     signing, you'd obviously have to pick a tool to use.

This patch introduces a set of configuration options for
defining a "signing tool", of which gpg may be just one.
With this patch you can:

  - define a new tool "foo" with signingtool.foo.program

  - map PEM strings to it for verification using
    signingtool.foo.pemtype

  - set it as your default tool for creating signatures
    using using signingtool.default

This subsumes the existing gpg config, as we have baked-in
config for signingtool.gpg that works just like the current
hard-coded behavior. And setting gpg.program becomes an
alias for signingtool.gpg.program.

This is enough to plug in gpgsm like:

  [signingtool "gpgsm"]
  program = gpgsm
  pemtype = "SIGNED MESSAGE"

In the future, this config scheme gives us options for
extending to other tools:

  - the tools all have to accept gpg-like options for now,
    but we could add signingtool.interface to meet other
    standards

  - we only match PEM headers now, but we could add other
    config for matching non-PEM types

The implementation is still in gpg-interface.c, and most of
the code internally refers to this as "gpg". I've left it
this way to keep the diff sane, and to make it clear that
we're not breaking anything gpg-related. This is probably OK
for now, as our tools must be gpg-like anyway. But renaming
everything would be an obvious next-step refactoring if we
want to offer support for more exotic tools (e.g., people
have asked before on the list about using OpenBSD signify).
---
 Documentation/config.txt |  40 +++++++++---
 builtin/fmt-merge-msg.c  |   6 +-
 builtin/receive-pack.c   |   7 +-
 builtin/tag.c            |   2 +-
 commit.c                 |   2 +-
 gpg-interface.c          | 165 ++++++++++++++++++++++++++++++++++++++++++-----
 gpg-interface.h          |  24 ++++++-
 log-tree.c               |   7 +-
 ref-filter.c             |   2 +-
 t/lib-gpg.sh             |  26 ++++++++
 t/t7510-signed-commit.sh |  32 ++++++++-
 tag.c                    |   2 +-
 12 files changed, 272 insertions(+), 43 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4e0cff87f6..7906123a59 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1727,16 +1727,38 @@ grep.fallbackToNoIndex::
 	If set to true, fall back to git grep --no-index if git grep
 	is executed outside of a git repository.  Defaults to false.
 
-gpg.program::
-	Use this custom program instead of "`gpg`" found on `$PATH` when
-	making or verifying a PGP signature. The program must support the
-	same command-line interface as GPG, namely, to verify a detached
-	signature, "`gpg --verify $file - <$signature`" is run, and the
-	program is expected to signal a good signature by exiting with
-	code 0, and to generate an ASCII-armored detached signature, the
-	standard input of "`gpg -bsau $key`" is fed with the contents to be
+signingtool.<name>.program::
+	The name of the program on `$PATH` to execute when making or
+	verifying a signature. This program will be used for making
+	signatures if `<name>` is configured as `signingtool.default`.
+	This program will be used for verifying signatures whose PEM
+	block type matches `signingtool.<name>.pemtype` (see below). The
+	program must support the same command-line interface as GPG.
+	To verify a detached signature,
+	"`gpg --verify $file - <$signature`" is run, and the program is
+	expected to signal a good signature by exiting with code 0.
+	To generate an ASCII-armored detached signature, the standard
+	input of "`gpg -bsau $key`" is fed with the contents to be
 	signed, and the program is expected to send the result to its
-	standard output.
+	standard output. By default, `signingtool.gpg.program` is set to
+	`gpg`.
+
+signingtool.<name>.pemtype::
+	The PEM block type associated with the signing tool named
+	`<name>`. For example, the block type of a GPG signature
+	starting with `-----BEGIN PGP SIGNATURE-----` is `PGP
+	SIGNATURE`. When verifying a signature with this PEM block type
+	the program specified in `signingtool.<name>.program` will be
+	used. By default `signingtool.gpg.pemtype` contains `PGP
+	SIGNATURE` and `PGP MESSAGE`.
+
+signingtool.default::
+	The `<name>` of the signing tool to use when creating
+	signatures (e.g., setting it to "foo" will use use the program
+	specified by `signingtool.foo.program`). Defaults to `gpg`.
+
+gpg.program::
+	Historical alias for `signingtool.gpg.program`.
 
 gui.commitMsgWidth::
 	Defines how wide the commit message window is in the
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 8e8a15ea4a..6aa8226308 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -490,14 +490,16 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
 		unsigned long size, len;
 		char *buf = read_sha1_file(sha1, &type, &size);
 		struct strbuf sig = STRBUF_INIT;
+		const struct signing_tool *tool;
 
 		if (!buf || type != OBJ_TAG)
 			goto next;
-		len = parse_signature(buf, size);
+		len = parse_signature(buf, size, &tool);
 
 		if (size == len)
 			; /* merely annotated */
-		else if (verify_signed_buffer(buf, len, buf + len, size - len, &sig, NULL)) {
+		else if (verify_signed_buffer(buf, len, buf + len, size - len,
+					      &sig, NULL, tool)) {
 			if (!sig.len)
 				strbuf_addstr(&sig, "gpg verification failed.\n");
 		}
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 75e7f18ace..be17dcaa78 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -631,6 +631,7 @@ static void prepare_push_cert_sha1(struct child_process *proc)
 		struct strbuf gpg_output = STRBUF_INIT;
 		struct strbuf gpg_status = STRBUF_INIT;
 		int bogs /* beginning_of_gpg_sig */;
+		const struct signing_tool *tool;
 
 		already_done = 1;
 		if (write_object_file(push_cert.buf, push_cert.len, "blob",
@@ -640,10 +641,10 @@ static void prepare_push_cert_sha1(struct child_process *proc)
 		memset(&sigcheck, '\0', sizeof(sigcheck));
 		sigcheck.result = 'N';
 
-		bogs = parse_signature(push_cert.buf, push_cert.len);
+		bogs = parse_signature(push_cert.buf, push_cert.len, &tool);
 		if (verify_signed_buffer(push_cert.buf, bogs,
 					 push_cert.buf + bogs, push_cert.len - bogs,
-					 &gpg_output, &gpg_status) < 0) {
+					 &gpg_output, &gpg_status, tool) < 0) {
 			; /* error running gpg */
 		} else {
 			sigcheck.payload = push_cert.buf;
@@ -1565,7 +1566,7 @@ static void queue_commands_from_cert(struct command **tail,
 		die("malformed push certificate %.*s", 100, push_cert->buf);
 	else
 		boc += 2;
-	eoc = push_cert->buf + parse_signature(push_cert->buf, push_cert->len);
+	eoc = push_cert->buf + parse_signature(push_cert->buf, push_cert->len, NULL);
 
 	while (boc < eoc) {
 		const char *eol = memchr(boc, '\n', eoc - boc);
diff --git a/builtin/tag.c b/builtin/tag.c
index da186691ed..adc37a28c5 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -178,7 +178,7 @@ static void write_tag_body(int fd, const struct object_id *oid)
 		return;
 	}
 	sp += 2; /* skip the 2 LFs */
-	write_or_die(fd, sp, parse_signature(sp, buf + size - sp));
+	write_or_die(fd, sp, parse_signature(sp, buf + size - sp, NULL));
 
 	free(buf);
 }
diff --git a/commit.c b/commit.c
index 00c99c7272..056942df7f 100644
--- a/commit.c
+++ b/commit.c
@@ -1209,7 +1209,7 @@ static void handle_signed_tag(struct commit *parent, struct commit_extra_header
 	buf = read_sha1_file(desc->obj->oid.hash, &type, &size);
 	if (!buf || type != OBJ_TAG)
 		goto free_return;
-	len = parse_signature(buf, size);
+	len = parse_signature(buf, size, NULL);
 	if (size == len)
 		goto free_return;
 	/*
diff --git a/gpg-interface.c b/gpg-interface.c
index 0ba4a8ac3b..0e2a82e8e5 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -7,7 +7,60 @@
 #include "tempfile.h"
 
 static char *configured_signing_key;
-static const char *gpg_program = "gpg";
+static const char *default_signing_tool = "gpg";
+static struct signing_tool *signing_tool_config;
+
+static struct signing_tool *alloc_signing_tool(void)
+{
+	struct signing_tool *ret;
+	ret = xcalloc(1, sizeof(*ret));
+	ret->pemtype.strdup_strings = 1;
+	return ret;
+}
+
+/*
+ * Our default tool config is too complicated to specify as a constant
+ * initializer, so we lazily create it as needed.
+ */
+static void init_signing_tool_defaults(void) {
+	struct signing_tool *tool;
+
+	if (signing_tool_config)
+		return;
+
+	tool = alloc_signing_tool();
+	tool->name = xstrdup("gpg");
+	tool->program = xstrdup("gpg");
+	string_list_append(&tool->pemtype, "PGP SIGNATURE");
+	string_list_append(&tool->pemtype, "PGP MESSAGE");
+
+	tool->next = signing_tool_config;
+	signing_tool_config = tool;
+}
+
+static struct signing_tool *get_signing_tool(const char *name) {
+	struct signing_tool *tool;
+
+	init_signing_tool_defaults();
+
+	for (tool = signing_tool_config; tool; tool = tool->next) {
+		if (!strcmp(name, tool->name))
+			return tool;
+	}
+	return NULL;
+}
+
+static struct signing_tool *get_or_create_signing_tool(const char *name)
+{
+	struct signing_tool *tool = get_signing_tool(name);
+	if (!tool) {
+		tool = alloc_signing_tool();
+		tool->name = xstrdup(name);
+		tool->next = signing_tool_config;
+		signing_tool_config = tool;
+	}
+	return tool;
+}
 
 void signature_check_clear(struct signature_check *sigc)
 {
@@ -71,7 +124,7 @@ int check_signature(const char *payload, size_t plen, const char *signature,
 	sigc->result = 'N';
 
 	status = verify_signed_buffer(payload, plen, signature, slen,
-				      &gpg_output, &gpg_status);
+				      &gpg_output, &gpg_status, NULL);
 	if (status && !gpg_output.len)
 		goto out;
 	sigc->payload = xmemdupz(payload, plen);
@@ -98,32 +151,49 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
 		fputs(output, stderr);
 }
 
-static int is_pem_start(const char *line)
+static int is_pem_start(const char *line, struct signing_tool **out_tool)
 {
+	struct signing_tool *tool;
+
 	if (!skip_prefix(line, "-----BEGIN ", &line))
 		return 0;
-	if (!skip_prefix(line, "PGP SIGNATURE", &line) &&
-	    !skip_prefix(line, "PGP MESSAGE", &line))
-		return 0;
-	if (!starts_with(line, "-----"))
-		return 0;
 
-	return 1;
+	init_signing_tool_defaults();
+
+	for (tool = signing_tool_config; tool; tool = tool->next) {
+		int i;
+		for (i = 0; i < tool->pemtype.nr; i++) {
+			const char *match = tool->pemtype.items[i].string;
+			const char *end;
+			if (skip_prefix(line, match, &end) &&
+			    starts_with(end, "-----")) {
+				*out_tool = tool;
+				return 1;
+			}
+		}
+	}
+	return 0;
 }
 
-size_t parse_signature(const char *buf, size_t size)
+size_t parse_signature(const char *buf, size_t size,
+		       const struct signing_tool **out_tool)
 {
 	size_t len = 0;
 	size_t match = size;
+	struct signing_tool *tool = NULL;
+
 	while (len < size) {
 		const char *eol;
 
-		if (is_pem_start(buf + len))
+		if (is_pem_start(buf + len, &tool))
 			match = len;
 
 		eol = memchr(buf + len, '\n', size - len);
 		len += eol ? eol - (buf + len) + 1 : size - len;
 	}
+
+	if (out_tool)
+		*out_tool = tool;
 	return match;
 }
 
@@ -135,6 +205,9 @@ void set_signing_key(const char *key)
 
 int git_gpg_config(const char *var, const char *value, void *cb)
 {
+	const char *key, *name = NULL;
+	int name_len;
+
 	if (!strcmp(var, "user.signingkey")) {
 		if (!value)
 			return config_error_nonbool(var);
@@ -143,12 +216,43 @@ int git_gpg_config(const char *var, const char *value, void *cb)
 	}
 
 	if (!strcmp(var, "gpg.program")) {
+		struct signing_tool *tool = get_or_create_signing_tool("gpg");
+
 		if (!value)
 			return config_error_nonbool(var);
-		gpg_program = xstrdup(value);
+
+		free(tool->program);
+		tool->program = xstrdup(value);
 		return 0;
 	}
 
+	if (!strcmp(var, "signingtool.default"))
+		return git_config_string(&default_signing_tool, var, value);
+
+	if (!parse_config_key(var, "signingtool", &name, &name_len, &key) && name) {
+		char *tmpname = xmemdupz(name, name_len);
+		struct signing_tool *tool = get_or_create_signing_tool(tmpname);
+
+		free(tmpname);
+
+		if (!strcmp(key, "program")) {
+			if (!value)
+				return config_error_nonbool(var);
+
+			free(tool->program);
+			tool->program = xstrdup(value);
+			return 0;
+		}
+
+		if (!strcmp(key, "pemtype")) {
+			if (!value)
+				return config_error_nonbool(var);
+
+			string_list_append(&tool->pemtype, value);
+			return 0;
+		}
+	}
+
 	return 0;
 }
 
@@ -159,7 +263,9 @@ const char *get_signing_key(void)
 	return git_committer_info(IDENT_STRICT|IDENT_NO_DATE);
 }
 
-int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
+static int sign_buffer_with(struct strbuf *buffer, struct strbuf *signature,
+			    const char *signing_key,
+			    const struct signing_tool *tool)
 {
 	struct child_process gpg = CHILD_PROCESS_INIT;
 	int ret;
@@ -167,7 +273,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
 	struct strbuf gpg_status = STRBUF_INIT;
 
 	argv_array_pushl(&gpg.args,
-			 gpg_program,
+			 tool->program,
 			 "--status-fd=2",
 			 "-bsau", signing_key,
 			 NULL);
@@ -200,15 +306,42 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
 	return 0;
 }
 
+int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
+{
+	struct signing_tool *tool = get_signing_tool(default_signing_tool);
+	if (!tool || !tool->program)
+		return error(_("default signing tool '%s' has no program configured"),
+			     default_signing_tool);
+	return sign_buffer_with(buffer, signature, signing_key, tool);
+}
+
 int verify_signed_buffer(const char *payload, size_t payload_size,
 			 const char *signature, size_t signature_size,
-			 struct strbuf *gpg_output, struct strbuf *gpg_status)
+			 struct strbuf *gpg_output, struct strbuf *gpg_status,
+			 const struct signing_tool *tool)
 {
 	struct child_process gpg = CHILD_PROCESS_INIT;
 	struct tempfile *temp;
 	int ret;
 	struct strbuf buf = STRBUF_INIT;
 
+	if (!tool) {
+		parse_signature(signature, signature_size, &tool);
+		if (!tool) {
+			/*
+			 * The caller didn't tell us which tool to use, and we
+			 * didn't recognize the format. Historically we've fed
+			 * these cases to blindly to gpg, so let's continue to
+			 * do so.
+			 */
+			tool = get_signing_tool("gpg");
+		}
+	}
+
+	if (!tool->program)
+		return error(_("signing tool '%s' has no program configured"),
+			     tool->name);
+
 	temp = mks_tempfile_t(".git_vtag_tmpXXXXXX");
 	if (!temp)
 		return error_errno(_("could not create temporary file"));
@@ -221,7 +354,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 	}
 
 	argv_array_pushl(&gpg.args,
-			 gpg_program,
+			 tool->program,
 			 "--status-fd=1",
 			 "--keyid-format=long",
 			 "--verify", temp->filename.buf, "-",
diff --git a/gpg-interface.h b/gpg-interface.h
index a5e6517ae6..cee0dfe401 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -23,15 +23,27 @@ struct signature_check {
 	char *key;
 };
 
+struct signing_tool {
+	char *name;
+	char *program;
+	struct string_list pemtype;
+	struct signing_tool *next;
+};
+
 void signature_check_clear(struct signature_check *sigc);
 
 /*
- * Look at GPG signed content (e.g. a signed tag object), whose
+ * Look for signed content (e.g. a signed tag object), whose
  * payload is followed by a detached signature on it.  Return the
  * offset where the embedded detached signature begins, or the end of
  * the data when there is no such signature.
+ *
+ * If out_tool is non-NULL and a signature is found, it will be
+ * pointed at the signing_tool that corresponds to the found
+ * signature type.
  */
-size_t parse_signature(const char *buf, size_t size);
+size_t parse_signature(const char *buf, unsigned long size,
+		       const struct signing_tool **out_tool);
 
 void parse_gpg_output(struct signature_check *);
 
@@ -48,10 +60,16 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature,
  * Run "gpg" to see if the payload matches the detached signature.
  * gpg_output, when set, receives the diagnostic output from GPG.
  * gpg_status, when set, receives the status output from GPG.
+ *
+ * Typically the "tool" argument should come from a previous call to
+ * parse_signature(). If it's NULL, then verify_signed_buffer() will
+ * try to choose the appropriate tool based on the contents of the
+ * "signature" buffer.
  */
 int verify_signed_buffer(const char *payload, size_t payload_size,
 			 const char *signature, size_t signature_size,
-			 struct strbuf *gpg_output, struct strbuf *gpg_status);
+			 struct strbuf *gpg_output, struct strbuf *gpg_status,
+			 const struct signing_tool *tool);
 
 int git_gpg_config(const char *, const char *, void *);
 void set_signing_key(const char *);
diff --git a/log-tree.c b/log-tree.c
index bdf23c5f7b..f8fdff6e65 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -456,7 +456,7 @@ static void show_signature(struct rev_info *opt, struct commit *commit)
 
 	status = verify_signed_buffer(payload.buf, payload.len,
 				      signature.buf, signature.len,
-				      &gpg_output, NULL);
+				      &gpg_output, NULL, NULL);
 	if (status && !gpg_output.len)
 		strbuf_addstr(&gpg_output, "No signature\n");
 
@@ -498,6 +498,7 @@ static void show_one_mergetag(struct commit *commit,
 	struct strbuf verify_message;
 	int status, nth;
 	size_t payload_size, gpg_message_offset;
+	const struct signing_tool *tool;
 
 	hash_object_file(extra->value, extra->len, type_name(OBJ_TAG), &oid);
 	tag = lookup_tag(&oid);
@@ -520,14 +521,14 @@ static void show_one_mergetag(struct commit *commit,
 			    "parent #%d, tagged '%s'\n", nth + 1, tag->tag);
 	gpg_message_offset = verify_message.len;
 
-	payload_size = parse_signature(extra->value, extra->len);
+	payload_size = parse_signature(extra->value, extra->len, &tool);
 	status = -1;
 	if (extra->len > payload_size) {
 		/* could have a good signature */
 		if (!verify_signed_buffer(extra->value, payload_size,
 					  extra->value + payload_size,
 					  extra->len - payload_size,
-					  &verify_message, NULL))
+					  &verify_message, NULL, tool))
 			status = 0; /* good */
 		else if (verify_message.len <= gpg_message_offset)
 			strbuf_addstr(&verify_message, "No signature\n");
diff --git a/ref-filter.c b/ref-filter.c
index 45fc56216a..6ab66b9a7c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1009,7 +1009,7 @@ static void find_subpos(const char *buf, unsigned long sz,
 		buf++;
 
 	/* parse signature first; we might not even have a subject line */
-	*sig = buf + parse_signature(buf, strlen(buf));
+	*sig = buf + parse_signature(buf, strlen(buf), NULL);
 	*siglen = strlen(*sig);
 
 	/* subject is first non-empty line */
diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index a5d3b2cbaa..08d23b0cf5 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -56,3 +56,29 @@ sanitize_pgp() {
 		/^-----BEGIN PGP/ and $in_pgp = 1;
 	'
 }
+
+create_fake_signer () {
+	write_script fake-signer <<-\EOF
+	if [ "$1 $2" = "--status-fd=2 -bsau" ]; then
+		echo >&2 "[GNUPG:] BEGIN_SIGNING"
+		echo >&2 "[GNUPG:] SIG_CREATED D 1 SHA256 0 1513792449 4A7FF9E2330D22B19213A4E9E9C423BE17EFEE70"
+		# avoid "-" in echo arguments
+		printf "%s\n" \
+		  "-----BEGIN FAKE SIGNER SIGNATURE-----" \
+		  "fake-signature" \
+		  "-----END FAKE SIGNER SIGNATURE-----"
+		exit 0
+
+	elif [ "$1 $2 $3" = "--status-fd=1 --keyid-format=long --verify" ]; then
+		echo "[GNUPG:] NEWSIG"
+		echo "[GNUPG:] GOODSIG 4A7FF9E2330D22B19213A4E9E9C423BE17EFEE70 /CN=Some User/EMail=some@user.email"
+		echo "[GNUPG:] TRUST_FULLY 0 shell"
+		echo >&2 "Good signature from /CN=Some User/EMail=some@user.email"
+		exit 0
+
+	else
+		echo "bad arguments" 1>&2
+		exit 1
+	fi
+	EOF
+}
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 762135adea..848a823302 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -5,6 +5,10 @@ test_description='signed commit tests'
 GNUPGHOME_NOT_USED=$GNUPGHOME
 . "$TEST_DIRECTORY/lib-gpg.sh"
 
+test_expect_success 'create fake signing program' '
+	create_fake_signer
+'
+
 test_expect_success GPG 'create signed commits' '
 	test_when_finished "test_unconfig commit.gpgsign" &&
 
@@ -51,13 +55,33 @@ test_expect_success GPG 'create signed commits' '
 	# commit.gpgsign is still on but this must not be signed
 	git tag ninth-unsigned $(echo 9 | git commit-tree HEAD^{tree}) &&
 	# explicit -S of course must sign.
-	git tag tenth-signed $(echo 9 | git commit-tree -S HEAD^{tree})
+	git tag tenth-signed $(echo 9 | git commit-tree -S HEAD^{tree}) &&
+
+	git config signingtool.fake.program "$TRASH_DIRECTORY/fake-signer" &&
+	git config signingtool.fake.pemtype "FAKE SIGNER SIGNATURE" &&
+	echo 11 >file && test_tick && git commit -a -m eleventh &&
+	git tag eleventh-pgp-signed &&
+	git cat-file -p eleventh-pgp-signed >actual &&
+	grep "PGP SIGNATURE" actual &&
+
+	git config gpg.program "$TRASH_DIRECTORY/fake-signer" &&
+	echo 12 >file && test_tick && git commit -a -m twelfth && test_unconfig gpg.program &&
+	git tag twelfth-fake-signed &&
+	git cat-file -p twelfth-fake-signed >actual &&
+	grep "FAKE SIGNER SIGNATURE" actual &&
+
+	git config signingtool.default fake &&
+	echo 13 >file && test_tick && git commit -a -m thirteenth && test_unconfig signingtool.default &&
+	git tag thirteenth-fake-signed &&
+	git cat-file -p thirteenth-fake-signed >actual &&
+	grep "FAKE SIGNER SIGNATURE" actual
 '
 
 test_expect_success GPG 'verify and show signatures' '
 	(
 		for commit in initial second merge fourth-signed \
-			fifth-signed sixth-signed seventh-signed tenth-signed
+			fifth-signed sixth-signed seventh-signed tenth-signed \
+			eleventh-pgp-signed twelfth-fake-signed thirteenth-fake-signed
 		do
 			git verify-commit $commit &&
 			git show --pretty=short --show-signature $commit >actual &&
@@ -98,7 +122,9 @@ test_expect_success GPG 'verify-commit exits success on untrusted signature' '
 
 test_expect_success GPG 'verify signatures with --raw' '
 	(
-		for commit in initial second merge fourth-signed fifth-signed sixth-signed seventh-signed
+		for commit in initial second merge fourth-signed fifth-signed sixth-signed \
+			seventh-signed eleventh-pgp-signed twelfth-fake-signed \
+			thirteenth-fake-signed
 		do
 			git verify-commit --raw $commit 2>actual &&
 			grep "GOODSIG" actual &&
diff --git a/tag.c b/tag.c
index 66210fd477..2d557e1f5e 100644
--- a/tag.c
+++ b/tag.c
@@ -15,7 +15,7 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
 
 	memset(&sigc, 0, sizeof(sigc));
 
-	payload_size = parse_signature(buf, size);
+	payload_size = parse_signature(buf, size, NULL);
 
 	if (size == payload_size) {
 		if (flags & GPG_VERIFY_VERBOSE)
-- 
2.15.1 (Apple Git-101)

Re: [PATCH 8/8] gpg-interface: handle alternative signature types

[Stefan Beller]

Hi Ben,

On Mon, Apr 9, 2018 at 1:41 PM, Ben Toews <mastahyeti@gmail.com> wrote:
> From: Ben Toews <btoews@github.com>
>
> Currently you can only sign commits and tags using "gpg".
> You can _almost_ plug in a related tool like "gpgsm" (which
> uses S/MIME-style signatures instead of PGP) using
> gpg.program, as it has command-line compatibility. But there
> are a few rough edges:
>
>   1. gpgsm generates a slightly different PEM format than
>      gpg. It says:
>
>        -----BEGIN SIGNED MESSAGE-----
>
>      instead of:
>
>        -----BEGIN PGP SIGNATURE-----
>
>      This actually works OK for signed commits, where we
>      just dump the gpgsig header to gpg.program regardless.
>
>      But for tags, we actually have to parse out the
>      detached signature, and we fail to recognize the gpgsm
>      version.
>
>   2. You can't mix the two types of signatures easily, as
>      we'd send the output to whatever tool you've
>      configured. For verification, we'd ideally dispatch gpg
>      signatures to gpg, gpgsm ones to gpgsm, and so on. For
>      signing, you'd obviously have to pick a tool to use.
>
> This patch introduces a set of configuration options for
> defining a "signing tool", of which gpg may be just one.
> With this patch you can:
>
>   - define a new tool "foo" with signingtool.foo.program
>
>   - map PEM strings to it for verification using
>     signingtool.foo.pemtype
>
>   - set it as your default tool for creating signatures
>     using using signingtool.default
>
> This subsumes the existing gpg config, as we have baked-in
> config for signingtool.gpg that works just like the current
> hard-coded behavior. And setting gpg.program becomes an
> alias for signingtool.gpg.program.
>
> This is enough to plug in gpgsm like:
>
>   [signingtool "gpgsm"]
>   program = gpgsm
>   pemtype = "SIGNED MESSAGE"
>
> In the future, this config scheme gives us options for
> extending to other tools:
>
>   - the tools all have to accept gpg-like options for now,
>     but we could add signingtool.interface to meet other
>     standards
>
>   - we only match PEM headers now, but we could add other
>     config for matching non-PEM types
>
> The implementation is still in gpg-interface.c, and most of
> the code internally refers to this as "gpg". I've left it
> this way to keep the diff sane, and to make it clear that
> we're not breaking anything gpg-related. This is probably OK
> for now, as our tools must be gpg-like anyway. But renaming
> everything would be an obvious next-step refactoring if we
> want to offer support for more exotic tools (e.g., people
> have asked before on the list about using OpenBSD signify).

Please sign off your patch, see Documentation/SubmittingPatches

> ---
>  Documentation/config.txt |  40 +++++++++---
>  builtin/fmt-merge-msg.c  |   6 +-
>  builtin/receive-pack.c   |   7 +-
>  builtin/tag.c            |   2 +-
>  commit.c                 |   2 +-
>  gpg-interface.c          | 165 ++++++++++++++++++++++++++++++++++++++++++-----
>  gpg-interface.h          |  24 ++++++-
>  log-tree.c               |   7 +-
>  ref-filter.c             |   2 +-
>  t/lib-gpg.sh             |  26 ++++++++
>  t/t7510-signed-commit.sh |  32 ++++++++-
>  tag.c                    |   2 +-
>  12 files changed, 272 insertions(+), 43 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 4e0cff87f6..7906123a59 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1727,16 +1727,38 @@ grep.fallbackToNoIndex::
>         If set to true, fall back to git grep --no-index if git grep
>         is executed outside of a git repository.  Defaults to false.
>
> -gpg.program::
> -       Use this custom program instead of "`gpg`" found on `$PATH` when
> -       making or verifying a PGP signature. The program must support the
> -       same command-line interface as GPG, namely, to verify a detached
> -       signature, "`gpg --verify $file - <$signature`" is run, and the
> -       program is expected to signal a good signature by exiting with
> -       code 0, and to generate an ASCII-armored detached signature, the
> -       standard input of "`gpg -bsau $key`" is fed with the contents to be
> +signingtool.<name>.program::
> +       The name of the program on `$PATH` to execute when making or
> +       verifying a signature. This program will be used for making
> +       signatures if `<name>` is configured as `signingtool.default`.
> +       This program will be used for verifying signatures whose PEM
> +       block type matches `signingtool.<name>.pemtype` (see below). The
> +       program must support the same command-line interface as GPG.
> +       To verify a detached signature,
> +       "`gpg --verify $file - <$signature`" is run, and the program is
> +       expected to signal a good signature by exiting with code 0.
> +       To generate an ASCII-armored detached signature, the standard
> +       input of "`gpg -bsau $key`" is fed with the contents to be
>         signed, and the program is expected to send the result to its
> -       standard output.
> +       standard output. By default, `signingtool.gpg.program` is set to
> +       `gpg`.

I wonder if the mention of the default is best put at the pgp.program
instead of here, although the mention of it being an alias strongly suggest
we'd want to deprecate it eventually.

> +
> +signingtool.<name>.pemtype::
> +       The PEM block type associated with the signing tool named
> +       `<name>`. For example, the block type of a GPG signature
> +       starting with `-----BEGIN PGP SIGNATURE-----` is `PGP
> +       SIGNATURE`. When verifying a signature with this PEM block type
> +       the program specified in `signingtool.<name>.program` will be
> +       used. By default `signingtool.gpg.pemtype` contains `PGP
> +       SIGNATURE` and `PGP MESSAGE`.
> +
> +signingtool.default::
> +       The `<name>` of the signing tool to use when creating
> +       signatures (e.g., setting it to "foo" will use use the program
> +       specified by `signingtool.foo.program`). Defaults to `gpg`.
> +
> +gpg.program::
> +       Historical alias for `signingtool.gpg.program`.
>
>  gui.commitMsgWidth::
>         Defines how wide the commit message window is in the


> +/*
> + * Our default tool config is too complicated to specify as a constant
> + * initializer, so we lazily create it as needed.
> + */
> +static void init_signing_tool_defaults(void) {

Coding Style: please put the opening brace in a new
line when staring a function (but not for statements).

Re: [PATCH 8/8] gpg-interface: handle alternative signature types

[Eric Sunshine]

On Mon, Apr 9, 2018 at 4:41 PM, Ben Toews <mastahyeti@gmail.com> wrote:
> [...]
> This patch introduces a set of configuration options for
> defining a "signing tool", of which gpg may be just one.
> With this patch you can:
>
>   - define a new tool "foo" with signingtool.foo.program
>
>   - map PEM strings to it for verification using
>     signingtool.foo.pemtype
>
>   - set it as your default tool for creating signatures
>     using using signingtool.default

s/using using/using/

> This subsumes the existing gpg config, as we have baked-in
> config for signingtool.gpg that works just like the current
> hard-coded behavior. And setting gpg.program becomes an
> alias for signingtool.gpg.program.
>
> This is enough to plug in gpgsm like:
>
>   [signingtool "gpgsm"]
>   program = gpgsm
>   pemtype = "SIGNED MESSAGE"

How confident are we that _all_ possible signing programs will conform
to the "-----BEGIN %s-----" pattern? If we're not confident, then
perhaps the user should be providing the full string here, not just
the '%s' part?

Further, I can infer from PGP itself, as well as from reading the code
that the 'pemtype' key can be specified multiple times; it would be
nice to mention that in the commit message.

> [...]
> The implementation is still in gpg-interface.c, and most of
> the code internally refers to this as "gpg". I've left it
> this way to keep the diff sane, and to make it clear that
> we're not breaking anything gpg-related. This is probably OK
> for now, as our tools must be gpg-like anyway. But renaming
> everything would be an obvious next-step refactoring if we
> want to offer support for more exotic tools (e.g., people
> have asked before on the list about using OpenBSD signify).

I can't decide if this paragraph belongs in the commit message proper
(as it currently is) or if it is commentary which should be placed
below the "---" line just above the diffstat. It feels more like
commentary, but not a big deal, and not itself worth a re-roll.

> ---
>  Documentation/config.txt |  40 +++++++++---
>  builtin/fmt-merge-msg.c  |   6 +-
>  builtin/receive-pack.c   |   7 +-
>  builtin/tag.c            |   2 +-
>  commit.c                 |   2 +-
>  gpg-interface.c          | 165 ++++++++++++++++++++++++++++++++++++++++++-----
>  gpg-interface.h          |  24 ++++++-
>  log-tree.c               |   7 +-
>  ref-filter.c             |   2 +-
>  t/lib-gpg.sh             |  26 ++++++++
>  t/t7510-signed-commit.sh |  32 ++++++++-
>  tag.c                    |   2 +-
>  12 files changed, 272 insertions(+), 43 deletions(-)

Due to its length, this patch is painfully time-consuming to review,
and asks reviewers to keep track of too many details at one time. As a
consequence, it's likely to scare away potential reviewers and limit
the quality of those reviews it does receive. If you break the changes
down into a series of much smaller patches which hold the hands of
reviewers, then you are likely to attract more and better reviews.
Please consider doing so.

Each patch should be reasonably small and easy to digest. Each patch
should build logically upon the patch or patches preceding it, thus
incrementally building up a picture of the complete change. A (very)
rough idea of a series of smaller patches corresponding to this
feature might be:

1. introduce 'struct signing_tool' and the bare machinery for managing them

2. convert PGP from a hard-coded signer to a 'signing_tool' signer

3. add support for the new configuration variables

It's likely that these steps can be broken into even smaller, more
reviewer-friendly ones; exactly how to do so may become apparent as
you think about how the patch series should be structured. For
instance, perhaps step 3 could be divided into:

3.1. add support for "signingtool.foo" variables
3.2. retrofit "gpg.program" to be alias of "signingtool.gpg.program"

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -1727,16 +1727,38 @@ grep.fallbackToNoIndex::
> -gpg.program::
> -       Use this custom program instead of "`gpg`" found on `$PATH` when
> -       making or verifying a PGP signature. The program must support the
> -       same command-line interface as GPG, namely, to verify a detached
> -       signature, "`gpg --verify $file - <$signature`" is run, and the
> -       program is expected to signal a good signature by exiting with
> -       code 0, and to generate an ASCII-armored detached signature, the
> -       standard input of "`gpg -bsau $key`" is fed with the contents to be
> +signingtool.<name>.program::
> +       The name of the program on `$PATH` to execute when making or

Why does the program need to be on $PATH? That seems like an
unnecessarily harsh limitation, one which wasn't shared by
gpg.program. (Reading the code, it looks like 'program' does not in
fact need to be on $PATH, so this documentation is wrong.)

> +       verifying a signature. This program will be used for making
> +       signatures if `<name>` is configured as `signingtool.default`.
> +       This program will be used for verifying signatures whose PEM
> +       block type matches `signingtool.<name>.pemtype` (see below). The
> +       program must support the same command-line interface as GPG.
> [...]
> +signingtool.<name>.pemtype::
> +       The PEM block type associated with the signing tool named
> +       `<name>`. For example, the block type of a GPG signature
> +       starting with `-----BEGIN PGP SIGNATURE-----` is `PGP
> +       SIGNATURE`. When verifying a signature with this PEM block type
> +       the program specified in `signingtool.<name>.program` will be
> +       used. By default `signingtool.gpg.pemtype` contains `PGP
> +       SIGNATURE` and `PGP MESSAGE`.

Although it's somewhat inferred for the PGP case, the documentation
really needs to state explicitly that multiple 'pemtype's can be
specified, and it needs to explain how to do so. Reading the code, I
see that one does so by specifying 'pemtype' key multiple times, but
the documentation should say this.

> diff --git a/gpg-interface.c b/gpg-interface.c
> @@ -143,12 +216,43 @@ int git_gpg_config(const char *var, const char *value, void *cb)
>         if (!strcmp(var, "gpg.program")) {
> +               struct signing_tool *tool = get_or_create_signing_tool("gpg");

Not at all worth a re-roll, but the get_or_create_signing_tool()
invocation could be moved below the "if (!value)" conditional. (The
declaration of 'tool', of course, would remain here.)

>                 if (!value)
>                         return config_error_nonbool(var);
> -               gpg_program = xstrdup(value);
> +
> +               free(tool->program);
> +               tool->program = xstrdup(value);
>                 return 0;
>         }
> @@ -200,15 +306,42 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
>  int verify_signed_buffer(const char *payload, size_t payload_size,
>                          const char *signature, size_t signature_size,
> -                        struct strbuf *gpg_output, struct strbuf *gpg_status)
> +                        struct strbuf *gpg_output, struct strbuf *gpg_status,
> +                        const struct signing_tool *tool)
>  {
>         [...]
> +       if (!tool) {
> +               parse_signature(signature, signature_size, &tool);
> +               if (!tool) {
> +                       /*
> +                        * The caller didn't tell us which tool to use, and we
> +                        * didn't recognize the format. Historically we've fed
> +                        * these cases to blindly to gpg, so let's continue to

s/to blindly/blindly/

> +                        * do so.
> +                        */
> +                       tool = get_signing_tool("gpg");
> +               }
> +       }
> diff --git a/gpg-interface.h b/gpg-interface.h
> @@ -48,10 +60,16 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature,
>   * Run "gpg" to see if the payload matches the detached signature.
>   * gpg_output, when set, receives the diagnostic output from GPG.
>   * gpg_status, when set, receives the status output from GPG.
> + *
> + * Typically the "tool" argument should come from a previous call to

s/Typically/&,/

> + * parse_signature(). If it's NULL, then verify_signed_buffer() will
> + * try to choose the appropriate tool based on the contents of the
> + * "signature" buffer.
>   */
> diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
> @@ -56,3 +56,29 @@ sanitize_pgp() {
> +create_fake_signer () {
> +       write_script fake-signer <<-\EOF
> +       if [ "$1 $2" = "--status-fd=2 -bsau" ]; then

Style: This codebase uses 'test' rather than '['. Also, 'then' is
placed on its own line and the semicolon omitted.

> +               echo >&2 "[GNUPG:] BEGIN_SIGNING"
> +               echo >&2 "[GNUPG:] SIG_CREATED D 1 SHA256 0 1513792449 4A7FF9E2330D22B19213A4E9E9C423BE17EFEE70"
> +               # avoid "-" in echo arguments
> +               printf "%s\n" \
> +                 "-----BEGIN FAKE SIGNER SIGNATURE-----" \
> +                 "fake-signature" \
> +                 "-----END FAKE SIGNER SIGNATURE-----"

If you use 'cat' with a here-doc, then you don't need the comment
about avoiding "-".

    cat <<-\END
    -----BEGIN FAKE SIGNER SIGNATURE-----
    fake-signature
    -----END FAKE SIGNER SIGNATURE-----
   END

> +               exit 0
> +
> +       elif [ "$1 $2 $3" = "--status-fd=1 --keyid-format=long --verify" ]; then
> +               echo "[GNUPG:] NEWSIG"
> +               echo "[GNUPG:] GOODSIG 4A7FF9E2330D22B19213A4E9E9C423BE17EFEE70 /CN=Some User/EMail=some@user.email"
> +               echo "[GNUPG:] TRUST_FULLY 0 shell"

Another good place to use 'cat' with here-doc.

> +               echo >&2 "Good signature from /CN=Some User/EMail=some@user.email"
> +               exit 0
> +
> +       else
> +               echo "bad arguments" 1>&2
> +               exit 1
> +       fi
> +       EOF
> +}
> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
> @@ -51,13 +55,33 @@ test_expect_success GPG 'create signed commits' '
> +       git tag tenth-signed $(echo 9 | git commit-tree -S HEAD^{tree}) &&
> +
> +       git config signingtool.fake.program "$TRASH_DIRECTORY/fake-signer" &&
> +       git config signingtool.fake.pemtype "FAKE SIGNER SIGNATURE" &&
> +       echo 11 >file && test_tick && git commit -a -m eleventh &&

It's normally frowned upon in tests to string together a bunch of
&&-chained commands on a single line, but since this script is already
full of this stylized 'commit' invocation, it may be okay. However...

> +       git tag eleventh-pgp-signed &&
> +       git cat-file -p eleventh-pgp-signed >actual &&
> +       grep "PGP SIGNATURE" actual &&
> +
> +       git config gpg.program "$TRASH_DIRECTORY/fake-signer" &&
> +       echo 12 >file && test_tick && git commit -a -m twelfth && test_unconfig gpg.program &&

Place the test_unconfig() on its own line so it can be clearly seen by
someone scanning an eye over the test; otherwise, the test_unconfig()
is easily overlooked, with the result that the reader may get a false
impression of what's going on.

> +       git tag twelfth-fake-signed &&
> +       git cat-file -p twelfth-fake-signed >actual &&
> +       grep "FAKE SIGNER SIGNATURE" actual &&
> +
> +       git config signingtool.default fake &&
> +       echo 13 >file && test_tick && git commit -a -m thirteenth && test_unconfig signingtool.default &&

Ditto.

> +       git tag thirteenth-fake-signed &&
> +       git cat-file -p thirteenth-fake-signed >actual &&
> +       grep "FAKE SIGNER SIGNATURE" actual
>  '

Re: [PATCH 8/8] gpg-interface: handle alternative signature types

[Ben Toews]

On Tue, Apr 10, 2018 at 2:24 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Apr 9, 2018 at 4:41 PM, Ben Toews <mastahyeti@gmail.com> wrote:
>> [...]
>> This patch introduces a set of configuration options for
>> defining a "signing tool", of which gpg may be just one.
>> With this patch you can:
>>
>>   - define a new tool "foo" with signingtool.foo.program
>>
>>   - map PEM strings to it for verification using
>>     signingtool.foo.pemtype
>>
>>   - set it as your default tool for creating signatures
>>     using using signingtool.default
>
> s/using using/using/
>
>> This subsumes the existing gpg config, as we have baked-in
>> config for signingtool.gpg that works just like the current
>> hard-coded behavior. And setting gpg.program becomes an
>> alias for signingtool.gpg.program.
>>
>> This is enough to plug in gpgsm like:
>>
>>   [signingtool "gpgsm"]
>>   program = gpgsm
>>   pemtype = "SIGNED MESSAGE"
>
> How confident are we that _all_ possible signing programs will conform
> to the "-----BEGIN %s-----" pattern? If we're not confident, then
> perhaps the user should be providing the full string here, not just
> the '%s' part?

These changes are only intended to work with PEM encoded signatures.
The new config format leaves room for working with other signature
formats in the future, though this would require further code changes.
Requiring the user to specify the whole PEM start/end markers in the
config doesn't make sense to me, since it assumes that non-PEM
encodings would have similarly simple start/end delimiters.

>
> Further, I can infer from PGP itself, as well as from reading the code
> that the 'pemtype' key can be specified multiple times; it would be
> nice to mention that in the commit message.
>
>> [...]
>> The implementation is still in gpg-interface.c, and most of
>> the code internally refers to this as "gpg". I've left it
>> this way to keep the diff sane, and to make it clear that
>> we're not breaking anything gpg-related. This is probably OK
>> for now, as our tools must be gpg-like anyway. But renaming
>> everything would be an obvious next-step refactoring if we
>> want to offer support for more exotic tools (e.g., people
>> have asked before on the list about using OpenBSD signify).
>
> I can't decide if this paragraph belongs in the commit message proper
> (as it currently is) or if it is commentary which should be placed
> below the "---" line just above the diffstat. It feels more like
> commentary, but not a big deal, and not itself worth a re-roll.
>
>> ---
>>  Documentation/config.txt |  40 +++++++++---
>>  builtin/fmt-merge-msg.c  |   6 +-
>>  builtin/receive-pack.c   |   7 +-
>>  builtin/tag.c            |   2 +-
>>  commit.c                 |   2 +-
>>  gpg-interface.c          | 165 ++++++++++++++++++++++++++++++++++++++++++-----
>>  gpg-interface.h          |  24 ++++++-
>>  log-tree.c               |   7 +-
>>  ref-filter.c             |   2 +-
>>  t/lib-gpg.sh             |  26 ++++++++
>>  t/t7510-signed-commit.sh |  32 ++++++++-
>>  tag.c                    |   2 +-
>>  12 files changed, 272 insertions(+), 43 deletions(-)
>
> Due to its length, this patch is painfully time-consuming to review,
> and asks reviewers to keep track of too many details at one time. As a
> consequence, it's likely to scare away potential reviewers and limit
> the quality of those reviews it does receive. If you break the changes
> down into a series of much smaller patches which hold the hands of
> reviewers, then you are likely to attract more and better reviews.
> Please consider doing so.
>
> Each patch should be reasonably small and easy to digest. Each patch
> should build logically upon the patch or patches preceding it, thus
> incrementally building up a picture of the complete change. A (very)
> rough idea of a series of smaller patches corresponding to this
> feature might be:
>
> 1. introduce 'struct signing_tool' and the bare machinery for managing them
>
> 2. convert PGP from a hard-coded signer to a 'signing_tool' signer
>
> 3. add support for the new configuration variables
>
> It's likely that these steps can be broken into even smaller, more
> reviewer-friendly ones; exactly how to do so may become apparent as
> you think about how the patch series should be structured. For
> instance, perhaps step 3 could be divided into:
>
> 3.1. add support for "signingtool.foo" variables
> 3.2. retrofit "gpg.program" to be alias of "signingtool.gpg.program"
>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> @@ -1727,16 +1727,38 @@ grep.fallbackToNoIndex::
>> -gpg.program::
>> -       Use this custom program instead of "`gpg`" found on `$PATH` when
>> -       making or verifying a PGP signature. The program must support the
>> -       same command-line interface as GPG, namely, to verify a detached
>> -       signature, "`gpg --verify $file - <$signature`" is run, and the
>> -       program is expected to signal a good signature by exiting with
>> -       code 0, and to generate an ASCII-armored detached signature, the
>> -       standard input of "`gpg -bsau $key`" is fed with the contents to be
>> +signingtool.<name>.program::
>> +       The name of the program on `$PATH` to execute when making or
>
> Why does the program need to be on $PATH? That seems like an
> unnecessarily harsh limitation, one which wasn't shared by
> gpg.program. (Reading the code, it looks like 'program' does not in
> fact need to be on $PATH, so this documentation is wrong.)
>

Yep. It looks like this inaccuracy was carried over from the existing docs.

>> +       verifying a signature. This program will be used for making
>> +       signatures if `<name>` is configured as `signingtool.default`.
>> +       This program will be used for verifying signatures whose PEM
>> +       block type matches `signingtool.<name>.pemtype` (see below). The
>> +       program must support the same command-line interface as GPG.
>> [...]
>> +signingtool.<name>.pemtype::
>> +       The PEM block type associated with the signing tool named
>> +       `<name>`. For example, the block type of a GPG signature
>> +       starting with `-----BEGIN PGP SIGNATURE-----` is `PGP
>> +       SIGNATURE`. When verifying a signature with this PEM block type
>> +       the program specified in `signingtool.<name>.program` will be
>> +       used. By default `signingtool.gpg.pemtype` contains `PGP
>> +       SIGNATURE` and `PGP MESSAGE`.
>
> Although it's somewhat inferred for the PGP case, the documentation
> really needs to state explicitly that multiple 'pemtype's can be
> specified, and it needs to explain how to do so. Reading the code, I
> see that one does so by specifying 'pemtype' key multiple times, but
> the documentation should say this.
>
>> diff --git a/gpg-interface.c b/gpg-interface.c
>> @@ -143,12 +216,43 @@ int git_gpg_config(const char *var, const char *value, void *cb)
>>         if (!strcmp(var, "gpg.program")) {
>> +               struct signing_tool *tool = get_or_create_signing_tool("gpg");
>
> Not at all worth a re-roll, but the get_or_create_signing_tool()
> invocation could be moved below the "if (!value)" conditional. (The
> declaration of 'tool', of course, would remain here.)
>
>>                 if (!value)
>>                         return config_error_nonbool(var);
>> -               gpg_program = xstrdup(value);
>> +
>> +               free(tool->program);
>> +               tool->program = xstrdup(value);
>>                 return 0;
>>         }
>> @@ -200,15 +306,42 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
>>  int verify_signed_buffer(const char *payload, size_t payload_size,
>>                          const char *signature, size_t signature_size,
>> -                        struct strbuf *gpg_output, struct strbuf *gpg_status)
>> +                        struct strbuf *gpg_output, struct strbuf *gpg_status,
>> +                        const struct signing_tool *tool)
>>  {
>>         [...]
>> +       if (!tool) {
>> +               parse_signature(signature, signature_size, &tool);
>> +               if (!tool) {
>> +                       /*
>> +                        * The caller didn't tell us which tool to use, and we
>> +                        * didn't recognize the format. Historically we've fed
>> +                        * these cases to blindly to gpg, so let's continue to
>
> s/to blindly/blindly/
>
>> +                        * do so.
>> +                        */
>> +                       tool = get_signing_tool("gpg");
>> +               }
>> +       }
>> diff --git a/gpg-interface.h b/gpg-interface.h
>> @@ -48,10 +60,16 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature,
>>   * Run "gpg" to see if the payload matches the detached signature.
>>   * gpg_output, when set, receives the diagnostic output from GPG.
>>   * gpg_status, when set, receives the status output from GPG.
>> + *
>> + * Typically the "tool" argument should come from a previous call to
>
> s/Typically/&,/
>
>> + * parse_signature(). If it's NULL, then verify_signed_buffer() will
>> + * try to choose the appropriate tool based on the contents of the
>> + * "signature" buffer.
>>   */
>> diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
>> @@ -56,3 +56,29 @@ sanitize_pgp() {
>> +create_fake_signer () {
>> +       write_script fake-signer <<-\EOF
>> +       if [ "$1 $2" = "--status-fd=2 -bsau" ]; then
>
> Style: This codebase uses 'test' rather than '['. Also, 'then' is
> placed on its own line and the semicolon omitted.
>
>> +               echo >&2 "[GNUPG:] BEGIN_SIGNING"
>> +               echo >&2 "[GNUPG:] SIG_CREATED D 1 SHA256 0 1513792449 4A7FF9E2330D22B19213A4E9E9C423BE17EFEE70"
>> +               # avoid "-" in echo arguments
>> +               printf "%s\n" \
>> +                 "-----BEGIN FAKE SIGNER SIGNATURE-----" \
>> +                 "fake-signature" \
>> +                 "-----END FAKE SIGNER SIGNATURE-----"
>
> If you use 'cat' with a here-doc, then you don't need the comment
> about avoiding "-".
>
>     cat <<-\END
>     -----BEGIN FAKE SIGNER SIGNATURE-----
>     fake-signature
>     -----END FAKE SIGNER SIGNATURE-----
>    END
>
>> +               exit 0
>> +
>> +       elif [ "$1 $2 $3" = "--status-fd=1 --keyid-format=long --verify" ]; then
>> +               echo "[GNUPG:] NEWSIG"
>> +               echo "[GNUPG:] GOODSIG 4A7FF9E2330D22B19213A4E9E9C423BE17EFEE70 /CN=Some User/EMail=some@user.email"
>> +               echo "[GNUPG:] TRUST_FULLY 0 shell"
>
> Another good place to use 'cat' with here-doc.
>
>> +               echo >&2 "Good signature from /CN=Some User/EMail=some@user.email"
>> +               exit 0
>> +
>> +       else
>> +               echo "bad arguments" 1>&2
>> +               exit 1
>> +       fi
>> +       EOF
>> +}
>> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
>> @@ -51,13 +55,33 @@ test_expect_success GPG 'create signed commits' '
>> +       git tag tenth-signed $(echo 9 | git commit-tree -S HEAD^{tree}) &&
>> +
>> +       git config signingtool.fake.program "$TRASH_DIRECTORY/fake-signer" &&
>> +       git config signingtool.fake.pemtype "FAKE SIGNER SIGNATURE" &&
>> +       echo 11 >file && test_tick && git commit -a -m eleventh &&
>
> It's normally frowned upon in tests to string together a bunch of
> &&-chained commands on a single line, but since this script is already
> full of this stylized 'commit' invocation, it may be okay. However...
>
>> +       git tag eleventh-pgp-signed &&
>> +       git cat-file -p eleventh-pgp-signed >actual &&
>> +       grep "PGP SIGNATURE" actual &&
>> +
>> +       git config gpg.program "$TRASH_DIRECTORY/fake-signer" &&
>> +       echo 12 >file && test_tick && git commit -a -m twelfth && test_unconfig gpg.program &&
>
> Place the test_unconfig() on its own line so it can be clearly seen by
> someone scanning an eye over the test; otherwise, the test_unconfig()
> is easily overlooked, with the result that the reader may get a false
> impression of what's going on.
>
>> +       git tag twelfth-fake-signed &&
>> +       git cat-file -p twelfth-fake-signed >actual &&
>> +       grep "FAKE SIGNER SIGNATURE" actual &&
>> +
>> +       git config signingtool.default fake &&
>> +       echo 13 >file && test_tick && git commit -a -m thirteenth && test_unconfig signingtool.default &&
>
> Ditto.
>
>> +       git tag thirteenth-fake-signed &&
>> +       git cat-file -p thirteenth-fake-signed >actual &&
>> +       grep "FAKE SIGNER SIGNATURE" actual
>>  '



-- 
-Ben Toews

Re: [PATCH 8/8] gpg-interface: handle alternative signature types

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1039 bytes --]

On Tue, Apr 10, 2018 at 04:24:27AM -0400, Eric Sunshine wrote:
> How confident are we that _all_ possible signing programs will conform
> to the "-----BEGIN %s-----" pattern? If we're not confident, then
> perhaps the user should be providing the full string here, not just
> the '%s' part?

This is not likely to be true of other signing schemes.  In fact, other
than OpenPGP, PEM, and CMS (S/MIME), this is probably not true at all.
I know OpenBSD's signify has no wrappers (except a mandatory "untrusted
comment:" line at the beginning).  There wouldn't be a way to match such
a signature unless we implemented prefix or regex support.

It's currently possible to hack other signatures in with wrappers if
they wrap the actual signature in OpenPGP-like armor; someone (I believe
Eric Wong) has gotten this to work with signify.  I only mention signify
because other than OpenPGP and CMS, it's the only scheme I've seen
people use with Git.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

Re: [PATCH 8/8] gpg-interface: handle alternative signature types

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> On Tue, Apr 10, 2018 at 04:24:27AM -0400, Eric Sunshine wrote:
>> How confident are we that _all_ possible signing programs will conform
>> to the "-----BEGIN %s-----" pattern? If we're not confident, then
>> perhaps the user should be providing the full string here, not just
>> the '%s' part?
>
> This is not likely to be true of other signing schemes.  In fact, other
> than OpenPGP, PEM, and CMS (S/MIME), this is probably not true at all.

Hmph.  

That argues more strongly that we would regret unless we make the
end-user configuration to at least the whole string (which later can
be promoted to "a pattern that matches the whole string"), not just
the part after mandatory "-----BEGIN ", methinks.

Re: [PATCH 8/8] gpg-interface: handle alternative signature types

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1737 bytes --]

On Mon, Apr 16, 2018 at 02:05:32PM +0900, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > On Tue, Apr 10, 2018 at 04:24:27AM -0400, Eric Sunshine wrote:
> >> How confident are we that _all_ possible signing programs will conform
> >> to the "-----BEGIN %s-----" pattern? If we're not confident, then
> >> perhaps the user should be providing the full string here, not just
> >> the '%s' part?
> >
> > This is not likely to be true of other signing schemes.  In fact, other
> > than OpenPGP, PEM, and CMS (S/MIME), this is probably not true at all.
> 
> Hmph.  
> 
> That argues more strongly that we would regret unless we make the
> end-user configuration to at least the whole string (which later can
> be promoted to "a pattern that matches the whole string"), not just
> the part after mandatory "-----BEGIN ", methinks.

Yeah, I think this patch set is "add gpgsm support", which I can see as
a valuable goal in and of itself, but I'm not sure the attempt to make
it generic is in the right place.  If we want to be truly generic, the
way to do that is to invoke a helper based on signature type (e.g.
git-sign-gpg, git-sign-gpgsm, git-sign-signify) to do the signing and
verification.  We need not ship these helpers ourselves; interested
third-parties can provide them, and we can add configuration to match
against regexes for non-built-in types (which is required for many other
formats).

If we just want to add gpgsm support, that's fine, but we should be
transparent about that fact and try to avoid making an interface which
is at once too generic and not generic enough.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

Re: [PATCH 8/8] gpg-interface: handle alternative signature types

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> If we just want to add gpgsm support, that's fine, but we should be
> transparent about that fact and try to avoid making an interface which
> is at once too generic and not generic enough.

One thing that makes me somewhat worried is that "add gpgsm support"
may mean "don't encourage people to use the same PGP like everybody
else does" and instead promote fragmenting the world.

But that aside, assuming that it is a good idea to support gpgsm, I
fully agree with your above assessment.

Thanks.

Re: [PATCH 8/8] gpg-interface: handle alternative signature types

[Ben Toews]

On Mon, Apr 16, 2018 at 7:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
>> If we just want to add gpgsm support, that's fine, but we should be
>> transparent about that fact and try to avoid making an interface which
>> is at once too generic and not generic enough.

This patch is definitely designed around PGP and CMS, but the config
options were intended to leave room for supporting other tools in the
future. I think allowing a PEM type to be specified makes a lot of
sense for PGP and CMS, but in the future, we can add a
`signingtool.<name>.regex` option. Similarly, the GnuPG specific
command line options and output parsing can be moved into a helper in
the future.

My motivation with this series is not just to "add gpgsm support"
though. I've been working on some other CMS tooling that will be open
source eventually. My goal was to distribute this tool with a wrapper
that emulates the GnuPG interface.

To me, this series feels like a good stepping stone towards more
generalized support for other tooling.

> One thing that makes me somewhat worried is that "add gpgsm support"
> may mean "don't encourage people to use the same PGP like everybody
> else does" and instead promote fragmenting the world.

There are a lot of projects for which PGP doesn't make sense. For
example, many large organizations and government entities don't
operate based on a web of trust, but have established PKI based on
centralized trust. For organizations like this, adopting commit/tag
signing with CMS would be far easier than adopting PGP signing.

There's a chance that 10 different software projects will end up using
10 different signing tools, but I don't see that as a problem if those
tools are well suited to the projects. Developers are already
responsible for learning how to work with the software projects they
contribute to.

Re: [PATCH 8/8] gpg-interface: handle alternative signature types

[Taylor Blau]

On Tue, Apr 17, 2018 at 12:08:20PM -0600, Ben Toews wrote:
> On Mon, Apr 16, 2018 at 7:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> >
> >> If we just want to add gpgsm support, that's fine, but we should be
> >> transparent about that fact and try to avoid making an interface which
> >> is at once too generic and not generic enough.
>
> [...]
>
> My motivation with this series is not just to "add gpgsm support"
> though. I've been working on some other CMS tooling that will be open
> source eventually. My goal was to distribute this tool with a wrapper
> that emulates the GnuPG interface.
>
> To me, this series feels like a good stepping stone towards more
> generalized support for other tooling.

I agree with Ben's assessment. A stand-in tool for gpgsm support will
not be useful without a way to configure it with Git. I think that
treating this series as ``add support for _gpgsm-like tools_'' is
sensible, and a reasonable compromise between:

  - More generalized support.
  - No support at all.

Thanks,
Taylor

Re: [PATCH 8/8] gpg-interface: handle alternative signature types

[Ben Toews]

On Tue, Apr 17, 2018 at 12:33 PM, Taylor Blau <me@ttaylorr.com> wrote:
>
> On Tue, Apr 17, 2018 at 12:08:20PM -0600, Ben Toews wrote:
> > On Mon, Apr 16, 2018 at 7:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > > "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> > >
> > >> If we just want to add gpgsm support, that's fine, but we should be
> > >> transparent about that fact and try to avoid making an interface which
> > >> is at once too generic and not generic enough.
> >
> > [...]
> >
> > My motivation with this series is not just to "add gpgsm support"
> > though. I've been working on some other CMS tooling that will be open
> > source eventually. My goal was to distribute this tool with a wrapper
> > that emulates the GnuPG interface.
> >
> > To me, this series feels like a good stepping stone towards more
> > generalized support for other tooling.
>
> I agree with Ben's assessment. A stand-in tool for gpgsm support will
> not be useful without a way to configure it with Git. I think that
> treating this series as ``add support for _gpgsm-like tools_'' is
> sensible, and a reasonable compromise between:
>
>   - More generalized support.
>   - No support at all.
>
> Thanks,
> Taylor


Any more thoughts as to whether adding support for CMS tooling is
worthwhile as a stepping stone towards supporting more general
tooling?

Re: [PATCH 8/8] gpg-interface: handle alternative signature types

[Jeff King]

On Tue, Apr 17, 2018 at 12:12:12AM +0000, brian m. carlson wrote:

> > That argues more strongly that we would regret unless we make the
> > end-user configuration to at least the whole string (which later can
> > be promoted to "a pattern that matches the whole string"), not just
> > the part after mandatory "-----BEGIN ", methinks.
> 
> Yeah, I think this patch set is "add gpgsm support", which I can see as
> a valuable goal in and of itself, but I'm not sure the attempt to make
> it generic is in the right place.  If we want to be truly generic, the
> way to do that is to invoke a helper based on signature type (e.g.
> git-sign-gpg, git-sign-gpgsm, git-sign-signify) to do the signing and
> verification.  We need not ship these helpers ourselves; interested
> third-parties can provide them, and we can add configuration to match
> against regexes for non-built-in types (which is required for many other
> formats).

Isn't that basically what this patch is, though? Or at least a step in
that direction? For generic signing support, you need:

  1. A way to tell Git to recognize that a signature exists, and what
     type it is.

  2. A way to tell Git how to invoke the signing tool.

Let me discuss (2) first.  In your git-sign-* world, then (2) requires
us to define a set interface to those helpers, including which action to
perform, which key to use, etc. And then the logic is inside the helper
to translate that to the tool's interface.

The direction I anticipated for this patch was more like:

 - for now, we just piggy-back on gpg's interface for interacting with
   sub-programs. That makes gpgsm Just Work, and it means that you can
   plug in any other tool by writing a wrapper which converts from gpg
   options to the tool's interface. I.e., gpg's "-bsau" becomes the
   lingua franca, rather than us inventing a new one.

 - the config schema leaves room for adding new properties to each tool.
   So eventually we could support other option microformats by adding
   signingtool.foo.interface = "signify" or whatever.

   That still leaves room if we want to design our own helper interface.
   One thing we could do that this patch doesn't is require the user to
   explicitly ask for "interface = gpg" (and set it by default for the
   gpg tool stanza). And then leave it as an error if you have a tool
   config that doesn't specify its interface type, which leaves room for
   us later to make that default our generic interface.

Getting back to (1), how do we tell Git to recognize a signature? I
think we have to use config here, since it would not know to invoke a
helper without recognizing the type (and we certainly do not want to
speculatively invoke each helper saying "do you understand this?" for
efficiency reasons).

So let's assume we have some config to do matching. What should that
look like? Is it a substring match? A line match? A regex? There's a
continuum of matching techniques that trade off simplicity for
flexibility. My thought was that we'd eventually need to add multiple
matching methods, and users can pick the one that's simplest for the
format they're using.

So here we add pem-type matching, which errs very much on the side of
"very specific and easy, but not very flexible". We can go further down
the continuum to "match a line" and require the user say the full:

  [signingtool "gpg"]
  matchLine = "----- BEGIN PGP SIGNATURE -----"

(obviously they don't need to reconfigure gpg, but imagine they have a
new similar tool). That's not too bad. But does it extend things enough
to match other types? It sounds like signify doesn't use a line-oriented
armoring, and even matching a whole line wouldn't be enough. So now
we've erred on the other side; we made something more generic, but it's
not clear if it's actually generic enough to be useful.

So I'm open to the idea of line-matching, since the config above isn't
significantly more complicated than the current pemType matcher. It does
mean we can't later do pem-specific things, like matching the "END"
delimiter of the block (but so far we haven't needed to do that, since
the detached signature is always supposed to come at the end of the
content).

But I'm not sure we're ready to go any further in making it generic,
since we don't know what the requirements will be (and won't until
somebody starts trying to plug in a new tool).

-Peff

Re: [PATCH 8/8] gpg-interface: handle alternative signature types

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Apr 17, 2018 at 12:12:12AM +0000, brian m. carlson wrote:
>
>> > That argues more strongly that we would regret unless we make the
>> > end-user configuration to at least the whole string (which later can
>> > be promoted to "a pattern that matches the whole string"), not just
>> > the part after mandatory "-----BEGIN ", methinks.
>> 
>> Yeah, I think this patch set is "add gpgsm support", which I can see as
>> a valuable goal in and of itself, but I'm not sure the attempt to make
>> it generic is in the right place.  If we want to be truly generic, the
>> way to do that is to invoke a helper based on signature type (e.g.
>> git-sign-gpg, git-sign-gpgsm, git-sign-signify) to do the signing and
>> verification.  We need not ship these helpers ourselves; interested
>> third-parties can provide them, and we can add configuration to match
>> against regexes for non-built-in types (which is required for many other
>> formats).
>
> Isn't that basically what this patch is, though? Or at least a step in
> that direction?

I think that is what Brian is saying, too (and if so I would also
agree).  It probably is a good step.  It is just the feature may be
sold under a wrong (or, overly wide) label, perhaps?

Re: [PATCH 8/8] gpg-interface: handle alternative signature types

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 3740 bytes --]

On Mon, May 07, 2018 at 05:45:00AM -0400, Jeff King wrote:
> Isn't that basically what this patch is, though? Or at least a step in
> that direction? For generic signing support, you need:
> 
>   1. A way to tell Git to recognize that a signature exists, and what
>      type it is.
> 
>   2. A way to tell Git how to invoke the signing tool.
> 
> Let me discuss (2) first.  In your git-sign-* world, then (2) requires
> us to define a set interface to those helpers, including which action to
> perform, which key to use, etc. And then the logic is inside the helper
> to translate that to the tool's interface.
> 
> The direction I anticipated for this patch was more like:
> 
>  - for now, we just piggy-back on gpg's interface for interacting with
>    sub-programs. That makes gpgsm Just Work, and it means that you can
>    plug in any other tool by writing a wrapper which converts from gpg
>    options to the tool's interface. I.e., gpg's "-bsau" becomes the
>    lingua franca, rather than us inventing a new one.
> 
>  - the config schema leaves room for adding new properties to each tool.
>    So eventually we could support other option microformats by adding
>    signingtool.foo.interface = "signify" or whatever.
> 
>    That still leaves room if we want to design our own helper interface.
>    One thing we could do that this patch doesn't is require the user to
>    explicitly ask for "interface = gpg" (and set it by default for the
>    gpg tool stanza). And then leave it as an error if you have a tool
>    config that doesn't specify its interface type, which leaves room for
>    us later to make that default our generic interface.
> 
> Getting back to (1), how do we tell Git to recognize a signature? I
> think we have to use config here, since it would not know to invoke a
> helper without recognizing the type (and we certainly do not want to
> speculatively invoke each helper saying "do you understand this?" for
> efficiency reasons).

I think my main objection to this series is that it is generic in a way
that isn't necessarily useful.  We know there are essentially only two
formats of PEM-style signatures: OpenPGP and CMS[0].  Even if there are
more, they aren't intrinsically useful, because our codebase can only
handle GnuPG-style tools, and those are the only formats GnuPG-style
tools really support (although, as you point out, other tools could
mimic the interface).

I think if we aren't going to implement some sort of interface that's
generically useful for all signing tools, it would be better to simply
say that we support gpg and gpgsm and have signingtool.gpg.program and
signingtool.gpgsm.program and hard-code the logic for those two formats.
That way we don't have a generic interface that's really only useful for
PEM-style tools, when we know it likely won't be useful for other tools
as well.  We can add a more generic interface when we have more varied
tools to support and we know more about what the requirements will be.

This doesn't address Junio's concern about whether adding CMS support is
the right direction to go.  I personally think OpenPGP is the right
direction for most open-source projects, but I know some companies want
to use CMS internally and I'm not intrinsically opposed to that[1].
That decision is ultimately up to Junio, though.

[0] I'm ignoring the original PEM, which specifies MD2 and MD5,
algorithms that nobody should be using these days.
[1] I would welcome, though, if one could configure only one type of
signature verification by, say, setting the signing program to
/bin/false in the config.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

Re: [PATCH 8/8] gpg-interface: handle alternative signature types

[Jeff King]

On Mon, May 07, 2018 at 11:06:50PM +0000, brian m. carlson wrote:

> I think my main objection to this series is that it is generic in a way
> that isn't necessarily useful.  We know there are essentially only two
> formats of PEM-style signatures: OpenPGP and CMS[0].  Even if there are
> more, they aren't intrinsically useful, because our codebase can only
> handle GnuPG-style tools, and those are the only formats GnuPG-style
> tools really support (although, as you point out, other tools could
> mimic the interface).
> 
> I think if we aren't going to implement some sort of interface that's
> generically useful for all signing tools, it would be better to simply
> say that we support gpg and gpgsm and have signingtool.gpg.program and
> signingtool.gpgsm.program and hard-code the logic for those two formats.
> That way we don't have a generic interface that's really only useful for
> PEM-style tools, when we know it likely won't be useful for other tools
> as well.  We can add a more generic interface when we have more varied
> tools to support and we know more about what the requirements will be.

OK, so my question then is: what does just-gpgsm support look like?

Do we literally add gpgsm.program? My thought was that taking us the
first step towards a more generic config scheme would prevent us having
to backtrack later.

There are also more CMS signers than gpgsm (and I know Ben is working on
a tool). So it feels a little ugly to make it "gpgsm.program", since it
really is a more generic format.

Or would you be happy if we just turned the matcher into a whole-line
substring or regex match?

> This doesn't address Junio's concern about whether adding CMS support is
> the right direction to go.  I personally think OpenPGP is the right
> direction for most open-source projects, but I know some companies want
> to use CMS internally and I'm not intrinsically opposed to that[1].
> That decision is ultimately up to Junio, though.

My guess is that fragmentation isn't likely to be much of a problem in
practice, because the tool choice generally falls along
culture/community boundaries. I'd expect that open source projects are
never going to choose CMS, because the centralized cert management is
awful. But it's exactly what many closed-source enterprises want, and
they will literally choose "no signing" over wrestling with PGP.

I'd be much more worried about the open source world splitting into
"signify" and "gpg" camps or similar. OTOH, I just don't see it as all
that big a deal. It's a project decision, and it may even allow for some
healthy competition between standards.

-Peff

Re: [PATCH 8/8] gpg-interface: handle alternative signature types

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1384 bytes --]

On Tue, May 08, 2018 at 09:28:14AM -0400, Jeff King wrote:
> OK, so my question then is: what does just-gpgsm support look like?
> 
> Do we literally add gpgsm.program? My thought was that taking us the
> first step towards a more generic config scheme would prevent us having
> to backtrack later.

I think the signingtool prefix is fine, or something similar.  My "just
gpgsm" proposal is literally just "check for PGP header" and "check for
CMS header" in parse_signature and dispatch appropriately.

> There are also more CMS signers than gpgsm (and I know Ben is working on
> a tool). So it feels a little ugly to make it "gpgsm.program", since it
> really is a more generic format.

Okay, so signingtool.cms.program?  signingtool.smime.program?

I suppose Ben still intends to use the same command-line interface as
for gpgsm.

> Or would you be happy if we just turned the matcher into a whole-line
> substring or regex match?

A first line regex would probably be fine, if you want to go that way.
That, I think, is generic enough that we can make use of it down the
line, since it distinguishes all known formats, TTBOMK.

It would be nice if we could still continue to use gpg without having to
add specific configuration for it, at least for compatibility reasons.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

Re: [PATCH 8/8] gpg-interface: handle alternative signature types

[Jeff King]

On Tue, May 08, 2018 at 11:09:22PM +0000, brian m. carlson wrote:

> On Tue, May 08, 2018 at 09:28:14AM -0400, Jeff King wrote:
> > OK, so my question then is: what does just-gpgsm support look like?
> > 
> > Do we literally add gpgsm.program? My thought was that taking us the
> > first step towards a more generic config scheme would prevent us having
> > to backtrack later.
> 
> I think the signingtool prefix is fine, or something similar.  My "just
> gpgsm" proposal is literally just "check for PGP header" and "check for
> CMS header" in parse_signature and dispatch appropriately.

Hmm. I suppose that would work. I just didn't want to go the route of
adding more hard-coded magic that the user couldn't override (or
anything that is more complex than what the user could specify for their
own tool if they wanted to).  But I suppose there's probably not a big
need to override the GPG or CMS matching in practice.

> > There are also more CMS signers than gpgsm (and I know Ben is working on
> > a tool). So it feels a little ugly to make it "gpgsm.program", since it
> > really is a more generic format.
> 
> Okay, so signingtool.cms.program?  signingtool.smime.program?
> 
> I suppose Ben still intends to use the same command-line interface as
> for gpgsm.

AFAIK, yes.

> > Or would you be happy if we just turned the matcher into a whole-line
> > substring or regex match?
> 
> A first line regex would probably be fine, if you want to go that way.
> That, I think, is generic enough that we can make use of it down the
> line, since it distinguishes all known formats, TTBOMK.

That seems like a happy medium to me. I worried at first that a regex
might be noticeably expensive, but probably not. During "git log" we
only feed "gpgsig" headers from each commit to the signature parsing
code (so it's few lines, and no cost when you aren't using signatures).
For tags we have to scan the whole tag body, but per-tag performance is
much less important there, because you're not typically traversing
hundreds of thousands of them.

> It would be nice if we could still continue to use gpg without having to
> add specific configuration for it, at least for compatibility reasons.

Definitely. Maintaining compatibility with the existing out-of-the-box
behavior and with the existing config options is non-negotiable (and is
already the case with the existing patch under discussion).

-Peff

Re: [PATCH 8/8] gpg-interface: handle alternative signature types

[Junio C Hamano]

Ben Toews <mastahyeti@gmail.com> writes:

> From: Ben Toews <btoews@github.com>
>
> Currently you can only sign commits and tags using "gpg".
> ...
> have asked before on the list about using OpenBSD signify).
> ---

Missing sign-off.

> -gpg.program::
> -	Use this custom program instead of "`gpg`" found on `$PATH` when
> -	making or verifying a PGP signature. The program must support the
> -	same command-line interface as GPG, namely, to verify a detached
> -	signature, "`gpg --verify $file - <$signature`" is run, and the
> -	program is expected to signal a good signature by exiting with
> -	code 0, and to generate an ASCII-armored detached signature, the
> -	standard input of "`gpg -bsau $key`" is fed with the contents to be
> +signingtool.<name>.program::
> +	The name of the program on `$PATH` to execute when making or
> +	verifying a signature.

I think you do not want "on `$PATH`", as you should be able to
specify a full path /opt/some/where/not/on/my/path/pgp and have it
work just fine.  The mention of "found on `$PATH`" in the original
is talking about the behaviour _WITHOUT_ the configuration, i.e. by
default we just invoke "gpg" and expect that it is found in the
usual measure, i.e. being on user's $PATH.  What you are describing
in this updated explanation is what happens _WITH_ the configuration.

> +	This program will be used for making
> +	signatures if `<name>` is configured as `signingtool.default`.
> +	This program will be used for verifying signatures whose PEM
> +	block type matches `signingtool.<name>.pemtype` (see below). The
> +	program must support the same command-line interface as GPG.
> +	To verify a detached signature,
> +	"`gpg --verify $file - <$signature`" is run, and the program is
> +	expected to signal a good signature by exiting with code 0.
> +	To generate an ASCII-armored detached signature, the standard
> +	input of "`gpg -bsau $key`" is fed with the contents to be
>  	signed, and the program is expected to send the result to its
> -	standard output.
> +	standard output. By default, `signingtool.gpg.program` is set to
> +	`gpg`.

I do not think the description is wrong per-se, but reading it made
me realize that with this "custom" program, you still require that
the "custom" program MUST accept the command line options as if it
were an implementation of GPG.  Most likely you'd write a thin
wrapper to call your custom program with whatever options that are
appropriate when asked to --verify or -bsau (aka "sign")?  If that
is the case, I have to wonder if such a wrappper program can also
trivially reformat the --- BEGIN WHATEVER --- block and behave as if
it were an implementation of GPG.  That makes the primary point of
this long series somewhat moot, as we won't need that pemtype thing
at all, no?

> +signingtool.<name>.pemtype::
> +	The PEM block type associated with the signing tool named
> +	`<name>`. For example, the block type of a GPG signature
> +	starting with `-----BEGIN PGP SIGNATURE-----` is `PGP
> +	SIGNATURE`. When verifying a signature with this PEM block type
> +	the program specified in `signingtool.<name>.program` will be
> +	used. By default `signingtool.gpg.pemtype` contains `PGP
> +	SIGNATURE` and `PGP MESSAGE`.

As Eric noted elsewhere, I suspect that it is cleaner and more
useful if this were *NOT* "pemtype" but were "boundary", i.e.
letting "-----BEGIN PGP SIGNATURE-----\n" string be specified.

> +signingtool.default::
> +	The `<name>` of the signing tool to use when creating
> +	signatures (e.g., setting it to "foo" will use use the program
> +	specified by `signingtool.foo.program`). Defaults to `gpg`.

Will there be a command line option to say "I may usually be using
whatever I configured with signingtool.default, but for this single
invocation only, let me use something else"?  Without such a command
line option that overrides such a default, I do not quite get the
point of adding this configuration variable.

Thanks.

Re: [PATCH 8/8] gpg-interface: handle alternative signature types

[Ben Toews]

On Tue, Apr 10, 2018 at 3:35 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ben Toews <mastahyeti@gmail.com> writes:
>
>> From: Ben Toews <btoews@github.com>
>>
>> Currently you can only sign commits and tags using "gpg".
>> ...
>> have asked before on the list about using OpenBSD signify).
>> ---
>
> Missing sign-off.
>
>> -gpg.program::
>> -     Use this custom program instead of "`gpg`" found on `$PATH` when
>> -     making or verifying a PGP signature. The program must support the
>> -     same command-line interface as GPG, namely, to verify a detached
>> -     signature, "`gpg --verify $file - <$signature`" is run, and the
>> -     program is expected to signal a good signature by exiting with
>> -     code 0, and to generate an ASCII-armored detached signature, the
>> -     standard input of "`gpg -bsau $key`" is fed with the contents to be
>> +signingtool.<name>.program::
>> +     The name of the program on `$PATH` to execute when making or
>> +     verifying a signature.
>
> I think you do not want "on `$PATH`", as you should be able to
> specify a full path /opt/some/where/not/on/my/path/pgp and have it
> work just fine.  The mention of "found on `$PATH`" in the original
> is talking about the behaviour _WITHOUT_ the configuration, i.e. by
> default we just invoke "gpg" and expect that it is found in the
> usual measure, i.e. being on user's $PATH.  What you are describing
> in this updated explanation is what happens _WITH_ the configuration.
>
>> +     This program will be used for making
>> +     signatures if `<name>` is configured as `signingtool.default`.
>> +     This program will be used for verifying signatures whose PEM
>> +     block type matches `signingtool.<name>.pemtype` (see below). The
>> +     program must support the same command-line interface as GPG.
>> +     To verify a detached signature,
>> +     "`gpg --verify $file - <$signature`" is run, and the program is
>> +     expected to signal a good signature by exiting with code 0.
>> +     To generate an ASCII-armored detached signature, the standard
>> +     input of "`gpg -bsau $key`" is fed with the contents to be
>>       signed, and the program is expected to send the result to its
>> -     standard output.
>> +     standard output. By default, `signingtool.gpg.program` is set to
>> +     `gpg`.
>
> I do not think the description is wrong per-se, but reading it made
> me realize that with this "custom" program, you still require that
> the "custom" program MUST accept the command line options as if it
> were an implementation of GPG.  Most likely you'd write a thin
> wrapper to call your custom program with whatever options that are
> appropriate when asked to --verify or -bsau (aka "sign")?  If that
> is the case, I have to wonder if such a wrappper program can also
> trivially reformat the --- BEGIN WHATEVER --- block and behave as if
> it were an implementation of GPG.  That makes the primary point of
> this long series somewhat moot, as we won't need that pemtype thing
> at all, no?
>

Just because a signature is PEM encoded and claims to be a "PGP
SIGNATURE", doesn't mean it can be understood or verified by a PGP
implementation. Without different tools specifying different PEM types
we would have no way of knowing which tool to route the signature to
for verification.

>> +signingtool.<name>.pemtype::
>> +     The PEM block type associated with the signing tool named
>> +     `<name>`. For example, the block type of a GPG signature
>> +     starting with `-----BEGIN PGP SIGNATURE-----` is `PGP
>> +     SIGNATURE`. When verifying a signature with this PEM block type
>> +     the program specified in `signingtool.<name>.program` will be
>> +     used. By default `signingtool.gpg.pemtype` contains `PGP
>> +     SIGNATURE` and `PGP MESSAGE`.
>
> As Eric noted elsewhere, I suspect that it is cleaner and more
> useful if this were *NOT* "pemtype" but were "boundary", i.e.
> letting "-----BEGIN PGP SIGNATURE-----\n" string be specified.
>
>> +signingtool.default::
>> +     The `<name>` of the signing tool to use when creating
>> +     signatures (e.g., setting it to "foo" will use use the program
>> +     specified by `signingtool.foo.program`). Defaults to `gpg`.
>
> Will there be a command line option to say "I may usually be using
> whatever I configured with signingtool.default, but for this single
> invocation only, let me use something else"?  Without such a command
> line option that overrides such a default, I do not quite get the
> point of adding this configuration variable.
>
> Thanks.



-- 
-Ben Toews

Re: [PATCH 8/8] gpg-interface: handle alternative signature types

[SZEDER Gábor]

> diff --git a/gpg-interface.h b/gpg-interface.h
> index a5e6517ae6..cee0dfe401 100644
> --- a/gpg-interface.h
> +++ b/gpg-interface.h
> @@ -23,15 +23,27 @@ struct signature_check {
>  	char *key;
>  };
>  
> +struct signing_tool {
> +	char *name;
> +	char *program;
> +	struct string_list pemtype;
> +	struct signing_tool *next;
> +};
> +
>  void signature_check_clear(struct signature_check *sigc);
>  
>  /*
> - * Look at GPG signed content (e.g. a signed tag object), whose
> + * Look for signed content (e.g. a signed tag object), whose
>   * payload is followed by a detached signature on it.  Return the
>   * offset where the embedded detached signature begins, or the end of
>   * the data when there is no such signature.
> + *
> + * If out_tool is non-NULL and a signature is found, it will be
> + * pointed at the signing_tool that corresponds to the found
> + * signature type.
>   */
> -size_t parse_signature(const char *buf, size_t size);
> +size_t parse_signature(const char *buf, unsigned long size,
> +		       const struct signing_tool **out_tool);

This hunk changes the type of the 'size' argument from size_t to
unsigned long, but leaves the function's signature in
'gpg-interface.c' unchanged.  This breaks the build on 32 bit systems
(and Windows?), where unsigned long is only 32 bits, and I presume is
unintended, as it goes against the earlier patch 3/8 "gpg-interface:
use size_t for signature buffer size".

[PATCH v2 0/9] gpg-interface: Multiple signing tools

[Ben Toews]

Updated to incorporate feedback from v1. In addition to changes to the patches
from v1, I added the missing `t7004: fix mistaken tag name` patch, which had
caused some confusion (sorry about that). Thanks for everyone's feedback on v1.

### Interdiff (v1..v2):

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 7906123a59..691b309306 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1728,7 +1728,7 @@ grep.fallbackToNoIndex::
 	is executed outside of a git repository.  Defaults to false.

 signingtool.<name>.program::
-	The name of the program on `$PATH` to execute when making or
+	The name or path of the program to execute when making or
 	verifying a signature. This program will be used for making
 	signatures if `<name>` is configured as `signingtool.default`.
 	This program will be used for verifying signatures whose PEM
@@ -1750,7 +1750,9 @@ signingtool.<name>.pemtype::
 	SIGNATURE`. When verifying a signature with this PEM block type
 	the program specified in `signingtool.<name>.program` will be
 	used. By default `signingtool.gpg.pemtype` contains `PGP
-	SIGNATURE` and `PGP MESSAGE`.
+	SIGNATURE` and `PGP MESSAGE`. Multiple PEM types may be specified
+	for a single signing tool by including the `pemtype` directive
+	multiple times within the `signingtool` configuration.

 signingtool.default::
 	The `<name>` of the signing tool to use when creating
diff --git a/gpg-interface.c b/gpg-interface.c
index 0e2a82e8e5..5d4ae2a7ed 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -22,7 +22,8 @@ static struct signing_tool *alloc_signing_tool(void)
  * Our default tool config is too complicated to specify as a constant
  * initializer, so we lazily create it as needed.
  */
-static void init_signing_tool_defaults(void) {
+static void init_signing_tool_defaults(void)
+{
 	struct signing_tool *tool;

 	if (signing_tool_config)
@@ -38,7 +39,8 @@ static void init_signing_tool_defaults(void) {
 	signing_tool_config = tool;
 }

-static struct signing_tool *get_signing_tool(const char *name) {
+static struct signing_tool *get_signing_tool(const char *name)
+{
 	struct signing_tool *tool;

 	init_signing_tool_defaults();
@@ -216,11 +218,12 @@ int git_gpg_config(const char *var, const char *value, void *cb)
 	}

 	if (!strcmp(var, "gpg.program")) {
-		struct signing_tool *tool = get_or_create_signing_tool("gpg");
+		struct signing_tool *tool;

 		if (!value)
 			return config_error_nonbool(var);

+		tool = get_or_create_signing_tool("gpg");
 		free(tool->program);
 		tool->program = xstrdup(value);
 		return 0;
@@ -331,7 +334,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 			/*
 			 * The caller didn't tell us which tool to use, and we
 			 * didn't recognize the format. Historically we've fed
-			 * these cases to blindly to gpg, so let's continue to
+			 * these cases blindly to gpg, so let's continue to
 			 * do so.
 			 */
 			tool = get_signing_tool("gpg");
diff --git a/gpg-interface.h b/gpg-interface.h
index cee0dfe401..8e22e67b6f 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -42,7 +42,7 @@ void signature_check_clear(struct signature_check *sigc);
  * pointed at the signing_tool that corresponds to the found
  * signature type.
  */
-size_t parse_signature(const char *buf, unsigned long size,
+size_t parse_signature(const char *buf, size_t size,
 		       const struct signing_tool **out_tool);

 void parse_gpg_output(struct signature_check *);
@@ -61,7 +61,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature,
  * gpg_output, when set, receives the diagnostic output from GPG.
  * gpg_status, when set, receives the status output from GPG.
  *
- * Typically the "tool" argument should come from a previous call to
+ * Typically, the "tool" argument should come from a previous call to
  * parse_signature(). If it's NULL, then verify_signed_buffer() will
  * try to choose the appropriate tool based on the contents of the
  * "signature" buffer.
diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index 08d23b0cf5..b9ba47057f 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -59,20 +59,24 @@ sanitize_pgp() {

 create_fake_signer () {
 	write_script fake-signer <<-\EOF
-	if [ "$1 $2" = "--status-fd=2 -bsau" ]; then
+	if test "$1 $2" = "--status-fd=2 -bsau"
+	then
 		echo >&2 "[GNUPG:] BEGIN_SIGNING"
 		echo >&2 "[GNUPG:] SIG_CREATED D 1 SHA256 0 1513792449 4A7FF9E2330D22B19213A4E9E9C423BE17EFEE70"
-		# avoid "-" in echo arguments
-		printf "%s\n" \
-		  "-----BEGIN FAKE SIGNER SIGNATURE-----" \
-		  "fake-signature" \
-		  "-----END FAKE SIGNER SIGNATURE-----"
+		cat <<-\END
+		-----BEGIN FAKE SIGNER SIGNATURE-----
+		fake-signature
+		-----END FAKE SIGNER SIGNATURE-----
+		END
 		exit 0

-	elif [ "$1 $2 $3" = "--status-fd=1 --keyid-format=long --verify" ]; then
-		echo "[GNUPG:] NEWSIG"
-		echo "[GNUPG:] GOODSIG 4A7FF9E2330D22B19213A4E9E9C423BE17EFEE70 /CN=Some User/EMail=some@user.email"
-		echo "[GNUPG:] TRUST_FULLY 0 shell"
+	elif test "$1 $2 $3" = "--status-fd=1 --keyid-format=long --verify"
+	then
+		cat <<-\END
+		[GNUPG:] NEWSIG
+		[GNUPG:] GOODSIG 4A7FF9E2330D22B19213A4E9E9C423BE17EFEE70 /CN=Some User/EMail=some@user.email
+		[GNUPG:] TRUST_FULLY 0 shell
+		END
 		echo >&2 "Good signature from /CN=Some User/EMail=some@user.email"
 		exit 0

diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 848a823302..fb41f98ca6 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -65,13 +65,15 @@ test_expect_success GPG 'create signed commits' '
 	grep "PGP SIGNATURE" actual &&

 	git config gpg.program "$TRASH_DIRECTORY/fake-signer" &&
-	echo 12 >file && test_tick && git commit -a -m twelfth && test_unconfig gpg.program &&
+	echo 12 >file && test_tick && git commit -a -m twelfth &&
+	test_unconfig gpg.program &&
 	git tag twelfth-fake-signed &&
 	git cat-file -p twelfth-fake-signed >actual &&
 	grep "FAKE SIGNER SIGNATURE" actual &&

 	git config signingtool.default fake &&
-	echo 13 >file && test_tick && git commit -a -m thirteenth && test_unconfig signingtool.default &&
+	echo 13 >file && test_tick && git commit -a -m thirteenth &&
+	test_unconfig signingtool.default &&
 	git tag thirteenth-fake-signed &&
 	git cat-file -p thirteenth-fake-signed >actual &&
 	grep "FAKE SIGNER SIGNATURE" actual

### Patches

Ben Toews (1):
  gpg-interface: handle alternative signature types

Jeff King (8):
  t7004: fix mistaken tag name
  gpg-interface: handle bool user.signingkey
  gpg-interface: modernize function declarations
  gpg-interface: use size_t for signature buffer size
  gpg-interface: fix const-correctness of "eol" pointer
  gpg-interface: extract gpg line matching helper
  gpg-interface: find the last gpg signature line
  gpg-interface: prepare for parsing arbitrary PEM blocks

 Documentation/config.txt |  42 +++++++---
 builtin/fmt-merge-msg.c  |   6 +-
 builtin/receive-pack.c   |   7 +-
 builtin/tag.c            |   2 +-
 commit.c                 |   2 +-
 gpg-interface.c          | 201 +++++++++++++++++++++++++++++++++++++++--------
 gpg-interface.h          |  67 +++++++++++++---
 log-tree.c               |   7 +-
 ref-filter.c             |   2 +-
 t/lib-gpg.sh             |  30 +++++++
 t/t7004-tag.sh           |  13 ++-
 t/t7510-signed-commit.sh |  34 +++++++-
 tag.c                    |   2 +-
 13 files changed, 348 insertions(+), 67 deletions(-)

--
2.15.1 (Apple Git-101)

[PATCH v2 1/9] t7004: fix mistaken tag name

[Ben Toews]

From: Jeff King <peff@peff.net>

We have a series of tests which create signed tags with
various properties, but one test accidentally verifies a tag
from much earlier in the series.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ben Toews <mastahyeti@gmail.com>
---
 t/t7004-tag.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 2aac77af70..ee093b393d 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1056,7 +1056,7 @@ test_expect_success GPG \
 	git tag -s -F sigblanknonlfile blanknonlfile-signed-tag &&
 	get_tag_msg blanknonlfile-signed-tag >actual &&
 	test_cmp expect actual &&
-	git tag -v signed-tag
+	git tag -v blanknonlfile-signed-tag
 '

 # messages with commented lines for signed tags:
--
2.15.1 (Apple Git-101)

[PATCH v2 2/9] gpg-interface: handle bool user.signingkey

[Ben Toews]

From: Jeff King <peff@peff.net>

The config handler for user.signingkey does not check for a
boolean value, and thus:

  git -c user.signingkey tag

will segfault. We could fix this and even shorten the code
by using git_config_string(). But our set_signing_key()
helper is used by other code outside of gpg-interface.c, so
we must keep it (and we may as well use it, because unlike
git_config_string() it does not leak when we overwrite an
old value).

Ironically, the handler for gpg.program just below _could_
use git_config_string() but doesn't. But since we're going
to touch that in a future patch, we'll leave it alone for
now. We will add some whitespace and returns in preparation
for adding more config keys, though.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ben Toews <mastahyeti@gmail.com>
---
 gpg-interface.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gpg-interface.c b/gpg-interface.c
index 4feacf16e5..61c0690e12 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -128,13 +128,19 @@ void set_signing_key(const char *key)
 int git_gpg_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "user.signingkey")) {
+		if (!value)
+			return config_error_nonbool(var);
 		set_signing_key(value);
+		return 0;
 	}
+
 	if (!strcmp(var, "gpg.program")) {
 		if (!value)
 			return config_error_nonbool(var);
 		gpg_program = xstrdup(value);
+		return 0;
 	}
+
 	return 0;
 }

--
2.15.1 (Apple Git-101)

[PATCH v2 3/9] gpg-interface: modernize function declarations

[Ben Toews]

From: Jeff King <peff@peff.net>

Let's drop "extern" from our declarations, which brings us
in line with our modern style guidelines. While we're
here, let's wrap some of the overly long lines, and move
docstrings for public functions to their declarations, since
they document the interface.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ben Toews <mastahyeti@gmail.com>
---
 gpg-interface.c | 17 -----------------
 gpg-interface.h | 49 ++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 38 insertions(+), 28 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 61c0690e12..08de0daa41 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -101,12 +101,6 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
 		fputs(output, stderr);
 }

-/*
- * Look at GPG signed content (e.g. a signed tag object), whose
- * payload is followed by a detached signature on it.  Return the
- * offset where the embedded detached signature begins, or the end of
- * the data when there is no such signature.
- */
 size_t parse_signature(const char *buf, unsigned long size)
 {
 	char *eol;
@@ -151,12 +145,6 @@ const char *get_signing_key(void)
 	return git_committer_info(IDENT_STRICT|IDENT_NO_DATE);
 }

-/*
- * Create a detached signature for the contents of "buffer" and append
- * it after "signature"; "buffer" and "signature" can be the same
- * strbuf instance, which would cause the detached signature appended
- * at the end.
- */
 int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
 {
 	struct child_process gpg = CHILD_PROCESS_INIT;
@@ -198,11 +186,6 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
 	return 0;
 }

-/*
- * Run "gpg" to see if the payload matches the detached signature.
- * gpg_output, when set, receives the diagnostic output from GPG.
- * gpg_status, when set, receives the status output from GPG.
- */
 int verify_signed_buffer(const char *payload, size_t payload_size,
 			 const char *signature, size_t signature_size,
 			 struct strbuf *gpg_output, struct strbuf *gpg_status)
diff --git a/gpg-interface.h b/gpg-interface.h
index d2d4fd3a65..2c40a9175f 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -23,16 +23,43 @@ struct signature_check {
 	char *key;
 };

-extern void signature_check_clear(struct signature_check *sigc);
-extern size_t parse_signature(const char *buf, unsigned long size);
-extern void parse_gpg_output(struct signature_check *);
-extern int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key);
-extern int verify_signed_buffer(const char *payload, size_t payload_size, const char *signature, size_t signature_size, struct strbuf *gpg_output, struct strbuf *gpg_status);
-extern int git_gpg_config(const char *, const char *, void *);
-extern void set_signing_key(const char *);
-extern const char *get_signing_key(void);
-extern int check_signature(const char *payload, size_t plen,
-	const char *signature, size_t slen, struct signature_check *sigc);
-void print_signature_buffer(const struct signature_check *sigc, unsigned flags);
+void signature_check_clear(struct signature_check *sigc);
+
+/*
+ * Look at GPG signed content (e.g. a signed tag object), whose
+ * payload is followed by a detached signature on it.  Return the
+ * offset where the embedded detached signature begins, or the end of
+ * the data when there is no such signature.
+ */
+size_t parse_signature(const char *buf, unsigned long size);
+
+void parse_gpg_output(struct signature_check *);
+
+/*
+ * Create a detached signature for the contents of "buffer" and append
+ * it after "signature"; "buffer" and "signature" can be the same
+ * strbuf instance, which would cause the detached signature appended
+ * at the end.
+ */
+int sign_buffer(struct strbuf *buffer, struct strbuf *signature,
+		const char *signing_key);
+
+/*
+ * Run "gpg" to see if the payload matches the detached signature.
+ * gpg_output, when set, receives the diagnostic output from GPG.
+ * gpg_status, when set, receives the status output from GPG.
+ */
+int verify_signed_buffer(const char *payload, size_t payload_size,
+			 const char *signature, size_t signature_size,
+			 struct strbuf *gpg_output, struct strbuf *gpg_status);
+
+int git_gpg_config(const char *, const char *, void *);
+void set_signing_key(const char *);
+const char *get_signing_key(void);
+int check_signature(const char *payload, size_t plen,
+		    const char *signature, size_t slen,
+		    struct signature_check *sigc);
+void print_signature_buffer(const struct signature_check *sigc,
+			    unsigned flags);

 #endif
--
2.15.1 (Apple Git-101)

[PATCH v2 4/9] gpg-interface: use size_t for signature buffer size

[Ben Toews]

From: Jeff King <peff@peff.net>

Even though our object sizes (from which these buffers would
come) are typically "unsigned long", this is something we'd
like to eventually fix (since it's only 32-bits even on
64-bit Windows). It makes more sense to use size_t when
taking an in-memory buffer.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ben Toews <mastahyeti@gmail.com>
---
 gpg-interface.c | 2 +-
 gpg-interface.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 08de0daa41..ac852ad4b9 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -101,7 +101,7 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
 		fputs(output, stderr);
 }

-size_t parse_signature(const char *buf, unsigned long size)
+size_t parse_signature(const char *buf, size_t size)
 {
 	char *eol;
 	size_t len = 0;
diff --git a/gpg-interface.h b/gpg-interface.h
index 2c40a9175f..a5e6517ae6 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -31,7 +31,7 @@ void signature_check_clear(struct signature_check *sigc);
  * offset where the embedded detached signature begins, or the end of
  * the data when there is no such signature.
  */
-size_t parse_signature(const char *buf, unsigned long size);
+size_t parse_signature(const char *buf, size_t size);

 void parse_gpg_output(struct signature_check *);

--
2.15.1 (Apple Git-101)

[PATCH v2 5/9] gpg-interface: fix const-correctness of "eol" pointer

[Ben Toews]

From: Jeff King <peff@peff.net>

We accidentally shed the "const" of our buffer by passing it
through memchr. Let's fix that, and while we're at it, move
our variable declaration inside the loop, which is the only
place that uses it.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ben Toews <mastahyeti@gmail.com>
---
 gpg-interface.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index ac852ad4b9..3414af38b9 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -103,11 +103,10 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)

 size_t parse_signature(const char *buf, size_t size)
 {
-	char *eol;
 	size_t len = 0;
 	while (len < size && !starts_with(buf + len, PGP_SIGNATURE) &&
 			!starts_with(buf + len, PGP_MESSAGE)) {
-		eol = memchr(buf + len, '\n', size - len);
+		const char *eol = memchr(buf + len, '\n', size - len);
 		len += eol ? eol - (buf + len) + 1 : size - len;
 	}
 	return len;
--
2.15.1 (Apple Git-101)

[PATCH v2 6/9] gpg-interface: extract gpg line matching helper

[Ben Toews]

From: Jeff King <peff@peff.net>

Let's separate the actual line-by-line parsing of signatures
from the notion of "is this a gpg signature line". That will
make it easier to do more refactoring of this loop in future
patches.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ben Toews <mastahyeti@gmail.com>
---
 gpg-interface.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 3414af38b9..79333c1ee8 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -101,11 +101,16 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
 		fputs(output, stderr);
 }

+static int is_gpg_start(const char *line)
+{
+	return starts_with(line, PGP_SIGNATURE) ||
+		starts_with(line, PGP_MESSAGE);
+}
+
 size_t parse_signature(const char *buf, size_t size)
 {
 	size_t len = 0;
-	while (len < size && !starts_with(buf + len, PGP_SIGNATURE) &&
-			!starts_with(buf + len, PGP_MESSAGE)) {
+	while (len < size && !is_gpg_start(buf + len)) {
 		const char *eol = memchr(buf + len, '\n', size - len);
 		len += eol ? eol - (buf + len) + 1 : size - len;
 	}
--
2.15.1 (Apple Git-101)

[PATCH v2 7/9] gpg-interface: find the last gpg signature line

[Ben Toews]

From: Jeff King <peff@peff.net>

A signed tag has a detached signature like this:

  object ...
  [...more header...]

  This is the tag body.

  -----BEGIN PGP SIGNATURE-----
  [opaque gpg data]
  -----END PGP SIGNATURE-----

Our parser finds the _first_ line that appears to start a
PGP signature block, meaning we may be confused by a
signature (or a signature-like line) in the actual body.
Let's keep parsing and always find the final block, which
should be the detached signature over all of the preceding
content.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ben Toews <mastahyeti@gmail.com>
---
 gpg-interface.c | 12 +++++++++---
 t/t7004-tag.sh  | 11 +++++++++++
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 79333c1ee8..0647bd6348 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -110,11 +110,17 @@ static int is_gpg_start(const char *line)
 size_t parse_signature(const char *buf, size_t size)
 {
 	size_t len = 0;
-	while (len < size && !is_gpg_start(buf + len)) {
-		const char *eol = memchr(buf + len, '\n', size - len);
+	size_t match = size;
+	while (len < size) {
+		const char *eol;
+
+		if (is_gpg_start(buf + len))
+			match = len;
+
+		eol = memchr(buf + len, '\n', size - len);
 		len += eol ? eol - (buf + len) + 1 : size - len;
 	}
-	return len;
+	return match;
 }

 void set_signing_key(const char *key)
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index ee093b393d..e3f1e014aa 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1059,6 +1059,17 @@ test_expect_success GPG \
 	git tag -v blanknonlfile-signed-tag
 '

+test_expect_success GPG 'signed tag with embedded PGP message' '
+	cat >msg <<-\EOF &&
+	-----BEGIN PGP MESSAGE-----
+
+	this is not a real PGP message
+	-----END PGP MESSAGE-----
+	EOF
+	git tag -s -F msg confusing-pgp-message &&
+	git tag -v confusing-pgp-message
+'
+
 # messages with commented lines for signed tags:

 cat >sigcommentsfile <<EOF
--
2.15.1 (Apple Git-101)

[PATCH v2 8/9] gpg-interface: prepare for parsing arbitrary PEM blocks

[Ben Toews]

From: Jeff King <peff@peff.net>

In preparation for handling more PEM blocks besides "PGP
SIGNATURE" and "PGP MESSAGE', let's break up the parsing to
parameterize the actual block type.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ben Toews <mastahyeti@gmail.com>
---
 gpg-interface.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 0647bd6348..0ba4a8ac3b 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -9,9 +9,6 @@
 static char *configured_signing_key;
 static const char *gpg_program = "gpg";

-#define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
-#define PGP_MESSAGE "-----BEGIN PGP MESSAGE-----"
-
 void signature_check_clear(struct signature_check *sigc)
 {
 	FREE_AND_NULL(sigc->payload);
@@ -101,10 +98,17 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
 		fputs(output, stderr);
 }

-static int is_gpg_start(const char *line)
+static int is_pem_start(const char *line)
 {
-	return starts_with(line, PGP_SIGNATURE) ||
-		starts_with(line, PGP_MESSAGE);
+	if (!skip_prefix(line, "-----BEGIN ", &line))
+		return 0;
+	if (!skip_prefix(line, "PGP SIGNATURE", &line) &&
+	    !skip_prefix(line, "PGP MESSAGE", &line))
+		return 0;
+	if (!starts_with(line, "-----"))
+		return 0;
+
+	return 1;
 }

 size_t parse_signature(const char *buf, size_t size)
@@ -114,7 +118,7 @@ size_t parse_signature(const char *buf, size_t size)
 	while (len < size) {
 		const char *eol;

-		if (is_gpg_start(buf + len))
+		if (is_pem_start(buf + len))
 			match = len;

 		eol = memchr(buf + len, '\n', size - len);
--
2.15.1 (Apple Git-101)

[PATCH v2 9/9] gpg-interface: handle alternative signature types

[Ben Toews]

Currently you can only sign commits and tags using "gpg".
You can _almost_ plug in a related tool like "gpgsm" (which
uses S/MIME-style signatures instead of PGP) using
gpg.program, as it has command-line compatibility. But there
are a few rough edges:

  1. gpgsm generates a slightly different PEM format than
     gpg. It says:

       -----BEGIN SIGNED MESSAGE-----

     instead of:

       -----BEGIN PGP SIGNATURE-----

     This actually works OK for signed commits, where we
     just dump the gpgsig header to gpg.program regardless.

     But for tags, we actually have to parse out the
     detached signature, and we fail to recognize the gpgsm
     version.

  2. You can't mix the two types of signatures easily, as
     we'd send the output to whatever tool you've
     configured. For verification, we'd ideally dispatch gpg
     signatures to gpg, gpgsm ones to gpgsm, and so on. For
     signing, you'd obviously have to pick a tool to use.

This patch introduces a set of configuration options for
defining a "signing tool", of which gpg may be just one.
With this patch you can:

  - define a new tool "foo" with signingtool.foo.program

  - map PEM strings to it for verification using
    signingtool.foo.pemtype

  - set it as your default tool for creating signatures
    using signingtool.default

This subsumes the existing gpg config, as we have baked-in
config for signingtool.gpg that works just like the current
hard-coded behavior. And setting gpg.program becomes an
alias for signingtool.gpg.program.

This is enough to plug in gpgsm like:

  [signingtool "gpgsm"]
  program = gpgsm
  pemtype = "SIGNED MESSAGE"

In the future, this config scheme gives us options for
extending to other tools:

  - the tools all have to accept gpg-like options for now,
    but we could add signingtool.interface to meet other
    standards

  - we only match PEM headers now, but we could add other
    config for matching non-PEM types

The implementation is still in gpg-interface.c, and most of
the code internally refers to this as "gpg". I've left it
this way to keep the diff sane, and to make it clear that
we're not breaking anything gpg-related. This is probably OK
for now, as our tools must be gpg-like anyway. But renaming
everything would be an obvious next-step refactoring if we
want to offer support for more exotic tools (e.g., people
have asked before on the list about using OpenBSD signify).

Signed-off-by: Ben Toews <mastahyeti@gmail.com>
---
 Documentation/config.txt |  42 +++++++++---
 builtin/fmt-merge-msg.c  |   6 +-
 builtin/receive-pack.c   |   7 +-
 builtin/tag.c            |   2 +-
 commit.c                 |   2 +-
 gpg-interface.c          | 168 ++++++++++++++++++++++++++++++++++++++++++-----
 gpg-interface.h          |  24 ++++++-
 log-tree.c               |   7 +-
 ref-filter.c             |   2 +-
 t/lib-gpg.sh             |  30 +++++++++
 t/t7510-signed-commit.sh |  34 +++++++++-
 tag.c                    |   2 +-
 12 files changed, 283 insertions(+), 43 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4e0cff87f6..691b309306 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1727,16 +1727,40 @@ grep.fallbackToNoIndex::
 	If set to true, fall back to git grep --no-index if git grep
 	is executed outside of a git repository.  Defaults to false.

-gpg.program::
-	Use this custom program instead of "`gpg`" found on `$PATH` when
-	making or verifying a PGP signature. The program must support the
-	same command-line interface as GPG, namely, to verify a detached
-	signature, "`gpg --verify $file - <$signature`" is run, and the
-	program is expected to signal a good signature by exiting with
-	code 0, and to generate an ASCII-armored detached signature, the
-	standard input of "`gpg -bsau $key`" is fed with the contents to be
+signingtool.<name>.program::
+	The name or path of the program to execute when making or
+	verifying a signature. This program will be used for making
+	signatures if `<name>` is configured as `signingtool.default`.
+	This program will be used for verifying signatures whose PEM
+	block type matches `signingtool.<name>.pemtype` (see below). The
+	program must support the same command-line interface as GPG.
+	To verify a detached signature,
+	"`gpg --verify $file - <$signature`" is run, and the program is
+	expected to signal a good signature by exiting with code 0.
+	To generate an ASCII-armored detached signature, the standard
+	input of "`gpg -bsau $key`" is fed with the contents to be
 	signed, and the program is expected to send the result to its
-	standard output.
+	standard output. By default, `signingtool.gpg.program` is set to
+	`gpg`.
+
+signingtool.<name>.pemtype::
+	The PEM block type associated with the signing tool named
+	`<name>`. For example, the block type of a GPG signature
+	starting with `-----BEGIN PGP SIGNATURE-----` is `PGP
+	SIGNATURE`. When verifying a signature with this PEM block type
+	the program specified in `signingtool.<name>.program` will be
+	used. By default `signingtool.gpg.pemtype` contains `PGP
+	SIGNATURE` and `PGP MESSAGE`. Multiple PEM types may be specified
+	for a single signing tool by including the `pemtype` directive
+	multiple times within the `signingtool` configuration.
+
+signingtool.default::
+	The `<name>` of the signing tool to use when creating
+	signatures (e.g., setting it to "foo" will use use the program
+	specified by `signingtool.foo.program`). Defaults to `gpg`.
+
+gpg.program::
+	Historical alias for `signingtool.gpg.program`.

 gui.commitMsgWidth::
 	Defines how wide the commit message window is in the
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 8e8a15ea4a..6aa8226308 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -490,14 +490,16 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
 		unsigned long size, len;
 		char *buf = read_sha1_file(sha1, &type, &size);
 		struct strbuf sig = STRBUF_INIT;
+		const struct signing_tool *tool;

 		if (!buf || type != OBJ_TAG)
 			goto next;
-		len = parse_signature(buf, size);
+		len = parse_signature(buf, size, &tool);

 		if (size == len)
 			; /* merely annotated */
-		else if (verify_signed_buffer(buf, len, buf + len, size - len, &sig, NULL)) {
+		else if (verify_signed_buffer(buf, len, buf + len, size - len,
+					      &sig, NULL, tool)) {
 			if (!sig.len)
 				strbuf_addstr(&sig, "gpg verification failed.\n");
 		}
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 75e7f18ace..be17dcaa78 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -631,6 +631,7 @@ static void prepare_push_cert_sha1(struct child_process *proc)
 		struct strbuf gpg_output = STRBUF_INIT;
 		struct strbuf gpg_status = STRBUF_INIT;
 		int bogs /* beginning_of_gpg_sig */;
+		const struct signing_tool *tool;

 		already_done = 1;
 		if (write_object_file(push_cert.buf, push_cert.len, "blob",
@@ -640,10 +641,10 @@ static void prepare_push_cert_sha1(struct child_process *proc)
 		memset(&sigcheck, '\0', sizeof(sigcheck));
 		sigcheck.result = 'N';

-		bogs = parse_signature(push_cert.buf, push_cert.len);
+		bogs = parse_signature(push_cert.buf, push_cert.len, &tool);
 		if (verify_signed_buffer(push_cert.buf, bogs,
 					 push_cert.buf + bogs, push_cert.len - bogs,
-					 &gpg_output, &gpg_status) < 0) {
+					 &gpg_output, &gpg_status, tool) < 0) {
 			; /* error running gpg */
 		} else {
 			sigcheck.payload = push_cert.buf;
@@ -1565,7 +1566,7 @@ static void queue_commands_from_cert(struct command **tail,
 		die("malformed push certificate %.*s", 100, push_cert->buf);
 	else
 		boc += 2;
-	eoc = push_cert->buf + parse_signature(push_cert->buf, push_cert->len);
+	eoc = push_cert->buf + parse_signature(push_cert->buf, push_cert->len, NULL);

 	while (boc < eoc) {
 		const char *eol = memchr(boc, '\n', eoc - boc);
diff --git a/builtin/tag.c b/builtin/tag.c
index da186691ed..adc37a28c5 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -178,7 +178,7 @@ static void write_tag_body(int fd, const struct object_id *oid)
 		return;
 	}
 	sp += 2; /* skip the 2 LFs */
-	write_or_die(fd, sp, parse_signature(sp, buf + size - sp));
+	write_or_die(fd, sp, parse_signature(sp, buf + size - sp, NULL));

 	free(buf);
 }
diff --git a/commit.c b/commit.c
index 00c99c7272..056942df7f 100644
--- a/commit.c
+++ b/commit.c
@@ -1209,7 +1209,7 @@ static void handle_signed_tag(struct commit *parent, struct commit_extra_header
 	buf = read_sha1_file(desc->obj->oid.hash, &type, &size);
 	if (!buf || type != OBJ_TAG)
 		goto free_return;
-	len = parse_signature(buf, size);
+	len = parse_signature(buf, size, NULL);
 	if (size == len)
 		goto free_return;
 	/*
diff --git a/gpg-interface.c b/gpg-interface.c
index 0ba4a8ac3b..5d4ae2a7ed 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -7,7 +7,62 @@
 #include "tempfile.h"

 static char *configured_signing_key;
-static const char *gpg_program = "gpg";
+static const char *default_signing_tool = "gpg";
+static struct signing_tool *signing_tool_config;
+
+static struct signing_tool *alloc_signing_tool(void)
+{
+	struct signing_tool *ret;
+	ret = xcalloc(1, sizeof(*ret));
+	ret->pemtype.strdup_strings = 1;
+	return ret;
+}
+
+/*
+ * Our default tool config is too complicated to specify as a constant
+ * initializer, so we lazily create it as needed.
+ */
+static void init_signing_tool_defaults(void)
+{
+	struct signing_tool *tool;
+
+	if (signing_tool_config)
+		return;
+
+	tool = alloc_signing_tool();
+	tool->name = xstrdup("gpg");
+	tool->program = xstrdup("gpg");
+	string_list_append(&tool->pemtype, "PGP SIGNATURE");
+	string_list_append(&tool->pemtype, "PGP MESSAGE");
+
+	tool->next = signing_tool_config;
+	signing_tool_config = tool;
+}
+
+static struct signing_tool *get_signing_tool(const char *name)
+{
+	struct signing_tool *tool;
+
+	init_signing_tool_defaults();
+
+	for (tool = signing_tool_config; tool; tool = tool->next) {
+		if (!strcmp(name, tool->name))
+			return tool;
+	}
+	return NULL;
+}
+
+static struct signing_tool *get_or_create_signing_tool(const char *name)
+{
+	struct signing_tool *tool = get_signing_tool(name);
+	if (!tool) {
+		tool = alloc_signing_tool();
+		tool->name = xstrdup(name);
+		tool->next = signing_tool_config;
+		signing_tool_config = tool;
+	}
+	return tool;
+}

 void signature_check_clear(struct signature_check *sigc)
 {
@@ -71,7 +126,7 @@ int check_signature(const char *payload, size_t plen, const char *signature,
 	sigc->result = 'N';

 	status = verify_signed_buffer(payload, plen, signature, slen,
-				      &gpg_output, &gpg_status);
+				      &gpg_output, &gpg_status, NULL);
 	if (status && !gpg_output.len)
 		goto out;
 	sigc->payload = xmemdupz(payload, plen);
@@ -98,32 +153,49 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
 		fputs(output, stderr);
 }

-static int is_pem_start(const char *line)
+static int is_pem_start(const char *line, struct signing_tool **out_tool)
 {
+	struct signing_tool *tool;
+
 	if (!skip_prefix(line, "-----BEGIN ", &line))
 		return 0;
-	if (!skip_prefix(line, "PGP SIGNATURE", &line) &&
-	    !skip_prefix(line, "PGP MESSAGE", &line))
-		return 0;
-	if (!starts_with(line, "-----"))
-		return 0;

-	return 1;
+	init_signing_tool_defaults();
+
+	for (tool = signing_tool_config; tool; tool = tool->next) {
+		int i;
+		for (i = 0; i < tool->pemtype.nr; i++) {
+			const char *match = tool->pemtype.items[i].string;
+			const char *end;
+			if (skip_prefix(line, match, &end) &&
+			    starts_with(end, "-----")) {
+				*out_tool = tool;
+				return 1;
+			}
+		}
+	}
+	return 0;
 }

-size_t parse_signature(const char *buf, size_t size)
+size_t parse_signature(const char *buf, size_t size,
+		       const struct signing_tool **out_tool)
 {
 	size_t len = 0;
 	size_t match = size;
+	struct signing_tool *tool = NULL;
+
 	while (len < size) {
 		const char *eol;

-		if (is_pem_start(buf + len))
+		if (is_pem_start(buf + len, &tool))
 			match = len;

 		eol = memchr(buf + len, '\n', size - len);
 		len += eol ? eol - (buf + len) + 1 : size - len;
 	}
+
+	if (out_tool)
+		*out_tool = tool;
 	return match;
 }

@@ -135,6 +207,9 @@ void set_signing_key(const char *key)

 int git_gpg_config(const char *var, const char *value, void *cb)
 {
+	const char *key, *name = NULL;
+	int name_len;
+
 	if (!strcmp(var, "user.signingkey")) {
 		if (!value)
 			return config_error_nonbool(var);
@@ -143,12 +218,44 @@ int git_gpg_config(const char *var, const char *value, void *cb)
 	}

 	if (!strcmp(var, "gpg.program")) {
+		struct signing_tool *tool;
+
 		if (!value)
 			return config_error_nonbool(var);
-		gpg_program = xstrdup(value);
+
+		tool = get_or_create_signing_tool("gpg");
+		free(tool->program);
+		tool->program = xstrdup(value);
 		return 0;
 	}

+	if (!strcmp(var, "signingtool.default"))
+		return git_config_string(&default_signing_tool, var, value);
+
+	if (!parse_config_key(var, "signingtool", &name, &name_len, &key) && name) {
+		char *tmpname = xmemdupz(name, name_len);
+		struct signing_tool *tool = get_or_create_signing_tool(tmpname);
+
+		free(tmpname);
+
+		if (!strcmp(key, "program")) {
+			if (!value)
+				return config_error_nonbool(var);
+
+			free(tool->program);
+			tool->program = xstrdup(value);
+			return 0;
+		}
+
+		if (!strcmp(key, "pemtype")) {
+			if (!value)
+				return config_error_nonbool(var);
+
+			string_list_append(&tool->pemtype, value);
+			return 0;
+		}
+	}
+
 	return 0;
 }

@@ -159,7 +266,9 @@ const char *get_signing_key(void)
 	return git_committer_info(IDENT_STRICT|IDENT_NO_DATE);
 }

-int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
+static int sign_buffer_with(struct strbuf *buffer, struct strbuf *signature,
+			    const char *signing_key,
+			    const struct signing_tool *tool)
 {
 	struct child_process gpg = CHILD_PROCESS_INIT;
 	int ret;
@@ -167,7 +276,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
 	struct strbuf gpg_status = STRBUF_INIT;

 	argv_array_pushl(&gpg.args,
-			 gpg_program,
+			 tool->program,
 			 "--status-fd=2",
 			 "-bsau", signing_key,
 			 NULL);
@@ -200,15 +309,42 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
 	return 0;
 }

+int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
+{
+	struct signing_tool *tool = get_signing_tool(default_signing_tool);
+	if (!tool || !tool->program)
+		return error(_("default signing tool '%s' has no program configured"),
+			     default_signing_tool);
+	return sign_buffer_with(buffer, signature, signing_key, tool);
+}
+
 int verify_signed_buffer(const char *payload, size_t payload_size,
 			 const char *signature, size_t signature_size,
-			 struct strbuf *gpg_output, struct strbuf *gpg_status)
+			 struct strbuf *gpg_output, struct strbuf *gpg_status,
+			 const struct signing_tool *tool)
 {
 	struct child_process gpg = CHILD_PROCESS_INIT;
 	struct tempfile *temp;
 	int ret;
 	struct strbuf buf = STRBUF_INIT;

+	if (!tool) {
+		parse_signature(signature, signature_size, &tool);
+		if (!tool) {
+			/*
+			 * The caller didn't tell us which tool to use, and we
+			 * didn't recognize the format. Historically we've fed
+			 * these cases blindly to gpg, so let's continue to
+			 * do so.
+			 */
+			tool = get_signing_tool("gpg");
+		}
+	}
+
+	if (!tool->program)
+		return error(_("signing tool '%s' has no program configured"),
+			     tool->name);
+
 	temp = mks_tempfile_t(".git_vtag_tmpXXXXXX");
 	if (!temp)
 		return error_errno(_("could not create temporary file"));
@@ -221,7 +357,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 	}

 	argv_array_pushl(&gpg.args,
-			 gpg_program,
+			 tool->program,
 			 "--status-fd=1",
 			 "--keyid-format=long",
 			 "--verify", temp->filename.buf, "-",
diff --git a/gpg-interface.h b/gpg-interface.h
index a5e6517ae6..8e22e67b6f 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -23,15 +23,27 @@ struct signature_check {
 	char *key;
 };

+struct signing_tool {
+	char *name;
+	char *program;
+	struct string_list pemtype;
+	struct signing_tool *next;
+};
+
 void signature_check_clear(struct signature_check *sigc);

 /*
- * Look at GPG signed content (e.g. a signed tag object), whose
+ * Look for signed content (e.g. a signed tag object), whose
  * payload is followed by a detached signature on it.  Return the
  * offset where the embedded detached signature begins, or the end of
  * the data when there is no such signature.
+ *
+ * If out_tool is non-NULL and a signature is found, it will be
+ * pointed at the signing_tool that corresponds to the found
+ * signature type.
  */
-size_t parse_signature(const char *buf, size_t size);
+size_t parse_signature(const char *buf, size_t size,
+		       const struct signing_tool **out_tool);

 void parse_gpg_output(struct signature_check *);

@@ -48,10 +60,16 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature,
  * Run "gpg" to see if the payload matches the detached signature.
  * gpg_output, when set, receives the diagnostic output from GPG.
  * gpg_status, when set, receives the status output from GPG.
+ *
+ * Typically, the "tool" argument should come from a previous call to
+ * parse_signature(). If it's NULL, then verify_signed_buffer() will
+ * try to choose the appropriate tool based on the contents of the
+ * "signature" buffer.
  */
 int verify_signed_buffer(const char *payload, size_t payload_size,
 			 const char *signature, size_t signature_size,
-			 struct strbuf *gpg_output, struct strbuf *gpg_status);
+			 struct strbuf *gpg_output, struct strbuf *gpg_status,
+			 const struct signing_tool *tool);

 int git_gpg_config(const char *, const char *, void *);
 void set_signing_key(const char *);
diff --git a/log-tree.c b/log-tree.c
index bdf23c5f7b..f8fdff6e65 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -456,7 +456,7 @@ static void show_signature(struct rev_info *opt, struct commit *commit)

 	status = verify_signed_buffer(payload.buf, payload.len,
 				      signature.buf, signature.len,
-				      &gpg_output, NULL);
+				      &gpg_output, NULL, NULL);
 	if (status && !gpg_output.len)
 		strbuf_addstr(&gpg_output, "No signature\n");

@@ -498,6 +498,7 @@ static void show_one_mergetag(struct commit *commit,
 	struct strbuf verify_message;
 	int status, nth;
 	size_t payload_size, gpg_message_offset;
+	const struct signing_tool *tool;

 	hash_object_file(extra->value, extra->len, type_name(OBJ_TAG), &oid);
 	tag = lookup_tag(&oid);
@@ -520,14 +521,14 @@ static void show_one_mergetag(struct commit *commit,
 			    "parent #%d, tagged '%s'\n", nth + 1, tag->tag);
 	gpg_message_offset = verify_message.len;

-	payload_size = parse_signature(extra->value, extra->len);
+	payload_size = parse_signature(extra->value, extra->len, &tool);
 	status = -1;
 	if (extra->len > payload_size) {
 		/* could have a good signature */
 		if (!verify_signed_buffer(extra->value, payload_size,
 					  extra->value + payload_size,
 					  extra->len - payload_size,
-					  &verify_message, NULL))
+					  &verify_message, NULL, tool))
 			status = 0; /* good */
 		else if (verify_message.len <= gpg_message_offset)
 			strbuf_addstr(&verify_message, "No signature\n");
diff --git a/ref-filter.c b/ref-filter.c
index 45fc56216a..6ab66b9a7c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1009,7 +1009,7 @@ static void find_subpos(const char *buf, unsigned long sz,
 		buf++;

 	/* parse signature first; we might not even have a subject line */
-	*sig = buf + parse_signature(buf, strlen(buf));
+	*sig = buf + parse_signature(buf, strlen(buf), NULL);
 	*siglen = strlen(*sig);

 	/* subject is first non-empty line */
diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index a5d3b2cbaa..b9ba47057f 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -56,3 +56,33 @@ sanitize_pgp() {
 		/^-----BEGIN PGP/ and $in_pgp = 1;
 	'
 }
+
+create_fake_signer () {
+	write_script fake-signer <<-\EOF
+	if test "$1 $2" = "--status-fd=2 -bsau"
+	then
+		echo >&2 "[GNUPG:] BEGIN_SIGNING"
+		echo >&2 "[GNUPG:] SIG_CREATED D 1 SHA256 0 1513792449 4A7FF9E2330D22B19213A4E9E9C423BE17EFEE70"
+		cat <<-\END
+		-----BEGIN FAKE SIGNER SIGNATURE-----
+		fake-signature
+		-----END FAKE SIGNER SIGNATURE-----
+		END
+		exit 0
+
+	elif test "$1 $2 $3" = "--status-fd=1 --keyid-format=long --verify"
+	then
+		cat <<-\END
+		[GNUPG:] NEWSIG
+		[GNUPG:] GOODSIG 4A7FF9E2330D22B19213A4E9E9C423BE17EFEE70 /CN=Some User/EMail=some@user.email
+		[GNUPG:] TRUST_FULLY 0 shell
+		END
+		echo >&2 "Good signature from /CN=Some User/EMail=some@user.email"
+		exit 0
+
+	else
+		echo "bad arguments" 1>&2
+		exit 1
+	fi
+	EOF
+}
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 762135adea..fb41f98ca6 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -5,6 +5,10 @@ test_description='signed commit tests'
 GNUPGHOME_NOT_USED=$GNUPGHOME
 . "$TEST_DIRECTORY/lib-gpg.sh"

+test_expect_success 'create fake signing program' '
+	create_fake_signer
+'
+
 test_expect_success GPG 'create signed commits' '
 	test_when_finished "test_unconfig commit.gpgsign" &&

@@ -51,13 +55,35 @@ test_expect_success GPG 'create signed commits' '
 	# commit.gpgsign is still on but this must not be signed
 	git tag ninth-unsigned $(echo 9 | git commit-tree HEAD^{tree}) &&
 	# explicit -S of course must sign.
-	git tag tenth-signed $(echo 9 | git commit-tree -S HEAD^{tree})
+	git tag tenth-signed $(echo 9 | git commit-tree -S HEAD^{tree}) &&
+
+	git config signingtool.fake.program "$TRASH_DIRECTORY/fake-signer" &&
+	git config signingtool.fake.pemtype "FAKE SIGNER SIGNATURE" &&
+	echo 11 >file && test_tick && git commit -a -m eleventh &&
+	git tag eleventh-pgp-signed &&
+	git cat-file -p eleventh-pgp-signed >actual &&
+	grep "PGP SIGNATURE" actual &&
+
+	git config gpg.program "$TRASH_DIRECTORY/fake-signer" &&
+	echo 12 >file && test_tick && git commit -a -m twelfth &&
+	test_unconfig gpg.program &&
+	git tag twelfth-fake-signed &&
+	git cat-file -p twelfth-fake-signed >actual &&
+	grep "FAKE SIGNER SIGNATURE" actual &&
+
+	git config signingtool.default fake &&
+	echo 13 >file && test_tick && git commit -a -m thirteenth &&
+	test_unconfig signingtool.default &&
+	git tag thirteenth-fake-signed &&
+	git cat-file -p thirteenth-fake-signed >actual &&
+	grep "FAKE SIGNER SIGNATURE" actual
 '

 test_expect_success GPG 'verify and show signatures' '
 	(
 		for commit in initial second merge fourth-signed \
-			fifth-signed sixth-signed seventh-signed tenth-signed
+			fifth-signed sixth-signed seventh-signed tenth-signed \
+			eleventh-pgp-signed twelfth-fake-signed thirteenth-fake-signed
 		do
 			git verify-commit $commit &&
 			git show --pretty=short --show-signature $commit >actual &&
@@ -98,7 +124,9 @@ test_expect_success GPG 'verify-commit exits success on untrusted signature' '

 test_expect_success GPG 'verify signatures with --raw' '
 	(
-		for commit in initial second merge fourth-signed fifth-signed sixth-signed seventh-signed
+		for commit in initial second merge fourth-signed fifth-signed sixth-signed \
+			seventh-signed eleventh-pgp-signed twelfth-fake-signed \
+			thirteenth-fake-signed
 		do
 			git verify-commit --raw $commit 2>actual &&
 			grep "GOODSIG" actual &&
diff --git a/tag.c b/tag.c
index 66210fd477..2d557e1f5e 100644
--- a/tag.c
+++ b/tag.c
@@ -15,7 +15,7 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)

 	memset(&sigc, 0, sizeof(sigc));

-	payload_size = parse_signature(buf, size);
+	payload_size = parse_signature(buf, size, NULL);

 	if (size == payload_size) {
 		if (flags & GPG_VERIFY_VERBOSE)
--
2.15.1 (Apple Git-101)