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

["Christian Noé Ramos López" <chris@nortesoftware.dev>]

When gpg.ssh.revocationFile names a file that does not exist, Git
warns and runs ssh-keygen without it, so a signature made with a key
listed in the intended revocation file verifies as good. A revocation
file that exists but cannot be read already makes ssh-keygen refuse,
and the signature shows as bad; sshd likewise refuses all public key
authentication when its RevokedKeys file is not readable.

Fail the verification when the file is missing too, and turn the
warning into an error so that the reason is shown. Do it in the block
that already fails a key that is not in the allowed signers file, so
that a key that is in it still gets its signature info shown, and %G?
gives "U" for both. While at it, reword the comment in that block,
which read as if check-novalidate were to be called again later.

Users who want verification to go on without the file when it is
absent can prefix the path with ":(optional)", which makes Git treat
the variable as unset if the file does not exist.

A setup whose configured path is wrong now fails to verify where it
used to warn; until now its signatures were verified without any
revocation check.

gpg.ssh.revocationFile had no tests. Add tests for a revoked key, with
and without ":(optional)", and for a missing file, with and without
the prefix.

Suggested-by: Patrick Steinhardt <ps@pks.im>
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Christian Noé Ramos López <chris@nortesoftware.dev>
---
Changes since v1: the missing-file check moves into the block that
fails an unknown key, and that block's comment is reworded. With the
file missing, a known key now gets the error and then its signature
info, and %G? gives U instead of v1's B; an unknown key now gets the
error too. The missing-file test checks that the signature info is
shown.

With or without this patch, a local `:(optional)` value naming a
missing file drops a gpg.ssh.revocationFile set in the global config.
That is existing `:(optional)` behavior, not introduced here.

 Documentation/config/gpg.adoc |  3 +++
 gpg-interface.c               | 30 +++++++++++++-----------------
 t/t7528-signed-commit-ssh.sh  | 29 +++++++++++++++++++++++++++++
 3 files changed, 45 insertions(+), 17 deletions(-)

diff --git a/Documentation/config/gpg.adoc b/Documentation/config/gpg.adoc
index 240e46c05..cce49d53e 100644
--- a/Documentation/config/gpg.adoc
+++ b/Documentation/config/gpg.adoc
@@ -83,3 +83,6 @@ gpg.ssh.revocationFile::
 	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.
+	If the file does not exist, signature verification fails, unless
+	the path is prefixed with `:(optional)` (see the `pathname` type in
+	linkgit:git-config[1]).
diff --git a/gpg-interface.c b/gpg-interface.c
index 60c315fba..f36ba8dd0 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -466,6 +466,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
 	struct child_process ssh_keygen = CHILD_PROCESS_INIT;
 	struct tempfile *buffer_file;
 	int ret = -1;
+	int revocation_file_missing;
 	const char *line;
 	char *principal;
 	struct strbuf ssh_principals_out = STRBUF_INIT;
@@ -513,11 +514,17 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
 		error(_("ssh-keygen -Y find-principals/verify is needed for ssh signature verification (available in openssh version 8.2p1+)"));
 		goto out;
 	}
-	if (ret || !ssh_principals_out.len) {
+	revocation_file_missing = ssh_revocation_file &&
+				  !file_exists(ssh_revocation_file);
+	if (ret || !ssh_principals_out.len || revocation_file_missing) {
 		/*
-		 * We did not find a matching principal in the allowedSigners
-		 * Check without validation
+		 * We did not find a matching principal in the allowedSigners,
+		 * or the revocation file is missing. Either way we fail, and
+		 * check without validation only to display the signature info.
 		 */
+		if (revocation_file_missing)
+			error(_("ssh signing revocation file configured but not found: %s"),
+			      ssh_revocation_file);
 		child_process_init(&ssh_keygen);
 		strvec_pushl(&ssh_keygen.args, fmt->program,
 			     "-Y", "check-novalidate",
@@ -527,11 +534,6 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
 			     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;
 	} else {
 		/* Check every principal we found (one per line) */
@@ -576,15 +578,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,
diff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh
index 991678264..d67f9b489 100755
--- a/t/t7528-signed-commit-ssh.sh
+++ b/t/t7528-signed-commit-ssh.sh
@@ -211,6 +211,35 @@ test_expect_success GPGSSH 'verify-commit exits failure with high minTrustLevel'
 	test_must_fail git verify-commit eighth-signed-alt
 '
 
+test_expect_success GPGSSH 'verify-commit exits failure on revoked signing key' '
+	test_when_finished "rm -f revoked_keys" &&
+	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
+	cp "${GPGSSH_KEY_PRIMARY}.pub" revoked_keys &&
+	test_config gpg.ssh.revocationFile "$(pwd)/revoked_keys" &&
+	test_must_fail git verify-commit sixth-signed 2>actual &&
+	test_grep ! "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual &&
+	test_config gpg.ssh.revocationFile ":(optional)$(pwd)/revoked_keys" &&
+	test_must_fail git verify-commit sixth-signed 2>actual &&
+	test_grep ! "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual
+'
+
+test_expect_success GPGSSH 'verify-commit exits failure on missing revocation file' '
+	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
+	test_config gpg.ssh.revocationFile "$(pwd)/no-such-file" &&
+	test_must_fail git verify-commit sixth-signed 2>actual &&
+	test_grep "revocation file configured but not found" actual &&
+	test_grep "${GPGSSH_GOOD_SIGNATURE_UNTRUSTED}" actual &&
+	test_grep ! "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual
+'
+
+test_expect_success GPGSSH 'verify-commit ignores missing optional revocation file' '
+	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
+	test_config gpg.ssh.revocationFile ":(optional)$(pwd)/no-such-file" &&
+	git verify-commit sixth-signed 2>actual &&
+	test_grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual &&
+	test_grep ! "revocation file" actual
+'
+
 test_expect_success GPGSSH 'verify signatures with --raw' '
 	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
 	(

base-commit: 47fdf2208c5b94c8cc01c106b8f7c85efb91c926
-- 
2.47.3