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

[Eric Sunshine <sunshine@sunshineco.com>]

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

s/using using/using/

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

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

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

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

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

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

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

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

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

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

3. add support for the new configuration variables

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

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

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

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

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

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

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

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

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

s/to blindly/blindly/

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

s/Typically/&,/

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

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

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

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

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

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

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

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

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

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

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

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

Ditto.

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