ssh signing: valid-before is checked at the signer's own date, and a missing revocationFile fails open

ssh signing: valid-before is checked at the signer's own date, and a missing revocationFile fails open

[Christian Noé Ramos López]

Two things that, together, mean an SSH signing key cannot be reliably
stopped from being trusted. git 2.47.3, OpenSSH 10.0p2, Debian 13;
source read at v2.47.3 and at master (c46c1e37724f).

1. valid-before is checked at a date the signer writes.

SSH signatures carry no time of their own, so git passes -Overify-time
from the committer or tagger line (gpg-interface.c,
parse_payload_metadata). alice's key is in the allowed signers file
with valid-before="20260101":

    ssh-old        %G?=G 2025-06-01 12:00:00 +0000 verify-commit=0 merge=0
    ssh-backdated  %G?=G 2025-06-01 12:00:00 +0000 verify-commit=0 merge=0
    ssh-honest     %G?=U 2026-10-08 02:15:15 -0400 verify-commit=1 merge=128

ssh-backdated was signed today, with only the committer and author
dates set to 2025-06-01. Nothing distinguishes it from ssh-old except
when it was made, which only its author knows. ssh-honest, signed and
dated today, is refused: "key has expired: verify time ... >
valid-before 2026-01-01T00:00:00".

The GPG backend refuses both:

    gpg-old        %G?=Y verify-commit=1 merge=128
    gpg-backdated  %G?=Y verify-commit=1 merge=128

The documentation for gpg.ssh.allowedSignersFile says "Git will mark
signatures as valid if the signing key was valid at the time of the
signature's creation", which is the intent, but does not say the time
comes from the commit. So valid-before rotates a key; it does not
retire one.

2. A configured revocation file that does not exist fails open.

gpg-interface.c:568-574 at v2.47.3 (579-586 at master): if the
revocation file exists, pass -r; otherwise warn and verify without it.
The same file, present and listing alice's key, refuses:

    S2-revoked       %G?=B verify-commit=1 merged=no
    S3-revfile-gone  %G?=G verify-commit=0 merged=yes
                     warning: ssh signing revocation file configured
but not found

S3 merged under `git merge --ff-only --verify-signatures`. An
unreadable file and a directory both fail closed:

    S4-revfile-0000  %G?=B verify-commit=1 merged=no
    S6-revfile-dir   %G?=B verify-commit=1 merged=no

ssh-keygen, given the same missing path, refuses: exit 255, "Could not
verify signature". git avoids that by not passing -r. OpenSSH's
RevokedKeys says, in sshd_config(5), "Note that if this file is not
readable, then public key authentication will be refused for all
users."

No test in git exercises gpg.ssh.revocationFile; it appears only in
Documentation/config/gpg.adoc and gpg-interface.c.

Controls for both runs, fixed beforehand: a good signature gives G and
merges; an unsigned commit gives N and is refused; the revocation file
present and listing the key gives B and is refused; a commit with its
message changed and the signature kept gives B and is refused.

Together: the two ways to stop trusting an SSH signing key are
valid-before, which the signer can date around, and revocationFile,
which does nothing if its path is wrong. Either would be enough on its
own if it held.

What I would ask for: refuse when the revocation file is configured and
missing, as ssh-keygen and sshd do, or say in the documentation that it
is ignored; and say, under valid-before, where the time compared
against it comes from.

On prior art: the ssh signing series (Fabian Stelzer, 2021) carried the
warning from before v4, and the review raised the config name's case,
not what a missing file should do. The key-lifetime series (RFC
2021-10-15 to v6 2021-12-09) passes the commit date to the check, and
the replies are about style. The N for an unconfigured allowed signers
file is already on the list (Grayson Tinker, 2026-06-25) and is not
part of this.

Christian Ramos
Norte Software
chris@nortesoftware.dev

Re: ssh signing: valid-before is checked at the signer's own date, and a missing revocationFile fails open

[Phillip Wood]

Hi Christian

Having waded through this here is a human readable summary:

(1) Our documentation implies that we check the expiry date of the key 
(which is recorded in the allowed signers file) against the date the 
commit was signed, but we actually use the committer date which can 
easily be faked.

(2) If the revocation file does not exist we print a warning rather than 
failing the operation like the gpg backend does.

For (1) I'd be happy to see a patch that tightens the wording, but we 
should also note that the timestamp in the gpg signature can also be faked.

For (2) I agree failing seems like the safer option.

Thanks

Phillip

