| Message ID | pull.1270.git.git.1674029874363.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ssh signing: better error message when key not in agent | expand |
Hi Adam On 18/01/2023 08:17, Adam Szkoda via GitGitGadget wrote: > From: Adam Szkoda <adaszko@gmail.com> > > When signing a commit with a SSH key, with the private key missing from > ssh-agent, a confusing error message is produced: > > error: Load key > "/var/folders/t5/cscwwl_n3n1_8_5j_00x_3t40000gn/T//.git_signing_key_tmpkArSj7": > invalid format? fatal: failed to write commit object > > The temporary file .git_signing_key_tmpkArSj7 created by git contains a > valid *public* key. The error message comes from `ssh-keygen -Y sign' and > is caused by a fallback mechanism in ssh-keygen whereby it tries to > interpret .git_signing_key_tmpkArSj7 as a *private* key if it can't find in > the agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All that > needs to be done is to pass an additional backward-compatible option -U to > 'ssh-keygen -Y sign' call. With '-U', ssh-keygen always interprets the file > as public key and expects to find the private key in the agent. 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 <adaszko@gmail.com> > --- > 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 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
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 <phillip.wood123@gmail.com> 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
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 <phillip.wood123@gmail.com> 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 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 <phillip.wood123@gmail.com> 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 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 >>> <phillip.wood123@gmail.com> 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 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 >>>><phillip.wood123@gmail.com> 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
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 <fs@gigacodes.de> 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 > >>>><phillip.wood123@gmail.com> 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
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);