On 28.07.21 23:55, Junio C Hamano wrote:
"Fabian Stelzer via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
From: Fabian Stelzer <fs@xxxxxxxxxxxx>
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
