Re: [PATCH] gpg-interface: fail when the ssh revocation file is missing

[Junio C Hamano <gitster@pobox.com>]

Christian Noé Ramos López <chris@nortesoftware.dev> writes:

> diff --git a/gpg-interface.c b/gpg-interface.c
> index 60c315fba..ddbb29e78 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -533,6 +533,9 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
>  		 * we still call check-novalidate to display the signature info
>  		 */
>  		ret = -1;
> +	} else if (ssh_revocation_file && !file_exists(ssh_revocation_file)) {
> +		ret = error(_("ssh signing revocation file configured but not found: %s"),
> +			    ssh_revocation_file);
>  	} else {
>  		/* Check every principal we found (one per line) */
>  		const char *next;
> @@ -576,15 +579,9 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
>  				     verify_time.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);
> -				}
> -			}
> +			if (ssh_revocation_file)
> +				strvec_pushl(&ssh_keygen.args, "-r",
> +					     ssh_revocation_file, NULL);
>  
>  			sigchain_push(SIGPIPE, SIG_IGN);
>  			ret = pipe_command(&ssh_keygen, sigc->payload, sigc->payload_len,

It is a bit hard to see what really goes on with the default
context, so here is an excerpt from "git show -W" output:

> diff --git a/gpg-interface.c b/gpg-interface.c
> index 95abf1ef4e..904165a892 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -461,164 +461,161 @@ static void parse_ssh_output(struct signature_check *sigc)
> ...
>  	/* 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,
>  		     verify_time.buf,
>  		     NULL);
>  	ret = pipe_command(&ssh_keygen, NULL, 0, &ssh_principals_out, 0,
>  			   &ssh_principals_err, 0);
>  	if (ret && strstr(ssh_principals_err.buf, "usage:")) {
>  		error(_("ssh-keygen -Y find-principals/verify is needed for ssh signature verification (available in openssh version 8.2p1+)"));
>  		goto out;
>  	}

We asked "-Y find-principals" with allowed_signers file.  There are
a few cases.

>  	if (ret || !ssh_principals_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,
>  			     verify_time.buf,
>  			     NULL);
>  		pipe_command(&ssh_keygen, sigc->payload, sigc->payload_len,
>  				   &ssh_keygen_out, 0, &ssh_keygen_err, 0);
>  
>  		/*
>  		 * Fail on unknown keys
>  		 * we still call check-novalidate to display the signature info
>  		 */
>  		ret = -1;

If we find that the signature is done with an unknown key, we run
another ssh-keygen with check-validate to show the signature info.

By the way, it is not a problem this patch introduces, but the
comment at the end of this block is a bit misleading.  I first
thought another check-novalidate invocation will happen later in the
function but "we still call" refers to the invocation of ssh-keygen
we just made before the comment.

> +	} else if (ssh_revocation_file && !file_exists(ssh_revocation_file)) {
> +		ret = error(_("ssh signing revocation file configured but not found: %s"),
> +			    ssh_revocation_file);

Inserting the missing revocation file check here means we do not
even give signature information for known keys from this "else if"
block that we would have given for unknown keys in the "if" block,
which is not quite satisfactory.

I wonder if we should be able to somehow fold this into the earlier
"if()" block, making it the single place that deals with failures?

>  	} else {
>  		/* Check every principal we found (one per line) */
>  		const char *next;
>  		for (line = ssh_principals_out.buf;
>  		     *line;
>  		     line = next) {
>  			const char *end_of_text;

And everything we do when we recognise the signing key does not
change, except that ...

>  			/*
>  			 * 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,
>  				     verify_time.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);
> -				}
> -			}
> +			if (ssh_revocation_file)
> +				strvec_pushl(&ssh_keygen.args, "-r",
> +					     ssh_revocation_file, NULL);

... we no longer need to see if the file is missing, which makes sense.