Re: [PATCH v3] transport: add protocol policy config option

On 11/04, Jeff King wrote:

quoted

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 27069ac..5d845c4 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -2308,6 +2308,31 @@ pretty.<name>::
 	Note that an alias with the same name as a built-in format
 	will be silently ignored.
 
+protocol.allow::
+	If set, provide a user defined default policy for all protocols which
+	don't explicitly have a policy (protocol.<name>.allow).  By default,
+	if unset, known-safe protocols (http, https, git, ssh, file) have a
+	default policy of `always`, known-dangerous protocols (ext) have a
+	default policy of `never`, and all other protocols have a default policy
+	of `user`.  Supported policies:
++
+--
+
+* `always` - protocol is always able to be used.
+
+* `never` - protocol is never able to be used.
+
+* `user` - protocol is only able to be used when `GIT_PROTOCOL_FROM_USER` is
+  either unset or has a value of 1.  This policy should be used when you want a
+  protocol to be usable by the user but don't want it used by commands which
+  execute clone/fetch/pull commands without user input, e.g. recursive
+  submodule initialization.

Makes sense. I wonder if it would be good to emphasize _directly_ usable
here. I.e., "...when you want a protocol to be directly usable by the
user but don't want...".

Should clone/fetch/pull also include push?

You're right, that should really have been clone/fetch/push.

quoted

+protocol.<name>.allow::
+	Set a policy to be used by protocol <name> with clone/fetch/pull commands.
+

Nice that this matches protocol.allow, so we don't need to re-explain
that.

Should the list of protocols be here? I know they're covered under
GIT_ALLOW_PROTOCOL already, but if this is the preferred system, we
should probably explain them here, and then just have GIT_ALLOW_PROTOCOL
refer the user.

Right now the list of protocols under GIT_ALLOW_PROTOCOL looks like it
has a bit more documentation with how the colon list works.  The
protocols are also mentioned above with their default behaviour.

quoted

diff --git a/Documentation/git.txt b/Documentation/git.txt
index ab7215e..ab25580 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt

@@ -1150,13 +1150,13 @@ of clones and fetches.
 	cloning a repository to make a backup).
 
 `GIT_ALLOW_PROTOCOL`::
-	If set, provide a colon-separated list of protocols which are
-	allowed to be used with fetch/push/clone. This is useful to
-	restrict recursive submodule initialization from an untrusted
-	repository. Any protocol not mentioned will be disallowed (i.e.,
-	this is a whitelist, not a blacklist). If the variable is not
-	set at all, all protocols are enabled.  The protocol names
-	currently used by git are:
+	The new way to configure allowed protocols is done through the config
+	interface, though this setting takes precedences.  See
+	linkgit:git-config[1] for more details.  If set, provide a
+	colon-separated list of protocols which are allowed to be used with
+	fetch/push/clone.  Any protocol not mentioned will be disallowed (i.e.,
+	this is a whitelist, not a blacklist).  The protocol names currently
+	used by git are:

I wonder if we can explain this in terms of the config system. Something
like:

  If set to a colon-separated list of zero or more protocols, behave as
  if `protocol.allow` is set to `never`, and each of the listed
  protocols has `protocol.$protocol.allow` set to `always`.

Yeah that makes sense.

quoted

+`GIT_PROTOCOL_FROM_USER`::
+	Set to 0 to prevent protocols used by fetch/push/clone which are
+	configured to the `user` state.  This is useful to restrict recursive
+	submodule initialization from an untrusted repository.  See
+	linkgit:git-config[1] for more details.

Under "this is useful", it may make sense to make it clear that external
programs can use this, too. Something like:

  It may also be useful for programs which feed potentially-untrusted
  URLs to git commands.

I'll put that in too.

quoted

diff --git a/t/lib-proto-disable.sh b/t/lib-proto-disable.sh
index b0917d9..5950fbf 100644
--- a/t/lib-proto-disable.sh
+++ b/t/lib-proto-disable.sh

@@ -1,15 +1,12 @@
 # Test routines for checking protocol disabling.
 
-# test cloning a particular protocol
-#   $1 - description of the protocol
-#   $2 - machine-readable name of the protocol
-#   $3 - the URL to try cloning
-test_proto () {
+# Test clone/fetch/push with GIT_ALLOW_PROTOCOL whitelist
+test_whitelist () {
 	desc=$1
 	proto=$2
 	url=$3
 
-	test_expect_success "clone $1 (enabled)" '
+	test_expect_success "clone $desc (enabled)" '

Yeah, this should have been $desc all along. It makes the diff really
noisy, though. Should it be split out into a preparatory change?

I'll pull it out to make the patch a bit cleaner.

-- 
Brandon Williams