From: Fabian Stelzer Date: Thu, 15 Jul 2021 08:13:13 GMT Subject: Re: [PATCH v3 3/9] ssh signing: retrieve a default key from ssh-agent Message-ID: In-Reply-To: On 14.07.21 22:20, Junio C Hamano wrote: > "Fabian Stelzer via GitGitGadget" writes: > >> From: Fabian Stelzer >> >> calls ssh-add -L and uses the first key > Documentation/SubmittingPatches::[[describe-changes]]. > >> Signed-off-by: Fabian Stelzer >> --- >> gpg-interface.c | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/gpg-interface.c b/gpg-interface.c >> index 3c9a48c8e7e..c956ed87475 100644 >> --- a/gpg-interface.c >> +++ b/gpg-interface.c >> @@ -467,6 +467,23 @@ int git_gpg_config(const char *var, const char *value, void *cb) >> return 0; >> } >> >> +/* Returns the first public key from an ssh-agent to use for signing */ >> +static char *get_default_ssh_signing_key(void) { > Style. Open and close braces around a function sit on their own > lines by themselves. >> + struct child_process ssh_add = CHILD_PROCESS_INIT; >> + int ret = -1; >> + struct strbuf key_stdout = STRBUF_INIT; >> + struct strbuf **keys; > Whose releasing the resource held by "keys" when we return? > >> + strvec_pushl(&ssh_add.args, "ssh-add", "-L", NULL); >> + ret = pipe_command(&ssh_add, NULL, 0, &key_stdout, 0, NULL, 0); > I often load about half a dozen keys to my ssh-agent so "ssh-add -L" > will give me multi-line output. I know you wrote "the first public > key" above, but that does not mean users who needs to have multiple > keys can be limited to use only the first key for signing. There > should be a way to say "I may have many keys for other reasons, but > for signing I want to use this key, not the other ones". I will make the commit message clearer. This function only provides a default key in case no key is configured in user.signingkey. If you set user.signingkey to a public key the correct private key from your agent will be used for signing. > >> + if (!ret) { >> + keys = strbuf_split_max(&key_stdout, '\n', 2); > Let's not use strbuf_split_*() that is a horribly wrong interface. > You do not want a set of elastic buffer after splitting. You only > are peeking the first line, no? You are leaking keys[] array and > probably keys[1], too. > > eol = strchrnul(key_stdout.buf, '\n'); > strbuf_setlen(&key_stdout, eol - key_stdout.buf); > > or something along that line, perhaps? I have changed it to what you suggested. I'm always a bit hesitant to use arithmetic with string pointers. I know its simple and efficient, but IMHO can be hard to read. > >> + if (keys[0]) >> + return strbuf_detach(keys[0], NULL); >> + } >> + >> + return ""; >> +} >> const char *get_signing_key(void) > Missing blank line after the function body. > >> { >> if (configured_signing_key) -- GIGACODES GmbH | Dr. Hermann-Neubauer-Ring 32 | D-63500 Seligenstadt www.gigacodes.de | fs@gigacodes.de Phone +49 6182 8955-114 | Fax +49 6182 8955-299 | HRB 40711 AG Offenbach a. Main Geschäftsführer: Fabian Stelzer | Umsatzsteuer-ID DE219379936