Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for GPG signing commits #219

Closed
wants to merge 4 commits into from

Conversation

grantbdev
Copy link

@grantbdev grantbdev commented Jul 29, 2020

Overview

This PR is an initial stab at resolving #97. This is a recreation of #218, which I accidentally created without marking as a draft PR and I was unable to set it back so I decided to close it.

With these changes, I was able to create this PR's commits signed with gitui:
Screen Shot 2020-07-29 at 5 51 07 PM

Details

Unfortunately, libgit2 appears to not provide an interface for creating signed commits as friendly as standard commit creation so there is a lot of code added. The GPG signing must be done manually which brings in dependencies for gpgme. Unlike standard commit creation, there is not an option to automatically update HEAD so it must be done manually when after creating a signed commit. When dealing with a newly initialized repo, this means the default branch has to be created as well! There is no official documentation creating signed commits with libgit2, so credit goes to rust-lang/git2-rs#507 for figuring it out.

I am not sure how test suite should handle this new functionality. The main conditional is currently determined by the user's git config option, so on my computer it will always use the GPG signing. We could add extra tests and manage whether GPG signing should be on for each one or try to run the entire suite with GPG signing on and off. The main commit tests are working, but locally I am getting ~4-6 failures each time. Here is a sample run:

...

failures:

---- sync::commit_details::tests::test_msg_invalid_utf8 stdout ----
thread 'sync::commit_details::tests::test_msg_invalid_utf8' panicked at 'Buffer was not valid UTF-8', asyncgit/src/sync/commit.rs:83:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- sync::commits_info::tests::test_invalid_utf8 stdout ----
thread 'sync::commits_info::tests::test_invalid_utf8' panicked at 'Buffer was not valid UTF-8', asyncgit/src/sync/commit.rs:83:13

---- sync::commit::tests::test_tag stdout ----
Error: Gpg(Error { source: Some("gcrypt"), code: 32854, description: "Cannot allocate memory" })
thread 'sync::commit::tests::test_tag' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `0`: the test returned a termination value with a non-zero status code (1) which indicates a failure', /Users/grantbdev/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/src/rust/src/libstd/macros.rs:16:9

---- sync::commits_info::tests::test_log stdout ----
thread 'sync::commits_info::tests::test_log' panicked at 'called `Result::unwrap()` on an `Err` value: Gpg(Error { source: Some("Pinentry"), code: 32854, description: "Cannot allocate memory" })', asyncgit/src/sync/commits_info.rs:137:18

---- sync::stash::tests::test_stash_without_2nd_parent stdout ----
Error: Gpg(Error { source: Some("Pinentry"), code: 32854, description: "Cannot allocate memory" })
thread 'sync::stash::tests::test_stash_without_2nd_parent' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `0`: the test returned a termination value with a non-zero status code (1) which indicates a failure', /Users/grantbdev/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/src/rust/src/libstd/macros.rs:16:9

...

test result: FAILED. 48 passed; 5 failed; 0 ignored; 0 measured; 0 filtered out

The invalid UTF-8 failures seem to happen every time. There is a point in the GPG commit logic that will panic if the message contains invalid UTF-8 and I guess that differs from the standard commit logic? I'm not sure how that should be resolved other than having different expectations with and without the GPG signing feature.

The rest are random tests failing each run, pointing to gcrypt or Pinentry and saying it couldn't allocate memory. This appears to only happen when the tests are run in parallel by default. cargo test -- --test-threads=1 will eliminate the random failures.

@grantbdev grantbdev force-pushed the gb/gpg-sign-commits branch from 4fd93df to 179d6d4 Compare July 29, 2020 23:20
@extrawurst
Copy link
Owner

@grantbdev thanks for looking into this! Can we maybe learn something from libgit2 in how they manage to unittest stuff in the presence of configs?

@grantbdev
Copy link
Author

@extrawurst From what I can tell, the libraries tests things at low enough of a level that typically the config isn't a factor. In git2 there are some occasional config setups: https://github.com/rust-lang/git2-rs/blob/23954937d69ed5c3a798ba85f653099fbecf3cf2/src/build.rs#L720-L721

Setting values during a test like this will impact the execution: repo.config().unwrap().set_bool("commit.gpgsign", false).unwrap(); The config values set in the test code will take priority over the git config on the computer. So that seems like a good way to control it which branches to take in gitui's higher-level logic that would depend on the config.

