ssh signing: preliminary refactoring and clean-up

Openssh v8.2p1 added some new options to ssh-keygen for signature
creation and verification. These allow us to use ssh keys for git
signatures easily.

In our corporate environment we use PIV x509 Certs on Yubikeys for email
signing/encryption and ssh keys which I think is quite common
(at least for the email part). This way we can establish the correct
trust for the SSH Keys without setting up a separate GPG Infrastructure
(which is still quite painful for users) or implementing x509 signing
support for git (which lacks good forwarding mechanisms).
Using ssh agent forwarding makes this feature easily usable in todays
development environments where code is often checked out in remote VMs / containers.
In such a setup the keyring & revocationKeyring can be centrally
generated from the x509 CA information and distributed to the users.

To be able to implement new signing formats this commit:
 - makes the sigc structure more generic by renaming "gpg_output" to
   "output"
 - introduces function pointers in the gpg_format structure to call
   format specific signing and verification functions
 - moves format detection from verify_signed_buffer into the check_signature
   api function and calls the format specific verify
 - renames and wraps sign_buffer to handle format specific signing logic
   as well

Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
index 0f66818e0f839f08cf5cc46cb6943f6542c4208b..fb300bb4b67861cbd6b31fdf4911f1fca9c3bf0c 100644 (file)
--- a/fmt-merge-msg.c
+++ b/fmt-merge-msg.c
@@ -526,11 +526,11 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
                        buf = payload.buf;
                        len = payload.len;
                        if (check_signature(payload.buf, payload.len, sig.buf,
-                                        sig.len, &sigc) &&
-                               !sigc.gpg_output)
+                                           sig.len, &sigc) &&
+                           !sigc.output)
                                strbuf_addstr(&sig, "gpg verification failed.\n");
                        else
-                               strbuf_addstr(&sig, sigc.gpg_output);
+                               strbuf_addstr(&sig, sigc.output);
                }
                signature_check_clear(&sigc);
 

