Re: [PATCH 2/3] receive-pack: verify push options in cert

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 6, 2017, 00:20 UTC
Message-ID: <20170506002007.GL28740@aiede.svl.corp.google.com>
In-Reply-To: <CAGZ79ka4vG1yd1JtK9bDBjWPUG0UCPMvw2XQUsfX_e_xFCpVLA@mail.gmail.com>

Hi,

Stefan Beller wrote:

> On Fri, May 5, 2017 at 4:46 PM, Jonathan Tan <jonathantanmy@google.com> wrote:

Show 11 quoted lines

>> This sets in stone the requirement that send-pack redundantly send its
>> push options in 2 places, but I think that this is better than the
>> alternatives. Sending push options only within the cert is
>> backwards-incompatible with existing Git servers (which read push
>> options only from outside the cert), and sending push options only
>> outside the cert means that the push options are not signed for.
>
> As the combination of push certs and push options are broken
> (at least when using JGit as well, which are used in Gerrit
> installations), I would not feel too bad about actually going
> the backwards-incompatible way.

Per offline discussion, what you're proposing is to omit the second copy of push options from the client request, so the server does not have to see two copies. I agree that that would be a better protocol, but I think that ship has sailed.

Current versions of git the client and git the server are able to interoperate without trouble (though the server-side bug is ugly in terms of what the push certs mean).

Current versions of JGit the server *require* that the push options be omitted from the push certificate. I don't think there exists a sensible way to interoperate with that, so we can ignore that JGit server behavior as a bug (like you've said).

If we want to omit the second copy of the push certs (which sounds like a reasonable thing to want), that would require a new capability to be added to the protocol to do so. That way, interoperability with existing versions of git the client wouldn't be broken. That could happen on top of this series --- it is not needed for fixing the bug that this series fixes.

To summarize:

 1. I agree that we shouldn't feel bad about breaking compatibility
    with the JGit server behavior at issue, since there is no
    reasonable way to be compatible with it.  And that's what this
    series does --- it breaks compatibility with the broken versions
    of JGit server and picks what the Git client has been doing
    instead.

 2. I don't think we should break new versions of Git's ability to
    interoperate with current versions of the server, even though I
    consider the current server behavior to be broken.

 3. If someone is interested in getting rid of the redundant second
    copy of push options, we have a way to do so, by introducing a
    new capability

Thanks, Jonathan