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

[Fabian Stelzer <fs@gigacodes.de>]

On 22.10.21 20:12, Ævar Arnfjörð Bjarmason wrote:
> 
> 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.

> 
> 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