Re: [PATCH v3 4/9] ssh signing: sign using either gpg or ssh keys

[Fabian Stelzer <fs@gigacodes.de>]

On 14.07.21 22:32, Junio C Hamano wrote:
> "Fabian Stelzer via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>>   int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
>>   {
>> -	struct child_process gpg = CHILD_PROCESS_INIT;
>> +	struct child_process signer = CHILD_PROCESS_INIT;
>>   	int ret;
>>   	size_t i, j, bottom;
>> -	struct strbuf gpg_status = STRBUF_INIT;
>> +	struct strbuf signer_stderr = STRBUF_INIT;
>> +	struct tempfile *temp = NULL, *buffer_file = NULL;
>> +	char *ssh_signing_key_file = NULL;
>> +	struct strbuf ssh_signature_filename = STRBUF_INIT;
>>   
>> -	strvec_pushl(&gpg.args,
>> -		     use_format->program,
>> +	if (!strcmp(use_format->name, "ssh")) {
> I wonder if we can split the body of these if/else clauses into
> separate helper functions, point them with fmt structure and
> dispatch via use_format->sign_buffer pointer, just like I suggested
> how to do the same on the signature validation side.
yes, i like the idea and will do that.
>
>> +		if (!signing_key || signing_key[0] == '\0')
>> +			return error(_("user.signingkey needs to be set for ssh signing"));
>> +
>> +
>> +		if (istarts_with(signing_key, "ssh-")) {
>> +			/* A literal ssh key */
>> +			temp = mks_tempfile_t(".git_signing_key_tmpXXXXXX");
>> +			if (!temp)
>> +				return error_errno(_("could not create temporary file"));
>> +			if (write_in_full(temp->fd, signing_key, strlen(signing_key)) < 0 ||
>> +				close_tempfile_gently(temp) < 0) {
>> +				error_errno(_("failed writing ssh signing key to '%s'"),
>> +					temp->filename.buf);
>> +				delete_tempfile(&temp);
>> +				return -1;
>> +			}
>> +			ssh_signing_key_file= temp->filename.buf;
>> +		} 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)
>> +			return error_errno(_("could not create temporary file"));
>> +		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);
>> +			delete_tempfile(&buffer_file);
>> +			return -1;
>> +		}
>> +
>> +		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);
>> +
>> +		strbuf_addbuf(&ssh_signature_filename, &buffer_file->filename);
>> +		strbuf_addstr(&ssh_signature_filename, ".sig");
>> +		if (strbuf_read_file(signature, ssh_signature_filename.buf, 2048) < 0) {
>> +			error_errno(_("failed reading ssh signing data buffer from '%s'"),
>> +				ssh_signature_filename.buf);
>> +		}
>> +		unlink_or_warn(ssh_signature_filename.buf);
>> +		strbuf_release(&ssh_signature_filename);
>> +		delete_tempfile(&buffer_file);
>> +	} else {
>> +		strvec_pushl(&signer.args, use_format->program ,
>>   		     "--status-fd=2",
>>   		     "-bsau", signing_key,
>>   		     NULL);
>>   
>> -	bottom = signature->len;
>> -
>>   	/*
>>   	 * When the username signingkey is bad, program could be terminated
>>   	 * because gpg exits without reading and then write gets SIGPIPE.
>>   	 */
>>   	sigchain_push(SIGPIPE, SIG_IGN);
>> -	ret = pipe_command(&gpg, buffer->buf, buffer->len,
>> -			   signature, 1024, &gpg_status, 0);
>> +		ret = pipe_command(&signer, buffer->buf, buffer->len, signature, 1024, &signer_stderr, 0);
>>   	sigchain_pop(SIGPIPE);
>> +	}
>> +
>> +	bottom = signature->len;
>> +
>> +	if (temp)
>> +		delete_tempfile(&temp);
>>   
>> -	ret |= !strstr(gpg_status.buf, "\n[GNUPG:] SIG_CREATED ");
>> -	strbuf_release(&gpg_status);
>> +	if (!strcmp(use_format->name, "ssh")) {
>> +		if (strstr(signer_stderr.buf, "usage:")) {
>> +			error(_("openssh version > 8.2p1 is needed for ssh signing (ssh-keygen needs -Y sign option)"));
> This looks iffy.  You do call error() to show the error message, but
> you do not set "ret", which affects how the return value from the
> function is computed at the end of the function.
I can of course fix the ret logic, but i'm not happy with this check in 
general either :/
The problem is that ssh-keygen seems to give different error messages 
especially in between the versions when the command was added (8.1 -> 
8.2) and mac os x has one of those by default. The check in the 
t/lib-gpg.sh is much safer, but requires an additional call to 
ssh-keygen which i wanted to avoid here.
>
>> +		}
>> +	} else {
>> +		ret |= !strstr(signer_stderr.buf, "\n[GNUPG:] SIG_CREATED ");
>> +	}
>> +	strbuf_release(&signer_stderr);
>>   	if (ret)
>>   		return error(_("gpg failed to sign the data"));
> And this error message belongs to the GPG half of the logic, not
> ssh (you are allowed to have a separate "ssh failed to sign"
> message, of course, but the point is that the error message emission
> should happen in the codepath dispatched for each crypto backend.
>
> And of course, again the "if (ssh) {do this shiny new ssh thing}
> else {do gpg thing}" structure is questionable.  We should be
> dispatching with use_format->fn (whatever the method name is), no?
  I will rewrite this with the fmt->fn logic.
>
> THanks.
>
-- 
GIGACODES GmbH | Dr. Hermann-Neubauer-Ring 32 | D-63500 Seligenstadt
www.gigacodes.de | fs@gigacodes.de
Phone +49 6182 8955-114 | Fax +49 6182 8955-299 |
HRB 40711 AG Offenbach a. Main
Geschäftsführer: Fabian Stelzer | Umsatzsteuer-ID DE219379936