[RFC/PATCH] receive-pack.c: only accept push-cert if push_cert_nonce was advertised

[RFC/PATCH] receive-pack.c: only accept push-cert if push_cert_nonce was advertised

[Stefan Beller]

If the server did not advertise the capability to have signed pushes
it should not accept signed pushes as stated in
Documentation/technical/protocol-capabilities.txt:

    Client will then send a space separated list of capabilities it wants
    to be in effect. The client MUST NOT ask for capabilities the server
    did not say it supports.

    Server MUST diagnose and abort if capabilities it does not understand
    was sent.  Server MUST NOT ignore capabilities that client requested
    and server advertised.  As a consequence of these rules, server MUST
    NOT advertise capabilities it does not understand.

After rereading the second paragraph I think they should also be reworded to

    Server MUST diagnose and abort if capabilities it did not advertise
    was sent.


Suppose there would be hypothetical flaw in the capability of signed
pushes (or any capability for the current reasoning) which may harm
the server. This would require a bugfix release if it was severe and
would put us on time pressure to get it done.

A change like the one proposed would allow us to tell the community to
simply configure the server to not advertise the feature and if not
advertised the flaw could not be abused.

I am not saying there is a problem now, but I am rather saying patches
similar to this one would buy us time in case of problems arising.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Notes:
    As I discovered the idea while composing the
    atomic push series and the changes line of this
    patch is closeby, this applies on top of
    origin/sb/atomic-push (v12 as sent on Jan. 7th)
    
    This patch is RFC, thinking about security best practice.
    It's not enough to document the intended behavior in
    Documentation/technical/protocol-capabilities.txt, but
    rather enforce it in the code as well.
    
    Any thoughts on that welcome!
    
    Thanks,
    Stefan

 builtin/receive-pack.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 4c069c5..628d13a 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1276,7 +1276,8 @@ static struct command *read_head_info(struct sha1_array *shallow)
 				use_atomic = 1;
 		}
 
-		if (!strcmp(line, "push-cert")) {
+		if (push_cert_nonce &&
+		    !strcmp(line, "push-cert")) {
 			int true_flush = 0;
 			char certbuf[1024];
 
-- 
2.2.1.62.g3f15098

Re: [RFC/PATCH] receive-pack.c: only accept push-cert if push_cert_nonce was advertised

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> If the server did not advertise the capability to have signed pushes
> it should not accept signed pushes as stated in
> Documentation/technical/protocol-capabilities.txt:
>
>     Client will then send a space separated list of capabilities it wants
>     to be in effect. The client MUST NOT ask for capabilities the server
>     did not say it supports.
>
>     Server MUST diagnose and abort if capabilities it does not understand
>     was sent.  Server MUST NOT ignore capabilities that client requested
>     and server advertised.  As a consequence of these rules, server MUST
>     NOT advertise capabilities it does not understand.
>
> After rereading the second paragraph I think they should also be reworded to
>
>     Server MUST diagnose and abort if capabilities it did not advertise
>     was sent.

Except for s/was sent/was requested/, I think that rule makes sense
very much.

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 4c069c5..628d13a 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1276,7 +1276,8 @@ static struct command *read_head_info(struct sha1_array *shallow)
>  				use_atomic = 1;
>  		}
>  
> -		if (!strcmp(line, "push-cert")) {
> +		if (push_cert_nonce &&
> +		    !strcmp(line, "push-cert")) {
>  			int true_flush = 0;
>  			char certbuf[1024];

This implementation is somewhat questionable.

The server knows how to parse "push-cert" line, knows that what
follows after that line up to "push-cert-end" line are shaped very
differently from protocol commands outside the push-cert block.  In
other words, it knows how to parse the request meant for the capable
server; it just wants to refuse to serve that request.

The patched code will make it fail by hoping that queue_command()
that only handles "40-hex 40-hex ref" will reject the line that
begins with "push-cert".  Instead of relying on such a hidden
dependency, wouldn't it be cleaner to actually parse the push-cert
block and then at the end notice and explictly say "Your requests
were syntactically correct, but I am not going to honor your request
to use the push-cert extension, because I never told you that I'd
offer you that capability", instead of rejecting the request with "I
was expecting old/new/ref but you sent a line with 'push-cert' on
it; what are you talking about?"

Re: [RFC/PATCH] receive-pack.c: only accept push-cert if push_cert_nonce was advertised

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
>> index 4c069c5..628d13a 100644
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -1276,7 +1276,8 @@ static struct command *read_head_info(struct sha1_array *shallow)
>>  				use_atomic = 1;
>>  		}
>>  
>> -		if (!strcmp(line, "push-cert")) {
>> +		if (push_cert_nonce &&
>> +		    !strcmp(line, "push-cert")) {
>>  			int true_flush = 0;
>>  			char certbuf[1024];
>
> This implementation is somewhat questionable.
>
> The server knows how to parse "push-cert" line, knows that what
> follows after that line up to "push-cert-end" line are shaped very
> differently from protocol commands outside the push-cert block.  In
> other words, it knows how to parse the request meant for the capable
> server; it just wants to refuse to serve that request.
>
> The patched code will make it fail by hoping that queue_command()
> that only handles "40-hex 40-hex ref" will reject the line that
> begins with "push-cert".  Instead of relying on such a hidden
> dependency, wouldn't it be cleaner to actually parse the push-cert
> block and then at the end notice and explictly say "Your requests
> were syntactically correct, but I am not going to honor your request
> to use the push-cert extension, because I never told you that I'd
> offer you that capability", instead of rejecting the request with "I
> was expecting old/new/ref but you sent a line with 'push-cert' on
> it; what are you talking about?"

Note that I said "somewhat" questionable, not "horribly broken",
because another part of me wants to say that this implementation may
be better than "parse and reject" from security point of view.  The
pusher cannot tell if the push failed because the server is old to
know about the extension, or the server is new but is refusing, by
observing the failure.

But the other side of the same coin is that it makes it harder to
diagnose the failure.

When the protocol exchange gets to this state, in practice, we know
we are talking with somebody who has push privilege into the
repository, so revealing a bit more about the server by taking
"parse and reject" route may be a reasonable price to give diagnosis
that is more useful to help the users in this case, though.

Re: [RFC/PATCH] receive-pack.c: only accept push-cert if push_cert_nonce was advertised

[Stefan Beller]

On Fri, Jan 9, 2015 at 2:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
> The patched code will make it fail by hoping that queue_command()
> that only handles "40-hex 40-hex ref" will reject the line that
> begins with "push-cert".  Instead of relying on such a hidden
> dependency, wouldn't it be cleaner to actually parse the push-cert
> block and then at the end notice and explictly say "Your requests
> were syntactically correct, but I am not going to honor your request
> to use the push-cert extension, because I never told you that I'd
> offer you that capability", instead of rejecting the request with "I
> was expecting old/new/ref but you sent a line with 'push-cert' on
> it; what are you talking about?"

When reading the documentation at first I understood it the way:
"If I did not advertise the feature, I am expected to pretend it
doesn't even exist". That lead me to the implementation as
proposed.

Your proposal to acknowledge the correctness of the message leads
to more questions. How would we proceed? We would not  accept the
featured capability, but we could still do a normal push operation. This
would not be desired by a authentic user, so aborting the whole push
process would be ok.

I expect such behavior only from malicious clients which actively want
to abuse a feature which wasn't advertised, so I wouldn't mind being
slightly dishonest to the client and pretend we don't know that capability.
But relying on the hidden dependency is bad indeed. So maybe just
answer more rudely "I did not advertise one of the capability you
requested, go away! (closes connection)".


> But the other side of the same coin is that it makes it harder to
> diagnose the failure.

The push-cert case is special compared to other capabilities as it
is quite intrusive to the protocol compared to say "quiet", so more
diagnosis may be helpful.

> When the protocol exchange gets to this state, in practice, we know
> we are talking with somebody who has push privilege into the
> repository,

Yeah but what is one repository compared to the whole server?
(Just painting the devil on the wall now:) Say you could abuse one
capability to gain access to the server or create a huge memory
allocation such that the server becomes unresponsive for others.

I know my argument is weak for the "parse and reject route", so
I will see if I can adjust the patch to give diagnosis but still reject
the un-advertised capability.

Re: [RFC/PATCH] receive-pack.c: only accept push-cert if push_cert_nonce was advertised

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Your proposal to acknowledge the correctness of the message leads
> to more questions. How would we proceed?

How would it fail if we pretend that "push-cert" line had to be
old/new/ref line?  Failing the same way, but with a better
diagnosis, would be sufficient.

> I expect such behavior only from malicious clients which actively
> want to abuse a feature which wasn't advertised,...

Do not assume malice; it is not 2005 anymore.  You have to remember
that we are mature enough that there are many reimplementations of
Git, all of which (us included ;-) start with a buggy version.

>> When the protocol exchange gets to this state, in practice, we know
>> we are talking with somebody who has push privilege into the
>> repository,
>
> Yeah but what is one repository compared to the whole server?

Huh?  If an auth good enough for one repository allows things to
another repository, then I consider that to that other repository
the pusher also has push privilege.  So what is the problem?

But again, our first version could just be "pretend we do not know
anything about push-cert", with discussions on alternative
considered in its log message.  I do not think it is a blocker to
lack the "more helpful diagnosis" feature.

[PATCH] receive-pack.c: don't miss exporting unsolicited push certificates

[Stefan Beller]

If the server is configured to not advertise push certificates,
a push certificate that gets pushed nevertheless will not be fully
recorded because push_cert_nonce is NULL.

The recording of GIT_PUSH_CERT_NONCE_STATUS should be dependent on
the status being there instead of push_cert_nonce being non NULL.

Without this patch an unsolicited nonce never makes to the stage when
being exported with GIT_PUSH_CERT_NONCE_STATUS, because in the unsolicited
case push_cert_nonce is always NULL.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/receive-pack.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 628d13a..0e4878e 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -504,18 +504,18 @@ static void prepare_push_cert_sha1(struct child_process *proc)
 				 sigcheck.key ? sigcheck.key : "");
 		argv_array_pushf(&proc->env_array, "GIT_PUSH_CERT_STATUS=%c",
 				 sigcheck.result);
-		if (push_cert_nonce) {
+		if (push_cert_nonce)
 			argv_array_pushf(&proc->env_array,
 					 "GIT_PUSH_CERT_NONCE=%s",
 					 push_cert_nonce);
+		if (nonce_status)
 			argv_array_pushf(&proc->env_array,
 					 "GIT_PUSH_CERT_NONCE_STATUS=%s",
 					 nonce_status);
-			if (nonce_status == NONCE_SLOP)
-				argv_array_pushf(&proc->env_array,
-						 "GIT_PUSH_CERT_NONCE_SLOP=%ld",
-						 nonce_stamp_slop);
-		}
+		if (nonce_status == NONCE_SLOP)
+			argv_array_pushf(&proc->env_array,
+					 "GIT_PUSH_CERT_NONCE_SLOP=%ld",
+					 nonce_stamp_slop);
 	}
 }
 
-- 
2.2.1.62.g3f15098

Re: [PATCH] receive-pack.c: don't miss exporting unsolicited push certificates

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> If the server is configured to not advertise push certificates,
> a push certificate that gets pushed nevertheless will not be fully
> recorded because push_cert_nonce is NULL.

Sorry, but I do not quite see what you are trying to get at.

When we did not advertise that this feature is supported, we know
the incoming nonce is meaningless junk because we didn't supply the
correct answer the pusher can give us; we do not even have the
correct answer ourselves.

Besides, while reviewing the other patch, didn't we agree that we
should reject such a push?  Then there is nothing to log anyway, no?

    ... reads the patch and code beyond the context while scratching
    head ...

I notice that the above three lines do not correspond what you did
in this patch.  The certificate is exported via the blob interface
fully with or without this change.

What is not given to the hook is the push-cert-nonce-status.  While
it is sufficient to tell the hook that we are not getting a good
nonce (i.e. the hook does not see GIT_PUSH_CERT_NONCE_STATUS=G), we
are not showing that nonce-status is "unsolicited", even though we
internally compute and decide that; is that what you are trying to
fix?

Still puzzled...

>
> The recording of GIT_PUSH_CERT_NONCE_STATUS should be dependent on
> the status being there instead of push_cert_nonce being non NULL.
>
> Without this patch an unsolicited nonce never makes to the stage when
> being exported with GIT_PUSH_CERT_NONCE_STATUS, because in the unsolicited
> case push_cert_nonce is always NULL.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/receive-pack.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 628d13a..0e4878e 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -504,18 +504,18 @@ static void prepare_push_cert_sha1(struct child_process *proc)
>  				 sigcheck.key ? sigcheck.key : "");
>  		argv_array_pushf(&proc->env_array, "GIT_PUSH_CERT_STATUS=%c",
>  				 sigcheck.result);
> -		if (push_cert_nonce) {
> +		if (push_cert_nonce)
>  			argv_array_pushf(&proc->env_array,
>  					 "GIT_PUSH_CERT_NONCE=%s",
>  					 push_cert_nonce);
> +		if (nonce_status)
>  			argv_array_pushf(&proc->env_array,
>  					 "GIT_PUSH_CERT_NONCE_STATUS=%s",
>  					 nonce_status);
> -			if (nonce_status == NONCE_SLOP)
> -				argv_array_pushf(&proc->env_array,
> -						 "GIT_PUSH_CERT_NONCE_SLOP=%ld",
> -						 nonce_stamp_slop);
> -		}
> +		if (nonce_status == NONCE_SLOP)
> +			argv_array_pushf(&proc->env_array,
> +					 "GIT_PUSH_CERT_NONCE_SLOP=%ld",
> +					 nonce_stamp_slop);
>  	}
>  }

Re: [PATCH] receive-pack.c: don't miss exporting unsolicited push certificates

[Stefan Beller]

On 09.01.2015 17:52, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:
> 
>> If the server is configured to not advertise push certificates,
>> a push certificate that gets pushed nevertheless will not be fully
>> recorded because push_cert_nonce is NULL.
> 
> Sorry, but I do not quite see what you are trying to get at.
> 
> When we did not advertise that this feature is supported, we know
> the incoming nonce is meaningless junk because we didn't supply the
> correct answer the pusher can give us; we do not even have the
> correct answer ourselves.

> 
> Besides, while reviewing the other patch, didn't we agree that we
> should reject such a push?  Then there is nothing to log anyway, no?

Yes we did. This is unrelated to the previous patch. I stuffed it into
this thread as I found it was touching the same feature.

> 
>     ... reads the patch and code beyond the context while scratching
>     head ...
> 
> I notice that the above three lines do not correspond what you did
> in this patch.  The certificate is exported via the blob interface
> fully with or without this change.

I had problems with wording the commit message because I have no
expertise with the feature. I am sorry for wasting your time there.

> 
> What is not given to the hook is the push-cert-nonce-status.  While
> it is sufficient to tell the hook that we are not getting a good
> nonce (i.e. the hook does not see GIT_PUSH_CERT_NONCE_STATUS=G), we
> are not showing that nonce-status is "unsolicited", even though we
> internally compute and decide that; is that what you are trying to
> fix?