git2 doesn't test GPG signing at all. The tests I see in libgit2 provide pre-determined signatures which isn't really an option here: https://github.com/libgit2/libgit2/blob/02d61a3b66a6e5f5bc0154d780daaf5f7b71ccd9/tests/commit/write.c#L302-L400

@grantbdev grantbdev force-pushed the gb/gpg-sign-commits branch 3 times, most recently from 4af293c to 519547d Compare July 30, 2020 02:03
@grantbdev grantbdev force-pushed the gb/gpg-sign-commits branch 3 times, most recently from 8912c4d to 02284d3 Compare July 30, 2020 15:40
@grantbdev grantbdev force-pushed the gb/gpg-sign-commits branch 2 times, most recently from 7d84c89 to 9077deb Compare July 30, 2020 17:50
@extrawurst
Copy link
Owner

@grantbdev what's the status on this?

@grantbdev
Copy link
Author

@extrawurst Sorry, definitely got distracted from working on this. I know I was getting discouraged that it seems system (non-Rust) libraries were needed to get this to work and I was having trouble getting it set up correctly for the Windows CI build. I have a feeling this could be too burdensome to the project overall to maintain and I'm uncertain about whether it will negatively impact users who don't care about GPG signing, but let me know what you think.

My gut feeling is that libgit2could be making adding this feature much harder than it needs to be. It appears GitX shelled out to the git CLI which made it much easier to add GPG support and while Fork is not open-source I suspect it's doing something similar since it selects installed git instances.

I know that is a pretty radical departure from the current design of your project, but I'm curious if you have considered or experimented with that approach at all. I also wonder if it's something that could be used in some parts rather than a complete overhaul of not using libgit2 for anything.

@extrawurst
Copy link
Owner

@grantbdev it would be a shame to see this bitrot away. but I agree adding big bunch of system dependencies is a bit of a downer. did you look into https://gitlab.com/sequoia-pgp/sequoia if that might be a solution here?

@grantbdev
Copy link
Author

@extrawurst I don't think I previously looked at Sequoia, so thanks for linking. I took a short spin at it and from what I can tell I don't think it well help. I think it's generally more low-level than what GPGme provides and I'm not familiar with the implementation details. For instance, I am not sure how to replicate finding a public key from the local keychain which is the first step in using the user's Git GPG settings.

@nils-a nils-a mentioned this pull request Feb 23, 2021
@extrawurst extrawurst force-pushed the master branch 2 times, most recently from 81a8849 to f84f6f4 Compare March 3, 2021 21:27
@solidnerd
Copy link

Hey @extrawurst and @grantbdev,
What`s the current status of this PR? How I can help to get, this shipped at least at an experimental feature(Experimental Feature Flag or Config).
I really want to use it since I use signed commits everywhere.

You can create commit objects with git commit-tree or sign them directly with git commit-tree -S afterward it will return the git commit object id. Probably we can use it in libgit2 to create the final commit.

I would also propose signing commits directly with the git binary because it’s more convenient to configure it and reduce dependencies. Since People who sign commits already have these tools installed on their machine, it works with the git binary properly.

References

https://git-scm.com/docs/git-commit-tree
https://jwiegley.github.io/git-from-the-bottom-up/1-Repository/4-how-trees-are-made.html

@grantbdev
Copy link
Author

@solidnerd This PR is effectively abandoned by me at this point. You're welcome to take your own stab at it and use any of my commits if they are helpful. My understanding was libgit2 wouldn't work (at least easily) for this task, and I don't know what @extrawurst's opinion is on utilizing the git binary in this project.

@albfan
Copy link

albfan commented Oct 10, 2022

Just if that helps. I'm gitg maintainer and use this PR as reference to confirm steps needed to sign a commit.

libgit2 is agnostic about signing a commit, that's why it offers a commit_create_buffer (with contents for commit) and a commit_create_with_signature which just add this signature to commit object. In the middle you sign with gpgme that content. I hear there's an spec to sign commits with ssh keys, that would be the only part to change here if you want to implement that too.

The tricky part is create_commit_from_ids updates the reference (symbolic like master or real as HEAD) while with this functions you need to do on your own.

I just merge it: https://gitlab.gnome.org/GNOME/gitg/-/merge_requests/198 and libgit2-glib (glib wrapper around libgit2) related PR is linked there.

@grantbdev grantbdev closed this Jan 19, 2023
@nigelatdas
Copy link

image
This is probably a dumb question... but why not just shell out to the git cli to do the commit?

@albfan
Copy link

albfan commented Dec 4, 2023

@nigelatdas that's the approach gitg will take. libgit2 has different scopes and goals than git GUI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants