Re: [PATCH v6 2/9] ssh signing: add ssh signature format and signing using ssh keys

[Fabian Stelzer <fs@gigacodes.de>]

On 29.07.21 00:45, Jonathan Tan wrote:
> Keep the commit titles to 50 characters or fewer. E.g.:
> 
>    gpg-interface: teach "ssh" gpg.format
> 

i will go over my commits and shorten them although I find your example 
very unclear. or did you mean: teach "ssh" to gpg.format ?

>> implements the actual sign_buffer_ssh operation and move some shared
>> cleanup code into a strbuf function
> 
> Capitalization and punctuation.
fixed

> 
>> Set gpg.format = ssh and user.signingkey to either a ssh public key
>> string (like from an authorized_keys file), or a ssh key file.
>> If the key file or the config value itself contains only a public key
>> then the private key needs to be available via ssh-agent.
>>
>> gpg.ssh.program can be set to an alternative location of ssh-keygen.
>> A somewhat recent openssh version (8.2p1+) of ssh-keygen is needed for
>> this feature. Since only ssh-keygen is needed it can this way be
>> installed seperately without upgrading your system openssh packages.
> 
> I notice that end-user documentation (e.g. about gpg.ssh.program) is in
> its own patch, but could that be added as functionality is being
> implemented? That makes it easier for reviewers to understand what's
> being implemented in each patch.
> 

I can move the user.signingkey & gpg.format part into the signing 
implementation commit and the rest into the verification. I don't see 
much benefit in splitting it up further. I don't want to split up parts 
of the same documentation block into separate commits.

>> +
>> +	if (starts_with(signing_key, "ssh-")) {
>> +		/* A literal ssh key */
>> +		key_file = mks_tempfile_t(".git_signing_key_tmpXXXXXX");
>> +		if (!key_file)
>> +			return error_errno(
>> +				_("could not create temporary file"));
>> +		keylen = strlen(signing_key);
>> +		if (write_in_full(key_file->fd, signing_key, keylen) < 0 ||
>> +		    close_tempfile_gently(key_file) < 0) {
>> +			error_errno(_("failed writing ssh signing key to '%s'"),
>> +				    key_file->filename.buf);
>> +			goto out;
>> +		}
>> +		ssh_signing_key_file = key_file->filename.buf;
>> +	} else {
>> +		/* We assume a file */
>> +		ssh_signing_key_file = expand_user_path(signing_key, 1);
>> +	}
> 
> A config that has 2 modes of operation is quite error-prone, I think.
> For example, a user could put a path starting with "ssh-" (admittedly
> unlikely since it would usually be an absolute path, but not
> impossible). And also from an implementation point of view, here the
> "ssh-" is case-sensitive, but in a future patch, there is a "ssh-" that
> is case-insensitive.
> 
> Can this just always take a path?
> 

I found the ability to specify the key literally useful since i don't 
need an extra file for my public key. In my case all keys come from an 
ssh-agent anyway but I'd like to be able to select which one to use for 
signing. But i'm not hard pressed on this feature. If consenus is this 
complicates things then i can remove it.

>> +	if (ret) {
>> +		if (strstr(signer_stderr.buf, "usage:"))
>> +			error(_("ssh-keygen -Y sign is needed for ssh signing (available in openssh version 8.2p1+)"));
>> +
>> +		error("%s", signer_stderr.buf);
>> +		goto out;
>> +	}
> 
> Checking for "usage:" seems fragile -  a binary running in a different
> locale might emit a different string, and legitimate output may somehow
> contain the string "usage:". Is there a different way to detect a
> version mismatch?
> 

I agree. Unfortunately i did not find any better way. But i think the 
risk of doing something wrong here is quite low. We only check for 
"usage:" in case ssh-keygen fails. And all we do if we find it is give 
the user an extra hint on what the problem probably is.
In any case we print the full stderr output as well.