On 08/10/2026 07:56, Christian Noé Ramos López wrote:
> Two things that, together, mean an SSH signing key cannot be reliably
> stopped from being trusted. git 2.47.3, OpenSSH 10.0p2, Debian 13;
> source read at v2.47.3 and at master (c46c1e37724f).
> 
> 1. valid-before is checked at a date the signer writes.
> 
> SSH signatures carry no time of their own, so git passes -Overify-time
> from the committer or tagger line (gpg-interface.c,
> parse_payload_metadata). alice's key is in the allowed signers file
> with valid-before="20260101":
> 
>      ssh-old        %G?=G 2025-06-01 12:00:00 +0000 verify-commit=0 merge=0
>      ssh-backdated  %G?=G 2025-06-01 12:00:00 +0000 verify-commit=0 merge=0
>      ssh-honest     %G?=U 2026-10-08 02:15:15 -0400 verify-commit=1 merge=128
> 
> ssh-backdated was signed today, with only the committer and author
> dates set to 2025-06-01. Nothing distinguishes it from ssh-old except
> when it was made, which only its author knows. ssh-honest, signed and
> dated today, is refused: "key has expired: verify time ... >
> valid-before 2026-01-01T00:00:00".
> 
> The GPG backend refuses both:
> 
>      gpg-old        %G?=Y verify-commit=1 merge=128
>      gpg-backdated  %G?=Y verify-commit=1 merge=128
> 
> The documentation for gpg.ssh.allowedSignersFile says "Git will mark
> signatures as valid if the signing key was valid at the time of the
> signature's creation", which is the intent, but does not say the time
> comes from the commit. So valid-before rotates a key; it does not
> retire one.
> 
> 2. A configured revocation file that does not exist fails open.
> 
> gpg-interface.c:568-574 at v2.47.3 (579-586 at master): if the
> revocation file exists, pass -r; otherwise warn and verify without it.
> The same file, present and listing alice's key, refuses:
> 
>      S2-revoked       %G?=B verify-commit=1 merged=no
>      S3-revfile-gone  %G?=G verify-commit=0 merged=yes
>                       warning: ssh signing revocation file configured
> but not found
> 
> S3 merged under `git merge --ff-only --verify-signatures`. An
> unreadable file and a directory both fail closed:
> 
>      S4-revfile-0000  %G?=B verify-commit=1 merged=no
>      S6-revfile-dir   %G?=B verify-commit=1 merged=no
> 
> ssh-keygen, given the same missing path, refuses: exit 255, "Could not
> verify signature". git avoids that by not passing -r. OpenSSH's
> RevokedKeys says, in sshd_config(5), "Note that if this file is not
> readable, then public key authentication will be refused for all
> users."
> 
> No test in git exercises gpg.ssh.revocationFile; it appears only in
> Documentation/config/gpg.adoc and gpg-interface.c.
> 
> Controls for both runs, fixed beforehand: a good signature gives G and
> merges; an unsigned commit gives N and is refused; the revocation file
> present and listing the key gives B and is refused; a commit with its
> message changed and the signature kept gives B and is refused.
> 
> Together: the two ways to stop trusting an SSH signing key are
> valid-before, which the signer can date around, and revocationFile,
> which does nothing if its path is wrong. Either would be enough on its
> own if it held.
> 
> What I would ask for: refuse when the revocation file is configured and
> missing, as ssh-keygen and sshd do, or say in the documentation that it
> is ignored; and say, under valid-before, where the time compared
> against it comes from.
> 
> On prior art: the ssh signing series (Fabian Stelzer, 2021) carried the
> warning from before v4, and the review raised the config name's case,
> not what a missing file should do. The key-lifetime series (RFC
> 2021-10-15 to v6 2021-12-09) passes the commit date to the check, and
> the replies are about style. The N for an unconfigured allowed signers
> file is already on the list (Grayson Tinker, 2026-06-25) and is not
> part of this.
> 
> Christian Ramos
> Norte Software
> chris@nortesoftware.dev

Re: ssh signing: valid-before is checked at the signer's own date, and a missing revocationFile fails open

[Patrick Steinhardt]

On Thu, Oct 08, 2026 at 02:42:42PM +0100, Phillip Wood wrote:
> Hi Christian
> 
> Having waded through this here is a human readable summary:

thanks a lot for the summary, I really appreciate it as I already lost
interest after having read the first sentence.

> (1) Our documentation implies that we check the expiry date of the key
> (which is recorded in the allowed signers file) against the date the commit
> was signed, but we actually use the committer date which can easily be
> faked.

