-
Notifications
You must be signed in to change notification settings - Fork 691
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
adds support for gpg commit signing (fixes #1018) #1448
Conversation
How does this pull request add signing support? It seems like it's just adding a new convenience |
That's exactly right - it wraps the Commit.createWithSignature method. Would you rather see this method extend
Or do you have something else in mind? |
Make sure to enable support for |
@implausible awesome! I'll give that a try |
@implausible I'm having some trouble calling extractSignature. Do you know a way to handle these buffers to write the signature and signed_data to? Here's the generated .cc
|
You can circumvent the |
Let me know if you need any other help on this. I would really like to get GPG signing in sooner rather than later. Awesome work so far! |
@cjhoward92 thanks. I did give that a try, but I need to read the value back out.
I can get I'm not sure what I am missing. |
test/tests/commit.js
Outdated
signature); | ||
}) | ||
.then(function(commitId) { | ||
assert.equal(expectedCommitId, commitId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you are still working, but I would like to see a test that verifies the signature in the gpgsig
field of the commit header. That will definitely lend credence to fact that we have GPG signing.
Oh, I see. I didn't realize you were talking about the I think the final signature will probably look different since we need to return some of this stuff... |
Check out
You will have to specify both Does that all make sense? I think what I told you can get you closer, but not all the way there quite yet. |
yes it makes sense. I'll try out generating with the buffer and see if I can get it to work. For what to return. We could return an object that has both, or just return the signature.
and
or
the method I was planning to add to lib/commit.js would look something like this, but with the promises
|
Do you know what the difference between Overall I like your ideas! We just need to find the best path for you to be able to execute. For now, you can try setting just the |
sounds good. I'll try to get something working. I believe the intention of ExtractSignature is so that someone could take the signed_data and the signature and verify it against a public key signature is the actual signature produced from signing the commit. This is the full commit:
this is the signature:
This is signed_data:
|
Cool, that makes sense. Yeah, we should probably get that in. Let me know if you want any extra help or just a second set of eyes! Thanks again for working on this. I know a lot of people want GPG in NodeGit so you will be a hero! |
@dabutvin I am opening up a PR shortly that will get git_commit_extract_signature built into nodegit for you. You'll want to rebase on that. I will post here when it's open. |
98e2130
to
a957def
Compare
@implausible I updated to verify the signature from the signed commit using the new |
Can you work your magic in descriptor.json for This is what I am trying to set up in repository.js onSignature would be a function that is up to the caller to implement
|
test/tests/commit.js
Outdated
it("Can create a signed commit", function() { | ||
var expectedCommitId = "b78570159824e6fc161418a626a646933a10ea89"; | ||
|
||
// https://github.com/libgit2/libgit2/blob/master/tests/commit/write.c#L362-L384 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can just copy/paste this since libgit2 is licensed under the GPLv2 and NodeGit is licensed under MIT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you can replace it with these Axosoft signatures that were merged in from #1458?
Lines 723 to 740 in 210d02a
it.only("Can retrieve the gpg signature from a commit", function() { | |
var expectedSignedData = | |
"tree f4661419a6fbbe865f78644fec722c023ce4b65f\n" + | |
"parent 32789a79e71fbc9e04d3eff7425e1771eb595150\n" + | |
"author Tyler Ang-Wanek <tylerw@axosoft.com> 1521227848 -0700\n" + | |
"committer Tyler Ang-Wanek <tylerw@axosoft.com> 1521227848 -0700\n\n" + | |
"GPG Signed commit\n"; | |
var expectedSignature = | |
"-----BEGIN PGP SIGNATURE-----\n\n" + | |
"iQEcBAABCAAGBQJarBhIAAoJEE8pfTd/81lKQA4IAL8Mu5kc4B/MX9s4XB26Ahap\n" + | |
"n06kCx3RQ1KHMZIRomAjCnb48WieNVuy1y+Ut0RgfCxxrJ1ZnzFG3kF2bIKwIxNI\n" + | |
"tYIC76iWny+mrVnb2mjKYjn/3F4c4VJGENq9ITiV1WeE4yJ8dHw2ox2D+hACzTvQ\n" + | |
"KVroedk8BDFJxS6DFb20To35xbAVhwBnAGRcII4Wi5PPMFpqAhGLfq3Czv95ddSz\n" + | |
"BHlyp27+YWSpV0Og0dqOEhsdDYaPrOBGRcoRiqjue+l5tgK/QerLFZ4aovZzpuEP\n" + | |
"Xx1yZfqXIiy4Bo40qScSrdnmnp/kMq/NQGR3jYU+SleFHVKNFsya9UwurMaezY0=\n" + | |
"=eZzi\n-----END PGP SIGNATURE-----"; | |
One thing to keep in mind with the onSignature callback is that it may be asynchronous. I would change the flow in your code to account for that. |
a957def
to
d174c3c
Compare
@implausible thanks for exposing the |
4d80a04
to
2aea28b
Compare
No changes for a while? Still working on it? @dabutvin |
I forgot all about this actually. I'll rebase the commit now - it looks finished, but there is a broken build. |
2aea28b
to
caee388
Compare
it passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good but this one bit here. Thanks again for your work on this 😄
@implausible not sure what happened to the build, but the documentation config looks like it hung. |
6494298
to
e07103e
Compare
- the commit that gets created and the signed content have to match
e07103e
to
90e9d89
Compare
@implausible check out newline I appended to the end of the commit buffer and let me know what you think. I was doing some end to end testing of the signing and verification and found that when verifying the signature from the commit content, this newline was required to get an exact match with the commit content. |
Thanks for the reminder! |
This is great. We're also working on adding: libgit2/libgit2#4913. Hopefully the next release of nodegit will include signing for rebase and commit. |
This wraps the libgit commit signing methods.
I couldn't get togit_commit_extract_signature
as it's not available until 0.24https://libgit2.github.com/libgit2/#HEAD/group/commit/git_commit_extract_signature