On 09.01.2022 16:37, Eric Sunshine wrote:
On Fri, Jan 07, 2022 at 10:07:35AM +0100, Fabian Stelzer wrote:
We need to trim \r from the output of 'ssh-keygen -Y find-principals' on
Windows, or we end up calling 'ssh-keygen -Y verify' with a bogus signer
identity. ssh-keygen.c:2841 contains a call to puts(3), which confirms
this hypothesis. Signature verification passes with the fix.
Helped-by: Pedro Martelletto <pedro@xxxxxxxxxx>
Signed-off-by: Fabian Stelzer <fs@xxxxxxxxxxxx>
Should this also have a "Helped-by: Junio" since this code was heavily
inspired by his suggestion[1]?
Yeah, this should have a "Written-by: Junio" ^^
I'm never sure when to add these headers (except the signed-off).
[1]: https://lore.kernel.org/git/xmqqo84rcn3j.fsf@gitster.g/
---
diff --git a/gpg-interface.c b/gpg-interface.c
@@ -502,15 +501,30 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
+ const char *next;
+ for (line = ssh_principals_out.buf;
+ *line;
+ line = next) {
+ const char *end_of_text;
+
+ next = end_of_text = strchrnul(line, '\n');
+
+ /* Did we find a LF, and did we have CR before it? */
+ if (*end_of_text &&
+ line < end_of_text &&
+ end_of_text[-1] == '\r')
+ end_of_text--;
+
+ /* Unless we hit NUL, skip over the LF we found */
+ if (*next)
+ next++;
+
+ /* Not all lines are data. Skip empty ones */
+ if (line == end_of_text)
+ continue;
+
+ /* We now know we have an non-empty line. Process it */
+ principal = xmemdupz(line, end_of_text - line);
Considering that this code makes a copy of the line _anyhow_ which it
assigns to `principal`, it still seems like it would be simpler and
far easier to understand at-a-glance to instead take advantage of one
of the existing string-splitting functions. For instance, something
like this:
struct strbuf **line, **to_free;
line = to_free = strbuf_split(&ssh_principals_out, '\n');
for (; *line; line++) {
strbuf_trim_trailing_newline(*line);
if (!(*line)->len)
continue;
principal = (*line)->buf;
keeping in mind that strbuf_trim_trailing_newline() takes care of
CR/LF, and with appropriate cleanup at the end of the loop:
strbuf_list_free(to_free);
(and removal of `FREE_AND_NULL(principal)` which is no longer needed).
Something similar can be done with string_list_split(), as well.
I agree that this is the most readable of the variants (and it works just as
well). Since in most cases there even will only ever be a single line of
output the extra work/allocation we might be doing with it is quite minimal.
I have done something quite similar in get_default_ssh_signing_key() and got
a bit of negative feedback for it (being overkill for retrieving a single
line) but ended up using it anyway.