Right. I think there isn't even a proper fix for this as we have no way
to establish the actual time the data was signed. I think this is a
simple fact in a distributed system, as without coordination there is
basically nothing that the contributor can give us that would make us
trust the claimed signature time.

You may be able to create upper bounds if there are subsequent signed
commits that you trust and that have the untrusted commit as child. But
that is not going to be always useful.

> (2) If the revocation file does not exist we print a warning rather than
> failing the operation like the gpg backend does.
> 
> For (1) I'd be happy to see a patch that tightens the wording, but we should
> also note that the timestamp in the gpg signature can also be faked.

Yeah, agreed. This mode is only safe if the signing key has never
leaked, but once it has leaked you can basically not guarantee anything
via "valid-before" and "valid-after". And documenting that would be a
good idea to not give a sense of false trustworthiness.

> For (2) I agree failing seems like the safer option.

Maybe this is another usecase where we can use the ":(optional)" prefix
that we introduced recently for some of the other pathname options? So
we'd fail by default, but give the user an escape hatch if they really
want one.

Patrick

Re: ssh signing: valid-before is checked at the signer's own date, and a missing revocationFile fails open

[Phillip Wood]

Hi Patrick

On 09/10/2026 13:19, Patrick Steinhardt wrote:
> On Thu, Oct 08, 2026 at 02:42:42PM +0100, Phillip Wood wrote:
>>
>> Having waded through this here is a human readable summary:
> 
> thanks a lot for the summary, I really appreciate it as I already lost
> interest after having read the first sentence.
> 
>> (1) Our documentation implies that we check the expiry date of the key
>> (which is recorded in the allowed signers file) against the date the commit
>> was signed, but we actually use the committer date which can easily be
>> faked.
> 
> Right. I think there isn't even a proper fix for this as we have no way
> to establish the actual time the data was signed. I think this is a
> simple fact in a distributed system, as without coordination there is
> basically nothing that the contributor can give us that would make us
> trust the claimed signature time.

Indeed

> You may be able to create upper bounds if there are subsequent signed
> commits that you trust and that have the untrusted commit as child. But
> that is not going to be always useful.

Yes, on its own checking the signature just tells you that the commit 
was signed by that key, it does not tell you when it was signed.

>> (2) If the revocation file does not exist we print a warning rather than
>> failing the operation like the gpg backend does.
>>
>> For (1) I'd be happy to see a patch that tightens the wording, but we should
>> also note that the timestamp in the gpg signature can also be faked.
> 
> Yeah, agreed. This mode is only safe if the signing key has never
> leaked, but once it has leaked you can basically not guarantee anything
> via "valid-before" and "valid-after". And documenting that would be a
> good idea to not give a sense of false trustworthiness.
> 
>> For (2) I agree failing seems like the safer option.
> 
> Maybe this is another usecase where we can use the ":(optional)" prefix
> that we introduced recently for some of the other pathname options? So
> we'd fail by default, but give the user an escape hatch if they really
> want one.

Oh, I'd not though of that - that's a good idea

Thanks

Phillip

Re: ssh signing: valid-before is checked at the signer's own date, and a missing revocationFile fails open

[Phillip Wood]

Hi Chirstian

I've add back the the mailing list cc so others can comment as well.

On 08/10/2026 20:40, Christian Noé Ramos López wrote:
> 
>> Having waded through this here is a human readable summary:
> 
> Fair -- your summary is the shape I should have sent. Next time.
> 
> (1) You are right, and I did not test it. gpg takes the signature's own
> creation time, so a manipulated clock moves that too. What I measured is
> narrower: backdating the committer and author dates does not move it --
> ssh-backdated is accepted, gpg-backdated is refused. A difference in cost,
> not in kind.
> 
> So the wording should say two things: that the time compared against
> valid-before comes from the commit, and that a signature timestamp is not
> evidence of when the signing happened. That covers both backends. I will
> send that patch.

That sounds sensible - a valid signature doesn't tell us anything about 
when the commit was signed.

> (2) One thing before I write it. Failing closed changes behaviour for
> anyone whose configured path is already wrong, from a warning to a failed
> verification. I think that is the right trade, but it is a behaviour
> change and not only a fix. There is no test for gpg.ssh.revocationFile
> today, so the patch should add one either way.

A test would be very welcome. Patrick had a good suggestion for allowing 
the path to be optional if the user wanted.

Thanks

Phillip

