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

From: Fabian Stelzer <fs@gigacodes.de>
Date: Jul 29, 2021, 09:48 UTC
Message-ID: <d4bda019-bbea-6645-e46a-18a702d3f0ad@gigacodes.de>
In-Reply-To: <20210728230452.2719333-1-jonathantanmy@google.com>

On 29.07.21 01:04, Jonathan Tan wrote:

Show 6 quoted lines

>> 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.

Show 14 quoted lines

> 
>> 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.

Show 7 quoted lines

> 
>> 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.

Show 6 quoted lines

> 
> 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.

Show 5 quoted lines

>> +	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

Show 13 quoted lines

>> +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 ");

Show 13 quoted lines

> 
>> +	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