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

> "Fabian Stelzer via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> @@ -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
>> +	},
>>   };
> A payload a malicious person may feed this version of Git can have a
> pattern that happens to match the ssh_sigs[] string, and the code
> will blindly try to call .verify_signed_buffer==NULL and die, no?
>
> That is not the end of the world; as long as we know that with the
> above "TODO" comment it is probably OK.

>
>> @@ -463,12 +482,26 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
>>   	return use_format->sign_buffer(buffer, signature, signing_key);
>>   }
>>   
>> +static void strbuf_trim_trailing_cr(struct strbuf *buffer, int offset)
>> +{
> This removes any and all CR, not just trimming the trailing ones, so
> the function is misnamed. Call it remove_cr_after() perhaps?
>
> Alternatively we could tighten the implementation and strip only the
> CR that come immediately before a LF.  That would be a better longer
> term thing to do, but because you are lifting an existing code from
> the end of the gpg side of the thing, it may make sense to keep the
> implementation as-is, but give it a name that is more faithful to
> what it actually does.  When the dust settles, we may want to
> revisit and fix this helper function to actually trim CRLF into LF
> (and leave CR in the middle of lines intact), but I do not think it
> is urgent.  Just leaving "NEEDSWORK: make it trim only CRs before LFs
> and rename" comment would be OK.

>> +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;
>> +	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;
>> +
>> +	if (!signing_key || signing_key[0] == '\0')
>> +		return error(
>> +			_("user.signingkey needs to be set for ssh signing"));
>> +
>> +	if (istarts_with(signing_key, "ssh-")) {
> Is it common in the ssh world to treat ssh- prefix as case
> insensitive?  Not a strong objection but I tend to prefer to start
> strict unless there is a good reason to be loose when we do not have
> to, as loosening after the fact is much easier than tightening after
> starting with a loose definition.

>
>> +		/* 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 ||
> "keylen = strlen(signing_key)" before that line, for example, could
> have easily avoided the line-wrapping at such a place.  Wrapping at
> places like after ||, i.e. after an operator with a low precedence,
> would make the code easier to follow.

>
>> +		    close_tempfile_gently(temp) < 0) {
>> +			error_errno(_("failed writing ssh signing key to '%s'"),
>> +				    temp->filename.buf);
>> +			goto out;
>> +		}
>> +		ssh_signing_key_file = temp->filename.buf;
> It is kind'a sad that we need a fresh temporary file every time, but
> we can easily tell the user in the documentation that they can use a
> file with a key in it to avoid it, so it's OK (actually, better than
> OK, as without this, we may not consume temporary files but we won't
> offer an ability to take a literal key string).
>
> Is ".git_whatever file in the current directory" a good place to
> have this temporary file?  I would have expected that we would use
> either $GIT_DIR, $HOME, or $TMPDIR for a thing like this (with
> different pros-and-cons discussion).  At least it is consistent with
> how a temporary file for the payload to be sign-verified is created,
> so let's leave it as-is.

>
>> +	} 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,
> Wrap the line before "-n" to keep "-n" and "git" together, if "git"
> is meant as an argument to the "-n" option.

>
>> +		     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 && strstr(signer_stderr.buf, "usage:")) {
>> +		error(_("ssh-keygen -Y sign is needed for ssh signing (available in openssh version 8.2p1+)"));
>> +		goto out;
> This error message is important to give to the end users, but is it
> enough?  That is, unless "usage:" does not appear, we show the whole
> raw error message and that would help end users and those helping
> them to diagnose the issue, but once the underlying program says
> "usage:", no matter what else it says, it is hidden by this code,
> since we assume it is a wrong version of openssh.
>
>> +	}
>> +
>> +	if (ret) {
>> +		error("%s", signer_stderr.buf);
>> +		goto out;
>> +	}
> Also, prehaps
>
> 	if (ret) {
> 		if (strstr(..., "usage"))
> 			error(_("ssh-keygen -Y sign is needed..."));
> 		else
>                          error("%s", signer_stderr.buf);
> 		goto out;
> 	}
>
> would be easier to follow.

>> +		error(_("ssh failed to sign the data"));
>> +		goto out;
>> +	}
>> +
>> +	/* Strip CR from the line endings, in case we are on Windows. */
>> +	strbuf_trim_trailing_cr(signature, bottom);
>> +
>> +out:
>> +	if (temp)
>> +		delete_tempfile(&temp);
>> +	if (buffer_file)
>> +		delete_tempfile(&buffer_file);
> It is clear that the latter one was holding the contents of the
> buffer to be signed, but reminding the readers what "temp" was about
> would be a good move.  Perhaps renaming the variable to "key_file"
> or something may help?

>
>> +	strbuf_release(&signer_stderr);
>> +	strbuf_release(&ssh_signature_filename);
>> +	return ret;
>> +}
> Looking good, except for the "when does 'ret' get updated?
> shouldn't we refrain from reading the resulting buffer when it is
> set?" question.
>
> Thanks for a pleasant read.