From: Patrick Steinhardt <ps@pks•im>
To: Junio C Hamano <gitster@pobox•com>
Cc: Christian Couder <christian.couder@gmail•com>,
git@vger•kernel.org, Taylor Blau <me@ttaylorr•com>,
Eric Sunshine <sunshine@sunshineco•com>,
Karthik Nayak <karthik.188@gmail•com>,
Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail•com>,
"brian m . carlson" <sandals@crustytoothpaste•net>,
"Randall S . Becker" <rsbecker@nexbridge•com>,
Christian Couder <chriscool@tuxfamily•org>
Subject: Re: [PATCH v4 5/6] promisor-remote: check advertised name or URL
Date: Thu, 30 Jan 2025 11:51:14 +0100 [thread overview]
Message-ID: <Z5tZoiAHK-2OqjYJ@pks.im> (raw)
In-Reply-To: <xmqqa5bbq0nb.fsf@gitster.g>
On Mon, Jan 27, 2025 at 03:48:08PM -0800, Junio C Hamano wrote:
> Christian Couder <christian.couder@gmail•com> writes:
> > promisor.acceptFromServer::
> > If set to "all", a client will accept all the promisor remotes
> > a server might advertise using the "promisor-remote"
> > - capability. Default is "none", which means no promisor remote
> > - advertised by a server will be accepted. By accepting a
> > - promisor remote, the client agrees that the server might omit
> > - objects that are lazily fetchable from this promisor remote
> > - from its responses to "fetch" and "clone" requests from the
> > - client. See linkgit:gitprotocol-v2[5].
> > + capability. If set to "knownName" the client will accept
> > + promisor remotes which are already configured on the client
> > + and have the same name as those advertised by the client. This
> > + is not very secure, but could be used in a corporate setup
> > + where servers and clients are trusted to not switch name and
> > + URLs.
>
> 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.
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.
Instead of announcing remote names, each announced promisor would have
an ID. This ID is opaque and merely used to identify the promisor after
the fact. It could for example be a UUID or something else that is
mostly unique.
The client will then create a promisor remote for each of the remote
names. The name of the promisor is derived from the remote name that it
is being created from. When there's a single promisor only it could for
example be called "origin-promisor". When there are multiple ones they
could be enumerated as "origin-promisor-1". In practice, we can even
roll the dice to generate the name, even though that may not be as user
friendly.
These names are _not_ used to identify the promisor. Instead, we also
write "remote.origin-promisor.id" and point it to the UUID that the
server has advertised. Furthermore, for each promisor that gets added in
this way, we'll also add "remote.origin.promisor" pointing to the
promisor name.
So on a subsequent fetch, we can now:
1. Look up all the promisors for the remote we're fetching from via
the "remote.origin.promisor" multivalue config.
2. For each promisor, we figure out whether its ID is still being
advertised by the remote server. If not, then it is a stale
promisor and we can optionally remove it.
3. If the promisor ID is still being announced we double check whether
the URL we have stored is still valid. If not, we can optionally
update it to point to the new URL.
This buys us a bunch of things:
- We have promisor agility and are easily able to update URLs and
prune out stale promisors.
- Promisors can be renamed by the user at will, as they are identified
by ID and not by remote name. We have to add logic to update the
"remote.*.promisor" links, but that should be doable.
- Each remote has its own set of promisors that cannot conflict with
one another.
From hereon, I'd also redesign "promisor.acceptFromServer" a bit:
- "new" allows newly announced promisor remotes.
- "update" allows updating existing promisor remotes.
- "prune" allows pruning existing promisor remotes.
All of that only applies to promisors connected to the current remote,
of course. Furthermore, the values may be combined arbitrarily with one
another, e.g. you can say "new,update" to only accept new or updated
remotes but not allow pruning, or "update,prune" to only allow updating
or pruning promisors without adding new ones.
I realize that this is a bit more work than what we currently have, but
I think that the design is significantly better than the proposed one.
From my point of view none of this really needs to be part of the
current patch series though, as these are all client-side changes in the
first place, and as far as I understand we don't have the client-side
ready yet anyway.
The only change required would be to adapt the protocol so that we don't
advertise a promisor names anymore, but instead promisor IDs.
Patrick
next prev parent reply other threads:[~2025-01-30 10:51 UTC|newest]
Thread overview: 110+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-07-31 13:40 [PATCH 0/4] Introduce a "promisor-remote" capability Christian Couder
2024-07-31 13:40 ` [PATCH 1/4] version: refactor strbuf_sanitize() Christian Couder
2024-07-31 17:18 ` Junio C Hamano
2024-08-20 11:29 ` Christian Couder
2024-07-31 13:40 ` [PATCH 2/4] strbuf: refactor strbuf_trim_trailing_ch() Christian Couder
2024-07-31 17:29 ` Junio C Hamano
2024-07-31 21:49 ` Taylor Blau
2024-08-20 11:29 ` Christian Couder
2024-08-20 11:29 ` Christian Couder
2024-07-31 13:40 ` [PATCH 3/4] Add 'promisor-remote' capability to protocol v2 Christian Couder
2024-07-31 15:40 ` Taylor Blau
2024-08-20 11:32 ` Christian Couder
2024-08-20 17:01 ` Junio C Hamano
2024-09-10 16:32 ` Christian Couder
2024-07-31 16:16 ` Taylor Blau
2024-08-20 11:32 ` Christian Couder
2024-08-20 16:55 ` Junio C Hamano
2024-09-10 16:32 ` Christian Couder
2024-09-10 17:46 ` Junio C Hamano
2024-07-31 18:25 ` Junio C Hamano
2024-07-31 19:34 ` Junio C Hamano
2024-08-20 12:21 ` Christian Couder
2024-08-05 13:48 ` Patrick Steinhardt
2024-08-19 20:00 ` Junio C Hamano
2024-09-10 16:31 ` Christian Couder
2024-07-31 13:40 ` [PATCH 4/4] promisor-remote: check advertised name or URL Christian Couder
2024-07-31 18:35 ` Junio C Hamano
2024-09-10 16:32 ` Christian Couder
2024-07-31 16:01 ` [PATCH 0/4] Introduce a "promisor-remote" capability Junio C Hamano
2024-07-31 16:17 ` Taylor Blau
2024-09-10 16:29 ` [PATCH v2 " Christian Couder
2024-09-10 16:29 ` [PATCH v2 1/4] version: refactor strbuf_sanitize() Christian Couder
2024-09-10 16:29 ` [PATCH v2 2/4] strbuf: refactor strbuf_trim_trailing_ch() Christian Couder
2024-09-10 16:29 ` [PATCH v2 3/4] Add 'promisor-remote' capability to protocol v2 Christian Couder
2024-09-30 7:56 ` Patrick Steinhardt
2024-09-30 13:28 ` Christian Couder
2024-10-01 10:14 ` Patrick Steinhardt
2024-10-01 18:47 ` Junio C Hamano
2024-11-06 14:04 ` Patrick Steinhardt
2024-11-28 5:47 ` Junio C Hamano
2024-11-28 15:31 ` Christian Couder
2024-11-29 1:31 ` Junio C Hamano
2024-09-10 16:30 ` [PATCH v2 4/4] promisor-remote: check advertised name or URL Christian Couder
2024-09-30 7:57 ` Patrick Steinhardt
2024-09-26 18:09 ` [PATCH v2 0/4] Introduce a "promisor-remote" capability Junio C Hamano
2024-09-27 9:15 ` Christian Couder
2024-09-27 22:48 ` Junio C Hamano
2024-09-27 23:31 ` rsbecker
2024-09-28 10:56 ` Kristoffer Haugsbakk
2024-09-30 7:57 ` Patrick Steinhardt
2024-09-30 9:17 ` Christian Couder
2024-09-30 16:52 ` Junio C Hamano
2024-10-01 10:14 ` Patrick Steinhardt
2024-09-30 16:34 ` Junio C Hamano
2024-09-30 21:26 ` brian m. carlson
2024-09-30 22:27 ` Junio C Hamano
2024-10-01 10:13 ` Patrick Steinhardt
2024-12-06 12:42 ` [PATCH v3 0/5] " Christian Couder
2024-12-06 12:42 ` [PATCH v3 1/5] version: refactor strbuf_sanitize() Christian Couder
2024-12-07 6:21 ` Junio C Hamano
2025-01-27 15:07 ` Christian Couder
2024-12-06 12:42 ` [PATCH v3 2/5] strbuf: refactor strbuf_trim_trailing_ch() Christian Couder
2024-12-07 6:35 ` Junio C Hamano
2025-01-27 15:07 ` Christian Couder
2024-12-16 11:47 ` karthik nayak
2024-12-06 12:42 ` [PATCH v3 3/5] Add 'promisor-remote' capability to protocol v2 Christian Couder
2024-12-07 7:59 ` Junio C Hamano
2025-01-27 15:08 ` Christian Couder
2024-12-06 12:42 ` [PATCH v3 4/5] promisor-remote: check advertised name or URL Christian Couder
2024-12-06 12:42 ` [PATCH v3 5/5] doc: add technical design doc for large object promisors Christian Couder
2024-12-10 1:28 ` Junio C Hamano
2025-01-27 15:12 ` Christian Couder
2024-12-10 11:43 ` Junio C Hamano
2024-12-16 9:00 ` Patrick Steinhardt
2025-01-27 15:11 ` Christian Couder
2025-01-27 18:02 ` Junio C Hamano
2025-02-18 11:42 ` Christian Couder
2024-12-09 8:04 ` [PATCH v3 0/5] Introduce a "promisor-remote" capability Junio C Hamano
2024-12-09 10:40 ` Christian Couder
2024-12-09 10:42 ` Christian Couder
2024-12-09 23:01 ` Junio C Hamano
2025-01-27 15:05 ` Christian Couder
2025-01-27 19:38 ` Junio C Hamano
2025-01-27 15:16 ` [PATCH v4 0/6] " Christian Couder
2025-01-27 15:16 ` [PATCH v4 1/6] version: replace manual ASCII checks with isprint() for clarity Christian Couder
2025-01-27 15:16 ` [PATCH v4 2/6] version: refactor redact_non_printables() Christian Couder
2025-01-27 15:16 ` [PATCH v4 3/6] version: make redact_non_printables() non-static Christian Couder
2025-01-30 10:51 ` Patrick Steinhardt
2025-02-18 11:42 ` Christian Couder
2025-01-27 15:16 ` [PATCH v4 4/6] Add 'promisor-remote' capability to protocol v2 Christian Couder
2025-01-30 10:51 ` Patrick Steinhardt
2025-02-18 11:41 ` Christian Couder
2025-01-27 15:17 ` [PATCH v4 5/6] promisor-remote: check advertised name or URL Christian Couder
2025-01-27 23:48 ` Junio C Hamano
2025-01-28 0:01 ` Junio C Hamano
2025-01-30 10:51 ` Patrick Steinhardt [this message]
2025-02-18 11:41 ` Christian Couder
2025-02-18 11:42 ` Christian Couder
2025-01-27 15:17 ` [PATCH v4 6/6] doc: add technical design doc for large object promisors Christian Couder
2025-01-27 21:14 ` [PATCH v4 0/6] Introduce a "promisor-remote" capability Junio C Hamano
2025-02-18 11:40 ` Christian Couder
2025-02-18 11:32 ` [PATCH v5 0/3] " Christian Couder
2025-02-18 11:32 ` [PATCH v5 1/3] Add 'promisor-remote' capability to protocol v2 Christian Couder
2025-02-18 11:32 ` [PATCH v5 2/3] promisor-remote: check advertised name or URL Christian Couder
2025-02-18 11:32 ` [PATCH v5 3/3] doc: add technical design doc for large object promisors Christian Couder
2025-02-21 8:33 ` Patrick Steinhardt
2025-03-03 16:58 ` Junio C Hamano
2025-02-18 19:07 ` [PATCH v5 0/3] Introduce a "promisor-remote" capability Junio C Hamano
2025-02-21 8:34 ` Patrick Steinhardt
2025-02-21 18:40 ` Junio C Hamano
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Z5tZoiAHK-2OqjYJ@pks.im \
--to=ps@pks$(echo .)im \
--cc=chriscool@tuxfamily$(echo .)org \
--cc=christian.couder@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=karthik.188@gmail$(echo .)com \
--cc=kristofferhaugsbakk@fastmail$(echo .)com \
--cc=me@ttaylorr$(echo .)com \
--cc=rsbecker@nexbridge$(echo .)com \
--cc=sandals@crustytoothpaste$(echo .)net \
--cc=sunshine@sunshineco$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox