[PATCH] rebase: add --verify-signatures

[PATCH] rebase: add --verify-signatures

[Alexander 'z33ky' Hirsch]

This option works analogous to --verify-signatures for git-merge by
checking that the commits, that are rebased onto, have good GPG
signatures.

Additionally, git-pull now forwards --verify-signatures to rebase as
well.

Signed-off-by: Alexander 'z33ky' Hirsch <1zeeky@gmail.com>
---

I'm unsure if the opt_verify_signatures check in builtin/pull.c should
be moved up to the "/* Shared options */" now.

The output strings from the GPG check are identical to the ones in
builtin/merge.c; I am unsure about the implications for l10n.

The test is mostly copied from t7612-merge-verify-signatures.sh.

 Documentation/git-rebase.txt        |  6 ++++
 builtin/pull.c                      |  2 ++
 git-rebase.sh                       | 44 +++++++++++++++++++++++++
 t/t3427-rebase-verify-signatures.sh | 65 +++++++++++++++++++++++++++++++++++++
 4 files changed, 117 insertions(+)
 create mode 100755 t/t3427-rebase-verify-signatures.sh

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 6cca8bb..959b12b 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -292,6 +292,12 @@ which makes little sense.
 	specified, `-s recursive`.  Note the reversal of 'ours' and
 	'theirs' as noted above for the `-m` option.
 
+--verify-signatures::
+--no-verify-signatures::
+	Verify that the commits in the branch the rebase is onto, but not
+	present in the working branch, have good GPG signatures and abort the
+	operation in case they do not.
+
 -S[<keyid>]::
 --gpg-sign[=<keyid>]::
 	GPG-sign commits. The `keyid` argument is optional and
diff --git a/builtin/pull.c b/builtin/pull.c
index bf3fd3f..37ec0f8 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -774,6 +774,8 @@ static int run_rebase(const unsigned char *curr_head,
 		argv_array_push(&args, "--preserve-merges");
 	if (opt_diffstat)
 		argv_array_push(&args, opt_diffstat);
+	if (opt_verify_signatures)
+		argv_array_push(&args, opt_verify_signatures);
 	argv_array_pushv(&args, opt_strategies.argv);
 	argv_array_pushv(&args, opt_strategy_opts.argv);
 	if (opt_gpg_sign)
diff --git a/git-rebase.sh b/git-rebase.sh
index af7ba5f..dcfbc3a 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -38,6 +38,7 @@ whitespace=!       passed to 'git apply'
 ignore-whitespace! passed to 'git apply'
 C=!                passed to 'git apply'
 S,gpg-sign?        GPG-sign commits
+verify-signatures  verify that the commits of onto have valid GPG signatures
  Actions:
 continue!          continue
 abort!             abort and check out the original branch
@@ -88,6 +89,7 @@ autosquash=
 keep_empty=
 test "$(git config --bool rebase.autosquash)" = "true" && autosquash=t
 gpg_sign_opt=
+verify_signatures=
 
 read_basic_state () {
 	test -f "$state_dir/head-name" &&
@@ -339,6 +341,12 @@ do
 	--gpg-sign=*)
 		gpg_sign_opt="-S${1#--gpg-sign=}"
 		;;
+	--verify-signatures)
+		verify_signatures=t
+		;;
+	--no-verify-signatures)
+		verify_signatures=
+		;;
 	--)
 		shift
 		break
@@ -594,6 +602,42 @@ then
 	fi
 fi
 
+if test "$verify_signatures"
+then
+	if test -n "$rebase_root"
+	then
+		foreign_revisions="$orig_head..$onto"
+	else
+		foreign_revisions="$orig_head..${restrict_revision-$upstream}"
+	fi
+
+	for cmt in $(git rev-list --reverse "$foreign_revisions")
+	do
+		if ! git log -1 --pretty=format:'%G?%n%GS' "$cmt" |
+		(
+			read cmt_sig
+			read cmt_signer
+			case "$cmt_sig" in
+			'G')
+				;;
+			'U')
+				die "$(gettext "Commit $cmt has an untrusted GPG signature, allegedly by $cmt_signer.")"
+				;;
+			'B')
+				die "$(gettext "Commit $cmt has a bad GPG signature allegedly by $cmt_signer.")"
+				;;
+			*) #'N'
+				die "$(gettext "Commit $cmt does not have a GPG signature.")"
+				;;
+			esac
+			test "$verbose" && test 'G' = "$cmt_sig" && echo "Commit $cmt has a good GPG signature by $cmt_signer."
+		)
+		then
+			exit 1
+		fi
+	done
+fi
+
 # If a hook exists, give it a chance to interrupt
 run_pre_rebase_hook "$upstream_arg" "$@"
 
