Support git commit signing using OpenPGP #1544

hendrikmaus · 2023-02-13T20:35:20Z

This Pull Request fixes/closes #97.

It changes the following:

Add a new trait called Sign
Implement Sign for gpg commit signing via shellout
- Users with a working signing setup will find that it just works in gitui as well
Add entrypoints for all natively supported methods
Add potential entrypoint for pure rust implementations behind a gitconfig value of gitui.signing_methods

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

Todo:

Test in Windows 🪟
~~Documentation~~
~~Draft an isolated integration test running in a container~~

Draft an isolated integration test running in a container

Since shellouts call out to actual system binaries, @extrawurst and I came up with the idea to create a test that runs in the isolation of a container. Inside of that environment, it could generate a GPG key and do an actual commit to verify the signature was created as expected.

Update: will be tracked in a follow-up issue.

hendrikmaus · 2023-02-14T20:24:25Z

Will attend to the failures in CI within the next days.

ZCShou · 2023-02-20T01:03:34Z

waiting for this feature ready

hendrikmaus · 2023-02-20T07:24:48Z

Looking into the CI errors again now.

hendrikmaus · 2023-02-20T07:45:00Z

🆗 ✅ The previous error was caused by a doctest I added. The other unit tests prepare their environment, the doctest does not. Since I added it as documentation only, I deactivated the doctest and the CI run in my fork was good now: https://github.com/hendrikmaus/gitui/actions/runs/4221076989

@extrawurst if you'd approve another CI run here, thanks.

melMass · 2023-03-14T15:51:55Z

Hey just checking the status of this PR?
I can test on macOS and windows easily if needed :)

hendrikmaus · 2023-03-14T20:27:39Z

Both would be appreciated, thank you.

asyncgit/src/sync/sign.rs

utkarshgupta137 · 2023-03-17T16:55:04Z

Tested it on macOS. Authoring commits works fine, but some simple operations like amending & rewording the last commit don't work. I've created a PR with workarounds: hendrikmaus#1.

tmikeschu · 2023-04-25T18:20:35Z

👋 hello! Any movement on this by chance? Thanks!

ZCShou · 2023-04-26T01:05:11Z

👋 hello! Any movement on this by chance? Thanks!

I‘m also waiting for this feature

hendrikmaus · 2023-04-26T05:27:56Z

Haven’t had time to continue, but will do so in the beginning of ~~May~~ June (sry for the delay).

Will tackle these:

Fix amending and rewording, thanks @utkarshgupta137 for already preparing a change-set
Align configuration with the existing pattern (in progress)
Test on Windows
Documentation
Integration test

nrabulinski · 2023-05-23T09:15:52Z

Hey, how's the progress on this? :)

hendrikmaus · 2023-06-08T08:05:47Z

Can someone test a current build on a Windows environment? I'd appreciate that.

damccull · 2023-06-08T18:14:04Z

To test this, would we need to download this PR's code, compile, and run? Or is there some auto-generated binary for testing I should download somewhere?

hendrikmaus · 2023-06-08T19:02:37Z

At this point there are no pre-compiled binaries. So you have to download and compile it.

SirusCodes · 2023-06-08T19:18:37Z

I compiled it with cargo install gitui and tried to run the binary from .cargo dir but it is showing me the same error. I might be doing something wrong...

PS: I don't have experience in working with Rust.

hendrikmaus · 2023-06-08T19:29:37Z

I reckon you compiled the master branch as that is the error that was shown before this initiative started.
Here is the zip file of my branch on github: https://github.com/hendrikmaus/gitui/archive/refs/heads/gpg-commit-signing.zip

Sped0n · 2023-06-17T03:21:47Z

works like a charm on ventura👌

just want this pr to be merged as soon as possible

Sped0n · 2023-06-17T03:29:09Z

I compiled it with cargo install gitui and tried to run the binary from .cargo dir but it is showing me the same error. I might be doing something wrong...

PS: I don't have experience in working with Rust.

maybe install with command below?

cargo install --git https://github.com/hendrikmaus/gitui --branch gpg-commit-signing

eclairevoyant · 2023-06-20T18:02:39Z

@hendrikmaus works for me on windows 11 (built with rust using MSVC toolchain, GnuPG as program for signing)

Password prompt appears correctly when signing is enabled by default
Commits are in fact signed (verified with git log --show-signature)

hendrikmaus · 2023-06-20T19:55:58Z

Thank you for the feedback, that is good news.

hendrikmaus · 2023-06-20T20:03:16Z

@extrawurst re configuration.

As we were talking about this the other day, did you take another look at the available approaches? I.e. .gitconfig file versus the gitui options?

damccull · 2023-06-21T02:25:21Z

Well, I installed it from the git repo and it definitely tries to sign now. However, I'm using my ssh key to sign rather than a gpg key and it says that's unsupported. Would this capability be difficult to add?

hendrikmaus · 2023-06-21T07:47:53Z

