git
ssh signing: better error message when key not in agent
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
Thanks for taking the time to contribute to Git! Please be advised that the Git community does not use github.com for their contributions. Instead, we use a mailing list ([email protected]) for code submissions, code reviews, and bug reports. Nevertheless, you can use GitGitGadget (https://gitgitgadget.github.io/) to conveniently send your Pull Requests commits to our mailing list.
Please read the "guidelines for contributing" linked above!
Welcome to GitGitGadget
Hi @adaszko, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.
Please make sure that your Pull Request has a good description, as it will be used as cover letter. You can CC potential reviewers by adding a footer to the PR description with the following syntax:
CC: Revi Ewer <[email protected]>, Ill Takalook <[email protected]>
Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
- the lines should not exceed 76 columns,
- the first line should be like a header and typically start with a prefix like "tests:" or "revisions:" to state which subsystem the change is about, and
- the commit messages' body should be describing the "why?" of the change.
- Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.
Contributing the patches
Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:
Both the person who commented /allow and the PR author are able to /allow you.
An alternative is the channel #git-devel on the Libera Chat IRC network:
<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!
Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.
If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this. Note that any reviewers CC'd via the list in the PR description will not actually be sent emails.
After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).
If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:
curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt
To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):
Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...
To send a new iteration, just add another PR comment with the contents: /submit.
Need help?
New contributors who want advice are encouraged to join [email protected], where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.
You may also be able to find help in real time in the developer IRC channel, #git-devel on Libera Chat. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.
![gitgitgadget-git[bot] avatar](https://avatars.githubusercontent.com/in/46807?v=4 "Published by a pub.dev verified publisher"){kind=small}
There are issues in commit 31bbfde9307c0d6e39777846c9579c13119936af:
ssh signing: better error message when key not in agent
Commit not signed off
Lines in the body of the commit messages should be wrapped between 60 and 76 characters.
There are issues in commit c33e553f63a389e9ec7125e719f199a085e0343c:
ssh signing: better error message when key not in agent
Lines in the body of the commit messages should be wrapped between 60 and 76 characters.
User adaszko is now allowed to use GitGitGadget.
WARNING: adaszko has no public email address set on GitHub; GitGitGadget needs an email address to Cc: you on your contribution, so that you receive any feedback on the Git mailing list. Go to https://github.com/settings/profile to make your preferred email public to let GitGitGadget know which email address to use.
My email address should now be public and the CI failures in this PR are unrelated to my changes. Is there anything else I need to do?
Hi. The patch on the OpenSSH side has been released in OpenSSH 9.1:
- ssh-keygen(1): improve error message when 'ssh-keygen -Y sign' is unable to load a private key; bz3429
- ssh-keygen(1): allow the existing -U (use agent) flag to work with "-Y sign" operations, where it will be interpreted to require that the private keys is hosted in an agent; bz3429
Can this PR be scheduled to be merged in the next git release?
Submitted as [email protected]
To fetch this version into FETCH_HEAD:
git fetch https://github.com/gitgitgadget/git/ pr-git-1270/radicle-dev/maint-v1
To fetch this version to local tag pr-git-1270/radicle-dev/maint-v1:
git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-1270/radicle-dev/maint-v1
On the Git mailing list, Phillip Wood wrote (reply to this):
Hi Adam
On 18/01/2023 08:17, Adam Szkoda via GitGitGadget wrote:
> From: Adam Szkoda <[email protected]>
> > 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.
The documentation for user.signingKey says
If gpg.format is set to ssh this can contain the path to either your private ssh key or the public key when ssh-agent is used.
If I've understood correctly passing -U will prevent users from setting this to a private key.
Best Wishes
Phillip
> 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 <[email protected]>
> ---
> ssh signing: better error message when key not in agent
> > 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
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1270%2Fradicle-dev%2Fmaint-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1270/radicle-dev/maint-v1
> Pull-Request: https://github.com/git/git/pull/1270
> > gpg-interface.c | 1 +
> 1 file changed, 1 insertion(+)
> > diff --git a/gpg-interface.c b/gpg-interface.c
> index 280f1fa1a58..4a5913ae942 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -1022,6 +1022,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
> strvec_pushl(&signer.args, use_format->program,
> "-Y", "sign",
> "-n", "git",
> + "-U",
> "-f", ssh_signing_key_file,
> buffer_file->filename.buf,
> NULL);
> > base-commit: e54793a95afeea1e10de1e5ad7eab914e7416250
On the Git mailing list, Phillip Wood wrote (reply to this):
On 18/01/2023 11:10, Phillip Wood wrote:
>> 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.
> > The documentation for user.signingKey says
> > If gpg.format is set to ssh this can contain the path to either your > private ssh key or the public key when ssh-agent is used.
> > If I've understood correctly passing -U will prevent users from setting > this to a private key.
If there is an easy way to tell if the user has given us a public key then we could pass "-U" in that case.
Best Wishes
Phillip
On the Git mailing list, Adam Szkoda wrote (reply to this):
Hi Phillip,
Good point! My first thought is to try doing a stat() syscall on the
path from 'user.signingKey' to see if it exists and if not, treat it
as a public key (and pass the -U option). If that sounds reasonable,
I can update the patch.
Best
— Adam
On Wed, Jan 18, 2023 at 3:34 PM Phillip Wood <[email protected]> wrote:
>
> On 18/01/2023 11:10, Phillip Wood wrote:
> >> 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.
> >
> > The documentation for user.signingKey says
> >
> > If gpg.format is set to ssh this can contain the path to either your
> > private ssh key or the public key when ssh-agent is used.
> >
> > If I've understood correctly passing -U will prevent users from setting
> > this to a private key.
>
> If there is an easy way to tell if the user has given us a public key
> then we could pass "-U" in that case.
>
> Best Wishes
>
> Phillip
On the Git mailing list, Phillip Wood wrote (reply to this):
Hi Adam
I've cc'd Fabian who knows more about the ssh signing code that I do.
On 18/01/2023 15:28, Adam Szkoda wrote:
> Hi Phillip,
> > Good point! My first thought is to try doing a stat() syscall on the
> path from 'user.signingKey' to see if it exists and if not, treat it
> as a public key (and pass the -U option). If that sounds reasonable,
> I can update the patch.
My reading of the documentation is that user.signingKey may point to a public or private key so I'm not sure how stat()ing would help. Looking at the code in sign_buffer_ssh() we have a function is_literal_ssh_key() that checks if the config value is a public key. When the user passes the path to a key we could read the file check use is_literal_ssh_key() to check if it is a public key (or possibly just check if the file begins with "ssh-"). Fabian - does that sound reasonable?
Best Wishes
Phillip
> Best
> — Adam
> > > On Wed, Jan 18, 2023 at 3:34 PM Phillip Wood <[email protected]> wrote:
>>
>> On 18/01/2023 11:10, Phillip Wood wrote:
>>>> 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.
>>>
>>> The documentation for user.signingKey says
>>>
>>> If gpg.format is set to ssh this can contain the path to either your
>>> private ssh key or the public key when ssh-agent is used.
>>>
>>> If I've understood correctly passing -U will prevent users from setting
>>> this to a private key.
>>
>> If there is an easy way to tell if the user has given us a public key
>> then we could pass "-U" in that case.
>>
>> Best Wishes
>>
>> Phillip
On the Git mailing list, Fabian Stelzer wrote (reply to this):
On 18.01.2023 16:29, Phillip Wood wrote:
>Hi Adam
>
>I've cc'd Fabian who knows more about the ssh signing code that I do.
>
>On 18/01/2023 15:28, Adam Szkoda wrote:
>>Hi Phillip,
>>
>>Good point! My first thought is to try doing a stat() syscall on the
>>path from 'user.signingKey' to see if it exists and if not, treat it
>>as a public key (and pass the -U option). If that sounds reasonable,
>>I can update the patch.
>
>My reading of the documentation is that user.signingKey may point to a >public or private key so I'm not sure how stat()ing would help. >Looking at the code in sign_buffer_ssh() we have a function >is_literal_ssh_key() that checks if the config value is a public key. >When the user passes the path to a key we could read the file check >use is_literal_ssh_key() to check if it is a public key (or possibly >just check if the file begins with "ssh-"). Fabian - does that sound >reasonable?
Hi,
I have encountered the mentioned problem before as well and tried to fix it but did not find a good / reasonable way to do so. Git just passes the user.signingKey to ssh-keygen which states:
`The key used for signing is specified using the -f option and may refer to either a private key, or a public key with the private half available via ssh-agent(1)`
I don't think it's a good idea for git to parse the key and try to determine if it's public or private. The fix should probably be in openssh (different error message) but when looking into it last time i remember that the logic for using the key is quite deeply embedded into the ssh code and not easily adjusted for the signing use case. At the moment I don't have the time to look into it but the openssh code for signing is quite readable so feel free to give it a try. Maybe you find a good way.
Best regards,
Fabian
>
>Best Wishes
>
>Phillip
>
>>Best
>>— Adam
>>
>>
>>On Wed, Jan 18, 2023 at 3:34 PM Phillip Wood <[email protected]> wrote:
>>>
>>>On 18/01/2023 11:10, Phillip Wood wrote:
>>>>>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.
>>>>
>>>>The documentation for user.signingKey says
>>>>
>>>> If gpg.format is set to ssh this can contain the path to either your
>>>>private ssh key or the public key when ssh-agent is used.
>>>>
>>>>If I've understood correctly passing -U will prevent users from setting
>>>>this to a private key.
>>>
>>>If there is an easy way to tell if the user has given us a public key
>>>then we could pass "-U" in that case.
>>>
>>>Best Wishes
>>>
>>>Phillip
On the Git mailing list, Phillip Wood wrote (reply to this):
On 20/01/2023 09:03, Fabian Stelzer wrote:
> On 18.01.2023 16:29, Phillip Wood wrote:
>> Hi Adam
>>
>> I've cc'd Fabian who knows more about the ssh signing code that I do.
>>
>> On 18/01/2023 15:28, Adam Szkoda wrote:
>>> Hi Phillip,
>>>
>>> Good point! My first thought is to try doing a stat() syscall on the
>>> path from 'user.signingKey' to see if it exists and if not, treat it
>>> as a public key (and pass the -U option). If that sounds reasonable,
>>> I can update the patch.
>>
>> My reading of the documentation is that user.signingKey may point to a >> public or private key so I'm not sure how stat()ing would help. >> Looking at the code in sign_buffer_ssh() we have a function >> is_literal_ssh_key() that checks if the config value is a public key. >> When the user passes the path to a key we could read the file check >> use is_literal_ssh_key() to check if it is a public key (or possibly >> just check if the file begins with "ssh-"). Fabian - does that sound >> reasonable?
> > Hi,
> I have encountered the mentioned problem before as well and tried to fix > it but did not find a good / reasonable way to do so. Git just passes > the user.signingKey to ssh-keygen which states:
> `The key used for signing is specified using the -f option and may refer > to either a private key, or a public key with the private half available > via ssh-agent(1)`
> > I don't think it's a good idea for git to parse the key and try to > determine if it's public or private. The fix should probably be in > openssh (different error message) but when looking into it last time i > remember that the logic for using the key is quite deeply embedded into > the ssh code and not easily adjusted for the signing use case. At the > moment I don't have the time to look into it but the openssh code for > signing is quite readable so feel free to give it a try. Maybe you find > a good way.
Thanks Fabian, perhaps the easiest way forward is for us to only pass "-U" when we have a literal key in user.signingKey as we know it must a be public key in that case.
Best Wishes
Phillip
> Best regards,
> Fabian
> >>
>> Best Wishes
>>
>> Phillip
>>
>>> Best
>>> — Adam
>>>
>>>
>>> On Wed, Jan 18, 2023 at 3:34 PM Phillip Wood >>> <[email protected]> wrote:
>>>>
>>>> On 18/01/2023 11:10, Phillip Wood wrote:
>>>>>> 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.
>>>>>
>>>>> The documentation for user.signingKey says
>>>>>
>>>>> If gpg.format is set to ssh this can contain the path to either your
>>>>> private ssh key or the public key when ssh-agent is used.
>>>>>
>>>>> If I've understood correctly passing -U will prevent users from >>>>> setting
>>>>> this to a private key.
>>>>
>>>> If there is an easy way to tell if the user has given us a public key
>>>> then we could pass "-U" in that case.
>>>>
>>>> Best Wishes
>>>>
>>>> Phillip
On the Git mailing list, Fabian Stelzer wrote (reply to this):
On 23.01.2023 09:33, Phillip Wood wrote:
>On 20/01/2023 09:03, Fabian Stelzer wrote:
>>On 18.01.2023 16:29, Phillip Wood wrote:
>>>Hi Adam
>>>
>>>I've cc'd Fabian who knows more about the ssh signing code that I do.
>>>
>>>On 18/01/2023 15:28, Adam Szkoda wrote:
>>>>Hi Phillip,
>>>>
>>>>Good point! My first thought is to try doing a stat() syscall on the
>>>>path from 'user.signingKey' to see if it exists and if not, treat it
>>>>as a public key (and pass the -U option). If that sounds reasonable,
>>>>I can update the patch.
>>>
>>>My reading of the documentation is that user.signingKey may point >>>to a public or private key so I'm not sure how stat()ing would >>>help. Looking at the code in sign_buffer_ssh() we have a function >>>is_literal_ssh_key() that checks if the config value is a public >>>key. When the user passes the path to a key we could read the file >>>check use is_literal_ssh_key() to check if it is a public key (or >>>possibly just check if the file begins with "ssh-"). Fabian - does >>>that sound reasonable?
>>
>>Hi,
>>I have encountered the mentioned problem before as well and tried to >>fix it but did not find a good / reasonable way to do so. Git just >>passes the user.signingKey to ssh-keygen which states:
>>`The key used for signing is specified using the -f option and may >>refer to either a private key, or a public key with the private half >>available via ssh-agent(1)`
>>
>>I don't think it's a good idea for git to parse the key and try to >>determine if it's public or private. The fix should probably be in >>openssh (different error message) but when looking into it last time >>i remember that the logic for using the key is quite deeply embedded >>into the ssh code and not easily adjusted for the signing use case. >>At the moment I don't have the time to look into it but the openssh >>code for signing is quite readable so feel free to give it a try. >>Maybe you find a good way.
>
>Thanks Fabian, perhaps the easiest way forward is for us to only pass >"-U" when we have a literal key in user.signingKey as we know it must >a be public key in that case.
Yes, i think that's a good idea as long as the `-U` flag is ignored in older ssh versions and shouldn't be too hard to implement. And it should work just as well when using `defaultKeyCommand`.
Best,
Fabian
>
>Best Wishes
>
>Phillip
>
>>Best regards,
>>Fabian
>>
>>>
>>>Best Wishes
>>>
>>>Phillip
>>>
>>>>Best
>>>>— Adam
>>>>
>>>>
>>>>On Wed, Jan 18, 2023 at 3:34 PM Phillip Wood >>>><[email protected]> wrote:
>>>>>
>>>>>On 18/01/2023 11:10, Phillip Wood wrote:
>>>>>>>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.
>>>>>>
>>>>>>The documentation for user.signingKey says
>>>>>>
>>>>>> If gpg.format is set to ssh this can contain the path to either your
>>>>>>private ssh key or the public key when ssh-agent is used.
>>>>>>
>>>>>>If I've understood correctly passing -U will prevent users >>>>>>from setting
>>>>>>this to a private key.
>>>>>
>>>>>If there is an easy way to tell if the user has given us a public key
>>>>>then we could pass "-U" in that case.
>>>>>
>>>>>Best Wishes
>>>>>
>>>>>Phillip
The pull request has 297 commits. The max allowed is 30. Please split the patch series into multiple pull requests. Also consider squashing related commits.
There are merge commits in this Pull Request:
a61c70a7c890f48678bab8964f9f8d2173fb414a
4948ed473168a36d1ca03765c599e95258a29715
395bec6b3930ce3e23ef90bfb01be4922b21259e
Please rebase the branch and force-push.
On the Git mailing list, Adam Szkoda wrote (reply to this):
Hi! I've pushed a patch that adds `-U` conditional on is_literal_ssh_key().
According to the OpenSSH issue ([1]), that option is backward compatible:
> 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.
[1]: https://bugzilla.mindrot.org/show_bug.cgi?id=3429
Cheers
— Adam
On Mon, Jan 23, 2023 at 11:02 AM Fabian Stelzer <[email protected]> wrote:
>
> On 23.01.2023 09:33, Phillip Wood wrote:
> >On 20/01/2023 09:03, Fabian Stelzer wrote:
> >>On 18.01.2023 16:29, Phillip Wood wrote:
> >>>Hi Adam
> >>>
> >>>I've cc'd Fabian who knows more about the ssh signing code that I do.
> >>>
> >>>On 18/01/2023 15:28, Adam Szkoda wrote:
> >>>>Hi Phillip,
> >>>>
> >>>>Good point! My first thought is to try doing a stat() syscall on the
> >>>>path from 'user.signingKey' to see if it exists and if not, treat it
> >>>>as a public key (and pass the -U option). If that sounds reasonable,
> >>>>I can update the patch.
> >>>
> >>>My reading of the documentation is that user.signingKey may point
> >>>to a public or private key so I'm not sure how stat()ing would
> >>>help. Looking at the code in sign_buffer_ssh() we have a function
> >>>is_literal_ssh_key() that checks if the config value is a public
> >>>key. When the user passes the path to a key we could read the file
> >>>check use is_literal_ssh_key() to check if it is a public key (or
> >>>possibly just check if the file begins with "ssh-"). Fabian - does
> >>>that sound reasonable?
> >>
> >>Hi,
> >>I have encountered the mentioned problem before as well and tried to
> >>fix it but did not find a good / reasonable way to do so. Git just
> >>passes the user.signingKey to ssh-keygen which states:
> >>`The key used for signing is specified using the -f option and may
> >>refer to either a private key, or a public key with the private half
> >>available via ssh-agent(1)`
> >>
> >>I don't think it's a good idea for git to parse the key and try to
> >>determine if it's public or private. The fix should probably be in
> >>openssh (different error message) but when looking into it last time
> >>i remember that the logic for using the key is quite deeply embedded
> >>into the ssh code and not easily adjusted for the signing use case.
> >>At the moment I don't have the time to look into it but the openssh
> >>code for signing is quite readable so feel free to give it a try.
> >>Maybe you find a good way.
> >
> >Thanks Fabian, perhaps the easiest way forward is for us to only pass
> >"-U" when we have a literal key in user.signingKey as we know it must
> >a be public key in that case.
>
> Yes, i think that's a good idea as long as the `-U` flag is ignored in older
> ssh versions and shouldn't be too hard to implement. And it should work just
> as well when using `defaultKeyCommand`.
>
> Best,
> Fabian
>
> >
> >Best Wishes
> >
> >Phillip
> >
> >>Best regards,
> >>Fabian
> >>
> >>>
> >>>Best Wishes
> >>>
> >>>Phillip
> >>>
> >>>>Best
> >>>>— Adam
> >>>>
> >>>>
> >>>>On Wed, Jan 18, 2023 at 3:34 PM Phillip Wood
> >>>><[email protected]> wrote:
> >>>>>
> >>>>>On 18/01/2023 11:10, Phillip Wood wrote:
> >>>>>>>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.
> >>>>>>
> >>>>>>The documentation for user.signingKey says
> >>>>>>
> >>>>>> If gpg.format is set to ssh this can contain the path to either your
> >>>>>>private ssh key or the public key when ssh-agent is used.
> >>>>>>
> >>>>>>If I've understood correctly passing -U will prevent users
> >>>>>>from setting
> >>>>>>this to a private key.
> >>>>>
> >>>>>If there is an easy way to tell if the user has given us a public key
> >>>>>then we could pass "-U" in that case.
> >>>>>
> >>>>>Best Wishes
> >>>>>
> >>>>>Phillip
This is looking good, let's submit this to the list so it can be reviewed there and hopefully merged soon.
Submitted as [email protected]
To fetch this version into FETCH_HEAD:
git fetch https://github.com/gitgitgadget/git/ pr-git-1270/radicle-dev/maint-v2
To fetch this version to local tag pr-git-1270/radicle-dev/maint-v2:
git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-1270/radicle-dev/maint-v2
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Adam Szkoda via GitGitGadget" <[email protected]> writes:
> From: Adam Szkoda <[email protected]>
>
> 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 <[email protected]>
> ---
Well explained.
> 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?
> 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