Re: [PATCH v4 7/7] gpg-interface t: extend the existing GPG tests with GPGSM

[Henning Schild <henning.schild@siemens.com>]

Am Tue, 17 Jul 2018 14:31:36 -0700
schrieb Junio C Hamano <gitster@pobox.com>:

> Henning Schild <henning.schild@siemens.com> writes:
> 
> > diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
> > index a5d3b2cba..3fe02876c 100755
> > --- a/t/lib-gpg.sh
> > +++ b/t/lib-gpg.sh
> > @@ -38,7 +38,33 @@ then
> >  			"$TEST_DIRECTORY"/lib-gpg/ownertrust &&
> >  		gpg --homedir "${GNUPGHOME}" </dev/null >/dev/null
> > 2>&1 \ --sign -u committer@example.com &&
> > -		test_set_prereq GPG
> > +		test_set_prereq GPG &&  
> 
> We do not mind making GPGSM dependent on GPG, hence this && is
> justified.
> 
> > +		# Available key info:
> > +		# * see t/lib-gpg/gpgsm-gen-key.in
> > +		# To generate new certificate:
> > +		#  * no passphrase
> > +		#	gpgsm --homedir /tmp/gpghome/ \
> > +		#		-o /tmp/gpgsm.crt.user \
> > +		#		--generate-key \
> > +		#		--batch t/lib-gpg/gpgsm-gen-key.in
> > +		# To import certificate:
> > +		#	gpgsm --homedir /tmp/gpghome/ \
> > +		#		--import /tmp/gpgsm.crt.user
> > +		# To export into a .p12 we can later import:
> > +		#	gpgsm --homedir /tmp/gpghome/ \
> > +		#		-o t/lib-gpg/gpgsm_cert.p12 \
> > +		#		--export-secret-key-p12
> > "committer@example.com"
> > +		echo | gpgsm --homedir "${GNUPGHOME}" 2>/dev/null \
> > +			--passphrase-fd 0 --pinentry-mode loopback
> > \
> > +			--import
> > "$TEST_DIRECTORY"/lib-gpg/gpgsm_cert.p12 &&
> > +		gpgsm --homedir "${GNUPGHOME}" 2>/dev/null -K \
> > +			| grep fingerprint: | cut -d" " -f4 | tr
> > -d '\n' > \
> > +			${GNUPGHOME}/trustlist.txt &&
> > +		echo " S relax" >> ${GNUPGHOME}/trustlist.txt &&
> > +		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
> > +		echo hello | gpgsm --homedir "${GNUPGHOME}"
> > >/dev/null \
> > +			-u committer@example.com -o /dev/null
> > --sign - 2>&1 &&
> > +		test_set_prereq GPGSM  
> 
> And when any of the above fails, we refrain from setting GPGSM
> prereq.  Otherwise we are prepared to perform tests with gpgsm
> and get the prereq.
> 
> > diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> > index 25b1f8cc7..f57781e39 100755
> > --- a/t/t4202-log.sh
> > +++ b/t/t4202-log.sh
> > @@ -1556,12 +1556,28 @@ test_expect_success GPG 'setup signed
> > branch' ' git commit -S -m signed_commit
> >  '
> >  
> > +test_expect_success GPGSM 'setup signed branch x509' '
> > +	test_when_finished "git reset --hard && git checkout
> > master" &&
> > +	git checkout -b signed-x509 master &&
> > +	echo foo >foo &&
> > +	git add foo &&
> > +	test_config gpg.format x509 &&
> > +	test_config user.signingkey $GIT_COMMITTER_EMAIL &&
> > +	git commit -S -m signed_commit
> > +'  
> 
> OK.
> 
> > +test_expect_success GPGSM 'log --graph --show-signature x509' '
> > +	git log --graph --show-signature -n1 signed-x509 >actual &&
> > +	grep "^| gpgsm: Signature made" actual &&
> > +	grep "^| gpgsm: Good signature" actual
> > +'  
> 
> OK.
> 
> > @@ -1581,6 +1597,29 @@ test_expect_success GPG 'log --graph
> > --show-signature for merged tag' ' grep "^| | gpg: Good signature"
> > actual '
> >  
> > +test_expect_success GPGSM 'log --graph --show-signature for merged
> > tag x509' '
> > +	test_when_finished "git reset --hard && git checkout
> > master" &&
> > +	test_config gpg.format x509 &&
> > +	test_config user.signingkey $GIT_COMMITTER_EMAIL &&
> > +	git checkout -b plain-x509 master &&
> > +	echo aaa >bar &&
> > +	git add bar &&
> > +	git commit -m bar_commit &&
> > +	git checkout -b tagged-x509 master &&
> > +	echo bbb >baz &&
> > +	git add baz &&
> > +	git commit -m baz_commit &&
> > +	git tag -s -m signed_tag_msg signed_tag_x509 &&
> > +	git checkout plain-x509 &&
> > +	git merge --no-ff -m msg signed_tag_x509 &&
> > +	git log --graph --show-signature -n1 plain-x509 >actual &&
> > +	grep "^|\\\  merged tag" actual &&
> > +	grep "^| | gpgsm: Signature made" actual &&
> > +	grep "^| | gpgsm: Good signature" actual &&
> > +	git config --unset gpg.format &&
> > +	git config --unset user.signingkey  
> 
> You are using test_config early enough in this test; doesn't that
> take care of the last two steps for you, even when an earlier step
> failed?  If that is the case, then remove the last two line (and &&
> at the end of the line before).

