Re: [PATCH v4 5/6] promisor-remote: check advertised name or URL

From: Christian Couder <christian.couder@gmail.com>
Date: Feb 18, 2025, 11:41 UTC
Message-ID: <CAP8UFD3OmrgPhhRNZ4aF48G3hoi7qr9kka+m9CAoUCw205UP3w@mail.gmail.com>
In-Reply-To: <Z5tZoiAHK-2OqjYJ@pks.im>

On Thu, Jan 30, 2025 at 11:51 AM Patrick Steinhardt <ps@pks.im> wrote:

>
> On Mon, Jan 27, 2025 at 03:48:08PM -0800, Junio C Hamano wrote:

Show 19 quoted lines

> > I wonder if the reader needs to be told a bit more about the
> > security argument here.  I imagine that the attack vector behind the
> > use of "secure" in the above paragraph is for a malicious server
> > that guesses a promisor remote name the client already uses, which
> > has a different URL from what the client expects to be associated
> > with the name, thereby such an acceptance means that the URL used in
> > future fetches would be replaced without the user's consent.  Being
> > able to silently repoint the remote.origin.url at an evil repository
> > you control is indeed a powerful thing, I would guess.  Of course,
> > in a corp environment, such a mechanism to drive the clients to a
> > new repository after upgrading or migrating may be extremely handy.
>
> I'm still very hesitant about letting the server-side control remote
> names at all, as I've already mentioned in previous review rounds. I
> think that it opens up the client for a whole lot of issues that should
> rather be avoided. Most importantly, it takes control away from the
> user, as they are not free anymore to name the remotes however they want
> to. It also casts into stone current behaviour because it is now part of
> the protocol.

The server-side doesn't control remote names at all in this series. There is just a match or no match, depending on the value of promisor.acceptFromServer on the client-side, between what the client already has configured (for example using the clone -c option) and what the server advertises.

> That being said, I get the point that it may make sense to be "agile"
> regarding the promisor remotes. But I think we can achieve that without
> having to compromise on either usability or security by using something
> like a promisor ID instead.

Thanks for the suggestion and the ideas, but I think that what you suggest could be discussed and implemented as part of a follow up patch series. This patch series implements basic checks with information (name and URL) that already exists on the server side and might also be available on the client side. For a number of use cases it is likely enough, and it's also not very complex.

I would be fine with resending the series without this patch, if that's what is prefered though.