[v6,2/9] ssh signing: add ssh signature format and signing using ssh keys

Commit Message

Fabian Stelzer July 28, 2021, 7:36 p.m. UTC

From: Fabian Stelzer <[e-mail]>

implements the actual sign_buffer_ssh operation and move some shared
cleanup code into a strbuf function

Set gpg.format = ssh and user.signingkey to either a ssh public key
string (like from an authorized_keys file), or a ssh key file.
If the key file or the config value itself contains only a public key
then the private key needs to be available via ssh-agent.

gpg.ssh.program can be set to an alternative location of ssh-keygen.
A somewhat recent openssh version (8.2p1+) of ssh-keygen is needed for
this feature. Since only ssh-keygen is needed it can this way be
installed seperately without upgrading your system openssh packages.

Signed-off-by: Fabian Stelzer <[e-mail]>
---
 gpg-interface.c | 137 +++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 129 insertions(+), 8 deletions(-)

Comments

Jonathan Tan July 28, 2021, 10:45 p.m. UTC | #1

Keep the commit titles to 50 characters or fewer. E.g.:

  gpg-interface: teach "ssh" gpg.format

> implements the actual sign_buffer_ssh operation and move some shared
> cleanup code into a strbuf function

Capitalization and punctuation.

> Set gpg.format = ssh and user.signingkey to either a ssh public key
> string (like from an authorized_keys file), or a ssh key file.
> If the key file or the config value itself contains only a public key
> then the private key needs to be available via ssh-agent.
> 
> gpg.ssh.program can be set to an alternative location of ssh-keygen.
> A somewhat recent openssh version (8.2p1+) of ssh-keygen is needed for
> this feature. Since only ssh-keygen is needed it can this way be
> installed seperately without upgrading your system openssh packages.

I notice that end-user documentation (e.g. about gpg.ssh.program) is in
its own patch, but could that be added as functionality is being
implemented? That makes it easier for reviewers to understand what's
being implemented in each patch.

> @@ -463,12 +482,30 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
>  	return use_format->sign_buffer(buffer, signature, signing_key);
>  }
>  
> +/*
> + * Strip CR from the line endings, in case we are on Windows.
> + * NEEDSWORK: make it trim only CRs before LFs and rename
> + */
> +static void remove_cr_after(struct strbuf *buffer, size_t offset)
> +{
> +	size_t i, j;
> +
> +	for (i = j = offset; i < buffer->len; i++) {
> +		if (buffer->buf[i] != '\r') {
> +			if (i != j)
> +				buffer->buf[j] = buffer->buf[i];
> +			j++;
> +		}
> +	}
> +	strbuf_setlen(buffer, j);
> +}

In the future, I would prefer refactoring like this to be in its own
patch. For the moment, this should probably be called "remove_cr" (no
"after" as CRs are removed wherever they are in the string).

> +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, keylen;
> +	struct strbuf signer_stderr = STRBUF_INIT;
> +	struct tempfile *key_file = 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 (starts_with(signing_key, "ssh-")) {
> +		/* A literal ssh key */
> +		key_file = mks_tempfile_t(".git_signing_key_tmpXXXXXX");
> +		if (!key_file)
> +			return error_errno(
> +				_("could not create temporary file"));
> +		keylen = strlen(signing_key);
> +		if (write_in_full(key_file->fd, signing_key, keylen) < 0 ||
> +		    close_tempfile_gently(key_file) < 0) {
> +			error_errno(_("failed writing ssh signing key to '%s'"),
> +				    key_file->filename.buf);
> +			goto out;
> +		}
> +		ssh_signing_key_file = key_file->filename.buf;
> +	} else {
> +		/* We assume a file */
> +		ssh_signing_key_file = expand_user_path(signing_key, 1);
> +	}

A config that has 2 modes of operation is quite error-prone, I think.
For example, a user could put a path starting with "ssh-" (admittedly
unlikely since it would usually be an absolute path, but not
impossible). And also from an implementation point of view, here the
"ssh-" is case-sensitive, but in a future patch, there is a "ssh-" that
is case-insensitive.

