Re: [PATCH v6 4/9] ssh signing: provide a textual representation of the signing key

[Fabian Stelzer <fs@gigacodes.de>]

On 28.07.21 23:34, Junio C Hamano wrote:
> "Fabian Stelzer via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Fabian Stelzer <fs@gigacodes.de>
>>
>> for ssh the user.signingkey can be a filename/path or even a literal ssh pubkey.
>> in push certs and textual output we prefer the ssh fingerprint instead.
> 
> These sentences that lack the initial capital letters would look
> unusual and distracting in our "git log --no-merges" stream.
> 

Fixed

>>   
>> +/* Returns a textual but unique representation ot the signing key */
> 
> "ot" -> "of".
> 

Fixed

>> +const char *get_signing_key_id(void)
>> +{
>> +	if (!strcmp(use_format->name, "ssh")) {
>> +		return get_ssh_key_fingerprint(get_signing_key());
>> +	} else {
>> +		/* GPG/GPGSM only store a key id on this variable */
>> +		return get_signing_key();
> 
> Hmph, we could ask gpg key fingerprint if we wanted to, and we
> cannot tell why "ssh" side needs a separate "key" and "key_id"
> while "gpg" side does not.  Hopefully it will become clear as we
> read on?
> 
> Again, dispatching on use_format->name looked rather unexpected.
> 

i will put the two strcmp(ssh) ifs on my todo list to also replace with 
a callback function.

>> -	char *signing_key = xstrdup(get_signing_key());
>> +	char *signing_key_id = xstrdup(get_signing_key_id());
>>   	const char *cp, *np;
>>   	struct strbuf cert = STRBUF_INIT;
>>   	int update_seen = 0;
>>   
>>   	strbuf_addstr(&cert, "certificate version 0.1\n");
>> -	strbuf_addf(&cert, "pusher %s ", signing_key);
>> +	strbuf_addf(&cert, "pusher %s ", signing_key_id);
> 
> Ahh...  We do not send GPG fingerprint in push certificate but you
> want to use the fingerprint when signing with SSH keys, and that is
> where the need for signing_key_id comes from?
> 
> OK.
> 

Previously the push certs contained the configured user.signingkey as 
"pusher". For gpg this is usually the key id. (e.g.: ABCDEF01)
For ssh signing this can now be a file path which would not make much 
sense to put into the push cert. I did not use the public ssh key since 
the file can also contain an encrypted private key, so i would have to 
ask ssh-keygen for the public key anyway.
Since the ssh fingerprint more resembles the gpg key id i used it instead.

As far as i understand the actual contents of the "pusher" header is not 
really relevant for the push-cert. The unique nonce is important but 
besides that its just a signed text blob.
If we don't care about having a local users file path in this header we 
could drop this commit.