SSH Key Signatures: Feedback

SSH Key Signatures: Feedback

[Danilo]

Hello, and thanks for your work to support signing git commits/tags with SSH keys!

I tries this feature with git version 2.34.0-rc2. Here's some feedback from my first use:

- To find out how this feature is used, I used "man git commit" and searched for "ssh". However, no result showed up. Maybe the manpage could be expanded to include a mention of other signing methods in the documentation for the "-S" command? For example, something like this:

    2,4c2,5
    <            GPG-sign commits. The keyid argument is optional and defaults to the
    <            committer identity; if specified, it must be stuck to the option without a
    <            space.  --no-gpg-sign is useful to countermand both commit.gpgSign
    ---
    >            Sign commits with GPG or another method like SSH (see `gpg.format` config).
    >            The keyid argument is optional and defaults to the committer identity; if
    >            specified, it must be stuck to the option without a space.
    >            --no-gpg-sign is useful to countermand both commit.gpgSign

- When I tried to sign a commit with my SSH key, I got this error message:

    $ git commit -S -m "Release v${VERSION}"
    error: Load key "/tmp/.git_signing_key_tmpvhKT9L": invalid format?
    
    fatal: failed to write commit object

This message was very confusing to me, because the SSH key format in "user.signingkey" was correct. In the end it turns out that I had loaded a few SSH keys into the key agent, but not this one. Could this situation be detected, to show a message like "No private key available for signing key X"?

- If `gpg.ssh.allowedSignersFile` is not set, `git log --show-signature` will show a "No signature" error message next to the commit. However, this isn't true, there is a signature but it cannot be verified. Maybe the error message should be updated to reflect this?

- If `gpg.ssh.allowedSignersFile` is set to an empty file, the error message in `git log --show-signature` includes "sig_find_principals: sshsig_find_principal: unexpected internal error^M". First of all, the message seems to include a stray "^M", and it could be updated to show the same output as when the file contains signers, but none of them matches ("No principal matched").

That's it from me, I hope this feedback can be useful!
Danilo

Re: SSH Key Signatures: Feedback

[Fabian Stelzer]

On 15.11.2021 10:52, Danilo wrote:
>Hello, and thanks for your work to support signing git commits/tags with SSH keys!
>
>I tries this feature with git version 2.34.0-rc2. Here's some feedback from my first use:
>
>- To find out how this feature is used, I used "man git commit" and searched for "ssh". However, no result showed up. Maybe the manpage could be expanded to include a mention of other signing methods in the documentation for the "-S" command? For example, something like this:
>
>    2,4c2,5
>    <            GPG-sign commits. The keyid argument is optional and defaults to the
>    <            committer identity; if specified, it must be stuck to the option without a
>    <            space.  --no-gpg-sign is useful to countermand both commit.gpgSign
>    ---
>    >            Sign commits with GPG or another method like SSH (see `gpg.format` config).
>    >            The keyid argument is optional and defaults to the committer identity; if
>    >            specified, it must be stuck to the option without a space.
>    >            --no-gpg-sign is useful to countermand both commit.gpgSign
>
>- When I tried to sign a commit with my SSH key, I got this error message:
>
>    $ git commit -S -m "Release v${VERSION}"
>    error: Load key "/tmp/.git_signing_key_tmpvhKT9L": invalid format?
>
>    fatal: failed to write commit object
>
>This message was very confusing to me, because the SSH key format in "user.signingkey" was correct. In the end it turns out that I had loaded a few SSH keys into the key agent, but not this one. Could this situation be detected, to show a message like "No private key available for signing key X"?
>
>- If `gpg.ssh.allowedSignersFile` is not set, `git log --show-signature` will show a "No signature" error message next to the commit. However, this isn't true, there is a signature but it cannot be verified. Maybe the error message should be updated to reflect this?
>
>- If `gpg.ssh.allowedSignersFile` is set to an empty file, the error message in `git log --show-signature` includes "sig_find_principals: sshsig_find_principal: unexpected internal error^M". First of all, the message seems to include a stray "^M", and it could be updated to show the same output as when the file contains signers, but none of them matches ("No principal matched").
>
>That's it from me, I hope this feedback can be useful!
>Danilo

Hi Danilo,
thank you for your feedback. I will search through the docs and see if I
can make improvements like the one you suggested. Unfortunately the flag
themselves are often named --gpg-sign / commit.gpgSign which we can't
change. We might add a new, more generically named flag & config as an
alias to these in the future.

Regarding the error messages I quite agree with you and had similar
feedback with our internal testers. These error messages (invalid format
& unexpected internal error) originate from ssh-keygen. I already
checked if we can improve these but its not easy since those come from
quite deep within ssh library code :/. I'll see what i can do, but since
this changes ssh-keygen behaviour I'm not sure how well received changes
like this would be.

When you say `gpg.ssh.allowedSigners` is not set is the option not
present? or is it empty? The code should actually trigger
`error(_("gpg.ssh.allowedSignersFile needs to be configured and exist
for ssh signature verification"));` in this case.

Kind regards,
Fabian

Re: SSH Key Signatures: Feedback

[Danilo]

Hello Fabian

> Unfortunately the flag
> themselves are often named --gpg-sign / commit.gpgSign which we can't
> change. We might add a new, more generically named flag & config as an
> alias to these in the future.

Aliases sound like a good idea, this would allow for a slow phase-out of the old flags.

> Regarding the error messages I quite agree with you and had similar
> feedback with our internal testers. These error messages (invalid format
> & unexpected internal error) originate from ssh-keygen. I already
> checked if we can improve these but its not easy since those come from
> quite deep within ssh library code :/. I'll see what i can do, but since
> this changes ssh-keygen behaviour I'm not sure how well received changes
> like this would be.

I see. However, maybe some common cases (like missing or empty files) could be detected by git before even invoking the ssh-keygen command?

> When you say `gpg.ssh.allowedSigners` is not set is the option not
> present? or is it empty? The code should actually trigger
> `error(_("gpg.ssh.allowedSignersFile needs to be configured and exist
> for ssh signature verification"));` in this case.

The option `gpg.ssh.allowedSigners` is not present at all. I just checked again, and noticed that the error message you mentioned *is* there, however it's not part of the red error block, so I overlooked it: https://i.imgur.com/LLvrrxO.png

All other errors are part of the red error block, and are thus much more obvious to see.

Cheers,
Danilo