Sign a commit if ssh secret key is provided #2047

yanganto · 2024-02-13T09:07:57Z

This Pull Request fixes #1149

It changes the following:

A ssh key path in user.signingKey of gitconfig will be used to sign the commit

I followed the checklist:

I added unittests
I ran make check without errors
I tested the overall application
I added an appropriate item to the changelog

extrawurst · 2024-02-13T09:48:30Z

based on your last comment in the previous PR:

Just hard to find another crate with the same function but using syn2. This is the same issue as the previous point.

still cannot follow what you refer to here. gitui now only depends on syn2, there is no old syn1 in the dependencies anymore.

what I am asking is: what is needed to make CI not fail?

I think we need some slightly simpler UX for providing this. Is there a standard way how this key is provided for example to git (the normal cli)? maybe env variables? or is it also purely via cli args?

I will happy to use GITUI_SSH_KEY_PATH and GITUI_SSH_KEY_PASSWORD for the first iteration and moving forward from this.

I think we have some misunderstanding here. I am not saying lets introduce a bespoke env var that is gitui specific instead of a cli arg. My question is: how do other tools get fed the SSH key/pwd? Primarily the regular git cli and why can't we use the same way?

yanganto · 2024-02-13T12:02:15Z

based on your last comment in the previous PR:

Just hard to find another crate with the same function but using syn2. This is the same issue as the previous point.

still cannot follow what you refer to here. gitui now only depends on syn2, there is no old syn1 in the dependencies anymore.

what I am asking is: what is needed to make CI not fail?

https://github.com/extrawurst/gitui/actions/runs/7794027383/job/21254692240
Yes, it was an issue in previous PR, but it is okay now.

I think we need some slightly simpler UX for providing this. Is there a standard way how this key is provided for example to git (the normal cli)? maybe env variables? or is it also purely via cli args?

I will happy to use GITUI_SSH_KEY_PATH and GITUI_SSH_KEY_PASSWORD for the first iteration and moving forward from this.

I think we have some misunderstanding here. I am not saying lets introduce a bespoke env var that is gitui specific instead of a cli arg. My question is: how do other tools get fed the SSH key/pwd? Primarily the regular git cli and why can't we use the same way?

If the cli is good, I will pick these back and drop the env::var here.

The best practice for using an ssh password is to ask interactively with UI, such that the user can avoid writing down the password in a file or env. And the default ssh key will be ~/.ssh/id_rsa or ~/.ssh/id_ed25519, which will depend on the way the user generates his key. I think for the first iteration it is good to have a password here. If you think this is bad, I can remove the password feature, because password is not a requirement for ssh key.

extrawurst · 2024-02-13T12:07:57Z

And the default ssh key will be ~/.ssh/id_rsa or ~/.ssh/id_ed25519, which will depend on the way the user generates his key. I think for the first iteration it is good to have a password here. If you think this is bad, I can remove the password feature, because password is not a requirement for ssh key.

Ok lets make things easy then and scan for the default keys, no arg or env for now.
Is there a git config that tells us that an ssh key is supposed to be used so we can show an error if we are supposed to use ssh and cannot find a key?

also if we know we shall encrypt via ssh and we find a default key do we know if that key requires a pwd? so that we can fail for the MVP in that case because we don't know the pwd

extrawurst · 2024-02-13T12:08:17Z

@yanganto but mostly we need to get the CI green

yanganto · 2024-02-13T12:18:41Z

And the default ssh key will be ~/.ssh/id_rsa or ~/.ssh/id_ed25519, which will depend on the way the user generates his key. I think for the first iteration it is good to have a password here. If you think this is bad, I can remove the password feature, because password is not a requirement for ssh key.

Ok lets make things easy then and scan for the default keys, no arg or env for now. Is there a git config that tells us that an ssh key is supposed to be used so we can show an error if we are supposed to use ssh and cannot find a key?

You misunderstand the mechanism. The commit can be signed with any ssh secret key, but only one. Github will use verify the commit only if the corresponding pub key is already set in Github by user. So we can not scan all the ssh key to select some one who is valid, because all key are valid and possible in use. The default key path will be different because user can use different kinds of crypto mechanism. We can have a default value as now, but we need a interface let user change it easier.

