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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 19, 2021, 23:53 UTC
Message-ID: <xmqqpmvdj1xp.fsf@gitster.g>
In-Reply-To: <2c75adee8e1d6147c5be1b3b0832cc90d44ba6df.1626701596.git.gitgitgadget@gmail.com>

"Fabian Stelzer via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 13 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

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

Now it gets interesting ;-)

Show 16 quoted lines

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

Show 7 quoted lines

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

Show 6 quoted lines

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

Show 20 quoted lines

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

Show 9 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 5 quoted lines

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

Show 12 quoted lines

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