Merge branch 'cc/lazy-fetch-trusted-bit' into seen

A new 'uploadpack.lazyFetchTrusted' configuration variable has been
introduced to allow 'upload-pack' to lazily fetch missing objects from
configured promisor remotes when serving trusted repositories.

* cc/lazy-fetch-trusted-bit:
  builtin/upload-pack: set GIT_NO_LAZY_FETCH to 0 on trusted repo
  upload-pack: read uploadpack.lazyFetchTrusted
  setup: add 'allow_dot' arg to path_allowlist_apply()
  setup: extract path_allowlist_apply()
  promisor-remote: factor out lazy_fetch_objects()

Documentation/config/uploadpack.adoc

@@ -86,3 +86,45 @@ uploadpack.allowRefInWant::
 	is intended for the benefit of load-balanced servers which may
 	not have the same view of what OIDs their refs point to due to
 	replication delay.
+
+uploadpack.lazyFetchTrusted::
+	These config entries specify repositories that `upload-pack` is
+	allowed to lazily fetch missing objects for. By default,
+	`upload-pack` refuses to lazily fetch (see the description of the
+	`GIT_NO_LAZY_FETCH` environment variable in
+	linkgit:git-upload-pack[1]), because doing so would run `git fetch`,
+	which may execute arbitrary commands specified in the configuration
+	and hooks of the served repository. Listing a repository here tells
+	`upload-pack` that it is trusted, so lazy fetching from the promisor
+	remotes configured in it is allowed. This is equivalent to setting
+	`GIT_NO_LAZY_FETCH` to `0` for the matching repositories. An
+	explicitly set `GIT_NO_LAZY_FETCH` takes precedence over this
+	setting.
++
+Note that this allows lazy fetching from any promisor remote
+configured in the served repository, not only from the promisor
+remotes that the client accepted using the "promisor-remote" protocol
+v2 capability (see linkgit:gitprotocol-v2[5]). The served repository
+is trusted as a whole, including its configuration, so the promisor
+remotes it configures are trusted too. It is the server operator's
+responsibility to make sure that the promisor remotes of a trusted
+repository are also trustworthy.
++
+This is a multi-valued setting, i.e. you can add more than one
+repository via `git config (--global|--system) --add`. To reset the
+list of trusted repositories (e.g. to override any such repositories
+specified in the system config), add a `uploadpack.lazyFetchTrusted`
+entry with an empty value.
++
+A repository is identified by its worktree, or its git directory for a bare
+repository, and the value must be an absolute path. Giving a path with `/*`
+appended to it will trust all repositories under the named directory. To trust
+all served repositories, set `uploadpack.lazyFetchTrusted` to the string `*`.
++
+The value of this setting is interpolated, i.e. `~/<path>` expands to a
+path relative to the home directory and `%(prefix)/<path>` expands to a
+path relative to Git's (runtime) prefix.
++
+Note that this configuration variable is only respected when it is specified
+in protected configuration (see <<SCOPES>>). This prevents untrusted
+repositories from tampering with this value.

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`.
++
+Instead of setting `GIT_NO_LAZY_FETCH` to `0` in the environment, a
+server operator can allow lazy fetching on a per-repository basis by
+listing trusted repositories in the `uploadpack.lazyFetchTrusted`
+configuration variable. See linkgit:git-config[1].
 
 SECURITY
 --------

Documentation/git.adoc

@@ -949,7 +949,9 @@ for full details.
 `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.
+	on demand. On the server side, the `uploadpack.lazyFetchTrusted`
+	configuration variable can control this per-repository. See
+	linkgit:git-upload-pack[1].
 
 `GIT_REFLOG_ACTION`::
 	When a ref is updated, reflog entries are created to keep

builtin/upload-pack.c

@@ -42,10 +42,13 @@ int cmd_upload_pack(int argc,
 		OPT_END()
 	};
 	unsigned enter_repo_flags = ENTER_REPO_ANY_OWNER_OK;