@damccull out of scope here, but the change-set already prepares an entry point for that in the code. There is another open issue about signing with ssh keys in #1149

damccull · 2023-06-21T11:49:56Z

Ah, thanks.

hendrikmaus · 2023-06-22T07:43:26Z

@extrawurst re configuration.

As we were talking about this the other day, did you take another look at the available approaches? I.e. .gitconfig file versus the gitui options?

I think it would be feasible to pass a new optional struct with the signing options down into the asyncgit crate from here: https://github.com/hendrikmaus/gitui/blob/24bb8ac57e2f7c8320eb4e6b75c8dc4739b6b7e0/src/components/commit.rs#L274

The asyncgit::sync::commit function has more than 40 call sites, mostly tests, that would need to be adjusted. The signatures of asyncgit::sync::reword and asyncgit::sync::amend would also be affected as they call commit.

I would propose something like Option<CommitConfig> to implement it open for extension. We might see the need to pass more configurations down to commit and I wouldn't want to limit it to only the signing options now. What do you think @extrawurst ?

1vnt · 2023-07-19T17:34:48Z

Any updates on this?

extrawurst · 2024-02-21T10:03:42Z

@hendrikmaus lets do that. this way we can move forward and set the right macherinery in place that allows implementing other signing methods too. the only important thing to please look over is: just making sure the way ssh singing is done in #2047 can be adopted to be "just" another implementation of your trait?

hendrikmaus · 2024-02-22T09:41:39Z

I’ll have a look and verify that.

hendrikmaus · 2024-03-23T18:37:26Z

Did not mean to close this. Was updating the PR.

extrawurst · 2024-03-23T18:39:30Z

ok lets reopen :)

these will be tracked in another issue

