From: Josh Steadmon <steadmon@google.com>
To: Stefan Beller <sbeller@google.com>
Cc: git <git@vger.kernel.org>, Jonathan Nieder <jrnieder@gmail.com>,
Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 1/1] protocol: limit max protocol version per service
Date: Wed, 3 Oct 2018 14:33:49 -0700 [thread overview]
Message-ID: <20181003213349.GA32105@google.com> (raw)
In-Reply-To: <CAGZ79kbD=P__8GU9rV87wREF_MbQA9i2ij6C2qXyaJfqHD3Szg@mail.gmail.com>
On 2018.10.02 15:28, Stefan Beller wrote:
> 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.
> 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?
> 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
next prev parent reply other threads:[~2018-10-03 21:34 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-02 21:59 [PATCH 0/1] Limit client version advertisements Josh Steadmon
2018-10-02 21:59 ` [PATCH 1/1] protocol: limit max protocol version per service Josh Steadmon
2018-10-02 22:28 ` Stefan Beller
2018-10-03 21:33 ` Josh Steadmon [this message]
2018-10-03 22:47 ` Stefan Beller
2018-10-05 0:18 ` Josh Steadmon
2018-10-05 19:25 ` Stefan Beller
2018-10-10 23:53 ` Josh Steadmon
2018-10-12 23:30 ` Jonathan Nieder
2018-10-12 23:32 ` Jonathan Nieder
2018-10-12 23:45 ` Josh Steadmon
2018-10-12 23:53 ` Jonathan Nieder
2018-10-13 7:58 ` Junio C Hamano
2018-10-12 1:02 ` [PATCH v2 0/1] Advertise multiple supported proto versions steadmon
2018-10-12 1:02 ` [PATCH v2 1/1] protocol: advertise multiple supported versions steadmon
2018-10-12 22:30 ` Stefan Beller
2018-10-22 22:55 ` Josh Steadmon
2018-10-23 0:37 ` Stefan Beller
2018-11-12 21:49 ` [PATCH v3 0/1] Advertise multiple supported proto versions steadmon
2018-11-12 21:49 ` [PATCH v3 1/1] protocol: advertise multiple supported versions steadmon
2018-11-12 22:33 ` Stefan Beller
2018-11-13 3:24 ` Re* " Junio C Hamano
2018-11-13 19:21 ` Stefan Beller
2018-11-14 2:31 ` Junio C Hamano
2018-11-13 4:01 ` Junio C Hamano
2018-11-13 22:53 ` Josh Steadmon
2018-11-14 2:38 ` Junio C Hamano
2018-11-14 19:57 ` Josh Steadmon
2018-11-16 2:45 ` Junio C Hamano
2018-11-16 19:59 ` Josh Steadmon
2018-11-13 13:35 ` Junio C Hamano
2018-11-13 18:28 ` SZEDER Gábor
2018-11-13 23:03 ` Josh Steadmon
2018-11-14 0:47 ` SZEDER Gábor
2018-11-14 2:44 ` Junio C Hamano
2018-11-14 11:01 ` SZEDER Gábor
2018-11-14 2:30 ` [PATCH v4 0/1] Advertise multiple supported proto versions Josh Steadmon
2018-11-14 2:30 ` [PATCH v4 1/1] protocol: advertise multiple supported versions Josh Steadmon
2018-11-14 10:22 ` [PATCH v4 0/1] Advertise multiple supported proto versions Junio C Hamano
2018-11-14 19:51 ` Josh Steadmon
2018-11-16 10:46 ` Junio C Hamano
2018-11-16 22:34 ` [PATCH v5 " Josh Steadmon
2018-11-16 22:34 ` [PATCH v5 1/1] protocol: advertise multiple supported versions Josh Steadmon
2018-12-14 20:20 ` Ævar Arnfjörð Bjarmason
2018-12-14 21:58 ` Josh Steadmon
2018-12-14 22:39 ` Ævar Arnfjörð Bjarmason
2018-12-18 23:05 ` Josh Steadmon
2018-12-20 21:58 ` [PATCH v6 0/1] Advertise multiple supported proto versions Josh Steadmon
2018-12-20 21:58 ` [PATCH v6 1/1] protocol: advertise multiple supported versions Josh Steadmon
2019-02-05 19:42 ` Jonathan Tan
2019-02-07 23:58 ` Josh Steadmon
2019-02-11 18:53 ` Jonathan Tan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181003213349.GA32105@google.com \
--to=steadmon@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jrnieder@gmail.com \
--cc=sbeller@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).