yes that's what I was trying to hint at. The hook would just see
it is unsolicited instead of not having the state available.


> 
> Still puzzled...
> 
>>
>> The recording of GIT_PUSH_CERT_NONCE_STATUS should be dependent on
>> the status being there instead of push_cert_nonce being non NULL.
>>
>> Without this patch an unsolicited nonce never makes to the stage when
>> being exported with GIT_PUSH_CERT_NONCE_STATUS, because in the unsolicited
>> case push_cert_nonce is always NULL.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  builtin/receive-pack.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
>> index 628d13a..0e4878e 100644
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -504,18 +504,18 @@ static void prepare_push_cert_sha1(struct child_process *proc)
>>  				 sigcheck.key ? sigcheck.key : "");
>>  		argv_array_pushf(&proc->env_array, "GIT_PUSH_CERT_STATUS=%c",
>>  				 sigcheck.result);
>> -		if (push_cert_nonce) {
>> +		if (push_cert_nonce)
>>  			argv_array_pushf(&proc->env_array,
>>  					 "GIT_PUSH_CERT_NONCE=%s",
>>  					 push_cert_nonce);
>> +		if (nonce_status)
>>  			argv_array_pushf(&proc->env_array,
>>  					 "GIT_PUSH_CERT_NONCE_STATUS=%s",
>>  					 nonce_status);
>> -			if (nonce_status == NONCE_SLOP)
>> -				argv_array_pushf(&proc->env_array,
>> -						 "GIT_PUSH_CERT_NONCE_SLOP=%ld",
>> -						 nonce_stamp_slop);
>> -		}
>> +		if (nonce_status == NONCE_SLOP)
>> +			argv_array_pushf(&proc->env_array,
>> +					 "GIT_PUSH_CERT_NONCE_SLOP=%ld",
>> +					 nonce_stamp_slop);
>>  	}
>>  }
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] receive-pack.c: don't miss exporting unsolicited push certificates

