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

Commit Message

Fabian Stelzer July 28, 2021, 7:36 p.m. UTC

From: Fabian Stelzer <[e-mail]>

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.

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.

To revoke a key put the public key without the principal prefix into
gpg.ssh.revocationKeyring or generate a KRL (see ssh-keygen(1)
"KEY REVOCATION LISTS"). The same considerations about who to trust for
verification as with the allowedSignersFile apply.

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.

Signed-off-by: Fabian Stelzer <[e-mail]>
---
 builtin/receive-pack.c |   2 +
 gpg-interface.c        | 179 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 180 insertions(+), 1 deletion(-)

Comments

Junio C Hamano July 28, 2021, 9:55 p.m. UTC | #1

"Fabian Stelzer via GitGitGadget" <[e-mail]> writes:

> From: Fabian Stelzer <[e-mail]>
>
> to verify a ssh signature we first call ssh-keygen -Y find-principal to

"to" -> "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.
>
> 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.
>
> To revoke a key put the public key without the principal prefix into
> gpg.ssh.revocationKeyring or generate a KRL (see ssh-keygen(1)
> "KEY REVOCATION LISTS"). The same considerations about who to trust for
> verification as with the allowedSignersFile apply.
>
> 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.
>
> Signed-off-by: Fabian Stelzer <[e-mail]>
> ---
>  builtin/receive-pack.c |   2 +
>  gpg-interface.c        | 179 ++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 180 insertions(+), 1 deletion(-)

A lot of additions to support a new system, all looking quite
straight-forward.

> @@ -78,7 +84,7 @@ static struct gpg_format gpg_format[] = {
>  		.program = "ssh-keygen",
>  		.verify_args = ssh_verify_args,
>  		.sigs = ssh_sigs,
> -		.verify_signed_buffer = NULL, /* TODO */
> +		.verify_signed_buffer = verify_ssh_signed_buffer,
>  		.sign_buffer = sign_buffer_ssh
>  	},
>  };

Nice.

> @@ -343,6 +349,165 @@ static int verify_gpg_signed_buffer(struct signature_check *sigc,
>  	return ret;
>  }
>  
> +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

A bit unfortunate line that is overly long.  These two are not
mutually exclusive two different choices, but one is a special case
of the other, no?  How about phrasing it like so instead?

	/*
	 * ssh-keysign output should be:
	 * Good "git" signature for PRINCIPAL with RSA key SHA256:FINGERPRINT
         *
	 * or for valid but unknown keys:
	 * Good "git" signature with RSA key SHA256:FINGERPRINT
         *
	 * Note that "PRINCIPAL" can contain whitespace, "RSA" and
	 * "SHA256" part could be a different token that names of
	 * the algorithms used, and "FINGERPRINT" is a hexadecimal
         * string.  By finding the last occurence of " with ", we can
         * reliably parse out the PRINCIPAL.
	 */

> +	 * or for valid but unknown keys:
> +	 * Good "git" signature with RSA key SHA256:FINGERPRINT
> +	 */
> +	sigc->result = 'B';
> +	sigc->trust_level = TRUST_NEVER;
> +
> +	line = xmemdupz(sigc->output, strcspn(sigc->output, "\n"));
> +
> +	if (skip_prefix(line, "Good \"git\" signature for ", &line)) {
> +		/* Valid signature and known principal */
> +		sigc->result = 'G';
> +		sigc->trust_level = TRUST_FULLY;
> +
> +		/* Search for the last "with" to get the full principal */
> +		principal = line;
> +		do {
> +			search = strstr(line, " with ");
> +			if (search)
> +				line = search + 1;
> +		} while (search != NULL);
> +		sigc->signer = xmemdupz(principal, line - principal - 1);
> +		sigc->fingerprint = xstrdup(strstr(line, "key") + 4);

OK.  This does not care the "RSA" part, which is future resistant.
It assumes the <algo>:<fingerprint> comes after literal " key ",
which I think is a reasonable thing to do.

However, we never checked if the line has "key" in it, so
strstr(line, "key") + 4 may not be pointing at where this code
expects.

> +		sigc->key = xstrdup(sigc->fingerprint);
> +	} else if (skip_prefix(line, "Good \"git\" signature with ", &line)) {
> +		/* Valid signature, but key unknown */
> +		sigc->result = 'G';
> +		sigc->trust_level = TRUST_UNDEFINED;
> +		sigc->fingerprint = xstrdup(strstr(line, "key") + 4);
> +		sigc->key = xstrdup(sigc->fingerprint);

Likewise, I guess.

> +	}
> +}
> +
> +static int verify_ssh_signed_buffer(struct signature_check *sigc,
> +				    struct gpg_format *fmt, const char *payload,
> +				    size_t payload_size, const char *signature,
> +				    size_t signature_size)
> +{
> +	struct child_process ssh_keygen = CHILD_PROCESS_INIT;
> +	struct tempfile *buffer_file;
> +	int ret = -1;
> +	const char *line;
> +	size_t trust_size;
> +	char *principal;
> +	struct strbuf ssh_keygen_out = STRBUF_INIT;
> +	struct strbuf ssh_keygen_err = STRBUF_INIT;
> +
> +	if (!ssh_allowed_signers) {
> +		error(_("gpg.ssh.allowedSignersFile needs to be configured and exist for ssh signature verification"));
> +		return -1;
> +	}
> +
> +	buffer_file = mks_tempfile_t(".git_vtag_tmpXXXXXX");
> +	if (!buffer_file)
> +		return error_errno(_("could not create temporary file"));
> +	if (write_in_full(buffer_file->fd, signature, signature_size) < 0 ||
> +	    close_tempfile_gently(buffer_file) < 0) {
> +		error_errno(_("failed writing detached signature to '%s'"),
> +			    buffer_file->filename.buf);
> +		delete_tempfile(&buffer_file);
> +		return -1;
> +	}
> +
> +	/* Find the principal from the signers */
> +	strvec_pushl(&ssh_keygen.args, fmt->program,
> +		     "-Y", "find-principals",
> +		     "-f", ssh_allowed_signers,
> +		     "-s", buffer_file->filename.buf,
> +		     NULL);
> +	ret = pipe_command(&ssh_keygen, NULL, 0, &ssh_keygen_out, 0,
> +			   &ssh_keygen_err, 0);
> +	if (ret && strstr(ssh_keygen_err.buf, "usage:")) {
> +		error(_("ssh-keygen -Y find-principals/verify is needed for ssh signature verification (available in openssh version 8.2p1+)"));
> +		goto out;
> +	}
> +	if (ret || !ssh_keygen_out.len) {
> +		/* We did not find a matching principal in the allowedSigners - Check
> +		 * without validation */
> +		child_process_init(&ssh_keygen);
> +		strvec_pushl(&ssh_keygen.args, fmt->program,
> +			     "-Y", "check-novalidate",
> +			     "-n", "git",
> +			     "-s", buffer_file->filename.buf,
> +			     NULL);
> +		ret = pipe_command(&ssh_keygen, payload, payload_size,
> +				   &ssh_keygen_out, 0, &ssh_keygen_err, 0);
> +	} else {
> +		/* Check every principal we found (one per line) */
> +		for (line = ssh_keygen_out.buf; *line;
> +		     line = strchrnul(line + 1, '\n')) {
> +			while (*line == '\n')
> +				line++;
> +			if (!*line)
> +				break;
> +
> +			trust_size = strcspn(line, "\n");
> +			principal = xmemdupz(line, trust_size);
> +
> +			child_process_init(&ssh_keygen);
> +			strbuf_release(&ssh_keygen_out);
> +			strbuf_release(&ssh_keygen_err);
> +			strvec_push(&ssh_keygen.args, fmt->program);
> +			/* We found principals - Try with each until we find a
> +			 * match */

                        /*
                         * Do not forget our multi-line comment
                         * style, please.
                         */

> +			strvec_pushl(&ssh_keygen.args, "-Y", "verify",
> +				     "-n", "git",
> +				     "-f", ssh_allowed_signers,
> +				     "-I", principal,
> +				     "-s", buffer_file->filename.buf,
> +				     NULL);
> +
> +			if (ssh_revocation_file) {
> +				if (file_exists(ssh_revocation_file)) {
> +					strvec_pushl(&ssh_keygen.args, "-r",
> +						     ssh_revocation_file, NULL);
> +				} else {
> +					warning(_("ssh signing revocation file configured but not found: %s"),
> +						ssh_revocation_file);
> +				}
> +			}
> +
> +			sigchain_push(SIGPIPE, SIG_IGN);
> +			ret = pipe_command(&ssh_keygen, payload, payload_size,
> +					   &ssh_keygen_out, 0, &ssh_keygen_err, 0);
> +			sigchain_pop(SIGPIPE);
> +
> +			FREE_AND_NULL(principal);
> +
> +			ret &= starts_with(ssh_keygen_out.buf, "Good");

This is somewhat unusual construct in our codebase, I suspect.  And
probably is even wrong.  Didn't you mean

			if (!ret)
				ret = starts_with(...);

instead?  Surely, when pipe_command() failed, it is likely that
ssh_keygen_out may not have anything useful, and checking what the
first up-to-four bytes of it contain unconditionally may be cheap
enough, but the person reading the code would expect you to peek
into the result only when you actually got the result, no?

> +			if (ret == 0)
> +				break;

It's more common to do

			if (!ret)
				break;

in our codebase; in other words, we prefer not to compare with
literal 0, like "if (x == 0)" or "if (y != 0)".

Thanks.

Jonathan Tan July 28, 2021, 11:04 p.m. UTC | #2

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

Fabian Stelzer July 29, 2021, 9:12 a.m. UTC | #3

On 28.07.21 23:55, Junio C Hamano wrote:
> "Fabian Stelzer via GitGitGadget" <[e-mail]> writes:
> 
>> From: Fabian Stelzer <[e-mail]>
>>
>> to verify a ssh signature we first call ssh-keygen -Y find-principal to
> 
> "to" -> "To".

fixed

[snip]

>> +	/*
>> +	 * 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
> 
> A bit unfortunate line that is overly long.  These two are not
> mutually exclusive two different choices, but one is a special case
> of the other, no?  How about phrasing it like so instead?
> 
> 	/*
> 	 * ssh-keysign output should be:
> 	 * Good "git" signature for PRINCIPAL with RSA key SHA256:FINGERPRINT
>           *
> 	 * or for valid but unknown keys:
> 	 * Good "git" signature with RSA key SHA256:FINGERPRINT
>           *
> 	 * Note that "PRINCIPAL" can contain whitespace, "RSA" and
> 	 * "SHA256" part could be a different token that names of
> 	 * the algorithms used, and "FINGERPRINT" is a hexadecimal
>           * string.  By finding the last occurence of " with ", we can
>           * reliably parse out the PRINCIPAL.
> 	 */
> 

Yes, it's a special case that makes it a bit harder to parse. I will 
change the comment like you suggested. That makes it clear.

>> +		/* Search for the last "with" to get the full principal */
>> +		principal = line;
>> +		do {
>> +			search = strstr(line, " with ");
>> +			if (search)
>> +				line = search + 1;
>> +		} while (search != NULL);
>> +		sigc->signer = xmemdupz(principal, line - principal - 1);
>> +		sigc->fingerprint = xstrdup(strstr(line, "key") + 4);
> 
> OK.  This does not care the "RSA" part, which is future resistant.
> It assumes the <algo>:<fingerprint> comes after literal " key ",
> which I think is a reasonable thing to do.
> 
> However, we never checked if the line has "key" in it, so
> strstr(line, "key") + 4 may not be pointing at where this code
> expects.
> 

Hmm. What would i do if i don't find "key"? Still mark the signature as 
valid an just leave fingerprint & key empty?

>> +			/* We found principals - Try with each until we find a
>> +			 * match */
> 
>                          /*
>                           * Do not forget our multi-line comment
>                           * style, please.
>                           */
> 

fixed. clang-format wordwrapped those :/


>> +
>> +			ret &= starts_with(ssh_keygen_out.buf, "Good");
> 
> This is somewhat unusual construct in our codebase, I suspect.  And
> probably is even wrong.  Didn't you mean
> 
> 			if (!ret)
> 				ret = starts_with(...);
> 
> instead?  Surely, when pipe_command() failed, it is likely that
> ssh_keygen_out may not have anything useful, and checking what the
> first up-to-four bytes of it contain unconditionally may be cheap
> enough, but the person reading the code would expect you to peek
> into the result only when you actually got the result, no?

you are correct. we don't need to look at the output when the command fails.

> 
>> +			if (ret == 0)
>> +				break;
> 
> It's more common to do
> 
> 			if (!ret)
> 				break;
> 

changed.

Thanks

Fabian Stelzer July 29, 2021, 9:48 a.m. UTC | #4

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

Fabian Stelzer July 29, 2021, 1:52 p.m. UTC | #5

On 29.07.21 11:48, Fabian Stelzer wrote:
> 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.
> 

I thought a bit more about this and my approach is indeed problematic 
especially when a repo has both gpg and ssh signatures. The trust level 
setting can then not behave differently for both.

My intention of still showing valid but unknown signatures in the log as 
ok (but unknown) was to encourage users to always sign their work even 
if they are not (yet) trusted in the allowedSignersFile.

I think the way forward should be to treat unknown singing keys as not 
verified like gpg does.

If a ssh key is verified and in the allowedSignersFile i would still set 
its trust level to "FULLY".

Junio C Hamano July 29, 2021, 8:43 p.m. UTC | #6

Fabian Stelzer <[e-mail]> writes:

>>> +		/* Search for the last "with" to get the full principal */
>>> +		principal = line;
>>> +		do {
>>> +			search = strstr(line, " with ");
>>> +			if (search)
>>> +				line = search + 1;
>>> +		} while (search != NULL);
>>> +		sigc->signer = xmemdupz(principal, line - principal - 1);
>>> +		sigc->fingerprint = xstrdup(strstr(line, "key") + 4);
>> OK.  This does not care the "RSA" part, which is future resistant.
>> It assumes the <algo>:<fingerprint> comes after literal " key ",
>> which I think is a reasonable thing to do.
>> However, we never checked if the line has "key" in it, so
>> strstr(line, "key") + 4 may not be pointing at where this code
>> expects.
>
> Hmm. What would i do if i don't find "key"? Still mark the signature
> as valid an just leave fingerprint & key empty?

We didn't get a satisfactory response from the ssh-keygen we expect
that tells us that the external tool successfully decided that the
signature is good or bad.  I would feel safer if we said we did not
see a good signature in such a case.

Junio C Hamano July 29, 2021, 8:46 p.m. UTC | #7

Fabian Stelzer <[e-mail]> writes:

> On 29.07.21 01:04, Jonathan Tan wrote:
>
>> Also, is this output documented to be stable even across locales?
> Not really :/ (it currently is not locale specific)

We probably want to defeat l10n of the message by spawning it in the
C locale regardless.

> The documentation states to only check the commands exit code. Do we
> trust the exit code enough to rely on it for verification?

Is the exit code sufficient to learn who signed it?  Without knowing
that, we cannot see if the principal is in or not in our keychain,
no?

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

Does ssh-keygen have a mode similar to gpg's --status-fd feature
where its output is geared more towards being stable and marchine
parseable than being human friendly, by the way?

Thanks.

Randall S. Becker July 29, 2021, 9:01 p.m. UTC | #8

On July 29, 2021 4:46 PM, Junio wrote:
>Fabian Stelzer <[e-mail]> writes:
>
>> On 29.07.21 01:04, Jonathan Tan wrote:
>>
>>> Also, is this output documented to be stable even across locales?
>> Not really :/ (it currently is not locale specific)
>
>We probably want to defeat l10n of the message by spawning it in the C locale regardless.
>
>> The documentation states to only check the commands exit code. Do we
>> trust the exit code enough to rely on it for verification?
>
>Is the exit code sufficient to learn who signed it?  Without knowing that, we cannot see if the principal is in or not in our
keychain, no?

Have we not had issues in the past depending on exit code? I'm not sure this can be made entirely portable.

>> 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 ");
>
>Does ssh-keygen have a mode similar to gpg's --status-fd feature where its output is geared more towards being stable and marchine
>parseable than being human friendly, by the way?

I do not think this can be done in a platform independent way. Not every platform that has ssh-keygen conforms to the OpenSSH UI or
output - a particular annoyance I get daily.

Fabian Stelzer July 29, 2021, 9:12 p.m. UTC | #9

On 29.07.21 23:01, Randall S. Becker wrote:
> On July 29, 2021 4:46 PM, Junio wrote:
>> Fabian Stelzer <[e-mail]> writes:
>>
>>> On 29.07.21 01:04, Jonathan Tan wrote:
>>>
>>>> Also, is this output documented to be stable even across locales?
>>> Not really :/ (it currently is not locale specific)
>>
>> We probably want to defeat l10n of the message by spawning it in the C locale regardless.
>>
>>> The documentation states to only check the commands exit code. Do we
>>> trust the exit code enough to rely on it for verification?
>>
>> Is the exit code sufficient to learn who signed it?  Without knowing that, we cannot see if the principal is in or not in our
> keychain, no?
> 
> Have we not had issues in the past depending on exit code? I'm not sure this can be made entirely portable.
>

To find the principal (who signed it) we don't have to parse the output. 
Since verification is first a call to look up the principals matching 
the signatures public key from the allowedSignersFile and then trying 
verification with each one we already know which one matched (usually 
there is only one. I think multiples is only possible with an SSH CA).
Of course this even more relies on the exit code of ssh-keygen.

Not sure which is more portable and reliable. Parsing the textual output 
or the exit code. At the moment my patch does both.

>>> 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 ");
>>
>> Does ssh-keygen have a mode similar to gpg's --status-fd feature where its output is geared more towards being stable and marchine
>> parseable than being human friendly, by the way?
> 
> I do not think this can be done in a platform independent way. Not every platform that has ssh-keygen conforms to the OpenSSH UI or
> output - a particular annoyance I get daily.
>

Randall S. Becker July 29, 2021, 9:25 p.m. UTC | #10

On July 29, 2021 5:13 PM, Fabian Stelzer wrote:
>Subject: Re: [PATCH v6 5/9] ssh signing: parse ssh-keygen output and verify signatures
>
>On 29.07.21 23:01, Randall S. Becker wrote:
>> On July 29, 2021 4:46 PM, Junio wrote:
>>> Fabian Stelzer <[e-mail]> writes:
>>>
>>>> On 29.07.21 01:04, Jonathan Tan wrote:
>>>>
>>>>> Also, is this output documented to be stable even across locales?
>>>> Not really :/ (it currently is not locale specific)
>>>
>>> We probably want to defeat l10n of the message by spawning it in the C locale regardless.
>>>
>>>> The documentation states to only check the commands exit code. Do we
>>>> trust the exit code enough to rely on it for verification?
>>>
>>> Is the exit code sufficient to learn who signed it?  Without knowing
>>> that, we cannot see if the principal is in or not in our
>> keychain, no?
>>
>> Have we not had issues in the past depending on exit code? I'm not sure this can be made entirely portable.
>>
>
>To find the principal (who signed it) we don't have to parse the output.
>Since verification is first a call to look up the principals matching the signatures public key from the allowedSignersFile and then trying
>verification with each one we already know which one matched (usually there is only one. I think multiples is only possible with an SSH
>CA).
>Of course this even more relies on the exit code of ssh-keygen.
>
>Not sure which is more portable and reliable. Parsing the textual output or the exit code. At the moment my patch does both.

What about a configurable exit code for this? See the comment below about that.

>>>> 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 ");
>>>
>>> Does ssh-keygen have a mode similar to gpg's --status-fd feature
>>> where its output is geared more towards being stable and marchine parseable than being human friendly, by the way?
>>
>> I do not think this can be done in a platform independent way. Not
>> every platform that has ssh-keygen conforms to the OpenSSH UI or output - a particular annoyance I get daily.
>>

What about a configurable command, like GIT_SSH_COMMAND to allow someone to plug in a mechanism or write something that supplies a result you can handle? That's something I could probably work out on my own platforms.

Fabian Stelzer July 29, 2021, 9:28 p.m. UTC | #11

On 29.07.21 23:25, Randall S. Becker wrote:
> On July 29, 2021 5:13 PM, Fabian Stelzer wrote:
>> Subject: Re: [PATCH v6 5/9] ssh signing: parse ssh-keygen output and verify signatures
>>
>> On 29.07.21 23:01, Randall S. Becker wrote:
>>> On July 29, 2021 4:46 PM, Junio wrote:
>>>> Fabian Stelzer <[e-mail]> writes:
>>>>
>>>>> On 29.07.21 01:04, Jonathan Tan wrote:
>>>>>
>>>>>> Also, is this output documented to be stable even across locales?
>>>>> Not really :/ (it currently is not locale specific)
>>>>
>>>> We probably want to defeat l10n of the message by spawning it in the C locale regardless.
>>>>
>>>>> The documentation states to only check the commands exit code. Do we
>>>>> trust the exit code enough to rely on it for verification?
>>>>
>>>> Is the exit code sufficient to learn who signed it?  Without knowing
>>>> that, we cannot see if the principal is in or not in our
>>> keychain, no?
>>>
>>> Have we not had issues in the past depending on exit code? I'm not sure this can be made entirely portable.
>>>
>>
>> To find the principal (who signed it) we don't have to parse the output.
>> Since verification is first a call to look up the principals matching the signatures public key from the allowedSignersFile and then trying
>> verification with each one we already know which one matched (usually there is only one. I think multiples is only possible with an SSH
>> CA).
>> Of course this even more relies on the exit code of ssh-keygen.
>>
>> Not sure which is more portable and reliable. Parsing the textual output or the exit code. At the moment my patch does both.
> 
> What about a configurable exit code for this? See the comment below about that.
>

I'm not sure what you mean. Something like "treat exit(123) as success"?

>>>>> 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 ");
>>>>
>>>> Does ssh-keygen have a mode similar to gpg's --status-fd feature
>>>> where its output is geared more towards being stable and marchine parseable than being human friendly, by the way?
>>>
>>> I do not think this can be done in a platform independent way. Not
>>> every platform that has ssh-keygen conforms to the OpenSSH UI or output - a particular annoyance I get daily.
>>>
> 
> What about a configurable command, like GIT_SSH_COMMAND to allow someone to plug in a mechanism or write something that supplies a result you can handle? That's something I could probably work out on my own platforms.
> 

This is already possible by setting gpg.ssh.program (although you'd have 
to pass the sign operation as well)

Randall S. Becker July 29, 2021, 10:28 p.m. UTC | #12

On July 29, 2021 5:29 PM, Fabian Stelzer wrote:
>On 29.07.21 23:25, Randall S. Becker wrote:
>> On July 29, 2021 5:13 PM, Fabian Stelzer wrote:
>>> Subject: Re: [PATCH v6 5/9] ssh signing: parse ssh-keygen output and
>>> verify signatures
>>>
>>> On 29.07.21 23:01, Randall S. Becker wrote:
>>>> On July 29, 2021 4:46 PM, Junio wrote:
>>>>> Fabian Stelzer <[e-mail]> writes:
>>>>>
>>>>>> On 29.07.21 01:04, Jonathan Tan wrote:
>>>>>>
>>>>>>> Also, is this output documented to be stable even across locales?
>>>>>> Not really :/ (it currently is not locale specific)
>>>>>
>>>>> We probably want to defeat l10n of the message by spawning it in the C locale regardless.
>>>>>
>>>>>> The documentation states to only check the commands exit code. Do
>>>>>> we trust the exit code enough to rely on it for verification?
>>>>>
>>>>> Is the exit code sufficient to learn who signed it?  Without
>>>>> knowing that, we cannot see if the principal is in or not in our
>>>> keychain, no?
>>>>
>>>> Have we not had issues in the past depending on exit code? I'm not sure this can be made entirely portable.
>>>>
>>>
>>> To find the principal (who signed it) we don't have to parse the output.
>>> Since verification is first a call to look up the principals matching
>>> the signatures public key from the allowedSignersFile and then trying
>>> verification with each one we already know which one matched (usually there is only one. I think multiples is only possible with an SSH
>CA).
>>> Of course this even more relies on the exit code of ssh-keygen.
>>>
>>> Not sure which is more portable and reliable. Parsing the textual output or the exit code. At the moment my patch does both.
>>
>> What about a configurable exit code for this? See the comment below about that.
>>
>
>I'm not sure what you mean. Something like "treat exit(123) as success"?

How about gpg.ssh.successExit=123 or something like that.

>>>>>> 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 ");
>>>>>
>>>>> Does ssh-keygen have a mode similar to gpg's --status-fd feature
>>>>> where its output is geared more towards being stable and marchine parseable than being human friendly, by the way?
>>>>
>>>> I do not think this can be done in a platform independent way. Not
>>>> every platform that has ssh-keygen conforms to the OpenSSH UI or output - a particular annoyance I get daily.
>>>>
>>
>> What about a configurable command, like GIT_SSH_COMMAND to allow someone to plug in a mechanism or write something that
>supplies a result you can handle? That's something I could probably work out on my own platforms.
>>
>
>This is already possible by setting gpg.ssh.program (although you'd have to pass the sign operation as well)

Is there documentation on the possible arguments the patch series will use for this so one can create a wrapper script? I had to look into the code to find out what GIT_SSH_COMMAND actually required when the ssh variant was "ssh". I'd rather not have to do that in this case.

Thanks,
Randall

Fabian Stelzer July 30, 2021, 8:17 a.m. UTC | #13

On 30.07.21 00:28, Randall S. Becker wrote:
> On July 29, 2021 5:29 PM, Fabian Stelzer wrote:
>> On 29.07.21 23:25, Randall S. Becker wrote:
>>> On July 29, 2021 5:13 PM, Fabian Stelzer wrote:
>>>> Subject: Re: [PATCH v6 5/9] ssh signing: parse ssh-keygen output and
>>>> verify signatures
>>>>
>>>> On 29.07.21 23:01, Randall S. Becker wrote:
>>>>> On July 29, 2021 4:46 PM, Junio wrote:
>>>>>> Fabian Stelzer <[e-mail]> writes:
>>>>>>
>>>>>>> On 29.07.21 01:04, Jonathan Tan wrote:
>>>>>>>
>>>>>>>> Also, is this output documented to be stable even across locales?
>>>>>>> Not really :/ (it currently is not locale specific)
>>>>>>
>>>>>> We probably want to defeat l10n of the message by spawning it in the C locale regardless.
>>>>>>
>>>>>>> The documentation states to only check the commands exit code. Do
>>>>>>> we trust the exit code enough to rely on it for verification?
>>>>>>
>>>>>> Is the exit code sufficient to learn who signed it?  Without
>>>>>> knowing that, we cannot see if the principal is in or not in our
>>>>> keychain, no?
>>>>>
>>>>> Have we not had issues in the past depending on exit code? I'm not sure this can be made entirely portable.
>>>>>
>>>>
>>>> To find the principal (who signed it) we don't have to parse the output.
>>>> Since verification is first a call to look up the principals matching
>>>> the signatures public key from the allowedSignersFile and then trying
>>>> verification with each one we already know which one matched (usually there is only one. I think multiples is only possible with an SSH
>> CA).
>>>> Of course this even more relies on the exit code of ssh-keygen.
>>>>
>>>> Not sure which is more portable and reliable. Parsing the textual output or the exit code. At the moment my patch does both.
>>>
>>> What about a configurable exit code for this? See the comment below about that.
>>>
>>
>> I'm not sure what you mean. Something like "treat exit(123) as success"?
> 
> How about gpg.ssh.successExit=123 or something like that.
>

I don't quite understand what the benefit would be. Do you have any 
specific portability problems/concerns where the ssh-keygen format is 
different or exit codes differ?
I think using a script that provides exit(0) on success and the correct 
output to wrap ssh-keygen and setting it in gpg.ssh.command can already 
cover edge cases when needed.

> 
> Is there documentation on the possible arguments the patch series will use for this so one can create a wrapper script? I had to look into the code to find out what GIT_SSH_COMMAND actually required when the ssh variant was "ssh". I'd rather not have to do that in this case.
> 

The documentation in ssh-keygen(1) is quite good and straight forward 
for verification and signing. Again if you have any specific portability 
concerns i'd be glad to help.

Randall S. Becker July 30, 2021, 2:26 p.m. UTC | #14

On July 30, 2021 4:17 AM, Fabian Stelzer wrote:
>Subject: Re: [PATCH v6 5/9] ssh signing: parse ssh-keygen output and verify signatures
>
>On 30.07.21 00:28, Randall S. Becker wrote:
>> On July 29, 2021 5:29 PM, Fabian Stelzer wrote:
>>> On 29.07.21 23:25, Randall S. Becker wrote:
>>>> On July 29, 2021 5:13 PM, Fabian Stelzer wrote:
>>>>> Subject: Re: [PATCH v6 5/9] ssh signing: parse ssh-keygen output
>>>>> and verify signatures
>>>>>
>>>>> On 29.07.21 23:01, Randall S. Becker wrote:
>>>>>> On July 29, 2021 4:46 PM, Junio wrote:
>>>>>>> Fabian Stelzer <[e-mail]> writes:
>>>>>>>
>>>>>>>> On 29.07.21 01:04, Jonathan Tan wrote:
>>>>>>>>
>>>>>>>>> Also, is this output documented to be stable even across locales?
>>>>>>>> Not really :/ (it currently is not locale specific)
>>>>>>>
>>>>>>> We probably want to defeat l10n of the message by spawning it in the C locale regardless.
>>>>>>>
>>>>>>>> The documentation states to only check the commands exit code.
>>>>>>>> Do we trust the exit code enough to rely on it for verification?
>>>>>>>
>>>>>>> Is the exit code sufficient to learn who signed it?  Without
>>>>>>> knowing that, we cannot see if the principal is in or not in our
>>>>>> keychain, no?
>>>>>>
>>>>>> Have we not had issues in the past depending on exit code? I'm not sure this can be made entirely portable.
>>>>>>
>>>>>
>>>>> To find the principal (who signed it) we don't have to parse the output.
>>>>> Since verification is first a call to look up the principals
>>>>> matching the signatures public key from the allowedSignersFile and
>>>>> then trying verification with each one we already know which one
>>>>> matched (usually there is only one. I think multiples is only
>>>>> possible with an SSH
>>> CA).
>>>>> Of course this even more relies on the exit code of ssh-keygen.
>>>>>
>>>>> Not sure which is more portable and reliable. Parsing the textual output or the exit code. At the moment my patch does both.
>>>>
>>>> What about a configurable exit code for this? See the comment below about that.
>>>>
>>>
>>> I'm not sure what you mean. Something like "treat exit(123) as success"?
>>
>> How about gpg.ssh.successExit=123 or something like that.
>>
>
>I don't quite understand what the benefit would be. Do you have any specific portability problems/concerns where the ssh-keygen format
>is different or exit codes differ?
>I think using a script that provides exit(0) on success and the correct output to wrap ssh-keygen and setting it in gpg.ssh.command can
>already cover edge cases when needed.
>
>>
>> Is there documentation on the possible arguments the patch series will use for this so one can create a wrapper script? I had to look into
>the code to find out what GIT_SSH_COMMAND actually required when the ssh variant was "ssh". I'd rather not have to do that in this case.
>>
>
>The documentation in ssh-keygen(1) is quite good and straight forward for verification and signing. Again if you have any specific
>portability concerns i'd be glad to help.

I do know the ssh-keygen interface and that does not really answer my doubts.

My point here is that ssh-keygen is not always available in the same form on all platforms. Providing a full emulation of all arguments is not effective or likely even possible, and a waste of time. I'm asking for documentation on what specific options you are using for each function. OpenSSL is not available everywhere, and even where it is, the latest versions are not always available. It is important to know what the specific interface is being used.

Fabian Stelzer July 30, 2021, 2:32 p.m. UTC | #15

On 30.07.21 16:26, Randall S. Becker wrote:
> On July 30, 2021 4:17 AM, Fabian Stelzer wrote:
>> Subject: Re: [PATCH v6 5/9] ssh signing: parse ssh-keygen output and verify signatures
>>
>> On 30.07.21 00:28, Randall S. Becker wrote:
>>> On July 29, 2021 5:29 PM, Fabian Stelzer wrote:
>>>> On 29.07.21 23:25, Randall S. Becker wrote:
>>>>> On July 29, 2021 5:13 PM, Fabian Stelzer wrote:
>>>>>> Subject: Re: [PATCH v6 5/9] ssh signing: parse ssh-keygen output
>>>>>> and verify signatures
>>>>>>
>>>>>> On 29.07.21 23:01, Randall S. Becker wrote:
>>>>>>> On July 29, 2021 4:46 PM, Junio wrote:
>>>>>>>> Fabian Stelzer <[e-mail]> writes:
>>>>>>>>
>>>>>>>>> On 29.07.21 01:04, Jonathan Tan wrote:
>>>>>>>>>
>>>>>>>>>> Also, is this output documented to be stable even across locales?
>>>>>>>>> Not really :/ (it currently is not locale specific)
>>>>>>>>
>>>>>>>> We probably want to defeat l10n of the message by spawning it in the C locale regardless.
>>>>>>>>
>>>>>>>>> The documentation states to only check the commands exit code.
>>>>>>>>> Do we trust the exit code enough to rely on it for verification?
>>>>>>>>
>>>>>>>> Is the exit code sufficient to learn who signed it?  Without
>>>>>>>> knowing that, we cannot see if the principal is in or not in our
>>>>>>> keychain, no?
>>>>>>>
>>>>>>> Have we not had issues in the past depending on exit code? I'm not sure this can be made entirely portable.
>>>>>>>
>>>>>>
>>>>>> To find the principal (who signed it) we don't have to parse the output.
>>>>>> Since verification is first a call to look up the principals
>>>>>> matching the signatures public key from the allowedSignersFile and
>>>>>> then trying verification with each one we already know which one
>>>>>> matched (usually there is only one. I think multiples is only
>>>>>> possible with an SSH
>>>> CA).
>>>>>> Of course this even more relies on the exit code of ssh-keygen.
>>>>>>
>>>>>> Not sure which is more portable and reliable. Parsing the textual output or the exit code. At the moment my patch does both.
>>>>>
>>>>> What about a configurable exit code for this? See the comment below about that.
>>>>>
>>>>
>>>> I'm not sure what you mean. Something like "treat exit(123) as success"?
>>>
>>> How about gpg.ssh.successExit=123 or something like that.
>>>
>>
>> I don't quite understand what the benefit would be. Do you have any specific portability problems/concerns where the ssh-keygen format
>> is different or exit codes differ?
>> I think using a script that provides exit(0) on success and the correct output to wrap ssh-keygen and setting it in gpg.ssh.command can
>> already cover edge cases when needed.
>>
>>>
>>> Is there documentation on the possible arguments the patch series will use for this so one can create a wrapper script? I had to look into
>> the code to find out what GIT_SSH_COMMAND actually required when the ssh variant was "ssh". I'd rather not have to do that in this case.
>>>
>>
>> The documentation in ssh-keygen(1) is quite good and straight forward for verification and signing. Again if you have any specific
>> portability concerns i'd be glad to help.
> 
> I do know the ssh-keygen interface and that does not really answer my doubts.
> 
> My point here is that ssh-keygen is not always available in the same form on all platforms. Providing a full emulation of all arguments is not effective or likely even possible, and a waste of time. I'm asking for documentation on what specific options you are using for each function. OpenSSL is not available everywhere, and even where it is, the latest versions are not always available. It is important to know what the specific interface is being used.
> 
> 

Fair enough. Where would you expect to look for such documentation?
I'm not sure sth like config/gpg.txt is the right place for this.

Randall S. Becker July 30, 2021, 3:05 p.m. UTC | #16

On July 30, 2021 10:32 AM, Fabian Stelzer wrote:
>On 30.07.21 16:26, Randall S. Becker wrote:
>> On July 30, 2021 4:17 AM, Fabian Stelzer wrote:
>>> Subject: Re: [PATCH v6 5/9] ssh signing: parse ssh-keygen output and
>>> verify signatures
>>>
>>> On 30.07.21 00:28, Randall S. Becker wrote:
>>>> On July 29, 2021 5:29 PM, Fabian Stelzer wrote:
>>>>> On 29.07.21 23:25, Randall S. Becker wrote:
>>>>>> On July 29, 2021 5:13 PM, Fabian Stelzer wrote:
>>>>>>> Subject: Re: [PATCH v6 5/9] ssh signing: parse ssh-keygen output
>>>>>>> and verify signatures
>>>>>>>
>>>>>>> On 29.07.21 23:01, Randall S. Becker wrote:
>>>>>>>> On July 29, 2021 4:46 PM, Junio wrote:
>>>>>>>>> Fabian Stelzer <[e-mail]> writes:
>>>>>>>>>
>>>>>>>>>> On 29.07.21 01:04, Jonathan Tan wrote:
>>>>>>>>>>
>>>>>>>>>>> Also, is this output documented to be stable even across locales?
>>>>>>>>>> Not really :/ (it currently is not locale specific)
>>>>>>>>>
>>>>>>>>> We probably want to defeat l10n of the message by spawning it in the C locale regardless.
>>>>>>>>>
>>>>>>>>>> The documentation states to only check the commands exit code.
>>>>>>>>>> Do we trust the exit code enough to rely on it for verification?
>>>>>>>>>
>>>>>>>>> Is the exit code sufficient to learn who signed it?  Without
>>>>>>>>> knowing that, we cannot see if the principal is in or not in
>>>>>>>>> our
>>>>>>>> keychain, no?
>>>>>>>>
>>>>>>>> Have we not had issues in the past depending on exit code? I'm not sure this can be made entirely portable.
>>>>>>>>
>>>>>>>
>>>>>>> To find the principal (who signed it) we don't have to parse the output.
>>>>>>> Since verification is first a call to look up the principals
>>>>>>> matching the signatures public key from the allowedSignersFile
>>>>>>> and then trying verification with each one we already know which
>>>>>>> one matched (usually there is only one. I think multiples is only
>>>>>>> possible with an SSH
>>>>> CA).
>>>>>>> Of course this even more relies on the exit code of ssh-keygen.
>>>>>>>
>>>>>>> Not sure which is more portable and reliable. Parsing the textual output or the exit code. At the moment my patch does both.
>>>>>>
>>>>>> What about a configurable exit code for this? See the comment below about that.
>>>>>>
>>>>>
>>>>> I'm not sure what you mean. Something like "treat exit(123) as success"?
>>>>
>>>> How about gpg.ssh.successExit=123 or something like that.
>>>>
>>>
>>> I don't quite understand what the benefit would be. Do you have any
>>> specific portability problems/concerns where the ssh-keygen format is different or exit codes differ?
>>> I think using a script that provides exit(0) on success and the
>>> correct output to wrap ssh-keygen and setting it in gpg.ssh.command can already cover edge cases when needed.
>>>
>>>>
>>>> Is there documentation on the possible arguments the patch series
>>>> will use for this so one can create a wrapper script? I had to look
>>>> into
>>> the code to find out what GIT_SSH_COMMAND actually required when the ssh variant was "ssh". I'd rather not have to do that in this
>case.
>>>>
>>>
>>> The documentation in ssh-keygen(1) is quite good and straight forward
>>> for verification and signing. Again if you have any specific portability concerns i'd be glad to help.
>>
>> I do know the ssh-keygen interface and that does not really answer my doubts.
>>
>> My point here is that ssh-keygen is not always available in the same form on all platforms. Providing a full emulation of all arguments is
>not effective or likely even possible, and a waste of time. I'm asking for documentation on what specific options you are using for each
>function. OpenSSL is not available everywhere, and even where it is, the latest versions are not always available. It is important to know
>what the specific interface is being used.
>>
>>
>
>Fair enough. Where would you expect to look for such documentation?
>I'm not sure sth like config/gpg.txt is the right place for this.

My suggestion is wherever gpg.ssh.command is documented. So really, I think config/gpg.txt is the place. It's that or we create some common location for compatibility layer documentation (what I would really prefer). If there is a good place to put that, I might be willing to take on the documentation task, but my $DAYJOB is keeping me from anything heavy at this point.

With my thanks,
Randall

Fabian Stelzer Aug. 3, 2021, 7:43 a.m. UTC | #17

On 29.07.21 15:52, Fabian Stelzer wrote:
> On 29.07.21 11:48, Fabian Stelzer wrote:
>> 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.
>>
> 
> I thought a bit more about this and my approach is indeed problematic 
> especially when a repo has both gpg and ssh signatures. The trust level 
> setting can then not behave differently for both.
> 
> My intention of still showing valid but unknown signatures in the log as 
> ok (but unknown) was to encourage users to always sign their work even 
> if they are not (yet) trusted in the allowedSignersFile.
> 
> I think the way forward should be to treat unknown singing keys as not 
> verified like gpg does.
> 
> If a ssh key is verified and in the allowedSignersFile i would still set 
> its trust level to "FULLY".

i dug a bit deeper into the gpg code/tests and it actually already 
behaves the same. untrusted signatures still return successfull on a 
verify-commit/tag even if the key is completely untrusted. my patch does 
the same thing for ssh signatures. i'll send a new revision later today 
with all the other fixes.

Fabian Stelzer Aug. 3, 2021, 9:33 a.m. UTC | #18

On 03.08.21 09:43, Fabian Stelzer wrote:
> 
> 
> On 29.07.21 15:52, Fabian Stelzer wrote:
>> On 29.07.21 11:48, Fabian Stelzer wrote:
>>> 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.
>>>
>>
>> I thought a bit more about this and my approach is indeed problematic 
>> especially when a repo has both gpg and ssh signatures. The trust 
>> level setting can then not behave differently for both.
>>
>> My intention of still showing valid but unknown signatures in the log 
>> as ok (but unknown) was to encourage users to always sign their work 
>> even if they are not (yet) trusted in the allowedSignersFile.
>>
>> I think the way forward should be to treat unknown singing keys as not 
>> verified like gpg does.
>>
>> If a ssh key is verified and in the allowedSignersFile i would still 
>> set its trust level to "FULLY".
> 
> i dug a bit deeper into the gpg code/tests and it actually already 
> behaves the same. untrusted signatures still return successfull on a 
> verify-commit/tag even if the key is completely untrusted. my patch does 
> the same thing for ssh signatures. i'll send a new revision later today 
> with all the other fixes.

oh boy... sorry for all the emails. the gpg stuff can be really 
confusing. especially since there's different meanings of "untrusted", 
"unknown" and "undefined" depending on which docs/codebase you look 
into. Especially "untrusted" is not really a gpg term but used in the 
codebase in tests like 'verify-commit exits success on untrusted 
signature' which tests for a key already in the keyring but not with any 
specified trust level. I could not actually find any gpg test for a 
signature that is completely unknown. (i will add one)

GPG does a successful verify-commit/tag on keys that are "known". 
Meaning that to be marked as good signatures all you need is to have the 
public key in your keyring. This key can still have an unknown/undefined 
trust level (meaning its in the keyring but no decision on trust has 
been made). A key thats not in the keyring has no trustlevel or anything 
but fails hard with "no public key".

SSH signing does not really make this distinction. A key is either in 
the allowedSigners file (and therefore trusted), completely unknown, or 
revoked via the revokedSigners file.
To make this behave like gpg does i will make verification fail on 
completely unknown keys. There is no use of the undefined trust level 
for ssh then and i will set keys in the allowedSigners file to fully 
trusted so they will be accepted for merges as well. I don't see any way 
to have keys that are valid for commits but not merge with ssh then but 
that should be the only difference to gpg.

Patch

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;
 
diff --git a/gpg-interface.c b/gpg-interface.c
index ec48a37b6cc..703225c3cd3 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -3,11 +3,13 @@ 
 #include "config.h"
 #include "run-command.h"
 #include "strbuf.h"
+#include "dir.h"
 #include "gpg-interface.h"
 #include "sigchain.h"
 #include "tempfile.h"
 
 static char *configured_signing_key;
+static const char *ssh_allowed_signers, *ssh_revocation_file;
 static enum signature_trust_level configured_min_trust_level = TRUST_UNDEFINED;
 
 struct gpg_format {
@@ -51,6 +53,10 @@  static int verify_gpg_signed_buffer(struct signature_check *sigc,
 				    struct gpg_format *fmt, const char *payload,
 				    size_t payload_size, const char *signature,
 				    size_t signature_size);
+static int verify_ssh_signed_buffer(struct signature_check *sigc,
+				    struct gpg_format *fmt, const char *payload,
+				    size_t payload_size, const char *signature,
+				    size_t signature_size);
 static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,
 			   const char *signing_key);
 static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
@@ -78,7 +84,7 @@  static struct gpg_format gpg_format[] = {
 		.program = "ssh-keygen",
 		.verify_args = ssh_verify_args,
 		.sigs = ssh_sigs,
-		.verify_signed_buffer = NULL, /* TODO */
+		.verify_signed_buffer = verify_ssh_signed_buffer,
 		.sign_buffer = sign_buffer_ssh
 	},
 };
@@ -343,6 +349,165 @@  static int verify_gpg_signed_buffer(struct signature_check *sigc,
 	return ret;
 }
 
+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
+	 */
+	sigc->result = 'B';
+	sigc->trust_level = TRUST_NEVER;
+
+	line = xmemdupz(sigc->output, strcspn(sigc->output, "\n"));
+
+	if (skip_prefix(line, "Good \"git\" signature for ", &line)) {
+		/* Valid signature and known principal */
+		sigc->result = 'G';
+		sigc->trust_level = TRUST_FULLY;
+
+		/* Search for the last "with" to get the full principal */
+		principal = line;
+		do {
+			search = strstr(line, " with ");
+			if (search)
+				line = search + 1;
+		} while (search != NULL);
+		sigc->signer = xmemdupz(principal, line - principal - 1);
+		sigc->fingerprint = xstrdup(strstr(line, "key") + 4);
+		sigc->key = xstrdup(sigc->fingerprint);
+	} else if (skip_prefix(line, "Good \"git\" signature with ", &line)) {
+		/* Valid signature, but key unknown */
+		sigc->result = 'G';
+		sigc->trust_level = TRUST_UNDEFINED;
+		sigc->fingerprint = xstrdup(strstr(line, "key") + 4);
+		sigc->key = xstrdup(sigc->fingerprint);
+	}
+}
+
+static int verify_ssh_signed_buffer(struct signature_check *sigc,
+				    struct gpg_format *fmt, const char *payload,
+				    size_t payload_size, const char *signature,
+				    size_t signature_size)
+{
+	struct child_process ssh_keygen = CHILD_PROCESS_INIT;
+	struct tempfile *buffer_file;
+	int ret = -1;
+	const char *line;
+	size_t trust_size;
+	char *principal;
+	struct strbuf ssh_keygen_out = STRBUF_INIT;
+	struct strbuf ssh_keygen_err = STRBUF_INIT;
+
+	if (!ssh_allowed_signers) {
+		error(_("gpg.ssh.allowedSignersFile needs to be configured and exist for ssh signature verification"));
+		return -1;
+	}
+
+	buffer_file = mks_tempfile_t(".git_vtag_tmpXXXXXX");
+	if (!buffer_file)
+		return error_errno(_("could not create temporary file"));
+	if (write_in_full(buffer_file->fd, signature, signature_size) < 0 ||
+	    close_tempfile_gently(buffer_file) < 0) {
+		error_errno(_("failed writing detached signature to '%s'"),
+			    buffer_file->filename.buf);
+		delete_tempfile(&buffer_file);
+		return -1;
+	}
+
+	/* Find the principal from the signers */
+	strvec_pushl(&ssh_keygen.args, fmt->program,
+		     "-Y", "find-principals",
+		     "-f", ssh_allowed_signers,
+		     "-s", buffer_file->filename.buf,
+		     NULL);
+	ret = pipe_command(&ssh_keygen, NULL, 0, &ssh_keygen_out, 0,
+			   &ssh_keygen_err, 0);
+	if (ret && strstr(ssh_keygen_err.buf, "usage:")) {
+		error(_("ssh-keygen -Y find-principals/verify is needed for ssh signature verification (available in openssh version 8.2p1+)"));
+		goto out;
+	}
+	if (ret || !ssh_keygen_out.len) {
+		/* We did not find a matching principal in the allowedSigners - Check
+		 * without validation */
+		child_process_init(&ssh_keygen);
+		strvec_pushl(&ssh_keygen.args, fmt->program,
+			     "-Y", "check-novalidate",
+			     "-n", "git",
+			     "-s", buffer_file->filename.buf,
+			     NULL);
+		ret = pipe_command(&ssh_keygen, payload, payload_size,
+				   &ssh_keygen_out, 0, &ssh_keygen_err, 0);
+	} else {
+		/* Check every principal we found (one per line) */
+		for (line = ssh_keygen_out.buf; *line;
+		     line = strchrnul(line + 1, '\n')) {
+			while (*line == '\n')
+				line++;
+			if (!*line)
+				break;
+
+			trust_size = strcspn(line, "\n");
+			principal = xmemdupz(line, trust_size);
+
+			child_process_init(&ssh_keygen);
+			strbuf_release(&ssh_keygen_out);
+			strbuf_release(&ssh_keygen_err);
+			strvec_push(&ssh_keygen.args, fmt->program);
+			/* We found principals - Try with each until we find a
+			 * match */
+			strvec_pushl(&ssh_keygen.args, "-Y", "verify",
+				     "-n", "git",
+				     "-f", ssh_allowed_signers,
+				     "-I", principal,
+				     "-s", buffer_file->filename.buf,
+				     NULL);
+
+			if (ssh_revocation_file) {
+				if (file_exists(ssh_revocation_file)) {
+					strvec_pushl(&ssh_keygen.args, "-r",
+						     ssh_revocation_file, NULL);
+				} else {
+					warning(_("ssh signing revocation file configured but not found: %s"),
+						ssh_revocation_file);
+				}
+			}
+
+			sigchain_push(SIGPIPE, SIG_IGN);
+			ret = pipe_command(&ssh_keygen, payload, payload_size,
+					   &ssh_keygen_out, 0, &ssh_keygen_err, 0);
+			sigchain_pop(SIGPIPE);
+
+			FREE_AND_NULL(principal);
+
+			ret &= starts_with(ssh_keygen_out.buf, "Good");
+			if (ret == 0)
+				break;
+		}
+	}
+
+	sigc->payload = xmemdupz(payload, payload_size);
+	strbuf_stripspace(&ssh_keygen_out, 0);
+	strbuf_stripspace(&ssh_keygen_err, 0);
+	strbuf_add(&ssh_keygen_out, ssh_keygen_err.buf, ssh_keygen_err.len);
+	sigc->output = strbuf_detach(&ssh_keygen_out, NULL);
+	sigc->gpg_status = xstrdup(sigc->output);
+
+	parse_ssh_output(sigc);
+
+out:
+	if (buffer_file)
+		delete_tempfile(&buffer_file);
+	strbuf_release(&ssh_keygen_out);
+	strbuf_release(&ssh_keygen_err);
+
+	return ret;
+}
+
 int check_signature(const char *payload, size_t plen, const char *signature,
 	size_t slen, struct signature_check *sigc)
 {
@@ -453,6 +618,18 @@  int git_gpg_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(var, "gpg.ssh.allowedsignersfile")) {
+		if (!value)
+			return config_error_nonbool(var);
+		return git_config_string(&ssh_allowed_signers, var, value);
+	}
+
+	if (!strcmp(var, "gpg.ssh.revocationFile")) {
+		if (!value)
+			return config_error_nonbool(var);
+		return git_config_string(&ssh_revocation_file, var, value);
+	}
+
 	if (!strcmp(var, "gpg.program") || !strcmp(var, "gpg.openpgp.program"))
 		fmtname = "openpgp";