Re: [PATCH v7 4/5] merge/pull Check for untrusted good GPG signatures

From: Sebastian Götte <jaseg@physik.tu-berlin.de>
Date: Mar 31, 2013, 15:21 UTC
Message-ID: <51585491.1070604@physik.tu-berlin.de>
In-Reply-To: <87mwtjwqzz.fsf@linux-k42r.v.cablecom.net>

On 03/31/2013 05:03 PM, Thomas Rast wrote:

Show 33 quoted lines

> John Keeping <john@keeping.me.uk> writes:
> 
>> On Sun, Mar 31, 2013 at 04:33:57PM +0200, Sebastian Götte wrote:
>>> diff --git a/commit.c b/commit.c
>>> index eda7f90..bb2d9ad 100644
>>> --- a/commit.c
>>> +++ b/commit.c
>>> @@ -1029,6 +1029,8 @@ static struct {
>>>  } sigcheck_gpg_status[] = {
>>>  	{ 'G', "[GNUPG:] GOODSIG " },
>>>  	{ 'B', "[GNUPG:] BADSIG " },
>>> +	{ 'U', "[GNUPG:] TRUST_NEVER" },
>>> +	{ 'U', "[GNUPG:] TRUST_UNDEFINED" },
>>>  };
>>>  
>>>  static void parse_gpg_output(struct signature_check *sigc)
>>> @@ -1050,11 +1052,12 @@ static void parse_gpg_output(struct signature_check *sigc)
>>>  			found += strlen(sigcheck_gpg_status[i].check);
>>>  		}
>>>  		sigc->result = sigcheck_gpg_status[i].result;
>>> -		sigc->key = xmemdupz(found, 16);
>>> -		found += 17;
>>> -		next = strchrnul(found, '\n');
>>> -		sigc->signer = xmemdupz(found, next - found);
>>> -		break;
>>> +		if (sigc->result != 'U') {
>>
>> This could use a comment; we know now that only GOODSIG and BADSIG
>> are followed by a signature, but someone looking at this code in the
>> future will probably appreciate an explanation.
> 
> Wouldn't it be even less magical if there was an explicit field in the
> struct that says whether we need to read a sig from such a line?

I think that special-case if a few lines below is OK for now.

> And furthermore, to use an enum instead of a char so that you can easily
> spell out the details in the code?  This also has the advantage that the
> compiler can check that your 'switch'es cover all cases.

This char is actually from Junios original code. I think we can afford three chars. This could be changed if we ever need more than that. Another possible future feature would be to distinguish between "no signature" and "public key not found" and to allow pass-through of that GPG "retrieve missing public keys from keyserver" option.

> That's of course assuming that I interpret the logic right; my current
> understanding is that:
> 
> * U means untrusted, which is bettern than B but worse than G;

Correct. Also, BADSIG is never followed by trust information.

> * GPG guarantees that the last line matching any of the patterns is the
>   one we care about, so we can blindly override one with the other

Actually, we are searching *all* GPG status lines for every search string in the array. This way, first GOODSIG may be matched to fill in the key and signer information, but a subsequent TRUST_* match finally sets the result code.