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

From: Josh Steadmon <steadmon@google.com>
Date: Oct 3, 2018, 21:33 UTC
Message-ID: <20181003213349.GA32105@google.com>
In-Reply-To: <CAGZ79kbD=P__8GU9rV87wREF_MbQA9i2ij6C2qXyaJfqHD3Szg@mail.gmail.com>

On 2018.10.02 15:28, Stefan Beller wrote:

Show 35 quoted lines

> On Tue, Oct 2, 2018 at 3:00 PM Josh Steadmon <steadmon@google.com> wrote:
> >
> > 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).
> 
> > +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?

I believe that git-upload-archive can still speak version 1 without any trouble, but it at least doesn't break anything in the test suite to limit this to v0 either.

Show 12 quoted lines

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

Is there a method or design for advertising multiple acceptable versions from the client? From my understanding, we can only add a single version=X field in the advertisement, but IIUC we can extend this fairly easily? Perhaps we can have "version=X" to mean the preferred version, and then a repeatable "acceptable_version=Y" field or similar?

Show 10 quoted lines

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

I like having it centralized, because enforcing this in git_connect() and discover_refs() catches all the outgoing version advertisements, but there's lots of code paths that lead to those two functions that would all have to have the acceptable version numbers plumbed through.

I suppose we could also have a registry of services to version numbers, but I tend to dislike non-local sources of data. But if the list likes that approach better, I'll be happy to implement it.

> 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") ?

I'm not sure about this. In my series to add a v2 archive command, I added support for a new endpoint for proto v2 and I don't recall seeing any complaints, but that is still open for review.

I suppose if we are strict about serving from a single endpoint, the version registry makes more sense, and individual operations can declare acceptable version numbers before calling any network code?

Thanks for the review, Josh