also if we know we shall encrypt via ssh and we find a default key do we know if that key requires a pwd? so that we can fail for the MVP in that case because we don't know the pwd

No, if we do not have a password, we still can sign the commit but it will not verified by GITHUB, because the bytes will not be different from a real one.

extrawurst · 2024-02-13T12:21:02Z

You misunderstand the mechanism

then lets go back to square one: how does it work with the git-cli, how does it get configured and why cant we use the same method?

extrawurst · 2024-02-13T12:24:34Z

as far as i can see it has to be configured like this: https://docs.github.com/en/authentication/managing-commit-signature-verification/telling-git-about-your-signing-key#telling-git-about-your-ssh-key

so we can just use these git configs to know what signing key to use and whether signing is supposed to happen: so lets use that

yanganto · 2024-02-13T12:32:38Z

git commit use following option to sign a commit with GPG
git-commit -S[<keyid>], --gpg-sign[=<keyid>]

And the ssh sign is using ssh secret key but the header still gpgsig, and GITHUB or other repository will using the pub key.

There is not exactly an interface in git-cli for the ssh key. The way we are using now is -s. I think it is good enough now.

yanganto · 2024-02-13T12:46:38Z

as far as i can see it has to be configured like this: https://docs.github.com/en/authentication/managing-commit-signature-verification/telling-git-about-your-signing-key#telling-git-about-your-ssh-key

so we can just use these git configs to know what signing key to use and whether signing is supposed to happen: so lets use that

You can see the config is a pub key for verification not a secret key for a sign. Do we have an assumption that there is a secret key and the key is the pub key path remove .pub?

yanganto · 2024-02-13T13:03:26Z

By the way, the way to set git config with gpg.format ssh will not work as what you see on the web page.

└─[$] <git:(ssh*)> git commit -m 'test ssh with git'
error: unsupported value for gpg.format: ssh
fatal: bad config variable 'gpg.format' in file '.git/config' at line 28
└─[$] <git:(ssh*)> git --version
git version 2.32.0

So I think nobody write config in that way for now

yanganto · 2024-02-13T15:26:23Z

You may wonder why we need Clone trait on CliArgs
Because clippy will cry on passing too many fields of CliArgs in run_app function.
https://github.com/extrawurst/gitui/actions/runs/7888377158/job/21525757826

yanganto · 2024-02-13T15:27:06Z

@extrawurst It now is ready to review

extrawurst · 2024-02-13T17:50:58Z

You can see the config is a pub key for verification not a secret key for a sign.

this is exactly what every source I find about ssh-commit-signing tells you to do. to sign with your public key. what am I missing?

but don't take my word for it, see Gitlab docs, tower (slightly popular git GUI app) and this high ranking blog post here

yanganto · 2024-02-14T13:33:56Z

You can see the config is a pub key for verification not a secret key for a sign.

this is exactly what every source I find about ssh-commit-signing tells you to do. to sign with your public key. what am I missing?

but don't take my word for it, see Gitlab docs, tower (slightly popular git GUI app) and this high ranking blog post here

A good sign mechanism is to use a secret key and pass the public key to verify whether it is true or not. If you sign with a public key, then you need to pass the secret key to Github for verification. 🙄
The commit in the current PR is signed by my SSH secret key with this code, and I set the public key on GitHub, so you can see there are green verified flags.

The blog post here is signed with a secret key, but he put the content of key in the signingKey, That is the same as what I did in this PR.

The ssh setting will need a git after 2.34. The legacy git will be broken when using it, so people may not set up the gitconfig with ssh. (I already use gitui to sign the commit, why do I need to update the git to the specific version?)
Also, there is a lot of possible value in signingKey, the path to the pub key, the path to the secret key, the content of the SSH secret key, or possibly some content of gpg keys.

Does current gitui handle a global git config and overlayed with a local git config? I don't think the first iteration needs to handle these variations.

extrawurst · 2024-02-15T10:55:15Z

The blog post here is signed with a secret key, but he put the content of key in the signingKey, That is the same as what I did in this PR.

It literally says in the link above Simply use your public key

The ssh setting will need a git after 2.34.

Fine with me.

Lets use signingKey config no cli param for now. if user wants to use ssh singing signingKey and gpg.format ssh have to be configured.

