git no longer prompting for password

git no longer prompting for password

[Iain Paton]

Hi List,

A recent update to git 1.7.12 from 1.7.3.5 seems to have changed something - trying to push to a smart http backend no longer prompts for a password and hence fails the server auth.

The server is currently running git 1.7.9 behind apache 2.4.3 with an almost verbatim copy of the apache config from the git-http-backend manpage.

Backtracking through the versions I've skipped and this doesn't seem to be a new problem, client side up to 1.7.7.7 works, 1.7.8 onwards don't. Server side version doesn't seem to make a difference.

user@fubar01:~/test# git --version
git version 1.7.7.7
user@fubar01:~/test# git push http://ipaton@10.0.0.1/git/test.git master
Password: 

type the password in and the push is successful

user@fubar01:~/test# git --version
git version 1.7.8
user@fubar01:~/test# git push http://ipaton@10.0.0.1/git/test.git master --verbose
Pushing to http://ipaton@10.0.0.1/git/test.git
Counting objects: 6, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (3/3), done.
Writing objects: 100% (5/5), 491 bytes, done.
Total 5 (delta 0), reused 0 (delta 0)
error: RPC failed; result=22, HTTP code = 401
fatal: The remote end hung up unexpectedly
fatal: The remote end hung up unexpectedly

Watching the connection with wireshark shows that it does appear to try to authenticate with the correct username, but without a password. Not surprising since it doesn't ask for one..

googling for git and password just seems to give results where people want it to stop asking for a password, which is the oppsite of what I want!  
Looking at changelogs for 1.7.8 and I'm not really seeing anything that says I need to do something different.

Any help or pointers appreciated.

Thanks,
Iain

Re: git no longer prompting for password

[Jeff King]

On Fri, Aug 24, 2012 at 09:19:28PM +0100, Iain Paton wrote:

> A recent update to git 1.7.12 from 1.7.3.5 seems to have changed
> something - trying to push to a smart http backend no longer prompts
> for a password and hence fails the server auth.
> [...]
> Backtracking through the versions I've skipped and this doesn't seem
> to be a new problem, client side up to 1.7.7.7 works, 1.7.8 onwards
> don't. Server side version doesn't seem to make a difference.

There was some work in v1.7.8 to avoid prompting for a password when it
is not necessary; I suspect this is a fallout of that.

You could try bisecting the bug. My guess is that you will end up at
commit 986bbc0 (http: don't always prompt for password, 2011-11-04).

> user@fubar01:~/test# git --version
> git version 1.7.7.7
> user@fubar01:~/test# git push http://ipaton@10.0.0.1/git/test.git master
> Password: 

As per the discussion in 986bbc0, this is actually prompting you before
git makes any request. Whereas here:

> user@fubar01:~/test# git --version
> git version 1.7.8
> user@fubar01:~/test# git push http://ipaton@10.0.0.1/git/test.git master --verbose

We should get an HTTP 401 from the server, then prompt, then retry.
What's weird is that it sort of works:

> Pushing to http://ipaton@10.0.0.1/git/test.git
> Counting objects: 6, done.
> Delta compression using up to 8 threads.
> Compressing objects: 100% (3/3), done.
> Writing objects: 100% (5/5), 491 bytes, done.
> Total 5 (delta 0), reused 0 (delta 0)
> error: RPC failed; result=22, HTTP code = 401
> fatal: The remote end hung up unexpectedly
> fatal: The remote end hung up unexpectedly

It's like the initial http requests do not get a 401, and the push
proceeds, and then some later request causes a 401 when we do not expect
it. Which is doubly odd, since we should also be able to handle that
case (the first 401 we get should cause us to ask for a password).

Can you show us the result of running with GIT_CURL_VERBOSE=1? I'd
really like to see which requests are being made with and without
authentication.

> Looking at changelogs for 1.7.8 and I'm not really seeing anything
> that says I need to do something different.

No, you shouldn't need to do anything different. I'd suspect the
weirdness you are seeing is from a credential helper trying to supply a
blank password, except that you would have to have configured one
manually for it to run (I assume you are not on a shared machine where
somebody might have tweaked /etc/gitconfig or anything like that).

-Peff

Re: git no longer prompting for password

[Jeff King]

On Sat, Aug 25, 2012 at 03:56:01PM +0100, Iain Paton wrote:

> > It's like the initial http requests do not get a 401, and the push
> > proceeds, and then some later request causes a 401 when we do not expect
> > it. Which is doubly odd, since we should also be able to handle that
> > case (the first 401 we get should cause us to ask for a password).
> 
> Yes, I deliberately have it set for anonymous pull and authenticated push. 
> So the initial contact with the server doesn't ask for auth.

OK, I see what's going on. It looks like it is configured to do so by
rejecting the POST request. So this first request works:

> > GET /git/test.git/info/refs?service=git-receive-pack HTTP/1.1
> User-Agent: git/1.7.8
> Host: 10.44.16.74
> Accept: */*
> Pragma: no-cache
> 
> < HTTP/1.1 200 OK

which is the first step of the conversation, in which the client gets
the set of refs from the remote. Then it tries to POST the pack:

> > POST /git/test.git/git-receive-pack HTTP/1.1
> User-Agent: git/1.7.8
> Host: 10.44.16.74
> Accept-Encoding: deflate, gzip
> Content-Type: application/x-git-receive-pack-request
> Accept: application/x-git-receive-pack-result
> Content-Length: 412
> 
> * upload completely sent off: 412 out of 412 bytes
> < HTTP/1.1 401 Unauthorized

And we get blocked on that request. I didn't quote it above, but note
how the client actually generates and sends the full pack before being
told "no, you can't do this".

So that explains the output you see; we really are generating and
sending the pack, and only then getting a 401. And it also explains why
git does not prompt and retry; we follow a different code path for POSTs
that does not trigger the retry code.

This is not optimal, as we send the pack data only to find out that we
are not authenticated. There is code to avoid sending the _whole_ pack
(it's the probe_rpc code in remote-curl.c), so I think you'd just be
wasting 64K, which is not too bad. So we could teach git to retry if the
POST fails, and I think it would work OK.

But I don't think there is any reason not to block the push request
right from the first receive-pack request we see, which catches the
issue even earlier, and with less overhead (and of course works with
existing git clients :) ).

> apache config has the following:
> [...]
> <LocationMatch "^/git/.*/git-receive-pack$">
>         AuthType Basic
>         AuthUserFile /data/git/htpasswd
>         AuthGroupfile /data/git/groups 
>         AuthName "Git Access"
> 
>         Require group committers
> </LocationMatch>
> 
> nothing untoward there I think and google turns up lots of examples where 
> people are doing essentially the same thing.

I think your regex is the culprit. The first request comes in with:

> > GET /git/test.git/info/refs?service=git-receive-pack HTTP/1.1

The odd URL is because we are probing to see if the server even supports
smart-http. But note that it does not match your regex above, which
requires "/git-receive-pack". It looks like that is pulled straight from
the git-http-backend manpage. I think the change in v1.7.8 broke people
using that configuration.

I tend to think the right thing is to fix the configuration (both on
your system and in the documentation), but we should probably also fix
git to handle this situation more gracefully, since it used to work and
has been advertised in the documentation for a long time.

-Peff

Re: git no longer prompting for password

[Iain Paton]

On 25/08/12 21:39, Jeff King wrote:

> I think your regex is the culprit. The first request comes in with:
> 
>>> GET /git/test.git/info/refs?service=git-receive-pack HTTP/1.1
> 
> The odd URL is because we are probing to see if the server even supports
> smart-http. But note that it does not match your regex above, which
> requires "/git-receive-pack". It looks like that is pulled straight from
> the git-http-backend manpage. I think the change in v1.7.8 broke people
> using that configuration.

Yes, it was lifted straight out of the manpage, albeit a couple of years 
ago now and there have been additions to the manpage since then. 
I did check, and the basic config is identical in the current manpage.

I can't be the only one using a config that's based on the example in 
the manpage surely ?  So I'm surprised this hasn't come up previously.


> I tend to think the right thing is to fix the configuration (both on
> your system and in the documentation), but we should probably also fix
> git to handle this situation more gracefully, since it used to work and
> has been advertised in the documentation for a long time.

So after some head scratching trying to work out how to do the equivalent of 
LocationMatch but on the query string I came up with the following:

ScriptAlias /git/ /usr/libexec/git-core/git-http-backend/

<Directory /usr/libexec/git-core>
        Require ip 10.44.0.0/16
        <If "%{THE_REQUEST} =~ /git-receive-pack/">
                AuthType Basic
                AuthUserFile /data/git/htpasswd
                AuthGroupfile /data/git/groups
                AuthName "Git Access"

                Require group committers
        </If>
</Directory>

and I've removed the LocationMatch section completely.

So for accesses to git-http-backend I require auth if anything in the request 
includes git-receive-pack and that causes a prompt for the username/password 
as required, while at the same time it still allows anonymous pull.

It appears that the clone operation uses

GET /git/test.git/info/refs?service=git-upload-pack HTTP/1.1

to probe for smart-http ?  So this would be ok ?

I'm not sure this is ideal, I don't really know enough about the protocol to know 
if I'll see git-receive-pack elsewhere. Possibly if someone includes it in the 
name of a repo it'll blow up in my face.
I can always change it to match only on QUERY_STRING and put the LocationMatch 
back in if that happens.

If that's all that's required, I'm fine with an easy change to httpd.conf

Thanks for the help Jeff.

Re: git no longer prompting for password

[Jeff King]

On Sun, Aug 26, 2012 at 10:57:59AM +0100, Iain Paton wrote:

> > The odd URL is because we are probing to see if the server even supports
> > smart-http. But note that it does not match your regex above, which
> > requires "/git-receive-pack". It looks like that is pulled straight from
> > the git-http-backend manpage. I think the change in v1.7.8 broke people
> > using that configuration.
> 
> Yes, it was lifted straight out of the manpage, albeit a couple of years 
> ago now and there have been additions to the manpage since then. 
> I did check, and the basic config is identical in the current manpage.
> 
> I can't be the only one using a config that's based on the example in 
> the manpage surely ?  So I'm surprised this hasn't come up previously.

Yeah, I'm surprised it took this long to come up, too. Perhaps most
people just do anonymous http, and then rely on ssh for pushing to
achieve the same effect. Or maybe my analysis of the problem is wrong.
:)

