Re: [PATCH v4 3/5] merge/pull: verify GPG signatures of commits being merged

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Sebastian Götte <jaseg@xxxxxxxxxxxxxxxxxxx> writes:

> When --verify-signatures is specified on the command-line of git-merge
> or git-pull, check whether the commits being merged have good gpg
> signatures and abort the merge in case they do not. This allows e.g.
> auto-deployment from untrusted repo hosts.
>
> Signed-off-by: Sebastian Götte <jaseg@xxxxxxxxxxxxxxxxxxxxxxxx>
> ---
> ...
> +	if (verify_signatures) {
> +		/* Verify the commit signatures */
> +		for (p = remoteheads; p; p = p->next) {
> +			struct commit *commit = p->item;
> +			struct signature signature;
> +			signature.check_result = 0;

Even though you may happen to know all other fields are output only,
it is a good practice to clear it with

			memset(&signature, 0, sizeof(signature);

By the way, I think this variable and type should be called more
like "signature_check" or something with "check" in its name, not
"signature".  After all it is _not_ a signature itself.

> +			check_commit_signature(commit, &signature);
> +
> +			char hex[41];

builtin/merge.c: In function 'cmd_merge':
builtin/merge.c:1247: error: ISO C90 forbids mixed declarations and code

> +
> +test_expect_success GPG 'merge unsigned commit with verification' '
> +	test_must_fail git merge --ff-only --verify-signatures side-unsigned 2> mergeerror &&

No SP between redirection and the target filename.

> +	grep "does not have a GPG signature" mergeerror

Do we plan to make this message localized?  If so I think you would
need to do this with test_i18ngrep.

> +'
> +
> +test_expect_success GPG 'merge commit with bad signature with verification' '
> +	test_must_fail git merge --ff-only --verify-signatures $(cat forged.commit) 2> mergeerror &&
> +	grep "has a bad GPG signature" mergeerror
> +'
> +
> +test_expect_success GPG 'merge signed commit with verification' '
> +	git merge -v --ff-only --verify-signatures side-signed > mergeoutput &&
> +	grep "has a good GPG signature" mergeoutput
> +'

Hmph.

So the caller needs to check both the standard output and the
standard error to get the whole picture?  Is there a reason why we
are not sending everything to the standard output consistently?

> +test_expect_success GPG 'merge commit with bad signature without verification' '
> +	git merge $(cat forged.commit)
> +'

Good to see that negative case where the new feature should _not_
trigger is properly tested.

> +
> +test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]