> Christian Ramos
> Norte Software
> chris@nortesoftware.dev
> 
> 
> El jue, 8 oct 2026 a la(s) 7:42 a.m., Phillip Wood
> (phillip.wood123@gmail.com) escribió:
>>
>> Hi Christian
>>
>> Having waded through this here is a human readable summary:
>>
>> (1) Our documentation implies that we check the expiry date of the key
>> (which is recorded in the allowed signers file) against the date the
>> commit was signed, but we actually use the committer date which can
>> easily be faked.
>>
>> (2) If the revocation file does not exist we print a warning rather than
>> failing the operation like the gpg backend does.
>>
>> For (1) I'd be happy to see a patch that tightens the wording, but we
>> should also note that the timestamp in the gpg signature can also be faked.
>>
>> For (2) I agree failing seems like the safer option.
>>
>> Thanks
>>
>> Phillip
>>
>> On 08/10/2026 07:56, Christian Noé Ramos López wrote:
>>> Two things that, together, mean an SSH signing key cannot be reliably
>>> stopped from being trusted. git 2.47.3, OpenSSH 10.0p2, Debian 13;
>>> source read at v2.47.3 and at master (c46c1e37724f).
>>>
>>> 1. valid-before is checked at a date the signer writes.
>>>
>>> SSH signatures carry no time of their own, so git passes -Overify-time
>>> from the committer or tagger line (gpg-interface.c,
>>> parse_payload_metadata). alice's key is in the allowed signers file
>>> with valid-before="20260101":
>>>
>>>       ssh-old        %G?=G 2025-06-01 12:00:00 +0000 verify-commit=0 merge=0
>>>       ssh-backdated  %G?=G 2025-06-01 12:00:00 +0000 verify-commit=0 merge=0
>>>       ssh-honest     %G?=U 2026-10-08 02:15:15 -0400 verify-commit=1 merge=128
>>>
>>> ssh-backdated was signed today, with only the committer and author
>>> dates set to 2025-06-01. Nothing distinguishes it from ssh-old except
>>> when it was made, which only its author knows. ssh-honest, signed and
>>> dated today, is refused: "key has expired: verify time ... >
>>> valid-before 2026-01-01T00:00:00".
>>>
>>> The GPG backend refuses both:
>>>
>>>       gpg-old        %G?=Y verify-commit=1 merge=128
>>>       gpg-backdated  %G?=Y verify-commit=1 merge=128
>>>
>>> The documentation for gpg.ssh.allowedSignersFile says "Git will mark
>>> signatures as valid if the signing key was valid at the time of the
>>> signature's creation", which is the intent, but does not say the time
>>> comes from the commit. So valid-before rotates a key; it does not
>>> retire one.
>>>
>>> 2. A configured revocation file that does not exist fails open.
>>>
>>> gpg-interface.c:568-574 at v2.47.3 (579-586 at master): if the
>>> revocation file exists, pass -r; otherwise warn and verify without it.
>>> The same file, present and listing alice's key, refuses:
>>>
>>>       S2-revoked       %G?=B verify-commit=1 merged=no
>>>       S3-revfile-gone  %G?=G verify-commit=0 merged=yes
>>>                        warning: ssh signing revocation file configured
>>> but not found
>>>
>>> S3 merged under `git merge --ff-only --verify-signatures`. An
>>> unreadable file and a directory both fail closed:
>>>
>>>       S4-revfile-0000  %G?=B verify-commit=1 merged=no
>>>       S6-revfile-dir   %G?=B verify-commit=1 merged=no
>>>
>>> ssh-keygen, given the same missing path, refuses: exit 255, "Could not
>>> verify signature". git avoids that by not passing -r. OpenSSH's
>>> RevokedKeys says, in sshd_config(5), "Note that if this file is not
>>> readable, then public key authentication will be refused for all
>>> users."
>>>
>>> No test in git exercises gpg.ssh.revocationFile; it appears only in
>>> Documentation/config/gpg.adoc and gpg-interface.c.
>>>
>>> Controls for both runs, fixed beforehand: a good signature gives G and
>>> merges; an unsigned commit gives N and is refused; the revocation file
>>> present and listing the key gives B and is refused; a commit with its
>>> message changed and the signature kept gives B and is refused.
>>>
>>> Together: the two ways to stop trusting an SSH signing key are
>>> valid-before, which the signer can date around, and revocationFile,
>>> which does nothing if its path is wrong. Either would be enough on its
>>> own if it held.
>>>
>>> What I would ask for: refuse when the revocation file is configured and
>>> missing, as ssh-keygen and sshd do, or say in the documentation that it
>>> is ignored; and say, under valid-before, where the time compared
>>> against it comes from.
>>>
>>> On prior art: the ssh signing series (Fabian Stelzer, 2021) carried the
>>> warning from before v4, and the review raised the config name's case,
>>> not what a missing file should do. The key-lifetime series (RFC
>>> 2021-10-15 to v6 2021-12-09) passes the commit date to the check, and
>>> the replies are about style. The N for an unconfigured allowed signers
>>> file is already on the list (Grayson Tinker, 2026-06-25) and is not
>>> part of this.
>>>
>>> Christian Ramos
>>> Norte Software
>>> chris@nortesoftware.dev
>>
> 
> 