[Junio C Hamano]

Stefan Beller <stefanbeller@gmail.com> writes:

> I had problems with wording the commit message because I have no
> expertise with the feature. I am sorry for wasting your time there.

Heh, remember, the time spent discussing Git on this list is not a
wasted time.

>> What is not given to the hook is the push-cert-nonce-status.  While
>> it is sufficient to tell the hook that we are not getting a good
>> nonce (i.e. the hook does not see GIT_PUSH_CERT_NONCE_STATUS=G), we
>> are not showing that nonce-status is "unsolicited", even though we
>> internally compute and decide that; is that what you are trying to
>> fix?
>
> yes that's what I was trying to hint at. The hook would just see
> it is unsolicited instead of not having the state available.

OK.  That makes sort of sense.  So if we:

 1) did not apply either patch (i.e. we accept unsolicited
    certificate, and the fact that we did not exchange "give me this
    nonce" "here is your nonce" is conveyed to the hooks by the lack
    of GIT_PUSH_CERT_NONCE environment and possible presense of
    unsolicited nonce in the GIT_PUSH_CERT blob); and then

 2) applied this patch first (i.e. we still allow unsolicited
    certificate, and the same fact is now conveyed to the hooks also
    by GIT_PUSH_CERT_NONCE_STATUS=UNSOLICITED if they sent a nonce,
    or GIT_PUSH_CERT_NONCE_STATUS=MISSING); and then finally

 3) applied the other patch to reject unsolicited certificate.

then we can stop at any of these three steps and the behaviour of
three resulting systems make sense and the pre-receive hook can
reject unsolicited certificates if it wants to, even at step #1.

The second step makes it easier for the hook to make that decision
because it would make $GIT_PUSH_CERT_NONCE_STATUS the only thing it
needs to inspect, instead of checking it and also having to check
$GIT_PUSH_CERT_NONCE, which is a simplification [*1*].

And then in the third step, the hook does not even have to worry,
which makes what #2 does more or less pointless.

