[PATCH] 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 in the same way when the file is missing, and
turn the warning into an error so that the reason is shown. 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>
Signed-off-by: Christian Noé Ramos López <chris@nortesoftware.dev>
---
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               | 15 ++++++---------
 t/t7528-signed-commit-ssh.sh  | 28 ++++++++++++++++++++++++++++
 3 files changed, 37 insertions(+), 9 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..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,
diff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh
index 991678264..080ebf9db 100755
--- a/t/t7528-signed-commit-ssh.sh
+++ b/t/t7528-signed-commit-ssh.sh
@@ -211,6 +211,34 @@ 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_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