+	bool no_lazy_fetch_set;
 
 	packet_trace_identity("upload-pack");
 	disable_replace_refs();
 	save_commit_buffer = 0;
+
+	no_lazy_fetch_set = !!getenv(NO_LAZY_FETCH_ENVIRONMENT);
 	xsetenv(NO_LAZY_FETCH_ENVIRONMENT, "1", 0);
 
 	argc = parse_options(argc, argv, prefix, options, upload_pack_usage, 0);
@@ -62,6 +65,14 @@ int cmd_upload_pack(int argc,
 	if (!enter_repo(the_repository, dir, enter_repo_flags))
 		die("'%s' does not appear to be a git repository", dir);
 
+	/*
+	 * Relax the GIT_NO_LAZY_FETCH=1 default if the served repo is in
+	 * the "uploadpack.lazyFetchTrusted" protected allowlist and
+	 * GIT_NO_LAZY_FETCH was not already set explicitly.
+	 */
+	if (!no_lazy_fetch_set && upload_pack_lazy_fetch_trusted(the_repository))
+		xsetenv(NO_LAZY_FETCH_ENVIRONMENT, "0", 1);
+
 	switch (determine_protocol_version_server()) {
 	case protocol_v2:
 		if (advertise_refs)

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,
+ * Return 'true' if all the objects could be fetched from the
-				int *remaining_nr, int *to_free,
+ * (non-)accepted remotes, 'false' otherwise.
-				bool accepted_only)
+ */
+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);
+	if (!lazy_fetch_objects(repo, &remaining_oids, &remaining_nr, &to_free)) {
-
+		for (int i = 0; i < remaining_nr; i++) {
-	/* Try accepted remotes first (those the server told us to use) */
+			if (is_promisor_object(repo, &remaining_oids[i]))
-	if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,
+				die(_("could not fetch %s from promisor remote"),
-				 &to_free, true))
+				    oid_to_hex(&remaining_oids[i]));
-		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]));
 	}
 
-all_fetched:
 	if (to_free)
 		free(remaining_oids);
 }

setup.c

@@ -1355,6 +1355,65 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
 	}
 }
 