I'm preparing some patches to the test suite that will demonstrate the
problem (we test dumb-http auth, but we don't do any smart-http auth at
all in the test suite), and then a fix on top to let us prompt for the
password in this instance. I think we should also update the
documentation, but the existing advice has been given long enough that
people are going to use it for some time, and I consider your issue to
be a regression in v1.7.8 that should be fixed.

> So after some head scratching trying to work out how to do the equivalent of 
> LocationMatch but on the query string I came up with the following:
> 
> ScriptAlias /git/ /usr/libexec/git-core/git-http-backend/
> 
> <Directory /usr/libexec/git-core>
>         Require ip 10.44.0.0/16
>         <If "%{THE_REQUEST} =~ /git-receive-pack/">
>                 AuthType Basic
>                 AuthUserFile /data/git/htpasswd
>                 AuthGroupfile /data/git/groups
>                 AuthName "Git Access"
> 
>                 Require group committers
>         </If>
> </Directory>
> 
> and I've removed the LocationMatch section completely.

Yeah, I think that will work. It feels a little weird and hacky. E.g.,
what if you had a repo named git-receive-pack? Unlikely, of course, but
I'd want the config we advertise in the manpage to be as robust as
possible.

I don't know enough about Apache to know off-hand if there is a cleaner
way. I'll investigate a bit more before doing my documentation patch.

> So for accesses to git-http-backend I require auth if anything in the request 
> includes git-receive-pack and that causes a prompt for the username/password 
> as required, while at the same time it still allows anonymous pull.
> 
> It appears that the clone operation uses
> 
> GET /git/test.git/info/refs?service=git-upload-pack HTTP/1.1
> 
> to probe for smart-http ?  So this would be ok ?

Right. Anything invoking receive-pack is always a push.

> I'm not sure this is ideal, I don't really know enough about the protocol to know 
> if I'll see git-receive-pack elsewhere. Possibly if someone includes it in the 
> name of a repo it'll blow up in my face.

Yep, exactly. That should be the only place, though, I think (branch
names, for example, are never part of the URL).

> I can always change it to match only on QUERY_STRING and put the LocationMatch 
> back in if that happens.

I think that would be cleaner. It would be even nicer if you could
really just match "service=" as a query parameter, but I don't know that
apache parses that at all. I also don't know if Apache does any
canonicalization of the QUERY_STRING. When matching, you'd want to make
sure there is no way of a client sneaking in a parameter that git would
understand to mean a push, but that your pattern would not notice (so,
e.g., just matching "git-receive-pack$" would not be sufficient, as I
could request "?service=git-receive-pack&fooled_you=true". I don't
recall whether git rejects nonsense like that itself.

> If that's all that's required, I'm fine with an easy change to httpd.conf
> 
> Thanks for the help Jeff.

No problem. I'll probably be a day or two on the patches, as the http
tests are in need of some refactoring before adding more tests. But in
the meantime, I think your config change is a sane work-around.

-Peff

Re: git no longer prompting for password

[Iain Paton]

On 26/08/12 11:13, Jeff King wrote:

> Yeah, I'm surprised it took this long to come up, too. Perhaps most
> people just do anonymous http, and then rely on ssh for pushing to
> achieve the same effect. Or maybe my analysis of the problem is wrong.
> :)

I'd be using ssh to push too, but the simple fact is that the http way 
works through a proxy and so essentially works from anywhere. The same 
isn't true for ssh or git protocols. Well that's my reason anyway :)

> Yeah, I think that will work. It feels a little weird and hacky. E.g.,

Yeah, it does. I couldn't find a simple way though, most stuff like 
LocationMatch specifically excludes the query string which makes it 
rather more difficult.

> I don't know enough about Apache to know off-hand if there is a cleaner
> way. I'll investigate a bit more before doing my documentation patch.

I'm not an apache expert either. What I could find was using mod_rewrite to 
set an env var based on something in the query string, but not actually do 
any rewrite. Then looking at how to check the env var and do something based 
on that got me the example of simply using If with an expression to match 
directly on the query string.

> I think that would be cleaner. It would be even nicer if you could
> really just match "service=" as a query parameter, but I don't know that
> apache parses that at all. I also don't know if Apache does any
> canonicalization of the QUERY_STRING. When matching, you'd want to make

>From what I can tell apache really doesn't care much about the query string 
at all, it seems to just pass it through unless you start messing with it 
using mod_rewrite, but even then you're still regex based. I couldn't find 
anything that parsed out individual parameters. Of course I could just be 
looking in all the wrong places :) 

