{"thread":{"id":"56903","subject":"SSH Key Signatures: Feedback","startedAt":"2021-11-15T09:52:54Z","lastAt":"2021-11-15T10:31:33Z","messageCount":3,"participants":["Danilo","Fabian Stelzer"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"441124","messageId":"30999b1a-70b3-4489-b751-024c26bbbd11@www.fastmail.com","threadId":"56903","inReplyTo":null,"subject":"SSH Key Signatures: Feedback","fromName":"Danilo","fromEmail":"mail@dbrgn.ch","sentAt":"2021-11-15T09:52:24Z","receivedAt":"2021-11-15T09:52:54Z","isPatch":false,"sender":{"key":"mail@dbrgn.ch","avatar":null},"body":"Hello, and thanks for your work to support signing git commits/tags with SSH keys!\n\nI tries this feature with git version 2.34.0-rc2. Here's some feedback from my first use:\n\n- 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:\n\n    2,4c2,5\n    <            GPG-sign commits. The keyid argument is optional and defaults to the\n    <            committer identity; if specified, it must be stuck to the option without a\n    <            space.  --no-gpg-sign is useful to countermand both commit.gpgSign\n    ---\n    >            Sign commits with GPG or another method like SSH (see `gpg.format` config).\n    >            The keyid argument is optional and defaults to the committer identity; if\n    >            specified, it must be stuck to the option without a space.\n    >            --no-gpg-sign is useful to countermand both commit.gpgSign\n\n- When I tried to sign a commit with my SSH key, I got this error message:\n\n    $ git commit -S -m \"Release v${VERSION}\"\n    error: Load key \"/tmp/.git_signing_key_tmpvhKT9L\": invalid format?\n    \n    fatal: failed to write commit object\n\nThis 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\"?\n\n- 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?\n\n- 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\").\n\nThat's it from me, I hope this feedback can be useful!\nDanilo\n"},{"id":"441125","messageId":"20211115101945.scikeaptstuy64mi@fs","threadId":"56903","inReplyTo":"30999b1a-70b3-4489-b751-024c26bbbd11@www.fastmail.com","subject":"Re: SSH Key Signatures: Feedback","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-15T10:19:45Z","receivedAt":"2021-11-15T10:19:55Z","isPatch":false,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 15.11.2021 10:52, Danilo wrote:\n>Hello, and thanks for your work to support signing git commits/tags with SSH keys!\n>\n>I tries this feature with git version 2.34.0-rc2. Here's some feedback from my first use:\n>\n>- 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:\n>\n>    2,4c2,5\n>    <            GPG-sign commits. The keyid argument is optional and defaults to the\n>    <            committer identity; if specified, it must be stuck to the option without a\n>    <            space.  --no-gpg-sign is useful to countermand both commit.gpgSign\n>    ---\n>    >            Sign commits with GPG or another method like SSH (see `gpg.format` config).\n>    >            The keyid argument is optional and defaults to the committer identity; if\n>    >            specified, it must be stuck to the option without a space.\n>    >            --no-gpg-sign is useful to countermand both commit.gpgSign\n>\n>- When I tried to sign a commit with my SSH key, I got this error message:\n>\n>    $ git commit -S -m \"Release v${VERSION}\"\n>    error: Load key \"/tmp/.git_signing_key_tmpvhKT9L\": invalid format?\n>\n>    fatal: failed to write commit object\n>\n>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\"?\n>\n>- 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?\n>\n>- 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\").\n>\n>That's it from me, I hope this feedback can be useful!\n>Danilo\n\nHi Danilo,\nthank you for your feedback. I will search through the docs and see if I\ncan make improvements like the one you suggested. Unfortunately the flag\nthemselves are often named --gpg-sign / commit.gpgSign which we can't\nchange. We might add a new, more generically named flag & config as an\nalias to these in the future.\n\nRegarding the error messages I quite agree with you and had similar\nfeedback with our internal testers. These error messages (invalid format\n& unexpected internal error) originate from ssh-keygen. I already\nchecked if we can improve these but its not easy since those come from\nquite deep within ssh library code :/. I'll see what i can do, but since\nthis changes ssh-keygen behaviour I'm not sure how well received changes\nlike this would be.\n\nWhen you say `gpg.ssh.allowedSigners` is not set is the option not\npresent? or is it empty? The code should actually trigger\n`error(_(\"gpg.ssh.allowedSignersFile needs to be configured and exist\nfor ssh signature verification\"));` in this case.\n\nKind regards,\nFabian\n"},{"id":"441126","messageId":"9d264d41-7b24-40db-9c3e-28a8275db835@www.fastmail.com","threadId":"56903","inReplyTo":"20211115101945.scikeaptstuy64mi@fs","subject":"Re: SSH Key Signatures: Feedback","fromName":"Danilo","fromEmail":"mail@dbrgn.ch","sentAt":"2021-11-15T10:31:00Z","receivedAt":"2021-11-15T10:31:33Z","isPatch":false,"sender":{"key":"mail@dbrgn.ch","avatar":null},"body":"Hello Fabian\n\n> Unfortunately the flag\n> themselves are often named --gpg-sign / commit.gpgSign which we can't\n> change. We might add a new, more generically named flag & config as an\n> alias to these in the future.\n\nAliases sound like a good idea, this would allow for a slow phase-out of the old flags.\n\n> Regarding the error messages I quite agree with you and had similar\n> feedback with our internal testers. These error messages (invalid format\n> & unexpected internal error) originate from ssh-keygen. I already\n> checked if we can improve these but its not easy since those come from\n> quite deep within ssh library code :/. I'll see what i can do, but since\n> this changes ssh-keygen behaviour I'm not sure how well received changes\n> like this would be.\n\nI see. However, maybe some common cases (like missing or empty files) could be detected by git before even invoking the ssh-keygen command?\n\n> When you say `gpg.ssh.allowedSigners` is not set is the option not\n> present? or is it empty? The code should actually trigger\n> `error(_(\"gpg.ssh.allowedSignersFile needs to be configured and exist\n> for ssh signature verification\"));` in this case.\n\nThe 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\n\nAll other errors are part of the red error block, and are thus much more obvious to see.\n\nCheers,\nDanilo\n"}]}