+void path_allowlist_apply(const char *key, const char *value,
+			  const char *target_path, int *is_match,
+			  bool allow_dot)
+{
+	char *allowed = NULL;
+	char *normalized = NULL;
+
+	if (!value || !*value) {
+		*is_match = 0;
+		return;
+	}
+
+	if (!strcmp(value, "*")) {
+		*is_match = 1;
+		return;
+	}
+
+	if (git_config_pathname(&allowed, key, value) || !allowed)
+		return;
+
+	/*
+	 * Setting the config variable to a non-absolute path makes
+	 * little sense---it won't be relative to the configuration
+	 * file the item is defined in.  Except for ".", which means
+	 * "if we are at the top level of a repository, then it is
+	 * OK", which is slightly tighter than "*" that allows
+	 * discovery.
+	 */
+	if (!is_absolute_path(allowed) && (!allow_dot || strcmp(allowed, "."))) {
+		warning(_("%s '%s' not absolute"), key, allowed);
+		goto end;
+	}
+
+	/*
+	 * A .gitconfig in $HOME may be shared across different
+	 * machines and the config variable entries may or may not
+	 * exist as paths on all of these machines.  In other words,
+	 * it is not a warning worthy event when there is no such path
+	 * on this machine---the entry may be useful elsewhere.
+	 */
+	normalized = real_pathdup(allowed, 0);
+	if (!normalized)
+		goto end;
+
+	if (ends_with(normalized, "/*")) {
+		size_t len = strlen(normalized);
+		if (!fspathncmp(normalized, target_path, len - 1))
+			*is_match = 1;
+		goto end;
+	}
+
+	if (!fspathcmp(target_path, normalized))
+		*is_match = 1;
+
+end:
+	free(normalized);
+	free(allowed);
+}
+
 struct safe_directory_data {
 	char *path;
 	int is_safe;
@@ -1368,54 +1427,7 @@ static int safe_directory_cb(const char *key, const char *value,
 	if (strcmp(key, "safe.directory"))
 		return 0;
 
-	if (!value || !*value) {
+	path_allowlist_apply(key, value, data->path, &data->is_safe, true);
-		data->is_safe = 0;
-	} else if (!strcmp(value, "*")) {
-		data->is_safe = 1;
-	} else {
-		char *allowed = NULL;
-
-		if (!git_config_pathname(&allowed, key, value) && allowed) {
-			char *normalized = NULL;
-
-			/*
-			 * Setting safe.directory to a non-absolute path
-			 * makes little sense---it won't be relative to
-			 * the configuration file the item is defined in.
-			 * Except for ".", which means "if we are at the top
-			 * level of a repository, then it is OK", which is
-			 * slightly tighter than "*" that allows discovery.
-			 */
-			if (!is_absolute_path(allowed) && strcmp(allowed, ".")) {
-				warning(_("safe.directory '%s' not absolute"),
-					allowed);
-				goto next;
-			}
-
-			/*
-			 * A .gitconfig in $HOME may be shared across
-			 * different machines and safe.directory entries
-			 * may or may not exist as paths on all of these
-			 * machines.  In other words, it is not a warning
-			 * worthy event when there is no such path on this
-			 * machine---the entry may be useful elsewhere.
-			 */
-			normalized = real_pathdup(allowed, 0);
-			if (!normalized)
-				goto next;
-
-			if (ends_with(normalized, "/*")) {
-				size_t len = strlen(normalized);
-				if (!fspathncmp(normalized, data->path, len - 1))
-					data->is_safe = 1;
-			} else if (!fspathcmp(data->path, normalized)) {
-				data->is_safe = 1;
-			}
-		next:
-			free(normalized);
-			free(allowed);
-		}
-	}
 
 	return 0;
 }

setup.h

@@ -310,4 +310,32 @@ struct startup_info {
 extern struct startup_info *startup_info;
 extern const char *tmp_original_cwd;
 
+/*
+ * Apply the path allowlist in 'value' against 'target_path' setting
+ * '*is_match' accordingly.
+ *
+ * `value` is the value of a multi-valued config variable named `key`
+ * that holds an allowlist of paths. `target_path` is the (normalized)
+ * path being tested. `*is_match` is updated in place:
+ *
+ *   - an empty value resets it to 0 (so a later, more specific config
+ *     scope can clear entries from a broader one),
+ *   - "*" sets it to 1 (allow everything),
+ *   - "<path>" sets it to 1 if <path> equals `target_path`,
+ *   - "<path>" + "/" + "*" sets it to 1 if <path> is a leading
+ *     directory of `target_path`,
+ *   - any other (unmatching) value leaves `*is_match` unchanged.
+ *
+ * Non-absolute values are rejected with a warning, except "." when
+ * `allow_dot` is set (used by 'safe.directory' to mean "the top level
+ * of the current repository").
+ *
+ * Callers are expected to invoke this once per config value,
+ * typically from a protected-config callback, so that untrusted
+ * repository config cannot influence the decision.
+ */
+void path_allowlist_apply(const char *key, const char *value,
+			  const char *target_path, int *is_match,
+			  bool allow_dot);
+
 #endif /* SETUP_H */

t/t5710-promisor-remote-capability.sh

@@ -173,6 +173,76 @@ test_expect_success "clone with promisor.acceptfromserver set to 'None'" '
 	initialize_server 1 "$oid"
 '
 
+test_expect_success "clone with uploadpack.lazyFetchTrusted" '
+	# No promisors are advertised
+	git -C server config promisor.advertise false &&
+	test_when_finished "rm -rf client" &&
+
+	# The served repo is trusted for lazy fetching
+	test_config_global uploadpack.lazyFetchTrusted "$(pwd)/server" &&
+
+	# Clone without GIT_NO_LAZY_FETCH=0
+	git clone --no-local --filter="blob:limit=5k" server client &&
+
+	# Check that the largest object is not missing on the server
+	# This means the server lazy fetched it
+	check_missing_objects server 0 "" &&
+
+	# Reinitialize server so that the largest object is missing again
+	initialize_server 1 "$oid"
+'
+
+test_expect_success "clone without uploadpack.lazyFetchTrusted fails" '
+	# No promisors are advertised
+	git -C server config promisor.advertise false &&
+	test_when_finished "rm -rf client" &&
+
+	# Note: no uploadpack.lazyFetchTrusted config is set here, so
+	# the served repo is NOT trusted for lazy fetching.
+
+	# Clone without GIT_NO_LAZY_FETCH=0 fails
+	test_must_fail git clone --no-local --filter="blob:limit=5k" server client 2>err &&
+	test_grep "lazy fetching disabled" err &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "uploadpack.lazyFetchTrusted is ignored in repo config" '
+	# No promisors are advertised
+	git -C server config promisor.advertise false &&
+	test_when_finished "rm -rf client" &&
+
+	# The served repo is trusted for lazy fetching, but this is
+	# done in the repo config, not in protected config, so this is
+	# ignored.
+	test_config -C server uploadpack.lazyFetchTrusted "$(pwd)/server" &&
+
+	# Clone without GIT_NO_LAZY_FETCH=0 fails
+	test_must_fail git clone --no-local --filter="blob:limit=5k" server client 2>err &&
+	test_grep "lazy fetching disabled" err &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "explicit GIT_NO_LAZY_FETCH overrides uploadpack.lazyFetchTrusted" '
+	# No promisors are advertised
+	git -C server config promisor.advertise false &&
+	test_when_finished "rm -rf client" &&
+
+	# The served repo is trusted for lazy fetching
+	test_config_global uploadpack.lazyFetchTrusted "$(pwd)/server" &&
+
+	# But GIT_NO_LAZY_FETCH=1 disables lazy fetching, so clone fails
+	test_must_fail env GIT_NO_LAZY_FETCH=1 git clone --no-local \
+		--filter="blob:limit=5k" server client 2>err &&
+	test_grep "lazy fetching disabled" 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" &&

upload-pack.c

@@ -34,6 +34,8 @@
 #include "json-writer.h"
 #include "strmap.h"
 #include "promisor-remote.h"
+#include "setup.h"
+#include "abspath.h"
 
 /* Remember to update object flag allocation in object.h */
 #define THEY_HAVE	(1u << 11)
@@ -1343,6 +1345,41 @@ static int upload_pack_config(const char *var, const char *value,
 	return parse_hide_refs_config(var, value, "uploadpack", &data->hidden_refs);
 }
 
+struct lazy_fetch_trusted {
+	int trusted;
+	char *repo_path;
+};
+
+static int upload_pack_protected_lazy_fetch_config(const char *var, const char *value,
+						   const struct config_context *ctx UNUSED,
+						   void *cb_data)
+{
+	struct lazy_fetch_trusted *data = cb_data;
+
+	if (!strcmp("uploadpack.lazyfetchtrusted", var)) {
+		path_allowlist_apply(var, value, data->repo_path,
+				     &data->trusted, false);
+		return 0;
+	}
+
+	return 0;
+}
+
+bool upload_pack_lazy_fetch_trusted(struct repository *r)
+{
+	struct lazy_fetch_trusted data = { 0 };
+
+	data.repo_path = real_pathdup(r->worktree ? r->worktree : r->gitdir, 0);
+	if (!data.repo_path)
+		return false;
+
+	git_protected_config(upload_pack_protected_lazy_fetch_config, &data);
+
+	free(data.repo_path);
+
+	return !!data.trusted;
+}
+
 static int upload_pack_protected_config(const char *var, const char *value,
 					const struct config_context *ctx UNUSED,
 					void *cb_data)

upload-pack.h

@@ -12,4 +12,7 @@ struct strbuf;
 int upload_pack_advertise(struct repository *r,
 			  struct strbuf *value);
 
+/* Is this repo trusted for lazy fetching? */
+bool upload_pack_lazy_fetch_trusted(struct repository *r);
+
 #endif /* UPLOAD_PACK_H */