> sure there is no way of a client sneaking in a parameter that git would
> understand to mean a push, but that your pattern would not notice (so,
> e.g., just matching "git-receive-pack$" would not be sufficient, as I

yep, and matching on THE_REQUEST gets you the whole string, including the 
HTTP/1.1 on the end. I tried putting the $ on the end of the regex and it 
didn't work. 
It should be possible to combine the original regex from the LocationMatch 
example and something like /[?&]service=git-receive-pack/ though, which 
should make it somewhat safer.

> No problem. I'll probably be a day or two on the patches, as the http
> tests are in need of some refactoring before adding more tests. But in
> the meantime, I think your config change is a sane work-around.

Works-For-Me is all I need right now :)  I'll be interested if you come 
up with something better though.

Iain

[PATCH 0/8] fix password prompting for "half-auth" servers

[Jeff King]

On Sun, Aug 26, 2012 at 06:13:41AM -0400, Jeff King wrote:

> No problem. I'll probably be a day or two on the patches, as the http
> tests are in need of some refactoring before adding more tests. But in
> the meantime, I think your config change is a sane work-around.

OK, here is the series.  For those just joining us, the problem is that
git will not correctly prompt for credentials when pushing to a
repository which allows the initial GET of
".../info/refs?service=git-receive-pack", but then gives a 401 when we
try to POST the pack. This has never worked for a plain URL, but used to
work if you put the username in the URL (because we would
unconditionally load the credentials before making any requests). That
was broken by 986bbc0, which does not do that proactive prompting for
smart-http, meaning such repositories cannot be pushed to at all.

Such a server-side setup is questionable in my opinion (because the
client will actually create the pack before failing), but we have been
advertising it for a long time in git-http-backend(1) as the right way
to make repositories that are anonymous for fetching but require auth
for pushing.

The fix is somewhat uglier than I would like, but I think it's practical
and the right thing to do (see the final patch for lots of discussion).
I built this on the current tip of "master".  It might make sense to
backport it directly on top of 986bbc0 for the maint track. There are
conflicts, but they are all textual. Another option would be to revert
986bbc0 for the maint track, as that commit is itself fixing a minor bug
that is of decreasing relevance (it fixed extra password prompting when
.netrc was in use, but one can work around it by dropping the username
from the URL).