diff --git a/t/t3427-rebase-verify-signatures.sh b/t/t3427-rebase-verify-signatures.sh
new file mode 100755
index 0000000..1bd0a4d
--- /dev/null
+++ b/t/t3427-rebase-verify-signatures.sh
@@ -0,0 +1,65 @@
+#!/bin/sh
+
+test_description='rebase signature verification tests'
+. ./test-lib.sh
+. "$TEST_DIRECTORY/lib-gpg.sh"
+
+test_expect_success GPG 'create signed commits' '
+	echo 1 >file && git add file &&
+	test_tick && git commit -m initial &&
+	git tag initial &&
+
+	git checkout -b side-signed &&
+	echo 3 >elif && git add elif &&
+	test_tick && git commit -S -m "signed on side" &&
+	git checkout initial &&
+
+	git checkout -b side-unsigned &&
+	echo 3 >foo && git add foo &&
+	test_tick && git commit -m "unsigned on side" &&
+	git checkout initial &&
+
+	git checkout -b side-bad &&
+	echo 3 >bar && git add bar &&
+	test_tick && git commit -S -m "bad on side" &&
+	git cat-file commit side-bad >raw &&
+	sed -e "s/bad/forged bad/" raw >forged &&
+	git hash-object -w -t commit forged >forged.commit &&
+	git checkout initial &&
+
+	git checkout -b side-untrusted &&
+	echo 3 >baz && git add baz &&
+	test_tick && git commit -SB7227189 -m "untrusted on side" &&
+
+	git checkout master
+'
+
+test_expect_success GPG 'rebase unsigned commit with verification' '
+	test_must_fail git rebase --verify-signatures side-unsigned 2>rebaseerror &&
+	test_i18ngrep "does not have a GPG signature" rebaseerror
+'
+
+test_expect_success GPG 'rebase commit with bad signature with verification' '
+	test_must_fail git rebase --verify-signatures $(cat forged.commit) 2>rebaseerror &&
+	test_i18ngrep "has a bad GPG signature" rebaseerror
+'
+
+test_expect_success GPG 'rebase commit with untrusted signature with verification' '
+	test_must_fail git rebase --verify-signatures side-untrusted 2>rebaseerror &&
+	test_i18ngrep "has an untrusted GPG signature" rebaseerror
+'
+
+test_expect_success GPG 'rebase signed commit with verification' '
+	git rebase --verbose --verify-signatures side-signed >rebaseoutput &&
+	test_i18ngrep "has a good GPG signature" rebaseoutput
+'
+
+test_expect_success GPG 'rebase commit with bad signature without verification (implicit)' '
+	git rebase $(cat forged.commit)
+'
+
+test_expect_success GPG 'rebase commit with bad signature without verification (explicit)' '
+	git rebase --no-verify-signatures $(cat forged.commit)
+'
+
+test_done
-- 
2.6.3

Re: [PATCH] rebase: add --verify-signatures

[Junio C Hamano]

Alexander 'z33ky' Hirsch <1zeeky@gmail.com> writes:

> +	if test -n "$rebase_root"
> +	then
> +		foreign_revisions="$orig_head..$onto"
> +	else
> +		foreign_revisions="$orig_head..${restrict_revision-$upstream}"
> +	fi
> +
> +	for cmt in $(git rev-list --reverse "$foreign_revisions")
> +	do
> +		if ! git log -1 --pretty=format:'%G?%n%GS' "$cmt" |

I do not think this matches what the corresponding option in "git
merge" does, where the only tips of the histories being merged are
checked.

Re: [PATCH] rebase: add --verify-signatures

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Alexander 'z33ky' Hirsch <1zeeky@gmail.com> writes:
>
>> +	if test -n "$rebase_root"
>> +	then
>> +		foreign_revisions="$orig_head..$onto"
>> +	else
>> +		foreign_revisions="$orig_head..${restrict_revision-$upstream}"
>> +	fi
>> +
>> +	for cmt in $(git rev-list --reverse "$foreign_revisions")
>> +	do
>> +		if ! git log -1 --pretty=format:'%G?%n%GS' "$cmt" |
>
> I do not think this matches what the corresponding option in "git
> merge" does, where the only tips of the histories being merged are
> checked.

