Re: [PATCH 6/6] ssh signing: fmt-merge-msg/check_signature with tag date

From: Fabian Stelzer <fs@gigacodes.de>
Date: Oct 25, 2021, 08:39 UTC
Message-ID: <8ff17f0e-9877-1004-4057-4f4af21531a1@gigacodes.de>
In-Reply-To: <211022.86a6j0kjoi.gmgdl@evledraar.gmail.com>

On 22.10.21 20:12, Ævar Arnfjörð Bjarmason wrote:

Show 21 quoted lines

> 
> On Fri, Oct 22 2021, Fabian Stelzer wrote:
> 
>>  			buf = payload.buf;
>>  			len = payload.len;
>> -			if (check_signature(payload.buf, payload.len, 0, NULL,
>> +
>> +			if (parse_signed_buffer_metadata(payload.buf, "tagger",
>> +							 &payload_timestamp,
>> +							 &payload_signer))
>> +				strbuf_addstr(&sig,
>> +					_("failed to parse timestamp and signer info from payload"));
>> +
>> +			if (check_signature(payload.buf, payload.len,
>> +					    payload_timestamp, &payload_signer,
>>  					    sig.buf, sig.len, &sigc) &&
>>  			    !sigc.output)
>>  				strbuf_addstr(&sig, "gpg verification failed.\n");
> 
> I haven't tested this, but your addition to &sig here lacks a \n,
> compared to the &sig seen right above here in the diff context.

You are correct. I have added the newline.

Show 5 quoted lines

> 
> The whole business of seemingly mixing error messages and a signature
> payload in the same variable seems a bit odd, but maybe I'm misreading
> it. In any case it seems to pre-date this series...
>

True, it is a bit odd. This function generates the message text when merging a (signed) tag. It verifies the tag upon doing so and includes the result information in the message.

I'm not sure what should happen in this error case. (especially since this is one of those "this should never happen" errors). I don't know if i can just warn() here or if that would be lost or even corrupt the merge message :/ If the buffer parsing fails this could result in ssh signatures being marked as valid even though they expired. So i think it it important to include this info. But the error has no effect on gpg signatures. So telling users about not being able to check key lifetime here might paint a wrong picture.

Thanks, Fabian