The patches are:

  [1/8]: t5550: put auth-required repo in auth/dumb
  [2/8]: t5550: factor out http auth setup
  [3/8]: t/lib-httpd: only route auth/dumb to dumb repos
  [4/8]: t/lib-httpd: recognize */smart/* repos as smart-http
  [5/8]: t: test basic smart-http authentication

These are all refactoring of the test scripts in preparation for 6/8
(and are where all of the conflicts lie).

  [6/8]: t: test http access to "half-auth" repositories

This demonstrates the bug.

  [7/8]: http: factor out http error code handling

Refactoring to support 8/8.

  [8/8]: http: prompt for credentials on failed POST

And this one is the actual fix.

I'd like to have a 9/8 which tweaks the git-http-backend documentation
to provide better example apache config, but I haven't yet figured out
the right incantation. Suggestions from apache gurus are welcome.

-Peff

[PATCH 1/8] t5550: put auth-required repo in auth/dumb

[Jeff King]

In most of our tests, we put repos to be accessed by dumb
protocols in /dumb, and repos to be accessed by smart
protocols in /smart.  In our test apache setup, the whole
/auth hierarchy requires authentication. However, we don't
bother to split it by smart and dumb here because we are not
currently testing smart-http authentication at all.

That will change in future patches, so let's be explicit
that we are interested in testing dumb access here. This
also happens to match what t5540 does for the push tests.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5550-http-fetch.sh | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index b06f817..5ad2123 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -41,9 +41,9 @@ test_expect_success 'clone http repository' '
 '
 
 test_expect_success 'create password-protected repository' '
-	mkdir "$HTTPD_DOCUMENT_ROOT_PATH/auth/" &&
+	mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/" &&
 	cp -Rf "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
-	       "$HTTPD_DOCUMENT_ROOT_PATH/auth/repo.git"
+	       "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/repo.git"
 '
 
 test_expect_success 'setup askpass helpers' '
@@ -81,28 +81,28 @@ expect_askpass() {
 test_expect_success 'cloning password-protected repository can fail' '
 	>askpass-query &&
 	echo wrong >askpass-response &&
-	test_must_fail git clone "$HTTPD_URL/auth/repo.git" clone-auth-fail &&
+	test_must_fail git clone "$HTTPD_URL/auth/dumb/repo.git" clone-auth-fail &&
 	expect_askpass both wrong
 '
 
 test_expect_success 'http auth can use user/pass in URL' '
 	>askpass-query &&
 	echo wrong >askpass-response &&
-	git clone "$HTTPD_URL_USER_PASS/auth/repo.git" clone-auth-none &&
+	git clone "$HTTPD_URL_USER_PASS/auth/dumb/repo.git" clone-auth-none &&
 	expect_askpass none
 '
 
 test_expect_success 'http auth can use just user in URL' '
 	>askpass-query &&
 	echo user@host >askpass-response &&
-	git clone "$HTTPD_URL_USER/auth/repo.git" clone-auth-pass &&
+	git clone "$HTTPD_URL_USER/auth/dumb/repo.git" clone-auth-pass &&
 	expect_askpass pass user@host
 '
 
 test_expect_success 'http auth can request both user and pass' '
 	>askpass-query &&
 	echo user@host >askpass-response &&
-	git clone "$HTTPD_URL/auth/repo.git" clone-auth-both &&
+	git clone "$HTTPD_URL/auth/dumb/repo.git" clone-auth-both &&
 	expect_askpass both user@host
 '
 
@@ -114,7 +114,7 @@ test_expect_success 'http auth respects credential helper config' '
 	}; f" &&
 	>askpass-query &&
 	echo wrong >askpass-response &&
-	git clone "$HTTPD_URL/auth/repo.git" clone-auth-helper &&
+	git clone "$HTTPD_URL/auth/dumb/repo.git" clone-auth-helper &&
 	expect_askpass none
 '
 
@@ -122,7 +122,7 @@ test_expect_success 'http auth can get username from config' '
 	test_config_global "credential.$HTTPD_URL.username" user@host &&
 	>askpass-query &&
 	echo user@host >askpass-response &&
-	git clone "$HTTPD_URL/auth/repo.git" clone-auth-user &&
+	git clone "$HTTPD_URL/auth/dumb/repo.git" clone-auth-user &&
 	expect_askpass pass user@host
 '
 
@@ -130,7 +130,7 @@ test_expect_success 'configured username does not override URL' '
 	test_config_global "credential.$HTTPD_URL.username" wrong &&
 	>askpass-query &&
 	echo user@host >askpass-response &&
-	git clone "$HTTPD_URL_USER/auth/repo.git" clone-auth-user2 &&
+	git clone "$HTTPD_URL_USER/auth/dumb/repo.git" clone-auth-user2 &&
 	expect_askpass pass user@host
 '
 
-- 
1.7.11.5.10.g3c8125b

[PATCH 2/8] t5550: factor out http auth setup

[Jeff King]

The t5550 script sets up a nice askpass helper for
simulating user input and checking what git prompted for.
Let's make it available to other http scripts by migrating
it to lib-httpd.

We can use this immediately in t5540 to make our tests more
robust (previously, we did not check at all that hitting the
password-protected repo actually involved a password).
Unfortunately, we end up failing the test because the
current code erroneously prompts twice (once for
git-remote-http, and then again when the former spawns
git-http-push).

More importantly, though, it will let us easily add
smart-http authentication tests in t5541 and t5551; we
currently do not test smart-http authentication at all.

As part of making it generic, let's always look for and
store auxiliary askpass files at the top-level trash
directory; this makes it compatible with t5540, which runs
some tests from sub-repositories. We can abstract away the
ugliness with a short helper function.

Signed-off-by: Jeff King <peff@peff.net>
---
If we do backport this to v1.7.8-era, note that write_script did not
exist then.

 t/lib-httpd.sh        | 39 +++++++++++++++++++++++++++++++++++++
 t/t5540-http-push.sh  | 17 ++++++++---------
 t/t5550-http-fetch.sh | 53 ++++++++-------------------------------------------
 3 files changed, 55 insertions(+), 54 deletions(-)

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index d773542..02f442b 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -167,3 +167,42 @@ test_http_push_nonff() {
 		test_i18ngrep "Updates were rejected because" output
 	'
 }
+
+setup_askpass_helper() {
+	test_expect_success 'setup askpass helper' '
+		write_script "$TRASH_DIRECTORY/askpass" <<-\EOF &&
+		echo >>"$TRASH_DIRECTORY/askpass-query" "askpass: $*" &&
+		cat "$TRASH_DIRECTORY/askpass-response"
+		EOF
+		GIT_ASKPASS="$TRASH_DIRECTORY/askpass" &&
+		export GIT_ASKPASS &&
+		export TRASH_DIRECTORY
+	'
+}
+
+set_askpass() {
+	>"$TRASH_DIRECTORY/askpass-query" &&
+	echo "$*" >"$TRASH_DIRECTORY/askpass-response"
+}
+
+expect_askpass() {
+	dest=$HTTPD_DEST
+	{
+		case "$1" in
+		none)
+			;;
+		pass)
+			echo "askpass: Password for 'http://$2@$dest': "
+			;;
+		both)
+			echo "askpass: Username for 'http://$dest': "
+			echo "askpass: Password for 'http://$2@$dest': "
+			;;
+		*)
+			false
+			;;
+		esac
+	} >"$TRASH_DIRECTORY/askpass-expect" &&
+	test_cmp "$TRASH_DIRECTORY/askpass-expect" \
+		 "$TRASH_DIRECTORY/askpass-query"
+}
diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index 1eea647..f141f2d 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -46,15 +46,7 @@ test_expect_success 'create password-protected repository' '
 	       "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/test_repo.git"
 '
 
-test_expect_success 'setup askpass helper' '
-	cat >askpass <<-\EOF &&
-	#!/bin/sh
-	echo user@host
-	EOF
-	chmod +x askpass &&
-	GIT_ASKPASS="$PWD/askpass" &&
-	export GIT_ASKPASS
-'
+setup_askpass_helper
 
 test_expect_success 'clone remote repository' '
 	cd "$ROOT_PATH" &&
@@ -162,6 +154,7 @@ test_http_push_nonff "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git \
 
 test_expect_success 'push to password-protected repository (user in URL)' '
 	test_commit pw-user &&
+	set_askpass user@host &&
 	git push "$HTTPD_URL_USER/auth/dumb/test_repo.git" HEAD &&
 	git rev-parse --verify HEAD >expect &&
 	git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/test_repo.git" \
@@ -169,9 +162,15 @@ test_expect_success 'push to password-protected repository (user in URL)' '
 	test_cmp expect actual
 '
 
+test_expect_failure 'user was prompted only once for password' '
+	expect_askpass pass user@host
+'
+
 test_expect_failure 'push to password-protected repository (no user in URL)' '
 	test_commit pw-nouser &&
+	set_askpass user@host &&
 	git push "$HTTPD_URL/auth/dumb/test_repo.git" HEAD &&
+	expect_askpass both user@host
 	git rev-parse --verify HEAD >expect &&
 	git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/test_repo.git" \
 		rev-parse --verify HEAD >actual &&
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index 5ad2123..16ef041 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -46,62 +46,28 @@ test_expect_success 'create password-protected repository' '
 	       "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/repo.git"
 '
 
-test_expect_success 'setup askpass helpers' '
-	cat >askpass <<-EOF &&
-	#!/bin/sh
-	echo >>"$PWD/askpass-query" "askpass: \$*" &&
-	cat "$PWD/askpass-response"
-	EOF
-	chmod +x askpass &&
-	GIT_ASKPASS="$PWD/askpass" &&
-	export GIT_ASKPASS
-'
-
-expect_askpass() {
-	dest=$HTTPD_DEST
-	{
-		case "$1" in
-		none)
-			;;
-		pass)
-			echo "askpass: Password for 'http://$2@$dest': "
-			;;
-		both)
-			echo "askpass: Username for 'http://$dest': "
-			echo "askpass: Password for 'http://$2@$dest': "
-			;;
-		*)
-			false
-			;;
-		esac
-	} >askpass-expect &&
-	test_cmp askpass-expect askpass-query
-}
+setup_askpass_helper
 
 test_expect_success 'cloning password-protected repository can fail' '
-	>askpass-query &&
-	echo wrong >askpass-response &&
+	set_askpass wrong &&
 	test_must_fail git clone "$HTTPD_URL/auth/dumb/repo.git" clone-auth-fail &&
 	expect_askpass both wrong
 '
 
 test_expect_success 'http auth can use user/pass in URL' '
-	>askpass-query &&
-	echo wrong >askpass-response &&
+	set_askpass wrong &&
 	git clone "$HTTPD_URL_USER_PASS/auth/dumb/repo.git" clone-auth-none &&
 	expect_askpass none
 '
 
 test_expect_success 'http auth can use just user in URL' '
-	>askpass-query &&
-	echo user@host >askpass-response &&
+	set_askpass user@host &&
 	git clone "$HTTPD_URL_USER/auth/dumb/repo.git" clone-auth-pass &&
 	expect_askpass pass user@host
 '
 
 test_expect_success 'http auth can request both user and pass' '
-	>askpass-query &&
-	echo user@host >askpass-response &&
+	set_askpass user@host &&
 	git clone "$HTTPD_URL/auth/dumb/repo.git" clone-auth-both &&
 	expect_askpass both user@host
 '
@@ -112,24 +78,21 @@ test_expect_success 'http auth respects credential helper config' '
 		echo username=user@host
 		echo password=user@host
 	}; f" &&
-	>askpass-query &&
-	echo wrong >askpass-response &&
+	set_askpass wrong &&
 	git clone "$HTTPD_URL/auth/dumb/repo.git" clone-auth-helper &&
 	expect_askpass none
 '
 
 test_expect_success 'http auth can get username from config' '
 	test_config_global "credential.$HTTPD_URL.username" user@host &&
-	>askpass-query &&
-	echo user@host >askpass-response &&
+	set_askpass user@host &&
 	git clone "$HTTPD_URL/auth/dumb/repo.git" clone-auth-user &&
 	expect_askpass pass user@host
 '
 
 test_expect_success 'configured username does not override URL' '
 	test_config_global "credential.$HTTPD_URL.username" wrong &&
-	>askpass-query &&
-	echo user@host >askpass-response &&
+	set_askpass user@host &&
 	git clone "$HTTPD_URL_USER/auth/dumb/repo.git" clone-auth-user2 &&
 	expect_askpass pass user@host
 '
-- 
1.7.11.5.10.g3c8125b

[PATCH 3/8] t/lib-httpd: only route auth/dumb to dumb repos

[Jeff King]

Our test apache config points all of auth/ directly to the
on-disk repositories via an Alias directive. This works fine
because everything authenticated is currently in auth/dumb,
which is a subset.  However, this would conflict with a
ScriptAlias for auth/smart (which will come in future
patches), so let's narrow the Alias.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/lib-httpd/apache.conf | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 36b1596..d13fe64 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -46,7 +46,7 @@ PassEnv GIT_VALGRIND
 PassEnv GIT_VALGRIND_OPTIONS
 
 Alias /dumb/ www/
-Alias /auth/ www/auth/
+Alias /auth/dumb/ www/auth/dumb/
 
 <Location /smart/>
 	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
-- 
1.7.11.5.10.g3c8125b

[PATCH 4/8] t/lib-httpd: recognize */smart/* repos as smart-http

[Jeff King]

We do not currently test authentication for smart-http repos
at all. Part of the infrastructure to do this is recognizing
that auth/smart is indeed a smart-http repo.

The current apache config recognizes only "^/smart/*" as
smart-http. Let's instead treat anything with /smart/ in the
URL as smart-http. This is obviously a stupid thing to do
for a real production site, but for our test suite we know
that our repositories will not have this magic string in the
name.

Note that we will route /foo/smart/bar.git directly to
git-http-backend/bar.git; in other words, everything before
the "/smart/" is irrelevant to finding the repo on disk (but
may impact apache config, for example by triggering auth
checks).

Signed-off-by: Jeff King <peff@peff.net>
---
Another backporting gotcha: the smart_custom_env bits did not exist back
in the v1.7.8 era.

 t/lib-httpd/apache.conf | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index d13fe64..c6a1a87 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -48,22 +48,20 @@ PassEnv GIT_VALGRIND_OPTIONS
 Alias /dumb/ www/
 Alias /auth/dumb/ www/auth/dumb/
 
-<Location /smart/>
+<LocationMatch /smart/>
 	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
 	SetEnv GIT_HTTP_EXPORT_ALL
-</Location>
-<Location /smart_noexport/>
+</LocationMatch>
+<LocationMatch /smart_noexport/>
 	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
-</Location>
-<Location /smart_custom_env/>
+</LocationMatch>
+<LocationMatch /smart_custom_env/>
 	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
 	SetEnv GIT_HTTP_EXPORT_ALL
 	SetEnv GIT_COMMITTER_NAME "Custom User"
 	SetEnv GIT_COMMITTER_EMAIL custom@example.com
-</Location>
-ScriptAlias /smart/ ${GIT_EXEC_PATH}/git-http-backend/
-ScriptAlias /smart_noexport/ ${GIT_EXEC_PATH}/git-http-backend/
-ScriptAlias /smart_custom_env/ ${GIT_EXEC_PATH}/git-http-backend/
+</LocationMatch>
+ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
 <Directory ${GIT_EXEC_PATH}>
 	Options FollowSymlinks
 </Directory>
-- 
1.7.11.5.10.g3c8125b

[PATCH 5/8] t: test basic smart-http authentication

[Jeff King]

We do not currently test authentication over smart-http at
all. In theory, it should work exactly as it does for dumb
http (which we do test). It does indeed work for these
simple tests, but this patch lays the groundwork for more
complex tests in future patches.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5541-http-push.sh  | 14 ++++++++++++++
 t/t5551-http-fetch.sh | 11 +++++++++++
 2 files changed, 25 insertions(+)

diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index 312e484..eeb9932 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -36,6 +36,8 @@ test_expect_success 'setup remote repository' '
 	mv test_repo.git "$HTTPD_DOCUMENT_ROOT_PATH"
 '
 
+setup_askpass_helper
+
 cat >exp <<EOF
 GET  /smart/test_repo.git/info/refs?service=git-upload-pack HTTP/1.1 200
 POST /smart/test_repo.git/git-upload-pack HTTP/1.1 200
@@ -266,5 +268,17 @@ test_expect_success 'http push respects GIT_COMMITTER_* in reflog' '
 	test_cmp expect actual
 '
 
+test_expect_success 'push over smart http with auth' '
+	cd "$ROOT_PATH/test_repo_clone" &&
+	echo push-auth-test >expect &&
+	test_commit push-auth-test &&
+	set_askpass user@host &&
+	git push "$HTTPD_URL"/auth/smart/test_repo.git &&
+	git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git" \
+		log -1 --format=%s >actual &&
+	expect_askpass both user@host &&
+	test_cmp expect actual
+'
+
 stop_httpd
 test_done
diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index 91eaf53..e653ae3 100755
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh
@@ -27,6 +27,8 @@ test_expect_success 'create http-accessible bare repository' '
 	git push public master:master
 '
 
+setup_askpass_helper
+
 cat >exp <<EOF
 > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
 > Accept: */*
