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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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

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

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

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index a34742513ac..62b11c5f3a4 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -131,6 +131,8 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
>  {
>  	int status = parse_hide_refs_config(var, value, "receive");
>  
> +	git_gpg_config(var, value, NULL);
> +
>  	if (status)
>  		return status;

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

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

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

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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux