From: Brandon Williams <hidden> Date: 2016-11-02 22:21:19
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 <redacted>
---
Documentation/config.txt | 9 +++++++++
git-submodule.sh | 3 ++-
transport.c | 2 +-
3 files changed, 12 insertions(+), 2 deletions(-)
@@ -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. 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. If the `GIT_ALLOW_PROTOCOL` enviornment variable is set, it is+ used as the protocol whitelist instead of this config option.+ 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
@@ -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=$(gitconfigcore.allowProtocol)+:${GIT_ALLOW_PROTOCOL=${config_whitelist:-file:git:http:https:ssh}}exportGIT_ALLOW_PROTOCOLcommand=
From: Stefan Beller <hidden> Date: 2016-11-02 22:41:08
On Wed, Nov 2, 2016 at 3:20 PM, Brandon Williams [off-list ref] wrote:
quoted hunk
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 <redacted>
---
Documentation/config.txt | 9 +++++++++
git-submodule.sh | 3 ++-
transport.c | 2 +-
3 files changed, 12 insertions(+), 2 deletions(-)
@@ -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.
quoted hunk
+
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
@@ -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=$(gitconfigcore.allowProtocol)
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?)
From: Brandon Williams <hidden> Date: 2016-11-02 22:47:43
On 11/02, Stefan Beller wrote:
quoted
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.
Oh I pretty much copied the description from what exists for
`GIT_ALLOW_PROTOCOL` which included this bit about submodules.
quoted
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?
Yep! This is done by explicitly setting GIT_ALLOW_PROTOCOL to the
hardcoded list if the user hasn't supplied a whitelist.
quoted
(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?
Simply to keep the name similar to the env variable that already exists
for this functionality.
So the env var is of higher priority than this config.
Then if they are not configured use the current hard coded white list.
The lookup of the configured whitelist is done first but wont be used
unless GIT_ALLOW_PROTOCOL is unset. If neither is set it will fallback
to the hardcoded list.
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?)
I can write a couple tests for a v2 of the patch.
--
Brandon Williams
From: Jeff King <hidden> Date: 2016-11-02 23:05:47
On Wed, Nov 02, 2016 at 03:20:47PM -0700, Brandon Williams wrote:
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.
This says "what", but not "why". What's the use case?
I can see somebody wanting to pare down the whitelist further (e.g.,
because they are carrying ssh credentials that they don't want to use on
behalf of a malicious repo). But in general I'd expect this setting to
be a function of the environment you're operating in, and not the
on-disk config.
Or is the intent to broaden it for cases where you have a clone that
uses some non-standard protocol, and you want it to Just Work on
subsequent recursive fetches?
+core.allowProtocol::
+ 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. If the `GIT_ALLOW_PROTOCOL` enviornment variable is set, it is
+ used as the protocol whitelist instead of this config option.
The "not set at all, all protocols are enabled" bit is not quite
correct, is it? It is true for a top-level fetch, but not for submodule
recursion (and especially since you are talking about submodule
recursion immediately before, it is rather confusing).
quoted hunk
--- 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=$(gitconfigcore.allowProtocol)+:${GIT_ALLOW_PROTOCOL=${config_whitelist:-file:git:http:https:ssh}}
The original uses "=" without a ":" so that an empty variable takes
precedence over the stock list (i.e., allowing nothing). Would you want
the same behavior for the config variable? I.e.:
# this should probably allow nothing, right?
git config core.allowProtocol ""
I think you'd have to check the return code of "git config" to
distinguish those cases.
I thought at first we'd have to deal with leaking "v", but "get_value"
is the "raw" version that gives you the uninterpreted value. I think
that means it may give you NULL, though if we see an implicit bool like:
[core]
allowProtocol
That's nonsense, of course, but we would still segfault. I
think the easiest way to test is:
git -c core.allowProtocol fetch
which seems to segfault for me with this patch.
-Peff
From: Jeff King <hidden> Date: 2016-11-02 23:09:00
On Wed, Nov 02, 2016 at 07:05:39PM -0400, Jeff King wrote:
quoted
+core.allowProtocol::
+ 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. If the `GIT_ALLOW_PROTOCOL` enviornment variable is set, it is
+ used as the protocol whitelist instead of this config option.
The "not set at all, all protocols are enabled" bit is not quite
correct, is it? It is true for a top-level fetch, but not for submodule
recursion (and especially since you are talking about submodule
recursion immediately before, it is rather confusing).
Heh, just saw that you copied this straight from the discussion of
GIT_ALLOW_PROTOCOL. What idiot wrote the original? :)
It might be worth fixing both places (or possibly just fixing the
original and phrasing this one as "If GIT_ALLOW_PROTOCOL is not set, use
this as the default value; see git(1) for details").
-Peff
From: Brandon Williams <hidden> Date: 2016-11-02 23:33:11
On 11/02, Jeff King wrote:
On Wed, Nov 02, 2016 at 03:20:47PM -0700, Brandon Williams wrote:
quoted
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.
This says "what", but not "why". What's the use case?
I can see somebody wanting to pare down the whitelist further (e.g.,
because they are carrying ssh credentials that they don't want to use on
behalf of a malicious repo). But in general I'd expect this setting to
be a function of the environment you're operating in, and not the
on-disk config.
Or is the intent to broaden it for cases where you have a clone that
uses some non-standard protocol, and you want it to Just Work on
subsequent recursive fetches?
quoted
+core.allowProtocol::
+ 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. If the `GIT_ALLOW_PROTOCOL` enviornment variable is set, it is
+ used as the protocol whitelist instead of this config option.
The "not set at all, all protocols are enabled" bit is not quite
correct, is it? It is true for a top-level fetch, but not for submodule
recursion (and especially since you are talking about submodule
recursion immediately before, it is rather confusing).
Yeah stefan mentioned this to me. I simply copied the documentaion from
GIT_ALLOW_PROTOCOL, perhaps that should be updated as well?
quoted
--- 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=$(gitconfigcore.allowProtocol)+:${GIT_ALLOW_PROTOCOL=${config_whitelist:-file:git:http:https:ssh}}
The original uses "=" without a ":" so that an empty variable takes
precedence over the stock list (i.e., allowing nothing). Would you want
the same behavior for the config variable? I.e.:
# this should probably allow nothing, right?
git config core.allowProtocol ""
I think you'd have to check the return code of "git config" to
distinguish those cases.
Oh, I didn't think of that case. That can be done easy enough, just
makes the code a bit more verbose.
I thought at first we'd have to deal with leaking "v", but "get_value"
is the "raw" version that gives you the uninterpreted value. I think
that means it may give you NULL, though if we see an implicit bool like:
[core]
allowProtocol
That's nonsense, of course, but we would still segfault. I
think the easiest way to test is:
git -c core.allowProtocol fetch
which seems to segfault for me with this patch.
what is the desired behavior when a user provides a config in a way that
isn't intended?
--
Brandon Williams
From: Brandon Williams <hidden> Date: 2016-11-02 23:35:03
On 11/02, Jeff King wrote:
On Wed, Nov 02, 2016 at 07:05:39PM -0400, Jeff King wrote:
quoted
quoted
+core.allowProtocol::
+ 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. If the `GIT_ALLOW_PROTOCOL` enviornment variable is set, it is
+ used as the protocol whitelist instead of this config option.
The "not set at all, all protocols are enabled" bit is not quite
correct, is it? It is true for a top-level fetch, but not for submodule
recursion (and especially since you are talking about submodule
recursion immediately before, it is rather confusing).
Heh, just saw that you copied this straight from the discussion of
GIT_ALLOW_PROTOCOL. What idiot wrote the original? :)
It might be worth fixing both places (or possibly just fixing the
original and phrasing this one as "If GIT_ALLOW_PROTOCOL is not set, use
this as the default value; see git(1) for details").
-Peff
haha K I'll fix the original as well.
--
Brandon Williams
I thought at first we'd have to deal with leaking "v", but "get_value"
is the "raw" version that gives you the uninterpreted value. I think
that means it may give you NULL, though if we see an implicit bool like:
[core]
allowProtocol
That's nonsense, of course, but we would still segfault. I
think the easiest way to test is:
git -c core.allowProtocol fetch
which seems to segfault for me with this patch.
what is the desired behavior when a user provides a config in a way that
isn't intended?
oh...I can just drop in git_config_get_string_const() instead.
--
Brandon Williams
From: Jeff King <hidden> Date: 2016-11-03 00:08:45
On Wed, Nov 02, 2016 at 04:46:13PM -0700, Brandon Williams wrote:
quoted
quoted
I thought at first we'd have to deal with leaking "v", but "get_value"
is the "raw" version that gives you the uninterpreted value. I think
that means it may give you NULL, though if we see an implicit bool like:
[core]
allowProtocol
That's nonsense, of course, but we would still segfault. I
think the easiest way to test is:
git -c core.allowProtocol fetch
which seems to segfault for me with this patch.
what is the desired behavior when a user provides a config in a way that
isn't intended?
oh...I can just drop in git_config_get_string_const() instead.
Yes, it will call git_config_string(), which will make sure there's an
actual value and die otherwise. But note that it also duplicates the
string, so you'd have to deal with freeing it.
-Peff
From: Jonathan Nieder <hidden> Date: 2016-11-03 00:23:02
(+peff and bburky, who introduced GIT_ALLOW_PROTOCOL)
Brandon Williams wrote:
Add configuration option 'core.allowProtocol' to allow users to create a
whitelist of allowed protocols for fetch/push/clone in their gitconfig.
Ooh.
This would be especially useful at $DAYJOB, where there is a custom
sso:// protocol that is often used by submodules. Using an envvar to
whitelist it globally is painful because
- it disables other protocols even when explicitly requested on a
plain "git clone" command line by the user. By comparison, the
built-in git-submodule.sh whitelist only applies to submodules.
- platform-specific instructions to set an environment variable can
be more difficult than "just set this git configuration"
Another difficulty with setting GIT_ALLOW_PROTOCOL globally is that it
requires copy/pasting the default value from upstream and then adding
the values I want. There's no straightforward way to get the current
value and add to it, in case I want to benefit from future upstream
fixes to the default list.
That is, would it be possible to use something like
[protocol "sso"]
allow = always
instead of
[core]
allowProtocol = file:git:http:https:....:sso
?
[...]
quoted hunk
--- 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=$(gitconfigcore.allowProtocol)+:${GIT_ALLOW_PROTOCOL=${config_whitelist:-file:git:http:https:ssh}}
optional: To avoid config parsing when GIT_ALLOW_PROTOCOL is already
set, could do something like
if ! test "${GIT_ALLOW_PROTOCOL+set}"
then
GIT_ALLOW_PROTOCOL=$(
git config --name-only --get-regexp 'protocol\..*\.allow' always |
sed -e 's/^protocol.//' -e 's/.allow$//' |
tr '\n' ':'
)
GIT_ALLOW_PROTOCOL=${GIT_ALLOW_PROTOCOL%:}
: ${GIT_ALLOW_PROTOCOL:=file:git:http:https:ssh}
fi
[...]
This has the effect of always disabling other protocols when
core.allowProtocol is set. Is that intended?
Like the default list used by submodule, I'd be happiest if this only
applied to repositories cloned implicitly instead of those passed
directly to 'git clone'.
That reminds me: external tools also set GIT_ALLOW_PROTOCOL when the
user hasn't set it explicitly, like git-submodule.sh does. E.g.
repo <https://gerrit.googlesource.com/git-repo/+/466b8c4e/git_command.py#171>,
mercurial <https://www.mercurial-scm.org/repo/hg/file/b032a7b676c6/mercurial/subrepo.py#l1404>.
Other external tools consume GIT_ALLOW_PROTOCOL, like 'go get'
<https://go.googlesource.com/go/+/55620a0e/src/cmd/go/vcs.go#64>.
Can we make it more convenient for them to support this configuration
too?
An example approach would be a GIT_ALLOW_PROTOCOL var returned by
"git var".
That way git-submodule.sh could do
: ${GIT_ALLOW_PROTOCOL=$(git var GIT_ALLOW_PROTOCOL)}
and it would just work. Other tools could do the same, with a
fallback to the current default until new enough git is in widespread
use.
Thanks and hope that helps,
Jonathan
Thanks for CCing me.
I haven't looked at this implementation in detail, but it would be
good to move this configuration into the config system because I think
we can more easily provide a default safe configuration.
It would be nice to use this to introduce a default list of
whitelisted protocols that even applies to `git clone`. I strongly
think we need to find a way to have git-remote-ext disabled by
default. This could be a way to do it.
On Wed, Nov 2, 2016 at 7:22 PM, Jonathan Nieder [off-list ref] wrote:
Most of these are my fault too. I encouraged git-repo and mercurial to
use GIT_ALLOW_PROTOCOL to avoid security issues from git-remote-ext.
--
Blake Burkhart
From: Brandon Williams <hidden> Date: 2016-11-03 00:50:42
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 <redacted>
---
Documentation/config.txt | 8 ++++++++
Documentation/git.txt | 6 ++++--
git-submodule.sh | 3 ++-
t/lib-proto-disable.sh | 27 +++++++++++++++++++++++++++
t/t5815-submodule-protos.sh | 22 ++++++++++++++++++++++
transport.c | 6 ++++++
6 files changed, 69 insertions(+), 3 deletions(-)
@@ -455,6 +455,14 @@ 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. Any protocol not mentioned will be+ disallowed (i.e., this is a whitelist, not a blacklist). If the+ `GIT_ALLOW_PROTOCOL` environment variable is set, it is used as the+ protocol whitelist instead of this config option. If neither is set,+ all protocols are enabled. See git(1) for more details.+ 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
@@ -1155,8 +1155,10 @@ of clones and fetches. 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:+ set at all, all protocols are enabled. The exception to this is when+ running `git-submodule` which will use a default list of known-safe+ protocols (file:git:http:https:ssh) in the event no whitelist is+ provided. The protocol names currently used by git are: - `file`: any local file-based path (including `file://` URLs, or local paths)
@@ -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}+whitelist=$(gitconfigcore.allowProtocol)||whitelist=file:git:http:https:ssh+:${GIT_ALLOW_PROTOCOL=$whitelist}exportGIT_ALLOW_PROTOCOLcommand=
@@ -62,6 +62,33 @@ test_proto () {test_must_failgitclone--bare"$url"tmp.git)'++# Run tests again using the gitconfig method for setting a whitelist+test_expect_success"clone $1 (enabled)"'+rm-rftmp.git&&+git-ccore.allowProtocol="$proto"clone--bare"$url"tmp.git+'++test_expect_success"fetch $1 (enabled)"'+git-Ctmp.git-ccore.allowProtocol="$proto"fetch+'++test_expect_success"push $1 (enabled)"'+git-Ctmp.git-ccore.allowProtocol="$proto"pushoriginHEAD:pushed+'++test_expect_success"push $1 (disabled)"'+test_must_failgit-Ctmp.git-ccore.allowProtocol=nonepushoriginHEAD:pushed+'++test_expect_success"fetch $1 (disabled)"'+test_must_failgit-Ctmp.git-ccore.allowProtocol=nonefetch+'++test_expect_success"clone $1 (disabled)"'+rm-rftmp.git&&+test_must_failgit-Ctmp.git-ccore.allowProtocol=noneclone--bare"$url"tmp.git+'}# set up an ssh wrapper that will access $host/$repo in the
@@ -40,4 +40,26 @@ test_expect_success 'user can override whitelist' 'GIT_ALLOW_PROTOCOL=extgit-Cdstsubmoduleupdateext-module'+test_expect_success'reset dst repo for config tests''+rm-rfdst&&+gitclone.dst&&+git-Cdstsubmoduleinit+'++test_expect_success'update of ssh not allowed when not in config whitelist''+test_must_failgit-Cdst-ccore.allowProtocol=nonesubmoduleupdatessh-module+'++test_expect_success'update of ssh allowed via config whitelist''+git-Cdst-ccore.allowProtocol="ssh:http:https"submoduleupdatessh-module+'++test_expect_success'update of ext not allowed''+test_must_failgit-Cdst-ccore.allowProtocol=sshsubmoduleupdateext-module+'++test_expect_success'user can override whitelist''+git-Cdst-ccore.allowProtocol=extsubmoduleupdateext-module+'+ test_done
From: Jeff King <hidden> Date: 2016-11-03 14:38:14
On Wed, Nov 02, 2016 at 05:22:25PM -0700, Jonathan Nieder wrote:
Another difficulty with setting GIT_ALLOW_PROTOCOL globally is that it
requires copy/pasting the default value from upstream and then adding
the values I want. There's no straightforward way to get the current
value and add to it, in case I want to benefit from future upstream
fixes to the default list.
I agree that this is a big drawback of the current scheme, and it would
be nice to be able to say "also allow".
That is, would it be possible to use something like
[protocol "sso"]
allow = always
instead of
[core]
allowProtocol = file:git:http:https:....:sso
?
One complication is that the whitelist has multiple states:
1. if it's not used at all, anything goes
2. if it exists and has zero or more entries, only those entries are
allowed
And then submodules are an exception to (1), because it's not anything
goes. It's "this default safe whitelist".
So when does protocol.sso.allow kick in? We wouldn't want it to trigger
case (2) for things like fetch (disabling other non-allowed protocols).
Nor do I think we'd only want it for the submodule case, as I would
assume that "protocol.sso.allow = false" should disable it.
So I think this probably needs to be a separate parallel system where
each protocol can be white- or black-listed in a context-specific way.
Like:
protocol.X.allow = always | user | never
Where "user" specifies that the protocol is OK coming directly from the
user, but not from other sources. And we default known-common-and-good
ones like protocol.http.allow to "always", unknown ones (like "foo"
which runs "remote-foo") to "user"), and possibly known-scary ones like
"ext") to "never".
Then we need some way of telling git "you are in a context where the URL
parameter is not coming from the user". Probably via the environment in
GIT_PROTOCOL_FROM_USER or similar. Which git-submodule would sent when
recursing clones, along with things like "go get".
In other words, stop asking git-submodule or "go get" to specify policy,
and let them specify context that can be used to implement policy that
the user specifies (and have git provide a sane default policy).
I think this would all take a backseat to GIT_ALLOW_PROTOCOL, for
backwards compatibility, and then GIT_ALLOW_PROTOCOL could slowly die
off over time.
I think under my proposal above this ugliness just goes away, as all
they have to do is say "trust my URLs less; they come from an automated
source" without specifying policy themselves.
But we may still want...
An example approach would be a GIT_ALLOW_PROTOCOL var returned by
"git var".
That way git-submodule.sh could do
: ${GIT_ALLOW_PROTOCOL=$(git var GIT_ALLOW_PROTOCOL)}
and it would just work. Other tools could do the same, with a
fallback to the current default until new enough git is in widespread
use.
...some automated way to say "is this protocol supported"? I think it is
not just "give me ALLOW_PROTOCOL" anymore, though, but "apply your rules
to this protocol, and tell me if it is supported".
I don't think things like "go get" would need it, but you would if you
had a porcelain built around git that was accessing a URL _not_ via
git-fetch, but wanted to apply git's rules. That could come as a step 2
later, though.
-Peff
From: Brandon Williams <hidden> Date: 2016-11-03 17:25:23
On 11/03, Jeff King wrote:
On Wed, Nov 02, 2016 at 05:22:25PM -0700, Jonathan Nieder wrote:
quoted
Another difficulty with setting GIT_ALLOW_PROTOCOL globally is that it
requires copy/pasting the default value from upstream and then adding
the values I want. There's no straightforward way to get the current
value and add to it, in case I want to benefit from future upstream
fixes to the default list.
I agree that this is a big drawback of the current scheme, and it would
be nice to be able to say "also allow".
quoted
That is, would it be possible to use something like
[protocol "sso"]
allow = always
instead of
[core]
allowProtocol = file:git:http:https:....:sso
?
One complication is that the whitelist has multiple states:
1. if it's not used at all, anything goes
2. if it exists and has zero or more entries, only those entries are
allowed
And then submodules are an exception to (1), because it's not anything
goes. It's "this default safe whitelist".
So when does protocol.sso.allow kick in? We wouldn't want it to trigger
case (2) for things like fetch (disabling other non-allowed protocols).
Nor do I think we'd only want it for the submodule case, as I would
assume that "protocol.sso.allow = false" should disable it.
So I think this probably needs to be a separate parallel system where
each protocol can be white- or black-listed in a context-specific way.
Like:
protocol.X.allow = always | user | never
It sounds like there is interest for this sort of behavior, it would
definitely require a larger change than what I initially proposed. One
problem I see though is that with this we have support for both a
blacklist and a whitelist. Which wins? Or do we simply generate a
whitelist of allowed protocols which includes all protocols with allow
set to 'always' and if it is set to 'never' then it just isn't included
in the whitelist?
I don't know if I'm sold on a 'user' state just yet, perhaps that's just
because I view a whitelist or blacklist as well black and white and
having this user state adds in a gray area.
quoted
An example approach would be a GIT_ALLOW_PROTOCOL var returned by
"git var".
That way git-submodule.sh could do
: ${GIT_ALLOW_PROTOCOL=$(git var GIT_ALLOW_PROTOCOL)}
and it would just work. Other tools could do the same, with a
fallback to the current default until new enough git is in widespread
use.
...some automated way to say "is this protocol supported"? I think it is
not just "give me ALLOW_PROTOCOL" anymore, though, but "apply your rules
to this protocol, and tell me if it is supported".
I agree, if we do add different states to a protocol then we couldn't
simply ask for a whitelist/blacklist of protocols anymore since its more
of a graylist :) (if such a thing exits).
--
Brandon Williams
From: Stefan Beller <hidden> Date: 2016-11-03 17:39:41
quoted
protocol.X.allow = always | user | never
It sounds like there is interest for this sort of behavior, it would
definitely require a larger change than what I initially proposed. One
problem I see though is that with this we have support for both a
blacklist and a whitelist. Which wins?
For the submodule operations we'll use a whitelist, because we want to
provide security and for the other case we can offer a blacklist as a bandaid.
My opinion on blacklists is roughly aligned with e.g. :
https://blog.codinghorror.com/blacklists-dont-work/http://blog.deepinstinct.com/2016/02/04/when-blacklists-dont-really-work/
So IMHO we could drop the "never" and substitute it with a "warn" or
"ask-user", such that this configuration becomes a white list for both cases:
protocol.X.allow = always | user | warn
Or do we simply generate a
whitelist of allowed protocols which includes all protocols with allow
set to 'always' and if it is set to 'never' then it just isn't included
in the whitelist?
So you're suggesting that setting it to "never" doesn't have any effect
except for cluttering the config file?
I don't think we should do that; each setting should have an impact.
So maybe the "never" would be there to disallow protocols of the hardcoded
white list (e.g. http)
I don't know if I'm sold on a 'user' state just yet, perhaps that's just
because I view a whitelist or blacklist as well black and white and
having this user state adds in a gray area.
Well the "user" state is to differentiate between the
* "I consciously typed `git clone ...` (and e.g. I know what happens as
I know the server admin and they are trustworthy.)
* a repository contains a possible hostile .gitmodules file such
that I am not aware of the network connection.
From: Brandon Williams <hidden> Date: 2016-11-03 17:51:42
On 11/03, Stefan Beller wrote:
quoted
quoted
protocol.X.allow = always | user | never
It sounds like there is interest for this sort of behavior, it would
definitely require a larger change than what I initially proposed. One
problem I see though is that with this we have support for both a
blacklist and a whitelist. Which wins?
For the submodule operations we'll use a whitelist, because we want to
provide security and for the other case we can offer a blacklist as a bandaid.
My opinion on blacklists is roughly aligned with e.g. :
https://blog.codinghorror.com/blacklists-dont-work/http://blog.deepinstinct.com/2016/02/04/when-blacklists-dont-really-work/
So IMHO we could drop the "never" and substitute it with a "warn" or
"ask-user", such that this configuration becomes a white list for both cases:
protocol.X.allow = always | user | warn
quoted
Or do we simply generate a
whitelist of allowed protocols which includes all protocols with allow
set to 'always' and if it is set to 'never' then it just isn't included
in the whitelist?
So you're suggesting that setting it to "never" doesn't have any effect
except for cluttering the config file?
I don't think we should do that; each setting should have an impact.
So maybe the "never" would be there to disallow protocols of the hardcoded
white list (e.g. http)
Thats what I meant, if a protocol is listed as 'never' then it just
removes that protocol from the whitelist. That way we still have the
benefit of using a whitelist vs a blacklist. Also, if we move in this
direction should we setup a default whitelist of allowed protocols?
quoted
I don't know if I'm sold on a 'user' state just yet, perhaps that's just
because I view a whitelist or blacklist as well black and white and
having this user state adds in a gray area.
Well the "user" state is to differentiate between the
* "I consciously typed `git clone ...` (and e.g. I know what happens as
I know the server admin and they are trustworthy.)
* a repository contains a possible hostile .gitmodules file such
that I am not aware of the network connection.
This is still a gray area to me. I think that if we have a whitelist of
protocols then it should be a true whitelist and not have some means of
going around it. It just seems like something that could be exploited.
--
Brandon Williams
From: Jeff King <hidden> Date: 2016-11-03 17:53:34
On Thu, Nov 03, 2016 at 10:25:15AM -0700, Brandon Williams wrote:
quoted
So I think this probably needs to be a separate parallel system where
each protocol can be white- or black-listed in a context-specific way.
Like:
protocol.X.allow = always | user | never
It sounds like there is interest for this sort of behavior, it would
definitely require a larger change than what I initially proposed. One
problem I see though is that with this we have support for both a
blacklist and a whitelist. Which wins? Or do we simply generate a
whitelist of allowed protocols which includes all protocols with allow
set to 'always' and if it is set to 'never' then it just isn't included
in the whitelist?
I think trying to combine the two or generate the whitelist from the
more flexible format is a recipe for madness.
I'd design the new system from scratch, and have it kick in _only_ when
GIT_ALLOW_PROTOCOL is not set. That lets existing callers continue to
have the safe behavior until they are ready to move to the new format.
Something like the patch below (which is just for illustration, and not
tested beyond compilation).
I don't know if I'm sold on a 'user' state just yet, perhaps that's just
because I view a whitelist or blacklist as well black and white and
having this user state adds in a gray area.
The lack of understanding the "user" context is what makes the current
system so painful. The only way a caller can influence the system is to
hand over the policy directly: this is allowed, this is not. But systems
like "go get" should not be setting policy. They should be giving us a
hint about the context, and letting git implement the policy.
-- >8 --
@@ -664,10 +664,69 @@ static const struct string_list *protocol_whitelist(void)returnenabled?&allowed:NULL;}+enumprotocol_allow_config{+PROTOCOL_ALLOW_NEVER,+PROTOCOL_ALLOW_USER_ONLY,+PROTOCOL_ALLOW_ALWAYS+};++staticenumprotocol_allow_configparse_protocol_config(constchar*key,+constchar*value)+{+if(!strcasecmp(value,"always"))+returnPROTOCOL_ALLOW_ALWAYS;+elseif(!strcasecmp(value,"never"))+returnPROTOCOL_ALLOW_NEVER;+elseif(!strcasecmp(value,"user"))+returnPROTOCOL_ALLOW_USER_ONLY;++/* XXX maybe also interpret git_config_bool() here? */+die("unknown value for config '%s': %s",key,value);+}++staticenumprotocol_allow_configget_protocol_config(constchar*type)+{+char*key=xstrfmt("protocol.%s.allow",type);+char*value;++if(!git_config_get_string(key,&value)){+enumprotocol_allow_configret=+parse_protocol_config(key,value);+free(key);+free(value);+returnret;+}+free(key);++/* known safe */+if(!strcmp(type,"http")||+!strcmp(type,"https")||+!strcmp(type,"git")||+!strcmp(type,"ssh"))+returnPROTOCOL_ALLOW_ALWAYS;++/* known scary; err on the side of caution */+if(!strcmp(type,"ext"))+returnPROTOCOL_ALLOW_NEVER;++/* unknown; let them be used only directly by the user */+returnPROTOCOL_ALLOW_USER_ONLY;+}+intis_transport_allowed(constchar*type){-conststructstring_list*allowed=protocol_whitelist();-return!allowed||string_list_has_string(allowed,type);+conststructstring_list*whitelist=protocol_whitelist();+if(whitelist)+returnstring_list_has_string(whitelist,type);++switch(get_protocol_config(type)){+casePROTOCOL_ALLOW_ALWAYS:+return1;+casePROTOCOL_ALLOW_NEVER:+return0;+casePROTOCOL_ALLOW_USER_ONLY:+returngit_env_bool("GIT_PROTOCOL_FROM_USER",1);+}}voidtransport_check_allowed(constchar*type)
From: Jeff King <hidden> Date: 2016-11-03 18:00:25
On Thu, Nov 03, 2016 at 10:39:35AM -0700, Stefan Beller wrote:
quoted
quoted
protocol.X.allow = always | user | never
It sounds like there is interest for this sort of behavior, it would
definitely require a larger change than what I initially proposed. One
problem I see though is that with this we have support for both a
blacklist and a whitelist. Which wins?
For the submodule operations we'll use a whitelist, because we want to
provide security and for the other case we can offer a blacklist as a bandaid.
My opinion on blacklists is roughly aligned with e.g. :
https://blog.codinghorror.com/blacklists-dont-work/http://blog.deepinstinct.com/2016/02/04/when-blacklists-dont-really-work/
So IMHO we could drop the "never" and substitute it with a "warn" or
"ask-user", such that this configuration becomes a white list for both cases:
protocol.X.allow = always | user | warn
I don't think blacklists work in the general case, because they grow out
of date and fail-open. But you want to have _some_ blacklisting
mechanism, in order override a decision of the whitelist.
For instance, the default submodule whitelist would probably include
https and ssh. But if I'm cloning potentially malicious repos and I
don't ever want them to trigger ssh (because I don't want them to use my
ssh keys, whereas I have explicitly set up my credentials such that http
is safe to use), I would want to be able to do:
git config protocol.ssh.allow never
(or "git -c", or whatever).
True, a whitelist is safer. If we add a new "foo" protocol that also
looks at your ssh keys, you're screwed. And that's why I designed it as
a pure-whitelist in the first place. But it comes at the price of
convenience, because you have to manually add each new innocent protocol
to the whitelist.
So you're suggesting that setting it to "never" doesn't have any effect
except for cluttering the config file?
I don't think we should do that; each setting should have an impact.
So maybe the "never" would be there to disallow protocols of the hardcoded
white list (e.g. http)
Exactly.
quoted
I don't know if I'm sold on a 'user' state just yet, perhaps that's just
because I view a whitelist or blacklist as well black and white and
having this user state adds in a gray area.
Well the "user" state is to differentiate between the
* "I consciously typed `git clone ...` (and e.g. I know what happens as
I know the server admin and they are trustworthy.)
* a repository contains a possible hostile .gitmodules file such
that I am not aware of the network connection.
Right. I had assumed that we would tell the difference between those
automatically (by seeing if we got the URL on the command line), but
things like "go get" show that the context is often before git is even
called.
-Peff
From: Jeff King <hidden> Date: 2016-11-03 18:02:30
On Thu, Nov 03, 2016 at 10:51:31AM -0700, Brandon Williams wrote:
quoted
quoted
I don't know if I'm sold on a 'user' state just yet, perhaps that's just
because I view a whitelist or blacklist as well black and white and
having this user state adds in a gray area.
Well the "user" state is to differentiate between the
* "I consciously typed `git clone ...` (and e.g. I know what happens as
I know the server admin and they are trustworthy.)
* a repository contains a possible hostile .gitmodules file such
that I am not aware of the network connection.
This is still a gray area to me. I think that if we have a whitelist of
protocols then it should be a true whitelist and not have some means of
going around it. It just seems like something that could be exploited.
How do you implement:
git clone --recursive trusted:foo.git
and use your ssh keys for the "trusted" server, but not for any servers
mentioned in .gitmodules?
You need some way of distinguishing between the two contexts (and
setting policy for each).
-Peff
From: Brandon Williams <hidden> Date: 2016-11-03 18:08:26
On 11/03, Jeff King wrote:
On Thu, Nov 03, 2016 at 10:51:31AM -0700, Brandon Williams wrote:
quoted
quoted
quoted
I don't know if I'm sold on a 'user' state just yet, perhaps that's just
because I view a whitelist or blacklist as well black and white and
having this user state adds in a gray area.
Well the "user" state is to differentiate between the
* "I consciously typed `git clone ...` (and e.g. I know what happens as
I know the server admin and they are trustworthy.)
* a repository contains a possible hostile .gitmodules file such
that I am not aware of the network connection.
This is still a gray area to me. I think that if we have a whitelist of
protocols then it should be a true whitelist and not have some means of
going around it. It just seems like something that could be exploited.
How do you implement:
git clone --recursive trusted:foo.git
and use your ssh keys for the "trusted" server, but not for any servers
mentioned in .gitmodules?
You need some way of distinguishing between the two contexts (and
setting policy for each).
-Peff
Interesting. Ok I can see how this would be a useful now. Thanks for
the example :)
--
Brandon Williams
From: Brandon Williams <hidden> Date: 2016-11-03 18:20:05
On 11/03, Jeff King wrote:
+
+ /* unknown; let them be used only directly by the user */
+ return PROTOCOL_ALLOW_USER_ONLY;
+}
+
int is_transport_allowed(const char *type)
{
- const struct string_list *allowed = protocol_whitelist();
- return !allowed || string_list_has_string(allowed, type);
+ const struct string_list *whitelist = protocol_whitelist();
+ if (whitelist)
+ return string_list_has_string(whitelist, type);
+
+ switch (get_protocol_config(type)) {
+ case PROTOCOL_ALLOW_ALWAYS:
+ return 1;
+ case PROTOCOL_ALLOW_NEVER:
+ return 0;
+ case PROTOCOL_ALLOW_USER_ONLY:
+ return git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
+ }
I know this is just a rough patch you wiped up but one question:
With the 'user' state, how exactly do you envision this env variable
working? Do we want the user to have to explicitly set
GIT_PROTOCOL_FROM_USER in their environment and then have these other
commands (like git-submodule) explicitly clear the env var or would we
rather these subcommands set a variable indicating they aren't coming
from the user and the deafult state (no var set) is a user run command?
--
Brandon Williams
From: Jeff King <hidden> Date: 2016-11-03 18:24:35
On Thu, Nov 03, 2016 at 01:53:27PM -0400, Jeff King wrote:
I'd design the new system from scratch, and have it kick in _only_ when
GIT_ALLOW_PROTOCOL is not set. That lets existing callers continue to
have the safe behavior until they are ready to move to the new format.
Something like the patch below (which is just for illustration, and not
tested beyond compilation).
Here's that same patch with a few tweaks:
- it changes git-submodule to use the new, more flexible system (which
also gets a it a lot more test coverage)
- it tweaks two tests which use the "ext" helper to enable it (since
it's blacklisted by default; I have mixed feelings on that, but I
see why Blake wants it, as it would have protected things like "go
get" out of the box).
- it adds "file://" as a known-good protocol, even for submodules,
which matches the current code. I am not sure if this is reasonable
or not. A malicious repository probably can't do much by pointing
you to cloning your own repo as a submodule unless you then _also_
run some arbitrary code to expose it, at which point it's generally
game-over anyway.
And I'd expect automated services (like GitHub Pages) to already
have a cut-down whitelist via GIT_ALLOW_PROTOCOL (and I happen to
know that it goes).
So this seems like a reasonable direction to me. It obviously needs
documentation and tests. Arguably there should be a fallback "allow"
value when a protocol is not mentioned in the config so that you could
convert the default from "user" to "never" if you wanted your config to
specify a pure whitelist.
Without that, I think we'd want to keep GIT_ALLOW_PROTOCOL for the truly
paranoid (though we should keep it indefinitely either way for backwards
compatibility).
Do you have interest in picking this up and running with it?
-Peff
@@ -21,14 +21,10 @@ require_work_treewt_prefix=$(gitrev-parse--show-prefix) cd_to_toplevel-# Restrict ourselves to a vanilla subset of protocols; the URLs-# we get are under control of a remote repository, and we do not-# want them kicking off arbitrary git-remote-* programs.-#-# 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}-exportGIT_ALLOW_PROTOCOL+# Tell the rest of git that any URLs we get don't come+# directly from the user, so it can apply policy as appropriate.+GIT_PROTOCOL_FROM_USER=0+exportGIT_PROTOCOL_FROM_USERcommand=branch=
@@ -664,10 +664,70 @@ static const struct string_list *protocol_whitelist(void)returnenabled?&allowed:NULL;}+enumprotocol_allow_config{+PROTOCOL_ALLOW_NEVER=0,+PROTOCOL_ALLOW_USER_ONLY,+PROTOCOL_ALLOW_ALWAYS+};++staticenumprotocol_allow_configparse_protocol_config(constchar*key,+constchar*value)+{+if(!strcasecmp(value,"always"))+returnPROTOCOL_ALLOW_ALWAYS;+elseif(!strcasecmp(value,"never"))+returnPROTOCOL_ALLOW_NEVER;+elseif(!strcasecmp(value,"user"))+returnPROTOCOL_ALLOW_USER_ONLY;++/* XXX maybe also interpret git_config_bool() here? */+die("unknown value for config '%s': %s",key,value);+}++staticenumprotocol_allow_configget_protocol_config(constchar*type)+{+char*key=xstrfmt("protocol.%s.allow",type);+char*value;++if(!git_config_get_string(key,&value)){+enumprotocol_allow_configret=+parse_protocol_config(key,value);+free(key);+free(value);+returnret;+}+free(key);++/* known safe */+if(!strcmp(type,"http")||+!strcmp(type,"https")||+!strcmp(type,"git")||+!strcmp(type,"ssh")||+!strcmp(type,"file"))+returnPROTOCOL_ALLOW_ALWAYS;++/* known scary; err on the side of caution */+if(!strcmp(type,"ext"))+returnPROTOCOL_ALLOW_NEVER;++/* unknown; let them be used only directly by the user */+returnPROTOCOL_ALLOW_USER_ONLY;+}+intis_transport_allowed(constchar*type){-conststructstring_list*allowed=protocol_whitelist();-return!allowed||string_list_has_string(allowed,type);+conststructstring_list*whitelist=protocol_whitelist();+if(whitelist)+returnstring_list_has_string(whitelist,type);++switch(get_protocol_config(type)){+casePROTOCOL_ALLOW_ALWAYS:+return1;+casePROTOCOL_ALLOW_NEVER:+return0;+casePROTOCOL_ALLOW_USER_ONLY:+returngit_env_bool("GIT_PROTOCOL_FROM_USER",1);+}}voidtransport_check_allowed(constchar*type)
From: Jeff King <hidden> Date: 2016-11-03 18:26:04
On Thu, Nov 03, 2016 at 11:19:54AM -0700, Brandon Williams wrote:
On 11/03, Jeff King wrote:
quoted
+
+ /* unknown; let them be used only directly by the user */
+ return PROTOCOL_ALLOW_USER_ONLY;
+}
+
int is_transport_allowed(const char *type)
{
- const struct string_list *allowed = protocol_whitelist();
- return !allowed || string_list_has_string(allowed, type);
+ const struct string_list *whitelist = protocol_whitelist();
+ if (whitelist)
+ return string_list_has_string(whitelist, type);
+
+ switch (get_protocol_config(type)) {
+ case PROTOCOL_ALLOW_ALWAYS:
+ return 1;
+ case PROTOCOL_ALLOW_NEVER:
+ return 0;
+ case PROTOCOL_ALLOW_USER_ONLY:
+ return git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
+ }
I know this is just a rough patch you wiped up but one question:
With the 'user' state, how exactly do you envision this env variable
working? Do we want the user to have to explicitly set
GIT_PROTOCOL_FROM_USER in their environment and then have these other
commands (like git-submodule) explicitly clear the env var or would we
rather these subcommands set a variable indicating they aren't coming
from the user and the deafult state (no var set) is a user run command?
See the follow-up I just posted, but basically, the rules are:
- if you don't say anything, then the URL is from the user
- git-submodule would set it to "0" (i.e., tell us to be more careful)
- tools like "go get" would similarly set it to "0" if they are
passing untrusted URLs
-Peff
From: Brandon Williams <hidden> Date: 2016-11-03 18:45:46
On 11/03, Jeff King wrote:
So this seems like a reasonable direction to me. It obviously needs
documentation and tests. Arguably there should be a fallback "allow"
value when a protocol is not mentioned in the config so that you could
convert the default from "user" to "never" if you wanted your config to
specify a pure whitelist.
Yes I agree there should probably be a fallback value of 'never' maybe?
What you currently have preserves the behavior of what git does
now, if we did instead have a fallback of 'never' it would break current
users who don't already use GIT_ALLOW_PROTOCOL (well only if they use
crazy protocols). We could ease into it though and start with default
to allow and then transition to a true whitelist sometime after this
change has been made?
Without that, I think we'd want to keep GIT_ALLOW_PROTOCOL for the truly
paranoid (though we should keep it indefinitely either way for backwards
compatibility).
Do you have interest in picking this up and running with it?
Yep! Thanks for the help in shaping this.
--
Brandon Williams
From: Jeff King <hidden> Date: 2016-11-03 18:51:15
On Thu, Nov 03, 2016 at 11:45:38AM -0700, Brandon Williams wrote:
On 11/03, Jeff King wrote:
quoted
So this seems like a reasonable direction to me. It obviously needs
documentation and tests. Arguably there should be a fallback "allow"
value when a protocol is not mentioned in the config so that you could
convert the default from "user" to "never" if you wanted your config to
specify a pure whitelist.
Yes I agree there should probably be a fallback value of 'never' maybe?
What you currently have preserves the behavior of what git does
now, if we did instead have a fallback of 'never' it would break current
users who don't already use GIT_ALLOW_PROTOCOL (well only if they use
crazy protocols). We could ease into it though and start with default
to allow and then transition to a true whitelist sometime after this
change has been made?
I don't see the value in moving the out-of-the-box install to any
default except "user". Right now the experience of using a third-party
helper is something like:
cp git-remote-hg /somewhere/in/your/PATH
git clone hg::whatever
We restrict its use in submodules by default, which is unlikely to bite
many people. But if we started falling back to "never" all the time,
then that second command would break until you officially "approve"
remote-hg in your config.
I was thinking of just something to let people decide to have that level
of paranoia themselves (especially if they want to just set up a
whole-system white list via the config without bothering with
environment variables). Like:
git config --system protocol.allow never
git config --system protocol.https.allow always
That behaves exactly like:
export GIT_ALLOW_PROTOCOL=https
except it just works everywhere, without having to tweak the environment
of every process.
quoted
Do you have interest in picking this up and running with it?
Yep! Thanks for the help in shaping this.
Great, thanks. I'm happy to review or discuss further as necessary.
-Peff
From: Brandon Williams <hidden> Date: 2016-11-03 18:56:18
On 11/03, Jeff King wrote:
On Thu, Nov 03, 2016 at 11:45:38AM -0700, Brandon Williams wrote:
quoted
On 11/03, Jeff King wrote:
quoted
So this seems like a reasonable direction to me. It obviously needs
documentation and tests. Arguably there should be a fallback "allow"
value when a protocol is not mentioned in the config so that you could
convert the default from "user" to "never" if you wanted your config to
specify a pure whitelist.
Yes I agree there should probably be a fallback value of 'never' maybe?
What you currently have preserves the behavior of what git does
now, if we did instead have a fallback of 'never' it would break current
users who don't already use GIT_ALLOW_PROTOCOL (well only if they use
crazy protocols). We could ease into it though and start with default
to allow and then transition to a true whitelist sometime after this
change has been made?
I don't see the value in moving the out-of-the-box install to any
default except "user". Right now the experience of using a third-party
helper is something like:
cp git-remote-hg /somewhere/in/your/PATH
git clone hg::whatever
We restrict its use in submodules by default, which is unlikely to bite
many people. But if we started falling back to "never" all the time,
then that second command would break until you officially "approve"
remote-hg in your config.
I was thinking of just something to let people decide to have that level
of paranoia themselves (especially if they want to just set up a
whole-system white list via the config without bothering with
environment variables). Like:
git config --system protocol.allow never
git config --system protocol.https.allow always
That behaves exactly like:
export GIT_ALLOW_PROTOCOL=https
except it just works everywhere, without having to tweak the environment
of every process.
Ah ok, so essentially letting the user specify a default behaviour
themselves.
--
Brandon Williams
From: Brandon Williams <hidden> Date: 2016-11-04 20:55:48
Previously the `GIT_ALLOW_PROTOCOL` environment variable was used to
specify a whitelist of protocols to be used in clone/fetch/pull
commands. This patch introduces new configuration options for more
fine-grained control for allowing/disallowing protocols. This also has
the added benefit of allowing easier construction of a protocol
whitelist on systems where setting an environment variable is
non-trivial.
Now users can specify a policy to be used for each type of protocol via
the 'protocol.<name>.allow' config option. A default policy for all
unknown protocols can be set with the 'protocol.allow' config option.
If no user configured default is made git, by default, will allow
known-safe protocols (http, https, git, ssh, file), disallow
known-dangerous protocols (ext), and have a default poliy of `user` for
all other protocols.
The supported policies are `always`, `never`, and `user`. The `user`
policy can be used to configure a protocol to be usable when explicitly
used by a user, while disallowing it for commands which run
clone/fetch/pull commands without direct user intervention (e.g.
recursive initialization of submodules). Commands which can potentially
clone/fetch/pull from untrusted repositories without user intervention
can export `GIT_PROTOCOL_FROM_USER` with a value of '0' to prevent
protocols configured to the `user` policy from being used.
Signed-off-by: Brandon Williams <redacted>
---
Documentation/config.txt | 25 ++++++++
Documentation/git.txt | 19 +++---
git-submodule.sh | 12 ++--
t/lib-proto-disable.sh | 132 +++++++++++++++++++++++++++++++++++----
t/t5509-fetch-push-namespaces.sh | 1 +
t/t5802-connect-helper.sh | 1 +
transport.c | 73 +++++++++++++++++++++-
7 files changed, 235 insertions(+), 28 deletions(-)
@@ -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.++--++protocol.<name>.allow::+ Set a policy to be used by protocol <name> with clone/fetch/pull commands.+ pull.ff:: By default, Git does not create an extra merge commit when merging a commit that is a descendant of the current commit. Instead, the
@@ -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: - `file`: any local file-based path (including `file://` URLs, or local paths)
@@ -1174,6 +1174,11 @@ of clones and fetches. - any external helpers are named by their protocol (e.g., use `hg` to allow the `git-remote-hg` helper)+`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. Discussion[[Discussion]] ------------------------
@@ -21,14 +21,10 @@ require_work_treewt_prefix=$(gitrev-parse--show-prefix) cd_to_toplevel-# Restrict ourselves to a vanilla subset of protocols; the URLs-# we get are under control of a remote repository, and we do not-# want them kicking off arbitrary git-remote-* programs.-#-# 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}-exportGIT_ALLOW_PROTOCOL+# Tell the rest of git that any URLs we get don't come+# directly from the user, so it can apply policy as appropriate.+GIT_PROTOCOL_FROM_USER=0+exportGIT_PROTOCOL_FROM_USERcommand=branch=
@@ -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=$1proto=$2url=$3-test_expect_success"clone $1 (enabled)"'+test_expect_success"clone $desc (enabled)"'rm-rftmp.git&&(GIT_ALLOW_PROTOCOL=$proto&&
@@ -64,6 +61,119 @@ test_proto () {'}+test_config(){+desc=$1+proto=$2+url=$3++# Test clone/fetch/push with protocol.<type>.allow config+test_expect_success"clone $desc (enabled with config)"'+rm-rftmp.git&&+git-cprotocol.$proto.allow=alwaysclone--bare"$url"tmp.git+'++test_expect_success"fetch $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=alwaysfetch+'++test_expect_success"push $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=alwayspushoriginHEAD:pushed+'++test_expect_success"push $desc (disabled)"'+test_must_failgit-Ctmp.git-cprotocol.$proto.allow=neverpushoriginHEAD:pushed+'++test_expect_success"fetch $desc (disabled)"'+test_must_failgit-Ctmp.git-cprotocol.$proto.allow=neverfetch+'++test_expect_success"clone $desc (disabled)"'+rm-rftmp.git&&+test_must_failgit-cprotocol.$proto.allow=neverclone--bare"$url"tmp.git+'++# Test clone/fetch/push with protocol.user.allow and its env var+test_expect_success"clone $desc (enabled)"'+rm-rftmp.git&&+git-cprotocol.$proto.allow=userclone--bare"$url"tmp.git+'++test_expect_success"fetch $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=userfetch+'++test_expect_success"push $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=userpushoriginHEAD:pushed+'++test_expect_success"push $desc (disabled)"'+(+cdtmp.git&&+GIT_PROTOCOL_FROM_USER=0&&+exportGIT_PROTOCOL_FROM_USER&&+test_must_failgit-cprotocol.$proto.allow=userpushoriginHEAD:pushed+)+'++test_expect_success"fetch $desc (disabled)"'+(+cdtmp.git&&+GIT_PROTOCOL_FROM_USER=0&&+exportGIT_PROTOCOL_FROM_USER&&+test_must_failgit-cprotocol.$proto.allow=userfetch+)+'++test_expect_success"clone $desc (disabled)"'+rm-rftmp.git&&+(+GIT_PROTOCOL_FROM_USER=0&&+exportGIT_PROTOCOL_FROM_USER&&+test_must_failgit-cprotocol.$proto.allow=userclone--bare"$url"tmp.git+)+'++# Test clone/fetch/push with protocol.allow user defined default+test_expect_success"clone $desc (enabled)"'+rm-rftmp.git&&+gitconfig--globalprotocol.allowalways&&+gitclone--bare"$url"tmp.git+'++test_expect_success"fetch $desc (enabled)"'+git-Ctmp.gitfetch+'++test_expect_success"push $desc (enabled)"'+git-Ctmp.gitpushoriginHEAD:pushed+'++test_expect_success"push $desc (disabled)"'+gitconfig--globalprotocol.allownever&&+test_must_failgit-Ctmp.gitpushoriginHEAD:pushed+'++test_expect_success"fetch $desc (disabled)"'+test_must_failgit-Ctmp.gitfetch+'++test_expect_success"clone $desc (disabled)"'+rm-rftmp.git&&+test_must_failgitclone--bare"$url"tmp.git+'+}++# 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_whitelist"$@"++test_config"$@"+}+# set up an ssh wrapper that will access $host/$repo in the# trash directory, and enable it for subsequent tests. setup_ssh_wrapper(){
@@ -664,10 +664,79 @@ static const struct string_list *protocol_whitelist(void)returnenabled?&allowed:NULL;}+enumprotocol_allow_config{+PROTOCOL_ALLOW_NEVER=0,+PROTOCOL_ALLOW_USER_ONLY,+PROTOCOL_ALLOW_ALWAYS+};++staticenumprotocol_allow_configparse_protocol_config(constchar*key,+constchar*value)+{+if(!strcasecmp(value,"always"))+returnPROTOCOL_ALLOW_ALWAYS;+elseif(!strcasecmp(value,"never"))+returnPROTOCOL_ALLOW_NEVER;+elseif(!strcasecmp(value,"user"))+returnPROTOCOL_ALLOW_USER_ONLY;++die("unknown value for config '%s': %s",key,value);+}++staticenumprotocol_allow_configget_protocol_config(constchar*type)+{+char*key=xstrfmt("protocol.%s.allow",type);+char*value;++if(!git_config_get_string(key,&value)){+enumprotocol_allow_configret=+parse_protocol_config(key,value);+free(key);+free(value);+returnret;+}+free(key);++/* if defined, use user default for unknown protocols */+if(!git_config_get_string("protocol.allow",&value)){+enumprotocol_allow_configret=+parse_protocol_config("protocol.allow",value);+free(value);+returnret;+}++/* known safe */+if(!strcmp(type,"http")||+!strcmp(type,"https")||+!strcmp(type,"git")||+!strcmp(type,"ssh")||+!strcmp(type,"file"))+returnPROTOCOL_ALLOW_ALWAYS;++/* known scary; err on the side of caution */+if(!strcmp(type,"ext"))+returnPROTOCOL_ALLOW_NEVER;++/* unknown; by default let them be used only directly by the user */+returnPROTOCOL_ALLOW_USER_ONLY;+}+intis_transport_allowed(constchar*type){-conststructstring_list*allowed=protocol_whitelist();-return!allowed||string_list_has_string(allowed,type);+conststructstring_list*whitelist=protocol_whitelist();+if(whitelist)+returnstring_list_has_string(whitelist,type);++switch(get_protocol_config(type)){+casePROTOCOL_ALLOW_ALWAYS:+return1;+casePROTOCOL_ALLOW_NEVER:+return0;+casePROTOCOL_ALLOW_USER_ONLY:+returngit_env_bool("GIT_PROTOCOL_FROM_USER",1);+}++die("BUG: invalid protocol_allow_config type");}voidtransport_check_allowed(constchar*type)
From: Stefan Beller <hidden> Date: 2016-11-04 22:38:47
On Fri, Nov 4, 2016 at 1:55 PM, Brandon Williams [off-list ref] wrote:
quoted hunk
Previously the `GIT_ALLOW_PROTOCOL` environment variable was used to
specify a whitelist of protocols to be used in clone/fetch/pull
commands. This patch introduces new configuration options for more
fine-grained control for allowing/disallowing protocols. This also has
the added benefit of allowing easier construction of a protocol
whitelist on systems where setting an environment variable is
non-trivial.
Now users can specify a policy to be used for each type of protocol via
the 'protocol.<name>.allow' config option. A default policy for all
unknown protocols can be set with the 'protocol.allow' config option.
If no user configured default is made git, by default, will allow
known-safe protocols (http, https, git, ssh, file), disallow
known-dangerous protocols (ext), and have a default poliy of `user` for
all other protocols.
The supported policies are `always`, `never`, and `user`. The `user`
policy can be used to configure a protocol to be usable when explicitly
used by a user, while disallowing it for commands which run
clone/fetch/pull commands without direct user intervention (e.g.
recursive initialization of submodules). Commands which can potentially
clone/fetch/pull from untrusted repositories without user intervention
can export `GIT_PROTOCOL_FROM_USER` with a value of '0' to prevent
protocols configured to the `user` policy from being used.
Signed-off-by: Brandon Williams <redacted>
---
Documentation/config.txt | 25 ++++++++
Documentation/git.txt | 19 +++---
git-submodule.sh | 12 ++--
t/lib-proto-disable.sh | 132 +++++++++++++++++++++++++++++++++++----
t/t5509-fetch-push-namespaces.sh | 1 +
t/t5802-connect-helper.sh | 1 +
transport.c | 73 +++++++++++++++++++++-
7 files changed, 235 insertions(+), 28 deletions(-)
@@ -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,
Use hyphens (`protocol.<name>.allow`) to highlight the config option.
By default, if unset, ... have a default policy ...
sounds strange. How about just dropping the first 4 words here:
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:
What happens if protocol.allow is set to true?
+ 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.
+
+--
+
+protocol.<name>.allow::
+ Set a policy to be used by protocol <name> with clone/fetch/pull commands.
How does this interact with protocol.allow?
When protocol.allow is set, this overrides the specific protocol.
If protocol is not set, it overrides the specific protocol as well(?)
quoted hunk
+
pull.ff::
By default, Git does not create an extra merge commit when merging
a commit that is a descendant of the current commit. Instead, the
@@ -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
This is not the right place to mention what is newer. ;)
However it is useful to know about the config interface, which is
* (supposedly) easier to use
* more fine grained
* taking less priority than this env var.
I could not spot a test for GIT_ALLOW_PROTOCOL overriding
any protocol*allow policy. Is that also worth testing? (for
backwards compatibility of tools that make use of GIT_ALLOW_PROTOCOL
but the user already setup a policy.
From: Jeff King <hidden> Date: 2016-11-04 23:06:21
On Fri, Nov 04, 2016 at 01:55:33PM -0700, Brandon Williams wrote:
Previously the `GIT_ALLOW_PROTOCOL` environment variable was used to
specify a whitelist of protocols to be used in clone/fetch/pull
commands. This patch introduces new configuration options for more
fine-grained control for allowing/disallowing protocols. This also has
the added benefit of allowing easier construction of a protocol
whitelist on systems where setting an environment variable is
non-trivial.
Good rationale.
Now users can specify a policy to be used for each type of protocol via
the 'protocol.<name>.allow' config option. A default policy for all
unknown protocols can be set with the 'protocol.allow' config option.
I think "unconfigured" is a better word here than "unknown", as it would
apply to known protocols like "https", etc.
That made me wonder if "unknown" would be a better behavior, but I'm
pretty sure it is not. It is harder to explain, and I think would be
less convenient in practice. I.e., you really do want:
git config protocol.allow never
git config protocol.https.allow always
to allow nothing but https.
If no user configured default is made git, by default, will allow
known-safe protocols (http, https, git, ssh, file), disallow
known-dangerous protocols (ext), and have a default poliy of `user` for
all other protocols.
I think this is a good way of thinking about it. The order of
enforcement becomes:
- GIT_ALLOW_PROTOCOL; environment variables always take precedence
over config, so this makes sense. And it also is nice to put the
blunt hammer at the front for backwards-compatibility.
- protocol-specific config
- protocol-generic config
- built-in defaults (known-safe, known-scary, unknown)
which seems right.
Also, s/poliy/policy/.
The supported policies are `always`, `never`, and `user`. The `user`
policy can be used to configure a protocol to be usable when explicitly
used by a user, while disallowing it for commands which run
clone/fetch/pull commands without direct user intervention (e.g.
recursive initialization of submodules). Commands which can potentially
clone/fetch/pull from untrusted repositories without user intervention
can export `GIT_PROTOCOL_FROM_USER` with a value of '0' to prevent
protocols configured to the `user` policy from being used.
Makes sense. I know "user" came from me. I don't know if there is a
better word to describe it. I originally called it "cmdline", but that
seemed too obscure (especially when a tool external to git sets it).
Something like "trusted" might make sense (we allow it only in a
more-trusted setting), but it's kind of vague. And it also doesn't leave
room for there to be more types of trust in the future. So "user" is
probably reasonable (or perhaps "user-only" or similar).
@@ -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?
+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.
@@ -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`.
+`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.
@@ -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=$1proto=$2url=$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?
+# 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_whitelist "$@"
+
+ test_config "$@"
+}
This makes sense. It's probably more testing than we actually need. We
could just check the config version per-protocol, and then confirm that
GIT_ALLOW_PROTOCOL behaves as I described above for at least one
protocol. The per-protocol code paths are really just making sure that
the protocol is correctly named for each code path.
That being said, simple and stupid test setup is nice as long as it does
not take too long to run.
These remote-ext fixups might be worth a note in the commit message, or
a comment here explaining what is going on.
+static enum protocol_allow_config get_protocol_config(const char *type)
+{
+ char *key = xstrfmt("protocol.%s.allow", type);
+ char *value;
+
+ if (!git_config_get_string(key, &value)) {
+ enum protocol_allow_config ret =
+ parse_protocol_config(key, value);
+ free(key);
+ free(value);
+ return ret;
+ }
+ free(key);
+
+ /* if defined, use user default for unknown protocols */
+ if (!git_config_get_string("protocol.allow", &value)) {
+ enum protocol_allow_config ret =
+ parse_protocol_config("protocol.allow", value);
+ free(value);
+ return ret;
+ }
+
+ /* known safe */
[...]
It's probably worth a comment at this point in the function to follow-up
on your "if defined" comment above. So the end result reads something
like:
/* first check the per-protocol config */
...
/* now fallback to the generic config */
...
/* and then fallback to our built-in defaults */
-Peff
From: Jeff King <hidden> Date: 2016-11-04 23:09:09
On Fri, Nov 04, 2016 at 02:35:57PM -0700, Stefan Beller wrote:
On Fri, Nov 4, 2016 at 1:58 PM, Brandon Williams [off-list ref] wrote:
quoted
On 11/04, Brandon Williams wrote:
quoted
Signed-off-by: Brandon Williams <redacted>
Is there an acceptable way to give credit to Jeff for helping with this patch?
What about:
Helped-by: Jeff King [off-list ref]
That, or often I would write:
Based on a patch by Jeff King [off-list ref]
in the commit message. Basically anything is OK _except_ forging
signed-off-by, because it has a very specific meaning. So let me also
say that I am happy to give my:
Signed-off-by: Jeff King [off-list ref]
to the original (which you should add in, to make clear that the
copyright issues are OK).
In some cases it makes sense to just roll somebody's patch into your
series, and then build on top. I'm fine with it all going into a single
patch here.
-Peff
From: Brandon Williams <hidden> Date: 2016-11-05 00:18:20
On 11/04, Jeff King wrote:
On Fri, Nov 04, 2016 at 02:35:57PM -0700, Stefan Beller wrote:
quoted
On Fri, Nov 4, 2016 at 1:58 PM, Brandon Williams [off-list ref] wrote:
quoted
On 11/04, Brandon Williams wrote:
quoted
Signed-off-by: Brandon Williams <redacted>
Is there an acceptable way to give credit to Jeff for helping with this patch?
What about:
Helped-by: Jeff King [off-list ref]
That, or often I would write:
Based on a patch by Jeff King [off-list ref]
in the commit message. Basically anything is OK _except_ forging
signed-off-by, because it has a very specific meaning. So let me also
say that I am happy to give my:
Signed-off-by: Jeff King [off-list ref]
to the original (which you should add in, to make clear that the
copyright issues are OK).
In some cases it makes sense to just roll somebody's patch into your
series, and then build on top. I'm fine with it all going into a single
patch here.
-Peff
Oh if it would be more clear I can easily break it up into two patches.
--
Brandon Williams
From: Brandon Williams <hidden> Date: 2016-11-07 18:48:25
On 11/04, Stefan Beller wrote:
By default, if unset, ... have a default policy ...
sounds strange. How about just dropping the first 4 words here:
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:
What happens if protocol.allow is set to true?
That wouldn't be allowed, its an unknown value as the only permitted
values are always, never, and user.
quoted
+ 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.
+
+--
+
+protocol.<name>.allow::
+ Set a policy to be used by protocol <name> with clone/fetch/pull commands.
How does this interact with protocol.allow?
When protocol.allow is set, this overrides the specific protocol.
If protocol is not set, it overrides the specific protocol as well(?)
protocol.allow is a default for protocols which don't have a specific
protocol.<name>.allow entry
I could not spot a test for GIT_ALLOW_PROTOCOL overriding
any protocol*allow policy. Is that also worth testing? (for
backwards compatibility of tools that make use of GIT_ALLOW_PROTOCOL
but the user already setup a policy.
I can add in one quick test for that.
--
Brandon Williams
@@ -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.
@@ -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.
@@ -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=$1proto=$2url=$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
From: Brandon Williams <hidden> Date: 2016-11-07 19:42:53
Previously the `GIT_ALLOW_PROTOCOL` environment variable was used to
specify a whitelist of protocols to be used in clone/fetch/push
commands. This patch introduces new configuration options for more
fine-grained control for allowing/disallowing protocols. This also has
the added benefit of allowing easier construction of a protocol
whitelist on systems where setting an environment variable is
non-trivial.
Now users can specify a policy to be used for each type of protocol via
the 'protocol.<name>.allow' config option. A default policy for all
unconfigured protocols can be set with the 'protocol.allow' config
option. If no user configured default is made git, by default, will
allow known-safe protocols (http, https, git, ssh, file), disallow
known-dangerous protocols (ext), and have a default policy of `user` for
all other protocols.
The supported policies are `always`, `never`, and `user`. The `user`
policy can be used to configure a protocol to be usable when explicitly
used by a user, while disallowing it for commands which run
clone/fetch/push commands without direct user intervention (e.g.
recursive initialization of submodules). Commands which can potentially
clone/fetch/push from untrusted repositories without user intervention
can export `GIT_PROTOCOL_FROM_USER` with a value of '0' to prevent
protocols configured to the `user` policy from being used.
Fix remote-ext tests to use the new config to allow the ext
protocol to be tested.
Based on a patch by Jeff King [off-list ref]
Signed-off-by: Brandon Williams <redacted>
---
Documentation/config.txt | 25 ++++++++
Documentation/git.txt | 21 ++++---
git-submodule.sh | 12 ++--
t/lib-proto-disable.sh | 129 +++++++++++++++++++++++++++++++++++++--
t/t5509-fetch-push-namespaces.sh | 1 +
t/t5802-connect-helper.sh | 1 +
transport.c | 75 ++++++++++++++++++++++-
7 files changed, 242 insertions(+), 22 deletions(-)
@@ -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 directly usable by the user but don't want it used by commands which+ execute clone/fetch/push commands without user input, e.g. recursive+ submodule initialization.++--++protocol.<name>.allow::+ Set a policy to be used by protocol <name> with clone/fetch/push commands.+ pull.ff:: By default, Git does not create an extra merge commit when merging a commit that is a descendant of the current commit. Instead, the
@@ -1150,13 +1150,14 @@ 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 preferred 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 to a colon-separated+ list of protocols, behave as if `protocol.allow` is set to `never`, and+ each of the listed protocols has `protocol.<name>.allow` set to+ `always`. In other words, any protocol not mentioned will be+ disallowed (i.e., this is a whitelist, not a blacklist). The protocol+ names currently used by git are: - `file`: any local file-based path (including `file://` URLs, or local paths)
@@ -1174,6 +1175,12 @@ of clones and fetches. - any external helpers are named by their protocol (e.g., use `hg` to allow the `git-remote-hg` helper)+`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 or for programs+ which feed potentially-untrusted URLS to git commands. See+ linkgit:git-config[1] for more details. Discussion[[Discussion]] ------------------------
@@ -21,14 +21,10 @@ require_work_treewt_prefix=$(gitrev-parse--show-prefix) cd_to_toplevel-# Restrict ourselves to a vanilla subset of protocols; the URLs-# we get are under control of a remote repository, and we do not-# want them kicking off arbitrary git-remote-* programs.-#-# 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}-exportGIT_ALLOW_PROTOCOL+# Tell the rest of git that any URLs we get don't come+# directly from the user, so it can apply policy as appropriate.+GIT_PROTOCOL_FROM_USER=0+exportGIT_PROTOCOL_FROM_USERcommand=branch=
@@ -1,10 +1,7 @@# 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=$1proto=$2url=$3
@@ -62,6 +59,128 @@ test_proto () {test_must_failgitclone--bare"$url"tmp.git)'++test_expect_success"clone $desc (env var has precedence)"'+rm-rftmp.git&&+(+GIT_ALLOW_PROTOCOL=none&&+exportGIT_ALLOW_PROTOCOL&&+test_must_failgit-cprotocol.$proto.allow=alwaysclone--bare"$url"tmp.git+)+'+}++test_config(){+desc=$1+proto=$2+url=$3++# Test clone/fetch/push with protocol.<type>.allow config+test_expect_success"clone $desc (enabled with config)"'+rm-rftmp.git&&+git-cprotocol.$proto.allow=alwaysclone--bare"$url"tmp.git+'++test_expect_success"fetch $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=alwaysfetch+'++test_expect_success"push $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=alwayspushoriginHEAD:pushed+'++test_expect_success"push $desc (disabled)"'+test_must_failgit-Ctmp.git-cprotocol.$proto.allow=neverpushoriginHEAD:pushed+'++test_expect_success"fetch $desc (disabled)"'+test_must_failgit-Ctmp.git-cprotocol.$proto.allow=neverfetch+'++test_expect_success"clone $desc (disabled)"'+rm-rftmp.git&&+test_must_failgit-cprotocol.$proto.allow=neverclone--bare"$url"tmp.git+'++# Test clone/fetch/push with protocol.user.allow and its env var+test_expect_success"clone $desc (enabled)"'+rm-rftmp.git&&+git-cprotocol.$proto.allow=userclone--bare"$url"tmp.git+'++test_expect_success"fetch $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=userfetch+'++test_expect_success"push $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=userpushoriginHEAD:pushed+'++test_expect_success"push $desc (disabled)"'+(+cdtmp.git&&+GIT_PROTOCOL_FROM_USER=0&&+exportGIT_PROTOCOL_FROM_USER&&+test_must_failgit-cprotocol.$proto.allow=userpushoriginHEAD:pushed+)+'++test_expect_success"fetch $desc (disabled)"'+(+cdtmp.git&&+GIT_PROTOCOL_FROM_USER=0&&+exportGIT_PROTOCOL_FROM_USER&&+test_must_failgit-cprotocol.$proto.allow=userfetch+)+'++test_expect_success"clone $desc (disabled)"'+rm-rftmp.git&&+(+GIT_PROTOCOL_FROM_USER=0&&+exportGIT_PROTOCOL_FROM_USER&&+test_must_failgit-cprotocol.$proto.allow=userclone--bare"$url"tmp.git+)+'++# Test clone/fetch/push with protocol.allow user defined default+test_expect_success"clone $desc (enabled)"'+rm-rftmp.git&&+gitconfig--globalprotocol.allowalways&&+gitclone--bare"$url"tmp.git+'++test_expect_success"fetch $desc (enabled)"'+git-Ctmp.gitfetch+'++test_expect_success"push $desc (enabled)"'+git-Ctmp.gitpushoriginHEAD:pushed+'++test_expect_success"push $desc (disabled)"'+gitconfig--globalprotocol.allownever&&+test_must_failgit-Ctmp.gitpushoriginHEAD:pushed+'++test_expect_success"fetch $desc (disabled)"'+test_must_failgit-Ctmp.gitfetch+'++test_expect_success"clone $desc (disabled)"'+rm-rftmp.git&&+test_must_failgitclone--bare"$url"tmp.git+'+}++# 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_whitelist"$@"++test_config"$@"}# set up an ssh wrapper that will access $host/$repo in the
@@ -664,10 +664,81 @@ static const struct string_list *protocol_whitelist(void)returnenabled?&allowed:NULL;}+enumprotocol_allow_config{+PROTOCOL_ALLOW_NEVER=0,+PROTOCOL_ALLOW_USER_ONLY,+PROTOCOL_ALLOW_ALWAYS+};++staticenumprotocol_allow_configparse_protocol_config(constchar*key,+constchar*value)+{+if(!strcasecmp(value,"always"))+returnPROTOCOL_ALLOW_ALWAYS;+elseif(!strcasecmp(value,"never"))+returnPROTOCOL_ALLOW_NEVER;+elseif(!strcasecmp(value,"user"))+returnPROTOCOL_ALLOW_USER_ONLY;++die("unknown value for config '%s': %s",key,value);+}++staticenumprotocol_allow_configget_protocol_config(constchar*type)+{+char*key=xstrfmt("protocol.%s.allow",type);+char*value;++/* first check the per-protocol config */+if(!git_config_get_string(key,&value)){+enumprotocol_allow_configret=+parse_protocol_config(key,value);+free(key);+free(value);+returnret;+}+free(key);++/* if defined, fallback to user-defined default for unknown protocols */+if(!git_config_get_string("protocol.allow",&value)){+enumprotocol_allow_configret=+parse_protocol_config("protocol.allow",value);+free(value);+returnret;+}++/* fallback to built-in defaults */+/* known safe */+if(!strcmp(type,"http")||+!strcmp(type,"https")||+!strcmp(type,"git")||+!strcmp(type,"ssh")||+!strcmp(type,"file"))+returnPROTOCOL_ALLOW_ALWAYS;++/* known scary; err on the side of caution */+if(!strcmp(type,"ext"))+returnPROTOCOL_ALLOW_NEVER;++/* unknown; by default let them be used only directly by the user */+returnPROTOCOL_ALLOW_USER_ONLY;+}+intis_transport_allowed(constchar*type){-conststructstring_list*allowed=protocol_whitelist();-return!allowed||string_list_has_string(allowed,type);+conststructstring_list*whitelist=protocol_whitelist();+if(whitelist)+returnstring_list_has_string(whitelist,type);++switch(get_protocol_config(type)){+casePROTOCOL_ALLOW_ALWAYS:+return1;+casePROTOCOL_ALLOW_NEVER:+return0;+casePROTOCOL_ALLOW_USER_ONLY:+returngit_env_bool("GIT_PROTOCOL_FROM_USER",1);+}++die("BUG: invalid protocol_allow_config type");}voidtransport_check_allowed(constchar*type)
From: Brandon Williams <hidden> Date: 2016-11-07 20:04:18
Small fix to use '$desc' instead of '$1' in lib-proto-disable.sh.
Signed-off-by: Brandon Williams <redacted>
---
t/lib-proto-disable.sh | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Jeff King <hidden> Date: 2016-11-07 20:26:46
On Mon, Nov 07, 2016 at 11:35:22AM -0800, Brandon Williams wrote:
Small fix to use '$desc' instead of '$1' in lib-proto-disable.sh.
Even for a trivial fixup like this, I think it's good to say why.
Because what seems trivial and obvious to you while working on the patch
may not be so to a reviewer, or somebody reading it 6 months later.
Just something simple like:
The test_proto function assigns the positional parameters to named
variables, but then still refers to "$desc" as "$1". Using $desc is
more readable and less error-prone.
-Peff
From: Jeff King <hidden> Date: 2016-11-07 20:44:39
On Mon, Nov 07, 2016 at 11:35:23AM -0800, Brandon Williams wrote:
Previously the `GIT_ALLOW_PROTOCOL` environment variable was used to
specify a whitelist of protocols to be used in clone/fetch/push
commands. This patch introduces new configuration options for more
fine-grained control for allowing/disallowing protocols. This also has
the added benefit of allowing easier construction of a protocol
whitelist on systems where setting an environment variable is
non-trivial.
Now users can specify a policy to be used for each type of protocol via
the 'protocol.<name>.allow' config option. A default policy for all
unconfigured protocols can be set with the 'protocol.allow' config
option. If no user configured default is made git, by default, will
allow known-safe protocols (http, https, git, ssh, file), disallow
A minor nit, but in "If no user configured default is made git, by
default, will..." the second "by default" is redundant. And possibly
misleading. This _is_ the default case, there is no other way to change
it. :)
+protocol.<name>.allow::
+ Set a policy to be used by protocol <name> with clone/fetch/push commands.
`<name>` isn't defined here at all. I still think the list of protocols
should go here, but at the very least, you need to point the user to the
existing list in git(1).
`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 preferred way to configure allowed protocols is done through the
+ config interface, though this setting takes precedences. See
s/precedences/precedence/.
I actually wonder if we should even drop "the preferred way" here. I had
initially thought we would want it just for backwards-compatibility, but
I actually think it is useful in its own right as a shorthand for more
complicated config (and since we have to keep it around effectively
forever anyway, there's no real cost to continuing to call it a feature
versus a deprecated feature).
I'm including a squashable patch at the end of this email with suggested
wording (and which also moves the protocol list).
This test is a good addition in this round.
I suppose we could test also that GIT_ALLOW_PROTOCOL overrides
protocol.allow, but I'm not sure if there is a point. If git were a
black box, it's a thing I might check, but we know from the design that
this is an unlikely bug (and that the implementation is unlikely to
change in a way to cause it). So I could go either way.
[...]
The rest of it looks good to me.
Squashable documentation suggestions are below.
-Peff
---
@@ -2331,7 +2331,28 @@ protocol.allow:: -- protocol.<name>.allow::- Set a policy to be used by protocol <name> with clone/fetch/push commands.+ Set a policy to be used by protocol `<name>` with clone/fetch/push+ commands. See `protocol.allow` above for the available policies.+++The protocol names currently used by git are:+++--+ - `file`: any local file-based path (including `file://` URLs,+ or local paths)++ - `git`: the anonymous git protocol over a direct TCP+ connection (or proxy, if configured)++ - `ssh`: git over ssh (including `host:path` syntax,+ `ssh://`, etc).++ - `http`: git over http, both "smart http" and "dumb http".+ Note that this does _not_ include `https`; if you want to configure+ both, you must do so individually.++ - any external helpers are named by their protocol (e.g., use+ `hg` to allow the `git-remote-hg` helper)+-- pull.ff:: By default, Git does not create an extra merge commit when merging
@@ -1150,30 +1150,13 @@ of clones and fetches. cloning a repository to make a backup). `GIT_ALLOW_PROTOCOL`::- The preferred 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 to a colon-separated- list of protocols, behave as if `protocol.allow` is set to `never`, and- each of the listed protocols has `protocol.<name>.allow` set to- `always`. In other words, any protocol not mentioned will be- disallowed (i.e., this is a whitelist, not a blacklist). The protocol- names currently used by git are:-- - `file`: any local file-based path (including `file://` URLs,- or local paths)-- - `git`: the anonymous git protocol over a direct TCP- connection (or proxy, if configured)-- - `ssh`: git over ssh (including `host:path` syntax,- `ssh://`, etc).-- - `http`: git over http, both "smart http" and "dumb http".- Note that this does _not_ include `https`; if you want both,- you should specify both as `http:https`.-- - any external helpers are named by their protocol (e.g., use- `hg` to allow the `git-remote-hg` helper)+ If set to a colon-separated list of protocols, behave as if+ `protocol.allow` is set to `never`, and each of the listed+ protocols has `protocol.<name>.allow` set to `always`+ (overriding any existing configuration). In other words, any+ protocol not mentioned will be disallowed (i.e., this is a+ whitelist, not a blacklist). See the description of+ `protocol.allow` in linkgit:git-config[1] for more details. `GIT_PROTOCOL_FROM_USER`:: Set to 0 to prevent protocols used by fetch/push/clone which are
From: Brandon Williams <hidden> Date: 2016-11-07 20:47:57
On 11/07, Jeff King wrote:
On Mon, Nov 07, 2016 at 11:35:22AM -0800, Brandon Williams wrote:
quoted
Small fix to use '$desc' instead of '$1' in lib-proto-disable.sh.
Even for a trivial fixup like this, I think it's good to say why.
Because what seems trivial and obvious to you while working on the patch
may not be so to a reviewer, or somebody reading it 6 months later.
Just something simple like:
The test_proto function assigns the positional parameters to named
variables, but then still refers to "$desc" as "$1". Using $desc is
more readable and less error-prone.
-Peff
Alright will do. Commit messages don't seem to be an area of strength
for me, but I'm working on it! :D
--
Brandon Williams
From: Jeff King <hidden> Date: 2016-11-07 20:48:45
On Mon, Nov 07, 2016 at 12:40:28PM -0800, Brandon Williams wrote:
On 11/07, Jeff King wrote:
quoted
On Mon, Nov 07, 2016 at 11:35:22AM -0800, Brandon Williams wrote:
quoted
Small fix to use '$desc' instead of '$1' in lib-proto-disable.sh.
Even for a trivial fixup like this, I think it's good to say why.
Because what seems trivial and obvious to you while working on the patch
may not be so to a reviewer, or somebody reading it 6 months later.
Just something simple like:
The test_proto function assigns the positional parameters to named
variables, but then still refers to "$desc" as "$1". Using $desc is
more readable and less error-prone.
-Peff
Alright will do. Commit messages don't seem to be an area of strength
for me, but I'm working on it! :D
It's possible that I'm overly picky about my commit messages, but that
does not stop me from trying to train an army of picky-commit-message
clones. :)
-Peff
This test is a good addition in this round.
I suppose we could test also that GIT_ALLOW_PROTOCOL overrides
protocol.allow, but I'm not sure if there is a point. If git were a
black box, it's a thing I might check, but we know from the design that
this is an unlikely bug (and that the implementation is unlikely to
change in a way to cause it). So I could go either way.
I'll add in another test for that, no reason not to test it.
From: Brandon Williams <hidden> Date: 2016-11-07 21:51:23
Previously the `GIT_ALLOW_PROTOCOL` environment variable was used to
specify a whitelist of protocols to be used in clone/fetch/push
commands. This patch introduces new configuration options for more
fine-grained control for allowing/disallowing protocols. This also has
the added benefit of allowing easier construction of a protocol
whitelist on systems where setting an environment variable is
non-trivial.
Now users can specify a policy to be used for each type of protocol via
the 'protocol.<name>.allow' config option. A default policy for all
unconfigured protocols can be set with the 'protocol.allow' config
option. If no user configured default is made git will allow known-safe
protocols (http, https, git, ssh, file), disallow known-dangerous
protocols (ext), and have a default policy of `user` for all other
protocols.
The supported policies are `always`, `never`, and `user`. The `user`
policy can be used to configure a protocol to be usable when explicitly
used by a user, while disallowing it for commands which run
clone/fetch/push commands without direct user intervention (e.g.
recursive initialization of submodules). Commands which can potentially
clone/fetch/push from untrusted repositories without user intervention
can export `GIT_PROTOCOL_FROM_USER` with a value of '0' to prevent
protocols configured to the `user` policy from being used.
Fix remote-ext tests to use the new config to allow the ext
protocol to be tested.
Based on a patch by Jeff King [off-list ref]
Signed-off-by: Brandon Williams <redacted>
---
Documentation/config.txt | 46 ++++++++++++++
Documentation/git.txt | 38 +++++-------
git-submodule.sh | 12 ++--
t/lib-proto-disable.sh | 130 +++++++++++++++++++++++++++++++++++++--
t/t5509-fetch-push-namespaces.sh | 1 +
t/t5802-connect-helper.sh | 1 +
transport.c | 75 +++++++++++++++++++++-
7 files changed, 264 insertions(+), 39 deletions(-)
@@ -2308,6 +2308,52 @@ 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 directly usable by the user but don't want it used by commands which+ execute clone/fetch/push commands without user input, e.g. recursive+ submodule initialization.++--++protocol.<name>.allow::+ Set a policy to be used by protocol `<name>` with clone/fetch/push+ commands. See `protocol.allow` above for the available policies.+++The protocol names currently used by git are:+++--+ - `file`: any local file-based path (including `file://` URLs,+ or local paths)++ - `git`: the anonymous git protocol over a direct TCP+ connection (or proxy, if configured)++ - `ssh`: git over ssh (including `host:path` syntax,+ `ssh://`, etc).++ - `http`: git over http, both "smart http" and "dumb http".+ Note that this does _not_ include `https`; if you want to configure+ both, you must do so individually.++ - any external helpers are named by their protocol (e.g., use+ `hg` to allow the `git-remote-hg` helper)+--+ pull.ff:: By default, Git does not create an extra merge commit when merging a commit that is a descendant of the current commit. Instead, the
@@ -1150,30 +1150,20 @@ 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:-- - `file`: any local file-based path (including `file://` URLs,- or local paths)-- - `git`: the anonymous git protocol over a direct TCP- connection (or proxy, if configured)-- - `ssh`: git over ssh (including `host:path` syntax,- `ssh://`, etc).-- - `http`: git over http, both "smart http" and "dumb http".- Note that this does _not_ include `https`; if you want both,- you should specify both as `http:https`.-- - any external helpers are named by their protocol (e.g., use- `hg` to allow the `git-remote-hg` helper)-+ If set to a colon-separated list of protocols, behave as if+ `protocol.allow` is set to `never`, and each of the listed+ protocols has `protocol.<name>.allow` set to `always`+ (overriding any existing configuration). In other words, any+ protocol not mentioned will be disallowed (i.e., this is a+ whitelist, not a blacklist). See the description of+ `protocol.allow` in linkgit:git-config[1] for more details.++`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 or for programs+ which feed potentially-untrusted URLS to git commands. See+ linkgit:git-config[1] for more details. Discussion[[Discussion]] ------------------------
@@ -21,14 +21,10 @@ require_work_treewt_prefix=$(gitrev-parse--show-prefix) cd_to_toplevel-# Restrict ourselves to a vanilla subset of protocols; the URLs-# we get are under control of a remote repository, and we do not-# want them kicking off arbitrary git-remote-* programs.-#-# 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}-exportGIT_ALLOW_PROTOCOL+# Tell the rest of git that any URLs we get don't come+# directly from the user, so it can apply policy as appropriate.+GIT_PROTOCOL_FROM_USER=0+exportGIT_PROTOCOL_FROM_USERcommand=branch=
@@ -1,10 +1,7 @@# 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=$1proto=$2url=$3
@@ -62,6 +59,129 @@ test_proto () {test_must_failgitclone--bare"$url"tmp.git)'++test_expect_success"clone $desc (env var has precedence)"'+rm-rftmp.git&&+(+GIT_ALLOW_PROTOCOL=none&&+exportGIT_ALLOW_PROTOCOL&&+test_must_failgit-cprotocol.allow=alwaysclone--bare"$url"tmp.git&&+test_must_failgit-cprotocol.$proto.allow=alwaysclone--bare"$url"tmp.git+)+'+}++test_config(){+desc=$1+proto=$2+url=$3++# Test clone/fetch/push with protocol.<type>.allow config+test_expect_success"clone $desc (enabled with config)"'+rm-rftmp.git&&+git-cprotocol.$proto.allow=alwaysclone--bare"$url"tmp.git+'++test_expect_success"fetch $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=alwaysfetch+'++test_expect_success"push $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=alwayspushoriginHEAD:pushed+'++test_expect_success"push $desc (disabled)"'+test_must_failgit-Ctmp.git-cprotocol.$proto.allow=neverpushoriginHEAD:pushed+'++test_expect_success"fetch $desc (disabled)"'+test_must_failgit-Ctmp.git-cprotocol.$proto.allow=neverfetch+'++test_expect_success"clone $desc (disabled)"'+rm-rftmp.git&&+test_must_failgit-cprotocol.$proto.allow=neverclone--bare"$url"tmp.git+'++# Test clone/fetch/push with protocol.user.allow and its env var+test_expect_success"clone $desc (enabled)"'+rm-rftmp.git&&+git-cprotocol.$proto.allow=userclone--bare"$url"tmp.git+'++test_expect_success"fetch $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=userfetch+'++test_expect_success"push $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=userpushoriginHEAD:pushed+'++test_expect_success"push $desc (disabled)"'+(+cdtmp.git&&+GIT_PROTOCOL_FROM_USER=0&&+exportGIT_PROTOCOL_FROM_USER&&+test_must_failgit-cprotocol.$proto.allow=userpushoriginHEAD:pushed+)+'++test_expect_success"fetch $desc (disabled)"'+(+cdtmp.git&&+GIT_PROTOCOL_FROM_USER=0&&+exportGIT_PROTOCOL_FROM_USER&&+test_must_failgit-cprotocol.$proto.allow=userfetch+)+'++test_expect_success"clone $desc (disabled)"'+rm-rftmp.git&&+(+GIT_PROTOCOL_FROM_USER=0&&+exportGIT_PROTOCOL_FROM_USER&&+test_must_failgit-cprotocol.$proto.allow=userclone--bare"$url"tmp.git+)+'++# Test clone/fetch/push with protocol.allow user defined default+test_expect_success"clone $desc (enabled)"'+rm-rftmp.git&&+gitconfig--globalprotocol.allowalways&&+gitclone--bare"$url"tmp.git+'++test_expect_success"fetch $desc (enabled)"'+git-Ctmp.gitfetch+'++test_expect_success"push $desc (enabled)"'+git-Ctmp.gitpushoriginHEAD:pushed+'++test_expect_success"push $desc (disabled)"'+gitconfig--globalprotocol.allownever&&+test_must_failgit-Ctmp.gitpushoriginHEAD:pushed+'++test_expect_success"fetch $desc (disabled)"'+test_must_failgit-Ctmp.gitfetch+'++test_expect_success"clone $desc (disabled)"'+rm-rftmp.git&&+test_must_failgitclone--bare"$url"tmp.git+'+}++# 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_whitelist"$@"++test_config"$@"}# set up an ssh wrapper that will access $host/$repo in the
@@ -664,10 +664,81 @@ static const struct string_list *protocol_whitelist(void)returnenabled?&allowed:NULL;}+enumprotocol_allow_config{+PROTOCOL_ALLOW_NEVER=0,+PROTOCOL_ALLOW_USER_ONLY,+PROTOCOL_ALLOW_ALWAYS+};++staticenumprotocol_allow_configparse_protocol_config(constchar*key,+constchar*value)+{+if(!strcasecmp(value,"always"))+returnPROTOCOL_ALLOW_ALWAYS;+elseif(!strcasecmp(value,"never"))+returnPROTOCOL_ALLOW_NEVER;+elseif(!strcasecmp(value,"user"))+returnPROTOCOL_ALLOW_USER_ONLY;++die("unknown value for config '%s': %s",key,value);+}++staticenumprotocol_allow_configget_protocol_config(constchar*type)+{+char*key=xstrfmt("protocol.%s.allow",type);+char*value;++/* first check the per-protocol config */+if(!git_config_get_string(key,&value)){+enumprotocol_allow_configret=+parse_protocol_config(key,value);+free(key);+free(value);+returnret;+}+free(key);++/* if defined, fallback to user-defined default for unknown protocols */+if(!git_config_get_string("protocol.allow",&value)){+enumprotocol_allow_configret=+parse_protocol_config("protocol.allow",value);+free(value);+returnret;+}++/* fallback to built-in defaults */+/* known safe */+if(!strcmp(type,"http")||+!strcmp(type,"https")||+!strcmp(type,"git")||+!strcmp(type,"ssh")||+!strcmp(type,"file"))+returnPROTOCOL_ALLOW_ALWAYS;++/* known scary; err on the side of caution */+if(!strcmp(type,"ext"))+returnPROTOCOL_ALLOW_NEVER;++/* unknown; by default let them be used only directly by the user */+returnPROTOCOL_ALLOW_USER_ONLY;+}+intis_transport_allowed(constchar*type){-conststructstring_list*allowed=protocol_whitelist();-return!allowed||string_list_has_string(allowed,type);+conststructstring_list*whitelist=protocol_whitelist();+if(whitelist)+returnstring_list_has_string(whitelist,type);++switch(get_protocol_config(type)){+casePROTOCOL_ALLOW_ALWAYS:+return1;+casePROTOCOL_ALLOW_NEVER:+return0;+casePROTOCOL_ALLOW_USER_ONLY:+returngit_env_bool("GIT_PROTOCOL_FROM_USER",1);+}++die("BUG: invalid protocol_allow_config type");}voidtransport_check_allowed(constchar*type)
From: Brandon Williams <hidden> Date: 2016-11-08 02:55:31
The test_proto function assigns the positional parameters to named
variables, but then still refers to "$desc" as "$1". Using $desc is
more readable and less error-prone.
Signed-off-by: Brandon Williams <redacted>
---
t/lib-proto-disable.sh | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Jacob Keller <hidden> Date: 2016-11-08 03:33:10
On Mon, Nov 7, 2016 at 12:48 PM, Jeff King [off-list ref] wrote:
It's possible that I'm overly picky about my commit messages, but that
does not stop me from trying to train an army of picky-commit-message
clones. :)
-Peff
From: Jeff King <hidden> Date: 2016-11-08 22:04:32
On Mon, Nov 07, 2016 at 01:51:02PM -0800, Brandon Williams wrote:
Previously the `GIT_ALLOW_PROTOCOL` environment variable was used to
specify a whitelist of protocols to be used in clone/fetch/push
commands. This patch introduces new configuration options for more
fine-grained control for allowing/disallowing protocols. This also has
the added benefit of allowing easier construction of a protocol
whitelist on systems where setting an environment variable is
non-trivial.
This v5 looks good to me (both patches 1 and 2).
-Peff
From: Brandon Williams <hidden> Date: 2016-11-08 22:05:50
On 11/08, Jeff King wrote:
On Mon, Nov 07, 2016 at 01:51:02PM -0800, Brandon Williams wrote:
quoted
Previously the `GIT_ALLOW_PROTOCOL` environment variable was used to
specify a whitelist of protocols to be used in clone/fetch/push
commands. This patch introduces new configuration options for more
fine-grained control for allowing/disallowing protocols. This also has
the added benefit of allowing easier construction of a protocol
whitelist on systems where setting an environment variable is
non-trivial.
This v5 looks good to me (both patches 1 and 2).
Thanks again for the help with this series!
--
Brandon Williams
From: Junio C Hamano <hidden> Date: 2016-11-09 05:56:22
Jacob Keller [off-list ref] writes:
On Mon, Nov 7, 2016 at 12:48 PM, Jeff King [off-list ref] wrote:
quoted
It's possible that I'm overly picky about my commit messages, but that
does not stop me from trying to train an army of picky-commit-message
clones. :)
-Peff
You're not the only one ;)
Somebody seems to have trained y'all very well ;-)
From: Brandon Williams <hidden> Date: 2016-12-01 19:45:53
v6 introduces 2 additional patches which address problems with protocols that
libcurl is allowed to use for redirection.
Brandon Williams (4):
lib-proto-disable: variable name fix
transport: add protocol policy config option
http: always warn if libcurl version is too old
transport: check if protocol can be used on a redirect
Documentation/config.txt | 46 +++++++++++++
Documentation/git.txt | 38 ++++-------
git-submodule.sh | 12 ++--
http.c | 13 ++--
t/lib-proto-disable.sh | 142 ++++++++++++++++++++++++++++++++++++---
t/t5509-fetch-push-namespaces.sh | 1 +
t/t5802-connect-helper.sh | 1 +
transport.c | 82 +++++++++++++++++++---
transport.h | 13 ++--
9 files changed, 281 insertions(+), 67 deletions(-)
--
2.8.0.rc3.226.g39d4020
From: Brandon Williams <hidden> Date: 2016-12-01 19:46:01
Add a the 'redirect' parameter to 'is_transport_allowed' which allows
callers to query if a transport protocol can be used on a redirect.
Signed-off-by: Brandon Williams <redacted>
---
http.c | 8 ++++----
transport.c | 6 +++---
transport.h | 7 ++++---
3 files changed, 11 insertions(+), 10 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-01 19:46:04
Previously the `GIT_ALLOW_PROTOCOL` environment variable was used to
specify a whitelist of protocols to be used in clone/fetch/push
commands. This patch introduces new configuration options for more
fine-grained control for allowing/disallowing protocols. This also has
the added benefit of allowing easier construction of a protocol
whitelist on systems where setting an environment variable is
non-trivial.
Now users can specify a policy to be used for each type of protocol via
the 'protocol.<name>.allow' config option. A default policy for all
unconfigured protocols can be set with the 'protocol.allow' config
option. If no user configured default is made git will allow known-safe
protocols (http, https, git, ssh, file), disallow known-dangerous
protocols (ext), and have a default policy of `user` for all other
protocols.
The supported policies are `always`, `never`, and `user`. The `user`
policy can be used to configure a protocol to be usable when explicitly
used by a user, while disallowing it for commands which run
clone/fetch/push commands without direct user intervention (e.g.
recursive initialization of submodules). Commands which can potentially
clone/fetch/push from untrusted repositories without user intervention
can export `GIT_PROTOCOL_FROM_USER` with a value of '0' to prevent
protocols configured to the `user` policy from being used.
Fix remote-ext tests to use the new config to allow the ext
protocol to be tested.
Based on a patch by Jeff King [off-list ref]
Signed-off-by: Brandon Williams <redacted>
---
Documentation/config.txt | 46 ++++++++++++++
Documentation/git.txt | 38 +++++-------
git-submodule.sh | 12 ++--
t/lib-proto-disable.sh | 130 +++++++++++++++++++++++++++++++++++++--
t/t5509-fetch-push-namespaces.sh | 1 +
t/t5802-connect-helper.sh | 1 +
transport.c | 75 +++++++++++++++++++++-
7 files changed, 264 insertions(+), 39 deletions(-)
@@ -2308,6 +2308,52 @@ 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 directly usable by the user but don't want it used by commands which+ execute clone/fetch/push commands without user input, e.g. recursive+ submodule initialization.++--++protocol.<name>.allow::+ Set a policy to be used by protocol `<name>` with clone/fetch/push+ commands. See `protocol.allow` above for the available policies.+++The protocol names currently used by git are:+++--+ - `file`: any local file-based path (including `file://` URLs,+ or local paths)++ - `git`: the anonymous git protocol over a direct TCP+ connection (or proxy, if configured)++ - `ssh`: git over ssh (including `host:path` syntax,+ `ssh://`, etc).++ - `http`: git over http, both "smart http" and "dumb http".+ Note that this does _not_ include `https`; if you want to configure+ both, you must do so individually.++ - any external helpers are named by their protocol (e.g., use+ `hg` to allow the `git-remote-hg` helper)+--+ pull.ff:: By default, Git does not create an extra merge commit when merging a commit that is a descendant of the current commit. Instead, the
@@ -1150,30 +1150,20 @@ 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:-- - `file`: any local file-based path (including `file://` URLs,- or local paths)-- - `git`: the anonymous git protocol over a direct TCP- connection (or proxy, if configured)-- - `ssh`: git over ssh (including `host:path` syntax,- `ssh://`, etc).-- - `http`: git over http, both "smart http" and "dumb http".- Note that this does _not_ include `https`; if you want both,- you should specify both as `http:https`.-- - any external helpers are named by their protocol (e.g., use- `hg` to allow the `git-remote-hg` helper)-+ If set to a colon-separated list of protocols, behave as if+ `protocol.allow` is set to `never`, and each of the listed+ protocols has `protocol.<name>.allow` set to `always`+ (overriding any existing configuration). In other words, any+ protocol not mentioned will be disallowed (i.e., this is a+ whitelist, not a blacklist). See the description of+ `protocol.allow` in linkgit:git-config[1] for more details.++`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 or for programs+ which feed potentially-untrusted URLS to git commands. See+ linkgit:git-config[1] for more details. Discussion[[Discussion]] ------------------------
@@ -21,14 +21,10 @@ require_work_treewt_prefix=$(gitrev-parse--show-prefix) cd_to_toplevel-# Restrict ourselves to a vanilla subset of protocols; the URLs-# we get are under control of a remote repository, and we do not-# want them kicking off arbitrary git-remote-* programs.-#-# 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}-exportGIT_ALLOW_PROTOCOL+# Tell the rest of git that any URLs we get don't come+# directly from the user, so it can apply policy as appropriate.+GIT_PROTOCOL_FROM_USER=0+exportGIT_PROTOCOL_FROM_USERcommand=branch=
@@ -1,10 +1,7 @@# 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=$1proto=$2url=$3
@@ -62,6 +59,129 @@ test_proto () {test_must_failgitclone--bare"$url"tmp.git)'++test_expect_success"clone $desc (env var has precedence)"'+rm-rftmp.git&&+(+GIT_ALLOW_PROTOCOL=none&&+exportGIT_ALLOW_PROTOCOL&&+test_must_failgit-cprotocol.allow=alwaysclone--bare"$url"tmp.git&&+test_must_failgit-cprotocol.$proto.allow=alwaysclone--bare"$url"tmp.git+)+'+}++test_config(){+desc=$1+proto=$2+url=$3++# Test clone/fetch/push with protocol.<type>.allow config+test_expect_success"clone $desc (enabled with config)"'+rm-rftmp.git&&+git-cprotocol.$proto.allow=alwaysclone--bare"$url"tmp.git+'++test_expect_success"fetch $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=alwaysfetch+'++test_expect_success"push $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=alwayspushoriginHEAD:pushed+'++test_expect_success"push $desc (disabled)"'+test_must_failgit-Ctmp.git-cprotocol.$proto.allow=neverpushoriginHEAD:pushed+'++test_expect_success"fetch $desc (disabled)"'+test_must_failgit-Ctmp.git-cprotocol.$proto.allow=neverfetch+'++test_expect_success"clone $desc (disabled)"'+rm-rftmp.git&&+test_must_failgit-cprotocol.$proto.allow=neverclone--bare"$url"tmp.git+'++# Test clone/fetch/push with protocol.user.allow and its env var+test_expect_success"clone $desc (enabled)"'+rm-rftmp.git&&+git-cprotocol.$proto.allow=userclone--bare"$url"tmp.git+'++test_expect_success"fetch $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=userfetch+'++test_expect_success"push $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=userpushoriginHEAD:pushed+'++test_expect_success"push $desc (disabled)"'+(+cdtmp.git&&+GIT_PROTOCOL_FROM_USER=0&&+exportGIT_PROTOCOL_FROM_USER&&+test_must_failgit-cprotocol.$proto.allow=userpushoriginHEAD:pushed+)+'++test_expect_success"fetch $desc (disabled)"'+(+cdtmp.git&&+GIT_PROTOCOL_FROM_USER=0&&+exportGIT_PROTOCOL_FROM_USER&&+test_must_failgit-cprotocol.$proto.allow=userfetch+)+'++test_expect_success"clone $desc (disabled)"'+rm-rftmp.git&&+(+GIT_PROTOCOL_FROM_USER=0&&+exportGIT_PROTOCOL_FROM_USER&&+test_must_failgit-cprotocol.$proto.allow=userclone--bare"$url"tmp.git+)+'++# Test clone/fetch/push with protocol.allow user defined default+test_expect_success"clone $desc (enabled)"'+rm-rftmp.git&&+gitconfig--globalprotocol.allowalways&&+gitclone--bare"$url"tmp.git+'++test_expect_success"fetch $desc (enabled)"'+git-Ctmp.gitfetch+'++test_expect_success"push $desc (enabled)"'+git-Ctmp.gitpushoriginHEAD:pushed+'++test_expect_success"push $desc (disabled)"'+gitconfig--globalprotocol.allownever&&+test_must_failgit-Ctmp.gitpushoriginHEAD:pushed+'++test_expect_success"fetch $desc (disabled)"'+test_must_failgit-Ctmp.gitfetch+'++test_expect_success"clone $desc (disabled)"'+rm-rftmp.git&&+test_must_failgitclone--bare"$url"tmp.git+'+}++# 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_whitelist"$@"++test_config"$@"}# set up an ssh wrapper that will access $host/$repo in the
@@ -664,10 +664,81 @@ static const struct string_list *protocol_whitelist(void)returnenabled?&allowed:NULL;}+enumprotocol_allow_config{+PROTOCOL_ALLOW_NEVER=0,+PROTOCOL_ALLOW_USER_ONLY,+PROTOCOL_ALLOW_ALWAYS+};++staticenumprotocol_allow_configparse_protocol_config(constchar*key,+constchar*value)+{+if(!strcasecmp(value,"always"))+returnPROTOCOL_ALLOW_ALWAYS;+elseif(!strcasecmp(value,"never"))+returnPROTOCOL_ALLOW_NEVER;+elseif(!strcasecmp(value,"user"))+returnPROTOCOL_ALLOW_USER_ONLY;++die("unknown value for config '%s': %s",key,value);+}++staticenumprotocol_allow_configget_protocol_config(constchar*type)+{+char*key=xstrfmt("protocol.%s.allow",type);+char*value;++/* first check the per-protocol config */+if(!git_config_get_string(key,&value)){+enumprotocol_allow_configret=+parse_protocol_config(key,value);+free(key);+free(value);+returnret;+}+free(key);++/* if defined, fallback to user-defined default for unknown protocols */+if(!git_config_get_string("protocol.allow",&value)){+enumprotocol_allow_configret=+parse_protocol_config("protocol.allow",value);+free(value);+returnret;+}++/* fallback to built-in defaults */+/* known safe */+if(!strcmp(type,"http")||+!strcmp(type,"https")||+!strcmp(type,"git")||+!strcmp(type,"ssh")||+!strcmp(type,"file"))+returnPROTOCOL_ALLOW_ALWAYS;++/* known scary; err on the side of caution */+if(!strcmp(type,"ext"))+returnPROTOCOL_ALLOW_NEVER;++/* unknown; by default let them be used only directly by the user */+returnPROTOCOL_ALLOW_USER_ONLY;+}+intis_transport_allowed(constchar*type){-conststructstring_list*allowed=protocol_whitelist();-return!allowed||string_list_has_string(allowed,type);+conststructstring_list*whitelist=protocol_whitelist();+if(whitelist)+returnstring_list_has_string(whitelist,type);++switch(get_protocol_config(type)){+casePROTOCOL_ALLOW_ALWAYS:+return1;+casePROTOCOL_ALLOW_NEVER:+return0;+casePROTOCOL_ALLOW_USER_ONLY:+returngit_env_bool("GIT_PROTOCOL_FROM_USER",1);+}++die("BUG: invalid protocol_allow_config type");}voidtransport_check_allowed(constchar*type)
From: Brandon Williams <hidden> Date: 2016-12-01 19:46:05
The test_proto function assigns the positional parameters to named
variables, but then still refers to "$desc" as "$1". Using $desc is
more readable and less error-prone.
Signed-off-by: Brandon Williams <redacted>
---
t/lib-proto-disable.sh | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-01 19:46:06
Now that there are default "known-good" and "known-bad" protocols which
are allowed/disallowed by 'is_transport_allowed' we should always warn
the user that older versions of libcurl can't respect the allowed
protocols for redirects.
Signed-off-by: Brandon Williams <redacted>
---
http.c | 5 ++---
transport.c | 5 -----
transport.h | 6 ------
3 files changed, 2 insertions(+), 14 deletions(-)
@@ -735,9 +735,8 @@ static CURL *get_curl_handle(void)allowed_protocols|=CURLPROTO_FTPS;curl_easy_setopt(result,CURLOPT_REDIR_PROTOCOLS,allowed_protocols);#else-if(transport_restrict_protocols())-warning("protocol restrictions not applied to curl redirects because\n"-"your curl version is too old (>= 7.19.4)");+warning("protocol restrictions not applied to curl redirects because\n"+"your curl version is too old (>= 7.19.4)");#endifif(getenv("GIT_CURL_VERBOSE"))curl_easy_setopt(result,CURLOPT_VERBOSE,1L);
@@ -164,12 +164,6 @@ int is_transport_allowed(const char *type);*/voidtransport_check_allowed(constchar*type);-/*-*Returnstrueiftheuserhasattemptedtoturnonprotocol-*restrictionsatall.-*/-inttransport_restrict_protocols(void);-/* Transport options which apply to git:// and scp-style URLs *//* The program to use on the remote side to send a pack */
From: Brandon Williams <hidden> Date: 2016-12-01 19:48:40
Add a the 'redirect' parameter to 'is_transport_allowed' which allows
callers to query if a transport protocol can be used on a redirect.
Signed-off-by: Brandon Williams <redacted>
---
http.c | 8 ++++----
transport.c | 6 +++---
transport.h | 7 ++++---
3 files changed, 11 insertions(+), 10 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-01 19:49:35
On 12/01, Brandon Williams wrote:
Add a the 'redirect' parameter to 'is_transport_allowed' which allows
callers to query if a transport protocol can be used on a redirect.
Signed-off-by: Brandon Williams <redacted>
---
http.c | 8 ++++----
transport.c | 6 +++---
transport.h | 7 ++++---
3 files changed, 11 insertions(+), 10 deletions(-)
Accidentally sent out an old version of just this patch. Here is the
updated one.
--
Brandon Williams
From: Jeff King <hidden> Date: 2016-12-01 19:50:38
On Thu, Dec 01, 2016 at 11:44:07AM -0800, Brandon Williams wrote:
Add a the 'redirect' parameter to 'is_transport_allowed' which allows
callers to query if a transport protocol can be used on a redirect.
s/a the/a/
quoted hunk
-int is_transport_allowed(const char *type)
+int is_transport_allowed(const char *type, int redirect)
{
const struct string_list *whitelist = protocol_whitelist();
if (whitelist)
@@ -735,7 +735,7 @@ int is_transport_allowed(const char *type) case PROTOCOL_ALLOW_NEVER: return 0; case PROTOCOL_ALLOW_USER_ONLY:- return git_env_bool("GIT_PROTOCOL_FROM_USER", 1);+ return git_env_bool("GIT_PROTOCOL_FROM_USER", !redirect); }
This has the older logic still.
I'm not sure if we should call this "redirect" here. That's how it's
used by the curl code, but I think from the perspective of the transport
whitelist, it is really "are you overriding the from_user environment".
Calling it "from_user" may be confusing though, as the default value
would become "1", even though it means only "as far as I know this is
from the user, but maybe the environment says otherwise". So bizarrely,
I think calling it "not_from_user" is the clearest value.
-Peff
From: Brandon Williams <hidden> Date: 2016-12-01 20:26:36
Changed the last patch in the series to use the parameter 'from_user' instead
of 'redirect'. This allows us to use the same logic polarity and maintain use
of the same vocabulary.
Brandon Williams (4):
lib-proto-disable: variable name fix
transport: add protocol policy config option
http: always warn if libcurl version is too old
transport: add from_user parameter to is_transport_allowed
Documentation/config.txt | 46 +++++++++++++
Documentation/git.txt | 38 ++++-------
git-submodule.sh | 12 ++--
http.c | 13 ++--
t/lib-proto-disable.sh | 142 ++++++++++++++++++++++++++++++++++++---
t/t5509-fetch-push-namespaces.sh | 1 +
t/t5802-connect-helper.sh | 1 +
transport.c | 84 ++++++++++++++++++++---
transport.h | 19 +++---
9 files changed, 289 insertions(+), 67 deletions(-)
--
2.8.0.rc3.226.g39d4020
From: Brandon Williams <hidden> Date: 2016-12-01 20:26:39
Now that there are default "known-good" and "known-bad" protocols which
are allowed/disallowed by 'is_transport_allowed' we should always warn
the user that older versions of libcurl can't respect the allowed
protocols for redirects.
Signed-off-by: Brandon Williams <redacted>
---
http.c | 5 ++---
transport.c | 5 -----
transport.h | 6 ------
3 files changed, 2 insertions(+), 14 deletions(-)
@@ -735,9 +735,8 @@ static CURL *get_curl_handle(void)allowed_protocols|=CURLPROTO_FTPS;curl_easy_setopt(result,CURLOPT_REDIR_PROTOCOLS,allowed_protocols);#else-if(transport_restrict_protocols())-warning("protocol restrictions not applied to curl redirects because\n"-"your curl version is too old (>= 7.19.4)");+warning("protocol restrictions not applied to curl redirects because\n"+"your curl version is too old (>= 7.19.4)");#endifif(getenv("GIT_CURL_VERBOSE"))curl_easy_setopt(result,CURLOPT_VERBOSE,1L);
@@ -164,12 +164,6 @@ int is_transport_allowed(const char *type);*/voidtransport_check_allowed(constchar*type);-/*-*Returnstrueiftheuserhasattemptedtoturnonprotocol-*restrictionsatall.-*/-inttransport_restrict_protocols(void);-/* Transport options which apply to git:// and scp-style URLs *//* The program to use on the remote side to send a pack */
From: Brandon Williams <hidden> Date: 2016-12-01 20:26:40
The test_proto function assigns the positional parameters to named
variables, but then still refers to "$desc" as "$1". Using $desc is
more readable and less error-prone.
Signed-off-by: Brandon Williams <redacted>
---
t/lib-proto-disable.sh | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-01 20:26:44
Previously the `GIT_ALLOW_PROTOCOL` environment variable was used to
specify a whitelist of protocols to be used in clone/fetch/push
commands. This patch introduces new configuration options for more
fine-grained control for allowing/disallowing protocols. This also has
the added benefit of allowing easier construction of a protocol
whitelist on systems where setting an environment variable is
non-trivial.
Now users can specify a policy to be used for each type of protocol via
the 'protocol.<name>.allow' config option. A default policy for all
unconfigured protocols can be set with the 'protocol.allow' config
option. If no user configured default is made git will allow known-safe
protocols (http, https, git, ssh, file), disallow known-dangerous
protocols (ext), and have a default policy of `user` for all other
protocols.
The supported policies are `always`, `never`, and `user`. The `user`
policy can be used to configure a protocol to be usable when explicitly
used by a user, while disallowing it for commands which run
clone/fetch/push commands without direct user intervention (e.g.
recursive initialization of submodules). Commands which can potentially
clone/fetch/push from untrusted repositories without user intervention
can export `GIT_PROTOCOL_FROM_USER` with a value of '0' to prevent
protocols configured to the `user` policy from being used.
Fix remote-ext tests to use the new config to allow the ext
protocol to be tested.
Based on a patch by Jeff King [off-list ref]
Signed-off-by: Brandon Williams <redacted>
---
Documentation/config.txt | 46 ++++++++++++++
Documentation/git.txt | 38 +++++-------
git-submodule.sh | 12 ++--
t/lib-proto-disable.sh | 130 +++++++++++++++++++++++++++++++++++++--
t/t5509-fetch-push-namespaces.sh | 1 +
t/t5802-connect-helper.sh | 1 +
transport.c | 75 +++++++++++++++++++++-
7 files changed, 264 insertions(+), 39 deletions(-)
@@ -2308,6 +2308,52 @@ 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 directly usable by the user but don't want it used by commands which+ execute clone/fetch/push commands without user input, e.g. recursive+ submodule initialization.++--++protocol.<name>.allow::+ Set a policy to be used by protocol `<name>` with clone/fetch/push+ commands. See `protocol.allow` above for the available policies.+++The protocol names currently used by git are:+++--+ - `file`: any local file-based path (including `file://` URLs,+ or local paths)++ - `git`: the anonymous git protocol over a direct TCP+ connection (or proxy, if configured)++ - `ssh`: git over ssh (including `host:path` syntax,+ `ssh://`, etc).++ - `http`: git over http, both "smart http" and "dumb http".+ Note that this does _not_ include `https`; if you want to configure+ both, you must do so individually.++ - any external helpers are named by their protocol (e.g., use+ `hg` to allow the `git-remote-hg` helper)+--+ pull.ff:: By default, Git does not create an extra merge commit when merging a commit that is a descendant of the current commit. Instead, the
@@ -1150,30 +1150,20 @@ 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:-- - `file`: any local file-based path (including `file://` URLs,- or local paths)-- - `git`: the anonymous git protocol over a direct TCP- connection (or proxy, if configured)-- - `ssh`: git over ssh (including `host:path` syntax,- `ssh://`, etc).-- - `http`: git over http, both "smart http" and "dumb http".- Note that this does _not_ include `https`; if you want both,- you should specify both as `http:https`.-- - any external helpers are named by their protocol (e.g., use- `hg` to allow the `git-remote-hg` helper)-+ If set to a colon-separated list of protocols, behave as if+ `protocol.allow` is set to `never`, and each of the listed+ protocols has `protocol.<name>.allow` set to `always`+ (overriding any existing configuration). In other words, any+ protocol not mentioned will be disallowed (i.e., this is a+ whitelist, not a blacklist). See the description of+ `protocol.allow` in linkgit:git-config[1] for more details.++`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 or for programs+ which feed potentially-untrusted URLS to git commands. See+ linkgit:git-config[1] for more details. Discussion[[Discussion]] ------------------------
@@ -21,14 +21,10 @@ require_work_treewt_prefix=$(gitrev-parse--show-prefix) cd_to_toplevel-# Restrict ourselves to a vanilla subset of protocols; the URLs-# we get are under control of a remote repository, and we do not-# want them kicking off arbitrary git-remote-* programs.-#-# 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}-exportGIT_ALLOW_PROTOCOL+# Tell the rest of git that any URLs we get don't come+# directly from the user, so it can apply policy as appropriate.+GIT_PROTOCOL_FROM_USER=0+exportGIT_PROTOCOL_FROM_USERcommand=branch=
@@ -1,10 +1,7 @@# 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=$1proto=$2url=$3
@@ -62,6 +59,129 @@ test_proto () {test_must_failgitclone--bare"$url"tmp.git)'++test_expect_success"clone $desc (env var has precedence)"'+rm-rftmp.git&&+(+GIT_ALLOW_PROTOCOL=none&&+exportGIT_ALLOW_PROTOCOL&&+test_must_failgit-cprotocol.allow=alwaysclone--bare"$url"tmp.git&&+test_must_failgit-cprotocol.$proto.allow=alwaysclone--bare"$url"tmp.git+)+'+}++test_config(){+desc=$1+proto=$2+url=$3++# Test clone/fetch/push with protocol.<type>.allow config+test_expect_success"clone $desc (enabled with config)"'+rm-rftmp.git&&+git-cprotocol.$proto.allow=alwaysclone--bare"$url"tmp.git+'++test_expect_success"fetch $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=alwaysfetch+'++test_expect_success"push $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=alwayspushoriginHEAD:pushed+'++test_expect_success"push $desc (disabled)"'+test_must_failgit-Ctmp.git-cprotocol.$proto.allow=neverpushoriginHEAD:pushed+'++test_expect_success"fetch $desc (disabled)"'+test_must_failgit-Ctmp.git-cprotocol.$proto.allow=neverfetch+'++test_expect_success"clone $desc (disabled)"'+rm-rftmp.git&&+test_must_failgit-cprotocol.$proto.allow=neverclone--bare"$url"tmp.git+'++# Test clone/fetch/push with protocol.user.allow and its env var+test_expect_success"clone $desc (enabled)"'+rm-rftmp.git&&+git-cprotocol.$proto.allow=userclone--bare"$url"tmp.git+'++test_expect_success"fetch $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=userfetch+'++test_expect_success"push $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=userpushoriginHEAD:pushed+'++test_expect_success"push $desc (disabled)"'+(+cdtmp.git&&+GIT_PROTOCOL_FROM_USER=0&&+exportGIT_PROTOCOL_FROM_USER&&+test_must_failgit-cprotocol.$proto.allow=userpushoriginHEAD:pushed+)+'++test_expect_success"fetch $desc (disabled)"'+(+cdtmp.git&&+GIT_PROTOCOL_FROM_USER=0&&+exportGIT_PROTOCOL_FROM_USER&&+test_must_failgit-cprotocol.$proto.allow=userfetch+)+'++test_expect_success"clone $desc (disabled)"'+rm-rftmp.git&&+(+GIT_PROTOCOL_FROM_USER=0&&+exportGIT_PROTOCOL_FROM_USER&&+test_must_failgit-cprotocol.$proto.allow=userclone--bare"$url"tmp.git+)+'++# Test clone/fetch/push with protocol.allow user defined default+test_expect_success"clone $desc (enabled)"'+rm-rftmp.git&&+gitconfig--globalprotocol.allowalways&&+gitclone--bare"$url"tmp.git+'++test_expect_success"fetch $desc (enabled)"'+git-Ctmp.gitfetch+'++test_expect_success"push $desc (enabled)"'+git-Ctmp.gitpushoriginHEAD:pushed+'++test_expect_success"push $desc (disabled)"'+gitconfig--globalprotocol.allownever&&+test_must_failgit-Ctmp.gitpushoriginHEAD:pushed+'++test_expect_success"fetch $desc (disabled)"'+test_must_failgit-Ctmp.gitfetch+'++test_expect_success"clone $desc (disabled)"'+rm-rftmp.git&&+test_must_failgitclone--bare"$url"tmp.git+'+}++# 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_whitelist"$@"++test_config"$@"}# set up an ssh wrapper that will access $host/$repo in the
@@ -664,10 +664,81 @@ static const struct string_list *protocol_whitelist(void)returnenabled?&allowed:NULL;}+enumprotocol_allow_config{+PROTOCOL_ALLOW_NEVER=0,+PROTOCOL_ALLOW_USER_ONLY,+PROTOCOL_ALLOW_ALWAYS+};++staticenumprotocol_allow_configparse_protocol_config(constchar*key,+constchar*value)+{+if(!strcasecmp(value,"always"))+returnPROTOCOL_ALLOW_ALWAYS;+elseif(!strcasecmp(value,"never"))+returnPROTOCOL_ALLOW_NEVER;+elseif(!strcasecmp(value,"user"))+returnPROTOCOL_ALLOW_USER_ONLY;++die("unknown value for config '%s': %s",key,value);+}++staticenumprotocol_allow_configget_protocol_config(constchar*type)+{+char*key=xstrfmt("protocol.%s.allow",type);+char*value;++/* first check the per-protocol config */+if(!git_config_get_string(key,&value)){+enumprotocol_allow_configret=+parse_protocol_config(key,value);+free(key);+free(value);+returnret;+}+free(key);++/* if defined, fallback to user-defined default for unknown protocols */+if(!git_config_get_string("protocol.allow",&value)){+enumprotocol_allow_configret=+parse_protocol_config("protocol.allow",value);+free(value);+returnret;+}++/* fallback to built-in defaults */+/* known safe */+if(!strcmp(type,"http")||+!strcmp(type,"https")||+!strcmp(type,"git")||+!strcmp(type,"ssh")||+!strcmp(type,"file"))+returnPROTOCOL_ALLOW_ALWAYS;++/* known scary; err on the side of caution */+if(!strcmp(type,"ext"))+returnPROTOCOL_ALLOW_NEVER;++/* unknown; by default let them be used only directly by the user */+returnPROTOCOL_ALLOW_USER_ONLY;+}+intis_transport_allowed(constchar*type){-conststructstring_list*allowed=protocol_whitelist();-return!allowed||string_list_has_string(allowed,type);+conststructstring_list*whitelist=protocol_whitelist();+if(whitelist)+returnstring_list_has_string(whitelist,type);++switch(get_protocol_config(type)){+casePROTOCOL_ALLOW_ALWAYS:+return1;+casePROTOCOL_ALLOW_NEVER:+return0;+casePROTOCOL_ALLOW_USER_ONLY:+returngit_env_bool("GIT_PROTOCOL_FROM_USER",1);+}++die("BUG: invalid protocol_allow_config type");}voidtransport_check_allowed(constchar*type)
From: Brandon Williams <hidden> Date: 2016-12-01 20:26:47
Add the from_user parameter to the 'is_transport_allowed' function.
This allows callers to query if a transport protocol is allowed, given
that the caller knows that the protocol is coming from the user (1) or
not from the user (0), such as redirects in libcurl. If unknown, a -1
should be provided which falls back to reading `GIT_PROTOCOL_FROM_USER`
to determine if the protocol came from the user.
Signed-off-by: Brandon Williams <redacted>
---
http.c | 8 ++++----
transport.c | 8 +++++---
transport.h | 13 ++++++++++---
3 files changed, 19 insertions(+), 10 deletions(-)
From: Jeff King <hidden> Date: 2016-12-01 21:40:12
On Thu, Dec 01, 2016 at 12:25:59PM -0800, Brandon Williams wrote:
Add the from_user parameter to the 'is_transport_allowed' function.
This allows callers to query if a transport protocol is allowed, given
that the caller knows that the protocol is coming from the user (1) or
not from the user (0), such as redirects in libcurl. If unknown, a -1
should be provided which falls back to reading `GIT_PROTOCOL_FROM_USER`
to determine if the protocol came from the user.
Patches 3 and 4 look good to me (1 and 2 are unchanged, right? They are
already in 'next' anyway, though I guess we are due for a post-release
reset of 'next').
This is better, but I think we still need to deal with http-alternates
on top.
I think we'd need to move this allowed_protocols setup into a function
like:
int generate_allowed_protocols(int from_user)
{
int ret;
if (is_transport_allowed("http", from_user))
ret |= CURLPROTO_HTTP;
... etc ...
return ret;
}
and then create a protocol list for each situation:
allowed_protocols = generate_allowed_protocols(-1);
allowed_redir_protocols = generate_allowed_protocols(0);
and then we know we can always set up the redir protocols:
curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_redir_protocols);
and which we feed for CURLOPT_PROTOCOLS depends on whether we are
following an http-alternates redirect or not. But I suspect it will be a
nasty change to plumb through the idea of "this request is on behalf of
an http-alternates redirect".
Given how few people probably care, I'm tempted to document it as a
quirk and direct people to the upcoming http.followRedirects. The newly
proposed default value of that disables http-alternates entirely anyway.
-Peff
From: Brandon Williams <hidden> Date: 2016-12-01 23:07:58
On 12/01, Jeff King wrote:
On Thu, Dec 01, 2016 at 12:25:59PM -0800, Brandon Williams wrote:
quoted
Add the from_user parameter to the 'is_transport_allowed' function.
This allows callers to query if a transport protocol is allowed, given
that the caller knows that the protocol is coming from the user (1) or
not from the user (0), such as redirects in libcurl. If unknown, a -1
should be provided which falls back to reading `GIT_PROTOCOL_FROM_USER`
to determine if the protocol came from the user.
Patches 3 and 4 look good to me (1 and 2 are unchanged, right? They are
already in 'next' anyway, though I guess we are due for a post-release
reset of 'next').
This is better, but I think we still need to deal with http-alternates
on top.
I think we'd need to move this allowed_protocols setup into a function
like:
int generate_allowed_protocols(int from_user)
{
int ret;
if (is_transport_allowed("http", from_user))
ret |= CURLPROTO_HTTP;
... etc ...
return ret;
}
and then create a protocol list for each situation:
allowed_protocols = generate_allowed_protocols(-1);
allowed_redir_protocols = generate_allowed_protocols(0);
and then we know we can always set up the redir protocols:
curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_redir_protocols);
and which we feed for CURLOPT_PROTOCOLS depends on whether we are
following an http-alternates redirect or not. But I suspect it will be a
nasty change to plumb through the idea of "this request is on behalf of
an http-alternates redirect".
Given how few people probably care, I'm tempted to document it as a
quirk and direct people to the upcoming http.followRedirects. The newly
proposed default value of that disables http-alternates entirely anyway.
-Peff
I started taking a look at your http redirect series (I really should
have taking a look at it sooner) and I see exactly what you're talking
about. We can easily move this logic into a function to make it easier
to generate the two whitelists.
--
Brandon Williams
From: Brandon Williams <hidden> Date: 2016-12-01 23:27:05
On 12/01, Brandon Williams wrote:
On 12/01, Jeff King wrote:
quoted
On Thu, Dec 01, 2016 at 12:25:59PM -0800, Brandon Williams wrote:
quoted
Add the from_user parameter to the 'is_transport_allowed' function.
This allows callers to query if a transport protocol is allowed, given
that the caller knows that the protocol is coming from the user (1) or
not from the user (0), such as redirects in libcurl. If unknown, a -1
should be provided which falls back to reading `GIT_PROTOCOL_FROM_USER`
to determine if the protocol came from the user.
Patches 3 and 4 look good to me (1 and 2 are unchanged, right? They are
already in 'next' anyway, though I guess we are due for a post-release
reset of 'next').
This is better, but I think we still need to deal with http-alternates
on top.
I think we'd need to move this allowed_protocols setup into a function
like:
int generate_allowed_protocols(int from_user)
{
int ret;
if (is_transport_allowed("http", from_user))
ret |= CURLPROTO_HTTP;
... etc ...
return ret;
}
and then create a protocol list for each situation:
allowed_protocols = generate_allowed_protocols(-1);
allowed_redir_protocols = generate_allowed_protocols(0);
and then we know we can always set up the redir protocols:
curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_redir_protocols);
and which we feed for CURLOPT_PROTOCOLS depends on whether we are
following an http-alternates redirect or not. But I suspect it will be a
nasty change to plumb through the idea of "this request is on behalf of
an http-alternates redirect".
Given how few people probably care, I'm tempted to document it as a
quirk and direct people to the upcoming http.followRedirects. The newly
proposed default value of that disables http-alternates entirely anyway.
-Peff
I started taking a look at your http redirect series (I really should
have taking a look at it sooner) and I see exactly what you're talking
about. We can easily move this logic into a function to make it easier
to generate the two whitelists.
Thinking about this some more...I was told that having http redirect to
file:// could be scary. The way the new protocol configuration is setup
we have file:// as a default known-safe protocol. Do we need to worry
about this or can we leave this be since this can be overridden by the
user?
--
Brandon Williams
From: Brandon Williams <hidden> Date: 2016-12-02 00:01:22
v8 of this series moves the creation of an allowed protocol whitelist for
CURLOPT_REDIR_PROTOCOLS to a helper function. This is to help out another
series which depends on the creation of a whitelist for CURLOPT_PROTOCOLS.
Brandon Williams (5):
lib-proto-disable: variable name fix
transport: add protocol policy config option
http: always warn if libcurl version is too old
http: create function to get curl allowed protocols
transport: add from_user parameter to is_transport_allowed
Documentation/config.txt | 46 +++++++++++++
Documentation/git.txt | 38 ++++-------
git-submodule.sh | 12 ++--
http.c | 32 +++++----
t/lib-proto-disable.sh | 142 ++++++++++++++++++++++++++++++++++++---
t/t5509-fetch-push-namespaces.sh | 1 +
t/t5802-connect-helper.sh | 1 +
transport.c | 84 ++++++++++++++++++++---
transport.h | 19 +++---
9 files changed, 302 insertions(+), 73 deletions(-)
--
2.8.0.rc3.226.g39d4020
From: Brandon Williams <hidden> Date: 2016-12-02 00:01:25
The test_proto function assigns the positional parameters to named
variables, but then still refers to "$desc" as "$1". Using $desc is
more readable and less error-prone.
Signed-off-by: Brandon Williams <redacted>
---
t/lib-proto-disable.sh | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-02 00:01:28
Now that there are default "known-good" and "known-bad" protocols which
are allowed/disallowed by 'is_transport_allowed' we should always warn
the user that older versions of libcurl can't respect the allowed
protocols for redirects.
Signed-off-by: Brandon Williams <redacted>
---
http.c | 5 ++---
transport.c | 5 -----
transport.h | 6 ------
3 files changed, 2 insertions(+), 14 deletions(-)
@@ -735,9 +735,8 @@ static CURL *get_curl_handle(void)allowed_protocols|=CURLPROTO_FTPS;curl_easy_setopt(result,CURLOPT_REDIR_PROTOCOLS,allowed_protocols);#else-if(transport_restrict_protocols())-warning("protocol restrictions not applied to curl redirects because\n"-"your curl version is too old (>= 7.19.4)");+warning("protocol restrictions not applied to curl redirects because\n"+"your curl version is too old (>= 7.19.4)");#endifif(getenv("GIT_CURL_VERBOSE"))curl_easy_setopt(result,CURLOPT_VERBOSE,1L);
@@ -164,12 +164,6 @@ int is_transport_allowed(const char *type);*/voidtransport_check_allowed(constchar*type);-/*-*Returnstrueiftheuserhasattemptedtoturnonprotocol-*restrictionsatall.-*/-inttransport_restrict_protocols(void);-/* Transport options which apply to git:// and scp-style URLs *//* The program to use on the remote side to send a pack */
From: Brandon Williams <hidden> Date: 2016-12-02 00:01:31
Add the from_user parameter to the 'is_transport_allowed' function.
This allows callers to query if a transport protocol is allowed, given
that the caller knows that the protocol is coming from the user (1) or
not from the user (0) such as redirects in libcurl. If unknown a -1
should be provided which falls back to reading `GIT_PROTOCOL_FROM_USER`
to determine if the protocol came from the user.
Signed-off-by: Brandon Williams <redacted>
---
http.c | 12 ++++++------
transport.c | 8 +++++---
transport.h | 13 ++++++++++---
3 files changed, 21 insertions(+), 12 deletions(-)
@@ -740,7 +740,7 @@ static CURL *get_curl_handle(void)#endif#if LIBCURL_VERSION_NUM >= 0x071304curl_easy_setopt(result,CURLOPT_REDIR_PROTOCOLS,-get_curl_allowed_protocols());+get_curl_allowed_protocols(0));#elsewarning("protocol restrictions not applied to curl redirects because\n""your curl version is too old (>= 7.19.4)");
From: Brandon Williams <hidden> Date: 2016-12-02 00:01:39
Previously the `GIT_ALLOW_PROTOCOL` environment variable was used to
specify a whitelist of protocols to be used in clone/fetch/push
commands. This patch introduces new configuration options for more
fine-grained control for allowing/disallowing protocols. This also has
the added benefit of allowing easier construction of a protocol
whitelist on systems where setting an environment variable is
non-trivial.
Now users can specify a policy to be used for each type of protocol via
the 'protocol.<name>.allow' config option. A default policy for all
unconfigured protocols can be set with the 'protocol.allow' config
option. If no user configured default is made git will allow known-safe
protocols (http, https, git, ssh, file), disallow known-dangerous
protocols (ext), and have a default policy of `user` for all other
protocols.
The supported policies are `always`, `never`, and `user`. The `user`
policy can be used to configure a protocol to be usable when explicitly
used by a user, while disallowing it for commands which run
clone/fetch/push commands without direct user intervention (e.g.
recursive initialization of submodules). Commands which can potentially
clone/fetch/push from untrusted repositories without user intervention
can export `GIT_PROTOCOL_FROM_USER` with a value of '0' to prevent
protocols configured to the `user` policy from being used.
Fix remote-ext tests to use the new config to allow the ext
protocol to be tested.
Based on a patch by Jeff King [off-list ref]
Signed-off-by: Brandon Williams <redacted>
---
Documentation/config.txt | 46 ++++++++++++++
Documentation/git.txt | 38 +++++-------
git-submodule.sh | 12 ++--
t/lib-proto-disable.sh | 130 +++++++++++++++++++++++++++++++++++++--
t/t5509-fetch-push-namespaces.sh | 1 +
t/t5802-connect-helper.sh | 1 +
transport.c | 75 +++++++++++++++++++++-
7 files changed, 264 insertions(+), 39 deletions(-)
@@ -2308,6 +2308,52 @@ 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 directly usable by the user but don't want it used by commands which+ execute clone/fetch/push commands without user input, e.g. recursive+ submodule initialization.++--++protocol.<name>.allow::+ Set a policy to be used by protocol `<name>` with clone/fetch/push+ commands. See `protocol.allow` above for the available policies.+++The protocol names currently used by git are:+++--+ - `file`: any local file-based path (including `file://` URLs,+ or local paths)++ - `git`: the anonymous git protocol over a direct TCP+ connection (or proxy, if configured)++ - `ssh`: git over ssh (including `host:path` syntax,+ `ssh://`, etc).++ - `http`: git over http, both "smart http" and "dumb http".+ Note that this does _not_ include `https`; if you want to configure+ both, you must do so individually.++ - any external helpers are named by their protocol (e.g., use+ `hg` to allow the `git-remote-hg` helper)+--+ pull.ff:: By default, Git does not create an extra merge commit when merging a commit that is a descendant of the current commit. Instead, the
@@ -1150,30 +1150,20 @@ 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:-- - `file`: any local file-based path (including `file://` URLs,- or local paths)-- - `git`: the anonymous git protocol over a direct TCP- connection (or proxy, if configured)-- - `ssh`: git over ssh (including `host:path` syntax,- `ssh://`, etc).-- - `http`: git over http, both "smart http" and "dumb http".- Note that this does _not_ include `https`; if you want both,- you should specify both as `http:https`.-- - any external helpers are named by their protocol (e.g., use- `hg` to allow the `git-remote-hg` helper)-+ If set to a colon-separated list of protocols, behave as if+ `protocol.allow` is set to `never`, and each of the listed+ protocols has `protocol.<name>.allow` set to `always`+ (overriding any existing configuration). In other words, any+ protocol not mentioned will be disallowed (i.e., this is a+ whitelist, not a blacklist). See the description of+ `protocol.allow` in linkgit:git-config[1] for more details.++`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 or for programs+ which feed potentially-untrusted URLS to git commands. See+ linkgit:git-config[1] for more details. Discussion[[Discussion]] ------------------------
@@ -21,14 +21,10 @@ require_work_treewt_prefix=$(gitrev-parse--show-prefix) cd_to_toplevel-# Restrict ourselves to a vanilla subset of protocols; the URLs-# we get are under control of a remote repository, and we do not-# want them kicking off arbitrary git-remote-* programs.-#-# 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}-exportGIT_ALLOW_PROTOCOL+# Tell the rest of git that any URLs we get don't come+# directly from the user, so it can apply policy as appropriate.+GIT_PROTOCOL_FROM_USER=0+exportGIT_PROTOCOL_FROM_USERcommand=branch=
@@ -1,10 +1,7 @@# 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=$1proto=$2url=$3
@@ -62,6 +59,129 @@ test_proto () {test_must_failgitclone--bare"$url"tmp.git)'++test_expect_success"clone $desc (env var has precedence)"'+rm-rftmp.git&&+(+GIT_ALLOW_PROTOCOL=none&&+exportGIT_ALLOW_PROTOCOL&&+test_must_failgit-cprotocol.allow=alwaysclone--bare"$url"tmp.git&&+test_must_failgit-cprotocol.$proto.allow=alwaysclone--bare"$url"tmp.git+)+'+}++test_config(){+desc=$1+proto=$2+url=$3++# Test clone/fetch/push with protocol.<type>.allow config+test_expect_success"clone $desc (enabled with config)"'+rm-rftmp.git&&+git-cprotocol.$proto.allow=alwaysclone--bare"$url"tmp.git+'++test_expect_success"fetch $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=alwaysfetch+'++test_expect_success"push $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=alwayspushoriginHEAD:pushed+'++test_expect_success"push $desc (disabled)"'+test_must_failgit-Ctmp.git-cprotocol.$proto.allow=neverpushoriginHEAD:pushed+'++test_expect_success"fetch $desc (disabled)"'+test_must_failgit-Ctmp.git-cprotocol.$proto.allow=neverfetch+'++test_expect_success"clone $desc (disabled)"'+rm-rftmp.git&&+test_must_failgit-cprotocol.$proto.allow=neverclone--bare"$url"tmp.git+'++# Test clone/fetch/push with protocol.user.allow and its env var+test_expect_success"clone $desc (enabled)"'+rm-rftmp.git&&+git-cprotocol.$proto.allow=userclone--bare"$url"tmp.git+'++test_expect_success"fetch $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=userfetch+'++test_expect_success"push $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=userpushoriginHEAD:pushed+'++test_expect_success"push $desc (disabled)"'+(+cdtmp.git&&+GIT_PROTOCOL_FROM_USER=0&&+exportGIT_PROTOCOL_FROM_USER&&+test_must_failgit-cprotocol.$proto.allow=userpushoriginHEAD:pushed+)+'++test_expect_success"fetch $desc (disabled)"'+(+cdtmp.git&&+GIT_PROTOCOL_FROM_USER=0&&+exportGIT_PROTOCOL_FROM_USER&&+test_must_failgit-cprotocol.$proto.allow=userfetch+)+'++test_expect_success"clone $desc (disabled)"'+rm-rftmp.git&&+(+GIT_PROTOCOL_FROM_USER=0&&+exportGIT_PROTOCOL_FROM_USER&&+test_must_failgit-cprotocol.$proto.allow=userclone--bare"$url"tmp.git+)+'++# Test clone/fetch/push with protocol.allow user defined default+test_expect_success"clone $desc (enabled)"'+rm-rftmp.git&&+gitconfig--globalprotocol.allowalways&&+gitclone--bare"$url"tmp.git+'++test_expect_success"fetch $desc (enabled)"'+git-Ctmp.gitfetch+'++test_expect_success"push $desc (enabled)"'+git-Ctmp.gitpushoriginHEAD:pushed+'++test_expect_success"push $desc (disabled)"'+gitconfig--globalprotocol.allownever&&+test_must_failgit-Ctmp.gitpushoriginHEAD:pushed+'++test_expect_success"fetch $desc (disabled)"'+test_must_failgit-Ctmp.gitfetch+'++test_expect_success"clone $desc (disabled)"'+rm-rftmp.git&&+test_must_failgitclone--bare"$url"tmp.git+'+}++# 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_whitelist"$@"++test_config"$@"}# set up an ssh wrapper that will access $host/$repo in the
@@ -664,10 +664,81 @@ static const struct string_list *protocol_whitelist(void)returnenabled?&allowed:NULL;}+enumprotocol_allow_config{+PROTOCOL_ALLOW_NEVER=0,+PROTOCOL_ALLOW_USER_ONLY,+PROTOCOL_ALLOW_ALWAYS+};++staticenumprotocol_allow_configparse_protocol_config(constchar*key,+constchar*value)+{+if(!strcasecmp(value,"always"))+returnPROTOCOL_ALLOW_ALWAYS;+elseif(!strcasecmp(value,"never"))+returnPROTOCOL_ALLOW_NEVER;+elseif(!strcasecmp(value,"user"))+returnPROTOCOL_ALLOW_USER_ONLY;++die("unknown value for config '%s': %s",key,value);+}++staticenumprotocol_allow_configget_protocol_config(constchar*type)+{+char*key=xstrfmt("protocol.%s.allow",type);+char*value;++/* first check the per-protocol config */+if(!git_config_get_string(key,&value)){+enumprotocol_allow_configret=+parse_protocol_config(key,value);+free(key);+free(value);+returnret;+}+free(key);++/* if defined, fallback to user-defined default for unknown protocols */+if(!git_config_get_string("protocol.allow",&value)){+enumprotocol_allow_configret=+parse_protocol_config("protocol.allow",value);+free(value);+returnret;+}++/* fallback to built-in defaults */+/* known safe */+if(!strcmp(type,"http")||+!strcmp(type,"https")||+!strcmp(type,"git")||+!strcmp(type,"ssh")||+!strcmp(type,"file"))+returnPROTOCOL_ALLOW_ALWAYS;++/* known scary; err on the side of caution */+if(!strcmp(type,"ext"))+returnPROTOCOL_ALLOW_NEVER;++/* unknown; by default let them be used only directly by the user */+returnPROTOCOL_ALLOW_USER_ONLY;+}+intis_transport_allowed(constchar*type){-conststructstring_list*allowed=protocol_whitelist();-return!allowed||string_list_has_string(allowed,type);+conststructstring_list*whitelist=protocol_whitelist();+if(whitelist)+returnstring_list_has_string(whitelist,type);++switch(get_protocol_config(type)){+casePROTOCOL_ALLOW_ALWAYS:+return1;+casePROTOCOL_ALLOW_NEVER:+return0;+casePROTOCOL_ALLOW_USER_ONLY:+returngit_env_bool("GIT_PROTOCOL_FROM_USER",1);+}++die("BUG: invalid protocol_allow_config type");}voidtransport_check_allowed(constchar*type)
From: Brandon Williams <hidden> Date: 2016-12-02 00:01:42
Move the creation of an allowed protocols whitelist to a helper
function.
Signed-off-by: Brandon Williams <redacted>
---
http.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
@@ -725,15 +739,8 @@ static CURL *get_curl_handle(void)curl_easy_setopt(result,CURLOPT_POST301,1);#endif#if LIBCURL_VERSION_NUM >= 0x071304-if(is_transport_allowed("http"))-allowed_protocols|=CURLPROTO_HTTP;-if(is_transport_allowed("https"))-allowed_protocols|=CURLPROTO_HTTPS;-if(is_transport_allowed("ftp"))-allowed_protocols|=CURLPROTO_FTP;-if(is_transport_allowed("ftps"))-allowed_protocols|=CURLPROTO_FTPS;-curl_easy_setopt(result,CURLOPT_REDIR_PROTOCOLS,allowed_protocols);+curl_easy_setopt(result,CURLOPT_REDIR_PROTOCOLS,+get_curl_allowed_protocols());#elsewarning("protocol restrictions not applied to curl redirects because\n""your curl version is too old (>= 7.19.4)");
From: Jeff King <hidden> Date: 2016-12-02 00:21:00
On Thu, Dec 01, 2016 at 03:26:56PM -0800, Brandon Williams wrote:
quoted
I started taking a look at your http redirect series (I really should
have taking a look at it sooner) and I see exactly what you're talking
about. We can easily move this logic into a function to make it easier
to generate the two whitelists.
Thinking about this some more...I was told that having http redirect to
file:// could be scary. The way the new protocol configuration is setup
we have file:// as a default known-safe protocol. Do we need to worry
about this or can we leave this be since this can be overridden by the
user?
Hmm. I'm not sure if file:// should actually be USER_ONLY. The submodule
code allows it, and it's certainly a convenience, but I guess you could
do tricky things by probing somebody's filesystem with submodules URLs.
On the other hand, if you are recursively cloning untrusted repos and
have sensitive contents on disk, you really _should_ be setting up a
protocol whitelist.
For HTTP redirects within curl, I think it's a non-issue; curl
automatically disallows file:// for redirects, even without us telling
it so.
For redirects via http-alternates, it's a bit more tricky, as we feed
the URL to curl ourselves, so it can't tell the difference between
trusted and untrusted input. The main protection provided by my series
is "don't follow http-alternates at all". But assuming you did want to
use them (by setting http.followRedirects to "true", at least for the
server in question), we could then feed file:// directly to curl. But I
think we are still OK, because the restricted CURLOPT_PROTOCOL setting
would prevent that from working. I.e., git _never_ wants curl to handle
file://, because it handles it without calling into remote-curl.c at
all.
So arguably file:// should be USER_ONLY, but I'm not sure how much it
matters in practice.
-Peff
From: Brandon Williams <hidden> Date: 2016-12-02 17:34:00
On 12/01, Jeff King wrote:
On Thu, Dec 01, 2016 at 03:26:56PM -0800, Brandon Williams wrote:
quoted
quoted
I started taking a look at your http redirect series (I really should
have taking a look at it sooner) and I see exactly what you're talking
about. We can easily move this logic into a function to make it easier
to generate the two whitelists.
Thinking about this some more...I was told that having http redirect to
file:// could be scary. The way the new protocol configuration is setup
we have file:// as a default known-safe protocol. Do we need to worry
about this or can we leave this be since this can be overridden by the
user?
Hmm. I'm not sure if file:// should actually be USER_ONLY. The submodule
code allows it, and it's certainly a convenience, but I guess you could
do tricky things by probing somebody's filesystem with submodules URLs.
On the other hand, if you are recursively cloning untrusted repos and
have sensitive contents on disk, you really _should_ be setting up a
protocol whitelist.
For HTTP redirects within curl, I think it's a non-issue; curl
automatically disallows file:// for redirects, even without us telling
it so.
For redirects via http-alternates, it's a bit more tricky, as we feed
the URL to curl ourselves, so it can't tell the difference between
trusted and untrusted input. The main protection provided by my series
is "don't follow http-alternates at all". But assuming you did want to
use them (by setting http.followRedirects to "true", at least for the
server in question), we could then feed file:// directly to curl. But I
think we are still OK, because the restricted CURLOPT_PROTOCOL setting
would prevent that from working. I.e., git _never_ wants curl to handle
file://, because it handles it without calling into remote-curl.c at
all.
So arguably file:// should be USER_ONLY, but I'm not sure how much it
matters in practice.
Ah ok thanks for the good explanation. I was mostly interested in the
http redirect case which, as you said, becomes a non-issue due to how we
configure curl.
Thanks!
--
Brandon Williams
From: Brandon Williams <hidden> Date: 2016-12-14 01:42:26
The test_proto function assigns the positional parameters to named
variables, but then still refers to "$desc" as "$1". Using $desc is
more readable and less error-prone.
Signed-off-by: Brandon Williams <redacted>
---
t/lib-proto-disable.sh | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Brandon Williams <hidden> Date: 2016-12-14 01:42:30
Only difference between v8 and v9 is that v9 has been rebased ontop of Jeff's
http-walker-limit-redirect series 'jk/http-walker-limit-redirect'.
Brandon Williams (5):
lib-proto-disable: variable name fix
transport: add protocol policy config option
http: always warn if libcurl version is too old
http: create function to get curl allowed protocols
transport: add from_user parameter to is_transport_allowed
Documentation/config.txt | 46 +++++++++++++
Documentation/git.txt | 38 ++++-------
git-submodule.sh | 12 ++--
http.c | 36 ++++++----
t/lib-proto-disable.sh | 142 ++++++++++++++++++++++++++++++++++++---
t/t5509-fetch-push-namespaces.sh | 1 +
t/t5802-connect-helper.sh | 1 +
transport.c | 84 ++++++++++++++++++++---
transport.h | 19 +++---
9 files changed, 305 insertions(+), 74 deletions(-)
--
2.8.0.rc3.226.g39d4020
From: Brandon Williams <hidden> Date: 2016-12-14 01:42:33
Previously the `GIT_ALLOW_PROTOCOL` environment variable was used to
specify a whitelist of protocols to be used in clone/fetch/push
commands. This patch introduces new configuration options for more
fine-grained control for allowing/disallowing protocols. This also has
the added benefit of allowing easier construction of a protocol
whitelist on systems where setting an environment variable is
non-trivial.
Now users can specify a policy to be used for each type of protocol via
the 'protocol.<name>.allow' config option. A default policy for all
unconfigured protocols can be set with the 'protocol.allow' config
option. If no user configured default is made git will allow known-safe
protocols (http, https, git, ssh, file), disallow known-dangerous
protocols (ext), and have a default policy of `user` for all other
protocols.
The supported policies are `always`, `never`, and `user`. The `user`
policy can be used to configure a protocol to be usable when explicitly
used by a user, while disallowing it for commands which run
clone/fetch/push commands without direct user intervention (e.g.
recursive initialization of submodules). Commands which can potentially
clone/fetch/push from untrusted repositories without user intervention
can export `GIT_PROTOCOL_FROM_USER` with a value of '0' to prevent
protocols configured to the `user` policy from being used.
Fix remote-ext tests to use the new config to allow the ext
protocol to be tested.
Based on a patch by Jeff King [off-list ref]
Signed-off-by: Brandon Williams <redacted>
---
Documentation/config.txt | 46 ++++++++++++++
Documentation/git.txt | 38 +++++-------
git-submodule.sh | 12 ++--
t/lib-proto-disable.sh | 130 +++++++++++++++++++++++++++++++++++++--
t/t5509-fetch-push-namespaces.sh | 1 +
t/t5802-connect-helper.sh | 1 +
transport.c | 75 +++++++++++++++++++++-
7 files changed, 264 insertions(+), 39 deletions(-)
@@ -2260,6 +2260,52 @@ 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 directly usable by the user but don't want it used by commands which+ execute clone/fetch/push commands without user input, e.g. recursive+ submodule initialization.++--++protocol.<name>.allow::+ Set a policy to be used by protocol `<name>` with clone/fetch/push+ commands. See `protocol.allow` above for the available policies.+++The protocol names currently used by git are:+++--+ - `file`: any local file-based path (including `file://` URLs,+ or local paths)++ - `git`: the anonymous git protocol over a direct TCP+ connection (or proxy, if configured)++ - `ssh`: git over ssh (including `host:path` syntax,+ `ssh://`, etc).++ - `http`: git over http, both "smart http" and "dumb http".+ Note that this does _not_ include `https`; if you want to configure+ both, you must do so individually.++ - any external helpers are named by their protocol (e.g., use+ `hg` to allow the `git-remote-hg` helper)+--+ pull.ff:: By default, Git does not create an extra merge commit when merging a commit that is a descendant of the current commit. Instead, the
@@ -1129,30 +1129,20 @@ 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:-- - `file`: any local file-based path (including `file://` URLs,- or local paths)-- - `git`: the anonymous git protocol over a direct TCP- connection (or proxy, if configured)-- - `ssh`: git over ssh (including `host:path` syntax,- `ssh://`, etc).-- - `http`: git over http, both "smart http" and "dumb http".- Note that this does _not_ include `https`; if you want both,- you should specify both as `http:https`.-- - any external helpers are named by their protocol (e.g., use- `hg` to allow the `git-remote-hg` helper)-+ If set to a colon-separated list of protocols, behave as if+ `protocol.allow` is set to `never`, and each of the listed+ protocols has `protocol.<name>.allow` set to `always`+ (overriding any existing configuration). In other words, any+ protocol not mentioned will be disallowed (i.e., this is a+ whitelist, not a blacklist). See the description of+ `protocol.allow` in linkgit:git-config[1] for more details.++`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 or for programs+ which feed potentially-untrusted URLS to git commands. See+ linkgit:git-config[1] for more details. Discussion[[Discussion]] ------------------------
@@ -22,14 +22,10 @@ require_work_treewt_prefix=$(gitrev-parse--show-prefix) cd_to_toplevel-# Restrict ourselves to a vanilla subset of protocols; the URLs-# we get are under control of a remote repository, and we do not-# want them kicking off arbitrary git-remote-* programs.-#-# 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}-exportGIT_ALLOW_PROTOCOL+# Tell the rest of git that any URLs we get don't come+# directly from the user, so it can apply policy as appropriate.+GIT_PROTOCOL_FROM_USER=0+exportGIT_PROTOCOL_FROM_USERcommand=branch=
@@ -1,10 +1,7 @@# 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=$1proto=$2url=$3
@@ -62,6 +59,129 @@ test_proto () {test_must_failgitclone--bare"$url"tmp.git)'++test_expect_success"clone $desc (env var has precedence)"'+rm-rftmp.git&&+(+GIT_ALLOW_PROTOCOL=none&&+exportGIT_ALLOW_PROTOCOL&&+test_must_failgit-cprotocol.allow=alwaysclone--bare"$url"tmp.git&&+test_must_failgit-cprotocol.$proto.allow=alwaysclone--bare"$url"tmp.git+)+'+}++test_config(){+desc=$1+proto=$2+url=$3++# Test clone/fetch/push with protocol.<type>.allow config+test_expect_success"clone $desc (enabled with config)"'+rm-rftmp.git&&+git-cprotocol.$proto.allow=alwaysclone--bare"$url"tmp.git+'++test_expect_success"fetch $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=alwaysfetch+'++test_expect_success"push $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=alwayspushoriginHEAD:pushed+'++test_expect_success"push $desc (disabled)"'+test_must_failgit-Ctmp.git-cprotocol.$proto.allow=neverpushoriginHEAD:pushed+'++test_expect_success"fetch $desc (disabled)"'+test_must_failgit-Ctmp.git-cprotocol.$proto.allow=neverfetch+'++test_expect_success"clone $desc (disabled)"'+rm-rftmp.git&&+test_must_failgit-cprotocol.$proto.allow=neverclone--bare"$url"tmp.git+'++# Test clone/fetch/push with protocol.user.allow and its env var+test_expect_success"clone $desc (enabled)"'+rm-rftmp.git&&+git-cprotocol.$proto.allow=userclone--bare"$url"tmp.git+'++test_expect_success"fetch $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=userfetch+'++test_expect_success"push $desc (enabled)"'+git-Ctmp.git-cprotocol.$proto.allow=userpushoriginHEAD:pushed+'++test_expect_success"push $desc (disabled)"'+(+cdtmp.git&&+GIT_PROTOCOL_FROM_USER=0&&+exportGIT_PROTOCOL_FROM_USER&&+test_must_failgit-cprotocol.$proto.allow=userpushoriginHEAD:pushed+)+'++test_expect_success"fetch $desc (disabled)"'+(+cdtmp.git&&+GIT_PROTOCOL_FROM_USER=0&&+exportGIT_PROTOCOL_FROM_USER&&+test_must_failgit-cprotocol.$proto.allow=userfetch+)+'++test_expect_success"clone $desc (disabled)"'+rm-rftmp.git&&+(+GIT_PROTOCOL_FROM_USER=0&&+exportGIT_PROTOCOL_FROM_USER&&+test_must_failgit-cprotocol.$proto.allow=userclone--bare"$url"tmp.git+)+'++# Test clone/fetch/push with protocol.allow user defined default+test_expect_success"clone $desc (enabled)"'+rm-rftmp.git&&+gitconfig--globalprotocol.allowalways&&+gitclone--bare"$url"tmp.git+'++test_expect_success"fetch $desc (enabled)"'+git-Ctmp.gitfetch+'++test_expect_success"push $desc (enabled)"'+git-Ctmp.gitpushoriginHEAD:pushed+'++test_expect_success"push $desc (disabled)"'+gitconfig--globalprotocol.allownever&&+test_must_failgit-Ctmp.gitpushoriginHEAD:pushed+'++test_expect_success"fetch $desc (disabled)"'+test_must_failgit-Ctmp.gitfetch+'++test_expect_success"clone $desc (disabled)"'+rm-rftmp.git&&+test_must_failgitclone--bare"$url"tmp.git+'+}++# 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_whitelist"$@"++test_config"$@"}# set up an ssh wrapper that will access $host/$repo in the
@@ -617,10 +617,81 @@ static const struct string_list *protocol_whitelist(void)returnenabled?&allowed:NULL;}+enumprotocol_allow_config{+PROTOCOL_ALLOW_NEVER=0,+PROTOCOL_ALLOW_USER_ONLY,+PROTOCOL_ALLOW_ALWAYS+};++staticenumprotocol_allow_configparse_protocol_config(constchar*key,+constchar*value)+{+if(!strcasecmp(value,"always"))+returnPROTOCOL_ALLOW_ALWAYS;+elseif(!strcasecmp(value,"never"))+returnPROTOCOL_ALLOW_NEVER;+elseif(!strcasecmp(value,"user"))+returnPROTOCOL_ALLOW_USER_ONLY;++die("unknown value for config '%s': %s",key,value);+}++staticenumprotocol_allow_configget_protocol_config(constchar*type)+{+char*key=xstrfmt("protocol.%s.allow",type);+char*value;++/* first check the per-protocol config */+if(!git_config_get_string(key,&value)){+enumprotocol_allow_configret=+parse_protocol_config(key,value);+free(key);+free(value);+returnret;+}+free(key);++/* if defined, fallback to user-defined default for unknown protocols */+if(!git_config_get_string("protocol.allow",&value)){+enumprotocol_allow_configret=+parse_protocol_config("protocol.allow",value);+free(value);+returnret;+}++/* fallback to built-in defaults */+/* known safe */+if(!strcmp(type,"http")||+!strcmp(type,"https")||+!strcmp(type,"git")||+!strcmp(type,"ssh")||+!strcmp(type,"file"))+returnPROTOCOL_ALLOW_ALWAYS;++/* known scary; err on the side of caution */+if(!strcmp(type,"ext"))+returnPROTOCOL_ALLOW_NEVER;++/* unknown; by default let them be used only directly by the user */+returnPROTOCOL_ALLOW_USER_ONLY;+}+intis_transport_allowed(constchar*type){-conststructstring_list*allowed=protocol_whitelist();-return!allowed||string_list_has_string(allowed,type);+conststructstring_list*whitelist=protocol_whitelist();+if(whitelist)+returnstring_list_has_string(whitelist,type);++switch(get_protocol_config(type)){+casePROTOCOL_ALLOW_ALWAYS:+return1;+casePROTOCOL_ALLOW_NEVER:+return0;+casePROTOCOL_ALLOW_USER_ONLY:+returngit_env_bool("GIT_PROTOCOL_FROM_USER",1);+}++die("BUG: invalid protocol_allow_config type");}voidtransport_check_allowed(constchar*type)