Re: [PATCH v6 2/9] ssh signing: add ssh signature format and signing using ssh keys
- From
Josh Steadmon <steadmon@google.com>
- Date
- Jul 29, 2021, 19:09 UTC
- Message-ID
- <YQL8zAHe8CkW1U6j@google.com>
- In-Reply-To
- <f05bab16096c080891ee8f7e179eecce7f32e839.1627501009.git.gitgitgadget@gmail.com>
Thanks for this series, it sounds like a great idea. I have a few comments, inline below.
On 2021.07.28 19:36, Fabian Stelzer via GitGitGadget wrote: [snip]
Show 29 quoted lines
> +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 = key_file->filename.buf;You probably want to call strbuf_detach() here, because...
> + } else {
> + /* We assume a file */
> + ssh_signing_key_file = expand_user_path(signing_key, 1);
> + }... you need to free the memory returned by expand_user_path(). If you detach the strbuf above, you can unconditionally free(ssh_signing_key_file) at the end of this function.
Show 28 quoted lines
> +
> + 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+)"));I share Jonathan Tan's concern about checking for "usage:" in the stderr output here. I think in patch 6 the tests rely on a specific return code to check that "-Y sign" is working as expected; can that be used here instead?
Show 31 quoted lines
> +
> + 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);
> + return ret;
> +}
> --
> gitgitgadget
>