From: Fabian Stelzer <fs@gigacodes.de>
Date: Thu, 29 Jul 2021 09:12:27 GMT
Subject: Re: [PATCH v6 5/9] ssh signing: parse ssh-keygen output and verify signatures
Message-ID: <54671c83-4b1f-5e24-a6ad-226a4f45f952@gigacodes.de>
In-Reply-To: <xmqq35ry85or.fsf@gitster.g>

On 28.07.21 23:55, Junio C Hamano wrote:
> "Fabian Stelzer via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Fabian Stelzer <fs@gigacodes.de>
>>
>> to verify a ssh signature we first call ssh-keygen -Y find-principal to
> 
> "to" -> "To".

fixed

[snip]

>> +	/*
>> +	 * ssh-keysign output should be:
>> +	 * Good "git" signature for PRINCIPAL with RSA key SHA256:FINGERPRINT
>> +	 * Good "git" signature for PRINCIPAL WITH WHITESPACE with RSA key SHA256:FINGERPRINT
> 
> A bit unfortunate line that is overly long.  These two are not
> mutually exclusive two different choices, but one is a special case
> of the other, no?  How about phrasing it like so instead?
> 
> 	/*
> 	 * ssh-keysign output should be:
> 	 * Good "git" signature for PRINCIPAL with RSA key SHA256:FINGERPRINT
>           *
> 	 * or for valid but unknown keys:
> 	 * Good "git" signature with RSA key SHA256:FINGERPRINT
>           *
> 	 * Note that "PRINCIPAL" can contain whitespace, "RSA" and
> 	 * "SHA256" part could be a different token that names of
> 	 * the algorithms used, and "FINGERPRINT" is a hexadecimal
>           * string.  By finding the last occurence of " with ", we can
>           * reliably parse out the PRINCIPAL.
> 	 */
> 

Yes, it's a special case that makes it a bit harder to parse. I will 
change the comment like you suggested. That makes it clear.

>> +		/* Search for the last "with" to get the full principal */
>> +		principal = line;
>> +		do {
>> +			search = strstr(line, " with ");
>> +			if (search)
>> +				line = search + 1;
>> +		} while (search != NULL);
>> +		sigc->signer = xmemdupz(principal, line - principal - 1);
>> +		sigc->fingerprint = xstrdup(strstr(line, "key") + 4);
> 
> OK.  This does not care the "RSA" part, which is future resistant.
> It assumes the <algo>:<fingerprint> comes after literal " key ",
> which I think is a reasonable thing to do.
> 
> However, we never checked if the line has "key" in it, so
> strstr(line, "key") + 4 may not be pointing at where this code
> expects.
> 

Hmm. What would i do if i don't find "key"? Still mark the signature as 
valid an just leave fingerprint & key empty?

>> +			/* We found principals - Try with each until we find a
>> +			 * match */
> 
>                          /*
>                           * Do not forget our multi-line comment
>                           * style, please.
>                           */
> 

fixed. clang-format wordwrapped those :/


>> +
>> +			ret &= starts_with(ssh_keygen_out.buf, "Good");
> 
> This is somewhat unusual construct in our codebase, I suspect.  And
> probably is even wrong.  Didn't you mean
> 
> 			if (!ret)
> 				ret = starts_with(...);
> 
> instead?  Surely, when pipe_command() failed, it is likely that
> ssh_keygen_out may not have anything useful, and checking what the
> first up-to-four bytes of it contain unconditionally may be cheap
> enough, but the person reading the code would expect you to peek
> into the result only when you actually got the result, no?

you are correct. we don't need to look at the output when the command fails.

> 
>> +			if (ret == 0)
>> +				break;
> 
> It's more common to do
> 
> 			if (!ret)
> 				break;
> 

changed.

Thanks