On Wed, Nov 17, 2021 at 05:27:27PM +0100, Fabian Stelzer wrote:
> The user.signingKey config for ssh signing supports either a path to a
> file containing the key or for the sake of convenience a literal string
> with the ssh public key. To differentiate between those two cases we
> check if the first few characters contain "ssh-" which is unlikely to be
> the start of a path. ssh supports other key types which are not prefixed
> with "ssh-" and will currently be treated as a file path and therefore
> fail to load. To remedy this we move the prefix check into its own
> function and add the other key types. "ssh -Q key" can be used to show a
> list of all supported types.
>
> Signed-off-by: Fabian Stelzer <fs@xxxxxxxxxxxx>
> ---
> gpg-interface.c | 16 +++++++++++++---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 3e7255a2a9..dd1df9f4ee 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -707,6 +707,16 @@ int git_gpg_config(const char *var, const char *value, void *cb)
> return 0;
> }
>
> +/* Determines wether key contains a literal ssh key or a path to a file */
Nit: s/wether/whether.
I had to re-read this comment before I realized that the "or a path to a
file" isn't checked by this function, but is what we assume to be true
if this function returns 0.
So I don't think anything you wrote there is wrong, but it may be
clearer to just say "Returns 1 if `key` contains a literal SSH
key, 0 otherwise."
> +static int is_literal_ssh_key(const char *key) {
> + return (
> + starts_with(key, "ssh-") ||
> + starts_with(key, "ecdsa-") ||
> + starts_with(key, "sk-ssh-") ||
> + starts_with(key, "sk-ecdsa-")
> + );
> +}
The outer-most parenthesis are unnecessary, but help with line wrapping.
Equally OK would have been:
return starts_with(...) ||
starts_with(...) ||
and so on, but it doesn't matter much one way or the other.
> +
> static char *get_ssh_key_fingerprint(const char *signing_key)
> {
> struct child_process ssh_keygen = CHILD_PROCESS_INIT;
> @@ -719,7 +729,7 @@ static char *get_ssh_key_fingerprint(const char *signing_key)
> * With SSH Signing this can contain a filename or a public key
> * For textual representation we usually want a fingerprint
> */
> - if (starts_with(signing_key, "ssh-")) {
> + if (is_literal_ssh_key(signing_key)) {
This (and all other replacements) are straightforward and exhaustive. It
would be nice to see an additional test confirming that we treat, for
e.g., literal ECDSA keys correctly.
Thanks,
Taylor
