From: Fabian Stelzer Date: Thu, 29 Jul 2021 08:21:04 GMT Subject: Re: [PATCH v6 4/9] ssh signing: provide a textual representation of the signing key Message-ID: In-Reply-To: On 28.07.21 23:34, Junio C Hamano wrote: > "Fabian Stelzer via GitGitGadget" writes: > >> From: Fabian Stelzer >> >> 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.