From c2223851649c88a4723b79e6c2fe87efe6feffc1 Mon Sep 17 00:00:00 2001 From: Fabian Stelzer Date: Tue, 3 Aug 2021 13:45:50 +0000 Subject: [PATCH 01/23] 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 Signed-off-by: Junio C Hamano --- fmt-merge-msg.c | 6 +-- gpg-interface.c | 104 +++++++++++++++++++++++++++++------------------- gpg-interface.h | 2 +- log-tree.c | 8 ++-- pretty.c | 4 +- 5 files changed, 74 insertions(+), 50 deletions(-) diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c index 0f66818e0f839f..fb300bb4b67861 100644 --- 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 127aecfc2b071f..db54b05416257d 100644 --- 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 @@ static void parse_gpg_output(struct signature_check *sigc) 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 80567e4894868d..feac4decf8b79f 100644 --- 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 7b823786c2cba7..20af9bd1c82575 100644 --- 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 b1ecd039cef29e..daa71394efd749 100644 --- 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) { From 3a3fdc0b4ea60f884f3bdb18058111e47014c998 Mon Sep 17 00:00:00 2001 From: Fabian Stelzer Date: Tue, 3 Aug 2021 13:45:51 +0000 Subject: [PATCH 02/23] ssh signing: add test prereqs Generate some ssh keys and a allowedSignersFile for testing Signed-off-by: Fabian Stelzer Signed-off-by: Junio C Hamano --- t/lib-gpg.sh | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh index 9fc5241228e800..f99ef3e859dd43 100644 --- a/t/lib-gpg.sh +++ b/t/lib-gpg.sh @@ -87,6 +87,34 @@ test_lazy_prereq RFC1991 ' echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991 >/dev/null ' +GPGSSH_KEY_PRIMARY="${GNUPGHOME}/ed25519_ssh_signing_key" +GPGSSH_KEY_SECONDARY="${GNUPGHOME}/rsa_2048_ssh_signing_key" +GPGSSH_KEY_UNTRUSTED="${GNUPGHOME}/untrusted_ssh_signing_key" +GPGSSH_KEY_WITH_PASSPHRASE="${GNUPGHOME}/protected_ssh_signing_key" +GPGSSH_KEY_PASSPHRASE="super_secret" +GPGSSH_ALLOWED_SIGNERS="${GNUPGHOME}/ssh.all_valid.allowedSignersFile" + +GPGSSH_GOOD_SIGNATURE_TRUSTED='Good "git" signature for' +GPGSSH_GOOD_SIGNATURE_UNTRUSTED='Good "git" signature with' +GPGSSH_KEY_NOT_TRUSTED="No principal matched" +GPGSSH_BAD_SIGNATURE="Signature verification failed" + +test_lazy_prereq GPGSSH ' + ssh_version=$(ssh-keygen -Y find-principals -n "git" 2>&1) + test $? != 127 || exit 1 + echo $ssh_version | grep -q "find-principals:missing signature file" + test $? = 0 || exit 1; + mkdir -p "${GNUPGHOME}" && + chmod 0700 "${GNUPGHOME}" && + ssh-keygen -t ed25519 -N "" -C "git ed25519 key" -f "${GPGSSH_KEY_PRIMARY}" >/dev/null && + echo "\"principal with number 1\" $(cat "${GPGSSH_KEY_PRIMARY}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" && + ssh-keygen -t rsa -b 2048 -N "" -C "git rsa2048 key" -f "${GPGSSH_KEY_SECONDARY}" >/dev/null && + echo "\"principal with number 2\" $(cat "${GPGSSH_KEY_SECONDARY}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" && + ssh-keygen -t ed25519 -N "${GPGSSH_KEY_PASSPHRASE}" -C "git ed25519 encrypted key" -f "${GPGSSH_KEY_WITH_PASSPHRASE}" >/dev/null && + echo "\"principal with number 3\" $(cat "${GPGSSH_KEY_WITH_PASSPHRASE}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" && + ssh-keygen -t ed25519 -N "" -f "${GPGSSH_KEY_UNTRUSTED}" >/dev/null +' + sanitize_pgp() { perl -ne ' /^-----END PGP/ and $in_pgp = 0; From c7e2d30efec488ff34afa5560c3f0fa364d04fbf Mon Sep 17 00:00:00 2001 From: Fabian Stelzer Date: Tue, 3 Aug 2021 13:45:52 +0000 Subject: [PATCH 03/23] ssh signing: add ssh key format and signing code Implements the actual sign_buffer_ssh operation and move some shared cleanup code into a strbuf function Set gpg.format = ssh and user.signingkey to either a ssh public key string (like from an authorized_keys file), or a ssh key file. If the key file or the config value itself contains only a public key then the private key needs to be available via ssh-agent. gpg.ssh.program can be set to an alternative location of ssh-keygen. A somewhat recent openssh version (8.2p1+) of ssh-keygen is needed for this feature. Since only ssh-keygen is needed it can this way be installed seperately without upgrading your system openssh packages. Signed-off-by: Fabian Stelzer Signed-off-by: Junio C Hamano --- Documentation/config/gpg.txt | 4 +- Documentation/config/user.txt | 5 ++ gpg-interface.c | 138 ++++++++++++++++++++++++++++++++-- 3 files changed, 137 insertions(+), 10 deletions(-) diff --git a/Documentation/config/gpg.txt b/Documentation/config/gpg.txt index d94025cb3684d8..88531b15f0ff32 100644 --- a/Documentation/config/gpg.txt +++ b/Documentation/config/gpg.txt @@ -11,13 +11,13 @@ gpg.program:: gpg.format:: Specifies which key format to use when signing with `--gpg-sign`. - Default is "openpgp" and another possible value is "x509". + Default is "openpgp". Other possible values are "x509", "ssh". gpg..program:: Use this to customize the program used for the signing format you chose. (see `gpg.program` and `gpg.format`) `gpg.program` can still be used as a legacy synonym for `gpg.openpgp.program`. The default - value for `gpg.x509.program` is "gpgsm". + value for `gpg.x509.program` is "gpgsm" and `gpg.ssh.program` is "ssh-keygen". gpg.minTrustLevel:: Specifies a minimum trust level for signature verification. If diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt index 59aec7c3aed32a..2155128957c93e 100644 --- a/Documentation/config/user.txt +++ b/Documentation/config/user.txt @@ -36,3 +36,8 @@ user.signingKey:: commit, you can override the default selection with this variable. This option is passed unchanged to gpg's --local-user parameter, so you may specify a key using any method that gpg supports. + If gpg.format is set to "ssh" this can contain the literal ssh public + key (e.g.: "ssh-rsa XXXXXX identifier") or a file which contains it and + corresponds to the private key used for signing. The private key + needs to be available via ssh-agent. Alternatively it can be set to + a file containing a private key directly. diff --git a/gpg-interface.c b/gpg-interface.c index db54b05416257d..7ca682ac6d6849 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -41,12 +41,20 @@ static const char *x509_sigs[] = { NULL }; +static const char *ssh_verify_args[] = { NULL }; +static const char *ssh_sigs[] = { + "-----BEGIN SSH SIGNATURE-----", + 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 int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature, + const char *signing_key); static struct gpg_format gpg_format[] = { { @@ -65,6 +73,14 @@ static struct gpg_format gpg_format[] = { .verify_signed_buffer = verify_gpg_signed_buffer, .sign_buffer = sign_buffer_gpg, }, + { + .name = "ssh", + .program = "ssh-keygen", + .verify_args = ssh_verify_args, + .sigs = ssh_sigs, + .verify_signed_buffer = NULL, /* TODO */ + .sign_buffer = sign_buffer_ssh + }, }; static struct gpg_format *use_format = &gpg_format[0]; @@ -443,6 +459,9 @@ int git_gpg_config(const char *var, const char *value, void *cb) if (!strcmp(var, "gpg.x509.program")) fmtname = "x509"; + if (!strcmp(var, "gpg.ssh.program")) + fmtname = "ssh"; + if (fmtname) { fmt = get_format_by_name(fmtname); return git_config_string(&fmt->program, var, value); @@ -463,12 +482,30 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig return use_format->sign_buffer(buffer, signature, signing_key); } +/* + * Strip CR from the line endings, in case we are on Windows. + * NEEDSWORK: make it trim only CRs before LFs and rename + */ +static void remove_cr_after(struct strbuf *buffer, size_t offset) +{ + size_t i, j; + + for (i = j = offset; i < buffer->len; i++) { + if (buffer->buf[i] != '\r') { + if (i != j) + buffer->buf[j] = buffer->buf[i]; + j++; + } + } + strbuf_setlen(buffer, j); +} + static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature, const char *signing_key) { struct child_process gpg = CHILD_PROCESS_INIT; int ret; - size_t i, j, bottom; + size_t bottom; struct strbuf gpg_status = STRBUF_INIT; strvec_pushl(&gpg.args, @@ -494,13 +531,98 @@ static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature, return error(_("gpg failed to sign the data")); /* Strip CR from the line endings, in case we are on Windows. */ - for (i = j = bottom; i < signature->len; i++) - if (signature->buf[i] != '\r') { - if (i != j) - signature->buf[j] = signature->buf[i]; - j++; - } - strbuf_setlen(signature, j); + remove_cr_after(signature, bottom); return 0; } + +static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature, + const char *signing_key) +{ + struct child_process signer = CHILD_PROCESS_INIT; + int ret = -1; + size_t bottom, keylen; + struct strbuf signer_stderr = STRBUF_INIT; + struct tempfile *key_file = NULL, *buffer_file = NULL; + char *ssh_signing_key_file = NULL; + struct strbuf ssh_signature_filename = STRBUF_INIT; + + if (!signing_key || signing_key[0] == '\0') + return error( + _("user.signingkey needs to be set for ssh signing")); + + if (starts_with(signing_key, "ssh-")) { + /* A literal ssh key */ + key_file = mks_tempfile_t(".git_signing_key_tmpXXXXXX"); + if (!key_file) + return error_errno( + _("could not create temporary file")); + keylen = strlen(signing_key); + if (write_in_full(key_file->fd, signing_key, keylen) < 0 || + close_tempfile_gently(key_file) < 0) { + error_errno(_("failed writing ssh signing key to '%s'"), + key_file->filename.buf); + goto out; + } + ssh_signing_key_file = strbuf_detach(&key_file->filename, NULL); + } else { + /* We assume a file */ + ssh_signing_key_file = expand_user_path(signing_key, 1); + } + + buffer_file = mks_tempfile_t(".git_signing_buffer_tmpXXXXXX"); + if (!buffer_file) { + error_errno(_("could not create temporary file")); + goto out; + } + + if (write_in_full(buffer_file->fd, buffer->buf, buffer->len) < 0 || + close_tempfile_gently(buffer_file) < 0) { + error_errno(_("failed writing ssh signing key buffer to '%s'"), + buffer_file->filename.buf); + goto out; + } + + strvec_pushl(&signer.args, use_format->program, + "-Y", "sign", + "-n", "git", + "-f", ssh_signing_key_file, + buffer_file->filename.buf, + NULL); + + sigchain_push(SIGPIPE, SIG_IGN); + ret = pipe_command(&signer, NULL, 0, NULL, 0, &signer_stderr, 0); + sigchain_pop(SIGPIPE); + + if (ret) { + if (strstr(signer_stderr.buf, "usage:")) + error(_("ssh-keygen -Y sign is needed for ssh signing (available in openssh version 8.2p1+)")); + + error("%s", signer_stderr.buf); + goto out; + } + + bottom = signature->len; + + strbuf_addbuf(&ssh_signature_filename, &buffer_file->filename); + strbuf_addstr(&ssh_signature_filename, ".sig"); + if (strbuf_read_file(signature, ssh_signature_filename.buf, 0) < 0) { + error_errno( + _("failed reading ssh signing data buffer from '%s'"), + ssh_signature_filename.buf); + } + unlink_or_warn(ssh_signature_filename.buf); + + /* Strip CR from the line endings, in case we are on Windows. */ + remove_cr_after(signature, bottom); + +out: + if (key_file) + delete_tempfile(&key_file); + if (buffer_file) + delete_tempfile(&buffer_file); + strbuf_release(&signer_stderr); + strbuf_release(&ssh_signature_filename); + FREE_AND_NULL(ssh_signing_key_file); + return ret; +} From 54937221225569332b92f6cd7876d4fa08dc3277 Mon Sep 17 00:00:00 2001 From: Fabian Stelzer Date: Tue, 3 Aug 2021 13:45:53 +0000 Subject: [PATCH 04/23] ssh signing: retrieve a default key from ssh-agent If user.signingkey is not set and a ssh signature is requested we call gpg.ssh.defaultKeyCommand (typically "ssh-add -L") and use the first key we get Signed-off-by: Fabian Stelzer Signed-off-by: Junio C Hamano --- Documentation/config/gpg.txt | 6 +++ Documentation/config/user.txt | 4 +- gpg-interface.c | 70 ++++++++++++++++++++++++++++++++++- 3 files changed, 77 insertions(+), 3 deletions(-) diff --git a/Documentation/config/gpg.txt b/Documentation/config/gpg.txt index 88531b15f0ff32..9b95dd280c3749 100644 --- a/Documentation/config/gpg.txt +++ b/Documentation/config/gpg.txt @@ -33,3 +33,9 @@ gpg.minTrustLevel:: * `marginal` * `fully` * `ultimate` + +gpg.ssh.defaultKeyCommand: + This command that will be run when user.signingkey is not set and a ssh + signature is requested. On successful exit a valid ssh public key is + expected in the first line of its output. To automatically use the first + available key from your ssh-agent set this to "ssh-add -L". diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt index 2155128957c93e..ad78dce9ecbfc6 100644 --- a/Documentation/config/user.txt +++ b/Documentation/config/user.txt @@ -40,4 +40,6 @@ user.signingKey:: key (e.g.: "ssh-rsa XXXXXX identifier") or a file which contains it and corresponds to the private key used for signing. The private key needs to be available via ssh-agent. Alternatively it can be set to - a file containing a private key directly. + a file containing a private key directly. If not set git will call + gpg.ssh.defaultKeyCommand (e.g.: "ssh-add -L") and try to use the first + key available. diff --git a/gpg-interface.c b/gpg-interface.c index 7ca682ac6d6849..3a0cca1b1d2109 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -6,8 +6,10 @@ #include "gpg-interface.h" #include "sigchain.h" #include "tempfile.h" +#include "alias.h" static char *configured_signing_key; +static const char *ssh_default_key_command; static enum signature_trust_level configured_min_trust_level = TRUST_UNDEFINED; struct gpg_format { @@ -21,6 +23,7 @@ struct gpg_format { size_t signature_size); int (*sign_buffer)(struct strbuf *buffer, struct strbuf *signature, const char *signing_key); + const char *(*get_default_key)(void); }; static const char *openpgp_verify_args[] = { @@ -56,6 +59,8 @@ static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature, static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature, const char *signing_key); +static const char *get_default_ssh_signing_key(void); + static struct gpg_format gpg_format[] = { { .name = "openpgp", @@ -64,6 +69,7 @@ static struct gpg_format gpg_format[] = { .sigs = openpgp_sigs, .verify_signed_buffer = verify_gpg_signed_buffer, .sign_buffer = sign_buffer_gpg, + .get_default_key = NULL, }, { .name = "x509", @@ -72,6 +78,7 @@ static struct gpg_format gpg_format[] = { .sigs = x509_sigs, .verify_signed_buffer = verify_gpg_signed_buffer, .sign_buffer = sign_buffer_gpg, + .get_default_key = NULL, }, { .name = "ssh", @@ -79,7 +86,8 @@ static struct gpg_format gpg_format[] = { .verify_args = ssh_verify_args, .sigs = ssh_sigs, .verify_signed_buffer = NULL, /* TODO */ - .sign_buffer = sign_buffer_ssh + .sign_buffer = sign_buffer_ssh, + .get_default_key = get_default_ssh_signing_key, }, }; @@ -453,6 +461,12 @@ int git_gpg_config(const char *var, const char *value, void *cb) return 0; } + if (!strcmp(var, "gpg.ssh.defaultkeycommand")) { + if (!value) + return config_error_nonbool(var); + return git_config_string(&ssh_default_key_command, var, value); + } + if (!strcmp(var, "gpg.program") || !strcmp(var, "gpg.openpgp.program")) fmtname = "openpgp"; @@ -470,11 +484,63 @@ int git_gpg_config(const char *var, const char *value, void *cb) return 0; } +/* Returns the first public key from an ssh-agent to use for signing */ +static const char *get_default_ssh_signing_key(void) +{ + struct child_process ssh_default_key = CHILD_PROCESS_INIT; + int ret = -1; + struct strbuf key_stdout = STRBUF_INIT, key_stderr = STRBUF_INIT; + struct strbuf **keys; + char *key_command = NULL; + const char **argv; + int n; + char *default_key = NULL; + + if (!ssh_default_key_command) + die(_("either user.signingkey or gpg.ssh.defaultKeyCommand needs to be configured")); + + key_command = xstrdup(ssh_default_key_command); + n = split_cmdline(key_command, &argv); + + if (n < 0) + die("malformed build-time gpg.ssh.defaultKeyCommand: %s", + split_cmdline_strerror(n)); + + strvec_pushv(&ssh_default_key.args, argv); + ret = pipe_command(&ssh_default_key, NULL, 0, &key_stdout, 0, + &key_stderr, 0); + + if (!ret) { + keys = strbuf_split_max(&key_stdout, '\n', 2); + if (keys[0] && starts_with(keys[0]->buf, "ssh-")) { + default_key = strbuf_detach(keys[0], NULL); + } else { + warning(_("gpg.ssh.defaultKeycommand succeeded but returned no keys: %s %s"), + key_stderr.buf, key_stdout.buf); + } + + strbuf_list_free(keys); + } else { + warning(_("gpg.ssh.defaultKeyCommand failed: %s %s"), + key_stderr.buf, key_stdout.buf); + } + + free(key_command); + free(argv); + strbuf_release(&key_stdout); + + return default_key; +} + const char *get_signing_key(void) { if (configured_signing_key) return configured_signing_key; - return git_committer_info(IDENT_STRICT|IDENT_NO_DATE); + if (use_format->get_default_key) { + return use_format->get_default_key(); + } + + return git_committer_info(IDENT_STRICT | IDENT_NO_DATE); } int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key) From 6869f1f60c1e9c9fa226d8d67ce11df904aaa963 Mon Sep 17 00:00:00 2001 From: Fabian Stelzer Date: Tue, 3 Aug 2021 13:45:54 +0000 Subject: [PATCH 05/23] ssh signing: provide a textual signing_key_id For ssh the user.signingkey can be a filename/path or even a literal ssh pubkey. In push certs and textual output we prefer the ssh fingerprint instead. Signed-off-by: Fabian Stelzer Signed-off-by: Junio C Hamano --- gpg-interface.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++ gpg-interface.h | 6 ++++++ send-pack.c | 8 +++---- 3 files changed, 66 insertions(+), 4 deletions(-) diff --git a/gpg-interface.c b/gpg-interface.c index 3a0cca1b1d2109..0f1c6a02e53257 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -24,6 +24,7 @@ struct gpg_format { int (*sign_buffer)(struct strbuf *buffer, struct strbuf *signature, const char *signing_key); const char *(*get_default_key)(void); + const char *(*get_key_id)(void); }; static const char *openpgp_verify_args[] = { @@ -61,6 +62,8 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature, static const char *get_default_ssh_signing_key(void); +static const char *get_ssh_key_id(void); + static struct gpg_format gpg_format[] = { { .name = "openpgp", @@ -70,6 +73,7 @@ static struct gpg_format gpg_format[] = { .verify_signed_buffer = verify_gpg_signed_buffer, .sign_buffer = sign_buffer_gpg, .get_default_key = NULL, + .get_key_id = NULL, }, { .name = "x509", @@ -79,6 +83,7 @@ static struct gpg_format gpg_format[] = { .verify_signed_buffer = verify_gpg_signed_buffer, .sign_buffer = sign_buffer_gpg, .get_default_key = NULL, + .get_key_id = NULL, }, { .name = "ssh", @@ -88,6 +93,7 @@ static struct gpg_format gpg_format[] = { .verify_signed_buffer = NULL, /* TODO */ .sign_buffer = sign_buffer_ssh, .get_default_key = get_default_ssh_signing_key, + .get_key_id = get_ssh_key_id, }, }; @@ -484,6 +490,41 @@ int git_gpg_config(const char *var, const char *value, void *cb) return 0; } +static char *get_ssh_key_fingerprint(const char *signing_key) +{ + struct child_process ssh_keygen = CHILD_PROCESS_INIT; + int ret = -1; + struct strbuf fingerprint_stdout = STRBUF_INIT; + struct strbuf **fingerprint; + + /* + * With SSH Signing this can contain a filename or a public key + * For textual representation we usually want a fingerprint + */ + if (starts_with(signing_key, "ssh-")) { + strvec_pushl(&ssh_keygen.args, "ssh-keygen", "-lf", "-", NULL); + ret = pipe_command(&ssh_keygen, signing_key, + strlen(signing_key), &fingerprint_stdout, 0, + NULL, 0); + } else { + strvec_pushl(&ssh_keygen.args, "ssh-keygen", "-lf", + configured_signing_key, NULL); + ret = pipe_command(&ssh_keygen, NULL, 0, &fingerprint_stdout, 0, + NULL, 0); + } + + if (!!ret) + die_errno(_("failed to get the ssh fingerprint for key '%s'"), + signing_key); + + fingerprint = strbuf_split_max(&fingerprint_stdout, ' ', 3); + if (!fingerprint[1]) + die_errno(_("failed to get the ssh fingerprint for key '%s'"), + signing_key); + + return strbuf_detach(fingerprint[1], NULL); +} + /* Returns the first public key from an ssh-agent to use for signing */ static const char *get_default_ssh_signing_key(void) { @@ -532,6 +573,21 @@ static const char *get_default_ssh_signing_key(void) return default_key; } +static const char *get_ssh_key_id(void) { + return get_ssh_key_fingerprint(get_signing_key()); +} + +/* Returns a textual but unique representation of the signing key */ +const char *get_signing_key_id(void) +{ + if (use_format->get_key_id) { + return use_format->get_key_id(); + } + + /* GPG/GPGSM only store a key id on this variable */ + return get_signing_key(); +} + const char *get_signing_key(void) { if (configured_signing_key) diff --git a/gpg-interface.h b/gpg-interface.h index feac4decf8b79f..beefacbb1e9025 100644 --- a/gpg-interface.h +++ b/gpg-interface.h @@ -64,6 +64,12 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, int git_gpg_config(const char *, const char *, void *); void set_signing_key(const char *); const char *get_signing_key(void); + +/* + * Returns a textual unique representation of the signing key in use + * Either a GPG KeyID or a SSH Key Fingerprint + */ +const char *get_signing_key_id(void); int check_signature(const char *payload, size_t plen, const char *signature, size_t slen, struct signature_check *sigc); diff --git a/send-pack.c b/send-pack.c index 5a79e0e7110319..50cca7e439b34b 100644 --- a/send-pack.c +++ b/send-pack.c @@ -341,13 +341,13 @@ static int generate_push_cert(struct strbuf *req_buf, { const struct ref *ref; struct string_list_item *item; - char *signing_key = xstrdup(get_signing_key()); + char *signing_key_id = xstrdup(get_signing_key_id()); const char *cp, *np; struct strbuf cert = STRBUF_INIT; int update_seen = 0; strbuf_addstr(&cert, "certificate version 0.1\n"); - strbuf_addf(&cert, "pusher %s ", signing_key); + strbuf_addf(&cert, "pusher %s ", signing_key_id); datestamp(&cert); strbuf_addch(&cert, '\n'); if (args->url && *args->url) { @@ -374,7 +374,7 @@ static int generate_push_cert(struct strbuf *req_buf, if (!update_seen) goto free_return; - if (sign_buffer(&cert, &cert, signing_key)) + if (sign_buffer(&cert, &cert, get_signing_key())) die(_("failed to sign the push certificate")); packet_buf_write(req_buf, "push-cert%c%s", 0, cap_string); @@ -386,7 +386,7 @@ static int generate_push_cert(struct strbuf *req_buf, packet_buf_write(req_buf, "push-cert-end\n"); free_return: - free(signing_key); + free(signing_key_id); strbuf_release(&cert); return update_seen; } From 9048bb3c9b829cb19298532e960a1e735b2f1b18 Mon Sep 17 00:00:00 2001 From: Fabian Stelzer Date: Tue, 3 Aug 2021 13:45:55 +0000 Subject: [PATCH 06/23] ssh signing: verify signatures using ssh-keygen To verify a ssh signature we first call ssh-keygen -Y find-principal to look up the signing principal by their public key from the allowedSignersFile. If the key is found then we do a verify. Otherwise we only validate the signature but can not verify the signers identity. Verification uses the gpg.ssh.allowedSignersFile (see ssh-keygen(1) "ALLOWED SIGNERS") which contains valid public keys and a principal (usually user@domain). Depending on the environment this file can be managed by the individual developer or for example generated by the central repository server from known ssh keys with push access. This file is usually stored outside the repository, but if the repository only allows signed commits/pushes, the user might choose to store it in the repository. To revoke a key put the public key without the principal prefix into gpg.ssh.revocationKeyring or generate a KRL (see ssh-keygen(1) "KEY REVOCATION LISTS"). The same considerations about who to trust for verification as with the allowedSignersFile apply. Using SSH CA Keys with these files is also possible. Add "cert-authority" as key option between the principal and the key to mark it as a CA and all keys signed by it as valid for this CA. See "CERTIFICATES" in ssh-keygen(1). Signed-off-by: Fabian Stelzer Signed-off-by: Junio C Hamano --- Documentation/config/gpg.txt | 35 ++++++ builtin/receive-pack.c | 4 + gpg-interface.c | 209 ++++++++++++++++++++++++++++++++++- 3 files changed, 246 insertions(+), 2 deletions(-) diff --git a/Documentation/config/gpg.txt b/Documentation/config/gpg.txt index 9b95dd280c3749..51a756b2f15f85 100644 --- a/Documentation/config/gpg.txt +++ b/Documentation/config/gpg.txt @@ -39,3 +39,38 @@ gpg.ssh.defaultKeyCommand: signature is requested. On successful exit a valid ssh public key is expected in the first line of its output. To automatically use the first available key from your ssh-agent set this to "ssh-add -L". + +gpg.ssh.allowedSignersFile:: + A file containing ssh public keys which you are willing to trust. + The file consists of one or more lines of principals followed by an ssh + public key. + e.g.: user1@example.com,user2@example.com ssh-rsa AAAAX1... + See ssh-keygen(1) "ALLOWED SIGNERS" for details. + The principal is only used to identify the key and is available when + verifying a signature. ++ +SSH has no concept of trust levels like gpg does. To be able to differentiate +between valid signatures and trusted signatures the trust level of a signature +verification is set to `fully` when the public key is present in the allowedSignersFile. +Therefore to only mark fully trusted keys as verified set gpg.minTrustLevel to `fully`. +Otherwise valid but untrusted signatures will still verify but show no principal +name of the signer. ++ +This file can be set to a location outside of the repository and every developer +maintains their own trust store. A central repository server could generate this +file automatically from ssh keys with push access to verify the code against. +In a corporate setting this file is probably generated at a global location +from automation that already handles developer ssh keys. ++ +A repository that only allows signed commits can store the file +in the repository itself using a path relative to the top-level of the working tree. +This way only committers with an already valid key can add or change keys in the keyring. ++ +Using a SSH CA key with the cert-authority option +(see ssh-keygen(1) "CERTIFICATES") is also valid. + +gpg.ssh.revocationFile:: + Either a SSH KRL or a list of revoked public keys (without the principal prefix). + See ssh-keygen(1) for details. + If a public key is found in this file then it will always be treated + as having trust level "never" and signatures will show as invalid. diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index a34742513aca7e..f17c7d2246b031 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -131,6 +131,10 @@ static int receive_pack_config(const char *var, const char *value, void *cb) { int status = parse_hide_refs_config(var, value, "receive"); + if (status) + return status; + + status = git_gpg_config(var, value, NULL); if (status) return status; diff --git a/gpg-interface.c b/gpg-interface.c index 0f1c6a02e53257..9c1ef11a563f3f 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -3,13 +3,14 @@ #include "config.h" #include "run-command.h" #include "strbuf.h" +#include "dir.h" #include "gpg-interface.h" #include "sigchain.h" #include "tempfile.h" #include "alias.h" static char *configured_signing_key; -static const char *ssh_default_key_command; +static const char *ssh_default_key_command, *ssh_allowed_signers, *ssh_revocation_file; static enum signature_trust_level configured_min_trust_level = TRUST_UNDEFINED; struct gpg_format { @@ -55,6 +56,10 @@ 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 verify_ssh_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 int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature, @@ -90,7 +95,7 @@ static struct gpg_format gpg_format[] = { .program = "ssh-keygen", .verify_args = ssh_verify_args, .sigs = ssh_sigs, - .verify_signed_buffer = NULL, /* TODO */ + .verify_signed_buffer = verify_ssh_signed_buffer, .sign_buffer = sign_buffer_ssh, .get_default_key = get_default_ssh_signing_key, .get_key_id = get_ssh_key_id, @@ -357,6 +362,194 @@ static int verify_gpg_signed_buffer(struct signature_check *sigc, return ret; } +static void parse_ssh_output(struct signature_check *sigc) +{ + const char *line, *principal, *search; + char *key = NULL; + + /* + * ssh-keygen output should be: + * Good "git" signature for PRINCIPAL with RSA key SHA256:FINGERPRINT + * + * or for valid but unknown keys: + * Good "git" signature with RSA key SHA256:FINGERPRINT + * + * Note that "PRINCIPAL" can contain whitespace, "RSA" and + * "SHA256" part could be a different token that names of + * the algorithms used, and "FINGERPRINT" is a hexadecimal + * string. By finding the last occurence of " with ", we can + * reliably parse out the PRINCIPAL. + */ + sigc->result = 'B'; + sigc->trust_level = TRUST_NEVER; + + line = xmemdupz(sigc->output, strcspn(sigc->output, "\n")); + + if (skip_prefix(line, "Good \"git\" signature for ", &line)) { + /* Valid signature and known principal */ + sigc->result = 'G'; + sigc->trust_level = TRUST_FULLY; + + /* Search for the last "with" to get the full principal */ + principal = line; + do { + search = strstr(line, " with "); + if (search) + line = search + 1; + } while (search != NULL); + sigc->signer = xmemdupz(principal, line - principal - 1); + } else if (skip_prefix(line, "Good \"git\" signature with ", &line)) { + /* Valid signature, but key unknown */ + sigc->result = 'G'; + sigc->trust_level = TRUST_UNDEFINED; + } else { + return; + } + + key = strstr(line, "key"); + if (key) { + sigc->fingerprint = xstrdup(strstr(line, "key") + 4); + sigc->key = xstrdup(sigc->fingerprint); + } else { + /* + * Output did not match what we expected + * Treat the signature as bad + */ + sigc->result = 'B'; + } +} + +static int verify_ssh_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 ssh_keygen = CHILD_PROCESS_INIT; + struct tempfile *buffer_file; + int ret = -1; + const char *line; + size_t trust_size; + char *principal; + struct strbuf ssh_keygen_out = STRBUF_INIT; + struct strbuf ssh_keygen_err = STRBUF_INIT; + + if (!ssh_allowed_signers) { + error(_("gpg.ssh.allowedSignersFile needs to be configured and exist for ssh signature verification")); + return -1; + } + + buffer_file = mks_tempfile_t(".git_vtag_tmpXXXXXX"); + if (!buffer_file) + return error_errno(_("could not create temporary file")); + if (write_in_full(buffer_file->fd, signature, signature_size) < 0 || + close_tempfile_gently(buffer_file) < 0) { + error_errno(_("failed writing detached signature to '%s'"), + buffer_file->filename.buf); + delete_tempfile(&buffer_file); + return -1; + } + + /* Find the principal from the signers */ + strvec_pushl(&ssh_keygen.args, fmt->program, + "-Y", "find-principals", + "-f", ssh_allowed_signers, + "-s", buffer_file->filename.buf, + NULL); + ret = pipe_command(&ssh_keygen, NULL, 0, &ssh_keygen_out, 0, + &ssh_keygen_err, 0); + if (ret && strstr(ssh_keygen_err.buf, "usage:")) { + error(_("ssh-keygen -Y find-principals/verify is needed for ssh signature verification (available in openssh version 8.2p1+)")); + goto out; + } + if (ret || !ssh_keygen_out.len) { + /* + * We did not find a matching principal in the allowedSigners + * Check without validation + */ + child_process_init(&ssh_keygen); + strvec_pushl(&ssh_keygen.args, fmt->program, + "-Y", "check-novalidate", + "-n", "git", + "-s", buffer_file->filename.buf, + NULL); + pipe_command(&ssh_keygen, payload, payload_size, + &ssh_keygen_out, 0, &ssh_keygen_err, 0); + + /* + * Fail on unknown keys + * we still call check-novalidate to display the signature info + */ + ret = -1; + } else { + /* Check every principal we found (one per line) */ + for (line = ssh_keygen_out.buf; *line; + line = strchrnul(line + 1, '\n')) { + while (*line == '\n') + line++; + if (!*line) + break; + + trust_size = strcspn(line, "\n"); + principal = xmemdupz(line, trust_size); + + child_process_init(&ssh_keygen); + strbuf_release(&ssh_keygen_out); + strbuf_release(&ssh_keygen_err); + strvec_push(&ssh_keygen.args, fmt->program); + /* + * We found principals + * Try with each until we find a match + */ + strvec_pushl(&ssh_keygen.args, "-Y", "verify", + "-n", "git", + "-f", ssh_allowed_signers, + "-I", principal, + "-s", buffer_file->filename.buf, + NULL); + + if (ssh_revocation_file) { + if (file_exists(ssh_revocation_file)) { + strvec_pushl(&ssh_keygen.args, "-r", + ssh_revocation_file, NULL); + } else { + warning(_("ssh signing revocation file configured but not found: %s"), + ssh_revocation_file); + } + } + + sigchain_push(SIGPIPE, SIG_IGN); + ret = pipe_command(&ssh_keygen, payload, payload_size, + &ssh_keygen_out, 0, &ssh_keygen_err, 0); + sigchain_pop(SIGPIPE); + + FREE_AND_NULL(principal); + + if (!ret) + ret = !starts_with(ssh_keygen_out.buf, "Good"); + + if (!ret) + break; + } + } + + sigc->payload = xmemdupz(payload, payload_size); + strbuf_stripspace(&ssh_keygen_out, 0); + strbuf_stripspace(&ssh_keygen_err, 0); + strbuf_add(&ssh_keygen_out, ssh_keygen_err.buf, ssh_keygen_err.len); + sigc->output = strbuf_detach(&ssh_keygen_out, NULL); + sigc->gpg_status = xstrdup(sigc->output); + + parse_ssh_output(sigc); + +out: + if (buffer_file) + delete_tempfile(&buffer_file); + strbuf_release(&ssh_keygen_out); + strbuf_release(&ssh_keygen_err); + + return ret; +} + int check_signature(const char *payload, size_t plen, const char *signature, size_t slen, struct signature_check *sigc) { @@ -473,6 +666,18 @@ int git_gpg_config(const char *var, const char *value, void *cb) return git_config_string(&ssh_default_key_command, var, value); } + if (!strcmp(var, "gpg.ssh.allowedsignersfile")) { + if (!value) + return config_error_nonbool(var); + return git_config_string(&ssh_allowed_signers, var, value); + } + + if (!strcmp(var, "gpg.ssh.revocationfile")) { + if (!value) + return config_error_nonbool(var); + return git_config_string(&ssh_revocation_file, var, value); + } + if (!strcmp(var, "gpg.program") || !strcmp(var, "gpg.openpgp.program")) fmtname = "openpgp"; From 587967698ab2d679a53f79162a35fda6e2eaac08 Mon Sep 17 00:00:00 2001 From: Fabian Stelzer Date: Tue, 3 Aug 2021 13:45:56 +0000 Subject: [PATCH 07/23] ssh signing: duplicate t7510 tests for commits Signed-off-by: Fabian Stelzer Signed-off-by: Junio C Hamano --- t/t7528-signed-commit-ssh.sh | 398 +++++++++++++++++++++++++++++++++++ 1 file changed, 398 insertions(+) create mode 100755 t/t7528-signed-commit-ssh.sh diff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh new file mode 100755 index 00000000000000..3e093168eef571 --- /dev/null +++ b/t/t7528-signed-commit-ssh.sh @@ -0,0 +1,398 @@ +#!/bin/sh + +test_description='ssh signed commit tests' +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + +. ./test-lib.sh +GNUPGHOME_NOT_USED=$GNUPGHOME +. "$TEST_DIRECTORY/lib-gpg.sh" + +test_expect_success GPGSSH 'create signed commits' ' + test_oid_cache <<-\EOF && + header sha1:gpgsig + header sha256:gpgsig-sha256 + EOF + + test_when_finished "test_unconfig commit.gpgsign" && + test_config gpg.format ssh && + test_config user.signingkey "${GPGSSH_KEY_PRIMARY}" && + + echo 1 >file && git add file && + test_tick && git commit -S -m initial && + git tag initial && + git branch side && + + echo 2 >file && test_tick && git commit -a -S -m second && + git tag second && + + git checkout side && + echo 3 >elif && git add elif && + test_tick && git commit -m "third on side" && + + git checkout main && + test_tick && git merge -S side && + git tag merge && + + echo 4 >file && test_tick && git commit -a -m "fourth unsigned" && + git tag fourth-unsigned && + + test_tick && git commit --amend -S -m "fourth signed" && + git tag fourth-signed && + + git config commit.gpgsign true && + echo 5 >file && test_tick && git commit -a -m "fifth signed" && + git tag fifth-signed && + + git config commit.gpgsign false && + echo 6 >file && test_tick && git commit -a -m "sixth" && + git tag sixth-unsigned && + + git config commit.gpgsign true && + echo 7 >file && test_tick && git commit -a -m "seventh" --no-gpg-sign && + git tag seventh-unsigned && + + test_tick && git rebase -f HEAD^^ && git tag sixth-signed HEAD^ && + git tag seventh-signed && + + echo 8 >file && test_tick && git commit -a -m eighth -S"${GPGSSH_KEY_UNTRUSTED}" && + git tag eighth-signed-alt && + + # commit.gpgsign is still on but this must not be signed + echo 9 | git commit-tree HEAD^{tree} >oid && + test_line_count = 1 oid && + git tag ninth-unsigned $(cat oid) && + # explicit -S of course must sign. + echo 10 | git commit-tree -S HEAD^{tree} >oid && + test_line_count = 1 oid && + git tag tenth-signed $(cat oid) && + + # --gpg-sign[=] must sign. + echo 11 | git commit-tree --gpg-sign HEAD^{tree} >oid && + test_line_count = 1 oid && + git tag eleventh-signed $(cat oid) && + echo 12 | git commit-tree --gpg-sign="${GPGSSH_KEY_UNTRUSTED}" HEAD^{tree} >oid && + test_line_count = 1 oid && + git tag twelfth-signed-alt $(cat oid) +' + +test_expect_success GPGSSH 'verify and show signatures' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + test_config gpg.mintrustlevel UNDEFINED && + ( + for commit in initial second merge fourth-signed \ + fifth-signed sixth-signed seventh-signed tenth-signed \ + eleventh-signed + do + git verify-commit $commit && + git show --pretty=short --show-signature $commit >actual && + grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual && + ! grep "${GPGSSH_BAD_SIGNATURE}" actual && + echo $commit OK || exit 1 + done + ) && + ( + for commit in merge^2 fourth-unsigned sixth-unsigned \ + seventh-unsigned ninth-unsigned + do + test_must_fail git verify-commit $commit && + git show --pretty=short --show-signature $commit >actual && + ! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual && + ! grep "${GPGSSH_BAD_SIGNATURE}" actual && + echo $commit OK || exit 1 + done + ) && + ( + for commit in eighth-signed-alt twelfth-signed-alt + do + git show --pretty=short --show-signature $commit >actual && + grep "${GPGSSH_GOOD_SIGNATURE_UNTRUSTED}" actual && + ! grep "${GPGSSH_BAD_SIGNATURE}" actual && + grep "${KEY_NOT_TRUSTED}" actual && + echo $commit OK || exit 1 + done + ) +' + +test_expect_success GPGSSH 'verify-commit exits failure on untrusted signature' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + test_must_fail git verify-commit eighth-signed-alt 2>actual && + grep "${GPGSSH_GOOD_SIGNATURE_UNTRUSTED}" actual && + ! grep "${GPGSSH_BAD_SIGNATURE}" actual && + grep "${KEY_NOT_TRUSTED}" actual +' + +test_expect_success GPGSSH 'verify-commit exits success with matching minTrustLevel' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + test_config gpg.minTrustLevel fully && + git verify-commit sixth-signed +' + +test_expect_success GPGSSH 'verify-commit exits success with low minTrustLevel' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + test_config gpg.minTrustLevel marginal && + git verify-commit sixth-signed +' + +test_expect_success GPGSSH 'verify-commit exits failure with high minTrustLevel' ' + test_config gpg.minTrustLevel ultimate && + test_must_fail git verify-commit eighth-signed-alt +' + +test_expect_success GPGSSH 'verify signatures with --raw' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + ( + for commit in initial second merge fourth-signed fifth-signed sixth-signed seventh-signed + do + git verify-commit --raw $commit 2>actual && + grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual && + ! grep "${GPGSSH_BAD_SIGNATURE}" actual && + echo $commit OK || exit 1 + done + ) && + ( + for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned + do + test_must_fail git verify-commit --raw $commit 2>actual && + ! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual && + ! grep "${GPGSSH_BAD_SIGNATURE}" actual && + echo $commit OK || exit 1 + done + ) && + ( + for commit in eighth-signed-alt + do + test_must_fail git verify-commit --raw $commit 2>actual && + grep "${GPGSSH_GOOD_SIGNATURE_UNTRUSTED}" actual && + ! grep "${GPGSSH_BAD_SIGNATURE}" actual && + echo $commit OK || exit 1 + done + ) +' + +test_expect_success GPGSSH 'proper header is used for hash algorithm' ' + git cat-file commit fourth-signed >output && + grep "^$(test_oid header) -----BEGIN SSH SIGNATURE-----" output +' + +test_expect_success GPGSSH 'show signed commit with signature' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + git show -s initial >commit && + git show -s --show-signature initial >show && + git verify-commit -v initial >verify.1 2>verify.2 && + git cat-file commit initial >cat && + grep -v -e "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" -e "Warning: " show >show.commit && + grep -e "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" -e "Warning: " show >show.gpg && + grep -v "^ " cat | grep -v "^gpgsig.* " >cat.commit && + test_cmp show.commit commit && + test_cmp show.gpg verify.2 && + test_cmp cat.commit verify.1 +' + +test_expect_success GPGSSH 'detect fudged signature' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + git cat-file commit seventh-signed >raw && + sed -e "s/^seventh/7th forged/" raw >forged1 && + git hash-object -w -t commit forged1 >forged1.commit && + test_must_fail git verify-commit $(cat forged1.commit) && + git show --pretty=short --show-signature $(cat forged1.commit) >actual1 && + grep "${GPGSSH_BAD_SIGNATURE}" actual1 && + ! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual1 && + ! grep "${GPGSSH_GOOD_SIGNATURE_UNTRUSTED}" actual1 +' + +test_expect_success GPGSSH 'detect fudged signature with NUL' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + git cat-file commit seventh-signed >raw && + cat raw >forged2 && + echo Qwik | tr "Q" "\000" >>forged2 && + git hash-object -w -t commit forged2 >forged2.commit && + test_must_fail git verify-commit $(cat forged2.commit) && + git show --pretty=short --show-signature $(cat forged2.commit) >actual2 && + grep "${GPGSSH_BAD_SIGNATURE}" actual2 && + ! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual2 +' + +test_expect_success GPGSSH 'amending already signed commit' ' + test_config gpg.format ssh && + test_config user.signingkey "${GPGSSH_KEY_PRIMARY}" && + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + git checkout fourth-signed^0 && + git commit --amend -S --no-edit && + git verify-commit HEAD && + git show -s --show-signature HEAD >actual && + grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual && + ! grep "${GPGSSH_BAD_SIGNATURE}" actual +' + +test_expect_success GPGSSH 'show good signature with custom format' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + FINGERPRINT=$(ssh-keygen -lf "${GPGSSH_KEY_PRIMARY}" | awk "{print \$2;}") && + cat >expect.tmpl <<-\EOF && + G + FINGERPRINT + principal with number 1 + FINGERPRINT + + EOF + sed "s|FINGERPRINT|$FINGERPRINT|g" expect.tmpl >expect && + git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual && + test_cmp expect actual +' + +test_expect_success GPGSSH 'show bad signature with custom format' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + cat >expect <<-\EOF && + B + + + + + EOF + git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" $(cat forged1.commit) >actual && + test_cmp expect actual +' + +test_expect_success GPGSSH 'show untrusted signature with custom format' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + cat >expect.tmpl <<-\EOF && + U + FINGERPRINT + + FINGERPRINT + + EOF + git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual && + FINGERPRINT=$(ssh-keygen -lf "${GPGSSH_KEY_UNTRUSTED}" | awk "{print \$2;}") && + sed "s|FINGERPRINT|$FINGERPRINT|g" expect.tmpl >expect && + test_cmp expect actual +' + +test_expect_success GPGSSH 'show untrusted signature with undefined trust level' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + cat >expect.tmpl <<-\EOF && + undefined + FINGERPRINT + + FINGERPRINT + + EOF + git log -1 --format="%GT%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual && + FINGERPRINT=$(ssh-keygen -lf "${GPGSSH_KEY_UNTRUSTED}" | awk "{print \$2;}") && + sed "s|FINGERPRINT|$FINGERPRINT|g" expect.tmpl >expect && + test_cmp expect actual +' + +test_expect_success GPGSSH 'show untrusted signature with ultimate trust level' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + cat >expect.tmpl <<-\EOF && + fully + FINGERPRINT + principal with number 1 + FINGERPRINT + + EOF + git log -1 --format="%GT%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual && + FINGERPRINT=$(ssh-keygen -lf "${GPGSSH_KEY_PRIMARY}" | awk "{print \$2;}") && + sed "s|FINGERPRINT|$FINGERPRINT|g" expect.tmpl >expect && + test_cmp expect actual +' + +test_expect_success GPGSSH 'show lack of signature with custom format' ' + cat >expect <<-\EOF && + N + + + + + EOF + git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" seventh-unsigned >actual && + test_cmp expect actual +' + +test_expect_success GPGSSH 'log.showsignature behaves like --show-signature' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + test_config log.showsignature true && + git show initial >actual && + grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual +' + +test_expect_success GPGSSH 'check config gpg.format values' ' + test_config gpg.format ssh && + test_config user.signingkey "${GPGSSH_KEY_PRIMARY}" && + test_config gpg.format ssh && + git commit -S --amend -m "success" && + test_config gpg.format OpEnPgP && + test_must_fail git commit -S --amend -m "fail" +' + +test_expect_failure GPGSSH 'detect fudged commit with double signature (TODO)' ' + sed -e "/gpgsig/,/END PGP/d" forged1 >double-base && + sed -n -e "/gpgsig/,/END PGP/p" forged1 | \ + sed -e "s/^$(test_oid header)//;s/^ //" | gpg --dearmor >double-sig1.sig && + gpg -o double-sig2.sig -u 29472784 --detach-sign double-base && + cat double-sig1.sig double-sig2.sig | gpg --enarmor >double-combined.asc && + sed -e "s/^\(-.*\)ARMORED FILE/\1SIGNATURE/;1s/^/$(test_oid header) /;2,\$s/^/ /" \ + double-combined.asc > double-gpgsig && + sed -e "/committer/r double-gpgsig" double-base >double-commit && + git hash-object -w -t commit double-commit >double-commit.commit && + test_must_fail git verify-commit $(cat double-commit.commit) && + git show --pretty=short --show-signature $(cat double-commit.commit) >double-actual && + grep "BAD signature from" double-actual && + grep "Good signature from" double-actual +' + +test_expect_failure GPGSSH 'show double signature with custom format (TODO)' ' + cat >expect <<-\EOF && + E + + + + + EOF + git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" $(cat double-commit.commit) >actual && + test_cmp expect actual +' + + +test_expect_failure GPGSSH 'verify-commit verifies multiply signed commits (TODO)' ' + git init multiply-signed && + cd multiply-signed && + test_commit first && + echo 1 >second && + git add second && + tree=$(git write-tree) && + parent=$(git rev-parse HEAD^{commit}) && + git commit --gpg-sign -m second && + git cat-file commit HEAD && + # Avoid trailing whitespace. + sed -e "s/^Q//" -e "s/^Z/ /" >commit <<-EOF && + Qtree $tree + Qparent $parent + Qauthor A U Thor 1112912653 -0700 + Qcommitter C O Mitter 1112912653 -0700 + Qgpgsig -----BEGIN PGP SIGNATURE----- + QZ + Q iHQEABECADQWIQRz11h0S+chaY7FTocTtvUezd5DDQUCX/uBDRYcY29tbWl0dGVy + Q QGV4YW1wbGUuY29tAAoJEBO29R7N3kMNd+8AoK1I8mhLHviPH+q2I5fIVgPsEtYC + Q AKCTqBh+VabJceXcGIZuF0Ry+udbBQ== + Q =tQ0N + Q -----END PGP SIGNATURE----- + Qgpgsig-sha256 -----BEGIN PGP SIGNATURE----- + QZ + Q iHQEABECADQWIQRz11h0S+chaY7FTocTtvUezd5DDQUCX/uBIBYcY29tbWl0dGVy + Q QGV4YW1wbGUuY29tAAoJEBO29R7N3kMN/NEAn0XO9RYSBj2dFyozi0JKSbssYMtO + Q AJwKCQ1BQOtuwz//IjU8TiS+6S4iUw== + Q =pIwP + Q -----END PGP SIGNATURE----- + Q + Qsecond + EOF + head=$(git hash-object -t commit -w commit) && + git reset --hard $head && + git verify-commit $head 2>actual && + grep "Good signature from" actual && + ! grep "BAD signature from" actual +' + +test_done From 52ac6bd36f7435c9f2b68be6691eacfcf5a80538 Mon Sep 17 00:00:00 2001 From: Fabian Stelzer Date: Tue, 3 Aug 2021 13:45:57 +0000 Subject: [PATCH 08/23] ssh signing: tests for logs, tags & push certs Signed-off-by: Fabian Stelzer Signed-off-by: Junio C Hamano --- t/t4202-log.sh | 23 +++++ t/t5534-push-signed.sh | 101 +++++++++++++++++++ t/t7031-verify-tag-signed-ssh.sh | 161 +++++++++++++++++++++++++++++++ 3 files changed, 285 insertions(+) create mode 100755 t/t7031-verify-tag-signed-ssh.sh diff --git a/t/t4202-log.sh b/t/t4202-log.sh index 39e746fbcbe228..79702716549a14 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -1616,6 +1616,16 @@ test_expect_success GPGSM 'setup signed branch x509' ' git commit -S -m signed_commit ' +test_expect_success GPGSSH 'setup sshkey signed branch' ' + test_config gpg.format ssh && + test_config user.signingkey "${GPGSSH_KEY_PRIMARY}" && + test_when_finished "git reset --hard && git checkout main" && + git checkout -b signed-ssh main && + echo foo >foo && + git add foo && + git commit -S -m signed_commit +' + test_expect_success GPGSM 'log x509 fingerprint' ' echo "F8BF62E0693D0694816377099909C779FA23FD65 | " >expect && git log -n1 --format="%GF | %GP" signed-x509 >actual && @@ -1628,6 +1638,13 @@ test_expect_success GPGSM 'log OpenPGP fingerprint' ' test_cmp expect actual ' +test_expect_success GPGSSH 'log ssh key fingerprint' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + ssh-keygen -lf "${GPGSSH_KEY_PRIMARY}" | awk "{print \$2\" | \"}" >expect && + git log -n1 --format="%GF | %GP" signed-ssh >actual && + test_cmp expect actual +' + test_expect_success GPG 'log --graph --show-signature' ' git log --graph --show-signature -n1 signed >actual && grep "^| gpg: Signature made" actual && @@ -1640,6 +1657,12 @@ test_expect_success GPGSM 'log --graph --show-signature x509' ' grep "^| gpgsm: Good signature" actual ' +test_expect_success GPGSSH 'log --graph --show-signature ssh' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + git log --graph --show-signature -n1 signed-ssh >actual && + grep "${GOOD_SIGNATURE_TRUSTED}" actual +' + test_expect_success GPG 'log --graph --show-signature for merged tag' ' test_when_finished "git reset --hard && git checkout main" && git checkout -b plain main && diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh index bba768f5ded1fc..24d374adbae884 100755 --- a/t/t5534-push-signed.sh +++ b/t/t5534-push-signed.sh @@ -137,6 +137,53 @@ test_expect_success GPG 'signed push sends push certificate' ' test_cmp expect dst/push-cert-status ' +test_expect_success GPGSSH 'ssh signed push sends push certificate' ' + prepare_dst && + mkdir -p dst/.git/hooks && + git -C dst config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + git -C dst config receive.certnonceseed sekrit && + write_script dst/.git/hooks/post-receive <<-\EOF && + # discard the update list + cat >/dev/null + # record the push certificate + if test -n "${GIT_PUSH_CERT-}" + then + git cat-file blob $GIT_PUSH_CERT >../push-cert + fi && + + cat >../push-cert-status <expect && + + noop=$(git rev-parse noop) && + ff=$(git rev-parse ff) && + noff=$(git rev-parse noff) && + grep "$noop $ff refs/heads/ff" dst/push-cert && + grep "$noop $noff refs/heads/noff" dst/push-cert && + test_cmp expect dst/push-cert-status +' + test_expect_success GPG 'inconsistent push options in signed push not allowed' ' # First, invoke receive-pack with dummy input to obtain its preamble. prepare_dst && @@ -276,6 +323,60 @@ test_expect_success GPGSM 'fail without key and heed user.signingkey x509' ' test_cmp expect dst/push-cert-status ' +test_expect_success GPGSSH 'fail without key and heed user.signingkey ssh' ' + test_config gpg.format ssh && + prepare_dst && + mkdir -p dst/.git/hooks && + git -C dst config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + git -C dst config receive.certnonceseed sekrit && + write_script dst/.git/hooks/post-receive <<-\EOF && + # discard the update list + cat >/dev/null + # record the push certificate + if test -n "${GIT_PUSH_CERT-}" + then + git cat-file blob $GIT_PUSH_CERT >../push-cert + fi && + + cat >../push-cert-status <expect && + + noop=$(git rev-parse noop) && + ff=$(git rev-parse ff) && + noff=$(git rev-parse noff) && + grep "$noop $ff refs/heads/ff" dst/push-cert && + grep "$noop $noff refs/heads/noff" dst/push-cert && + test_cmp expect dst/push-cert-status +' + test_expect_success GPG 'failed atomic push does not execute GPG' ' prepare_dst && git -C dst config receive.certnonceseed sekrit && diff --git a/t/t7031-verify-tag-signed-ssh.sh b/t/t7031-verify-tag-signed-ssh.sh new file mode 100755 index 00000000000000..06c9dd6c9339f2 --- /dev/null +++ b/t/t7031-verify-tag-signed-ssh.sh @@ -0,0 +1,161 @@ +#!/bin/sh + +test_description='signed tag tests' +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + +. ./test-lib.sh +. "$TEST_DIRECTORY/lib-gpg.sh" + +test_expect_success GPGSSH 'create signed tags ssh' ' + test_when_finished "test_unconfig commit.gpgsign" && + test_config gpg.format ssh && + test_config user.signingkey "${GPGSSH_KEY_PRIMARY}" && + + echo 1 >file && git add file && + test_tick && git commit -m initial && + git tag -s -m initial initial && + git branch side && + + echo 2 >file && test_tick && git commit -a -m second && + git tag -s -m second second && + + git checkout side && + echo 3 >elif && git add elif && + test_tick && git commit -m "third on side" && + + git checkout main && + test_tick && git merge -S side && + git tag -s -m merge merge && + + echo 4 >file && test_tick && git commit -a -S -m "fourth unsigned" && + git tag -a -m fourth-unsigned fourth-unsigned && + + test_tick && git commit --amend -S -m "fourth signed" && + git tag -s -m fourth fourth-signed && + + echo 5 >file && test_tick && git commit -a -m "fifth" && + git tag fifth-unsigned && + + git config commit.gpgsign true && + echo 6 >file && test_tick && git commit -a -m "sixth" && + git tag -a -m sixth sixth-unsigned && + + test_tick && git rebase -f HEAD^^ && git tag -s -m 6th sixth-signed HEAD^ && + git tag -m seventh -s seventh-signed && + + echo 8 >file && test_tick && git commit -a -m eighth && + git tag -u"${GPGSSH_KEY_UNTRUSTED}" -m eighth eighth-signed-alt +' + +test_expect_success GPGSSH 'verify and show ssh signatures' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + ( + for tag in initial second merge fourth-signed sixth-signed seventh-signed + do + git verify-tag $tag 2>actual && + grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual && + ! grep "${GPGSSH_BAD_SIGNATURE}" actual && + echo $tag OK || exit 1 + done + ) && + ( + for tag in fourth-unsigned fifth-unsigned sixth-unsigned + do + test_must_fail git verify-tag $tag 2>actual && + ! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual && + ! grep "${GPGSSH_BAD_SIGNATURE}" actual && + echo $tag OK || exit 1 + done + ) && + ( + for tag in eighth-signed-alt + do + test_must_fail git verify-tag $tag 2>actual && + grep "${GPGSSH_GOOD_SIGNATURE_UNTRUSTED}" actual && + ! grep "${GPGSSH_BAD_SIGNATURE}" actual && + grep "${GPGSSH_KEY_NOT_TRUSTED}" actual && + echo $tag OK || exit 1 + done + ) +' + +test_expect_success GPGSSH 'detect fudged ssh signature' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + git cat-file tag seventh-signed >raw && + sed -e "/^tag / s/seventh/7th forged/" raw >forged1 && + git hash-object -w -t tag forged1 >forged1.tag && + test_must_fail git verify-tag $(cat forged1.tag) 2>actual1 && + grep "${GPGSSH_BAD_SIGNATURE}" actual1 && + ! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual1 && + ! grep "${GPGSSH_GOOD_SIGNATURE_UNTRUSTED}" actual1 +' + +test_expect_success GPGSSH 'verify ssh signatures with --raw' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + ( + for tag in initial second merge fourth-signed sixth-signed seventh-signed + do + git verify-tag --raw $tag 2>actual && + grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual && + ! grep "${GPGSSH_BAD_SIGNATURE}" actual && + echo $tag OK || exit 1 + done + ) && + ( + for tag in fourth-unsigned fifth-unsigned sixth-unsigned + do + test_must_fail git verify-tag --raw $tag 2>actual && + ! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual && + ! grep "${GPGSSH_BAD_SIGNATURE}" actual && + echo $tag OK || exit 1 + done + ) && + ( + for tag in eighth-signed-alt + do + test_must_fail git verify-tag --raw $tag 2>actual && + grep "${GPGSSH_GOOD_SIGNATURE_UNTRUSTED}" actual && + ! grep "${GPGSSH_BAD_SIGNATURE}" actual && + echo $tag OK || exit 1 + done + ) +' + +test_expect_success GPGSSH 'verify signatures with --raw ssh' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + git verify-tag --raw sixth-signed 2>actual && + grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual && + ! grep "${GPGSSH_BAD_SIGNATURE}" actual && + echo sixth-signed OK +' + +test_expect_success GPGSSH 'verify multiple tags ssh' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + tags="seventh-signed sixth-signed" && + for i in $tags + do + git verify-tag -v --raw $i || return 1 + done >expect.stdout 2>expect.stderr.1 && + grep "^${GPGSSH_GOOD_SIGNATURE_TRUSTED}" expect.stderr && + git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 && + grep "^${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual.stderr && + test_cmp expect.stdout actual.stdout && + test_cmp expect.stderr actual.stderr +' + +test_expect_success GPGSSH 'verifying tag with --format - ssh' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + cat >expect <<-\EOF && + tagname : fourth-signed + EOF + git verify-tag --format="tagname : %(tag)" "fourth-signed" >actual && + test_cmp expect actual +' + +test_expect_success GPGSSH 'verifying a forged tag with --format should fail silently - ssh' ' + test_must_fail git verify-tag --format="tagname : %(tag)" $(cat forged1.tag) >actual-forged && + test_must_be_empty actual-forged +' + +test_done From b88bcd013ba705d8df06c48438e83adc9e65e68b Mon Sep 17 00:00:00 2001 From: Fabian Stelzer Date: Tue, 3 Aug 2021 13:45:58 +0000 Subject: [PATCH 09/23] ssh signing: test that gpg fails for unknown keys Test that verify-commit/tag will fail when a gpg key is completely unknown. To do this we have to generate a key, use it for a signature and delete it from our keyring aferwards completely. Signed-off-by: Fabian Stelzer Signed-off-by: Junio C Hamano --- t/t7510-signed-commit.sh | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh index 8df5a74f1db4ec..d65a0171f29c85 100755 --- a/t/t7510-signed-commit.sh +++ b/t/t7510-signed-commit.sh @@ -71,7 +71,25 @@ test_expect_success GPG 'create signed commits' ' git tag eleventh-signed $(cat oid) && echo 12 | git commit-tree --gpg-sign=B7227189 HEAD^{tree} >oid && test_line_count = 1 oid && - git tag twelfth-signed-alt $(cat oid) + git tag twelfth-signed-alt $(cat oid) && + + cat >keydetails <<-\EOF && + Key-Type: RSA + Key-Length: 2048 + Subkey-Type: RSA + Subkey-Length: 2048 + Name-Real: Unknown User + Name-Email: unknown@git.com + Expire-Date: 0 + %no-ask-passphrase + %no-protection + EOF + gpg --batch --gen-key keydetails && + echo 13 >file && git commit -a -S"unknown@git.com" -m thirteenth && + git tag thirteenth-signed && + DELETE_FINGERPRINT=$(gpg -K --with-colons --fingerprint --batch unknown@git.com | grep "^fpr" | head -n 1 | awk -F ":" "{print \$10;}") && + gpg --batch --yes --delete-secret-keys $DELETE_FINGERPRINT && + gpg --batch --yes --delete-keys unknown@git.com ' test_expect_success GPG 'verify and show signatures' ' @@ -110,6 +128,13 @@ test_expect_success GPG 'verify and show signatures' ' ) ' +test_expect_success GPG 'verify-commit exits failure on unknown signature' ' + test_must_fail git verify-commit thirteenth-signed 2>actual && + ! grep "Good signature from" actual && + ! grep "BAD signature from" actual && + grep -q -F -e "No public key" -e "public key not found" actual +' + test_expect_success GPG 'verify-commit exits success on untrusted signature' ' git verify-commit eighth-signed-alt 2>actual && grep "Good signature from" actual && @@ -338,6 +363,8 @@ test_expect_success GPG 'show double signature with custom format' ' ' +# NEEDSWORK: This test relies on the test_tick commit/author dates from the first +# 'create signed commits' test even though it creates its own test_expect_success GPG 'verify-commit verifies multiply signed commits' ' git init multiply-signed && cd multiply-signed && From 5e29049d33d12c9bf75c9b7214e9d7d19bb2e7fe Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 10 Sep 2021 14:02:04 -0700 Subject: [PATCH 10/23] Revert "Merge branch 'fs/ssh-signing' into next" This reverts commit eed4e814c0ba7b472a6c74d923b2fd0974e69d2e, reversing changes made to 348fe07b8733ef4c5afbb66831dbd4b6f09d8e9f, so that we can eventually replace it with a newer iteration. As it stands, the topic just makes it segfault when used with OpenSSH 8.7 (the latest). --- Documentation/config/gpg.txt | 45 +-- Documentation/config/user.txt | 7 - builtin/receive-pack.c | 4 - fmt-merge-msg.c | 6 +- gpg-interface.c | 571 +++---------------------------- gpg-interface.h | 8 +- log-tree.c | 8 +- pretty.c | 4 +- send-pack.c | 8 +- t/lib-gpg.sh | 28 -- t/t4202-log.sh | 23 -- t/t5534-push-signed.sh | 101 ------ t/t7031-verify-tag-signed-ssh.sh | 161 --------- t/t7510-signed-commit.sh | 29 +- t/t7528-signed-commit-ssh.sh | 398 --------------------- 15 files changed, 66 insertions(+), 1335 deletions(-) delete mode 100755 t/t7031-verify-tag-signed-ssh.sh delete mode 100755 t/t7528-signed-commit-ssh.sh diff --git a/Documentation/config/gpg.txt b/Documentation/config/gpg.txt index 51a756b2f15f85..d94025cb3684d8 100644 --- a/Documentation/config/gpg.txt +++ b/Documentation/config/gpg.txt @@ -11,13 +11,13 @@ gpg.program:: gpg.format:: Specifies which key format to use when signing with `--gpg-sign`. - Default is "openpgp". Other possible values are "x509", "ssh". + Default is "openpgp" and another possible value is "x509". gpg..program:: Use this to customize the program used for the signing format you chose. (see `gpg.program` and `gpg.format`) `gpg.program` can still be used as a legacy synonym for `gpg.openpgp.program`. The default - value for `gpg.x509.program` is "gpgsm" and `gpg.ssh.program` is "ssh-keygen". + value for `gpg.x509.program` is "gpgsm". gpg.minTrustLevel:: Specifies a minimum trust level for signature verification. If @@ -33,44 +33,3 @@ gpg.minTrustLevel:: * `marginal` * `fully` * `ultimate` - -gpg.ssh.defaultKeyCommand: - This command that will be run when user.signingkey is not set and a ssh - signature is requested. On successful exit a valid ssh public key is - expected in the first line of its output. To automatically use the first - available key from your ssh-agent set this to "ssh-add -L". - -gpg.ssh.allowedSignersFile:: - A file containing ssh public keys which you are willing to trust. - The file consists of one or more lines of principals followed by an ssh - public key. - e.g.: user1@example.com,user2@example.com ssh-rsa AAAAX1... - See ssh-keygen(1) "ALLOWED SIGNERS" for details. - The principal is only used to identify the key and is available when - verifying a signature. -+ -SSH has no concept of trust levels like gpg does. To be able to differentiate -between valid signatures and trusted signatures the trust level of a signature -verification is set to `fully` when the public key is present in the allowedSignersFile. -Therefore to only mark fully trusted keys as verified set gpg.minTrustLevel to `fully`. -Otherwise valid but untrusted signatures will still verify but show no principal -name of the signer. -+ -This file can be set to a location outside of the repository and every developer -maintains their own trust store. A central repository server could generate this -file automatically from ssh keys with push access to verify the code against. -In a corporate setting this file is probably generated at a global location -from automation that already handles developer ssh keys. -+ -A repository that only allows signed commits can store the file -in the repository itself using a path relative to the top-level of the working tree. -This way only committers with an already valid key can add or change keys in the keyring. -+ -Using a SSH CA key with the cert-authority option -(see ssh-keygen(1) "CERTIFICATES") is also valid. - -gpg.ssh.revocationFile:: - Either a SSH KRL or a list of revoked public keys (without the principal prefix). - See ssh-keygen(1) for details. - If a public key is found in this file then it will always be treated - as having trust level "never" and signatures will show as invalid. diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt index ad78dce9ecbfc6..59aec7c3aed32a 100644 --- a/Documentation/config/user.txt +++ b/Documentation/config/user.txt @@ -36,10 +36,3 @@ user.signingKey:: commit, you can override the default selection with this variable. This option is passed unchanged to gpg's --local-user parameter, so you may specify a key using any method that gpg supports. - If gpg.format is set to "ssh" this can contain the literal ssh public - key (e.g.: "ssh-rsa XXXXXX identifier") or a file which contains it and - corresponds to the private key used for signing. The private key - needs to be available via ssh-agent. Alternatively it can be set to - a file containing a private key directly. If not set git will call - gpg.ssh.defaultKeyCommand (e.g.: "ssh-add -L") and try to use the first - key available. diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 2c1db899c3e1fa..041e91545400e0 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -131,10 +131,6 @@ static int receive_pack_config(const char *var, const char *value, void *cb) { int status = parse_hide_refs_config(var, value, "receive"); - if (status) - return status; - - status = git_gpg_config(var, value, NULL); if (status) return status; diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c index 2901c5e4f8f0c9..b969dc6ebb6800 100644 --- a/fmt-merge-msg.c +++ b/fmt-merge-msg.c @@ -528,11 +528,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.output) + sig.len, &sigc) && + !sigc.gpg_output) strbuf_addstr(&sig, "gpg verification failed.\n"); else - strbuf_addstr(&sig, sigc.output); + strbuf_addstr(&sig, sigc.gpg_output); } signature_check_clear(&sigc); diff --git a/gpg-interface.c b/gpg-interface.c index 9c1ef11a563f3f..127aecfc2b071f 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -3,14 +3,11 @@ #include "config.h" #include "run-command.h" #include "strbuf.h" -#include "dir.h" #include "gpg-interface.h" #include "sigchain.h" #include "tempfile.h" -#include "alias.h" static char *configured_signing_key; -static const char *ssh_default_key_command, *ssh_allowed_signers, *ssh_revocation_file; static enum signature_trust_level configured_min_trust_level = TRUST_UNDEFINED; struct gpg_format { @@ -18,14 +15,6 @@ 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); - const char *(*get_default_key)(void); - const char *(*get_key_id)(void); }; static const char *openpgp_verify_args[] = { @@ -46,59 +35,14 @@ static const char *x509_sigs[] = { NULL }; -static const char *ssh_verify_args[] = { NULL }; -static const char *ssh_sigs[] = { - "-----BEGIN SSH SIGNATURE-----", - 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 verify_ssh_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 int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature, - const char *signing_key); - -static const char *get_default_ssh_signing_key(void); - -static const char *get_ssh_key_id(void); - static struct gpg_format gpg_format[] = { - { - .name = "openpgp", - .program = "gpg", - .verify_args = openpgp_verify_args, - .sigs = openpgp_sigs, - .verify_signed_buffer = verify_gpg_signed_buffer, - .sign_buffer = sign_buffer_gpg, - .get_default_key = NULL, - .get_key_id = NULL, - }, - { - .name = "x509", - .program = "gpgsm", - .verify_args = x509_verify_args, - .sigs = x509_sigs, - .verify_signed_buffer = verify_gpg_signed_buffer, - .sign_buffer = sign_buffer_gpg, - .get_default_key = NULL, - .get_key_id = NULL, + { .name = "openpgp", .program = "gpg", + .verify_args = openpgp_verify_args, + .sigs = openpgp_sigs }, - { - .name = "ssh", - .program = "ssh-keygen", - .verify_args = ssh_verify_args, - .sigs = ssh_sigs, - .verify_signed_buffer = verify_ssh_signed_buffer, - .sign_buffer = sign_buffer_ssh, - .get_default_key = get_default_ssh_signing_key, - .get_key_id = get_ssh_key_id, + { .name = "x509", .program = "gpgsm", + .verify_args = x509_verify_args, + .sigs = x509_sigs }, }; @@ -128,7 +72,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->output); + FREE_AND_NULL(sigc->gpg_output); FREE_AND_NULL(sigc->gpg_status); FREE_AND_NULL(sigc->signer); FREE_AND_NULL(sigc->key); @@ -313,16 +257,16 @@ static void parse_gpg_output(struct signature_check *sigc) FREE_AND_NULL(sigc->key); } -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 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 child_process gpg = CHILD_PROCESS_INIT; + struct gpg_format *fmt; struct tempfile *temp; int ret; - struct strbuf gpg_stdout = STRBUF_INIT; - struct strbuf gpg_stderr = STRBUF_INIT; + struct strbuf buf = STRBUF_INIT; temp = mks_tempfile_t(".git_vtag_tmpXXXXXX"); if (!temp) @@ -335,6 +279,10 @@ static int verify_gpg_signed_buffer(struct signature_check *sigc, 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, @@ -342,210 +290,18 @@ static int verify_gpg_signed_buffer(struct signature_check *sigc, "--verify", temp->filename.buf, "-", NULL); + if (!gpg_status) + gpg_status = &buf; + sigchain_push(SIGPIPE, SIG_IGN); - ret = pipe_command(&gpg, payload, payload_size, &gpg_stdout, 0, - &gpg_stderr, 0); + ret = pipe_command(&gpg, payload, payload_size, + gpg_status, 0, gpg_output, 0); sigchain_pop(SIGPIPE); delete_tempfile(&temp); - 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; -} - -static void parse_ssh_output(struct signature_check *sigc) -{ - const char *line, *principal, *search; - char *key = NULL; - - /* - * ssh-keygen output should be: - * Good "git" signature for PRINCIPAL with RSA key SHA256:FINGERPRINT - * - * or for valid but unknown keys: - * Good "git" signature with RSA key SHA256:FINGERPRINT - * - * Note that "PRINCIPAL" can contain whitespace, "RSA" and - * "SHA256" part could be a different token that names of - * the algorithms used, and "FINGERPRINT" is a hexadecimal - * string. By finding the last occurence of " with ", we can - * reliably parse out the PRINCIPAL. - */ - sigc->result = 'B'; - sigc->trust_level = TRUST_NEVER; - - line = xmemdupz(sigc->output, strcspn(sigc->output, "\n")); - - if (skip_prefix(line, "Good \"git\" signature for ", &line)) { - /* Valid signature and known principal */ - sigc->result = 'G'; - sigc->trust_level = TRUST_FULLY; - - /* Search for the last "with" to get the full principal */ - principal = line; - do { - search = strstr(line, " with "); - if (search) - line = search + 1; - } while (search != NULL); - sigc->signer = xmemdupz(principal, line - principal - 1); - } else if (skip_prefix(line, "Good \"git\" signature with ", &line)) { - /* Valid signature, but key unknown */ - sigc->result = 'G'; - sigc->trust_level = TRUST_UNDEFINED; - } else { - return; - } - - key = strstr(line, "key"); - if (key) { - sigc->fingerprint = xstrdup(strstr(line, "key") + 4); - sigc->key = xstrdup(sigc->fingerprint); - } else { - /* - * Output did not match what we expected - * Treat the signature as bad - */ - sigc->result = 'B'; - } -} - -static int verify_ssh_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 ssh_keygen = CHILD_PROCESS_INIT; - struct tempfile *buffer_file; - int ret = -1; - const char *line; - size_t trust_size; - char *principal; - struct strbuf ssh_keygen_out = STRBUF_INIT; - struct strbuf ssh_keygen_err = STRBUF_INIT; - - if (!ssh_allowed_signers) { - error(_("gpg.ssh.allowedSignersFile needs to be configured and exist for ssh signature verification")); - return -1; - } - - buffer_file = mks_tempfile_t(".git_vtag_tmpXXXXXX"); - if (!buffer_file) - return error_errno(_("could not create temporary file")); - if (write_in_full(buffer_file->fd, signature, signature_size) < 0 || - close_tempfile_gently(buffer_file) < 0) { - error_errno(_("failed writing detached signature to '%s'"), - buffer_file->filename.buf); - delete_tempfile(&buffer_file); - return -1; - } - - /* Find the principal from the signers */ - strvec_pushl(&ssh_keygen.args, fmt->program, - "-Y", "find-principals", - "-f", ssh_allowed_signers, - "-s", buffer_file->filename.buf, - NULL); - ret = pipe_command(&ssh_keygen, NULL, 0, &ssh_keygen_out, 0, - &ssh_keygen_err, 0); - if (ret && strstr(ssh_keygen_err.buf, "usage:")) { - error(_("ssh-keygen -Y find-principals/verify is needed for ssh signature verification (available in openssh version 8.2p1+)")); - goto out; - } - if (ret || !ssh_keygen_out.len) { - /* - * We did not find a matching principal in the allowedSigners - * Check without validation - */ - child_process_init(&ssh_keygen); - strvec_pushl(&ssh_keygen.args, fmt->program, - "-Y", "check-novalidate", - "-n", "git", - "-s", buffer_file->filename.buf, - NULL); - pipe_command(&ssh_keygen, payload, payload_size, - &ssh_keygen_out, 0, &ssh_keygen_err, 0); - - /* - * Fail on unknown keys - * we still call check-novalidate to display the signature info - */ - ret = -1; - } else { - /* Check every principal we found (one per line) */ - for (line = ssh_keygen_out.buf; *line; - line = strchrnul(line + 1, '\n')) { - while (*line == '\n') - line++; - if (!*line) - break; - - trust_size = strcspn(line, "\n"); - principal = xmemdupz(line, trust_size); - - child_process_init(&ssh_keygen); - strbuf_release(&ssh_keygen_out); - strbuf_release(&ssh_keygen_err); - strvec_push(&ssh_keygen.args, fmt->program); - /* - * We found principals - * Try with each until we find a match - */ - strvec_pushl(&ssh_keygen.args, "-Y", "verify", - "-n", "git", - "-f", ssh_allowed_signers, - "-I", principal, - "-s", buffer_file->filename.buf, - NULL); - - if (ssh_revocation_file) { - if (file_exists(ssh_revocation_file)) { - strvec_pushl(&ssh_keygen.args, "-r", - ssh_revocation_file, NULL); - } else { - warning(_("ssh signing revocation file configured but not found: %s"), - ssh_revocation_file); - } - } - - sigchain_push(SIGPIPE, SIG_IGN); - ret = pipe_command(&ssh_keygen, payload, payload_size, - &ssh_keygen_out, 0, &ssh_keygen_err, 0); - sigchain_pop(SIGPIPE); - - FREE_AND_NULL(principal); - - if (!ret) - ret = !starts_with(ssh_keygen_out.buf, "Good"); - - if (!ret) - break; - } - } - - sigc->payload = xmemdupz(payload, payload_size); - strbuf_stripspace(&ssh_keygen_out, 0); - strbuf_stripspace(&ssh_keygen_err, 0); - strbuf_add(&ssh_keygen_out, ssh_keygen_err.buf, ssh_keygen_err.len); - sigc->output = strbuf_detach(&ssh_keygen_out, NULL); - sigc->gpg_status = xstrdup(sigc->output); - - parse_ssh_output(sigc); - -out: - if (buffer_file) - delete_tempfile(&buffer_file); - strbuf_release(&ssh_keygen_out); - strbuf_release(&ssh_keygen_err); + ret |= !strstr(gpg_status->buf, "\n[GNUPG:] GOODSIG "); + strbuf_release(&buf); /* no matter it was used or not */ return ret; } @@ -553,32 +309,35 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc, int check_signature(const char *payload, size_t plen, const char *signature, size_t slen, struct signature_check *sigc) { - struct gpg_format *fmt; + struct strbuf gpg_output = STRBUF_INIT; + struct strbuf gpg_status = STRBUF_INIT; int status; sigc->result = 'N'; sigc->trust_level = -1; - 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 = 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); 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->output; + const char *output = flags & GPG_VERIFY_RAW ? + sigc->gpg_status : sigc->gpg_output; if (flags & GPG_VERIFY_VERBOSE && sigc->payload) fputs(sigc->payload, stdout); @@ -660,33 +419,12 @@ int git_gpg_config(const char *var, const char *value, void *cb) return 0; } - if (!strcmp(var, "gpg.ssh.defaultkeycommand")) { - if (!value) - return config_error_nonbool(var); - return git_config_string(&ssh_default_key_command, var, value); - } - - if (!strcmp(var, "gpg.ssh.allowedsignersfile")) { - if (!value) - return config_error_nonbool(var); - return git_config_string(&ssh_allowed_signers, var, value); - } - - if (!strcmp(var, "gpg.ssh.revocationfile")) { - if (!value) - return config_error_nonbool(var); - return git_config_string(&ssh_revocation_file, var, value); - } - if (!strcmp(var, "gpg.program") || !strcmp(var, "gpg.openpgp.program")) fmtname = "openpgp"; if (!strcmp(var, "gpg.x509.program")) fmtname = "x509"; - if (!strcmp(var, "gpg.ssh.program")) - fmtname = "ssh"; - if (fmtname) { fmt = get_format_by_name(fmtname); return git_config_string(&fmt->program, var, value); @@ -695,144 +433,18 @@ int git_gpg_config(const char *var, const char *value, void *cb) return 0; } -static char *get_ssh_key_fingerprint(const char *signing_key) -{ - struct child_process ssh_keygen = CHILD_PROCESS_INIT; - int ret = -1; - struct strbuf fingerprint_stdout = STRBUF_INIT; - struct strbuf **fingerprint; - - /* - * With SSH Signing this can contain a filename or a public key - * For textual representation we usually want a fingerprint - */ - if (starts_with(signing_key, "ssh-")) { - strvec_pushl(&ssh_keygen.args, "ssh-keygen", "-lf", "-", NULL); - ret = pipe_command(&ssh_keygen, signing_key, - strlen(signing_key), &fingerprint_stdout, 0, - NULL, 0); - } else { - strvec_pushl(&ssh_keygen.args, "ssh-keygen", "-lf", - configured_signing_key, NULL); - ret = pipe_command(&ssh_keygen, NULL, 0, &fingerprint_stdout, 0, - NULL, 0); - } - - if (!!ret) - die_errno(_("failed to get the ssh fingerprint for key '%s'"), - signing_key); - - fingerprint = strbuf_split_max(&fingerprint_stdout, ' ', 3); - if (!fingerprint[1]) - die_errno(_("failed to get the ssh fingerprint for key '%s'"), - signing_key); - - return strbuf_detach(fingerprint[1], NULL); -} - -/* Returns the first public key from an ssh-agent to use for signing */ -static const char *get_default_ssh_signing_key(void) -{ - struct child_process ssh_default_key = CHILD_PROCESS_INIT; - int ret = -1; - struct strbuf key_stdout = STRBUF_INIT, key_stderr = STRBUF_INIT; - struct strbuf **keys; - char *key_command = NULL; - const char **argv; - int n; - char *default_key = NULL; - - if (!ssh_default_key_command) - die(_("either user.signingkey or gpg.ssh.defaultKeyCommand needs to be configured")); - - key_command = xstrdup(ssh_default_key_command); - n = split_cmdline(key_command, &argv); - - if (n < 0) - die("malformed build-time gpg.ssh.defaultKeyCommand: %s", - split_cmdline_strerror(n)); - - strvec_pushv(&ssh_default_key.args, argv); - ret = pipe_command(&ssh_default_key, NULL, 0, &key_stdout, 0, - &key_stderr, 0); - - if (!ret) { - keys = strbuf_split_max(&key_stdout, '\n', 2); - if (keys[0] && starts_with(keys[0]->buf, "ssh-")) { - default_key = strbuf_detach(keys[0], NULL); - } else { - warning(_("gpg.ssh.defaultKeycommand succeeded but returned no keys: %s %s"), - key_stderr.buf, key_stdout.buf); - } - - strbuf_list_free(keys); - } else { - warning(_("gpg.ssh.defaultKeyCommand failed: %s %s"), - key_stderr.buf, key_stdout.buf); - } - - free(key_command); - free(argv); - strbuf_release(&key_stdout); - - return default_key; -} - -static const char *get_ssh_key_id(void) { - return get_ssh_key_fingerprint(get_signing_key()); -} - -/* Returns a textual but unique representation of the signing key */ -const char *get_signing_key_id(void) -{ - if (use_format->get_key_id) { - return use_format->get_key_id(); - } - - /* GPG/GPGSM only store a key id on this variable */ - return get_signing_key(); -} - const char *get_signing_key(void) { if (configured_signing_key) return configured_signing_key; - if (use_format->get_default_key) { - return use_format->get_default_key(); - } - - return git_committer_info(IDENT_STRICT | IDENT_NO_DATE); + return git_committer_info(IDENT_STRICT|IDENT_NO_DATE); } int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key) -{ - return use_format->sign_buffer(buffer, signature, signing_key); -} - -/* - * Strip CR from the line endings, in case we are on Windows. - * NEEDSWORK: make it trim only CRs before LFs and rename - */ -static void remove_cr_after(struct strbuf *buffer, size_t offset) -{ - size_t i, j; - - for (i = j = offset; i < buffer->len; i++) { - if (buffer->buf[i] != '\r') { - if (i != j) - buffer->buf[j] = buffer->buf[i]; - j++; - } - } - strbuf_setlen(buffer, j); -} - -static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature, - const char *signing_key) { struct child_process gpg = CHILD_PROCESS_INIT; int ret; - size_t bottom; + size_t i, j, bottom; struct strbuf gpg_status = STRBUF_INIT; strvec_pushl(&gpg.args, @@ -858,98 +470,13 @@ static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature, return error(_("gpg failed to sign the data")); /* Strip CR from the line endings, in case we are on Windows. */ - remove_cr_after(signature, bottom); - - return 0; -} - -static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature, - const char *signing_key) -{ - struct child_process signer = CHILD_PROCESS_INIT; - int ret = -1; - size_t bottom, keylen; - struct strbuf signer_stderr = STRBUF_INIT; - struct tempfile *key_file = NULL, *buffer_file = NULL; - char *ssh_signing_key_file = NULL; - struct strbuf ssh_signature_filename = STRBUF_INIT; - - if (!signing_key || signing_key[0] == '\0') - return error( - _("user.signingkey needs to be set for ssh signing")); - - if (starts_with(signing_key, "ssh-")) { - /* A literal ssh key */ - key_file = mks_tempfile_t(".git_signing_key_tmpXXXXXX"); - if (!key_file) - return error_errno( - _("could not create temporary file")); - keylen = strlen(signing_key); - if (write_in_full(key_file->fd, signing_key, keylen) < 0 || - close_tempfile_gently(key_file) < 0) { - error_errno(_("failed writing ssh signing key to '%s'"), - key_file->filename.buf); - goto out; + for (i = j = bottom; i < signature->len; i++) + if (signature->buf[i] != '\r') { + if (i != j) + signature->buf[j] = signature->buf[i]; + j++; } - ssh_signing_key_file = strbuf_detach(&key_file->filename, NULL); - } else { - /* We assume a file */ - ssh_signing_key_file = expand_user_path(signing_key, 1); - } - - buffer_file = mks_tempfile_t(".git_signing_buffer_tmpXXXXXX"); - if (!buffer_file) { - error_errno(_("could not create temporary file")); - goto out; - } - - if (write_in_full(buffer_file->fd, buffer->buf, buffer->len) < 0 || - close_tempfile_gently(buffer_file) < 0) { - error_errno(_("failed writing ssh signing key buffer to '%s'"), - buffer_file->filename.buf); - goto out; - } - - strvec_pushl(&signer.args, use_format->program, - "-Y", "sign", - "-n", "git", - "-f", ssh_signing_key_file, - buffer_file->filename.buf, - NULL); - - sigchain_push(SIGPIPE, SIG_IGN); - ret = pipe_command(&signer, NULL, 0, NULL, 0, &signer_stderr, 0); - sigchain_pop(SIGPIPE); - - if (ret) { - if (strstr(signer_stderr.buf, "usage:")) - error(_("ssh-keygen -Y sign is needed for ssh signing (available in openssh version 8.2p1+)")); - - error("%s", signer_stderr.buf); - goto out; - } + strbuf_setlen(signature, j); - bottom = signature->len; - - strbuf_addbuf(&ssh_signature_filename, &buffer_file->filename); - strbuf_addstr(&ssh_signature_filename, ".sig"); - if (strbuf_read_file(signature, ssh_signature_filename.buf, 0) < 0) { - error_errno( - _("failed reading ssh signing data buffer from '%s'"), - ssh_signature_filename.buf); - } - unlink_or_warn(ssh_signature_filename.buf); - - /* Strip CR from the line endings, in case we are on Windows. */ - remove_cr_after(signature, bottom); - -out: - if (key_file) - delete_tempfile(&key_file); - if (buffer_file) - delete_tempfile(&buffer_file); - strbuf_release(&signer_stderr); - strbuf_release(&ssh_signature_filename); - FREE_AND_NULL(ssh_signing_key_file); - return ret; + return 0; } diff --git a/gpg-interface.h b/gpg-interface.h index beefacbb1e9025..80567e4894868d 100644 --- a/gpg-interface.h +++ b/gpg-interface.h @@ -17,7 +17,7 @@ enum signature_trust_level { struct signature_check { char *payload; - char *output; + char *gpg_output; char *gpg_status; /* @@ -64,12 +64,6 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, int git_gpg_config(const char *, const char *, void *); void set_signing_key(const char *); const char *get_signing_key(void); - -/* - * Returns a textual unique representation of the signing key in use - * Either a GPG KeyID or a SSH Key Fingerprint - */ -const char *get_signing_key_id(void); int check_signature(const char *payload, size_t plen, const char *signature, size_t slen, struct signature_check *sigc); diff --git a/log-tree.c b/log-tree.c index 644893fd8cfff6..6dc4412268b8ae 100644 --- a/log-tree.c +++ b/log-tree.c @@ -515,10 +515,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.output) + if (status && !sigc.gpg_output) show_sig_lines(opt, status, "No signature\n"); else - show_sig_lines(opt, status, sigc.output); + show_sig_lines(opt, status, sigc.gpg_output); signature_check_clear(&sigc); out: @@ -585,8 +585,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.output) - strbuf_addstr(&verify_message, sigc.output); + if (sigc.gpg_output) + strbuf_addstr(&verify_message, sigc.gpg_output); else strbuf_addstr(&verify_message, "No signature\n"); signature_check_clear(&sigc); diff --git a/pretty.c b/pretty.c index fe95107ae5a415..73b5ead5099d3e 100644 --- a/pretty.c +++ b/pretty.c @@ -1436,8 +1436,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.output) - strbuf_addstr(sb, c->signature_check.output); + if (c->signature_check.gpg_output) + strbuf_addstr(sb, c->signature_check.gpg_output); break; case '?': switch (c->signature_check.result) { diff --git a/send-pack.c b/send-pack.c index bc0fcdbb000769..b3a495b7b1998f 100644 --- a/send-pack.c +++ b/send-pack.c @@ -341,13 +341,13 @@ static int generate_push_cert(struct strbuf *req_buf, { const struct ref *ref; struct string_list_item *item; - char *signing_key_id = xstrdup(get_signing_key_id()); + char *signing_key = xstrdup(get_signing_key()); const char *cp, *np; struct strbuf cert = STRBUF_INIT; int update_seen = 0; strbuf_addstr(&cert, "certificate version 0.1\n"); - strbuf_addf(&cert, "pusher %s ", signing_key_id); + strbuf_addf(&cert, "pusher %s ", signing_key); datestamp(&cert); strbuf_addch(&cert, '\n'); if (args->url && *args->url) { @@ -374,7 +374,7 @@ static int generate_push_cert(struct strbuf *req_buf, if (!update_seen) goto free_return; - if (sign_buffer(&cert, &cert, get_signing_key())) + if (sign_buffer(&cert, &cert, signing_key)) die(_("failed to sign the push certificate")); packet_buf_write(req_buf, "push-cert%c%s", 0, cap_string); @@ -386,7 +386,7 @@ static int generate_push_cert(struct strbuf *req_buf, packet_buf_write(req_buf, "push-cert-end\n"); free_return: - free(signing_key_id); + free(signing_key); strbuf_release(&cert); return update_seen; } diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh index f99ef3e859dd43..9fc5241228e800 100644 --- a/t/lib-gpg.sh +++ b/t/lib-gpg.sh @@ -87,34 +87,6 @@ test_lazy_prereq RFC1991 ' echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991 >/dev/null ' -GPGSSH_KEY_PRIMARY="${GNUPGHOME}/ed25519_ssh_signing_key" -GPGSSH_KEY_SECONDARY="${GNUPGHOME}/rsa_2048_ssh_signing_key" -GPGSSH_KEY_UNTRUSTED="${GNUPGHOME}/untrusted_ssh_signing_key" -GPGSSH_KEY_WITH_PASSPHRASE="${GNUPGHOME}/protected_ssh_signing_key" -GPGSSH_KEY_PASSPHRASE="super_secret" -GPGSSH_ALLOWED_SIGNERS="${GNUPGHOME}/ssh.all_valid.allowedSignersFile" - -GPGSSH_GOOD_SIGNATURE_TRUSTED='Good "git" signature for' -GPGSSH_GOOD_SIGNATURE_UNTRUSTED='Good "git" signature with' -GPGSSH_KEY_NOT_TRUSTED="No principal matched" -GPGSSH_BAD_SIGNATURE="Signature verification failed" - -test_lazy_prereq GPGSSH ' - ssh_version=$(ssh-keygen -Y find-principals -n "git" 2>&1) - test $? != 127 || exit 1 - echo $ssh_version | grep -q "find-principals:missing signature file" - test $? = 0 || exit 1; - mkdir -p "${GNUPGHOME}" && - chmod 0700 "${GNUPGHOME}" && - ssh-keygen -t ed25519 -N "" -C "git ed25519 key" -f "${GPGSSH_KEY_PRIMARY}" >/dev/null && - echo "\"principal with number 1\" $(cat "${GPGSSH_KEY_PRIMARY}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" && - ssh-keygen -t rsa -b 2048 -N "" -C "git rsa2048 key" -f "${GPGSSH_KEY_SECONDARY}" >/dev/null && - echo "\"principal with number 2\" $(cat "${GPGSSH_KEY_SECONDARY}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" && - ssh-keygen -t ed25519 -N "${GPGSSH_KEY_PASSPHRASE}" -C "git ed25519 encrypted key" -f "${GPGSSH_KEY_WITH_PASSPHRASE}" >/dev/null && - echo "\"principal with number 3\" $(cat "${GPGSSH_KEY_WITH_PASSPHRASE}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" && - ssh-keygen -t ed25519 -N "" -f "${GPGSSH_KEY_UNTRUSTED}" >/dev/null -' - sanitize_pgp() { perl -ne ' /^-----END PGP/ and $in_pgp = 0; diff --git a/t/t4202-log.sh b/t/t4202-log.sh index 6a650dacd6e70b..9dfead936b7b5b 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -1616,16 +1616,6 @@ test_expect_success GPGSM 'setup signed branch x509' ' git commit -S -m signed_commit ' -test_expect_success GPGSSH 'setup sshkey signed branch' ' - test_config gpg.format ssh && - test_config user.signingkey "${GPGSSH_KEY_PRIMARY}" && - test_when_finished "git reset --hard && git checkout main" && - git checkout -b signed-ssh main && - echo foo >foo && - git add foo && - git commit -S -m signed_commit -' - test_expect_success GPGSM 'log x509 fingerprint' ' echo "F8BF62E0693D0694816377099909C779FA23FD65 | " >expect && git log -n1 --format="%GF | %GP" signed-x509 >actual && @@ -1638,13 +1628,6 @@ test_expect_success GPGSM 'log OpenPGP fingerprint' ' test_cmp expect actual ' -test_expect_success GPGSSH 'log ssh key fingerprint' ' - test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - ssh-keygen -lf "${GPGSSH_KEY_PRIMARY}" | awk "{print \$2\" | \"}" >expect && - git log -n1 --format="%GF | %GP" signed-ssh >actual && - test_cmp expect actual -' - test_expect_success GPG 'log --graph --show-signature' ' git log --graph --show-signature -n1 signed >actual && grep "^| gpg: Signature made" actual && @@ -1657,12 +1640,6 @@ test_expect_success GPGSM 'log --graph --show-signature x509' ' grep "^| gpgsm: Good signature" actual ' -test_expect_success GPGSSH 'log --graph --show-signature ssh' ' - test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - git log --graph --show-signature -n1 signed-ssh >actual && - grep "${GOOD_SIGNATURE_TRUSTED}" actual -' - test_expect_success GPG 'log --graph --show-signature for merged tag' ' test_when_finished "git reset --hard && git checkout main" && git checkout -b plain main && diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh index 24d374adbae884..bba768f5ded1fc 100755 --- a/t/t5534-push-signed.sh +++ b/t/t5534-push-signed.sh @@ -137,53 +137,6 @@ test_expect_success GPG 'signed push sends push certificate' ' test_cmp expect dst/push-cert-status ' -test_expect_success GPGSSH 'ssh signed push sends push certificate' ' - prepare_dst && - mkdir -p dst/.git/hooks && - git -C dst config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - git -C dst config receive.certnonceseed sekrit && - write_script dst/.git/hooks/post-receive <<-\EOF && - # discard the update list - cat >/dev/null - # record the push certificate - if test -n "${GIT_PUSH_CERT-}" - then - git cat-file blob $GIT_PUSH_CERT >../push-cert - fi && - - cat >../push-cert-status <expect && - - noop=$(git rev-parse noop) && - ff=$(git rev-parse ff) && - noff=$(git rev-parse noff) && - grep "$noop $ff refs/heads/ff" dst/push-cert && - grep "$noop $noff refs/heads/noff" dst/push-cert && - test_cmp expect dst/push-cert-status -' - test_expect_success GPG 'inconsistent push options in signed push not allowed' ' # First, invoke receive-pack with dummy input to obtain its preamble. prepare_dst && @@ -323,60 +276,6 @@ test_expect_success GPGSM 'fail without key and heed user.signingkey x509' ' test_cmp expect dst/push-cert-status ' -test_expect_success GPGSSH 'fail without key and heed user.signingkey ssh' ' - test_config gpg.format ssh && - prepare_dst && - mkdir -p dst/.git/hooks && - git -C dst config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - git -C dst config receive.certnonceseed sekrit && - write_script dst/.git/hooks/post-receive <<-\EOF && - # discard the update list - cat >/dev/null - # record the push certificate - if test -n "${GIT_PUSH_CERT-}" - then - git cat-file blob $GIT_PUSH_CERT >../push-cert - fi && - - cat >../push-cert-status <expect && - - noop=$(git rev-parse noop) && - ff=$(git rev-parse ff) && - noff=$(git rev-parse noff) && - grep "$noop $ff refs/heads/ff" dst/push-cert && - grep "$noop $noff refs/heads/noff" dst/push-cert && - test_cmp expect dst/push-cert-status -' - test_expect_success GPG 'failed atomic push does not execute GPG' ' prepare_dst && git -C dst config receive.certnonceseed sekrit && diff --git a/t/t7031-verify-tag-signed-ssh.sh b/t/t7031-verify-tag-signed-ssh.sh deleted file mode 100755 index 06c9dd6c9339f2..00000000000000 --- a/t/t7031-verify-tag-signed-ssh.sh +++ /dev/null @@ -1,161 +0,0 @@ -#!/bin/sh - -test_description='signed tag tests' -GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main -export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME - -. ./test-lib.sh -. "$TEST_DIRECTORY/lib-gpg.sh" - -test_expect_success GPGSSH 'create signed tags ssh' ' - test_when_finished "test_unconfig commit.gpgsign" && - test_config gpg.format ssh && - test_config user.signingkey "${GPGSSH_KEY_PRIMARY}" && - - echo 1 >file && git add file && - test_tick && git commit -m initial && - git tag -s -m initial initial && - git branch side && - - echo 2 >file && test_tick && git commit -a -m second && - git tag -s -m second second && - - git checkout side && - echo 3 >elif && git add elif && - test_tick && git commit -m "third on side" && - - git checkout main && - test_tick && git merge -S side && - git tag -s -m merge merge && - - echo 4 >file && test_tick && git commit -a -S -m "fourth unsigned" && - git tag -a -m fourth-unsigned fourth-unsigned && - - test_tick && git commit --amend -S -m "fourth signed" && - git tag -s -m fourth fourth-signed && - - echo 5 >file && test_tick && git commit -a -m "fifth" && - git tag fifth-unsigned && - - git config commit.gpgsign true && - echo 6 >file && test_tick && git commit -a -m "sixth" && - git tag -a -m sixth sixth-unsigned && - - test_tick && git rebase -f HEAD^^ && git tag -s -m 6th sixth-signed HEAD^ && - git tag -m seventh -s seventh-signed && - - echo 8 >file && test_tick && git commit -a -m eighth && - git tag -u"${GPGSSH_KEY_UNTRUSTED}" -m eighth eighth-signed-alt -' - -test_expect_success GPGSSH 'verify and show ssh signatures' ' - test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - ( - for tag in initial second merge fourth-signed sixth-signed seventh-signed - do - git verify-tag $tag 2>actual && - grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual && - ! grep "${GPGSSH_BAD_SIGNATURE}" actual && - echo $tag OK || exit 1 - done - ) && - ( - for tag in fourth-unsigned fifth-unsigned sixth-unsigned - do - test_must_fail git verify-tag $tag 2>actual && - ! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual && - ! grep "${GPGSSH_BAD_SIGNATURE}" actual && - echo $tag OK || exit 1 - done - ) && - ( - for tag in eighth-signed-alt - do - test_must_fail git verify-tag $tag 2>actual && - grep "${GPGSSH_GOOD_SIGNATURE_UNTRUSTED}" actual && - ! grep "${GPGSSH_BAD_SIGNATURE}" actual && - grep "${GPGSSH_KEY_NOT_TRUSTED}" actual && - echo $tag OK || exit 1 - done - ) -' - -test_expect_success GPGSSH 'detect fudged ssh signature' ' - test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - git cat-file tag seventh-signed >raw && - sed -e "/^tag / s/seventh/7th forged/" raw >forged1 && - git hash-object -w -t tag forged1 >forged1.tag && - test_must_fail git verify-tag $(cat forged1.tag) 2>actual1 && - grep "${GPGSSH_BAD_SIGNATURE}" actual1 && - ! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual1 && - ! grep "${GPGSSH_GOOD_SIGNATURE_UNTRUSTED}" actual1 -' - -test_expect_success GPGSSH 'verify ssh signatures with --raw' ' - test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - ( - for tag in initial second merge fourth-signed sixth-signed seventh-signed - do - git verify-tag --raw $tag 2>actual && - grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual && - ! grep "${GPGSSH_BAD_SIGNATURE}" actual && - echo $tag OK || exit 1 - done - ) && - ( - for tag in fourth-unsigned fifth-unsigned sixth-unsigned - do - test_must_fail git verify-tag --raw $tag 2>actual && - ! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual && - ! grep "${GPGSSH_BAD_SIGNATURE}" actual && - echo $tag OK || exit 1 - done - ) && - ( - for tag in eighth-signed-alt - do - test_must_fail git verify-tag --raw $tag 2>actual && - grep "${GPGSSH_GOOD_SIGNATURE_UNTRUSTED}" actual && - ! grep "${GPGSSH_BAD_SIGNATURE}" actual && - echo $tag OK || exit 1 - done - ) -' - -test_expect_success GPGSSH 'verify signatures with --raw ssh' ' - test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - git verify-tag --raw sixth-signed 2>actual && - grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual && - ! grep "${GPGSSH_BAD_SIGNATURE}" actual && - echo sixth-signed OK -' - -test_expect_success GPGSSH 'verify multiple tags ssh' ' - test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - tags="seventh-signed sixth-signed" && - for i in $tags - do - git verify-tag -v --raw $i || return 1 - done >expect.stdout 2>expect.stderr.1 && - grep "^${GPGSSH_GOOD_SIGNATURE_TRUSTED}" expect.stderr && - git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 && - grep "^${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual.stderr && - test_cmp expect.stdout actual.stdout && - test_cmp expect.stderr actual.stderr -' - -test_expect_success GPGSSH 'verifying tag with --format - ssh' ' - test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - cat >expect <<-\EOF && - tagname : fourth-signed - EOF - git verify-tag --format="tagname : %(tag)" "fourth-signed" >actual && - test_cmp expect actual -' - -test_expect_success GPGSSH 'verifying a forged tag with --format should fail silently - ssh' ' - test_must_fail git verify-tag --format="tagname : %(tag)" $(cat forged1.tag) >actual-forged && - test_must_be_empty actual-forged -' - -test_done diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh index d65a0171f29c85..8df5a74f1db4ec 100755 --- a/t/t7510-signed-commit.sh +++ b/t/t7510-signed-commit.sh @@ -71,25 +71,7 @@ test_expect_success GPG 'create signed commits' ' git tag eleventh-signed $(cat oid) && echo 12 | git commit-tree --gpg-sign=B7227189 HEAD^{tree} >oid && test_line_count = 1 oid && - git tag twelfth-signed-alt $(cat oid) && - - cat >keydetails <<-\EOF && - Key-Type: RSA - Key-Length: 2048 - Subkey-Type: RSA - Subkey-Length: 2048 - Name-Real: Unknown User - Name-Email: unknown@git.com - Expire-Date: 0 - %no-ask-passphrase - %no-protection - EOF - gpg --batch --gen-key keydetails && - echo 13 >file && git commit -a -S"unknown@git.com" -m thirteenth && - git tag thirteenth-signed && - DELETE_FINGERPRINT=$(gpg -K --with-colons --fingerprint --batch unknown@git.com | grep "^fpr" | head -n 1 | awk -F ":" "{print \$10;}") && - gpg --batch --yes --delete-secret-keys $DELETE_FINGERPRINT && - gpg --batch --yes --delete-keys unknown@git.com + git tag twelfth-signed-alt $(cat oid) ' test_expect_success GPG 'verify and show signatures' ' @@ -128,13 +110,6 @@ test_expect_success GPG 'verify and show signatures' ' ) ' -test_expect_success GPG 'verify-commit exits failure on unknown signature' ' - test_must_fail git verify-commit thirteenth-signed 2>actual && - ! grep "Good signature from" actual && - ! grep "BAD signature from" actual && - grep -q -F -e "No public key" -e "public key not found" actual -' - test_expect_success GPG 'verify-commit exits success on untrusted signature' ' git verify-commit eighth-signed-alt 2>actual && grep "Good signature from" actual && @@ -363,8 +338,6 @@ test_expect_success GPG 'show double signature with custom format' ' ' -# NEEDSWORK: This test relies on the test_tick commit/author dates from the first -# 'create signed commits' test even though it creates its own test_expect_success GPG 'verify-commit verifies multiply signed commits' ' git init multiply-signed && cd multiply-signed && diff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh deleted file mode 100755 index 3e093168eef571..00000000000000 --- a/t/t7528-signed-commit-ssh.sh +++ /dev/null @@ -1,398 +0,0 @@ -#!/bin/sh - -test_description='ssh signed commit tests' -GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main -export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME - -. ./test-lib.sh -GNUPGHOME_NOT_USED=$GNUPGHOME -. "$TEST_DIRECTORY/lib-gpg.sh" - -test_expect_success GPGSSH 'create signed commits' ' - test_oid_cache <<-\EOF && - header sha1:gpgsig - header sha256:gpgsig-sha256 - EOF - - test_when_finished "test_unconfig commit.gpgsign" && - test_config gpg.format ssh && - test_config user.signingkey "${GPGSSH_KEY_PRIMARY}" && - - echo 1 >file && git add file && - test_tick && git commit -S -m initial && - git tag initial && - git branch side && - - echo 2 >file && test_tick && git commit -a -S -m second && - git tag second && - - git checkout side && - echo 3 >elif && git add elif && - test_tick && git commit -m "third on side" && - - git checkout main && - test_tick && git merge -S side && - git tag merge && - - echo 4 >file && test_tick && git commit -a -m "fourth unsigned" && - git tag fourth-unsigned && - - test_tick && git commit --amend -S -m "fourth signed" && - git tag fourth-signed && - - git config commit.gpgsign true && - echo 5 >file && test_tick && git commit -a -m "fifth signed" && - git tag fifth-signed && - - git config commit.gpgsign false && - echo 6 >file && test_tick && git commit -a -m "sixth" && - git tag sixth-unsigned && - - git config commit.gpgsign true && - echo 7 >file && test_tick && git commit -a -m "seventh" --no-gpg-sign && - git tag seventh-unsigned && - - test_tick && git rebase -f HEAD^^ && git tag sixth-signed HEAD^ && - git tag seventh-signed && - - echo 8 >file && test_tick && git commit -a -m eighth -S"${GPGSSH_KEY_UNTRUSTED}" && - git tag eighth-signed-alt && - - # commit.gpgsign is still on but this must not be signed - echo 9 | git commit-tree HEAD^{tree} >oid && - test_line_count = 1 oid && - git tag ninth-unsigned $(cat oid) && - # explicit -S of course must sign. - echo 10 | git commit-tree -S HEAD^{tree} >oid && - test_line_count = 1 oid && - git tag tenth-signed $(cat oid) && - - # --gpg-sign[=] must sign. - echo 11 | git commit-tree --gpg-sign HEAD^{tree} >oid && - test_line_count = 1 oid && - git tag eleventh-signed $(cat oid) && - echo 12 | git commit-tree --gpg-sign="${GPGSSH_KEY_UNTRUSTED}" HEAD^{tree} >oid && - test_line_count = 1 oid && - git tag twelfth-signed-alt $(cat oid) -' - -test_expect_success GPGSSH 'verify and show signatures' ' - test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - test_config gpg.mintrustlevel UNDEFINED && - ( - for commit in initial second merge fourth-signed \ - fifth-signed sixth-signed seventh-signed tenth-signed \ - eleventh-signed - do - git verify-commit $commit && - git show --pretty=short --show-signature $commit >actual && - grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual && - ! grep "${GPGSSH_BAD_SIGNATURE}" actual && - echo $commit OK || exit 1 - done - ) && - ( - for commit in merge^2 fourth-unsigned sixth-unsigned \ - seventh-unsigned ninth-unsigned - do - test_must_fail git verify-commit $commit && - git show --pretty=short --show-signature $commit >actual && - ! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual && - ! grep "${GPGSSH_BAD_SIGNATURE}" actual && - echo $commit OK || exit 1 - done - ) && - ( - for commit in eighth-signed-alt twelfth-signed-alt - do - git show --pretty=short --show-signature $commit >actual && - grep "${GPGSSH_GOOD_SIGNATURE_UNTRUSTED}" actual && - ! grep "${GPGSSH_BAD_SIGNATURE}" actual && - grep "${KEY_NOT_TRUSTED}" actual && - echo $commit OK || exit 1 - done - ) -' - -test_expect_success GPGSSH 'verify-commit exits failure on untrusted signature' ' - test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - test_must_fail git verify-commit eighth-signed-alt 2>actual && - grep "${GPGSSH_GOOD_SIGNATURE_UNTRUSTED}" actual && - ! grep "${GPGSSH_BAD_SIGNATURE}" actual && - grep "${KEY_NOT_TRUSTED}" actual -' - -test_expect_success GPGSSH 'verify-commit exits success with matching minTrustLevel' ' - test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - test_config gpg.minTrustLevel fully && - git verify-commit sixth-signed -' - -test_expect_success GPGSSH 'verify-commit exits success with low minTrustLevel' ' - test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - test_config gpg.minTrustLevel marginal && - git verify-commit sixth-signed -' - -test_expect_success GPGSSH 'verify-commit exits failure with high minTrustLevel' ' - test_config gpg.minTrustLevel ultimate && - test_must_fail git verify-commit eighth-signed-alt -' - -test_expect_success GPGSSH 'verify signatures with --raw' ' - test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - ( - for commit in initial second merge fourth-signed fifth-signed sixth-signed seventh-signed - do - git verify-commit --raw $commit 2>actual && - grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual && - ! grep "${GPGSSH_BAD_SIGNATURE}" actual && - echo $commit OK || exit 1 - done - ) && - ( - for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned - do - test_must_fail git verify-commit --raw $commit 2>actual && - ! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual && - ! grep "${GPGSSH_BAD_SIGNATURE}" actual && - echo $commit OK || exit 1 - done - ) && - ( - for commit in eighth-signed-alt - do - test_must_fail git verify-commit --raw $commit 2>actual && - grep "${GPGSSH_GOOD_SIGNATURE_UNTRUSTED}" actual && - ! grep "${GPGSSH_BAD_SIGNATURE}" actual && - echo $commit OK || exit 1 - done - ) -' - -test_expect_success GPGSSH 'proper header is used for hash algorithm' ' - git cat-file commit fourth-signed >output && - grep "^$(test_oid header) -----BEGIN SSH SIGNATURE-----" output -' - -test_expect_success GPGSSH 'show signed commit with signature' ' - test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - git show -s initial >commit && - git show -s --show-signature initial >show && - git verify-commit -v initial >verify.1 2>verify.2 && - git cat-file commit initial >cat && - grep -v -e "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" -e "Warning: " show >show.commit && - grep -e "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" -e "Warning: " show >show.gpg && - grep -v "^ " cat | grep -v "^gpgsig.* " >cat.commit && - test_cmp show.commit commit && - test_cmp show.gpg verify.2 && - test_cmp cat.commit verify.1 -' - -test_expect_success GPGSSH 'detect fudged signature' ' - test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - git cat-file commit seventh-signed >raw && - sed -e "s/^seventh/7th forged/" raw >forged1 && - git hash-object -w -t commit forged1 >forged1.commit && - test_must_fail git verify-commit $(cat forged1.commit) && - git show --pretty=short --show-signature $(cat forged1.commit) >actual1 && - grep "${GPGSSH_BAD_SIGNATURE}" actual1 && - ! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual1 && - ! grep "${GPGSSH_GOOD_SIGNATURE_UNTRUSTED}" actual1 -' - -test_expect_success GPGSSH 'detect fudged signature with NUL' ' - test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - git cat-file commit seventh-signed >raw && - cat raw >forged2 && - echo Qwik | tr "Q" "\000" >>forged2 && - git hash-object -w -t commit forged2 >forged2.commit && - test_must_fail git verify-commit $(cat forged2.commit) && - git show --pretty=short --show-signature $(cat forged2.commit) >actual2 && - grep "${GPGSSH_BAD_SIGNATURE}" actual2 && - ! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual2 -' - -test_expect_success GPGSSH 'amending already signed commit' ' - test_config gpg.format ssh && - test_config user.signingkey "${GPGSSH_KEY_PRIMARY}" && - test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - git checkout fourth-signed^0 && - git commit --amend -S --no-edit && - git verify-commit HEAD && - git show -s --show-signature HEAD >actual && - grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual && - ! grep "${GPGSSH_BAD_SIGNATURE}" actual -' - -test_expect_success GPGSSH 'show good signature with custom format' ' - test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - FINGERPRINT=$(ssh-keygen -lf "${GPGSSH_KEY_PRIMARY}" | awk "{print \$2;}") && - cat >expect.tmpl <<-\EOF && - G - FINGERPRINT - principal with number 1 - FINGERPRINT - - EOF - sed "s|FINGERPRINT|$FINGERPRINT|g" expect.tmpl >expect && - git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual && - test_cmp expect actual -' - -test_expect_success GPGSSH 'show bad signature with custom format' ' - test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - cat >expect <<-\EOF && - B - - - - - EOF - git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" $(cat forged1.commit) >actual && - test_cmp expect actual -' - -test_expect_success GPGSSH 'show untrusted signature with custom format' ' - test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - cat >expect.tmpl <<-\EOF && - U - FINGERPRINT - - FINGERPRINT - - EOF - git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual && - FINGERPRINT=$(ssh-keygen -lf "${GPGSSH_KEY_UNTRUSTED}" | awk "{print \$2;}") && - sed "s|FINGERPRINT|$FINGERPRINT|g" expect.tmpl >expect && - test_cmp expect actual -' - -test_expect_success GPGSSH 'show untrusted signature with undefined trust level' ' - test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - cat >expect.tmpl <<-\EOF && - undefined - FINGERPRINT - - FINGERPRINT - - EOF - git log -1 --format="%GT%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual && - FINGERPRINT=$(ssh-keygen -lf "${GPGSSH_KEY_UNTRUSTED}" | awk "{print \$2;}") && - sed "s|FINGERPRINT|$FINGERPRINT|g" expect.tmpl >expect && - test_cmp expect actual -' - -test_expect_success GPGSSH 'show untrusted signature with ultimate trust level' ' - test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - cat >expect.tmpl <<-\EOF && - fully - FINGERPRINT - principal with number 1 - FINGERPRINT - - EOF - git log -1 --format="%GT%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual && - FINGERPRINT=$(ssh-keygen -lf "${GPGSSH_KEY_PRIMARY}" | awk "{print \$2;}") && - sed "s|FINGERPRINT|$FINGERPRINT|g" expect.tmpl >expect && - test_cmp expect actual -' - -test_expect_success GPGSSH 'show lack of signature with custom format' ' - cat >expect <<-\EOF && - N - - - - - EOF - git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" seventh-unsigned >actual && - test_cmp expect actual -' - -test_expect_success GPGSSH 'log.showsignature behaves like --show-signature' ' - test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - test_config log.showsignature true && - git show initial >actual && - grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual -' - -test_expect_success GPGSSH 'check config gpg.format values' ' - test_config gpg.format ssh && - test_config user.signingkey "${GPGSSH_KEY_PRIMARY}" && - test_config gpg.format ssh && - git commit -S --amend -m "success" && - test_config gpg.format OpEnPgP && - test_must_fail git commit -S --amend -m "fail" -' - -test_expect_failure GPGSSH 'detect fudged commit with double signature (TODO)' ' - sed -e "/gpgsig/,/END PGP/d" forged1 >double-base && - sed -n -e "/gpgsig/,/END PGP/p" forged1 | \ - sed -e "s/^$(test_oid header)//;s/^ //" | gpg --dearmor >double-sig1.sig && - gpg -o double-sig2.sig -u 29472784 --detach-sign double-base && - cat double-sig1.sig double-sig2.sig | gpg --enarmor >double-combined.asc && - sed -e "s/^\(-.*\)ARMORED FILE/\1SIGNATURE/;1s/^/$(test_oid header) /;2,\$s/^/ /" \ - double-combined.asc > double-gpgsig && - sed -e "/committer/r double-gpgsig" double-base >double-commit && - git hash-object -w -t commit double-commit >double-commit.commit && - test_must_fail git verify-commit $(cat double-commit.commit) && - git show --pretty=short --show-signature $(cat double-commit.commit) >double-actual && - grep "BAD signature from" double-actual && - grep "Good signature from" double-actual -' - -test_expect_failure GPGSSH 'show double signature with custom format (TODO)' ' - cat >expect <<-\EOF && - E - - - - - EOF - git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" $(cat double-commit.commit) >actual && - test_cmp expect actual -' - - -test_expect_failure GPGSSH 'verify-commit verifies multiply signed commits (TODO)' ' - git init multiply-signed && - cd multiply-signed && - test_commit first && - echo 1 >second && - git add second && - tree=$(git write-tree) && - parent=$(git rev-parse HEAD^{commit}) && - git commit --gpg-sign -m second && - git cat-file commit HEAD && - # Avoid trailing whitespace. - sed -e "s/^Q//" -e "s/^Z/ /" >commit <<-EOF && - Qtree $tree - Qparent $parent - Qauthor A U Thor 1112912653 -0700 - Qcommitter C O Mitter 1112912653 -0700 - Qgpgsig -----BEGIN PGP SIGNATURE----- - QZ - Q iHQEABECADQWIQRz11h0S+chaY7FTocTtvUezd5DDQUCX/uBDRYcY29tbWl0dGVy - Q QGV4YW1wbGUuY29tAAoJEBO29R7N3kMNd+8AoK1I8mhLHviPH+q2I5fIVgPsEtYC - Q AKCTqBh+VabJceXcGIZuF0Ry+udbBQ== - Q =tQ0N - Q -----END PGP SIGNATURE----- - Qgpgsig-sha256 -----BEGIN PGP SIGNATURE----- - QZ - Q iHQEABECADQWIQRz11h0S+chaY7FTocTtvUezd5DDQUCX/uBIBYcY29tbWl0dGVy - Q QGV4YW1wbGUuY29tAAoJEBO29R7N3kMN/NEAn0XO9RYSBj2dFyozi0JKSbssYMtO - Q AJwKCQ1BQOtuwz//IjU8TiS+6S4iUw== - Q =pIwP - Q -----END PGP SIGNATURE----- - Q - Qsecond - EOF - head=$(git hash-object -t commit -w commit) && - git reset --hard $head && - git verify-commit $head 2>actual && - grep "Good signature from" actual && - ! grep "BAD signature from" actual -' - -test_done From 03b6d62271f76ee1aa515ff403bd2f15efe542bf Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Mon, 4 Oct 2021 16:57:39 +0000 Subject: [PATCH 11/23] tmp-objdir: new API for creating temporary writable databases The tmp_objdir API provides the ability to create temporary object directories, but was designed with the goal of having subprocesses access these object stores, followed by the main process migrating objects from it to the main object store or just deleting it. The subprocesses would view it as their primary datastore and write to it. Here we add the tmp_objdir_replace_primary_odb function that replaces the current process's writable "main" object directory with the specified one. The previous main object directory is restored in either tmp_objdir_migrate or tmp_objdir_destroy. For the --remerge-diff usecase, add a new `will_destroy` flag in `struct object_database` to mark ephemeral object databases that do not require fsync durability. Add 'git prune' support for removing temporary object databases, and make sure that they have a name starting with tmp_ and containing an operation-specific name. Based-on-patch-by: Elijah Newren Signed-off-by: Neeraj Singh Signed-off-by: Junio C Hamano --- builtin/prune.c | 22 +++++++++++++++++---- builtin/receive-pack.c | 2 +- object-file.c | 44 ++++++++++++++++++++++++++++++++++++++++-- object-store.h | 19 ++++++++++++++++++ object.c | 2 +- tmp-objdir.c | 30 +++++++++++++++++++++++++--- tmp-objdir.h | 14 +++++++++++--- 7 files changed, 119 insertions(+), 14 deletions(-) diff --git a/builtin/prune.c b/builtin/prune.c index 02c6ab7cbaafba..9c72ecf5a58bbc 100644 --- a/builtin/prune.c +++ b/builtin/prune.c @@ -18,6 +18,7 @@ static int show_only; static int verbose; static timestamp_t expire; static int show_progress = -1; +static struct strbuf remove_dir_buf = STRBUF_INIT; static int prune_tmp_file(const char *fullpath) { @@ -26,10 +27,19 @@ static int prune_tmp_file(const char *fullpath) return error("Could not stat '%s'", fullpath); if (st.st_mtime > expire) return 0; - if (show_only || verbose) - printf("Removing stale temporary file %s\n", fullpath); - if (!show_only) - unlink_or_warn(fullpath); + if (S_ISDIR(st.st_mode)) { + if (show_only || verbose) + printf("Removing stale temporary directory %s\n", fullpath); + if (!show_only) { + strbuf_addstr(&remove_dir_buf, fullpath); + remove_dir_recursively(&remove_dir_buf, 0); + } + } else { + if (show_only || verbose) + printf("Removing stale temporary file %s\n", fullpath); + if (!show_only) + unlink_or_warn(fullpath); + } return 0; } @@ -97,6 +107,9 @@ static int prune_cruft(const char *basename, const char *path, void *data) static int prune_subdir(unsigned int nr, const char *path, void *data) { + if (verbose) + printf("Removing directory %s\n", path); + if (!show_only) rmdir(path); return 0; @@ -185,5 +198,6 @@ int cmd_prune(int argc, const char **argv, const char *prefix) prune_shallow(show_only ? PRUNE_SHOW_ONLY : 0); } + strbuf_release(&remove_dir_buf); return 0; } diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 48960a9575b3fe..418a42ca069e97 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -2208,7 +2208,7 @@ static const char *unpack(int err_fd, struct shallow_info *si) strvec_push(&child.args, alt_shallow_file); } - tmp_objdir = tmp_objdir_create(); + tmp_objdir = tmp_objdir_create("incoming"); if (!tmp_objdir) { if (err_fd > 0) close(err_fd); diff --git a/object-file.c b/object-file.c index be4f94ecf3bc1f..990381abee57cd 100644 --- a/object-file.c +++ b/object-file.c @@ -751,6 +751,43 @@ void add_to_alternates_memory(const char *reference) '\n', NULL, 0); } +struct object_directory *set_temporary_primary_odb(const char *dir, int will_destroy) +{ + struct object_directory *new_odb; + + /* + * Make sure alternates are initialized, or else our entry may be + * overwritten when they are. + */ + prepare_alt_odb(the_repository); + + /* + * Make a new primary odb and link the old primary ODB in as an + * alternate + */ + new_odb = xcalloc(1, sizeof(*new_odb)); + new_odb->path = xstrdup(dir); + new_odb->will_destroy = will_destroy; + new_odb->next = the_repository->objects->odb; + the_repository->objects->odb = new_odb; + return new_odb->next; +} + +void restore_primary_odb(struct object_directory *restore_odb, const char *old_path) +{ + struct object_directory *cur_odb = the_repository->objects->odb; + + if (strcmp(old_path, cur_odb->path)) + BUG("expected %s as primary object store; found %s", + old_path, cur_odb->path); + + if (cur_odb->next != restore_odb) + BUG("we expect the old primary object store to be the first alternate"); + + the_repository->objects->odb = restore_odb; + free_object_directory(cur_odb); +} + /* * Compute the exact path an alternate is at and returns it. In case of * error NULL is returned and the human readable error is added to `err` @@ -1888,8 +1925,11 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf, /* Finalize a file on disk, and close it. */ static void close_loose_object(int fd) { - if (fsync_object_files) - fsync_or_die(fd, "loose object file"); + if (!the_repository->objects->odb->will_destroy) { + if (fsync_object_files) + fsync_or_die(fd, "loose object file"); + } + if (close(fd) != 0) die_errno(_("error when closing loose object file")); } diff --git a/object-store.h b/object-store.h index c5130d8baea54b..74b1b5872a6adf 100644 --- a/object-store.h +++ b/object-store.h @@ -27,6 +27,11 @@ struct object_directory { uint32_t loose_objects_subdir_seen[8]; /* 256 bits */ struct oidtree *loose_objects_cache; + /* + * This object store is ephemeral, so there is no need to fsync. + */ + int will_destroy; + /* * Path to the alternative object store. If this is a relative path, * it is relative to the current working directory. @@ -58,6 +63,17 @@ void add_to_alternates_file(const char *dir); */ void add_to_alternates_memory(const char *dir); +/* + * Replace the current writable object directory with the specified temporary + * object directory; returns the former primary object directory. + */ +struct object_directory *set_temporary_primary_odb(const char *dir, int will_destroy); + +/* + * Restore a previous ODB replaced by set_temporary_main_odb. + */ +void restore_primary_odb(struct object_directory *restore_odb, const char *old_path); + /* * Populate and return the loose object cache array corresponding to the * given object ID. @@ -68,6 +84,9 @@ struct oidtree *odb_loose_cache(struct object_directory *odb, /* Empty the loose object cache for the specified object directory. */ void odb_clear_loose_cache(struct object_directory *odb); +/* Clear and free the specified object directory */ +void free_object_directory(struct object_directory *odb); + struct packed_git { struct hashmap_entry packmap_ent; struct packed_git *next; diff --git a/object.c b/object.c index 4e85955a941168..98635bc4043625 100644 --- a/object.c +++ b/object.c @@ -513,7 +513,7 @@ struct raw_object_store *raw_object_store_new(void) return o; } -static void free_object_directory(struct object_directory *odb) +void free_object_directory(struct object_directory *odb) { free(odb->path); odb_clear_loose_cache(odb); diff --git a/tmp-objdir.c b/tmp-objdir.c index b8d880e3626a9c..45d42a7bcf0ba6 100644 --- a/tmp-objdir.c +++ b/tmp-objdir.c @@ -11,6 +11,7 @@ struct tmp_objdir { struct strbuf path; struct strvec env; + struct object_directory *prev_odb; }; /* @@ -38,6 +39,9 @@ static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal) if (t == the_tmp_objdir) the_tmp_objdir = NULL; + if (!on_signal && t->prev_odb) + restore_primary_odb(t->prev_odb, t->path.buf); + /* * This may use malloc via strbuf_grow(), but we should * have pre-grown t->path sufficiently so that this @@ -52,6 +56,7 @@ static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal) */ if (!on_signal) tmp_objdir_free(t); + return err; } @@ -121,7 +126,7 @@ static int setup_tmp_objdir(const char *root) return ret; } -struct tmp_objdir *tmp_objdir_create(void) +struct tmp_objdir *tmp_objdir_create(const char *prefix) { static int installed_handlers; struct tmp_objdir *t; @@ -129,11 +134,16 @@ struct tmp_objdir *tmp_objdir_create(void) if (the_tmp_objdir) BUG("only one tmp_objdir can be used at a time"); - t = xmalloc(sizeof(*t)); + t = xcalloc(1, sizeof(*t)); strbuf_init(&t->path, 0); strvec_init(&t->env); - strbuf_addf(&t->path, "%s/incoming-XXXXXX", get_object_directory()); + /* + * Use a string starting with tmp_ so that the builtin/prune.c code + * can recognize any stale objdirs left behind by a crash and delete + * them. + */ + strbuf_addf(&t->path, "%s/tmp_objdir-%s-XXXXXX", get_object_directory(), prefix); /* * Grow the strbuf beyond any filename we expect to be placed in it. @@ -269,6 +279,13 @@ int tmp_objdir_migrate(struct tmp_objdir *t) if (!t) return 0; + if (t->prev_odb) { + if (the_repository->objects->odb->will_destroy) + BUG("migrating an ODB that was marked for destruction"); + restore_primary_odb(t->prev_odb, t->path.buf); + t->prev_odb = NULL; + } + strbuf_addbuf(&src, &t->path); strbuf_addstr(&dst, get_object_directory()); @@ -292,3 +309,10 @@ void tmp_objdir_add_as_alternate(const struct tmp_objdir *t) { add_to_alternates_memory(t->path.buf); } + +void tmp_objdir_replace_primary_odb(struct tmp_objdir *t, int will_destroy) +{ + if (t->prev_odb) + BUG("the primary object database is already replaced"); + t->prev_odb = set_temporary_primary_odb(t->path.buf, will_destroy); +} diff --git a/tmp-objdir.h b/tmp-objdir.h index b1e45b4c75d2d8..75754cbfba618e 100644 --- a/tmp-objdir.h +++ b/tmp-objdir.h @@ -10,7 +10,7 @@ * * Example: * - * struct tmp_objdir *t = tmp_objdir_create(); + * struct tmp_objdir *t = tmp_objdir_create("incoming"); * if (!run_command_v_opt_cd_env(cmd, 0, NULL, tmp_objdir_env(t)) && * !tmp_objdir_migrate(t)) * printf("success!\n"); @@ -22,9 +22,10 @@ struct tmp_objdir; /* - * Create a new temporary object directory; returns NULL on failure. + * Create a new temporary object directory with the specified prefix; + * returns NULL on failure. */ -struct tmp_objdir *tmp_objdir_create(void); +struct tmp_objdir *tmp_objdir_create(const char *prefix); /* * Return a list of environment strings, suitable for use with @@ -51,4 +52,11 @@ int tmp_objdir_destroy(struct tmp_objdir *); */ void tmp_objdir_add_as_alternate(const struct tmp_objdir *); +/* + * Replaces the main object store in the current process with the temporary + * object directory and makes the former main object store an alternate. + * If will_destroy is nonzero, the object directory may not be migrated. + */ +void tmp_objdir_replace_primary_odb(struct tmp_objdir *, int will_destroy); + #endif /* TMP_OBJDIR_H */ From 50741b157f2f90df76a60418e2781b2c1e6e3c78 Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Mon, 4 Oct 2021 16:57:40 +0000 Subject: [PATCH 12/23] tmp-objdir: disable ref updates when replacing the primary odb When creating a subprocess with a temporary ODB, we set the GIT_QUARANTINE_ENVIRONMENT env var to tell child Git processes not to update refs, since the tmp-objdir may go away. Introduce a similar mechanism for in-process temporary ODBs when we call tmp_objdir_replace_primary_odb. Now both mechanisms set the disable_ref_updates flag on the odb, which is queried by the ref_transaction_prepare function. Note: This change adds an assumption that the state of the_repository is relevant for any ref transaction that might be initiated. Unwinding this assumption should be straightforward by saving the relevant repository to query in the transaction or the ref_store. Peff's test case was invoking ref updates via the cachetextconv setting. That particular code silently does nothing when a ref update is forbidden. See the call to notes_cache_put in fill_textconv where errors are ignored. Reported-by: Jeff King Signed-off-by: Neeraj Singh Signed-off-by: Junio C Hamano --- environment.c | 4 ++++ object-file.c | 6 ++++++ object-store.h | 9 ++++++++- refs.c | 2 +- repository.c | 2 ++ repository.h | 1 + 6 files changed, 22 insertions(+), 2 deletions(-) diff --git a/environment.c b/environment.c index b4ba4fa22dbe41..46ec5072c059b3 100644 --- a/environment.c +++ b/environment.c @@ -176,6 +176,10 @@ void setup_git_env(const char *git_dir) args.graft_file = getenv_safe(&to_free, GRAFT_ENVIRONMENT); args.index_file = getenv_safe(&to_free, INDEX_ENVIRONMENT); args.alternate_db = getenv_safe(&to_free, ALTERNATE_DB_ENVIRONMENT); + if (getenv(GIT_QUARANTINE_ENVIRONMENT)) { + args.disable_ref_updates = 1; + } + repo_set_gitdir(the_repository, git_dir, &args); strvec_clear(&to_free); diff --git a/object-file.c b/object-file.c index 990381abee57cd..f16441afb938b6 100644 --- a/object-file.c +++ b/object-file.c @@ -767,6 +767,12 @@ struct object_directory *set_temporary_primary_odb(const char *dir, int will_des */ new_odb = xcalloc(1, sizeof(*new_odb)); new_odb->path = xstrdup(dir); + + /* + * Disable ref updates while a temporary odb is active, since + * the objects in the database may roll back. + */ + new_odb->disable_ref_updates = 1; new_odb->will_destroy = will_destroy; new_odb->next = the_repository->objects->odb; the_repository->objects->odb = new_odb; diff --git a/object-store.h b/object-store.h index 74b1b5872a6adf..bd53bdf2f2efbc 100644 --- a/object-store.h +++ b/object-store.h @@ -27,10 +27,17 @@ struct object_directory { uint32_t loose_objects_subdir_seen[8]; /* 256 bits */ struct oidtree *loose_objects_cache; + /* + * This is a temporary object store created by the tmp_objdir + * facility. Disable ref updates since the objects in the store + * might be discarded on rollback. + */ + unsigned int disable_ref_updates : 1; + /* * This object store is ephemeral, so there is no need to fsync. */ - int will_destroy; + unsigned int will_destroy : 1; /* * Path to the alternative object store. If this is a relative path, diff --git a/refs.c b/refs.c index 8b9f7c3a80a0f6..7c182607dcffb4 100644 --- a/refs.c +++ b/refs.c @@ -2126,7 +2126,7 @@ int ref_transaction_prepare(struct ref_transaction *transaction, break; } - if (getenv(GIT_QUARANTINE_ENVIRONMENT)) { + if (the_repository->objects->odb->disable_ref_updates) { strbuf_addstr(err, _("ref updates forbidden inside quarantine environment")); return -1; diff --git a/repository.c b/repository.c index 710a3b4bf872fb..18e0526da01d0e 100644 --- a/repository.c +++ b/repository.c @@ -80,6 +80,8 @@ void repo_set_gitdir(struct repository *repo, expand_base_dir(&repo->objects->odb->path, o->object_dir, repo->commondir, "objects"); + repo->objects->odb->disable_ref_updates = o->disable_ref_updates; + free(repo->objects->alternate_db); repo->objects->alternate_db = xstrdup_or_null(o->alternate_db); expand_base_dir(&repo->graft_file, o->graft_file, diff --git a/repository.h b/repository.h index 3740c93bc0fe27..77316367d998a3 100644 --- a/repository.h +++ b/repository.h @@ -162,6 +162,7 @@ struct set_gitdir_args { const char *graft_file; const char *index_file; const char *alternate_db; + int disable_ref_updates; }; void repo_set_gitdir(struct repository *repo, const char *root, From 4780496f20e2cc09554a84473bd63250391d662a Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Mon, 4 Oct 2021 16:57:41 +0000 Subject: [PATCH 13/23] bulk-checkin: rename 'state' variable and separate 'plugged' boolean Preparation for adding bulk-fsync to the bulk-checkin.c infrastructure. * Rename 'state' variable to 'bulk_checkin_state', since we will later be adding 'bulk_fsync_state'. This also makes the variable easier to find in the debugger, since the name is more unique. * Move the 'plugged' data member of 'bulk_checkin_state' into a separate static variable. Doing this avoids resetting the variable in finish_bulk_checkin when zeroing the 'bulk_checkin_state'. As-is, we seem to unintentionally disable the plugging functionality the first time a new packfile must be created due to packfile size limits. While disabling the plugging state only results in suboptimal behavior for the current code, it would be fatal for the bulk-fsync functionality later in this patch series. Signed-off-by: Neeraj Singh Signed-off-by: Junio C Hamano --- bulk-checkin.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/bulk-checkin.c b/bulk-checkin.c index 8785b2ac806992..6ae18401e04ecd 100644 --- a/bulk-checkin.c +++ b/bulk-checkin.c @@ -10,9 +10,9 @@ #include "packfile.h" #include "object-store.h" -static struct bulk_checkin_state { - unsigned plugged:1; +static int bulk_checkin_plugged; +static struct bulk_checkin_state { char *pack_tmp_name; struct hashfile *f; off_t offset; @@ -21,7 +21,7 @@ static struct bulk_checkin_state { struct pack_idx_entry **written; uint32_t alloc_written; uint32_t nr_written; -} state; +} bulk_checkin_state; static void finish_tmp_packfile(struct strbuf *basename, const char *pack_tmp_name, @@ -277,21 +277,23 @@ int index_bulk_checkin(struct object_id *oid, int fd, size_t size, enum object_type type, const char *path, unsigned flags) { - int status = deflate_to_pack(&state, oid, fd, size, type, + int status = deflate_to_pack(&bulk_checkin_state, oid, fd, size, type, path, flags); - if (!state.plugged) - finish_bulk_checkin(&state); + if (!bulk_checkin_plugged) + finish_bulk_checkin(&bulk_checkin_state); return status; } void plug_bulk_checkin(void) { - state.plugged = 1; + assert(!bulk_checkin_plugged); + bulk_checkin_plugged = 1; } void unplug_bulk_checkin(void) { - state.plugged = 0; - if (state.f) - finish_bulk_checkin(&state); + assert(bulk_checkin_plugged); + bulk_checkin_plugged = 0; + if (bulk_checkin_state.f) + finish_bulk_checkin(&bulk_checkin_state); } From ec983eb5d21a7c3f46de1fb9c35f3f8c15642396 Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Mon, 4 Oct 2021 16:57:42 +0000 Subject: [PATCH 14/23] core.fsyncobjectfiles: batched disk flushes When adding many objects to a repo with core.fsyncObjectFiles set to true, the cost of fsync'ing each object file can become prohibitive. One major source of the cost of fsync is the implied flush of the hardware writeback cache within the disk drive. Fortunately, Windows, and macOS offer mechanisms to write data from the filesystem page cache without initiating a hardware flush. Linux has the sync_file_range API, which issues a pagecache writeback request reliably after version 5.2. This patch introduces a new 'core.fsyncObjectFiles = batch' option that batches up hardware flushes. It hooks into the bulk-checkin plugging and unplugging functionality and takes advantage of tmp-objdir. When the new mode is enabled do the following for each new object: 1. Create the object in a tmp-objdir. 2. Issue a pagecache writeback request and wait for it to complete. At the end of the entire transaction when unplugging bulk checkin: 1. Issue an fsync against a dummy file to flush the hardware writeback cache, which should by now have processed the tmp-objdir writes. 2. Rename all of the tmp-objdir files to their final names. 3. When updating the index and/or refs, we assume that Git will issue another fsync internal to that operation. This is not the case today, but may be a good extension to those components. On a filesystem with a singular journal that is updated during name operations (e.g. create, link, rename, etc), such as NTFS, HFS+, or XFS we would expect the fsync to trigger a journal writeout so that this sequence is enough to ensure that the user's data is durable by the time the git command returns. This change also updates the macOS code to trigger a real hardware flush via fnctl(fd, F_FULLFSYNC) when fsync_or_die is called. Previously, on macOS there was no guarantee of durability since a simple fsync(2) call does not flush any hardware caches. _Performance numbers_: Linux - Hyper-V VM running Kernel 5.11 (Ubuntu 20.04) on a fast SSD. Mac - macOS 11.5.1 running on a Mac mini on a 1TB Apple SSD. Windows - Same host as Linux, a preview version of Windows 11. This number is from a patch later in the series. Adding 500 files to the repo with 'git add' Times reported in seconds. core.fsyncObjectFiles | Linux | Mac | Windows ----------------------|-------|-------|-------- false | 0.06 | 0.35 | 0.61 true | 1.88 | 11.18 | 2.47 batch | 0.15 | 0.41 | 1.53 Signed-off-by: Neeraj Singh Signed-off-by: Junio C Hamano --- Documentation/config/core.txt | 29 +++++++++++---- Makefile | 6 ++++ bulk-checkin.c | 68 +++++++++++++++++++++++++++++++++++ bulk-checkin.h | 2 ++ cache.h | 8 ++++- config.c | 7 +++- config.mak.uname | 1 + configure.ac | 8 +++++ environment.c | 2 +- git-compat-util.h | 7 ++++ object-file.c | 12 ++++++- wrapper.c | 44 +++++++++++++++++++++++ write-or-die.c | 2 +- 13 files changed, 185 insertions(+), 11 deletions(-) diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt index c04f62a54a154c..200b4d9f06ea65 100644 --- a/Documentation/config/core.txt +++ b/Documentation/config/core.txt @@ -548,12 +548,29 @@ core.whitespace:: errors. The default tab width is 8. Allowed values are 1 to 63. core.fsyncObjectFiles:: - This boolean will enable 'fsync()' when writing object files. -+ -This is a total waste of time and effort on a filesystem that orders -data writes properly, but can be useful for filesystems that do not use -journalling (traditional UNIX filesystems) or that only journal metadata -and not file contents (OS X's HFS+, or Linux ext3 with "data=writeback"). + A value indicating the level of effort Git will expend in + trying to make objects added to the repo durable in the event + of an unclean system shutdown. This setting currently only + controls loose objects in the object store, so updates to any + refs or the index may not be equally durable. ++ +* `false` allows data to remain in file system caches according to + operating system policy, whence it may be lost if the system loses power + or crashes. +* `true` triggers a data integrity flush for each loose object added to the + object store. This is the safest setting that is likely to ensure durability + across all operating systems and file systems that honor the 'fsync' system + call. However, this setting comes with a significant performance cost on + common hardware. Git does not currently fsync parent directories for + newly-added files, so some filesystems may still allow data to be lost on + system crash. +* `batch` enables an experimental mode that uses interfaces available in some + operating systems to write loose object data with a minimal set of FLUSH + CACHE (or equivalent) commands sent to the storage controller. If the + operating system interfaces are not available, this mode behaves the same as + `true`. This mode is expected to be as safe as `true` on macOS for repos + stored on HFS+ or APFS filesystems and on Windows for repos stored on NTFS or + ReFS. core.preloadIndex:: Enable parallel index preload for operations like 'git diff' diff --git a/Makefile b/Makefile index e04c942f58f981..11ca53b7403956 100644 --- a/Makefile +++ b/Makefile @@ -406,6 +406,8 @@ all:: # # Define HAVE_CLOCK_MONOTONIC if your platform has CLOCK_MONOTONIC. # +# Define HAVE_SYNC_FILE_RANGE if your platform has sync_file_range. +# # Define NEEDS_LIBRT if your platform requires linking with librt (glibc version # before 2.17) for clock_gettime and CLOCK_MONOTONIC. # @@ -1876,6 +1878,10 @@ ifdef HAVE_CLOCK_MONOTONIC BASIC_CFLAGS += -DHAVE_CLOCK_MONOTONIC endif +ifdef HAVE_SYNC_FILE_RANGE + BASIC_CFLAGS += -DHAVE_SYNC_FILE_RANGE +endif + ifdef NEEDS_LIBRT EXTLIBS += -lrt endif diff --git a/bulk-checkin.c b/bulk-checkin.c index 6ae18401e04ecd..4deee1af46e946 100644 --- a/bulk-checkin.c +++ b/bulk-checkin.c @@ -3,14 +3,20 @@ */ #include "cache.h" #include "bulk-checkin.h" +#include "lockfile.h" #include "repository.h" #include "csum-file.h" #include "pack.h" #include "strbuf.h" +#include "string-list.h" +#include "tmp-objdir.h" #include "packfile.h" #include "object-store.h" static int bulk_checkin_plugged; +static int needs_batch_fsync; + +static struct tmp_objdir *bulk_fsync_objdir; static struct bulk_checkin_state { char *pack_tmp_name; @@ -79,6 +85,34 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state) reprepare_packed_git(the_repository); } +/* + * Cleanup after batch-mode fsync_object_files. + */ +static void do_batch_fsync(void) +{ + /* + * Issue a full hardware flush against a temporary file to ensure + * that all objects are durable before any renames occur. The code in + * fsync_loose_object_bulk_checkin has already issued a writeout + * request, but it has not flushed any writeback cache in the storage + * hardware. + */ + + if (needs_batch_fsync) { + struct strbuf temp_path = STRBUF_INIT; + struct tempfile *temp; + + strbuf_addf(&temp_path, "%s/bulk_fsync_XXXXXX", get_object_directory()); + temp = xmks_tempfile(temp_path.buf); + fsync_or_die(get_tempfile_fd(temp), get_tempfile_path(temp)); + delete_tempfile(&temp); + strbuf_release(&temp_path); + } + + if (bulk_fsync_objdir) + tmp_objdir_migrate(bulk_fsync_objdir); +} + static int already_written(struct bulk_checkin_state *state, struct object_id *oid) { int i; @@ -273,6 +307,25 @@ static int deflate_to_pack(struct bulk_checkin_state *state, return 0; } +void fsync_loose_object_bulk_checkin(int fd) +{ + assert(fsync_object_files == FSYNC_OBJECT_FILES_BATCH); + + /* + * If we have a plugged bulk checkin, we issue a call that + * cleans the filesystem page cache but avoids a hardware flush + * command. Later on we will issue a single hardware flush + * before as part of do_batch_fsync. + */ + if (bulk_checkin_plugged && + git_fsync(fd, FSYNC_WRITEOUT_ONLY) >= 0) { + if (!needs_batch_fsync) + needs_batch_fsync = 1; + } else { + fsync_or_die(fd, "loose object file"); + } +} + int index_bulk_checkin(struct object_id *oid, int fd, size_t size, enum object_type type, const char *path, unsigned flags) @@ -287,6 +340,19 @@ int index_bulk_checkin(struct object_id *oid, void plug_bulk_checkin(void) { assert(!bulk_checkin_plugged); + + /* + * A temporary object directory is used to hold the files + * while they are not fsynced. + */ + if (fsync_object_files == FSYNC_OBJECT_FILES_BATCH) { + bulk_fsync_objdir = tmp_objdir_create("bulk-fsync"); + if (!bulk_fsync_objdir) + die(_("Could not create temporary object directory for core.fsyncobjectfiles=batch")); + + tmp_objdir_replace_primary_odb(bulk_fsync_objdir, 0); + } + bulk_checkin_plugged = 1; } @@ -296,4 +362,6 @@ void unplug_bulk_checkin(void) bulk_checkin_plugged = 0; if (bulk_checkin_state.f) finish_bulk_checkin(&bulk_checkin_state); + + do_batch_fsync(); } diff --git a/bulk-checkin.h b/bulk-checkin.h index b26f3dc3b74cb9..08f292379b6cf9 100644 --- a/bulk-checkin.h +++ b/bulk-checkin.h @@ -6,6 +6,8 @@ #include "cache.h" +void fsync_loose_object_bulk_checkin(int fd); + int index_bulk_checkin(struct object_id *oid, int fd, size_t size, enum object_type type, const char *path, unsigned flags); diff --git a/cache.h b/cache.h index 3e5658c6ddaca4..bcf93ab0f93341 100644 --- a/cache.h +++ b/cache.h @@ -984,7 +984,13 @@ void reset_shared_repository(void); extern int read_replace_refs; extern char *git_replace_ref_base; -extern int fsync_object_files; +enum fsync_object_files_mode { + FSYNC_OBJECT_FILES_OFF, + FSYNC_OBJECT_FILES_ON, + FSYNC_OBJECT_FILES_BATCH +}; + +extern enum fsync_object_files_mode fsync_object_files; extern int core_preload_index; extern int precomposed_unicode; extern int protect_hfs; diff --git a/config.c b/config.c index 2edf835262fa24..8315d020eebbb6 100644 --- a/config.c +++ b/config.c @@ -1506,7 +1506,12 @@ static int git_default_core_config(const char *var, const char *value, void *cb) } if (!strcmp(var, "core.fsyncobjectfiles")) { - fsync_object_files = git_config_bool(var, value); + if (value && !strcmp(value, "batch")) + fsync_object_files = FSYNC_OBJECT_FILES_BATCH; + else if (git_config_bool(var, value)) + fsync_object_files = FSYNC_OBJECT_FILES_ON; + else + fsync_object_files = FSYNC_OBJECT_FILES_OFF; return 0; } diff --git a/config.mak.uname b/config.mak.uname index 76516aaa9a5d56..e6d482fbcc6286 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -53,6 +53,7 @@ ifeq ($(uname_S),Linux) HAVE_CLOCK_MONOTONIC = YesPlease # -lrt is needed for clock_gettime on glibc <= 2.16 NEEDS_LIBRT = YesPlease + HAVE_SYNC_FILE_RANGE = YesPlease HAVE_GETDELIM = YesPlease SANE_TEXT_GREP=-a FREAD_READS_DIRECTORIES = UnfortunatelyYes diff --git a/configure.ac b/configure.ac index 031e8d3fee8f22..c711037d625da7 100644 --- a/configure.ac +++ b/configure.ac @@ -1090,6 +1090,14 @@ AC_COMPILE_IFELSE([CLOCK_MONOTONIC_SRC], [AC_MSG_RESULT([no]) HAVE_CLOCK_MONOTONIC=]) GIT_CONF_SUBST([HAVE_CLOCK_MONOTONIC]) + +# +# Define HAVE_SYNC_FILE_RANGE=YesPlease if sync_file_range is available. +GIT_CHECK_FUNC(sync_file_range, + [HAVE_SYNC_FILE_RANGE=YesPlease], + [HAVE_SYNC_FILE_RANGE]) +GIT_CONF_SUBST([HAVE_SYNC_FILE_RANGE]) + # # Define NO_SETITIMER if you don't have setitimer. GIT_CHECK_FUNC(setitimer, diff --git a/environment.c b/environment.c index 8a8cfbfcd39a10..837c78c9930703 100644 --- a/environment.c +++ b/environment.c @@ -42,7 +42,7 @@ const char *git_attributes_file; const char *git_hooks_path; int zlib_compression_level = Z_BEST_SPEED; int pack_compression_level = Z_DEFAULT_COMPRESSION; -int fsync_object_files; +enum fsync_object_files_mode fsync_object_files; size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE; size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT; size_t delta_base_cache_limit = 96 * 1024 * 1024; diff --git a/git-compat-util.h b/git-compat-util.h index 141bb86351e63c..5eebe92811ddde 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -1215,6 +1215,13 @@ __attribute__((format (printf, 1, 2))) NORETURN void BUG(const char *fmt, ...); #endif +enum fsync_action { + FSYNC_WRITEOUT_ONLY, + FSYNC_HARDWARE_FLUSH +}; + +int git_fsync(int fd, enum fsync_action action); + /* * Preserves errno, prints a message, but gives no warning for ENOENT. * Returns 0 on success, which includes trying to unlink an object that does diff --git a/object-file.c b/object-file.c index 0eb1bd8931fc1f..bef526a30b31d1 100644 --- a/object-file.c +++ b/object-file.c @@ -1864,8 +1864,18 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf, static void close_loose_object(int fd) { if (!the_repository->objects->odb->will_destroy) { - if (fsync_object_files) + switch (fsync_object_files) { + case FSYNC_OBJECT_FILES_OFF: + break; + case FSYNC_OBJECT_FILES_ON: fsync_or_die(fd, "loose object file"); + break; + case FSYNC_OBJECT_FILES_BATCH: + fsync_loose_object_bulk_checkin(fd); + break; + default: + BUG("Invalid fsync_object_files mode."); + } } if (close(fd) != 0) diff --git a/wrapper.c b/wrapper.c index 1460d4e27b03cc..e5dd6b68c1931c 100644 --- a/wrapper.c +++ b/wrapper.c @@ -552,6 +552,50 @@ int xmkstemp_mode(char *filename_template, int mode) return fd; } +int git_fsync(int fd, enum fsync_action action) +{ + switch (action) { + case FSYNC_WRITEOUT_ONLY: + +#ifdef __APPLE__ + /* + * on macOS, fsync just causes filesystem cache writeback but does not + * flush hardware caches. + */ + return fsync(fd); +#endif + +#ifdef HAVE_SYNC_FILE_RANGE + /* + * On linux 2.6.17 and above, sync_file_range is the way to issue + * a writeback without a hardware flush. An offset of 0 and size of 0 + * indicates writeout of the entire file and the wait flags ensure that all + * dirty data is written to the disk (potentially in a disk-side cache) + * before we continue. + */ + + return sync_file_range(fd, 0, 0, SYNC_FILE_RANGE_WAIT_BEFORE | + SYNC_FILE_RANGE_WRITE | + SYNC_FILE_RANGE_WAIT_AFTER); +#endif + + errno = ENOSYS; + return -1; + + case FSYNC_HARDWARE_FLUSH: + +#ifdef __APPLE__ + return fcntl(fd, F_FULLFSYNC); +#else + return fsync(fd); +#endif + + default: + BUG("unexpected git_fsync(%d) call", action); + } + +} + static int warn_if_unremovable(const char *op, const char *file, int rc) { int err; diff --git a/write-or-die.c b/write-or-die.c index 0b1ec8190b622d..cc8291d9794040 100644 --- a/write-or-die.c +++ b/write-or-die.c @@ -57,7 +57,7 @@ void fprintf_or_die(FILE *f, const char *fmt, ...) void fsync_or_die(int fd, const char *msg) { - while (fsync(fd) < 0) { + while (git_fsync(fd, FSYNC_HARDWARE_FLUSH) < 0) { if (errno != EINTR) die_errno("fsync error on '%s'", msg); } From d11da094aa1e0de458ec6f7e9e008cfff7c454db Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Mon, 4 Oct 2021 16:57:43 +0000 Subject: [PATCH 15/23] core.fsyncobjectfiles: add windows support for batch mode This commit adds a win32 implementation for fsync_no_flush that is called git_fsync. The 'NtFlushBuffersFileEx' function being called is available since Windows 8. If the function is not available, we return -1 and Git falls back to doing a full fsync. The operating system is told to flush data only without a hardware flush primitive. A later full fsync will cause the metadata log to be flushed and then the disk cache to be flushed on NTFS and ReFS. Other filesystems will treat this as a full flush operation. I added a new file here for this system call so as not to conflict with downstream changes in the git-for-windows repository related to fscache. Signed-off-by: Neeraj Singh Signed-off-by: Junio C Hamano --- compat/mingw.h | 3 +++ compat/win32/flush.c | 28 ++++++++++++++++++++++++++++ config.mak.uname | 2 ++ contrib/buildsystems/CMakeLists.txt | 3 ++- wrapper.c | 4 ++++ 5 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 compat/win32/flush.c diff --git a/compat/mingw.h b/compat/mingw.h index c9a52ad64a681f..6074a3d3cedb56 100644 --- a/compat/mingw.h +++ b/compat/mingw.h @@ -329,6 +329,9 @@ int mingw_getpagesize(void); #define getpagesize mingw_getpagesize #endif +int win32_fsync_no_flush(int fd); +#define fsync_no_flush win32_fsync_no_flush + struct rlimit { unsigned int rlim_cur; }; diff --git a/compat/win32/flush.c b/compat/win32/flush.c new file mode 100644 index 00000000000000..75324c24ee7cef --- /dev/null +++ b/compat/win32/flush.c @@ -0,0 +1,28 @@ +#include "../../git-compat-util.h" +#include +#include "lazyload.h" + +int win32_fsync_no_flush(int fd) +{ + IO_STATUS_BLOCK io_status; + +#define FLUSH_FLAGS_FILE_DATA_ONLY 1 + + DECLARE_PROC_ADDR(ntdll.dll, NTSTATUS, NtFlushBuffersFileEx, + HANDLE FileHandle, ULONG Flags, PVOID Parameters, ULONG ParameterSize, + PIO_STATUS_BLOCK IoStatusBlock); + + if (!INIT_PROC_ADDR(NtFlushBuffersFileEx)) { + errno = ENOSYS; + return -1; + } + + memset(&io_status, 0, sizeof(io_status)); + if (NtFlushBuffersFileEx((HANDLE)_get_osfhandle(fd), FLUSH_FLAGS_FILE_DATA_ONLY, + NULL, 0, &io_status)) { + errno = EINVAL; + return -1; + } + + return 0; +} diff --git a/config.mak.uname b/config.mak.uname index e6d482fbcc6286..34c93314a505da 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -451,6 +451,7 @@ endif CFLAGS = BASIC_CFLAGS = -nologo -I. -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE COMPAT_OBJS = compat/msvc.o compat/winansi.o \ + compat/win32/flush.o \ compat/win32/path-utils.o \ compat/win32/pthread.o compat/win32/syslog.o \ compat/win32/trace2_win32_process_info.o \ @@ -626,6 +627,7 @@ ifneq (,$(findstring MINGW,$(uname_S))) COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\" COMPAT_OBJS += compat/mingw.o compat/winansi.o \ compat/win32/trace2_win32_process_info.o \ + compat/win32/flush.o \ compat/win32/path-utils.o \ compat/win32/pthread.o compat/win32/syslog.o \ compat/win32/dirent.o diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt index 171b4124afef58..b573a5ee122e33 100644 --- a/contrib/buildsystems/CMakeLists.txt +++ b/contrib/buildsystems/CMakeLists.txt @@ -261,7 +261,8 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Windows") NOGDI OBJECT_CREATION_MODE=1 __USE_MINGW_ANSI_STDIO=0 USE_NED_ALLOCATOR OVERRIDE_STRDUP MMAP_PREVENTS_DELETE USE_WIN32_MMAP UNICODE _UNICODE HAVE_WPGMPTR ENSURE_MSYSTEM_IS_SET) - list(APPEND compat_SOURCES compat/mingw.c compat/winansi.c compat/win32/path-utils.c + list(APPEND compat_SOURCES compat/mingw.c compat/winansi.c + compat/win32/flush.c compat/win32/path-utils.c compat/win32/pthread.c compat/win32mmap.c compat/win32/syslog.c compat/win32/trace2_win32_process_info.c compat/win32/dirent.c compat/nedmalloc/nedmalloc.c compat/strdup.c) diff --git a/wrapper.c b/wrapper.c index e5dd6b68c1931c..ea07a5c373df4b 100644 --- a/wrapper.c +++ b/wrapper.c @@ -579,6 +579,10 @@ int git_fsync(int fd, enum fsync_action action) SYNC_FILE_RANGE_WAIT_AFTER); #endif +#ifdef fsync_no_flush + return fsync_no_flush(fd); +#endif + errno = ENOSYS; return -1; From 085d91472d33ba5e9849873d07d29ea91c8ff0ee Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Mon, 4 Oct 2021 16:57:44 +0000 Subject: [PATCH 16/23] update-index: use the bulk-checkin infrastructure The update-index functionality is used internally by 'git stash push' to setup the internal stashed commit. This change enables bulk-checkin for update-index infrastructure to speed up adding new objects to the object database by leveraging the pack functionality and the new bulk-fsync functionality. There is some risk with this change, since under batch fsync, the object files will not be available until the update-index is entirely complete. This usage is unlikely, since any tool invoking update-index and expecting to see objects would have to synchronize with the update-index process after passing it a file path. Signed-off-by: Neeraj Singh Signed-off-by: Junio C Hamano --- builtin/update-index.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/builtin/update-index.c b/builtin/update-index.c index 187203e8bb53cb..dc7368bb1ee2e5 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -5,6 +5,7 @@ */ #define USE_THE_INDEX_COMPATIBILITY_MACROS #include "cache.h" +#include "bulk-checkin.h" #include "config.h" #include "lockfile.h" #include "quote.h" @@ -1088,6 +1089,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) the_index.updated_skipworktree = 1; + /* we might be adding many objects to the object database */ + plug_bulk_checkin(); + /* * Custom copy of parse_options() because we want to handle * filename arguments as they come. @@ -1168,6 +1172,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) strbuf_release(&buf); } + /* by now we must have added all of the new objects */ + unplug_bulk_checkin(); if (split_index > 0) { if (git_config_get_split_index() == 0) warning(_("core.splitIndex is set to false; " From 2881e3edb7a2546d227ff8f353e0608a72d90a32 Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Mon, 4 Oct 2021 16:57:45 +0000 Subject: [PATCH 17/23] unpack-objects: use the bulk-checkin infrastructure The unpack-objects functionality is used by fetch, push, and fast-import to turn the transfered data into object database entries when there are fewer objects than the 'unpacklimit' setting. By enabling bulk-checkin when unpacking objects, we can take advantage of batched fsyncs. Signed-off-by: Neeraj Singh Signed-off-by: Junio C Hamano --- builtin/unpack-objects.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index 4a9466295ba10d..51eb4f7b531aab 100644 --- a/builtin/unpack-objects.c +++ b/builtin/unpack-objects.c @@ -1,5 +1,6 @@ #include "builtin.h" #include "cache.h" +#include "bulk-checkin.h" #include "config.h" #include "object-store.h" #include "object.h" @@ -503,10 +504,12 @@ static void unpack_all(void) if (!quiet) progress = start_progress(_("Unpacking objects"), nr_objects); CALLOC_ARRAY(obj_list, nr_objects); + plug_bulk_checkin(); for (i = 0; i < nr_objects; i++) { unpack_one(i); display_progress(progress, i + 1); } + unplug_bulk_checkin(); stop_progress(&progress); if (delta_list) From e058d35e94646d41172df92cf0b300287314accd Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Mon, 4 Oct 2021 16:57:46 +0000 Subject: [PATCH 18/23] core.fsyncobjectfiles: tests for batch mode Add test cases to exercise batch mode for: * 'git add' * 'git stash' * 'git update-index' * 'git unpack-objects' These tests ensure that the added data winds up in the object database. In this change we introduce a new test helper lib-unique-files.sh. The goal of this library is to create a tree of files that have different oids from any other files that may have been created in the current test repo. This helps us avoid missing validation of an object being added due to it already being in the repo. Signed-off-by: Neeraj Singh Signed-off-by: Junio C Hamano --- t/lib-unique-files.sh | 36 ++++++++++++++++++++++++++++++++++++ t/t3700-add.sh | 20 ++++++++++++++++++++ t/t3903-stash.sh | 14 ++++++++++++++ t/t5300-pack-object.sh | 30 +++++++++++++++++++----------- 4 files changed, 89 insertions(+), 11 deletions(-) create mode 100644 t/lib-unique-files.sh diff --git a/t/lib-unique-files.sh b/t/lib-unique-files.sh new file mode 100644 index 00000000000000..a7de4ca8512ac5 --- /dev/null +++ b/t/lib-unique-files.sh @@ -0,0 +1,36 @@ +# Helper to create files with unique contents + + +# Create multiple files with unique contents. Takes the number of +# directories, the number of files in each directory, and the base +# directory. +# +# test_create_unique_files 2 3 my_dir -- Creates 2 directories with 3 files +# each in my_dir, all with unique +# contents. + +test_create_unique_files() { + test "$#" -ne 3 && BUG "3 param" + + local dirs=$1 + local files=$2 + local basedir=$3 + local counter=0 + test_tick + local basedata=$test_tick + + + rm -rf $basedir + + for i in $(test_seq $dirs) + do + local dir=$basedir/dir$i + + mkdir -p "$dir" + for j in $(test_seq $files) + do + counter=$((counter + 1)) + echo "$basedata.$counter" >"$dir/file$j.txt" + done + done +} diff --git a/t/t3700-add.sh b/t/t3700-add.sh index 4086e1ebbc97f1..36049a53ff7f0a 100755 --- a/t/t3700-add.sh +++ b/t/t3700-add.sh @@ -7,6 +7,8 @@ test_description='Test of git add, including the -- option.' . ./test-lib.sh +. $TEST_DIRECTORY/lib-unique-files.sh + # Test the file mode "$1" of the file "$2" in the index. test_mode_in_index () { case "$(git ls-files -s "$2")" in @@ -33,6 +35,24 @@ test_expect_success \ 'Test that "git add -- -q" works' \ 'touch -- -q && git add -- -q' +test_expect_success 'git add: core.fsyncobjectfiles=batch' " + test_create_unique_files 2 4 fsync-files && + git -c core.fsyncobjectfiles=batch add -- ./fsync-files/ && + rm -f fsynced_files && + git ls-files --stage fsync-files/ > fsynced_files && + test_line_count = 8 fsynced_files && + awk -- '{print \$2}' fsynced_files | xargs -n1 git cat-file -e +" + +test_expect_success 'git update-index: core.fsyncobjectfiles=batch' " + test_create_unique_files 2 4 fsync-files2 && + find fsync-files2 ! -type d -print | xargs git -c core.fsyncobjectfiles=batch update-index --add -- && + rm -f fsynced_files2 && + git ls-files --stage fsync-files2/ > fsynced_files2 && + test_line_count = 8 fsynced_files2 && + awk -- '{print \$2}' fsynced_files2 | xargs -n1 git cat-file -e +" + test_expect_success \ 'git add: Test that executable bit is not used if core.filemode=0' \ 'git config core.filemode 0 && diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index f0a82be9de7654..6324b52c874bef 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -9,6 +9,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh +. $TEST_DIRECTORY/lib-unique-files.sh diff_cmp () { for i in "$1" "$2" @@ -1293,6 +1294,19 @@ test_expect_success 'stash handles skip-worktree entries nicely' ' git rev-parse --verify refs/stash:A.t ' +test_expect_success 'stash with core.fsyncobjectfiles=batch' " + test_create_unique_files 2 4 fsync-files && + git -c core.fsyncobjectfiles=batch stash push -u -- ./fsync-files/ && + rm -f fsynced_files && + + # The files were untracked, so use the third parent, + # which contains the untracked files + git ls-tree -r stash^3 -- ./fsync-files/ > fsynced_files && + test_line_count = 8 fsynced_files && + awk -- '{print \$3}' fsynced_files | xargs -n1 git cat-file -e +" + + test_expect_success 'stash -c stash.useBuiltin=false warning ' ' expected="stash.useBuiltin support has been removed" && diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh index e13a884207589d..38663dc1393654 100755 --- a/t/t5300-pack-object.sh +++ b/t/t5300-pack-object.sh @@ -162,23 +162,23 @@ test_expect_success 'pack-objects with bogus arguments' ' check_unpack () { test_when_finished "rm -rf git2" && - git init --bare git2 && - git -C git2 unpack-objects -n <"$1".pack && - git -C git2 unpack-objects <"$1".pack && - (cd .git && find objects -type f -print) | - while read path - do - cmp git2/$path .git/$path || { - echo $path differs. - return 1 - } - done + git $2 init --bare git2 && + ( + git $2 -C git2 unpack-objects -n <"$1".pack && + git $2 -C git2 unpack-objects <"$1".pack && + git $2 -C git2 cat-file --batch-check="%(objectname)" + ) current && + cmp obj-list current } test_expect_success 'unpack without delta' ' check_unpack test-1-${packname_1} ' +test_expect_success 'unpack without delta (core.fsyncobjectfiles=batch)' ' + check_unpack test-1-${packname_1} "-c core.fsyncobjectfiles=batch" +' + test_expect_success 'pack with REF_DELTA' ' packname_2=$(git pack-objects --progress test-2 stderr) && check_deltas stderr -gt 0 @@ -188,6 +188,10 @@ test_expect_success 'unpack with REF_DELTA' ' check_unpack test-2-${packname_2} ' +test_expect_success 'unpack with REF_DELTA (core.fsyncobjectfiles=batch)' ' + check_unpack test-2-${packname_2} "-c core.fsyncobjectfiles=batch" +' + test_expect_success 'pack with OFS_DELTA' ' packname_3=$(git pack-objects --progress --delta-base-offset test-3 \ stderr) && @@ -198,6 +202,10 @@ test_expect_success 'unpack with OFS_DELTA' ' check_unpack test-3-${packname_3} ' +test_expect_success 'unpack with OFS_DELTA (core.fsyncobjectfiles=batch)' ' + check_unpack test-3-${packname_3} "-c core.fsyncobjectfiles=batch" +' + test_expect_success 'compare delta flavors' ' perl -e '\'' defined($_ = -s $_) or die for @ARGV; From e2d74931156f0b97aa8a6b2084fd176c721f9320 Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Mon, 4 Oct 2021 16:57:47 +0000 Subject: [PATCH 19/23] core.fsyncobjectfiles: performance tests for add and stash Add a basic performance test for "git add" and "git stash" of a lot of new objects with various fsync settings. Signed-off-by: Neeraj Singh Signed-off-by: Junio C Hamano --- t/perf/p3700-add.sh | 43 ++++++++++++++++++++++++++++++++++++++++ t/perf/p3900-stash.sh | 46 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+) create mode 100755 t/perf/p3700-add.sh create mode 100755 t/perf/p3900-stash.sh diff --git a/t/perf/p3700-add.sh b/t/perf/p3700-add.sh new file mode 100755 index 00000000000000..e93c08a2e70a8f --- /dev/null +++ b/t/perf/p3700-add.sh @@ -0,0 +1,43 @@ +#!/bin/sh +# +# This test measures the performance of adding new files to the object database +# and index. The test was originally added to measure the effect of the +# core.fsyncObjectFiles=batch mode, which is why we are testing different values +# of that setting explicitly and creating a lot of unique objects. + +test_description="Tests performance of add" + +. ./perf-lib.sh + +. $TEST_DIRECTORY/lib-unique-files.sh + +test_perf_default_repo +test_checkout_worktree + +dir_count=10 +files_per_dir=50 +total_files=$((dir_count * files_per_dir)) + +# We need to create the files each time we run the perf test, but +# we do not want to measure the cost of creating the files, so run +# the tet once. +if test "${GIT_PERF_REPEAT_COUNT-1}" -ne 1 +then + echo "warning: Setting GIT_PERF_REPEAT_COUNT=1" >&2 + GIT_PERF_REPEAT_COUNT=1 +fi + +for m in false true batch +do + test_expect_success "create the files for core.fsyncObjectFiles=$m" ' + git reset --hard && + # create files across directories + test_create_unique_files $dir_count $files_per_dir files + ' + + test_perf "add $total_files files (core.fsyncObjectFiles=$m)" " + git -c core.fsyncobjectfiles=$m add files + " +done + +test_done diff --git a/t/perf/p3900-stash.sh b/t/perf/p3900-stash.sh new file mode 100755 index 00000000000000..c9fcd0c03eb9b7 --- /dev/null +++ b/t/perf/p3900-stash.sh @@ -0,0 +1,46 @@ +#!/bin/sh +# +# This test measures the performance of adding new files to the object database +# and index. The test was originally added to measure the effect of the +# core.fsyncObjectFiles=batch mode, which is why we are testing different values +# of that setting explicitly and creating a lot of unique objects. + +test_description="Tests performance of stash" + +. ./perf-lib.sh + +. $TEST_DIRECTORY/lib-unique-files.sh + +test_perf_default_repo +test_checkout_worktree + +dir_count=10 +files_per_dir=50 +total_files=$((dir_count * files_per_dir)) + +# We need to create the files each time we run the perf test, but +# we do not want to measure the cost of creating the files, so run +# the tet once. +if test "${GIT_PERF_REPEAT_COUNT-1}" -ne 1 +then + echo "warning: Setting GIT_PERF_REPEAT_COUNT=1" >&2 + GIT_PERF_REPEAT_COUNT=1 +fi + +for m in false true batch +do + test_expect_success "create the files for core.fsyncObjectFiles=$m" ' + git reset --hard && + # create files across directories + test_create_unique_files $dir_count $files_per_dir files + ' + + # We only stash files in the 'files' subdirectory since + # the perf test infrastructure creates files in the + # current working directory that need to be preserved + test_perf "stash 500 files (core.fsyncObjectFiles=$m)" " + git -c core.fsyncobjectfiles=$m stash push -u -- files + " +done + +test_done From cb443dfd78addbdd552630a09b24b006b8d7da48 Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Tue, 26 Oct 2021 22:35:29 +0000 Subject: [PATCH 20/23] fixup! tmp-objdir: new API for creating temporary writable databases Fix prune code to be able to delete multiple object directories. I wasn't properly resetting the strbuf with the path. Signed-off-by: Neeraj Singh Signed-off-by: Junio C Hamano --- builtin/prune.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/prune.c b/builtin/prune.c index 9c72ecf5a58bbc..6b6b0c7b011836 100644 --- a/builtin/prune.c +++ b/builtin/prune.c @@ -31,6 +31,7 @@ static int prune_tmp_file(const char *fullpath) if (show_only || verbose) printf("Removing stale temporary directory %s\n", fullpath); if (!show_only) { + strbuf_reset(&remove_dir_buf); strbuf_addstr(&remove_dir_buf, fullpath); remove_dir_recursively(&remove_dir_buf, 0); } From a082404cf104d1c685d204aa3fcd26bf5a69673d Mon Sep 17 00:00:00 2001 From: Neeraj Singh Date: Tue, 26 Oct 2021 22:35:30 +0000 Subject: [PATCH 21/23] fixup! tmp-objdir: new API for creating temporary writable databases When setup_work_tree executes, it redoes setup of the object database path and various other aspects of the_repository. This destroys the temporary object database state. This commit removes the temporary object database and reapplies it around the operations in the chdir_notify callback. Signed-off-by: Neeraj Singh Signed-off-by: Junio C Hamano --- environment.c | 5 +++++ tmp-objdir.c | 25 +++++++++++++++++++++++++ tmp-objdir.h | 15 +++++++++++++++ 3 files changed, 45 insertions(+) diff --git a/environment.c b/environment.c index 46ec5072c059b3..7ba5ae06c71669 100644 --- a/environment.c +++ b/environment.c @@ -17,6 +17,7 @@ #include "commit.h" #include "strvec.h" #include "object-store.h" +#include "tmp-objdir.h" #include "chdir-notify.h" #include "shallow.h" @@ -344,10 +345,14 @@ static void update_relative_gitdir(const char *name, void *data) { char *path = reparent_relative_path(old_cwd, new_cwd, get_git_dir()); + struct tmp_objdir *tmp_objdir = tmp_objdir_unapply_primary_odb(); trace_printf_key(&trace_setup_key, "setup: move $GIT_DIR to '%s'", path); + set_git_dir_1(path); + if (tmp_objdir) + tmp_objdir_reapply_primary_odb(tmp_objdir, old_cwd, new_cwd); free(path); } diff --git a/tmp-objdir.c b/tmp-objdir.c index 45d42a7bcf0ba6..3d38eeab66bfb0 100644 --- a/tmp-objdir.c +++ b/tmp-objdir.c @@ -1,5 +1,6 @@ #include "cache.h" #include "tmp-objdir.h" +#include "chdir-notify.h" #include "dir.h" #include "sigchain.h" #include "string-list.h" @@ -12,6 +13,7 @@ struct tmp_objdir { struct strbuf path; struct strvec env; struct object_directory *prev_odb; + int will_destroy; }; /* @@ -315,4 +317,27 @@ void tmp_objdir_replace_primary_odb(struct tmp_objdir *t, int will_destroy) if (t->prev_odb) BUG("the primary object database is already replaced"); t->prev_odb = set_temporary_primary_odb(t->path.buf, will_destroy); + t->will_destroy = will_destroy; +} + +struct tmp_objdir *tmp_objdir_unapply_primary_odb(void) +{ + if (!the_tmp_objdir || !the_tmp_objdir->prev_odb) + return NULL; + + restore_primary_odb(the_tmp_objdir->prev_odb, the_tmp_objdir->path.buf); + the_tmp_objdir->prev_odb = NULL; + return the_tmp_objdir; +} + +void tmp_objdir_reapply_primary_odb(struct tmp_objdir *t, const char *old_cwd, + const char *new_cwd) +{ + char *path; + + path = reparent_relative_path(old_cwd, new_cwd, t->path.buf); + strbuf_reset(&t->path); + strbuf_addstr(&t->path, path); + free(path); + tmp_objdir_replace_primary_odb(t, t->will_destroy); } diff --git a/tmp-objdir.h b/tmp-objdir.h index 75754cbfba618e..a3145051f25bf6 100644 --- a/tmp-objdir.h +++ b/tmp-objdir.h @@ -59,4 +59,19 @@ void tmp_objdir_add_as_alternate(const struct tmp_objdir *); */ void tmp_objdir_replace_primary_odb(struct tmp_objdir *, int will_destroy); +/* + * If the primary object database was replaced by a temporary object directory, + * restore it to its original value while keeping the directory contents around. + * Returns NULL if the primary object database was not replaced. + */ +struct tmp_objdir *tmp_objdir_unapply_primary_odb(void); + +/* + * Reapplies the former primary temporary object database, after protentially + * changing its relative path. + */ +void tmp_objdir_reapply_primary_odb(struct tmp_objdir *, const char *old_cwd, + const char *new_cwd); + + #endif /* TMP_OBJDIR_H */ From 2d687baeed82e7b90d383bad8e209f50e0ce8c87 Mon Sep 17 00:00:00 2001 From: John Cai Date: Fri, 5 Nov 2021 16:18:17 -0400 Subject: [PATCH 22/23] cat-file: force flush of stdout on empty string When in --buffer mode, it is very useful for the caller to have control over when the buffer is flushed. Currently there is no convenient way to signal for the buffer to be flushed. One workaround is to provide any nonexisting commit to git-cat-file's stdin, in which case the buffer will be flushed and a "$FOO missing" message will be displayed. However, this is not an ideal workaround. Instead, this commit teaches git-cat-file to look for an empty string in stdin, which will trigger a flush of stdout. Signed-off-by: John Cai --- builtin/cat-file.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 86fc03242b87c3..4d17f30f24e5d3 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -405,6 +405,11 @@ static void batch_one_object(const char *obj_name, int flags = opt->follow_symlinks ? GET_OID_FOLLOW_SYMLINKS : 0; enum get_oid_result result; + if (opt->buffer_output && obj_name[0] == '\0') { + fflush(stdout); + return; + } + result = get_oid_with_context(the_repository, obj_name, flags, &data->oid, &ctx); if (result != FOUND) { @@ -609,7 +614,11 @@ static int batch_objects(struct batch_options *opt) data.rest = p; } - batch_one_object(input.buf, &output, opt, &data); + /* + * When in buffer mode and input.buf is an empty string, + * flush to stdout. + */ + batch_one_object(input.buf, &output, opt, &data); } strbuf_release(&input); From 18d6c2819ade2e514a408e4173c6a5048e3f2cd1 Mon Sep 17 00:00:00 2001 From: John Cai Date: Fri, 5 Nov 2021 16:19:12 -0400 Subject: [PATCH 23/23] docs: update behavior of git-cat-file --buffer When an empty string is entered into stdin, git-cat-file --buffer will flush stdout immediately. This commit updates the man page accordingly. Signed-off-by: John Cai --- Documentation/git-cat-file.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt index 27b27e2b300c49..c98e8dc366965d 100644 --- a/Documentation/git-cat-file.txt +++ b/Documentation/git-cat-file.txt @@ -104,7 +104,8 @@ OPTIONS that a process can interactively read and write from `cat-file`. With this option, the output uses normal stdio buffering; this is much more efficient when invoking - `--batch-check` on a large number of objects. + `--batch-check` on a large number of objects. An empty string will + force a flush of stdout. --unordered:: When `--batch-all-objects` is in use, visit objects in an