…signing * 'master' of github.com:extrawurst/gitui: (52 commits) Git Config Commit Comments (extrawurst#2145) update changelog for 0.25.2 fix another github action warning upgrade version prepare release preapre for release changelog fixes tag annotation being broken in 0.25 (extrawurst#2139) update changelog fix: 2114 Bump backtrace from 0.3.69 to 0.3.70 Fixed: - Cargo clippy errors cargo updates cargo update todo update changelog fix: index out of bounds when blaming a file ending with a blank line (extrawurst#2130) Bump bitflags from 2.4.2 to 2.5.0 Bump clap from 4.5.2 to 4.5.3 fix chrono deprecations ...

hendrikmaus · 2024-03-23T18:41:06Z

We're back ;)

hendrikmaus · 2024-03-23T18:42:05Z

Removed the testing files, as discussed, and will track them in a new issue later.

Now taking a look at the ssh signing PR to see if it could easily be refactored to use the Sign trait.

hendrikmaus · 2024-03-23T19:03:56Z

The steps should roughly be:

move the logic of fn fetch_ssh_key(config: &Config) -> Option<PrivateKey> to SignBuilder::from_gitconfig's match arm for ssh
create smth like pub struct SSHSign and implement Sign for it using sk.sign which is all that is needed once the keypair was found
the logic in the commit function does not need to change as it uses whatever the SignBuilder returns to sign the data. That is given the ssh signing also works on the commit's UTF-8 data
- the Sign impl expects a UTF-8 encoded string slice, while the ssh impl seems to sign the buffer byte slice; will do a quick check to see I should change the trait to accept a byte slice as well

hendrikmaus · 2024-03-23T19:12:02Z

According to git's documentation, commits are encouraged to be UTF-8, but any encoding can be used. So the raw bytes of the commit buffer should be signed rather than converting it to UTF-8 first.

Will implement the change in the Sign trait.

hendrikmaus · 2024-03-23T19:28:50Z

Done.

hendrikmaus · 2024-03-23T19:32:32Z

@extrawurst if you would approve the CI run, this is ready to be merged.

asyncgit/src/sync/commit.rs

extrawurst · 2024-03-24T02:39:39Z

asyncgit/src/sync/commit.rs

+		let sign = SignBuilder::from_gitconfig(&repo, &config)?;
+		let signed_commit = sign.sign(&buffer)?;
+		let commit_id =
+			repo.commit_signed(commit, &signed_commit, None)?;


if we want to support more than just gpg the third parameter signature_field needs to take something out of sign which in this case has to be gpgsig but something else for ssh

if we want to support more than just gpg the third parameter signature_field needs to take something out of sign which in this case has to be gpgsig but something else for ssh

I very much wish for ssh signing. I don't PGP/GPG but i ssh all the time.

Fixed.

@damccull this one only adds openpgp; there is another PR that wants to add ssh signing. @extrawurst wants them to adopt the Sign trait once this one here is merged though.

extrawurst · 2024-03-24T02:43:24Z

another followup: this does not sign tags

extrawurst · 2024-03-24T20:09:38Z

wohooooo its merged! @hendrikmaus thanks for sticking with this through the end. could you please write an issue with the way you planned to do actual testing of this in CI via docker? also ideally pointing to the commits you reverted as inspiration, so that someone can take that up!

hendrikmaus · 2024-03-24T20:14:50Z

another followup: this does not sign tags

@extrawurst correct. it tags a signed commit just fine, but the tag itself isn't signed due to that. The way git signs tags is different from how it is done for commits. I will open an issue with git2-rs to see if there is more information on it since I cannot seem to find anything in the docs that hints how to sign a tag.

I prepped the trait to differ between signing commits and tags, but I won't provide an implementation for tags with this PR.

hendrikmaus mentioned this pull request Feb 13, 2023

GPG sign commits #97

Closed

hendrikmaus force-pushed the gpg-commit-signing branch from d99f30d to 6aefadd Compare February 20, 2023 07:27

hendrikmaus commented Mar 16, 2023

View reviewed changes

asyncgit/src/sync/sign.rs Outdated Show resolved Hide resolved

hendrikmaus force-pushed the gpg-commit-signing branch from 850cfdc to 1b8f8f8 Compare March 17, 2023 07:42

kachick mentioned this pull request Mar 11, 2024

Prefer GPG(GnuPG) rather than SSH key to sign commits kachick/dotfiles#311

Merged

hendrikmaus closed this Mar 23, 2024

hendrikmaus force-pushed the gpg-commit-signing branch from bf81abd to b15c864 Compare March 23, 2024 18:34

hendrikmaus added 2 commits March 23, 2024 19:39

remove testing files

29cea7a

these will be tracked in another issue

hendrikmaus reopened this Mar 23, 2024

hendrikmaus added 2 commits March 23, 2024 20:26

sign the bytes of the commit buffer instead of utf-8

7d81738

changelog entry

618f208

format code

a27c7ef

hendrikmaus requested a review from extrawurst March 23, 2024 19:31

extrawurst reviewed Mar 24, 2024

View reviewed changes

asyncgit/src/sync/commit.rs Outdated Show resolved Hide resolved

extrawurst reviewed Mar 24, 2024

View reviewed changes

Repository owner deleted a comment from damccull Mar 24, 2024

hendrikmaus added 2 commits March 24, 2024 20:04

support signing initial commit

3a264d5

return both signature and signature_field value from sign

a8bb4bb

extrawurst merged commit 5b3e2c9 into extrawurst:master Mar 24, 2024
17 of 18 checks passed

hendrikmaus mentioned this pull request Mar 24, 2024

Signing a tag rust-lang/git2-rs#1039

Open

Support git commit signing using OpenPGP #1544

Support git commit signing using OpenPGP #1544

Conversation

hendrikmaus commented Feb 13, 2023 • edited

Draft an isolated integration test running in a container

hendrikmaus commented Feb 14, 2023

ZCShou commented Feb 20, 2023

hendrikmaus commented Feb 20, 2023

hendrikmaus commented Feb 20, 2023 • edited

melMass commented Mar 14, 2023

hendrikmaus commented Mar 14, 2023

utkarshgupta137 commented Mar 17, 2023

tmikeschu commented Apr 25, 2023

ZCShou commented Apr 26, 2023

hendrikmaus commented Apr 26, 2023 • edited

nrabulinski commented May 23, 2023

hendrikmaus commented Jun 8, 2023

damccull commented Jun 8, 2023

hendrikmaus commented Jun 8, 2023

SirusCodes commented Jun 8, 2023

hendrikmaus commented Jun 8, 2023

Sped0n commented Jun 17, 2023

Sped0n commented Jun 17, 2023

eclairevoyant commented Jun 20, 2023 • edited

hendrikmaus commented Jun 20, 2023

hendrikmaus commented Jun 20, 2023

damccull commented Jun 21, 2023

hendrikmaus commented Jun 21, 2023

damccull commented Jun 21, 2023

hendrikmaus commented Jun 22, 2023

1vnt commented Jul 19, 2023

extrawurst commented Feb 21, 2024

hendrikmaus commented Feb 22, 2024

hendrikmaus commented Mar 23, 2024

extrawurst commented Mar 23, 2024 • edited

hendrikmaus commented Mar 23, 2024

hendrikmaus commented Mar 23, 2024

hendrikmaus commented Mar 23, 2024

hendrikmaus commented Mar 23, 2024

hendrikmaus commented Mar 23, 2024

hendrikmaus commented Mar 23, 2024

extrawurst Mar 24, 2024

Choose a reason for hiding this comment

damccull Mar 24, 2024

Choose a reason for hiding this comment

hendrikmaus Mar 24, 2024

Choose a reason for hiding this comment

extrawurst commented Mar 24, 2024

extrawurst commented Mar 24, 2024

hendrikmaus commented Mar 24, 2024 • edited

hendrikmaus commented Feb 13, 2023 •

edited

hendrikmaus commented Feb 20, 2023 •

edited

hendrikmaus commented Apr 26, 2023 •

edited

eclairevoyant commented Jun 20, 2023 •

edited

extrawurst commented Mar 23, 2024 •

edited

hendrikmaus commented Mar 24, 2024 •

edited