[PATCH] doc: gpg: say which date SSH key lifetimes are checked against

[Christian Noé Ramos López]

The description of gpg.ssh.allowedSignersFile says that Git marks a
signature as valid if the signing key was valid at the time of the
signature's creation. An SSH signature does not record when it was
made, though. When verifying a commit or a tag, Git passes the
committer or tagger date to ssh-keygen with -Overify-time, so the
valid-after and valid-before options of a key are checked against a
date that whoever creates the commit or tag can set to any value.

Say which date is used, and that it is not evidence of when the
signature was made. The same holds for the creation time in a GPG
signature, which comes from the signer's clock, so say that too.
Since whoever holds the key chooses the date, also say that a key
lifetime is only safe as long as the key has never leaked.

While at it, fix the spelling of "OpenSSH" in the same paragraph.

Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Christian Noé Ramos López <chris@nortesoftware.dev>
---
 Documentation/config/gpg.adoc | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/Documentation/config/gpg.adoc b/Documentation/config/gpg.adoc
index 240e46c05..714dd1457 100644
--- a/Documentation/config/gpg.adoc
+++ b/Documentation/config/gpg.adoc
@@ -70,10 +70,15 @@ A repository that only allows signed commits can store the file
 in the repository itself using a path 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.
 +
-Since OpensSSH 8.8 this file allows specifying a key lifetime using valid-after &
-valid-before options. Git will mark signatures as valid if the signing key was
-valid at the time of the signature's creation. This allows users to change a
-signing key without invalidating all previously made signatures.
+Since OpenSSH 8.8 this file allows specifying a key lifetime using valid-after &
+valid-before options. When verifying a commit or a tag, Git checks that lifetime
+against the committer or tagger date recorded in the commit or tag. This allows
+users to change a signing key without invalidating all previously made
+signatures. That date is chosen by whoever creates the commit or tag, so it is
+not evidence of when the signature was made; the same is true of the creation
+time in a GPG signature, which is set by the signer's clock. Relying on a key
+lifetime is therefore only safe as long as the signing key has never leaked;
+once it has, valid-after and valid-before guarantee nothing.
 +
 Using a SSH CA key with the cert-authority option
 (see ssh-keygen(1) "CERTIFICATES") is also valid.

base-commit: 47fdf2208c5b94c8cc01c106b8f7c85efb91c926
-- 
2.47.3

Re: [PATCH] doc: gpg: say which date SSH key lifetimes are checked against

[Junio C Hamano]

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

> -Since OpensSSH 8.8 this file allows specifying a key lifetime using valid-after &
> -valid-before options. Git will mark signatures as valid if the signing key was
> -valid at the time of the signature's creation. This allows users to change a
> -signing key without invalidating all previously made signatures.
> +Since OpenSSH 8.8 this file allows specifying a key lifetime using valid-after &
> +valid-before options. When verifying a commit or a tag, Git checks that lifetime
> +against the committer or tagger date recorded in the commit or tag. This allows
> +users to change a signing key without invalidating all previously made
> +signatures. That date is chosen by whoever creates the commit or tag, so it is
> +not evidence of when the signature was made; the same is true of the creation
> +time in a GPG signature, which is set by the signer's clock. Relying on a key
> +lifetime is therefore only safe as long as the signing key has never leaked;
> +once it has, valid-after and valid-before guarantee nothing.

Saying what we compare against may be valuable, but everything else
sounds more like an empty fluff to me.  Isn't it self evident that
validity time range is a useless concept unless you cannot tell when
the signature was made?  In other words, instead of the long
discussion of how useless the validity checks are, something
succinct like

    Since OpensSSH 8.8 this file allows specifying a key lifetime using
    valid-after & valid-before options. Git validates the signature by
    checking the committer or tagger timestamp is within the validity
    time range.  This allows users to change a signing key without
    invalidating all previously made signatures.

should be sufficient, I would have to say.  Or am I missing
something?

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

[Christian Noé Ramos López]

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

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

[Junio C Hamano]

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.