Re: [PATCH 4/4] promisor-remote: check advertised name or URL

From: Christian Couder <christian.couder@gmail.com>
Date: Sep 10, 2024, 16:32 UTC
Message-ID: <CAP8UFD3sNTfUnzMAJ3xeqBhZbWStYn13ybKMMSdKWy=d4uCdgg@mail.gmail.com>
In-Reply-To: <xmqqbk2d2yw8.fsf@gitster.g>

On Wed, Jul 31, 2024 at 8:35 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > A previous commit introduced a "promisor.acceptFromServer" configuration
> > variable with only "None" or "All" as valid values.
> >
> > Let's introduce "KnownName" and "KnownUrl" as valid values for this
> > configuration option to give more choice to a client about which
> > promisor remotes it might accept among those that the server advertised.
>
> A malicous server can swich name and url correspondence.  The URLs
> this repository uses to lazily fetch missing objects from are the
> only thing that matters, and it does not matter what name the server
> calls these URLs as, I am not sure what value, if any, KnownName has,
> other than adding a potential security hole.

In a corporate setup where clients and servers trust each other to not
switch names and URLs, it could be valuable to still have a bit of
control in a simple way, for example:
  - if servers use many promisor remotes, but clients should only use
a subset of them, or:
  - if the URLs used by clients should not be the same as the URLs
used by servers

In version 2, I have updated the "promisor.acceptFromServer" documentation and the commit message of this patch to better explain cases where the new "KnownName" and "KnownUrl" could be useful.

Show 14 quoted lines

> > In case of "KnownUrl", the client will accept promisor remotes which
> > have both the same name and the same URL configured on the client as the
> > name and URL advertised by the server.
>
> This makes sense, especially if we had updates to documents I
> suggested in my review of [3/4].  If the side effect of "accepting"
> a suggested promisor remote were to only use it as a promisor remote
> on this side, there is no reason to "accept" the same thing again,
> but because the main effect at the protocol level of "accepting" is
> to affect the behaviour of the server in such a way that it is now
> allowed to omit objects that are requested but would be available
> lazily from the promisor remotes in the response, we _do_ need to
> be able to respond with the promisor remotes we are willing to and
> have been using.

Yeah, it is better to let the server know.

> This iteration does not seem to have the true server side support to
> slim its response by omitting objects that are available elsewhere,

Yeah, in version 2, the commit message of patch 3/4 has been improved to say that implementation of this case, which would require S to omit in its response the objects available on X, is left for future improvement.

> but I agree that it is a good approach to get the protocol support
> right.

Thanks.