From: Michael J Gruber <hidden> Date: 2016-06-15 23:01:31
Hi there,
Some of you may remember me from my more active times...
Anyways, a recent blog post about signed commits in git triggered me to
look at our tools for that again. It seems that we only have the
log/pretty family on the user facing side, but everything we need under
the hood.
So here's a suggestion to implement verify-commit in a way which is
completely analogous to verify-tag. In fact, it could be coded more
elegantly, but I kept it this way so that we could merge the two more
easily in case we wish to do so.
I will follow up with tests if the design principle is something we agree
upon.
Michael J Gruber (3):
pretty: free the gpg status buf
gpg-interface: provide access to the payload
verify-commit: scriptable commit signature verification
Documentation/git-verify-commit.txt | 28 +++++++++++
Makefile | 1 +
builtin.h | 1 +
builtin/merge.c | 1 +
builtin/verify-commit.c | 98 +++++++++++++++++++++++++++++++++++++
command-list.txt | 1 +
commit.c | 1 +
git.c | 1 +
gpg-interface.h | 1 +
pretty.c | 2 +
10 files changed, 135 insertions(+)
create mode 100644 Documentation/git-verify-commit.txt
create mode 100644 builtin/verify-commit.c
--
2.0.0.533.gae2e602
From: Michael J Gruber <hidden> Date: 2016-06-15 23:01:31
4a868fd (pretty: parse the gpg status lines rather than the output, 2013-02-14)
made the gpg status lines available to callers and made sure they freed
the used space, but missed one spot.
Free the status line buffer also in the remaining spot.
Signed-off-by: Michael J Gruber <redacted>
---
pretty.c | 1 +
1 file changed, 1 insertion(+)
From: Michael J Gruber <hidden> Date: 2016-06-15 23:01:31
Commit signatures can be verified using "git show -s --show-signature"
or the "%G?" pretty format and parsing the output, which is well suited
for user inspection, but not for scripting.
Provide a command "verify-commit" which is analogous to "verify-tag": It
returns 0 for good signatures and non-zero otherwise, has the gpg output
on stderr and (optionally) the commit object on stdout, sans the
signature, just like "verify-tag" does.
Signed-off-by: Michael J Gruber <redacted>
---
Documentation/git-verify-commit.txt | 28 +++++++++++
Makefile | 1 +
builtin.h | 1 +
builtin/verify-commit.c | 98 +++++++++++++++++++++++++++++++++++++
command-list.txt | 1 +
git.c | 1 +
6 files changed, 130 insertions(+)
create mode 100644 Documentation/git-verify-commit.txt
create mode 100644 builtin/verify-commit.c
@@ -0,0 +1,28 @@+git-verify-commit(1)+=================++NAME+----+git-verify-commit - Check the GPG signature of commits++SYNOPSIS+--------+[verse]+'git verify-commit' <commit>...++DESCRIPTION+-----------+Validates the gpg signature created by 'git commit -S'.++OPTIONS+-------+-v::+--verbose::+ Print the contents of the commit object before validating it.++<commit>...::+ SHA-1 identifiers of Git commit objects.++GIT+---+Part of the linkgit:git[1] suite
@@ -0,0 +1,98 @@+/*+*Builtin"git commit-commit"+*+*Copyright(c)2014MichaelJGruber<git@drmicha.warpmail.net>+*+*Basedongit-verify-tag+*/+#include"cache.h"+#include"builtin.h"+#include"commit.h"+#include"run-command.h"+#include<signal.h>+#include"parse-options.h"+#include"gpg-interface.h"++staticconstchar*constverify_commit_usage[]={+N_("git verify-commit [-v|--verbose] <commit>..."),+NULL+};++staticintrun_gpg_verify(constunsignedchar*sha1,constchar*buf,unsignedlongsize,intverbose)+{+structsignature_checksignature_check;++memset(&signature_check,0,sizeof(signature_check));++check_commit_signature(lookup_commit(sha1),&signature_check);++if(verbose&&signature_check.payload)+fputs(signature_check.payload,stdout);++if(signature_check.gpg_output)+fputs(signature_check.gpg_output,stderr);++free(signature_check.gpg_output);+free(signature_check.gpg_status);+free(signature_check.signer);+free(signature_check.key);+returnsignature_check.result!='G';+}++staticintverify_commit(constchar*name,intverbose)+{+enumobject_typetype;+unsignedcharsha1[20];+char*buf;+unsignedlongsize;+intret;++if(get_sha1(name,sha1))+returnerror("commit '%s' not found.",name);++type=sha1_object_info(sha1,NULL);+if(type!=OBJ_COMMIT)+returnerror("%s: cannot verify a non-commit object of type %s.",+name,typename(type));++buf=read_sha1_file(sha1,&type,&size);+if(!buf)+returnerror("%s: unable to read file.",name);++ret=run_gpg_verify(sha1,buf,size,verbose);++free(buf);+returnret;+}++staticintgit_verify_commit_config(constchar*var,constchar*value,void*cb)+{+intstatus=git_gpg_config(var,value,cb);+if(status)+returnstatus;+returngit_default_config(var,value,cb);+}++intcmd_verify_commit(intargc,constchar**argv,constchar*prefix)+{+inti=1,verbose=0,had_error=0;+conststructoptionverify_commit_options[]={+OPT__VERBOSE(&verbose,N_("print commit contents")),+OPT_END()+};++git_config(git_verify_commit_config,NULL);++argc=parse_options(argc,argv,prefix,verify_commit_options,+verify_commit_usage,PARSE_OPT_KEEP_ARGV0);+if(argc<=i)+usage_with_options(verify_commit_usage,verify_commit_options);++/* sometimes the program was terminated because this signal+*wasreceivedintheprocessofwritingthegpginput:*/+signal(SIGPIPE,SIG_IGN);+while(i<argc)+if(verify_commit(argv[i++],verbose))+had_error=1;+returnhad_error;+}
From: Michael J Gruber <hidden> Date: 2016-06-15 23:01:31
In contrast to tag signatures, commit signatures are put into the
header, that is between the other header parts and commit messages.
Provide access to the commit content sans the signature, which is the
payload that is actually signed. Commit signature verification does the
parsing anyways, and callers may wish to act on or display the commit
object sans the signature.
Signed-off-by: Michael J Gruber <redacted>
---
builtin/merge.c | 1 +
commit.c | 1 +
gpg-interface.h | 1 +
pretty.c | 1 +
4 files changed, 4 insertions(+)
@@ -1282,6 +1282,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)printf(_("Commit %s has a good GPG signature by %s\n"),hex,signature_check.signer);+free(signature_check.payload);free(signature_check.gpg_output);free(signature_check.gpg_status);free(signature_check.signer);
From: Michael J Gruber <hidden> Date: 2016-06-15 23:01:35
On 6. Juni 2014 16:15:28 MESZ, Michael J Gruber [off-list ref] wrote:
quoted hunk
Commit signatures can be verified using "git show -s --show-signature"
or the "%G?" pretty format and parsing the output, which is well suited
for user inspection, but not for scripting.
Provide a command "verify-commit" which is analogous to "verify-tag":
It
returns 0 for good signatures and non-zero otherwise, has the gpg
output
on stderr and (optionally) the commit object on stdout, sans the
signature, just like "verify-tag" does.
Signed-off-by: Michael J Gruber <redacted>
---
Documentation/git-verify-commit.txt | 28 +++++++++++
Makefile | 1 +
builtin.h | 1 +
builtin/verify-commit.c | 98
+++++++++++++++++++++++++++++++++++++
command-list.txt | 1 +
git.c | 1 +
6 files changed, 130 insertions(+)
create mode 100644 Documentation/git-verify-commit.txt
create mode 100644 builtin/verify-commit.c
diff --git a/Documentation/git-verify-commit.txt
b/Documentation/git-verify-commit.txt
new file mode 100644
index 0000000..dcd7803
That line will need 3 more "=" thanks to asciidoc stubbornness.
v2 plus the tests coming tomorrow when my dev box is online again.
quoted hunk
+
+NAME
+----
+git-verify-commit - Check the GPG signature of commits
+
+SYNOPSIS
+--------
+[verse]
+'git verify-commit' <commit>...
+
+DESCRIPTION
+-----------
+Validates the gpg signature created by 'git commit -S'.
+
+OPTIONS
+-------
+-v::
+--verbose::
+ Print the contents of the commit object before validating it.
+
+<commit>...::
+ SHA-1 identifiers of Git commit objects.
+
+GIT
+---
+Part of the linkgit:git[1] suite
From: Jeff King <hidden> Date: 2016-06-15 23:01:36
On Fri, Jun 06, 2014 at 04:15:28PM +0200, Michael J Gruber wrote:
Commit signatures can be verified using "git show -s --show-signature"
or the "%G?" pretty format and parsing the output, which is well suited
for user inspection, but not for scripting.
Provide a command "verify-commit" which is analogous to "verify-tag": It
returns 0 for good signatures and non-zero otherwise, has the gpg output
on stderr and (optionally) the commit object on stdout, sans the
signature, just like "verify-tag" does.
Signed-off-by: Michael J Gruber <redacted>
I think the general direction of this series is reasonable.
Did you give any thought to just having a "git verify" command, instead
of separate tag/verify commands?
Another thought, that may be orthogonal to your series: what does it
mean to verify a commit? We check for _some_ signature from a key that
is in your keyring. But we do not check whether the signature matches
the committer field (or for tags, the tagger field). You have to parse
the gpg output, run "git cat-file", and then correlate the two. Should
there be an option to have git check that one of the signed uids from
gpg matches the commit's committer?
-Peff
Perhaps this is a sign that we need a "signature_check_clear()" helper?
... or simply switch to language which has (or can overload) free for an
object :)
Do we have prior art for such helpers so that the new one would be
analogous?
Michael
From: Michael J Gruber <hidden> Date: 2016-06-15 23:01:36
Jeff King venit, vidit, dixit 13.06.2014 10:02:
On Fri, Jun 06, 2014 at 04:15:28PM +0200, Michael J Gruber wrote:
quoted
Commit signatures can be verified using "git show -s --show-signature"
or the "%G?" pretty format and parsing the output, which is well suited
for user inspection, but not for scripting.
Provide a command "verify-commit" which is analogous to "verify-tag": It
returns 0 for good signatures and non-zero otherwise, has the gpg output
on stderr and (optionally) the commit object on stdout, sans the
signature, just like "verify-tag" does.
Signed-off-by: Michael J Gruber <redacted>
I think the general direction of this series is reasonable.
Did you give any thought to just having a "git verify" command, instead
of separate tag/verify commands?
Yes. (mathematician's answer)
You know not only the outcome but also why I refrained from doing so:
compatibility. We would need to deprecate verify-tag.
But there is also a more subtle reason: If you want to verify a signed
commit, you want to be sure that it actually is a commit. "verify" could
easily branch code paths based on the object type, but I'm not sure that
is desirable, at least not by default.
Another thought, that may be orthogonal to your series: what does it
mean to verify a commit? We check for _some_ signature from a key that
is in your keyring. But we do not check whether the signature matches
the committer field (or for tags, the tagger field). You have to parse
the gpg output, run "git cat-file", and then correlate the two. Should
there be an option to have git check that one of the signed uids from
gpg matches the commit's committer?
-Peff
That is a general issue with verifying signatures: it can be automated
only if you employ a strict trust model and a very limited keyring.
"valid signature" means only as much as the signatures that your gpg
accepts can be really trusted.
Comparing uid's really buys you nothing in the sense that everyone can
have a key with uid "Jeff King [off-list ref] signed by some other
keys. On the other hand, it's perfectly OK to use different uids for git
commits and signatures. The e-mail address I use for the git list and
commits, for example, is clearly a "plus address", which helps me
organize things; my personal key has the primary address as uid.
I really think all this is up to local policies for individual use cases.
Michael
From: Jeff King <hidden> Date: 2016-06-15 23:01:36
On Fri, Jun 13, 2014 at 11:44:28AM +0200, Michael J Gruber wrote:
quoted
Perhaps this is a sign that we need a "signature_check_clear()" helper?
... or simply switch to language which has (or can overload) free for an
object :)
I hear somebody has reimplemented git in pure javascript. ;P
Do we have prior art for such helpers so that the new one would be
analogous?
I was thinking of credential_clear, string_list_clear, etc. Literally
just:
void signature_check_clear(struct signature_check *s)
{
free(s->gpg_output);
free(s->gpg_status);
free(s->signer);
free(s->key);
}
Your first commit fixed a leak on gpg_status. Did it also need to handle
the "key" field there?
For some structs, we'd also do:
memset(s, 0, sizeof(*s));
to get us back to a usable, initialized state so the struct can be
reused. However, check_commit_signature doesn't care if the struct is
initialized or not (i.e., there is no initialized state). Doing so does
help detect use-after-free conditions, though.
-Peff
From: Michael J Gruber <hidden> Date: 2016-06-15 23:01:36
This mixes the "git verify-commit" tests in with the "git show
--show-signature" tests, to keep the tests more readable.
The tests already mix in the "call show" tests with the "verify" tests.
So in case of a test beakage, a '-v' run would be needed to reveal the
exact point of breakage anyway.
Additionally, test the actual output of "git verify-commit" and "git
show --show-signature" and compare to "git cat-file".
Signed-off-by: Michael J Gruber <redacted>
---
t/t7510-signed-commit.sh | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
From: Michael J Gruber <hidden> Date: 2016-06-15 23:01:36
4a868fd (pretty: parse the gpg status lines rather than the output, 2013-02-14)
made the gpg status lines available to callers and made sure they freed
the used space, but missed one spot.
Free the status line buffer also in the remaining spot.
Signed-off-by: Michael J Gruber <redacted>
---
pretty.c | 1 +
1 file changed, 1 insertion(+)
From: Michael J Gruber <hidden> Date: 2016-06-15 23:01:36
In contrast to tag signatures, commit signatures are put into the
header, that is between the other header parts and commit messages.
Provide access to the commit content sans the signature, which is the
payload that is actually signed. Commit signature verification does the
parsing anyways, and callers may wish to act on or display the commit
object sans the signature.
Signed-off-by: Michael J Gruber <redacted>
---
builtin/merge.c | 1 +
commit.c | 1 +
gpg-interface.h | 1 +
pretty.c | 1 +
4 files changed, 4 insertions(+)
@@ -1282,6 +1282,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)printf(_("Commit %s has a good GPG signature by %s\n"),hex,signature_check.signer);+free(signature_check.payload);free(signature_check.gpg_output);free(signature_check.gpg_status);free(signature_check.signer);
From: Michael J Gruber <hidden> Date: 2016-06-15 23:01:36
When t7510 was introduced, the author made sure that a for loop in
a subshell would return with the appropriate error code.
Make sure this is true also the for the first line in each loop, which
was missed.
Signed-off-by: Michael J Gruber <redacted>
---
t/t7510-signed-commit.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Michael J Gruber <hidden> Date: 2016-06-15 23:01:36
The struct has been growing members whose malloced memory needs to be
freed. Do this with one helper function so that no malloced memory shall
be left unfreed.
Signed-off-by: Michael J Gruber <redacted>
---
builtin/merge.c | 6 +-----
builtin/verify-commit.c | 5 +----
gpg-interface.c | 14 ++++++++++++++
gpg-interface.h | 1 +
pretty.c | 5 +----
5 files changed, 18 insertions(+), 13 deletions(-)
@@ -1282,11 +1282,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)printf(_("Commit %s has a good GPG signature by %s\n"),hex,signature_check.signer);-free(signature_check.payload);-free(signature_check.gpg_output);-free(signature_check.gpg_status);-free(signature_check.signer);-free(signature_check.key);+signature_check_clear(&signature_check);}}
From: Michael J Gruber <hidden> Date: 2016-06-15 23:01:36
Commit signatures can be verified using "git show -s --show-signature"
or the "%G?" pretty format and parsing the output, which is well suited
for user inspection, but not for scripting.
Provide a command "verify-commit" which is analogous to "verify-tag": It
returns 0 for good signatures and non-zero otherwise, has the gpg output
on stderr and (optionally) the commit object on stdout, sans the
signature, just like "verify-tag" does.
Signed-off-by: Michael J Gruber <redacted>
---
Documentation/git-verify-commit.txt | 28 +++++++++++
Makefile | 1 +
builtin.h | 1 +
builtin/verify-commit.c | 98 +++++++++++++++++++++++++++++++++++++
command-list.txt | 1 +
git.c | 1 +
6 files changed, 130 insertions(+)
create mode 100644 Documentation/git-verify-commit.txt
create mode 100644 builtin/verify-commit.c
@@ -0,0 +1,28 @@+git-verify-commit(1)+====================++NAME+----+git-verify-commit - Check the GPG signature of commits++SYNOPSIS+--------+[verse]+'git verify-commit' <commit>...++DESCRIPTION+-----------+Validates the gpg signature created by 'git commit -S'.++OPTIONS+-------+-v::+--verbose::+ Print the contents of the commit object before validating it.++<commit>...::+ SHA-1 identifiers of Git commit objects.++GIT+---+Part of the linkgit:git[1] suite
@@ -0,0 +1,98 @@+/*+*Builtin"git commit-commit"+*+*Copyright(c)2014MichaelJGruber<git@drmicha.warpmail.net>+*+*Basedongit-verify-tag+*/+#include"cache.h"+#include"builtin.h"+#include"commit.h"+#include"run-command.h"+#include<signal.h>+#include"parse-options.h"+#include"gpg-interface.h"++staticconstchar*constverify_commit_usage[]={+N_("git verify-commit [-v|--verbose] <commit>..."),+NULL+};++staticintrun_gpg_verify(constunsignedchar*sha1,constchar*buf,unsignedlongsize,intverbose)+{+structsignature_checksignature_check;++memset(&signature_check,0,sizeof(signature_check));++check_commit_signature(lookup_commit(sha1),&signature_check);++if(verbose&&signature_check.payload)+fputs(signature_check.payload,stdout);++if(signature_check.gpg_output)+fputs(signature_check.gpg_output,stderr);++free(signature_check.gpg_output);+free(signature_check.gpg_status);+free(signature_check.signer);+free(signature_check.key);+returnsignature_check.result!='G';+}++staticintverify_commit(constchar*name,intverbose)+{+enumobject_typetype;+unsignedcharsha1[20];+char*buf;+unsignedlongsize;+intret;++if(get_sha1(name,sha1))+returnerror("commit '%s' not found.",name);++type=sha1_object_info(sha1,NULL);+if(type!=OBJ_COMMIT)+returnerror("%s: cannot verify a non-commit object of type %s.",+name,typename(type));++buf=read_sha1_file(sha1,&type,&size);+if(!buf)+returnerror("%s: unable to read file.",name);++ret=run_gpg_verify(sha1,buf,size,verbose);++free(buf);+returnret;+}++staticintgit_verify_commit_config(constchar*var,constchar*value,void*cb)+{+intstatus=git_gpg_config(var,value,cb);+if(status)+returnstatus;+returngit_default_config(var,value,cb);+}++intcmd_verify_commit(intargc,constchar**argv,constchar*prefix)+{+inti=1,verbose=0,had_error=0;+conststructoptionverify_commit_options[]={+OPT__VERBOSE(&verbose,N_("print commit contents")),+OPT_END()+};++git_config(git_verify_commit_config,NULL);++argc=parse_options(argc,argv,prefix,verify_commit_options,+verify_commit_usage,PARSE_OPT_KEEP_ARGV0);+if(argc<=i)+usage_with_options(verify_commit_usage,verify_commit_options);++/* sometimes the program was terminated because this signal+*wasreceivedintheprocessofwritingthegpginput:*/+signal(SIGPIPE,SIG_IGN);+while(i<argc)+if(verify_commit(argv[i++],verbose))+had_error=1;+returnhad_error;+}
From: Jeff King <hidden> Date: 2016-06-15 23:01:36
On Fri, Jun 13, 2014 at 11:55:22AM +0200, Michael J Gruber wrote:
quoted
Did you give any thought to just having a "git verify" command, instead
of separate tag/verify commands?
Yes. (mathematician's answer)
Cute.
You know not only the outcome but also why I refrained from doing so:
compatibility. We would need to deprecate verify-tag.
Yes, we'd certainly leave verify-tag in place for compatibility. I don't
think that makes "git verify" a bad idea necessarily, if it is a better
interface. But...
But there is also a more subtle reason: If you want to verify a signed
commit, you want to be sure that it actually is a commit. "verify" could
easily branch code paths based on the object type, but I'm not sure that
is desirable, at least not by default.
Yes, I wasn't sure about that part. I think it really depends on what
people want to use it for. I was thinking more of a porcelain, anyway,
to just check whatever signatures are available. But I don't really sign
my commits right now anyway, so I'm somewhat guessing.
Even if we decide to do something like that, I suppose having
verify-commit isn't the end of the world. It could just remain the
plumbing interface, as verify-tag would. So I don't think my half-formed
thoughts are any reason to block your series.
That is a general issue with verifying signatures: it can be automated
only if you employ a strict trust model and a very limited keyring.
"valid signature" means only as much as the signatures that your gpg
accepts can be really trusted.
Comparing uid's really buys you nothing in the sense that everyone can
have a key with uid "Jeff King [off-list ref] signed by some other
keys.
Sort of. The crypto proves that the commit was signed by a particular
key, and then there are two mappings:
1. What is the identity associated with that key?
2. Is that identity somebody who "should" have signed the commit?
We assume that GPG takes care of the first one with the web of trust.
Even if you have the key and can check the signature, it will still
complain about an untrusted uid (and we reflect that with 'U' in the gpg
status). There is no point in looking at step 2 if step 1 did not check
out.
For the second one, I think it really depends on the project workflow,
and you may even want multiple policies within a project (e.g., perhaps
only some uids can merge to master). But one obvious check we can make
is "does the identity in the commit data match one of the uids?". True,
you don't _need_ that; you can always just use "%GS", and throw away the
committer and author headers. But we show those names in lots of output.
You could, for example, "verify" that each commit's author id matches
its gpg signature, and then use the regular tools to do further work
(e.g., running "git blame"), and be confident that the author you see
matches the signature.
This should definitely be optional. Even something as simple as "author
id matches the gpg key" would not always work (for example, in git.git
we pick patches from the list, which means only Junio can sign the
objects, but he is not the author). But just because it is optional does
not mean it would not be a useful tool for some workflows.
On the other hand, it's perfectly OK to use different uids for git
commits and signatures. The e-mail address I use for the git list and
commits, for example, is clearly a "plus address", which helps me
organize things; my personal key has the primary address as uid.
I really think all this is up to local policies for individual use cases.
Very much agreed. My suggestion was more about providing tools for
people to build those policies.
I realize this isn't really your itch to scratch. It's just that when I
see a description like "verify a commit", I wonder what exactly "verify"
means.
-Peff
+ type = sha1_object_info(sha1, NULL);
+ if (type != OBJ_COMMIT)
+ return error("%s: cannot verify a non-commit object of type %s.",
+ name, typename(type));
+
+ buf = read_sha1_file(sha1, &type, &size);
+ if (!buf)
+ return error("%s: unable to read file.", name);
I think you can drop the sha1_object_info call and just check "type"
from read_sha1_file (they _should_ agree, but if they do not, I'd rather
pay attention to the one that came along with the buffer). And this is
the uncommon error path, so expanding the object into memory before we
die is not a big deal.
Should this peel to a commit if given a tag? I'd say probably. I know
you raised the issue elsewhere of keeping things simple, but I think if
you are calling verify-commit, you know you want a commit, and we should
treat the argument as a commit-ish. Anyway, if you go that route, then
lookup_commit_or_die is probably what you want.
Also, minor nit, but we typically do not end the error messages with a
full stop (we've been rather inconsistent in the past, but these days
seem to mostly settle on no punctuation).
-Peff
From: Jeff King <hidden> Date: 2016-06-15 23:01:36
On Fri, Jun 13, 2014 at 12:42:43PM +0200, Michael J Gruber wrote:
quoted hunk
4a868fd (pretty: parse the gpg status lines rather than the output, 2013-02-14)
made the gpg status lines available to callers and made sure they freed
the used space, but missed one spot.
Free the status line buffer also in the remaining spot.
Signed-off-by: Michael J Gruber <redacted>
---
pretty.c | 1 +
1 file changed, 1 insertion(+)
What about .key?
I would have expected your patch 6 to come first, which would fix this,
and then save you from making similar mistakes in patch 3. :)
-Peff
I sneekily fix this in 6/6... I thought 3/6 is on next already, too late
for a real v2. Otherwise I would put 6/6 before everything else.
quoted
+ type = sha1_object_info(sha1, NULL);
+ if (type != OBJ_COMMIT)
+ return error("%s: cannot verify a non-commit object of type %s.",
+ name, typename(type));
+
+ buf = read_sha1_file(sha1, &type, &size);
+ if (!buf)
+ return error("%s: unable to read file.", name);
I think you can drop the sha1_object_info call and just check "type"
from read_sha1_file (they _should_ agree, but if they do not, I'd rather
pay attention to the one that came along with the buffer). And this is
the uncommon error path, so expanding the object into memory before we
die is not a big deal.
Should this peel to a commit if given a tag? I'd say probably. I know
you raised the issue elsewhere of keeping things simple, but I think if
you are calling verify-commit, you know you want a commit, and we should
treat the argument as a commit-ish. Anyway, if you go that route, then
lookup_commit_or_die is probably what you want.
Also, minor nit, but we typically do not end the error messages with a
full stop (we've been rather inconsistent in the past, but these days
seem to mostly settle on no punctuation).
-Peff
Both of these issues actually come for following verify-tag as closely
as possible. If 3 is not applied already, I should do away with
sha1_object_info.
About the peeling I'm not so sure, since there's a difference between a
signed tag pointing to a commit and a signed commit. Since
verify-{tag,commit} are bare metal plumbing, I would expect callers to
use <rev>^{commit} explicitly if they don't care how <rev> peels to a
commit.
Michael
From: Jeff King <hidden> Date: 2016-06-15 23:01:36
On Fri, Jun 13, 2014 at 12:42:46PM +0200, Michael J Gruber wrote:
quoted hunk
When t7510 was introduced, the author made sure that a for loop in
a subshell would return with the appropriate error code.
Make sure this is true also the for the first line in each loop, which
was missed.
Signed-off-by: Michael J Gruber <redacted>
---
t/t7510-signed-commit.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Hrm. The original is:
X &&
Y || exit 1
Won't that still exit (i.e., it is already correct)? Doing:
for X in true false; do
for Y in true false; do
($X && $Y || exit 1)
echo "$X/$Y: $?"
done
done
yields:
true/true: 0
true/false: 1
false/true: 1
false/false: 1
(and should still short-circuit Y, because we go from left-to-right).
I do not mind changing it to keep the style of each line consistent,
though. I would have written it as a series of "&&"-chains, with a
single exit at the end, but I think that is just a matter of preference.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 23:01:36
On Fri, Jun 13, 2014 at 01:45:58PM +0200, Michael J Gruber wrote:
I sneekily fix this in 6/6... I thought 3/6 is on next already, too late
for a real v2. Otherwise I would put 6/6 before everything else.
Ah, yeah, I assumed we were still re-rolling (and it looks like you're
just on pu so far).
About the peeling I'm not so sure, since there's a difference between a
signed tag pointing to a commit and a signed commit.
There is, but "verify-commit" is always going to verify the commit, no?
Not peeling will always result in an error, and never do anything
useful.
I admit it's probably not going to come up too often, though. And I
don't have any argument beyond "it makes sense to me", so I won't push
for it further.
-Peff
Please use test_must_fail here (and further down), which will catch
things like signal death.
Again, that is an issue of keeping the style of the surrounding code
(which is relatively new) vs. doing it differently. I don't mind
changing t7510 to a different style, though.
Michael
From: Michael J Gruber <hidden> Date: 2016-06-15 23:01:36
Jeff King venit, vidit, dixit 13.06.2014 13:46:
On Fri, Jun 13, 2014 at 12:42:46PM +0200, Michael J Gruber wrote:
quoted
When t7510 was introduced, the author made sure that a for loop in
a subshell would return with the appropriate error code.
Make sure this is true also the for the first line in each loop, which
was missed.
Signed-off-by: Michael J Gruber <redacted>
---
t/t7510-signed-commit.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Hrm. The original is:
X &&
Y || exit 1
Won't that still exit (i.e., it is already correct)? Doing:
for X in true false; do
for Y in true false; do
($X && $Y || exit 1)
echo "$X/$Y: $?"
done
done
yields:
true/true: 0
true/false: 1
false/true: 1
false/false: 1
(and should still short-circuit Y, because we go from left-to-right).
I do not mind changing it to keep the style of each line consistent,
though. I would have written it as a series of "&&"-chains, with a
single exit at the end, but I think that is just a matter of preference.
If I remember correctly, I put something failing on the first line of
the original version, and the test succeeded. I think the point is that
we have a for loop in a subshell, and we need to make sure that the
false of one iteration is not overwritten by the true of the next one -
"exit 1" makes sure to "break" the for loop and exit the subshell.
(The chain should do that as well, I'll recheck.)
Michael
From: Michael J Gruber <hidden> Date: 2016-06-15 23:01:36
Jeff King venit, vidit, dixit 13.06.2014 13:50:
On Fri, Jun 13, 2014 at 01:45:58PM +0200, Michael J Gruber wrote:
quoted
I sneekily fix this in 6/6... I thought 3/6 is on next already, too late
for a real v2. Otherwise I would put 6/6 before everything else.
Ah, yeah, I assumed we were still re-rolling (and it looks like you're
just on pu so far).
We are, I had misread a "What's cooking". So a reroll it is.
quoted
About the peeling I'm not so sure, since there's a difference between a
signed tag pointing to a commit and a signed commit.
There is, but "verify-commit" is always going to verify the commit, no?
Not peeling will always result in an error, and never do anything
useful.
I admit it's probably not going to come up too often, though. And I
don't have any argument beyond "it makes sense to me", so I won't push
for it further.
-Peff
I guess it boils down to the fact how plumberish it's supposed to be.
Since it's about verification, for some definition of "verify", I'd
rather apply as few automatisms as possible. If the caller wants to know
whether commit deadbeef carries a valid commit signature I'd rather
check that very object.
I also picture doing the "git-verify" thing in the future (with
"--commit" and "--tag" options which make the command insist on the
object type), and then we would not want to peel tags under the hood.
Michael
From: Michael J Gruber <hidden> Date: 2016-06-15 23:01:37
Michael J Gruber venit, vidit, dixit 13.06.2014 14:04:
Jeff King venit, vidit, dixit 13.06.2014 13:46:
quoted
On Fri, Jun 13, 2014 at 12:42:46PM +0200, Michael J Gruber wrote:
quoted
When t7510 was introduced, the author made sure that a for loop in
a subshell would return with the appropriate error code.
Make sure this is true also the for the first line in each loop, which
was missed.
Signed-off-by: Michael J Gruber <redacted>
---
t/t7510-signed-commit.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Hrm. The original is:
X &&
Y || exit 1
Won't that still exit (i.e., it is already correct)? Doing:
for X in true false; do
for Y in true false; do
($X && $Y || exit 1)
echo "$X/$Y: $?"
done
done
yields:
true/true: 0
true/false: 1
false/true: 1
false/false: 1
(and should still short-circuit Y, because we go from left-to-right).
I do not mind changing it to keep the style of each line consistent,
though. I would have written it as a series of "&&"-chains, with a
single exit at the end, but I think that is just a matter of preference.
If I remember correctly, I put something failing on the first line of
the original version, and the test succeeded. I think the point is that
we have a for loop in a subshell, and we need to make sure that the
false of one iteration is not overwritten by the true of the next one -
"exit 1" makes sure to "break" the for loop and exit the subshell.
(The chain should do that as well, I'll recheck.)
... the chain does not, which is the point :)
With X && Y || exit 1 inside the loop, the loop statement will return
false, but the loop will continue (if X returns false), which is exactly
the problem that the exit avoids.
Make your example iterate over false true instead in the inner loop and
you'll see ;)
Michael
From: Michael J Gruber <hidden> Date: 2016-06-15 23:01:37
Michael J Gruber venit, vidit, dixit 13.06.2014 14:22:
Michael J Gruber venit, vidit, dixit 13.06.2014 14:04:
quoted
Jeff King venit, vidit, dixit 13.06.2014 13:46:
quoted
On Fri, Jun 13, 2014 at 12:42:46PM +0200, Michael J Gruber wrote:
quoted
When t7510 was introduced, the author made sure that a for loop in
a subshell would return with the appropriate error code.
Make sure this is true also the for the first line in each loop, which
was missed.
Signed-off-by: Michael J Gruber <redacted>
---
t/t7510-signed-commit.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Hrm. The original is:
X &&
Y || exit 1
Won't that still exit (i.e., it is already correct)? Doing:
for X in true false; do
for Y in true false; do
($X && $Y || exit 1)
echo "$X/$Y: $?"
done
done
yields:
true/true: 0
true/false: 1
false/true: 1
false/false: 1
(and should still short-circuit Y, because we go from left-to-right).
I do not mind changing it to keep the style of each line consistent,
though. I would have written it as a series of "&&"-chains, with a
single exit at the end, but I think that is just a matter of preference.
If I remember correctly, I put something failing on the first line of
the original version, and the test succeeded. I think the point is that
we have a for loop in a subshell, and we need to make sure that the
false of one iteration is not overwritten by the true of the next one -
"exit 1" makes sure to "break" the for loop and exit the subshell.
(The chain should do that as well, I'll recheck.)
... the chain does not, which is the point :)
With X && Y || exit 1 inside the loop, the loop statement will return
false, but the loop will continue (if X returns false), which is exactly
the problem that the exit avoids.
Make your example iterate over false true instead in the inner loop and
you'll see ;)
Michael
... with this loop, sorry:
for X in true false; do
for Y in false true; do
($X && $Y || exit 1)
done
echo "$X/last inner $Y: $?"
done
gives
true/last inner true: 0
false/last inner true: 1
even though on both cases we have at least one failure of Y. (failure of
one subtest = failure of the test)
Looping in the other order:
for X in true false; do
for Y in true false; do
($X && $Y || exit 1)
done
echo "$X/last inner $Y: $?"
done
gives
true/last inner false: 1
false/last inner false: 1
as it should be.
From: Jeff King <hidden> Date: 2016-06-15 23:01:37
On Fri, Jun 13, 2014 at 02:33:02PM +0200, Michael J Gruber wrote:
quoted
With X && Y || exit 1 inside the loop, the loop statement will return
false, but the loop will continue (if X returns false), which is exactly
the problem that the exit avoids.
Make your example iterate over false true instead in the inner loop and
you'll see ;)
Michael
... with this loop, sorry:
for X in true false; do
for Y in false true; do
($X && $Y || exit 1)
done
echo "$X/last inner $Y: $?"
done
I'm somewhat confused, as my loops were meant only to expand the truth
table, not to simulate a real loop in the code. That is why I have a
subshell in the loop around my exit, to make sure we keep looping. In
the real code, the subshell surrounds the whole loop (so that an "exit"
leaves the entire loop without leaving the whole script).
The actual code is more like:
(
for i in a b c; do
echo $i: got to first step &&
test $i != b &&
echo $i: got to second step || exit 1
done
)
echo overall status: $?
which should fail on the second loop iteration. And it does:
a: got to first step
a: got to second step
b: got to first step
overall status: 1
That is, we short-circuit to the "exit 1" as soon as "test $i != b"
fails. You can replace the use of "$?" above with more "&&"-chaining, of
course.
-Peff
From: Johannes Sixt <hidden> Date: 2016-06-15 23:01:37
Am 6/13/2014 14:33, schrieb Michael J Gruber:
.... with this loop, sorry:
for X in true false; do
for Y in false true; do
($X && $Y || exit 1)
done
echo "$X/last inner $Y: $?"
done
gives
true/last inner true: 0
false/last inner true: 1
even though on both cases we have at least one failure of Y. (failure of
one subtest = failure of the test)
Place the loop(s) inside the subshell, and you observe termination on the
first failure, and a failure exit code of the subshell.
The change proposed in this patch should not be necessary. Something else
must be wrong with your tests.
Ah, here it is:
@@ -58,7 +58,7 @@ test_expect_success GPG 'show signatures' ' ( for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned do- git show --pretty=short --show-signature $commit >actual &&+ git show --pretty=short --show-signature $commit >actual || exit 1 grep "Good signature from" actual && exit 1 ! grep "BAD signature from" actual || exit 1 echo $commit OK
Notice the '&& exit 1'! It should be '|| exit 1', right?
-- Hannes
From: Michael J Gruber <hidden> Date: 2016-06-15 23:01:37
Johannes Sixt venit, vidit, dixit 13.06.2014 14:54:
Am 6/13/2014 14:33, schrieb Michael J Gruber:
quoted
.... with this loop, sorry:
for X in true false; do
for Y in false true; do
($X && $Y || exit 1)
done
echo "$X/last inner $Y: $?"
done
gives
true/last inner true: 0
false/last inner true: 1
even though on both cases we have at least one failure of Y. (failure of
one subtest = failure of the test)
Place the loop(s) inside the subshell, and you observe termination on the
first failure, and a failure exit code of the subshell.
The change proposed in this patch should not be necessary. Something else
must be wrong with your tests.
I know I started this (or Jeff did, who knows ;) ), but we keep
confusing each other more and more:
quoted hunk
Ah, here it is:
@@ -58,7 +58,7 @@ test_expect_success GPG 'show signatures' ' ( for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned do- git show --pretty=short --show-signature $commit >actual &&+ git show --pretty=short --show-signature $commit >actual || exit 1 grep "Good signature from" actual && exit 1
This is as in the original, it tests invalid signatures, so "Good
signature" should not be in the response.
! grep "BAD signature from" actual || exit 1
echo $commit OK
Notice the '&& exit 1'! It should be '|| exit 1', right?
-- Hannes
In other words, the original tests already had
grep foo && exit 1
! grep bar || exit 1
to test that we have neither foo nor bar. The reason is (supposedly) to
have this portion of the test mostly analogous to the previous one,
where we want foo and do want bar.
So this is completely unrelated.
Otoh, it seems the original test could have had
a &&
b &&
c || exit 1
or
a || exit 1
b || exit 1
c || exit 1
rather than
a &&
b || exit 1
c || exit 1
which I thought was incorrect (but I can't recreate the proof right
now). I'd say both of the former versions are preferable to the last one
unless there is a difference that neither Jeff nor I see.
I need a break before looking at this again ;)
Michael
From: Johannes Sixt <hidden> Date: 2016-06-15 23:01:37
Am 6/13/2014 15:06, schrieb Michael J Gruber:
Johannes Sixt venit, vidit, dixit 13.06.2014 14:54:
quoted
Am 6/13/2014 14:33, schrieb Michael J Gruber:
quoted
.... with this loop, sorry:
for X in true false; do
for Y in false true; do
($X && $Y || exit 1)
done
echo "$X/last inner $Y: $?"
done
gives
true/last inner true: 0
false/last inner true: 1
even though on both cases we have at least one failure of Y. (failure of
one subtest = failure of the test)
Place the loop(s) inside the subshell, and you observe termination on the
first failure, and a failure exit code of the subshell.
The change proposed in this patch should not be necessary. Something else
must be wrong with your tests.
I know I started this (or Jeff did, who knows ;) ), but we keep
confusing each other more and more:
quoted
Ah, here it is:
@@ -58,7 +58,7 @@ test_expect_success GPG 'show signatures' ' ( for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned do- git show --pretty=short --show-signature $commit >actual &&+ git show --pretty=short --show-signature $commit >actual || exit 1 grep "Good signature from" actual && exit 1
This is as in the original, it tests invalid signatures, so "Good
signature" should not be in the response.
quoted
! grep "BAD signature from" actual || exit 1
echo $commit OK
Notice the '&& exit 1'! It should be '|| exit 1', right?
-- Hannes
In other words, the original tests already had
grep foo && exit 1
! grep bar || exit 1
to test that we have neither foo nor bar. The reason is (supposedly) to
have this portion of the test mostly analogous to the previous one,
where we want foo and do want bar.
So this is completely unrelated.
I don't think so. What is the outcome of
false && # simulate a regression
grep foo && exit 1
! grep bar || exit 1
assuming that the '! grep bar' happens to be true? Answer: The regression
is not diagnosed because the &&-chain is broken.
*That* is what I think you described earlier in this thread as "I put
something failing on the first line of the original version, and the test
succeeded."
-- Hannes
From: Jeff King <hidden> Date: 2016-06-15 23:01:37
On Fri, Jun 13, 2014 at 03:21:55PM +0200, Johannes Sixt wrote:
I don't think so. What is the outcome of
false && # simulate a regression
grep foo && exit 1
! grep bar || exit 1
assuming that the '! grep bar' happens to be true? Answer: The regression
is not diagnosed because the &&-chain is broken.
*That* is what I think you described earlier in this thread as "I put
something failing on the first line of the original version, and the test
succeeded."
Yeah, I think that is the bit that I was missing from my original
confusion.
false &&
anything || exit 1
_does_ work. But that is not what is written. ;)
Thanks for pointing it out.
-Peff
From: Michael J Gruber <hidden> Date: 2016-06-15 23:01:37
Johannes Sixt venit, vidit, dixit 13.06.2014 15:21:
Am 6/13/2014 15:06, schrieb Michael J Gruber:
quoted
Johannes Sixt venit, vidit, dixit 13.06.2014 14:54:
quoted
Am 6/13/2014 14:33, schrieb Michael J Gruber:
quoted
.... with this loop, sorry:
for X in true false; do
for Y in false true; do
($X && $Y || exit 1)
done
echo "$X/last inner $Y: $?"
done
gives
true/last inner true: 0
false/last inner true: 1
even though on both cases we have at least one failure of Y. (failure of
one subtest = failure of the test)
Place the loop(s) inside the subshell, and you observe termination on the
first failure, and a failure exit code of the subshell.
The change proposed in this patch should not be necessary. Something else
must be wrong with your tests.
I know I started this (or Jeff did, who knows ;) ), but we keep
confusing each other more and more:
quoted
Ah, here it is:
@@ -58,7 +58,7 @@ test_expect_success GPG 'show signatures' ' ( for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned do- git show --pretty=short --show-signature $commit >actual &&+ git show --pretty=short --show-signature $commit >actual || exit 1 grep "Good signature from" actual && exit 1
This is as in the original, it tests invalid signatures, so "Good
signature" should not be in the response.
quoted
! grep "BAD signature from" actual || exit 1
echo $commit OK
Notice the '&& exit 1'! It should be '|| exit 1', right?
-- Hannes
In other words, the original tests already had
grep foo && exit 1
! grep bar || exit 1
to test that we have neither foo nor bar. The reason is (supposedly) to
have this portion of the test mostly analogous to the previous one,
where we want foo and do want bar.
So this is completely unrelated.
I don't think so. What is the outcome of
false && # simulate a regression
grep foo && exit 1
! grep bar || exit 1
assuming that the '! grep bar' happens to be true? Answer: The regression
is not diagnosed because the &&-chain is broken.
*That* is what I think you described earlier in this thread as "I put
something failing on the first line of the original version, and the test
succeeded."
-- Hannes
If you say that something I have said makes sense I'm happy, because I
can't confirm that myself right now. I'll take a break and look into a
rewrite of the form
a &&
b &&
test_must_fail c &&
d || exit 1
hoping that will make things both readable (by avoiding !) and concise
(by avoiding repeated exits).
Michael
From: Johannes Sixt <hidden> Date: 2016-06-15 23:01:37
Am 6/13/2014 15:31, schrieb Michael J Gruber:
rewrite of the form
a &&
b &&
test_must_fail c &&
d || exit 1
hoping that will make things both readable (by avoiding !) and concise
(by avoiding repeated exits).
Thanks!
Please note that we use 'test_must_fail' only for git invocations, but we
do write '!' in front of system commands that we expect to fail, e.g.,
'! grep'.
-- Hannes
From: Michael J Gruber <hidden> Date: 2016-06-15 23:01:43
This incorporates all remarks about the test coding guidelines and
rearranging the series.
Open questions:
- There was some debate about (optionally) verifying more than what
git-verify-{commit,tag} currently do, or going for a generic git-verify command.
The former would require both to be changed (in order to treat similar cases similarly),
the latter would need a deprecation for git-verify-tag.
- I haven't looked yet at what happened over the weekend.
Michael J Gruber (5):
gpg-interface: provide clear helper for struct signature_check
gpg-interface: provide access to the payload
verify-commit: scriptable commit signature verification
t7510: exit for loop with test result
t7510: test verify-commit
Documentation/git-verify-commit.txt | 28 +++++++++++
Makefile | 1 +
builtin.h | 1 +
builtin/merge.c | 5 +-
builtin/verify-commit.c | 93 +++++++++++++++++++++++++++++++++++++
command-list.txt | 1 +
commit.c | 1 +
git.c | 1 +
gpg-interface.c | 14 ++++++
gpg-interface.h | 2 +
pretty.c | 3 +-
t/t7510-signed-commit.sh | 24 ++++++++--
12 files changed, 165 insertions(+), 9 deletions(-)
create mode 100644 Documentation/git-verify-commit.txt
create mode 100644 builtin/verify-commit.c
--
2.0.0.426.g37dbf84
From: Michael J Gruber <hidden> Date: 2016-06-15 23:01:43
The struct has been growing members whose malloced memory needs to be
freed. Do this with one helper function so that no malloced memory shall
be left unfreed.
Signed-off-by: Michael J Gruber <redacted>
---
builtin/merge.c | 5 +----
gpg-interface.c | 12 ++++++++++++
gpg-interface.h | 1 +
pretty.c | 3 +--
4 files changed, 15 insertions(+), 6 deletions(-)
@@ -1282,10 +1282,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)printf(_("Commit %s has a good GPG signature by %s\n"),hex,signature_check.signer);-free(signature_check.gpg_output);-free(signature_check.gpg_status);-free(signature_check.signer);-free(signature_check.key);+signature_check_clear(&signature_check);}}
From: Michael J Gruber <hidden> Date: 2016-06-15 23:01:43
t7510 uses for loops in a subshell, which need to make sure that the test
returns with the appropriate error code from within the loop.
Restructure the loops as the usual && chains with a single point of
"exit 1" at the end of the loop to make this clearer.
Signed-off-by: Michael J Gruber <redacted>
---
t/t7510-signed-commit.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Michael J Gruber <hidden> Date: 2016-06-15 23:01:43
This mixes the "git verify-commit" tests in with the "git show
--show-signature" tests, to keep the tests more readable.
The tests already mix in the "call show" tests with the "verify" tests.
So in case of a test beakage, a '-v' run would be needed to reveal the
exact point of breakage anyway.
Additionally, test the actual output of "git verify-commit" and "git
show --show-signature" and compare to "git cat-file".
Signed-off-by: Michael J Gruber <redacted>
---
t/t7510-signed-commit.sh | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
From: Michael J Gruber <hidden> Date: 2016-06-15 23:01:43
In contrast to tag signatures, commit signatures are put into the
header, that is between the other header parts and commit messages.
Provide access to the commit content sans the signature, which is the
payload that is actually signed. Commit signature verification does the
parsing anyways, and callers may wish to act on or display the commit
object sans the signature.
Signed-off-by: Michael J Gruber <redacted>
---
commit.c | 1 +
gpg-interface.c | 2 ++
gpg-interface.h | 1 +
3 files changed, 4 insertions(+)
From: Michael J Gruber <hidden> Date: 2016-06-15 23:01:43
Commit signatures can be verified using "git show -s --show-signature"
or the "%G?" pretty format and parsing the output, which is well suited
for user inspection, but not for scripting.
Provide a command "verify-commit" which is analogous to "verify-tag": It
returns 0 for good signatures and non-zero otherwise, has the gpg output
on stderr and (optionally) the commit object on stdout, sans the
signature, just like "verify-tag" does.
Signed-off-by: Michael J Gruber <redacted>
---
Documentation/git-verify-commit.txt | 28 +++++++++++
Makefile | 1 +
builtin.h | 1 +
builtin/verify-commit.c | 93 +++++++++++++++++++++++++++++++++++++
command-list.txt | 1 +
git.c | 1 +
6 files changed, 125 insertions(+)
create mode 100644 Documentation/git-verify-commit.txt
create mode 100644 builtin/verify-commit.c
@@ -0,0 +1,28 @@+git-verify-commit(1)+====================++NAME+----+git-verify-commit - Check the GPG signature of commits++SYNOPSIS+--------+[verse]+'git verify-commit' <commit>...++DESCRIPTION+-----------+Validates the gpg signature created by 'git commit -S'.++OPTIONS+-------+-v::+--verbose::+ Print the contents of the commit object before validating it.++<commit>...::+ SHA-1 identifiers of Git commit objects.++GIT+---+Part of the linkgit:git[1] suite
@@ -0,0 +1,93 @@+/*+*Builtin"git commit-commit"+*+*Copyright(c)2014MichaelJGruber<git@drmicha.warpmail.net>+*+*Basedongit-verify-tag+*/+#include"cache.h"+#include"builtin.h"+#include"commit.h"+#include"run-command.h"+#include<signal.h>+#include"parse-options.h"+#include"gpg-interface.h"++staticconstchar*constverify_commit_usage[]={+N_("git verify-commit [-v|--verbose] <commit>..."),+NULL+};++staticintrun_gpg_verify(constunsignedchar*sha1,constchar*buf,unsignedlongsize,intverbose)+{+structsignature_checksignature_check;++memset(&signature_check,0,sizeof(signature_check));++check_commit_signature(lookup_commit(sha1),&signature_check);++if(verbose&&signature_check.payload)+fputs(signature_check.payload,stdout);++if(signature_check.gpg_output)+fputs(signature_check.gpg_output,stderr);++signature_check_clear(&signature_check);+returnsignature_check.result!='G';+}++staticintverify_commit(constchar*name,intverbose)+{+enumobject_typetype;+unsignedcharsha1[20];+char*buf;+unsignedlongsize;+intret;++if(get_sha1(name,sha1))+returnerror("commit '%s' not found.",name);++buf=read_sha1_file(sha1,&type,&size);+if(!buf)+returnerror("%s: unable to read file.",name);+if(type!=OBJ_COMMIT)+returnerror("%s: cannot verify a non-commit object of type %s.",+name,typename(type));++ret=run_gpg_verify(sha1,buf,size,verbose);++free(buf);+returnret;+}++staticintgit_verify_commit_config(constchar*var,constchar*value,void*cb)+{+intstatus=git_gpg_config(var,value,cb);+if(status)+returnstatus;+returngit_default_config(var,value,cb);+}++intcmd_verify_commit(intargc,constchar**argv,constchar*prefix)+{+inti=1,verbose=0,had_error=0;+conststructoptionverify_commit_options[]={+OPT__VERBOSE(&verbose,N_("print commit contents")),+OPT_END()+};++git_config(git_verify_commit_config,NULL);++argc=parse_options(argc,argv,prefix,verify_commit_options,+verify_commit_usage,PARSE_OPT_KEEP_ARGV0);+if(argc<=i)+usage_with_options(verify_commit_usage,verify_commit_options);++/* sometimes the program was terminated because this signal+*wasreceivedintheprocessofwritingthegpginput:*/+signal(SIGPIPE,SIG_IGN);+while(i<argc)+if(verify_commit(argv[i++],verbose))+had_error=1;+returnhad_error;+}
From: Jeff King <hidden> Date: 2016-06-15 23:01:44
On Mon, Jun 23, 2014 at 09:05:46AM +0200, Michael J Gruber wrote:
This incorporates all remarks about the test coding guidelines and
rearranging the series.
Open questions:
- There was some debate about (optionally) verifying more than what
git-verify-{commit,tag} currently do, or going for a generic git-verify command.
The former would require both to be changed (in order to treat similar cases similarly),
the latter would need a deprecation for git-verify-tag.
I think that a potential "git verify" doesn't need to block this series,
per the logic I gave elsewhere.
The one thing that does give me pause is that we do not seem to have any
way of accessing mergetag signatures. We should perhaps stop and think
for a second about how we might expose those (and whether it would fit
into the "git-verify-{commit,tag}" paradigm). I am tempted to say that
"git verify-tag" on a commit should verify the mergetag (right now it
would simply be an error). But I haven't though that hard on it.
I don't think implementation of that needs to be a blocker for this
series, but I'd rather see at least a vague plan so that we do not paint
ourselves into a corner.
Michael J Gruber (5):
gpg-interface: provide clear helper for struct signature_check
gpg-interface: provide access to the payload
verify-commit: scriptable commit signature verification
t7510: exit for loop with test result
t7510: test verify-commit
I didn't see anything objectionable here. I think you may want to rebase
on top of jk/pretty-G-format-fixes. That makes your patch 4 redundant,
and your patch 5 will probably need a few minor updates to match
cleanups in the surrounding code.
-Peff