{"thread":{"id":"59112","subject":"[PATCH] ssh signing: better error message when key not in agent","startedAt":"2023-01-18T09:00:44Z","lastAt":"2023-02-15T01:23:38Z","messageCount":18,"participants":["Adam Szkoda via GitGitGadget","Phillip Wood","Adam Szkoda","Fabian Stelzer","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"470589","messageId":"pull.1270.git.git.1674029874363.gitgitgadget@gmail.com","threadId":"59112","inReplyTo":null,"subject":"[PATCH] ssh signing: better error message when key not in agent","fromName":"Adam Szkoda via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-18T08:17:54Z","receivedAt":"2023-01-18T09:00:44Z","isPatch":true,"sender":{"key":"adaszko@gmail.com","avatar":"https://avatars.githubusercontent.com/u/165678?v=4"},"body":"From: Adam Szkoda \n\nWhen signing a commit with a SSH key, with the private key missing from\nssh-agent, a confusing error message is produced:\n\n error: Load key\n \"/var/folders/t5/cscwwl_n3n1_8_5j_00x_3t40000gn/T//.git_signing_key_tmpkArSj7\":\n invalid format? fatal: failed to write commit object\n\nThe temporary file .git_signing_key_tmpkArSj7 created by git contains a\nvalid *public* key. The error message comes from `ssh-keygen -Y sign' and\nis caused by a fallback mechanism in ssh-keygen whereby it tries to\ninterpret .git_signing_key_tmpkArSj7 as a *private* key if it can't find in\nthe agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All that\nneeds to be done is to pass an additional backward-compatible option -U to\n'ssh-keygen -Y sign' call. With '-U', ssh-keygen always interprets the file\nas public key and expects to find the private key in the agent.\n\nAs a result, when the private key is missing from the agent, a more accurate\nerror message gets produced:\n\n error: Couldn't find key in agent\n\n[1] https://bugzilla.mindrot.org/show_bug.cgi?id=3429\n\nSigned-off-by: Adam Szkoda \n---\n ssh signing: better error message when key not in agent\n \n When signing a commit with a SSH key, with the private key missing from\n ssh-agent, a confusing error message is produced:\n \n error: Load key \"/var/folders/t5/cscwwl_n3n1_8_5j_00x_3t40000gn/T//.git_signing_key_tmpkArSj7\": invalid format?\n fatal: failed to write commit object\n \n \n The temporary file .git_signing_key_tmpkArSj7 created by git contains a\n valid public key. The error message comes from `ssh-keygen -Y sign' and\n is caused by a fallback mechanism in ssh-keygen whereby it tries to\n interpret .git_signing_key_tmpkArSj7 as a private key if it can't find\n in the agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All\n that needs to be done is to pass an additional backward-compatible\n option -U to 'ssh-keygen -Y sign' call. With '-U', ssh-keygen always\n interprets the file as public key and expects to find the private key in\n the agent.\n \n As a result, when the private key is missing from the agent, a more\n accurate error message gets produced:\n \n error: Couldn't find key in agent\n \n \n [1] https://bugzilla.mindrot.org/show_bug.cgi?id=3429\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1270%2Fradicle-dev%2Fmaint-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1270/radicle-dev/maint-v1\nPull-Request: https://github.com/git/git/pull/1270\n\n gpg-interface.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 280f1fa1a58..4a5913ae942 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -1022,6 +1022,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n \tstrvec_pushl(&signer.args, use_format->program,\n \t\t \"-Y\", \"sign\",\n \t\t \"-n\", \"git\",\n+\t\t \"-U\",\n \t\t \"-f\", ssh_signing_key_file,\n \t\t buffer_file->filename.buf,\n \t\t NULL);\n\nbase-commit: e54793a95afeea1e10de1e5ad7eab914e7416250\n-- \ngitgitgadget\n"},{"id":"470594","messageId":"abec912c-065d-2098-962e-41f9646dd046@dunelm.org.uk","threadId":"59112","inReplyTo":"pull.1270.git.git.1674029874363.gitgitgadget@gmail.com","subject":"Re: [PATCH] ssh signing: better error message when key not in agent","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-01-18T11:10:04Z","receivedAt":"2023-01-18T11:56:04Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Adam\n\nOn 18/01/2023 08:17, Adam Szkoda via GitGitGadget wrote:\n> From: Adam Szkoda \n> \n> When signing a commit with a SSH key, with the private key missing from\n> ssh-agent, a confusing error message is produced:\n> \n> error: Load key\n> \"/var/folders/t5/cscwwl_n3n1_8_5j_00x_3t40000gn/T//.git_signing_key_tmpkArSj7\":\n> invalid format? fatal: failed to write commit object\n> \n> The temporary file .git_signing_key_tmpkArSj7 created by git contains a\n> valid *public* key. The error message comes from `ssh-keygen -Y sign' and\n> is caused by a fallback mechanism in ssh-keygen whereby it tries to\n> interpret .git_signing_key_tmpkArSj7 as a *private* key if it can't find in\n> the agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All that\n> needs to be done is to pass an additional backward-compatible option -U to\n> 'ssh-keygen -Y sign' call. With '-U', ssh-keygen always interprets the file\n> as public key and expects to find the private key in the agent.\n\nThe documentation for user.signingKey says\n\n If gpg.format is set to ssh this can contain the path to either your \nprivate ssh key or the public key when ssh-agent is used.\n\nIf I've understood correctly passing -U will prevent users from setting \nthis to a private key.\n\nBest Wishes\n\nPhillip\n\n> As a result, when the private key is missing from the agent, a more accurate\n> error message gets produced:\n> \n> error: Couldn't find key in agent\n> \n> [1] https://bugzilla.mindrot.org/show_bug.cgi?id=3429\n> \n> Signed-off-by: Adam Szkoda \n> ---\n> ssh signing: better error message when key not in agent\n> \n> When signing a commit with a SSH key, with the private key missing from\n> ssh-agent, a confusing error message is produced:\n> \n> error: Load key \"/var/folders/t5/cscwwl_n3n1_8_5j_00x_3t40000gn/T//.git_signing_key_tmpkArSj7\": invalid format?\n> fatal: failed to write commit object\n> \n> \n> The temporary file .git_signing_key_tmpkArSj7 created by git contains a\n> valid public key. The error message comes from `ssh-keygen -Y sign' and\n> is caused by a fallback mechanism in ssh-keygen whereby it tries to\n> interpret .git_signing_key_tmpkArSj7 as a private key if it can't find\n> in the agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All\n> that needs to be done is to pass an additional backward-compatible\n> option -U to 'ssh-keygen -Y sign' call. With '-U', ssh-keygen always\n> interprets the file as public key and expects to find the private key in\n> the agent.\n> \n> As a result, when the private key is missing from the agent, a more\n> accurate error message gets produced:\n> \n> error: Couldn't find key in agent\n> \n> \n> [1] https://bugzilla.mindrot.org/show_bug.cgi?id=3429\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1270%2Fradicle-dev%2Fmaint-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1270/radicle-dev/maint-v1\n> Pull-Request: https://github.com/git/git/pull/1270\n> \n> gpg-interface.c | 1 +\n> 1 file changed, 1 insertion(+)\n> \n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index 280f1fa1a58..4a5913ae942 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -1022,6 +1022,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n> \tstrvec_pushl(&signer.args, use_format->program,\n> \t\t \"-Y\", \"sign\",\n> \t\t \"-n\", \"git\",\n> +\t\t \"-U\",\n> \t\t \"-f\", ssh_signing_key_file,\n> \t\t buffer_file->filename.buf,\n> \t\t NULL);\n> \n> base-commit: e54793a95afeea1e10de1e5ad7eab914e7416250\n"},{"id":"470623","messageId":"8025d5c7-ab55-c533-1997-05b4c7339d61@dunelm.org.uk","threadId":"59112","inReplyTo":"abec912c-065d-2098-962e-41f9646dd046@dunelm.org.uk","subject":"Re: [PATCH] ssh signing: better error message when key not in agent","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-01-18T14:34:50Z","receivedAt":"2023-01-18T14:44:04Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 18/01/2023 11:10, Phillip Wood wrote:\n>> the agent [1].  A fix is scheduled to be released in OpenSSH 9.1. All \n>> that\n>> needs to be done is to pass an additional backward-compatible option \n>> -U to\n>> 'ssh-keygen -Y sign' call.  With '-U', ssh-keygen always interprets \n>> the file\n>> as public key and expects to find the private key in the agent.\n> \n> The documentation for user.signingKey says\n> \n>  If gpg.format is set to ssh this can contain the path to either your \n> private ssh key or the public key when ssh-agent is used.\n> \n> If I've understood correctly passing -U will prevent users from setting \n> this to a private key.\n\nIf there is an easy way to tell if the user has given us a public key \nthen we could pass \"-U\" in that case.\n\nBest Wishes\n\nPhillip\n"},{"id":"470624","messageId":"CAEroKagqxC86X0SD8=tK0w+yXL7QecZ+z_7sja-K6ajs0=Z=BQ@mail.gmail.com","threadId":"59112","inReplyTo":"8025d5c7-ab55-c533-1997-05b4c7339d61@dunelm.org.uk","subject":"Re: [PATCH] ssh signing: better error message when key not in agent","fromName":"Adam Szkoda","fromEmail":"adaszko@gmail.com","sentAt":"2023-01-18T15:28:50Z","receivedAt":"2023-01-18T15:30:35Z","isPatch":true,"sender":{"key":"adaszko@gmail.com","avatar":"https://avatars.githubusercontent.com/u/165678?v=4"},"body":"Hi Phillip,\n\nGood point! My first thought is to try doing a stat() syscall on the\npath from 'user.signingKey' to see if it exists and if not, treat it\nas a public key (and pass the -U option). If that sounds reasonable,\nI can update the patch.\n\nBest\n— Adam\n\n\nOn Wed, Jan 18, 2023 at 3:34 PM Phillip Wood wrote:\n>\n> On 18/01/2023 11:10, Phillip Wood wrote:\n> >> the agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All\n> >> that\n> >> needs to be done is to pass an additional backward-compatible option\n> >> -U to\n> >> 'ssh-keygen -Y sign' call. With '-U', ssh-keygen always interprets\n> >> the file\n> >> as public key and expects to find the private key in the agent.\n> >\n> > The documentation for user.signingKey says\n> >\n> > If gpg.format is set to ssh this can contain the path to either your\n> > private ssh key or the public key when ssh-agent is used.\n> >\n> > If I've understood correctly passing -U will prevent users from setting\n> > this to a private key.\n>\n> If there is an easy way to tell if the user has given us a public key\n> then we could pass \"-U\" in that case.\n>\n> Best Wishes\n>\n> Phillip\n"},{"id":"470644","messageId":"55282dec-825f-8c4b-1fb0-6e26ec326db1@dunelm.org.uk","threadId":"59112","inReplyTo":"CAEroKagqxC86X0SD8=tK0w+yXL7QecZ+z_7sja-K6ajs0=Z=BQ@mail.gmail.com","subject":"Re: [PATCH] ssh signing: better error message when key not in agent","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-01-18T16:29:11Z","receivedAt":"2023-01-18T16:31:43Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Adam\n\nI've cc'd Fabian who knows more about the ssh signing code that I do.\n\nOn 18/01/2023 15:28, Adam Szkoda wrote:\n> Hi Phillip,\n> \n> Good point! My first thought is to try doing a stat() syscall on the\n> path from 'user.signingKey' to see if it exists and if not, treat it\n> as a public key (and pass the -U option). If that sounds reasonable,\n> I can update the patch.\n\nMy reading of the documentation is that user.signingKey may point to a \npublic or private key so I'm not sure how stat()ing would help. Looking \nat the code in sign_buffer_ssh() we have a function is_literal_ssh_key() \nthat checks if the config value is a public key. When the user passes \nthe path to a key we could read the file check use is_literal_ssh_key() \nto check if it is a public key (or possibly just check if the file \nbegins with \"ssh-\"). Fabian - does that sound reasonable?\n\nBest Wishes\n\nPhillip\n\n> Best\n> — Adam\n> \n> \n> On Wed, Jan 18, 2023 at 3:34 PM Phillip Wood wrote:\n>>\n>> On 18/01/2023 11:10, Phillip Wood wrote:\n>>>> the agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All\n>>>> that\n>>>> needs to be done is to pass an additional backward-compatible option\n>>>> -U to\n>>>> 'ssh-keygen -Y sign' call. With '-U', ssh-keygen always interprets\n>>>> the file\n>>>> as public key and expects to find the private key in the agent.\n>>>\n>>> The documentation for user.signingKey says\n>>>\n>>> If gpg.format is set to ssh this can contain the path to either your\n>>> private ssh key or the public key when ssh-agent is used.\n>>>\n>>> If I've understood correctly passing -U will prevent users from setting\n>>> this to a private key.\n>>\n>> If there is an easy way to tell if the user has given us a public key\n>> then we could pass \"-U\" in that case.\n>>\n>> Best Wishes\n>>\n>> Phillip\n"},{"id":"470775","messageId":"20230120090331.37dxkko6bgxbjae7@fs","threadId":"59112","inReplyTo":"55282dec-825f-8c4b-1fb0-6e26ec326db1@dunelm.org.uk","subject":"Re: [PATCH] ssh signing: better error message when key not in agent","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2023-01-20T09:03:31Z","receivedAt":"2023-01-20T09:03:38Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 18.01.2023 16:29, Phillip Wood wrote:\n>Hi Adam\n>\n>I've cc'd Fabian who knows more about the ssh signing code that I do.\n>\n>On 18/01/2023 15:28, Adam Szkoda wrote:\n>>Hi Phillip,\n>>\n>>Good point! My first thought is to try doing a stat() syscall on the\n>>path from 'user.signingKey' to see if it exists and if not, treat it\n>>as a public key (and pass the -U option). If that sounds reasonable,\n>>I can update the patch.\n>\n>My reading of the documentation is that user.signingKey may point to a \n>public or private key so I'm not sure how stat()ing would help. \n>Looking at the code in sign_buffer_ssh() we have a function \n>is_literal_ssh_key() that checks if the config value is a public key. \n>When the user passes the path to a key we could read the file check \n>use is_literal_ssh_key() to check if it is a public key (or possibly \n>just check if the file begins with \"ssh-\"). Fabian - does that sound \n>reasonable?\n\nHi,\nI have encountered the mentioned problem before as well and tried to fix it \nbut did not find a good / reasonable way to do so. Git just passes the \nuser.signingKey to ssh-keygen which states:\n`The key used for signing is specified using the -f option and may refer to \neither a private key, or a public key with the private half available via \nssh-agent(1)`\n\nI don't think it's a good idea for git to parse the key and try to determine \nif it's public or private. The fix should probably be in openssh (different \nerror message) but when looking into it last time i remember that the logic \nfor using the key is quite deeply embedded into the ssh code and not easily \nadjusted for the signing use case. At the moment I don't have the time to \nlook into it but the openssh code for signing is quite readable so feel free \nto give it a try. Maybe you find a good way.\n\nBest regards,\nFabian\n\n>\n>Best Wishes\n>\n>Phillip\n>\n>>Best\n>>— Adam\n>>\n>>\n>>On Wed, Jan 18, 2023 at 3:34 PM Phillip Wood wrote:\n>>>\n>>>On 18/01/2023 11:10, Phillip Wood wrote:\n>>>>>the agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All\n>>>>>that\n>>>>>needs to be done is to pass an additional backward-compatible option\n>>>>>-U to\n>>>>>'ssh-keygen -Y sign' call. With '-U', ssh-keygen always interprets\n>>>>>the file\n>>>>>as public key and expects to find the private key in the agent.\n>>>>\n>>>>The documentation for user.signingKey says\n>>>>\n>>>> If gpg.format is set to ssh this can contain the path to either your\n>>>>private ssh key or the public key when ssh-agent is used.\n>>>>\n>>>>If I've understood correctly passing -U will prevent users from setting\n>>>>this to a private key.\n>>>\n>>>If there is an easy way to tell if the user has given us a public key\n>>>then we could pass \"-U\" in that case.\n>>>\n>>>Best Wishes\n>>>\n>>>Phillip\n"},{"id":"470903","messageId":"6e57bef8-7387-3341-5ed5-4bcfa7ded7a0@dunelm.org.uk","threadId":"59112","inReplyTo":"20230120090331.37dxkko6bgxbjae7@fs","subject":"Re: [PATCH] ssh signing: better error message when key not in agent","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-01-23T09:33:09Z","receivedAt":"2023-01-23T09:33:17Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 20/01/2023 09:03, Fabian Stelzer wrote:\n> On 18.01.2023 16:29, Phillip Wood wrote:\n>> Hi Adam\n>>\n>> I've cc'd Fabian who knows more about the ssh signing code that I do.\n>>\n>> On 18/01/2023 15:28, Adam Szkoda wrote:\n>>> Hi Phillip,\n>>>\n>>> Good point!  My first thought is to try doing a stat() syscall on the\n>>> path from 'user.signingKey' to see if it exists and if not, treat it\n>>> as a public key (and pass the -U option).  If that sounds reasonable,\n>>> I can update the patch.\n>>\n>> My reading of the documentation is that user.signingKey may point to a \n>> public or private key so I'm not sure how stat()ing would help. \n>> Looking at the code in sign_buffer_ssh() we have a function \n>> is_literal_ssh_key() that checks if the config value is a public key. \n>> When the user passes the path to a key we could read the file check \n>> use is_literal_ssh_key() to check if it is a public key (or possibly \n>> just check if the file begins with \"ssh-\"). Fabian - does that sound \n>> reasonable?\n> \n> Hi,\n> I have encountered the mentioned problem before as well and tried to fix \n> it but did not find a good / reasonable way to do so. Git just passes \n> the user.signingKey to ssh-keygen which states:\n> `The key used for signing is specified using the -f option and may refer \n> to either a private key, or a public key with the private half available \n> via ssh-agent(1)`\n> \n> I don't think it's a good idea for git to parse the key and try to \n> determine if it's public or private. The fix should probably be in \n> openssh (different error message) but when looking into it last time i \n> remember that the logic for using the key is quite deeply embedded into \n> the ssh code and not easily adjusted for the signing use case. At the \n> moment I don't have the time to look into it but the openssh code for \n> signing is quite readable so feel free to give it a try. Maybe you find \n> a good way.\n\nThanks Fabian, perhaps the easiest way forward is for us to only pass \n\"-U\" when we have a literal key in user.signingKey as we know it must a \nbe public key in that case.\n\nBest Wishes\n\nPhillip\n\n> Best regards,\n> Fabian\n> \n>>\n>> Best Wishes\n>>\n>> Phillip\n>>\n>>> Best\n>>> — Adam\n>>>\n>>>\n>>> On Wed, Jan 18, 2023 at 3:34 PM Phillip Wood \n>>> wrote:\n>>>>\n>>>> On 18/01/2023 11:10, Phillip Wood wrote:\n>>>>>> the agent [1].  A fix is scheduled to be released in OpenSSH 9.1. All\n>>>>>> that\n>>>>>> needs to be done is to pass an additional backward-compatible option\n>>>>>> -U to\n>>>>>> 'ssh-keygen -Y sign' call.  With '-U', ssh-keygen always interprets\n>>>>>> the file\n>>>>>> as public key and expects to find the private key in the agent.\n>>>>>\n>>>>> The documentation for user.signingKey says\n>>>>>\n>>>>>   If gpg.format is set to ssh this can contain the path to either your\n>>>>> private ssh key or the public key when ssh-agent is used.\n>>>>>\n>>>>> If I've understood correctly passing -U will prevent users from \n>>>>> setting\n>>>>> this to a private key.\n>>>>\n>>>> If there is an easy way to tell if the user has given us a public key\n>>>> then we could pass \"-U\" in that case.\n>>>>\n>>>> Best Wishes\n>>>>\n>>>> Phillip\n"},{"id":"470904","messageId":"20230123100245.3qbscxkgvbnh7ilt@fs","threadId":"59112","inReplyTo":"6e57bef8-7387-3341-5ed5-4bcfa7ded7a0@dunelm.org.uk","subject":"Re: [PATCH] ssh signing: better error message when key not in agent","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2023-01-23T10:02:45Z","receivedAt":"2023-01-23T10:03:03Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 23.01.2023 09:33, Phillip Wood wrote:\n>On 20/01/2023 09:03, Fabian Stelzer wrote:\n>>On 18.01.2023 16:29, Phillip Wood wrote:\n>>>Hi Adam\n>>>\n>>>I've cc'd Fabian who knows more about the ssh signing code that I do.\n>>>\n>>>On 18/01/2023 15:28, Adam Szkoda wrote:\n>>>>Hi Phillip,\n>>>>\n>>>>Good point!  My first thought is to try doing a stat() syscall on the\n>>>>path from 'user.signingKey' to see if it exists and if not, treat it\n>>>>as a public key (and pass the -U option).  If that sounds reasonable,\n>>>>I can update the patch.\n>>>\n>>>My reading of the documentation is that user.signingKey may point \n>>>to a public or private key so I'm not sure how stat()ing would \n>>>help. Looking at the code in sign_buffer_ssh() we have a function \n>>>is_literal_ssh_key() that checks if the config value is a public \n>>>key. When the user passes the path to a key we could read the file \n>>>check use is_literal_ssh_key() to check if it is a public key (or \n>>>possibly just check if the file begins with \"ssh-\"). Fabian - does \n>>>that sound reasonable?\n>>\n>>Hi,\n>>I have encountered the mentioned problem before as well and tried to \n>>fix it but did not find a good / reasonable way to do so. Git just \n>>passes the user.signingKey to ssh-keygen which states:\n>>`The key used for signing is specified using the -f option and may \n>>refer to either a private key, or a public key with the private half \n>>available via ssh-agent(1)`\n>>\n>>I don't think it's a good idea for git to parse the key and try to \n>>determine if it's public or private. The fix should probably be in \n>>openssh (different error message) but when looking into it last time \n>>i remember that the logic for using the key is quite deeply embedded \n>>into the ssh code and not easily adjusted for the signing use case. \n>>At the moment I don't have the time to look into it but the openssh \n>>code for signing is quite readable so feel free to give it a try. \n>>Maybe you find a good way.\n>\n>Thanks Fabian, perhaps the easiest way forward is for us to only pass \n>\"-U\" when we have a literal key in user.signingKey as we know it must \n>a be public key in that case.\n\nYes, i think that's a good idea as long as the `-U` flag is ignored in older \nssh versions and shouldn't be too hard to implement. And it should work just \nas well when using `defaultKeyCommand`.\n\nBest,\nFabian\n\n>\n>Best Wishes\n>\n>Phillip\n>\n>>Best regards,\n>>Fabian\n>>\n>>>\n>>>Best Wishes\n>>>\n>>>Phillip\n>>>\n>>>>Best\n>>>>— Adam\n>>>>\n>>>>\n>>>>On Wed, Jan 18, 2023 at 3:34 PM Phillip Wood \n>>>> wrote:\n>>>>>\n>>>>>On 18/01/2023 11:10, Phillip Wood wrote:\n>>>>>>>the agent [1].  A fix is scheduled to be released in OpenSSH 9.1. All\n>>>>>>>that\n>>>>>>>needs to be done is to pass an additional backward-compatible option\n>>>>>>>-U to\n>>>>>>>'ssh-keygen -Y sign' call.  With '-U', ssh-keygen always interprets\n>>>>>>>the file\n>>>>>>>as public key and expects to find the private key in the agent.\n>>>>>>\n>>>>>>The documentation for user.signingKey says\n>>>>>>\n>>>>>>  If gpg.format is set to ssh this can contain the path to either your\n>>>>>>private ssh key or the public key when ssh-agent is used.\n>>>>>>\n>>>>>>If I've understood correctly passing -U will prevent users \n>>>>>>from setting\n>>>>>>this to a private key.\n>>>>>\n>>>>>If there is an easy way to tell if the user has given us a public key\n>>>>>then we could pass \"-U\" in that case.\n>>>>>\n>>>>>Best Wishes\n>>>>>\n>>>>>Phillip\n"},{"id":"470928","messageId":"CAEroKaifs8uLnOCsAhqJkEpkpEfRd+HTnTG3i+6syZZ7Ex3dVA@mail.gmail.com","threadId":"59112","inReplyTo":"20230123100245.3qbscxkgvbnh7ilt@fs","subject":"Re: [PATCH] ssh signing: better error message when key not in agent","fromName":"Adam Szkoda","fromEmail":"adaszko@gmail.com","sentAt":"2023-01-23T16:17:30Z","receivedAt":"2023-01-23T16:18:19Z","isPatch":true,"sender":{"key":"adaszko@gmail.com","avatar":"https://avatars.githubusercontent.com/u/165678?v=4"},"body":"Hi! I've pushed a patch that adds `-U` conditional on is_literal_ssh_key().\n\nAccording to the OpenSSH issue ([1]), that option is backward compatible:\n\n> It should be safe to use -U even for older versions. It won't require the agent (as openssh-9.1 will) but it won't cause an error.\n\n[1]: https://bugzilla.mindrot.org/show_bug.cgi?id=3429\n\nCheers\n— Adam\n\n\nOn Mon, Jan 23, 2023 at 11:02 AM Fabian Stelzer wrote:\n>\n> On 23.01.2023 09:33, Phillip Wood wrote:\n> >On 20/01/2023 09:03, Fabian Stelzer wrote:\n> >>On 18.01.2023 16:29, Phillip Wood wrote:\n> >>>Hi Adam\n> >>>\n> >>>I've cc'd Fabian who knows more about the ssh signing code that I do.\n> >>>\n> >>>On 18/01/2023 15:28, Adam Szkoda wrote:\n> >>>>Hi Phillip,\n> >>>>\n> >>>>Good point! My first thought is to try doing a stat() syscall on the\n> >>>>path from 'user.signingKey' to see if it exists and if not, treat it\n> >>>>as a public key (and pass the -U option). If that sounds reasonable,\n> >>>>I can update the patch.\n> >>>\n> >>>My reading of the documentation is that user.signingKey may point\n> >>>to a public or private key so I'm not sure how stat()ing would\n> >>>help. Looking at the code in sign_buffer_ssh() we have a function\n> >>>is_literal_ssh_key() that checks if the config value is a public\n> >>>key. When the user passes the path to a key we could read the file\n> >>>check use is_literal_ssh_key() to check if it is a public key (or\n> >>>possibly just check if the file begins with \"ssh-\"). Fabian - does\n> >>>that sound reasonable?\n> >>\n> >>Hi,\n> >>I have encountered the mentioned problem before as well and tried to\n> >>fix it but did not find a good / reasonable way to do so. Git just\n> >>passes the user.signingKey to ssh-keygen which states:\n> >>`The key used for signing is specified using the -f option and may\n> >>refer to either a private key, or a public key with the private half\n> >>available via ssh-agent(1)`\n> >>\n> >>I don't think it's a good idea for git to parse the key and try to\n> >>determine if it's public or private. The fix should probably be in\n> >>openssh (different error message) but when looking into it last time\n> >>i remember that the logic for using the key is quite deeply embedded\n> >>into the ssh code and not easily adjusted for the signing use case.\n> >>At the moment I don't have the time to look into it but the openssh\n> >>code for signing is quite readable so feel free to give it a try.\n> >>Maybe you find a good way.\n> >\n> >Thanks Fabian, perhaps the easiest way forward is for us to only pass\n> >\"-U\" when we have a literal key in user.signingKey as we know it must\n> >a be public key in that case.\n>\n> Yes, i think that's a good idea as long as the `-U` flag is ignored in older\n> ssh versions and shouldn't be too hard to implement. And it should work just\n> as well when using `defaultKeyCommand`.\n>\n> Best,\n> Fabian\n>\n> >\n> >Best Wishes\n> >\n> >Phillip\n> >\n> >>Best regards,\n> >>Fabian\n> >>\n> >>>\n> >>>Best Wishes\n> >>>\n> >>>Phillip\n> >>>\n> >>>>Best\n> >>>>— Adam\n> >>>>\n> >>>>\n> >>>>On Wed, Jan 18, 2023 at 3:34 PM Phillip Wood\n> >>>> wrote:\n> >>>>>\n> >>>>>On 18/01/2023 11:10, Phillip Wood wrote:\n> >>>>>>>the agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All\n> >>>>>>>that\n> >>>>>>>needs to be done is to pass an additional backward-compatible option\n> >>>>>>>-U to\n> >>>>>>>'ssh-keygen -Y sign' call. With '-U', ssh-keygen always interprets\n> >>>>>>>the file\n> >>>>>>>as public key and expects to find the private key in the agent.\n> >>>>>>\n> >>>>>>The documentation for user.signingKey says\n> >>>>>>\n> >>>>>> If gpg.format is set to ssh this can contain the path to either your\n> >>>>>>private ssh key or the public key when ssh-agent is used.\n> >>>>>>\n> >>>>>>If I've understood correctly passing -U will prevent users\n> >>>>>>from setting\n> >>>>>>this to a private key.\n> >>>>>\n> >>>>>If there is an easy way to tell if the user has given us a public key\n> >>>>>then we could pass \"-U\" in that case.\n> >>>>>\n> >>>>>Best Wishes\n> >>>>>\n> >>>>>Phillip\n"},{"id":"470981","messageId":"pull.1270.v2.git.git.1674573972087.gitgitgadget@gmail.com","threadId":"59112","inReplyTo":"pull.1270.git.git.1674029874363.gitgitgadget@gmail.com","subject":"[PATCH v2] ssh signing: better error message when key not in agent","fromName":"Adam Szkoda via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-24T15:26:11Z","receivedAt":"2023-01-24T15:26:18Z","isPatch":true,"sender":{"key":"adaszko@gmail.com","avatar":"https://avatars.githubusercontent.com/u/165678?v=4"},"body":"From: Adam Szkoda \n\nWhen signing a commit with a SSH key, with the private key missing from\nssh-agent, a confusing error message is produced:\n\n error: Load key\n \"/var/folders/t5/cscwwl_n3n1_8_5j_00x_3t40000gn/T//.git_signing_key_tmpkArSj7\":\n invalid format? fatal: failed to write commit object\n\nThe temporary file .git_signing_key_tmpkArSj7 created by git contains a\nvalid *public* key. The error message comes from `ssh-keygen -Y sign' and\nis caused by a fallback mechanism in ssh-keygen whereby it tries to\ninterpret .git_signing_key_tmpkArSj7 as a *private* key if it can't find in\nthe agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All that\nneeds to be done is to pass an additional backward-compatible option -U to\n'ssh-keygen -Y sign' call. With '-U', ssh-keygen always interprets the file\nas public key and expects to find the private key in the agent.\n\nAs a result, when the private key is missing from the agent, a more accurate\nerror message gets produced:\n\n error: Couldn't find key in agent\n\n[1] https://bugzilla.mindrot.org/show_bug.cgi?id=3429\n\nSigned-off-by: Adam Szkoda \n---\n ssh signing: better error message when key not in agent\n \n When signing a commit with a SSH key, with the private key missing from\n ssh-agent, a confusing error message is produced:\n \n error: Load key \"/var/folders/t5/cscwwl_n3n1_8_5j_00x_3t40000gn/T//.git_signing_key_tmpkArSj7\": invalid format?\n fatal: failed to write commit object\n \n \n The temporary file .git_signing_key_tmpkArSj7 created by git contains a\n valid public key. The error message comes from `ssh-keygen -Y sign' and\n is caused by a fallback mechanism in ssh-keygen whereby it tries to\n interpret .git_signing_key_tmpkArSj7 as a private key if it can't find\n in the agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All\n that needs to be done is to pass an additional backward-compatible\n option -U to 'ssh-keygen -Y sign' call. With '-U', ssh-keygen always\n interprets the file as public key and expects to find the private key in\n the agent.\n \n As a result, when the private key is missing from the agent, a more\n accurate error message gets produced:\n \n error: Couldn't find key in agent\n \n \n [1] https://bugzilla.mindrot.org/show_bug.cgi?id=3429\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1270%2Fradicle-dev%2Fmaint-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1270/radicle-dev/maint-v2\nPull-Request: https://github.com/git/git/pull/1270\n\nRange-diff vs v1:\n\n 1: 0ce06076242 < -: ----------- ssh signing: better error message when key not in agent\n -: ----------- > 1: 03dfca79387 ssh signing: better error message when key not in agent\n\n\n gpg-interface.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex f877a1ea564..33899a450eb 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -998,6 +998,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n \tchar *ssh_signing_key_file = NULL;\n \tstruct strbuf ssh_signature_filename = STRBUF_INIT;\n \tconst char *literal_key = NULL;\n+\tint literal_ssh_key = 0;\n \n \tif (!signing_key || signing_key[0] == '\\0')\n \t\treturn error(\n@@ -1005,6 +1006,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n \n \tif (is_literal_ssh_key(signing_key, &literal_key)) {\n \t\t/* A literal ssh key */\n+\t\tliteral_ssh_key = 1;\n \t\tkey_file = mks_tempfile_t(\".git_signing_key_tmpXXXXXX\");\n \t\tif (!key_file)\n \t\t\treturn error_errno(\n@@ -1036,11 +1038,14 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n \t}\n \n \tstrvec_pushl(&signer.args, use_format->program,\n-\t\t \"-Y\", \"sign\",\n-\t\t \"-n\", \"git\",\n-\t\t \"-f\", ssh_signing_key_file,\n-\t\t buffer_file->filename.buf,\n-\t\t NULL);\n+\t\t\t\"-Y\", \"sign\",\n+\t\t\t\"-n\", \"git\",\n+\t\t\t\"-f\", ssh_signing_key_file,\n+\t\t\tNULL);\n+\tif (literal_ssh_key) {\n+\t\tstrvec_push(&signer.args, \"-U\");\n+\t}\n+\tstrvec_push(&signer.args, buffer_file->filename.buf);\n \n \tsigchain_push(SIGPIPE, SIG_IGN);\n \tret = pipe_command(&signer, NULL, 0, NULL, 0, &signer_stderr, 0);\n\nbase-commit: 844ede312b4e988881b6e27e352f469d8ab80b2a\n-- \ngitgitgadget\n"},{"id":"470993","messageId":"xmqq1qnjhlbf.fsf@gitster.g","threadId":"59112","inReplyTo":"pull.1270.v2.git.git.1674573972087.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] ssh signing: better error message when key not in agent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-24T17:52:20Z","receivedAt":"2023-01-24T17:52:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Adam Szkoda via GitGitGadget\" writes:\n\n> From: Adam Szkoda \n>\n> When signing a commit with a SSH key, with the private key missing from\n> ssh-agent, a confusing error message is produced:\n>\n> error: Load key\n> \"/var/folders/t5/cscwwl_n3n1_8_5j_00x_3t40000gn/T//.git_signing_key_tmpkArSj7\":\n> invalid format? fatal: failed to write commit object\n>\n> The temporary file .git_signing_key_tmpkArSj7 created by git contains a\n> valid *public* key. The error message comes from `ssh-keygen -Y sign' and\n> is caused by a fallback mechanism in ssh-keygen whereby it tries to\n> interpret .git_signing_key_tmpkArSj7 as a *private* key if it can't find in\n> the agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All that\n> needs to be done is to pass an additional backward-compatible option -U to\n> 'ssh-keygen -Y sign' call. With '-U', ssh-keygen always interprets the file\n> as public key and expects to find the private key in the agent.\n>\n> As a result, when the private key is missing from the agent, a more accurate\n> error message gets produced:\n>\n> error: Couldn't find key in agent\n>\n> [1] https://bugzilla.mindrot.org/show_bug.cgi?id=3429\n>\n> Signed-off-by: Adam Szkoda \n> ---\n\nWell explained.\n\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1270/radicle-dev/maint-v2\n> Pull-Request: https://github.com/git/git/pull/1270\n>\n> Range-diff vs v1:\n>\n> 1: 0ce06076242 < -: ----------- ssh signing: better error message when key not in agent\n> -: ----------- > 1: 03dfca79387 ssh signing: better error message when key not in agent\n\nThis is a fairly useless range-diff.\n\nEven when a range-diff shows the differences in the patches,\nmechanically generated range-diff can only show _what_ changed. It\nis helpful to explain the changes in your own words to highlight\n_why_ such changes are done, and this place after the \"---\" line\nand the diffstat we see below is the place to do so.\n\nDoes GitGitGadget allow its users to describe the differences since\nthe previous iteration yourself?\n\n> gpg-interface.c | 15 ++++++++++-----\n> 1 file changed, 10 insertions(+), 5 deletions(-)\n>\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index f877a1ea564..33899a450eb 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -998,6 +998,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n> \tchar *ssh_signing_key_file = NULL;\n> \tstruct strbuf ssh_signature_filename = STRBUF_INIT;\n> \tconst char *literal_key = NULL;\n> +\tint literal_ssh_key = 0;\n> \n> \tif (!signing_key || signing_key[0] == '\\0')\n> \t\treturn error(\n> @@ -1005,6 +1006,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n> \n> \tif (is_literal_ssh_key(signing_key, &literal_key)) {\n> \t\t/* A literal ssh key */\n> +\t\tliteral_ssh_key = 1;\n> \t\tkey_file = mks_tempfile_t(\".git_signing_key_tmpXXXXXX\");\n> \t\tif (!key_file)\n> \t\t\treturn error_errno(\n> @@ -1036,11 +1038,14 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n> \t}\n> \n> \tstrvec_pushl(&signer.args, use_format->program,\n> -\t\t \"-Y\", \"sign\",\n> -\t\t \"-n\", \"git\",\n> -\t\t \"-f\", ssh_signing_key_file,\n> -\t\t buffer_file->filename.buf,\n> -\t\t NULL);\n> +\t\t\t\"-Y\", \"sign\",\n> +\t\t\t\"-n\", \"git\",\n> +\t\t\t\"-f\", ssh_signing_key_file,\n> +\t\t\tNULL);\n\nPlease avoid making a pointless indentation change like this. We do\nnot pass filename yet with this pushl(), because ...\n\n> +\tif (literal_ssh_key) {\n> +\t\tstrvec_push(&signer.args, \"-U\");\n> +\t}\n\n... when we give a literal key, we want to insert \"-U\" in front, and then\n\n> +\tstrvec_push(&signer.args, buffer_file->filename.buf);\n\n... the filename. Which makes sense.\n\nThe insertion of \"-U\" is a single statement as the body of a if()\nstatement. We do not want {} around it, by the way.\n\nOther than that, nicely done. Thanks.\n\n> \tsigchain_push(SIGPIPE, SIG_IGN);\n> \tret = pipe_command(&signer, NULL, 0, NULL, 0, &signer_stderr, 0);\n>\n> base-commit: 844ede312b4e988881b6e27e352f469d8ab80b2a\n"},{"id":"471018","messageId":"pull.1270.v3.git.git.1674650450662.gitgitgadget@gmail.com","threadId":"59112","inReplyTo":"pull.1270.v2.git.git.1674573972087.gitgitgadget@gmail.com","subject":"[PATCH v3] ssh signing: better error message when key not in agent","fromName":"Adam Szkoda via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-25T12:40:50Z","receivedAt":"2023-01-25T12:40:56Z","isPatch":true,"sender":{"key":"adaszko@gmail.com","avatar":"https://avatars.githubusercontent.com/u/165678?v=4"},"body":"From: Adam Szkoda \n\nWhen signing a commit with a SSH key, with the private key missing from\nssh-agent, a confusing error message is produced:\n\n error: Load key\n \"/var/folders/t5/cscwwl_n3n1_8_5j_00x_3t40000gn/T//.git_signing_key_tmpkArSj7\":\n invalid format? fatal: failed to write commit object\n\nThe temporary file .git_signing_key_tmpkArSj7 created by git contains a\nvalid *public* key. The error message comes from `ssh-keygen -Y sign' and\nis caused by a fallback mechanism in ssh-keygen whereby it tries to\ninterpret .git_signing_key_tmpkArSj7 as a *private* key if it can't find in\nthe agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All that\nneeds to be done is to pass an additional backward-compatible option -U to\n'ssh-keygen -Y sign' call. With '-U', ssh-keygen always interprets the file\nas public key and expects to find the private key in the agent.\n\nAs a result, when the private key is missing from the agent, a more accurate\nerror message gets produced:\n\n error: Couldn't find key in agent\n\n[1] https://bugzilla.mindrot.org/show_bug.cgi?id=3429\n\nSigned-off-by: Adam Szkoda \n---\n ssh signing: better error message when key not in agent\n \n When signing a commit with a SSH key, with the private key missing from\n ssh-agent, a confusing error message is produced:\n \n error: Load key \"/var/folders/t5/cscwwl_n3n1_8_5j_00x_3t40000gn/T//.git_signing_key_tmpkArSj7\": invalid format?\n fatal: failed to write commit object\n \n \n The temporary file .git_signing_key_tmpkArSj7 created by git contains a\n valid public key. The error message comes from `ssh-keygen -Y sign' and\n is caused by a fallback mechanism in ssh-keygen whereby it tries to\n interpret .git_signing_key_tmpkArSj7 as a private key if it can't find\n in the agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All\n that needs to be done is to pass an additional backward-compatible\n option -U to 'ssh-keygen -Y sign' call. With '-U', ssh-keygen always\n interprets the file as public key and expects to find the private key in\n the agent.\n \n As a result, when the private key is missing from the agent, a more\n accurate error message gets produced:\n \n error: Couldn't find key in agent\n \n \n [1] https://bugzilla.mindrot.org/show_bug.cgi?id=3429\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1270%2Fradicle-dev%2Fmaint-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1270/radicle-dev/maint-v3\nPull-Request: https://github.com/git/git/pull/1270\n\nRange-diff vs v2:\n\n 1: 03dfca79387 ! 1: dc7acff3b95 ssh signing: better error message when key not in agent\n @@ gpg-interface.c: static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf\n \t\tif (!key_file)\n \t\t\treturn error_errno(\n @@ gpg-interface.c: static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n - \t}\n - \n - \tstrvec_pushl(&signer.args, use_format->program,\n --\t\t \"-Y\", \"sign\",\n --\t\t \"-n\", \"git\",\n --\t\t \"-f\", ssh_signing_key_file,\n + \t\t \"-Y\", \"sign\",\n + \t\t \"-n\", \"git\",\n + \t\t \"-f\", ssh_signing_key_file,\n -\t\t buffer_file->filename.buf,\n --\t\t NULL);\n -+\t\t\t\"-Y\", \"sign\",\n -+\t\t\t\"-n\", \"git\",\n -+\t\t\t\"-f\", ssh_signing_key_file,\n -+\t\t\tNULL);\n -+\tif (literal_ssh_key) {\n + \t\t NULL);\n ++\tif (literal_ssh_key)\n +\t\tstrvec_push(&signer.args, \"-U\");\n -+\t}\n +\tstrvec_push(&signer.args, buffer_file->filename.buf);\n \n \tsigchain_push(SIGPIPE, SIG_IGN);\n\n\n gpg-interface.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex f877a1ea564..687236430bf 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -998,6 +998,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n \tchar *ssh_signing_key_file = NULL;\n \tstruct strbuf ssh_signature_filename = STRBUF_INIT;\n \tconst char *literal_key = NULL;\n+\tint literal_ssh_key = 0;\n \n \tif (!signing_key || signing_key[0] == '\\0')\n \t\treturn error(\n@@ -1005,6 +1006,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n \n \tif (is_literal_ssh_key(signing_key, &literal_key)) {\n \t\t/* A literal ssh key */\n+\t\tliteral_ssh_key = 1;\n \t\tkey_file = mks_tempfile_t(\".git_signing_key_tmpXXXXXX\");\n \t\tif (!key_file)\n \t\t\treturn error_errno(\n@@ -1039,8 +1041,10 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n \t\t \"-Y\", \"sign\",\n \t\t \"-n\", \"git\",\n \t\t \"-f\", ssh_signing_key_file,\n-\t\t buffer_file->filename.buf,\n \t\t NULL);\n+\tif (literal_ssh_key)\n+\t\tstrvec_push(&signer.args, \"-U\");\n+\tstrvec_push(&signer.args, buffer_file->filename.buf);\n \n \tsigchain_push(SIGPIPE, SIG_IGN);\n \tret = pipe_command(&signer, NULL, 0, NULL, 0, &signer_stderr, 0);\n\nbase-commit: 844ede312b4e988881b6e27e352f469d8ab80b2a\n-- \ngitgitgadget\n"},{"id":"471019","messageId":"CAEroKagUY5PfuC2CDn=pTJ=brPsjPy6MVz54mH1tvN8E-Pvk7g@mail.gmail.com","threadId":"59112","inReplyTo":"xmqq1qnjhlbf.fsf@gitster.g","subject":"Re: [PATCH v2] ssh signing: better error message when key not in agent","fromName":"Adam Szkoda","fromEmail":"adaszko@gmail.com","sentAt":"2023-01-25T12:46:36Z","receivedAt":"2023-01-25T12:47:17Z","isPatch":true,"sender":{"key":"adaszko@gmail.com","avatar":"https://avatars.githubusercontent.com/u/165678?v=4"},"body":"On Tue, Jan 24, 2023 at 6:52 PM Junio C Hamano wrote:\n> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1270/radicle-dev/maint-v2\n> > Pull-Request: https://github.com/git/git/pull/1270\n> >\n> > Range-diff vs v1:\n> >\n> > 1: 0ce06076242 < -: ----------- ssh signing: better error message when key not in agent\n> > -: ----------- > 1: 03dfca79387 ssh signing: better error message when key not in agent\n>\n> This is a fairly useless range-diff.\n>\n> Even when a range-diff shows the differences in the patches,\n> mechanically generated range-diff can only show _what_ changed. It\n> is helpful to explain the changes in your own words to highlight\n> _why_ such changes are done, and this place after the \"---\" line\n> and the diffstat we see below is the place to do so.\n>\n> Does GitGitGadget allow its users to describe the differences since\n> the previous iteration yourself?\n\nNo, I don't think it does. It got generated automatically without\ngiving me an opportunity to edit.\n\n> > gpg-interface.c | 15 ++++++++++-----\n> > 1 file changed, 10 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/gpg-interface.c b/gpg-interface.c\n> > index f877a1ea564..33899a450eb 100644\n> > --- a/gpg-interface.c\n> > +++ b/gpg-interface.c\n> > @@ -998,6 +998,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n> > char *ssh_signing_key_file = NULL;\n> > struct strbuf ssh_signature_filename = STRBUF_INIT;\n> > const char *literal_key = NULL;\n> > + int literal_ssh_key = 0;\n> >\n> > if (!signing_key || signing_key[0] == '\\0')\n> > return error(\n> > @@ -1005,6 +1006,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n> >\n> > if (is_literal_ssh_key(signing_key, &literal_key)) {\n> > /* A literal ssh key */\n> > + literal_ssh_key = 1;\n> > key_file = mks_tempfile_t(\".git_signing_key_tmpXXXXXX\");\n> > if (!key_file)\n> > return error_errno(\n> > @@ -1036,11 +1038,14 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n> > }\n> >\n> > strvec_pushl(&signer.args, use_format->program,\n> > - \"-Y\", \"sign\",\n> > - \"-n\", \"git\",\n> > - \"-f\", ssh_signing_key_file,\n> > - buffer_file->filename.buf,\n> > - NULL);\n> > + \"-Y\", \"sign\",\n> > + \"-n\", \"git\",\n> > + \"-f\", ssh_signing_key_file,\n> > + NULL);\n>\n> Please avoid making a pointless indentation change like this.\n\nYep, removed. It was largely accidental.\n\n> We do\n> not pass filename yet with this pushl(), because ...\n>\n> > + if (literal_ssh_key) {\n> > + strvec_push(&signer.args, \"-U\");\n> > + }\n>\n> ... when we give a literal key, we want to insert \"-U\" in front, and then\n>\n> > + strvec_push(&signer.args, buffer_file->filename.buf);\n>\n> ... the filename. Which makes sense.\n\nI'm not sure what you mean in this paragraph. If there's something\nmore that needs to be done, I'd appreciate it if you could rephrase\nit.\n\n> The insertion of \"-U\" is a single statement as the body of a if()\n> statement. We do not want {} around it, by the way.\n\nRemoved the superfluous {}.\n\nThanks\n\n— Adam\n"},{"id":"471027","messageId":"xmqq4jsefsvy.fsf@gitster.g","threadId":"59112","inReplyTo":"CAEroKagUY5PfuC2CDn=pTJ=brPsjPy6MVz54mH1tvN8E-Pvk7g@mail.gmail.com","subject":"Re: [PATCH v2] ssh signing: better error message when key not in agent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-25T17:04:01Z","receivedAt":"2023-01-25T17:04:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Szkoda writes:\n\n> On Tue, Jan 24, 2023 at 6:52 PM Junio C Hamano wrote:\n>> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1270/radicle-dev/maint-v2\n>> > Pull-Request: https://github.com/git/git/pull/1270\n>> >\n>> > Range-diff vs v1:\n>> >\n>> > 1: 0ce06076242 < -: ----------- ssh signing: better error message when key not in agent\n>> > -: ----------- > 1: 03dfca79387 ssh signing: better error message when key not in agent\n>>\n>> This is a fairly useless range-diff.\n>>\n>> Even when a range-diff shows the differences in the patches,\n>> mechanically generated range-diff can only show _what_ changed. It\n>> is helpful to explain the changes in your own words to highlight\n>> _why_ such changes are done, and this place after the \"---\" line\n>> and the diffstat we see below is the place to do so.\n>>\n>> Does GitGitGadget allow its users to describe the differences since\n>> the previous iteration yourself?\n>\n> No, I don't think it does. It got generated automatically without\n> giving me an opportunity to edit.\n\nHmph. \n\nThe text after \"---\" and before \"Fetch-it-via:\" does look like\nsomething a human wrote. The part often is byte-for-byte identical\nduplicate of the proposed log message, but I think I have seen\npatches via GitGitGadget that have different text there, and was\nhoping perhaps the authors can use it to describe commentary for the\nrange-diff.\n\n> Yep, removed. It was largely accidental.\n> ...\n> Removed the superfluous {}.\n>\n> Thanks\n\nThanks. Looking good. Will queue and merge down to 'next'.\n\n"},{"id":"471029","messageId":"xmqqsffyedoh.fsf@gitster.g","threadId":"59112","inReplyTo":"CAEroKagUY5PfuC2CDn=pTJ=brPsjPy6MVz54mH1tvN8E-Pvk7g@mail.gmail.com","subject":"Re: [PATCH v2] ssh signing: better error message when key not in agent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-25T17:17:50Z","receivedAt":"2023-01-25T17:17:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Szkoda writes:\n\n>> We do\n>> not pass filename yet with this pushl(), because ...\n>>\n>> > + if (literal_ssh_key) {\n>> > + strvec_push(&signer.args, \"-U\");\n>> > + }\n>>\n>> ... when we give a literal key, we want to insert \"-U\" in front, and then\n>>\n>> > + strvec_push(&signer.args, buffer_file->filename.buf);\n>>\n>> ... the filename. Which makes sense.\n>\n> I'm not sure what you mean in this paragraph. If there's something\n> more that needs to be done, I'd appreciate it if you could rephrase\n> it.\n\n\"Which makes sense.\" is the key conclusion. Instead of saying \"This\npart of the patch looks good\" without explaining why I say so (it\ncould be that I am saying so without really reading or understanding\nthe changes or thinking the ramifications of the change through),\nthe earlier parts that lead to the conclusion is a way to give\nweight to the conclusion.\n\nIn other words, it is meant to show that the reviewer did read the\npatch well enough to understand the reasoning behind it.\n\nThanks.\n"},{"id":"471034","messageId":"CAPig+cTNk1RvfdFembKcxTOUs0UsiXmz8rsEnab-0fQp-QE3Lg@mail.gmail.com","threadId":"59112","inReplyTo":"CAEroKagUY5PfuC2CDn=pTJ=brPsjPy6MVz54mH1tvN8E-Pvk7g@mail.gmail.com","subject":"Re: [PATCH v2] ssh signing: better error message when key not in agent","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-01-25T21:42:26Z","receivedAt":"2023-01-25T21:42:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 25, 2023 at 8:05 AM Adam Szkoda wrote:\n> On Tue, Jan 24, 2023 at 6:52 PM Junio C Hamano wrote:\n> > > Range-diff vs v1:\n> > >\n> > > 1: 0ce06076242 < -: ----------- ssh signing: better error message when key not in agent\n> > > -: ----------- > 1: 03dfca79387 ssh signing: better error message when key not in agent\n> >\n> > This is a fairly useless range-diff.\n> >\n> > Even when a range-diff shows the differences in the patches,\n> > mechanically generated range-diff can only show _what_ changed. It\n> > is helpful to explain the changes in your own words to highlight\n> > _why_ such changes are done, and this place after the \"---\" line\n> > and the diffstat we see below is the place to do so.\n> >\n> > Does GitGitGadget allow its users to describe the differences since\n> > the previous iteration yourself?\n>\n> No, I don't think it does. It got generated automatically without\n> giving me an opportunity to edit.\n\nYes, the user can describe the differences since the previous\niteration by editing the pull-request's description. Specifically,\nwhen ready to send a new iteration:\n\n(1) force push the new iteration to the same branch on GitHub\n\n(2) edit the pull-request description; this is the very first\n\"comment\" on the pull-request page; press the \"...\" button on that\ncomment and choose the \"Edit\" menu item; revise the text to describe\nthe changes since the previous revision, then press the \"Update\ncomment\" button to save\n\n(3) post a \"/submit\" comment to the pull-request to tell GitGitGadget\nto send the new revision to the Git mailing list\n"},{"id":"471035","messageId":"xmqqedridzjw.fsf@gitster.g","threadId":"59112","inReplyTo":"CAPig+cTNk1RvfdFembKcxTOUs0UsiXmz8rsEnab-0fQp-QE3Lg@mail.gmail.com","subject":"Re: [PATCH v2] ssh signing: better error message when key not in agent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-25T22:22:59Z","receivedAt":"2023-01-25T22:23:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Wed, Jan 25, 2023 at 8:05 AM Adam Szkoda wrote:\n>> On Tue, Jan 24, 2023 at 6:52 PM Junio C Hamano wrote:\n>> > > Range-diff vs v1:\n>> > >\n>> > > 1: 0ce06076242 < -: ----------- ssh signing: better error message when key not in agent\n>> > > -: ----------- > 1: 03dfca79387 ssh signing: better error message when key not in agent\n>> >\n>> > This is a fairly useless range-diff.\n>> >\n>> > Even when a range-diff shows the differences in the patches,\n>> > mechanically generated range-diff can only show _what_ changed. It\n>> > is helpful to explain the changes in your own words to highlight\n>> > _why_ such changes are done, and this place after the \"---\" line\n>> > and the diffstat we see below is the place to do so.\n>> >\n>> > Does GitGitGadget allow its users to describe the differences since\n>> > the previous iteration yourself?\n>>\n>> No, I don't think it does. It got generated automatically without\n>> giving me an opportunity to edit.\n>\n> Yes, the user can describe the differences since the previous\n> iteration by editing the pull-request's description. Specifically,\n> when ready to send a new iteration:\n>\n> (1) force push the new iteration to the same branch on GitHub\n>\n> (2) edit the pull-request description; this is the very first\n> \"comment\" on the pull-request page; press the \"...\" button on that\n> comment and choose the \"Edit\" menu item; revise the text to describe\n> the changes since the previous revision, then press the \"Update\n> comment\" button to save\n>\n> (3) post a \"/submit\" comment to the pull-request to tell GitGitGadget\n> to send the new revision to the Git mailing list\n\nThanks. I thought the above would make a good addition to our\ndocumentation set. Documentation/MyFirstContribution.txt does have\nthis to say:\n\n Once you have your branch again in the shape you want following all review\n comments, you can submit again:\n\n ----\n $ git push -f remotename psuh\n ----\n\n Next, go look at your pull request against GitGitGadget; you should see the CI\n has been kicked off again. Now while the CI is running is a good time for you\n to modify your description at the top of the pull request thread; it will be\n used again as the cover letter. You should use this space to describe what\n has changed since your previous version, so that your reviewers have some idea\n of what they're looking at. When the CI is done running, you can comment once\n more with `/submit` - GitGitGadget will automatically add a v2 mark to your\n changes.\n\nbefore it talks about doing the \"/submit\" again. Expanding the\nabove into a bulletted list form like you did might make it easier\nto follow through, perhaps? I dunno.\n\n\n"},{"id":"472106","messageId":"CAPig+cSO1kMp_ck6q-2xRZth2LkfF5sw9rf0MijtkKjRhEoCQQ@mail.gmail.com","threadId":"59112","inReplyTo":"xmqqedridzjw.fsf@gitster.g","subject":"Re: [PATCH v2] ssh signing: better error message when key not in agent","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-02-15T01:22:15Z","receivedAt":"2023-02-15T01:23:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 25, 2023 at 5:23 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > On Wed, Jan 25, 2023 at 8:05 AM Adam Szkoda wrote:\n> >> On Tue, Jan 24, 2023 at 6:52 PM Junio C Hamano wrote:\n> >> > Does GitGitGadget allow its users to describe the differences since\n> >> > the previous iteration yourself?\n> >>\n> >> No, I don't think it does. It got generated automatically without\n> >> giving me an opportunity to edit.\n> >\n> > Yes, the user can describe the differences since the previous\n> > iteration by editing the pull-request's description. Specifically,\n> > when ready to send a new iteration:\n> >\n> > (1) force push the new iteration to the same branch on GitHub\n> >\n> > (2) edit the pull-request description; this is the very first\n> > \"comment\" on the pull-request page; press the \"...\" button on that\n> > comment and choose the \"Edit\" menu item; revise the text to describe\n> > the changes since the previous revision, then press the \"Update\n> > comment\" button to save\n> >\n> > (3) post a \"/submit\" comment to the pull-request to tell GitGitGadget\n> > to send the new revision to the Git mailing list\n>\n> Thanks. I thought the above would make a good addition to our\n> documentation set. Documentation/MyFirstContribution.txt does have\n> this to say:\n>\n> Next, go look at your pull request against GitGitGadget; you should see the CI\n> has been kicked off again. Now while the CI is running is a good time for you\n> to modify your description at the top of the pull request thread; it will be\n> used again as the cover letter. You should use this space to describe what\n> has changed since your previous version, so that your reviewers have some idea\n> of what they're looking at. When the CI is done running, you can comment once\n> more with `/submit` - GitGitGadget will automatically add a v2 mark to your\n> changes.\n>\n> before it talks about doing the \"/submit\" again. Expanding the\n> above into a bulletted list form like you did might make it easier\n> to follow through, perhaps? I dunno.\n\nPerhaps, though I wonder how many people consult MyFirstConstribution.\nMaybe SubmittingPatches would be a better location for such\ninstructions, although (I notice now that) that would be an even\nbigger change since SubmittingPatches doesn't mention GitGitGadget at\nall. Another appropriate place might be the \"welcome\" message that\nGitGitGadget posts the very first time someone submits a patch series\nvia that tool (assuming that the welcome message doesn't already\nexplain it).\n"}]}