@@ -109,6 +111,15 @@ test_expect_success 'follow redirects (302)' '
 	git clone $HTTPD_URL/smart-redir-temp/repo.git --quiet repo-t
 '
 
+test_expect_success 'clone from password-protected repository' '
+	echo two >expect &&
+	set_askpass user@host &&
+	git clone --bare "$HTTPD_URL/auth/smart/repo.git" smart-auth &&
+	expect_askpass both user@host &&
+	git --git-dir=smart-auth log -1 --format=%s >actual &&
+	test_cmp expect actual
+'
+
 test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
 
 test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '
-- 
1.7.11.5.10.g3c8125b

[PATCH 6/8] t: test http access to "half-auth" repositories

[Jeff King]

Some sites set up http access to repositories such that
fetching is anonymous and unauthenticated, but pushing is
authenticated. While there are multiple ways to do this, the
technique advertised in the git-http-backend manpage is to
block access to locations matching "/git-receive-pack$".

Let's emulate that advice in our test setup, which makes it
clear that this advice does not actually work.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/lib-httpd/apache.conf |  7 +++++++
 t/t5541-http-push.sh    | 12 ++++++++++++
 t/t5551-http-fetch.sh   |  9 +++++++++
 3 files changed, 28 insertions(+)

diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index c6a1a87..49d5d87 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -92,6 +92,13 @@ SSLEngine On
 	Require valid-user
 </Location>
 
+<LocationMatch "^/auth-push/.*/git-receive-pack$">
+	AuthType Basic
+	AuthName "git-auth"
+	AuthUserFile passwd
+	Require valid-user
+</LocationMatch>
+
 <IfDefine DAV>
 	LoadModule dav_module modules/mod_dav.so
 	LoadModule dav_fs_module modules/mod_dav_fs.so
diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index eeb9932..9b1cd60 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -280,5 +280,17 @@ test_expect_success 'push over smart http with auth' '
 	test_cmp expect actual
 '
 
+test_expect_failure 'push to auth-only-for-push repo' '
+	cd "$ROOT_PATH/test_repo_clone" &&
+	echo push-half-auth >expect &&
+	test_commit push-half-auth &&
+	set_askpass user@host &&
+	git push "$HTTPD_URL"/auth-push/smart/test_repo.git &&
+	git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git" \
+		log -1 --format=%s >actual &&
+	expect_askpass both user@host &&
+	test_cmp expect actual
+'
+
 stop_httpd
 test_done
diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index e653ae3..2db5c35 100755
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh
@@ -120,6 +120,15 @@ test_expect_success 'clone from password-protected repository' '
 	test_cmp expect actual
 '
 
+test_expect_success 'clone from auth-only-for-push repository' '
+	echo two >expect &&
+	set_askpass wrong &&
+	git clone --bare "$HTTPD_URL/auth-push/smart/repo.git" smart-noauth &&
+	expect_askpass none &&
+	git --git-dir=smart-noauth log -1 --format=%s >actual &&
+	test_cmp expect actual
+'
+
 test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
 
 test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '
-- 
1.7.11.5.10.g3c8125b

[PATCH 7/8] http: factor out http error code handling

[Jeff King]

Most of our http requests go through the http_request()
interface, which does some nice post-processing on the
results. In particular, it handles prompting for missing
credentials as well as approving and rejecting valid or
invalid credentials. Unfortunately, it only handles GET
requests. Making it handle POSTs would be quite complex, so
let's pull result handling code into its own function so
that it can be reused from the POST code paths.

Signed-off-by: Jeff King <peff@peff.net>
---
 http.c | 51 ++++++++++++++++++++++++++++-----------------------
 http.h |  1 +
 2 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/http.c b/http.c
index b61ac85..6793137 100644
--- a/http.c
+++ b/http.c
@@ -745,6 +745,33 @@ char *get_remote_object_url(const char *url, const char *hex,
 	return strbuf_detach(&buf, NULL);
 }
 
+int handle_curl_result(struct active_request_slot *slot)
+{
+	struct slot_results *results = slot->results;
+
+	if (results->curl_result == CURLE_OK) {
+		credential_approve(&http_auth);
+		return HTTP_OK;
+	} else if (missing_target(results))
+		return HTTP_MISSING_TARGET;
+	else if (results->http_code == 401) {
+		if (http_auth.username && http_auth.password) {
+			credential_reject(&http_auth);
+			return HTTP_NOAUTH;
+		} else {
+			credential_fill(&http_auth);
+			init_curl_http_auth(slot->curl);
+			return HTTP_REAUTH;
+		}
+	} else {
+		if (!curl_errorstr[0])
+			strlcpy(curl_errorstr,
+				curl_easy_strerror(results->curl_result),
+				sizeof(curl_errorstr));
+		return HTTP_ERROR;
+	}
+}
+
 /* http_request() targets */
 #define HTTP_REQUEST_STRBUF	0
 #define HTTP_REQUEST_FILE	1
@@ -792,26 +819,7 @@ static int http_request(const char *url, void *result, int target, int options)
 
 	if (start_active_slot(slot)) {
 		run_active_slot(slot);
-		if (results.curl_result == CURLE_OK)
-			ret = HTTP_OK;
-		else if (missing_target(&results))
-			ret = HTTP_MISSING_TARGET;
-		else if (results.http_code == 401) {
-			if (http_auth.username && http_auth.password) {
-				credential_reject(&http_auth);
-				ret = HTTP_NOAUTH;
-			} else {
-				credential_fill(&http_auth);
-				init_curl_http_auth(slot->curl);
-				ret = HTTP_REAUTH;
-			}
-		} else {
-			if (!curl_errorstr[0])
-				strlcpy(curl_errorstr,
-					curl_easy_strerror(results.curl_result),
-					sizeof(curl_errorstr));
-			ret = HTTP_ERROR;
-		}
+		ret = handle_curl_result(slot);
 	} else {
 		error("Unable to start HTTP request for %s", url);
 		ret = HTTP_START_FAILED;
@@ -820,9 +828,6 @@ static int http_request(const char *url, void *result, int target, int options)
 	curl_slist_free_all(headers);
 	strbuf_release(&buf);
 
-	if (ret == HTTP_OK)
-		credential_approve(&http_auth);
-
 	return ret;
 }
 
diff --git a/http.h b/http.h
index 915c286..12de255 100644
--- a/http.h
+++ b/http.h
@@ -78,6 +78,7 @@ extern int start_active_slot(struct active_request_slot *slot);
 extern void run_active_slot(struct active_request_slot *slot);
 extern void finish_active_slot(struct active_request_slot *slot);
 extern void finish_all_active_slots(void);
+extern int handle_curl_result(struct active_request_slot *slot);
 
 #ifdef USE_CURL_MULTI
 extern void fill_active_slots(void);
-- 
1.7.11.5.10.g3c8125b

Re: [PATCH 7/8] http: factor out http error code handling

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Most of our http requests go through the http_request()
> interface, which does some nice post-processing on the
> results. In particular, it handles prompting for missing
> credentials as well as approving and rejecting valid or
> invalid credentials. Unfortunately, it only handles GET
> requests. Making it handle POSTs would be quite complex, so
> let's pull result handling code into its own function so
> that it can be reused from the POST code paths.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  http.c | 51 ++++++++++++++++++++++++++++-----------------------
>  http.h |  1 +
>  2 files changed, 29 insertions(+), 23 deletions(-)
>
> diff --git a/http.c b/http.c
> index b61ac85..6793137 100644
> --- a/http.c
> +++ b/http.c
> @@ -745,6 +745,33 @@ char *get_remote_object_url(const char *url, const char *hex,
>  	return strbuf_detach(&buf, NULL);
>  }
>  
> +int handle_curl_result(struct active_request_slot *slot)
> +{
> +	struct slot_results *results = slot->results;
> +
> +	if (results->curl_result == CURLE_OK) {
> +		credential_approve(&http_auth);
> +		return HTTP_OK;
> +	} else if (missing_target(results))
> +...
> +		return HTTP_ERROR;
> +	}
> +}
> +
> @@ -820,9 +828,6 @@ static int http_request(const char *url, void *result, int target, int options)
>  	curl_slist_free_all(headers);
>  	strbuf_release(&buf);
>  
> -	if (ret == HTTP_OK)
> -		credential_approve(&http_auth);

OK, now this is part of handle_curl_result() so the caller does not
have to worry about it, which is nice ;-)

>  	return ret;
>  }

[PATCH 8/8] http: prompt for credentials on failed POST

[Jeff King]

All of the smart-http GET requests go through the http_get_*
functions, which will prompt for credentials and retry if we
see an HTTP 401.

POST requests, however, do not go through any central point.
Moreover, it is difficult to retry in the general case; we
cannot assume the request body fits in memory or is even
seekable, and we don't know how much of it was consumed
during the attempt.

Most of the time, this is not a big deal; for both fetching
and pushing, we make a GET request before doing any POSTs,
so typically we figure out the credentials during the first
request, then reuse them during the POST. However, some
servers may allow a client to get the list of refs from
receive-pack without authentication, and then require
authentication when the client actually tries to POST the
pack.

This is not ideal, as the client may do a non-trivial amount
of work to generate the pack (e.g., delta-compressing
objects). However, for a long time it has been the
recommended example configuration in git-http-backend(1) for
setting up a repository with anonymous fetch and
authenticated push. This setup has always been broken
without putting a username into the URL. Prior to commit
986bbc0, it did work with a username in the URL, because git
would prompt for credentials before making any requests at
all. However, post-986bbc0, it is totally broken. Since it
has been advertised in the manpage for some time, we should
make sure it works.

Unfortunately, it is not as easy as simply calling post_rpc
again when it fails, due to the input issue mentioned above.
However, we can still make this specific case work by
retrying in two specific instances:

  1. If the request is large (bigger than LARGE_PACKET_MAX),
     we will first send a probe request with a single flush
     packet. Since this request is static, we can freely
     retry it.

  2. If the request is small and we are not using gzip, then
     we have the whole thing in-core, and we can freely
     retry.

That means we will not retry in some instances, including:

  1. If we are using gzip. However, we only do so when
     calling git-upload-pack, so it does not apply to
     pushes.

  2. If we have a large request, the probe succeeds, but
     then the real POST wants authentication. This is an
     extremely unlikely configuration and not worth worrying
     about.

While it might be nice to cover those instances, doing so
would be significantly more complex for very little
real-world gain. In the long run, we will be much better off
when curl learns to internally handle authentication as a
callback, and we can cleanly handle all cases that way.

