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

[Jeff King]

On Tue, May 08, 2018 at 11:09:22PM +, 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

[brian m. carlson]

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


signature.asc
Description: PGP signature

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

[Jeff King]

On Mon, May 07, 2018 at 11:06:50PM +, 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]

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


signature.asc
Description: PGP signature

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

[Junio C Hamano]

Jeff King  writes:

> On Tue, Apr 17, 2018 at 12:12:12AM +, 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

[Jeff King]

On Tue, Apr 17, 2018 at 12:12:12AM +, 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

[Ben Toews]

On Tue, Apr 17, 2018 at 12:33 PM, Taylor Blau  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  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.
> >
> > [...]
> >
> > 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

[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  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.
>
> [...]
>
> 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 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.

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

[Junio C Hamano]

"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.

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

[brian m. carlson]

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


signature.asc
Description: PGP signature

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

[Junio C Hamano]

"brian m. carlson"  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]

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


signature.asc
Description: PGP signature

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".

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

[Ben Toews]

On Tue, Apr 10, 2018 at 3:35 AM, Junio C Hamano  wrote:
> Ben Toews  writes:
>
>> From: Ben Toews 
>>
>> 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  wrote:
> On Mon, Apr 9, 2018 at 4:41 PM, Ben Toews  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 

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

[Junio C Hamano]

Ben Toews  writes:

> From: Ben Toews 
>
> 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?

> +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.

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

[Eric Sunshine]

On Mon, Apr 9, 2018 at 4:41 PM, Ben Toews  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..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 `` is configured as 

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  wrote:
> From: 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 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..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 

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

[Ben Toews]

From: 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 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..pemtype::
+   The PEM block type associated with the signing tool named
+   ``. For example, the block type of a GPG signature
+