Re: [PATCH v6 5/9] ssh signing: parse ssh-keygen output and verify signatures

[Fabian Stelzer <fs@gigacodes.de>]

On 29.07.21 01:04, Jonathan Tan wrote:
>> to verify a ssh signature we first call ssh-keygen -Y find-principal to
>> look up the signing principal by their public key from the
>> allowedSignersFile. If the key is found then we do a verify. Otherwise
>> we only validate the signature but can not verify the signers identity.
> 
> Is this the same behavior as GPG signing in Git?

Not quite. GPG requires every signers public key to be in the keyring. 
But even then, the "UNDEFINED" Trust level is enough to be valid for 
commits (but not for merges).
For SSH i did set the unknown keys to UNDEFINED as well and they will 
show up as valid but not have a principal to identify them.
This way a project can decide wether to accept unknown keys by setting 
the gpg.mintrustlevel. So the default behaviour is different.
The alternative would be to treat unknown keys always as invalid.

> 
>> Verification uses the gpg.ssh.allowedSignersFile (see ssh-keygen(1) "ALLOWED
>> SIGNERS") which contains valid public keys and a principal (usually
>> user@domain). Depending on the environment this file can be managed by
>> the individual developer or for example generated by the central
>> repository server from known ssh keys with push access. If the
>> repository only allows signed commits / pushes then the file can even be
>> stored inside it.
> 
> Storing the allowedSignersFile in the repo is technically possible even
> if the repository does not allow signed commits/pushes, right? I would
> reword the last sentence as "This file is usually stored outside the
> repository, but if the repository only allows signed commits/pushes, the
> user might choose to store it in the repository".

yes, thats correct. I have changed the wording.

> 
>> Using SSH CA Keys with these files is also possible. Add
>> "cert-authority" as key option between the principal and the key to mark
>> it as a CA and all keys signed by it as valid for this CA.
> 
> Is this functionality provided by SSH? I don't see "cert-authority"
> anywhere in the diff below.

I'll add "See "CERTIFICATES" in ssh-keygen(1)."
It is a SSH feature that i just wanted to make people aware of.

> 
> Also, I notice that the tests are all provided at the end. I think that
> it would be better for the tests to be incrementally provided along with
> the commit that introduces the relevant functionality, so it is clearer
> to the reviewers how it is supposed to work (and also for us to observe
> test coverage).

The problem is that nearly all of the tests use both signing & 
verification of signatures. I could move the initial test that creates 
all the signed commits but probably not much else.

>> +	git_gpg_config(var, value, NULL);
> 
> Check the return value of git_gpg_config() to see if that config was
> processed by that function - if yes, we can return early.
> 

fixed


>> +static void parse_ssh_output(struct signature_check *sigc)
>> +{
>> +	const char *line, *principal, *search;
>> +
>> +	/*
>> +	 * ssh-keysign output should be:
>> +	 * Good "git" signature for PRINCIPAL with RSA key SHA256:FINGERPRINT
>> +	 * Good "git" signature for PRINCIPAL WITH WHITESPACE with RSA key SHA256:FINGERPRINT
>> +	 * or for valid but unknown keys:
>> +	 * Good "git" signature with RSA key SHA256:FINGERPRINT
>> +	 */
> 
> Is this "ssh-keysign" or "ssh-keygen" output?

ssh-keygen. ssh-keysign is only used for host keys. But the names can 
get a bit confusing sometimes. i changed it to ssh-keygen here.

> 
> Also, is this output documented to be stable even across locales?

Not really :/ (it currently is not locale specific)
The documentation states to only check the commands exit code. Do we 
trust the exit code enough to rely on it for verification?
If so then i can move the main result and only parse the text for the 
signer/fingerprint info thats used in log formats. This way only the 
logs would break in case the output changes.

I added the output check since the gpg code did so as well:
ret |= !strstr(gpg_stdout.buf, "\n[GNUPG:] GOODSIG ");

> 
>> +	sigc->result = 'B';
>> +	sigc->trust_level = TRUST_NEVER;
> 
> A discussion of trust levels should also be in the commit message or
> user documentation.
> 
>> +	if (!strcmp(var, "gpg.ssh.revocationFile")) {
> 
> The "F" in "revocationFile" has to be lowercase. (If tests were
> included, as I suggested above, it might have been easier to catch
> this.)
> 

fixed

Thanks