Re: [PATCH v6 2/9] ssh signing: add ssh signature format and signing using ssh keys

[Josh Steadmon <steadmon@google.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]
> +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.

> +
> +	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?

> +
> +		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
>