Having said that, I somehow doubt that verify-signatures is a
feature that is desirable in a workflow around "pull --rebase" in
the larger picture.  If you step back a bit, in a "merge" based
workflow, you are the keeper of the sanity, cleanliness, and all the
good things in the authoritative history when doing a "git pull".
That is why you would want to validate what gets merged from another
place and in that context having --verify-signatures may make sense
(and it might even make more sense if the code did so for all new
commits, not just the tip, but that is a separate topic).  If the
validation fails, you would tell the owner of that side branch you
just attempted to pull from to get her act together before asking to
be pulled again.  There is a clear path to make further progress
after the validation fails.

In a workflow that is built around "pull --rebase", you are _given_
the authoritative history with all the good things from another
place and then you rebuild your own work on top of it.  The sanity
and cleanliness of what you built on top is given, and rejecting it
at that point would not help you make further progress in any way,
as that is a published history that is shared and more authoritative
than what you have.

Hence, while I 100% agree with Brian's "it is not there because
nobody bothered to add the corresponding option on the rebase side",
I do not necessarily think "nobody bothered" is the same as "they
were too lazy"--perhaps some people thought about doing it, and then
decided not to, because the option made no sense when they stepped
back to look at the larger picture.

Re: [PATCH] rebase: add --verify-signatures

[Alexander 'z33ky' Hirsch]

On Thu, Dec 10, 2015 at 11:53:45AM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Alexander 'z33ky' Hirsch <1zeeky@gmail.com> writes:
> >
> >> +	if test -n "$rebase_root"
> >> +	then
> >> +		foreign_revisions="$orig_head..$onto"
> >> +	else
> >> +		foreign_revisions="$orig_head..${restrict_revision-$upstream}"
> >> +	fi
> >> +
> >> +	for cmt in $(git rev-list --reverse "$foreign_revisions")
> >> +	do
> >> +		if ! git log -1 --pretty=format:'%G?%n%GS' "$cmt" |
> >
> > I do not think this matches what the corresponding option in "git
> > merge" does, where the only tips of the histories being merged are
> > checked.

Oh, indeed. I saw the loop in merge.c and by brain went there without
any further thought. This would be easy to fix though.

> Having said that, I somehow doubt that verify-signatures is a
> feature that is desirable in a workflow around "pull --rebase" in
> the larger picture.  If you step back a bit, in a "merge" based
> workflow, you are the keeper of the sanity, cleanliness, and all the
> good things in the authoritative history when doing a "git pull".
> That is why you would want to validate what gets merged from another
> place and in that context having --verify-signatures may make sense
> (and it might even make more sense if the code did so for all new
> commits, not just the tip, but that is a separate topic).  If the
> validation fails, you would tell the owner of that side branch you
> just attempted to pull from to get her act together before asking to
> be pulled again.  There is a clear path to make further progress
> after the validation fails.
> 
> In a workflow that is built around "pull --rebase", you are _given_
> the authoritative history with all the good things from another
> place and then you rebuild your own work on top of it.  The sanity
> and cleanliness of what you built on top is given, and rejecting it
> at that point would not help you make further progress in any way,
> as that is a published history that is shared and more authoritative
> than what you have.

Well, the rejection would not refer to the work you put on top, but to
the commits you want to base your work on.
If validation fails, then an empty commit that is signed can be
committed on top of the previously unsigned branch if commit rewriting
is not allowed.

> Hence, while I 100% agree with Brian's "it is not there because
> nobody bothered to add the corresponding option on the rebase side",
> I do not necessarily think "nobody bothered" is the same as "they
> were too lazy"--perhaps some people thought about doing it, and then
> decided not to, because the option made no sense when they stepped
> back to look at the larger picture.

That's why I was asking in my first mail if such an addition would make
sense. I don't really have an agenda or a pressing need for this
feature, I just noticed that a `git pull --rebase --verify-signatures`
did not complain when it looked like it ought to.
If this patch gets rejected then I will propose one which makes git-pull
warn, or even error, when both --rebase and --verify-signatures is
passed.

Regards,
Alexander Hirsch

Re: [PATCH] rebase: add --verify-signatures

[Junio C Hamano]

Alexander 'z33ky' Hirsch <1zeeky@gmail.com> writes:

> On Thu, Dec 10, 2015 at 11:53:45AM -0800, Junio C Hamano wrote:
>
>> In a workflow that is built around "pull --rebase", you are _given_
>> the authoritative history with all the good things from another
>> place and then you rebuild your own work on top of it.  The sanity
>> and cleanliness of what you built on top is given, and rejecting it
>> at that point would not help you make further progress in any way,
>> as that is a published history that is shared and more authoritative
>> than what you have.
>
> Well, the rejection would not refer to the work you put on top, but to
> the commits you want to base your work on.
> If validation fails, then an empty commit that is signed can be
> committed on top of the previously unsigned branch if commit rewriting
> is not allowed.

