Re: [PATCH v3 2/9] ssh signing: add documentation

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 14, 2021, 20:07 UTC
Message-ID: <xmqq35sgwtga.fsf@gitster.g>
In-Reply-To: <2f8452f6570b1811682863441020a6e43fc556c7.1626264613.git.gitgitgadget@gmail.com>

"Fabian Stelzer via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 18 quoted lines

> From: Fabian Stelzer <fs@gigacodes.de>
>
> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
> ---
>  Documentation/config/gpg.txt  | 35 +++++++++++++++++++++++++++++++++--
>  Documentation/config/user.txt |  6 ++++++
>  2 files changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config/gpg.txt b/Documentation/config/gpg.txt
> index d94025cb368..16af0b0ada8 100644
> --- a/Documentation/config/gpg.txt
> +++ b/Documentation/config/gpg.txt
> @@ -11,13 +11,13 @@ gpg.program::
>  
>  gpg.format::
>  	Specifies which key format to use when signing with `--gpg-sign`.
> -	Default is "openpgp" and another possible value is "x509".
> +	Default is "openpgp". Other possible values are "x509", "ssh".

Makes sense.

Show 6 quoted lines

>  gpg.<format>.program::
>  	Use this to customize the program used for the signing format you
>  	chose. (see `gpg.program` and `gpg.format`) `gpg.program` can still
>  	be used as a legacy synonym for `gpg.openpgp.program`. The default
> -	value for `gpg.x509.program` is "gpgsm".
> +	value for `gpg.x509.program` is "gpgsm" and `gpg.ssh.program` is "ssh-keygen".

Again, makes sense.

Once the dust settles, we might want to move the hierarchy from gpg.* to a more neutral name, with proper backward compatibility migration plan, but there is no need to do so right away.

Below, I'll ask many questions. They are mostly not rhetorical and questions that you should anticipate readers of the documentation will ask (hence, you would want to update your documentation in such a way that future readers will not have to ask for clarification).

Show 7 quoted lines

> @@ -33,3 +33,34 @@ gpg.minTrustLevel::
>  * `marginal`
>  * `fully`
>  * `ultimate`
> +
> +gpg.ssh.keyring::
> +	A file containing all valid SSH public signing keys.

Is "SSH public signing key" the phrase we want to use here? At first glance I mistakenly thought that I maintain a bag of my keys I will use for signing, but from the mention of "authorized keys", it apparently is the other way around, i.e. I have a bag of public keys that I can use to _verify_ signatures other people made.

What do we exactly want to convey with the phrase "all valid" to our readers? Even if I have a valid SSH key that I could sign with, if you and your project do not trust me enough, such a valid key of mine would not be in your keyring, so the phrase "all valid keys" is not all that meaningful without further qualification in the context of this sentence. A file containing ssh public keys, signatures made with which you are willing to accept, or something?

> +	Similar to an .ssh/authorized_keys file.

It is unclear what "similarity" is of interest here. Similar to authorized keys file, meaning that presense of this file allows holders of the listed ssh keys to remotely log-in to the repository? I somehow doubt that it is what you meant, but then ...? Did you mean "Uses the same format as .ssh/authorized_keys file" or something like that?

> +	See ssh-keygen(1) "ALLOWED SIGNERS" for details.
> +	If a signing key is found in this file then the trust level will
> +	be set to "fully". Otherwise if the key is not present
> +	but the signature is still valid then the trust level will be "undefined".

I tried to look up the "ALLOWED SIGNERS" section for details, but failed to find what "trust level" is and how trusted "fully" level is (is there higher or lower trust levels than that???). Or is the notion of "trust level" foreign to ssh signing world and the readers are expected to read this description as "listed ones are treated as having the same trust level as 'fully' trusted keys in the GPG/PGP world"?

I suspect that the section is only useful to learn the details of what the file looks like? If so, perhaps instead of saying that the keyring file looks similar to authorized-keys, be more direct and say that the keyring file uses the "ALLOWED SIGNERS" file format described in that manual page (i.e. bypassing the redirection of authorized-keys)?

Show 6 quoted lines

> +	This file can be set to a location outside of the repository
> +	and every developer maintains their own trust store.
> +	A central repository server could generate this file automatically
> +	from ssh keys with push	access to verify the code against.
> +	In a corporate setting this file is probably generated at a global location
> +	from some automation that already handles developer ssh keys.

OK.

> +	A repository that is only allowing signed commits can store the file

"is only allowing" -> "only allows".

> +	in the repository itself using a relative path.

It is unclear relative to what. Relative to the top-level of the working tree?

> +	This way only committers
> +	with an already valid key can add or change keys in the keyring.

OK.

Show 5 quoted lines

> +	Using a SSH CA key with the cert-authority option
> +	(see ssh-keygen(1) "CERTIFICATES") is also valid.
> +
> +	To revoke a key place the public key without the principal into the
> +	revocationKeyring.

All of the above unfortunately would not format correctly with multiple paragraphs. The second and subsequent paragraphs are preceded by a line with single '+' on it (instead of a blank line) and not indented.

Mimick the way the entry for "ssh.variant" uses multiple paragraphs.

Show 19 quoted lines

> +gpg.ssh.revocationKeyring::
> +	Either a SSH KRL or a list of revoked public keys (without the principal prefix).
> +	See ssh-keygen(1) for details.
> +	If a public key is found in this file then it will always be treated
> +	as having trust level "never" and signatures will show as invalid.
> diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt
> index 59aec7c3aed..b3c2f2c541e 100644
> --- a/Documentation/config/user.txt
> +++ b/Documentation/config/user.txt
> @@ -36,3 +36,9 @@ user.signingKey::
>  	commit, you can override the default selection with this variable.
>  	This option is passed unchanged to gpg's --local-user parameter,
>  	so you may specify a key using any method that gpg supports.
> +	If gpg.format is set to "ssh" this can contain the literal ssh public
> +	key (e.g.: "ssh-rsa XXXXXX identifier") or a file which contains it and
> +	corresponds to the private key used for signing. The private key
> +	needs to be available via ssh-agent. Alternatively it can be set to
> +	a file containing a private key directly. If not set git will call
> +	"ssh-add -L" and try to use the first key available.

Thanks.