Does current gitui handle a global git config and overlayed with a local git config

yes git2-rs (libgit2) make that pretty easy

yanganto · 2024-03-04T15:59:15Z

What is the thing in your allowedSignersFile? Did it auto-append something there?
The allowedSignersFile is just like we put the public key to the GITHUB if we don't do that GITHUB will not flag verified on the commit. Did 86aba81 really work on GITHUB after you pushed it?

thePanz · 2024-03-04T16:01:47Z

What is the thing in your allowedSignersFile? Did it auto-append something there?

in my .gitconfig I have:

[gpg "ssh"]
        allowedSignersFile = ~/.ssh/allowed_signers

Is there anything I should check for you?

yanganto · 2024-03-04T16:03:39Z

What is the content of ~/.ssh/allowed_signers? This is the thing you should provide to GITHUB, else it not really work on the internet I think.

patka-123 · 2024-03-04T16:06:57Z

I'm fairly certain allowed_signers shouldn't need to exist. Other programs don't rely on it and can still properly sign commits.

vscode nor any jetbrains product needs it in order to sign a commit.

yanganto · 2024-03-04T16:10:28Z

I'm fairly certain allowed_signers shouldn't need to exist. Other programs don't rely on it and can still properly sign commits.

Is the sign an ssh sign or gpg sign?

vscode nor any jetbrains product needs it in order to sign a commit.

We don't need allowed_signers to sign a commit, but it is for display.

thePanz · 2024-03-04T16:18:00Z

I am using ssh signing

yanganto · 2024-03-04T16:19:22Z

What is the content of ~/.ssh/allowed_signers from your end? @thePanz
Does the commit work after being pushed to remote?
I think the sign should work remotely and it is much more important than on the local end.

thePanz · 2024-03-04T16:58:50Z

I guess the contents of the allowed_signers is not relevant to this issue, as I show in the screenshot: when the signature is created by the git CLI, it works as expected, and the commit is properly shown as signed.

yanganto · 2024-03-04T17:31:14Z

The important problem is if set up correctly, we can see the verified tag on the GITHUB web page.
That is why we need to sign a commit and show the tag to the public, do you agree?

Did the commit(86aba81) work well on the remote side when you pushed?
I guess it wasn't.
If not, you are showing something not meaningful and want me to follow a bug.

If the allowed_signers is not important why do we set it up in the test?
Do you agree we should only provide the public key to GITHUB?
I guess the allowed_signers was not set up with public keys on your end?

Did this PR correctly sign and show verified when you gave your public key to GITHUB?
You can go here to add your ssh key.

supleed2 · 2024-03-04T17:35:06Z

I'm glad to see the work done so far! Just wanted to add a datapoint.

Compiled from this commit: yanganto@43bea21, I ran git init, created a file and added a remote, then started gitui, committed and pushed giving this signed commit on GitHub: https://github.com/supleed2/gitui-test/commit/62429dafbed4448b7a79688cf48b99072e9e2eff

Seems like it works fine?

thePanz · 2024-03-05T08:23:23Z

I will test it again @yanganto , I forked @supleed2 test repo on my org and start pushing commits there

ps: I am not asking to chase and fix a bug, I am trying to check if this feature can be used with my workflow without altering any GIT config file

yanganto · 2024-03-05T08:45:32Z

If you do not put the SSH key setting in the git config, there is no alert and no sign.

If you put the ssh_key settings in the git config (no allowedSignersFile), you can sign without an alert.

If you put the pub key to GITHUB, you can get a verified tag, else you will get an unverified tag.

If you put the allowedSignersFile and the public key to the file, you can check the sign locally without an alert.

If you want something you need to provide the correct setting for the tool. It is reasonable.

thePanz · 2024-03-05T09:14:48Z

Update: it still does not work for my use-case
See: https://github.com/thePanz/gitui-test/commits/master/

Git CLI:

added a line to the md file
git add testing.md
git commit -m "Test: commit with signature, git CLI"
git push

With GitUI (compiled with cargo build --release, from the latest commit in this branch)

$ gitui
3 → goes to Files
select the testing.md file
e → Edit
[enter] → stages testing.md file
c → Commit, add the commit message, then Ctrl+d
p → Push the changes

The the two tests have been run in the same folder, with no changes to any git configuration file

About the allowedSignersFile, this file is used solely to locally verify the signatures, see: https://docs.gitlab.com/ee/user/project/repository/signed_commits/ssh.html#verify-commits-locally . If such file is not configured, the git tree --show-signatures will mark the commits as not verified. It should have no impact on the signing process itself.

yanganto · 2024-03-05T09:31:18Z

Are these settings in your gitconfig?

[user]
	email = *
	name = *
	signingKey = /path/to/keyfile
[gpg]
	format = ssh
[commit]
	gpgsign = true
[gpg "ssh"]
	allowedSignersFile = /path/to/allowed_signers

Can you also provide this? Do you put your pub key on GITHUB?

Aggreed, the allowedSignersFile will not impact on the signing process/

thePanz · 2024-03-05T09:43:36Z

Yes, those are the settings I have:

# File: ~/.gitconfig
[includeIf "gitdir:~/Works/oss/**"]
        path = .config/git/gitconfig-oss
[credential]
        helper = store
[gpg "ssh"]
        allowedSignersFile = ~/.ssh/allowed_signers

# File: ~/.config/git/gitconfig-oss
[user]
    email = ***
    name = ***
    signingkey = ~/.ssh/github.pub
[gpg]
    format = ssh
[commit]
    gpgsign = true

As mentioned above: I tested both cases from the same folder (~/Works/oss/gitui-test) where I checked out the repository.

Notes:

the signing key is the public key, not the private key
I am using a credentials store helper git-credential-libsecret

thePanz · 2024-03-05T09:44:19Z

Can you also provide this? Do you put your pub key on GITHUB?

yes, as you can see from https://github.com/thePanz/gitui-test/commits/master/, the commit I did with the git CLI is verified

yanganto · 2024-03-05T09:52:58Z

Is there a key pair ~/.ssh/github.pub and ~/.ssh/github on your machine?

If not.
You can not sign your pubkey and verify with a pubkey. I believe git talk to the store and use the corresponding private key.

In the change log, I already mentioned the keys are on disk, if not gitui did not sign but no error happened and no alert. This is good behavior you want to see, right?

thePanz · 2024-03-05T10:04:46Z

No, there is no ~/.ssh/github on my machine, the private key is provided via the git-credential-libsecret to GIT.

I guess we spotted the issue then 👍

I believe that a message or warning should be shown in that case, as the git configuration is explicitly asking to add the signature, while gitui is ignoring the missing private key needed to sign it, and thus not respect the configuration.

This is not 100% clear from the changelog IMO

yanganto · 2024-03-05T10:21:16Z

So now you are asking for an alert box with a warning message for this first implementation.
I think it should be another feature about talking to the store, but not an alert box at this PR. If you buy in the current PR, and list this as a known issue. Or another alert message feature, it is also related with #2047 (comment).

Or, another solution is.
We can refuse to commit if I can not get the private key. Just like current behavior refuse to commit, if there is a sign setting in gitconfig.

thePanz · 2024-03-07T13:15:08Z

@yanganto I am not asking to do it, I am suggesting to have such behavior as it would help the users to better understand why the signing is required in the git config file, but gitui is adding the commit without the signature (thus ignoring the git config setting)

just my 2 cents

yanganto · 2024-03-07T14:56:37Z

So you want me to refuse commit when gitui can not sign.

yanganto · 2024-03-14T11:06:54Z

@thePanz I don't see any blocker at this moment. Now anything gitconfig set but gitui cannot do will block which follows the current behavior of master branch.

If you agree with my point and try to warn the user not to block the user you can open an issue and mention these #2047 (comment) #2047 (comment). These are already clear enough to solve in other PRs but not this one.

extrawurst

please update to latest master and integrate with the new signing trait

extrawurst · 2024-04-14T18:25:20Z

closed via #2175

yanganto added 2 commits February 13, 2024 17:03

sign a commit if ssh secret key is provided

93176c9

update CHANGELOG

ace7465

yanganto mentioned this pull request Feb 13, 2024

Add ssh sign feature #2023

Closed

4 tasks

update cargo audit

a1e7b5b

update test case

596b28a

fix linter

