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

[Josh Steadmon <steadmon@google.com>]

On 2018.10.05 12:25, Stefan Beller wrote:
> > > > 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?
> > >
> > > Ah yeah, that makes sense!
> >
> > Thinking out loud here. Please let me know if I say something stupid :)
> >
> > So we'll have (up to) three pieces of version information we'll care
> > about for version negotiation:
> >
> > 1. (maybe) a client-side protocol.version config entry
> 
> (and in case we don't, we have it implicit ly hardcoded, as
> we have to choose the default for users that don't care themselves about
> this setting)
> 
> > 2. a list of acceptable proto versions for the currently running
> >    operation on the client
> > 3. a list of acceptable proto versions for the server endpoint that
> >    handles the request
> 
> Yes that matches my understanding. The issue is between (1) and (2)
> as (1) is in a generic config, whereas (2) is specific to the command,
> such that it may differ. And as a user you may want to express things
> like: "use the highest version", which is done by setting (1) to "version 2"
> despite (2) not having support of all commands for v2.
> 
> > According to the doc on protocol.version: "If set, clients will attempt
> > to communicate with a server using the specified protocol version. If
> > unset, no attempt will be made by the client to communicate using a
> > particular protocol version, this results in protocol version 0 being
> > used."
> >
> > So, if protocol.version is not set, or set to 0, the client should not
> > attempt any sort of version negotiation.
> 
> Yes, as version 0 is unaware of versions, i.e. there are old installations
> out there where all the versioning code is not there, so in case of an
> old client the new server *must* speak v0 to be able to communicate
> (and vice versa).
> 
> 
> > Otherwise, the client prefers a
> > particular version, but we don't guarantee that they will actually use
> > that version after the (unspecified) version negotiation procedure.
> >
> > If protocol.version is set to something other than 0, we construct a
> > list of acceptable versions for the given operation. If the
> > protocol.version entry is present in that list, we move it to the front
> > of the list to note that it is the preferred version. We send all of
> > these, in order, in the request.
> >
> > When the server endpoint begins to handle a request, it first constructs
> > a list of acceptable versions. If the client specifies a list of
> > versions, we check them one-by-one to see if they are acceptable. If we
> > find a match, we use that version. If we don't find any matches or if
> > the client did not send a version list, we default to v0.
> >
> > Seem reasonable?
> 
> This sounds super reasonable!

So this runs into problems with remote-curl (and possibly other remote
helpers):

builtin/push.c can declare whatever allowed versions it wants, but those
are not carried over when remote-curl is started to actually talk to the
remote. What's worse, remote-curl starts its HTTP connection before it
knows what command it's actually acting as a helper for.

For now, I'm going to try adding an --allowed_versions flag for the
remote helpers, but if anyone has a better idea, let me know.


Thanks,
Josh