git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2] ssh signing: better error message when key not in agent

From
Junio C Hamano <gitster@pobox.com>
Date
Jan 24, 2023, 17:52 UTC
Message-ID
<xmqq1qnjhlbf.fsf@gitster.g>
In-Reply-To
<pull.1270.v2.git.git.1674573972087.gitgitgadget@gmail.com>
"Adam Szkoda via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 27 quoted lines
> From: Adam Szkoda <adaszko@gmail.com>
>
> When signing a commit with a SSH key, with the private key missing from
> ssh-agent, a confusing error message is produced:
>
>     error: Load key
>     "/var/folders/t5/cscwwl_n3n1_8_5j_00x_3t40000gn/T//.git_signing_key_tmpkArSj7":
>     invalid format? fatal: failed to write commit object
>
> The temporary file .git_signing_key_tmpkArSj7 created by git contains a
> valid *public* key.  The error message comes from `ssh-keygen -Y sign' and
> is caused by a fallback mechanism in ssh-keygen whereby it tries to
> interpret .git_signing_key_tmpkArSj7 as a *private* key if it can't find in
> the agent [1].  A fix is scheduled to be released in OpenSSH 9.1. All that
> needs to be done is to pass an additional backward-compatible option -U to
> 'ssh-keygen -Y sign' call.  With '-U', ssh-keygen always interprets the file
> as public key and expects to find the private key in the agent.
>
> As a result, when the private key is missing from the agent, a more accurate
> error message gets produced:
>
>     error: Couldn't find key in agent
>
> [1] https://bugzilla.mindrot.org/show_bug.cgi?id=3429
>
> Signed-off-by: Adam Szkoda <adaszko@gmail.com>
> ---
Well explained.
Show 7 quoted lines
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1270/radicle-dev/maint-v2
> Pull-Request: https://github.com/git/git/pull/1270
>
> Range-diff vs v1:
>
>  1:  0ce06076242 < -:  ----------- ssh signing: better error message when key not in agent
>  -:  ----------- > 1:  03dfca79387 ssh signing: better error message when key not in agent
This is a fairly useless range-diff.

Even when a range-diff shows the differences in the patches, mechanically generated range-diff can only show _what_ changed. It is helpful to explain the changes in your own words to highlight _why_ such changes are done, and this place after the "---" line and the diffstat we see below is the place to do so.

Does GitGitGadget allow its users to describe the differences since the previous iteration yourself?

Show 36 quoted lines
>  gpg-interface.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index f877a1ea564..33899a450eb 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -998,6 +998,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
>  	char *ssh_signing_key_file = NULL;
>  	struct strbuf ssh_signature_filename = STRBUF_INIT;
>  	const char *literal_key = NULL;
> +	int literal_ssh_key = 0;
>  
>  	if (!signing_key || signing_key[0] == '\0')
>  		return error(
> @@ -1005,6 +1006,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
>  
>  	if (is_literal_ssh_key(signing_key, &literal_key)) {
>  		/* A literal ssh key */
> +		literal_ssh_key = 1;
>  		key_file = mks_tempfile_t(".git_signing_key_tmpXXXXXX");
>  		if (!key_file)
>  			return error_errno(
> @@ -1036,11 +1038,14 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
>  	}
>  
>  	strvec_pushl(&signer.args, use_format->program,
> -		     "-Y", "sign",
> -		     "-n", "git",
> -		     "-f", ssh_signing_key_file,
> -		     buffer_file->filename.buf,
> -		     NULL);
> +			"-Y", "sign",
> +			"-n", "git",
> +			"-f", ssh_signing_key_file,
> +			NULL);

Please avoid making a pointless indentation change like this. We do not pass filename yet with this pushl(), because ...

> +	if (literal_ssh_key) {
> +		strvec_push(&signer.args, "-U");
> +	}
... when we give a literal key, we want to insert "-U" in front, and then
> +	strvec_push(&signer.args, buffer_file->filename.buf);
... the filename.  Which makes sense.

The insertion of "-U" is a single statement as the body of a if() statement. We do not want {} around it, by the way.

Other than that, nicely done.  Thanks.
>  	sigchain_push(SIGPIPE, SIG_IGN);
>  	ret = pipe_command(&signer, NULL, 0, NULL, 0, &signer_stderr, 0);
>
> base-commit: 844ede312b4e988881b6e27e352f469d8ab80b2a
Previous: Adam Szkoda via GitGitGadgetNext: Adam Szkoda
Message 11 of 18 in “ssh signing: better error message when key not in agent”
  1. ssh signing: better error message when key not in agentAdam Szkoda via GitGitGadget, Jan 18, 2023
  2. Phillip WoodJan 18, 2023
  3. Phillip WoodJan 18, 2023
  4. Adam SzkodaJan 18, 2023
  5. Phillip WoodJan 18, 2023
  6. Fabian StelzerJan 20, 2023
  7. Phillip WoodJan 23, 2023
  8. Fabian StelzerJan 23, 2023
  9. Adam SzkodaJan 23, 2023
  10. ssh signing: better error message when key not in agentAdam Szkoda via GitGitGadget, Jan 24, 2023
  11. Junio C HamanoJan 24, 2023
  12. Adam SzkodaJan 25, 2023
  13. Junio C HamanoJan 25, 2023
  14. Junio C HamanoJan 25, 2023
  15. Eric SunshineJan 25, 2023
  16. Junio C HamanoJan 25, 2023
  17. Eric SunshineFeb 15, 2023
  18. ssh signing: better error message when key not in agentAdam Szkoda via GitGitGadget, Jan 25, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.