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

[Stefan Beller <sbeller@google.com>]

Hi Ben,

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

Please sign off your patch, see Documentation/SubmittingPatches

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

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

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


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

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