In our corporate environemnt we use PIV x509 Certs on Yubikeys for email
signing/encryption and ssh keys which i think is quite common
Upcase "I".
(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.
All of the above promises a wonderful new world, but what is left
unclear is with this step alone how much of the new world we already
gain. When you ask others to read and understand your code, please
give them a bit more hint to guide them what to expect and where you
are taking them next.
diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
index 0f66818e0f8..1d7b64fa021 100644
--- a/fmt-merge-msg.c
+++ b/fmt-merge-msg.c
@@ -527,10 +527,10 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
len = payload.len;
if (check_signature(payload.buf, payload.len, sig.buf,
sig.len, &sigc) &&
- !sigc.gpg_output)
+ !sigc.output)
strbuf_addstr(&sig, "gpg verification failed.\n");
else
- strbuf_addstr(&sig, sigc.gpg_output);
+ strbuf_addstr(&sig, sigc.output);
These are "rename some fields for consistency" the proposed log
message promised. Makes sense, as you are taking the sigc structure
away from pgp/gpg dependency.
diff --git a/gpg-interface.c b/gpg-interface.c
index 127aecfc2b0..3c9a48c8e7e 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -8,6 +8,7 @@
#include "tempfile.h"
static char *configured_signing_key;
+const char *ssh_allowed_signers, *ssh_revocation_file;
Very likely these want to be file-scope statics?
static enum signature_trust_level configured_min_trust_level = TRUST_UNDEFINED;
struct gpg_format {
@@ -35,6 +36,14 @@ static const char *x509_sigs[] = {
NULL
};
+static const char *ssh_verify_args[] = {
+ NULL
+};
A blank line is missing from here.
+static const char *ssh_sigs[] = {
+ "-----BEGIN SSH SIGNATURE-----",
+ NULL
+};
+
static struct gpg_format gpg_format[] = {
{ .name = "openpgp", .program = "gpg",
.verify_args = openpgp_verify_args,
@@ -44,6 +53,9 @@ static struct gpg_format gpg_format[] = {
.verify_args = x509_verify_args,
.sigs = x509_sigs
},
+ { .name = "ssh", .program = "ssh-keygen",
+ .verify_args = ssh_verify_args,
+ .sigs = ssh_sigs },
};
static struct gpg_format *use_format = &gpg_format[0];
@@ -72,7 +84,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 +269,15 @@ 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_signature(struct signature_check *sigc, struct gpg_format *fmt,
+ const char *payload, size_t payload_size,
+ const char *signature, size_t signature_size)
{
What is this hunk about? The more generic name "verify-signed-buffer"
is rescinded and gets replaced by a more GPG/PGP specific helper?
You'd need to help readers a bit more by explaining in the proposed
log message that you shifted the boundary of responsibility between
check_signature() and verify_signed_buffer()---it used to be that
the latter inspected the signed payload to see if it a valid GPG/PGP
signature before doing GPG specific validation, but you want to make
the former responsible for calling get_format_by_sig(), so that you
can dispatch a totally new backend that sits next to this GPG
specific one.
struct child_process gpg = CHILD_PROCESS_INIT;
- struct gpg_format *fmt;
struct tempfile *temp;
int ret;
- struct strbuf buf = STRBUF_INIT;
+ struct strbuf gpg_out = STRBUF_INIT;
+ struct strbuf gpg_err = STRBUF_INIT;
temp = mks_tempfile_t(".git_vtag_tmpXXXXXX");
if (!temp)
@@ -279,29 +290,28 @@ 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,
- "--status-fd=1",
- "--verify", temp->filename.buf, "-",
- NULL);
-
- if (!gpg_status)
- gpg_status = &buf;
+ "--status-fd=1",
+ "--verify", temp->filename.buf, "-",
+ NULL);
What is going on around here? Ahh, an unnecessary indentation
change is fooling the diff and made the patch unreadable. Sigh...
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_out, 0,
+ &gpg_err, 0);
What is this change about? Is it another unnecessary indentation
change? Please make sure you keep distraction to your readers to
the minimum.
@@ -309,35 +319,36 @@ 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) {
+ error(_("bad/incompatible signature '%s'"), signature);
+ return -1;
+ }
+
+ if (!strcmp(fmt->name, "ssh")) {
+ status = verify_ssh_signature(sigc, fmt, payload, plen, signature, slen);
+ } else {
+ status = verify_gpg_signature(sigc, fmt, payload, plen, signature, slen);
+ }
OK, so get_format_by_sig() now is used to dispatch to the right
backend. Which sort of makes sense, but ...
* "ssh" is the newcomer; it has no right to come before the
battle-tested existing one.
* If we are dispatching via "fmt" variable, we should add
fmt->verify() method to each of these formats, so that we don't
have to switch based on the name.
IOW, this part should just be
fmt = get_format_by_sig(signature);
if (!fmt)
return error(_("...bad signature..."));
fmt->verify_signature(sigc, fmt, payload, plen, signature, slen);
