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

[Brandon Williams <bmwill@google.com>]

On 11/04, Jeff King wrote:
> > 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.

> 
> > +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.

> 
> > 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.

> 
> > +`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.

> 
> > 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