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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 12, 2015, 19:07 UTC
Message-ID: <xmqqiogb95t4.fsf@gitster.dls.corp.google.com>
In-Reply-To: <54B0A2C1.4040602@gmail.com>

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.

Show 9 quoted lines

>> 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.