Re: [PATCH 1/1] protocol: limit max protocol version per service

From: Stefan Beller <sbeller@google.com>
Date: Oct 2, 2018, 22:28 UTC
Message-ID: <CAGZ79kbD=P__8GU9rV87wREF_MbQA9i2ij6C2qXyaJfqHD3Szg@mail.gmail.com>
In-Reply-To: <59357266bd86e8e0ace9217a97717129a6f76188.1538516853.git.steadmon@google.com>

On Tue, Oct 2, 2018 at 3:00 PM Josh Steadmon <steadmon@google.com> wrote:

Show 14 quoted lines

>
> For services other than git-receive-pack, clients currently advertise
> that they support the version set in the protocol.version config,
> regardless of whether or not there is actually an implementation of that
> service for the given protocol version. This causes backwards-
> compatibility problems when a new implementation for the given
> protocol version is added.
>
> This patch sets maximum allowed protocol versions for git-receive-pack,
> git-upload-archive, and git-upload-pack.
>
> Previously, git-receive-pack would downgrade from v2 to v0, but would
> allow v1 if set in protocol.version. Now, it will downgrade from v2 to
> v1.

But does git-receive-pack understand v1? As to my understanding we have not even defined v1 for push (receive-pack) and archive --remote (upload-archive). v1 is only known to fetch (upload-pack).

Show 7 quoted lines

> +enum protocol_version determine_maximum_protocol_version(
> +               const char *service, enum protocol_version default_version)
> +{
> +       if (!strcmp(service, "git-receive-pack"))
> +               return protocol_v1;
> +       else if (!strcmp(service, "git-upload-archive"))
> +               return protocol_v1;

so I would think these two would be _v0. ... goes and checks ... aa9bab29b8 (upload-pack, receive-pack: introduce protocol version 1, 2017-10-16) seems to actually teach v1 to receive-pack as well, but upload-archive was completely off radar, so I think returning (v1, v0, v2 in the order as in the code) would make sense?

Asides from this, I thought there was a deliberate decision that we'd want to avoid a strict order on the protocol versions, but I could not find prior discussion on list to back up this claim. :/

For example we'd go with e.g. enums instead of integers for version numbers, as then some internal setup could also have things like protocol_v2018-10-02 or protocol_vWhatever; some protocol version may be advantageous to the client, some to the server, and we'd need to negotiate the best version that both are happy with. (e.g. the server may like version 0, 2 and 3, and the client may like 0,2,4 as 3 is bad security wise for the client, so both would negotiate to 2 as their best case)

From a maintenance perspective, do we want to keep this part of the code central, as it ties protocol (as proxied by service name) to the max version number? I would think that we'd rather have the decision local to the code, i.e. builtin/fetch would need to tell protocol.c that it can do (0,1,2) and builtin/push can do (0,1), and then the networking layers of code would figure out by the input from the caller and the input from the user (configured protocol.version) what is the best to go forward from then on.

But I guess having the central place here is not to bad either. How will it cope with the desire of protocol v2 to have only one end point (c.f. serve.{c,h} via builtin/serve as "git serve") ?

Stefan