Can this just always take a path?

> +	if (ret) {
> +		if (strstr(signer_stderr.buf, "usage:"))
> +			error(_("ssh-keygen -Y sign is needed for ssh signing (available in openssh version 8.2p1+)"));
> +
> +		error("%s", signer_stderr.buf);
> +		goto out;
> +	}

Checking for "usage:" seems fragile -  a binary running in a different
locale might emit a different string, and legitimate output may somehow
contain the string "usage:". Is there a different way to detect a
version mismatch?

Junio C Hamano July 29, 2021, 1:01 a.m. UTC | #2

Jonathan Tan <[e-mail]> writes:

>> +/*
>> + * Strip CR from the line endings, in case we are on Windows.
>> + * NEEDSWORK: make it trim only CRs before LFs and rename
>> + */
>> +static void remove_cr_after(struct strbuf *buffer, size_t offset)
>> +{
>> +	size_t i, j;
>> +
>> +	for (i = j = offset; i < buffer->len; i++) {
>> +		if (buffer->buf[i] != '\r') {
>> +			if (i != j)
>> +				buffer->buf[j] = buffer->buf[i];
>> +			j++;
>> +		}
>> +	}
>> +	strbuf_setlen(buffer, j);
>> +}
>
> In the future, I would prefer refactoring like this to be in its own
> patch. For the moment, this should probably be called "remove_cr" (no
> "after" as CRs are removed wherever they are in the string).

You have me to blame for that "after".  It was meant to signal that
CR's before the given "offset" are retained.

> A config that has 2 modes of operation is quite error-prone, I think.
> For example, a user could put a path starting with "ssh-" (admittedly
> unlikely since it would usually be an absolute path, but not
> impossible). And also from an implementation point of view, here the
> "ssh-" is case-sensitive, but in a future patch, there is a "ssh-" that
> is case-insensitive.
>
> Can this just always take a path?

Sensible simplification, I guess.

Thanks for a careful review.

>> +	if (ret) {
>> +		if (strstr(signer_stderr.buf, "usage:"))
>> +			error(_("ssh-keygen -Y sign is needed for ssh signing (available in openssh version 8.2p1+)"));
>> +
>> +		error("%s", signer_stderr.buf);
>> +		goto out;
>> +	}
>
> Checking for "usage:" seems fragile -  a binary running in a different
> locale might emit a different string, and legitimate output may somehow
> contain the string "usage:". Is there a different way to detect a
> version mismatch?

Fabian Stelzer July 29, 2021, 11:01 a.m. UTC | #3

On 29.07.21 00:45, Jonathan Tan wrote:
> Keep the commit titles to 50 characters or fewer. E.g.:
> 
>    gpg-interface: teach "ssh" gpg.format
> 

i will go over my commits and shorten them although I find your example 
very unclear. or did you mean: teach "ssh" to gpg.format ?

>> implements the actual sign_buffer_ssh operation and move some shared
>> cleanup code into a strbuf function
> 
> Capitalization and punctuation.
fixed

> 
>> Set gpg.format = ssh and user.signingkey to either a ssh public key
>> string (like from an authorized_keys file), or a ssh key file.
>> If the key file or the config value itself contains only a public key
>> then the private key needs to be available via ssh-agent.
>>
>> gpg.ssh.program can be set to an alternative location of ssh-keygen.
>> A somewhat recent openssh version (8.2p1+) of ssh-keygen is needed for
>> this feature. Since only ssh-keygen is needed it can this way be
>> installed seperately without upgrading your system openssh packages.
> 
> I notice that end-user documentation (e.g. about gpg.ssh.program) is in
> its own patch, but could that be added as functionality is being
> implemented? That makes it easier for reviewers to understand what's
> being implemented in each patch.
> 

I can move the user.signingkey & gpg.format part into the signing 
implementation commit and the rest into the verification. I don't see 
much benefit in splitting it up further. I don't want to split up parts 
of the same documentation block into separate commits.