diff --git a/gpg-interface.c b/gpg-interface.c
index 127aecfc2b071f9a745a871a9ea205931eeb672f..db54b05416257d1411119439ff71ad2dce9917ab 100644 (file)
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -15,6 +15,12 @@ struct gpg_format {
        const char *program;
        const char **verify_args;
        const char **sigs;
+       int (*verify_signed_buffer)(struct signature_check *sigc,
+                                   struct gpg_format *fmt, const char *payload,
+                                   size_t payload_size, const char *signature,
+                                   size_t signature_size);
+       int (*sign_buffer)(struct strbuf *buffer, struct strbuf *signature,
+                          const char *signing_key);
 };
 
 static const char *openpgp_verify_args[] = {
@@ -35,14 +41,29 @@ static const char *x509_sigs[] = {
        NULL
 };
 
+static int verify_gpg_signed_buffer(struct signature_check *sigc,
+                                   struct gpg_format *fmt, const char *payload,
+                                   size_t payload_size, const char *signature,
+                                   size_t signature_size);
+static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,
+                          const char *signing_key);
+
 static struct gpg_format gpg_format[] = {
-       { .name = "openpgp", .program = "gpg",
-         .verify_args = openpgp_verify_args,
-         .sigs = openpgp_sigs
+       {
+               .name = "openpgp",
+               .program = "gpg",
+               .verify_args = openpgp_verify_args,
+               .sigs = openpgp_sigs,
+               .verify_signed_buffer = verify_gpg_signed_buffer,
+               .sign_buffer = sign_buffer_gpg,
        },
-       { .name = "x509", .program = "gpgsm",
-         .verify_args = x509_verify_args,
-         .sigs = x509_sigs
+       {
+               .name = "x509",
+               .program = "gpgsm",
+               .verify_args = x509_verify_args,
+               .sigs = x509_sigs,
+               .verify_signed_buffer = verify_gpg_signed_buffer,
+               .sign_buffer = sign_buffer_gpg,
        },
 };
 
@@ -72,7 +93,7 @@ static struct gpg_format *get_format_by_sig(const char *sig)
 void signature_check_clear(struct signature_check *sigc)
 {
        FREE_AND_NULL(sigc->payload);
-       FREE_AND_NULL(sigc->gpg_output);
+       FREE_AND_NULL(sigc->output);
        FREE_AND_NULL(sigc->gpg_status);
        FREE_AND_NULL(sigc->signer);
        FREE_AND_NULL(sigc->key);
@@ -257,16 +278,16 @@ error:
        FREE_AND_NULL(sigc->key);
 }
 
-static 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)
+static int verify_gpg_signed_buffer(struct signature_check *sigc,
+                                   struct gpg_format *fmt, const char *payload,
+                                   size_t payload_size, const char *signature,
+                                   size_t signature_size)
 {
        struct child_process gpg = CHILD_PROCESS_INIT;
-       struct gpg_format *fmt;
        struct tempfile *temp;
        int ret;
-       struct strbuf buf = STRBUF_INIT;
+       struct strbuf gpg_stdout = STRBUF_INIT;
+       struct strbuf gpg_stderr = STRBUF_INIT;
 
        temp = mks_tempfile_t(".git_vtag_tmpXXXXXX");
        if (!temp)
@@ -279,10 +300,6 @@ static int verify_signed_buffer(const char *payload, size_t payload_size,
                return -1;
        }
 
-       fmt = get_format_by_sig(signature);
-       if (!fmt)
-               BUG("bad signature '%s'", signature);
-
        strvec_push(&gpg.args, fmt->program);
        strvec_pushv(&gpg.args, fmt->verify_args);
        strvec_pushl(&gpg.args,
@@ -290,18 +307,22 @@ static int verify_signed_buffer(const char *payload, size_t payload_size,
                     "--verify", temp->filename.buf, "-",
                     NULL);
 
-       if (!gpg_status)
-               gpg_status = &buf;
-
        sigchain_push(SIGPIPE, SIG_IGN);
-       ret = pipe_command(&gpg, payload, payload_size,
-                          gpg_status, 0, gpg_output, 0);
+       ret = pipe_command(&gpg, payload, payload_size, &gpg_stdout, 0,
+                          &gpg_stderr, 0);
        sigchain_pop(SIGPIPE);
 
        delete_tempfile(&temp);
 
-       ret |= !strstr(gpg_status->buf, "\n[GNUPG:] GOODSIG ");
-       strbuf_release(&buf); /* no matter it was used or not */
+       ret |= !strstr(gpg_stdout.buf, "\n[GNUPG:] GOODSIG ");
+       sigc->payload = xmemdupz(payload, payload_size);
+       sigc->output = strbuf_detach(&gpg_stderr, NULL);
+       sigc->gpg_status = strbuf_detach(&gpg_stdout, NULL);
+
+       parse_gpg_output(sigc);
+
+       strbuf_release(&gpg_stdout);
+       strbuf_release(&gpg_stderr);
 
        return ret;
 }
@@ -309,35 +330,32 @@ static int verify_signed_buffer(const char *payload, size_t payload_size,
 int check_signature(const char *payload, size_t plen, const char *signature,
        size_t slen, struct signature_check *sigc)
 {
-       struct strbuf gpg_output = STRBUF_INIT;
-       struct strbuf gpg_status = STRBUF_INIT;
+       struct gpg_format *fmt;
        int status;
 
        sigc->result = 'N';
        sigc->trust_level = -1;
 
-       status = verify_signed_buffer(payload, plen, signature, slen,
-                                     &gpg_output, &gpg_status);
-       if (status && !gpg_output.len)
-               goto out;
-       sigc->payload = xmemdupz(payload, plen);
-       sigc->gpg_output = strbuf_detach(&gpg_output, NULL);
-       sigc->gpg_status = strbuf_detach(&gpg_status, NULL);
-       parse_gpg_output(sigc);
+       fmt = get_format_by_sig(signature);
+       if (!fmt)
+               die(_("bad/incompatible signature '%s'"), signature);
+
+       status = fmt->verify_signed_buffer(sigc, fmt, payload, plen, signature,
+                                          slen);
+
+       if (status && !sigc->output)
+               return !!status;
+
        status |= sigc->result != 'G';
        status |= sigc->trust_level < configured_min_trust_level;
 
- out:
-       strbuf_release(&gpg_status);
-       strbuf_release(&gpg_output);
-
        return !!status;
 }
 
 void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
 {
-       const char *output = flags & GPG_VERIFY_RAW ?
-               sigc->gpg_status : sigc->gpg_output;
+       const char *output = flags & GPG_VERIFY_RAW ? sigc->gpg_status :
+                                                           sigc->output;
 
        if (flags & GPG_VERIFY_VERBOSE && sigc->payload)
                fputs(sigc->payload, stdout);
@@ -441,6 +459,12 @@ const char *get_signing_key(void)
 }
 
 int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
+{
+       return use_format->sign_buffer(buffer, signature, signing_key);
+}
+
+static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,
+                         const char *signing_key)
 {
        struct child_process gpg = CHILD_PROCESS_INIT;
        int ret;

diff --git a/gpg-interface.h b/gpg-interface.h
index 80567e4894868d5d7a192cab0afb9ca9c09cb70d..feac4decf8b79f81435a87aba502b1d42e0cee6a 100644 (file)
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -17,7 +17,7 @@ enum signature_trust_level {
 
 struct signature_check {
        char *payload;
-       char *gpg_output;
+       char *output;
        char *gpg_status;
 
        /*

diff --git a/log-tree.c b/log-tree.c
index 7b823786c2cba7733df8c2977b4a3d6c0478b459..20af9bd1c8257567e5a42a6de8a27f83a0e67c92 100644 (file)
--- a/log-tree.c
+++ b/log-tree.c
@@ -513,10 +513,10 @@ static void show_signature(struct rev_info *opt, struct commit *commit)
 
        status = check_signature(payload.buf, payload.len, signature.buf,
                                 signature.len, &sigc);
-       if (status && !sigc.gpg_output)
+       if (status && !sigc.output)
                show_sig_lines(opt, status, "No signature\n");
        else
-               show_sig_lines(opt, status, sigc.gpg_output);
+               show_sig_lines(opt, status, sigc.output);
        signature_check_clear(&sigc);
 
  out:
@@ -583,8 +583,8 @@ static int show_one_mergetag(struct commit *commit,
                /* could have a good signature */
                status = check_signature(payload.buf, payload.len,
                                         signature.buf, signature.len, &sigc);
-               if (sigc.gpg_output)
-                       strbuf_addstr(&verify_message, sigc.gpg_output);
+               if (sigc.output)
+                       strbuf_addstr(&verify_message, sigc.output);
                else
                        strbuf_addstr(&verify_message, "No signature\n");
                signature_check_clear(&sigc);

diff --git a/pretty.c b/pretty.c
index b1ecd039cef29ecc77edd894c500dfadf49058c8..daa71394efd749e5170b410d5a7dc9daa6ab28d9 100644 (file)
--- a/pretty.c
+++ b/pretty.c
@@ -1432,8 +1432,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
                        check_commit_signature(c->commit, &(c->signature_check));
                switch (placeholder[1]) {
                case 'G':
-                       if (c->signature_check.gpg_output)
-                               strbuf_addstr(sb, c->signature_check.gpg_output);
+                       if (c->signature_check.output)
+                               strbuf_addstr(sb, c->signature_check.output);
                        break;
                case '?':
                        switch (c->signature_check.result) {