* [PATCH 1/3] promisor-remote: factor out lazy_fetch_objects()
2026-07-10 8:51 [PATCH 0/3] Introduce a 'fromAccepted' option to GIT_NO_LAZY_FETCH Christian Couder
@ 2026-07-10 8:51 ` Christian Couder
2026-07-10 8:51 ` [PATCH 2/3] promisor-remote: introduce enum allow_lazy_fetch Christian Couder
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Christian Couder @ 2026-07-10 8:51 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Karthik Nayak, Jeff King,
Elijah Newren, Christian Couder, Christian Couder
In "promisor-remote.c:fetch_objects()", there is a check to disable
lazy fetching when the `GIT_NO_LAZY_FETCH` environment variable is
set. The fetch_objects() function is called once per promisor remote
though. So the check might be performed more times than necessary.
Also promisor_remote_get_direct() mixes up the logic deciding which
promisor remotes to try with the logic checking that the objects
that could not be fetched are promisor objects.
Let's refactor the lazy fetching logic out of these two functions
into a new lazy_fetch_objects() function. This will make it easier
to extend the lazy fetching logic in following commits.
This is a pure refactoring with no intended behavior change. Two
things shift in ways that are observably equivalent though:
- the `GIT_NO_LAZY_FETCH` check is now performed once up front,
instead of once per promisor remote, and
- promisor_remote_init() is no longer called when lazy fetching
is disabled, which is fine as nothing downstream of it, like
is_promisor_object(), needs it in that case.
While at it, let's also convert try_promisor_remotes() to return
'bool' instead of 'int', as it just returns whether all the objects
could be fetched, and document its return value.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
promisor-remote.c | 76 ++++++++++++++++++++++++++++-------------------
1 file changed, 45 insertions(+), 31 deletions(-)
diff --git a/promisor-remote.c b/promisor-remote.c
index 43505d1e1a..65496c69cf 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -31,15 +31,6 @@ static int fetch_objects(struct repository *repo,
FILE *child_in;
int quiet;
- if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {
- static int warning_shown;
- if (!warning_shown) {
- warning_shown = 1;
- warning(_("lazy fetching disabled; some objects may not be available"));
- }
- return -1;
- }
-
child.git_cmd = 1;
child.in = -1;
if (repo != the_repository)
@@ -270,10 +261,15 @@ static int remove_fetched_oids(struct repository *repo,
return remaining_nr;
}
-static int try_promisor_remotes(struct repository *repo,
- struct object_id **remaining_oids,
- int *remaining_nr, int *to_free,
- bool accepted_only)
+/*
+ * Return 'true' if all the objects could be fetched from the
+ * (non-)accepted remotes, 'false' otherwise.
+ */
+static bool try_promisor_remotes(struct repository *repo,
+ struct object_id **remaining_oids,
+ int *remaining_nr,
+ int *to_free,
+ bool accepted_only)
{
struct promisor_remote *r = repo->promisor_remote_config->promisors;
@@ -290,9 +286,37 @@ static int try_promisor_remotes(struct repository *repo,
continue;
}
}
- return 1; /* all fetched */
+ return true; /* all fetched */
}
- return 0;
+ return false;
+}
+
+/*
+ * Return 'true' if all the objects could be fetched, 'false' otherwise.
+ */
+static bool lazy_fetch_objects(struct repository *repo,
+ struct object_id **remaining_oids,
+ int *remaining_nr,
+ int *to_free)
+{
+ if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {
+ static int warning_shown;
+ if (!warning_shown) {
+ warning_shown = 1;
+ warning(_("lazy fetching disabled; some objects may not be available"));
+ }
+ return false;
+ }
+
+ promisor_remote_init(repo);
+
+ /* Try accepted remotes first (those the server told us to use) */
+ if (try_promisor_remotes(repo, remaining_oids, remaining_nr,
+ to_free, true))
+ return true;
+
+ return try_promisor_remotes(repo, remaining_oids, remaining_nr,
+ to_free, false);
}
void promisor_remote_get_direct(struct repository *repo,
@@ -302,28 +326,18 @@ void promisor_remote_get_direct(struct repository *repo,
struct object_id *remaining_oids = (struct object_id *)oids;
int remaining_nr = oid_nr;
int to_free = 0;
- int i;
if (oid_nr == 0)
return;
- promisor_remote_init(repo);
-
- /* Try accepted remotes first (those the server told us to use) */
- if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,
- &to_free, true))
- goto all_fetched;
- if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,
- &to_free, false))
- goto all_fetched;
-
- for (i = 0; i < remaining_nr; i++) {
- if (is_promisor_object(repo, &remaining_oids[i]))
- die(_("could not fetch %s from promisor remote"),
- oid_to_hex(&remaining_oids[i]));
+ if (!lazy_fetch_objects(repo, &remaining_oids, &remaining_nr, &to_free)) {
+ for (int i = 0; i < remaining_nr; i++) {
+ if (is_promisor_object(repo, &remaining_oids[i]))
+ die(_("could not fetch %s from promisor remote"),
+ oid_to_hex(&remaining_oids[i]));
+ }
}
-all_fetched:
if (to_free)
free(remaining_oids);
}
--
2.55.0.125.g395cd2c8ec.dirty
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/3] promisor-remote: introduce enum allow_lazy_fetch
2026-07-10 8:51 [PATCH 0/3] Introduce a 'fromAccepted' option to GIT_NO_LAZY_FETCH Christian Couder
2026-07-10 8:51 ` [PATCH 1/3] promisor-remote: factor out lazy_fetch_objects() Christian Couder
@ 2026-07-10 8:51 ` Christian Couder
2026-07-10 8:51 ` [PATCH 3/3] promisor-remote: teach 'fromAccepted' to GIT_NO_LAZY_FETCH Christian Couder
2026-07-10 19:50 ` [PATCH 0/3] Introduce a 'fromAccepted' option " brian m. carlson
3 siblings, 0 replies; 6+ messages in thread
From: Christian Couder @ 2026-07-10 8:51 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Karthik Nayak, Jeff King,
Elijah Newren, Christian Couder, Christian Couder
The `GIT_NO_LAZY_FETCH` environment variable is currently parsed as
a Boolean, using git_env_bool(), in both "setup.c" and
"promisor-remote.c".
In a following commit, we are going to allow a third value for this
variable, on top of 'true' and 'false'.
To prepare for that, let's introduce an `enum allow_lazy_fetch` with
the possible results of parsing the variable, along with a
parse_allow_lazy_fetch_env() function to parse it, and let's use them
everywhere the variable is parsed.
Note that, as before, an invalid value makes us die(), only the error
message changes from "bad boolean environment value ..." to "bad
environment value ...".
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
promisor-remote.c | 24 +++++++++++++++++++++++-
promisor-remote.h | 13 +++++++++++++
setup.c | 5 ++++-
3 files changed, 40 insertions(+), 2 deletions(-)
diff --git a/promisor-remote.c b/promisor-remote.c
index 65496c69cf..56f57c5267 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -21,6 +21,26 @@ struct promisor_remote_config {
struct promisor_remote **promisors_tail;
};
+enum allow_lazy_fetch parse_allow_lazy_fetch_env(void)
+{
+ const char *v = getenv(NO_LAZY_FETCH_ENVIRONMENT);
+ int val;
+
+ if (!v)
+ return LAZY_FETCH_ALL;
+
+ val = git_parse_maybe_bool(v);
+
+ if (!val)
+ return LAZY_FETCH_ALL;
+ if (val > 0)
+ return LAZY_FETCH_NONE;
+
+ die(_("bad environment value '%s' for '%s'; "
+ "only 'false/0' and 'true/1' are valid"),
+ v, NO_LAZY_FETCH_ENVIRONMENT);
+}
+
static int fetch_objects(struct repository *repo,
const char *remote_name,
const struct object_id *oids,
@@ -299,7 +319,9 @@ static bool lazy_fetch_objects(struct repository *repo,
int *remaining_nr,
int *to_free)
{
- if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {
+ enum allow_lazy_fetch lf = parse_allow_lazy_fetch_env();
+
+ if (lf == LAZY_FETCH_NONE) {
static int warning_shown;
if (!warning_shown) {
warning_shown = 1;
diff --git a/promisor-remote.h b/promisor-remote.h
index 301f5ac5cb..87fc24c9eb 100644
--- a/promisor-remote.h
+++ b/promisor-remote.h
@@ -25,6 +25,19 @@ void promisor_remote_clear(struct promisor_remote_config *config);
struct promisor_remote *repo_promisor_remote_find(struct repository *r, const char *remote_name);
int repo_has_promisor_remote(struct repository *r);
+/* Enum for lazy fetching parsing */
+enum allow_lazy_fetch {
+ LAZY_FETCH_NONE = 0, /* No lazy fetching */
+ LAZY_FETCH_ALL /* Lazy fetch from any promisor remotes */
+};
+
+/*
+ * Parse the NO_LAZY_FETCH_ENVIRONMENT env variable into an
+ * `enum allow_lazy_fetch`.
+ * If parsing fails, then die().
+ */
+enum allow_lazy_fetch parse_allow_lazy_fetch_env(void);
+
/*
* Fetches all requested objects from all promisor remotes, trying them one at
* a time until all objects are fetched.
diff --git a/setup.c b/setup.c
index 0de56a074f..0a81d9f045 100644
--- a/setup.c
+++ b/setup.c
@@ -24,6 +24,7 @@
#include "trace.h"
#include "trace2.h"
#include "worktree.h"
+#include "promisor-remote.h"
enum allowed_bare_repo {
ALLOWED_BARE_REPO_EXPLICIT = 0,
@@ -1051,6 +1052,7 @@ static void setup_git_env_internal(struct repository *repo,
const char *replace_ref_base;
struct set_gitdir_args args = { NULL };
struct strvec to_free = STRVEC_INIT;
+ enum allow_lazy_fetch lf;
args.commondir = getenv_safe(&to_free, GIT_COMMON_DIR_ENVIRONMENT);
args.graft_file = getenv_safe(&to_free, GRAFT_ENVIRONMENT);
@@ -1072,7 +1074,8 @@ static void setup_git_env_internal(struct repository *repo,
if (shallow_file)
set_alternate_shallow_file(repo, shallow_file, 0);
- if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0))
+ lf = parse_allow_lazy_fetch_env();
+ if (lf == LAZY_FETCH_NONE)
fetch_if_missing = 0;
}
--
2.55.0.125.g395cd2c8ec.dirty
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/3] promisor-remote: teach 'fromAccepted' to GIT_NO_LAZY_FETCH
2026-07-10 8:51 [PATCH 0/3] Introduce a 'fromAccepted' option to GIT_NO_LAZY_FETCH Christian Couder
2026-07-10 8:51 ` [PATCH 1/3] promisor-remote: factor out lazy_fetch_objects() Christian Couder
2026-07-10 8:51 ` [PATCH 2/3] promisor-remote: introduce enum allow_lazy_fetch Christian Couder
@ 2026-07-10 8:51 ` Christian Couder
2026-07-10 19:50 ` [PATCH 0/3] Introduce a 'fromAccepted' option " brian m. carlson
3 siblings, 0 replies; 6+ messages in thread
From: Christian Couder @ 2026-07-10 8:51 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Karthik Nayak, Jeff King,
Elijah Newren, Christian Couder, Christian Couder
The `GIT_NO_LAZY_FETCH` environment variable can be set to 'true' or
'false' to enable or disable lazy fetching. By default it is set to
'true' when calling `git upload-pack` to avoid security issues, see
7b70e9efb1 (upload-pack: disable lazy-fetching by default, 2024-04-16).
Recently though, the "promisor-remote" capability was introduced into
protocol v2, which allows a server to advertise some promisor remotes
and clients to accept them or not.
When promisor remotes are advertised by the server and accepted by the
client, it means that they are quite trusted. So the security risks
which come from lazy fetching from them could be considered much more
acceptable.
Let's introduce a 'fromAccepted' option on top of 'true' and 'false'
for `GIT_NO_LAZY_FETCH` to allow lazy fetching only from accepted
promisor remotes.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Documentation/git-upload-pack.adoc | 5 +++
Documentation/git.adoc | 6 ++--
promisor-remote.c | 14 +++++++-
promisor-remote.h | 1 +
t/t5710-promisor-remote-capability.sh | 49 +++++++++++++++++++++++++++
5 files changed, 71 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-upload-pack.adoc b/Documentation/git-upload-pack.adoc
index 9167a321d0..1c2ed9d7ba 100644
--- a/Documentation/git-upload-pack.adoc
+++ b/Documentation/git-upload-pack.adoc
@@ -71,6 +71,11 @@ This is implemented by having `upload-pack` internally set the
(because you are fetching from a partial clone, and you are sure
you trust it), you can explicitly set `GIT_NO_LAZY_FETCH` to
`0`.
++
+`GIT_NO_LAZY_FETCH` can also be set to 'fromAccepted' which allows
+lazy fetching only from remotes that are advertised and accepted using
+the "promisor-remote" protocol v2 capability. See
+linkgit:gitprotocol-v2[5]. This is safer than setting it to `0`.
SECURITY
--------
diff --git a/Documentation/git.adoc b/Documentation/git.adoc
index 8a5cdd3b3d..14a083bcdb 100644
--- a/Documentation/git.adoc
+++ b/Documentation/git.adoc
@@ -947,9 +947,9 @@ for full details.
pathspecs as case-insensitive.
`GIT_NO_LAZY_FETCH`::
- Setting this Boolean environment variable to true tells Git
- not to lazily fetch missing objects from the promisor remote
- on demand.
+ Setting this environment variable controls whether Git is
+ allowed to lazily fetch missing objects from a promisor remote
+ on demand. See linkgit:git-upload-pack[1].
`GIT_REFLOG_ACTION`::
When a ref is updated, reflog entries are created to keep
diff --git a/promisor-remote.c b/promisor-remote.c
index 56f57c5267..c80319f966 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -35,9 +35,11 @@ enum allow_lazy_fetch parse_allow_lazy_fetch_env(void)
return LAZY_FETCH_ALL;
if (val > 0)
return LAZY_FETCH_NONE;
+ if (!strcasecmp(v, "fromAccepted"))
+ return LAZY_FETCH_ACCEPTED;
die(_("bad environment value '%s' for '%s'; "
- "only 'false/0' and 'true/1' are valid"),
+ "only 'false/0', 'true/1' and 'fromAccepted' are valid"),
v, NO_LAZY_FETCH_ENVIRONMENT);
}
@@ -337,6 +339,16 @@ static bool lazy_fetch_objects(struct repository *repo,
to_free, true))
return true;
+ if (lf == LAZY_FETCH_ACCEPTED) {
+ static int warning_shown;
+ if (!warning_shown) {
+ warning_shown = 1;
+ warning(_("lazy fetching from accepted promisor remotes only; "
+ "some objects may not be available"));
+ }
+ return false;
+ }
+
return try_promisor_remotes(repo, remaining_oids, remaining_nr,
to_free, false);
}
diff --git a/promisor-remote.h b/promisor-remote.h
index 87fc24c9eb..0d05ff9d84 100644
--- a/promisor-remote.h
+++ b/promisor-remote.h
@@ -28,6 +28,7 @@ int repo_has_promisor_remote(struct repository *r);
/* Enum for lazy fetching parsing */
enum allow_lazy_fetch {
LAZY_FETCH_NONE = 0, /* No lazy fetching */
+ LAZY_FETCH_ACCEPTED, /* Lazy fetching only from accepted promisor remotes */
LAZY_FETCH_ALL /* Lazy fetch from any promisor remotes */
};
diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh
index 549acff23f..1c61b100b9 100755
--- a/t/t5710-promisor-remote-capability.sh
+++ b/t/t5710-promisor-remote-capability.sh
@@ -173,6 +173,55 @@ test_expect_success "clone with promisor.acceptfromserver set to 'None'" '
initialize_server 1 "$oid"
'
+test_expect_success "clone with GIT_NO_LAZY_FETCH=fromAccepted and accepted promisor remote" '
+ git -C server config promisor.advertise true &&
+ test_when_finished "rm -rf client" &&
+
+ # Clone from server to create a client
+ GIT_NO_LAZY_FETCH=fromAccepted git clone -c remote.lop.promisor=true \
+ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
+ -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
+ -c promisor.acceptfromserver=All \
+ --no-local --filter="blob:limit=5k" server client &&
+
+ # Check that the largest object is still missing on the server
+ check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "clone with GIT_NO_LAZY_FETCH=fromAccepted and no accepted promisor remote" '
+ git -C server config promisor.advertise true &&
+ test_when_finished "rm -rf client" &&
+
+ # Clone from server to create a client
+ # It should fail because the server cannot lazy fetch the missing blob
+ test_must_fail env GIT_NO_LAZY_FETCH=fromAccepted git clone -c remote.lop.promisor=true \
+ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
+ -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
+ -c promisor.acceptfromserver=None \
+ --no-local --filter="blob:limit=5k" server client 2>err &&
+
+ test_grep "lazy fetching from accepted promisor remotes only" err &&
+
+ # Check that the largest object is still missing on the server
+ check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "clone failure with GIT_NO_LAZY_FETCH=bogus" '
+ git -C server config promisor.advertise true &&
+ test_when_finished "rm -rf client" &&
+
+ test_must_fail env GIT_NO_LAZY_FETCH=bogus git clone -c remote.lop.promisor=true \
+ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
+ -c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
+ -c promisor.acceptfromserver=All \
+ --no-local --filter="blob:limit=5k" server client 2>err &&
+
+ test_grep "bad environment value" err &&
+
+ # Check that the largest object is still missing on the server
+ check_missing_objects server 1 "$oid"
+'
+
test_expect_success "init + fetch with promisor.advertise set to 'true'" '
git -C server config promisor.advertise true &&
test_when_finished "rm -rf client" &&
--
2.55.0.125.g395cd2c8ec.dirty
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 0/3] Introduce a 'fromAccepted' option to GIT_NO_LAZY_FETCH
2026-07-10 8:51 [PATCH 0/3] Introduce a 'fromAccepted' option to GIT_NO_LAZY_FETCH Christian Couder
` (2 preceding siblings ...)
2026-07-10 8:51 ` [PATCH 3/3] promisor-remote: teach 'fromAccepted' to GIT_NO_LAZY_FETCH Christian Couder
@ 2026-07-10 19:50 ` brian m. carlson
2026-07-12 9:06 ` Christian Couder
3 siblings, 1 reply; 6+ messages in thread
From: brian m. carlson @ 2026-07-10 19:50 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Patrick Steinhardt, Karthik Nayak, Jeff King,
Elijah Newren
[-- Attachment #1: Type: text/plain, Size: 2097 bytes --]
On 2026-07-10 at 08:51:34, Christian Couder wrote:
> Since 7b70e9efb1 (upload-pack: disable lazy-fetching by default,
> 2024-04-16), lazy fetching has been controlled by the
> `GIT_NO_LAZY_FETCH` environment variable. This is currently an "all or
> nothing" boolean that is set to 'true' by default when calling `git
> upload-pack` for security reasons.
>
> Recently the "promisor-remote" capability was added to protocol v2,
> allowing servers and clients to agree on the promisor remotes they
> can safely use.
>
> This series leverages that capability to implement a pragmatic middle
> ground. By setting `GIT_NO_LAZY_FETCH` to 'fromAccepted', lazy
> fetching is allowed only when fetching from promisor remotes that are
> both advertised by the server and accepted by the client.
>
> Note that using an environment variable for this is probably not the
> best from a usability perspective. An `upload-pack.allowLazyFetch`
> configuration variable would likely be better.
>
> Unfortunately the `GIT_NO_LAZY_FETCH` environment variable is the way
> things currently work. It would be a much bigger and more invasive
> change to implement `upload-pack.allowLazyFetch` in a way that is
> compatible with `GIT_NO_LAZY_FETCH` which has to stay anyway for
> backward compatibility. Therefore, transitioning to a configuration
> variable is left for future work.
I don't think this is a good idea. We get a lot of reports on the
security list involving various tooling that isn't within the scope of
our threat model. This substantially increases the amount of code which
is now subject to that threat model and therefore our security
guarantees and I don't think we should do that as it stands, very
especially while so much of our network-facing code is written in C.
The fetch code by default reads lots of configuration information from
the repository, including remote settings and information and we really
want absolutely none of that code running in the context of an untrusted
repository.
--
brian m. carlson (they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 325 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 0/3] Introduce a 'fromAccepted' option to GIT_NO_LAZY_FETCH
2026-07-10 19:50 ` [PATCH 0/3] Introduce a 'fromAccepted' option " brian m. carlson
@ 2026-07-12 9:06 ` Christian Couder
0 siblings, 0 replies; 6+ messages in thread
From: Christian Couder @ 2026-07-12 9:06 UTC (permalink / raw)
To: brian m. carlson, Christian Couder, git, Junio C Hamano,
Patrick Steinhardt, Karthik Nayak, Jeff King, Elijah Newren
On Fri, Jul 10, 2026 at 9:50 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On 2026-07-10 at 08:51:34, Christian Couder wrote:
> > Since 7b70e9efb1 (upload-pack: disable lazy-fetching by default,
> > 2024-04-16), lazy fetching has been controlled by the
> > `GIT_NO_LAZY_FETCH` environment variable. This is currently an "all or
> > nothing" boolean that is set to 'true' by default when calling `git
> > upload-pack` for security reasons.
> >
> > Recently the "promisor-remote" capability was added to protocol v2,
> > allowing servers and clients to agree on the promisor remotes they
> > can safely use.
> >
> > This series leverages that capability to implement a pragmatic middle
> > ground. By setting `GIT_NO_LAZY_FETCH` to 'fromAccepted', lazy
> > fetching is allowed only when fetching from promisor remotes that are
> > both advertised by the server and accepted by the client.
> >
> > Note that using an environment variable for this is probably not the
> > best from a usability perspective. An `upload-pack.allowLazyFetch`
> > configuration variable would likely be better.
> >
> > Unfortunately the `GIT_NO_LAZY_FETCH` environment variable is the way
> > things currently work. It would be a much bigger and more invasive
> > change to implement `upload-pack.allowLazyFetch` in a way that is
> > compatible with `GIT_NO_LAZY_FETCH` which has to stay anyway for
> > backward compatibility. Therefore, transitioning to a configuration
> > variable is left for future work.
>
> I don't think this is a good idea. We get a lot of reports on the
> security list involving various tooling that isn't within the scope of
> our threat model. This substantially increases the amount of code which
> is now subject to that threat model and therefore our security
> guarantees and I don't think we should do that as it stands, very
> especially while so much of our network-facing code is written in C.
This small series doesn't change any defaults, especially
GIT_NO_LAZY_FETCH is still set to 1 when calling `git upload-pack` by
default. And the new option is more restrictive than the
GIT_NO_LAZY_FETCH=0 option which already exists.
So I don't think it's fair to say that this _substantially increases_
the amount of code subject to some threat model.
I agree that client acceptance of some promisor remotes doesn't make
the served repo trusted. It's a real concern, but I think it's
addressable by different mechanisms. See below.
> The fetch code by default reads lots of configuration information from
> the repository, including remote settings and information and we really
> want absolutely none of that code running in the context of an untrusted
> repository.
When a promisor remote has been accepted, it means both the client and
the server trust it, so at least the promisor remote is not untrusted.
Now the main security issue on the server side is making sure the
served repo itself is also trusted. And I agree that the operator of
the server should decide and mark that trust, not the client.
I also agree that on GitLab/GitHub-style multi-tenant hosts most
repositories shouldn't be marked as trusted.
However note that:
- The operator of the server is the only actor which can set
GIT_NO_LAZY_FETCH on the server (where it matters).
- In the case of corporate/self-hosted repos, the operator also
controls the repos.
- Different features could be developed (in future work) to improve on
the current state:
- a way for lazy fetching to work without reading config files,
triggering hooks, or doing potentially sensitive things,
- an explicit way for operators to mark trusted repos (like
perhaps a server-side config the operator sets per-repo),
- operator-defined allow/deny rules, or maybe
- some ways/scripts/commands to scan repos and check configuration
information, remote settings and everything potentially sensitive to
decide if a repo looks safe enough to allow lazy fetching or not.
I would be happy to hear opinions about those potential features or
any other ways to address the issue.
So I agree that this series doesn't fix all the problems on the server
side, but I think it's still valuable to be able to restrict lazy
fetching to accepted promisor remotes.
Also I definitely agree that the current series should have better
documentation about this, and I plan to improve on that in the v2 of
this series.
Thanks for your insightful comments.
^ permalink raw reply [flat|nested] 6+ messages in thread