Re: [PATCH v6 4/9] ssh signing: provide a textual representation of the signing key
- From
Fabian Stelzer <fs@gigacodes.de>
- Date
- Jul 29, 2021, 08:21 UTC
- Message-ID
- <e79a63f3-d86b-d4e7-2aa5-12aa1f2e6090@gigacodes.de>
- In-Reply-To
- <xmqqeebi86m7.fsf@gitster.g>
On 28.07.21 23:34, Junio C Hamano wrote:
Show 10 quoted lines
> "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
Show 5 quoted lines
>> >> +/* Returns a textual but unique representation ot the signing key */ > > "ot" -> "of". >
Fixed
Show 15 quoted lines
>> +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.
Show 16 quoted lines
>> - 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.