>> +
>> +	if (starts_with(signing_key, "ssh-")) {
>> +		/* A literal ssh key */
>> +		key_file = mks_tempfile_t(".git_signing_key_tmpXXXXXX");
>> +		if (!key_file)
>> +			return error_errno(
>> +				_("could not create temporary file"));
>> +		keylen = strlen(signing_key);
>> +		if (write_in_full(key_file->fd, signing_key, keylen) < 0 ||
>> +		    close_tempfile_gently(key_file) < 0) {
>> +			error_errno(_("failed writing ssh signing key to '%s'"),
>> +				    key_file->filename.buf);
>> +			goto out;
>> +		}
>> +		ssh_signing_key_file = key_file->filename.buf;
>> +	} else {
>> +		/* We assume a file */
>> +		ssh_signing_key_file = expand_user_path(signing_key, 1);
>> +	}
> 
> A config that has 2 modes of operation is quite error-prone, I think.
> For example, a user could put a path starting with "ssh-" (admittedly
> unlikely since it would usually be an absolute path, but not
> impossible). And also from an implementation point of view, here the
> "ssh-" is case-sensitive, but in a future patch, there is a "ssh-" that
> is case-insensitive.
> 
> Can this just always take a path?
> 

I found the ability to specify the key literally useful since i don't 
need an extra file for my public key. In my case all keys come from an 
ssh-agent anyway but I'd like to be able to select which one to use for 
signing. But i'm not hard pressed on this feature. If consenus is this 
complicates things then i can remove it.

>> +	if (ret) {
>> +		if (strstr(signer_stderr.buf, "usage:"))
>> +			error(_("ssh-keygen -Y sign is needed for ssh signing (available in openssh version 8.2p1+)"));
>> +
>> +		error("%s", signer_stderr.buf);
>> +		goto out;
>> +	}
> 
> Checking for "usage:" seems fragile -  a binary running in a different
> locale might emit a different string, and legitimate output may somehow
> contain the string "usage:". Is there a different way to detect a
> version mismatch?
> 

I agree. Unfortunately i did not find any better way. But i think the 
risk of doing something wrong here is quite low. We only check for 
"usage:" in case ssh-keygen fails. And all we do if we find it is give 
the user an extra hint on what the problem probably is.
In any case we print the full stderr output as well.

Josh Steadmon July 29, 2021, 7:09 p.m. UTC | #4

Thanks for this series, it sounds like a great idea. I have a few
comments, inline below.

On 2021.07.28 19:36, Fabian Stelzer via GitGitGadget wrote:
[snip]
> +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, keylen;
> +	struct strbuf signer_stderr = STRBUF_INIT;
> +	struct tempfile *key_file = 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 (starts_with(signing_key, "ssh-")) {
> +		/* A literal ssh key */
> +		key_file = mks_tempfile_t(".git_signing_key_tmpXXXXXX");
> +		if (!key_file)
> +			return error_errno(
> +				_("could not create temporary file"));
> +		keylen = strlen(signing_key);
> +		if (write_in_full(key_file->fd, signing_key, keylen) < 0 ||
> +		    close_tempfile_gently(key_file) < 0) {
> +			error_errno(_("failed writing ssh signing key to '%s'"),
> +				    key_file->filename.buf);
> +			goto out;
> +		}
> +		ssh_signing_key_file = key_file->filename.buf;

You probably want to call strbuf_detach() here, because...

> +	} else {
> +		/* We assume a file */
> +		ssh_signing_key_file = expand_user_path(signing_key, 1);
> +	}

... you need to free the memory returned by expand_user_path(). If you
detach the strbuf above, you can unconditionally
free(ssh_signing_key_file) at the end of this function.

