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

[Junio C Hamano <gitster@pobox.com>]

"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.

Shouldn't the offset (aka bottom) be of type size_t?

I do not recommend giving the function a name that begins with
"strbuf_", as it would tempt unthinking person to suggest moving it
to strbuf.c, but the presense of the "offset" thing means it will be
klunky to reuse in other more generic contexts as a part of the
strbuf API.

> +	size_t i, j;
> +
> +	for (i = j = offset; i < buffer->len; i++) {
> +		if (buffer->buf[i] != '\r') {
> ...

Now it gets interesting ;-)

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

> +	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, 2048) < 0) {

Is it likely that signature file is smaller than 2kB?  I am just
wondering how much we care to pick the default that is specific to
this codepath vs passing 0 to ask the strbuf API to use whatever
default it wants to.

> +		error_errno(
> +			_("failed reading ssh signing data buffer from '%s'"),
> +			ssh_signature_filename.buf);
> +	}
> +	unlink_or_warn(ssh_signature_filename.buf);

Wait a bit.  Even when pipe_command() tells us that we failed, we
read from ssh_signature_filename anyway?  What is going on?  And ...

> +	if (ret) {

... does this ever trigger?  I thought we would have hit one of the
two "goto out" when ret signals an error by being non-zero earlier,
and since then nobody touched the variable so far.

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