Signed-off-by: Jeff King <peff@peff.net>
---
Sorry for the wordy explanation. I really tried to refactor this into a
nice single code path for making both GET and POST requests, but I think
there are just too many corner cases. Suggestions welcome if somebody
has a better idea of how to refactor it (preferably in the form of a
patch).

 remote-curl.c        | 23 +++++++++++++++--------
 t/t5541-http-push.sh |  2 +-
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 04a9d62..3ec474f 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -362,16 +362,17 @@ static size_t rpc_in(char *ptr, size_t eltsize,
 
 static int run_slot(struct active_request_slot *slot)
 {
-	int err = 0;
+	int err;
 	struct slot_results results;
 
 	slot->results = &results;
 	slot->curl_result = curl_easy_perform(slot->curl);
 	finish_active_slot(slot);
 
-	if (results.curl_result != CURLE_OK) {
-		err |= error("RPC failed; result=%d, HTTP code = %ld",
-			results.curl_result, results.http_code);
+	err = handle_curl_result(slot);
+	if (err != HTTP_OK && err != HTTP_REAUTH) {
+		error("RPC failed; result=%d, HTTP code = %ld",
+		      results.curl_result, results.http_code);
 	}
 
 	return err;
@@ -436,9 +437,11 @@ static int post_rpc(struct rpc_state *rpc)
 	}
 
 	if (large_request) {
-		err = probe_rpc(rpc);
-		if (err)
-			return err;
+		do {
+			err = probe_rpc(rpc);
+		} while (err == HTTP_REAUTH);
+		if (err != HTTP_OK)
+			return -1;
 	}
 
 	slot = get_active_slot();
@@ -525,7 +528,11 @@ static int post_rpc(struct rpc_state *rpc)
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
 
-	err = run_slot(slot);
+	do {
+		err = run_slot(slot);
+	} while (err == HTTP_REAUTH && !large_request && !use_gzip);
+	if (err != HTTP_OK)
+		err = -1;
 
 	curl_slist_free_all(headers);
 	free(gzip_body);
diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index 9b1cd60..ef6d6b6 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -280,7 +280,7 @@ test_expect_success 'push over smart http with auth' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'push to auth-only-for-push repo' '
+test_expect_success 'push to auth-only-for-push repo' '
 	cd "$ROOT_PATH/test_repo_clone" &&
 	echo push-half-auth >expect &&
 	test_commit push-half-auth &&
-- 
1.7.11.5.10.g3c8125b

Re: [PATCH 8/8] http: prompt for credentials on failed POST

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Most of the time, this is not a big deal; for both fetching
> and pushing, we make a GET request before doing any POSTs,
> so typically we figure out the credentials during the first
> request, then reuse them during the POST. However, some
> servers may allow a client to get the list of refs from
> receive-pack without authentication, and then require
> authentication when the client actually tries to POST the
> pack.

A silly question.  Does the initial GET request when we push look
any different from the initial GET request when we fetch?  Can we
make them look different in an updated client, so that the server
side can say "this GET is about pushing into us, and we require
authentication"?

> Unfortunately, it is not as easy as simply calling post_rpc
> again when it fails, due to the input issue mentioned above.
> However, we can still make this specific case work by
> retrying in two specific instances:
>
>   1. If the request is large (bigger than LARGE_PACKET_MAX),
>      we will first send a probe request with a single flush
>      packet. Since this request is static, we can freely
>      retry it.
>
>   2. If the request is small and we are not using gzip, then
>      we have the whole thing in-core, and we can freely
>      retry.
>
> That means we will not retry in some instances, including:
>
>   1. If we are using gzip. However, we only do so when
>      calling git-upload-pack, so it does not apply to
>      pushes.
>
>   2. If we have a large request, the probe succeeds, but
>      then the real POST wants authentication. This is an
>      extremely unlikely configuration and not worth worrying
>      about.
>
> While it might be nice to cover those instances, doing so
> would be significantly more complex for very little
> real-world gain. In the long run, we will be much better off
> when curl learns to internally handle authentication as a
> callback, and we can cleanly handle all cases that way.

I suspect that in real life, almost nobody runs smart HTTP server
that allows anonymous push.

How much usability penalty would it be if we always fill credential
before pushing?  Alternatively, how much latency penalty would it
incur if we always send a probe request regardless of the request
size when we try to push without having an authentication material?

> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Sorry for the wordy explanation. I really tried to refactor this into a
> nice single code path for making both GET and POST requests, but I think
> there are just too many corner cases. Suggestions welcome if somebody
> has a better idea of how to refactor it (preferably in the form of a
> patch).
>
>  remote-curl.c        | 23 +++++++++++++++--------
>  t/t5541-http-push.sh |  2 +-
>  2 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/remote-curl.c b/remote-curl.c
> index 04a9d62..3ec474f 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -362,16 +362,17 @@ static size_t rpc_in(char *ptr, size_t eltsize,
>  
>  static int run_slot(struct active_request_slot *slot)
>  {
> -	int err = 0;
> +	int err;
>  	struct slot_results results;
>  
>  	slot->results = &results;
>  	slot->curl_result = curl_easy_perform(slot->curl);
>  	finish_active_slot(slot);
>  
> -	if (results.curl_result != CURLE_OK) {
> -		err |= error("RPC failed; result=%d, HTTP code = %ld",
> -			results.curl_result, results.http_code);
> +	err = handle_curl_result(slot);
> +	if (err != HTTP_OK && err != HTTP_REAUTH) {
> +		error("RPC failed; result=%d, HTTP code = %ld",
> +		      results.curl_result, results.http_code);
>  	}
>  
>  	return err;
> @@ -436,9 +437,11 @@ static int post_rpc(struct rpc_state *rpc)
>  	}
>  
>  	if (large_request) {
> -		err = probe_rpc(rpc);
> -		if (err)
> -			return err;
> +		do {
> +			err = probe_rpc(rpc);
> +		} while (err == HTTP_REAUTH);
> +		if (err != HTTP_OK)
> +			return -1;
>  	}
>  
>  	slot = get_active_slot();
> @@ -525,7 +528,11 @@ static int post_rpc(struct rpc_state *rpc)
>  	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
>  	curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
>  
> -	err = run_slot(slot);
> +	do {
> +		err = run_slot(slot);
> +	} while (err == HTTP_REAUTH && !large_request && !use_gzip);
> +	if (err != HTTP_OK)
> +		err = -1;
>  
>  	curl_slist_free_all(headers);
>  	free(gzip_body);
> diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
> index 9b1cd60..ef6d6b6 100755
> --- a/t/t5541-http-push.sh
> +++ b/t/t5541-http-push.sh
> @@ -280,7 +280,7 @@ test_expect_success 'push over smart http with auth' '
>  	test_cmp expect actual
>  '
>  
> -test_expect_failure 'push to auth-only-for-push repo' '
> +test_expect_success 'push to auth-only-for-push repo' '
>  	cd "$ROOT_PATH/test_repo_clone" &&
>  	echo push-half-auth >expect &&
>  	test_commit push-half-auth &&

Re: [PATCH 8/8] http: prompt for credentials on failed POST

[Jeff King]

On Mon, Aug 27, 2012 at 10:48:28AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Most of the time, this is not a big deal; for both fetching
> > and pushing, we make a GET request before doing any POSTs,
> > so typically we figure out the credentials during the first
> > request, then reuse them during the POST. However, some
> > servers may allow a client to get the list of refs from
> > receive-pack without authentication, and then require
> > authentication when the client actually tries to POST the
> > pack.
> 
> A silly question.  Does the initial GET request when we push look
> any different from the initial GET request when we fetch?  Can we
> make them look different in an updated client, so that the server
> side can say "this GET is about pushing into us, and we require
> authentication"?

Yes, they are already different. A fetch asks for

  info/refs?service=git-upload-pack

and a push asks for

  info/refs?service-git-receive-pack

And I definitely think the optimal server config will authenticate the
client at that first GET step, because the client may do a significant
amount of work for the POST (due to the probe_rpc, it won't actually
_send_ a large pack, but it will do the complete delta-compression phase
before generating any output, which can be slow).

But doing it this way has been advertised in our manpage for so long, I
assume some people are using it. And given that it used to work for
older clients (prior to v1.7.8), and that the person who upgraded their
client is not always in charge of telling the person running the server
to fix their server, I think it's worth un-breaking it.

And we should definitely tweak what git-http-backend advertises on top
so that eventually this sub-optimal config dies out.

> >   1. If we are using gzip. However, we only do so when
> >      calling git-upload-pack, so it does not apply to
> >      pushes.
> >
> >   2. If we have a large request, the probe succeeds, but
> >      then the real POST wants authentication. This is an
> >      extremely unlikely configuration and not worth worrying
> >      about.
> >
> > While it might be nice to cover those instances, doing so
> > would be significantly more complex for very little
> > real-world gain. In the long run, we will be much better off
> > when curl learns to internally handle authentication as a
> > callback, and we can cleanly handle all cases that way.
> 
> I suspect that in real life, almost nobody runs smart HTTP server
> that allows anonymous push.
> 
> How much usability penalty would it be if we always fill credential
> before pushing?

It would reintroduce the problem that 986bbc0 was fixing: we would
prompt even when curl would end up pulling the credential from .netrc.
I find that somewhat less compelling a problem now that we have
credential helpers, though. And of course it does not fix (1) or (2)
above, either.

> Alternatively, how much latency penalty would it incur if we always
> send a probe request regardless of the request size when we try to
> push without having an authentication material?

It would be one http round-trip and no-op invocation of request-pack on
the server. If we did it only on push, that would probably not be too
bad, as we would hit it only when we were actually pushing something.

But that would still suffer from (1) and (2) above, so I don't see it as
a real advantage. You _could_ fix both cases by buffering the input data
and restarting the request. I just didn't think it was worth doing,
since they are unlikely configurations and the code complexity is much
higher.

-Peff

Re: [PATCH 8/8] http: prompt for credentials on failed POST

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> A silly question.  Does the initial GET request when we push look
>> any different from the initial GET request when we fetch?  Can we
>> make them look different in an updated client, so that the server
>> side can say "this GET is about pushing into us, and we require
>> authentication"?
>
> Yes, they are already different. A fetch asks for
> ...
> But doing it this way has been advertised in our manpage for so long, I
> assume some people are using it. And given that it used to work for
> older clients (prior to v1.7.8), and that the person who upgraded their
> client is not always in charge of telling the person running the server
> to fix their server, I think it's worth un-breaking it.

Oh, I wasn't saying the fix is unnecessary.  I was trying to see if
there is something people who _care_ about wasted effort on the
client side can do to fix their configuration properly (otherwise
while we are patching the client, make sure we give them a way).

> But that would still suffer from (1) and (2) above, so I don't see it as
> a real advantage. You _could_ fix both cases by buffering the input data
> and restarting the request. I just didn't think it was worth doing,
> since they are unlikely configurations and the code complexity is much
> higher.

OK.

Re: [PATCH 0/8] fix password prompting for "half-auth" servers

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

(+cc: Shawn)

> On Sun, Aug 26, 2012 at 06:13:41AM -0400, Jeff King wrote:
>
>> No problem. I'll probably be a day or two on the patches, as the http
>> tests are in need of some refactoring before adding more tests. But in
>> the meantime, I think your config change is a sane work-around.
>
> OK, here is the series.  For those just joining us, the problem is that
> git will not correctly prompt for credentials when pushing to a
> repository which allows the initial GET of
> ".../info/refs?service=git-receive-pack", but then gives a 401 when we
> try to POST the pack. This has never worked for a plain URL, but used to
> work if you put the username in the URL (because we would
> unconditionally load the credentials before making any requests). That
> was broken by 986bbc0, which does not do that proactive prompting for
> smart-http, meaning such repositories cannot be pushed to at all.
>
> Such a server-side setup is questionable in my opinion (because the
> client will actually create the pack before failing), but we have been
> advertising it for a long time in git-http-backend(1) as the right way
> to make repositories that are anonymous for fetching but require auth
> for pushing.
>
> The fix is somewhat uglier than I would like, but I think it's practical
> and the right thing to do (see the final patch for lots of discussion).
> I built this on the current tip of "master".  It might make sense to
> backport it directly on top of 986bbc0 for the maint track. There are
> conflicts, but they are all textual. Another option would be to revert
> 986bbc0 for the maint track, as that commit is itself fixing a minor bug
> that is of decreasing relevance (it fixed extra password prompting when
> .netrc was in use, but one can work around it by dropping the username
> from the URL).
>
> The patches are:
>
>   [1/8]: t5550: put auth-required repo in auth/dumb
>   [2/8]: t5550: factor out http auth setup
>   [3/8]: t/lib-httpd: only route auth/dumb to dumb repos
>   [4/8]: t/lib-httpd: recognize */smart/* repos as smart-http
>   [5/8]: t: test basic smart-http authentication
>
> These are all refactoring of the test scripts in preparation for 6/8
> (and are where all of the conflicts lie).
>
>   [6/8]: t: test http access to "half-auth" repositories
>
> This demonstrates the bug.
>
>   [7/8]: http: factor out http error code handling
>
> Refactoring to support 8/8.
>
>   [8/8]: http: prompt for credentials on failed POST
>
> And this one is the actual fix.
>
> I'd like to have a 9/8 which tweaks the git-http-backend documentation
> to provide better example apache config, but I haven't yet figured out
> the right incantation. Suggestions from apache gurus are welcome.
>
> -Peff

Re: git no longer prompting for password

[Iain Paton]

On 26/08/12 10:57, Iain Paton wrote:

>         <If "%{THE_REQUEST} =~ /git-receive-pack/">

I've just discovered that the <If ..> directive only appears in apache 2.4 
so something more generic will probably be a better idea. Not everyone will 
be running 2.4.x for a while yet.

Iain

Re: git no longer prompting for password

[BJ Hargrave]

On Aug 27, 2012, at 04:28 , Iain Paton wrote:

> On 26/08/12 10:57, Iain Paton wrote:
> 
>>        <If "%{THE_REQUEST} =~ /git-receive-pack/">
> 
> I've just discovered that the <If ..> directive only appears in apache 2.4 
> so something more generic will probably be a better idea. Not everyone will 
> be running 2.4.x for a while yet.


You could try something like this:

<Location /git>
  # Require authentication for git push
  RewriteCond %{QUERY_STRING} service=git-receive-pack
  RewriteRule .* - [E=AUTHREQUIRED:yes]
  Order Allow,Deny
  Deny from env=AUTHREQUIRED
  Allow from all
  Satisfy Any
  # Whatever auth rules you want ...

I haven't tested this specific example but it is based upon similar rules I use on a 2.0 server to require auth when specific query parameters are present. In my case, I have the Rewrite rules in the <VirtualHost> and the other directives in the <Directory> being protected.
-- 

BJ