> +
> +	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,
> +		     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) {
> +		if (strstr(signer_stderr.buf, "usage:"))
> +			error(_("ssh-keygen -Y sign is needed for ssh signing (available in openssh version 8.2p1+)"));

I share Jonathan Tan's concern about checking for "usage:" in the stderr
output here. I think in patch 6 the tests rely on a specific return code
to check that "-Y sign" is working as expected; can that be used here
instead?

> +
> +		error("%s", signer_stderr.buf);
> +		goto out;
> +	}
> +
> +	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, 0) < 0) {
> +		error_errno(
> +			_("failed reading ssh signing data buffer from '%s'"),
> +			ssh_signature_filename.buf);
> +	}
> +	unlink_or_warn(ssh_signature_filename.buf);
> +
> +	/* Strip CR from the line endings, in case we are on Windows. */
> +	remove_cr_after(signature, bottom);
> +
> +out:
> +	if (key_file)
> +		delete_tempfile(&key_file);
> +	if (buffer_file)
> +		delete_tempfile(&buffer_file);
> +	strbuf_release(&signer_stderr);
> +	strbuf_release(&ssh_signature_filename);
> +	return ret;
> +}
> -- 
> gitgitgadget
>

Fabian Stelzer July 29, 2021, 9:25 p.m. UTC | #5

On 29.07.21 21:09, Josh Steadmon wrote:
> Thanks for this series, it sounds like a great idea. I have a few
> comments, inline below.
>

Thanks for your review and help with this patch.

> On 2021.07.28 19:36, Fabian Stelzer via GitGitGadget wrote:
> [snip]
>> +		ssh_signing_key_file = key_file->filename.buf;
> 
> You probably want to call strbuf_detach() here, because...
> 
>> +	} else {
>> +		/* We assume a file */
>> +		ssh_signing_key_file = expand_user_path(signing_key, 1);
>> +	}
> 
> ... you need to free the memory returned by expand_user_path(). If you
> detach the strbuf above, you can unconditionally
> free(ssh_signing_key_file) at the end of this function.
> 

fixed. thanks

>> +
>> +	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,
>> +		     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) {
>> +		if (strstr(signer_stderr.buf, "usage:"))
>> +			error(_("ssh-keygen -Y sign is needed for ssh signing (available in openssh version 8.2p1+)"));
> 
> I share Jonathan Tan's concern about checking for "usage:" in the stderr
> output here. I think in patch 6 the tests rely on a specific return code
> to check that "-Y sign" is working as expected; can that be used here
> instead?

In the test setup i first check if ssh-keygen at all is present (exit 
code 127 means command not found). Afterwards i check for a specific 
error message from the command if it is present. Not sure how portable 
this is, but i can do that because i give known invalid parameters to 
it. I can't do this here without doing an additional call to ssh-keygen 
just to check this.

> 
>> +
>> +		error("%s", signer_stderr.buf);
>> +		goto out;
>> +	}
>> +
>> +	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, 0) < 0) {
>> +		error_errno(
>> +			_("failed reading ssh signing data buffer from '%s'"),
>> +			ssh_signature_filename.buf);
>> +	}
>> +	unlink_or_warn(ssh_signature_filename.buf);
>> +
>> +	/* Strip CR from the line endings, in case we are on Windows. */
>> +	remove_cr_after(signature, bottom);
>> +
>> +out:
>> +	if (key_file)
>> +		delete_tempfile(&key_file);
>> +	if (buffer_file)
>> +		delete_tempfile(&buffer_file);
>> +	strbuf_release(&signer_stderr);
>> +	strbuf_release(&ssh_signature_filename);
>> +	return ret;
>> +}
>> -- 
>> gitgitgadget
>>

Patch

diff --git a/gpg-interface.c b/gpg-interface.c
index 31cf4ba3938..c131977b347 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -41,12 +41,20 @@  static const char *x509_sigs[] = {
 	NULL
 };
 
+static const char *ssh_verify_args[] = { NULL };
+static const char *ssh_sigs[] = {
+	"-----BEGIN SSH SIGNATURE-----",
+	NULL
+};
+
 static int verify_gpg_signed_buffer(struct signature_check *sigc,
 				    struct gpg_format *fmt, const char *payload,
 				    size_t payload_size, const char *signature,
 				    size_t signature_size);
 static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,
 			   const char *signing_key);
+static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
+			   const char *signing_key);
 
 static struct gpg_format gpg_format[] = {
 	{
@@ -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
+	},
 };
 
 static struct gpg_format *use_format = &gpg_format[0];
@@ -443,6 +459,9 @@  int git_gpg_config(const char *var, const char *value, void *cb)
 	if (!strcmp(var, "gpg.x509.program"))
 		fmtname = "x509";
 
+	if (!strcmp(var, "gpg.ssh.program"))
+		fmtname = "ssh";
+
 	if (fmtname) {
 		fmt = get_format_by_name(fmtname);
 		return git_config_string(&fmt->program, var, value);
@@ -463,12 +482,30 @@  int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
 	return use_format->sign_buffer(buffer, signature, signing_key);
 }
 
+/*
+ * Strip CR from the line endings, in case we are on Windows.
+ * NEEDSWORK: make it trim only CRs before LFs and rename
+ */
+static void remove_cr_after(struct strbuf *buffer, size_t offset)
+{
+	size_t i, j;
+
+	for (i = j = offset; i < buffer->len; i++) {
+		if (buffer->buf[i] != '\r') {
+			if (i != j)
+				buffer->buf[j] = buffer->buf[i];
+			j++;
+		}
+	}
+	strbuf_setlen(buffer, j);
+}
+
 static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,
 		    const char *signing_key)
 {
 	struct child_process gpg = CHILD_PROCESS_INIT;
 	int ret;
-	size_t i, j, bottom;
+	size_t bottom;
 	struct strbuf gpg_status = STRBUF_INIT;
 
 	strvec_pushl(&gpg.args,
@@ -494,13 +531,97 @@  static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,
 		return error(_("gpg failed to sign the data"));
 
 	/* Strip CR from the line endings, in case we are on Windows. */
-	for (i = j = bottom; i < signature->len; i++)
-		if (signature->buf[i] != '\r') {
-			if (i != j)
-				signature->buf[j] = signature->buf[i];
-			j++;
-		}
-	strbuf_setlen(signature, j);
+	remove_cr_after(signature, bottom);
 
 	return 0;
 }
+
+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, keylen;
+	struct strbuf signer_stderr = STRBUF_INIT;
+	struct tempfile *key_file = 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 (starts_with(signing_key, "ssh-")) {
+		/* A literal ssh key */
+		key_file = mks_tempfile_t(".git_signing_key_tmpXXXXXX");
+		if (!key_file)
+			return error_errno(
+				_("could not create temporary file"));
+		keylen = strlen(signing_key);
+		if (write_in_full(key_file->fd, signing_key, keylen) < 0 ||
+		    close_tempfile_gently(key_file) < 0) {
+			error_errno(_("failed writing ssh signing key to '%s'"),
+				    key_file->filename.buf);
+			goto out;
+		}
+		ssh_signing_key_file = key_file->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) {
+		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,
+		     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) {
+		if (strstr(signer_stderr.buf, "usage:"))
+			error(_("ssh-keygen -Y sign is needed for ssh signing (available in openssh version 8.2p1+)"));
+
+		error("%s", signer_stderr.buf);
+		goto out;
+	}
+
+	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, 0) < 0) {
+		error_errno(
+			_("failed reading ssh signing data buffer from '%s'"),
+			ssh_signature_filename.buf);
+	}
+	unlink_or_warn(ssh_signature_filename.buf);
+
+	/* Strip CR from the line endings, in case we are on Windows. */
+	remove_cr_after(signature, bottom);
+
+out:
+	if (key_file)
+		delete_tempfile(&key_file);
+	if (buffer_file)
+		delete_tempfile(&buffer_file);
+	strbuf_release(&signer_stderr);
+	strbuf_release(&ssh_signature_filename);
+	return ret;
+}