Right, dropped those two lines and &&

> > diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
> > index 1cea758f7..a3a12bd05 100755
> > --- a/t/t5534-push-signed.sh
> > +++ b/t/t5534-push-signed.sh
> > @@ -218,4 +218,56 @@ test_expect_success GPG 'fail without key and
> > heed user.signingkey' ' test_cmp expect dst/push-cert-status
> >  '
> >  
> > +test_expect_success GPGSM 'fail without key and heed
> > user.signingkey x509' '
> > +	test_config gpg.format x509 &&
> > +	env | grep GIT > envfile &&  
> 
> The "envfile" is unused, no?  Remove this line.

Thanks, debugging leftovers.

> > +	prepare_dst &&
> > +	mkdir -p dst/.git/hooks &&
> > +	git -C dst config receive.certnonceseed sekrit &&
> > +	write_script dst/.git/hooks/post-receive <<-\EOF &&
> > +	# discard the update list
> > +	cat >/dev/null
> > +	# record the push certificate
> > +	if test -n "${GIT_PUSH_CERT-}"
> > +	then
> > +		git cat-file blob $GIT_PUSH_CERT >../push-cert
> > +	fi &&
> > +
> > +	cat >../push-cert-status <<E_O_F
> > +	SIGNER=${GIT_PUSH_CERT_SIGNER-nobody}
> > +	KEY=${GIT_PUSH_CERT_KEY-nokey}
> > +	STATUS=${GIT_PUSH_CERT_STATUS-nostatus}
> > +	NONCE_STATUS=${GIT_PUSH_CERT_NONCE_STATUS-nononcestatus}
> > +	NONCE=${GIT_PUSH_CERT_NONCE-nononce}
> > +	E_O_F
> > +
> > +	EOF  
> 
> OK, so up to this are what is done by post-receive, including the
> overwriting of ../push-cert (which is one level above the receiving
> repository's .git/, i.e. dst/push-cert) and ../push-cert-status.
> 
> > +	unset GIT_COMMITTER_EMAIL &&
> > +	git config user.email hasnokey@nowhere.com &&
> > +	git config user.signingkey "" &&
> > +	test_must_fail git push --signed dst noop ff +noff &&  
> 
> This is OK for a test that is known to be always at the end, but
> also forbids others to further update this script to add more tests
> at the end, as the standard setting of environment is blown away
> (the config is probably OK, but test_config to arrange them to be
> cleaned up would have been nicer), which is not very nice.  I think
> it should be easy to fix it when it becomes necessary, but at the
> same time if it is easy to fix, then probably we shouldn't introduce
> a breakage in the first place, so I am on the fence.

Switched to test_config, this is all coming from copying the previous
tests, which i left as is.

> > +	git config user.signingkey committer@example.com &&
> > +	git push --signed dst noop ff +noff &&  
> 
> So,... this is run without resetting user.email and demonstrates
> that signingkey is the only thing that matters, which makes sense.
> 
> > +	(
> > +		cat <<-\EOF &&
> > +		SIGNER=/CN=C O Mitter/O=Example/SN=C O/GN=Mitter
> > +		KEY=
> > +		STATUS=G
> > +		NONCE_STATUS=OK
> > +		EOF
> > +		sed -n -e "s/^nonce /NONCE=/p" -e "/^$/q"
> > dst/push-cert
> > +	) >expect.in &&
> > +	key=$(cat "${GNUPGHOME}/trustlist.txt" | cut -d" " -f1 |
> > tr -d ":") &&
> > +	sed -e "s/^KEY=/KEY=${key}/" expect.in > expect &&  
> 
> s/> expect/>expect/;  

Done.

> > +	noop=$(git rev-parse noop) &&
> > +	ff=$(git rev-parse ff) &&
> > +	noff=$(git rev-parse noff) &&
> > +	grep "$noop $ff refs/heads/ff" dst/push-cert &&
> > +	grep "$noop $noff refs/heads/noff" dst/push-cert &&
> > +	test_cmp expect dst/push-cert-status
> > +'
> > +
> > +
> >  test_done
> > diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> > index d7b319e91..2147938aa 100755
> > --- a/t/t7004-tag.sh
> > +++ b/t/t7004-tag.sh
> > @@ -1354,6 +1354,19 @@ test_expect_success GPG \
> >  	'test_config gpg.program echo &&
> >  	 test_must_fail git tag -s -m tail tag-gpg-failure'
> >  
> > +# try to sign with bad user.signingkey
> > +test_expect_success GPGSM \
> > +	'git tag -s fails if gpgsm is misconfigured (bad key)' \
> > +	'test_config user.signingkey BobTheMouse &&
> > +	 test_config gpg.format x509 &&
> > +	 test_must_fail git tag -s -m tail tag-gpg-failure'
> > +
> > +# try to produce invalid signature
> > +test_expect_success GPGSM \
> > +	'git tag -s fails if gpgsm is misconfigured (bad signature
> > format)' \
> > +	'test_config gpg.x509.program echo &&
> > +	 test_config gpg.format x509 &&
> > +	 test_must_fail git tag -s -m tail tag-gpg-failure'  
> 
> I can see that it is a gpgsm parallel of the earlier test we can see
> in the precontext of this hunk done for gpg, but how does the last
> one (and the original this one was modeled after) fail?
> 
> We say "echo" is the program that signs for the chosen format, "tag
> -s" tries to run "echo" instead of "gpgsm" with "--status-fd=2
> -bsau" or whatever args we usually give, and...?
> 
> I would guess you would either get "I don't know what you wanted me
> to do with --status-fd=2 option, I am erroring out" from "echo", or
> the "echo" command exiting without consuming any input, causing the
> feeder in "tag -s" to get SIGPIPE (or write(2) error), but the latter
> happens only when the payload to be signed is large enough.  On a
> platform whose "echo" pays no attention to unknown option, "echo"
> itself may not even error out.  And then we try to read from "echo"
> and we do not get anything (which is expected).  
> 
> And then who in "git tag -s" notice the breakage?
> 
> 	... goes and looks at gpg-interface.c::sign_buffer() ...
> 
> Ah, we check the status-fd output for "[GnuPG:] SIG_CREATED", which
> would never happen if we are talking to "echo".  OK, that is how
> this thing is expected to fail.
> 
> What I have been getting at is if this is really trying to trigger
> the "(bad signature format)" breakage.  The test uses a wrong
> program to simulate the case where a configured gpg/gpgsm failed to
> report "SIG_CREATED".  "bad signature format" does not sound exactly 
> like that, but you inherited the badness from the original, so let's
> leave it as is.

All valid points and yes this is coming from copying the other test.
Leaving as is.
 
> Thanks.  Modulo a few nits I pointed out above, buried in all the
> other good bits, this looks reasonable to me.

Cool, Thanks.

Henning