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