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

[Fabian Stelzer <fs@gigacodes.de>]

On 20.07.21 01:53, Junio C Hamano wrote:
> "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.
I thought about adding an if(!fmt->sign) BUG() but since these callbacks 
are static it shoud really only be an issue between patches of the set.
>
>> @@ -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.
Agreed. I've renamed it and added the comment.
>
> Shouldn't the offset (aka bottom) be of type size_t?
fixed
>> +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.
I don't think so. I will make it case sensitive.
>
>> +		/* 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.
agreed.
>
>> +		    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.
Intuitively i thought just using mks_tempfile_t() would choose a good 
dir for such files.
>
>> +	} 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.
done. some things clang-format can't really understand. overall it is 
quite helpful but a few things i still had to reformat.
>
>> +		     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.

I have changed this to:
if (ret) {
     if (strstr(..., "usage"))
         error(_("ssh-keygen -Y sign is needed..."));

     error("%s", signer_stderr.buf);
     goto out;
}
and removed the if (ret) further down in the function that had no effect.

I had removed the raw stderr output from verify & sign because it 
becomes quite unreadable when doing a "git log --show-signature" with 
ssh signatures present and not support in ssh for it.
I think in case of signing the full output is good. The user is taking 
an active action (wanting to sign something) so we should give them all 
the help we can when things go wrong.
The output for verification is debatable. The config might have a 
"log.showSignature" for verifying gpg signatures and when ssh signatures 
show up we should tell them it's not supported on their setup but 
probably not showing endless lines of errors when they do a "git log". A 
"git verify-commit" might be a different case. But code-path-wise this 
is the same thing at the moment.

>> +		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?
renamed to "key_file"
>
>> +	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.
Thanks a lot for your support with this!
>