I do not quite understand how that would help anything.  I do not
personally believe in projects that wants to sign each and every
commit, but to them, "an empty signed commit on top" would not fix
anything once they have an unsigned commit at the tip of a public
branch.  And for those that care about only the tip to be signed,
instead of adding such an empty commit, you would rebuild and sign
your work on top of that unsigned public tip and push back---at
which point the tip of the public branch would have a signature from
you.  So such an empty signed commit would either not help, or not
necessary, to make the resulting history kosher again.

Re: [PATCH] rebase: add --verify-signatures

[Alexander 'z33ky' Hirsch]

On Wed, Dec 16, 2015 at 10:12:12AM -0800, Junio C Hamano wrote:
> I do not quite understand how that would help anything.  I do not
> personally believe in projects that wants to sign each and every
> commit, but to them, "an empty signed commit on top" would not fix
> anything once they have an unsigned commit at the tip of a public
> branch.  And for those that care about only the tip to be signed,
> instead of adding such an empty commit, you would rebuild and sign
> your work on top of that unsigned public tip and push back---at
> which point the tip of the public branch would have a signature from
> you.  So such an empty signed commit would either not help, or not
> necessary, to make the resulting history kosher again.
> 

Checking all commits was a mistake I made because of misinterpreting the
git-merge code. Only the tip should be checked for a signature.
And the reason to get it signed instead of just signing the commits
rebased on top is to defer to the judgement of the author of the branch
you're rebasing onto instead of checking the unsigned commits for
validity yourself.

As I understand it, this is the same reason for the existence of
--verify-signatures for git-merge. Otherwise the same argument could be
made for git-merge - just do whatever cleanup you need after merging and
sign it yourself.
Or maybe I haven't grasped what --verify-signatures is for.

Re: [PATCH] rebase: add --verify-signatures

[Junio C Hamano]

Alexander 'z33ky' Hirsch <1zeeky@gmail.com> writes:

> As I understand it, this is the same reason for the existence of
> --verify-signatures for git-merge. Otherwise the same argument could be
> made for git-merge 

I suspect that you are missing the bigger workflow issues, if you
think this and merge are the same.

git-merge will check the other history on the side branch that you
are merging _into_ the trunk, to give you an opportunity to reject
what does not pass and keep the trunk sane without doing anything
else.  How you (or others who asked you to pull) clean up the side
branch is outside the scope of its verification.

Your change to "git pull --rebase" checks the other way---the
history, which is already the trunk, onto which your work will be
rebased.  There is nothing you can do without messing with the trunk
if the validation did not pass, be it with a rewind-and-rebuild or a
sealing empty commit which is pointless.

Re: [PATCH] rebase: add --verify-signatures

[Alexander 'z33ky' Hirsch]

Sorry, I didn't do a group-reply in my last mail.

On Mon, Dec 21, 2015 at 03:46:54PM -0800, Junio C Hamano wrote:
> Alexander 'z33ky' Hirsch <1zeeky@gmail.com> writes:
> 
> > On Thu, Dec 17, 2015 at 10:22:20AM -0800, Junio C Hamano wrote:
> >> I suspect that you are missing the bigger workflow issues, if you
> >> think this and merge are the same.
> >> 
> >> git-merge will check the other history on the side branch that you
> >> are merging _into_ the trunk, to give you an opportunity to reject
> >> what does not pass and keep the trunk sane without doing anything
> >> else.  How you (or others who asked you to pull) clean up the side
> >> branch is outside the scope of its verification.
> >> 
> >> Your change to "git pull --rebase" checks the other way---the
> >> history, which is already the trunk, onto which your work will be
> >> rebased.  There is nothing you can do without messing with the trunk
> >> if the validation did not pass, be it with a rewind-and-rebuild or a
> >> sealing empty commit which is pointless.
> >
> > It would still make sense for long-lived development branches that
> > contain experimental or controversial features, or for forks/private
> > copies that add a couple of commits onto a branch. Merging is certainly
> > an option, but I don't see why rebasing would be a wrong alternative.
> 
> Nobody says rebase is a wrong alternative.
> 
> It is just the time you decide to rebase is a wrong time to check,
> iow, too late, for the validation of the tip.

In that case I would like to submit a patch that warns or even errors in
case both --rebase and --verify-signatures is passed to git-pull.
I think an error would be appropriate, but in theory this could break
scripts that have done that, albeit it probably didn't do what the user
expected, and I don't know git's policy about breaking something like
this.