* Re: [PATCH v5 03/19] builtin/diff-tree: convert to struct object_id
From: Junio C Hamano @ 2017-02-22 20:04 UTC (permalink / raw)
To: Jeff King; +Cc: brian m. carlson, git, Michael Haggerty, Ramsay Jones
In-Reply-To: <20170222191641.o2rtt2ymtb4h2yqe@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Feb 22, 2017 at 10:50:21AM -0800, Junio C Hamano wrote:
>
>> What the function does appears somewhat iffy in the modern world.
>> We are relying on the fact that while Git is operating in this mode
>> of reading a tuple of commits per line and doing log-tree, that Git
>> itself will not do the history traversal (instead, another instance
>> of Git that feeds us via our standard input is walking the history)
>> for this piece of code to work correctly.
>>
>> Of course, the "diff-tree --stdin" command was meant to sit on the
>> downstream of "rev-list --parents", so the assumption the code makes
>> (i.e. the parents field of the in-core commit objects do not have to
>> be usable for history traversal) may be reasonable, but still...
>
> I'm not sure it's that weird. "diff-tree" is about diffing, not
> traversal. The only reason it touches commit->parents at all (and
> doesn't just kick off a diff between the arguments it gets) is that it's
> been stuck with pretty-printing the commits, which might ask to show the
> parents.
Yeah, I understand all that as 45392a648d ("git-diff-tree --stdin:
show all parents.", 2006-02-05) was mostly mine. It's just I sense
that the recent trend is to take whatever existing code and see if
they are reusable in other contexts, and this is one of the things
that people might want to libify, but cannot be as-is.
^ permalink raw reply
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: Junio C Hamano @ 2017-02-22 20:19 UTC (permalink / raw)
To: David Turner; +Cc: git, sandals, Johannes Schindelin, Eric Sunshine, Jeff King
In-Reply-To: <20170222173936.25016-1-dturner@twosigma.com>
David Turner <dturner@twosigma.com> writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> It is common in corporate setups to have permissions managed via a
> domain account. That means that the user does not really have to log in
> when accessing a central repository via https://, but that the login
> credentials are used to authenticate with that repository.
>
> The common way to do that used to require empty credentials, i.e. hitting
> Enter twice when being asked for user name and password, or by using the
> very funny notation https://:@server/repository
>
> A recent commit (5275c3081c (http: http.emptyauth should allow empty (not
> just NULL) usernames, 2016-10-04)) broke that usage, though, all of a
> sudden requiring users to set http.emptyAuth = true.
>
> Which brings us to the bigger question why http.emptyAuth defaults to
> false, to begin with.
This is a valid question, and and I do not see it explicitly asked
in the thread:
https://public-inbox.org/git/CAPig+cSphEu3iRJrkdBA+BRhi9HnopLJnKOHVuGhUqavtV1RXg@mail.gmail.com/#t
even though there is a hint of it already there.
> It would be one thing if cURL would not let the user specify credentials
> interactively after attempting NTLM authentication (i.e. login
> credentials), but that is not the case.
>
> It would be another thing if attempting NTLM authentication was not
> usually what users need to do when trying to authenticate via https://.
> But that is also not the case.
Some other possible worries we may have had I can think of are:
- With this enabled unconditionally, would we leak some information?
- With this enabled unconditionally, would we always incur an extra
roundtrip for people who are not running NTLM at all?
I do not think the former is the case, but what would I know (adding a
few people involved in the original thread to CC: ;-)
> Documentation/config.txt | 3 ++-
> http.c | 2 +-
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index fc5a28a320..b0da64ed33 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1742,7 +1742,8 @@ http.emptyAuth::
> Attempt authentication without seeking a username or password. This
> can be used to attempt GSS-Negotiate authentication without specifying
> a username in the URL, as libcurl normally requires a username for
> - authentication.
> + authentication. Default is true, since if this fails, git will fall
> + back to asking the user for their username/password.
>
> http.delegation::
> Control GSSAPI credential delegation. The delegation is disabled
> diff --git a/http.c b/http.c
> index 90a1c0f113..943e630ea6 100644
> --- a/http.c
> +++ b/http.c
> @@ -109,7 +109,7 @@ static int curl_save_cookies;
> struct credential http_auth = CREDENTIAL_INIT;
> static int http_proactive_auth;
> static const char *user_agent;
> -static int curl_empty_auth;
> +static int curl_empty_auth = 1;
>
> enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;
^ permalink raw reply
* Re: [PATCH] Documentation: Link git-ls-files to core.quotePath variable.
From: Philip Oakley @ 2017-02-22 12:21 UTC (permalink / raw)
To: Junio C Hamano, Andreas Heiduk; +Cc: git
In-Reply-To: <e55b3cb7-65bf-1609-2e8d-823b4336e07a@gmail.com>
From: "Andreas Heiduk" <asheiduk@gmail.com>
> [PATCH] Documentation: Clarify core.quotePath, remove cruft in
> git-ls-files.
>
> Signed-off-by: Andreas Heiduk <asheiduk@gmail.com>
> ---
>
> I have merged the best parts about quoting into the core.quotePath
> description and cleaned up the text in git-ls-files.txt regarding the
> control characters.
>
>
> Documentation/config.txt | 22 ++++++++++++----------
> Documentation/git-ls-files.txt | 11 ++++++-----
> 2 files changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index f4721a0..25e65ae 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -340,16 +340,18 @@ core.checkStat::
> all fields, including the sub-second part of mtime and ctime.
>
> core.quotePath::
> - The commands that output paths (e.g. 'ls-files',
> - 'diff'), when not given the `-z` option, will quote
> - "unusual" characters in the pathname by enclosing the
> - pathname in a double-quote pair and with backslashes the
> - same way strings in C source code are quoted. If this
> - variable is set to false, the bytes higher than 0x80 are
> - not quoted but output as verbatim. Note that double
> - quote, backslash and control characters are always
> - quoted without `-z` regardless of the setting of this
> - variable.
> + Commands that output paths (e.g. 'ls-files', 'diff'), will
> + quote "unusual" characters in the pathname by enclosing the
> + pathname in double-quotes and escaping those characters with
> + backslashes in the same way C escapes control characters (e.g.
> + `\t` for TAB, `\n` for LF, `\\` for backslash) or bytes with
> + values larger than 0x80 (e.g. octal `\265` for "micro"). If
> + this variable is set to false, bytes higher than 0x80 are not
> + considered "unusual" any more. Double-quotes, backslash and
> + control characters are always escaped regardless of the
> + setting of this variable. Many commands can output pathnames
> + completely verbatim using the `-z` option. The default value is
> + true.
>
> core.eol::
> Sets the line ending type to use in the working directory for
> diff --git a/Documentation/git-ls-files.txt
> b/Documentation/git-ls-files.txt
> index d2b17f2..88df561 100644
> --- a/Documentation/git-ls-files.txt
> +++ b/Documentation/git-ls-files.txt
> @@ -76,7 +76,8 @@ OPTIONS
> succeed.
>
> -z::
> - \0 line termination on output.
> + \0 line termination on output and do not quote filenames.
> + See OUTPUT below for more information.
>
> -x <pattern>::
> --exclude=<pattern>::
> @@ -192,10 +193,10 @@ the index records up to three such pairs; one from
> tree O in stage
> the user (or the porcelain) to see what should eventually be recorded
> at the
> path. (see linkgit:git-read-tree[1] for more information on state)
>
> -When `-z` option is not used, TAB, LF, and backslash characters
> -in pathnames are represented as `\t`, `\n`, and `\\`,
> -respectively. The path is also quoted according to the
> -configuration variable `core.quotePath` (see linkgit:git-config[1]).
> +Without the `-z` option pathnamens with "unusual" characters are
s/option pathnamens/option, pathnames/ # comma and spelling.
> +quoted as explained for the configuration variable `core.quotePath`
> +(see linkgit:git-config[1]). Using `-z` the filename is output
> +verbatim and the line is terminated by a NUL byte.
>
--
Philip
^ permalink raw reply
* Re: [PATCH] submodule init: warn about falling back to a local path
From: Philip Oakley @ 2017-02-22 15:01 UTC (permalink / raw)
To: Stefan Beller, jrnieder; +Cc: git, gitster, sop, Stefan Beller
In-Reply-To: <20170221231815.4123-1-sbeller@google.com>
From: "Stefan Beller" <sbeller@google.com>
> When a submodule is initialized, the config variable
> 'submodule.<name>.url'
> is set depending on the value of the same variable in the .gitmodules
> file. When the URL indicates to be relative, then the url is computed
> relative to its default remote. The default remote cannot be determined
> accurately in all cases, such that it falls back to 'origin'.
>
> The 'origin' remote may not exist, though. In that case we give up looking
> for a suitable remote and we'll just assume it to be a local relative
> path.
>
> This can be confusing to users as there is a lot of guessing involved,
> which is not obvious to the user.
Would a note in the user docs about the fallback you list above also be
useful?
>
> So in the corner case of assuming a local autoritative truth, warn the
> user to lessen the confusion.
>
> This behavior was introduced in 4d6893200 (submodule add: allow relative
> repository path even when no url is set, 2011-06-06), which shared the
> code with submodule-init and then ported to C in 3604242f080a (submodule:
> port init from shell to C, 2016-04-15).
>
> In case of submodule-add, this behavior makes sense in some use cases[1],
> however for submodule-init there does not seem to be an immediate obvious
> use case to fall back to a local submodule. However there might be, so
> warn instead of die here.
>
> [1] e.g.
> http://stackoverflow.com/questions/8721984/git-ignore-files-for-public-repository-but-not-for-private
> "store a secret locally in a submodule, with no intention to publish it"
>
> Reported-by: Shawn Pearce <spearce@spearce.org>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> builtin/submodule--helper.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
--
Philip
^ permalink raw reply
* Re: [PATCH] git add -i: replace \t with blanks in the help message
From: Junio C Hamano @ 2017-02-22 20:50 UTC (permalink / raw)
To: Ralf Thielow; +Cc: git, Jiang Xin
In-Reply-To: <20170222184627.3811-1-ralf.thielow@gmail.com>
Ralf Thielow <ralf.thielow@gmail.com> writes:
> Within the help message of 'git add -i', the 'diff' command uses one
> tab character and blanks to create the space between the name and the
> description while the others use blanks only. So if the tab size is
> not at 4 characters, this description will not be in range.
> Replace the tab character with blanks.
>
> Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
> ---
> git-add--interactive.perl | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Thanks, this was me being sloppy in the very first version.
Will queue.
>
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index cf6fc926a..982593c89 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -1678,7 +1678,7 @@ status - show paths with changes
> update - add working tree state to the staged set of changes
> revert - revert staged set of changes back to the HEAD version
> patch - pick hunks and update selectively
> -diff - view diff between HEAD and index
> +diff - view diff between HEAD and index
> add untracked - add contents of untracked files to the staged set of changes
> EOF
> }
^ permalink raw reply
* Re: [PATCH] Documentation: Link git-ls-files to core.quotePath variable.
From: Junio C Hamano @ 2017-02-22 21:02 UTC (permalink / raw)
To: Philip Oakley; +Cc: Andreas Heiduk, git
In-Reply-To: <F71515D0E29940CA9C421D18480AD726@PhilipOakley>
"Philip Oakley" <philipoakley@iee.org> writes:
>> -When `-z` option is not used, TAB, LF, and backslash characters
>> -in pathnames are represented as `\t`, `\n`, and `\\`,
>> -respectively. The path is also quoted according to the
>> -configuration variable `core.quotePath` (see linkgit:git-config[1]).
>> +Without the `-z` option pathnamens with "unusual" characters are
>
> s/option pathnamens/option, pathnames/ # comma and spelling.
Thanks. Will squash it in while queuing (I need to discard the new
text added in the previous attempt in the preimage to make it apply,
too).
-- >8 --
From: Andreas Heiduk <asheiduk@gmail.com>
Date: Wed, 22 Feb 2017 02:38:21 +0100
Subject: [PATCH] Documentation: clarify core.quotePath and update git-ls-files doc
Signed-off-by: Andreas Heiduk <asheiduk@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/config.txt | 22 ++++++++++++----------
Documentation/git-ls-files.txt | 10 ++++++----
2 files changed, 18 insertions(+), 14 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index f4721a048b..25e65aeb01 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -340,16 +340,18 @@ core.checkStat::
all fields, including the sub-second part of mtime and ctime.
core.quotePath::
- The commands that output paths (e.g. 'ls-files',
- 'diff'), when not given the `-z` option, will quote
- "unusual" characters in the pathname by enclosing the
- pathname in a double-quote pair and with backslashes the
- same way strings in C source code are quoted. If this
- variable is set to false, the bytes higher than 0x80 are
- not quoted but output as verbatim. Note that double
- quote, backslash and control characters are always
- quoted without `-z` regardless of the setting of this
- variable.
+ Commands that output paths (e.g. 'ls-files', 'diff'), will
+ quote "unusual" characters in the pathname by enclosing the
+ pathname in double-quotes and escaping those characters with
+ backslashes in the same way C escapes control characters (e.g.
+ `\t` for TAB, `\n` for LF, `\\` for backslash) or bytes with
+ values larger than 0x80 (e.g. octal `\265` for "micro"). If
+ this variable is set to false, bytes higher than 0x80 are not
+ considered "unusual" any more. Double-quotes, backslash and
+ control characters are always escaped regardless of the
+ setting of this variable. Many commands can output pathnames
+ completely verbatim using the `-z` option. The default value is
+ true.
core.eol::
Sets the line ending type to use in the working directory for
diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 078b556665..a415223b45 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -76,7 +76,8 @@ OPTIONS
succeed.
-z::
- \0 line termination on output.
+ \0 line termination on output and do not quote filenames.
+ See OUTPUT below for more information.
-x <pattern>::
--exclude=<pattern>::
@@ -192,9 +193,10 @@ the index records up to three such pairs; one from tree O in stage
the user (or the porcelain) to see what should eventually be recorded at the
path. (see linkgit:git-read-tree[1] for more information on state)
-When `-z` option is not used, TAB, LF, and backslash characters
-in pathnames are represented as `\t`, `\n`, and `\\`,
-respectively.
+Without the `-z` option, pathnames with "unusual" characters are
+quoted as explained for the configuration variable `core.quotePath`
+(see linkgit:git-config[1]). Using `-z` the filename is output
+verbatim and the line is terminated by a NUL byte.
Exclude Patterns
--
2.12.0-rc2-250-gd33575c7f2
^ permalink raw reply related
* Re: [PATCH v5 00/24] Remove submodule from files-backend.c
From: Junio C Hamano @ 2017-02-22 21:04 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Michael Haggerty, Johannes Schindelin, Ramsay Jones,
Stefan Beller, novalis
In-Reply-To: <xmqqlgsycfeu.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> v5 goes a bit longer than v4, basically:
>>
>> - files_path() is broken down into three smaller functions,
>> files_{packed_refs,reflog,refname}_path().
>>
>> - most of store-based api is added because..
>>
>> - test-ref-store.c is added with t1405 and t1406 for some basic tests
>> I'm not aimimg for complete ref store coverage. But we can continue
>> to improve from there.
>>
>> - refs_store_init() now takes a "permission" flag, like open().
>> Operations are allowed or forbidden based on this flag. The
>> submodule_allowed flag is killed. files_assert_main.. remains.
>>
>> - get_*_ref_store() remain public api because it's used by
>> test-ref-store.c and pack-refs.c.
>>
>> - files-backend.c should now make no function calls that implicitly
>> target the main store. But this will have to be tested more to be
>> sure. I'm tempted to add a tracing backend just for this purpose.
>
> OK.
>
>> Junio, if you take this on 'pu', you'll have to kick my other two
>> series out (they should not even compile). I'm not resending them
>> until I get a "looks mostly ok" from Michael. No point in updating
>> them when this series keeps moving.
>
> Thanks for a note.
As this round seems to have added conflicts with another topic that
is already in flight that the previous round did not conflict with,
I'll eject all three for now until this one solidifies a bit more.
^ permalink raw reply
* RE: [PATCH] http(s): automatically try NTLM authentication first
From: David Turner @ 2017-02-22 21:04 UTC (permalink / raw)
To: 'Junio C Hamano'
Cc: git@vger.kernel.org, sandals@crustytoothpaste.net,
Johannes Schindelin, Eric Sunshine, Jeff King
In-Reply-To: <xmqqpoiaasgj.fsf@gitster.mtv.corp.google.com>
> -----Original Message-----
> From: Junio C Hamano [mailto:jch2355@gmail.com] On Behalf Of Junio C
> Hamano
> Sent: Wednesday, February 22, 2017 3:20 PM
> To: David Turner <David.Turner@twosigma.com>
> Cc: git@vger.kernel.org; sandals@crustytoothpaste.net; Johannes Schindelin
> <johannes.schindelin@gmx.de>; Eric Sunshine
> <sunshine@sunshineco.com>; Jeff King <peff@peff.net>
> Subject: Re: [PATCH] http(s): automatically try NTLM authentication first
>
> David Turner <dturner@twosigma.com> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > It is common in corporate setups to have permissions managed via a
> > domain account. That means that the user does not really have to log
> > in when accessing a central repository via https://, but that the
> > login credentials are used to authenticate with that repository.
> >
> > The common way to do that used to require empty credentials, i.e.
> > hitting Enter twice when being asked for user name and password, or by
> > using the very funny notation https://:@server/repository
> >
> > A recent commit (5275c3081c (http: http.emptyauth should allow empty
> > (not just NULL) usernames, 2016-10-04)) broke that usage, though, all
> > of a sudden requiring users to set http.emptyAuth = true.
> >
> > Which brings us to the bigger question why http.emptyAuth defaults to
> > false, to begin with.
>
> This is a valid question, and and I do not see it explicitly asked in the thread:
>
> https://public-
> inbox.org/git/CAPig+cSphEu3iRJrkdBA+BRhi9HnopLJnKOHVuGhUqavtV1RXg
> @mail.gmail.com/#t
>
> even though there is a hint of it already there.
>
> > It would be one thing if cURL would not let the user specify
> > credentials interactively after attempting NTLM authentication (i.e.
> > login credentials), but that is not the case.
> >
> > It would be another thing if attempting NTLM authentication was not
> > usually what users need to do when trying to authenticate via https://.
> > But that is also not the case.
>
> Some other possible worries we may have had I can think of are:
>
> - With this enabled unconditionally, would we leak some information?
I think "NTLM" is actually a misnomer here (I just copied Johannes's
commit message). The mechanism is actually SPNEGO, if I understand this
correctly. It seems to me that this is probably secure, since it is apparently
widely implemented in browsers.
> - With this enabled unconditionally, would we always incur an extra
> roundtrip for people who are not running NTLM at all?
>
> I do not think the former is the case, but what would I know (adding a few
> people involved in the original thread to CC: ;-)
Always, no. For failed authentication (or authorization), apparently, yes.
I tested this by setting the variable to false and then true, and trying to
Push to a github repository which I didn't have write access to, with
both an empty username (https://@:github.com/...) and no username
(http://github.com/...). I ran this under GIT_CURL_VERBOSE=1 and
I saw two 401 responses in the "http.emptyauth=true" case and one
in the false case. I also tried with a repo that I did have access to (first
configuring the necessary tokens for HTTPS push access), and saw two
401 responses in *both* cases.
^ permalink raw reply
* Re: [PATCH] Documentation: Link git-ls-files to core.quotePath variable.
From: Andreas Heiduk @ 2017-02-22 21:08 UTC (permalink / raw)
To: Junio C Hamano, Philip Oakley; +Cc: git
In-Reply-To: <xmqqh93maqgw.fsf@gitster.mtv.corp.google.com>
@Phillip: Thanks.
@Junio: Don't bother, I'm about to fix other man-pages with the text
from ls-files. I will include that typo-fix and prepare a new patch
based on maint.
Am 22.02.2017 um 22:02 schrieb Junio C Hamano:
> "Philip Oakley" <philipoakley@iee.org> writes:
>> s/option pathnamens/option, pathnames/ # comma and spelling.
>
> Thanks. Will squash it in while queuing (I need to discard the new
> text added in the previous attempt in the preimage to make it apply,
> too).
>
^ permalink raw reply
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: Jeff King @ 2017-02-22 21:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: David Turner, git, sandals, Johannes Schindelin, Eric Sunshine
In-Reply-To: <xmqqpoiaasgj.fsf@gitster.mtv.corp.google.com>
On Wed, Feb 22, 2017 at 12:19:56PM -0800, Junio C Hamano wrote:
> > It would be one thing if cURL would not let the user specify credentials
> > interactively after attempting NTLM authentication (i.e. login
> > credentials), but that is not the case.
> >
> > It would be another thing if attempting NTLM authentication was not
> > usually what users need to do when trying to authenticate via https://.
> > But that is also not the case.
>
> Some other possible worries we may have had I can think of are:
>
> - With this enabled unconditionally, would we leak some information?
>
> - With this enabled unconditionally, would we always incur an extra
> roundtrip for people who are not running NTLM at all?
>
> I do not think the former is the case, but what would I know (adding a
> few people involved in the original thread to CC: ;-)
I don't think it incurs an extra round-trip now, because of the way
libcurl works. Though I think it _does_ make it harder for curl to later
optimize out that extra round-trip.
The easiest way to see the difference is to run something like:
GIT_CURL_VERBOSE=1 \
git ls-remote https://example.com/repo-which-needs-auth.git 2>trace
egrep '^>|^< HTTP|Authorization' trace
Before this patch, I get (this is against github.com, which only does
Basic auth):
> GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
< HTTP/1.1 401 Authorization Required
> GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
< HTTP/1.1 401 Authorization Required
> GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
Authorization: Basic <actual credentials>
< HTTP/1.1 200 OK
And after I get:
> GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
< HTTP/1.1 401 Authorization Required
> GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
Authorization: Basic Og==
< HTTP/1.1 401 Authorization Required
> GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
Authorization: Basic <actual credentials>
< HTTP/1.1 200 OK
In the current trace, you can see that libcurl insists on making a
second auth-less request after we've fed it credentials. I'm not sure
how to get rid of this useless extra round-trip, but it would be nice to
do so (IIRC, it is a probe request to find out the list of auth types
that the server supports, which are not remembered from the previous
request).
With http.emptyauth, the second round-trip _isn't_ useless. It's trying
to send the empty credential.
So while curl isn't currently optimizing out the second call, I think
http.emptyauth makes it harder to do the right thing. That's probably
fixable if the logic ends up more like:
- curl reports a 401 to us; actually look at the list of auth methods.
- if there was gss-negotiate, then kick in the empty-auth magic
automatically.
- if empty-auth failed (or if we decided not to try it), ask for a
credential and retry the request. Either way, tell curl that we want
to use "Basic" so it doesn't have to do the probe request (and
obviously if the server did not support Basic, then fail
immediately).
I think that would keep it to 2 round-trips for the normal "Basic" case,
as well as for the GSSNegotiate case. It would be 3 requests when the
server offers GSSNegotiate but you can't use it (but you could set
http.emptyauth=false to optimize that out).
That's all theoretical, though. I might not even be right about the
reason for the second request, and I certainly haven't written any code
(nor do I have a GSSNegotiate setup to test against).
-Peff
^ permalink raw reply
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: Junio C Hamano @ 2017-02-22 21:16 UTC (permalink / raw)
To: David Turner
Cc: git@vger.kernel.org, sandals@crustytoothpaste.net,
Johannes Schindelin, Eric Sunshine, Jeff King
In-Reply-To: <97ab9a812f7b46d7b10d4d06f73259d8@exmbdft7.ad.twosigma.com>
David Turner <David.Turner@twosigma.com> writes:
> Always, no. For failed authentication (or authorization), apparently, yes.
> I tested this by setting the variable to false and then true, and trying to
> Push to a github repository which I didn't have write access to, with
> both an empty username (https://@:github.com/...) and no username
> (http://github.com/...). I ran this under GIT_CURL_VERBOSE=1 and
> I saw two 401 responses in the "http.emptyauth=true" case and one
> in the false case. I also tried with a repo that I did have access to (first
> configuring the necessary tokens for HTTPS push access), and saw two
> 401 responses in *both* cases.
Thanks; that matches my observation. I do not think we care about
an extra roundtrip for the failure case, but as long as we do not
increase the number of roundtrip in the normal case, we can declare
that this is an improvement. I am not quite sure where that extra
401 comes from in the normal case, and that might be an indication
that we already are doing something wrong, though.
^ permalink raw reply
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: Junio C Hamano @ 2017-02-22 21:25 UTC (permalink / raw)
To: Jeff King; +Cc: David Turner, git, sandals, Johannes Schindelin, Eric Sunshine
In-Reply-To: <20170222210636.k2ps3qhhpiyyv6cp@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I don't think it incurs an extra round-trip now, because of the way
> libcurl works. Though I think it _does_ make it harder for curl to later
> optimize out that extra round-trip.
> ...
> In the current trace, you can see that libcurl insists on making a
> second auth-less request after we've fed it credentials. I'm not sure
> how to get rid of this useless extra round-trip, but it would be nice to
> do so (IIRC, it is a probe request to find out the list of auth types
> that the server supports, which are not remembered from the previous
> request).
> ...
> With http.emptyauth, the second round-trip _isn't_ useless. It's trying
> to send the empty credential.
>
> So while curl isn't currently optimizing out the second call, I think
> http.emptyauth makes it harder to do the right thing.
> ...
> I think that would keep it to 2 round-trips for the normal "Basic" case,
> as well as for the GSSNegotiate case. It would be 3 requests when the
> server offers GSSNegotiate but you can't use it (but you could set
> http.emptyauth=false to optimize that out).
Thanks for your thoughts. I'd think that we should take this change
and leave the optimization for later, then. It's not like the
change of the default is making the normal situation any worse, it
seems.
^ permalink raw reply
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: Jeff King @ 2017-02-22 21:34 UTC (permalink / raw)
To: Junio C Hamano
Cc: David Turner, git@vger.kernel.org, sandals@crustytoothpaste.net,
Johannes Schindelin, Eric Sunshine
In-Reply-To: <xmqq8toyapu6.fsf@gitster.mtv.corp.google.com>
On Wed, Feb 22, 2017 at 01:16:33PM -0800, Junio C Hamano wrote:
> David Turner <David.Turner@twosigma.com> writes:
>
> > Always, no. For failed authentication (or authorization), apparently, yes.
> > I tested this by setting the variable to false and then true, and trying to
> > Push to a github repository which I didn't have write access to, with
> > both an empty username (https://@:github.com/...) and no username
> > (http://github.com/...). I ran this under GIT_CURL_VERBOSE=1 and
> > I saw two 401 responses in the "http.emptyauth=true" case and one
> > in the false case. I also tried with a repo that I did have access to (first
> > configuring the necessary tokens for HTTPS push access), and saw two
> > 401 responses in *both* cases.
>
> Thanks; that matches my observation. I do not think we care about
> an extra roundtrip for the failure case, but as long as we do not
> increase the number of roundtrip in the normal case, we can declare
> that this is an improvement. I am not quite sure where that extra
> 401 comes from in the normal case, and that might be an indication
> that we already are doing something wrong, though.
This patch drops the useless probe request:
diff --git a/http.c b/http.c
index 943e630ea..7b4c2db86 100644
--- a/http.c
+++ b/http.c
@@ -1663,6 +1663,9 @@ static int http_request(const char *url,
curlinfo_strbuf(slot->curl, CURLINFO_EFFECTIVE_URL,
options->effective_url);
+ if (results.auth_avail == CURLAUTH_BASIC)
+ http_auth_methods = CURLAUTH_BASIC;
+
curl_slist_free_all(headers);
strbuf_release(&buf);
but setting http.emptyauth adds back in the useless request. I think
that could be fixed by skipping the empty-auth thing when
http_auth_methods does not have CURLAUTH_NEGOTIATE in it (or perhaps
other methods need it to, so maybe skip it if _just_ BASIC is set).
I suspect the patch above could probably be generalized as:
/* cut out methods we know the server doesn't support */
http_auth_methods &= results.auth_avail;
and let curl figure it out from there.
-Peff
^ permalink raw reply related
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: Jeff King @ 2017-02-22 21:35 UTC (permalink / raw)
To: Junio C Hamano
Cc: David Turner, git, sandals, Johannes Schindelin, Eric Sunshine
In-Reply-To: <xmqq4lzlc408.fsf@gitster.mtv.corp.google.com>
On Wed, Feb 22, 2017 at 01:25:11PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > I don't think it incurs an extra round-trip now, because of the way
> > libcurl works. Though I think it _does_ make it harder for curl to later
> > optimize out that extra round-trip.
> > ...
> > In the current trace, you can see that libcurl insists on making a
> > second auth-less request after we've fed it credentials. I'm not sure
> > how to get rid of this useless extra round-trip, but it would be nice to
> > do so (IIRC, it is a probe request to find out the list of auth types
> > that the server supports, which are not remembered from the previous
> > request).
> > ...
> > With http.emptyauth, the second round-trip _isn't_ useless. It's trying
> > to send the empty credential.
> >
> > So while curl isn't currently optimizing out the second call, I think
> > http.emptyauth makes it harder to do the right thing.
> > ...
> > I think that would keep it to 2 round-trips for the normal "Basic" case,
> > as well as for the GSSNegotiate case. It would be 3 requests when the
> > server offers GSSNegotiate but you can't use it (but you could set
> > http.emptyauth=false to optimize that out).
>
> Thanks for your thoughts. I'd think that we should take this change
> and leave the optimization for later, then. It's not like the
> change of the default is making the normal situation any worse, it
> seems.
I'm not excited that it will start making known bogus-username requests
by default to servers which do not even support Negotiate. I guess that
is really the server-operators problem, but it feels pretty hacky.
-Peff
^ permalink raw reply
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: Junio C Hamano @ 2017-02-22 21:57 UTC (permalink / raw)
To: Jeff King; +Cc: David Turner, git, sandals, Johannes Schindelin, Eric Sunshine
In-Reply-To: <20170222213542.opunuepfmj557zyr@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Feb 22, 2017 at 01:25:11PM -0800, Junio C Hamano wrote:
>>
>> Thanks for your thoughts. I'd think that we should take this change
>> and leave the optimization for later, then. It's not like the
>> change of the default is making the normal situation any worse, it
>> seems.
>
> I'm not excited that it will start making known bogus-username requests
> by default to servers which do not even support Negotiate. I guess that
> is really the server-operators problem, but it feels pretty hacky.
I guess that's another valid concern. The servers used to be able
to say "Ah, this repository needs auth and this request does not, so
reject it without asking the auth-db". Now it must say "Ah, this
repository needs auth and this request does have one, but it is
empty so let's not even bother the auth-db" in order to reject a
useless "empty-auth" request with the same efficiency.
After the first request without auth (that fails), do we learn
anything useful from the server side (like "it knows Negotiate")
that we can use to flip the "empty-auth" bit to give a better
default to people from both worlds, I wonder...?
^ permalink raw reply
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: Jeff King @ 2017-02-22 21:58 UTC (permalink / raw)
To: Junio C Hamano
Cc: David Turner, git, sandals, Johannes Schindelin, Eric Sunshine
In-Reply-To: <xmqqwpchanxz.fsf@gitster.mtv.corp.google.com>
On Wed, Feb 22, 2017 at 01:57:28PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Wed, Feb 22, 2017 at 01:25:11PM -0800, Junio C Hamano wrote:
> >>
> >> Thanks for your thoughts. I'd think that we should take this change
> >> and leave the optimization for later, then. It's not like the
> >> change of the default is making the normal situation any worse, it
> >> seems.
> >
> > I'm not excited that it will start making known bogus-username requests
> > by default to servers which do not even support Negotiate. I guess that
> > is really the server-operators problem, but it feels pretty hacky.
>
> I guess that's another valid concern. The servers used to be able
> to say "Ah, this repository needs auth and this request does not, so
> reject it without asking the auth-db". Now it must say "Ah, this
> repository needs auth and this request does have one, but it is
> empty so let's not even bother the auth-db" in order to reject a
> useless "empty-auth" request with the same efficiency.
>
> After the first request without auth (that fails), do we learn
> anything useful from the server side (like "it knows Negotiate")
> that we can use to flip the "empty-auth" bit to give a better
> default to people from both worlds, I wonder...?
Yes, that's exactly what I was trying to say in my first message.
-Peff
^ permalink raw reply
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: Junio C Hamano @ 2017-02-22 22:35 UTC (permalink / raw)
To: Jeff King; +Cc: David Turner, git, sandals, Johannes Schindelin, Eric Sunshine
In-Reply-To: <20170222215833.d7htyo32ptfse5l4@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Feb 22, 2017 at 01:57:28PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > On Wed, Feb 22, 2017 at 01:25:11PM -0800, Junio C Hamano wrote:
>> >>
>> >> Thanks for your thoughts. I'd think that we should take this change
>> >> and leave the optimization for later, then. It's not like the
>> >> change of the default is making the normal situation any worse, it
>> >> seems.
>> >
>> > I'm not excited that it will start making known bogus-username requests
>> > by default to servers which do not even support Negotiate. I guess that
>> > is really the server-operators problem, but it feels pretty hacky.
>>
>> I guess that's another valid concern. The servers used to be able
>> to say "Ah, this repository needs auth and this request does not, so
>> reject it without asking the auth-db". Now it must say "Ah, this
>> repository needs auth and this request does have one, but it is
>> empty so let's not even bother the auth-db" in order to reject a
>> useless "empty-auth" request with the same efficiency.
>>
>> After the first request without auth (that fails), do we learn
>> anything useful from the server side (like "it knows Negotiate")
>> that we can use to flip the "empty-auth" bit to give a better
>> default to people from both worlds, I wonder...?
>
> Yes, that's exactly what I was trying to say in my first message.
I see. I am still inclined to take this as-is for now to cook in
'next', though.
A solution along your line would help Negotiate users OOB experience
without hurting the servers that do not offer Negotiate, but until
that materializes, users can set the lazier http.emptyAuth on
(without selectively setting http.<host>.emptyAuth off for sites
without Negotiate) and hurt the servers by throwing an empty auth
anyway regardless of the default, so the flipping of the default is
not fundamentally adding more harm in that sense.
^ permalink raw reply
* Re: feature request: user email config per domain
From: Andrew Ardill @ 2017-02-22 23:21 UTC (permalink / raw)
To: Tushar Kapila; +Cc: git@vger.kernel.org
In-Reply-To: <CAN0Skmmjd5Y0uWz_WC69mAStucZ6nR0mjdp4-ODJz2UnTaB-eQ@mail.gmail.com>
On 23 February 2017 at 00:12, Tushar Kapila <tgkprog@gmail.com> wrote:
> I can set my email via:
> git config --global user.email tgkprog@xyz.dom
>
> this is dangerous when I use this my office or in a multi repository
> provider environment where my email is different for a few (like
> tgkprog@search.com for github and tushar@mycompany.com for my company
> private repo).
There has been a large discussion around this idea before, see this
thread for example [0].
The proposed idea was to have something set in your global config that
would only be turned on if you were within a given directory. This
would allow you to have two root directories, one for your work
projects and one for open source projects (for example) and any git
repositories within those folders would each would have their own
config options automatically applied based on their location.
There was a patch suggested, and it worked quite well, but nothing
further has been done to my knowledge.
[0] http://public-inbox.org/git/7vvc3d1o01.fsf@alter.siamese.dyndns.org/
Regards,
Andrew Ardill
^ permalink raw reply
* Re: [PATCH v5 03/19] builtin/diff-tree: convert to struct object_id
From: brian m. carlson @ 2017-02-22 23:22 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Michael Haggerty, Ramsay Jones
In-Reply-To: <20170222191641.o2rtt2ymtb4h2yqe@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1570 bytes --]
On Wed, Feb 22, 2017 at 02:16:42PM -0500, Jeff King wrote:
> On Wed, Feb 22, 2017 at 10:50:21AM -0800, Junio C Hamano wrote:
>
> > "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> >
> > > Convert most leaf functions to struct object_id. Change several
> > > hardcoded numbers to uses of parse_oid_hex. In doing so, verify that we
> > > when we want two trees, we have exactly two trees.
> > >
> > > Finally, in stdin_diff_commit, avoid accessing the byte after the NUL.
> > > This will be a NUL as well, since the first NUL was a newline we
> > > overwrote. However, with parse_oid_hex, we no longer need to increment
> > > the pointer directly, and can simply increment it as part of our check
> > > for the space character.
> >
> > After reading the pre- and post-image twice, I think I convinced
> > myself that this is a faithful conersion and they do the same thing.
>
> I think this is correct, too (but then, I think it largely comes from
> the patch I wrote the other night. So I did look at it carefully, but
> it's not exactly an independent review).
I did take that part, as well as other parts, from your patch.
I have a test, which I'll send in a minute, that verifies that they do
the same thing. At first I introduced a segfault, and the test caught
it, so I feel confident that the patch does indeed function as the old
code did.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: Jeff King @ 2017-02-22 23:33 UTC (permalink / raw)
To: Junio C Hamano
Cc: David Turner, git, sandals, Johannes Schindelin, Eric Sunshine
In-Reply-To: <xmqqshn5am74.fsf@gitster.mtv.corp.google.com>
On Wed, Feb 22, 2017 at 02:35:11PM -0800, Junio C Hamano wrote:
> A solution along your line would help Negotiate users OOB experience
> without hurting the servers that do not offer Negotiate, but until
> that materializes, users can set the lazier http.emptyAuth on
> (without selectively setting http.<host>.emptyAuth off for sites
> without Negotiate) and hurt the servers by throwing an empty auth
> anyway regardless of the default, so the flipping of the default is
> not fundamentally adding more harm in that sense.
I was hoping to materialize it today. :)
Here's what I came up with. I have a lot of questions about the second
patch which I'll outline there. But I think it may be a good start.
[1/2]: http: restrict auth methods to what the server advertises
[2/2]: http: add an "auto" mode for http.emptyauth
http.c | 38 +++++++++++++++++++++++++++++++++++---
1 file changed, 35 insertions(+), 3 deletions(-)
-Peff
^ permalink raw reply
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: brian m. carlson @ 2017-02-22 23:34 UTC (permalink / raw)
To: David Turner
Cc: 'Junio C Hamano', git@vger.kernel.org,
Johannes Schindelin, Eric Sunshine, Jeff King
In-Reply-To: <97ab9a812f7b46d7b10d4d06f73259d8@exmbdft7.ad.twosigma.com>
[-- Attachment #1: Type: text/plain, Size: 2075 bytes --]
On Wed, Feb 22, 2017 at 09:04:14PM +0000, David Turner wrote:
> > -----Original Message-----
> > From: Junio C Hamano [mailto:jch2355@gmail.com] On Behalf Of Junio C
> > Hamano
> > Sent: Wednesday, February 22, 2017 3:20 PM
> > To: David Turner <David.Turner@twosigma.com>
> > Cc: git@vger.kernel.org; sandals@crustytoothpaste.net; Johannes Schindelin
> > <johannes.schindelin@gmx.de>; Eric Sunshine
> > <sunshine@sunshineco.com>; Jeff King <peff@peff.net>
> > Subject: Re: [PATCH] http(s): automatically try NTLM authentication first
> >
> >
> > Some other possible worries we may have had I can think of are:
> >
> > - With this enabled unconditionally, would we leak some information?
>
> I think "NTLM" is actually a misnomer here (I just copied Johannes's
> commit message). The mechanism is actually SPNEGO, if I understand this
> correctly. It seems to me that this is probably secure, since it is apparently
> widely implemented in browsers.
This is SPNEGO. It will work with NTLM as well as Kerberos.
Browsers usually disable this feature by default, as it basically will
attempt to authenticate to any site that sends a 401. For Kerberos
against a malicious site, the user will either not have a valid ticket
for that domain, or the user's Kerberos server will refuse to provide a
ticket to pass to the server, so there's no security risk involved.
I'm unclear how SPNEGO works with NTLM, so I can't speak for the
security of it. From what I understand of NTLM and from RFC 4559, it
consists of a shared secret. I'm unsure what security measures are in
place to not send that to an untrusted server.
As far as Kerberos, this is a desirable feature to have enabled, with
little downside. I just don't know about the security of the NTLM part,
and I don't think we should take this patch unless we're sure we know
the consequences of it.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: [RFC 03/14] upload-pack: test negotiation with changing repo
From: Junio C Hamano @ 2017-02-22 23:36 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
In-Reply-To: <0c727063-0583-f713-68fb-bd284be696b2@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
>> This somehow looks like a good thing to do even in production. Am I
>> mistaken?
>
> Yes, that's true. If this patch set stalls (for whatever reason), I'll
> spin this off into an independent patch.
... which may be needed.
As to the main goal of this topic, I think the "ask by refname (with
glob)" is very good thing to start the "client speaks first" v2
protocol, as it would allow us to reduce the size of the initial
advertisement for common cases (i.e. remote.<name>.fetch is likely
to list only refs/heads/* on the left hand side of a refspec). And
adding this to v1 is probably a good first step to make sure the
code that is currently used by v1 protocol exchange that will be
shared with the v2 protocols are prepared to be driven by refname
without knowing the exact object name until the final round.
^ permalink raw reply
* [PATCH] t: add multi-parent test of diff-tree --stdin
From: brian m. carlson @ 2017-02-22 23:38 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Michael Haggerty, Ramsay Jones
In-Reply-To: <20170222232215.u2agozagwsyy2ooe@genre.crustytoothpaste.net>
We were lacking a test that covered the multi-parent case for commits.
Add a basic test to ensure we do not regress this behavior in the
future.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
t/t4013-diff-various.sh | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
It's a little bit ugly to me that this test hard-codes so many values,
and I'm concerned that it may be unnecessarily brittle. However, I
don't have a good idea of how to perform the kind of comprehensive test
we need otherwise.
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index d09acfe48e..e6c2a7a369 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -349,4 +349,23 @@ test_expect_success 'diff-tree --stdin with log formatting' '
test_cmp expect actual
'
+test_expect_success 'diff-tree --stdin with two parent commits' '
+ cat >expect <<-\EOF &&
+ c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a
+ :040000 040000 da7a33fa77d8066d6698643940ce5860fe2d7fb3 f977ed46ae6873c1c30ab878e15a4accedc3618b M dir
+ :100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d f4615da674c09df322d6ba8d6b21ecfb1b1ba510 M file0
+ :000000 100644 0000000000000000000000000000000000000000 7289e35bff32727c08dda207511bec138fdb9ea5 A file3
+ 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
+ :040000 040000 847b63d013de168b2de618c5ff9720d5ab27e775 65f5c9dd60ce3b2b3324b618ac7accf8d912c113 M dir
+ :000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A file1
+ 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44
+ :040000 040000 da7a33fa77d8066d6698643940ce5860fe2d7fb3 847b63d013de168b2de618c5ff9720d5ab27e775 M dir
+ :100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d b414108e81e5091fe0974a1858b4d0d22b107f70 M file0
+ :100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D file2
+ EOF
+ git rev-list --parents master | git diff-tree --stdin >actual &&
+ test_cmp expect actual
+'
+
+
test_done
^ permalink raw reply related
* [PATCH 2/2] http: add an "auto" mode for http.emptyauth
From: Jeff King @ 2017-02-22 23:40 UTC (permalink / raw)
To: Junio C Hamano
Cc: David Turner, git, sandals, Johannes Schindelin, Eric Sunshine
In-Reply-To: <20170222233333.dx5lknw4fpopu5hy@sigill.intra.peff.net>
This variable needs to be specified to make some types of
non-basic authentication work, but ideally this would just
work out of the box for everyone.
However, simply setting it to "1" by default introduces an
extra round-trip for cases where it _isn't_ useful. We end
up sending a bogus empty credential that the server rejects.
Instead, let's introduce an automatic mode, that works like
this:
1. We won't try to send the bogus credential on the first
request. We'll wait to get an HTTP 401, as usual.
2. After seeing an HTTP 401, the empty-auth hack will kick
in only when we know there is an auth method beyond
"Basic" to be tried.
That should make it work out of the box, without incurring
any extra round-trips for people hitting Basic-only servers.
This _does_ incur an extra round-trip if you really want to
use "Basic" but your server advertises other methods (the
emptyauth hack will kick in but fail, and then Git will
actually ask for a password).
The auto mode may incur an extra round-trip over setting
http.emptyauth=true, because part of the emptyauth hack is
to feed this blank password to curl even before we've made a
single request.
Signed-off-by: Jeff King <peff@peff.net>
---
My open questions are:
- I don't have anything but a Basic server to test against. So it's
entirely possible that this doesn't actually work in the NTLM case.
- what does a request log look like for somebody actually using NTLM?
It's possible if the initial request sends a restrict auth_avail,
that curl could get away without the extra probe request, and we'd
end up with the same number of requests for "auto" mode versus
http.emptyauth=true.
- the whole "don't use this on the initial request" flag feels really
hacky. It's a side effect of how emptyauth tries to kick in even
before we have sent any requests. Probably it should have been
handled in the 401 code path originally, but I'm hesitant to change
it now. I suspect it is eliminating a round-trip in practice when it
is enabled.
- I didn't test a server that advertises Basic and something else, but
really only takes Basic. So I'm just assuming that it incurs the
extra round-trip (actually probably two, one for curl's method
probe).
- When your curl is too old to do CURLAUTH_ANY, I just left the
default to disable emptyauth. But it could easily be "1" if people
care.
http.c | 38 ++++++++++++++++++++++++++++++++++----
1 file changed, 34 insertions(+), 4 deletions(-)
diff --git a/http.c b/http.c
index a05609766..ea70ec1ee 100644
--- a/http.c
+++ b/http.c
@@ -109,7 +109,7 @@ static int curl_save_cookies;
struct credential http_auth = CREDENTIAL_INIT;
static int http_proactive_auth;
static const char *user_agent;
-static int curl_empty_auth;
+static int curl_empty_auth = -1;
enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;
@@ -125,6 +125,7 @@ static struct credential cert_auth = CREDENTIAL_INIT;
static int ssl_cert_password_required;
#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
static unsigned long http_auth_methods = CURLAUTH_ANY;
+static int http_auth_methods_restricted;
#endif
static struct curl_slist *pragma_header;
@@ -382,10 +383,37 @@ static int http_options(const char *var, const char *value, void *cb)
return git_default_config(var, value, cb);
}
+static int curl_empty_auth_enabled(void)
+{
+ if (curl_empty_auth < 0) {
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+ /*
+ * In the automatic case, kick in the empty-auth
+ * hack as long as we would potentially try some
+ * method more exotic than "Basic".
+ *
+ * But only do so when this is _not_ our initial
+ * request, as we would not then yet know what
+ * methods are available.
+ */
+ return http_auth_methods_restricted &&
+ http_auth_methods != CURLAUTH_BASIC;
+#else
+ /*
+ * Our libcurl is too old to do AUTH_ANY in the first place;
+ * just default to turning the feature off.
+ */
+ return 0;
+#endif
+ }
+
+ return curl_empty_auth;
+}
+
static void init_curl_http_auth(CURL *result)
{
if (!http_auth.username || !*http_auth.username) {
- if (curl_empty_auth)
+ if (curl_empty_auth_enabled())
curl_easy_setopt(result, CURLOPT_USERPWD, ":");
return;
}
@@ -1079,7 +1107,7 @@ struct active_request_slot *get_active_slot(void)
#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);
#endif
- if (http_auth.password || curl_empty_auth)
+ if (http_auth.password || curl_empty_auth_enabled())
init_curl_http_auth(slot->curl);
return slot;
@@ -1347,8 +1375,10 @@ static int handle_curl_result(struct slot_results *results)
} else {
#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
- if (results->auth_avail)
+ if (results->auth_avail) {
http_auth_methods &= results->auth_avail;
+ http_auth_methods_restricted = 1;
+ }
#endif
return HTTP_REAUTH;
}
--
2.12.0.rc2.597.g959f68882
^ permalink raw reply related
* [PATCH 1/2] http: restrict auth methods to what the server advertises
From: Jeff King @ 2017-02-22 23:34 UTC (permalink / raw)
To: Junio C Hamano
Cc: David Turner, git, sandals, Johannes Schindelin, Eric Sunshine
In-Reply-To: <20170222233333.dx5lknw4fpopu5hy@sigill.intra.peff.net>
By default, we tell curl to use CURLAUTH_ANY, which does not
limit its set of auth methods. However, this results in an
extra round-trip to the server when authentication is
required. After we've fed the credential to curl, it wants
to probe the server to find its list of available methods
before sending an Authorization header.
We can shortcut this by limiting our http_auth_methods by
what the server told us it supports. In some cases (such as
when the server only supports Basic), that lets curl skip
the extra probe request.
The end result should look the same to the user, but you can
use GIT_TRACE_CURL to verify the sequence of requests:
GIT_TRACE_CURL=1 \
git ls-remote https://example.com/repo.git \
2>&1 >/dev/null |
egrep '(Send|Recv) header: (GET|HTTP|Auth)'
Before this patch, hitting a Basic-only server like
github.com results in:
Send header: GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
Recv header: HTTP/1.1 401 Authorization Required
Send header: GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
Recv header: HTTP/1.1 401 Authorization Required
Send header: GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
Send header: Authorization: Basic <redacted>
Recv header: HTTP/1.1 200 OK
And after:
Send header: GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
Recv header: HTTP/1.1 401 Authorization Required
Send header: GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
Send header: Authorization: Basic <redacted>
Recv header: HTTP/1.1 200 OK
The possible downsides are:
- This only helps for a Basic-only server; for a server
with multiple auth options, curl may still send a probe
request to see which ones are available (IOW, there's no
way to say "don't probe, I already know what the server
will say").
- The http_auth_methods variable is global, so this will
apply to all further requests. That's acceptable for
Git's usage of curl, though, which also treats the
credentials as global. I.e., in any given program
invocation we hit only one conceptual server (we may be
redirected at the outset, but in that case that's whose
auth_avail field we'd see).
Signed-off-by: Jeff King <peff@peff.net>
---
http.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/http.c b/http.c
index 90a1c0f11..a05609766 100644
--- a/http.c
+++ b/http.c
@@ -1347,6 +1347,8 @@ static int handle_curl_result(struct slot_results *results)
} else {
#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
+ if (results->auth_avail)
+ http_auth_methods &= results->auth_avail;
#endif
return HTTP_REAUTH;
}
--
2.12.0.rc2.597.g959f68882
^ permalink raw reply related
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