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

[Ben Toews]

On Tue, Apr 17, 2018 at 12:33 PM, Taylor Blau <m...@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 <gits...@pobox.com> wrote:
> > > "brian m. carlson" <sand...@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

[Ben Toews]

On Mon, Apr 16, 2018 at 7:54 PM, Junio C Hamano  wrote:
> "brian m. carlson"  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..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.

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

[Ben Toews]

From: Jeff King <p...@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 <p...@peff.net>
Signed-off-by: Ben Toews <mastahy...@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 <p...@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 <p...@peff.net>
Signed-off-by: Ben Toews <mastahy...@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 <

[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 <mastahy...@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..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 `` is configured as `signingtool.default`.
+   This program will be used for verifying signatures whose PEM
+   block type matches `signingtool..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..pe

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

[Ben Toews]

From: Jeff King <p...@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 <p...@peff.net>
Signed-off-by: Ben Toews <mastahy...@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 <p...@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 <p...@peff.net>
Signed-off-by: Ben Toews <mastahy...@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 5/9] gpg-interface: fix const-correctness of "eol" pointer

[Ben Toews]

From: Jeff King <p...@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 <p...@peff.net>
Signed-off-by: Ben Toews <mastahy...@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 4/9] gpg-interface: use size_t for signature buffer size

[Ben Toews]

From: Jeff King <p...@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 <p...@peff.net>
Signed-off-by: Ben Toews <mastahy...@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 8/9] gpg-interface: prepare for parsing arbitrary PEM blocks

[Ben Toews]

From: Jeff King <p...@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 <p...@peff.net>
Signed-off-by: Ben Toews <mastahy...@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 ", ))
+   return 0;
+   if (!skip_prefix(line, "PGP SIGNATURE", ) &&
+   !skip_prefix(line, "PGP MESSAGE", ))
+   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 0/9] gpg-interface: Multiple signing tools

[Ben Toews]

FF9E2330D22B19213A4E9E9C423BE17EFEE70"
-   # 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 <p...@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 <p...@peff.net>
Signed-off-by: Ben Toews <mastahy...@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)

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 <gits...@pobox.com> wrote:
> Junio C Hamano <gits...@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 <p...@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

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

[Ben Toews]

On Tue, Apr 10, 2018 at 3:35 AM, Junio C Hamano <gits...@pobox.com> wrote:
> Ben Toews <mastahy...@gmail.com> writes:
>
>> From: Ben Toews <bto...@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..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 `` is configured as `signingtool.default`.
>> + This program will be used for verifying signatures whose PEM
>> + block type matches `signingtool..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..pemtype::
>> + The PEM block type associated with the signing tool named
>> + ``. 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..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 `` 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

[Ben Toews]

On Tue, Apr 10, 2018 at 2:24 AM, Eric Sunshine <sunsh...@sunshineco.com> wrote:
> On Mon, Apr 9, 2018 at 4:41 PM, Ben Toews <mastahy...@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 "signingtoo

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 <gits...@pobox.com> wrote:
> Ben Toews <mastahy...@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 <
> H, 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

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

[Ben Toews]

From: Ben Toews <bto...@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..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 `` is configured as `signingtool.default`.
+   This program will be used for verifying signatures whose PEM
+   block type matches `signingtool..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..pe

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

[Ben Toews]

From: Jeff King 

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)

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

[Ben Toews]

From: Jeff King 

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 2/8] gpg-interface: modernize function declarations

[Ben Toews]

From: Jeff King 

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 4/8] gpg-interface: fix const-correctness of "eol" pointer

[Ben Toews]

From: Jeff King 

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 6/8] gpg-interface: find the last gpg signature line

[Ben Toews]

From: Jeff King 

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 <

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

[Ben Toews]

From: Jeff King 

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 ", ))
+   return 0;
+   if (!skip_prefix(line, "PGP SIGNATURE", ) &&
+   !skip_prefix(line, "PGP MESSAGE", ))
+   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 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 5/8] gpg-interface: extract gpg line matching helper

[Ben Toews]

From: Jeff King 

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)