86f2f53

fix linter for gitui

9acfb03

yanganto added 2 commits February 13, 2024 22:56

add option for ssh_secret_key

c48d0a3

fix clippy

1d84f61

yanganto marked this pull request as ready for review February 13, 2024 15:26

yanganto added 4 commits February 15, 2024 22:27

use gitconfig user.signingKey to sign commit

d82ff46

Merge branch 'master' into ssh-sign-2

f9b1dd2

update CHANGELOG

871424f

fix cargo deny config

1a0cadd

yanganto force-pushed the ssh-sign-2 branch from cc23f4e to 1a0cadd Compare February 15, 2024 14:56

disallow commit when ssh sign set but the key is not found in the disk

411b936

yanganto force-pushed the ssh-sign-2 branch from 6319258 to 411b936 Compare March 7, 2024 16:32

raise error if commit with a ssh setting but without key

4af7a60

extrawurst requested changes Apr 3, 2024

View reviewed changes

This was referenced Apr 9, 2024

Signing implementation currently not interchangeable (impl trait return) #2173

Closed

sign with ssh key in disk #2175

Merged

extrawurst closed this Apr 14, 2024

Sign a commit if ssh secret key is provided #2047

Sign a commit if ssh secret key is provided #2047

Conversation

yanganto commented Feb 13, 2024 • edited Loading

extrawurst commented Feb 13, 2024

yanganto commented Feb 13, 2024

extrawurst commented Feb 13, 2024

extrawurst commented Feb 13, 2024

yanganto commented Feb 13, 2024

extrawurst commented Feb 13, 2024

extrawurst commented Feb 13, 2024

yanganto commented Feb 13, 2024

yanganto commented Feb 13, 2024 • edited Loading

yanganto commented Feb 13, 2024 • edited Loading

yanganto commented Feb 13, 2024

yanganto commented Feb 13, 2024

extrawurst commented Feb 13, 2024

yanganto commented Feb 14, 2024 • edited Loading

extrawurst commented Feb 15, 2024

yanganto commented Mar 4, 2024 • edited Loading

thePanz commented Mar 4, 2024

yanganto commented Mar 4, 2024

patka-123 commented Mar 4, 2024

yanganto commented Mar 4, 2024 • edited Loading

thePanz commented Mar 4, 2024

yanganto commented Mar 4, 2024 • edited Loading

thePanz commented Mar 4, 2024 • edited Loading

yanganto commented Mar 4, 2024 • edited Loading

supleed2 commented Mar 4, 2024

thePanz commented Mar 5, 2024

yanganto commented Mar 5, 2024 • edited Loading

thePanz commented Mar 5, 2024 • edited Loading

yanganto commented Mar 5, 2024 • edited Loading

thePanz commented Mar 5, 2024

thePanz commented Mar 5, 2024

yanganto commented Mar 5, 2024 • edited Loading

thePanz commented Mar 5, 2024 • edited Loading

yanganto commented Mar 5, 2024 • edited Loading

thePanz commented Mar 7, 2024

yanganto commented Mar 7, 2024

yanganto commented Mar 14, 2024 • edited Loading

extrawurst left a comment

Choose a reason for hiding this comment

extrawurst commented Apr 14, 2024

yanganto commented Feb 13, 2024 •

edited

Loading

yanganto commented Feb 13, 2024 •

edited

Loading

yanganto commented Feb 13, 2024 •

edited

Loading

yanganto commented Feb 14, 2024 •

edited

Loading

yanganto commented Mar 4, 2024 •

edited

Loading

yanganto commented Mar 4, 2024 •

edited

Loading

yanganto commented Mar 4, 2024 •

edited

Loading

thePanz commented Mar 4, 2024 •

edited

Loading

yanganto commented Mar 4, 2024 •

edited

Loading

yanganto commented Mar 5, 2024 •

edited

Loading

thePanz commented Mar 5, 2024 •

edited

Loading

yanganto commented Mar 5, 2024 •

edited

Loading

yanganto commented Mar 5, 2024 •

edited

Loading

thePanz commented Mar 5, 2024 •

edited

Loading

yanganto commented Mar 5, 2024 •

edited

Loading

yanganto commented Mar 14, 2024 •

edited

Loading