This patch is still a good thing to do from the "correctness" point
of view; the current code may accept certificates without nonce only
because in an earlier iteration of the protocol design, the nonce
was optional and the code the earlier patch fixes is a remnant of
that.  As we do not advertise push-cert without nonce in the final
and current protocol, there is no reason to be loose anymore.


[Footnote]

*1* A hypothetical naive hook implementation:

	case "$GIT_PUSH_CERT_NONCE_STATUS" in
        OK | SLOP)
        	: good and happy
                exit 0;;
	*)
        	: bad and unhappy
                exit 1;;
	esac

would diagnose an unsolicited certificate with or without nonce as
bad.  Though it cannot tell between unsolicited and missing cases,
it would not be so serious a defect in practice.

Re: [PATCH] receive-pack.c: don't miss exporting unsolicited push certificates

[Stefan Beller]

On Mon, Jan 12, 2015 at 11:07 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> yes that's what I was trying to hint at. The hook would just see
>> it is unsolicited instead of not having the state available.
>
> OK.  That makes sort of sense.  So if we:
>
>  1) did not apply either patch (i.e. we accept unsolicited
>     certificate, and the fact that we did not exchange "give me this
>     nonce" "here is your nonce" is conveyed to the hooks by the lack
>     of GIT_PUSH_CERT_NONCE environment and possible presense of
>     unsolicited nonce in the GIT_PUSH_CERT blob); and then
>
>  2) applied this patch first (i.e. we still allow unsolicited
>     certificate, and the same fact is now conveyed to the hooks also
>     by GIT_PUSH_CERT_NONCE_STATUS=UNSOLICITED if they sent a nonce,
>     or GIT_PUSH_CERT_NONCE_STATUS=MISSING); and then finally
>
>  3) applied the other patch to reject unsolicited certificate.
>
> then we can stop at any of these three steps and the behaviour of
> three resulting systems make sense and the pre-receive hook can
> reject unsolicited certificates if it wants to, even at step #1.

I do not quite understand the intention of your mail. Are you saying I should
make a patch series to solve the problems as outlined above?

Re: [PATCH] receive-pack.c: don't miss exporting unsolicited push certificates

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Mon, Jan 12, 2015 at 11:07 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>> yes that's what I was trying to hint at. The hook would just see
>>> it is unsolicited instead of not having the state available.
>>
>> OK.  That makes sort of sense.  So if we:
>>
>>  1) did not apply either patch (i.e. we accept unsolicited
>>     certificate, and the fact that we did not exchange "give me this
>>     nonce" "here is your nonce" is conveyed to the hooks by the lack
>>     of GIT_PUSH_CERT_NONCE environment and possible presense of
>>     unsolicited nonce in the GIT_PUSH_CERT blob); and then
>>
>>  2) applied this patch first (i.e. we still allow unsolicited
>>     certificate, and the same fact is now conveyed to the hooks also
>>     by GIT_PUSH_CERT_NONCE_STATUS=UNSOLICITED if they sent a nonce,
>>     or GIT_PUSH_CERT_NONCE_STATUS=MISSING); and then finally
>>
>>  3) applied the other patch to reject unsolicited certificate.
>>
>> then we can stop at any of these three steps and the behaviour of
>> three resulting systems make sense and the pre-receive hook can
>> reject unsolicited certificates if it wants to, even at step #1.
>
> I do not quite understand the intention of your mail. Are you saying I should
> make a patch series to solve the problems as outlined above?

Not really.  All I was saying was that a hypothetical patch series
that progressed in the order above would "make sort-of sense".

I was hoping that readers would ask themselves this question: if we
know that our endgame will be #3, then does it still make much sense
to have the state that only patch #2 is applied?

I think #3 makes #2 unnecessary, as we always ask for nonce when
advertising push-cert capability in the released versions of Git, so
we might get an unsolicited push-cert (which #3 will reject), but we
will never see an unsolicited nonce in a push-cert, as long as we
told the pusher that it is OK to send a push-cert to us.