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

From: Fabian Stelzer <fs@gigacodes.de>
Date: Jul 29, 2021, 21:25 UTC
Message-ID: <309c7c9a-f38f-ca3b-ad30-4e22a27a692a@gigacodes.de>
In-Reply-To: <YQL8zAHe8CkW1U6j@google.com>

On 29.07.21 21:09, Josh Steadmon wrote:

> Thanks for this series, it sounds like a great idea. I have a few
> comments, inline below.
>

Thanks for your review and help with this patch.

Show 15 quoted lines

> On 2021.07.28 19:36, Fabian Stelzer via GitGitGadget wrote:
> [snip]
>> +		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.
>

fixed. thanks

Show 33 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?

In the test setup i first check if ssh-keygen at all is present (exit code 127 means command not found). Afterwards i check for a specific error message from the command if it is present. Not sure how portable this is, but i can do that because i give known invalid parameters to it. I can't do this here without doing an additional call to ssh-keygen just to check this.

Show 32 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
>>