Re: [PATCH] transport: add core.allowProtocol config option

From: Stefan Beller <sbeller@google.com>
Date: Nov 2, 2016, 22:41 UTC
Message-ID: <CAGZ79kbytopD1UgvU6R0X18Ge6JsJ46K2KQ-SFLh+hu5tprTmA@mail.gmail.com>
In-Reply-To: <1478125247-62372-1-git-send-email-bmwill@google.com>

On Wed, Nov 2, 2016 at 3:20 PM, Brandon Williams <bmwill@google.com> wrote:

Show 25 quoted lines

> Add configuration option 'core.allowProtocol' to allow users to create a
> whitelist of allowed protocols for fetch/push/clone in their gitconfig.
>
> For git-submodule.sh, fallback to default whitelist only if the user
> hasn't explicitly set `GIT_ALLOW_PROTOCOL` or doesn't have a whitelist
> in their gitconfig.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  Documentation/config.txt | 9 +++++++++
>  git-submodule.sh         | 3 ++-
>  transport.c              | 2 +-
>  3 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 27069ac..7f83e40 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -455,6 +455,15 @@ core.sshCommand::
>         the `GIT_SSH_COMMAND` environment variable and is overridden
>         when the environment variable is set.
>
> +core.allowProtocol::
> +       Provide a colon-separated list of protocols which are allowed to be
> +       used with fetch/push/clone.

ok.

> This is useful to restrict recursive
> +       submodule initialization from an untrusted repository.

ok. Though as a user submodules may not spring to mind immediately here. I think this is generally useful, too. e.g. an admin could put this in the system wide config to prevent certain protocols from being used.

> Any protocol not
> +       mentioned will be disallowed

For the regular fetch/clone/pull case. For the submodule case we still fall back to the hardcoded list of known good things?

> (i.e., this is a whitelist, not a
> +       blacklist).

That is very explicit, I'd drop it. However this inspires bike shedding on the name: What about core.protocolWhitelist instead?

>  If the variable is not set at all, all protocols are
> +       enabled. If the `GIT_ALLOW_PROTOCOL` enviornment variable is set, it is
> +       used as the protocol whitelist instead of this config option.

So the env var is of higher priority than this config.

Show 14 quoted lines

> +
>  core.ignoreStat::
>         If true, Git will avoid using lstat() calls to detect if files have
>         changed by setting the "assume-unchanged" bit for those tracked files
> diff --git a/git-submodule.sh b/git-submodule.sh
> index a024a13..ad94c75 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -27,7 +27,8 @@ cd_to_toplevel
>  #
>  # If the user has already specified a set of allowed protocols,
>  # we assume they know what they're doing and use that instead.
> -: ${GIT_ALLOW_PROTOCOL=file:git:http:https:ssh}
> +config_whitelist=$(git config core.allowProtocol)

So first we lookup the configured protocols.

> +: ${GIT_ALLOW_PROTOCOL=${config_whitelist:-file:git:http:https:ssh}}

Then if they are not configured use the current hard coded white list.

Show 13 quoted lines

>  export GIT_ALLOW_PROTOCOL
>
>  command=
> diff --git a/transport.c b/transport.c
> index d57e8de..b1098cd 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -652,7 +652,7 @@ static const struct string_list *protocol_whitelist(void)
>
>         if (enabled < 0) {
>                 const char *v = getenv("GIT_ALLOW_PROTOCOL");
> -               if (v) {
> +               if (v || !git_config_get_value("core.allowProtocol", &v)) {

This implementation matches what the config promised, I would think.

Do we have any tests for this that could be extended? (Otherwise we'd maybe want to add a test for both the regular case as well as a forbidden submodule?)

Show 6 quoted lines

>                         string_list_split(&allowed, v, ':', -1);
>                         string_list_sort(&allowed);
>                         enabled = 1;
> --
> 2.8.0.rc3.226.g39d4020
>