Re: [PATCH v3 3/9] ssh signing: retrieve a default key from ssh-agent

[Fabian Stelzer <fs@gigacodes.de>]

On 14.07.21 22:20, Junio C Hamano wrote:

> "Fabian Stelzer via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Fabian Stelzer <fs@gigacodes.de>
>>
>> calls ssh-add -L and uses the first key
> Documentation/SubmittingPatches::[[describe-changes]].
>
>> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
>> ---
>>   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