* Where is Doc to configure Git + Apache + kerberos for Project level access in repo?
From: ken edward @ 2016-12-02 18:15 UTC (permalink / raw)
To: git
Where is Doc to configure Git + Apache + kerberos for Project level
access in repo?
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Brandon Williams @ 2016-12-02 17:33 UTC (permalink / raw)
To: Jeff King; +Cc: git, sbeller, bburky, jrnieder
In-Reply-To: <20161202001353.jiw4hjqg75dr6psw@sigill.intra.peff.net>
On 12/01, Jeff King wrote:
> On Thu, Dec 01, 2016 at 03:26:56PM -0800, Brandon Williams wrote:
>
> > > 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
^ permalink raw reply
* Re: Error after calling git difftool -d with
From: Johannes Schindelin @ 2016-12-02 16:05 UTC (permalink / raw)
To: P. Duijst; +Cc: git
In-Reply-To: <5f630c90-cf54-3a23-c9a9-af035d4514e0@gmail.com>
Hi Peter,
On Fri, 2 Dec 2016, P. Duijst wrote:
> Incase filenames are used with a quote ' or a bracket [ (and maybe some more
> characters), git "diff" and "difftool -y" works fine, but git *difftool **-d*
> gives the next error message:
>
> peter@scm_ws_10 MINGW64 /d/Dev/test (master)
> $ git diff
> diff --git a/Test ''inch.txt b/Test ''inch.txt
> index dbff793..41f3257 100644
> --- a/Test ''inch.txt
> +++ b/Test ''inch.txt
> @@ -1 +1,3 @@
> +
> +ddd
> Test error in simple repository
> warning: LF will be replaced by CRLF in Test ''inch.txt.
> The file will have its original line endings in your working directory.
>
> peter@scm_ws_10 MINGW64 /d/Dev/test (master)
> *$ git difftool -d*
> *fatal: Cannot open '/d/Dev/test//Test ''inch.txt': No such file or
> directory*
> *hash-object /d/Dev/test//Test ''inch.txt: command returned error: 128*
>
> peter@scm_ws_10 MINGW64 /d/Dev/test (master)
> $
>
>
> This issue is inside V2.10.x and V2.11.0.
> V2.9.0 is working correctly...
You say v2.11.0, but did you also try the new, experimental builtin
difftool? You can test without reinstalling:
git -c difftool.useBuiltin=true difftool -d ...
Ciao,
Johannes
^ permalink raw reply
* Re: "Your branch is ahead of 'origin' by X commits"
From: Matthieu Moy @ 2016-12-02 13:42 UTC (permalink / raw)
To: Alfonsogonzalez, Ernesto (GE Digital); +Cc: git@vger.kernel.org
In-Reply-To: <D465BDE6.B7DE%ernesto.alfonsogonzalez@ge.com>
"Alfonsogonzalez, Ernesto (GE Digital)" <ernesto.alfonsogonzalez@ge.com>
writes:
> Hi,
>
> Git status tells me "Your branch is ahead of 'origin' by 108 commits.²,
> but my local and origin/master are pointing to the same commit.
>
> What am I doing wrong?
>
> $ git diff origin/master
> $ git status
> On branch master
> Your branch is ahead of 'origin' by 108 commits.
This line should say "ahead of 'origin/master'" in common setups, where
'origin/master' is the remote-tracking branch configured as upstream for
'master'.
My guess is that you have a badly configured upstream.
What does "git pull -v" say? What's the content of the [branch "master"]
section of .git/config?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH v2 1/1] convert: git cherry-pick -Xrenormalize did not work
From: Torsten Bögershausen @ 2016-12-02 12:20 UTC (permalink / raw)
To: Junio C Hamano, Jacob Keller; +Cc: Git mailing list, eevee.reply
In-Reply-To: <xmqqtwan8kjv.fsf@gitster.mtv.corp.google.com>
> Yup, this is what I queued.
>
Looks good, thanks you all.
^ permalink raw reply
* Error after calling git difftool -d with
From: P. Duijst @ 2016-12-02 12:15 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 1059 bytes --]
Dear,
Incase filenames are used with a quote ' or a bracket [ (and maybe some
more characters), git "diff" and "difftool -y" works fine, but git
*difftool **-d* gives the next error message:
peter@scm_ws_10 MINGW64 /d/Dev/test (master)
$ git diff
diff --git a/Test ''inch.txt b/Test ''inch.txt
index dbff793..41f3257 100644
--- a/Test ''inch.txt
+++ b/Test ''inch.txt
@@ -1 +1,3 @@
+
+ddd
Test error in simple repository
warning: LF will be replaced by CRLF in Test ''inch.txt.
The file will have its original line endings in your working directory.
peter@scm_ws_10 MINGW64 /d/Dev/test (master)
*$ git difftool -d*
*fatal: Cannot open '/d/Dev/test//Test ''inch.txt': No such file or
directory*
*hash-object /d/Dev/test//Test ''inch.txt: command returned error: 128*
peter@scm_ws_10 MINGW64 /d/Dev/test (master)
$
This issue is inside V2.10.x and V2.11.0.
V2.9.0 is working correctly...
I am using git for windows and beyond compare 4.0 as difftool.
Best regards,
Peter
[-- Attachment #2: IssueWithFiles.zip --]
[-- Type: application/zip, Size: 13922 bytes --]
^ permalink raw reply
* Re: [PATCH] Documentation/install-webdoc.sh: quote a potentially unsafe shell expansion
From: Austin English @ 2016-12-02 1:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <xmqqfum78jq0.fsf@gitster.mtv.corp.google.com>
On Thu, Dec 1, 2016 at 2:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Austin English <austinenglish@gmail.com> writes:
>
>> diff --git a/Documentation/install-webdoc.sh b/Documentation/install-webdoc.sh
>> index ed8b4ff..5fb2dc5 100755
>> --- a/Documentation/install-webdoc.sh
>> +++ b/Documentation/install-webdoc.sh
>> @@ -18,7 +18,7 @@ do
>> else
>> echo >&2 "# install $h $T/$h"
>> rm -f "$T/$h"
>> - mkdir -p $(dirname "$T/$h")
>> + mkdir -p "$(dirname "$T/$h")"
>> cp "$h" "$T/$h"
>> fi
>> done
>
> We know $h is safe without quoting (see what the for loop iterates
> over a list and binding each element of it to this variable), but T
> is the parameter given to this script, which comes from these
>
> install-html: html
> '$(SHELL_PATH_SQ)' ./install-webdoc.sh $(DESTDIR)$(htmldir)
>
> install-webdoc : html
> '$(SHELL_PATH_SQ)' ./install-webdoc.sh $(WEBDOC_DEST)
>
> in the Makefile. So quoting the result of $(dirname "$T/$h") is
> just as necessary as quoting the argument given to this dirname.
>
> But I do not think it is sufficient, if we are truly worried about
> people who specify a path that contains IFS whitespace in DESTDIR,
> WEBDOC_DEST, htmldir and other *dir variables used in the Makefile.
> The references to these variables, when they are mentioned on the
> command lines of Makefile actions, all need to be quoted. The
> remainder of the Makefile tells me that we decided that we are not
> worried about those people at all.
>
> So while I could take your patch as-is, I am not sure how much value
> it adds to the overall callchain that would reach the location that
> is updated by the patch. If you run
>
> make DESTDIR="/tmp/My Temporary Place" install
>
> it would still not do the right thing even with your patch, I would
> suspect.
>
> Thanks.
Hi Junio,
Thanks for reply and reviewing. Your concerns are totally valid.
Some context for the change. I wrote a wrapper script for
checkbashisms/shellcheck that I use in my project. I decided to run it
on other projects I have checked out, out of curiosity, and looked at
some of the results. This was the only one in git, so I thought it was
worth fixing. I did not test the full pipeline.
I'll look again and send a follow up patch soon.
--
-Austin
GPG: 14FB D7EA A041 937B
^ permalink raw reply
* Re: [PATCH 2/6] http: always update the base URL for redirects
From: Ramsay Jones @ 2016-12-02 1:21 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Philip Oakley, Brandon Williams, git, Jann Horn
In-Reply-To: <20161202001852.arasy44d6iczeeez@sigill.intra.peff.net>
On 02/12/16 00:18, Jeff King wrote:
> On Fri, Dec 02, 2016 at 12:07:50AM +0000, Ramsay Jones wrote:
>
>>>> In a British context "Mallory and Irvine" were two (male) climbers who
>>>> died on Everest in 1924 (tales of daring...), so it's easy to expect
>>>> (from this side of the pond) that 'Mallory' would be male. However he
>>>> was really George Mallory.
>>>>
>>>> Meanwhile that search engine's images shows far more female Mallorys,
>>>> so I've learnt something.
>>>
>>> "baby name Mallory" in search engine gave me several sites, most of
>>> them telling me that is a girl's name except for one.
>>>
>>> Didn't think of doing image search, but that's a good way ;-)
>>
>> Heh, I didn't think about any of this. I was remembering the
>> description of 'Man-in-the-middle Attack' from Applied Cryptography
>> (Bruce Schneier) which implies that Mallory is male.
>
> I admit that I always assumed Applied Cryptography (and other papers)
> were always talking about a female. But that's probably because I
> started with an assumption about the name in the first place. That
> probably came from watching the Family Ties sitcom as a kid; the older
> daughter is named Mallory (and if you google it, you can see some
> amazing 80's haircuts and clothes).
>
> We can call her Marsha if you want, instead evoking Brady Bunch memories
> of 60's clothing and haircuts.
I'm not sure it matters too much, but if you are going to
change the name then Eve is also used in the description
of Man-in-the-middle (see "Practical Cryptography", Ferguson
and Schneier).
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Jeff King @ 2016-12-02 0:13 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, bburky, jrnieder
In-Reply-To: <20161201232656.GK54082@google.com>
On Thu, Dec 01, 2016 at 03:26:56PM -0800, Brandon Williams wrote:
> > 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
^ permalink raw reply
* Re: [PATCH 2/6] http: always update the base URL for redirects
From: Jeff King @ 2016-12-02 0:18 UTC (permalink / raw)
To: Ramsay Jones
Cc: Junio C Hamano, Philip Oakley, Brandon Williams, git, Jann Horn
In-Reply-To: <7cd35131-2e0c-0dff-8864-c099e313251d@ramsayjones.plus.com>
On Fri, Dec 02, 2016 at 12:07:50AM +0000, Ramsay Jones wrote:
> >> In a British context "Mallory and Irvine" were two (male) climbers who
> >> died on Everest in 1924 (tales of daring...), so it's easy to expect
> >> (from this side of the pond) that 'Mallory' would be male. However he
> >> was really George Mallory.
> >>
> >> Meanwhile that search engine's images shows far more female Mallorys,
> >> so I've learnt something.
> >
> > "baby name Mallory" in search engine gave me several sites, most of
> > them telling me that is a girl's name except for one.
> >
> > Didn't think of doing image search, but that's a good way ;-)
>
> Heh, I didn't think about any of this. I was remembering the
> description of 'Man-in-the-middle Attack' from Applied Cryptography
> (Bruce Schneier) which implies that Mallory is male.
I admit that I always assumed Applied Cryptography (and other papers)
were always talking about a female. But that's probably because I
started with an assumption about the name in the first place. That
probably came from watching the Family Ties sitcom as a kid; the older
daughter is named Mallory (and if you google it, you can see some
amazing 80's haircuts and clothes).
We can call her Marsha if you want, instead evoking Brady Bunch memories
of 60's clothing and haircuts.
-Peff
^ permalink raw reply
* Re: [PATCH 2/6] http: always update the base URL for redirects
From: Ramsay Jones @ 2016-12-02 0:07 UTC (permalink / raw)
To: Junio C Hamano, Philip Oakley; +Cc: Brandon Williams, Jeff King, git, Jann Horn
In-Reply-To: <xmqq8trz6wrq.fsf@gitster.mtv.corp.google.com>
On 01/12/16 23:43, Junio C Hamano wrote:
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>>> Depends, I only know Mallorys who are women so her seems appropriate.
>>>
>> In a British context "Mallory and Irvine" were two (male) climbers who
>> died on Everest in 1924 (tales of daring...), so it's easy to expect
>> (from this side of the pond) that 'Mallory' would be male. However he
>> was really George Mallory.
>>
>> Meanwhile that search engine's images shows far more female Mallorys,
>> so I've learnt something.
>
> "baby name Mallory" in search engine gave me several sites, most of
> them telling me that is a girl's name except for one.
>
> Didn't think of doing image search, but that's a good way ;-)
Heh, I didn't think about any of this. I was remembering the
description of 'Man-in-the-middle Attack' from Applied Cryptography
(Bruce Schneier) which implies that Mallory is male.
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH 5/6] http: treat http-alternates like redirects
From: Jeff King @ 2016-12-02 0:06 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, Jann Horn
In-Reply-To: <20161201230223.GI54082@google.com>
On Thu, Dec 01, 2016 at 03:02:23PM -0800, Brandon Williams wrote:
> > diff --git a/http.c b/http.c
> > index 825118481..051fe6e5a 100644
> > --- a/http.c
> > +++ b/http.c
> > @@ -745,6 +745,7 @@ static CURL *get_curl_handle(void)
> > if (is_transport_allowed("ftps"))
> > allowed_protocols |= CURLPROTO_FTPS;
> > curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols);
> > + curl_easy_setopt(result, CURLOPT_PROTOCOLS, allowed_protocols);
> > #else
> > if (transport_restrict_protocols())
> > warning("protocol restrictions not applied to curl redirects because\n"
>
> Because I don't know much about how curl works....Only
> http/https/ftp/ftps protocols are allowed to be passed to curl? Is that
> because curl only understands those particular protocols?
No, curl understands more protocols, and that is exactly the problem. We
don't want to accidentally have curl access file://, smtp://, or
similar, based on what some server puts in their http-alternates file.
You should only be able to get to this code-path by calling one of
git-remote-{http,https,ftp,ftps}. So there is no problem with
restricting the protocol beyond those options. And there should be no
problem with restricting within that set; if the protocol we intend to
feed to curl had been disallowed by policy, git would have blocked it
before hitting git-remote in the first place.
-Peff
^ permalink raw reply
* [PATCH v8 4/5] http: create function to get curl allowed protocols
From: Brandon Williams @ 2016-12-02 0:01 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, peff, sbeller, bburky, jrnieder
In-Reply-To: <1480636862-40489-1-git-send-email-bmwill@google.com>
Move the creation of an allowed protocols whitelist to a helper
function.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
http.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/http.c b/http.c
index fee128b..a1c3a0e 100644
--- a/http.c
+++ b/http.c
@@ -624,11 +624,25 @@ void setup_curl_trace(CURL *handle)
curl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);
}
+static long get_curl_allowed_protocols(void)
+{
+ long allowed_protocols = 0;
+
+ 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;
+
+ return allowed_protocols;
+}
static CURL *get_curl_handle(void)
{
CURL *result = curl_easy_init();
- long allowed_protocols = 0;
if (!result)
die("curl_easy_init failed");
@@ -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());
#else
warning("protocol restrictions not applied to curl redirects because\n"
"your curl version is too old (>= 7.19.4)");
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v8 2/5] transport: add protocol policy config option
From: Brandon Williams @ 2016-12-02 0:00 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, peff, sbeller, bburky, jrnieder
In-Reply-To: <1480636862-40489-1-git-send-email-bmwill@google.com>
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 <peff@peff.net>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
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(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 27069ac..5fe50bc 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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
diff --git a/Documentation/git.txt b/Documentation/git.txt
index ab7215e..c52cec8 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -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]]
------------------------
diff --git a/git-submodule.sh b/git-submodule.sh
index a024a13..0a477b4 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -21,14 +21,10 @@ require_work_tree
wt_prefix=$(git rev-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}
-export GIT_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
+export GIT_PROTOCOL_FROM_USER
command=
branch=
diff --git a/t/lib-proto-disable.sh b/t/lib-proto-disable.sh
index be88e9a..02f49cb 100644
--- a/t/lib-proto-disable.sh
+++ b/t/lib-proto-disable.sh
@@ -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=$1
proto=$2
url=$3
@@ -62,6 +59,129 @@ test_proto () {
test_must_fail git clone --bare "$url" tmp.git
)
'
+
+ test_expect_success "clone $desc (env var has precedence)" '
+ rm -rf tmp.git &&
+ (
+ GIT_ALLOW_PROTOCOL=none &&
+ export GIT_ALLOW_PROTOCOL &&
+ test_must_fail git -c protocol.allow=always clone --bare "$url" tmp.git &&
+ test_must_fail git -c protocol.$proto.allow=always clone --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 -rf tmp.git &&
+ git -c protocol.$proto.allow=always clone --bare "$url" tmp.git
+ '
+
+ test_expect_success "fetch $desc (enabled)" '
+ git -C tmp.git -c protocol.$proto.allow=always fetch
+ '
+
+ test_expect_success "push $desc (enabled)" '
+ git -C tmp.git -c protocol.$proto.allow=always push origin HEAD:pushed
+ '
+
+ test_expect_success "push $desc (disabled)" '
+ test_must_fail git -C tmp.git -c protocol.$proto.allow=never push origin HEAD:pushed
+ '
+
+ test_expect_success "fetch $desc (disabled)" '
+ test_must_fail git -C tmp.git -c protocol.$proto.allow=never fetch
+ '
+
+ test_expect_success "clone $desc (disabled)" '
+ rm -rf tmp.git &&
+ test_must_fail git -c protocol.$proto.allow=never clone --bare "$url" tmp.git
+ '
+
+ # Test clone/fetch/push with protocol.user.allow and its env var
+ test_expect_success "clone $desc (enabled)" '
+ rm -rf tmp.git &&
+ git -c protocol.$proto.allow=user clone --bare "$url" tmp.git
+ '
+
+ test_expect_success "fetch $desc (enabled)" '
+ git -C tmp.git -c protocol.$proto.allow=user fetch
+ '
+
+ test_expect_success "push $desc (enabled)" '
+ git -C tmp.git -c protocol.$proto.allow=user push origin HEAD:pushed
+ '
+
+ test_expect_success "push $desc (disabled)" '
+ (
+ cd tmp.git &&
+ GIT_PROTOCOL_FROM_USER=0 &&
+ export GIT_PROTOCOL_FROM_USER &&
+ test_must_fail git -c protocol.$proto.allow=user push origin HEAD:pushed
+ )
+ '
+
+ test_expect_success "fetch $desc (disabled)" '
+ (
+ cd tmp.git &&
+ GIT_PROTOCOL_FROM_USER=0 &&
+ export GIT_PROTOCOL_FROM_USER &&
+ test_must_fail git -c protocol.$proto.allow=user fetch
+ )
+ '
+
+ test_expect_success "clone $desc (disabled)" '
+ rm -rf tmp.git &&
+ (
+ GIT_PROTOCOL_FROM_USER=0 &&
+ export GIT_PROTOCOL_FROM_USER &&
+ test_must_fail git -c protocol.$proto.allow=user clone --bare "$url" tmp.git
+ )
+ '
+
+ # Test clone/fetch/push with protocol.allow user defined default
+ test_expect_success "clone $desc (enabled)" '
+ rm -rf tmp.git &&
+ git config --global protocol.allow always &&
+ git clone --bare "$url" tmp.git
+ '
+
+ test_expect_success "fetch $desc (enabled)" '
+ git -C tmp.git fetch
+ '
+
+ test_expect_success "push $desc (enabled)" '
+ git -C tmp.git push origin HEAD:pushed
+ '
+
+ test_expect_success "push $desc (disabled)" '
+ git config --global protocol.allow never &&
+ test_must_fail git -C tmp.git push origin HEAD:pushed
+ '
+
+ test_expect_success "fetch $desc (disabled)" '
+ test_must_fail git -C tmp.git fetch
+ '
+
+ test_expect_success "clone $desc (disabled)" '
+ rm -rf tmp.git &&
+ test_must_fail git clone --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
diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh
index bc44ac3..75c570a 100755
--- a/t/t5509-fetch-push-namespaces.sh
+++ b/t/t5509-fetch-push-namespaces.sh
@@ -4,6 +4,7 @@ test_description='fetch/push involving ref namespaces'
. ./test-lib.sh
test_expect_success setup '
+ git config --global protocol.ext.allow user &&
test_tick &&
git init original &&
(
diff --git a/t/t5802-connect-helper.sh b/t/t5802-connect-helper.sh
index b7a7f9d..c6c2661 100755
--- a/t/t5802-connect-helper.sh
+++ b/t/t5802-connect-helper.sh
@@ -4,6 +4,7 @@ test_description='ext::cmd remote "connect" helper'
. ./test-lib.sh
test_expect_success setup '
+ git config --global protocol.ext.allow user &&
test_tick &&
git commit --allow-empty -m initial &&
test_tick &&
diff --git a/transport.c b/transport.c
index d57e8de..2c0ec76 100644
--- a/transport.c
+++ b/transport.c
@@ -664,10 +664,81 @@ static const struct string_list *protocol_whitelist(void)
return enabled ? &allowed : NULL;
}
+enum protocol_allow_config {
+ PROTOCOL_ALLOW_NEVER = 0,
+ PROTOCOL_ALLOW_USER_ONLY,
+ PROTOCOL_ALLOW_ALWAYS
+};
+
+static enum protocol_allow_config parse_protocol_config(const char *key,
+ const char *value)
+{
+ if (!strcasecmp(value, "always"))
+ return PROTOCOL_ALLOW_ALWAYS;
+ else if (!strcasecmp(value, "never"))
+ return PROTOCOL_ALLOW_NEVER;
+ else if (!strcasecmp(value, "user"))
+ return PROTOCOL_ALLOW_USER_ONLY;
+
+ die("unknown value for config '%s': %s", key, value);
+}
+
+static enum protocol_allow_config get_protocol_config(const char *type)
+{
+ char *key = xstrfmt("protocol.%s.allow", type);
+ char *value;
+
+ /* first check the per-protocol config */
+ 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, fallback to user-defined 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;
+ }
+
+ /* fallback to built-in defaults */
+ /* known safe */
+ if (!strcmp(type, "http") ||
+ !strcmp(type, "https") ||
+ !strcmp(type, "git") ||
+ !strcmp(type, "ssh") ||
+ !strcmp(type, "file"))
+ return PROTOCOL_ALLOW_ALWAYS;
+
+ /* known scary; err on the side of caution */
+ if (!strcmp(type, "ext"))
+ return PROTOCOL_ALLOW_NEVER;
+
+ /* unknown; by default 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);
+ }
+
+ die("BUG: invalid protocol_allow_config type");
}
void transport_check_allowed(const char *type)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v8 5/5] transport: add from_user parameter to is_transport_allowed
From: Brandon Williams @ 2016-12-02 0:01 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, peff, sbeller, bburky, jrnieder
In-Reply-To: <1480636862-40489-1-git-send-email-bmwill@google.com>
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 <bmwill@google.com>
---
http.c | 12 ++++++------
transport.c | 8 +++++---
transport.h | 13 ++++++++++---
3 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/http.c b/http.c
index a1c3a0e..2a02941 100644
--- a/http.c
+++ b/http.c
@@ -624,17 +624,17 @@ void setup_curl_trace(CURL *handle)
curl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);
}
-static long get_curl_allowed_protocols(void)
+static long get_curl_allowed_protocols(int from_user)
{
long allowed_protocols = 0;
- if (is_transport_allowed("http"))
+ if (is_transport_allowed("http", from_user))
allowed_protocols |= CURLPROTO_HTTP;
- if (is_transport_allowed("https"))
+ if (is_transport_allowed("https", from_user))
allowed_protocols |= CURLPROTO_HTTPS;
- if (is_transport_allowed("ftp"))
+ if (is_transport_allowed("ftp", from_user))
allowed_protocols |= CURLPROTO_FTP;
- if (is_transport_allowed("ftps"))
+ if (is_transport_allowed("ftps", from_user))
allowed_protocols |= CURLPROTO_FTPS;
return allowed_protocols;
@@ -740,7 +740,7 @@ static CURL *get_curl_handle(void)
#endif
#if LIBCURL_VERSION_NUM >= 0x071304
curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
- get_curl_allowed_protocols());
+ get_curl_allowed_protocols(0));
#else
warning("protocol restrictions not applied to curl redirects because\n"
"your curl version is too old (>= 7.19.4)");
diff --git a/transport.c b/transport.c
index 186de9a..8a3597b 100644
--- a/transport.c
+++ b/transport.c
@@ -723,7 +723,7 @@ static enum protocol_allow_config get_protocol_config(const char *type)
return PROTOCOL_ALLOW_USER_ONLY;
}
-int is_transport_allowed(const char *type)
+int is_transport_allowed(const char *type, int from_user)
{
const struct string_list *whitelist = protocol_whitelist();
if (whitelist)
@@ -735,7 +735,9 @@ 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);
+ if (from_user < 0)
+ from_user = git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
+ return from_user;
}
die("BUG: invalid protocol_allow_config type");
@@ -743,7 +745,7 @@ int is_transport_allowed(const char *type)
void transport_check_allowed(const char *type)
{
- if (!is_transport_allowed(type))
+ if (!is_transport_allowed(type, -1))
die("transport '%s' not allowed", type);
}
diff --git a/transport.h b/transport.h
index f4998bc..9820f10 100644
--- a/transport.h
+++ b/transport.h
@@ -153,10 +153,17 @@ extern int transport_summary_width(const struct ref *refs);
struct transport *transport_get(struct remote *, const char *);
/*
- * Check whether a transport is allowed by the environment. Type should
- * generally be the URL scheme, as described in Documentation/git.txt
+ * Check whether a transport is allowed by the environment.
+ *
+ * Type should generally be the URL scheme, as described in
+ * Documentation/git.txt
+ *
+ * from_user specifies if the transport was given by the user. If unknown pass
+ * a -1 to read from the environment to determine if the transport was given by
+ * the user.
+ *
*/
-int is_transport_allowed(const char *type);
+int is_transport_allowed(const char *type, int from_user);
/*
* Check whether a transport is allowed by the environment,
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v8 3/5] http: always warn if libcurl version is too old
From: Brandon Williams @ 2016-12-02 0:01 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, peff, sbeller, bburky, jrnieder
In-Reply-To: <1480636862-40489-1-git-send-email-bmwill@google.com>
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 <bmwill@google.com>
---
http.c | 5 ++---
transport.c | 5 -----
transport.h | 6 ------
3 files changed, 2 insertions(+), 14 deletions(-)
diff --git a/http.c b/http.c
index 4c4a812..fee128b 100644
--- a/http.c
+++ b/http.c
@@ -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)");
#endif
if (getenv("GIT_CURL_VERBOSE"))
curl_easy_setopt(result, CURLOPT_VERBOSE, 1L);
diff --git a/transport.c b/transport.c
index 2c0ec76..186de9a 100644
--- a/transport.c
+++ b/transport.c
@@ -747,11 +747,6 @@ void transport_check_allowed(const char *type)
die("transport '%s' not allowed", type);
}
-int transport_restrict_protocols(void)
-{
- return !!protocol_whitelist();
-}
-
struct transport *transport_get(struct remote *remote, const char *url)
{
const char *helper;
diff --git a/transport.h b/transport.h
index b8e4ee8..f4998bc 100644
--- a/transport.h
+++ b/transport.h
@@ -164,12 +164,6 @@ int is_transport_allowed(const char *type);
*/
void transport_check_allowed(const char *type);
-/*
- * Returns true if the user has attempted to turn on protocol
- * restrictions at all.
- */
-int transport_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 */
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v8 1/5] lib-proto-disable: variable name fix
From: Brandon Williams @ 2016-12-02 0:00 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, peff, sbeller, bburky, jrnieder
In-Reply-To: <1480636862-40489-1-git-send-email-bmwill@google.com>
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 <bmwill@google.com>
---
t/lib-proto-disable.sh | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/t/lib-proto-disable.sh b/t/lib-proto-disable.sh
index b0917d9..be88e9a 100644
--- a/t/lib-proto-disable.sh
+++ b/t/lib-proto-disable.sh
@@ -9,7 +9,7 @@ test_proto () {
proto=$2
url=$3
- test_expect_success "clone $1 (enabled)" '
+ test_expect_success "clone $desc (enabled)" '
rm -rf tmp.git &&
(
GIT_ALLOW_PROTOCOL=$proto &&
@@ -18,7 +18,7 @@ test_proto () {
)
'
- test_expect_success "fetch $1 (enabled)" '
+ test_expect_success "fetch $desc (enabled)" '
(
cd tmp.git &&
GIT_ALLOW_PROTOCOL=$proto &&
@@ -27,7 +27,7 @@ test_proto () {
)
'
- test_expect_success "push $1 (enabled)" '
+ test_expect_success "push $desc (enabled)" '
(
cd tmp.git &&
GIT_ALLOW_PROTOCOL=$proto &&
@@ -36,7 +36,7 @@ test_proto () {
)
'
- test_expect_success "push $1 (disabled)" '
+ test_expect_success "push $desc (disabled)" '
(
cd tmp.git &&
GIT_ALLOW_PROTOCOL=none &&
@@ -45,7 +45,7 @@ test_proto () {
)
'
- test_expect_success "fetch $1 (disabled)" '
+ test_expect_success "fetch $desc (disabled)" '
(
cd tmp.git &&
GIT_ALLOW_PROTOCOL=none &&
@@ -54,7 +54,7 @@ test_proto () {
)
'
- test_expect_success "clone $1 (disabled)" '
+ test_expect_success "clone $desc (disabled)" '
rm -rf tmp.git &&
(
GIT_ALLOW_PROTOCOL=none &&
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v8 0/5] transport protocol policy configuration
From: Brandon Williams @ 2016-12-02 0:00 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, peff, sbeller, bburky, jrnieder
In-Reply-To: <1480623959-126129-1-git-send-email-bmwill@google.com>
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
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Brandon Williams @ 2016-12-01 23:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, sbeller, bburky, jrnieder
In-Reply-To: <xmqqh96n6x63.fsf@gitster.mtv.corp.google.com>
On 12/01, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> > 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.
>
> OK. With both of them queued, t5812 seems to barf, just FYI; I'll
> expect that a future reroll would make things better.
Yeah I expected we would see an issue since both these series collide in
http.c
I'm sending out another reroll of this series so that in Jeff's he can
just call 'get_curl_allowed_protocols(-1)' for the non-redirection curl
option, which should make this test stop barfing.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 2/6] http: always update the base URL for redirects
From: Junio C Hamano @ 2016-12-01 23:43 UTC (permalink / raw)
To: Philip Oakley; +Cc: Brandon Williams, Ramsay Jones, Jeff King, git, Jann Horn
In-Reply-To: <2297C36B9A1441748D7E68363A05F8C5@PhilipOakley>
"Philip Oakley" <philipoakley@iee.org> writes:
>> Depends, I only know Mallorys who are women so her seems appropriate.
>>
> In a British context "Mallory and Irvine" were two (male) climbers who
> died on Everest in 1924 (tales of daring...), so it's easy to expect
> (from this side of the pond) that 'Mallory' would be male. However he
> was really George Mallory.
>
> Meanwhile that search engine's images shows far more female Mallorys,
> so I've learnt something.
"baby name Mallory" in search engine gave me several sites, most of
them telling me that is a girl's name except for one.
Didn't think of doing image search, but that's a good way ;-)
^ permalink raw reply
* Re: [PATCH 1/3] compat: add qsort_s()
From: Junio C Hamano @ 2016-12-01 23:37 UTC (permalink / raw)
To: René Scharfe; +Cc: Jeff King, Git List, Johannes Schindelin
In-Reply-To: <955e9bf4-d1cd-f01a-13f1-7a335dea011a@web.de>
René Scharfe <l.s.r@web.de> writes:
>> You can hack around it by passing a wrapper callback that flips the
>> arguments. Since we have a "void *" data pointer, that would point to a
>> struct holding the "real" callback and chaining to the original data
>> pointer.
>>
>> It does incur the cost of an extra level of indirection for each
>> comparison, though (not just for each qsort call).
>
> Indeed. We'd need a perf test to measure that overhead before we
> could determine if that's a problem, though.
I agree. Hopefully it won't be too much cost.
>> You could do it as zero-cost if you were willing to turn the comparison
>> function definition into a macro.
>
> Ugh. That either requires changing the signature of qsort_s() based
> on the underlying native function as well, or using a void pointer to
> pass the comparison function, no? Let's not do that, at least not
> without a good reason.
Let's not go there. It may be zero runtime cost, but the cognitive
cost for people who need to code the comparison callback using the
macro is high.
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Junio C Hamano @ 2016-12-01 23:34 UTC (permalink / raw)
To: Brandon Williams; +Cc: Jeff King, git, sbeller, bburky, jrnieder
In-Reply-To: <20161201230738.GJ54082@google.com>
Brandon Williams <bmwill@google.com> writes:
> 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.
OK. With both of them queued, t5812 seems to barf, just FYI; I'll
expect that a future reroll would make things better.
Thanks.
^ permalink raw reply
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Junio C Hamano @ 2016-12-01 23:33 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jeff King, git, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <alpine.DEB.2.20.1611301325210.117539@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> The config kinda works now. But for what price. It stole 4 hours I did not
> have. When the libexec/git-core/use-builtin-difftool solution took me a
> grand total of half an hour to devise, implement and test.
>
> And you know what? I still do not really see what is so bad about it.
I was wondering if I should explain myself again, even though I do
not see what good it would do, as clearly my point did not come
across in the other emails. And then you would just complain that I
am making work for you. Clearly you do not seem to see why placing
random files in $GIT_EXEC_PATH, which is a place for git subcommand
implementations, is wrong, so I won't repeat it to you again.
But you need to remember that you are not working on a Windows-only
project. In non-Windows environment, many users would not have
write access to /usr/libexec/git-core directory, but it is not just
easy for them to write into ~/.gitconfig, but that is the way they
are accustomed to, in order to affect the behaviour of Git for them.
As to "I have to spawn config", I think it is sensible to start the
cmd_difftool() wrapper without adding RUN_SETUP to the command
table, then call git_config_get_bool() to check the configuration
only from system and per-user files, and then finally either call
into builtin_difftool() where setup_git_directory() is called, or
spawn the scripted difftool, as Peff already said. Your "users
opt-in while installing" is not about setting per-repository option.
Calling git_config*(), setup_git_directory() and then git_config*()
in this order should be safe, as setup_git_directory() would clear
potentially cached configuration values read by any previous
git_config*() calls, so any configuration enquiry made by
builtin_difftool() would read from all three sources, not just
system and per-user.
So there is no chicken-and-egg issue, either.
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Brandon Williams @ 2016-12-01 23:26 UTC (permalink / raw)
To: Jeff King; +Cc: git, sbeller, bburky, jrnieder
In-Reply-To: <20161201230738.GJ54082@google.com>
On 12/01, Brandon Williams wrote:
> On 12/01, Jeff King wrote:
> > 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').
> >
> > > diff --git a/http.c b/http.c
> > > index fee128b..e74c0f0 100644
> > > --- a/http.c
> > > +++ b/http.c
> > > @@ -725,13 +725,13 @@ static CURL *get_curl_handle(void)
> > > curl_easy_setopt(result, CURLOPT_POST301, 1);
> > > #endif
> > > #if LIBCURL_VERSION_NUM >= 0x071304
> > > - if (is_transport_allowed("http"))
> > > + if (is_transport_allowed("http", 0))
> > > allowed_protocols |= CURLPROTO_HTTP;
> > > - if (is_transport_allowed("https"))
> > > + if (is_transport_allowed("https", 0))
> > > allowed_protocols |= CURLPROTO_HTTPS;
> > > - if (is_transport_allowed("ftp"))
> > > + if (is_transport_allowed("ftp", 0))
> > > allowed_protocols |= CURLPROTO_FTP;
> > > - if (is_transport_allowed("ftps"))
> > > + if (is_transport_allowed("ftps", 0))
> > > allowed_protocols |= CURLPROTO_FTPS;
> > > curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols);
> > > #else
> >
> > 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
^ permalink raw reply
* Re: [PATCH 2/6] http: always update the base URL for redirects
From: Philip Oakley @ 2016-12-01 23:12 UTC (permalink / raw)
To: Brandon Williams, Ramsay Jones; +Cc: Jeff King, git, Jann Horn
In-Reply-To: <20161201225331.GH54082@google.com>
From: "Brandon Williams" <bmwill@google.com>
> On 12/01, Ramsay Jones wrote:
>>
>>
>> On 01/12/16 09:04, Jeff King wrote:
>> > If a malicious server redirects the initial ref
>> > advertisement, it may be able to leak sha1s from other,
>> > unrelated servers that the client has access to. For
>> > example, imagine that Alice is a git user, she has access to
>> > a private repository on a server hosted by Bob, and Mallory
>> > runs a malicious server and wants to find out about Bob's
>> > private repository.
>> >
>> > Mallory asks Alice to clone an unrelated repository from her
>> -----------------------------------------------------------^^^
>> ... from _him_ ? (ie Mallory)
>>
>> > over HTTP. When Alice's client contacts Mallory's server for
>> > the initial ref advertisement, the server issues an HTTP
>> > redirect for Bob's server. Alice contacts Bob's server and
>> > gets the ref advertisement for the private repository. If
>> > there is anything to fetch, she then follows up by asking
>> > the server for one or more sha1 objects. But who is the
>> > server?
>> >
>> > If it is still Mallory's server, then Alice will leak the
>> > existence of those sha1s to her.
>> ------------------------------^^^
>> ... to _him_ ? (again Mallory)
>>
>> ATB,
>> Ramsay Jones
>
> Depends, I only know Mallorys who are women so her seems appropriate.
>
> --
> Brandon Williams
>
In a British context "Mallory and Irvine" were two (male) climbers who died
on Everest in 1924 (tales of daring...), so it's easy to expect (from this
side of the pond) that 'Mallory' would be male. However he was really George
Mallory.
Meanwhile that search engine's images shows far more female Mallorys, so
I've learnt something.
--
Philip
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox